[PATCH 0/8] usb: dwc3: qcom: fix wakeup implementation

[PATCH 0/8] usb: dwc3: qcom: fix wakeup implementation

[Johan Hovold]

This series fixes some of the fallout after the recently merged series
that added wakeup support to the Qualcomm dwc3 driver:

	https://lore.kernel.org/all/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/

The first patch fixes a long standing PHY power sequencing issue in dwc3
core.

The second patch reverts a power-domain hack that was added by the above
series. There are other ways to implement this which doesn't violate the
genpd interface, and if for some reason those are not sufficient, the
genpd implementation needs to be extended, not hacked around.

The third patch fixes a couple of NULL-pointer dereferences when
suspending controllers in peripheral or OTG mode due to a hack that was
added to suspend path. Unfortunately, it seems the hack needs to stay
for now if we want functioning suspend on some Qualcomm platforms.

The fourth patch fixes another issue in the Qualcomm dwc3 implementation
that has been added a while back and which breaks runtime PM.

The remaining patches moves the wakeup-source property over from the
core node to the glue node in the binding and instead propagates the
wakeup capability to the former during probe.

Note that this incidentally also avoids adding probe-deferral hacks to
the driver as was recently proposed to deal with another problem with
the current implementation:

	https://lore.kernel.org/all/1657891312-21748-1-git-send-email-quic_kriskura@quicinc.com/

With this series I have functioning USB system suspend and wakeup as
well as somewhat functioning runtime PM in host mode on sc8280xp. Note
that the PHYs apparently do not need to be shutdown for wakeup on this
platform.

Some issues remain such as that the controller needs to be suspended to
handle remote wakeup during runtime PM (the wakeup interrupts probably
needs to be enabled whenever there's a wakeup-capable device connected
to the bus) and that root hub connect/disconnect events cannot
selectively be disabled.

And of course, the suspend speed hack needs to be replaced at some
point but that likely requires some more heavy lifting in the dwc3
implementation.

Johan


Johan Hovold (8):
  usb: dwc3: fix PHY disable sequence
  Revert "usb: dwc3: qcom: Keep power domain on to retain controller
    status"
  usb: dwc3: qcom: fix broken non-host-mode suspend
  usb: dwc3: qcom: fix runtime PM wakeup
  Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"
  dt-bindings: usb: qcom,dwc3: add wakeup-source property
  usb: dwc3: qcom: fix wakeup implementation
  usb: dwc3: qcom: clean up suspend callbacks

 .../devicetree/bindings/usb/qcom,dwc3.yaml    |  2 +
 .../devicetree/bindings/usb/snps,dwc3.yaml    |  5 --
 drivers/usb/dwc3/core.c                       | 24 +++---
 drivers/usb/dwc3/dwc3-qcom.c                  | 77 ++++++++++---------
 4 files changed, 55 insertions(+), 53 deletions(-)

-- 
2.35.1

[PATCH 1/8] usb: dwc3: fix PHY disable sequence

[Johan Hovold]

Generic PHYs must be powered-off before they can be tore down.

Similarly, suspending legacy PHYs after having powered them off makes no
sense.

Fix the dwc3_core_exit() (e.g. called during suspend) and open-coded
dwc3_probe() error-path sequences that got this wrong.

Note that this makes dwc3_core_exit() match the dwc3_core_init() error
path with respect to powering off the PHYs.

Fixes: 03c1fd622f72 ("usb: dwc3: core: add phy cleanup for probe error handling")
Fixes: c499ff71ff2a ("usb: dwc3: core: re-factor init and exit paths")
Cc: stable@vger.kernel.org      # 4.8
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c5c238ab3083..16d1f328775f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -833,15 +833,16 @@ static void dwc3_core_exit(struct dwc3 *dwc)
 {
 	dwc3_event_buffers_cleanup(dwc);
 
+	usb_phy_set_suspend(dwc->usb2_phy, 1);
+	usb_phy_set_suspend(dwc->usb3_phy, 1);
+	phy_power_off(dwc->usb2_generic_phy);
+	phy_power_off(dwc->usb3_generic_phy);
+
 	usb_phy_shutdown(dwc->usb2_phy);
 	usb_phy_shutdown(dwc->usb3_phy);
 	phy_exit(dwc->usb2_generic_phy);
 	phy_exit(dwc->usb3_generic_phy);
 
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
-	phy_power_off(dwc->usb2_generic_phy);
-	phy_power_off(dwc->usb3_generic_phy);
 	dwc3_clk_disable(dwc);
 	reset_control_assert(dwc->reset);
 }
@@ -1879,16 +1880,16 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc3_debugfs_exit(dwc);
 	dwc3_event_buffers_cleanup(dwc);
 
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
-
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
 	phy_power_off(dwc->usb2_generic_phy);
 	phy_power_off(dwc->usb3_generic_phy);
 
+	usb_phy_shutdown(dwc->usb2_phy);
+	usb_phy_shutdown(dwc->usb3_phy);
+	phy_exit(dwc->usb2_generic_phy);
+	phy_exit(dwc->usb3_generic_phy);
+
 	dwc3_ulpi_exit(dwc);
 
 err4:
-- 
2.35.1

[PATCH 2/8] Revert "usb: dwc3: qcom: Keep power domain on to retain controller status"

[Johan Hovold]

This reverts commit d9be8d5c5b032e5383ff5c404ff4155e9c705429.

Generic power-domain flags must be set before the power-domain is
initialised and must specifically not be modified by drivers for devices
that happen to be in the domain.

