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@xxxxxxxxxxx>
> > > >>
> > > >>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@xxxxxxxxxxx>
> > > >>Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > > >>---
> > > >> 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@xxxxxxxxxxxxxx>
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@xxxxxx>
Signed-off-by: Manu Gautam <mgautam@xxxxxxxxxxxxxx>
Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
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.
