| Message ID | 20220804151001.23612-9-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
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@kernel.org> > --- > 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.
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@kernel.org> > > --- > > 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).
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. > The "wakeup-source" property is still defined in the DWC binding, so other platform glue drivers are free to assume that wakeup capability is handled by the DWC driver. > 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. > Can you please elaborate on how this is handled in the patch? Thanks, Mani > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > 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)) { > dwc3_core_exit(dwc); > break; > } > @@ -2045,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_can_wakeup(dwc->dev)) { > + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > 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 6ae0b7fc4e2c..b05f67d206d2 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > struct resource *res, *parent_res = NULL; > int ret, i; > bool ignore_pipe_clk; > + bool wakeup_source; > > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > if (!qcom) > @@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > if (ret) > goto interconnect_exit; > > - device_init_wakeup(&pdev->dev, 1); > + 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); > pm_runtime_enable(dev); > -- > 2.35.1 >
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@kernel.org> > > > --- > > > 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. 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. Johan
On Sat, Aug 06, 2022 at 08:27:19PM +0530, Manivannan Sadhasivam 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. > The "wakeup-source" property is still defined in the DWC binding, so other > platform glue drivers are free to assume that wakeup capability is handled by > the DWC driver. No, just because the binding says that the hardware supports something doesn't mean it's implemented. And in this case it isn't. There's no core support for wakeup and this is just used to determine the PHY power state during suspend (see my reply to Matthias). But this is also why I initially suggested reverting the binding change until some platform actually turns out to need it. > > 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. > > > > Can you please elaborate on how this is handled in the patch? Generally device_can_wakeup() should not be used to make policy decisions (e.g. whether to power of the PHYs) as this should be configurable through sysfs which is honoured by device_may_wakeup(). But I'll revisit this in a couple of weeks. We should probably just revert the patch that made the PHY power state depend on device_can_wakeup() as it apparently isn't needed for wakeup at all. > > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend") > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > 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)) { > > dwc3_core_exit(dwc); > > break; > > } > > @@ -2045,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_can_wakeup(dwc->dev)) { > > + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > > 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 6ae0b7fc4e2c..b05f67d206d2 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > struct resource *res, *parent_res = NULL; > > int ret, i; > > bool ignore_pipe_clk; > > + bool wakeup_source; > > > > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > > if (!qcom) > > @@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > if (ret) > > goto interconnect_exit; > > > > - device_init_wakeup(&pdev->dev, 1); > > + 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); > > pm_runtime_enable(dev); > > -- > > 2.35.1 Johan
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@kernel.org> > > > > --- > > > > 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.
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)) { dwc3_core_exit(dwc); break; } @@ -2045,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_can_wakeup(dwc->dev)) { + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { 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 6ae0b7fc4e2c..b05f67d206d2 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) struct resource *res, *parent_res = NULL; int ret, i; bool ignore_pipe_clk; + bool wakeup_source; qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); if (!qcom) @@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ret) goto interconnect_exit; - device_init_wakeup(&pdev->dev, 1); + 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); pm_runtime_enable(dev);
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@kernel.org> --- drivers/usb/dwc3/core.c | 5 ++--- drivers/usb/dwc3/dwc3-qcom.c | 6 +++++- 2 files changed, 7 insertions(+), 4 deletions(-)