[PATCH v9 0/5] USB DWC3 host wake up support from system suspend

[PATCH v9 0/5] USB DWC3 host wake up support from system suspend

[Sandeep Maheswaram]

Avoiding phy powerdown in host mode when wakeup capable devices are 
connected, 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.

Changes in v9:
Checking with device_may_makeup property instead of phy_power_off flag.
Changed the IRQ flags and removed hs_phy_mode variable.

Changes in v8:
Moved the dwc3 suspend quirk code in dwc3/host.c to xhci-plat.c
Checking phy_power_off flag instead of usb_wakeup_enabled_descendants 
to keep gdsc active.

Changes in v7:
Change in commit text and message in PATCH 1/5 and PATCH 5/5
as per Matthias suggestion.
Added curly braces for if and else if sections in PATCH 4/5.

Changes in v6:
Addressed comments in host.c and core.c
Separated the patches in dwc3-qcom.c to make it simple.
Dropped wakeup-source change as it is not related to this series.

Changes in v5:
Added phy_power_off flag to check presence of wakeup capable devices.
Dropped patch[v4,4/5] as it is present linux-next.
Addressed comments in host.c and dwc3-qcom.c.

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: host: xhci: plat: Add suspend quirk for dwc3 controller
  usb: dwc3: core: Host wake up support from system suspend
  usb: dwc3: qcom: Add helper functions to enable,disable wake irqs
  usb: dwc3: qcom: Change the IRQ flag for DP/DM hs phy irq
  usb: dwc3: qcom: Keep power domain on to support wakeup

 drivers/usb/dwc3/core.c      |  7 +++--
 drivers/usb/dwc3/dwc3-qcom.c | 73 +++++++++++++++++++++++---------------------
 drivers/usb/host/xhci-plat.c | 12 ++++++++
 3 files changed, 56 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH v9 1/5] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Sandeep Maheswaram]

During suspend check if any wakeup capable devices are connected to the
controller (directly or through hubs), and set the wakeup capable property
for xhci plat device.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
 drivers/usb/host/xhci-plat.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c1edcc9..7ab272b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev)
 	return 0;
 }
 
+static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev)
+{
+	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+		device_set_wakeup_capable(dev, true);
+	else
+		device_set_wakeup_capable(dev, false);
+}
+
 static int __maybe_unused xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
@@ -440,6 +448,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 	ret = xhci_priv_suspend_quirk(hcd);
 	if (ret)
 		return ret;
+
+	if (of_device_is_compatible(dev->parent->of_node, "snps,dwc3"))
+		xhci_dwc3_suspend_quirk(hcd, dev);
+
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
 	 * to do wakeup during suspend.
-- 
2.7.4

Re: [PATCH v9 1/5] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Matthias Kaehlcke]

Hi Sandeep,