To make sure that USB power-domains are left enabled during system
suspend when a device in the domain is in the wakeup path, the
GENPD_FLAG_ACTIVE_WAKEUP flag should instead be set for the domain
unconditionally when it is registered.

Note that this also avoids keeping power-domains on during suspend when
wakeup has not been enabled (e.g. through sysfs).

For the runtime PM case, making sure that the PHYs are not suspended and
that they are in the same domain as the controller prevents the domain
from being suspended. If there are cases where this is not possible or
desirable, the genpd implementation may need to be extended.

Fixes: d9be8d5c5b03 ("usb: dwc3: qcom: Keep power domain on to retain controller status")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index c5e482f53e9d..be2e3dd36440 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -17,7 +17,6 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
-#include <linux/pm_domain.h>
 #include <linux/usb/of.h>
 #include <linux/reset.h>
 #include <linux/iopoll.h>
@@ -757,13 +756,12 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
-	struct dwc3_qcom *qcom;
-	struct resource	*res, *parent_res = NULL;
-	int ret, i;
-	bool ignore_pipe_clk;
-	struct generic_pm_domain *genpd;
+	struct device_node	*np = pdev->dev.of_node;
+	struct device		*dev = &pdev->dev;
+	struct dwc3_qcom	*qcom;
+	struct resource		*res, *parent_res = NULL;
+	int			ret, i;
+	bool			ignore_pipe_clk;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)
@@ -772,8 +770,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, qcom);
 	qcom->dev = &pdev->dev;
 
