Skip to contentSkip to conversation

usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720

PATCH Superseded Pali Rohár 7 participants 16 messages Dec 23, 2020 – Feb 3, 2021

Subsystems (2)
USB USB xHCI driver
lore
Share & tools
lore

Pali Rohár

author
#0 lore raw

PATCH 1/1 usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720

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(+)
drivers/usb/host/xhci-mvebu.c +35 -0
index 60651a50770f..ec4f6d6e44cf 100644
@@ -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;
 }
Signature · 1 line
2.20.1
#1 lore raw
Quoted 69 lines · Pali Rohár

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&amp;data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=vjuJxi9Kg1C7ZHJLB7rsct0kr93JSo4aYkitFubkLao%3D&amp;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&amp;data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2FIULc1sakzNVWjbVPA2TRYZAMv72DGOhmYv4NGijrT8%3D&amp;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&amp;data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2mLHMBc9lgpB4BCrlJYBfO7OJk%2BCi%2Bq3AgpxJxfiCSU%3D&amp;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(+)
drivers/usb/host/xhci-mvebu.c +2 -0
index 60651a50770f..ec4f6d6e44cf 100644
@@ -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;

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 91

Quoted 10 lines
drivers/usb/host/xhci-mvebu.c +6 -0
+	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).

Quoted 17 lines
drivers/usb/host/xhci-mvebu.c +17 -0
+	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;
+	}

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 118

Quoted 10 lines
drivers/usb/host/xhci-mvebu.c +10 -0
+	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?

Signature · 2 lines
Thanks,
Peter Chen
author
#2 lore raw
Quoted 76 lines · On Thursday 24 December 2020 05:54:55 Peter Chen wrote:
Quoted 75 lines · Pali Rohár

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&amp;data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=vjuJxi9Kg1C7ZHJLB7rsct0kr93JSo4aYkitFubkLao%3D&amp;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&amp;data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2FIULc1sakzNVWjbVPA2TRYZAMv72DGOhmYv4NGijrT8%3D&amp;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&amp;data=04%7C01%7Cpeter.chen%40nxp.com%7Ccc158fcd30104268b27008d8a75f32e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637443374600182963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2mLHMBc9lgpB4BCrlJYBfO7OJk%2BCi%2Bq3AgpxJxfiCSU%3D&amp;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(+)
drivers/usb/host/xhci-mvebu.c +4 -0
index 60651a50770f..ec4f6d6e44cf 100644
@@ -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;
Quoted 6 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 91

Quoted 4 lines
drivers/usb/host/xhci-mvebu.c +4 -0
+	/* 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.

Quoted 22 lines
Quoted 22 lines
drivers/usb/host/xhci-mvebu.c +22 -0
+	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:
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 118

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +5 -0
+	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.

#3 lore raw
Quoted 43 lines
Quoted 7 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 91

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +4 -0
+	/* 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.

Quoted 6 lines
Quoted 6 lines
drivers/usb/host/xhci-mvebu.c +6 -0
+	else if (IS_ERR(phy))
+		goto phy_out;
+
+	ret = phy_init(phy);
+	if (ret)
+		goto phy_put;
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)

Quoted 8 lines
drivers/usb/host/xhci-mvebu.c +8 -0
+
+	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 */

Quoted 8 lines
Quoted 8 lines
drivers/usb/host/xhci-mvebu.c +8 -0
+		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:
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 118

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +5 -0
+	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?

Quoted 5 lines

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

author
#4 lore raw
Quoted 43 lines · On Thursday 24 December 2020 13:15:21 Peter Chen wrote:
Quoted 43 lines
Quoted 7 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 91

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +4 -0
+	/* 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.

Quoted 6 lines
Quoted 6 lines
drivers/usb/host/xhci-mvebu.c +6 -0
+	else if (IS_ERR(phy))
+		goto phy_out;
+
+	ret = phy_init(phy);
+	if (ret)
+		goto phy_put;
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)

Quoted 8 lines
drivers/usb/host/xhci-mvebu.c +8 -0
+
+	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 */

Quoted 16 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 118

Quoted 13 lines
drivers/usb/host/xhci-mvebu.c +13 -0
+		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?

Quoted 7 lines
Quoted 5 lines

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.

author
#5 lore raw
Quoted 50 lines · On Thursday 24 December 2020 14:24:01 Pali Rohár wrote:
Quoted 49 lines · On Thursday 24 December 2020 13:15:21 Peter Chen wrote:
Quoted 48 lines
Quoted 7 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 91

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +4 -0
+	/* 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.

Quoted 6 lines
Quoted 6 lines
drivers/usb/host/xhci-mvebu.c +6 -0
+	else if (IS_ERR(phy))
+		goto phy_out;
+
+	ret = phy_init(phy);
+	if (ret)
+		goto phy_put;
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)

Quoted 8 lines
drivers/usb/host/xhci-mvebu.c +8 -0
+
+	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 */

