Skip to contentSkip to conversation

USB DWC3 host wake up support from system suspend

PATCH Superseded Sandeep Maheswaram 4 participants 11 messages Oct 27, 2020 – Feb 25, 2021

Subsystems (6)
Device tree bindings USB Arm Qualcomm mailing list Arm Qualcomm support +2 more subsystems

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Sandeep Maheswaram

author
#0 lore raw

v4 cover USB DWC3 host wake up support from system suspend

Avoiding phy powerdown in host mode so that it can be wake up by devices. Set GENPD_FLAG_ACTIVE_WAKEUP flag to keep usb30_prim gdsc active when wakeup capable devices are connected to the host.
Using PDC interrupts instead of GIC interrupst to support wakeup in xo shutdown case.

Changes in v4:
Addressed Matthias comments raised in v3.

Changes in v3:
Removed need_phy_for_wakeup flag and by default avoiding phy powerdown. Addressed Matthias comments and added entry for DEV_SUPERSPEED. Added suspend_quirk in dwc3 host and moved the dwc3_set_phy_speed_flags. Added wakeup-source dt entry and reading in dwc-qcom.c glue driver.

Changes in v2:
Dropped the patch in clock to set GENPD_FLAG_ACTIVE_WAKEUP flag and setting in usb dwc3 driver.
Separated the core patch and glue driver patch.
Made need_phy_for_wakeup flag part of dwc structure and hs_phy_flags as unsgined int.
Adrressed the comment on device_init_wakeup call. Corrected offset for reading portsc register.
Added pacth to support wakeup in xo shutdown case.

Sandeep Maheswaram (5):
usb: dwc3: core: Host wake up support from system suspend
usb: dwc3: host: Add suspend_quirk for dwc3 host
usb: dwc3: qcom: Configure wakeup interrupts and set genpd active
wakeup flag
arm64: dts: qcom: sc7180: Use pdc interrupts for USB instead of GIC
interrupts
arm64: dts: qcom: sc7180: Add wakeup-source property for USB node in
IDP and trogdor

 arch/arm64/boot/dts/qcom/sc7180-idp.dts      |  1 +
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  1 +
 arch/arm64/boot/dts/qcom/sc7180.dtsi         |  8 +--
 drivers/usb/dwc3/core.c                      | 14 +----
 drivers/usb/dwc3/core.h                      |  2 +
 drivers/usb/dwc3/dwc3-qcom.c                 | 82 ++++++++++++++++++----------
 drivers/usb/dwc3/host.c                      | 51 +++++++++++++++++
 7 files changed, 114 insertions(+), 45 deletions(-)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
author
#1 lore raw

PATCH v4 1/5 usb: dwc3: core: Host wake up support from system suspend

Avoiding phy powerdown in host mode so that it can be woken up by devices. Added hs_phy_mode flag to check connection status and set phy mode and configure interrupts.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/core.c | 14 +++-----------
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 5 insertions(+), 11 deletions(-)
drivers/usb/dwc3/core.c +3 -11
index bdf0925..0e4bc1e 100644
@@ -1672,10 +1672,6 @@ 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)) {
-			dwc3_core_exit(dwc);
-			break;
-		}
 
 		/* Let controller to suspend HSPHY before PHY driver suspends */
 		if (dwc->dis_u2_susphy_quirk ||

@@ -1733,13 +1729,9 @@ 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)) {
-			ret = dwc3_core_init_for_resume(dwc);
-			if (ret)
-				return ret;
-			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
-			break;
-		}
+
+		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 		if (dwc->dis_u2_susphy_quirk)
drivers/usb/dwc3/core.h +2 -0
index 74323b1..da63d4a3 100644
@@ -1101,6 +1101,8 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#2 lore raw

Re: [PATCH v4 1/5] usb: dwc3: core: Host wake up support from system suspend

Hi,

Quoted 15 lines · Sandeep Maheswaram

