On 20-07-08 11:02:04, Alan Stern wrote: > On Wed, Jul 08, 2020 at 06:47:31AM +0000, Peter Chen wrote: > > On 20-07-07 12:11:53, Alan Stern wrote: > > > > > But, that's not all the use cases. There are still two other use cases: > > > > (Taking xhci-plat.c as an example): > > > > - It is a platform bus device created by platform bus driver > > > > - It is a platform bus device created by glue layer parents > > > > (eg, dwc3/cdns3), usually, it is dual-role controller. > > > > > > In these cases there would be a choice: xhci-plat.c could call > > > device_init_wakeup, or the platform-bus/glue-layer driver could call > > > device_set_wakeup_capable and xhci-plat.c could continue to call > > > device_enable_wakeup. > > > > You said "the PCI core calls device_set_wakeup_capable() when a new device is > > discovered and register", why PCI core does this, is every device on > > PCI bus wakeup capable? > > Sorry, I should have said that the PCI core does this for all devices > that are able to generate wakeup requests. This ability is indicated by > a PCI capability setting, which the PCI core can read. > > > The reason I ask this is not every device on platform-bus is wakeup > > capable, to let the controller device have defaulted "enabled" value, > > we need to use device_init_wakeup at xhci-plat.c > > Yes. In your case it makes sense for the glue layer or platform code to > call device_set_wakeup_capable for the appropriate devices. Then the > generic code can call device_enable_wakeup (which doesn't do anything > if the device isn't wakeup-capable). > Yes, in my case, I could do it. But xhci-plat.c is generic code, some controller devices using this driver are created by generic platform bus driver. So I think we should use device_init_wakeup(dev, true) like you suggested at the first, this driver should not be used by PCI USB controller, so no effect on PCI USB, right? > > > > From hardware level: > > Controller includes core part and non-core part, core part is usually > > designed by IP vendor, non-core part is usually designed by each SoC > > vendors. Wakeup handling is part of non-core. The USB PHY gets > > ID/VBUS/DP/DM/RX change events, the related signal ties to non-core part, > > and non-core part knows the wakeup occurs. > > > > From software level: > > Taking single role controller as example: > > Glue layer is a platform device, and handles non-core part events, > > including wakeup events, it is the parent of common layer which handles > > core part events (eg, xhci-plat.c) > > Can you give an example of how these different layers show up in sysfs > (the device names and paths)? Our platforms are more complicated than this example, there are dual-role controllers (chipidea/cdns3/dwc3) at SoCs. Taking cdns3 as an example: /sys/bus/platform/devices/: the devices on the platform bus 5b110000.usb3: non-core part (cdns3/cdns3-imx.c) 5b130000.cdns3: the dual-role core part (cdns3/core.c) xhci-hcd.1.auto: the host core part (xhci-plat.c) usb1/usb2: roothubs for USB2 and USB3 root@imx8qmmek:~# cat /sys/bus/platform/devices/5b110000.usb3/ 5b130000.cdns3/ driver_override power/ uevent consumers modalias subsystem/ driver/ of_node/ suppliers root@imx8qmmek:~# cat /sys/bus/platform/devices/5b110000.usb3/5b130000.cdns3/ consumers modalias power/ uevent driver/ of_node/ subsystem/ usb_role/ driver_override pools suppliers xhci-hcd.1.auto/ root@imx8qmmek:~# cat /sys/bus/platform/devices/5b110000.usb3/5b130000.cdns3/xhci-hcd.1.auto/ consumers modalias suppliers usb2/ driver/ power/ uevent driver_override subsystem/ usb1/ > > > So, one controller includes two platform devices, one for glue layer, > > one for common layer. > > Is there a mechanism that allows the xhci-hcd core driver to tell the > non-core or PHY driver to enable or disable these wakeup events? > Not easy, see my comments below. > Or maybe better would be a way for the non-core/PHY driver to examine > the root hub's usb_device structure to see whether these wakeup events > should be enabled. > > > You are right, ID/VBUS/DP/DM/RX signal changing occurs at the USB bus, > > and detected by USB PHY physically. > > > > The controller device (core part) or glue layer device > > (non-core part)'s wakeup setting is only used to enable/disable platform > > related powers (regulators) for USB (PHY) which are used to detect > > ID/VBUS/DP/DM/RX signal. If the platform doesn't need USB wakeup capabilities > > for system suspend, it could turn off related powers. Besides, it could tell > > the system if USB interrupt can be the wakeup interrupt. > > We want to make the system simple and logical for users, not necessarily > easy for hardware engineers. This means that wakeup events such as port > connect, disconnect, and so on should be controlled by a single sysfs > setting, for a single device. The most logical device for this purpose > is the root hub, as I mentioned earlier in this discussion. > > As a result, the wakeup settings for the other components (non-core or > PHY) should be based on the settings for the root hub. If the root hub > is disabled for wakeup then the non-core hardware components shouldn't > generate any wakeup requests, no matter what their power/wakeup files > contain. And if the root hub is enabled for wakeup then the non-core > hardware components should generate these requests, unless their own > power/wakeup settings prevent them from doing so. > > From these conclusions, it logically follows that the default wakeup > setting for the non-core components should be "enabled" -- unless those > components are physically incapable of waking up the system. > I agree with you that the default wakeup setting of core part for host (xhci-plat.c) should be "enabled", but if for the dual-role controller, the wakeup events like VBUS and ID do not related with roothub, we can't set core part for controller (cdns3/core.c) for defaulted enabled, and there is no thing like gadget bus's wakeup setting we could depend on. Besides, the non-core part (glue layer) somethings can't visit core part, we had to depend on itself wakeup setting, but not the child's wakeup setting. -- Thanks, Peter Chen
