[PATCH v14 0/7] USB DWC3 host wake up support from system suspend

[PATCH v14 0/7] 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.
Keep usb30_prim gdsc active to retain controller status
during suspend/resume.

Changes in v14:
Added patch for device_children_wakeup_capable.
Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
Fixed minor nit picks in v13 reported by Matthias.

Changes in v13:
Moved the dt bindings patch to start.
Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
Check wakep-source property for dwc3 core node to set the
wakeup capability. Drop the device_init_wakeup call from
runtime suspend and resume.
Added GENPD_FLAG_RPM_ALWAYS_ON and set GENPD_FLAG_ALWAYS_ON if
wakeup is supported.

Changes in v12:
Squashed PATCH 1/5 and 2/5 of v11.
Added dt bindings and device tree entry for wakeup-source property
for dwc3 core node.
Dropped redundant phy_set_mode call.


Changes in v11:
Moving back to v8 version
https://patchwork.kernel.org/project/linux-arm-msm/cover/1624882097-23265-1-git-send-email-sanm@codeaurora.org
as we are getting interrupts during suspend
when enabling both DP hs phy irq and DM hs phy irq.
Moved the set phy mode function to dwc3/core.c from xhci-plat.c
We didn't find any other option other than accessing xhci from dwc.

Changes in v10:
PATCH 1/6: Change device_set_wakeup_capable to device_set_wakeup_enable
PATCH 2/6: Remove redundant else part in dwc3_resume_common
PATCH 4/6: Change the irg flags
PATCH 5/6: Set flag GENPD_FLAG_ALWAYS_ON
PATCH 6/6: Remove disable interrupts function and enable
interrupts in probe.


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.

Matthias Kaehlcke (1):
  PM / wakeup: Add device_children_wakeup_capable()

Sandeep Maheswaram (6):
  dt-bindings: usb: dwc3: Add wakeup-source property support
  usb: dwc3: core: Host wake up support from system suspend
  usb: dwc3: qcom: Add helper functions to enable,disable wake irqs
  usb: dwc3: qcom: Configure wakeup interrupts during suspend
  usb: dwc3: qcom: Keep power domain on to retain controller status
  arm64: dts: qcom: sc7280: Add wakeup-source property for USB node

 .../devicetree/bindings/usb/snps,dwc3.yaml         |  5 ++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |  1 +
 drivers/base/power/wakeup.c                        | 18 +++++
 drivers/usb/dwc3/core.c                            | 33 ++++----
 drivers/usb/dwc3/core.h                            |  4 +
 drivers/usb/dwc3/dwc3-qcom.c                       | 91 +++++++++++++---------
 drivers/usb/dwc3/host.c                            | 24 ++++++
 include/linux/pm_wakeup.h                          |  7 ++
 8 files changed, 133 insertions(+), 50 deletions(-)

-- 
2.7.4

[PATCH v14 1/7] dt-bindings: usb: dwc3: Add wakeup-source property support

[Sandeep Maheswaram]

Added support for wakeup-source property. This property can be
used to check and power down the phy during system suspend if
wake up is not supported.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index f4471f8..4d4de2f 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -341,6 +341,11 @@ properties:
       This port is used with the 'usb-role-switch' property  to connect the
       dwc3 to type C connector.
 
+  wakeup-source:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Enable USB remote wakeup.
+
 unevaluatedProperties: false
 
 required:
-- 
2.7.4

[PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Sandeep Maheswaram]

From: Matthias Kaehlcke <mka@chromium.org>

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

Suggested-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/base/power/wakeup.c | 18 ++++++++++++++++++
 include/linux/pm_wakeup.h   |  7 +++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a57d469..1900637 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -541,6 +541,24 @@ 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 196a157..9a3005b 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -109,6 +109,7 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws);
 extern int device_wakeup_enable(struct device *dev);
 extern int device_wakeup_disable(struct device *dev);
 extern void device_set_wakeup_capable(struct device *dev, bool capable);
+extern bool device_children_wakeup_capable(struct device *dev);
 extern int device_init_wakeup(struct device *dev, bool val);
 extern int device_set_wakeup_enable(struct device *dev, bool enable);
 extern void __pm_stay_awake(struct wakeup_source *ws);
@@ -186,6 +187,12 @@ static inline bool device_wakeup_path(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) {}
-- 
2.7.4

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Rafael J. Wysocki]

On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
<quic_c_sanm@quicinc.com> wrote:
>
> From: Matthias Kaehlcke <mka@chromium.org>
>
> Add device_children_wakeup_capable() which checks whether the device itself
> or one if its descendants is wakeup capable.

device_wakeup_path() exists for a very similar purpose.

Is it not usable for whatever you need the new function introduced here?

> Suggested-by: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/base/power/wakeup.c | 18 ++++++++++++++++++
>  include/linux/pm_wakeup.h   |  7 +++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index a57d469..1900637 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -541,6 +541,24 @@ 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 196a157..9a3005b 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -109,6 +109,7 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws);
>  extern int device_wakeup_enable(struct device *dev);
>  extern int device_wakeup_disable(struct device *dev);
>  extern void device_set_wakeup_capable(struct device *dev, bool capable);
> +extern bool device_children_wakeup_capable(struct device *dev);
>  extern int device_init_wakeup(struct device *dev, bool val);
>  extern int device_set_wakeup_enable(struct device *dev, bool enable);
>  extern void __pm_stay_awake(struct wakeup_source *ws);
> @@ -186,6 +187,12 @@ static inline bool device_wakeup_path(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) {}
> --
> 2.7.4
>

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Matthias Kaehlcke]