On Mon, Nov 01, 2021 at 01:23:40PM +0530, Sandeep Maheswaram wrote:
> During suspend check if any wakeup capable devices are connected to the
> controller (directly or through hubs), and set the wakeup capable property
> for xhci plat device.
> 
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> ---
>  drivers/usb/host/xhci-plat.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c1edcc9..7ab272b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev)
> +{
> +	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> +		device_set_wakeup_capable(dev, true);
> +	else
> +		device_set_wakeup_capable(dev, false);

IIUC wakeup capability is typically a static property that reflects the
actual hardware (or firmware) support for wakeup. In that sense it doesn't
seem a good idea to change it dynamically at suspend time, depending on
what is connected to the bus. I understand though that the odd split
of the dwc3 driver makes it hard to do things properly ...

Earlier in this discussion Felipe suggested to add a function like
device_children_wakeup_capable(), to avoid having to call
usb_wakeup_enabled_descendants() from the dwc3 drivers.

Below is an initial implementation for device_children_wakeup_capable(),
could you try if calling it from dwc3_suspend/resume_common() and
dwc3_qcom_suspend() would work instead of relying on the wakeup
capability?

Thanks

Matthias

From 97c838334045ed67c3943f8e035ac70acd12b89b Mon Sep 17 00:00:00 2001
From: Matthias Kaehlcke <mka@chromium.org>
Date: Mon, 1 Nov 2021 11:37:19 -0700
Subject: [PATCH] PM / wakeup: Add device_children_wakeup_capable()

Add device_children_wakeup_capable() which checks whether the device itself
or one if its descendants is wakeup capable.

Change-Id: Ib359eb5ac8650ddf9889c7d1f77707f50777fe99
Suggested-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/base/power/wakeup.c | 17 +++++++++++++++++
 include/linux/pm_wakeup.h   |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 9c0932603642..2aee7fa8230f 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -483,6 +483,23 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
 
+static int __device_children_wakeup_capable(struct device *dev, void *dummy)
+{
+	return device_may_wakeup(dev) ||
+		device_for_each_child(dev, NULL, __device_children_wakeup_capable);
+}
+
+/**
+ * device_children_wakeup_capable - Check whether a device or one of its descendants is
+ *                                  wakeup capable.
+ * @dev: Device to handle.
+ */
+bool device_children_wakeup_capable(struct device *dev)
+{
+	return __device_children_wakeup_capable(dev, NULL);
+}
+EXPORT_SYMBOL_GPL(device_children_wakeup_capable);
+
 /**
  * wakeup_source_not_registered - validate the given wakeup source.
  * @ws: Wakeup source to be validated.
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 661efa029c96..a0ca42aac6d6 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -97,6 +97,7 @@ extern int device_wakeup_disable(struct device *dev);
 extern void device_set_wakeup_capable(struct device *dev, bool capable);
 extern int device_init_wakeup(struct device *dev, bool val);
 extern int device_set_wakeup_enable(struct device *dev, bool enable);
+extern bool device_children_wakeup_capable(struct device *dev);
 extern void __pm_stay_awake(struct wakeup_source *ws);
 extern void pm_stay_awake(struct device *dev);
 extern void __pm_relax(struct wakeup_source *ws);
@@ -167,6 +168,11 @@ static inline bool device_may_wakeup(struct device *dev)
 
 static inline void device_set_wakeup_path(struct device *dev) {}
 
+static inline bool device_children_wakeup_capable(struct device *dev)
+{
+	return false;
+}
+
 static inline void __pm_stay_awake(struct wakeup_source *ws) {}
 
 static inline void pm_stay_awake(struct device *dev) {}

Re: [PATCH v9 1/5] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Bjorn Andersson]

On Mon 01 Nov 11:59 PDT 2021, Matthias Kaehlcke wrote:

> Hi Sandeep,
> 
> On Mon, Nov 01, 2021 at 01:23:40PM +0530, Sandeep Maheswaram wrote:
> > During suspend check if any wakeup capable devices are connected to the
> > controller (directly or through hubs), and set the wakeup capable property
> > for xhci plat device.
> > 
> > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index c1edcc9..7ab272b 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	return 0;
> >  }
> >  
> > +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev)
> > +{
> > +	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> > +		device_set_wakeup_capable(dev, true);
> > +	else
> > +		device_set_wakeup_capable(dev, false);
> 
> IIUC wakeup capability is typically a static property that reflects the
> actual hardware (or firmware) support for wakeup. In that sense it doesn't
> seem a good idea to change it dynamically at suspend time, depending on
> what is connected to the bus. I understand though that the odd split
> of the dwc3 driver makes it hard to do things properly ...
> 
> Earlier in this discussion Felipe suggested to add a function like
> device_children_wakeup_capable(), to avoid having to call
> usb_wakeup_enabled_descendants() from the dwc3 drivers.
> 
> Below is an initial implementation for device_children_wakeup_capable(),
> could you try if calling it from dwc3_suspend/resume_common() and
> dwc3_qcom_suspend() would work instead of relying on the wakeup
> capability?
> 
> Thanks
> 
> Matthias
> 
> From 97c838334045ed67c3943f8e035ac70acd12b89b Mon Sep 17 00:00:00 2001
> From: Matthias Kaehlcke <mka@chromium.org>
> Date: Mon, 1 Nov 2021 11:37:19 -0700
> Subject: [PATCH] PM / wakeup: Add device_children_wakeup_capable()
> 
> Add device_children_wakeup_capable() which checks whether the device itself
> or one if its descendants is wakeup capable.
> 
> Change-Id: Ib359eb5ac8650ddf9889c7d1f77707f50777fe99
> Suggested-by: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Looks neat and useful.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

(Without the Change-Id of course...)

Regards,
Bjorn

> ---
>  drivers/base/power/wakeup.c | 17 +++++++++++++++++
>  include/linux/pm_wakeup.h   |  6 ++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 9c0932603642..2aee7fa8230f 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -483,6 +483,23 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>  
> +static int __device_children_wakeup_capable(struct device *dev, void *dummy)
> +{
> +	return device_may_wakeup(dev) ||
> +		device_for_each_child(dev, NULL, __device_children_wakeup_capable);
> +}
> +
> +/**
> + * device_children_wakeup_capable - Check whether a device or one of its descendants is
> + *                                  wakeup capable.
> + * @dev: Device to handle.
> + */
> +bool device_children_wakeup_capable(struct device *dev)
> +{
> +	return __device_children_wakeup_capable(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(device_children_wakeup_capable);
> +
>  /**
>   * wakeup_source_not_registered - validate the given wakeup source.
>   * @ws: Wakeup source to be validated.
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 661efa029c96..a0ca42aac6d6 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -97,6 +97,7 @@ extern int device_wakeup_disable(struct device *dev);
>  extern void device_set_wakeup_capable(struct device *dev, bool capable);
>  extern int device_init_wakeup(struct device *dev, bool val);
>  extern int device_set_wakeup_enable(struct device *dev, bool enable);
> +extern bool device_children_wakeup_capable(struct device *dev);
>  extern void __pm_stay_awake(struct wakeup_source *ws);
>  extern void pm_stay_awake(struct device *dev);
>  extern void __pm_relax(struct wakeup_source *ws);
> @@ -167,6 +168,11 @@ static inline bool device_may_wakeup(struct device *dev)
>  
>  static inline void device_set_wakeup_path(struct device *dev) {}
>  
> +static inline bool device_children_wakeup_capable(struct device *dev)
> +{
> +	return false;
> +}
> +
>  static inline void __pm_stay_awake(struct wakeup_source *ws) {}
>  
>  static inline void pm_stay_awake(struct device *dev) {}

Re: [PATCH v9 1/5] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Matthias Kaehlcke]

On Mon, Nov 01, 2021 at 01:50:31PM -0700, Bjorn Andersson wrote:
> On Mon 01 Nov 11:59 PDT 2021, Matthias Kaehlcke wrote:
> 
> > Hi Sandeep,
> > 
> > On Mon, Nov 01, 2021 at 01:23:40PM +0530, Sandeep Maheswaram wrote:
> > > During suspend check if any wakeup capable devices are connected to the
> > > controller (directly or through hubs), and set the wakeup capable property
> > > for xhci plat device.
> > > 
> > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > ---
> > >  drivers/usb/host/xhci-plat.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > index c1edcc9..7ab272b 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev)
> > >  	return 0;
> > >  }
> > >  
> > > +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev)
> > > +{
> > > +	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> > > +		device_set_wakeup_capable(dev, true);
> > > +	else
> > > +		device_set_wakeup_capable(dev, false);
> > 
> > IIUC wakeup capability is typically a static property that reflects the
> > actual hardware (or firmware) support for wakeup. In that sense it doesn't
> > seem a good idea to change it dynamically at suspend time, depending on
> > what is connected to the bus. I understand though that the odd split
> > of the dwc3 driver makes it hard to do things properly ...
> > 
> > Earlier in this discussion Felipe suggested to add a function like
> > device_children_wakeup_capable(), to avoid having to call
> > usb_wakeup_enabled_descendants() from the dwc3 drivers.
> > 
> > Below is an initial implementation for device_children_wakeup_capable(),
> > could you try if calling it from dwc3_suspend/resume_common() and
> > dwc3_qcom_suspend() would work instead of relying on the wakeup
> > capability?
> > 
> > Thanks
> > 
> > Matthias
> > 
> > From 97c838334045ed67c3943f8e035ac70acd12b89b Mon Sep 17 00:00:00 2001
> > From: Matthias Kaehlcke <mka@chromium.org>
> > Date: Mon, 1 Nov 2021 11:37:19 -0700
> > Subject: [PATCH] PM / wakeup: Add device_children_wakeup_capable()
> > 
> > Add device_children_wakeup_capable() which checks whether the device itself
> > or one if its descendants is wakeup capable.
> > 
> > Change-Id: Ib359eb5ac8650ddf9889c7d1f77707f50777fe99
> > Suggested-by: Felipe Balbi <balbi@kernel.org>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> Looks neat and useful.
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Thanks!

> (Without the Change-Id of course...)

Sure, I usually use patman to send patches upstream, which filters the
Change-Id if present, forgot to remove it when copying and pasting the
patch manually.

[PATCH v9 2/5] usb: dwc3: core: Host wake up support from system suspend

[Sandeep Maheswaram]

Avoiding phy powerdown when wakeup capable devices are connected
by checking wakeup property of xhci plat device.
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 <quic_c_sanm@quicinc.com>
---
 drivers/usb/dwc3/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 643239d..a6ad0ed 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1787,7 +1787,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) && !device_may_wakeup(&dwc->xhci->dev)) {
 			dwc3_core_exit(dwc);
 			break;
 		}
@@ -1848,13 +1848,16 @@ 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)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;
 			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
 			break;
+		} else {
+			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)
-- 
2.7.4

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

[Brian Norris]

On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown when wakeup capable devices are connected
> by checking wakeup property of xhci plat device.
> 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 <quic_c_sanm@quicinc.com>

Because you've now factored in a device_may_wakeup() (which evaluates
'false' for me), this no longer breaks my Rockchip RK3399 systems
(previous versions did). So from that angle:

Tested-by: Brian Norris <briannorris@chromium.org>

> ---
>  drivers/usb/dwc3/core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 643239d..a6ad0ed 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1787,7 +1787,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) && !device_may_wakeup(&dwc->xhci->dev)) {

I still find it odd to use device_may_wakeup(), since that's something
controlled by user space (sysfs) and doesn't fully factor in hardware
support. But that's what xhci-plat.c is doing, so I guess I see why
you're imitating it...
...still, feels wrong to me. But so does a lot of how dwc3 works.

Brian

>  			dwc3_core_exit(dwc);
>  			break;
>  		}
> @@ -1848,13 +1848,16 @@ 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)) {
> +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
>  			ret = dwc3_core_init_for_resume(dwc);
>  			if (ret)
>  				return ret;
>  			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>  			break;
> +		} else {
> +			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)
> -- 
> 2.7.4
> 

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

[Matthias Kaehlcke]

On Tue, Nov 16, 2021 at 04:28:16PM -0800, Brian Norris wrote:
> On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> > Avoiding phy powerdown when wakeup capable devices are connected
> > by checking wakeup property of xhci plat device.
> > 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 <quic_c_sanm@quicinc.com>
> 
> Because you've now factored in a device_may_wakeup() (which evaluates
> 'false' for me), this no longer breaks my Rockchip RK3399 systems
> (previous versions did). So from that angle:
> 
> Tested-by: Brian Norris <briannorris@chromium.org>
> 
> > ---
> >  drivers/usb/dwc3/core.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 643239d..a6ad0ed 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1787,7 +1787,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) && !device_may_wakeup(&dwc->xhci->dev)) {
> 
> I still find it odd to use device_may_wakeup(), since that's something
> controlled by user space (sysfs) and doesn't fully factor in hardware
> support. But that's what xhci-plat.c is doing, so I guess I see why
> you're imitating it...
> ...still, feels wrong to me. But so does a lot of how dwc3 works.

device_may_wakeup() actually factors in hardware support, at least if the
driver does the right thing (TM). The (current) implementation is:

static inline bool device_may_wakeup(struct device *dev)
{
	return dev->power.can_wakeup && !!dev->power.wakeup;
}

'.can_wakeup' should describe the hardware capability to wake up and the
other flag whether wakeup is enabled (which can be altered by userspace).

What this series currently does with the .can_wakeup flag is still wrong
though IMO. Patch "[1/5] usb: host: xhci: plat: Add suspend quirk for dwc3
controller" [1] dynamically sets the flag with a value that depends on what
is connected to the bus, so it doesn't specify any longer whether the
hardware supports wakeup or not.

[1] https://patchwork.kernel.org/project/linux-usb/patch/1635753224-23975-2-git-send-email-quic_c_sanm@quicinc.com/

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

[Brian Norris]

On Tue, Nov 16, 2021 at 05:14:14PM -0800, Matthias Kaehlcke wrote:
> On Tue, Nov 16, 2021 at 04:28:16PM -0800, Brian Norris wrote:
> > On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> > > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
> > 
> > I still find it odd to use device_may_wakeup(), since that's something
> > controlled by user space (sysfs) and doesn't fully factor in hardware
> > support. But that's what xhci-plat.c is doing, so I guess I see why
> > you're imitating it...
> > ...still, feels wrong to me. But so does a lot of how dwc3 works.
> 
> device_may_wakeup() actually factors in hardware support, at least if the
> driver does the right thing (TM).

Well in theory, maybe. But the latter half of the sentence is the key :)

In particular, xhci-plat does the Wrong Thing before this series:

	device_set_wakeup_capable(&pdev->dev, true);

i.e., it doesn't factor any "capability" in at all; it just assumes it.

And per your thoughts below, it's still Wrong after this series.

> The (current) implementation is:
> 
> static inline bool device_may_wakeup(struct device *dev)
> {
> 	return dev->power.can_wakeup && !!dev->power.wakeup;
> }
> 
> '.can_wakeup' should describe the hardware capability to wake up and the
> other flag whether wakeup is enabled (which can be altered by userspace).
> 
> What this series currently does with the .can_wakeup flag is still wrong
> though IMO. Patch "[1/5] usb: host: xhci: plat: Add suspend quirk for dwc3
> controller" [1] dynamically sets the flag with a value that depends on what
> is connected to the bus, so it doesn't specify any longer whether the
> hardware supports wakeup or not.
> 
> [1] https://patchwork.kernel.org/project/linux-usb/patch/1635753224-23975-2-git-send-email-quic_c_sanm@quicinc.com/

I'm not sure either your patch nor Sandeep's patch really get at the
heart of my problem here, which is that neither dwc3 nor xhci-plat are
trying to reflect wakeup capability of the host controller at all. (And
if the host controller doesn't suppor wakeup, it doesn't really matter
what any of its children think.) It seems that
drivers/usb/dwc3/dwc3-imx8mp.c is the only one that actually sets the
correct wakeup flag at the level that we _really_ know what's up -- the
platform/"glue" driver.

Maybe we need to do a little of both: teach the glue drivers (e.g.,
dwc3-qcom.c) to reflect their wakeup capability properly, and then look
at *that* capability (as well as any children, recursively, but only if
the glue driver supports it) when trying to make wakeup decisions.

It still feels wrong that there are 3 separate "can wakeup"
determinations for the host controller though: 1 dwc3-{glue}, 1
dwc3(core), and 1 xhci-plat. But maybe we have to hold our noses on that
one.

Brian

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

[Pavan Kondeti]

Hi Sandeep,

On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown when wakeup capable devices are connected
> by checking wakeup property of xhci plat device.
> 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 <quic_c_sanm@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 643239d..a6ad0ed 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1787,7 +1787,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) && !device_may_wakeup(&dwc->xhci->dev)) {
>  			dwc3_core_exit(dwc);
>  			break;
>  		}
> @@ -1848,13 +1848,16 @@ 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)) {
> +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
>  			ret = dwc3_core_init_for_resume(dwc);
>  			if (ret)
>  				return ret;
>  			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>  			break;
> +		} else {
> +			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>  		}
> +
During runtime resume, we enter else block and call dwc3_set_prtcap() which
is not done before. Is that intentional?

Thanks,
Pavan

[PATCH v9 3/5] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs

[Sandeep Maheswaram]

Adding helper functions to enable,disable wake irqs to make
the code simple and readable.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 58 ++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 9abbd01..54461f1 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 	icc_put(qcom->icc_path_apps);
 }
 
+static void dwc3_qcom_enable_wakeup_irq(int irq)
+{
+	if (!irq)
+		return;
+
+	enable_irq(irq);
+	enable_irq_wake(irq);
+}
+
+static void dwc3_qcom_disable_wakeup_irq(int irq)
+{
+	if (!irq)
+		return;
+
+	disable_irq_wake(irq);
+	disable_irq_nosync(irq);
+}
+
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
-	if (qcom->hs_phy_irq) {
-		disable_irq_wake(qcom->hs_phy_irq);
-		disable_irq_nosync(qcom->hs_phy_irq);
-	}
+	dwc3_qcom_disable_wakeup_irq(qcom->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);
-	}
+	dwc3_qcom_disable_wakeup_irq(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);
-	}
+	dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
 
-	if (qcom->ss_phy_irq) {
-		disable_irq_wake(qcom->ss_phy_irq);
-		disable_irq_nosync(qcom->ss_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);
-	}
+	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
 
-	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->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);
-	}
+	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)
-- 
2.7.4

Re: [PATCH v9 3/5] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs

[Matthias Kaehlcke]

Hi Felipe,

This patch is (supposedly) an improvement regardless of whether the rest of
the series lands or not. It hasn't changed in the last iterations nor did
it receive any comments. Can this be landed rather than carrying it around
until the rest of the series is ready?

Thanks

Matthias

On Mon, Nov 01, 2021 at 01:23:42PM +0530, Sandeep Maheswaram wrote:
> Adding helper functions to enable,disable wake irqs to make
> the code simple and readable.
> 
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 58 ++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 9abbd01..54461f1 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  	icc_put(qcom->icc_path_apps);
>  }
>  
> +static void dwc3_qcom_enable_wakeup_irq(int irq)
> +{
> +	if (!irq)
> +		return;
> +
> +	enable_irq(irq);
> +	enable_irq_wake(irq);
> +}
> +
> +static void dwc3_qcom_disable_wakeup_irq(int irq)
> +{
> +	if (!irq)
> +		return;
> +
> +	disable_irq_wake(irq);
> +	disable_irq_nosync(irq);
> +}
> +
>  static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>  {
> -	if (qcom->hs_phy_irq) {
> -		disable_irq_wake(qcom->hs_phy_irq);
> -		disable_irq_nosync(qcom->hs_phy_irq);
> -	}
> +	dwc3_qcom_disable_wakeup_irq(qcom->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);
> -	}
> +	dwc3_qcom_disable_wakeup_irq(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);
> -	}
> +	dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
>  
> -	if (qcom->ss_phy_irq) {
> -		disable_irq_wake(qcom->ss_phy_irq);
> -		disable_irq_nosync(qcom->ss_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);
> -	}
> +	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
>  
> -	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->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);
> -	}
> +	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)
> -- 
> 2.7.4
> 

Re: [PATCH v9 3/5] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs

[Bjorn Andersson]

On Mon 01 Nov 00:53 PDT 2021, Sandeep Maheswaram wrote:

> Adding helper functions to enable,disable wake irqs to make
> the code simple and readable.
> 
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 58 ++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 9abbd01..54461f1 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  	icc_put(qcom->icc_path_apps);
>  }
>  
> +static void dwc3_qcom_enable_wakeup_irq(int irq)
> +{
> +	if (!irq)
> +		return;
> +
> +	enable_irq(irq);
> +	enable_irq_wake(irq);
> +}
> +
> +static void dwc3_qcom_disable_wakeup_irq(int irq)
> +{
> +	if (!irq)
> +		return;
> +
> +	disable_irq_wake(irq);

Now that you touch this code path.

I presume keeping these interrupts enabled during runtime would cause
the interrupt to fire during normal operation, but why do we need to
toggle the irq_wake? Can't we just leave that flag set always?

> +	disable_irq_nosync(irq);
> +}
> +
>  static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>  {
> -	if (qcom->hs_phy_irq) {
> -		disable_irq_wake(qcom->hs_phy_irq);
> -		disable_irq_nosync(qcom->hs_phy_irq);
> -	}
> +	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);

Why don't we just replace *_phy_irq with an array and turn these two
function into two loops? That seems to be quite suitable for the
multi-port dwc found in e.g. sc8180x as well...

Regards,
Bjorn

>  
> -	if (qcom->dp_hs_phy_irq) {
> -		disable_irq_wake(qcom->dp_hs_phy_irq);
> -		disable_irq_nosync(qcom->dp_hs_phy_irq);
> -	}
> +	dwc3_qcom_disable_wakeup_irq(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);
> -	}
> +	dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
>  
> -	if (qcom->ss_phy_irq) {
> -		disable_irq_wake(qcom->ss_phy_irq);
> -		disable_irq_nosync(qcom->ss_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);
> -	}
> +	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
>  
> -	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->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);
> -	}
> +	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)
> -- 
> 2.7.4
> 

[PATCH v9 4/5] usb: dwc3: qcom: Change the IRQ flag for DP/DM hs phy irq

[Sandeep Maheswaram]

Change the IRQ flags for DP/DM hs phy irq to avoid interrupt
triggering during system suspend.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 54461f1..356f4f8 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -473,7 +473,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
 					qcom_dwc3_resume_irq,
-					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					"qcom_dwc3 DP_HS", qcom);
 		if (ret) {
 			dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
@@ -488,7 +488,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
 					qcom_dwc3_resume_irq,
-					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					"qcom_dwc3 DM_HS", qcom);
 		if (ret) {
 			dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
-- 
2.7.4

Re: [PATCH v9 4/5] usb: dwc3: qcom: Change the IRQ flag for DP/DM hs phy irq

[Bjorn Andersson]

On Mon 01 Nov 00:53 PDT 2021, Sandeep Maheswaram wrote:

> Change the IRQ flags for DP/DM hs phy irq to avoid interrupt
> triggering during system suspend.
> 

Why does replacing HIGH with RISING change this behavior, or do you get
a RISING interrupt just before hitting suspend which you ignore?

I think it would be nice to have the commit message for this (or per
below request the DTS change) include some details about what's really
happening on the irq line.

> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 54461f1..356f4f8 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -473,7 +473,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>  		irq_set_status_flags(irq, IRQ_NOAUTOEN);
>  		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>  					qcom_dwc3_resume_irq,
> -					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

IRQF_TRIGGER_* should be omitted from the driver and supplied by the DT.

The dtbs out there should all have IRQ_TYPE_LEVEL_HIGH at this time, so
simply dropping that from this list and updating the dts would be the
right thing to do.

Regards,
Bjorn

>  					"qcom_dwc3 DP_HS", qcom);
>  		if (ret) {
>  			dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
> @@ -488,7 +488,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>  		irq_set_status_flags(irq, IRQ_NOAUTOEN);
>  		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>  					qcom_dwc3_resume_irq,
> -					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  					"qcom_dwc3 DM_HS", qcom);
>  		if (ret) {
>  			dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
> -- 
> 2.7.4
> 

Re: [PATCH v9 4/5] usb: dwc3: qcom: Change the IRQ flag for DP/DM hs phy irq

[Sandeep Maheswaram]

On 11/1/2021 10:01 PM, Bjorn Andersson wrote:
> On Mon 01 Nov 00:53 PDT 2021, Sandeep Maheswaram wrote:
>
>> Change the IRQ flags for DP/DM hs phy irq to avoid interrupt
>> triggering during system suspend.
>>
> Why does replacing HIGH with RISING change this behavior, or do you get
> a RISING interrupt just before hitting suspend which you ignore?
>
> I think it would be nice to have the commit message for this (or per
> below request the DTS change) include some details about what's really
> happening on the irq line.

When we use IRQF_TRIGGER_HIGH we get interrupt during PM suspend and 
causes resume.

[  119.743083] Resume caused by IRQ 101, qcom_dwc3 DP_HS

>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
>> ---
>>   drivers/usb/dwc3/dwc3-qcom.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 54461f1..356f4f8 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -473,7 +473,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>>   		irq_set_status_flags(irq, IRQ_NOAUTOEN);
>>   		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>>   					qcom_dwc3_resume_irq,
>> -					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>> +					IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> IRQF_TRIGGER_* should be omitted from the driver and supplied by the DT.
>
> The dtbs out there should all have IRQ_TYPE_LEVEL_HIGH at this time, so
> simply dropping that from this list and updating the dts would be the
> right thing to do.
>
> Regards,
> Bjorn
Ok. Dropping IRQF_TRIGGER_*  solved the resume issue during PM suspend.
>>   					"qcom_dwc3 DP_HS", qcom);
>>   		if (ret) {
>>   			dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
>> @@ -488,7 +488,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>>   		irq_set_status_flags(irq, IRQ_NOAUTOEN);
>>   		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>>   					qcom_dwc3_resume_irq,
>> -					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>> +					IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>   					"qcom_dwc3 DM_HS", qcom);
>>   		if (ret) {
>>   			dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
>> -- 
>> 2.7.4
>>

[PATCH v9 5/5] usb: dwc3: qcom: Keep power domain on to support wakeup

[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 <quic_c_sanm@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 356f4f8..23bf2c1 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -17,6 +17,7 @@
 #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>
@@ -340,10 +341,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 {
 	u32 val;
 	int i, ret;
+	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 (device_may_wakeup(&dwc->xhci->dev) && dwc->xhci)
+		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");
@@ -367,10 +373,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;
+
 	if (device_may_wakeup(qcom->dev))
 		dwc3_qcom_disable_interrupts(qcom);
 
-- 
2.7.4