Rework "xhci: clear root port wake on bits if controller isn't wake-up capable"
Subsystems (2)
v3 cover Rework "xhci: clear root port wake on bits if controller isn't wake-up capable"
This serie of patch reworks commit ff8cbf250b448aac35589f6075082c3fcad8a8fe. This has been discussed at http://www.spinics.net/lists/linux-usb/msg114986.html
It also includes a patch to fix a comment in drivers/usb/host/xhci.h.
Changes in v3:
* Need to consider run-time suspend as well.
* Change "wake-up capable" to "allowed to do wakeup" in both comments and patch description.
* Add "Suggested-by: Alan Stern"
Changes in v2:
* Should not be a quirk.
* Should be applied to all xhci controllers.
Lu Baolu (3):
usb: xhci: Revert "xhci: clear root port wake on bits if controller
isn't wake-up capable"
usb: xhci: This reworks ff8cbf250b448aac35589f6075082c3fcad8a8fe
usb: xhci: fix comment for PORT_DEV_REMOVE
drivers/usb/host/xhci-hub.c | 5 +---- drivers/usb/host/xhci-pci.c | 2 +- drivers/usb/host/xhci-plat.c | 10 +++++++++- drivers/usb/host/xhci.c | 44 +++++++++++++++++++++++++++++++++++++++++++- drivers/usb/host/xhci.h | 4 ++-- 5 files changed, 56 insertions(+), 9 deletions(-)
Signature · 1 line
1.9.1
PATCH v3 1/3 usb: xhci: Revert "xhci: clear root port wake on bits if controller isn't wake-up capable"
This reverts commit ff8cbf250b448aac35589f6075082c3fcad8a8fe.
Commit ff8cbf250b448aac35589f6075082c3fcad8a8fe triggers the bug logged at
https://bugzilla.kernel.org/show_bug.cgi?id=85701
--- drivers/usb/host/xhci-hub.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/usb/host/xhci-hub.c +1 -4
@@ -22,7 +22,6 @@#include <linux/slab.h> -#include <linux/device.h> #include <asm/unaligned.h> #include "xhci.h"
@@ -1149,9 +1148,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)* including the USB 3.0 roothub, but only if CONFIG_PM_RUNTIME * is enabled, so also enable remote wake here. */ -if (hcd->self.root_hub->do_remote_wakeup - &&device_may_wakeup (hcd->self.controller)) { - +if (hcd->self.root_hub->do_remote_wakeup) {if (t1 &PORT_CONNECT ) { t2 |=PORT_WKOC_E |PORT_WKDISC_E ; t2 &= ~PORT_WKCONN_E ;
Signature · 1 line
1.9.1
PATCH v3 2/3 usb: xhci: This reworks ff8cbf250b448aac35589f6075082c3fcad8a8fe
In reply to Lu Baoluxhci: clear root port wake on bits if controller isn't allowed to do wakeup
When system is being suspended, if host device is not allowed to do wakeup, xhci_suspend() needs to clear all root port wake on bits. Otherwise, some platforms may generate spurious wakeup, even if PCI PME# is disabled.
--- drivers/usb/host/xhci-pci.c | 2 +- drivers/usb/host/xhci-plat.c | 10 +++++++++- drivers/usb/host/xhci.c | 44 +++++++++++++++++++++++++++++++++++++++++++- drivers/usb/host/xhci.h | 2 +- 4 files changed, 54 insertions(+), 4 deletions(-)
drivers/usb/host/xhci-pci.c +1 -1
@@ -291,7 +291,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)if (xhci->quirks &XHCI_COMP_MODE_QUIRK ) pdev->no_d3cold = true; -return xhci_suspend (xhci); +return xhci_suspend (xhci, do_wakeup); }static int xhci_pci_resume (struct usb_hcd *hcd,bool hibernated)
drivers/usb/host/xhci-plat.c +9 -1
@@ -204,7 +204,15 @@ static int xhci_plat_suspend(struct device *dev)struct usb_hcd *hcd =dev_get_drvdata (dev);struct xhci_hcd *xhci =hcd_to_xhci (hcd); -return xhci_suspend (xhci); +/* +* xhci_suspend() needs `do_wakeup` to know whether host is allowed +* to do wakeup during suspend. Since xhci_plat_suspend is currently +* only designed for system suspend, device_may_wakeup() is enough +* to dertermine whether host is allowed to do wakeup. Need to +* reconsider this when xhci_plat_suspend enlarges its scope, e.g., +* also applies to runtime suspend. +*/ +return xhci_suspend (xhci,device_may_wakeup (dev)); }static int xhci_plat_resume (struct device *dev)
drivers/usb/host/xhci.c +43 -1
68 lines not on this page · raw message
drivers/usb/host/xhci.h +1 -1
@@ -1746,7 +1746,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks);void xhci_init_driver (struct hc_driver *drv,int (*setup_fn)(struct usb_hcd *));#ifdef CONFIG_PM -int xhci_suspend (struct xhci_hcd *xhci); +int xhci_suspend (struct xhci_hcd *xhci,bool do_wakeup);int xhci_resume (struct xhci_hcd *xhci,bool hibernated);#else #define xhci_suspend NULL
Signature · 1 line
1.9.1
Re: [PATCH v3 2/3] usb: xhci: This reworks ff8cbf250b448aac35589f6075082c3fcad8a8fe
Quoted 8 lines · Lu Baolu
xhci: clear root port wake on bits if controller isn't allowed to do wakeup
When system is being suspended, if host device is not allowed to do wakeup,
xhci_suspend()needs to clear all root port wake on bits. Otherwise, some platforms may generate spurious wakeup, even if PCI PME# is disabled.Signed-off-by Lu Baolu <[email protected]>Suggested-by Alan Stern <[email protected]>
This looks pretty good now.
Quoted 9 lines
drivers/usb/host/xhci.c +9
-0+static void xhci_disable_port_wake_on_bits (struct xhci_hcd *xhci) +{ +int port_index; +__le32 __iomem **port_array; +unsigned long flags; +u32 t1, t2; + +spin_lock_irqsave (&xhci->lock, flags); +
drivers/usb/host/xhci.c static void xhci_clear_command_ring(struct xhci_hcd *xhci) · line 874
Quoted 10 lines
drivers/usb/host/xhci.c +10
-0+/* disble usb3 ports Wake bits*/ + port_index = xhci->num_usb3_ports; + port_array = xhci->usb3_ports; +while (port_index--) { + t1 =readl (port_array[port_index]); + t2 =xhci_port_state_to_neutral (t1); + t2 &= ~PORT_WAKE_BITS ; + t1 =xhci_port_state_to_neutral (t1); +if (t1 != t2) +writel (t2, port_array[port_index]);
Why not just do:
t1 = readl(port_array[port_index]); t1 = xhci_port_state_to_neutral(t1); t2 = t1 & ~PORT_WAKE_BITS; if (t1 != t2) ...
Apart from that minor point,
Alan Stern
Re: [PATCH v3 2/3] usb: xhci: This reworks ff8cbf250b448aac35589f6075082c3fcad8a8fe
Quoted 33 lines · Alan Stern
Quoted 8 lines · Lu Baolu
xhci: clear root port wake on bits if controller isn't allowed to do wakeup
When system is being suspended, if host device is not allowed to do wakeup,
xhci_suspend()needs to clear all root port wake on bits. Otherwise, some platforms may generate spurious wakeup, even if PCI PME# is disabled.Signed-off-by Lu Baolu <[email protected]>Suggested-by Alan Stern <[email protected]>This looks pretty good now.
Quoted 9 lines
drivers/usb/host/xhci.c +9
-0+static void xhci_disable_port_wake_on_bits (struct xhci_hcd *xhci) +{ +int port_index; +__le32 __iomem **port_array; +unsigned long flags; +u32 t1, t2; + +spin_lock_irqsave (&xhci->lock, flags); +drivers/usb/host/xhci.c static void xhci_clear_command_ring(struct xhci_hcd *xhci) · line 874
Quoted 10 lines
drivers/usb/host/xhci.c +10
-0+/* disble usb3 ports Wake bits*/ + port_index = xhci->num_usb3_ports; + port_array = xhci->usb3_ports; +while (port_index--) { + t1 =readl (port_array[port_index]); + t2 =xhci_port_state_to_neutral (t1); + t2 &= ~PORT_WAKE_BITS ; + t1 =xhci_port_state_to_neutral (t1); +if (t1 != t2) +writel (t2, port_array[port_index]);Why not just do:
Quoted 4 lines
t1 = readl(port_array[port_index]); t1 = xhci_port_state_to_neutral(t1); t2 = t1 & ~PORT_WAKE_BITS; if (t1 != t2) ...
Right, this looks better. I will test and resend the patches with this change.
Quoted 11 lines
Apart from that minor point,
Acked-by Alan Stern <[email protected]>Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 3/3 usb: xhci: fix comment for PORT_DEV_REMOVE
In reply to Lu BaoluAccording to xHCI specification, PORT_DEV_REMOVE(bit 30) in PORTSC true means "Device is non-removable".
--- drivers/usb/host/xhci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/host/xhci.h +1 -1
@@ -358,7 +358,7 @@ struct xhci_op_regs {/* wake on over-current (enable) */ #define PORT_WKOC_E (1 << 27)/* bits 28:29 reserved */ -/* true: device is removable - for USB 3.0 roothub emulation */ +/* true: device is non-removable - for USB 3.0 roothub emulation */ #define PORT_DEV_REMOVE (1 << 30)/* Initiate a warm port reset - complete when PORT_WRC is '1' */ #define PORT_WR (1 << 31)
Signature · 1 line
1.9.1