On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> <quic_c_sanm@quicinc.com> wrote:
> >
> > From: Matthias Kaehlcke <mka@chromium.org>
> >
> > Add device_children_wakeup_capable() which checks whether the device itself
> > or one if its descendants is wakeup capable.
> 
> device_wakeup_path() exists for a very similar purpose.
> 
> Is it not usable for whatever you need the new function introduced here?

I wasn't aware of it's function, there are no doc comments and the
name isn't really self explanatory.

In a quick test device_wakeup_path() returned inconsistent values for the
root hub, sometimes true, others false when a wakeup capable USB device was
connected.

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Pavan Kondeti]

Hi Matthias,

On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > <quic_c_sanm@quicinc.com> wrote:
> > >
> > > From: Matthias Kaehlcke <mka@chromium.org>
> > >
> > > Add device_children_wakeup_capable() which checks whether the device itself
> > > or one if its descendants is wakeup capable.
> > 
> > device_wakeup_path() exists for a very similar purpose.
> > 
> > Is it not usable for whatever you need the new function introduced here?
> 
> I wasn't aware of it's function, there are no doc comments and the
> name isn't really self explanatory.
> 
> In a quick test device_wakeup_path() returned inconsistent values for the
> root hub, sometimes true, others false when a wakeup capable USB device was
> connected.

We will also test the same to double confirm the behavior of
device_wakeup_path(). I am assuming that you checked device_wakeup_path()
only during system suspend path.

Here is what I understood by looking at __device_suspend(). Please share
your thoughts on this.

power.wakeup_path is set to true for the parent *after* a wakeup capable
device is suspended. This means when the root hub(s) is suspended, it is
propagated to xhci-plat and when xhci-plat is suspended, it is propagated
to dwc3. bottom up propgation during system suspend.

I believe we can directly check something like this in the dwc3 driver
instead of having another wrapper like device_children_wakeup_capable().

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1170b80..a783257 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
 		if (!PMSG_IS_AUTO(msg)) {
+			/*
+			 * Don't kill the host when dwc3 is wakeup capable and
+			 * its children needs wakeup.
+			 */
+			if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
+				handle_it();
+		} else {
 			dwc3_core_exit(dwc);
-			break;
 		}
 
 		/* Let controller to suspend HSPHY before PHY driver suspends */

Thanks,
Pavan

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Pavan Kondeti]

Hi Matthias,

On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> Hi Matthias,
> 
> On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > <quic_c_sanm@quicinc.com> wrote:
> > > >
> > > > From: Matthias Kaehlcke <mka@chromium.org>
> > > >
> > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > or one if its descendants is wakeup capable.
> > > 
> > > device_wakeup_path() exists for a very similar purpose.
> > > 
> > > Is it not usable for whatever you need the new function introduced here?
> > 
> > I wasn't aware of it's function, there are no doc comments and the
> > name isn't really self explanatory.
> > 
> > In a quick test device_wakeup_path() returned inconsistent values for the
> > root hub, sometimes true, others false when a wakeup capable USB device was
> > connected.
> 
> We will also test the same to double confirm the behavior of
> device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> only during system suspend path.
> 
> Here is what I understood by looking at __device_suspend(). Please share
> your thoughts on this.
> 
> power.wakeup_path is set to true for the parent *after* a wakeup capable
> device is suspended. This means when the root hub(s) is suspended, it is
> propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> to dwc3. bottom up propgation during system suspend.
> 
> I believe we can directly check something like this in the dwc3 driver
> instead of having another wrapper like device_children_wakeup_capable().
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 1170b80..a783257 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		if (!PMSG_IS_AUTO(msg)) {
> +			/*
> +			 * Don't kill the host when dwc3 is wakeup capable and
> +			 * its children needs wakeup.
> +			 */
> +			if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> +				handle_it();
> +		} else {
>  			dwc3_core_exit(dwc);
> -			break;
>  		}
>  
>  		/* Let controller to suspend HSPHY before PHY driver suspends */
> 

device_wakeup_path(dwc->dev) is returning true all the time irrespective of
the wakeup capability (and enabled status) of the connected USB devices. That
is because xhci-plat device is configured to wakeup all the time. Since the
child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
device_children_wakeup_capable() will also suffer the problem. However,

device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
patch is using. That is not correct. we have two root hubs (HS and SS) associated
with a USB3 controller and calling it on one root hub is incorrect. 
device_children_wakeup_capable() must be called on xhci-plat so that it covers
both HS and SS root hubs

I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
that the wakeup path is correctly propagated to dwc3. something like below.
Does it make sense to you?

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 649ffd8..be0c55b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	int ret;
 
+	if (!device_wakeup_path(dev))
+		device_wakeup_disable(dev);
+
 	if (pm_runtime_suspended(dev))
 		pm_runtime_resume(dev);
 
@@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 
+	device_wakeup_enable(dev);
+
 	return 0;
 }
 
Thanks,
Pavan

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Matthias Kaehlcke]

Hi Pavan,

