* [PATCH 1/2] usb: host: xhci: avoid calling two contineous times for xhci_suspend
@ 2020-07-03 6:25 Peter Chen
2020-07-03 6:25 ` [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-03 6:25 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, gregkh, linux-imx, Peter Chen
If the xhci-plat.c is the platform driver, after the runtime pm is
enabled, the xhci_suspend is called if nothing is connected on
the port. When the system goes to suspend, it will call xhci_suspend again
if USB wakeup is enabled.
Since the runtime suspend wakeup setting is not always the same with
system suspend wakeup setting, eg, at runtime suspend, we always need
wakeup if the controller is in low power mode; but at suspend system,
we may not need wakeup. So, we move the judgement after changing
wakeup setting.
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
drivers/usb/host/xhci.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ed468eed299c..66c894626be6 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -982,12 +982,15 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
xhci->shared_hcd->state != HC_STATE_SUSPENDED)
return -EINVAL;
- xhci_dbc_suspend(xhci);
-
/* Clear root port wake on bits if wakeup not allowed. */
if (!do_wakeup)
xhci_disable_port_wake_on_bits(xhci);
+ if (!HCD_HW_ACCESSIBLE(hcd))
+ return 0;
+
+ xhci_dbc_suspend(xhci);
+
/* Don't poll the roothubs on bus suspend. */
xhci_dbg(xhci, "%s: stopping port polling.\n", __func__);
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-03 6:25 [PATCH 1/2] usb: host: xhci: avoid calling two contineous times for xhci_suspend Peter Chen
@ 2020-07-03 6:25 ` Peter Chen
2020-07-03 14:19 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-03 6:25 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, gregkh, linux-imx, Peter Chen
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);
xhci->main_hcd = hcd;
xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-03 6:25 ` [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys Peter Chen
@ 2020-07-03 14:19 ` Alan Stern
2020-07-04 9:22 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-03 14:19 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-03 14:19 ` Alan Stern
@ 2020-07-04 9:22 ` Peter Chen
2020-07-04 14:48 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-04 9:22 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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?
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-04 9:22 ` Peter Chen
@ 2020-07-04 14:48 ` Alan Stern
2020-07-05 2:12 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-04 14:48 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-04 14:48 ` Alan Stern
@ 2020-07-05 2:12 ` Peter Chen
2020-07-05 14:31 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-05 2:12 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-05 2:12 ` Peter Chen
@ 2020-07-05 14:31 ` Alan Stern
2020-07-06 4:03 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-05 14:31 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-05 14:31 ` Alan Stern
@ 2020-07-06 4:03 ` Peter Chen
2020-07-06 16:22 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-06 4:03 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
> > > > > > ---
> > > > > > 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-06 4:03 ` Peter Chen
@ 2020-07-06 16:22 ` Alan Stern
2020-07-07 2:01 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-06 16:22 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-06 16:22 ` Alan Stern
@ 2020-07-07 2:01 ` Peter Chen
2020-07-07 16:11 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-07 2:01 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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?
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-07 2:01 ` Peter Chen
@ 2020-07-07 16:11 ` Alan Stern
2020-07-08 6:47 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-07 16:11 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-07 16:11 ` Alan Stern
@ 2020-07-08 6:47 ` Peter Chen
2020-07-08 15:02 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-08 6:47 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-08 6:47 ` Peter Chen
@ 2020-07-08 15:02 ` Alan Stern
2020-07-09 5:15 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-08 15:02 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-08 15:02 ` Alan Stern
@ 2020-07-09 5:15 ` Peter Chen
2020-07-09 14:50 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-09 5:15 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-09 5:15 ` Peter Chen
@ 2020-07-09 14:50 ` Alan Stern
2020-07-21 11:03 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Alan Stern @ 2020-07-09 14:50 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-09 14:50 ` Alan Stern
@ 2020-07-21 11:03 ` Peter Chen
2020-07-21 14:10 ` Alan Stern
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-07-21 11:03 UTC (permalink / raw)
To: Alan Stern; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx, Jun Li
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.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys
2020-07-21 11:03 ` Peter Chen
@ 2020-07-21 14:10 ` Alan Stern
0 siblings, 0 replies; 17+ messages in thread
From: Alan Stern @ 2020-07-21 14:10 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb, gregkh, dl-linux-imx, Jun Li
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-07-21 14:10 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-03 6:25 [PATCH 1/2] usb: host: xhci: avoid calling two contineous times for xhci_suspend Peter Chen
2020-07-03 6:25 ` [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys Peter Chen
2020-07-03 14:19 ` Alan Stern
2020-07-04 9:22 ` Peter Chen
2020-07-04 14:48 ` Alan Stern
2020-07-05 2:12 ` Peter Chen
2020-07-05 14:31 ` Alan Stern
2020-07-06 4:03 ` Peter Chen
2020-07-06 16:22 ` Alan Stern
2020-07-07 2:01 ` Peter Chen
2020-07-07 16:11 ` Alan Stern
2020-07-08 6:47 ` Peter Chen
2020-07-08 15:02 ` Alan Stern
2020-07-09 5:15 ` Peter Chen
2020-07-09 14:50 ` Alan Stern
2020-07-21 11:03 ` Peter Chen
2020-07-21 14:10 ` Alan Stern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.