| Message ID | 20220823124047.14634-1-johan+linaro@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: keep PHYs disabled during suspend | expand |
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.
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
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.
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);
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(-)