[permalink] [raw]

Subject: Re: [PATCH v2 9/9] usb: dwc3: qcom: clean up suspend callbacks

On Thu, Aug 04, 2022 at 05:10:01PM +0200, Johan Hovold wrote:
> Clean up the suspend callbacks by separating the error and success paths
> to improve readability.
>
> Also drop a related redundant initialisation.
>
> Signed-off-by: Johan Hovold <[email protected]>

Reviewed-by: Matthias Kaehlcke <[email protected]>

2022-08-04 17:17:41

by Matthias Kaehlcke

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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.

2022-08-04 19:03:06

by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

Hi John,

On 8/4/22 08:09, Johan Hovold wrote:
> A recent change added a dependency to the USB host stack and broke
> gadget-only builds of the driver.
>
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Reported-by: Randy Dunlap <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>

LGTM. Thanks.

Reviewed-by: Randy Dunlap <[email protected]>

> ---
>
> Changes in v2
> - new patch
>
> drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index be2e3dd36440..e9364141661b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> * currently supports only 1 port per controller. So
> * this is sufficient.
> */
> +#ifdef CONFIG_USB
> udev = usb_hub_find_child(hcd->self.root_hub, 1);
> -
> +#else
> + udev = NULL;
> +#endif
> if (!udev)
> return USB_SPEED_UNKNOWN;
>

--
~Randy

2022-08-04 20:38:41

by Matthias Kaehlcke

[permalink] [raw]

On Mon, Aug 08, 2022 at 03:34:10PM +0200, Johan Hovold wrote:
> On Mon, Aug 08, 2022 at 03:05:36PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > > A recent change added a dependency to the USB host stack and broke
> > > gadget-only builds of the driver.
> > >
> > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> > > Reported-by: Randy Dunlap <[email protected]>
> > > Signed-off-by: Johan Hovold <[email protected]>
> > > ---
> > >
> > > Changes in v2
> > > - new patch
> > >
> > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index be2e3dd36440..e9364141661b 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> > > * currently supports only 1 port per controller. So
> > > * this is sufficient.
> > > */
> > > +#ifdef CONFIG_USB
> > > udev = usb_hub_find_child(hcd->self.root_hub, 1);
> >
> > If a gadget driver needs this for some reason, then the #ifdef should be
> > put in a .h file, not in a .c file.
>
> Yeah, if we're keeping this long-term then yes, and possibly also
> otherwise.
>
> > But step back a minute and ask why a host-config-only function is being
> > called when a device is in gadget-only mode? This feels like a
> > design/logic issue in this file, NOT something to paper over with a
> > #ifdef in a .c file
>
> We're not as I'm fixing that bug in later in the series. I should
> probably have put this one after that fix, but figured fixing the build
> was more important than a harder-to-hit NULL-deref due to non-host mode
> not being considered when the offending series was merged.
>
> > This implies that if this device is NOT in a host configuration, then
> > the suspend path of it is not configured properly at all, as why would
> > it be checking or caring about this at all if this is in gadget-only
> > mode?
>
> Right, so see path 6/9 which addresses this by only calling this hack
> when in host mode:
>
> https://lore.kernel.org/all/[email protected]/
>
> > Something else is wrong here, let's fix the root problem please. Maybe
> > this driver should just never be built in gadget-only mode, as it is
> > never intended to support that option?
>
> The problem is commit 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup
> interrupts during suspend"), which I considered simply reverting but as
> that breaks suspend completely on some boards I decided to try and fix
> it up while we work on a proper long-term solution (i.e. for how the
> dwc/xhci layers should be communicating to implement this).
>
> Remember that it took two years and 21 revisions to get to the state
> we're at now after you merged the wakeup series in June.

Very good point. This is a mess, thanks for cleaning it up. I've
applied this series now and will get it into 6.0-final, hopefully all
should be good now.

thanks for doing this work.

greg k-h