usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping

usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping

[Martin Blumenstingl]

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/

usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping

[Miquel Raynal]

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

usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping

[Martin Blumenstingl]

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

usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping

[Miquel Raynal]

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

usb: host: xhci-plat: Prevent an abnormally restrictive PHY init skipping

[Miquel Raynal]

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

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