[PATCH 1/2] usb: host: xhci: avoid calling two contineous times for xhci_suspend

[PATCH 1/2] usb: host: xhci: avoid calling two contineous times for xhci_suspend

[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

[PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

RE: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

> > > > > > ---
> > > > > >  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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Peter Chen]

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

Re: [PATCH 2/2] usb: host: xhci-plat: add wakeup entry at /sys

[Alan Stern]

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