| Message ID | 20220802151404.1797-5-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote: > A device must enable wakeups during runtime suspend regardless of > whether it is capable and allowed to wake the system up from system > suspend. > > Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend. In how far is runtime PM actually supported/used by this driver? The device is set 'active' in _probe(), and there are no other pm_runtime_* calls, except in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from 'active' into 'suspended'?
On Wed, Aug 03, 2022 at 02:58:33PM -0700, Matthias Kaehlcke wrote: > On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote: > > A device must enable wakeups during runtime suspend regardless of > > whether it is capable and allowed to wake the system up from system > > suspend. > > > > Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state") > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend. > > In how far is runtime PM actually supported/used by this driver? The device is > set 'active' in _probe(), and there are no other pm_runtime_* calls, except > in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from > 'active' into 'suspended'? It will be runtime suspended when the child (core) device suspends, but you need to enable runtime PM through sysfs first. And the controller is resumed in the wakeup-interrupt handler for the runtime PM case. It seems to work ok, and it looks like the driver has supported this since it was first merged. Johan
On Thu, Aug 04, 2022 at 09:35:16AM +0200, Johan Hovold wrote: > On Wed, Aug 03, 2022 at 02:58:33PM -0700, Matthias Kaehlcke wrote: > > On Tue, Aug 02, 2022 at 05:14:00PM +0200, Johan Hovold wrote: > > > A device must enable wakeups during runtime suspend regardless of > > > whether it is capable and allowed to wake the system up from system > > > suspend. > > > > > > Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state") > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > Ah, I wasn't aware that the same wakeup mechanism is used in runtime suspend. > > > > In how far is runtime PM actually supported/used by this driver? The device is > > set 'active' in _probe(), and there are no other pm_runtime_* calls, except > > in dwc3_qcom_remove() and qcom_dwc3_resume_irq(). How does the device get from > > 'active' into 'suspended'? > > It will be runtime suspended when the child (core) device suspends, but > you need to enable runtime PM through sysfs first. Thanks for the clarification. After enabling runtime suspend for the dwc3 core, dwc3 glue and the xHCI the dwc3-qcom enters autosuspend when the delay expires. > And the controller is resumed in the wakeup-interrupt handler for the > runtime PM case. > > It seems to work ok, and it looks like the driver has supported this > since it was first merged. With and without your patch dwc3-qcom enters autosuspend and stays there. USB devices like a mouse or a USB to Ethernet adapter keep working while the glue is suspended. How is the runtime resume triggered for the dwc3 glue? Sorry if my questions are very basic, so far I haven't dealt much with autosuspend and I'm trying to get a better understanding in the context of the dwc3 and why it is currently broken.
On Thu, Aug 04, 2022 at 08:35:10AM -0700, Matthias Kaehlcke wrote: > On Thu, Aug 04, 2022 at 09:35:16AM +0200, Johan Hovold wrote: > After enabling runtime suspend for the dwc3 core, dwc3 glue and the xHCI > the dwc3-qcom enters autosuspend when the delay expires. > > > And the controller is resumed in the wakeup-interrupt handler for the > > runtime PM case. > > > > It seems to work ok, and it looks like the driver has supported this > > since it was first merged. > > With and without your patch dwc3-qcom enters autosuspend and stays there. > USB devices like a mouse or a USB to Ethernet adapter keep working while > the glue is suspended. Are you sure you're looking at the right controller? And that it is actually suspended? If you plug in a keyboard, enable autosuspend for all devices in the path (from glue to the keyboard device) and type away, then the controller must remain active. Stop typing, and all devices in the chain should suspend. > How is the runtime resume triggered for the dwc3 glue? Either by the host driver when it needs to access the device, or by the device if it is remote-wakeup capable (e.g. a keyboard, but not necessarily a speaker). Note that the latter part is what is broken currently as the wakeup interrupts were not enabled and those are needed to wake up sc8280xp when the dwc3 glue has been runtime suspended. Johan
On Thu, Aug 04, 2022 at 06:04:53PM +0200, Johan Hovold wrote: > On Thu, Aug 04, 2022 at 08:35:10AM -0700, Matthias Kaehlcke wrote: > > On Thu, Aug 04, 2022 at 09:35:16AM +0200, Johan Hovold wrote: > > > After enabling runtime suspend for the dwc3 core, dwc3 glue and the xHCI > > the dwc3-qcom enters autosuspend when the delay expires. > > > > > And the controller is resumed in the wakeup-interrupt handler for the > > > runtime PM case. > > > > > > It seems to work ok, and it looks like the driver has supported this > > > since it was first merged. > > > > With and without your patch dwc3-qcom enters autosuspend and stays there. > > USB devices like a mouse or a USB to Ethernet adapter keep working while > > the glue is suspended. > > Are you sure you're looking at the right controller? And that it is > actually suspended? Good point! My mind was set on a SC7180 system, which has a single dwc3 controller, but this time I was tinkering on a SC7280 board, which has two dwc3, and obviously I was looking at the wrong one (－‸ლ) > If you plug in a keyboard, enable autosuspend for all devices in the > path (from glue to the keyboard device) and type away, then the > controller must remain active. Stop typing, and all devices in the chain > should suspend. That's what I expected, and now that I'm looking at the right controller I'm seeing it. I wondered whether the glue device was somehow special. > > How is the runtime resume triggered for the dwc3 glue? > > Either by the host driver when it needs to access the device, or by the > device if it is remote-wakeup capable (e.g. a keyboard, but not > necessarily a speaker). > > Note that the latter part is what is broken currently as the wakeup > interrupts were not enabled and those are needed to wake up sc8280xp > when the dwc3 glue has been runtime suspended. Thanks for helping me to get a better understanding!
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index b75ff40f75a2..57d3a0e6f280 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -395,7 +395,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0); } -static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) +static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) { u32 val; int i, ret; @@ -414,7 +414,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) if (ret) dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret); - if (device_may_wakeup(qcom->dev)) { + if (wakeup) { qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); dwc3_qcom_enable_interrupts(qcom); } @@ -424,7 +424,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) return 0; } -static int dwc3_qcom_resume(struct dwc3_qcom *qcom) +static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) { int ret; int i; @@ -432,7 +432,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) if (!qcom->is_suspended) return 0; - if (device_may_wakeup(qcom->dev)) + if (wakeup) dwc3_qcom_disable_interrupts(qcom); for (i = 0; i < qcom->num_clocks; i++) { @@ -939,9 +939,11 @@ static int dwc3_qcom_remove(struct platform_device *pdev) static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev) { struct dwc3_qcom *qcom = dev_get_drvdata(dev); + bool wakeup = device_may_wakeup(dev); int ret = 0; - ret = dwc3_qcom_suspend(qcom); + + ret = dwc3_qcom_suspend(qcom, wakeup); if (!ret) qcom->pm_suspended = true; @@ -951,9 +953,10 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev) static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev) { struct dwc3_qcom *qcom = dev_get_drvdata(dev); + bool wakeup = device_may_wakeup(dev); int ret; - ret = dwc3_qcom_resume(qcom); + ret = dwc3_qcom_resume(qcom, wakeup); if (!ret) qcom->pm_suspended = false; @@ -964,14 +967,14 @@ static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev) { struct dwc3_qcom *qcom = dev_get_drvdata(dev); - return dwc3_qcom_suspend(qcom); + return dwc3_qcom_suspend(qcom, true); } static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev) { struct dwc3_qcom *qcom = dev_get_drvdata(dev); - return dwc3_qcom_resume(qcom); + return dwc3_qcom_resume(qcom, true); } static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
A device must enable wakeups during runtime suspend regardless of whether it is capable and allowed to wake the system up from system suspend. Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state") Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)