On Mon, May 09, 2022 at 10:08:41AM -0400, Alan Stern wrote: > On Mon, May 09, 2022 at 09:02:38AM +0530, Pavan Kondeti wrote: > > 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: > > > > 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. > > Oh, I see. I misread the code above, thinking that dwc->dev referred to > the root hub. It doesn't; it refers to the controller. Sorry for the > mistake. > > BTW, if there's any trouble with getting device_wakeup_path() to work > the way you want, you could consider instead calling > usb_wakeup_enabled_descendants() on the root hub. This function returns > a count of the number of wakeup-enabled USB devices at or below the > device you pass to it. > This series [1] started with usb_wakeup_enabled_descendants() actually. one of the problem with this API is that we have to call this on both USB2.0 and USB3.0 root hubs. Do you think we can have a wrapper function like usb_hcd_wakeup_enabled_descendants() that accepts hcd as an argument and internally call usb_wakeup_enabled_descendants() on both root hubs and return the result. Thanks, Pavan