Hi Pavan, On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote: > Hi Matthias, > > On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote: > > Hi Matthias, > > > > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote: > > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote: > > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram > > > > <quic_c_sanm@xxxxxxxxxxx> wrote: > > > > > > > > > > From: Matthias Kaehlcke <mka@xxxxxxxxxxxx> > > > > > > > > > > Add device_children_wakeup_capable() which checks whether the device itself > > > > > or one if its descendants is wakeup capable. > > > > > > > > device_wakeup_path() exists for a very similar purpose. > > > > > > > > Is it not usable for whatever you need the new function introduced here? > > > > > > I wasn't aware of it's function, there are no doc comments and the > > > name isn't really self explanatory. > > > > > > In a quick test device_wakeup_path() returned inconsistent values for the > > > root hub, sometimes true, others false when a wakeup capable USB device was > > > connected. > > > > We will also test the same to double confirm the behavior of > > device_wakeup_path(). I am assuming that you checked device_wakeup_path() > > only during system suspend path. > > > > Here is what I understood by looking at __device_suspend(). Please share > > your thoughts on this. > > > > power.wakeup_path is set to true for the parent *after* a wakeup capable > > device is suspended. This means when the root hub(s) is suspended, it is > > propagated to xhci-plat and when xhci-plat is suspended, it is propagated > > to dwc3. bottom up propgation during system suspend. > > > > I believe we can directly check something like this in the dwc3 driver > > instead of having another wrapper like device_children_wakeup_capable(). > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 1170b80..a783257 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > break; > > case DWC3_GCTL_PRTCAP_HOST: > > if (!PMSG_IS_AUTO(msg)) { > > + /* > > + * Don't kill the host when dwc3 is wakeup capable and > > + * its children needs wakeup. > > + */ > > + if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev)) > > + handle_it(); > > + } else { > > dwc3_core_exit(dwc); > > - break; > > } > > > > /* Let controller to suspend HSPHY before PHY driver suspends */ > > > > device_wakeup_path(dwc->dev) is returning true all the time irrespective of > the wakeup capability (and enabled status) of the connected USB devices. That > is because xhci-plat device is configured to wakeup all the time. Since the > child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set. > device_children_wakeup_capable() will also suffer the problem. However, > > device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's > patch is using. That is not correct. we have two root hubs (HS and SS) associated > with a USB3 controller and calling it on one root hub is incorrect. > device_children_wakeup_capable() must be called on xhci-plat so that it covers > both HS and SS root hubs Thanks for pointing that out! > I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so > that the wakeup path is correctly propagated to dwc3. something like below. > Does it make sense to you? > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 649ffd8..be0c55b 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > int ret; > > + if (!device_wakeup_path(dev)) > + device_wakeup_disable(dev); > + > if (pm_runtime_suspended(dev)) > pm_runtime_resume(dev); > > @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev) > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > + device_wakeup_enable(dev); I think this also needs to be done conditionally, otherwise it would create a new wake source on every resume when wakeup is already enabled. Other than that this seems to do the trick and keeps the USB layer out of the dwc3 code.