On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> Hi Matthias,
> 
> On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > Hi Matthias,
> > 
> > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > <quic_c_sanm@quicinc.com> wrote:
> > > > >
> > > > > From: Matthias Kaehlcke <mka@chromium.org>
> > > > >
> > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > or one if its descendants is wakeup capable.
> > > > 
> > > > device_wakeup_path() exists for a very similar purpose.
> > > > 
> > > > Is it not usable for whatever you need the new function introduced here?
> > > 
> > > I wasn't aware of it's function, there are no doc comments and the
> > > name isn't really self explanatory.
> > > 
> > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > connected.
> > 
> > We will also test the same to double confirm the behavior of
> > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > only during system suspend path.
> > 
> > Here is what I understood by looking at __device_suspend(). Please share
> > your thoughts on this.
> > 
> > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > device is suspended. This means when the root hub(s) is suspended, it is
> > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > to dwc3. bottom up propgation during system suspend.
> > 
> > I believe we can directly check something like this in the dwc3 driver
> > instead of having another wrapper like device_children_wakeup_capable().
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 1170b80..a783257 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> >  		if (!PMSG_IS_AUTO(msg)) {
> > +			/*
> > +			 * Don't kill the host when dwc3 is wakeup capable and
> > +			 * its children needs wakeup.
> > +			 */
> > +			if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > +				handle_it();
> > +		} else {
> >  			dwc3_core_exit(dwc);
> > -			break;
> >  		}
> >  
> >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > 
> 
> device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> the wakeup capability (and enabled status) of the connected USB devices. That
> is because xhci-plat device is configured to wakeup all the time. Since the
> child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> device_children_wakeup_capable() will also suffer the problem. However,
> 
> device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> patch is using. That is not correct. we have two root hubs (HS and SS) associated
> with a USB3 controller and calling it on one root hub is incorrect. 
> device_children_wakeup_capable() must be called on xhci-plat so that it covers
> both HS and SS root hubs

Thanks for pointing that out!

> I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> that the wakeup path is correctly propagated to dwc3. something like below.
> Does it make sense to you?
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 649ffd8..be0c55b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	int ret;
>  
> +	if (!device_wakeup_path(dev))
> +		device_wakeup_disable(dev);
> +
>  	if (pm_runtime_suspended(dev))
>  		pm_runtime_resume(dev);
>  
> @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
>  
> +	device_wakeup_enable(dev);

I think this also needs to be done conditionally, otherwise it would
create a new wake source on every resume when wakeup is already
enabled.

Other than that this seems to do the trick and keeps the USB layer out of
the dwc3 code.

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Pavan Kondeti]

Hi Matthias,

On Fri, Apr 29, 2022 at 12:19:22PM -0700, Matthias Kaehlcke wrote:
> Hi Pavan,
> 
> On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> > Hi Matthias,
> > 
> > On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > > Hi Matthias,
> > > 
> > > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > > <quic_c_sanm@quicinc.com> wrote:
> > > > > >
> > > > > > From: Matthias Kaehlcke <mka@chromium.org>
> > > > > >
> > > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > > or one if its descendants is wakeup capable.
> > > > > 
> > > > > device_wakeup_path() exists for a very similar purpose.
> > > > > 
> > > > > Is it not usable for whatever you need the new function introduced here?
> > > > 
> > > > I wasn't aware of it's function, there are no doc comments and the
> > > > name isn't really self explanatory.
> > > > 
> > > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > > connected.
> > > 
> > > We will also test the same to double confirm the behavior of
> > > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > > only during system suspend path.
> > > 
> > > Here is what I understood by looking at __device_suspend(). Please share
> > > your thoughts on this.
> > > 
> > > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > > device is suspended. This means when the root hub(s) is suspended, it is
> > > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > > to dwc3. bottom up propgation during system suspend.
> > > 
> > > I believe we can directly check something like this in the dwc3 driver
> > > instead of having another wrapper like device_children_wakeup_capable().
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 1170b80..a783257 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > >  		break;
> > >  	case DWC3_GCTL_PRTCAP_HOST:
> > >  		if (!PMSG_IS_AUTO(msg)) {
> > > +			/*
> > > +			 * Don't kill the host when dwc3 is wakeup capable and
> > > +			 * its children needs wakeup.
> > > +			 */
> > > +			if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > > +				handle_it();
> > > +		} else {
> > >  			dwc3_core_exit(dwc);
> > > -			break;
> > >  		}
> > >  
> > >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > > 
> > 
> > device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> > the wakeup capability (and enabled status) of the connected USB devices. That
> > is because xhci-plat device is configured to wakeup all the time. Since the
> > child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> > device_children_wakeup_capable() will also suffer the problem. However,
> > 
> > device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> > patch is using. That is not correct. we have two root hubs (HS and SS) associated
> > with a USB3 controller and calling it on one root hub is incorrect. 
> > device_children_wakeup_capable() must be called on xhci-plat so that it covers
> > both HS and SS root hubs
> 
> Thanks for pointing that out!
> 
> > I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> > that the wakeup path is correctly propagated to dwc3. something like below.
> > Does it make sense to you?
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 649ffd8..be0c55b 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> >  	int ret;
> >  
> > +	if (!device_wakeup_path(dev))
> > +		device_wakeup_disable(dev);
> > +
> >  	if (pm_runtime_suspended(dev))
> >  		pm_runtime_resume(dev);
> >  
> > @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> >  
> > +	device_wakeup_enable(dev);
> 
> I think this also needs to be done conditionally, otherwise it would
> create a new wake source on every resume when wakeup is already
> enabled.
> 
Right, this needs to be done conditionally. However, there is a silent
warning inside device_wakeup_enable() if it is called during system
transition. Not sure if we really need to worry about that or not.

Thanks,
Pavan

Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

[Matthias Kaehlcke]

Hi Pavan,

