[v15 0/6] USB DWC3 host wake up support from system suspend

[v15 0/6] USB DWC3 host wake up support from system suspend

[Krishna Kurapati]

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 v15:
Added patch to enable wakeup for xhci-plat based on children wakeup status.
Used device_wakeup_path instead of device_children_wakeup_capable

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.

Krishna Kurapati (1):
  usb: host: xhci-plat: Enable wakeup based on children wakeup status

Sandeep Maheswaram (5):
  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

 .../devicetree/bindings/usb/snps,dwc3.yaml         |  5 ++
 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 ++++++
 drivers/usb/host/xhci-plat.c                       |  8 ++
 6 files changed, 115 insertions(+), 50 deletions(-)

-- 
2.7.4

[v15 1/6] dt-bindings: usb: dwc3: Add wakeup-source property support

[Krishna Kurapati]

From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

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>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 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

[v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

[Krishna Kurapati]

device_wakeup_path() tells if any of the children devices needs
wakeup. Use this hint to enable/disable wakeup of our device. This
helps the parent device of xhci-plat (like sysdev) to retrieve
the wakeup setting via device_wakeup_path().

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/host/xhci-plat.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 649ffd8..ad585fa 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 	if (pm_runtime_suspended(dev))
 		pm_runtime_resume(dev);
 
+	if (device_wakeup_path(dev)) {
+		if (!device_may_wakeup(dev))
+			device_wakeup_enable(dev);
+	} else {
+		if (device_may_wakeup(dev))
+			device_wakeup_disable(dev);
+	}
+
 	ret = xhci_priv_suspend_quirk(hcd);
 	if (ret)
 		return ret;
-- 
2.7.4

Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

[Matthias Kaehlcke]

On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote:
> device_wakeup_path() tells if any of the children devices needs
> wakeup. Use this hint to enable/disable wakeup of our device. This
> helps the parent device of xhci-plat (like sysdev) to retrieve
> the wakeup setting via device_wakeup_path().
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/host/xhci-plat.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 649ffd8..ad585fa 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
>  	if (pm_runtime_suspended(dev))
>  		pm_runtime_resume(dev);
>  
> +	if (device_wakeup_path(dev)) {
> +		if (!device_may_wakeup(dev))
> +			device_wakeup_enable(dev);
> +	} else {
> +		if (device_may_wakeup(dev))
> +			device_wakeup_disable(dev);
> +	}

This code is not self-explantatory and deserves a comment.

Enabling/disabling wakeup for the purpose if signalling is a bit of a
hack. It might be an acceptable hack as long as it has no side effects.
However with the current implementation the wakeup state of the xHCI can
be different after resuming than it was before going to suspend:

after boot
  grep -h xhci /sys/class/wakeup/*/name
    => xhci-hcd.14.auto

after suspend w/o wakeup capable device
  grep -h xhci /sys/class/wakeup/*/name
    => no results

after suspend with wakeup capable device
  grep -h xhci /sys/class/wakeup/*/name
    => xhci-hcd.14.auto

The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have
to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7]
PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done
conditionally though).

Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

[Pavan Kondeti]

On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote:
> On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote:
> > device_wakeup_path() tells if any of the children devices needs
> > wakeup. Use this hint to enable/disable wakeup of our device. This
> > helps the parent device of xhci-plat (like sysdev) to retrieve
> > the wakeup setting via device_wakeup_path().
> > 
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 649ffd8..ad585fa 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> >  	if (pm_runtime_suspended(dev))
> >  		pm_runtime_resume(dev);
> >  
> > +	if (device_wakeup_path(dev)) {
> > +		if (!device_may_wakeup(dev))
> > +			device_wakeup_enable(dev);
> > +	} else {
> > +		if (device_may_wakeup(dev))
> > +			device_wakeup_disable(dev);
> > +	}
> 
> This code is not self-explantatory and deserves a comment.
> 
> Enabling/disabling wakeup for the purpose if signalling is a bit of a
> hack. It might be an acceptable hack as long as it has no side effects.
> However with the current implementation the wakeup state of the xHCI can
> be different after resuming than it was before going to suspend:
> 
> after boot
>   grep -h xhci /sys/class/wakeup/*/name
>     => xhci-hcd.14.auto
> 
> after suspend w/o wakeup capable device
>   grep -h xhci /sys/class/wakeup/*/name
>     => no results
> 
> after suspend with wakeup capable device
>   grep -h xhci /sys/class/wakeup/*/name
>     => xhci-hcd.14.auto
> 
> The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have
> to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7]
> PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done
> conditionally though).

