USB DWC3 host wake up support from system suspend

During suspend read the status of all port and make sure the PHYs are in the correct mode based on current speed.
Phy interrupt masks are set based on this mode. Keep track of the mode of the HS PHY to be able to configure wakeup properly.

Also check during suspend if any wakeup capable devices are connected to the controller (directly or through hubs), if there are none set a flag to indicate that the PHY should be powered down during suspend.

Signed-off-by Sandeep Maheswaram <[email protected]>

---
 drivers/usb/dwc3/core.h |  3 +++
 drivers/usb/dwc3/host.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

drivers/usb/dwc3/core.h +3 -0

index b1e875c..cecd278 100644
@@ -1123,6 +1123,9 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+	bool			phy_power_off;
+
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
 

drivers/usb/dwc3/host.c +0 -0

index f29a264..527f04c 100644
@@ -11,6 +11,14 @@
 #include <linux/platform_device.h>

drivers/usb/dwc3/host.c +10 -0

@@ -115,6 +123,13 @@ int dwc3_host_init(struct dwc3 *dwc)
 		}
 	}
 
+	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
+			sizeof(struct xhci_plat_priv));
+	if (ret) {
+		dev_err(dwc->dev, "failed to add data to xHCI\n");
+		goto err;
+	}
+
 	ret = platform_device_add(xhci);
 	if (ret) {
 		dev_err(dwc->dev, "failed to register xHCI device\n");

@@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;
 }
 
+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+

Quoted 13 lines

drivers/usb/dwc3/host.c line 21

Quoted 12 lines

drivers/usb/dwc3/host.c +8 -0

@@ -11,6 +11,14 @@
  #include <linux/platform_device.h>
 
  #include "core.h"
+#include "../host/xhci.h"
+#include "../host/xhci-plat.h"
+
+static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
+
+static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
+	.suspend_quirk = xhci_dwc3_suspend_quirk,
+};

we're passing data using device_properties, why do you want this here?

Similar implemenation was done in drivers/usb/cdns3/host.c<https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/usb/cdns3/host.c?h=next-20210503>

Quoted 20 lines

Quoted 7 lines

drivers/usb/dwc3/host.c +3 -0

@@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
  	return ret;
  }
 
+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 157

Quoted 10 lines

drivers/usb/dwc3/host.c +10 -0

+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_mode = 0;
+
+	reg = readl(&xhci_hcd->cap_regs->hcs_params1);
+	num_ports = HCS_MAX_PORTS(reg);

there's a big assumption here that xhci is still alive. Why isn't this quirk implemented in xhci-plat itself?

Quoted 2 lines

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 175

Quoted 1 line

drivers/usb/dwc3/host.c +1 -0

+int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)

who calls this?

This will be called from xhci_plat_suspend->xhci_priv_suspend_quirk->xhci_dwc3_suspend_quirk

Hi,

Quoted 36 lines · Sandeep Maheswaram

During suspend read the status of all port and make sure the PHYs are in the correct mode based on current speed.
Phy interrupt masks are set based on this mode. Keep track of the mode of the HS PHY to be able to configure wakeup properly.

Also check during suspend if any wakeup capable devices are connected to the controller (directly or through hubs), if there are none set a flag to indicate that the PHY should be powered down during suspend.

Signed-off-by Sandeep Maheswaram <[email protected]>

---
 drivers/usb/dwc3/core.h |  3 +++
 drivers/usb/dwc3/host.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

drivers/usb/dwc3/core.h +3 -0

index b1e875c..cecd278 100644
@@ -1123,6 +1123,9 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+	bool			phy_power_off;
+
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
 

drivers/usb/dwc3/host.c +0 -0

index f29a264..527f04c 100644
@@ -11,6 +11,14 @@
 #include <linux/platform_device.h>

drivers/usb/dwc3/host.c line 21

Quoted 10 lines

drivers/usb/dwc3/host.c +8 -0

 
 #include "core.h"
+#include "../host/xhci.h"
+#include "../host/xhci-plat.h"
+
+static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
+
+static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
+	.suspend_quirk = xhci_dwc3_suspend_quirk,
+};

we're passing data using device_properties, why do you want this here?

Quoted 18 lines

drivers/usb/dwc3/host.c +7 -0

@@ -115,6 +123,13 @@ int dwc3_host_init(struct dwc3 *dwc)
 		}
 	}
 
+	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
+			sizeof(struct xhci_plat_priv));
+	if (ret) {
+		dev_err(dwc->dev, "failed to add data to xHCI\n");
+		goto err;
+	}
+
 	ret = platform_device_add(xhci);
 	if (ret) {
 		dev_err(dwc->dev, "failed to register xHCI device\n");

@@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;
 }
 

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 157

