| Message ID | 20190326083858.13241-1-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | eb6c2eb6c7fbd24b51fba2fa4b270c839b8b8349 |
| Headers | show |
| Series | usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping | expand |
Hello Miquel, On Tue, Mar 26, 2019 at 9:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > In the past, USB PHY handling has been moved in the HCD core. Some > host controller drivers needing more control of the PHYs, they have > been granted the freedom to handle themselves the PHY states and to > prevent the HCD core to do so in commit 4e88d4c08301 ("usb: add a flag > to skip PHY initialization to struct usb_hcd"). With this change, any > USB host controller could set the hcd->skip_phy_initialization flag so > that the HCD core would just skip the PHY initialization sequence. nit-pick: strictly speaking host controller drivers were able to skip the core's PHY initialization sequence by setting hcd->phy or hcd->usb_phy. My commit just made it easier to understand (at least for me) what's going on > However, in the USB subsystem, there are currently two entirely > different forms of PHY: one is called 'usb_phy' and is > USB-subsystem-wide, while there is also the generic and kernel-wide > 'phy' from the (recent) generic PHY framework. > > When the commit above was introduced, both type of PHYs where handled > by the HCD core. > > Later, commit bc40f5341741 ("USB: core: hcd: drop support for legacy > phys") removed the support for the former type of PHYs in the HCD > core. These 'usb_phy' are still present though, but managed from the > controller drivers only. Hence, setting the > hcd->skip_phy_initialization flag just because a 'usb_phy' is > initialized by a controller driver is a non-sense. > > For instance on Armada CP110, a 'usb_phy' is there to enable the power > supply to the USB host, while there is also a COMPHY block providing > SERDES lanes configuration that is referenced as a PHY from the common > PHY framework. arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts (using this as an example, because it's what I found first) could be changed to use the phy-supply property of the (recent) generic PHY framework. This is documented here: Documentation/devicetree/bindings/phy/phy-bindings.txt as far as I understand both, generic PHY's phy-supply and usb-nop-xceiv's vcc-supply are *always* enabling the supply when the PHY is enabled. for an OTG capable USB controller this may not be correct, because VBUS should only be provided in "host" mode, but not in "peripheral" mode. dwc2 has a special vbus-supply property for this (documentation is currently not reflecting this, but I sent a patch to fix it: [0]) I'm aware that this has nothing to do with your patch, I just wanted to let you know in case you didn't know about it yet (so you can judge for yourself whether another change somewhere is appropriate) > Right now, users of the xhci-plat.c driver either use a 'usb_phy' only > and do not care about the attempt of generic PHY initialization within > the HCD core (as there is none); or they use a single 'phy' and the > code flow does not pass through the block setting > hcd->skip_phy_initialization anyway. > > While there is not users of both PHY types at the same time, drop this > limitation from the xhci-plat.c driver. Note that the tegra driver > probably has the same limitation and could definitely benefit from a > similar change. > > Cc: Johan Hovold <johan@kernel.org> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> thank you very much for the patch and the detailed explanation! Martin [0] https://patchwork.kernel.org/patch/10841803/
Hi Martin, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Tue, 26 Mar 2019 18:29:25 +0100: > Hello Miquel, > > On Tue, Mar 26, 2019 at 9:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > In the past, USB PHY handling has been moved in the HCD core. Some > > host controller drivers needing more control of the PHYs, they have > > been granted the freedom to handle themselves the PHY states and to > > prevent the HCD core to do so in commit 4e88d4c08301 ("usb: add a flag > > to skip PHY initialization to struct usb_hcd"). With this change, any > > USB host controller could set the hcd->skip_phy_initialization flag so > > that the HCD core would just skip the PHY initialization sequence. > nit-pick: strictly speaking host controller drivers were able to skip > the core's PHY initialization sequence by setting hcd->phy or > hcd->usb_phy. Indeed! > My commit just made it easier to understand (at least > for me) what's going on Actually it also had the effect to merge the two conditions (having set either hcd->phy or hcd->usb_phy) in one bit of information which, IMHO, had an impact thereafter. > > > However, in the USB subsystem, there are currently two entirely > > different forms of PHY: one is called 'usb_phy' and is > > USB-subsystem-wide, while there is also the generic and kernel-wide > > 'phy' from the (recent) generic PHY framework. > > > > When the commit above was introduced, both type of PHYs where handled > > by the HCD core. > > > > Later, commit bc40f5341741 ("USB: core: hcd: drop support for legacy > > phys") removed the support for the former type of PHYs in the HCD > > core. These 'usb_phy' are still present though, but managed from the > > controller drivers only. Hence, setting the > > hcd->skip_phy_initialization flag just because a 'usb_phy' is > > initialized by a controller driver is a non-sense. > > > > For instance on Armada CP110, a 'usb_phy' is there to enable the power > > supply to the USB host, while there is also a COMPHY block providing > > SERDES lanes configuration that is referenced as a PHY from the common > > PHY framework. > arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts (using this > as an example, because it's what I found first) could be changed to > use the phy-supply property of the (recent) generic PHY framework. > This is documented here: > Documentation/devicetree/bindings/phy/phy-bindings.txt > > as far as I understand both, generic PHY's phy-supply and > usb-nop-xceiv's vcc-supply are *always* enabling the supply when the > PHY is enabled. > for an OTG capable USB controller this may not be correct, because > VBUS should only be provided in "host" mode, but not in "peripheral" > mode. > dwc2 has a special vbus-supply property for this (documentation is > currently not reflecting this, but I sent a patch to fix it: [0]) > > I'm aware that this has nothing to do with your patch, I just wanted > to let you know in case you didn't know about it yet (so you can judge > for yourself whether another change somewhere is appropriate) Actually this is really interesting! Thanks for sharing Rob's answer to your thread. I think this patch still has a meaning but in the mean time I will convert the usb-phy property to the common PHY framework using (as Rob told you) a connector and a phy-supply attached to it. > > > Right now, users of the xhci-plat.c driver either use a 'usb_phy' only > > and do not care about the attempt of generic PHY initialization within > > the HCD core (as there is none); or they use a single 'phy' and the > > code flow does not pass through the block setting > > hcd->skip_phy_initialization anyway. > > > > While there is not users of both PHY types at the same time, drop this > > limitation from the xhci-plat.c driver. Note that the tegra driver > > probably has the same limitation and could definitely benefit from a > > similar change. > > > > Cc: Johan Hovold <johan@kernel.org> > > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > thank you very much for the patch and the detailed explanation! > > > Martin > > [0] https://patchwork.kernel.org/patch/10841803/ Thanks, Miquèl
Hi Miquel, On Fri, Mar 29, 2019 at 4:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Martin, > > Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Tue, > 26 Mar 2019 18:29:25 +0100: > > > Hello Miquel, > > > > On Tue, Mar 26, 2019 at 9:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > In the past, USB PHY handling has been moved in the HCD core. Some > > > host controller drivers needing more control of the PHYs, they have > > > been granted the freedom to handle themselves the PHY states and to > > > prevent the HCD core to do so in commit 4e88d4c08301 ("usb: add a flag > > > to skip PHY initialization to struct usb_hcd"). With this change, any > > > USB host controller could set the hcd->skip_phy_initialization flag so > > > that the HCD core would just skip the PHY initialization sequence. > > nit-pick: strictly speaking host controller drivers were able to skip > > the core's PHY initialization sequence by setting hcd->phy or > > hcd->usb_phy. > > Indeed! > > > My commit just made it easier to understand (at least > > for me) what's going on > > Actually it also had the effect to merge the two conditions > (having set either hcd->phy or hcd->usb_phy) in one bit of information > which, IMHO, had an impact thereafter. excellent catch - I haven't noticed that before > > > > > However, in the USB subsystem, there are currently two entirely > > > different forms of PHY: one is called 'usb_phy' and is > > > USB-subsystem-wide, while there is also the generic and kernel-wide > > > 'phy' from the (recent) generic PHY framework. > > > > > > When the commit above was introduced, both type of PHYs where handled > > > by the HCD core. > > > > > > Later, commit bc40f5341741 ("USB: core: hcd: drop support for legacy > > > phys") removed the support for the former type of PHYs in the HCD > > > core. These 'usb_phy' are still present though, but managed from the > > > controller drivers only. Hence, setting the > > > hcd->skip_phy_initialization flag just because a 'usb_phy' is > > > initialized by a controller driver is a non-sense. > > > > > > For instance on Armada CP110, a 'usb_phy' is there to enable the power > > > supply to the USB host, while there is also a COMPHY block providing > > > SERDES lanes configuration that is referenced as a PHY from the common > > > PHY framework. > > arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts (using this > > as an example, because it's what I found first) could be changed to > > use the phy-supply property of the (recent) generic PHY framework. > > This is documented here: > > Documentation/devicetree/bindings/phy/phy-bindings.txt > > > > as far as I understand both, generic PHY's phy-supply and > > usb-nop-xceiv's vcc-supply are *always* enabling the supply when the > > PHY is enabled. > > for an OTG capable USB controller this may not be correct, because > > VBUS should only be provided in "host" mode, but not in "peripheral" > > mode. > > dwc2 has a special vbus-supply property for this (documentation is > > currently not reflecting this, but I sent a patch to fix it: [0]) > > > > I'm aware that this has nothing to do with your patch, I just wanted > > to let you know in case you didn't know about it yet (so you can judge > > for yourself whether another change somewhere is appropriate) > > Actually this is really interesting! Thanks for sharing Rob's answer to > your thread. I think this patch still has a meaning but in the mean > time I will convert the usb-phy property to the common PHY framework > using (as Rob told you) a connector and a phy-supply attached to it. that seems like a good plan. the current patch should still be applied, shouldn't it? also can you please CC me on the connector patches (whenever they are ready, I want to see whether we have to update the Amlogic platforms as well) Regards Martin
Hi Martin, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Sat, 30 Mar 2019 13:53:29 +0100: > Hi Miquel, > > On Fri, Mar 29, 2019 at 4:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Martin, > > > > Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Tue, > > 26 Mar 2019 18:29:25 +0100: > > > > > Hello Miquel, > > > > > > On Tue, Mar 26, 2019 at 9:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > In the past, USB PHY handling has been moved in the HCD core. Some > > > > host controller drivers needing more control of the PHYs, they have > > > > been granted the freedom to handle themselves the PHY states and to > > > > prevent the HCD core to do so in commit 4e88d4c08301 ("usb: add a flag > > > > to skip PHY initialization to struct usb_hcd"). With this change, any > > > > USB host controller could set the hcd->skip_phy_initialization flag so > > > > that the HCD core would just skip the PHY initialization sequence. > > > nit-pick: strictly speaking host controller drivers were able to skip > > > the core's PHY initialization sequence by setting hcd->phy or > > > hcd->usb_phy. > > > > Indeed! > > > > > My commit just made it easier to understand (at least > > > for me) what's going on > > > > Actually it also had the effect to merge the two conditions > > (having set either hcd->phy or hcd->usb_phy) in one bit of information > > which, IMHO, had an impact thereafter. > excellent catch - I haven't noticed that before > > > > > > > > However, in the USB subsystem, there are currently two entirely > > > > different forms of PHY: one is called 'usb_phy' and is > > > > USB-subsystem-wide, while there is also the generic and kernel-wide > > > > 'phy' from the (recent) generic PHY framework. > > > > > > > > When the commit above was introduced, both type of PHYs where handled > > > > by the HCD core. > > > > > > > > Later, commit bc40f5341741 ("USB: core: hcd: drop support for legacy > > > > phys") removed the support for the former type of PHYs in the HCD > > > > core. These 'usb_phy' are still present though, but managed from the > > > > controller drivers only. Hence, setting the > > > > hcd->skip_phy_initialization flag just because a 'usb_phy' is > > > > initialized by a controller driver is a non-sense. > > > > > > > > For instance on Armada CP110, a 'usb_phy' is there to enable the power > > > > supply to the USB host, while there is also a COMPHY block providing > > > > SERDES lanes configuration that is referenced as a PHY from the common > > > > PHY framework. > > > arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts (using this > > > as an example, because it's what I found first) could be changed to > > > use the phy-supply property of the (recent) generic PHY framework. > > > This is documented here: > > > Documentation/devicetree/bindings/phy/phy-bindings.txt > > > > > > as far as I understand both, generic PHY's phy-supply and > > > usb-nop-xceiv's vcc-supply are *always* enabling the supply when the > > > PHY is enabled. > > > for an OTG capable USB controller this may not be correct, because > > > VBUS should only be provided in "host" mode, but not in "peripheral" > > > mode. > > > dwc2 has a special vbus-supply property for this (documentation is > > > currently not reflecting this, but I sent a patch to fix it: [0]) > > > > > > I'm aware that this has nothing to do with your patch, I just wanted > > > to let you know in case you didn't know about it yet (so you can judge > > > for yourself whether another change somewhere is appropriate) > > > > Actually this is really interesting! Thanks for sharing Rob's answer to > > your thread. I think this patch still has a meaning but in the mean > > time I will convert the usb-phy property to the common PHY framework > > using (as Rob told you) a connector and a phy-supply attached to it. > that seems like a good plan. the current patch should still be > applied, shouldn't it? Yes indeed. > also can you please CC me on the connector patches (whenever they are > ready, I want to see whether we have to update the Amlogic platforms > as well) Sure, I'll Cc: you. Regards, Miquèl
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 0ac4ec975547..305b7a4231f6 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -310,7 +310,6 @@ static int xhci_plat_probe(struct platform_device *pdev) ret = usb_phy_init(hcd->usb_phy); if (ret) goto put_usb3_hcd; - hcd->skip_phy_initialization = 1; } hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
In the past, USB PHY handling has been moved in the HCD core. Some host controller drivers needing more control of the PHYs, they have been granted the freedom to handle themselves the PHY states and to prevent the HCD core to do so in commit 4e88d4c08301 ("usb: add a flag to skip PHY initialization to struct usb_hcd"). With this change, any USB host controller could set the hcd->skip_phy_initialization flag so that the HCD core would just skip the PHY initialization sequence. However, in the USB subsystem, there are currently two entirely different forms of PHY: one is called 'usb_phy' and is USB-subsystem-wide, while there is also the generic and kernel-wide 'phy' from the (recent) generic PHY framework. When the commit above was introduced, both type of PHYs where handled by the HCD core. Later, commit bc40f5341741 ("USB: core: hcd: drop support for legacy phys") removed the support for the former type of PHYs in the HCD core. These 'usb_phy' are still present though, but managed from the controller drivers only. Hence, setting the hcd->skip_phy_initialization flag just because a 'usb_phy' is initialized by a controller driver is a non-sense. For instance on Armada CP110, a 'usb_phy' is there to enable the power supply to the USB host, while there is also a COMPHY block providing SERDES lanes configuration that is referenced as a PHY from the common PHY framework. Right now, users of the xhci-plat.c driver either use a 'usb_phy' only and do not care about the attempt of generic PHY initialization within the HCD core (as there is none); or they use a single 'phy' and the code flow does not pass through the block setting hcd->skip_phy_initialization anyway. While there is not users of both PHY types at the same time, drop this limitation from the xhci-plat.c driver. Note that the tegra driver probably has the same limitation and could definitely benefit from a similar change. Cc: Johan Hovold <johan@kernel.org> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/usb/host/xhci-plat.c | 1 - 1 file changed, 1 deletion(-)