Skip to contentSkip to conversation

usb: dwc3: Host wake up support from system suspend

PATCH Superseded Sandeep Maheswaram 4 participants 12 messages Jul 8, 2020 – Aug 13, 2020

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

v2 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Sandeep Maheswaram

author
#0 lore raw

v2 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 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 (3):
usb: dwc3: core: Host wake up support from system suspend
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

 arch/arm64/boot/dts/qcom/sc7180.dtsi |  8 ++--
 drivers/usb/dwc3/core.c              | 47 +++++++++++++++++++----
 drivers/usb/dwc3/core.h              |  2 +
 drivers/usb/dwc3/dwc3-qcom.c         | 73 ++++++++++++++++++++++++++++--------
 4 files changed, 103 insertions(+), 27 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 v2 1/3 usb: dwc3: core: Host wake up support from system suspend

Avoiding phy powerdown in host mode so that it can be wake up by devices. Added need_phy_for_wakeup flag to distinugush resume path and hs_phy_flags to check connection status and set phy mode and configure interrupts.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/core.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 42 insertions(+), 7 deletions(-)
drivers/usb/dwc3/core.c +40 -7
index 25c686a7..eb7c225 100644
@@ -31,12 +31,14 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/of.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/hcd.h>
 
 #include "core.h"
 #include "gadget.h"
 #include "io.h"
 
 #include "debug.h"
+#include "../host/xhci.h"
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 

@@ -1627,10 +1629,36 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 	return ret;
 }
 
+static void dwc3_set_phy_speed_flags(struct dwc3 *dwc)
+{
+
+	int i, num_ports;
+	u32 reg;
+	struct usb_hcd	*hcd = platform_get_drvdata(dwc->xhci);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_flags &= ~(PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_LS);
+
+	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_flags |= PHY_MODE_USB_HOST_HS;
+			else if (DEV_LOWSPEED(reg))
+				dwc->hs_phy_flags |= PHY_MODE_USB_HOST_LS;
+		}
+	}
+	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_flags);
+}
+
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
 	u32 reg;
+	struct usb_hcd  *hcd = platform_get_drvdata(dwc->xhci);
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:

@@ -1643,9 +1671,12 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_set_phy_speed_flags(dwc);
 		if (!PMSG_IS_AUTO(msg)) {
-			dwc3_core_exit(dwc);
-			break;
+			if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+				dwc->need_phy_for_wakeup = true;
+			else
+				dwc->need_phy_for_wakeup = false;
 		}
 
 		/* Let controller to suspend HSPHY before PHY driver suspends */

@@ -1705,11 +1736,13 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		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;
+			if (!dwc->need_phy_for_wakeup) {
+				ret = dwc3_core_init_for_resume(dwc);
+				if (ret)
+					return ret;
+				dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+				break;
+			}
 		}
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
drivers/usb/dwc3/core.h +2 -0
index 013f42a..5367d510e 100644
@@ -1094,6 +1094,8 @@ struct dwc3 {
 	struct phy		*usb3_generic_phy;
 
 	bool			phys_ready;
+	bool                    need_phy_for_wakeup;
+	unsigned int            hs_phy_flags;
 
 	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
build bot
#2 lore raw

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

Thank you for the patch! Yet something to improve:

Hi Sandeep,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on balbi-usb/testing/next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sandeep-Maheswaram/usb-dwc3-Host-wake-up-support-from-system-suspend/20200709-031939
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: i386-randconfig-a012-20200713 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

ld: drivers/usb/dwc3/core.o: in function `dwc3_suspend_common':

>> core.c:(.text+0x3f0f): undefined reference to `usb_hcd_is_primary_hcd'
>> ld: core.c:(.text+0x40c7): undefined reference to `usb_wakeup_enabled_descendants'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#3 lore raw

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

Quoted 17 lines · kernel test robot

Hi Sandeep,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on balbi-usb/testing/next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sandeep-Maheswaram/usb-dwc3-Host-wake-up-support-from-system-suspend/20200709-031939
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: i386-randconfig-a012-20200713 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386
Quoted 8 lines

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

ld: drivers/usb/dwc3/core.o: in function `dwc3_suspend_common':

Quoted 2 lines
Quoted 2 lines

core.c:(.text+0x3f0f): undefined reference to `usb_hcd_is_primary_hcd'
ld: core.c:(.text+0x40c7): undefined reference to `usb_wakeup_enabled_descendants'

The problem here seems to be that:

CONFIG_USB_DWC3=y

and

CONFIG_USB=m

Add a Kconfig condition that disallows usb_dwc3 to be builtin when the USB core is built as a module?

#4 lore raw

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

In reply to Sandeep Maheswaram

Hi Sandeep,

Quoted 32 lines · Sandeep Maheswaram

Avoiding phy powerdown in host mode so that it can be wake up by devices. Added need_phy_for_wakeup flag to distinugush resume path and hs_phy_flags to check connection status and set phy mode and configure interrupts.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/core.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 42 insertions(+), 7 deletions(-)
drivers/usb/dwc3/core.c +2 -0
index 25c686a7..eb7c225 100644
@@ -31,12 +31,14 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/of.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/hcd.h>
 
 #include "core.h"
 #include "gadget.h"
 #include "io.h"
 
 #include "debug.h"
+#include "../host/xhci.h"
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 

@@ -1627,10 +1629,36 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 	return ret;
 }

