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

[Johan Hovold <johan@kernel.org>]

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