Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

[Matthias Kaehlcke]

On Sat, Aug 06, 2022 at 06:22:37PM +0200, Johan Hovold wrote:
> On Fri, Aug 05, 2022 at 09:58:35AM -0700, Matthias Kaehlcke wrote:
> > On Thu, Aug 04, 2022 at 09:59:44AM -0700, Matthias Kaehlcke wrote:
> > > On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > > > It is the Qualcomm glue wakeup interrupts that may be able to wake the
> > > > system from suspend and this can now be described in the devicetree.
> > > > 
> > > > Move the wakeup-source property handling over from the core driver and
> > > > instead propagate the capability setting to the core device during
> > > > probe.
> > > > 
> > > > This is needed as there is currently no way for the core driver to query
> > > > the wakeup setting of the glue device, but it is the core driver that
> > > > manages the PHY power state during suspend.
> > > > 
> > > > Also don't leave the PHYs enabled when system wakeup has been disabled
> > > > through sysfs.
> > > > 
> > > > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> > > > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > > > ---
> > > >  drivers/usb/dwc3/core.c      | 5 ++---
> > > >  drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
> > > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 16d1f328775f..8c8e32651473 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -1822,7 +1822,6 @@ 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);
> > > > @@ -1984,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_can_wakeup(dwc->dev)) {
> > > > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> > > 
> > > Let me explain the rationale for why device_can_wakeup() was used here:
> > > 
> > > On QCOM SC7180 based Chromebooks we observe that the onboard USB hub consumes
> > > ~80 mW during system suspend when the PHYs are disabled, as opposed to ~17 mW
> > > when the PHYs remain enabled. This is a significant delta when the device is
> > > on a battery power.
> > > 
> > > The initial idea was to leave the PHYs always enabled (in a low power mode),
> > > but then I dug up commit c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs
> > > on system_suspend in host mode"), which provides a rationale for the PHYs
> > > being powered off:
> > > 
> > >   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.
> > > 
> > > Unfortunately we don't know which platforms are impacted by this. The idea
> > > behind using device_can_wakeup() was to use it as a proxy for platforms
> > > that are *not* impacted. If a platform supports USB wakeup supposedly the
> > > SoC can enter its low power mode during system suspend with the PHYs
> > > enabled.
> > > 
> > > By now I'm not 100% sure if the above assumption is correct. I recently
> > > saw allegations that the power consumption of a given QC SoC with USB
> > > wakeup support drops significantly when wakeup is disabled (i.e. when
> > > the PHYs are off), but haven't confirmed this yet.
> > 
> > So far power measurements don't support the claim that SoC power
> > consumption is substantially lower with USB wakeup disabled/the PHYs
> > off. I asked the person who made that claim to provide more
> > details/data (the discussion is in an internal forum).
> 
> Thanks for the background on this. So clearly it has nothing to with
> supporting wakeup as the commit summary claimed, and this should
> probably never have been made to depend on wakeup capability either.

To be clear, there are two different (supposed) impacts on suspend power:

1. with the PHYs powered off an onboard hub on SC7180/SC7280 boards draws
   ~80mW during system suspend, vs. ~17mW with the PHYs being on. This is
   confirmed.

   For SC7180/SC7280 Chrome OS boards in particular it would be ok to
   power the PHYs off based on device_may_wakeup(), since Chrome OS
   leaves USB wakeup enabled, hence the PHYs would remain powered as
   desired.

   However boards that opt for disabling USB wakeup could be impacted
   by increased power consumption of USB peripherals, as seen with the
   hub of SC7180/SC7280 Chrome OS boards.

2. with the PHYs on during system suspend allegedly some QC SoCs can't
   reach their lowest power mode (commit c4a5153e87fd). I don't know
   which SoCs are impacted.

   Someone from QC claims that SC7280 has significantly lower power
   consumption with "USB wakeup disabled", so far this has not been
   confirmed by my colleague who takes power measurements, I'm in
   the process of clarifying what "USB wakeup disabled" exactly
   means in this context (e.g. no wakeup source flag vs. no wakeup
   capable device plugged).

> I'll revisit this after the merge window, but perhaps we should just rip
> this out completely and use a more descriptive property to configure the
> PHY suspend state. But depending on the results from your internal
> measurements, perhaps not even that is needed.

Ok, I'll keep you posted on power findings on our side, though that will
only cover SC7180/SC7280.