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 <[email protected]>
Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
---
When applying this patch, please include additional line
Cc: <[email protected]> # 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://lore.kernel.org/lkml/[email protected]/T/#u
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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=45aefe3d2251e4e229d7662052739f96ad1d08d9
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c6ae0f8948a2be6bf4e8b4bbab9ca1343289b6
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;
+ 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:
+
return 0;
}
--
2.20.1
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 <[email protected]>
> Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
> Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
>
> ---
>
> When applying this patch, please include additional line
>
> Cc: <[email protected]> # 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).
> + 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?
--
Thanks,
Peter Chen
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 <[email protected]>
> > Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
> > Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
> >
> > ---
> >
> > When applying this patch, please include additional line
> >
> > Cc: <[email protected]> # 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.
> > > + /* 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.
If not supported by ATF, then where to power on and off PHY since no other place calls PHY APIs? Is it always on?
Peter
On Thursday 24 December 2020 13:15:21 Peter Chen wrote:
>
> > > > + /* 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.
>
> If not supported by ATF, then where to power on and off PHY since no other place calls PHY APIs? Is it always on?
Yes, in this case (when -EOPNOTSUPP is returned) SMC API is not
supported by ATF, and PHY is always on.
On Thursday 24 December 2020 14:24:01 Pali Rohár wrote:
> On Thursday 24 December 2020 13:15:21 Peter Chen wrote:
> >
> > > > > + /* 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.
> >
> > If not supported by ATF, then where to power on and off PHY since no other place calls PHY APIs? Is it always on?
>
> Yes, in this case (when -EOPNOTSUPP is returned) SMC API is not
> supported by ATF, and PHY is always on.
To make it clear, core/hcd.c function usb_add_hcd() when
hcd->skip_phy_initialization is false is calling
usb_phy_roothub_power_on() which calls phy_power_on(). If this call
fails then error is propagated back to the usb_add_hcd() and this
function fails too.
But on boards with older ATF (which do not support PHY power on SMC API)
is phy_power_on() returning error -EOPNOTSUPP and therefore whole USB
3.0 initialization fails.
This patch is adding init hook to detect if ATF supports PHY power
on/off functions and in case it does not support it, code sets
XHCI_SKIP_PHY_INIT flag to instruct xhci-plat code to set
hcd->skip_phy_initialization flag to instruct core/hcd.c to skip calling
usb_phy_roothub_power_on() function as it is know that it would fail.
> > >
> > > If not supported by ATF, then where to power on and off PHY since no other
> place calls PHY APIs? Is it always on?
> >
> > Yes, in this case (when -EOPNOTSUPP is returned) SMC API is not
> > supported by ATF, and PHY is always on.
>
> To make it clear, core/hcd.c function usb_add_hcd() when
> hcd->skip_phy_initialization is false is calling
> usb_phy_roothub_power_on() which calls phy_power_on(). If this call fails then
> error is propagated back to the usb_add_hcd() and this function fails too.
>
> But on boards with older ATF (which do not support PHY power on SMC API) is
> phy_power_on() returning error -EOPNOTSUPP and therefore whole USB
> 3.0 initialization fails.
>
> This patch is adding init hook to detect if ATF supports PHY power on/off
> functions and in case it does not support it, code sets XHCI_SKIP_PHY_INIT flag
> to instruct xhci-plat code to set
> hcd->skip_phy_initialization flag to instruct core/hcd.c to skip calling
> usb_phy_roothub_power_on() function as it is know that it would fail.
Thanks for clarity, clear now.
Peter
Hi Pali and Miquel,
On Wed, 23 Dec 2020 17:24:03 +0100
Pali Rohár <[email protected]> wrote:
> 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;
> + 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;
I am not sure if this is the correct way to check whether PHY_INIT
should be skipped.
Moreover the subsequent phy_power_off:
> +
> + phy_power_off(phy);
won't power off the PHY, because the corresponding handler in ATF is
currently empty.
I guess the patch needs to be in kernel if users are unwilling to upgrade
ATF firmware.
The SMC calls for Marvell's comphy are designed to be generic for
several Marvell platforms (the constants are the same and so one), but
we still have different drivers for them anyway.
Maybe it would be better to just not use the ATF implementation at all,
and implement the comphy driver for A3720 entirely in kernel...
Miquel, what do you think?
Marek
Hello!
On Monday 28 December 2020 13:11:49 Marek Behun wrote:
> Hi Pali and Miquel,
>
> On Wed, 23 Dec 2020 17:24:03 +0100
> Pali Rohár <[email protected]> wrote:
>
> > 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;
> > + 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;
>
> I am not sure if this is the correct way to check whether PHY_INIT
> should be skipped.
I do not have a better idea how to fix described issue for Armada 3720
boards without touching other usb/core/hdc code. I wanted to put these
Armada 3720 changes only into xhci-mvebu/xhci_mvebu_a3700_init_quirk
code so it does not "pollute" other usb generic code.
> Moreover the subsequent phy_power_off:
>
> > +
> > + phy_power_off(phy);
>
> won't power off the PHY, because the corresponding handler in ATF is
> currently empty.
This is not an issue as the usb core will later power on PHY during
initialization. So I can remove this line, it has no effect on
functionality.
> I guess the patch needs to be in kernel if users are unwilling to upgrade
> ATF firmware.
Yes. If distributions which are running stable kernels (4.14, 4.19)
start updating their kernels to new stable versions (5.4+) then this
upgrade can break support for USB. Similarly for SATA and PCIe (already
fixed and backported to stable kernels). This is relevant e.g. to Debian
which stable version is on 4.19 and therefore is not affected by this
issue yet.
So my opinion is that such thing is a regression if kernel starts
depending on a new firmware version and cause malfunctions of some
subsystems.
Updating kernel on Espressobin or Turris MOX (both A3720) when using
Debian, OpenWRT (or any similar distribution) is relatively easy as
kernel is stored on SD card on rootfs filesystem. Package manager will
update it easily and can do it automatically (without user interation).
But updating ATF firmware is harder as it is stored in nand and on
Espressobin it is part of another M3 secure firmware image. I do not
know any distribution which can do it automatically, it needs to be done
by user.
> The SMC calls for Marvell's comphy are designed to be generic for
> several Marvell platforms (the constants are the same and so one), but
> we still have different drivers for them anyway.
>
> Maybe it would be better to just not use the ATF implementation at all,
> and implement the comphy driver for A3720 entirely in kernel...
Globalscaletechnolgies already implemented something in their kernel:
https://github.com/globalscaletechnologies/linux/commit/86fa2470c3a867ca9a78e5521a7af037617f5b3b
I do not like too an idea to depends on external RPC API in kernel.
I think that it is better to have native driver in linux kernel instead
of current RPC driver. Bugs in our kernel drivers can be fixed and
backported to stable kernels. But bugs in firmware we cannot fix and we
have to deal with them (in case we are using it).
I think that in past Linus said that Linux kernel does not use nor
depend on x86 BIOS routines/interrupts for similar reasons.
> Miquel, what do you think?
>
> Marek
On Saturday 30 January 2021 17:31:41 Tomasz Maciej Nowak wrote:
> W dniu 23.12.2020 o 17:24, Pali Rohár pisze:
> > 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 <[email protected]>
> > Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
> > Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
> >
> > ---
> >
> > When applying this patch, please include additional line
> >
> > Cc: <[email protected]> # 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:
>
> Hi,
> and sorry for late reply, but that might be good reminder for maintainers.
> Anyway I tested this patch in conjunction with v2 from this topic:
> > https://lore.kernel.org/lkml/[email protected]/T/#u
> The USB ports work flawlessly with older ATF, so:
>
> Tested-by: Tomasz Maciej Nowak <[email protected]>
Thank you for testing. But as was mentioned in discussion in above
patch, I have to rework both patches. I have new changes prepared but
have not had a chance to test new changes yet. If you want I can send
you a new version of those untested patches.
> At OpenWrt we are using patch which removes Comphy assignments from nodes
> in dts, but that is sub optimal, since that "discriminates" users with
> updated ATF. I would prefer this patch instead of what we are doing now
> in OpenWrt.
>
> Thanks.
>
> >
> > 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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=45aefe3d2251e4e229d7662052739f96ad1d08d9
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c6ae0f8948a2be6bf4e8b4bbab9ca1343289b6
> >
> > 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;
> > + 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:
> > +
> > return 0;
> > }
> >
>
> --
> TMN
W dniu 30.01.2021 o 17:35, Pali Rohár pisze:
> On Saturday 30 January 2021 17:31:41 Tomasz Maciej Nowak wrote:
>> W dniu 23.12.2020 o 17:24, Pali Rohár pisze:
>>> 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 <[email protected]>
>>> Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
>>> Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
>>>
>>> ---
>>>
>>> When applying this patch, please include additional line
>>>
>>> Cc: <[email protected]> # 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:
>>
>> Hi,
>> and sorry for late reply, but that might be good reminder for maintainers.
>> Anyway I tested this patch in conjunction with v2 from this topic:
>>> https://lore.kernel.org/lkml/[email protected]/T/#u
>> The USB ports work flawlessly with older ATF, so:
>>
>> Tested-by: Tomasz Maciej Nowak <[email protected]>
>
> Thank you for testing. But as was mentioned in discussion in above
> patch, I have to rework both patches. I have new changes prepared but
> have not had a chance to test new changes yet. If you want I can send
> you a new version of those untested patches.
Yes, will gladly test them.
>
>> At OpenWrt we are using patch which removes Comphy assignments from nodes
>> in dts, but that is sub optimal, since that "discriminates" users with
>> updated ATF. I would prefer this patch instead of what we are doing now
>> in OpenWrt.
>>
>> Thanks.
>>
>>>
>>> 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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=45aefe3d2251e4e229d7662052739f96ad1d08d9
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c6ae0f8948a2be6bf4e8b4bbab9ca1343289b6
>>>
>>> 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;
>>> + 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:
>>> +
>>> return 0;
>>> }
>>>
>>
>> --
>> TMN
--
TMN
W dniu 23.12.2020 o 17:24, Pali Rohár pisze:
> 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 <[email protected]>
> Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
> Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
>
> ---
>
> When applying this patch, please include additional line
>
> Cc: <[email protected]> # 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:
Hi,
and sorry for late reply, but that might be good reminder for maintainers.
Anyway I tested this patch in conjunction with v2 from this topic:
> https://lore.kernel.org/lkml/[email protected]/T/#u
The USB ports work flawlessly with older ATF, so:
Tested-by: Tomasz Maciej Nowak <[email protected]>
At OpenWrt we are using patch which removes Comphy assignments from nodes
in dts, but that is sub optimal, since that "discriminates" users with
updated ATF. I would prefer this patch instead of what we are doing now
in OpenWrt.
Thanks.
>
> 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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=45aefe3d2251e4e229d7662052739f96ad1d08d9
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c6ae0f8948a2be6bf4e8b4bbab9ca1343289b6
>
> 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;
> + 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:
> +
> return 0;
> }
>
--
TMN
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 introduces a new plat_setup callback for xhci platform drivers
which is called prior calling usb_add_hcd() function. This function at its
beginning skips PHY init if hcd->skip_phy_initialization is set.
Current init_quirk callback for xhci platform drivers is called from
xhci_plat_setup() function which is called after chip reset completes.
It happens in the middle of the usb_add_hcd() function and therefore this
callback cannot be used for setting if PHY init should be skipped or not.
For Armada 3720 this patch introduce a new xhci_mvebu_a3700_plat_setup()
function configured as a xhci platform plat_setup callback. This new
function calls phy_power_on() 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
xhci-hcd 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.
Signed-off-by: Pali Rohár <[email protected]>
Tested-by: Tomasz Maciej Nowak <[email protected]>
Fixes: bd3d25b07342 ("arm64: dts: marvell: armada-37xx: link USB hosts with their PHYs")
Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
---
Changes in v2:
* Drop dependency on patch "usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk"
* Introduce a new .plat_setup callback for xhci-plat drivers in this patch (to simplify backporting and review)
* Move implementation from xhci_mvebu_mbus_init_quirk() into xhci_mvebu_a3700_plat_setup()
---
drivers/usb/host/xhci-mvebu.c | 42 +++++++++++++++++++++++++++++++++++
drivers/usb/host/xhci-mvebu.h | 6 +++++
drivers/usb/host/xhci-plat.c | 20 ++++++++++++++++-
drivers/usb/host/xhci-plat.h | 1 +
4 files changed, 68 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-mvebu.c b/drivers/usb/host/xhci-mvebu.c
index 60651a50770f..8ca1a235d164 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>
@@ -74,6 +75,47 @@ int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
return 0;
}
+int xhci_mvebu_a3700_plat_setup(struct usb_hcd *hcd)
+{
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct device *dev = hcd->self.controller;
+ struct phy *phy;
+ int ret;
+
+ /* 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;
+ 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:
+
+ return 0;
+}
+
int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
diff --git a/drivers/usb/host/xhci-mvebu.h b/drivers/usb/host/xhci-mvebu.h
index 3be021793cc8..01bf3fcb3eca 100644
--- a/drivers/usb/host/xhci-mvebu.h
+++ b/drivers/usb/host/xhci-mvebu.h
@@ -12,6 +12,7 @@ struct usb_hcd;
#if IS_ENABLED(CONFIG_USB_XHCI_MVEBU)
int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd);
+int xhci_mvebu_a3700_plat_setup(struct usb_hcd *hcd);
int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd);
#else
static inline int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
@@ -19,6 +20,11 @@ static inline int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
return 0;
}
+static inline int xhci_mvebu_a3700_plat_setup(struct usb_hcd *hcd)
+{
+ return 0;
+}
+
static inline int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
{
return 0;
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 4d34f6005381..c1edcc9b13ce 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -44,6 +44,16 @@ static void xhci_priv_plat_start(struct usb_hcd *hcd)
priv->plat_start(hcd);
}
+static int xhci_priv_plat_setup(struct usb_hcd *hcd)
+{
+ struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+ if (!priv->plat_setup)
+ return 0;
+
+ return priv->plat_setup(hcd);
+}
+
static int xhci_priv_init_quirk(struct usb_hcd *hcd)
{
struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
@@ -111,6 +121,7 @@ static const struct xhci_plat_priv xhci_plat_marvell_armada = {
};
static const struct xhci_plat_priv xhci_plat_marvell_armada3700 = {
+ .plat_setup = xhci_mvebu_a3700_plat_setup,
.init_quirk = xhci_mvebu_a3700_init_quirk,
};
@@ -330,7 +341,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
xhci->shared_hcd->tpl_support = hcd->tpl_support;
- if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
+
+ if (priv) {
+ ret = xhci_priv_plat_setup(hcd);
+ if (ret)
+ goto disable_usb_phy;
+ }
+
+ if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
hcd->skip_phy_initialization = 1;
if (priv && (priv->quirks & XHCI_SG_TRB_CACHE_SIZE_QUIRK))
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 1fb149d1fbce..561d0b7bce09 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -13,6 +13,7 @@
struct xhci_plat_priv {
const char *firmware_name;
unsigned long long quirks;
+ int (*plat_setup)(struct usb_hcd *);
void (*plat_start)(struct usb_hcd *);
int (*init_quirk)(struct usb_hcd *);
int (*suspend_quirk)(struct usb_hcd *);
--
2.20.1
Hi Pali,
> From: Pali Rohár, Sent: Tuesday, February 2, 2021 12:08 AM
>
> 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 introduces a new plat_setup callback for xhci platform drivers
> which is called prior calling usb_add_hcd() function. This function at its
> beginning skips PHY init if hcd->skip_phy_initialization is set.
>
> Current init_quirk callback for xhci platform drivers is called from
> xhci_plat_setup() function which is called after chip reset completes.
> It happens in the middle of the usb_add_hcd() function and therefore this
> callback cannot be used for setting if PHY init should be skipped or not.
>
> For Armada 3720 this patch introduce a new xhci_mvebu_a3700_plat_setup()
> function configured as a xhci platform plat_setup callback. This new
> function calls phy_power_on() 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
> xhci-hcd 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.
>
> Signed-off-by: Pali Rohár <[email protected]>
> Tested-by: Tomasz Maciej Nowak <[email protected]>
> Fixes: bd3d25b07342 ("arm64: dts: marvell: armada-37xx: link USB hosts with their PHYs")
> Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to
> errno
> Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
Thank you for the patch!
Reviewed-by: Yoshihiro Shimoda <[email protected]> # xhci-plat
Also, I tested on my environment and didn't cause any regression.
So,
Tested-by: Yoshihiro Shimoda <[email protected]> # On R-Car
Best regards,
Yoshihiro Shimoda
On Mon, Feb 01, 2021 at 04:08:03PM +0100, 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 introduces a new plat_setup callback for xhci platform drivers
> which is called prior calling usb_add_hcd() function. This function at its
> beginning skips PHY init if hcd->skip_phy_initialization is set.
>
> Current init_quirk callback for xhci platform drivers is called from
> xhci_plat_setup() function which is called after chip reset completes.
> It happens in the middle of the usb_add_hcd() function and therefore this
> callback cannot be used for setting if PHY init should be skipped or not.
>
> For Armada 3720 this patch introduce a new xhci_mvebu_a3700_plat_setup()
> function configured as a xhci platform plat_setup callback. This new
> function calls phy_power_on() 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
> xhci-hcd 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.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> Tested-by: Tomasz Maciej Nowak <[email protected]>
> Fixes: bd3d25b07342 ("arm64: dts: marvell: armada-37xx: link USB hosts with their PHYs")
> Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
> Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
Mathias, any objection for me taking this now to get into 5.11-final?
thanks,
greg k-h
On 2.2.2021 20.41, Greg Kroah-Hartman wrote:
> On Mon, Feb 01, 2021 at 04:08:03PM +0100, 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 introduces a new plat_setup callback for xhci platform drivers
>> which is called prior calling usb_add_hcd() function. This function at its
>> beginning skips PHY init if hcd->skip_phy_initialization is set.
>>
>> Current init_quirk callback for xhci platform drivers is called from
>> xhci_plat_setup() function which is called after chip reset completes.
>> It happens in the middle of the usb_add_hcd() function and therefore this
>> callback cannot be used for setting if PHY init should be skipped or not.
>>
>> For Armada 3720 this patch introduce a new xhci_mvebu_a3700_plat_setup()
>> function configured as a xhci platform plat_setup callback. This new
>> function calls phy_power_on() 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
>> xhci-hcd 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.
>>
>> Signed-off-by: Pali Rohár <[email protected]>
>> Tested-by: Tomasz Maciej Nowak <[email protected]>
>> Fixes: bd3d25b07342 ("arm64: dts: marvell: armada-37xx: link USB hosts with their PHYs")
>> Cc: <[email protected]> # 5.1+: ea17a0f153af: phy: marvell: comphy: Convert internal SMCC firmware return codes to errno
>> Cc: <[email protected]> # 5.1+: f768e718911e: usb: host: xhci-plat: add priv quirk for skip PHY initialization
>
>
> Mathias, any objection for me taking this now to get into 5.11-final?
>
> thanks,
>
> greg k-h
>
No objections, looks good to me.
Acked-by: Mathias Nyman <[email protected]>