I am worried that we are not doing the right thing here. why should the
xhci-plat goes against the wishes of the user space policy here? Can we NOT
just do anything here? If some one wants xhci-plat to wakeup all the time,
dwc3 will be configured to wakeup the system provided that the support is
available. This way we don't break any existing users of xhci-plat i.e not
enabling wakeup from the kernel.

Thanks,
Pavan

Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

[Pavan Kondeti]

On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote:
> On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote:
> > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote:
> > > device_wakeup_path() tells if any of the children devices needs
> > > wakeup. Use this hint to enable/disable wakeup of our device. This
> > > helps the parent device of xhci-plat (like sysdev) to retrieve
> > > the wakeup setting via device_wakeup_path().
> > > 
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >  drivers/usb/host/xhci-plat.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > index 649ffd8..ad585fa 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > >  	if (pm_runtime_suspended(dev))
> > >  		pm_runtime_resume(dev);
> > >  
> > > +	if (device_wakeup_path(dev)) {
> > > +		if (!device_may_wakeup(dev))
> > > +			device_wakeup_enable(dev);
> > > +	} else {
> > > +		if (device_may_wakeup(dev))
> > > +			device_wakeup_disable(dev);
> > > +	}
> > 
> > This code is not self-explantatory and deserves a comment.
> > 
> > Enabling/disabling wakeup for the purpose if signalling is a bit of a
> > hack. It might be an acceptable hack as long as it has no side effects.
> > However with the current implementation the wakeup state of the xHCI can
> > be different after resuming than it was before going to suspend:
> > 
> > after boot
> >   grep -h xhci /sys/class/wakeup/*/name
> >     => xhci-hcd.14.auto
> > 
> > after suspend w/o wakeup capable device
> >   grep -h xhci /sys/class/wakeup/*/name
> >     => no results
> > 
> > after suspend with wakeup capable device
> >   grep -h xhci /sys/class/wakeup/*/name
> >     => xhci-hcd.14.auto
> > 
> > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have
> > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7]
> > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done
> > conditionally though).
> 
> I am worried that we are not doing the right thing here. why should the
> xhci-plat goes against the wishes of the user space policy here? Can we NOT
> just do anything here? If some one wants xhci-plat to wakeup all the time,
> dwc3 will be configured to wakeup the system provided that the support is
> available. This way we don't break any existing users of xhci-plat i.e not
> enabling wakeup from the kernel.
> 
Krishna,

can we please drop this patch and use device_wakeup_path() and verify the
following cases.

1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled
2. one of the downstream USB device supports wakeup and xhci-plat wakeup is
disabled
3. none of the downstream USB device supports wakeup (or disable) and
xhci-plat wakeup is enabled.
4. none of the downstream USB device supports wakeup (or disable) and
xhci-plat wakeup is disabled.

We don't want to enable xhci-plat wakeup capability like we do in this patch
and potentially break any other platform. Lets leave the policy to the user
space and rely on wakeup path to see if we can achieve the desired result.

Thanks,
Pavan

Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

[Matthias Kaehlcke]

On Wed, May 11, 2022 at 07:21:01AM +0530, Pavan Kondeti wrote:
> On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote:
> > On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote:
> > > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote:
> > > > device_wakeup_path() tells if any of the children devices needs
> > > > wakeup. Use this hint to enable/disable wakeup of our device. This
> > > > helps the parent device of xhci-plat (like sysdev) to retrieve
> > > > the wakeup setting via device_wakeup_path().
> > > > 
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > ---
> > > >  drivers/usb/host/xhci-plat.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > > index 649ffd8..ad585fa 100644
> > > > --- a/drivers/usb/host/xhci-plat.c
> > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > > >  	if (pm_runtime_suspended(dev))
> > > >  		pm_runtime_resume(dev);
> > > >  
> > > > +	if (device_wakeup_path(dev)) {
> > > > +		if (!device_may_wakeup(dev))
> > > > +			device_wakeup_enable(dev);
> > > > +	} else {
> > > > +		if (device_may_wakeup(dev))
> > > > +			device_wakeup_disable(dev);
> > > > +	}
> > > 
> > > This code is not self-explantatory and deserves a comment.
> > > 
> > > Enabling/disabling wakeup for the purpose if signalling is a bit of a
> > > hack. It might be an acceptable hack as long as it has no side effects.
> > > However with the current implementation the wakeup state of the xHCI can
> > > be different after resuming than it was before going to suspend:
> > > 
> > > after boot
> > >   grep -h xhci /sys/class/wakeup/*/name
> > >     => xhci-hcd.14.auto
> > > 
> > > after suspend w/o wakeup capable device
> > >   grep -h xhci /sys/class/wakeup/*/name
> > >     => no results
> > > 
> > > after suspend with wakeup capable device
> > >   grep -h xhci /sys/class/wakeup/*/name
> > >     => xhci-hcd.14.auto
> > > 
> > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have
> > > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7]
> > > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done
> > > conditionally though).
> > 
> > I am worried that we are not doing the right thing here. why should the
> > xhci-plat goes against the wishes of the user space policy here? Can we NOT
> > just do anything here? If some one wants xhci-plat to wakeup all the time,
> > dwc3 will be configured to wakeup the system provided that the support is
> > available. This way we don't break any existing users of xhci-plat i.e not
> > enabling wakeup from the kernel.
> > 
> Krishna,
> 
> can we please drop this patch and use device_wakeup_path() and verify the
> following cases.
> 
> 1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled
> 2. one of the downstream USB device supports wakeup and xhci-plat wakeup is
> disabled
> 3. none of the downstream USB device supports wakeup (or disable) and
> xhci-plat wakeup is enabled.
> 4. none of the downstream USB device supports wakeup (or disable) and
> xhci-plat wakeup is disabled.

I wonder if we couldn't keep this simpler: if the dwc3 is wakeup capable keep
the PHYs/core powered, otherwise power them down. Similar to what commit
689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host
bus-suspend/resume") intended, but with the additonal check for wakeup
capability. We now know that the PHYs need to be powered down on some SoCs
to allow the SoC to reach its low power mode during suspend:


  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.


With wakeup capable controllers this is apparently not an issue, otherwise
the SoC wouldn't be able to enter its low power state when wakeup is
enabled.

Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

[Pavan Kondeti]

On Wed, May 11, 2022 at 08:54:25AM -0700, Matthias Kaehlcke wrote:
> On Wed, May 11, 2022 at 07:21:01AM +0530, Pavan Kondeti wrote:
> > On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote:
> > > On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote:
> > > > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote:
> > > > > device_wakeup_path() tells if any of the children devices needs
> > > > > wakeup. Use this hint to enable/disable wakeup of our device. This
> > > > > helps the parent device of xhci-plat (like sysdev) to retrieve
> > > > > the wakeup setting via device_wakeup_path().
> > > > > 
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > ---
> > > > >  drivers/usb/host/xhci-plat.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > > > index 649ffd8..ad585fa 100644
> > > > > --- a/drivers/usb/host/xhci-plat.c
> > > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > > > >  	if (pm_runtime_suspended(dev))
> > > > >  		pm_runtime_resume(dev);
> > > > >  
> > > > > +	if (device_wakeup_path(dev)) {
> > > > > +		if (!device_may_wakeup(dev))
> > > > > +			device_wakeup_enable(dev);
> > > > > +	} else {
> > > > > +		if (device_may_wakeup(dev))
> > > > > +			device_wakeup_disable(dev);
> > > > > +	}
> > > > 
> > > > This code is not self-explantatory and deserves a comment.
> > > > 
> > > > Enabling/disabling wakeup for the purpose if signalling is a bit of a
> > > > hack. It might be an acceptable hack as long as it has no side effects.
> > > > However with the current implementation the wakeup state of the xHCI can
> > > > be different after resuming than it was before going to suspend:
> > > > 
> > > > after boot
> > > >   grep -h xhci /sys/class/wakeup/*/name
> > > >     => xhci-hcd.14.auto
> > > > 
> > > > after suspend w/o wakeup capable device
> > > >   grep -h xhci /sys/class/wakeup/*/name
> > > >     => no results
> > > > 
> > > > after suspend with wakeup capable device
> > > >   grep -h xhci /sys/class/wakeup/*/name
> > > >     => xhci-hcd.14.auto
> > > > 
> > > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have
> > > > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7]
> > > > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done
> > > > conditionally though).
> > > 
> > > I am worried that we are not doing the right thing here. why should the
> > > xhci-plat goes against the wishes of the user space policy here? Can we NOT
> > > just do anything here? If some one wants xhci-plat to wakeup all the time,
> > > dwc3 will be configured to wakeup the system provided that the support is
> > > available. This way we don't break any existing users of xhci-plat i.e not
> > > enabling wakeup from the kernel.
> > > 
> > Krishna,
> > 
> > can we please drop this patch and use device_wakeup_path() and verify the
> > following cases.
> > 
> > 1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled
> > 2. one of the downstream USB device supports wakeup and xhci-plat wakeup is
> > disabled
> > 3. none of the downstream USB device supports wakeup (or disable) and
> > xhci-plat wakeup is enabled.
> > 4. none of the downstream USB device supports wakeup (or disable) and
> > xhci-plat wakeup is disabled.
> 
> I wonder if we couldn't keep this simpler: if the dwc3 is wakeup capable keep
> the PHYs/core powered, otherwise power them down. Similar to what commit
> 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host
> bus-suspend/resume") intended, but with the additonal check for wakeup
> capability. We now know that the PHYs need to be powered down on some SoCs
> to allow the SoC to reach its low power mode during suspend:
> 
> 
>   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.
> 
> 
> With wakeup capable controllers this is apparently not an issue, otherwise
> the SoC wouldn't be able to enter its low power state when wakeup is
> enabled.

Agree to your suggestion. Thanks for your inputs.

Thanks,
Pavan

[v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Krishna Kurapati]

From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

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>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 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 950e238..cf377f5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -33,6 +33,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"
@@ -1787,6 +1788,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);
@@ -1936,6 +1938,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:
@@ -1948,10 +1951,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 ||
@@ -1967,6 +1967,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_wakeup_path(dwc->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 */
@@ -2010,11 +2020,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));
@@ -2086,8 +2097,6 @@ static int dwc3_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	device_init_wakeup(dev, true);
-
 	return 0;
 }
 