Quoted 13 lines

drivers/usb/dwc3/host.c +13 -0

+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+
+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_mode = 0;
+
+	reg = readl(&xhci_hcd->cap_regs->hcs_params1);
+	num_ports = HCS_MAX_PORTS(reg);

there's a big assumption here that xhci is still alive. Why isn't this quirk implemented in xhci-plat itself?

Quoted 4 lines

Quoted 4 lines

drivers/usb/dwc3/host.c +0 -0

@@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;
 }
 

Quoted 16 lines

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 157

Quoted 13 lines

drivers/usb/dwc3/host.c +13 -0

+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+
+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_mode = 0;
+
+	reg = readl(&xhci_hcd->cap_regs->hcs_params1);
+	num_ports = HCS_MAX_PORTS(reg);

there's a big assumption here that xhci is still alive. Why isn't this quirk implemented in xhci-plat itself?

That should work for determining which types of devices are connected to the PHYs, however IIUC the xhci-plat doesn't know about the PHY topology. Are you suggesting to move that info into the xhci-plat driver so that it can set the corresponding PHY modes?

Hi,

Quoted 21 lines · Matthias Kaehlcke

Quoted 20 lines

Quoted 7 lines

drivers/usb/dwc3/host.c +3 -0

@@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;
 }
 
+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 157

Quoted 10 lines

drivers/usb/dwc3/host.c +10 -0

+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_mode = 0;
+
+	reg = readl(&xhci_hcd->cap_regs->hcs_params1);
+	num_ports = HCS_MAX_PORTS(reg);

there's a big assumption here that xhci is still alive. Why isn't this quirk implemented in xhci-plat itself?

Quoted 4 lines

That should work for determining which types of devices are connected to the PHYs, however IIUC the xhci-plat doesn't know about the PHY topology. Are you suggesting to move that info into the xhci-plat driver so that it can set the corresponding PHY modes?

Yes, if xHCI needs to know about PHYs in order to properly configure the PHYs, so be it :-)

Avoiding phy powerdown when wakeup capable devices are connected by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable devices such as keyboard and mouse.

Signed-off-by Sandeep Maheswaram <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
 drivers/usb/dwc3/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/usb/dwc3/core.c +0 -0

index b6e53d8..bb414c3 100644

Hi,

Quoted 11 lines · Sandeep Maheswaram

Avoiding phy powerdown when wakeup capable devices are connected by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable devices such as keyboard and mouse.

Signed-off-by Sandeep Maheswaram <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
  drivers/usb/dwc3/core.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1741

Quoted 10 lines

drivers/usb/dwc3/core.c +1 -1

