Re: [v15 3/6] usb: dwc3: core: Host wake up support from system suspend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Alan Stern



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux