usb/core/phy fixes for v4.17

This is a follow-up to my previous series "initialize (multiple) PHYs for a HCD": [0].

Roger Quadros reported [1] that it "is breaking low power cases on TI SoCs when USB is in host mode". He further explains that "Not doing the phy_exit() here [when entering suspend] leaves the clocks enabled on our SoC and we're no longer able to reach low power states on system suspend."
Chunfeng Yun from Mediatek noted [2] that we cannot unconditionally call phy_exit while entering system suspend, because this would "disconnect plugged devices on MTK platforms, due to re-initialize u2 phys when resume"

In the discussion (which followed Roger's bug report: [1]) Roger, Chunfeng and me came to the conclusion that we can fix suspend on the TI SoCs without breaking it on the Mediatek SoCs by extending the suspend and resume code in usb/core/phy.c by checking whether the USB controller can wake up the system (which is the case for the Mediatek MTU3 controller, but now for the dwc3 controller used on the TI SoCs):
- if the controller can wake up the system (Mediatek MTU3 use-case) we only call usb_phy_roothub_power_off (which calls phy_power_off) when entering system suspend
- if the controller however cannot wake up the system (dwc3 on TI SoCs) we additionally call usb_phy_roothub_exit (which calls phy_exit) when entering system suspend
- (we undo the previous steps during system resume)

The goal of this series is to fix the issue reported by Roger without breaking suspend/resume on the Mediatek SoCs.
Since I neither have a TI nor a Mediatek device I am sending this as RFC. I have tested it on an Amlogic Meson GXM board (Khadas VIM2) which does NOT support suspend/resume yet.

Additionally Stefan Wahren reported [7] that booting on a Raspberry Pi with CONFIG_GENERIC_PHY being disabled (which is the case for bcm2835_defconfig) breaks USB. A fix for this is also included.

changes since v5 at [9]
- rebased on top of v4.17-rc1
- added Keerthy's Tested-by to the first three patches (thank you!)
- fixed a checkpatch warning (which was introduced in v4.17-rc1's checkpatch.pl) in patch #6 by using "/* SPDX-License-Identifier" instead of "// SPDX-License-Identifier"

changes since v4 at [8]
- updated series title since it now includes fixes for other functionality than suspend
- rebased on top of f8cf2f16a7c95a from Linus' tree -> I will re-send an updated version once v4.17-rc1 is out, I just sent it early to get some feedback early!
- included patch [3] "usb: core: phy: fix return value of usb_phy_roothub_exit()" in this series
- fix the logic if CONFIG_GENERIC_PHY is disabled (as reported by Stefan Wahren, new patch #4)
- two minor coding style fixes as suggested by Masahiro Yamada (new patches #4 and #5)

changes since RFC v3 at [6]:
- added Chunfeng Yun's Tested-by and Roger Quadros' Reviewed-by (thank you!)
- dropped RFC prefix

changes since RFC v2 at [5]:
- add missing INIT_LIST_HEAD call in usb_phy_roothub_add_phy (affects patch #1 - spotted by Roger Quadros, thank you!)
- fixed swapped conditions using device_may_wakeup() in usb_phy_roothub_resume because we need to call usb_phy_roothub_init if the controller cannot wake up the device (affects patch #2, spotted by Chunfeng Yun, thank you!)
- simplified the error condition to "undo" usb_phy_roothub_init if usb_phy_roothub_power_on failed in usb_phy_roothub_resume (suggested by Chunfeng Yun)
- updated the commit message (using Roger's wording) because (quote from Roger "it doesn't prevent the system from entering suspend but just prevents the system from reaching lowest power levels in the suspend state."

Changes since RFC v1 (blob attachments) at [4]:
- use device_may_wakeup instead of device_can_wakeup as suggested by Roger Quadros
- use the controller device from hcd->self.controller as suggested by Chunfeng Yun
- compile time fixes thanks to Roger Quadros
- if usb_phy_roothub_power_on in usb_phy_roothub_resume failes then we now call usb_phy_roothub_exit to keep the PHYs in the correct state if usb_phy_roothub_resume partially failed

[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006599.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006737.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006758.html
[3] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006819.html
[4] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006794.html
[5] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006820.html
[6] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006847.html
[7] https://www.spinics.net/lists/linux-usb/msg167472.html
[8] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006882.html
[9] http://lists.infradead.org/pipermail/linux-amlogic/2018-April/007009.html

Martin Blumenstingl (6):
usb: core: phy: fix return value of usb_phy_roothub_exit()
usb: core: split usb_phy_roothub_{init,alloc}
usb: core: use phy_exit during suspend if wake up is not supported
usb: core: phy: make it a no-op if CONFIG_GENERIC_PHY is disabled
usb: core: phy: add missing forward declaration for "struct device"
usb: core: phy: add the SPDX-License-Identifier and include guard

 drivers/usb/core/hcd.c | 18 +++++---
 drivers/usb/core/phy.c | 93 ++++++++++++++++++++++++++++++------------
 drivers/usb/core/phy.h | 22 +++++++++-
 3 files changed, 99 insertions(+), 34 deletions(-)

2.17.0

usb_phy_roothub_exit() should return the error code from the phy_exit() call if exiting the PHY failed.
However, since a wrong variable is used usb_phy_roothub_exit() currently always returns 0, even if one of the phy_exit calls returned an error. Clang also reports this bug:
kernel/drivers/usb/core/phy.c:114:8: warning: explicitly assigning value of variable of type 'int' to itself [-Wself-assign] error, forbidden
warning: phy.c:114

Fix this by assigning the error code from phy_exit() to the "ret" variable to propagate the error correctly.

---
 drivers/usb/core/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 09b7c43c0ea4..f19aaa3c899c 100644
@@ -111,7 +111,7 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
 	list_for_each_entry(roothub_entry, head, list) {
 		err = phy_exit(roothub_entry->phy);
 		if (err)
-			ret = ret;
+			ret = err;
 	}
 
 	return ret;

2.17.0

Before this patch usb_phy_roothub_init served two purposes (from a caller's point of view - like hcd.c):
- parsing the PHYs and allocating the list entries
- calling phy_init on each list entry

While this worked so far it has one disadvantage: if we need to call phy_init for each PHY instance then the existing code cannot be re-used. Solve this by splitting off usb_phy_roothub_alloc which only parses the PHYs and allocates the list entries.
usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls phy_init on each PHY instance (along with the corresponding cleanup if that failed somewhere).

This is a preparation step for adding proper suspend support for some hardware that requires phy_exit to be called during suspend and phy_init to be called during resume.

---
 drivers/usb/core/hcd.c | 10 +++++---
 drivers/usb/core/phy.c | 53 +++++++++++++++++++++---------------------
 drivers/usb/core/phy.h |  4 +++-
 3 files changed, 37 insertions(+), 30 deletions(-)

index 777036ae6367..15b0418e3b6a 100644
@@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
-		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
+		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
 		if (IS_ERR(hcd->phy_roothub)) {
 			retval = PTR_ERR(hcd->phy_roothub);
-			goto err_phy_roothub_init;
+			goto err_phy_roothub_alloc;
 		}
 
+		retval = usb_phy_roothub_init(hcd->phy_roothub);
+		if (retval)
+			goto err_phy_roothub_alloc;
+
 		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
 		if (retval)
 			goto err_usb_phy_roothub_power_on;

@@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	usb_phy_roothub_power_off(hcd->phy_roothub);
 err_usb_phy_roothub_power_on:
 	usb_phy_roothub_exit(hcd->phy_roothub);
-err_phy_roothub_init:
+err_phy_roothub_alloc:
 	if (hcd->remove_phy && hcd->usb_phy) {
 		usb_phy_shutdown(hcd->usb_phy);
 		usb_put_phy(hcd->usb_phy);

index 6fde59bfbff8..eb31253201ad 100644
@@ -1,6 +1,8 @@
 struct usb_phy_roothub;
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
 int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);

2.17.0

If the USB controller can wake up the system (which is the case for example with the Mediatek USB3 IP) then we must not call phy_exit during suspend to ensure that the USB controller doesn't have to re-enumerate the devices during resume.
However, if the USB controller cannot wake up the system (which is the case for example on various TI platforms using a dwc3 controller) then we must call phy_exit during suspend. Otherwise the PHY driver keeps the clocks enabled, which prevents the system from reaching the lowest power levels in the suspend state.

Solve this by introducing two new functions in the PHY wrapper which are dedicated to the suspend and resume handling.
If the controller can wake up the system the new usb_phy_roothub_suspend function will simply call usb_phy_roothub_power_off. However, if wake up is not supported by the controller it will also call usb_phy_roothub_exit.
The also new usb_phy_roothub_resume function takes care of calling usb_phy_roothub_init (if the controller can't wake up the system) in addition to usb_phy_roothub_power_on.

---
 drivers/usb/core/hcd.c |  8 +++++---
 drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/usb/core/phy.h |  5 +++++
 3 files changed, 45 insertions(+), 3 deletions(-)

index 15b0418e3b6a..78bae4ecd68b 100644
@@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 		hcd->state = HC_STATE_SUSPENDED;
 
 		if (!PMSG_IS_AUTO(msg))
-			usb_phy_roothub_power_off(hcd->phy_roothub);
+			usb_phy_roothub_suspend(hcd->self.sysdev,
+						hcd->phy_roothub);
 
 		/* Did we race with a root-hub wakeup event? */
 		if (rhdev->do_remote_wakeup) {

@@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	}
 
 	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_power_on(hcd->phy_roothub);
+		status = usb_phy_roothub_resume(hcd->self.sysdev,
+						hcd->phy_roothub);
 		if (status)
 			return status;
 	}

@@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
-		usb_phy_roothub_power_off(hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)

index 44f008cda7a8..a39d9bb26a4f 100644
@@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 		phy_power_off(roothub_entry->phy);
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub)
+{
+	usb_phy_roothub_power_off(phy_roothub);
+
+	/* keep the PHYs initialized so the device can wake up the system */
+	if (device_may_wakeup(controller_dev))
+		return 0;
+
+	return usb_phy_roothub_exit(phy_roothub);
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
+
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub)
+{
+	int err;
+
+	/* if the device can't wake up the system _exit was called */
+	if (!device_may_wakeup(controller_dev)) {
+		err = usb_phy_roothub_init(phy_roothub);
+		if (err)
+			return err;
+	}
+
+	err = usb_phy_roothub_power_on(phy_roothub);
+
+	/* undo _init if _power_on failed */
+	if (err && !device_may_wakeup(controller_dev))
+		usb_phy_roothub_exit(phy_roothub);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);

index eb31253201ad..605555901d44 100644
@@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub);

2.17.0

If the generic PHY support is disabled the stub of devm_of_phy_get_by_index returns ENOSYS. This corner case isn't handled properly by usb_phy_roothub_add_phy and at least breaks USB support on Raspberry Pi (bcm2835_defconfig):

    dwc2 20980000.usb: dwc2_hcd_init() FAILED, returning -38
    dwc2: probe of 20980000.usb failed with error -38

Let usb_phy_roothub_alloc() return in case CONFIG_GENERIC_PHY is disabled to fix this issue (compilers might even be smart enough to optimize away most of the code within usb_phy_roothub_alloc and usb_phy_roothub_add_phy if CONFIG_GENERIC_PHY is disabled). All existing usb_phy_roothub_* functions are already NULL-safe, so no special handling is required there.

---
 drivers/usb/core/phy.c | 3 +++
 1 file changed, 3 insertions(+)

index a39d9bb26a4f..9879767452a2 100644
@@ -50,6 +50,9 @@ struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
 	struct usb_phy_roothub *phy_roothub;
 	int i, num_phys, err;
 
+	if (!IS_ENABLED(CONFIG_GENERIC_PHY))
+		return NULL;
+
 	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
 					      "#phy-cells");
 	if (num_phys <= 0)

2.17.0

Currently hcd.c is the only consumer of the usb_phy_roothub logic. This already includes the required header files so struct device is known. However, future consumers might not know about struct device. Add a forward declaration for struct device to fix potential future consumers which don't include any of the struct device API headers.

---
 drivers/usb/core/phy.h | 1 +
 1 file changed, 1 insertion(+)

index 605555901d44..bbc969383074 100644
@@ -1,3 +1,4 @@
+struct device;
 struct usb_phy_roothub;
 
 struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);

2.17.0

This clarifies the license of the code. While here also add an include guard to the header file.

---
 drivers/usb/core/phy.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index bbc969383074..88a3c037e9df 100644
@@ -1,3 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * USB roothub wrapper
+ *
+ * Copyright (C) 2018 Martin Blumenstingl <[email protected]>
+ */
+
+#ifndef __USB_CORE_PHY_H_
+#define __USB_CORE_PHY_H_
+
 struct device;
 struct usb_phy_roothub;
 

@@ -13,3 +23,5 @@ int usb_phy_roothub_suspend(struct device *controller_dev,
 			    struct usb_phy_roothub *phy_roothub);
 int usb_phy_roothub_resume(struct device *controller_dev,
 			   struct usb_phy_roothub *phy_roothub);
+
+#endif /* __USB_CORE_PHY_H_ */

2.17.0

Hi Greg,

drivers/usb/core/hcd.c uses the same license identifier that code is much more "valuable" than my few lines which manage a list of PHYs - so I'm fine with "GPLv2 or anything later"

if you see any problems with this (for example that phy.h couldn't be used from some special module with another license, ...) then please let me know

Regards
Martin

Nope, that's fine, thanks for the quick response, I'll go apply this now.

greg k-h