On Sat, Apr 30, 2022 at 08:41:30AM +0530, Pavan Kondeti wrote:
> Hi Matthias,
> 
> On Fri, Apr 29, 2022 at 12:19:22PM -0700, Matthias Kaehlcke wrote:
> > Hi Pavan,
> > 
> > On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> > > Hi Matthias,
> > > 
> > > On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > > > Hi Matthias,
> > > > 
> > > > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > > > <quic_c_sanm@quicinc.com> wrote:
> > > > > > >
> > > > > > > From: Matthias Kaehlcke <mka@chromium.org>
> > > > > > >
> > > > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > > > or one if its descendants is wakeup capable.
> > > > > > 
> > > > > > device_wakeup_path() exists for a very similar purpose.
> > > > > > 
> > > > > > Is it not usable for whatever you need the new function introduced here?
> > > > > 
> > > > > I wasn't aware of it's function, there are no doc comments and the
> > > > > name isn't really self explanatory.
> > > > > 
> > > > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > > > connected.
> > > > 
> > > > We will also test the same to double confirm the behavior of
> > > > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > > > only during system suspend path.
> > > > 
> > > > Here is what I understood by looking at __device_suspend(). Please share
> > > > your thoughts on this.
> > > > 
> > > > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > > > device is suspended. This means when the root hub(s) is suspended, it is
> > > > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > > > to dwc3. bottom up propgation during system suspend.
> > > > 
> > > > I believe we can directly check something like this in the dwc3 driver
> > > > instead of having another wrapper like device_children_wakeup_capable().
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 1170b80..a783257 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > >  		break;
> > > >  	case DWC3_GCTL_PRTCAP_HOST:
> > > >  		if (!PMSG_IS_AUTO(msg)) {
> > > > +			/*
> > > > +			 * Don't kill the host when dwc3 is wakeup capable and
> > > > +			 * its children needs wakeup.
> > > > +			 */
> > > > +			if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > > > +				handle_it();
> > > > +		} else {
> > > >  			dwc3_core_exit(dwc);
> > > > -			break;
> > > >  		}
> > > >  
> > > >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > > > 
> > > 
> > > device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> > > the wakeup capability (and enabled status) of the connected USB devices. That
> > > is because xhci-plat device is configured to wakeup all the time. Since the
> > > child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> > > device_children_wakeup_capable() will also suffer the problem. However,
> > > 
> > > device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> > > patch is using. That is not correct. we have two root hubs (HS and SS) associated
> > > with a USB3 controller and calling it on one root hub is incorrect. 
> > > device_children_wakeup_capable() must be called on xhci-plat so that it covers
> > > both HS and SS root hubs
> > 
> > Thanks for pointing that out!
> > 
> > > I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> > > that the wakeup path is correctly propagated to dwc3. something like below.
> > > Does it make sense to you?
> > > 
> > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > index 649ffd8..be0c55b 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > >  	int ret;
> > >  
> > > +	if (!device_wakeup_path(dev))
> > > +		device_wakeup_disable(dev);
> > > +
> > >  	if (pm_runtime_suspended(dev))
> > >  		pm_runtime_resume(dev);
> > >  
> > > @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
> > >  	pm_runtime_set_active(dev);
> > >  	pm_runtime_enable(dev);
> > >  
> > > +	device_wakeup_enable(dev);
> > 
> > I think this also needs to be done conditionally, otherwise it would
> > create a new wake source on every resume when wakeup is already
> > enabled.
> > 
> Right, this needs to be done conditionally. However, there is a silent
> warning inside device_wakeup_enable() if it is called during system
> transition. Not sure if we really need to worry about that or not.

I guess it's up to the maintainers. Removing and adding the wakeup source on
suspend/resume is a bit of a hack, but it might be acceptable if it addresses
the issue and doesn't have negative side effects.

[PATCH v14 3/7] usb: dwc3: core: Host wake up support from system suspend

[Sandeep Maheswaram]

During suspend read the status of all port and set hs phy mode
based on current speed. Use this hs phy mode to configure wakeup
interrupts in qcom glue driver.

Check wakeup-source property for dwc3 core node to set the
wakeup capability. Drop the device_init_wakeup call from
runtime suspend and resume.

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

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
v14:
Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.

v13:
Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
as we have a new dt property wakeup-source.


 drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
 drivers/usb/dwc3/core.h |  4 ++++
 drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1170b80..898aa66 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -32,6 +32,7 @@
 #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"
@@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
+	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);
@@ -1865,6 +1867,7 @@ 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:
@@ -1877,10 +1880,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)) {
-			dwc3_core_exit(dwc);
-			break;
-		}
+		dwc3_check_phy_speed_mode(dwc);
 
 		/* Let controller to suspend HSPHY before PHY driver suspends */
 		if (dwc->dis_u2_susphy_quirk ||
@@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 
 		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
 		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
+
+		if (!PMSG_IS_AUTO(msg)) {
+			if (device_may_wakeup(dwc->dev) &&
+			    device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
+				dwc->phy_power_off = false;
+			} else {
+				dwc->phy_power_off = true;
+				dwc3_core_exit(dwc);
+			}
+		}
 		break;
 	case DWC3_GCTL_PRTCAP_OTG:
 		/* do nothing during runtime_suspend */
@@ -1939,11 +1949,12 @@ 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->phy_power_off) {
+				ret = dwc3_core_init_for_resume(dwc);
+				if (ret)
+					return ret;
+				dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+			}
 		}
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
@@ -2015,8 +2026,6 @@ static int dwc3_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	device_init_wakeup(dev, true);
-
 	return 0;
 }
 
@@ -2025,8 +2034,6 @@ static int dwc3_runtime_resume(struct device *dev)
 	struct dwc3     *dwc = dev_get_drvdata(dev);
 	int		ret;
 
-	device_init_wakeup(dev, false);
-
 	ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME);
 	if (ret)
 		return ret;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5c9d467..6a5845f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1154,6 +1154,9 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+	bool			phy_power_off;
+
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
 
@@ -1537,6 +1540,7 @@ int dwc3_core_soft_reset(struct dwc3 *dwc);
 #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
 int dwc3_host_init(struct dwc3 *dwc);
 void dwc3_host_exit(struct dwc3 *dwc);
+void dwc3_check_phy_speed_mode(struct dwc3 *dwc);
 #else
 static inline int dwc3_host_init(struct dwc3 *dwc)
 { return 0; }
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index eda8719..3902b56 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 
 #include "core.h"
+#include "../host/xhci.h"
 
 static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
 					int irq, char *name)
@@ -138,3 +139,26 @@ void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);
 }
+
+void dwc3_check_phy_speed_mode(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_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 * NUM_PORT_REGS);
+		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;
+		}
+	}
+}
-- 
2.7.4

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

[Matthias Kaehlcke]

On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> During suspend read the status of all port and set hs phy mode
> based on current speed. Use this hs phy mode to configure wakeup
> interrupts in qcom glue driver.
> 
> Check wakeup-source property for dwc3 core node to set the
> wakeup capability. Drop the device_init_wakeup call from
> runtime suspend and resume.
> 
> Also check during suspend if any wakeup capable devices are
> connected to the controller (directly or through hubs), if there
> are none set a flag to indicate that the PHY is powered
> down during suspend.
> 
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> ---
> v14:
> Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> 
> v13:
> Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> as we have a new dt property wakeup-source.
> 
> 
>  drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
>  drivers/usb/dwc3/core.h |  4 ++++
>  drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 1170b80..898aa66 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -32,6 +32,7 @@
>  #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"
> @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, dwc);
>  	dwc3_cache_hwparams(dwc);
> +	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
>  
>  	spin_lock_init(&dwc->lock);
>  	mutex_init(&dwc->mutex);
> @@ -1865,6 +1867,7 @@ 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:
> @@ -1877,10 +1880,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)) {
> -			dwc3_core_exit(dwc);
> -			break;
> -		}
> +		dwc3_check_phy_speed_mode(dwc);
>  
>  		/* Let controller to suspend HSPHY before PHY driver suspends */
>  		if (dwc->dis_u2_susphy_quirk ||
> @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
>  		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> +
> +		if (!PMSG_IS_AUTO(msg)) {
> +			if (device_may_wakeup(dwc->dev) &&
> +			    device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> +				dwc->phy_power_off = false;
> +			} else {
> +				dwc->phy_power_off = true;
> +				dwc3_core_exit(dwc);

I found that shutting the PHYs down during suspend leads to high power
consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
not shut down).

It would be interesting to know if this also impacts other non-hub
peripherals. Unfortunately I can't test that, the hub on my system is
soldered to the board.

I understand that shutting the PHYs down might be beneficial in terms
of power on some systems, however on those I'm looking at we'd strongly
prefer to save the 65mW of power consumed by the hub, rather than
whatever smaller amount of power that is saved by powering down the
PHYs.

Could we introduce a sysfs attribute (or some other sort of knob) to
allow the admin to configure whether the PHYs should remain on or off
during suspend? That is assuming that it is actually desirable to power
them off on some systems.

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

[Pavan Kondeti]

On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > During suspend read the status of all port and set hs phy mode
> > based on current speed. Use this hs phy mode to configure wakeup
> > interrupts in qcom glue driver.
> > 
> > Check wakeup-source property for dwc3 core node to set the
> > wakeup capability. Drop the device_init_wakeup call from
> > runtime suspend and resume.
> > 
> > Also check during suspend if any wakeup capable devices are
> > connected to the controller (directly or through hubs), if there
> > are none set a flag to indicate that the PHY is powered
> > down during suspend.
> > 
> > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > ---
> > v14:
> > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> > 
> > v13:
> > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > as we have a new dt property wakeup-source.
> > 
> > 
> >  drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> >  drivers/usb/dwc3/core.h |  4 ++++
> >  drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> >  3 files changed, 48 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 1170b80..898aa66 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -32,6 +32,7 @@
> >  #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"
> > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, dwc);
> >  	dwc3_cache_hwparams(dwc);
> > +	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> >  
> >  	spin_lock_init(&dwc->lock);
> >  	mutex_init(&dwc->mutex);
> > @@ -1865,6 +1867,7 @@ 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:
> > @@ -1877,10 +1880,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)) {
> > -			dwc3_core_exit(dwc);
> > -			break;
> > -		}
> > +		dwc3_check_phy_speed_mode(dwc);
> >  
> >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> >  		if (dwc->dis_u2_susphy_quirk ||
> > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >  
> >  		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> >  		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > +
> > +		if (!PMSG_IS_AUTO(msg)) {
> > +			if (device_may_wakeup(dwc->dev) &&
> > +			    device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > +				dwc->phy_power_off = false;
> > +			} else {
> > +				dwc->phy_power_off = true;
> > +				dwc3_core_exit(dwc);
> 
> I found that shutting the PHYs down during suspend leads to high power
> consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> not shut down).
> 
> It would be interesting to know if this also impacts other non-hub
> peripherals. Unfortunately I can't test that, the hub on my system is
> soldered to the board.
> 
> I understand that shutting the PHYs down might be beneficial in terms
> of power on some systems, however on those I'm looking at we'd strongly
> prefer to save the 65mW of power consumed by the hub, rather than
> whatever smaller amount of power that is saved by powering down the
> PHYs.
> 
> Could we introduce a sysfs attribute (or some other sort of knob) to
> allow the admin to configure whether the PHYs should remain on or off
> during suspend? That is assuming that it is actually desirable to power
> them off on some systems.

The result may vary across SoCs also. The current proposal is to keep PHY
powered during system suspend if any of the downstream USB devices are enabled
for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
always powered on even when no device is attached, they can do so by enabling
wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
needed if you want to detect a peripheral attach during system suspend.

Thanks,
Pavan

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

[Matthias Kaehlcke]

On Thu, May 05, 2022 at 08:56:18AM +0530, Pavan Kondeti wrote:
> On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> > On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > > During suspend read the status of all port and set hs phy mode
> > > based on current speed. Use this hs phy mode to configure wakeup
> > > interrupts in qcom glue driver.
> > > 
> > > Check wakeup-source property for dwc3 core node to set the
> > > wakeup capability. Drop the device_init_wakeup call from
> > > runtime suspend and resume.
> > > 
> > > Also check during suspend if any wakeup capable devices are
> > > connected to the controller (directly or through hubs), if there
> > > are none set a flag to indicate that the PHY is powered
> > > down during suspend.
> > > 
> > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > ---
> > > v14:
> > > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> > > 
> > > v13:
> > > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > > as we have a new dt property wakeup-source.
> > > 
> > > 
> > >  drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> > >  drivers/usb/dwc3/core.h |  4 ++++
> > >  drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> > >  3 files changed, 48 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 1170b80..898aa66 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -32,6 +32,7 @@
> > >  #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"
> > > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > >  
> > >  	platform_set_drvdata(pdev, dwc);
> > >  	dwc3_cache_hwparams(dwc);
> > > +	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> > >  
> > >  	spin_lock_init(&dwc->lock);
> > >  	mutex_init(&dwc->mutex);
> > > @@ -1865,6 +1867,7 @@ 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:
> > > @@ -1877,10 +1880,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)) {
> > > -			dwc3_core_exit(dwc);
> > > -			break;
> > > -		}
> > > +		dwc3_check_phy_speed_mode(dwc);
> > >  
> > >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > >  		if (dwc->dis_u2_susphy_quirk ||
> > > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > >  
> > >  		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> > >  		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > > +
> > > +		if (!PMSG_IS_AUTO(msg)) {
> > > +			if (device_may_wakeup(dwc->dev) &&
> > > +			    device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > > +				dwc->phy_power_off = false;
> > > +			} else {
> > > +				dwc->phy_power_off = true;
> > > +				dwc3_core_exit(dwc);
> > 
> > I found that shutting the PHYs down during suspend leads to high power
> > consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> > not shut down).
> > 
> > It would be interesting to know if this also impacts other non-hub
> > peripherals. Unfortunately I can't test that, the hub on my system is
> > soldered to the board.
> > 
> > I understand that shutting the PHYs down might be beneficial in terms
> > of power on some systems, however on those I'm looking at we'd strongly
> > prefer to save the 65mW of power consumed by the hub, rather than
> > whatever smaller amount of power that is saved by powering down the
> > PHYs.
> > 
> > Could we introduce a sysfs attribute (or some other sort of knob) to
> > allow the admin to configure whether the PHYs should remain on or off
> > during suspend? That is assuming that it is actually desirable to power
> > them off on some systems.
> 
> The result may vary across SoCs also. The current proposal is to keep PHY
> powered during system suspend if any of the downstream USB devices are enabled
> for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
> always powered on even when no device is attached, they can do so by enabling
> wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
> needed if you want to detect a peripheral attach during system suspend.

My concern is that it is not evident for an admin what causes the high power
consumption of the USB client (if they detect/localize it in the first place),
and even less that wakeup needs to be enabled to mitigate it.

Why can't we just put the PHYs in suspend, rather than taking the controller
down completely during suspend?

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

[Pavan Kondeti]

On Thu, May 05, 2022 at 09:45:49AM -0700, Matthias Kaehlcke wrote:
> On Thu, May 05, 2022 at 08:56:18AM +0530, Pavan Kondeti wrote:
> > On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> > > On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > > > During suspend read the status of all port and set hs phy mode
> > > > based on current speed. Use this hs phy mode to configure wakeup
> > > > interrupts in qcom glue driver.
> > > > 
> > > > Check wakeup-source property for dwc3 core node to set the
> > > > wakeup capability. Drop the device_init_wakeup call from
> > > > runtime suspend and resume.
> > > > 
> > > > Also check during suspend if any wakeup capable devices are
> > > > connected to the controller (directly or through hubs), if there
> > > > are none set a flag to indicate that the PHY is powered
> > > > down during suspend.
> > > > 
> > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > > ---
> > > > v14:
> > > > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> > > > 
> > > > v13:
> > > > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > > > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > > > as we have a new dt property wakeup-source.
> > > > 
> > > > 
> > > >  drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> > > >  drivers/usb/dwc3/core.h |  4 ++++
> > > >  drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> > > >  3 files changed, 48 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 1170b80..898aa66 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -32,6 +32,7 @@
> > > >  #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"
> > > > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > > >  
> > > >  	platform_set_drvdata(pdev, dwc);
> > > >  	dwc3_cache_hwparams(dwc);
> > > > +	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> > > >  
> > > >  	spin_lock_init(&dwc->lock);
> > > >  	mutex_init(&dwc->mutex);
> > > > @@ -1865,6 +1867,7 @@ 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:
> > > > @@ -1877,10 +1880,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)) {
> > > > -			dwc3_core_exit(dwc);
> > > > -			break;
> > > > -		}
> > > > +		dwc3_check_phy_speed_mode(dwc);
> > > >  
> > > >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > > >  		if (dwc->dis_u2_susphy_quirk ||
> > > > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > >  
> > > >  		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> > > >  		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > > > +
> > > > +		if (!PMSG_IS_AUTO(msg)) {
> > > > +			if (device_may_wakeup(dwc->dev) &&
> > > > +			    device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > > > +				dwc->phy_power_off = false;
> > > > +			} else {
> > > > +				dwc->phy_power_off = true;
> > > > +				dwc3_core_exit(dwc);
> > > 
> > > I found that shutting the PHYs down during suspend leads to high power
> > > consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> > > not shut down).
> > > 
> > > It would be interesting to know if this also impacts other non-hub
> > > peripherals. Unfortunately I can't test that, the hub on my system is
> > > soldered to the board.
> > > 
> > > I understand that shutting the PHYs down might be beneficial in terms
> > > of power on some systems, however on those I'm looking at we'd strongly
> > > prefer to save the 65mW of power consumed by the hub, rather than
> > > whatever smaller amount of power that is saved by powering down the
> > > PHYs.
> > > 
> > > Could we introduce a sysfs attribute (or some other sort of knob) to
> > > allow the admin to configure whether the PHYs should remain on or off
> > > during suspend? That is assuming that it is actually desirable to power
> > > them off on some systems.
> > 
> > The result may vary across SoCs also. The current proposal is to keep PHY
> > powered during system suspend if any of the downstream USB devices are enabled
> > for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
> > always powered on even when no device is attached, they can do so by enabling
> > wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
> > needed if you want to detect a peripheral attach during system suspend.
> 
> My concern is that it is not evident for an admin what causes the high power
> consumption of the USB client (if they detect/localize it in the first place),
> and even less that wakeup needs to be enabled to mitigate it.
> 
> Why can't we just put the PHYs in suspend, rather than taking the controller
> down completely during suspend?

Agreed and I also have the same question.

I don't know the background on why DWC3 chooses to power down the PHY(s)
during system suspend. Probably it is beneficial in some board designs.
Atleast this patch series provides a way to wakeup the USB from system
suspend, which also can be used not to power down the PHY(s). If all the users
of DWC3 agree that powering down the PHY is bad, then we can do something
about it.

Thanks,
Pavan

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

[Matthias Kaehlcke]

On Fri, May 06, 2022 at 08:31:07AM +0530, Pavan Kondeti wrote:
> On Thu, May 05, 2022 at 09:45:49AM -0700, Matthias Kaehlcke wrote:
> > On Thu, May 05, 2022 at 08:56:18AM +0530, Pavan Kondeti wrote:
> > > On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> > > > On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > > > > During suspend read the status of all port and set hs phy mode
> > > > > based on current speed. Use this hs phy mode to configure wakeup
> > > > > interrupts in qcom glue driver.
> > > > > 
> > > > > Check wakeup-source property for dwc3 core node to set the
> > > > > wakeup capability. Drop the device_init_wakeup call from
> > > > > runtime suspend and resume.
> > > > > 
> > > > > Also check during suspend if any wakeup capable devices are
> > > > > connected to the controller (directly or through hubs), if there
> > > > > are none set a flag to indicate that the PHY is powered
> > > > > down during suspend.
> > > > > 
> > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > > > ---
> > > > > v14:
> > > > > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> > > > > 
> > > > > v13:
> > > > > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > > > > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > > > > as we have a new dt property wakeup-source.
> > > > > 
> > > > > 
> > > > >  drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> > > > >  drivers/usb/dwc3/core.h |  4 ++++
> > > > >  drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> > > > >  3 files changed, 48 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > index 1170b80..898aa66 100644
> > > > > --- a/drivers/usb/dwc3/core.c
> > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > @@ -32,6 +32,7 @@
> > > > >  #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"
> > > > > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > > > >  
> > > > >  	platform_set_drvdata(pdev, dwc);
> > > > >  	dwc3_cache_hwparams(dwc);
> > > > > +	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> > > > >  
> > > > >  	spin_lock_init(&dwc->lock);
> > > > >  	mutex_init(&dwc->mutex);
> > > > > @@ -1865,6 +1867,7 @@ 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:
> > > > > @@ -1877,10 +1880,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)) {
> > > > > -			dwc3_core_exit(dwc);
> > > > > -			break;
> > > > > -		}
> > > > > +		dwc3_check_phy_speed_mode(dwc);
> > > > >  
> > > > >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > > > >  		if (dwc->dis_u2_susphy_quirk ||
> > > > > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > > >  
> > > > >  		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> > > > >  		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > > > > +
> > > > > +		if (!PMSG_IS_AUTO(msg)) {
> > > > > +			if (device_may_wakeup(dwc->dev) &&
> > > > > +			    device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > > > > +				dwc->phy_power_off = false;
> > > > > +			} else {
> > > > > +				dwc->phy_power_off = true;
> > > > > +				dwc3_core_exit(dwc);
> > > > 
> > > > I found that shutting the PHYs down during suspend leads to high power
> > > > consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> > > > not shut down).
> > > > 
> > > > It would be interesting to know if this also impacts other non-hub
> > > > peripherals. Unfortunately I can't test that, the hub on my system is
> > > > soldered to the board.
> > > > 
> > > > I understand that shutting the PHYs down might be beneficial in terms
> > > > of power on some systems, however on those I'm looking at we'd strongly
> > > > prefer to save the 65mW of power consumed by the hub, rather than
> > > > whatever smaller amount of power that is saved by powering down the
> > > > PHYs.
> > > > 
> > > > Could we introduce a sysfs attribute (or some other sort of knob) to
> > > > allow the admin to configure whether the PHYs should remain on or off
> > > > during suspend? That is assuming that it is actually desirable to power
> > > > them off on some systems.
> > > 
> > > The result may vary across SoCs also. The current proposal is to keep PHY
> > > powered during system suspend if any of the downstream USB devices are enabled
> > > for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
> > > always powered on even when no device is attached, they can do so by enabling
> > > wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
> > > needed if you want to detect a peripheral attach during system suspend.
> > 
> > My concern is that it is not evident for an admin what causes the high power
> > consumption of the USB client (if they detect/localize it in the first place),
> > and even less that wakeup needs to be enabled to mitigate it.
> > 
> > Why can't we just put the PHYs in suspend, rather than taking the controller
> > down completely during suspend?
> 
> Agreed and I also have the same question.
> 
> I don't know the background on why DWC3 chooses to power down the PHY(s)
> during system suspend. Probably it is beneficial in some board designs.
> Atleast this patch series provides a way to wakeup the USB from system
> suspend, which also can be used not to power down the PHY(s). If all the users
> of DWC3 agree that powering down the PHY is bad, then we can do something
> about it.

I came across this commit while doing a bit of archeology:

  commit c4a5153e87fdf6805f63ff57556260e2554155a5
  Author: Manu Gautam <mgautam@codeaurora.org>
  Date:   Thu Jan 18 16:54:30 2018 +0530

  usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode

  Commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during
  host bus-suspend/resume") updated suspend/resume routines to not
  power_off and reinit PHYs/core for host mode.
  It broke platforms that rely on DWC3 core to power_off PHYs to
  enter low power state on system suspend.

  Perform dwc3_core_exit/init only during host mode system_suspend/
  resume to addresses power regression from above mentioned patch
  and also allow USB session to stay connected across
  runtime_suspend/resume in host mode. While at it also replace
  existing checks for HOST only dr_mode with current_dr_role to
  have similar core driver behavior for both Host-only and DRD+Host
  configurations.

  Fixes: 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")
  Reviewed-by: Roger Quadros <rogerq@ti.com>
  Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
  Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>


So apparently powering off the core and PHYs is needed on some
platforms.

We could introduce a DT property that indicates that keeping the core/PHYs
on is supported. Another hint could be the fact that the controller is
marked as wakeup capable, however that would still power the core/PHYs
down if the 'wakeup-source' property isn't set for a controller that is
technically wakeup capable.

[PATCH v14 4/7] 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 6cba990..7352124 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

[PATCH v14 5/7] usb: dwc3: qcom: Configure wakeup interrupts during suspend

[Sandeep Maheswaram]

Configure DP/DM interrupts to detect line state changes based on
hs_phy_mode. Enable the triggers opposite of what the current
DP, DM levels. For HS/FS mode enable DM interrupt and for LS enable DP
interrupt.

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

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 7352124..9804a19 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -316,22 +316,36 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
 
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
-	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 
-	dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
 
-	dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_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)
 {
-	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 
-	dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
 
-	dwc3_qcom_enable_wakeup_irq(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);
+	}
 
 	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq);
 }
