On Thu, May 13, 2021 at 04:49:19PM +0300, Felipe Balbi wrote:

Hi,

Matthias Kaehlcke [off-list ref] writes:

quoted

quoted

Sandeep Maheswaram [off-list ref] writes:

quoted

If wakeup capable devices are connected to the controller (directly
or through hubs) at suspend time keep the power domain on in order
to support wakeup from these devices.

Signed-off-by: Sandeep Maheswaram <redacted>
---
 drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 82125bc..1e220af 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c

@@ -17,9 +17,11 @@
 #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>
+#include <linux/usb/hcd.h>
 
 #include "core.h"
 

@@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 {
 	u32 val;
 	int i, ret;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct usb_hcd  *hcd;
+	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
 
 	if (qcom->is_suspended)
 		return 0;
 
+	if (dwc->xhci) {
+		hcd = platform_get_drvdata(dwc->xhci);
+		if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+			genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
+	}

wow, you really need to find a way to do these things generically
instead of bypassing a bunch of layers and access stuff $this doesn't
directly own.

I'm gonna say 'no' to this, sorry. It looks like xhci should, directly,
learn about much of this instead of hiding it 3-layers deep into the
dwc3 glue layer for your specific SoC.

Maybe this could be addressed with a pair of wakeup quirks, one for suspend,
another for resume. An optional genpd field could be added to struct
xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP
of the genpd.

Does the above sound more palatable?

I don't get why you need quirk flags. All you're doing here is:

	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
        	genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;

If you move this test to xhci_suspend(), you shouldn't need all the
magic, right?

Right, the quirks shouldn't be necessary if setting/clearing the genpd
flag is the right thing to do for any controller that sets the genpd
field in _priv, which probably is the case.