-	genpd = pd_to_genpd(qcom->dev->pm_domain);
-
 	if (has_acpi_companion(dev)) {
 		qcom->acpi_pdata = acpi_device_get_match_data(dev);
 		if (!qcom->acpi_pdata) {
@@ -881,17 +877,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	if (device_can_wakeup(&qcom->dwc3->dev)) {
-		/*
-		 * Setting GENPD_FLAG_ALWAYS_ON flag takes care of keeping
-		 * genpd on in both runtime suspend and system suspend cases.
-		 */
-		genpd->flags |= GENPD_FLAG_ALWAYS_ON;
-		device_init_wakeup(&pdev->dev, true);
-	} else {
-		genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
-	}
-
+	device_init_wakeup(&pdev->dev, 1);
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-- 
2.35.1

[PATCH 3/8] usb: dwc3: qcom: fix broken non-host-mode suspend

[Johan Hovold]

A recent commit implementing wakeup support in host mode instead broke
suspend for peripheral and OTG mode.

The hack that was added in the suspend path to determine the speed of
any device connected to the USB2 bus not only accesses internal driver
data for a child device, but also dereferences a NULL pointer when not
in host mode and there is no HCD.

As the controller can switch role at any time when in OTG mode, there's
no quick fix to this, and since reverting would leave us with broken
suspend in host-mode (wakeup triggers immediately), keep the hack for
now and only fix the NULL-pointer dereference.

Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index be2e3dd36440..b75ff40f75a2 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -301,8 +301,17 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 {
 	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
-	struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
 	struct usb_device *udev;
+	struct usb_hcd *hcd;
+
+	if (qcom->mode != USB_DR_MODE_HOST)
+		return USB_SPEED_UNKNOWN;
+
+	/*
+	 * FIXME: Fix this nasty layering violation which currently only
+	 *        "works" in host mode.
+	 */
+	hcd = platform_get_drvdata(dwc->xhci);
 
 	/*
 	 * It is possible to query the speed of all children of
-- 
2.35.1

[PATCH 4/8] usb: dwc3: qcom: fix runtime PM wakeup

[Johan Hovold]

A device must enable wakeups during runtime suspend regardless of
whether it is capable and allowed to wake the system up from system
suspend.

Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index b75ff40f75a2..57d3a0e6f280 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -395,7 +395,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
 }
 
-static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
+static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 {
 	u32 val;
 	int i, ret;
@@ -414,7 +414,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 	if (ret)
 		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
 
-	if (device_may_wakeup(qcom->dev)) {
+	if (wakeup) {
 		qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
 		dwc3_qcom_enable_interrupts(qcom);
 	}
@@ -424,7 +424,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 	return 0;
 }
 
-static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
+static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 {
 	int ret;
 	int i;
@@ -432,7 +432,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 	if (!qcom->is_suspended)
 		return 0;
 
-	if (device_may_wakeup(qcom->dev))
+	if (wakeup)
 		dwc3_qcom_disable_interrupts(qcom);
 
 	for (i = 0; i < qcom->num_clocks; i++) {
@@ -939,9 +939,11 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+	bool wakeup = device_may_wakeup(dev);
 	int ret = 0;
 
-	ret = dwc3_qcom_suspend(qcom);
+
+	ret = dwc3_qcom_suspend(qcom, wakeup);
 	if (!ret)
 		qcom->pm_suspended = true;
 
@@ -951,9 +953,10 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
 static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+	bool wakeup = device_may_wakeup(dev);
 	int ret;
 
-	ret = dwc3_qcom_resume(qcom);
+	ret = dwc3_qcom_resume(qcom, wakeup);
 	if (!ret)
 		qcom->pm_suspended = false;
 
@@ -964,14 +967,14 @@ static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
 
-	return dwc3_qcom_suspend(qcom);
+	return dwc3_qcom_suspend(qcom, true);
 }
 
 static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
 
-	return dwc3_qcom_resume(qcom);
+	return dwc3_qcom_resume(qcom, true);
 }
 
 static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
-- 
2.35.1

[PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Johan Hovold]

This reverts commit 098c4d43b91a269e89f60331a26a3f3b914677ed.

A devicetree binding should describe hardware capabilities and not be
used to configure power-management policies (even if things are a bit
blurry when it comes to "wakeup-source"). It should also not be used to
work around Linux driver implementation issues such as how to coordinate
the glue and core dwc3 drivers.

For the Qualcomm dwc3 controllers, it is the glue device that manages
the wakeup interrupts, which may or may not be able to wake the system
up from system suspend.

Also note that USB remote wakeup is always enabled during runtime
suspend.

Fixes: 098c4d43b91a ("dt-bindings: usb: dwc3: Add wakeup-source property support")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 1779d08ba1c0..d41265ba8ce2 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -343,11 +343,6 @@ properties:
       This port is used with the 'usb-role-switch' property  to connect the
       dwc3 to type C connector.
 
-  wakeup-source:
-    $ref: /schemas/types.yaml#/definitions/flag
-    description:
-      Enable USB remote wakeup.
-
 unevaluatedProperties: false
 
 required:
-- 
2.35.1

[PATCH 6/8] dt-bindings: usb: qcom,dwc3: add wakeup-source property

[Johan Hovold]

Add a wakeup-source property to the binding to describe whether the
wakeup interrupts can wake the system from suspend.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index fea3e7092ace..f6e118bf04f6 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -108,6 +108,8 @@ properties:
       HS/FS/LS modes are supported.
     type: boolean
 
+  wakeup-source: true
+
 # Required child node:
 
 patternProperties:
-- 
2.35.1

[PATCH 7/8] usb: dwc3: qcom: fix wakeup implementation

[Johan Hovold]

It is the Qualcomm glue wakeup interrupts that may be able to wake the
system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and
instead propagate the capability setting to the core device during
probe.

This is needed as there is currently no way for the core driver to query
the wakeup setting of the glue device, but it is the core driver that
manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled
through sysfs.

Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 16d1f328775f..8c8e32651473 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);
@@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
 			dwc3_core_exit(dwc);
 			break;
 		}
@@ -2045,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 57d3a0e6f280..1bd2818b4daa 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -771,6 +771,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	struct resource		*res, *parent_res = NULL;
 	int			ret, i;
 	bool			ignore_pipe_clk;
+	bool			wakeup_source;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)
@@ -886,7 +887,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	device_init_wakeup(&pdev->dev, 1);
+	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
+	device_init_wakeup(&pdev->dev, wakeup_source);
+	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
+
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-- 
2.35.1

[PATCH 8/8] usb: dwc3: qcom: clean up suspend callbacks

[Johan Hovold]

Clean up the suspend callbacks by separating the error and success paths
to improve readability.

Also drop a related redundant initialisation.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 1bd2818b4daa..01e8c2fc6914 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -944,14 +944,15 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
 	bool wakeup = device_may_wakeup(dev);
-	int ret = 0;
-
+	int ret;
 
 	ret = dwc3_qcom_suspend(qcom, wakeup);
-	if (!ret)
-		qcom->pm_suspended = true;
+	if (ret)
+		return ret;
 
-	return ret;
+	qcom->pm_suspended = true;
+
+	return 0;
 }
 
 static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
@@ -961,10 +962,12 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
 	int ret;
 
 	ret = dwc3_qcom_resume(qcom, wakeup);
-	if (!ret)
-		qcom->pm_suspended = false;
+	if (ret)
+		return ret;
 
-	return ret;
+	qcom->pm_suspended = false;
+
+	return 0;
 }
 
 static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
-- 
2.35.1

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Rob Herring]

On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> This reverts commit 098c4d43b91a269e89f60331a26a3f3b914677ed.
>
> A devicetree binding should describe hardware capabilities and not be
> used to configure power-management policies (even if things are a bit
> blurry when it comes to "wakeup-source").

Whether a device's interrupt can cause a wakeup is a h/w feature.
That's not policy. If Linux also uses this to decide whether or not to
enable wakeup, then that's its policy.

> It should also not be used to
> work around Linux driver implementation issues such as how to coordinate
> the glue and core dwc3 drivers.
>
> For the Qualcomm dwc3 controllers, it is the glue device that manages
> the wakeup interrupts, which may or may not be able to wake the system
> up from system suspend.

While the reasoning to add this may have been for QCom, having this
property for other users makes sense. On some platforms, 'snps,dwc3'
is the only node (i.e. there's no wrapper node). So I don't think this
should be reverted.

Rob

Re: [PATCH 2/8] Revert "usb: dwc3: qcom: Keep power domain on to retain controller status"

[Krishna Kurapati PSSNV]

On 8/2/2022 8:43 PM, Johan Hovold wrote:
> This reverts commit d9be8d5c5b032e5383ff5c404ff4155e9c705429.
>
> Generic power-domain flags must be set before the power-domain is
> initialised and must specifically not be modified by drivers for devices
> that happen to be in the domain.
>
> To make sure that USB power-domains are left enabled during system
> suspend when a device in the domain is in the wakeup path, the
> GENPD_FLAG_ACTIVE_WAKEUP flag should instead be set for the domain
> unconditionally when it is registered.
Hi Johan, Thanks for the review and suggestion.

In case we need the genpd framework to set the GENPD_FLAG_ACTIVE_WAKEUP
flag for a particular domain that will be used by a device (which is
wakeup capable) and hasn't been probed yet , can you help me understand if
there any dt-flags we  can add to convey the same the genpd  framework
so that it will set that flag during domain registration ?

Regards,
Krishna,
>
> Note that this also avoids keeping power-domains on during suspend when
> wakeup has not been enabled (e.g. through sysfs).
>
> For the runtime PM case, making sure that the PHYs are not suspended and
> that they are in the same domain as the controller prevents the domain
> from being suspended. If there are cases where this is not possible or
> desirable, the genpd implementation may need to be extended.
>
> Fixes: d9be8d5c5b03 ("usb: dwc3: qcom: Keep power domain on to retain controller status")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/usb/dwc3/dwc3-qcom.c | 28 +++++++---------------------
>   1 file changed, 7 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index c5e482f53e9d..be2e3dd36440 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -17,7 +17,6 @@
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
>   #include <linux/phy/phy.h>
> -#include <linux/pm_domain.h>
>   #include <linux/usb/of.h>
>   #include <linux/reset.h>
>   #include <linux/iopoll.h>
> @@ -757,13 +756,12 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
>   
>   static int dwc3_qcom_probe(struct platform_device *pdev)
>   {
> -	struct device_node *np = pdev->dev.of_node;
> -	struct device *dev = &pdev->dev;
> -	struct dwc3_qcom *qcom;
> -	struct resource	*res, *parent_res = NULL;
> -	int ret, i;
> -	bool ignore_pipe_clk;
> -	struct generic_pm_domain *genpd;
> +	struct device_node	*np = pdev->dev.of_node;
> +	struct device		*dev = &pdev->dev;
> +	struct dwc3_qcom	*qcom;
> +	struct resource		*res, *parent_res = NULL;
> +	int			ret, i;
> +	bool			ignore_pipe_clk;
>   
>   	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
>   	if (!qcom)
> @@ -772,8 +770,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, qcom);
>   	qcom->dev = &pdev->dev;
>   
> -	genpd = pd_to_genpd(qcom->dev->pm_domain);
> -
>   	if (has_acpi_companion(dev)) {
>   		qcom->acpi_pdata = acpi_device_get_match_data(dev);
>   		if (!qcom->acpi_pdata) {
> @@ -881,17 +877,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto interconnect_exit;
>   
> -	if (device_can_wakeup(&qcom->dwc3->dev)) {
> -		/*
> -		 * Setting GENPD_FLAG_ALWAYS_ON flag takes care of keeping
> -		 * genpd on in both runtime suspend and system suspend cases.
> -		 */
> -		genpd->flags |= GENPD_FLAG_ALWAYS_ON;
> -		device_init_wakeup(&pdev->dev, true);
> -	} else {
> -		genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> -	}
> -
> +	device_init_wakeup(&pdev->dev, 1);
>   	qcom->is_suspended = false;
>   	pm_runtime_set_active(dev);
>   	pm_runtime_enable(dev);

Re: [PATCH 1/8] usb: dwc3: fix PHY disable sequence

[Andrew Halaney]

On Tue, Aug 02, 2022 at 05:13:57PM +0200, Johan Hovold wrote:
> Generic PHYs must be powered-off before they can be tore down.
> 
> Similarly, suspending legacy PHYs after having powered them off makes no
> sense.
> 
> Fix the dwc3_core_exit() (e.g. called during suspend) and open-coded
> dwc3_probe() error-path sequences that got this wrong.
> 
> Note that this makes dwc3_core_exit() match the dwc3_core_init() error
> path with respect to powering off the PHYs.
> 
> Fixes: 03c1fd622f72 ("usb: dwc3: core: add phy cleanup for probe error handling")
> Fixes: c499ff71ff2a ("usb: dwc3: core: re-factor init and exit paths")
> Cc: stable@vger.kernel.org      # 4.8
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

>  drivers/usb/dwc3/core.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c5c238ab3083..16d1f328775f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -833,15 +833,16 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>  {
>  	dwc3_event_buffers_cleanup(dwc);
>  
> +	usb_phy_set_suspend(dwc->usb2_phy, 1);
> +	usb_phy_set_suspend(dwc->usb3_phy, 1);
> +	phy_power_off(dwc->usb2_generic_phy);
> +	phy_power_off(dwc->usb3_generic_phy);
> +
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
>  	phy_exit(dwc->usb2_generic_phy);
>  	phy_exit(dwc->usb3_generic_phy);
>  
> -	usb_phy_set_suspend(dwc->usb2_phy, 1);
> -	usb_phy_set_suspend(dwc->usb3_phy, 1);
> -	phy_power_off(dwc->usb2_generic_phy);
> -	phy_power_off(dwc->usb3_generic_phy);
>  	dwc3_clk_disable(dwc);
>  	reset_control_assert(dwc->reset);
>  }
> @@ -1879,16 +1880,16 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc3_debugfs_exit(dwc);
>  	dwc3_event_buffers_cleanup(dwc);
>  
> -	usb_phy_shutdown(dwc->usb2_phy);
> -	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> -
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
>  	phy_power_off(dwc->usb2_generic_phy);
>  	phy_power_off(dwc->usb3_generic_phy);
>  
> +	usb_phy_shutdown(dwc->usb2_phy);
> +	usb_phy_shutdown(dwc->usb3_phy);
> +	phy_exit(dwc->usb2_generic_phy);
> +	phy_exit(dwc->usb3_generic_phy);
> +
>  	dwc3_ulpi_exit(dwc);
>  
>  err4:
> -- 
> 2.35.1
> 

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Johan Hovold]

On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote:
> On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> > It should also not be used to
> > work around Linux driver implementation issues such as how to coordinate
> > the glue and core dwc3 drivers.
> >
> > For the Qualcomm dwc3 controllers, it is the glue device that manages
> > the wakeup interrupts, which may or may not be able to wake the system
> > up from system suspend.
> 
> While the reasoning to add this may have been for QCom, having this
> property for other users makes sense. On some platforms, 'snps,dwc3'
> is the only node (i.e. there's no wrapper node). So I don't think this
> should be reverted.

Fair enough. Let's keep it in the core child node then where we can
still retrieve from the glue parent directly.

(I assume you're not suggesting also adding 'wakeup-source' to the qcom
glue node, which is where the actual wakeup interrupts live.)

The glue and core parts needs to work in concert even if the current
implementation tends to make that harder than it should be.

Johan

Re: [PATCH 2/8] Revert "usb: dwc3: qcom: Keep power domain on to retain controller status"

[Johan Hovold]

On Tue, Aug 02, 2022 at 11:30:34PM +0530, Krishna Kurapati PSSNV wrote:
> 
> On 8/2/2022 8:43 PM, Johan Hovold wrote:
> > This reverts commit d9be8d5c5b032e5383ff5c404ff4155e9c705429.
> >
> > Generic power-domain flags must be set before the power-domain is
> > initialised and must specifically not be modified by drivers for devices
> > that happen to be in the domain.
> >
> > To make sure that USB power-domains are left enabled during system
> > suspend when a device in the domain is in the wakeup path, the
> > GENPD_FLAG_ACTIVE_WAKEUP flag should instead be set for the domain
> > unconditionally when it is registered.

> In case we need the genpd framework to set the GENPD_FLAG_ACTIVE_WAKEUP
> flag for a particular domain that will be used by a device (which is
> wakeup capable) and hasn't been probed yet , can you help me understand if
> there any dt-flags we  can add to convey the same the genpd  framework
> so that it will set that flag during domain registration ?

This can't be expressed in DT (currently), so what you need is something
like the below:

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 7ff64d4d5920..4ff855269467 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -3125,6 +3125,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
        .gdscr = 0xf004,
        .pd = {
                .name = "gcc_usb30_prim_gdsc",
+               .flags = GENPD_FLAG_ACTIVE_WAKEUP,
        },
        .pwrsts = PWRSTS_OFF_ON,
        .flags = VOTABLE,
@@ -3134,6 +3135,7 @@ static struct gdsc gcc_usb30_sec_gdsc = {
        .gdscr = 0x9e004,
        .pd = {
                .name = "gcc_usb30_sec_gdsc",
+               .flags = GENPD_FLAG_ACTIVE_WAKEUP,
        },
        .pwrsts = PWRSTS_OFF_ON,
        .flags = VOTABLE,

to make sure that genpd keeps these domains powered during system
suspend if they are used by devices that are in the wakeup path.

Johan

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Krzysztof Kozlowski]

On 03/08/2022 09:31, Johan Hovold wrote:
> On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote:
>> On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> 
>>> It should also not be used to
>>> work around Linux driver implementation issues such as how to coordinate
>>> the glue and core dwc3 drivers.
>>>
>>> For the Qualcomm dwc3 controllers, it is the glue device that manages
>>> the wakeup interrupts, which may or may not be able to wake the system
>>> up from system suspend.
>>
>> While the reasoning to add this may have been for QCom, having this
>> property for other users makes sense. On some platforms, 'snps,dwc3'
>> is the only node (i.e. there's no wrapper node). So I don't think this
>> should be reverted.
> 
> Fair enough. Let's keep it in the core child node then where we can
> still retrieve from the glue parent directly.
> 
> (I assume you're not suggesting also adding 'wakeup-source' to the qcom
> glue node, which is where the actual wakeup interrupts live.)
> 
> The glue and core parts needs to work in concert even if the current
> implementation tends to make that harder than it should be.

I think it can still exist in the glue node (so your next patch),
because as you said this is the place with wakeup interrupt, so it looks
like correct hardware description. In the next patch you would need to
disallow it for the DWC node.

Best regards,
Krzysztof

Re: [PATCH 1/8] usb: dwc3: fix PHY disable sequence

[Matthias Kaehlcke]

On Tue, Aug 02, 2022 at 05:13:57PM +0200, Johan Hovold wrote:
> Generic PHYs must be powered-off before they can be tore down.
> 
> Similarly, suspending legacy PHYs after having powered them off makes no
> sense.
> 
> Fix the dwc3_core_exit() (e.g. called during suspend) and open-coded
> dwc3_probe() error-path sequences that got this wrong.
> 
> Note that this makes dwc3_core_exit() match the dwc3_core_init() error
> path with respect to powering off the PHYs.
> 
> Fixes: 03c1fd622f72 ("usb: dwc3: core: add phy cleanup for probe error handling")
> Fixes: c499ff71ff2a ("usb: dwc3: core: re-factor init and exit paths")
> Cc: stable@vger.kernel.org      # 4.8
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

I also wondered about this earlier, but didn't take action.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 3/8] usb: dwc3: qcom: fix broken non-host-mode suspend

[Matthias Kaehlcke]

On Tue, Aug 02, 2022 at 05:13:59PM +0200, Johan Hovold wrote:
> A recent commit implementing wakeup support in host mode instead broke
> suspend for peripheral and OTG mode.
> 
> The hack that was added in the suspend path to determine the speed of
> any device connected to the USB2 bus not only accesses internal driver
> data for a child device, but also dereferences a NULL pointer when not
> in host mode and there is no HCD.
> 
> As the controller can switch role at any time when in OTG mode, there's
> no quick fix to this, and since reverting would leave us with broken
> suspend in host-mode (wakeup triggers immediately), keep the hack for
> now and only fix the NULL-pointer dereference.
> 
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index be2e3dd36440..b75ff40f75a2 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -301,8 +301,17 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
>  {
>  	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> -	struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
>  	struct usb_device *udev;
> +	struct usb_hcd *hcd;
> +
> +	if (qcom->mode != USB_DR_MODE_HOST)
> +		return USB_SPEED_UNKNOWN;

Couldn't instead the below block in dwc3_qcom_suspend() be conditional on
the controller being in host mode?

	if (device_may_wakeup(qcom->dev)) {
		qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
		dwc3_qcom_enable_interrupts(qcom);
	}

I see, the problem is that the role switch could happen at any time as the
commit message says. With this patch there is also a race though, the role
switch could happen just after the check and before obtaining 'hcd'.

Re: [PATCH 4/8] usb: dwc3: qcom: fix runtime PM wakeup

[Matthias Kaehlcke]

On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote:
> A device must enable wakeups during runtime suspend regardless of
> whether it is capable and allowed to wake the system up from system
> suspend.
> 
> Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend.

In how far is runtime PM actually supported/used by this driver? The device is
set 'active' in _probe(), and there are no other pm_runtime_* calls, except
in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from
'active' into 'suspended'?

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Rob Herring]

On Wed, Aug 03, 2022 at 09:31:06AM +0200, Johan Hovold wrote:
> On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote:
> > On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> 
> > > It should also not be used to
> > > work around Linux driver implementation issues such as how to coordinate
> > > the glue and core dwc3 drivers.
> > >
> > > For the Qualcomm dwc3 controllers, it is the glue device that manages
> > > the wakeup interrupts, which may or may not be able to wake the system
> > > up from system suspend.
> > 
> > While the reasoning to add this may have been for QCom, having this
> > property for other users makes sense. On some platforms, 'snps,dwc3'
> > is the only node (i.e. there's no wrapper node). So I don't think this
> > should be reverted.
> 
> Fair enough. Let's keep it in the core child node then where we can
> still retrieve from the glue parent directly.
> 
> (I assume you're not suggesting also adding 'wakeup-source' to the qcom
> glue node, which is where the actual wakeup interrupts live.)

'wakeup-source' belongs wherever the interrupt that causes wakeup is 
defined.

Rob

Re: [PATCH 6/8] dt-bindings: usb: qcom,dwc3: add wakeup-source property

[Rob Herring]

On Tue, 02 Aug 2022 17:14:02 +0200, Johan Hovold wrote:
> Add a wakeup-source property to the binding to describe whether the
> wakeup interrupts can wake the system from suspend.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Johan Hovold]

On Wed, Aug 03, 2022 at 05:26:44PM -0600, Rob Herring wrote:
> On Wed, Aug 03, 2022 at 09:31:06AM +0200, Johan Hovold wrote:
> > On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote:
> > > On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > 
> > > > It should also not be used to
> > > > work around Linux driver implementation issues such as how to coordinate
> > > > the glue and core dwc3 drivers.
> > > >
> > > > For the Qualcomm dwc3 controllers, it is the glue device that manages
> > > > the wakeup interrupts, which may or may not be able to wake the system
> > > > up from system suspend.
> > > 
> > > While the reasoning to add this may have been for QCom, having this
> > > property for other users makes sense. On some platforms, 'snps,dwc3'
> > > is the only node (i.e. there's no wrapper node). So I don't think this
> > > should be reverted.
> > 
> > Fair enough. Let's keep it in the core child node then where we can
> > still retrieve from the glue parent directly.
> > 
> > (I assume you're not suggesting also adding 'wakeup-source' to the qcom
> > glue node, which is where the actual wakeup interrupts live.)
> 
> 'wakeup-source' belongs wherever the interrupt that causes wakeup is 
> defined.

Thanks for clarifying. That was my understanding as well, but I
misinterpreted your wish to keep the 'wakeup-source' property also in
the core node.

My thought was that if it turns out that there are systems that do not
use a glue node but that do indeed support wakeup, then such a property
could be added back later.

But let's keep it in the binding then.

Johan

Re: [PATCH 3/8] usb: dwc3: qcom: fix broken non-host-mode suspend

[Johan Hovold]

On Wed, Aug 03, 2022 at 12:11:41PM -0700, Matthias Kaehlcke wrote:
> On Tue, Aug 02, 2022 at 05:13:59PM +0200, Johan Hovold wrote:
> > A recent commit implementing wakeup support in host mode instead broke
> > suspend for peripheral and OTG mode.
> > 
> > The hack that was added in the suspend path to determine the speed of
> > any device connected to the USB2 bus not only accesses internal driver
> > data for a child device, but also dereferences a NULL pointer when not
> > in host mode and there is no HCD.
> > 
> > As the controller can switch role at any time when in OTG mode, there's
> > no quick fix to this, and since reverting would leave us with broken
> > suspend in host-mode (wakeup triggers immediately), keep the hack for
> > now and only fix the NULL-pointer dereference.
> > 
> > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index be2e3dd36440..b75ff40f75a2 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -301,8 +301,17 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
> >  static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> >  {
> >  	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > -	struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> >  	struct usb_device *udev;
> > +	struct usb_hcd *hcd;
> > +
> > +	if (qcom->mode != USB_DR_MODE_HOST)
> > +		return USB_SPEED_UNKNOWN;
> 
> Couldn't instead the below block in dwc3_qcom_suspend() be conditional on
> the controller being in host mode?
> 
> 	if (device_may_wakeup(qcom->dev)) {
> 		qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
> 		dwc3_qcom_enable_interrupts(qcom);
> 	}

Yeah, the authors clearly didn't consider non-host mode when
implementing this and keeping wakeups disabled in that case probably
doesn't break anything that was ever working.

> I see, the problem is that the role switch could happen at any time as the
> commit message says. With this patch there is also a race though, the role
> switch could happen just after the check and before obtaining 'hcd'.

No, there's no race here as I'm checking the static configuration that
comes from DT. Specifically, I'm not trying to add support for wakeup in
OTG mode, but just prevent suspend from crashing.

I may be possible address also the host-role in OTG mode, but that means
continuing to build on this layer violation.

Note that we're in the suspend callback of the parent so as long as the
drivers for the descendant devices has disabled role switching at this
stage during suspend, we should be good.

But I'm torn about simply ripping this patch out and trying to fix it
up. I want the feature, but the patch adding this clearly wasn't ready
for merging.

I'll take another look at this.

Johan

Re: [PATCH 4/8] usb: dwc3: qcom: fix runtime PM wakeup

[Johan Hovold]

On Wed, Aug 03, 2022 at 02:58:33PM -0700, Matthias Kaehlcke wrote:
> On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote:
> > A device must enable wakeups during runtime suspend regardless of
> > whether it is capable and allowed to wake the system up from system
> > suspend.
> > 
> > Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend.
> 
> In how far is runtime PM actually supported/used by this driver? The device is
> set 'active' in _probe(), and there are no other pm_runtime_* calls, except
> in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from
> 'active' into 'suspended'?

It will be runtime suspended when the child (core) device suspends, but
you need to enable runtime PM through sysfs first.

And the controller is resumed in the wakeup-interrupt handler for the
runtime PM case.

It seems to work ok, and it looks like the driver has supported this
since it was first merged.

Johan

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Johan Hovold]

On Wed, Aug 03, 2022 at 09:51:00AM +0200, Krzysztof Kozlowski wrote:
> On 03/08/2022 09:31, Johan Hovold wrote:
> > On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote:
> >> On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > 
> >>> It should also not be used to
> >>> work around Linux driver implementation issues such as how to coordinate
> >>> the glue and core dwc3 drivers.
> >>>
> >>> For the Qualcomm dwc3 controllers, it is the glue device that manages
> >>> the wakeup interrupts, which may or may not be able to wake the system
> >>> up from system suspend.
> >>
> >> While the reasoning to add this may have been for QCom, having this
> >> property for other users makes sense. On some platforms, 'snps,dwc3'
> >> is the only node (i.e. there's no wrapper node). So I don't think this
> >> should be reverted.
> > 
> > Fair enough. Let's keep it in the core child node then where we can
> > still retrieve from the glue parent directly.
> > 
> > (I assume you're not suggesting also adding 'wakeup-source' to the qcom
> > glue node, which is where the actual wakeup interrupts live.)
> > 
> > The glue and core parts needs to work in concert even if the current
> > implementation tends to make that harder than it should be.
> 
> I think it can still exist in the glue node (so your next patch),
> because as you said this is the place with wakeup interrupt, so it looks
> like correct hardware description. In the next patch you would need to
> disallow it for the DWC node.

Ok, will do. Thanks.

Johan

Re: [PATCH 5/8] Revert "dt-bindings: usb: dwc3: Add wakeup-source property support"

[Johan Hovold]

On Thu, Aug 04, 2022 at 07:47:35AM +0200, Johan Hovold wrote:
> On Wed, Aug 03, 2022 at 05:26:44PM -0600, Rob Herring wrote:
> > On Wed, Aug 03, 2022 at 09:31:06AM +0200, Johan Hovold wrote:
> > > On Tue, Aug 02, 2022 at 11:17:22AM -0600, Rob Herring wrote:
> > > > On Tue, Aug 2, 2022 at 9:14 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > > 
> > > > > It should also not be used to
> > > > > work around Linux driver implementation issues such as how to coordinate
> > > > > the glue and core dwc3 drivers.
> > > > >
> > > > > For the Qualcomm dwc3 controllers, it is the glue device that manages
> > > > > the wakeup interrupts, which may or may not be able to wake the system
> > > > > up from system suspend.
> > > > 
> > > > While the reasoning to add this may have been for QCom, having this
> > > > property for other users makes sense. On some platforms, 'snps,dwc3'
> > > > is the only node (i.e. there's no wrapper node). So I don't think this
> > > > should be reverted.
> > > 
> > > Fair enough. Let's keep it in the core child node then where we can
> > > still retrieve from the glue parent directly.
> > > 
> > > (I assume you're not suggesting also adding 'wakeup-source' to the qcom
> > > glue node, which is where the actual wakeup interrupts live.)
> > 
> > 'wakeup-source' belongs wherever the interrupt that causes wakeup is 
> > defined.
> 
> Thanks for clarifying. That was my understanding as well, but I
> misinterpreted your wish to keep the 'wakeup-source' property also in
> the core node.
> 
> My thought was that if it turns out that there are systems that do not
> use a glue node but that do indeed support wakeup, then such a property
> could be added back later.
> 
> But let's keep it in the binding then.

I realise it may not have been clear that the patch I suggested to
revert was first merged for 6.0-rc1, in case that makes any difference.

But I'll drop the revert unless I hear otherwise.

Johan

Re: [PATCH 4/8] usb: dwc3: qcom: fix runtime PM wakeup

[Matthias Kaehlcke]

On Thu, Aug 04, 2022 at 09:35:16AM +0200, Johan Hovold wrote:
> On Wed, Aug 03, 2022 at 02:58:33PM -0700, Matthias Kaehlcke wrote:
> > On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote:
> > > A device must enable wakeups during runtime suspend regardless of
> > > whether it is capable and allowed to wake the system up from system
> > > suspend.
> > > 
> > > Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > 
> > Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend.
> > 
> > In how far is runtime PM actually supported/used by this driver? The device is
> > set 'active' in _probe(), and there are no other pm_runtime_* calls, except
> > in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from
> > 'active' into 'suspended'?
> 
> It will be runtime suspended when the child (core) device suspends, but
> you need to enable runtime PM through sysfs first.

Thanks for the clarification.

After enabling runtime suspend for the dwc3 core, dwc3 glue and the xHCI
the dwc3-qcom enters autosuspend when the delay expires.

> And the controller is resumed in the wakeup-interrupt handler for the
> runtime PM case.
>
> It seems to work ok, and it looks like the driver has supported this
> since it was first merged.

With and without your patch dwc3-qcom enters autosuspend and stays there.
USB devices like a mouse or a USB to Ethernet adapter keep working while
the glue is suspended.

How is the runtime resume triggered for the dwc3 glue?

Sorry if my questions are very basic, so far I haven't dealt much with
autosuspend and I'm trying to get a better understanding in the context
of the dwc3 and why it is currently broken.

Re: [PATCH 4/8] usb: dwc3: qcom: fix runtime PM wakeup

[Johan Hovold]

On Thu, Aug 04, 2022 at 08:35:10AM -0700, Matthias Kaehlcke wrote:
> On Thu, Aug 04, 2022 at 09:35:16AM +0200, Johan Hovold wrote:

> After enabling runtime suspend for the dwc3 core, dwc3 glue and the xHCI
> the dwc3-qcom enters autosuspend when the delay expires.
> 
> > And the controller is resumed in the wakeup-interrupt handler for the
> > runtime PM case.
> >
> > It seems to work ok, and it looks like the driver has supported this
> > since it was first merged.
> 
> With and without your patch dwc3-qcom enters autosuspend and stays there.
> USB devices like a mouse or a USB to Ethernet adapter keep working while
> the glue is suspended.

Are you sure you're looking at the right controller? And that it is
actually suspended?

If you plug in a keyboard, enable autosuspend for all devices in the
path (from glue to the keyboard device) and type away, then the
controller must remain active. Stop typing, and all devices in the chain
should suspend.

> How is the runtime resume triggered for the dwc3 glue?

Either by the host driver when it needs to access the device, or by the
device if it is remote-wakeup capable (e.g. a keyboard, but not
necessarily a speaker).

Note that the latter part is what is broken currently as the wakeup
interrupts were not enabled and those are needed to wake up sc8280xp 
when the dwc3 glue has been runtime suspended.

Johan

Re: [PATCH 4/8] usb: dwc3: qcom: fix runtime PM wakeup

[Matthias Kaehlcke]

On Thu, Aug 04, 2022 at 06:04:53PM +0200, Johan Hovold wrote:
> On Thu, Aug 04, 2022 at 08:35:10AM -0700, Matthias Kaehlcke wrote:
> > On Thu, Aug 04, 2022 at 09:35:16AM +0200, Johan Hovold wrote:
> 
> > After enabling runtime suspend for the dwc3 core, dwc3 glue and the xHCI
> > the dwc3-qcom enters autosuspend when the delay expires.
> > 
> > > And the controller is resumed in the wakeup-interrupt handler for the
> > > runtime PM case.
> > >
> > > It seems to work ok, and it looks like the driver has supported this
> > > since it was first merged.
> > 
> > With and without your patch dwc3-qcom enters autosuspend and stays there.
> > USB devices like a mouse or a USB to Ethernet adapter keep working while
> > the glue is suspended.
> 
> Are you sure you're looking at the right controller? And that it is
> actually suspended?

Good point! My mind was set on a SC7180 system, which has a single dwc3
controller, but this time I was tinkering on a SC7280 board, which has
two dwc3, and obviously I was looking at the wrong one (－‸ლ)

> If you plug in a keyboard, enable autosuspend for all devices in the
> path (from glue to the keyboard device) and type away, then the
> controller must remain active. Stop typing, and all devices in the chain
> should suspend.

That's what I expected, and now that I'm looking at the right controller
I'm seeing it. I wondered whether the glue device was somehow special.

> > How is the runtime resume triggered for the dwc3 glue?
> 
> Either by the host driver when it needs to access the device, or by the
> device if it is remote-wakeup capable (e.g. a keyboard, but not
> necessarily a speaker).
> 
> Note that the latter part is what is broken currently as the wakeup
> interrupts were not enabled and those are needed to wake up sc8280xp 
> when the dwc3 glue has been runtime suspended.

Thanks for helping me to get a better understanding!