On Wed, Jan 25, 2012 at 03:36:10PM -0800, Sarah Sharp wrote:
> USB 3.0 hubs have a different remote wakeup policy than USB 2.0 hubs.
> USB 2.0 hubs, once they have remote wakeup enabled, will always send
> remote wakes when anything changes on a port.
>
> However, USB 3.0 hubs have a per-port remote wake up policy that is off
> by default. The Set Feature remote wake mask can be changed for any
> port, enabling remote wakeup for a connect, disconnect, or overcurrent
> event, much like EHCI and xHCI host controller "wake on" port status
> bits. The bits are cleared to zero on the initial hub power on, or
> after the hub has been reset.
>
> Without this patch, when a USB 3.0 hub gets suspended, it will not send
> a remote wakeup on device connect or disconnect. This would show up to
> the user as "dead ports" unless they ran lsusb -v (since newer versions
> of lsusb use the sysfs files, rather than sending control transfers).
>
> Change the hub driver's suspend method to enable remote wake up for
> disconnect, connect, and overcurrent for all ports on the hub. Modify
> the xHCI driver's roothub code to handle that request, and set the "wake
> on" bits in the port status registers accordingly.
>
> Alan: there are three things I'm uncertain about here, and I would
> appreciate your input.
Sorry Alan, I changed the patch and forgot to take out my questions that
you've already answered. Please ignore the remainder of the commit
message.
>
> First, should we follow what EHCI and xHCI do during bus suspend, by
> only enabling disconnect and overcurrent wake notifications for ports
> with a device connected to a USB 3.0 hub, and enabling connect and
> overcurrent wake notifications for ports without a connection? I've
> long wondered why xHCI can't just set all the wake-on bits for a port,
> rather than try to make the racy distinction between connected and
> unconnected ports.
>
> Second, what is the expected behavior of system suspend when
> CONFIG_USB_SUSPEND is turned off? It looks like in the current EHCI and
> xHCI code that the "wake on" bits are set in bus_suspend. Does that
> method get called if CONFIG_USB_SUSPEND is turned off? Is the intention
> that the system will resume if a new USB device is plugged in, even if
> CONFIG_USB_SUSPEND is off?
>
> I ask because it seems like hub_suspend is not going to be called for
> the USB 3.0 roothub if CONFIG_USB_SUSPEND is off. If I want to be sure
> the wake on bits are set for USB 3.0 ports (rather than relying on the
> new code in hub_suspend), I have to have duplicate code in xHCI's
> bus_suspend method to set those bits.
>
> Third, should we only set the wake on bits for USB 3.0 hubs during the
> hub initialization, or should we wait to do that in hub_suspend?
>
> Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/usb/core/hub.c | 12 ++++++++++++
> drivers/usb/host/xhci-hub.c | 41 +++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/ch11.h | 5 +++++
> 3 files changed, 58 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index eb6a76d..4423245 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2691,6 +2691,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
> struct usb_hub *hub = usb_get_intfdata (intf);
> struct usb_device *hdev = hub->hdev;
> unsigned port1;
> + int status;
>
> /* Warn if children aren't already suspended */
> for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> @@ -2703,6 +2704,17 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
> return -EBUSY;
> }
> }
> + if (hub_is_superspeed(hdev) && hdev->do_remote_wakeup) {
> + /* Enable hub to send remote wakeup for all ports. */
> + for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> + status = set_port_feature(hdev,
> + port1 |
> + USB_PORT_FEAT_REMOTE_WAKE_CONNECT |
> + USB_PORT_FEAT_REMOTE_WAKE_DISCONNECT |
> + USB_PORT_FEAT_REMOTE_WAKE_OVER_CURRENT,
> + USB_PORT_FEAT_REMOTE_WAKE_MASK);
> + }
> + }
>
> dev_dbg(&intf->dev, "%s\n", __func__);
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 35e257f..d98acc9 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -422,6 +422,32 @@ void xhci_set_link_state(struct xhci_hcd *xhci, __le32 __iomem **port_array,
> xhci_writel(xhci, temp, port_array[port_id]);
> }
>
> +void xhci_set_remote_wake_mask(struct xhci_hcd *xhci,
> + __le32 __iomem **port_array, int port_id, u16 wake_mask)
> +{
> + u32 temp;
> +
> + temp = xhci_readl(xhci, port_array[port_id]);
> + temp = xhci_port_state_to_neutral(temp);
> +
> + if (wake_mask & USB_PORT_FEAT_REMOTE_WAKE_CONNECT)
> + temp |= PORT_WKCONN_E;
> + else
> + temp &= ~PORT_WKCONN_E;
> +
> + if (wake_mask & USB_PORT_FEAT_REMOTE_WAKE_DISCONNECT)
> + temp |= PORT_WKDISC_E;
> + else
> + temp &= ~PORT_WKDISC_E;
> +
> + if (wake_mask & USB_PORT_FEAT_REMOTE_WAKE_OVER_CURRENT)
> + temp |= PORT_WKOC_E;
> + else
> + temp &= ~PORT_WKOC_E;
> +
> + xhci_writel(xhci, temp, port_array[port_id]);
> +}
> +
> /* Test and clear port RWC bit */
> void xhci_test_and_clear_bit(struct xhci_hcd *xhci, __le32 __iomem **port_array,
> int port_id, u32 port_bit)
> @@ -448,6 +474,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> int slot_id;
> struct xhci_bus_state *bus_state;
> u16 link_state = 0;
> + u16 wake_mask = 0;
>
> max_ports = xhci_get_ports(hcd, &port_array);
> bus_state = &xhci->bus_state[hcd_index(hcd)];
> @@ -593,6 +620,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> case SetPortFeature:
> if (wValue == USB_PORT_FEAT_LINK_STATE)
> link_state = (wIndex & 0xff00) >> 3;
> + if (wValue == USB_PORT_FEAT_REMOTE_WAKE_MASK)
> + wake_mask = wIndex & 0xff00;
> wIndex &= 0xff;
> if (!wIndex || wIndex > max_ports)
> goto error;
> @@ -703,6 +732,14 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> temp = xhci_readl(xhci, port_array[wIndex]);
> xhci_dbg(xhci, "set port reset, actual port %d status = 0x%x\n", wIndex, temp);
> break;
> + case USB_PORT_FEAT_REMOTE_WAKE_MASK:
> + xhci_set_remote_wake_mask(xhci, port_array,
> + wIndex, wake_mask);
> + temp = xhci_readl(xhci, port_array[wIndex]);
> + xhci_dbg(xhci, "set port remote wake mask, "
> + "actual port %d status = 0x%x\n",
> + wIndex, temp);
> + break;
> case USB_PORT_FEAT_BH_PORT_RESET:
> temp |= PORT_WR;
> xhci_writel(xhci, temp, port_array[wIndex]);
> @@ -883,6 +920,10 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> t2 |= PORT_LINK_STROBE | XDEV_U3;
> set_bit(port_index, &bus_state->bus_suspended);
> }
> + /* USB core sets remote wake mask for USB 3.0 hubs,
> + * including the USB 3.0 roothub, but only if CONFIG_USB_SUSPEND
> + * is enabled, so also enable remote wake here.
> + */
> if (hcd->self.root_hub->do_remote_wakeup) {
> if (t1 & PORT_CONNECT) {
> t2 |= PORT_WKOC_E | PORT_WKDISC_E;
> diff --git a/include/linux/usb/ch11.h b/include/linux/usb/ch11.h
> index 0b83acd..f1d26b6 100644
> --- a/include/linux/usb/ch11.h
> +++ b/include/linux/usb/ch11.h
> @@ -76,6 +76,11 @@
> #define USB_PORT_FEAT_C_BH_PORT_RESET 29
> #define USB_PORT_FEAT_FORCE_LINKPM_ACCEPT 30
>
> +/* USB 3.0 hub remote wake mask bits, see table 10-14 */
> +#define USB_PORT_FEAT_REMOTE_WAKE_CONNECT (1 << 8)
> +#define USB_PORT_FEAT_REMOTE_WAKE_DISCONNECT (1 << 9)
> +#define USB_PORT_FEAT_REMOTE_WAKE_OVER_CURRENT (1 << 10)
> +
> /*
> * Hub Status and Hub Change results
> * See USB 2.0 spec Table 11-19 and Table 11-20
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
