> > > > > > --- > > > > > > drivers/usb/host/xhci-plat.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/usb/host/xhci-plat.c > > > > > > b/drivers/usb/host/xhci-plat.c index > > > > > > cebe24ec80a5..bb5d73f0a796 100644 > > > > > > --- a/drivers/usb/host/xhci-plat.c > > > > > > +++ b/drivers/usb/host/xhci-plat.c > > > > > > @@ -287,7 +287,7 @@ static int xhci_plat_probe(struct platform_device > *pdev) > > > > > > *priv = *priv_match; > > > > > > } > > > > > > > > > > > > - device_wakeup_enable(hcd->self.controller); > > > > > > + device_set_wakeup_capable(hcd->self.controller, true); > > > > > > > > > > > > > > > In fact both this patch and the original code are wrong. It > > > > > really should > > > > > be: > > > > > > > > > > device_init_wakeup(hcd->self.controller, true); > > > > > > > > > > This will add the wakeup entry in sysfs and set it to Enabled. > > > > > This is the appropriate behavior, as explained in the kerneldoc > > > > > for device_init_wakeup(). The reason is because the controller > > > > > device doesn't create any wakeup events on its own; it merely > > > > > relays wakeup requests from descendant devices (root hubs or USB > devices). > > > > > > > > Hi Alan, > > > > > > > > At xhci-plat.c's system suspend API xhci_plat_suspend, it depends > > > > on power/wakeup value to determine whether the controller should > > > > enable port wakeup capabilities, and from the system level, > > > > whether the system supports USB wakeup or not is better to be > > > > determined by user, and is disabled by default. > > > > > > > > Yes, you are right. The wakeup events are from controller's > > > > descendant devices, and it is better to use roothub's wakeup > > > > capability to control portsc's wakeup setting. At controller > > > > driver, the meaning for wakeup setting is enabling wakeup > > > > interrupt for hardware signal events (dp/dm for USB2, and > > > > RX-detect for USB3), this hardware logic is the glue layer and > > > > designed by each vendors, without this logic, the USB controller > > > > can't be woken up due to the USBCMD.RS bit is cleared, and there > > > > is no standard EHCI or xHCI interrupt. The controller's wakeup setting is > better to be disabled by default, and decided by user too. > > > > > > > > For me, either this patch or use roothub's wakeup capability to > > > > control portsc wakeup setting, both are OK. Mathias, what's your > > > > opinion? > > > > > > Mathias is starting a long vacation, so he might not reply for a while. > > > > > > Note that hcd-pci.c, ohci-platform.c, and ehci-platform.c all call > > > device_wakeup_enable(). This indicates that xhci-plat.c should do > > > the same -- presumably device_set_wakeup_capable() is already called > > > somewhere in the platform-specific code. > > > > > > > Thanks for the information, Alan. > > > > I could not understand why device_wakeup_enable is used in these > > device drivers? At Documentation/driver-api/pm/devices.rst, L189, it also says: > > > > during a system sleep transition. Device drivers, however, are > > not expected to call :c:func:`device_set_wakeup_enable()` directly > > in any case. > > It also says: > > It should also default to "enabled" for devices that don't > generate wakeup requests on their own but merely forward wakeup > requests from one bus to another (like PCI Express ports). > > The controller device falls into this category. It doesn't generate wakeup requests > on its own; it merely forwards wakeup requests from the root hub or USB devices. I > think the intention was that drivers for these devices would call device_init_wakeup() > instead of calling both > device_set_wakeup_capable() and device_wakeup_enable(). > > In any case, the rule about setting the default value is more important than the rule > about not calling device_set_wakeup_enable() directly. > > If you're concerned about connect-detect or disconnect-detect wakeup signals, > these are supposed to be enabled or disabled by the root hub's wakeup setting. > The idea is that root hubs should behave the same as external hubs -- and whether > or not an external hub generates a wakeup request when a connect or disconnect > event occurs is controlled by the hub's wakeup setting. > So, you suggest: At hcd-pci.c, ohci-platform.c, ehci-platform.c, and xhci-plat.c: change device_wakeup_enable to device_init_wakeup(dev, true). For xhci_suspend, use both controller's wakeup setting and roothub wakeup setting to judge if connect or disconnect wakeup needs to enable or not, like function ehci_adjust_port_wakeup_flags. > If the controller's wakeup setting defaulted to "disabled", then anybody who wanted > to get USB wakeup requests would have to enable them on both the USB device > and the controller. This would confuse users and cause problems. > I think if controller's wakeup setting is only used for ehci or xhci common code, that's ok. If it is also used for glue layer's power control and wakeup setting; it may need to set "disabled" for default value. I am curious how PCI USB at PC determine whether it responds USB wakeup events or not? At Linux kernel or BIOS? It has two places for enabling USB wakeup, one for USB devices (including roothub), another is for PCI device? Peter