@@ -2096,8 +2105,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 81c486b..37397a8 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1155,6 +1155,9 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+	bool			phy_power_off;
+
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
 
@@ -1539,6 +1542,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: [v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Matthias Kaehlcke]

On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote:
> From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> 
> 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>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  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 950e238..cf377f5 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -33,6 +33,7 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/of.h>
>  #include <linux/usb/otg.h>
> +#include <linux/usb/hcd.h>

This is not needed anymore

>  
>  #include "core.h"
>  #include "gadget.h"
> @@ -1787,6 +1788,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);
> @@ -1936,6 +1938,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);

This isn't used anymore, delete it

>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> @@ -1948,10 +1951,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 ||
> @@ -1967,6 +1967,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_wakeup_path(dwc->dev)) {

nit: the indentation is odd, align it with device_may_wakeup()?

> +				dwc->phy_power_off = false;
> +			} else {
> +				dwc->phy_power_off = true;
> +				dwc3_core_exit(dwc);

As commented earlier, taking the controller and PHYs completely down causes a
significant power draw in some USB clients. Let's clarify what the specific
benefits are of doing dwc3_core_exit() vs. entering a low power mode.

Re: [v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Krishna Kurapati PSSNV]

On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote:
> On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote:
>> From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
>>
>> 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>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   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 950e238..cf377f5 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -33,6 +33,7 @@
>>   #include <linux/usb/gadget.h>
>>   #include <linux/usb/of.h>
>>   #include <linux/usb/otg.h>
>> +#include <linux/usb/hcd.h>
> This is not needed anymore
>
>>   
>>   #include "core.h"
>>   #include "gadget.h"
>> @@ -1787,6 +1788,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);
>> @@ -1936,6 +1938,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);
> This isn't used anymore, delete it
My bad, Will fix this in next version.
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> @@ -1948,10 +1951,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 ||
>> @@ -1967,6 +1967,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_wakeup_path(dwc->dev)) {
> nit: the indentation is odd, align it with device_may_wakeup()?
Sure, Will take care of it.
>> +				dwc->phy_power_off = false;
>> +			} else {
>> +				dwc->phy_power_off = true;
>> +				dwc3_core_exit(dwc);
> As commented earlier, taking the controller and PHYs completely down causes a
> significant power draw in some USB clients. Let's clarify what the specific
> benefits are of doing dwc3_core_exit() vs. entering a low power mode.
Sure, once we come to a conclusion on this, I will refresh the patches.

Re: [v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Pavan Kondeti]

On Fri, May 06, 2022 at 10:41:01AM +0530, Krishna Kurapati PSSNV wrote:
> 
> On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote:
> >On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote:
> >>From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> >>
> >>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>
> >>Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> >>---
> >>  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 950e238..cf377f5 100644
> >>--- a/drivers/usb/dwc3/core.c
> >>+++ b/drivers/usb/dwc3/core.c
> >>@@ -33,6 +33,7 @@
> >>  #include <linux/usb/gadget.h>
> >>  #include <linux/usb/of.h>
> >>  #include <linux/usb/otg.h>
> >>+#include <linux/usb/hcd.h>
> >This is not needed anymore
> >
> >>  #include "core.h"
> >>  #include "gadget.h"
> >>@@ -1787,6 +1788,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);
> >>@@ -1936,6 +1938,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);
> >This isn't used anymore, delete it
> My bad, Will fix this in next version.
> >>  	switch (dwc->current_dr_role) {
> >>  	case DWC3_GCTL_PRTCAP_DEVICE:
> >>@@ -1948,10 +1951,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 ||
> >>@@ -1967,6 +1967,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_wakeup_path(dwc->dev)) {
> >nit: the indentation is odd, align it with device_may_wakeup()?
> Sure, Will take care of it.
> >>+				dwc->phy_power_off = false;
> >>+			} else {
> >>+				dwc->phy_power_off = true;
> >>+				dwc3_core_exit(dwc);
> >As commented earlier, taking the controller and PHYs completely down causes a
> >significant power draw in some USB clients. Let's clarify what the specific
> >benefits are of doing dwc3_core_exit() vs. entering a low power mode.
> Sure, once we come to a conclusion on this, I will refresh the patches.

I think, Matthias is asking you to clarify in the commit description. we can
even quote Matthias observations.

Thanks,
Pavan

Re: [v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Matthias Kaehlcke]

On Fri, May 06, 2022 at 10:44:48AM +0530, Pavan Kondeti wrote:
> On Fri, May 06, 2022 at 10:41:01AM +0530, Krishna Kurapati PSSNV wrote:
> > 
> > On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote:
> > >On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote:
> > >>From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > >>
> > >>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>
> > >>Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > >>---
> > >>  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 950e238..cf377f5 100644
> > >>--- a/drivers/usb/dwc3/core.c
> > >>+++ b/drivers/usb/dwc3/core.c
> > >>@@ -33,6 +33,7 @@
> > >>  #include <linux/usb/gadget.h>
> > >>  #include <linux/usb/of.h>
> > >>  #include <linux/usb/otg.h>
> > >>+#include <linux/usb/hcd.h>
> > >This is not needed anymore
> > >
> > >>  #include "core.h"
> > >>  #include "gadget.h"
> > >>@@ -1787,6 +1788,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);
> > >>@@ -1936,6 +1938,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);
> > >This isn't used anymore, delete it
> > My bad, Will fix this in next version.
> > >>  	switch (dwc->current_dr_role) {
> > >>  	case DWC3_GCTL_PRTCAP_DEVICE:
> > >>@@ -1948,10 +1951,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 ||
> > >>@@ -1967,6 +1967,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_wakeup_path(dwc->dev)) {
> > >nit: the indentation is odd, align it with device_may_wakeup()?
> > Sure, Will take care of it.
> > >>+				dwc->phy_power_off = false;
> > >>+			} else {
> > >>+				dwc->phy_power_off = true;
> > >>+				dwc3_core_exit(dwc);
> > >As commented earlier, taking the controller and PHYs completely down causes a
> > >significant power draw in some USB clients. Let's clarify what the specific
> > >benefits are of doing dwc3_core_exit() vs. entering a low power mode.
> > Sure, once we come to a conclusion on this, I will refresh the patches.
> 
> I think, Matthias is asking you to clarify in the commit description. we can
> even quote Matthias observations.

Actually I would like to have a discussion about the benefits of powering down
the controller and PHYs vs. entering a low power state. Maybe there are good
reasons for powering everything down (e.g. significant power savings), but
as we have seen there are also significant downsides, so let's make sure
we understand both.

Re: [v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Matthias Kaehlcke]

On Fri, May 06, 2022 at 08:51:22AM -0700, Matthias Kaehlcke wrote:
> On Fri, May 06, 2022 at 10:44:48AM +0530, Pavan Kondeti wrote:
> > On Fri, May 06, 2022 at 10:41:01AM +0530, Krishna Kurapati PSSNV wrote:
> > > 
> > > On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote:
> > > >On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote:
> > > >>From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > >>
> > > >>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>
> > > >>Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > >>---
> > > >>  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 950e238..cf377f5 100644
> > > >>--- a/drivers/usb/dwc3/core.c
> > > >>+++ b/drivers/usb/dwc3/core.c
> > > >>@@ -33,6 +33,7 @@
> > > >>  #include <linux/usb/gadget.h>
> > > >>  #include <linux/usb/of.h>
> > > >>  #include <linux/usb/otg.h>
> > > >>+#include <linux/usb/hcd.h>
> > > >This is not needed anymore
> > > >
> > > >>  #include "core.h"
> > > >>  #include "gadget.h"
> > > >>@@ -1787,6 +1788,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);
> > > >>@@ -1936,6 +1938,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);
> > > >This isn't used anymore, delete it
> > > My bad, Will fix this in next version.
> > > >>  	switch (dwc->current_dr_role) {
> > > >>  	case DWC3_GCTL_PRTCAP_DEVICE:
> > > >>@@ -1948,10 +1951,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 ||
> > > >>@@ -1967,6 +1967,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_wakeup_path(dwc->dev)) {
> > > >nit: the indentation is odd, align it with device_may_wakeup()?
> > > Sure, Will take care of it.
> > > >>+				dwc->phy_power_off = false;
> > > >>+			} else {
> > > >>+				dwc->phy_power_off = true;
> > > >>+				dwc3_core_exit(dwc);
> > > >As commented earlier, taking the controller and PHYs completely down causes a
> > > >significant power draw in some USB clients. Let's clarify what the specific
> > > >benefits are of doing dwc3_core_exit() vs. entering a low power mode.
> > > Sure, once we come to a conclusion on this, I will refresh the patches.
> > 
> > I think, Matthias is asking you to clarify in the commit description. we can
> > even quote Matthias observations.
> 
> Actually I would like to have a discussion about the benefits of powering down
> the controller and PHYs vs. entering a low power state. Maybe there are good
> reasons for powering everything down (e.g. significant power savings), but
> as we have seen there are also significant downsides, so let's make sure
> we understand both.

I found this, as I commented on the other thread:

  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.

Let's move forward with the core/PHYs off for now and try to
come up with a solution (e.g. a DT property that indicates
that the core/PHYs can remain powererd) in a separate
patch/series.

[v15 4/6] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs

[Krishna Kurapati]

From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

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>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 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

[v15 5/6] usb: dwc3: qcom: Configure wakeup interrupts during suspend

[Krishna Kurapati]

From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

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>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 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

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

[Krishna Kurapati]

From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

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>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 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