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.