| Message ID | 20200703062532.29076-2-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] usb: host: xhci: avoid calling two contineous times for xhci_suspend | expand |
On Fri, Jul 03, 2020 at 02:25:32PM +0800, Peter Chen wrote: > After that, the user could enable controller as wakeup source > for system suspend through /sys entry. > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > 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). Alan Stern
On 20-07-03 10:19:11, Alan Stern wrote: > On Fri, Jul 03, 2020 at 02:25:32PM +0800, Peter Chen wrote: > > After that, the user could enable controller as wakeup source > > for system suspend through /sys entry. > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > --- > > 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?
On Sat, Jul 04, 2020 at 09:22:45AM +0000, Peter Chen wrote: > On 20-07-03 10:19:11, Alan Stern wrote: > > On Fri, Jul 03, 2020 at 02:25:32PM +0800, Peter Chen wrote: > > > After that, the user could enable controller as wakeup source > > > for system suspend through /sys entry. > > > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > > --- > > > 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. There is a brief discussion about this in Documentation/driver-api/pm/devices.rst. Alan Stern
On 20-07-04 10:48:16, Alan Stern wrote: > On Sat, Jul 04, 2020 at 09:22:45AM +0000, Peter Chen wrote: > > On 20-07-03 10:19:11, Alan Stern wrote: > > > On Fri, Jul 03, 2020 at 02:25:32PM +0800, Peter Chen wrote: > > > > After that, the user could enable controller as wakeup source > > > > for system suspend through /sys entry. > > > > > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > > > --- > > > > 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.
On Sun, Jul 05, 2020 at 02:12:47AM +0000, Peter Chen wrote: > On 20-07-04 10:48:16, Alan Stern wrote: > > On Sat, Jul 04, 2020 at 09:22:45AM +0000, Peter Chen wrote: > > > On 20-07-03 10:19:11, Alan Stern wrote: > > > > On Fri, Jul 03, 2020 at 02:25:32PM +0800, Peter Chen wrote: > > > > > After that, the user could enable controller as wakeup source > > > > > for system suspend through /sys entry. > > > > > > > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > > > > --- > > > > > 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. 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. Alan Stern
> > > > > > --- > > > > > > 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
On Mon, Jul 06, 2020 at 04:03:08AM +0000, Peter Chen wrote: > > > 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). I don't think it's necessary to do that. device_init_wakeup(dev, true) just calls device_set_wakeup_capable() and device_wakeup_enable(). The kernel already does the device_set_wakeup_capable() part for these devices in the code that registers them. For instance, the PCI core calls device_set_wakeup_capable() when a new device is discovered and registered, so there's no need for hcd-pci.c to repeat this action. > 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. Yes, something like that. This probably should be done in any case. Some hardware might not like it if port-level connect/disconnect wakeups are enabled but the controller-level wakeup signal is disabled. > > 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. What sort of wakeup events can the glue layer generate? It seems to me that if there is no controller driver bound to the controller device then the controller shouldn't be able to wake the system up in any case. > 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? PCI devices send wakeup requests via a special PCI power management signal called PME -- you can see its state in the output from "lspci -vv" in the Power Management Capability section. In legacy systems this signal was handled by the BIOS, but nowadays the PCI and ACPI subsystems in the Linux kernel handle it. If a PCI host controller is in the D3 low-power state when a wakeup event occurs, it uses the PME# signal to request a wakeup. If it is in the D0 full-power state when a wakeup event occurs, it uses its normal IRQ signal to tell the system about the event. Alan Stern
On 20-07-06 12:22:37, Alan Stern wrote: > On Mon, Jul 06, 2020 at 04:03:08AM +0000, Peter Chen wrote: > > > > 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). > > I don't think it's necessary to do that. > > device_init_wakeup(dev, true) just calls device_set_wakeup_capable() and > device_wakeup_enable(). The kernel already does the > device_set_wakeup_capable() part for these devices in the code that > registers them. For instance, the PCI core calls > device_set_wakeup_capable() when a new device is discovered and > registered, so there's no need for hcd-pci.c to repeat this action. 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. > > > 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. > > Yes, something like that. This probably should be done in any case. > Some hardware might not like it if port-level connect/disconnect wakeups > are enabled but the controller-level wakeup signal is disabled. > > > > 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. > > What sort of wakeup events can the glue layer generate? It seems to me > that if there is no controller driver bound to the controller device > then the controller shouldn't be able to wake the system up in any case. It should be the similar with PCI device you mentioned below. The glue layer device is a platform device which is the parent of controller device, it could detect ID/VBUS/DP/DM/RX changes and generate wakeup events, these wakeup events will trigger interrupt, this interrupt number could be the same with controller interrupt number or not. When the system is in suspend (D3), when the wakeup events occurs, the system interrupt controller could trigger wakeup request, and wake system up. When the system is in full-power state (D0), this interrupt just wakes up glue layer, and glue layer wakes up common USB stack (EHCI/XHCI). > > > 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? > > PCI devices send wakeup requests via a special PCI power management > signal called PME -- you can see its state in the output from "lspci > -vv" in the Power Management Capability section. In legacy systems this > signal was handled by the BIOS, but nowadays the PCI and ACPI subsystems > in the Linux kernel handle it. > > If a PCI host controller is in the D3 low-power state when a wakeup > event occurs, it uses the PME# signal to request a wakeup. If it is in > the D0 full-power state when a wakeup event occurs, it uses its normal > IRQ signal to tell the system about the event. > If the USBCMD.RS is cleared, does the interrupt could still occur when the system is at D0, or this interrupt is not USB interrupt, it is a PCI interrupt?
On Tue, Jul 07, 2020 at 02:01:28AM +0000, Peter Chen wrote: > On 20-07-06 12:22:37, Alan Stern wrote: > > On Mon, Jul 06, 2020 at 04:03:08AM +0000, Peter Chen wrote: > > > 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). > > > > I don't think it's necessary to do that. > > > > device_init_wakeup(dev, true) just calls device_set_wakeup_capable() and > > device_wakeup_enable(). The kernel already does the > > device_set_wakeup_capable() part for these devices in the code that > > registers them. For instance, the PCI core calls > > device_set_wakeup_capable() when a new device is discovered and > > registered, so there's no need for hcd-pci.c to repeat this action. > > 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. > > > 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. > > > > What sort of wakeup events can the glue layer generate? It seems to me > > that if there is no controller driver bound to the controller device > > then the controller shouldn't be able to wake the system up in any case. > > It should be the similar with PCI device you mentioned below. The > glue layer device is a platform device which is the parent of controller > device, I don't understand. Let's consider EHCI as an example. The controller device is something like 0000:00:1d.7, which is on the PCI bus and is a PCI device. Its child is usb1, which is a root-hub device on the USB bus. But you're saying that the glue layer device would be the parent of the controller device, right? This means it would be analogous to the parent of 0000:00:1d.7. In the PCI world, that parent would be a PCI bridge or something similar. It would have no understanding of ID/VBUS/DP/DM/RX changes, since those are all USB concepts and have nothing to do with PCI. > it could detect ID/VBUS/DP/DM/RX changes and generate wakeup > events, these wakeup events will trigger interrupt, this interrupt > number could be the same with controller interrupt number or not. This really sounds like you are talking about the controller, not the controller's parent. Or maybe a PHY, which is sort of next to the controller without being its parent or child. I like to think of it this way: A controller or device is something that sits at an endpoint of a bus or communication channel, or at the meeting place of two buses or communication channels. Thus, an EHCI controller sits at the meeting place of a PCI bus and a USB bus. As a result, it has interfaces to two buses: an upward-facing PCI bus interface and a downward-facing USB bus interface. The controller and root-hub devices are abstractions used by the kernel to represent these two interfaces. That's why the ehci-pci controller driver registers itself with a struct pci_driver and why root hubs are bound to the usb_generic_driver, even though the actual hardware is all part of a single EHCI controller chip. So now, in the situations you're talking about, what exactly are the buses, the interfaces, and the controllers/devices? > When the system is in suspend (D3), when the wakeup events occurs, > the system interrupt controller could trigger wakeup request, and > wake system up. When the system is in full-power state (D0), this > interrupt just wakes up glue layer, and glue layer wakes up common > USB stack (EHCI/XHCI). When a device or controller relays information from one bus or another, the wakeup setting indicates whether or not it should relay wakeup requests. ID/VBUS/DP/DM/RX events are all things that take place on the USB bus. As a result, the corresponding wakeup requests are theoretically generated by the root-hub device -- not by the controller device, since the controller device is attached to the upward-facing bus and not to the USB bus. The controller's wakeup setting thus indicates whether the controller should forward these wakeup requests from the root hub to the upward-facing bus. Once a request reaches the upward-facing bus, it could take the form of an ordinary IRQ signal or a system-level wakeup signal. > > > 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? > > > > PCI devices send wakeup requests via a special PCI power management > > signal called PME -- you can see its state in the output from "lspci > > -vv" in the Power Management Capability section. In legacy systems this > > signal was handled by the BIOS, but nowadays the PCI and ACPI subsystems > > in the Linux kernel handle it. > > > > If a PCI host controller is in the D3 low-power state when a wakeup > > event occurs, it uses the PME# signal to request a wakeup. If it is in > > the D0 full-power state when a wakeup event occurs, it uses its normal > > IRQ signal to tell the system about the event. > > > > If the USBCMD.RS is cleared, does the interrupt could still occur when > the system is at D0, or this interrupt is not USB interrupt, it is a > PCI interrupt? I don't remember the details offhand. I think pretty much the same events are generated regardless of whether USBCMD.RS is set or clear. Anyway, when one of these events occurs, it causes an interrupt flag to be set in the hardware. If the controller is in D0 then it will raise a PCI IRQ whenever the interrupt flag is set (and not masked). If the controller is in D3 then it is not allowed to raise a PCI IRQ, so it asserts the PCI PME signal instead. I'm not sure what you mean by "USB interrupt". The USB protocol doesn't have interrupts. (It has interrupt URBs, but those are completely different things as you know.) The closest thing USB has to an interrupt is a wakeup request. Alan Stern
On 20-07-07 12:11:53, Alan Stern wrote: > On Tue, Jul 07, 2020 at 02:01:28AM +0000, Peter Chen wrote: > > On 20-07-06 12:22:37, Alan Stern wrote: > > > On Mon, Jul 06, 2020 at 04:03:08AM +0000, Peter Chen wrote: > > > > 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). > > > > > > I don't think it's necessary to do that. > > > > > > device_init_wakeup(dev, true) just calls device_set_wakeup_capable() and > > > device_wakeup_enable(). The kernel already does the > > > device_set_wakeup_capable() part for these devices in the code that > > > registers them. For instance, the PCI core calls > > > device_set_wakeup_capable() when a new device is discovered and > > > registered, so there's no need for hcd-pci.c to repeat this action. > > > > 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? 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 > > > > > > 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. > > > > > > What sort of wakeup events can the glue layer generate? It seems to me > > > that if there is no controller driver bound to the controller device > > > then the controller shouldn't be able to wake the system up in any case. > > > > It should be the similar with PCI device you mentioned below. The > > glue layer device is a platform device which is the parent of controller > > device, > > I don't understand. Let's consider EHCI as an example. The controller > device is something like 0000:00:1d.7, which is on the PCI bus and is a > PCI device. Its child is usb1, which is a root-hub device on the USB > bus. > > But you're saying that the glue layer device would be the parent of the > controller device, right? This means it would be analogous to the > parent of 0000:00:1d.7. In the PCI world, that parent would be a PCI > bridge or something similar. It would have no understanding of > ID/VBUS/DP/DM/RX changes, since those are all USB concepts and have > nothing to do with PCI. Sorry, my words were not precise. 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) So, one controller includes two platform devices, one for glue layer, one for common layer. > > > it could detect ID/VBUS/DP/DM/RX changes and generate wakeup > > events, these wakeup events will trigger interrupt, this interrupt > > number could be the same with controller interrupt number or not. > > This really sounds like you are talking about the controller, not the > controller's parent. Or maybe a PHY, which is sort of next to the > controller without being its parent or child. > > I like to think of it this way: A controller or device is something that > sits at an endpoint of a bus or communication channel, or at the meeting > place of two buses or communication channels. Thus, an EHCI controller > sits at the meeting place of a PCI bus and a USB bus. As a result, it > has interfaces to two buses: an upward-facing PCI bus interface and a > downward-facing USB bus interface. The controller and root-hub devices > are abstractions used by the kernel to represent these two interfaces. > That's why the ehci-pci controller driver registers itself with a > struct pci_driver and why root hubs are bound to the usb_generic_driver, > even though the actual hardware is all part of a single EHCI controller > chip. > > So now, in the situations you're talking about, what exactly are the > buses, the interfaces, and the controllers/devices? roothub is the child of the core part, core part is the child of non-core part. > > > When the system is in suspend (D3), when the wakeup events occurs, > > the system interrupt controller could trigger wakeup request, and > > wake system up. When the system is in full-power state (D0), this > > interrupt just wakes up glue layer, and glue layer wakes up common > > USB stack (EHCI/XHCI). > > When a device or controller relays information from one bus or another, > the wakeup setting indicates whether or not it should relay wakeup > requests. ID/VBUS/DP/DM/RX events are all things that take place on the > USB bus. As a result, the corresponding wakeup requests are > theoretically generated by the root-hub device -- not by the controller > device, since the controller device is attached to the upward-facing bus > and not to the USB bus. The controller's wakeup setting thus indicates > whether the controller should forward these wakeup requests from the > root hub to the upward-facing bus. Once a request reaches the > upward-facing bus, it could take the form of an ordinary IRQ signal or a > system-level wakeup signal. 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. > > > > > 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? > > > > > > PCI devices send wakeup requests via a special PCI power management > > > signal called PME -- you can see its state in the output from "lspci > > > -vv" in the Power Management Capability section. In legacy systems this > > > signal was handled by the BIOS, but nowadays the PCI and ACPI subsystems > > > in the Linux kernel handle it. > > > > > > If a PCI host controller is in the D3 low-power state when a wakeup > > > event occurs, it uses the PME# signal to request a wakeup. If it is in > > > the D0 full-power state when a wakeup event occurs, it uses its normal > > > IRQ signal to tell the system about the event. > > > > > > > If the USBCMD.RS is cleared, does the interrupt could still occur when > > the system is at D0, or this interrupt is not USB interrupt, it is a > > PCI interrupt? > > I don't remember the details offhand. I think pretty much the same > events are generated regardless of whether USBCMD.RS is set or clear. > > Anyway, when one of these events occurs, it causes an interrupt flag to > be set in the hardware. If the controller is in D0 then it will raise a > PCI IRQ whenever the interrupt flag is set (and not masked). If the > controller is in D3 then it is not allowed to raise a PCI IRQ, so it > asserts the PCI PME signal instead. > > I'm not sure what you mean by "USB interrupt". The USB protocol doesn't > have interrupts. (It has interrupt URBs, but those are completely > different things as you know.) The closest thing USB has to an > interrupt is a wakeup request. > The "USB interrupt" I mean here is the interrupt event triggered by core part of the USB controller, for example the interrupt at usbsts for EHCI.
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). > > > It should be the similar with PCI device you mentioned below. The > > > glue layer device is a platform device which is the parent of controller > > > device, > > > > I don't understand. Let's consider EHCI as an example. The controller > > device is something like 0000:00:1d.7, which is on the PCI bus and is a > > PCI device. Its child is usb1, which is a root-hub device on the USB > > bus. > > > > But you're saying that the glue layer device would be the parent of the > > controller device, right? This means it would be analogous to the > > parent of 0000:00:1d.7. In the PCI world, that parent would be a PCI > > bridge or something similar. It would have no understanding of > > ID/VBUS/DP/DM/RX changes, since those are all USB concepts and have > > nothing to do with PCI. > > Sorry, my words were not precise. > > 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)? > 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? 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. Alan Stern
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.
On Thu, Jul 09, 2020 at 05:15:25AM +0000, Peter Chen wrote: > 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? 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/ Okay, thanks. > > > 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. All right. Note that almost everything I wrote before was meant for the case of a host controller; it did not consider what should happen with a UDC or dual-role controller. It seems like the best answer will be to call device_init_wakeup for the core controller device, but not for the non-core devices. Or maybe call it for the non-core devices if the controller is host-only. Alan Stern
On 20-07-09 10:50:04, Alan Stern wrote: > > > 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. > > All right. Note that almost everything I wrote before was meant for the > case of a host controller; it did not consider what should happen with a > UDC or dual-role controller. > > It seems like the best answer will be to call device_init_wakeup for the > core controller device, but not for the non-core devices. Or maybe call > it for the non-core devices if the controller is host-only. > > Alan Stern Hi Alan, Since the controller's wakeup setting (PORT_WKCONN_E/PORT_WKDISC_E) should be decided by roothub's wakeup setting, then, why controller's wakeup setting could affect it at current code, does the controller device may affect wakeup function at some situations? int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup) int ehci_suspend(struct usb_hcd *hcd, bool do_wakeup) It suggests the user needing to set both controller and roothub's wakeup enabled to let the wakeup function work.
On Tue, Jul 21, 2020 at 11:03:03AM +0000, Peter Chen wrote: > On 20-07-09 10:50:04, Alan Stern wrote: > > > > 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. > > > > All right. Note that almost everything I wrote before was meant for the > > case of a host controller; it did not consider what should happen with a > > UDC or dual-role controller. > > > > It seems like the best answer will be to call device_init_wakeup for the > > core controller device, but not for the non-core devices. Or maybe call > > it for the non-core devices if the controller is host-only. > > > > Alan Stern > > Hi Alan, > > Since the controller's wakeup setting (PORT_WKCONN_E/PORT_WKDISC_E) > should be decided by roothub's wakeup setting, then, why controller's > wakeup setting could affect it at current code, does the controller > device may affect wakeup function at some situations? > > int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup) > int ehci_suspend(struct usb_hcd *hcd, bool do_wakeup) > > It suggests the user needing to set both controller and roothub's wakeup > enabled to let the wakeup function work. That's right. This is a tricky area. A decision was made a long time ago when the current wakeup mechanisms were being added into the kernel; we decided that in order for a wakeup signal to propagate from the originating device all the way up to the CPU, each of the intermediate devices (representing various interconnects) along the way would have to be enabled for wakeup. So for example, even if the root hub's power/wakeup file says "enabled", if the controller's or one of the non-core devices' power/wakeup file is "disabled" then the wakeup signal shouldn't go through. It would be like a situation where a device's interrupt flag is enabled and turned on, but the device can't send an IRQ signal because it isn't allowed to access the PCI bus. This is why I originally suggested that the wakeup setting for the non-core devices and the controller should default to "enabled". Alan Stern
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); xhci->main_hcd = hcd; xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
After that, the user could enable controller as wakeup source for system suspend through /sys entry. Signed-off-by: Peter Chen <peter.chen@nxp.com> --- drivers/usb/host/xhci-plat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)