Quoted 8 lines
Quoted 8 lines
drivers/usb/host/xhci-mvebu.c +8 -0
+		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:
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 118

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +5 -0
+	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.

Quoted 4 lines
Quoted 1 line

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.

#6 lore raw
Quoted 17 lines
Quoted 3 lines
Quoted 3 lines

If not supported by ATF, then where to power on and off PHY since no other

place calls PHY APIs? Is it always on?

Quoted 3 lines

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.

Quoted 5 lines

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

#7 lore raw
In reply to Pali Rohár

Hi Pali and Miquel,

Quoted 23 lines · 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) {

drivers/usb/host/xhci-mvebu.c +16 -0
 	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)

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 110

Quoted 10 lines
drivers/usb/host/xhci-mvebu.c +10 -0
+		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:

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	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

author
#8 lore raw

Hello!

Quoted 33 lines · On Monday 28 December 2020 13:11:49 Marek Behun wrote:

Hi Pali and Miquel,

Quoted 28 lines · 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) {

drivers/usb/host/xhci-mvebu.c +21 -0
 	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 */
Quoted 8 lines

drivers/usb/host/xhci-mvebu.c int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) · line 110

Quoted 5 lines
drivers/usb/host/xhci-mvebu.c +5 -0
+		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.

Quoted 7 lines

Moreover the subsequent phy_power_off:

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	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.

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

Miquel, what do you think?

Marek

Tested-by on v1 1/1
#9 lore raw
In reply to Pali Rohár

W dniu 23.12.2020 o 17:24, Pali Rohár pisze:

Quoted 27 lines

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

---

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:

Quoted 1 line

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.

Quoted 74 lines

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(+)
drivers/usb/host/xhci-mvebu.c +35 -0
index 60651a50770f..ec4f6d6e44cf 100644
@@ -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;
 }
Signature · 1 line
TMN
author
#10 lore raw
Quoted 36 lines · On Saturday 30 January 2021 17:31:41 Tomasz Maciej Nowak wrote:

W dniu 23.12.2020 o 17:24, Pali Rohár pisze:

Quoted 27 lines

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

---

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:

Quoted 7 lines

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:

Quoted 1 line

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.

Quoted 84 lines

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.

Quoted 74 lines

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(+)
drivers/usb/host/xhci-mvebu.c +35 -0
index 60651a50770f..ec4f6d6e44cf 100644
@@ -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;
 }
Signature · 1 line
TMN
#11 lore raw

W dniu 30.01.2021 o 17:35, Pali Rohár pisze:

Quoted 41 lines · On Saturday 30 January 2021 17:31:41 Tomasz Maciej Nowak wrote:
Quoted 40 lines

W dniu 23.12.2020 o 17:24, Pali Rohár pisze:

Quoted 27 lines

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

---

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:

Quoted 1 line

https://lore.kernel.org/lkml/[email protected]/T/#u

Quoted 8 lines
Quoted 3 lines

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.

Quoted 85 lines
Quoted 85 lines

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.

Quoted 74 lines

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(+)
drivers/usb/host/xhci-mvebu.c +35 -0
index 60651a50770f..ec4f6d6e44cf 100644
@@ -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;
 }
Signature · 1 line
TMN
Signature · 1 line
TMN
author
#12 lore raw

PATCH v2 1/1 usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720

In reply to Pali Rohár

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(-)
drivers/usb/host/xhci-mvebu.c +42 -0
index 60651a50770f..8ca1a235d164 100644
@@ -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);
drivers/usb/host/xhci-mvebu.h +6 -0
index 3be021793cc8..01bf3fcb3eca 100644
@@ -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;
drivers/usb/host/xhci-plat.c +19 -1
index 4d34f6005381..c1edcc9b13ce 100644
@@ -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))
drivers/usb/host/xhci-plat.h +1 -0
index 1fb149d1fbce..561d0b7bce09 100644
@@ -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 *);
Signature · 1 line
2.20.1
Reviewed-by Tested-by on v2 1/1
#13 lore raw

Hi Pali,

Quoted 35 lines

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.

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

maintainer
#14 lore raw
In reply to Pali Rohár
Quoted 33 lines · Pali Rohár

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.

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

Acked-by
#15 lore raw
Quoted 41 lines · Greg Kroah-Hartman
Quoted 33 lines · Pali Rohár

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.

Quoted 5 lines
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
Quoted 6 lines

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