| Message ID | 1651740973-7944-1-git-send-email-quic_kriskura@quicinc.com |
|---|---|
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote: > From: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > During suspend read the status of all port and set hs phy mode > based on current speed. Use this hs phy mode to configure wakeup > interrupts in qcom glue driver. > > Check wakeup-source property for dwc3 core node to set the > wakeup capability. Drop the device_init_wakeup call from > runtime suspend and resume. > > Also check during suspend if any wakeup capable devices are > connected to the controller (directly or through hubs), if there > are none set a flag to indicate that the PHY is powered > down during suspend. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++------------- > drivers/usb/dwc3/core.h | 4 ++++ > drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++ > 3 files changed, 48 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 950e238..cf377f5 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -33,6 +33,7 @@ > #include <linux/usb/gadget.h> > #include <linux/usb/of.h> > #include <linux/usb/otg.h> > +#include <linux/usb/hcd.h> This is not needed anymore > > #include "core.h" > #include "gadget.h" > @@ -1787,6 +1788,7 @@ 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); > @@ -1936,6 +1938,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > unsigned long flags; > u32 reg; > + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci); This isn't used anymore, delete it > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > @@ -1948,10 +1951,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)) { > - dwc3_core_exit(dwc); > - break; > - } > + dwc3_check_phy_speed_mode(dwc); > > /* Let controller to suspend HSPHY before PHY driver suspends */ > if (dwc->dis_u2_susphy_quirk || > @@ -1967,6 +1967,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > phy_pm_runtime_put_sync(dwc->usb2_generic_phy); > phy_pm_runtime_put_sync(dwc->usb3_generic_phy); > + > + if (!PMSG_IS_AUTO(msg)) { > + if (device_may_wakeup(dwc->dev) && > + device_wakeup_path(dwc->dev)) { nit: the indentation is odd, align it with device_may_wakeup()? > + dwc->phy_power_off = false; > + } else { > + dwc->phy_power_off = true; > + dwc3_core_exit(dwc); As commented earlier, taking the controller and PHYs completely down causes a significant power draw in some USB clients. Let's clarify what the specific benefits are of doing dwc3_core_exit() vs. entering a low power mode.
On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote: > On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote: >> From: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >> >> During suspend read the status of all port and set hs phy mode >> based on current speed. Use this hs phy mode to configure wakeup >> interrupts in qcom glue driver. >> >> Check wakeup-source property for dwc3 core node to set the >> wakeup capability. Drop the device_init_wakeup call from >> runtime suspend and resume. >> >> Also check during suspend if any wakeup capable devices are >> connected to the controller (directly or through hubs), if there >> are none set a flag to indicate that the PHY is powered >> down during suspend. >> >> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++------------- >> drivers/usb/dwc3/core.h | 4 ++++ >> drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++ >> 3 files changed, 48 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 950e238..cf377f5 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -33,6 +33,7 @@ >> #include <linux/usb/gadget.h> >> #include <linux/usb/of.h> >> #include <linux/usb/otg.h> >> +#include <linux/usb/hcd.h> > This is not needed anymore > >> >> #include "core.h" >> #include "gadget.h" >> @@ -1787,6 +1788,7 @@ 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); >> @@ -1936,6 +1938,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> { >> unsigned long flags; >> u32 reg; >> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci); > This isn't used anymore, delete it My bad, Will fix this in next version. >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> @@ -1948,10 +1951,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)) { >> - dwc3_core_exit(dwc); >> - break; >> - } >> + dwc3_check_phy_speed_mode(dwc); >> >> /* Let controller to suspend HSPHY before PHY driver suspends */ >> if (dwc->dis_u2_susphy_quirk || >> @@ -1967,6 +1967,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> >> phy_pm_runtime_put_sync(dwc->usb2_generic_phy); >> phy_pm_runtime_put_sync(dwc->usb3_generic_phy); >> + >> + if (!PMSG_IS_AUTO(msg)) { >> + if (device_may_wakeup(dwc->dev) && >> + device_wakeup_path(dwc->dev)) { > nit: the indentation is odd, align it with device_may_wakeup()? Sure, Will take care of it. >> + dwc->phy_power_off = false; >> + } else { >> + dwc->phy_power_off = true; >> + dwc3_core_exit(dwc); > As commented earlier, taking the controller and PHYs completely down causes a > significant power draw in some USB clients. Let's clarify what the specific > benefits are of doing dwc3_core_exit() vs. entering a low power mode. Sure, once we come to a conclusion on this, I will refresh the patches.
On Fri, May 06, 2022 at 10:41:01AM +0530, Krishna Kurapati PSSNV wrote: > > On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote: > >On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote: > >>From: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >> > >>During suspend read the status of all port and set hs phy mode > >>based on current speed. Use this hs phy mode to configure wakeup > >>interrupts in qcom glue driver. > >> > >>Check wakeup-source property for dwc3 core node to set the > >>wakeup capability. Drop the device_init_wakeup call from > >>runtime suspend and resume. > >> > >>Also check during suspend if any wakeup capable devices are > >>connected to the controller (directly or through hubs), if there > >>are none set a flag to indicate that the PHY is powered > >>down during suspend. > >> > >>Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > >>--- > >> drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++------------- > >> drivers/usb/dwc3/core.h | 4 ++++ > >> drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++ > >> 3 files changed, 48 insertions(+), 13 deletions(-) > >> > >>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>index 950e238..cf377f5 100644 > >>--- a/drivers/usb/dwc3/core.c > >>+++ b/drivers/usb/dwc3/core.c > >>@@ -33,6 +33,7 @@ > >> #include <linux/usb/gadget.h> > >> #include <linux/usb/of.h> > >> #include <linux/usb/otg.h> > >>+#include <linux/usb/hcd.h> > >This is not needed anymore > > > >> #include "core.h" > >> #include "gadget.h" > >>@@ -1787,6 +1788,7 @@ 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); > >>@@ -1936,6 +1938,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >> { > >> unsigned long flags; > >> u32 reg; > >>+ struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci); > >This isn't used anymore, delete it > My bad, Will fix this in next version. > >> switch (dwc->current_dr_role) { > >> case DWC3_GCTL_PRTCAP_DEVICE: > >>@@ -1948,10 +1951,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)) { > >>- dwc3_core_exit(dwc); > >>- break; > >>- } > >>+ dwc3_check_phy_speed_mode(dwc); > >> /* Let controller to suspend HSPHY before PHY driver suspends */ > >> if (dwc->dis_u2_susphy_quirk || > >>@@ -1967,6 +1967,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >> phy_pm_runtime_put_sync(dwc->usb2_generic_phy); > >> phy_pm_runtime_put_sync(dwc->usb3_generic_phy); > >>+ > >>+ if (!PMSG_IS_AUTO(msg)) { > >>+ if (device_may_wakeup(dwc->dev) && > >>+ device_wakeup_path(dwc->dev)) { > >nit: the indentation is odd, align it with device_may_wakeup()? > Sure, Will take care of it. > >>+ dwc->phy_power_off = false; > >>+ } else { > >>+ dwc->phy_power_off = true; > >>+ dwc3_core_exit(dwc); > >As commented earlier, taking the controller and PHYs completely down causes a > >significant power draw in some USB clients. Let's clarify what the specific > >benefits are of doing dwc3_core_exit() vs. entering a low power mode. > Sure, once we come to a conclusion on this, I will refresh the patches. I think, Matthias is asking you to clarify in the commit description. we can even quote Matthias observations. Thanks, Pavan
On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote: > device_wakeup_path() tells if any of the children devices needs > wakeup. Use this hint to enable/disable wakeup of our device. This > helps the parent device of xhci-plat (like sysdev) to retrieve > the wakeup setting via device_wakeup_path(). > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/host/xhci-plat.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 649ffd8..ad585fa 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) > if (pm_runtime_suspended(dev)) > pm_runtime_resume(dev); > > + if (device_wakeup_path(dev)) { > + if (!device_may_wakeup(dev)) > + device_wakeup_enable(dev); > + } else { > + if (device_may_wakeup(dev)) > + device_wakeup_disable(dev); > + } This code is not self-explantatory and deserves a comment. Enabling/disabling wakeup for the purpose if signalling is a bit of a hack. It might be an acceptable hack as long as it has no side effects. However with the current implementation the wakeup state of the xHCI can be different after resuming than it was before going to suspend: after boot grep -h xhci /sys/class/wakeup/*/name => xhci-hcd.14.auto after suspend w/o wakeup capable device grep -h xhci /sys/class/wakeup/*/name => no results after suspend with wakeup capable device grep -h xhci /sys/class/wakeup/*/name => xhci-hcd.14.auto The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done conditionally though).
On Fri, May 06, 2022 at 10:44:48AM +0530, Pavan Kondeti wrote: > On Fri, May 06, 2022 at 10:41:01AM +0530, Krishna Kurapati PSSNV wrote: > > > > On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote: > > >On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote: > > >>From: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > >> > > >>During suspend read the status of all port and set hs phy mode > > >>based on current speed. Use this hs phy mode to configure wakeup > > >>interrupts in qcom glue driver. > > >> > > >>Check wakeup-source property for dwc3 core node to set the > > >>wakeup capability. Drop the device_init_wakeup call from > > >>runtime suspend and resume. > > >> > > >>Also check during suspend if any wakeup capable devices are > > >>connected to the controller (directly or through hubs), if there > > >>are none set a flag to indicate that the PHY is powered > > >>down during suspend. > > >> > > >>Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > >>Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > >>--- > > >> drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++------------- > > >> drivers/usb/dwc3/core.h | 4 ++++ > > >> drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++ > > >> 3 files changed, 48 insertions(+), 13 deletions(-) > > >> > > >>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > >>index 950e238..cf377f5 100644 > > >>--- a/drivers/usb/dwc3/core.c > > >>+++ b/drivers/usb/dwc3/core.c > > >>@@ -33,6 +33,7 @@ > > >> #include <linux/usb/gadget.h> > > >> #include <linux/usb/of.h> > > >> #include <linux/usb/otg.h> > > >>+#include <linux/usb/hcd.h> > > >This is not needed anymore > > > > > >> #include "core.h" > > >> #include "gadget.h" > > >>@@ -1787,6 +1788,7 @@ 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); > > >>@@ -1936,6 +1938,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > >> { > > >> unsigned long flags; > > >> u32 reg; > > >>+ struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci); > > >This isn't used anymore, delete it > > My bad, Will fix this in next version. > > >> switch (dwc->current_dr_role) { > > >> case DWC3_GCTL_PRTCAP_DEVICE: > > >>@@ -1948,10 +1951,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)) { > > >>- dwc3_core_exit(dwc); > > >>- break; > > >>- } > > >>+ dwc3_check_phy_speed_mode(dwc); > > >> /* Let controller to suspend HSPHY before PHY driver suspends */ > > >> if (dwc->dis_u2_susphy_quirk || > > >>@@ -1967,6 +1967,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > >> phy_pm_runtime_put_sync(dwc->usb2_generic_phy); > > >> phy_pm_runtime_put_sync(dwc->usb3_generic_phy); > > >>+ > > >>+ if (!PMSG_IS_AUTO(msg)) { > > >>+ if (device_may_wakeup(dwc->dev) && > > >>+ device_wakeup_path(dwc->dev)) { > > >nit: the indentation is odd, align it with device_may_wakeup()? > > Sure, Will take care of it. > > >>+ dwc->phy_power_off = false; > > >>+ } else { > > >>+ dwc->phy_power_off = true; > > >>+ dwc3_core_exit(dwc); > > >As commented earlier, taking the controller and PHYs completely down causes a > > >significant power draw in some USB clients. Let's clarify what the specific > > >benefits are of doing dwc3_core_exit() vs. entering a low power mode. > > Sure, once we come to a conclusion on this, I will refresh the patches. > > I think, Matthias is asking you to clarify in the commit description. we can > even quote Matthias observations. Actually I would like to have a discussion about the benefits of powering down the controller and PHYs vs. entering a low power state. Maybe there are good reasons for powering everything down (e.g. significant power savings), but as we have seen there are also significant downsides, so let's make sure we understand both.
On Fri, May 06, 2022 at 08:51:22AM -0700, Matthias Kaehlcke wrote: > On Fri, May 06, 2022 at 10:44:48AM +0530, Pavan Kondeti wrote: > > On Fri, May 06, 2022 at 10:41:01AM +0530, Krishna Kurapati PSSNV wrote: > > > > > > On 5/6/2022 4:18 AM, Matthias Kaehlcke wrote: > > > >On Thu, May 05, 2022 at 02:26:10PM +0530, Krishna Kurapati wrote: > > > >>From: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > >> > > > >>During suspend read the status of all port and set hs phy mode > > > >>based on current speed. Use this hs phy mode to configure wakeup > > > >>interrupts in qcom glue driver. > > > >> > > > >>Check wakeup-source property for dwc3 core node to set the > > > >>wakeup capability. Drop the device_init_wakeup call from > > > >>runtime suspend and resume. > > > >> > > > >>Also check during suspend if any wakeup capable devices are > > > >>connected to the controller (directly or through hubs), if there > > > >>are none set a flag to indicate that the PHY is powered > > > >>down during suspend. > > > >> > > > >>Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > >>Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > >>--- > > > >> drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++------------- > > > >> drivers/usb/dwc3/core.h | 4 ++++ > > > >> drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++ > > > >> 3 files changed, 48 insertions(+), 13 deletions(-) > > > >> > > > >>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > >>index 950e238..cf377f5 100644 > > > >>--- a/drivers/usb/dwc3/core.c > > > >>+++ b/drivers/usb/dwc3/core.c > > > >>@@ -33,6 +33,7 @@ > > > >> #include <linux/usb/gadget.h> > > > >> #include <linux/usb/of.h> > > > >> #include <linux/usb/otg.h> > > > >>+#include <linux/usb/hcd.h> > > > >This is not needed anymore > > > > > > > >> #include "core.h" > > > >> #include "gadget.h" > > > >>@@ -1787,6 +1788,7 @@ 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); > > > >>@@ -1936,6 +1938,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > >> { > > > >> unsigned long flags; > > > >> u32 reg; > > > >>+ struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci); > > > >This isn't used anymore, delete it > > > My bad, Will fix this in next version. > > > >> switch (dwc->current_dr_role) { > > > >> case DWC3_GCTL_PRTCAP_DEVICE: > > > >>@@ -1948,10 +1951,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)) { > > > >>- dwc3_core_exit(dwc); > > > >>- break; > > > >>- } > > > >>+ dwc3_check_phy_speed_mode(dwc); > > > >> /* Let controller to suspend HSPHY before PHY driver suspends */ > > > >> if (dwc->dis_u2_susphy_quirk || > > > >>@@ -1967,6 +1967,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > >> phy_pm_runtime_put_sync(dwc->usb2_generic_phy); > > > >> phy_pm_runtime_put_sync(dwc->usb3_generic_phy); > > > >>+ > > > >>+ if (!PMSG_IS_AUTO(msg)) { > > > >>+ if (device_may_wakeup(dwc->dev) && > > > >>+ device_wakeup_path(dwc->dev)) { > > > >nit: the indentation is odd, align it with device_may_wakeup()? > > > Sure, Will take care of it. > > > >>+ dwc->phy_power_off = false; > > > >>+ } else { > > > >>+ dwc->phy_power_off = true; > > > >>+ dwc3_core_exit(dwc); > > > >As commented earlier, taking the controller and PHYs completely down causes a > > > >significant power draw in some USB clients. Let's clarify what the specific > > > >benefits are of doing dwc3_core_exit() vs. entering a low power mode. > > > Sure, once we come to a conclusion on this, I will refresh the patches. > > > > I think, Matthias is asking you to clarify in the commit description. we can > > even quote Matthias observations. > > Actually I would like to have a discussion about the benefits of powering down > the controller and PHYs vs. entering a low power state. Maybe there are good > reasons for powering everything down (e.g. significant power savings), but > as we have seen there are also significant downsides, so let's make sure > we understand both. I found this, as I commented on the other thread: commit c4a5153e87fdf6805f63ff57556260e2554155a5 Author: Manu Gautam <mgautam@codeaurora.org> Date: Thu Jan 18 16:54:30 2018 +0530 usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode 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. Perform dwc3_core_exit/init only during host mode system_suspend/ resume to addresses power regression from above mentioned patch and also allow USB session to stay connected across runtime_suspend/resume in host mode. While at it also replace existing checks for HOST only dr_mode with current_dr_role to have similar core driver behavior for both Host-only and DRD+Host configurations. Fixes: 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") Reviewed-by: Roger Quadros <rogerq@ti.com> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> So apparently powering off the core and PHYs is needed on some platforms. Let's move forward with the core/PHYs off for now and try to come up with a solution (e.g. a DT property that indicates that the core/PHYs can remain powererd) in a separate patch/series.
On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote: > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote: > > device_wakeup_path() tells if any of the children devices needs > > wakeup. Use this hint to enable/disable wakeup of our device. This > > helps the parent device of xhci-plat (like sysdev) to retrieve > > the wakeup setting via device_wakeup_path(). > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > --- > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > index 649ffd8..ad585fa 100644 > > --- a/drivers/usb/host/xhci-plat.c > > +++ b/drivers/usb/host/xhci-plat.c > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) > > if (pm_runtime_suspended(dev)) > > pm_runtime_resume(dev); > > > > + if (device_wakeup_path(dev)) { > > + if (!device_may_wakeup(dev)) > > + device_wakeup_enable(dev); > > + } else { > > + if (device_may_wakeup(dev)) > > + device_wakeup_disable(dev); > > + } > > This code is not self-explantatory and deserves a comment. > > Enabling/disabling wakeup for the purpose if signalling is a bit of a > hack. It might be an acceptable hack as long as it has no side effects. > However with the current implementation the wakeup state of the xHCI can > be different after resuming than it was before going to suspend: > > after boot > grep -h xhci /sys/class/wakeup/*/name > => xhci-hcd.14.auto > > after suspend w/o wakeup capable device > grep -h xhci /sys/class/wakeup/*/name > => no results > > after suspend with wakeup capable device > grep -h xhci /sys/class/wakeup/*/name > => xhci-hcd.14.auto > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7] > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done > conditionally though). I am worried that we are not doing the right thing here. why should the xhci-plat goes against the wishes of the user space policy here? Can we NOT just do anything here? If some one wants xhci-plat to wakeup all the time, dwc3 will be configured to wakeup the system provided that the support is available. This way we don't break any existing users of xhci-plat i.e not enabling wakeup from the kernel. Thanks, Pavan
On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote: > On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote: > > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote: > > > device_wakeup_path() tells if any of the children devices needs > > > wakeup. Use this hint to enable/disable wakeup of our device. This > > > helps the parent device of xhci-plat (like sysdev) to retrieve > > > the wakeup setting via device_wakeup_path(). > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > > index 649ffd8..ad585fa 100644 > > > --- a/drivers/usb/host/xhci-plat.c > > > +++ b/drivers/usb/host/xhci-plat.c > > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) > > > if (pm_runtime_suspended(dev)) > > > pm_runtime_resume(dev); > > > > > > + if (device_wakeup_path(dev)) { > > > + if (!device_may_wakeup(dev)) > > > + device_wakeup_enable(dev); > > > + } else { > > > + if (device_may_wakeup(dev)) > > > + device_wakeup_disable(dev); > > > + } > > > > This code is not self-explantatory and deserves a comment. > > > > Enabling/disabling wakeup for the purpose if signalling is a bit of a > > hack. It might be an acceptable hack as long as it has no side effects. > > However with the current implementation the wakeup state of the xHCI can > > be different after resuming than it was before going to suspend: > > > > after boot > > grep -h xhci /sys/class/wakeup/*/name > > => xhci-hcd.14.auto > > > > after suspend w/o wakeup capable device > > grep -h xhci /sys/class/wakeup/*/name > > => no results > > > > after suspend with wakeup capable device > > grep -h xhci /sys/class/wakeup/*/name > > => xhci-hcd.14.auto > > > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have > > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7] > > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done > > conditionally though). > > I am worried that we are not doing the right thing here. why should the > xhci-plat goes against the wishes of the user space policy here? Can we NOT > just do anything here? If some one wants xhci-plat to wakeup all the time, > dwc3 will be configured to wakeup the system provided that the support is > available. This way we don't break any existing users of xhci-plat i.e not > enabling wakeup from the kernel. > Krishna, can we please drop this patch and use device_wakeup_path() and verify the following cases. 1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled 2. one of the downstream USB device supports wakeup and xhci-plat wakeup is disabled 3. none of the downstream USB device supports wakeup (or disable) and xhci-plat wakeup is enabled. 4. none of the downstream USB device supports wakeup (or disable) and xhci-plat wakeup is disabled. We don't want to enable xhci-plat wakeup capability like we do in this patch and potentially break any other platform. Lets leave the policy to the user space and rely on wakeup path to see if we can achieve the desired result. Thanks, Pavan
On Wed, May 11, 2022 at 07:21:01AM +0530, Pavan Kondeti wrote: > On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote: > > On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote: > > > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote: > > > > device_wakeup_path() tells if any of the children devices needs > > > > wakeup. Use this hint to enable/disable wakeup of our device. This > > > > helps the parent device of xhci-plat (like sysdev) to retrieve > > > > the wakeup setting via device_wakeup_path(). > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > --- > > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > > > index 649ffd8..ad585fa 100644 > > > > --- a/drivers/usb/host/xhci-plat.c > > > > +++ b/drivers/usb/host/xhci-plat.c > > > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) > > > > if (pm_runtime_suspended(dev)) > > > > pm_runtime_resume(dev); > > > > > > > > + if (device_wakeup_path(dev)) { > > > > + if (!device_may_wakeup(dev)) > > > > + device_wakeup_enable(dev); > > > > + } else { > > > > + if (device_may_wakeup(dev)) > > > > + device_wakeup_disable(dev); > > > > + } > > > > > > This code is not self-explantatory and deserves a comment. > > > > > > Enabling/disabling wakeup for the purpose if signalling is a bit of a > > > hack. It might be an acceptable hack as long as it has no side effects. > > > However with the current implementation the wakeup state of the xHCI can > > > be different after resuming than it was before going to suspend: > > > > > > after boot > > > grep -h xhci /sys/class/wakeup/*/name > > > => xhci-hcd.14.auto > > > > > > after suspend w/o wakeup capable device > > > grep -h xhci /sys/class/wakeup/*/name > > > => no results > > > > > > after suspend with wakeup capable device > > > grep -h xhci /sys/class/wakeup/*/name > > > => xhci-hcd.14.auto > > > > > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have > > > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7] > > > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done > > > conditionally though). > > > > I am worried that we are not doing the right thing here. why should the > > xhci-plat goes against the wishes of the user space policy here? Can we NOT > > just do anything here? If some one wants xhci-plat to wakeup all the time, > > dwc3 will be configured to wakeup the system provided that the support is > > available. This way we don't break any existing users of xhci-plat i.e not > > enabling wakeup from the kernel. > > > Krishna, > > can we please drop this patch and use device_wakeup_path() and verify the > following cases. > > 1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled > 2. one of the downstream USB device supports wakeup and xhci-plat wakeup is > disabled > 3. none of the downstream USB device supports wakeup (or disable) and > xhci-plat wakeup is enabled. > 4. none of the downstream USB device supports wakeup (or disable) and > xhci-plat wakeup is disabled. I wonder if we couldn't keep this simpler: if the dwc3 is wakeup capable keep the PHYs/core powered, otherwise power them down. Similar to what commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") intended, but with the additonal check for wakeup capability. We now know that the PHYs need to be powered down on some SoCs to allow the SoC to reach its low power mode during suspend: commit c4a5153e87fdf6805f63ff57556260e2554155a5 Author: Manu Gautam <mgautam@codeaurora.org> Date: Thu Jan 18 16:54:30 2018 +0530 usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode 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. With wakeup capable controllers this is apparently not an issue, otherwise the SoC wouldn't be able to enter its low power state when wakeup is enabled.
On Wed, May 11, 2022 at 08:54:25AM -0700, Matthias Kaehlcke wrote: > On Wed, May 11, 2022 at 07:21:01AM +0530, Pavan Kondeti wrote: > > On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote: > > > On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote: > > > > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote: > > > > > device_wakeup_path() tells if any of the children devices needs > > > > > wakeup. Use this hint to enable/disable wakeup of our device. This > > > > > helps the parent device of xhci-plat (like sysdev) to retrieve > > > > > the wakeup setting via device_wakeup_path(). > > > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > > --- > > > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > > > > index 649ffd8..ad585fa 100644 > > > > > --- a/drivers/usb/host/xhci-plat.c > > > > > +++ b/drivers/usb/host/xhci-plat.c > > > > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) > > > > > if (pm_runtime_suspended(dev)) > > > > > pm_runtime_resume(dev); > > > > > > > > > > + if (device_wakeup_path(dev)) { > > > > > + if (!device_may_wakeup(dev)) > > > > > + device_wakeup_enable(dev); > > > > > + } else { > > > > > + if (device_may_wakeup(dev)) > > > > > + device_wakeup_disable(dev); > > > > > + } > > > > > > > > This code is not self-explantatory and deserves a comment. > > > > > > > > Enabling/disabling wakeup for the purpose if signalling is a bit of a > > > > hack. It might be an acceptable hack as long as it has no side effects. > > > > However with the current implementation the wakeup state of the xHCI can > > > > be different after resuming than it was before going to suspend: > > > > > > > > after boot > > > > grep -h xhci /sys/class/wakeup/*/name > > > > => xhci-hcd.14.auto > > > > > > > > after suspend w/o wakeup capable device > > > > grep -h xhci /sys/class/wakeup/*/name > > > > => no results > > > > > > > > after suspend with wakeup capable device > > > > grep -h xhci /sys/class/wakeup/*/name > > > > => xhci-hcd.14.auto > > > > > > > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have > > > > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7] > > > > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done > > > > conditionally though). > > > > > > I am worried that we are not doing the right thing here. why should the > > > xhci-plat goes against the wishes of the user space policy here? Can we NOT > > > just do anything here? If some one wants xhci-plat to wakeup all the time, > > > dwc3 will be configured to wakeup the system provided that the support is > > > available. This way we don't break any existing users of xhci-plat i.e not > > > enabling wakeup from the kernel. > > > > > Krishna, > > > > can we please drop this patch and use device_wakeup_path() and verify the > > following cases. > > > > 1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled > > 2. one of the downstream USB device supports wakeup and xhci-plat wakeup is > > disabled > > 3. none of the downstream USB device supports wakeup (or disable) and > > xhci-plat wakeup is enabled. > > 4. none of the downstream USB device supports wakeup (or disable) and > > xhci-plat wakeup is disabled. > > I wonder if we couldn't keep this simpler: if the dwc3 is wakeup capable keep > the PHYs/core powered, otherwise power them down. Similar to what commit > 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host > bus-suspend/resume") intended, but with the additonal check for wakeup > capability. We now know that the PHYs need to be powered down on some SoCs > to allow the SoC to reach its low power mode during suspend: > > > commit c4a5153e87fdf6805f63ff57556260e2554155a5 > Author: Manu Gautam <mgautam@codeaurora.org> > Date: Thu Jan 18 16:54:30 2018 +0530 > > usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode > > 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. > > > With wakeup capable controllers this is apparently not an issue, otherwise > the SoC wouldn't be able to enter its low power state when wakeup is > enabled. Agree to your suggestion. Thanks for your inputs. Thanks, Pavan