On Fri, May 06, 2022 at 11:46:06AM -0700, Matthias Kaehlcke wrote:
> On Fri, May 06, 2022 at 02:18:06PM -0400, Alan Stern wrote:
> > [CC: list drastically reduced]
> >
> > On Fri, May 06, 2022 at 09:51:46AM -0700, Matthias Kaehlcke wrote:
> > > 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.
> >
> > Isn't it true that if you power off the PHY, the controller will be
> > unable to detect resume requests from attached devices? Similarly, won't
> > the controller will be unable to detect plug and unplug events on the
> > root hub?
> >
> > Doesn't this mean that if wakeup is enabled on the root hub or any of
> > the devices downstream from a root-hub port, the port's PHY must remain
> > powered during suspend?
>
> Yes.
>
> Currently the core/PHYs are always powered off during suspend in host mode:
>
> static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> {
> ...
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_HOST:
> if (!PMSG_IS_AUTO(msg)) {
> dwc3_core_exit(dwc);
> break;
> }
>
> ...
> }
>
> With that I would expect wakeup to be broken for all dwc3. I'm a bit confused
> though, since dwc3-imx8mp.c seems to support wakeup and the driver landed
> after the above patch ...
>
> This series intends to change the above code to something like this:
>
> if (!PMSG_IS_AUTO(msg)) {
> if (device_may_wakeup(dwc->dev) &&
> device_wakeup_path(dwc->dev)) {
> dwc->phy_power_off = false;
> } else {
> dwc->phy_power_off = true;
> dwc3_core_exit(dwc);
> }
> }
> i.e. the core/PHYs would only be powered down if wakeup is disabled or no
> wakeup capable devices are connected. With that plug/unplug events still
> wouldn't be detected.
Indeed. Shouldn't the "&&" and "||"? That is, don't you want to leave
the core and PHY powered if wakeup is enabled for the root hub or for
any devices beneath it?
It would be simpler to leave the core and PHY powered whenever wakeup is
enabled for the controller itself, regardless of the status of the root
hub and downstream devices. Users might not like this so much if the
default setting is to enable wakeup for the controller always. Still,
it's an easy solution.
Alan Stern
