On Tuesday 23 of July 2013 09:18:46 Greg KH wrote:
> On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
> > Hi,
> >
> > On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> > > On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > >> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> > >>> Hi Alan,
> > >
> > > Thanks for helping to clarify the issues here.
> > >
> > >>>> Okay. Are PHYs _always_ platform devices?
> > >>>
> > >>> They can be i2c, spi or any other device types as well.
> > >
> > > In those other cases, presumably there is no platform data associated
> > > with the PHY since it isn't a platform device. Then how does the
> > > kernel know which controller is attached to the PHY? Is this spelled
> > > out in platform data associated with the PHY's i2c/spi/whatever
> > > parent?
> >
> > Yes. I think we could use i2c_board_info for passing platform data.
> >
> > >>>>>> PHY. Currently this information is represented by name or
> > >>
> > >> ID
> > >>
> > >>>>>> strings embedded in platform data.
> > >>>>>
> > >>>>> right. It's embedded in the platform data of the controller.
> > >>>>
> > >>>> It must also be embedded in the PHY's platform data somehow.
> > >>>> Otherwise, how would the kernel know which PHY to use?
> > >>>
> > >>> By using a PHY lookup as Stephen and I suggested in our previous
> > >>> replies. Without any extra data in platform data. (I have even
> > >>> posted a
> > >>> code example.)
> > >
> > > I don't understand, because I don't know what "a PHY lookup" does.
> >
> > It is how the PHY framework finds a PHY, when the controller (say
> > USB)requests a PHY from the PHY framework.
> >
> > >>>> In this case, it doesn't matter where the platform_device
> > >>>> structures
> > >>>> are created or where the driver source code is. Let's take a
> > >>>> simple
> > >>>> example. Suppose the system design includes a PHY named "foo".
> > >>>> Then
> > >>>> the board file could contain:
> > >>>>
> > >>>> struct phy_info { ... } phy_foo;
> > >>>> EXPORT_SYMBOL_GPL(phy_foo);
> > >>>>
> > >>>> and a header file would contain:
> > >>>>
> > >>>> extern struct phy_info phy_foo;
> > >>>>
> > >>>> The PHY supplier could then call phy_create(&phy_foo), and the PHY
> > >>>> client could call phy_find(&phy_foo). Or something like that;
> > >>>> make up
> > >>>> your own structure tags and function names.
> > >>>>
> > >>>> It's still possible to have conflicts, but now two PHYs with the
> > >>>> same
> > >>>> name (or a misspelled name somewhere) will cause an error at link
> > >>>> time.
> > >>>
> > >>> This is incorrect, sorry. First of all it's a layering violation -
> > >>> you
> > >>> export random driver-specific symbols from one driver to another.
> > >>> Then
> > >
> > > No, that's not what I said. Neither the PHY driver nor the
> > > controller
> > > driver exports anything to the other. Instead, both drivers use data
> > > exported by the board file.
> >
> > I think instead we can use the same data while creating the platform
> > data of the controller and the PHY.
> > The PHY driver while creating the PHY (using PHY framework) will also
> > pass the *data* it actually got from the platform data to the
> > framework. The PHY user driver (USB), while requesting for the PHY
> > (from the PHY framework) will pass the *data* it got from its platform
> > data.
> > The PHY framework can do a comparison of the *data* pointers it has and
> > return the appropriate PHY to the controller.
> >
> > >>> imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2
> > >>> and
> > >>> there are two types of consumer drivers (e.g. USB host
> > >>> controllers). Now
> > >>> consider following mapping:
> > >>>
> > >>> SoC PHY consumer
> > >>> A PHY1 HOST1
> > >>> B PHY1 HOST2
> > >>> C PHY2 HOST1
> > >>> D PHY2 HOST2
> > >>>
> > >>> So we have to be able to use any of the PHYs with any of the host
> > >>> drivers. This means you would have to export symbol with the same
> > >>> name
> > >>> from both PHY drivers, which obviously would not work in this case,
> > >>> because having both drivers enabled (in a multiplatform aware
> > >>> configuration) would lead to linking conflict.
> > >
> > > You're right; the scheme was too simple. Instead, the board file
> > > must
> > > export two types of data structures, one for PHYs and one for
> > > controllers. Like this:
> > >
> > > struct phy_info {
> > >
> > > /* Info for the controller attached to this PHY */
> > > struct controller_info *hinfo;
> > >
> > > };
> > >
> > > struct controller_info {
> > >
> > > /* Info for the PHY which this controller is attached to */
> > > struct phy_info *pinfo;
> > >
> > > };
> > >
> > > The board file for SoC A would contain:
> > >
> > > struct phy_info phy1 = {&host1);
> > > EXPORT_SYMBOL(phy1);
> > > struct controller_info host1 = {&phy1};
> > > EXPORT_SYMBOL(host1);
> > >
> > > The board file for SoC B would contain:
> > >
> > > struct phy_info phy1 = {&host2);
> > > EXPORT_SYMBOL(phy1);
> > > struct controller_info host2 = {&phy1};
> > > EXPORT_SYMBOL(host2);
> >
> > I meant something like this
> > struct phy_info {
> >
> > const char *name;
> >
> > };
> >
> > struct phy_platform_data {
> >
> > .
> > .
> > struct phy_info *info;
> >
> > };
> >
> > struct usb_controller_platform_data {
> >
> > .
> > .
> > struct phy_info *info;
> >
> > };
> >
> > struct phy_info phy_info;
> >
> > While creating the phy device
> >
> > struct phy_platform_data phy_data;
> > phy_data.info = &info;
> > platform_device_add_data(pdev, &phy_data, sizeof(*phy_data))
> > platform_device_add();
> >
> > While creating the controller device
> >
> > struct usb_controller_platform_data controller_data;
> > controller_data.info = &info;
> > platform_device_add_data(pdev, &controller_data,
> > sizeof(*controller_data)) platform_device_add();
> >
> > Then modify PHY framework API phy create
> >
> > phy_create((struct device *dev, const struct phy_ops *ops,
> >
> > void *priv) {//API changed to take void pointer instead of
> > label
> >
> > . //existing implementation
> > .
> > phy->priv = priv;
> >
> > }
> >
> > struct phy *phy_get(struct device *dev, const char *string, void
> > *priv) {>
> > //API changed to take an additional pointer
> >
> > phy_lookup(priv)
> >
> > }
> >
> > static struct phy *phy_lookup(void *priv) {
> >
> > .
> > .
> > if (phy->priv==priv) //instead of string comparison, we'll use
> > pointer
> >
> > return phy;
> >
> > }
> >
> > PHY driver should be like
> >
> > phy_create((dev, ops, pdata->info);
> >
> > The controller driver would do
> >
> > phy_get(dev, NULL, pdata->info);
> >
> > Now the PHY framework will check for a match of *priv* pointer and
> > return the PHY.
> >
> > I think this should be possible?
>
> Ick, no. Why can't you just pass the pointer to the phy itself? If you
> had a "priv" pointer to search from, then you could have just passed the
> original phy pointer in the first place, right?
IMHO it would be better if you provided some code example, but let's try to
check if I understood you correctly.
8><------------------------------------------------------------------------
[Board file]
static struct phy my_phy;
static struct platform_device phy_pdev = {
/* ... */
.platform_data = &my_phy;
/* ... */
};
static struct platform_device phy_pdev = {
/* ... */
.platform_data = &my_phy;
/* ... */
};
[Provider driver]
struct phy *phy = pdev->dev.platform_data;
ret = phy_create(phy);
[Consumer driver]
struct phy *phy = pdev->dev.platform_data;
ret = phy_get(&pdev->dev, phy);
------------------------------------------------------------------------><8
Is this what you mean?
> The issue is that a string "name" is not going to scale at all, as it
> requires hard-coded information that will change over time (as the
> existing clock interface is already showing.)
I fully agree that a simple, single string will not scale even in some, not
so uncommon cases, but there is already a lot of existing lookup solutions
over the kernel and so there is no point in introducing another one.
> Please just pass the real "phy" pointer around, that's what it is there
> for. Your "board binding" logic/code should be able to handle this, as
> it somehow was going to do the same thing with a "name".
It's technically correct, but quality of this solution isn't really nice,
because it's a layering violation (at least if I understood what you mean).
This is because you need to have full definition of struct phy in board file
and a structure that is used as private data in PHY core comes from
platform code.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