Avoiding phy powerdown in host mode so that it can be woken up by devices. Added hs_phy_mode flag to check connection status and set phy mode and configure interrupts.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/core.c | 14 +++-----------
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 5 insertions(+), 11 deletions(-)
drivers/usb/dwc3/core.c +0 -0
index bdf0925..0e4bc1e 100644

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 · line 1679

Quoted 8 lines
drivers/usb/dwc3/core.c +0 -4
@@ -1672,10 +1672,6 @@ 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)) {
-			dwc3_core_exit(dwc);
-			break;
-		}

This could be a problem for platforms that don't support runtime_suspend and rely on dwc3_core_exit to power-down PHY.
IMO you can continue to do dwc3_core_exit() if runtime_pm isn't enabled for the device.

Quoted 26 lines
drivers/usb/dwc3/core.c +3 -7
 		/* Let controller to suspend HSPHY before PHY driver suspends */
 		if (dwc->dis_u2_susphy_quirk ||

@@ -1733,13 +1729,9 @@ 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)) {
-			ret = dwc3_core_init_for_resume(dwc);
-			if (ret)
-				return ret;
-			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
-			break;
-		}
+
+		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
drivers/usb/dwc3/core.c +0 -0
 		if (dwc->dis_u2_susphy_quirk)
drivers/usb/dwc3/core.h +0 -0
index 74323b1..da63d4a3 100644

drivers/usb/dwc3/core.h struct dwc3 { · line 1105

Quoted 5 lines
drivers/usb/dwc3/core.h +2 -0
@@ -1101,6 +1101,8 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+

This change should instead be part of the other patch ? "usb: dwc3: host: Add suspend_quirk for dwc3 host"

Quoted 2 lines
drivers/usb/dwc3/core.h +0 -0
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
#3 lore raw

Re: [PATCH v4 1/5] usb: dwc3: core: Host wake up support from system suspend

Quoted 26 lines · On Sat, Nov 28, 2020 at 08:01:31AM +0530, [email protected] wrote:

Hi,

Quoted 22 lines · Sandeep Maheswaram

Avoiding phy powerdown in host mode so that it can be woken up by devices.
Added hs_phy_mode flag to check connection status and set phy mode and configure interrupts.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/core.c | 14 +++-----------
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 5 insertions(+), 11 deletions(-)
drivers/usb/dwc3/core.c +0 -2
index bdf0925..0e4bc1e 100644
@@ -1672,10 +1672,6 @@ 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)) {
-			dwc3_core_exit(dwc);
Quoted 8 lines

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 · line 1679

Quoted 2 lines
drivers/usb/dwc3/core.c +0 -2
-			break;
-		}

This could be a problem for platforms that don't support runtime_suspend and rely on dwc3_core_exit to power-down PHY.
IMO you can continue to do dwc3_core_exit() if runtime_pm isn't enabled for the device.

This would imply that USB wakeup isn't supported on such platforms, right?

Could we have a flag that the dwc3-<glue> driver can set to indicate that wakeup is enabled for this suspend cycle to omit the PHY power-down?

Quoted 27 lines
Quoted 27 lines
drivers/usb/dwc3/core.c +3 -7
 		/* Let controller to suspend HSPHY before PHY driver suspends */
 		if (dwc->dis_u2_susphy_quirk ||

@@ -1733,13 +1729,9 @@ 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)) {
-			ret = dwc3_core_init_for_resume(dwc);
-			if (ret)
-				return ret;
-			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
-			break;
-		}
+
+		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
drivers/usb/dwc3/core.c +0 -0
 		if (dwc->dis_u2_susphy_quirk)