drivers/usb/dwc3/core.c static int dwc3_core_init_for_resume(struct dwc3 *dwc) · line 1640

Quoted 10 lines
drivers/usb/dwc3/core.c +9 -0
 
+static void dwc3_set_phy_speed_flags(struct dwc3 *dwc)
+{
+
+	int i, num_ports;
+	u32 reg;
+	struct usb_hcd	*hcd = platform_get_drvdata(dwc->xhci);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_flags &= ~(PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_LS);

Where is hs_phy_flags initialized? As far as I can tell it isn't, hence when dwc3_set_phy_speed_flags() is executed the first time it is 0 (from devm_kzalloc()), and after the '&=' it is still 0. The next time it will have whatever value it was set to in the below loop, which is then cleared by the '&='. It seems you could as well just write 'dwc->hs_phy_flags = 0', which is clearer, unless the field is used in some other way that isn't obvious to me.

drivers/usb/dwc3/core.c static int dwc3_core_init_for_resume(struct dwc3 *dwc) · line 1651

Quoted 11 lines
drivers/usb/dwc3/core.c +11 -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_flags |= PHY_MODE_USB_HOST_HS;
+			else if (DEV_LOWSPEED(reg))
+				dwc->hs_phy_flags |= PHY_MODE_USB_HOST_LS;

Is another entry for DEV_SUPERSPEED needed?

Quoted 29 lines
drivers/usb/dwc3/core.c +11 -2
+		}
+	}
+	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_flags);
+}
+
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
 	u32 reg;
+	struct usb_hcd  *hcd = platform_get_drvdata(dwc->xhci);
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:

@@ -1643,9 +1671,12 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_set_phy_speed_flags(dwc);
 		if (!PMSG_IS_AUTO(msg)) {
-			dwc3_core_exit(dwc);
-			break;
+			if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+				dwc->need_phy_for_wakeup = true;
+			else
+				dwc->need_phy_for_wakeup = false;
 		}
 
 		/* Let controller to suspend HSPHY before PHY driver suspends */

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

Quoted 10 lines
drivers/usb/dwc3/core.c +2 -5
@@ -1705,11 +1736,13 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		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;
+			if (!dwc->need_phy_for_wakeup) {
+				ret = dwc3_core_init_for_resume(dwc);

Before this patch we had the combo dwc3_core_exit() / dwc3_core_init_for_resume(), now it is only dwc3_core_init_for_resume() for !dwc->need_phy_for_wakeup. Doesn't this cause trouble with enable counts, e.g. with clk_bulk_prepare_enable() being called in dwc3_core_init_for_resume(), without the corresponding clk_bulk_disable_unprepare() calls in dwc3_core_exit()?

#5 lore raw

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

In reply to Sandeep Maheswaram
Quoted 105 lines · Sandeep Maheswaram

Avoiding phy powerdown in host mode so that it can be wake up by devices. Added need_phy_for_wakeup flag to distinugush resume path and hs_phy_flags to check connection status and set phy mode and configure interrupts.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/core.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/dwc3/core.h |  2 ++
 2 files changed, 42 insertions(+), 7 deletions(-)
drivers/usb/dwc3/core.c +40 -7
index 25c686a7..eb7c225 100644
@@ -31,12 +31,14 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/of.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/hcd.h>
 
 #include "core.h"
 #include "gadget.h"
 #include "io.h"
 
 #include "debug.h"
+#include "../host/xhci.h"
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 

@@ -1627,10 +1629,36 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 	return ret;
 }
 
