[PATCH] usb: dwc3: keep PHYs disabled during suspend

[PATCH] usb: dwc3: keep PHYs disabled during suspend

[Johan Hovold]

Commit 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system
suspend") started leaving the PHYs enabled during suspend for
wakeup-capable controllers even though it turns out this had nothing to
do with wakeup.

Rather, the wakeup capability flag was (ab-)used as a proxy to configure
the suspend behaviour in an attempt to reduce power leakage on some
platforms.

Stop abusing the wakeup configuration and restore the 5.19 behaviour of
keeping the PHYs powered off during suspend. If needed, a dedicated
mechanism for configuring the PHY power state during suspend can be
added later.

Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
Link: https://lore.kernel.org/r/Yuv7AM/5jtO/pgcm@google.com
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/core.c      | 4 ++--
 drivers/usb/dwc3/dwc3-qcom.c | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8c8e32651473..0cdb6be720e1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1983,7 +1983,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) && !device_may_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg)) {
 			dwc3_core_exit(dwc);
 			break;
 		}
@@ -2044,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg)) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 9a94b1ab8f7a..9995395baa12 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -904,7 +904,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
 	device_init_wakeup(&pdev->dev, wakeup_source);
-	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
 
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
-- 
2.35.1

Re: [PATCH] usb: dwc3: keep PHYs disabled during suspend

[Matthias Kaehlcke]

Hi Johan,