-- 
2.7.4

[PATCH v14 6/7] usb: dwc3: qcom: Keep power domain on to retain controller status

[Sandeep Maheswaram]

Keep the power domain always on during runtime suspend or if the
controller supports wakeup in order to retain controller status
and to support wakeup from devices.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
v14:
Removed the unwanted spaces between the type
and the variable name in dwc3_qcom_probe.
Avoiding device_may_wakeup condition check multiple times.

v13:
Added GENPD_FLAG_RPM_ALWAYS_ON irrespective of wakeup capability.
Enable GENPD_FLAG_ALWAYS_ON only if device_may_wakeup is true.

 drivers/usb/dwc3/dwc3-qcom.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 9804a19..1f9589a 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>
@@ -718,12 +719,13 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
-	struct device_node	*np = pdev->dev.of_node;
-	struct device		*dev = &pdev->dev;
-	struct dwc3_qcom	*qcom;
-	struct resource		*res, *parent_res = NULL;
-	int			ret, i;
-	bool			ignore_pipe_clk;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct dwc3_qcom *qcom;
+	struct resource	*res, *parent_res = NULL;
+	int ret, i;
+	bool ignore_pipe_clk;
+	struct generic_pm_domain *genpd;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)
@@ -732,6 +734,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, qcom);
 	qcom->dev = &pdev->dev;
 
+	genpd = pd_to_genpd(qcom->dev->pm_domain);
+
 	if (has_acpi_companion(dev)) {
 		qcom->acpi_pdata = acpi_device_get_match_data(dev);
 		if (!qcom->acpi_pdata) {
@@ -839,7 +843,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	device_init_wakeup(&pdev->dev, 1);
+	genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
+
+	if (device_may_wakeup(&qcom->dwc3->dev)) {
+		genpd->flags |= GENPD_FLAG_ALWAYS_ON;
+		device_init_wakeup(&pdev->dev, true);
+	}
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-- 
2.7.4

[PATCH v14 7/7] arm64: dts: qcom: sc7280: Add wakeup-source property for USB node

[Sandeep Maheswaram]

Adding wakeup-source property for USB controller in SC7280.
This property is added to inform that the USB controller is
wake up capable and to conditionally power down the phy during
system suspend.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 9ece357..00bacc4 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3106,6 +3106,7 @@
 				phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
 				phy-names = "usb2-phy", "usb3-phy";
 				maximum-speed = "super-speed";
+				wakeup-source;
 			};
 		};
 
-- 
2.7.4