index b6e53d8..bb414c3 100644
@@ -1738,7 +1738,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)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {

should be able to detect this generically, no? Shouldn't device_may_wakeup() be valid here and give you the answer you want?

Quoted 12 lines

Quoted 11 lines · Sandeep Maheswaram

Avoiding phy powerdown when wakeup capable devices are connected by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable devices such as keyboard and mouse.

Signed-off-by Sandeep Maheswaram <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
  drivers/usb/dwc3/core.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

Quoted 12 lines

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1741

Quoted 10 lines

drivers/usb/dwc3/core.c +1 -1

index b6e53d8..bb414c3 100644
@@ -1738,7 +1738,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)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {

should be able to detect this generically, no? Shouldn't device_may_wakeup() be valid here and give you the answer you want?

I think  device_may_wakeup() gives whether the controller is wake up capable or not.

But we want to keep phy powered on only when some wakeup capable devices (eg:keyboard ,mouse ) are connected to controller.

Hi,

Quoted 25 lines · Sandeep Maheswaram

Quoted 12 lines

Quoted 11 lines · Sandeep Maheswaram

Avoiding phy powerdown when wakeup capable devices are connected by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable devices such as keyboard and mouse.

Signed-off-by Sandeep Maheswaram <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
  drivers/usb/dwc3/core.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

Quoted 12 lines

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1741

Quoted 10 lines

drivers/usb/dwc3/core.c +1 -1

index b6e53d8..bb414c3 100644
@@ -1738,7 +1738,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)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {

should be able to detect this generically, no? Shouldn't device_may_wakeup() be valid here and give you the answer you want?

Quoted 2 lines

I think  device_may_wakeup() gives whether the controller is wake up capable or not.

Yes, but it's a bit more than that. Looking at devices.rst we read:

If :c:func:device_may_wakeup(dev) returns ``true``, the device should be prepared for generating hardware wakeup signals to trigger a system wakeup event when the system is in the sleep state. For example, :c:func:enable_irq_wake() might identify GPIO signals hooked up to a switch or other external hardware, and :c:func:pci_enable_wake() does something similar for the PCI PME signal.

So, if there is a condition where $this device has to, somehow, deal with wakeup, it should be configured accordingly. This ->phy_power_off flag is telling us the same thing.

Quoted 2 lines

But we want to keep phy powered on only when some wakeup capable devices (eg:keyboard ,mouse ) are connected to controller.

Understood, it could be that we're missing some method for propagating that state (i.e. keyboard with PM support) up to the parent device, but that's no excuse to bypass driver boundaries. Wouldn't you agree?

Quoted 41 lines · Felipe Balbi

Hi,

Quoted 24 lines · Sandeep Maheswaram

Quoted 12 lines

Quoted 11 lines · Sandeep Maheswaram

Avoiding phy powerdown when wakeup capable devices are connected by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable devices such as keyboard and mouse.

Signed-off-by Sandeep Maheswaram <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
   drivers/usb/dwc3/core.c | 7 +++++--
   1 file changed, 5 insertions(+), 2 deletions(-)

Quoted 12 lines

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1741

Quoted 10 lines

drivers/usb/dwc3/core.c +1 -1

index b6e53d8..bb414c3 100644
@@ -1738,7 +1738,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)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {

should be able to detect this generically, no? Shouldn't device_may_wakeup() be valid here and give you the answer you want?

Quoted 2 lines

I think  device_may_wakeup() gives whether the controller is wake up capable or not.

Yes, but it's a bit more than that. Looking at devices.rst we read:

If :c:func:device_may_wakeup(dev) returns ``true``, the device should be prepared for generating hardware wakeup signals to trigger a system wakeup event when the system is in the sleep state. For example, :c:func:enable_irq_wake() might identify GPIO signals hooked up to a switch or other external hardware, and :c:func:pci_enable_wake() does something similar for the PCI PME signal.

So, if there is a condition where $this device has to, somehow, deal with wakeup, it should be configured accordingly. This ->phy_power_off flag is telling us the same thing.

Quoted 5 lines

Quoted 2 lines

But we want to keep phy powered on only when some wakeup capable devices (eg:keyboard ,mouse ) are connected to controller.

Understood, it could be that we're missing some method for propagating that state (i.e. keyboard with PM support) up to the parent device, but that's no excuse to bypass driver boundaries. Wouldn't you agree?

I'm not sure if device_may_wakeup() is really the right tool for the job. This is the current implementation:

static inline bool device_may_wakeup(struct device *dev)

{
	return dev->power.can_wakeup && !!dev->power.wakeup;
}

IIUC power.can_wakeup specifies whether the device is wakeup capable, primarily in physical terms and indicating that the driver is ready to handle wakeups, and power.wakeup represents the policy which can be changed by userspace.

Supposing the hub is generally wakeup capable that flag shouldn't be changed. Neither should be the policy based on what is connected to the bus.

Configure interrupts based on hs_phy_mode to avoid triggering of interrupts during system suspend and suspend the device successfully.

Signed-off-by Sandeep Maheswaram <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/dwc3/dwc3-qcom.c +0 -0

index 66183c6..82125bc 100644

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 <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/usb/dwc3/dwc3-qcom.c +4 -0

index 82125bc..1e220af 100644
@@ -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;

Hi,

Quoted 31 lines · Sandeep Maheswaram

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 <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/usb/dwc3/dwc3-qcom.c +4 -0

index 82125bc..1e220af 100644
@@ -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;

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) · line 370

Quoted 10 lines

drivers/usb/dwc3/dwc3-qcom.c +6 -0

+	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.

Quoted 47 lines

Quoted 31 lines · Sandeep Maheswaram

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 <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/usb/dwc3/dwc3-qcom.c +4 -0

index 82125bc..1e220af 100644
@@ -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;

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) · line 370

Quoted 10 lines

drivers/usb/dwc3/dwc3-qcom.c +6 -0

+	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.

Quoted 3 lines

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?

Hi,

Quoted 51 lines · Matthias Kaehlcke

Quoted 47 lines

Quoted 31 lines · Sandeep Maheswaram

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 <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/usb/dwc3/dwc3-qcom.c +4 -0

index 82125bc..1e220af 100644
@@ -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;

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) · line 370

Quoted 10 lines

drivers/usb/dwc3/dwc3-qcom.c +6 -0

+	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.

Quoted 3 lines

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.

Quoted 6 lines

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?