On Tue, Aug 23, 2022 at 02:40:47PM +0200, Johan Hovold wrote:
> Commit 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system
> suspend") started leaving the PHYs enabled during suspend for
> wakeup-capable controllers even though it turns out this had nothing to
> do with wakeup.
> 
> Rather, the wakeup capability flag was (ab-)used as a proxy to configure
> the suspend behaviour in an attempt to reduce power leakage on some
> platforms.
> 
> Stop abusing the wakeup configuration and restore the 5.19 behaviour of
> keeping the PHYs powered off during suspend. If needed, a dedicated
> mechanism for configuring the PHY power state during suspend can be
> added later.
> 
> Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> Link: https://lore.kernel.org/r/Yuv7AM/5jtO/pgcm@google.com
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/usb/dwc3/core.c      | 4 ++--
>  drivers/usb/dwc3/dwc3-qcom.c | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 8c8e32651473..0cdb6be720e1 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1983,7 +1983,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) && !device_may_wakeup(dwc->dev)) {
> +		if (!PMSG_IS_AUTO(msg)) {

My assumption was that the PHYs need to be powered for wakeup to work, but
apparently that isn't the case, wakeup still works on sc7x80 with this part
of this patch.

>  			dwc3_core_exit(dwc);
>  			break;
>  		}
> @@ -2044,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  		spin_unlock_irqrestore(&dwc->lock, flags);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
> -		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> +		if (!PMSG_IS_AUTO(msg)) {
>  			ret = dwc3_core_init_for_resume(dwc);
>  			if (ret)
>  				return ret;
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 9a94b1ab8f7a..9995395baa12 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -904,7 +904,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>  	device_init_wakeup(&pdev->dev, wakeup_source);
> -	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);

Surprisingly this part breaks wakeup on sc7x80, with the above removal
of the device_may_wakeup() checks it is not clear to me why wakeup needs
to be enabled for the core.

Re: [PATCH] usb: dwc3: keep PHYs disabled during suspend

[Johan Hovold]

On Tue, Aug 23, 2022 at 11:33:18AM -0700, Matthias Kaehlcke wrote:
> Hi Johan,
> 
> On Tue, Aug 23, 2022 at 02:40:47PM +0200, Johan Hovold wrote:
> > Commit 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system
> > suspend") started leaving the PHYs enabled during suspend for
> > wakeup-capable controllers even though it turns out this had nothing to
> > do with wakeup.
> > 
> > Rather, the wakeup capability flag was (ab-)used as a proxy to configure
> > the suspend behaviour in an attempt to reduce power leakage on some
> > platforms.
> > 
> > Stop abusing the wakeup configuration and restore the 5.19 behaviour of
> > keeping the PHYs powered off during suspend. If needed, a dedicated
> > mechanism for configuring the PHY power state during suspend can be
> > added later.
> > 
> > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> > Link: https://lore.kernel.org/r/Yuv7AM/5jtO/pgcm@google.com
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/usb/dwc3/core.c      | 4 ++--
> >  drivers/usb/dwc3/dwc3-qcom.c | 1 -
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 8c8e32651473..0cdb6be720e1 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1983,7 +1983,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) && !device_may_wakeup(dwc->dev)) {
> > +		if (!PMSG_IS_AUTO(msg)) {
> 
> My assumption was that the PHYs need to be powered for wakeup to work, but
> apparently that isn't the case, wakeup still works on sc7x80 with this part
> of this patch.

Thanks for confirming.

> >  			dwc3_core_exit(dwc);
> >  			break;
> >  		}
> > @@ -2044,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> >  		spin_unlock_irqrestore(&dwc->lock, flags);
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> > -		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> > +		if (!PMSG_IS_AUTO(msg)) {
> >  			ret = dwc3_core_init_for_resume(dwc);
> >  			if (ret)
> >  				return ret;
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 9a94b1ab8f7a..9995395baa12 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -904,7 +904,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  
> >  	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> >  	device_init_wakeup(&pdev->dev, wakeup_source);
> > -	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> 
> Surprisingly this part breaks wakeup on sc7x80, with the above removal
> of the device_may_wakeup() checks it is not clear to me why wakeup needs
> to be enabled for the core.

I can't explain that behaviour either. This change doesn't affect the
wakeup_path flag and genpd, and notably wakeup still works here with
sc8280xp.

Could it be some Chromium user-space issue in that it expects all
devices on the wakeup path to be wakeup capable? Note that the
xhci-plat driver (e.g. for the descendant xhci-hcd.1.auto device)
unconditionally sets the wakeup-capable flag (but leaves it disabled by
default).

I guess we could do something similar for the dwc3 core device, but we'd
need to figure out if and why that is at all needed first.

Can you verify that the wakeup source (e.g. keyboard) you're using still
has power/wakeup set to "enabled"?

Johan

Re: [PATCH] usb: dwc3: keep PHYs disabled during suspend

[Matthias Kaehlcke]

On Wed, Aug 24, 2022 at 10:27:44AM +0200, Johan Hovold wrote:
> On Tue, Aug 23, 2022 at 11:33:18AM -0700, Matthias Kaehlcke wrote:
> > Hi Johan,
> > 
> > On Tue, Aug 23, 2022 at 02:40:47PM +0200, Johan Hovold wrote:
> > > Commit 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system
> > > suspend") started leaving the PHYs enabled during suspend for
> > > wakeup-capable controllers even though it turns out this had nothing to
> > > do with wakeup.
> > > 
> > > Rather, the wakeup capability flag was (ab-)used as a proxy to configure
> > > the suspend behaviour in an attempt to reduce power leakage on some
> > > platforms.
> > > 
> > > Stop abusing the wakeup configuration and restore the 5.19 behaviour of
> > > keeping the PHYs powered off during suspend. If needed, a dedicated
> > > mechanism for configuring the PHY power state during suspend can be
> > > added later.
> > > 
> > > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> > > Link: https://lore.kernel.org/r/Yuv7AM/5jtO/pgcm@google.com
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >  drivers/usb/dwc3/core.c      | 4 ++--
> > >  drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 8c8e32651473..0cdb6be720e1 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1983,7 +1983,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) && !device_may_wakeup(dwc->dev)) {
> > > +		if (!PMSG_IS_AUTO(msg)) {
> > 
> > My assumption was that the PHYs need to be powered for wakeup to work, but
> > apparently that isn't the case, wakeup still works on sc7x80 with this part
> > of this patch.
> 
> Thanks for confirming.
> 
> > >  			dwc3_core_exit(dwc);
> > >  			break;
> > >  		}
> > > @@ -2044,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> > >  		spin_unlock_irqrestore(&dwc->lock, flags);
> > >  		break;
> > >  	case DWC3_GCTL_PRTCAP_HOST:
> > > -		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> > > +		if (!PMSG_IS_AUTO(msg)) {
> > >  			ret = dwc3_core_init_for_resume(dwc);
> > >  			if (ret)
> > >  				return ret;
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index 9a94b1ab8f7a..9995395baa12 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -904,7 +904,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > >  
> > >  	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> > >  	device_init_wakeup(&pdev->dev, wakeup_source);
> > > -	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> > 
> > Surprisingly this part breaks wakeup on sc7x80, with the above removal
> > of the device_may_wakeup() checks it is not clear to me why wakeup needs
> > to be enabled for the core.
> 
> I can't explain that behaviour either. This change doesn't affect the
> wakeup_path flag and genpd, and notably wakeup still works here with
> sc8280xp.
> 
> Could it be some Chromium user-space issue in that it expects all
> devices on the wakeup path to be wakeup capable? Note that the
> xhci-plat driver (e.g. for the descendant xhci-hcd.1.auto device)
> unconditionally sets the wakeup-capable flag (but leaves it disabled by
> default).
> 
> I guess we could do something similar for the dwc3 core device, but we'd
> need to figure out if and why that is at all needed first.
> 
> Can you verify that the wakeup source (e.g. keyboard) you're using still
> has power/wakeup set to "enabled"?

I confirmed that the wakeup flag of the wakeup source is still enabled when
the wakeup source suspends.

It turns out the dwc3 core wakeup flags are evaluated by:

	int usb_phy_roothub_suspend(struct device *controller_dev,
	                            struct usb_phy_roothub *phy_roothub)
	{
		usb_phy_roothub_power_off(phy_roothub);

		/* keep the PHYs initialized so the device can wake up the system */
		if (device_may_wakeup(controller_dev))
			return 0;

		return usb_phy_roothub_exit(phy_roothub);
	}

'controller_dev' is the dwc3 core. The root hub is un-initialized when wakeup is
disabled. That causes wakeup to fail, and also happens to be the/one cause of
the high power consumption of an onboard USB hub that I mentioned earlier in
another thread.

Re: [PATCH] usb: dwc3: keep PHYs disabled during suspend

[Johan Hovold]

On Wed, Aug 24, 2022 at 09:41:49AM -0700, Matthias Kaehlcke wrote:
> On Wed, Aug 24, 2022 at 10:27:44AM +0200, Johan Hovold wrote:
> > On Tue, Aug 23, 2022 at 11:33:18AM -0700, Matthias Kaehlcke wrote:
> > > Hi Johan,
> > > 
> > > On Tue, Aug 23, 2022 at 02:40:47PM +0200, Johan Hovold wrote:
> > > > Commit 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system
> > > > suspend") started leaving the PHYs enabled during suspend for
> > > > wakeup-capable controllers even though it turns out this had nothing to
> > > > do with wakeup.
> > > > 
> > > > Rather, the wakeup capability flag was (ab-)used as a proxy to configure
> > > > the suspend behaviour in an attempt to reduce power leakage on some
> > > > platforms.
> > > > 
> > > > Stop abusing the wakeup configuration and restore the 5.19 behaviour of
> > > > keeping the PHYs powered off during suspend. If needed, a dedicated
> > > > mechanism for configuring the PHY power state during suspend can be
> > > > added later.
> > > > 
> > > > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> > > > Link: https://lore.kernel.org/r/Yuv7AM/5jtO/pgcm@google.com
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > > ---

> > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > > index 9a94b1ab8f7a..9995395baa12 100644
> > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > @@ -904,7 +904,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > > >  
> > > >  	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> > > >  	device_init_wakeup(&pdev->dev, wakeup_source);
> > > > -	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> > > 
> > > Surprisingly this part breaks wakeup on sc7x80, with the above removal
> > > of the device_may_wakeup() checks it is not clear to me why wakeup needs
> > > to be enabled for the core.
> > 
> > I can't explain that behaviour either. This change doesn't affect the
> > wakeup_path flag and genpd, and notably wakeup still works here with
> > sc8280xp.

> It turns out the dwc3 core wakeup flags are evaluated by:
> 
> 	int usb_phy_roothub_suspend(struct device *controller_dev,
> 	                            struct usb_phy_roothub *phy_roothub)
> 	{
> 		usb_phy_roothub_power_off(phy_roothub);
> 
> 		/* keep the PHYs initialized so the device can wake up the system */
> 		if (device_may_wakeup(controller_dev))
> 			return 0;
> 
> 		return usb_phy_roothub_exit(phy_roothub);
> 	}
> 
> 'controller_dev' is the dwc3 core. The root hub is un-initialized when wakeup is
> disabled. That causes wakeup to fail, and also happens to be the/one cause of
> the high power consumption of an onboard USB hub that I mentioned earlier in
> another thread.

Bah, I forgot about the USB core PHY management mess. This should be
disabled as the dwc3 driver manages the PHYs itself.

But because it currently enabled, the PHY was kept powered-on by USB
core (unless dropping device_init_wakeup()) and this indicates that that
is indeed needed for wakeup on your platform.

So this patch should be dropped, and I'll send a patch disabling the USB
core PHY management for dwc3 instead.

Johan