drivers/usb/dwc3/core.h +0 -0
index 74323b1..da63d4a3 100644
@@ -1101,6 +1101,8 @@ struct dwc3 {
Quoted 7 lines

drivers/usb/dwc3/core.h struct dwc3 { · line 1105

Quoted 4 lines
drivers/usb/dwc3/core.h +2 -0
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+

This change should instead be part of the other patch ? "usb: dwc3: host: Add suspend_quirk for dwc3 host"

+1

author
#4 lore raw

PATCH v4 2/5 usb: dwc3: host: Add suspend_quirk for dwc3 host

In reply to Sandeep Maheswaram

Adding suspend quirk function for dwc3 host which will be called during xhci suspend.
Setting hs_phy_mode, ss_phy_mode flags and phy mode during host suspend.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/host.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
drivers/usb/dwc3/host.c +51 -0
index e195176..db03db8 100644
@@ -11,6 +11,13 @@
 #include <linux/platform_device.h>
 
 #include "core.h"
+#include "../host/xhci.h"
+#include "../host/xhci-plat.h"
+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,
+};
 
 static int dwc3_host_get_irq(struct dwc3 *dwc)
 {

@@ -115,6 +122,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 +141,43 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;
 }
 
+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+
+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct device *dev = hcd->self.controller;
+	struct dwc3 *dwc = dev_get_drvdata(dev->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);
+	for (i = 0; i < num_ports; i++) {
+		reg = readl(&xhci_hcd->op_regs->port_status_base + i * 0x04);
+		if (reg & PORT_PE) {
+			if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg))
+				dwc->hs_phy_mode |= PHY_MODE_USB_HOST_HS;
+			else if (DEV_LOWSPEED(reg))
+				dwc->hs_phy_mode |= PHY_MODE_USB_HOST_LS;
+
+			if (DEV_SUPERSPEED(reg))
+				ss_phy_mode |= PHY_MODE_USB_HOST_SS;
+		}
+	}
+	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_mode);
+	phy_set_mode(dwc->usb3_generic_phy, ss_phy_mode);
+}
+
+int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)
+{
+	dwc3_set_phy_mode(hcd);
+	return 0;
+}
+
 void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Reviewed-by on v4 2/5
#5 lore raw

Re: [PATCH v4 2/5] usb: dwc3: host: Add suspend_quirk for dwc3 host

Quoted 43 lines · Sandeep Maheswaram

Adding suspend quirk function for dwc3 host which will be called during xhci suspend.
Setting hs_phy_mode, ss_phy_mode flags and phy mode during host suspend.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/host.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
drivers/usb/dwc3/host.c +14 -0
index e195176..db03db8 100644
@@ -11,6 +11,13 @@
 #include <linux/platform_device.h>
 
 #include "core.h"
+#include "../host/xhci.h"
+#include "../host/xhci-plat.h"
+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,
+};
 
 static int dwc3_host_get_irq(struct dwc3 *dwc)
 {

@@ -115,6 +122,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 +141,43 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;

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

Quoted 10 lines
drivers/usb/dwc3/host.c +8 -0
 }
 
+static void dwc3_set_phy_mode(struct usb_hcd *hcd)
+{
+
+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct device *dev = hcd->self.controller;
+	struct dwc3 *dwc = dev_get_drvdata(dev->parent);

nit: the use of 'dev' seems misleading here. By convention 'dev' refers to the 'struct device' of the driver, here it refers to a child of that device. It's probably not super-important, but 'dev' is also not realy needed, you could omit it and change the assignment of 'dwc3' to:

	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
Quoted 22 lines
drivers/usb/dwc3/host.c +22 -0
+	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);
+	for (i = 0; i < num_ports; i++) {
+		reg = readl(&xhci_hcd->op_regs->port_status_base + i * 0x04);
+		if (reg & PORT_PE) {
+			if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg))
+				dwc->hs_phy_mode |= PHY_MODE_USB_HOST_HS;
+			else if (DEV_LOWSPEED(reg))
+				dwc->hs_phy_mode |= PHY_MODE_USB_HOST_LS;
+
+			if (DEV_SUPERSPEED(reg))
+				ss_phy_mode |= PHY_MODE_USB_HOST_SS;
+		}
+	}
+	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_mode);
+	phy_set_mode(dwc->usb3_generic_phy, ss_phy_mode);
+}

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