+static void dwc3_set_phy_speed_flags(struct dwc3 *dwc)
+{
+
+	int i, num_ports;
+	u32 reg;
+	struct usb_hcd	*hcd = platform_get_drvdata(dwc->xhci);
+	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
+
+	dwc->hs_phy_flags &= ~(PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_LS);
+
+	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_flags |= PHY_MODE_USB_HOST_HS;
+			else if (DEV_LOWSPEED(reg))
+				dwc->hs_phy_flags |= PHY_MODE_USB_HOST_LS;
+		}
+	}
+	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_flags);
+}
+
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
 	u32 reg;
+	struct usb_hcd  *hcd = platform_get_drvdata(dwc->xhci);
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:

@@ -1643,9 +1671,12 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_set_phy_speed_flags(dwc);
 		if (!PMSG_IS_AUTO(msg)) {
-			dwc3_core_exit(dwc);
-			break;
+			if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+				dwc->need_phy_for_wakeup = true;
+			else
+				dwc->need_phy_for_wakeup = false;
 		}
 
 		/* Let controller to suspend HSPHY before PHY driver suspends */

@@ -1705,11 +1736,13 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		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;
+			if (!dwc->need_phy_for_wakeup) {
+				ret = dwc3_core_init_for_resume(dwc);
+				if (ret)
+					return ret;
+				dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+				break;
+			}
 		}
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
drivers/usb/dwc3/core.h +0 -0
index 013f42a..5367d510e 100644

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

