[PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Johan Hovold]

I've been carrying a patch out-of-tree since my work on improving the
USB device-tree support which is needed to be able to describe USB
topologies for musb based controllers.

This patch, which associates the platform controller device with the
glue device device-tree node, did not play well with the recent changes
which added generic phy support to USB core however.

Like the recent dwc2 regression fixed by Arnd after the device-tree
#phy-cell changes, the generic phy code in USB core can now also fail
indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
phy.

The second patch addresses this for musb, which handles its own (legacy
and generic) phys, but something more may possibly now be needed for
other platforms with legacy phys.

In the process of debugging this, I stumbled over another issue which
caused the dsps legacy phy init two be called twice on every probe and
which is fixed by the first patch.

Johan


Johan Hovold (3):
  USB: musb: dsps: drop duplicate phy initialisation
  USB: musb: host: prevent core phy initialisation
  USB: musb: dsps: propagate device-tree node

 drivers/usb/musb/musb_dsps.c | 3 +--
 drivers/usb/musb/musb_host.c | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.0

[PATCH 1/3] USB: musb: dsps: drop duplicate phy initialisation

[Johan Hovold]

Since commit 39cee200c23e ("usb: musb: core: call init and shutdown for
the usb phy") the musb USB phy is initialised by musb_core, but the
original initialisation in the dsps-glue init callback was left in
place resulting in two calls to phy init during probe (and similarly,
two shutdowns on remove).

Drop the duplicate phy init and shutdown calls from the dsps glue in
favour of the ones in musb core, which other glue drivers rely on.

Note however that any generic phy is still initialised in the glue init
callback (just as for the other drivers).

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_dsps.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 05a679d5e3a2..6a60bc0490c5 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -451,7 +451,6 @@ static int dsps_musb_init(struct musb *musb)
 	if (!rev)
 		return -ENODEV;
 
-	usb_phy_init(musb->xceiv);
 	if (IS_ERR(musb->phy))  {
 		musb->phy = NULL;
 	} else {
@@ -501,7 +500,6 @@ static int dsps_musb_exit(struct musb *musb)
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
 
 	del_timer_sync(&musb->dev_timer);
-	usb_phy_shutdown(musb->xceiv);
 	phy_power_off(musb->phy);
 	phy_exit(musb->phy);
 	debugfs_remove_recursive(glue->dbgfs_root);
-- 
2.17.0

Re: [PATCH 1/3] USB: musb: dsps: drop duplicate phy initialisation

[Uwe Kleine-König]

On Fri, Apr 13, 2018 at 05:15:03PM +0200, Johan Hovold wrote:
> Since commit 39cee200c23e ("usb: musb: core: call init and shutdown for
> the usb phy") the musb USB phy is initialised by musb_core, but the
> original initialisation in the dsps-glue init callback was left in
> place resulting in two calls to phy init during probe (and similarly,
> two shutdowns on remove).
> 
> Drop the duplicate phy init and shutdown calls from the dsps glue in
> favour of the ones in musb core, which other glue drivers rely on.

Hmm, I don't remember the details of my debug session that led to
39cee200c23e, and I don't have access to the hardware in question any
more.

But your commit logs makes sense, so

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Given that it took 2+ years to find this, backporting to stable and a
Fixes: line are probably not necessary.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/3] USB: musb: host: prevent core phy initialisation

[Johan Hovold]

Set the new HCD flag which prevents USB core from trying to manage our
phys.

This is needed to be able to associate the controller platform device
with the glue device device-tree node on the BBB which uses legacy USB
phys. Otherwise, the generic phy lookup in usb_phy_roothub_init() and
thus HCD registration fails repeatedly with -EPROBE_DEFER (see commit
178a0bce05cb ("usb: core: hcd: integrate the PHY wrapper into the HCD
core")).

Note that a related phy-lookup issue was recently worked around in the
phy core by commit b7563e2796f8 ("phy: work around 'phys' references to
usb-nop-xceiv devices"). Something similar may now be needed for other
USB phys, and in particular if we eventually want to let USB core manage
musb generic phys.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_host.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 3a8451a15f7f..4fa372c845e1 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2754,6 +2754,7 @@ int musb_host_setup(struct musb *musb, int power_budget)
 	hcd->self.otg_port = 1;
 	musb->xceiv->otg->host = &hcd->self;
 	hcd->power_budget = 2 * (power_budget ? : 250);
+	hcd->skip_phy_initialization = 1;
 
 	ret = usb_add_hcd(hcd, 0, 0);
 	if (ret < 0)
-- 
2.17.0

[PATCH 3/3] USB: musb: dsps: propagate device-tree node

[Johan Hovold]

To be able to use DSPS-based controllers with device-tree descriptions
of the USB topology, we need to associate the glue device's device-tree
node with the child controller device.

Note that this can also be used to eventually let USB core manage
generic phys.

Also note that the other glue drivers will require similar changes to be
able to describe their buses in DT.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_dsps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 6a60bc0490c5..23dba59045a7 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -786,6 +786,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
 	musb->dev.parent		= dev;
 	musb->dev.dma_mask		= &musb_dmamask;
 	musb->dev.coherent_dma_mask	= musb_dmamask;
+	device_set_of_node_from_dev(&musb->dev, &parent->dev);
 
 	glue->musb = musb;
 
-- 
2.17.0

Re: [PATCH 3/3] USB: musb: dsps: propagate device-tree node

[Bin Liu]

Johan,

On Fri, Apr 13, 2018 at 05:15:05PM +0200, Johan Hovold wrote:
> To be able to use DSPS-based controllers with device-tree descriptions
> of the USB topology, we need to associate the glue device's device-tree
> node with the child controller device.
> 
> Note that this can also be used to eventually let USB core manage
> generic phys.
> 
> Also note that the other glue drivers will require similar changes to be
> able to describe their buses in DT.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

I will take other two patches for v4.17 rc cycles, but is there any
problem if I take this patch for v4.18-rc1?

Regards,
-Bin.

Re: [PATCH 3/3] USB: musb: dsps: propagate device-tree node

[Johan Hovold]

On Mon, Apr 16, 2018 at 03:03:12PM -0500, Bin Liu wrote:
> Johan,
> 
> On Fri, Apr 13, 2018 at 05:15:05PM +0200, Johan Hovold wrote:
> > To be able to use DSPS-based controllers with device-tree descriptions
> > of the USB topology, we need to associate the glue device's device-tree
> > node with the child controller device.
> > 
> > Note that this can also be used to eventually let USB core manage
> > generic phys.
> > 
> > Also note that the other glue drivers will require similar changes to be
> > able to describe their buses in DT.
> > 
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> I will take other two patches for v4.17 rc cycles, but is there any
> problem if I take this patch for v4.18-rc1?

None at all. That sounds good to me.

Thanks,
Johan

Re: [PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Bin Liu]

Johan,

On Fri, Apr 13, 2018 at 05:15:02PM +0200, Johan Hovold wrote:
> I've been carrying a patch out-of-tree since my work on improving the
> USB device-tree support which is needed to be able to describe USB
> topologies for musb based controllers.
> 
> This patch, which associates the platform controller device with the
> glue device device-tree node, did not play well with the recent changes
> which added generic phy support to USB core however.
> 
> Like the recent dwc2 regression fixed by Arnd after the device-tree
> #phy-cell changes, the generic phy code in USB core can now also fail
> indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
> phy.
> 
> The second patch addresses this for musb, which handles its own (legacy
> and generic) phys, but something more may possibly now be needed for
> other platforms with legacy phys.
> 
> In the process of debugging this, I stumbled over another issue which
> caused the dsps legacy phy init two be called twice on every probe and
> which is fixed by the first patch.
> 
> Johan
> 
> 
> Johan Hovold (3):
>   USB: musb: dsps: drop duplicate phy initialisation
>   USB: musb: host: prevent core phy initialisation

Are the two bugs only affecting you with your out-of-tree patch? It
seems don't have any functional impact for me. I need to make a decision
if these two patches need to go to the stable trees...

Regards,
-Bin.

Re: [PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Johan Hovold]

On Wed, Apr 18, 2018 at 11:20:15AM -0500, Bin Liu wrote:
> Johan,
> 
> On Fri, Apr 13, 2018 at 05:15:02PM +0200, Johan Hovold wrote:
> > I've been carrying a patch out-of-tree since my work on improving the
> > USB device-tree support which is needed to be able to describe USB
> > topologies for musb based controllers.
> > 
> > This patch, which associates the platform controller device with the
> > glue device device-tree node, did not play well with the recent changes
> > which added generic phy support to USB core however.
> > 
> > Like the recent dwc2 regression fixed by Arnd after the device-tree
> > #phy-cell changes, the generic phy code in USB core can now also fail
> > indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
> > phy.
> > 
> > The second patch addresses this for musb, which handles its own (legacy
> > and generic) phys, but something more may possibly now be needed for
> > other platforms with legacy phys.
> > 
> > In the process of debugging this, I stumbled over another issue which
> > caused the dsps legacy phy init two be called twice on every probe and
> > which is fixed by the first patch.
> > 
> > Johan
> > 
> > 
> > Johan Hovold (3):
> >   USB: musb: dsps: drop duplicate phy initialisation
> >   USB: musb: host: prevent core phy initialisation
> 
> Are the two bugs only affecting you with your out-of-tree patch? It
> seems don't have any functional impact for me. I need to make a decision
> if these two patches need to go to the stable trees...

The first bug is independent of the third patch (the "out-of-tree"
patch), but as Uwe also noted it seems to be mostly benign since it took
two years for it to be discovered. For that reason, and to minimise the
risk of any stable regressions, I did not mark it for stable.

The second fix is really only needed with the third of_node patch since
I don't think any of the glue drivers propagates the device-tree node in
this way currently. Hence it could also wait for 3.18, and it is in any
case not stable material as the generic-phy support in USB core is new
in 3.17.

Note that other host controllers may have a device-tree node with
associated legacy-phys however and that this could now lead to similar
problems starting with 3.17.

Thanks,
Johan

Re: [PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Bin Liu]

On Wed, Apr 18, 2018 at 08:46:40PM +0200, Johan Hovold wrote:
> On Wed, Apr 18, 2018 at 11:20:15AM -0500, Bin Liu wrote:
> > Johan,
> > 
> > On Fri, Apr 13, 2018 at 05:15:02PM +0200, Johan Hovold wrote:
> > > I've been carrying a patch out-of-tree since my work on improving the
> > > USB device-tree support which is needed to be able to describe USB
> > > topologies for musb based controllers.
> > > 
> > > This patch, which associates the platform controller device with the
> > > glue device device-tree node, did not play well with the recent changes
> > > which added generic phy support to USB core however.
> > > 
> > > Like the recent dwc2 regression fixed by Arnd after the device-tree
> > > #phy-cell changes, the generic phy code in USB core can now also fail
> > > indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
> > > phy.
> > > 
> > > The second patch addresses this for musb, which handles its own (legacy
> > > and generic) phys, but something more may possibly now be needed for
> > > other platforms with legacy phys.
> > > 
> > > In the process of debugging this, I stumbled over another issue which
> > > caused the dsps legacy phy init two be called twice on every probe and
> > > which is fixed by the first patch.
> > > 
> > > Johan
> > > 
> > > 
> > > Johan Hovold (3):
> > >   USB: musb: dsps: drop duplicate phy initialisation
> > >   USB: musb: host: prevent core phy initialisation
> > 
> > Are the two bugs only affecting you with your out-of-tree patch? It
> > seems don't have any functional impact for me. I need to make a decision
> > if these two patches need to go to the stable trees...
> 
> The first bug is independent of the third patch (the "out-of-tree"
> patch), but as Uwe also noted it seems to be mostly benign since it took
> two years for it to be discovered. For that reason, and to minimise the
> risk of any stable regressions, I did not mark it for stable.
> 
> The second fix is really only needed with the third of_node patch since
> I don't think any of the glue drivers propagates the device-tree node in
> this way currently. Hence it could also wait for 3.18, and it is in any
> case not stable material as the generic-phy support in USB core is new
> in 3.17.

Great, thanks for confirming. I will not send them for stable trees.

> 
> Note that other host controllers may have a device-tree node with
> associated legacy-phys however and that this could now lead to similar
> problems starting with 3.17.

regards,
-Bin.

Re: [PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Martin Blumenstingl]

Hi Johan,

On Fri, Apr 13, 2018 at 5:15 PM, Johan Hovold <johan@kernel.org> wrote:
> I've been carrying a patch out-of-tree since my work on improving the
> USB device-tree support which is needed to be able to describe USB
> topologies for musb based controllers.
>
> This patch, which associates the platform controller device with the
> glue device device-tree node, did not play well with the recent changes
> which added generic phy support to USB core however.
I'm the one who added this

> Like the recent dwc2 regression fixed by Arnd after the device-tree
> #phy-cell changes, the generic phy code in USB core can now also fail
> indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
> phy.
>
> The second patch addresses this for musb, which handles its own (legacy
> and generic) phys, but something more may possibly now be needed for
> other platforms with legacy phys.
I'm not sure if I understand the problem yet - could you please
explain with your words what "legacy PHYs" are and how the "conflict"
with the PHY handling in USB core?

I am aware of two PHY subsystems:
- drivers/phy
-- also called "generic PHY framework"
-- uses a "phys" property
- drivers/usb/phy
-- also called "USB PHY framework"
-- AFAIK this should not be used for new drivers
-- uses an "usb-phy" property

the new PHY handling in USB core only parses the "phys" property and
thus should not conflict with "usb-phy" (the legacy property)

however, I probably missed something so I'd appreciate an explanation
how things can break

> In the process of debugging this, I stumbled over another issue which
> caused the dsps legacy phy init two be called twice on every probe and
> which is fixed by the first patch.
>
> Johan
>
>
> Johan Hovold (3):
>   USB: musb: dsps: drop duplicate phy initialisation
>   USB: musb: host: prevent core phy initialisation
>   USB: musb: dsps: propagate device-tree node
>
>  drivers/usb/musb/musb_dsps.c | 3 +--
>  drivers/usb/musb/musb_host.c | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> --
> 2.17.0
>

Regards
Martin

Re: [PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Johan Hovold]

On Wed, Apr 18, 2018 at 09:18:30PM +0200, Martin Blumenstingl wrote:
> Hi Johan,
> 
> On Fri, Apr 13, 2018 at 5:15 PM, Johan Hovold <johan@kernel.org> wrote:
> > I've been carrying a patch out-of-tree since my work on improving the
> > USB device-tree support which is needed to be able to describe USB
> > topologies for musb based controllers.
> >
> > This patch, which associates the platform controller device with the
> > glue device device-tree node, did not play well with the recent changes
> > which added generic phy support to USB core however.
> I'm the one who added this
> 
> > Like the recent dwc2 regression fixed by Arnd after the device-tree
> > #phy-cell changes, the generic phy code in USB core can now also fail
> > indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
> > phy.
> >
> > The second patch addresses this for musb, which handles its own (legacy
> > and generic) phys, but something more may possibly now be needed for
> > other platforms with legacy phys.
> I'm not sure if I understand the problem yet - could you please
> explain with your words what "legacy PHYs" are and how the "conflict"
> with the PHY handling in USB core?
> 
> I am aware of two PHY subsystems:
> - drivers/phy
> -- also called "generic PHY framework"
> -- uses a "phys" property

Right, and these are sometimes referred to as generic PHYs, as opposed
to...

> - drivers/usb/phy
> -- also called "USB PHY framework"
> -- AFAIK this should not be used for new drivers

...the legacy ones.

> -- uses an "usb-phy" property

This is unfortunately not always the case however; some legacy USB phys
are also referred to using a "phys" property...

> the new PHY handling in USB core only parses the "phys" property and
> thus should not conflict with "usb-phy" (the legacy property)

> however, I probably missed something so I'd appreciate an explanation
> how things can break

...and that is the problem. Specifically, since last fall when a number
of legacy-phy nodes had a #phy-cells property added to them (to silence
DTC warnings), the generic PHY subsubsystem can now return -EPROBE_DEFER
indefinitely when looking up a phy as it finds a matching device-tree
node, but for which there will never be a generic phy registered (since
it's handled by a legacy-phy driver).

I referred to Arnd's workaround for "usb-nop-xceiv" devices

	b7563e2796f8 ("phy: work around 'phys' references to
	usb-nop-xceiv devices")

which has some more background on this.

So if we have any other host controllers out there using
"phys"-properties with legacy phys other than "usb-nop-xceiv", then
these will now fail to register (with -EPROBE_DEFER) unless
hcd->skip_phy_initialization is set (or we blacklist them as well in the
generic phy code).

I'm not aware of any further examples, but we're sure to find out soon
enough if there are.

Perhaps we should blacklist also "ti,am335x-usb-phy" in the generic phy
code even if hcd->skip_phy_initialization is still needed for musb for
the time being anyway.

Johan

Re: [PATCH 0/3] USB: musb: dsps: phy fix and DT-topology support

[Martin Blumenstingl]

Hello Johan,

On Thu, Apr 19, 2018 at 9:43 AM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, Apr 18, 2018 at 09:18:30PM +0200, Martin Blumenstingl wrote:
>> Hi Johan,
>>
>> On Fri, Apr 13, 2018 at 5:15 PM, Johan Hovold <johan@kernel.org> wrote:
>> > I've been carrying a patch out-of-tree since my work on improving the
>> > USB device-tree support which is needed to be able to describe USB
>> > topologies for musb based controllers.
>> >
>> > This patch, which associates the platform controller device with the
>> > glue device device-tree node, did not play well with the recent changes
>> > which added generic phy support to USB core however.
>> I'm the one who added this
>>
>> > Like the recent dwc2 regression fixed by Arnd after the device-tree
>> > #phy-cell changes, the generic phy code in USB core can now also fail
>> > indefinitly with -EPROBE_DEFER when the controller uses a legacy USB
>> > phy.
>> >
>> > The second patch addresses this for musb, which handles its own (legacy
>> > and generic) phys, but something more may possibly now be needed for
>> > other platforms with legacy phys.
>> I'm not sure if I understand the problem yet - could you please
>> explain with your words what "legacy PHYs" are and how the "conflict"
>> with the PHY handling in USB core?
>>
>> I am aware of two PHY subsystems:
>> - drivers/phy
>> -- also called "generic PHY framework"
>> -- uses a "phys" property
>
> Right, and these are sometimes referred to as generic PHYs, as opposed
> to...
>
>> - drivers/usb/phy
>> -- also called "USB PHY framework"
>> -- AFAIK this should not be used for new drivers
>
> ...the legacy ones.
>
>> -- uses an "usb-phy" property
>
> This is unfortunately not always the case however; some legacy USB phys
> are also referred to using a "phys" property...
oh, I was not aware of that. this explains the issue you're seeing...
thank you for the explanation!

>> the new PHY handling in USB core only parses the "phys" property and
>> thus should not conflict with "usb-phy" (the legacy property)
>
>> however, I probably missed something so I'd appreciate an explanation
>> how things can break
>
> ...and that is the problem. Specifically, since last fall when a number
> of legacy-phy nodes had a #phy-cells property added to them (to silence
> DTC warnings), the generic PHY subsubsystem can now return -EPROBE_DEFER
> indefinitely when looking up a phy as it finds a matching device-tree
> node, but for which there will never be a generic phy registered (since
> it's handled by a legacy-phy driver).
>
> I referred to Arnd's workaround for "usb-nop-xceiv" devices
>
>         b7563e2796f8 ("phy: work around 'phys' references to
>         usb-nop-xceiv devices")
>
> which has some more background on this.
thank you for this pointer too

> So if we have any other host controllers out there using
> "phys"-properties with legacy phys other than "usb-nop-xceiv", then
> these will now fail to register (with -EPROBE_DEFER) unless
> hcd->skip_phy_initialization is set (or we blacklist them as well in the
> generic phy code).
>
> I'm not aware of any further examples, but we're sure to find out soon
> enough if there are.
>
> Perhaps we should blacklist also "ti,am335x-usb-phy" in the generic phy
> code even if hcd->skip_phy_initialization is still needed for musb for
> the time being anyway.
>
> Johan


Regards
Martin