Quoted 10 lines
drivers/usb/dwc3/host.c +7 -0
+
+int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)
+{
+	dwc3_set_phy_mode(hcd);
+	return 0;
+}
+
 void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);

Other than the nit:

Reviewed-by Matthias Kaehlcke <[email protected]>
author
#6 lore raw

PATCH v4 3/5 usb: dwc3: qcom: Configure wakeup interrupts and set genpd active wakeup flag

In reply to Sandeep Maheswaram

Configure interrupts based on hs_phy_mode to avoid triggering of interrupts during system suspend and suspends successfully. Set genpd active wakeup flag for usb gdsc if wakeup capable devices are connected so that wake up happens without reenumeration. Add helper functions to enable,disable wake irqs.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 82 ++++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 30 deletions(-)
drivers/usb/dwc3/dwc3-qcom.c +52 -30
index c703d55..c93f7bb 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"
 

@@ -291,60 +293,75 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 	icc_put(qcom->icc_path_apps);
 }
 
-static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+static void dwc3_qcom_enable_wakeup_irq(int wake_irq)
 {
-	if (qcom->hs_phy_irq) {
-		disable_irq_wake(qcom->hs_phy_irq);
-		disable_irq_nosync(qcom->hs_phy_irq);
+	if (wake_irq) {
+		enable_irq(wake_irq);
+		enable_irq_wake(wake_irq);
 	}
+}
 
-	if (qcom->dp_hs_phy_irq) {
-		disable_irq_wake(qcom->dp_hs_phy_irq);
-		disable_irq_nosync(qcom->dp_hs_phy_irq);
+static void dwc3_qcom_disable_wakeup_irq(int wake_irq)
+{
+	if (wake_irq) {
+		disable_irq_wake(wake_irq);
+		disable_irq_nosync(wake_irq);
 	}
+}
 
-	if (qcom->dm_hs_phy_irq) {
-		disable_irq_wake(qcom->dm_hs_phy_irq);
-		disable_irq_nosync(qcom->dm_hs_phy_irq);
-	}
+static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+{
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
+	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
 
-	if (qcom->ss_phy_irq) {
-		disable_irq_wake(qcom->ss_phy_irq);
-		disable_irq_nosync(qcom->ss_phy_irq);
+	if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS)
+		dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+	else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS)
+		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+	else {
+		dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
 	}
+
+	dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
 }
 
 static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 {
-	if (qcom->hs_phy_irq) {
-		enable_irq(qcom->hs_phy_irq);
-		enable_irq_wake(qcom->hs_phy_irq);
-	}
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 
-	if (qcom->dp_hs_phy_irq) {
-		enable_irq(qcom->dp_hs_phy_irq);
-		enable_irq_wake(qcom->dp_hs_phy_irq);
-	}
+	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
 
-	if (qcom->dm_hs_phy_irq) {
-		enable_irq(qcom->dm_hs_phy_irq);
-		enable_irq_wake(qcom->dm_hs_phy_irq);
+	if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS)
+		dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+	else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS)
+		dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
+	else {
+		dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+		dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
 	}
 
-	if (qcom->ss_phy_irq) {
-		enable_irq(qcom->ss_phy_irq);
-		enable_irq_wake(qcom->ss_phy_irq);
-	}
+	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq);
 }
 
 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;
+	}
+
 	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
 	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
 		dev_err(qcom->dev, "HS-PHY not in L2\n");

@@ -366,10 +383,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 {
 	int ret;
 	int i;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
 
 	if (!qcom->is_suspended)
 		return 0;
 
+	if (dwc->xhci)
+		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;
+
 	dwc3_qcom_disable_interrupts(qcom);
 
 	for (i = 0; i < qcom->num_clocks; i++) {

@@ -764,7 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, of_property_read_bool(np, "wakeup-source"));
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#7 lore raw

Re: [PATCH v4 3/5] usb: dwc3: qcom: Configure wakeup interrupts and set genpd active wakeup flag

Quoted 28 lines · Sandeep Maheswaram

Configure interrupts based on hs_phy_mode to avoid triggering of interrupts during system suspend and suspends successfully. Set genpd active wakeup flag for usb gdsc if wakeup capable devices are connected so that wake up happens without reenumeration. Add helper functions to enable,disable wake irqs.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 82 ++++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 30 deletions(-)
drivers/usb/dwc3/dwc3-qcom.c +2 -0
index c703d55..c93f7bb 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"

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 296

Quoted 5 lines
drivers/usb/dwc3/dwc3-qcom.c +1 -1
@@ -291,60 +293,75 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 	icc_put(qcom->icc_path_apps);
 }
 
