Re: [PATCH] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720
From: Pali Rohár
Date: Thu Dec 24 2020 - 07:54:47 EST
On Thursday 24 December 2020 05:54:55 Peter Chen wrote:
> On 20-12-23 17:24:03, Pali Rohár wrote:
> > Older ATF does not provide SMC call for USB 3.0 phy power on functionality
> > and therefore initialization of xhci-hcd is failing when older version of
> > ATF is used. In this case phy_power_on() function returns -EOPNOTSUPP.
> >
> > [ 3.108467] mvebu-a3700-comphy d0018300.phy: unsupported SMC call, try updating your firmware
> > [ 3.117250] phy phy-d0018300.phy.0: phy poweron failed --> -95
> > [ 3.123465] xhci-hcd: probe of d0058000.usb failed with error -95
> >
> > This patch calls phy_power_on() in xhci_mvebu_a3700_init_quirk() function
> > and in case it returns -EOPNOTSUPP then XHCI_SKIP_PHY_INIT quirk is set to
> > instruct xhci-plat to skip PHY initialization.
> >
> > This patch fixes above failure by ignoring 'not supported' error in
> > aardvark driver. In this case it is expected that phy is already power on.
> >
> > It fixes initialization of xhci-hcd on Espressobin boards where is older
> > Marvell's Arm Trusted Firmware without SMC call for USB 3.0 phy power.
> >
> > This is regression introduced in commit bd3d25b07342 ("arm64: dts: marvell:
> > armada-37xx: link USB hosts with their PHYs") where USB 3.0 phy was defined
> > and therefore xhci-hcd on Espressobin with older ATF started failing.
> >
> > Fixes: bd3d25b07342 ("arm64: dts: marvell: armada-37xx: link USB hosts with their PHYs")
> > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
> > Cc: <stable@xxxxxxxxxxxxxxx> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
> >
> > ---
> >
> > When applying this patch, please include additional line
> >
> > Cc: <stable@xxxxxxxxxxxxxxx> # 5.1+: <COMMIT_ID>: usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk
> >
> > with correct COMMIT_ID of mentioned patch which is available in the thread:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20201221150903.26630-1-pali%40kernel.org%2FT%2F%23u&data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vjuJxi9Kg1C7ZHJLB7rsct0kr93JSo4aYkitFubkLao%3D&reserved=0
> >
> > As mentioned patch is required for change in this patch to work. Above
> > mentioned patch is prerequisite for this patch and therefore needs to be
> > reviewed and applied prior this patch.
> >
> > Note that same issue as in this USB 3.0 PHY patch was already resolved and
> > applied also for SATA PHY and PCIe PHY on A3720 SOC in following commits:
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3D45aefe3d2251e4e229d7662052739f96ad1d08d9&data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2FIULc1sakzNVWjbVPA2TRYZAMv72DGOhmYv4NGijrT8%3D&reserved=0
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3Db0c6ae0f8948a2be6bf4e8b4bbab9ca1343289b6&data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2mLHMBc9lgpB4BCrlJYBfO7OJk%2BCi%2Bq3AgpxJxfiCSU%3D&reserved=0
> >
> > And these commits were also backported to stable kernel versions (where
> > were affected commits which broke drivers initialization).
> > ---
> > drivers/usb/host/xhci-mvebu.c | 35 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-mvebu.c b/drivers/usb/host/xhci-mvebu.c
> > index 60651a50770f..ec4f6d6e44cf 100644
> > --- a/drivers/usb/host/xhci-mvebu.c
> > +++ b/drivers/usb/host/xhci-mvebu.c
> > @@ -8,6 +8,7 @@
> > #include <linux/mbus.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > +#include <linux/phy/phy.h>
> >
> > #include <linux/usb.h>
> > #include <linux/usb/hcd.h>
> > @@ -77,9 +78,43 @@ int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
> > int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
> > {
> > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > + struct device *dev = hcd->self.controller;
> > + struct phy *phy;
> > + int ret;
> >
> > /* Without reset on resume, the HC won't work at all */
> > xhci->quirks |= XHCI_RESET_ON_RESUME;
> >
> > + /* Old bindings miss the PHY handle */
> > + phy = of_phy_get(dev->of_node, "usb3-phy");
> > + if (IS_ERR(phy) && PTR_ERR(phy) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> Doesn't need to judge IS_ERR(phy).
Ok, I can remove it. I used same condition which is already in SATA and
PCIe phy code.
> > + else if (IS_ERR(phy))
> > + goto phy_out;
> > +
> > + ret = phy_init(phy);
> > + if (ret)
> > + goto phy_put;
> > +
> > + ret = phy_set_mode(phy, PHY_MODE_USB_HOST_SS);
> > + if (ret)
> > + goto phy_exit;
> > +
> > + ret = phy_power_on(phy);
> > + if (ret == -EOPNOTSUPP) {
> > + /* Skip initializatin of XHCI PHY when it is unsupported by firmware */
> > + dev_warn(dev, "PHY unsupported by firmware\n");
> > + xhci->quirks |= XHCI_SKIP_PHY_INIT;
> > + }
> > + if (ret)
> > + goto phy_exit;
> > +
> > + phy_power_off(phy);
> > +phy_exit:
> > + phy_exit(phy);
> > +phy_put:
> > + of_phy_put(phy);
> > +phy_out:
> > +
>
> You do power on and off again only want to know if PHY has already powered at
> ATF, right?
I need to know if power on/off procedure is supported by ATF. And if not
(indicated by -EOPNOTSUPP) then I need to ensure that usb hdc code would
not try to call phy_power_on() as it would cause failure as described in
the commit message. You can look at those other two commits for PCIe and
SATA. Same thing is needed for USB.