Quoted 8 lines
drivers/usb/dwc3/core.h +2 -0
@@ -1094,6 +1094,8 @@ struct dwc3 {
 	struct phy		*usb3_generic_phy;
 
 	bool			phys_ready;
+	bool                    need_phy_for_wakeup;
+	unsigned int            hs_phy_flags;
 
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;

Should this include a check for the 'wakeup-source' DT attribute as in xhci-mtk.c, to make wakeup support optional?

author
#6 lore raw

PATCH v2 2/3 usb: dwc3: qcom: Configure wakeup interrupts and set genpd active wakeup flag

In reply to Sandeep Maheswaram

configure interrupts based on hs_phy_flag. Set genpd active wakeup flag for usb gdsc if wakeup capable devices are connected.

Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 73 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 16 deletions(-)
drivers/usb/dwc3/dwc3-qcom.c +57 -16
index 1dfd024..8902670 100644
@@ -16,9 +16,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"
 

@@ -192,21 +194,34 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
 	if (qcom->hs_phy_irq) {
 		disable_irq_wake(qcom->hs_phy_irq);
 		disable_irq_nosync(qcom->hs_phy_irq);
 	}
+	if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
+		if (qcom->dp_hs_phy_irq) {
+			disable_irq_wake(qcom->dp_hs_phy_irq);
+			disable_irq_nosync(qcom->dp_hs_phy_irq);
+		}
+	} else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
+		if (qcom->dm_hs_phy_irq) {
+			disable_irq_wake(qcom->dm_hs_phy_irq);
+			disable_irq_nosync(qcom->dm_hs_phy_irq);
+		}
+	} else {
 
-	if (qcom->dp_hs_phy_irq) {
-		disable_irq_wake(qcom->dp_hs_phy_irq);
-		disable_irq_nosync(qcom->dp_hs_phy_irq);
-	}
+		if (qcom->dp_hs_phy_irq) {
+			disable_irq_wake(qcom->dp_hs_phy_irq);
+			disable_irq_nosync(qcom->dp_hs_phy_irq);
+		}
 
-	if (qcom->dm_hs_phy_irq) {
-		disable_irq_wake(qcom->dm_hs_phy_irq);
-		disable_irq_nosync(qcom->dm_hs_phy_irq);
+		if (qcom->dm_hs_phy_irq) {
+			disable_irq_wake(qcom->dm_hs_phy_irq);
+			disable_irq_nosync(qcom->dm_hs_phy_irq);
+		}
 	}
-
 	if (qcom->ss_phy_irq) {
 		disable_irq_wake(qcom->ss_phy_irq);
 		disable_irq_nosync(qcom->ss_phy_irq);

@@ -215,21 +230,34 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 {
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
 	if (qcom->hs_phy_irq) {
 		enable_irq(qcom->hs_phy_irq);
 		enable_irq_wake(qcom->hs_phy_irq);
 	}
+	if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
+		if (qcom->dp_hs_phy_irq) {
+			enable_irq(qcom->dp_hs_phy_irq);
+			enable_irq_wake(qcom->dp_hs_phy_irq);
+		}
+	} else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
+		if (qcom->dm_hs_phy_irq) {
+			enable_irq(qcom->dm_hs_phy_irq);
+			enable_irq_wake(qcom->dm_hs_phy_irq);
+		}
+	} else {
 
-	if (qcom->dp_hs_phy_irq) {
-		enable_irq(qcom->dp_hs_phy_irq);
-		enable_irq_wake(qcom->dp_hs_phy_irq);
-	}
+		if (qcom->dp_hs_phy_irq) {
+			enable_irq(qcom->dp_hs_phy_irq);
+			enable_irq_wake(qcom->dp_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 (qcom->dm_hs_phy_irq) {
+			enable_irq(qcom->dm_hs_phy_irq);
+			enable_irq_wake(qcom->dm_hs_phy_irq);
+		}
 	}
-
 	if (qcom->ss_phy_irq) {
 		enable_irq(qcom->ss_phy_irq);
 		enable_irq_wake(qcom->ss_phy_irq);

@@ -240,6 +268,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 {
 	u32 val;
 	int i;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct usb_hcd  *hcd = platform_get_drvdata(dwc->xhci);
+	struct generic_pm_domain *genpd;
+
+	genpd = pd_to_genpd(qcom->dev->pm_domain);
+
+	if (genpd && usb_wakeup_enabled_descendants(hcd->self.root_hub))
+		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
 
 	if (qcom->is_suspended)
 		return 0;

@@ -261,6 +297,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 {
 	int ret;
 	int i;
+	struct generic_pm_domain *genpd;
+
+	genpd = pd_to_genpd(qcom->dev->pm_domain);
+	if (genpd)
+		genpd->flags &= !GENPD_FLAG_ACTIVE_WAKEUP;
 
 	if (!qcom->is_suspended)
 		return 0;
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
build bot
#7 lore raw

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

Thank you for the patch! Perhaps something to improve:

Hi Sandeep,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on balbi-usb/testing/next]
[also build test WARNING on robh/for-next v5.8-rc4 next-20200708] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sandeep-Maheswaram/usb-dwc3-Host-wake-up-support-from-system-suspend/20200709-031939
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/usb/dwc3/dwc3-qcom.c:304:20: warning: converting the result of '<<' to a boolean always evaluates to true [-Wtautological-constant-compare]
                   genpd->flags &= !GENPD_FLAG_ACTIVE_WAKEUP;
                                    ^
   include/linux/pm_domain.h:62:38: note: expanded from macro 'GENPD_FLAG_ACTIVE_WAKEUP'
   #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
                                        ^

1 warning generated.

vim +304 drivers/usb/dwc3/dwc3-qcom.c

295
296 static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
297 {
298 int ret;
299 int i;
300 struct generic_pm_domain *genpd;
301
302 genpd = pd_to_genpd(qcom->dev->pm_domain);
303 if (genpd)

 > 304			genpd->flags &= !GENPD_FLAG_ACTIVE_WAKEUP;

305
306 if (!qcom->is_suspended)
307 return 0;
308
309 dwc3_qcom_disable_interrupts(qcom);
310
311 for (i = 0; i < qcom->num_clocks; i++) {
312 ret = clk_prepare_enable(qcom->clks[i]);
313 if (ret < 0) {
314 while (--i >= 0)
315 clk_disable_unprepare(qcom->clks[i]);
316 return ret;
317 }
318 }
319
320 /* Clear existing events from PHY related to L2 in/out */
321 dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
322 PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
323
324 qcom->is_suspended = false;
325
326 return 0;
327 }
328

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#8 lore raw

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

In reply to Sandeep Maheswaram

Quoting Sandeep Maheswaram (2020-07-08 12:10:16)

Quoted 1 line

configure interrupts based on hs_phy_flag. Set genpd active wakeup flag

Please capitalize the start of a sentence. What is 'hs_phy_flag'?

Quoted 1 line

for usb gdsc if wakeup capable devices are connected.

This tells us what is happening in the code but doesn't tell us the important part, i.e. _why_ this patch is important. Why do we need to set the genpd active wakeup flag? Why configure interrupt based on hs_phy_flag, whatever that is.

Quoted 40 lines
Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 73 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 16 deletions(-)
drivers/usb/dwc3/dwc3-qcom.c +17 -4
index 1dfd024..8902670 100644
@@ -192,21 +194,34 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
+       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
        if (qcom->hs_phy_irq) {
                disable_irq_wake(qcom->hs_phy_irq);
                disable_irq_nosync(qcom->hs_phy_irq);
        }
+       if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
+               if (qcom->dp_hs_phy_irq) {
+                       disable_irq_wake(qcom->dp_hs_phy_irq);
+                       disable_irq_nosync(qcom->dp_hs_phy_irq);
+               }
+       } else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
+               if (qcom->dm_hs_phy_irq) {
+                       disable_irq_wake(qcom->dm_hs_phy_irq);
+                       disable_irq_nosync(qcom->dm_hs_phy_irq);
+               }
+       } else {
 
-       if (qcom->dp_hs_phy_irq) {
-               disable_irq_wake(qcom->dp_hs_phy_irq);
-               disable_irq_nosync(qcom->dp_hs_phy_irq);
-       }
+               if (qcom->dp_hs_phy_irq) {
+                       disable_irq_wake(qcom->dp_hs_phy_irq);
+                       disable_irq_nosync(qcom->dp_hs_phy_irq);
+               }

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

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +4 -4
 
-       if (qcom->dm_hs_phy_irq) {
-               disable_irq_wake(qcom->dm_hs_phy_irq);
-               disable_irq_nosync(qcom->dm_hs_phy_irq);
+               if (qcom->dm_hs_phy_irq) {
+                       disable_irq_wake(qcom->dm_hs_phy_irq);
+                       disable_irq_nosync(qcom->dm_hs_phy_irq);
+               }
        }
-

I liked the newline. Please keep it.

Quoted 36 lines
drivers/usb/dwc3/dwc3-qcom.c +17 -6
        if (qcom->ss_phy_irq) {
                disable_irq_wake(qcom->ss_phy_irq);
                disable_irq_nosync(qcom->ss_phy_irq);

@@ -215,21 +230,34 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 {
+       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
        if (qcom->hs_phy_irq) {
                enable_irq(qcom->hs_phy_irq);
                enable_irq_wake(qcom->hs_phy_irq);
        }
+       if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
+               if (qcom->dp_hs_phy_irq) {
+                       enable_irq(qcom->dp_hs_phy_irq);
+                       enable_irq_wake(qcom->dp_hs_phy_irq);
+               }
+       } else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
+               if (qcom->dm_hs_phy_irq) {
+                       enable_irq(qcom->dm_hs_phy_irq);
+                       enable_irq_wake(qcom->dm_hs_phy_irq);
+               }
+       } else {
 
-       if (qcom->dp_hs_phy_irq) {
-               enable_irq(qcom->dp_hs_phy_irq);
-               enable_irq_wake(qcom->dp_hs_phy_irq);
-       }
+               if (qcom->dp_hs_phy_irq) {
+                       enable_irq(qcom->dp_hs_phy_irq);
+                       enable_irq_wake(qcom->dp_hs_phy_irq);
+               }
 
-       if (qcom->dm_hs_phy_irq) {
-               enable_irq(qcom->dm_hs_phy_irq);

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

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +4 -2
-               enable_irq_wake(qcom->dm_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 (qcom->ss_phy_irq) {
                enable_irq(qcom->ss_phy_irq);
                enable_irq_wake(qcom->ss_phy_irq);

Can we use the wakeup irq support code in the kernel here? That would be preferred to having the driver enable and disable irq wake at various times when the irq is enabled and disabled (which is also odd by the way). Why can't we request the irqs and leave them enabled all the time? Also it seems like the binding should have 'wakeup-source' in it (see Documentation/devicetree/bindings/power/wakeup-source.txt for more info).

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

Quoted 11 lines
drivers/usb/dwc3/dwc3-qcom.c +7 -0
@@ -240,6 +268,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 {
        u32 val;
        int i;
+       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+       struct usb_hcd  *hcd = platform_get_drvdata(dwc->xhci);
+       struct generic_pm_domain *genpd;
+
+       genpd = pd_to_genpd(qcom->dev->pm_domain);
+
+       if (genpd && usb_wakeup_enabled_descendants(hcd->self.root_hub))

Feels like a comment would be good to explain why wakeup enabled descendants matters here.

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

Quoted 11 lines
drivers/usb/dwc3/dwc3-qcom.c +4 -0
+               genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
 
        if (qcom->is_suspended)
                return 0;

@@ -261,6 +297,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 {
        int ret;
        int i;
+       struct generic_pm_domain *genpd;
+
+       genpd = pd_to_genpd(qcom->dev->pm_domain);

This does container_of() so it can't return NULL.

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

Quoted 1 line
drivers/usb/dwc3/dwc3-qcom.c +1 -0
+       if (genpd)

So this check is wrong?

Quoted 1 line
drivers/usb/dwc3/dwc3-qcom.c +1 -0
+               genpd->flags &= !GENPD_FLAG_ACTIVE_WAKEUP;
#9 lore raw

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

In reply to Sandeep Maheswaram

Hi Sandeep,

Quoted 2 lines · Sandeep Maheswaram

configure interrupts based on hs_phy_flag. Set genpd active wakeup flag for usb gdsc if wakeup capable devices are connected.

as Stephen remarked, please describe why this patch is doing what it is doing.

Quoted 22 lines
Signed-off-by Sandeep Maheswaram <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 73 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 16 deletions(-)
drivers/usb/dwc3/dwc3-qcom.c +2 -0
index 1dfd024..8902670 100644
@@ -16,9 +16,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 int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom) · line 202

Quoted 9 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
@@ -192,21 +194,34 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
 	if (qcom->hs_phy_irq) {
 		disable_irq_wake(qcom->hs_phy_irq);
 		disable_irq_nosync(qcom->hs_phy_irq);
 	}

nit: add empty line

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

Quoted 12 lines
drivers/usb/dwc3/dwc3-qcom.c +11 -0
+	if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
+		if (qcom->dp_hs_phy_irq) {
+			disable_irq_wake(qcom->dp_hs_phy_irq);
+			disable_irq_nosync(qcom->dp_hs_phy_irq);
+		}
+	} else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
+		if (qcom->dm_hs_phy_irq) {
+			disable_irq_wake(qcom->dm_hs_phy_irq);
+			disable_irq_nosync(qcom->dm_hs_phy_irq);
+		}
+	} else {

delete empty line

Quoted 22 lines
drivers/usb/dwc3/dwc3-qcom.c +8 -8
-	if (qcom->dp_hs_phy_irq) {
-		disable_irq_wake(qcom->dp_hs_phy_irq);
-		disable_irq_nosync(qcom->dp_hs_phy_irq);
-	}
+		if (qcom->dp_hs_phy_irq) {
+			disable_irq_wake(qcom->dp_hs_phy_irq);
+			disable_irq_nosync(qcom->dp_hs_phy_irq);
+		}
 
-	if (qcom->dm_hs_phy_irq) {
-		disable_irq_wake(qcom->dm_hs_phy_irq);
-		disable_irq_nosync(qcom->dm_hs_phy_irq);
+		if (qcom->dm_hs_phy_irq) {
+			disable_irq_wake(qcom->dm_hs_phy_irq);
+			disable_irq_nosync(qcom->dm_hs_phy_irq);
+		}
 	}
-
 	if (qcom->ss_phy_irq) {
 		disable_irq_wake(qcom->ss_phy_irq);
 		disable_irq_nosync(qcom->ss_phy_irq);

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

Quoted 9 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
@@ -215,21 +230,34 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 {
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
 	if (qcom->hs_phy_irq) {
 		enable_irq(qcom->hs_phy_irq);
 		enable_irq_wake(qcom->hs_phy_irq);
 	}

nit: add empty line

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

Quoted 12 lines
drivers/usb/dwc3/dwc3-qcom.c +11 -0
+	if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
+		if (qcom->dp_hs_phy_irq) {
+			enable_irq(qcom->dp_hs_phy_irq);
+			enable_irq_wake(qcom->dp_hs_phy_irq);
+		}
+	} else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
+		if (qcom->dm_hs_phy_irq) {
+			enable_irq(qcom->dm_hs_phy_irq);
+			enable_irq_wake(qcom->dm_hs_phy_irq);
+		}
+	} else {

delete empty line

Quoted 22 lines
drivers/usb/dwc3/dwc3-qcom.c +8 -8
-	if (qcom->dp_hs_phy_irq) {
-		enable_irq(qcom->dp_hs_phy_irq);
-		enable_irq_wake(qcom->dp_hs_phy_irq);
-	}
+		if (qcom->dp_hs_phy_irq) {
+			enable_irq(qcom->dp_hs_phy_irq);
+			enable_irq_wake(qcom->dp_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 (qcom->dm_hs_phy_irq) {
+			enable_irq(qcom->dm_hs_phy_irq);
+			enable_irq_wake(qcom->dm_hs_phy_irq);
+		}
 	}
-
 	if (qcom->ss_phy_irq) {
 		enable_irq(qcom->ss_phy_irq);
 		enable_irq_wake(qcom->ss_phy_irq);

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

Quoted 8 lines
drivers/usb/dwc3/dwc3-qcom.c +5 -0
@@ -240,6 +268,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 {
 	u32 val;
 	int i;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct usb_hcd  *hcd = platform_get_drvdata(dwc->xhci);
+	struct generic_pm_domain *genpd; 
+
+	genpd = pd_to_genpd(qcom->dev->pm_domain);

nit: assign in declaration?

Quoted 7 lines
drivers/usb/dwc3/dwc3-qcom.c +3 -0
+
+	if (genpd && usb_wakeup_enabled_descendants(hcd->self.root_hub))
+		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
 
 	if (qcom->is_suspended)
 		return 0;

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

Quoted 8 lines
drivers/usb/dwc3/dwc3-qcom.c +5 -0
@@ -261,6 +297,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 {
 	int ret;
 	int i;
+	struct generic_pm_domain *genpd;
+
+	genpd = pd_to_genpd(qcom->dev->pm_domain);
+	if (genpd)
+		genpd->flags &= !GENPD_FLAG_ACTIVE_WAKEUP;

~GENPD_FLAG_ACTIVE_WAKEUP; ?

author
#10 lore raw

PATCH v2 3/3 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 xo shutdown.

Signed-off-by Sandeep Maheswaram <[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 2be81a2..2c220367 100644
@@ -2520,10 +2520,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
Reviewed-by on v2 3/3
#11 lore raw

Re: [PATCH v2 3/3] arm64: dts: qcom: sc7180: Use pdc interrupts for USB instead of GIC interrupts

Quoting Sandeep Maheswaram (2020-07-08 12:10:17)

Quoted 5 lines

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

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

---

Reviewed-by Stephen Boyd <[email protected]>

I suppose wakeup-source should be added in the board files.