-static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+static void dwc3_qcom_enable_wakeup_irq(int wake_irq)

super-nit: from the name of the function it's evident that this is a wakeup irq, you could omit the 'wake_' prefix.

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 301

Quoted 8 lines
drivers/usb/dwc3/dwc3-qcom.c +3 -3
 {
-	if (qcom->hs_phy_irq) {
-		disable_irq_wake(qcom->hs_phy_irq);
-		disable_irq_nosync(qcom->hs_phy_irq);
+	if (wake_irq) {
+		enable_irq(wake_irq);
+		enable_irq_wake(wake_irq);
 	}

nit: the following would be a more common structure:

	if (!wake_irq)
		return;

	enable_irq(wake_irq);
	enable_irq_wake(wake_irq);

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 304

Quoted 6 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -3
+}
 
-	if (qcom->dp_hs_phy_irq) {
-		disable_irq_wake(qcom->dp_hs_phy_irq);
-		disable_irq_nosync(qcom->dp_hs_phy_irq);
+static void dwc3_qcom_disable_wakeup_irq(int wake_irq)

same as above

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 309

Quoted 5 lines
drivers/usb/dwc3/dwc3-qcom.c +4 -0
+{
+	if (wake_irq) {
+		disable_irq_wake(wake_irq);
+		disable_irq_nosync(wake_irq);
 	}

same as above

Quoted 13 lines
drivers/usb/dwc3/dwc3-qcom.c +6 -5
+}
 
-	if (qcom->dm_hs_phy_irq) {
-		disable_irq_wake(qcom->dm_hs_phy_irq);
-		disable_irq_nosync(qcom->dm_hs_phy_irq);
-	}
+static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+{
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
+	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
 
-	if (qcom->ss_phy_irq) {

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 325

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +7 -2
-		disable_irq_wake(qcom->ss_phy_irq);
-		disable_irq_nosync(qcom->ss_phy_irq);
+	if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS)
+		dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+	else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS)
+		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+	else {
+		dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
 	}

The following would be clearer IMO:

	if (dwc->hs_phy_mode & (PHY_MODE_USB_HOST_LS | PHY_MODE_USB_HOST_SS))
		dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
    	if (dwc->hs_phy_mode & (PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_SS))
		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 327

Quoted 2 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
+
+	dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);

Why isn't this conditional on 'dwc->hs_phy_mode & PHY_MODE_USB_HOST_SS'?

Quoted 23 lines
drivers/usb/dwc3/dwc3-qcom.c +6 -11
 }
 
 static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 {
-	if (qcom->hs_phy_irq) {
-		enable_irq(qcom->hs_phy_irq);
-		enable_irq_wake(qcom->hs_phy_irq);
-	}
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 
-	if (qcom->dp_hs_phy_irq) {
-		enable_irq(qcom->dp_hs_phy_irq);
-		enable_irq_wake(qcom->dp_hs_phy_irq);
-	}
+	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
 
-	if (qcom->dm_hs_phy_irq) {
-		enable_irq(qcom->dm_hs_phy_irq);
-		enable_irq_wake(qcom->dm_hs_phy_irq);
+	if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS)
+		dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+	else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS)
+		dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 345

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +4 -4
+	else {
+		dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+		dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
 	}
 
-	if (qcom->ss_phy_irq) {
-		enable_irq(qcom->ss_phy_irq);
-		enable_irq_wake(qcom->ss_phy_irq);
-	}
+	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq);

