USB: musb: dsps: phy fix and DT-topology support

PATCH In review Johan Hovold 4 participants 13 messages Apr 13, 2018 – Apr 19, 2018

Subsystems (2)

USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Johan Hovold

author

2018-04-13 15:15 UTC #1 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/musb/musb_dsps.c | 2 --
 1 file changed, 2 deletions(-)

drivers/usb/musb/musb_dsps.c +0 -2

index 05a679d5e3a2..6a60bc0490c5 100644
@@ -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);

Johan Hovold

author

2018-04-13 15:15 UTC #3 lore raw

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

In reply to 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 <[email protected]>

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/musb/musb_host.c | 1 +
 1 file changed, 1 insertion(+)

drivers/usb/musb/musb_host.c +1 -0

index 3a8451a15f7f..4fa372c845e1 100644
@@ -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)

Johan Hovold

author

2018-04-13 15:15 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/usb/musb/musb_dsps.c | 1 +
 1 file changed, 1 insertion(+)

drivers/usb/musb/musb_dsps.c +1 -0

index 6a60bc0490c5..23dba59045a7 100644
@@ -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;

Johan Hovold

author

2018-04-13 15:17 UTC #0 lore raw

cover USB: musb: dsps: phy fix and DT-topology support

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(-)

Signature · 1 line

2.17.0

Uwe Kleine-König

Acked-by on v1 1/3

2018-04-13 18:45 UTC #2 lore raw

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

In reply to Johan Hovold

Quoted 8 lines · 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.

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 <[email protected]>

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

Best regards
Uwe

Bin Liu

maintainer

2018-04-16 20:03 UTC #5 lore raw

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

In reply to Johan Hovold

Johan,

Quoted 11 lines · 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 <[email protected]>

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.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

author

2018-04-17 07:07 UTC #6 lore raw

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

Quoted 10 lines · Bin Liu

Johan,
Quoted 7 lines · 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.

Quoted 7 lines

Quoted 4 lines
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 <[email protected]>
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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bin Liu

maintainer

2018-04-18 16:20 UTC #7 lore raw

In reply to Johan Hovold

Johan,

Quoted 21 lines · 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.

Quoted 6 lines

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.

Johan Hovold

author

2018-04-18 18:46 UTC #8 lore raw

Quoted 27 lines · Bin Liu

Johan,
Quoted 24 lines · 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

Quoted 7 lines

Quoted 3 lines
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

Bin Liu

maintainer

2018-04-18 18:56 UTC #9 lore raw

Quoted 41 lines · Johan Hovold

Quoted 27 lines · Bin Liu
Johan,
Quoted 24 lines · 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
Quoted 7 lines
Quoted 3 lines
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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Martin Blumenstingl

2018-04-18 19:18 UTC #10 lore raw

In reply to Johan Hovold

Hi Johan,

Quoted 7 lines · 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.

I'm the one who added this

Quoted 8 lines

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

Quoted 12 lines

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

Quoted 7 lines

 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

Johan Hovold

author

2018-04-19 07:43 UTC #11 lore raw

Quoted 20 lines · Martin Blumenstingl

Hi Johan,
Quoted 7 lines · 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.
I'm the one who added this
Quoted 8 lines
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.

Quoted 8 lines

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...

Quoted 3 lines

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

...the legacy ones.

Quoted 1 line

-- 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...

Quoted 5 lines

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

Martin Blumenstingl

2018-04-19 21:54 UTC #12 lore raw

Hello Johan,

Quoted 37 lines · Johan Hovold

Quoted 20 lines · Martin Blumenstingl
Hi Johan,
Quoted 7 lines · 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.
I'm the one who added this
Quoted 8 lines
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.
Quoted 8 lines
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...
Quoted 3 lines
- drivers/usb/phy
-- also called "USB PHY framework"
-- AFAIK this should not be used for new drivers

Quoted 6 lines

...the legacy ones.
Quoted 1 line
-- 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!

Quoted 13 lines

Quoted 5 lines
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).

Quoted 6 lines

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

Quoted 6 lines

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).

Quoted 8 lines

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