On Fri, May 06, 2022 at 04:30:33PM -0400, Alan Stern wrote:
> 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.
>
At this point it is not clear if all boards that has DWC3 controller can
support wakeup capability or not. Thats why we have introduced a wakeup device
tree property based on which we advertise our wakeup capability.
device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node,
"wakeup-source"));
Hence the && condition to make sure that we support wakeup and our children
needs it.
Thanks,
Pavan