same comments as for dwc3_qcom_disable_interrupts()

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 353

Quoted 8 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
 }
 
 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;

nit: remove one blank

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) · line 362

Quoted 9 lines
drivers/usb/dwc3/dwc3-qcom.c +5 -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;

Shouldn't this only be done when wakeup is enabled?

Quoted 7 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
+	}
+
 	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
 	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
 		dev_err(qcom->dev, "HS-PHY not in L2\n");

@@ -366,10 +383,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 {

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

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +4 -0
 	int ret;
 	int i;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
 
 	if (!qcom->is_suspended)
 		return 0;
 
+	if (dwc->xhci)
+		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;

Could also depend on wakeup being enabled, but less important.

Quoted 17 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -1
+
 	dwc3_qcom_disable_interrupts(qcom);
 
 	for (i = 0; i < qcom->num_clocks; i++) {

@@ -764,7 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, of_property_read_bool(np, "wakeup-source"));
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
author
#8 lore raw

PATCH v4 4/5 arm64: dts: qcom: sc7180: Use pdc interrupts for USB instead of GIC interrupts

In reply to Sandeep Maheswaram

Using pdc interrupts for USB instead of GIC interrupts to support wake up in case of XO shutdown.

Signed-off-by Sandeep Maheswaram <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/boot/dts/qcom/sc7180.dtsi +4 -4
index a02776c..a2c56528 100644
@@ -2687,10 +2687,10 @@
 					  <&gcc GCC_USB30_PRIM_MASTER_CLK>;
 			assigned-clock-rates = <19200000>, <150000000>;
 
-			interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts-extended = <&intc GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 8 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 9 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "hs_phy_irq", "ss_phy_irq",
 					  "dm_hs_phy_irq", "dp_hs_phy_irq";
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#9 lore raw

Re: [PATCH v4 4/5] arm64: dts: qcom: sc7180: Use pdc interrupts for USB instead of GIC interrupts

Hi,

Quoted 16 lines · Sandeep Maheswaram

Using pdc interrupts for USB instead of GIC interrupts to support wake up in case of XO shutdown.

Signed-off-by Sandeep Maheswaram <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/boot/dts/qcom/sc7180.dtsi +0 -0
index a02776c..a2c56528 100644
@@ -2687,10 +2687,10 @@
                                          <&gcc GCC_USB30_PRIM_MASTER_CLK>;

arch/arm64/boot/dts/qcom/sc7180.dtsi line 2693

Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +4 -4
                        assigned-clock-rates = <19200000>, <150000000>;
 
-                       interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
+                       interrupts-extended = <&intc GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+                                             <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
+                                             <&pdc 8 IRQ_TYPE_LEVEL_HIGH>,
+                                             <&pdc 9 IRQ_TYPE_LEVEL_HIGH>;

Is there any reason that this patch can't land? I'm not sure what the current status of everything is, but it should be fine to go through the PDC anyway, right?

-Doug

author
#10 lore raw

PATCH v4 5/5 arm64: dts: qcom: sc7180: Add wakeup-source property for USB node in IDP and trogdor

In reply to Sandeep Maheswaram

Adding wakeup-source property for USB controller in SC7180 IDP and trogdor boards.

Signed-off-by Sandeep Maheswaram <[email protected]>
Reviewed-by Matthias Kaehlcke <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7180-idp.dts      | 1 +
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
 2 files changed, 2 insertions(+)
arch/arm64/boot/dts/qcom/sc7180-idp.dts +1 -0
index e77a792..fe1366a 100644
@@ -369,6 +369,7 @@
 
 &usb_1 {
 	status = "okay";
+	wakeup-source;
 };
 
 &usb_1_dwc3 {
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +1 -0
index bf87558..c320c09 100644
@@ -818,6 +818,7 @@ ap_spi_fp: &spi10 {
 
 &usb_1 {
 	status = "okay";
+	wakeup-source;
 };
 
 &usb_1_dwc3 {
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation