| Message ID | 20130729150638.GD3063@radagast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Monday 29 July 2013 08:36 PM, Felipe Balbi wrote: > Hi, > > On Fri, Jul 26, 2013 at 02:33:38PM +0530, Kishon Vijay Abraham I wrote: >> Previously MUSB wrapper (OMAP) device used PLATFORM_DEVID_AUTO while creating >> MUSB core device. So in usb_bind_phy (binds the controller with the PHY), the >> device name of the controller had *.auto* in it. Since with using >> PLATFORM_DEVID_AUTO, there is no way to know the exact device name in advance, >> the data given in usb_bind_phy became obsolete and usb_get_phy was failing. >> So MUSB wrapper was modified not to use PLATFORM_DEVID_AUTO. Corresponding >> change is done in board file here. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> arch/arm/mach-omap2/board-2430sdp.c | 2 +- >> arch/arm/mach-omap2/board-3430sdp.c | 2 +- >> arch/arm/mach-omap2/board-cm-t35.c | 2 +- >> arch/arm/mach-omap2/board-devkit8000.c | 2 +- >> arch/arm/mach-omap2/board-igep0020.c | 2 +- >> arch/arm/mach-omap2/board-ldp.c | 2 +- >> arch/arm/mach-omap2/board-omap3beagle.c | 2 +- >> arch/arm/mach-omap2/board-omap3evm.c | 2 +- >> arch/arm/mach-omap2/board-omap3logic.c | 2 +- >> arch/arm/mach-omap2/board-omap3pandora.c | 2 +- >> arch/arm/mach-omap2/board-omap3stalker.c | 2 +- >> arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- >> arch/arm/mach-omap2/board-overo.c | 2 +- >> arch/arm/mach-omap2/board-rm680.c | 2 +- >> arch/arm/mach-omap2/board-rx51.c | 2 +- >> arch/arm/mach-omap2/board-zoom-peripherals.c | 2 +- >> 16 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c >> index 244d8a5..17bb076 100644 >> --- a/arch/arm/mach-omap2/board-2430sdp.c >> +++ b/arch/arm/mach-omap2/board-2430sdp.c >> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) >> omap_hsmmc_init(mmc); >> >> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); >> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); >> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); > > how about moving usb_bind_phy() calls to omap2430.c ? > > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > index f44e8b5..b6abc1a 100644 > --- a/drivers/usb/musb/omap2430.c > +++ b/drivers/usb/musb/omap2430.c > @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) > > pdata->board_data = data; > pdata->config = config; > + } else { > + /* bind the PHY */ > + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); This looks like a hack IMHO to workaround the usb phy library. otherwise it is similar to get_phy_by_name. > } > > if (pdata->has_mailbox) { > > The only problem is if some board comes with a PHY other than > twl4030_usb, but in that case we can add a separate compatible flag and > treat it properly. We only need to bind the phy in non-DT case anyway, right. > so that's temporary. It might be better than to reintroduce the IDR in > musb_core.c. that’s needed for generic phy framework anyway :-s Thanks Kishon
Hi, On Mon, Jul 29, 2013 at 08:59:26PM +0530, Kishon Vijay Abraham I wrote: > >> Previously MUSB wrapper (OMAP) device used PLATFORM_DEVID_AUTO while creating > >> MUSB core device. So in usb_bind_phy (binds the controller with the PHY), the > >> device name of the controller had *.auto* in it. Since with using > >> PLATFORM_DEVID_AUTO, there is no way to know the exact device name in advance, > >> the data given in usb_bind_phy became obsolete and usb_get_phy was failing. > >> So MUSB wrapper was modified not to use PLATFORM_DEVID_AUTO. Corresponding > >> change is done in board file here. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> arch/arm/mach-omap2/board-2430sdp.c | 2 +- > >> arch/arm/mach-omap2/board-3430sdp.c | 2 +- > >> arch/arm/mach-omap2/board-cm-t35.c | 2 +- > >> arch/arm/mach-omap2/board-devkit8000.c | 2 +- > >> arch/arm/mach-omap2/board-igep0020.c | 2 +- > >> arch/arm/mach-omap2/board-ldp.c | 2 +- > >> arch/arm/mach-omap2/board-omap3beagle.c | 2 +- > >> arch/arm/mach-omap2/board-omap3evm.c | 2 +- > >> arch/arm/mach-omap2/board-omap3logic.c | 2 +- > >> arch/arm/mach-omap2/board-omap3pandora.c | 2 +- > >> arch/arm/mach-omap2/board-omap3stalker.c | 2 +- > >> arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- > >> arch/arm/mach-omap2/board-overo.c | 2 +- > >> arch/arm/mach-omap2/board-rm680.c | 2 +- > >> arch/arm/mach-omap2/board-rx51.c | 2 +- > >> arch/arm/mach-omap2/board-zoom-peripherals.c | 2 +- > >> 16 files changed, 16 insertions(+), 16 deletions(-) > >> > >> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c > >> index 244d8a5..17bb076 100644 > >> --- a/arch/arm/mach-omap2/board-2430sdp.c > >> +++ b/arch/arm/mach-omap2/board-2430sdp.c > >> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) > >> omap_hsmmc_init(mmc); > >> > >> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); > >> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); > >> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); > > > > how about moving usb_bind_phy() calls to omap2430.c ? > > > > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > > index f44e8b5..b6abc1a 100644 > > --- a/drivers/usb/musb/omap2430.c > > +++ b/drivers/usb/musb/omap2430.c > > @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) > > > > pdata->board_data = data; > > pdata->config = config; > > + } else { > > + /* bind the PHY */ > > + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); > > This looks like a hack IMHO to workaround the usb phy library. otherwise it is > similar to get_phy_by_name. actually, this is a workaround to the fact that we're not creating all platform_devices in arch/arm/mach-omap2/ :-) If we had the musb allocation there, we could easily handle usb_bind_phy() > > so that's temporary. It might be better than to reintroduce the IDR in > > musb_core.c. > > that’s needed for generic phy framework anyway :-s right, but generic phy framework can handle everything just fine, the only problem is that names are changing.
Hi, On Monday 29 July 2013 11:24 PM, Felipe Balbi wrote: > Hi, > > On Mon, Jul 29, 2013 at 08:59:26PM +0530, Kishon Vijay Abraham I wrote: >>>> Previously MUSB wrapper (OMAP) device used PLATFORM_DEVID_AUTO while creating >>>> MUSB core device. So in usb_bind_phy (binds the controller with the PHY), the >>>> device name of the controller had *.auto* in it. Since with using >>>> PLATFORM_DEVID_AUTO, there is no way to know the exact device name in advance, >>>> the data given in usb_bind_phy became obsolete and usb_get_phy was failing. >>>> So MUSB wrapper was modified not to use PLATFORM_DEVID_AUTO. Corresponding >>>> change is done in board file here. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> arch/arm/mach-omap2/board-2430sdp.c | 2 +- >>>> arch/arm/mach-omap2/board-3430sdp.c | 2 +- >>>> arch/arm/mach-omap2/board-cm-t35.c | 2 +- >>>> arch/arm/mach-omap2/board-devkit8000.c | 2 +- >>>> arch/arm/mach-omap2/board-igep0020.c | 2 +- >>>> arch/arm/mach-omap2/board-ldp.c | 2 +- >>>> arch/arm/mach-omap2/board-omap3beagle.c | 2 +- >>>> arch/arm/mach-omap2/board-omap3evm.c | 2 +- >>>> arch/arm/mach-omap2/board-omap3logic.c | 2 +- >>>> arch/arm/mach-omap2/board-omap3pandora.c | 2 +- >>>> arch/arm/mach-omap2/board-omap3stalker.c | 2 +- >>>> arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- >>>> arch/arm/mach-omap2/board-overo.c | 2 +- >>>> arch/arm/mach-omap2/board-rm680.c | 2 +- >>>> arch/arm/mach-omap2/board-rx51.c | 2 +- >>>> arch/arm/mach-omap2/board-zoom-peripherals.c | 2 +- >>>> 16 files changed, 16 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c >>>> index 244d8a5..17bb076 100644 >>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) >>>> omap_hsmmc_init(mmc); >>>> >>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); >>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); >>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); >>> >>> how about moving usb_bind_phy() calls to omap2430.c ? >>> >>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c >>> index f44e8b5..b6abc1a 100644 >>> --- a/drivers/usb/musb/omap2430.c >>> +++ b/drivers/usb/musb/omap2430.c >>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) >>> >>> pdata->board_data = data; >>> pdata->config = config; >>> + } else { >>> + /* bind the PHY */ >>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); >> >> This looks like a hack IMHO to workaround the usb phy library. otherwise it is >> similar to get_phy_by_name. > > actually, this is a workaround to the fact that we're not creating all > platform_devices in arch/arm/mach-omap2/ :-) > > If we had the musb allocation there, we could easily handle > usb_bind_phy() > >>> so that's temporary. It might be better than to reintroduce the IDR in >>> musb_core.c. >> >> that’s needed for generic phy framework anyway :-s > > right, but generic phy framework can handle everything just fine, the > only problem is that names are changing. right. But if the names change, PHY framework wouldn't be able to return the reference to the PHY. Thanks Kishon
Hi, On Tue, Jul 30, 2013 at 10:44:48AM +0530, Kishon Vijay Abraham I wrote: > > On Mon, Jul 29, 2013 at 08:59:26PM +0530, Kishon Vijay Abraham I wrote: > >>>> Previously MUSB wrapper (OMAP) device used PLATFORM_DEVID_AUTO while creating > >>>> MUSB core device. So in usb_bind_phy (binds the controller with the PHY), the > >>>> device name of the controller had *.auto* in it. Since with using > >>>> PLATFORM_DEVID_AUTO, there is no way to know the exact device name in advance, > >>>> the data given in usb_bind_phy became obsolete and usb_get_phy was failing. > >>>> So MUSB wrapper was modified not to use PLATFORM_DEVID_AUTO. Corresponding > >>>> change is done in board file here. > >>>> > >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >>>> --- > >>>> arch/arm/mach-omap2/board-2430sdp.c | 2 +- > >>>> arch/arm/mach-omap2/board-3430sdp.c | 2 +- > >>>> arch/arm/mach-omap2/board-cm-t35.c | 2 +- > >>>> arch/arm/mach-omap2/board-devkit8000.c | 2 +- > >>>> arch/arm/mach-omap2/board-igep0020.c | 2 +- > >>>> arch/arm/mach-omap2/board-ldp.c | 2 +- > >>>> arch/arm/mach-omap2/board-omap3beagle.c | 2 +- > >>>> arch/arm/mach-omap2/board-omap3evm.c | 2 +- > >>>> arch/arm/mach-omap2/board-omap3logic.c | 2 +- > >>>> arch/arm/mach-omap2/board-omap3pandora.c | 2 +- > >>>> arch/arm/mach-omap2/board-omap3stalker.c | 2 +- > >>>> arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- > >>>> arch/arm/mach-omap2/board-overo.c | 2 +- > >>>> arch/arm/mach-omap2/board-rm680.c | 2 +- > >>>> arch/arm/mach-omap2/board-rx51.c | 2 +- > >>>> arch/arm/mach-omap2/board-zoom-peripherals.c | 2 +- > >>>> 16 files changed, 16 insertions(+), 16 deletions(-) > >>>> > >>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c > >>>> index 244d8a5..17bb076 100644 > >>>> --- a/arch/arm/mach-omap2/board-2430sdp.c > >>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c > >>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) > >>>> omap_hsmmc_init(mmc); > >>>> > >>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); > >>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); > >>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); > >>> > >>> how about moving usb_bind_phy() calls to omap2430.c ? > >>> > >>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > >>> index f44e8b5..b6abc1a 100644 > >>> --- a/drivers/usb/musb/omap2430.c > >>> +++ b/drivers/usb/musb/omap2430.c > >>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) > >>> > >>> pdata->board_data = data; > >>> pdata->config = config; > >>> + } else { > >>> + /* bind the PHY */ > >>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); > >> > >> This looks like a hack IMHO to workaround the usb phy library. otherwise it is > >> similar to get_phy_by_name. > > > > actually, this is a workaround to the fact that we're not creating all > > platform_devices in arch/arm/mach-omap2/ :-) > > > > If we had the musb allocation there, we could easily handle > > usb_bind_phy() > > > >>> so that's temporary. It might be better than to reintroduce the IDR in > >>> musb_core.c. > >> > >> that’s needed for generic phy framework anyway :-s > > > > right, but generic phy framework can handle everything just fine, the > > only problem is that names are changing. > > right. But if the names change, PHY framework wouldn't be able to return the > reference to the PHY. with my suggestion they can change whenever they want since we're using dev_name() of the just-created musb platform_device. Right ?
Hi, On Tuesday 30 July 2013 11:31 AM, Felipe Balbi wrote: > Hi, > > On Tue, Jul 30, 2013 at 10:44:48AM +0530, Kishon Vijay Abraham I wrote: >>> On Mon, Jul 29, 2013 at 08:59:26PM +0530, Kishon Vijay Abraham I wrote: >>>>>> Previously MUSB wrapper (OMAP) device used PLATFORM_DEVID_AUTO while creating >>>>>> MUSB core device. So in usb_bind_phy (binds the controller with the PHY), the >>>>>> device name of the controller had *.auto* in it. Since with using >>>>>> PLATFORM_DEVID_AUTO, there is no way to know the exact device name in advance, >>>>>> the data given in usb_bind_phy became obsolete and usb_get_phy was failing. >>>>>> So MUSB wrapper was modified not to use PLATFORM_DEVID_AUTO. Corresponding >>>>>> change is done in board file here. >>>>>> >>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>> --- >>>>>> arch/arm/mach-omap2/board-2430sdp.c | 2 +- >>>>>> arch/arm/mach-omap2/board-3430sdp.c | 2 +- >>>>>> arch/arm/mach-omap2/board-cm-t35.c | 2 +- >>>>>> arch/arm/mach-omap2/board-devkit8000.c | 2 +- >>>>>> arch/arm/mach-omap2/board-igep0020.c | 2 +- >>>>>> arch/arm/mach-omap2/board-ldp.c | 2 +- >>>>>> arch/arm/mach-omap2/board-omap3beagle.c | 2 +- >>>>>> arch/arm/mach-omap2/board-omap3evm.c | 2 +- >>>>>> arch/arm/mach-omap2/board-omap3logic.c | 2 +- >>>>>> arch/arm/mach-omap2/board-omap3pandora.c | 2 +- >>>>>> arch/arm/mach-omap2/board-omap3stalker.c | 2 +- >>>>>> arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- >>>>>> arch/arm/mach-omap2/board-overo.c | 2 +- >>>>>> arch/arm/mach-omap2/board-rm680.c | 2 +- >>>>>> arch/arm/mach-omap2/board-rx51.c | 2 +- >>>>>> arch/arm/mach-omap2/board-zoom-peripherals.c | 2 +- >>>>>> 16 files changed, 16 insertions(+), 16 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c >>>>>> index 244d8a5..17bb076 100644 >>>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) >>>>>> omap_hsmmc_init(mmc); >>>>>> >>>>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); >>>>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); >>>>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); >>>>> >>>>> how about moving usb_bind_phy() calls to omap2430.c ? >>>>> >>>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c >>>>> index f44e8b5..b6abc1a 100644 >>>>> --- a/drivers/usb/musb/omap2430.c >>>>> +++ b/drivers/usb/musb/omap2430.c >>>>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) >>>>> >>>>> pdata->board_data = data; >>>>> pdata->config = config; >>>>> + } else { >>>>> + /* bind the PHY */ >>>>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); >>>> >>>> This looks like a hack IMHO to workaround the usb phy library. otherwise it is >>>> similar to get_phy_by_name. >>> >>> actually, this is a workaround to the fact that we're not creating all >>> platform_devices in arch/arm/mach-omap2/ :-) >>> >>> If we had the musb allocation there, we could easily handle >>> usb_bind_phy() >>> >>>>> so that's temporary. It might be better than to reintroduce the IDR in >>>>> musb_core.c. >>>> >>>> that’s needed for generic phy framework anyway :-s >>> >>> right, but generic phy framework can handle everything just fine, the >>> only problem is that names are changing. >> >> right. But if the names change, PHY framework wouldn't be able to return the >> reference to the PHY. > > with my suggestion they can change whenever they want since we're using > dev_name() of the just-created musb platform_device. Right ? right. But the PHY device can be created in a different place from where the musb devices are created. And in the PHY framework, the PHY device should have the list of controller device (names) it can support (PHY framework does not maintain a separate list for binding like how we had in USB PHY library). e.g. http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such cases how do we pass the device names. Also will the MUSB core device be created before twl4030-usb PHY device? Thanks Kishon
Hi, On Tue, Jul 30, 2013 at 11:41:23AM +0530, Kishon Vijay Abraham I wrote: > >>>>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c > >>>>>> index 244d8a5..17bb076 100644 > >>>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c > >>>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c > >>>>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) > >>>>>> omap_hsmmc_init(mmc); > >>>>>> > >>>>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); > >>>>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); > >>>>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); > >>>>> > >>>>> how about moving usb_bind_phy() calls to omap2430.c ? > >>>>> > >>>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > >>>>> index f44e8b5..b6abc1a 100644 > >>>>> --- a/drivers/usb/musb/omap2430.c > >>>>> +++ b/drivers/usb/musb/omap2430.c > >>>>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) > >>>>> > >>>>> pdata->board_data = data; > >>>>> pdata->config = config; > >>>>> + } else { > >>>>> + /* bind the PHY */ > >>>>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); > >>>> > >>>> This looks like a hack IMHO to workaround the usb phy library. otherwise it is > >>>> similar to get_phy_by_name. > >>> > >>> actually, this is a workaround to the fact that we're not creating all > >>> platform_devices in arch/arm/mach-omap2/ :-) > >>> > >>> If we had the musb allocation there, we could easily handle > >>> usb_bind_phy() > >>> > >>>>> so that's temporary. It might be better than to reintroduce the IDR in > >>>>> musb_core.c. > >>>> > >>>> that’s needed for generic phy framework anyway :-s > >>> > >>> right, but generic phy framework can handle everything just fine, the > >>> only problem is that names are changing. > >> > >> right. But if the names change, PHY framework wouldn't be able to return the > >> reference to the PHY. > > > > with my suggestion they can change whenever they want since we're using > > dev_name() of the just-created musb platform_device. Right ? > > right. But the PHY device can be created in a different place from where the > musb devices are created. And in the PHY framework, the PHY device should have this shouldn't be a problem. As long as the phy is created, all should be good. > the list of controller device (names) it can support (PHY framework does not > maintain a separate list for binding like how we had in USB PHY library). e.g. > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such this has nothing to do with $subject though. We talk about generic PHY framework once all these PHY drivers are moved there :-) > cases how do we pass the device names. Also will the MUSB core device be > created before twl4030-usb PHY device? and why would that be a problem ? We're telling the framework that the musb device will use a phy with a name of 'twl4030'. If musb calls usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and try again later.
Hi, On Tuesday 30 July 2013 11:48 AM, Felipe Balbi wrote: > Hi, > > On Tue, Jul 30, 2013 at 11:41:23AM +0530, Kishon Vijay Abraham I wrote: >>>>>>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c >>>>>>>> index 244d8a5..17bb076 100644 >>>>>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>>>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>>>>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) >>>>>>>> omap_hsmmc_init(mmc); >>>>>>>> >>>>>>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); >>>>>>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); >>>>>>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); >>>>>>> >>>>>>> how about moving usb_bind_phy() calls to omap2430.c ? >>>>>>> >>>>>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c >>>>>>> index f44e8b5..b6abc1a 100644 >>>>>>> --- a/drivers/usb/musb/omap2430.c >>>>>>> +++ b/drivers/usb/musb/omap2430.c >>>>>>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) >>>>>>> >>>>>>> pdata->board_data = data; >>>>>>> pdata->config = config; >>>>>>> + } else { >>>>>>> + /* bind the PHY */ >>>>>>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); >>>>>> >>>>>> This looks like a hack IMHO to workaround the usb phy library. otherwise it is >>>>>> similar to get_phy_by_name. >>>>> >>>>> actually, this is a workaround to the fact that we're not creating all >>>>> platform_devices in arch/arm/mach-omap2/ :-) >>>>> >>>>> If we had the musb allocation there, we could easily handle >>>>> usb_bind_phy() >>>>> >>>>>>> so that's temporary. It might be better than to reintroduce the IDR in >>>>>>> musb_core.c. >>>>>> >>>>>> that’s needed for generic phy framework anyway :-s >>>>> >>>>> right, but generic phy framework can handle everything just fine, the >>>>> only problem is that names are changing. >>>> >>>> right. But if the names change, PHY framework wouldn't be able to return the >>>> reference to the PHY. >>> >>> with my suggestion they can change whenever they want since we're using >>> dev_name() of the just-created musb platform_device. Right ? >> >> right. But the PHY device can be created in a different place from where the >> musb devices are created. And in the PHY framework, the PHY device should have > > this shouldn't be a problem. As long as the phy is created, all should > be good. > >> the list of controller device (names) it can support (PHY framework does not >> maintain a separate list for binding like how we had in USB PHY library). e.g. >> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such > > this has nothing to do with $subject though. We talk about generic PHY > framework once all these PHY drivers are moved there :-) > >> cases how do we pass the device names. Also will the MUSB core device be >> created before twl4030-usb PHY device? > > and why would that be a problem ? We're telling the framework that the > musb device will use a phy with a name of 'twl4030'. If musb calls > usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and > try again later. I think we are talking about different problems here ;-) I'm trying to tell using idr in MUSB core is needed for Generic PHY Framework. So in a way, the Generic PHY Framework series depends on this patch series or else MUSB in OMAP3 platforms wont work after Generic PHY framework gets merged. Thanks Kishon
Hi, On Tue, Jul 30, 2013 at 11:55:04AM +0530, Kishon Vijay Abraham I wrote: > > On Tue, Jul 30, 2013 at 11:41:23AM +0530, Kishon Vijay Abraham I wrote: > >>>>>>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c > >>>>>>>> index 244d8a5..17bb076 100644 > >>>>>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c > >>>>>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c > >>>>>>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) > >>>>>>>> omap_hsmmc_init(mmc); > >>>>>>>> > >>>>>>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); > >>>>>>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); > >>>>>>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); > >>>>>>> > >>>>>>> how about moving usb_bind_phy() calls to omap2430.c ? > >>>>>>> > >>>>>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > >>>>>>> index f44e8b5..b6abc1a 100644 > >>>>>>> --- a/drivers/usb/musb/omap2430.c > >>>>>>> +++ b/drivers/usb/musb/omap2430.c > >>>>>>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) > >>>>>>> > >>>>>>> pdata->board_data = data; > >>>>>>> pdata->config = config; > >>>>>>> + } else { > >>>>>>> + /* bind the PHY */ > >>>>>>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); > >>>>>> > >>>>>> This looks like a hack IMHO to workaround the usb phy library. otherwise it is > >>>>>> similar to get_phy_by_name. > >>>>> > >>>>> actually, this is a workaround to the fact that we're not creating all > >>>>> platform_devices in arch/arm/mach-omap2/ :-) > >>>>> > >>>>> If we had the musb allocation there, we could easily handle > >>>>> usb_bind_phy() > >>>>> > >>>>>>> so that's temporary. It might be better than to reintroduce the IDR in > >>>>>>> musb_core.c. > >>>>>> > >>>>>> that’s needed for generic phy framework anyway :-s > >>>>> > >>>>> right, but generic phy framework can handle everything just fine, the > >>>>> only problem is that names are changing. > >>>> > >>>> right. But if the names change, PHY framework wouldn't be able to return the > >>>> reference to the PHY. > >>> > >>> with my suggestion they can change whenever they want since we're using > >>> dev_name() of the just-created musb platform_device. Right ? > >> > >> right. But the PHY device can be created in a different place from where the > >> musb devices are created. And in the PHY framework, the PHY device should have > > > > this shouldn't be a problem. As long as the phy is created, all should > > be good. > > > >> the list of controller device (names) it can support (PHY framework does not > >> maintain a separate list for binding like how we had in USB PHY library). e.g. > >> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such > > > > this has nothing to do with $subject though. We talk about generic PHY > > framework once all these PHY drivers are moved there :-) > > > >> cases how do we pass the device names. Also will the MUSB core device be > >> created before twl4030-usb PHY device? > > > > and why would that be a problem ? We're telling the framework that the > > musb device will use a phy with a name of 'twl4030'. If musb calls > > usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and > > try again later. > > I think we are talking about different problems here ;-) I'm trying to tell > using idr in MUSB core is needed for Generic PHY Framework. So in a way, the > Generic PHY Framework series depends on this patch series or else MUSB in OMAP3 > platforms wont work after Generic PHY framework gets merged. then you just found a limitation in your framework, right ? :-) I mean, imagine if now we have to add an IDR to every single user of your framework because they could end up in systems with multiple instances of the same IP ? Now consider that you aim to have your framework be used by Network, USB, SATA, Graphics, etc... Have you really only considered DT platforms ? DT is quite easy since you can require folks to pass the proper phandle, but drivers will want to use PLATFORM_DEVID_AUTO and your framework needs to cope with that.
Hi, On Tuesday 30 July 2013 11:58 AM, Felipe Balbi wrote: > Hi, > > On Tue, Jul 30, 2013 at 11:55:04AM +0530, Kishon Vijay Abraham I wrote: >>> On Tue, Jul 30, 2013 at 11:41:23AM +0530, Kishon Vijay Abraham I wrote: >>>>>>>>>> diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c >>>>>>>>>> index 244d8a5..17bb076 100644 >>>>>>>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>>>>>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>>>>>>>> @@ -233,7 +233,7 @@ static void __init omap_2430sdp_init(void) >>>>>>>>>> omap_hsmmc_init(mmc); >>>>>>>>>> >>>>>>>>>> omap_mux_init_signal("usb0hs_stp", OMAP_PULL_ENA | OMAP_PULL_UP); >>>>>>>>>> - usb_bind_phy("musb-hdrc.0.auto", 0, "twl4030_usb"); >>>>>>>>>> + usb_bind_phy("musb-hdrc.0", 0, "twl4030_usb"); >>>>>>>>> >>>>>>>>> how about moving usb_bind_phy() calls to omap2430.c ? >>>>>>>>> >>>>>>>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c >>>>>>>>> index f44e8b5..b6abc1a 100644 >>>>>>>>> --- a/drivers/usb/musb/omap2430.c >>>>>>>>> +++ b/drivers/usb/musb/omap2430.c >>>>>>>>> @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) >>>>>>>>> >>>>>>>>> pdata->board_data = data; >>>>>>>>> pdata->config = config; >>>>>>>>> + } else { >>>>>>>>> + /* bind the PHY */ >>>>>>>>> + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); >>>>>>>> >>>>>>>> This looks like a hack IMHO to workaround the usb phy library. otherwise it is >>>>>>>> similar to get_phy_by_name. >>>>>>> >>>>>>> actually, this is a workaround to the fact that we're not creating all >>>>>>> platform_devices in arch/arm/mach-omap2/ :-) >>>>>>> >>>>>>> If we had the musb allocation there, we could easily handle >>>>>>> usb_bind_phy() >>>>>>> >>>>>>>>> so that's temporary. It might be better than to reintroduce the IDR in >>>>>>>>> musb_core.c. >>>>>>>> >>>>>>>> that’s needed for generic phy framework anyway :-s >>>>>>> >>>>>>> right, but generic phy framework can handle everything just fine, the >>>>>>> only problem is that names are changing. >>>>>> >>>>>> right. But if the names change, PHY framework wouldn't be able to return the >>>>>> reference to the PHY. >>>>> >>>>> with my suggestion they can change whenever they want since we're using >>>>> dev_name() of the just-created musb platform_device. Right ? >>>> >>>> right. But the PHY device can be created in a different place from where the >>>> musb devices are created. And in the PHY framework, the PHY device should have >>> >>> this shouldn't be a problem. As long as the phy is created, all should >>> be good. >>> >>>> the list of controller device (names) it can support (PHY framework does not >>>> maintain a separate list for binding like how we had in USB PHY library). e.g. >>>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such >>> >>> this has nothing to do with $subject though. We talk about generic PHY >>> framework once all these PHY drivers are moved there :-) >>> >>>> cases how do we pass the device names. Also will the MUSB core device be >>>> created before twl4030-usb PHY device? >>> >>> and why would that be a problem ? We're telling the framework that the >>> musb device will use a phy with a name of 'twl4030'. If musb calls >>> usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and >>> try again later. >> >> I think we are talking about different problems here ;-) I'm trying to tell >> using idr in MUSB core is needed for Generic PHY Framework. So in a way, the >> Generic PHY Framework series depends on this patch series or else MUSB in OMAP3 >> platforms wont work after Generic PHY framework gets merged. > > then you just found a limitation in your framework, right ? :-) I mean, > imagine if now we have to add an IDR to every single user of your > framework because they could end up in systems with multiple instances > of the same IP ? I raised a similar concern in the PHY framework discussion [1] :-) And since it's used everywhere else regulators, clkdev, etc.. it's agreed to be used in PHY as well. Btw if PLATFORM_DEVID_AUTO is used even regulator, clk_get should fail IMO. [1] -> http://lkml.indiana.edu/hypermail/linux/kernel/1307.2/03573.html Thanks Kishon
Hi, On Tue, Jul 30, 2013 at 12:16:20PM +0530, Kishon Vijay Abraham I wrote: > >>>> the list of controller device (names) it can support (PHY framework does not > >>>> maintain a separate list for binding like how we had in USB PHY library). e.g. > >>>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such > >>> > >>> this has nothing to do with $subject though. We talk about generic PHY > >>> framework once all these PHY drivers are moved there :-) > >>> > >>>> cases how do we pass the device names. Also will the MUSB core device be > >>>> created before twl4030-usb PHY device? > >>> > >>> and why would that be a problem ? We're telling the framework that the > >>> musb device will use a phy with a name of 'twl4030'. If musb calls > >>> usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and > >>> try again later. > >> > >> I think we are talking about different problems here ;-) I'm trying to tell > >> using idr in MUSB core is needed for Generic PHY Framework. So in a way, the > >> Generic PHY Framework series depends on this patch series or else MUSB in OMAP3 > >> platforms wont work after Generic PHY framework gets merged. > > > > then you just found a limitation in your framework, right ? :-) I mean, > > imagine if now we have to add an IDR to every single user of your > > framework because they could end up in systems with multiple instances > > of the same IP ? > > I raised a similar concern in the PHY framework discussion [1] :-) And since > it's used everywhere else regulators, clkdev, etc.. it's agreed to be used in > PHY as well. Btw if PLATFORM_DEVID_AUTO is used even regulator, clk_get should > fail IMO. > > [1] -> http://lkml.indiana.edu/hypermail/linux/kernel/1307.2/03573.html look at Greg's and my reply to that email.
On Tuesday 30 July 2013 12:46 PM, Felipe Balbi wrote: > Hi, > > On Tue, Jul 30, 2013 at 12:16:20PM +0530, Kishon Vijay Abraham I wrote: >>>>>> the list of controller device (names) it can support (PHY framework does not >>>>>> maintain a separate list for binding like how we had in USB PHY library). e.g. >>>>>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such >>>>> >>>>> this has nothing to do with $subject though. We talk about generic PHY >>>>> framework once all these PHY drivers are moved there :-) >>>>> >>>>>> cases how do we pass the device names. Also will the MUSB core device be >>>>>> created before twl4030-usb PHY device? >>>>> >>>>> and why would that be a problem ? We're telling the framework that the >>>>> musb device will use a phy with a name of 'twl4030'. If musb calls >>>>> usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and >>>>> try again later. >>>> >>>> I think we are talking about different problems here ;-) I'm trying to tell >>>> using idr in MUSB core is needed for Generic PHY Framework. So in a way, the >>>> Generic PHY Framework series depends on this patch series or else MUSB in OMAP3 >>>> platforms wont work after Generic PHY framework gets merged. >>> >>> then you just found a limitation in your framework, right ? :-) I mean, >>> imagine if now we have to add an IDR to every single user of your >>> framework because they could end up in systems with multiple instances >>> of the same IP ? >> >> I raised a similar concern in the PHY framework discussion [1] :-) And since >> it's used everywhere else regulators, clkdev, etc.. it's agreed to be used in >> PHY as well. Btw if PLATFORM_DEVID_AUTO is used even regulator, clk_get should >> fail IMO. >> >> [1] -> http://lkml.indiana.edu/hypermail/linux/kernel/1307.2/03573.html > > look at Greg's and my reply to that email. but finally Greg agreed to what Tomasz proposed no? Thanks Kishon
On Tue, Jul 30, 2013 at 01:41:23PM +0530, Kishon Vijay Abraham I wrote: > On Tuesday 30 July 2013 12:46 PM, Felipe Balbi wrote: > > Hi, > > > > On Tue, Jul 30, 2013 at 12:16:20PM +0530, Kishon Vijay Abraham I wrote: > >>>>>> the list of controller device (names) it can support (PHY framework does not > >>>>>> maintain a separate list for binding like how we had in USB PHY library). e.g. > >>>>>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg92817.html. In such > >>>>> > >>>>> this has nothing to do with $subject though. We talk about generic PHY > >>>>> framework once all these PHY drivers are moved there :-) > >>>>> > >>>>>> cases how do we pass the device names. Also will the MUSB core device be > >>>>>> created before twl4030-usb PHY device? > >>>>> > >>>>> and why would that be a problem ? We're telling the framework that the > >>>>> musb device will use a phy with a name of 'twl4030'. If musb calls > >>>>> usb_get_phy_dev() and doesn't find a phy, it'll return -EPROBE_DEFER and > >>>>> try again later. > >>>> > >>>> I think we are talking about different problems here ;-) I'm trying to tell > >>>> using idr in MUSB core is needed for Generic PHY Framework. So in a way, the > >>>> Generic PHY Framework series depends on this patch series or else MUSB in OMAP3 > >>>> platforms wont work after Generic PHY framework gets merged. > >>> > >>> then you just found a limitation in your framework, right ? :-) I mean, > >>> imagine if now we have to add an IDR to every single user of your > >>> framework because they could end up in systems with multiple instances > >>> of the same IP ? > >> > >> I raised a similar concern in the PHY framework discussion [1] :-) And since > >> it's used everywhere else regulators, clkdev, etc.. it's agreed to be used in > >> PHY as well. Btw if PLATFORM_DEVID_AUTO is used even regulator, clk_get should > >> fail IMO. > >> > >> [1] -> http://lkml.indiana.edu/hypermail/linux/kernel/1307.2/03573.html > > > > look at Greg's and my reply to that email. > > but finally Greg agreed to what Tomasz proposed no? that's not what I see in the thread. I see Greg agreed to regulator's own IDs being sequentially created, but he mentions device names can change.
On Tue, Jul 30, 2013 at 11:15:20AM +0300, Felipe Balbi wrote: > > > look at Greg's and my reply to that email. > > > > but finally Greg agreed to what Tomasz proposed no? > > that's not what I see in the thread. I see Greg agreed to regulator's > own IDs being sequentially created, but he mentions device names can > change. And that no one should _ever_ rely on them to be a specific "name", the bus is responsible for creating the id, it could be "random" and everything should work just fine. thanks, greg k-h
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c index f44e8b5..b6abc1a 100644 --- a/drivers/usb/musb/omap2430.c +++ b/drivers/usb/musb/omap2430.c @@ -544,6 +544,9 @@ static int omap2430_probe(struct platform_device *pdev) pdata->board_data = data; pdata->config = config; + } else { + /* bind the PHY */ + usb_bind_phy(dev_name(&musb->dev), 0, "twl4030_usb"); } if (pdata->has_mailbox) {