Hi Libin,
This code looks good. I have just a few minor comments below. My
biggest concern is why you're using atomic bit operations when I think
holding the xHCI lock will suffice.
Sarah Sharp
On Thu, May 13, 2010 at 06:11:39PM +0800, Libin Yang wrote:
> >From 9c521847f589612f912757f32f512c0d311e2477 Mon Sep 17 00:00:00 2001
> From: Crane Cai <crane.cai@xxxxxxx>
> Date: Wed, 24 Mar 2010 10:46:50 +0800
> Subject: [PATCH 1/4] xHCI: port power management implementation
>
> Add software trigger USB device suspend resume function hook.
> Do port suspend & resume in terms of xHCI spec.
>
> Port Suspend:
> Stop all endpoints via Stop Endpoint Command with Suspend (SP) flag set.
> Place individual ports into suspend mode by writing '3' for Port Link State
> (PLS) field into PORTSC register. This can only be done when the port is in
> Enabled state. When writing, the Port Link State Write Strobe (LWS) bit shall
> be set to '1'.
> Allocate an xhci_command and stash it in xhci_virt_device to wait completion for
> the last Stop Endpoint Command. Use the Suspend bit in TRB to indicate the Stop
> Endpoint Command is for port suspend. Based on Sarah's suggestion.
>
> Port Resume:
> Write '0' in PLS field, device will transition to running state.
> Ring an endpoints' doorbell to restart it.
>
> Ref: USB device remote wake need another patch to implement. For details of
> how USB subsystem do power management, please see:
> Documentation/usb/power-management.txt
>
> Signed-off-by: Crane Cai <crane.cai@xxxxxxx>
> Signed-off-by: Libin Yang <libin.yang@xxxxxxx>
> ---
> drivers/usb/host/xhci-hub.c | 194 +++++++++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci-ring.c | 35 +++++++-
> drivers/usb/host/xhci.c | 2 +-
> drivers/usb/host/xhci.h | 16 +++-
> 4 files changed, 239 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index dd69df1..67b415f 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -129,6 +129,104 @@ static u32 xhci_port_state_to_neutral(u32 state)
> return (state & XHCI_PORT_RO) | (state & XHCI_PORT_RWS);
> }
>
> +/*
> + * find slot id based on port number.
> + */
> +static int xhci_find_slot_id_by_port(struct xhci_hcd *xhci, u16 port)
> +{
> + int slot_id;
> + int i;
> + struct xhci_container_ctx *out_ctx;
> + struct xhci_slot_ctx *slot_ctx;
> +
> + slot_id = 0;
> + for (i = 0; i < MAX_HC_SLOTS; i++) {
> + if (!xhci->devs[i])
> + continue;
> + out_ctx = xhci->devs[i]->out_ctx;
> + slot_ctx = xhci_get_slot_ctx(xhci, out_ctx);
> + if (DEVINFO_TO_ROOT_HUB_PORT(slot_ctx->dev_info2) == port) {
> + slot_id = i;
> + break;
I feel uncomfortable digging the root hub port out of the output
context, since the hardware has to maintain that, and the xHCI driver
doesn't do any checking to make sure the hardware is doing that
properly. Can you add a variable to the xhci_virt_device that holds the
root hub port instead?
> + }
> + }
> +
> + return slot_id;
> +}
> +
> +/*
> + * Stop device
> + * It issues stop endpoint command for EP 0 to 30. And wait the last command
> + * to complete.
> + * suspend will set to 1, if suspend bit need to set in command.
> + * Require xhci spin lock and will release it to wait commands finish.
This comment is confusing. Does the function require the xhci->lock to
be held by the caller, or does the function just need to take the
xhci->lock. From your code, it looks like the lock is not held when the
function is called.
> + */
> +static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> +{
> + struct xhci_virt_device *virt_dev;
> + struct xhci_command *cmd;
> + unsigned long flags;
> + int timeleft;
> + int ret;
> + int i;
> +
> + ret = 0;
> + virt_dev = xhci->devs[slot_id];
> + cmd = xhci_alloc_command(xhci, false, true, GFP_NOIO);
> + if (!cmd) {
> + xhci_dbg(xhci, "Couldn't allocate command structure.\n");
> + return -ENOMEM;
> + }
> +
> + spin_lock_irqsave(&xhci->lock, flags);
> + for (i = LAST_EP_INDEX; i > 0; i--) {
> + if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
> + xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
> + }
> + cmd->command_trb = xhci->cmd_ring->enqueue;
> + list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
> + xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
> + xhci_ring_cmd_db(xhci);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +
> + /* Wait for last stop endpoint command to finish */
> + timeleft = wait_for_completion_interruptible_timeout(
> + cmd->completion,
> + USB_CTRL_SET_TIMEOUT);
> + if (timeleft <= 0) {
> + xhci_warn(xhci, "%s while waiting for reset device command\n",
> + timeleft == 0 ? "Timeout" : "Signal");
Copy-paste error, this isn't a reset device command.
> + spin_lock_irqsave(&xhci->lock, flags);
> + /* The timeout might have raced with the event ring handler, so
> + * only delete from the list if the item isn't poisoned.
> + */
> + if (cmd->cmd_list.next != LIST_POISON1)
> + list_del(&cmd->cmd_list);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + ret = -ETIME;
> + goto command_cleanup;
> + }
> +
> +command_cleanup:
> + xhci_free_command(xhci, cmd);
> + return ret;
> +}
> +
> +/*
> + * Ring device, it rings the all doorbells unconditionally.
> + */
> +static void xhci_ring_device(struct xhci_hcd *xhci, int slot_id)
> +{
> + int i;
> +
> + for (i = 0; i < LAST_EP_INDEX + 1; i++)
> + if (xhci->devs[slot_id]->eps[i].ring &&
> + xhci->devs[slot_id]->eps[i].ring->dequeue)
> + ring_ep_doorbell(xhci, slot_id, i, 0);
> +
> + return;
> +}
> +
> static void xhci_disable_port(struct xhci_hcd *xhci, u16 wIndex,
> u32 __iomem *addr, u32 port_status)
> {
> @@ -162,6 +260,10 @@ static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue,
> status = PORT_PEC;
> port_change_bit = "enable/disable";
> break;
> + case USB_PORT_FEAT_C_SUSPEND:
> + status = PORT_PLC;
> + port_change_bit = "suspend/resume";
> + break;
> default:
> /* Should never happen */
> return;
> @@ -182,6 +284,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> u32 temp, status;
> int retval = 0;
> u32 __iomem *addr;
> + int slot_id;
>
> ports = HCS_MAX_PORTS(xhci->hcs_params1);
>
> @@ -211,9 +314,18 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> if ((temp & PORT_OCC))
> status |= 1 << USB_PORT_FEAT_C_OVER_CURRENT;
> /*
> - * FIXME ignoring suspend, reset, and USB 2.1/3.0 specific
> + * FIXME ignoring reset and USB 2.1/3.0 specific
> * changes
> */
> + if ((temp & PORT_PLS_MASK) == XDEV_U3
> + && (temp & PORT_POWER))
> + status |= 1 << USB_PORT_FEAT_SUSPEND;
> + if ((temp & PORT_PLS_MASK) == XDEV_U0
> + && (temp & PORT_POWER)
> + && test_bit(wIndex, &xhci->suspended_ports)) {
> + clear_bit(wIndex, &xhci->suspended_ports);
> + set_bit(wIndex, &xhci->port_c_suspend);
> + }
This code is under the xhci->lock, so why do you need atomic bit
operations?
> if (temp & PORT_CONNECT) {
> status |= 1 << USB_PORT_FEAT_CONNECTION;
> status |= xhci_port_speed(temp);
> @@ -226,6 +338,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> status |= 1 << USB_PORT_FEAT_RESET;
> if (temp & PORT_POWER)
> status |= 1 << USB_PORT_FEAT_POWER;
> + if (test_bit(wIndex, &xhci->port_c_suspend))
> + status |= 1 << USB_PORT_FEAT_C_SUSPEND;
> xhci_dbg(xhci, "Get port status returned 0x%x\n", status);
> put_unaligned(cpu_to_le32(status), (__le32 *) buf);
> break;
> @@ -238,6 +352,41 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> temp = xhci_readl(xhci, addr);
> temp = xhci_port_state_to_neutral(temp);
> switch (wValue) {
> + case USB_PORT_FEAT_SUSPEND:
> + temp = xhci_readl(xhci, addr);
> + /* In spec software should not attempt to suspend
> + * a port unless the port reports that it is in the
> + * enabled (PED = ‘1’,PLS < ‘3’) state.
> + */
> + if ((temp & PORT_PE) == 0 || (temp & PORT_RESET)
> + || (temp & PORT_PLS_MASK) >= XDEV_U3) {
> + xhci_dbg(xhci, "goto error status = 0x%x\n",
> + temp);
Could you make this debugging statement a warning and make it more
clear? If this case is triggered, it means there is a bug in the USB
core because it's attempting to suspend a device that is disconnected,
is still enumerating, or otherwise hasn't stabilized into the "active"
U0 state, or a lower link PM state like U1/U2. How about something
like:
xhci_warn(xhci, "USB core suspending device not in U0/U1/U2.\n");
> + goto error;
> + }
> +
> + slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
> + if (!slot_id) {
> + xhci_dbg(xhci, "slot_id is zero\n");
> + goto error;
> + }
> + /* unlock to execute stop endpoint commands */
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + xhci_stop_device(xhci, slot_id, 1);
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_U3;
> + xhci_writel(xhci, temp, addr);
> +
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + msleep(10); /* wait device to enter */
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_readl(xhci, addr);
> + set_bit(wIndex, &xhci->suspended_ports);
> + break;
> case USB_PORT_FEAT_POWER:
> /*
> * Turn on ports, even if there isn't per-port switching.
> @@ -271,6 +420,49 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> temp = xhci_readl(xhci, addr);
> temp = xhci_port_state_to_neutral(temp);
> switch (wValue) {
> + case USB_PORT_FEAT_SUSPEND:
> + temp = xhci_readl(xhci, addr);
> + xhci_dbg(xhci, "clear USB_PORT_FEAT_SUSPEND\n");
> + xhci_dbg(xhci, "PORTSC %04x\n", temp);
> + if (temp & PORT_RESET)
> + goto error;
> + if (temp & XDEV_U3) {
> + if ((temp & PORT_PE) == 0)
> + goto error;
> + if (DEV_SUPERSPEED(temp)) {
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_U0;
> + xhci_writel(xhci, temp, addr);
> + } else {
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_RESUME;
> + xhci_writel(xhci, temp, addr);
> +
> + spin_unlock_irqrestore(&xhci->lock,
> + flags);
> + msleep(20);
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_readl(xhci, addr);
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_U0;
> + xhci_writel(xhci, temp, addr);
> + }
> + set_bit(wIndex, &xhci->port_c_suspend);
> + }
> +
> + slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
> + if (!slot_id) {
> + xhci_dbg(xhci, "slot_id is zero\n");
> + goto error;
> + }
> + xhci_ring_device(xhci, slot_id);
> + break;
> + case USB_PORT_FEAT_C_SUSPEND:
> + clear_bit(wIndex, &xhci->port_c_suspend);
> case USB_PORT_FEAT_C_RESET:
> case USB_PORT_FEAT_C_CONNECTION:
> case USB_PORT_FEAT_C_OVER_CURRENT:
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index b648efa..f67f127 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -68,6 +68,10 @@
> #include <linux/slab.h>
> #include "xhci.h"
>
> +static int handle_cmd_in_cmd_wait_list(struct xhci_hcd *xhci,
> + struct xhci_virt_device *virt_dev,
> + struct xhci_event_cmd *event);
> +
> /*
> * Returns zero if the TRB isn't in this segment, otherwise it returns the DMA
> * address of the TRB.
> @@ -293,7 +297,7 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
> xhci_readl(xhci, &xhci->dba->doorbell[0]);
> }
>
> -static void ring_ep_doorbell(struct xhci_hcd *xhci,
> +void ring_ep_doorbell(struct xhci_hcd *xhci,
Please change this name to xhci_ring_ep_doorbell. Since it isn't a
static function anymore, it needs to have the xhci_ prefix.
> unsigned int slot_id,
> unsigned int ep_index,
> unsigned int stream_id)
> @@ -558,10 +562,11 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
> * bit cleared) so that the HW will skip over them.
> */
> static void handle_stopped_endpoint(struct xhci_hcd *xhci,
> - union xhci_trb *trb)
> + union xhci_trb *trb, struct xhci_event_cmd *event)
> {
> unsigned int slot_id;
> unsigned int ep_index;
> + struct xhci_virt_device *virt_dev;
> struct xhci_ring *ep_ring;
> struct xhci_virt_ep *ep;
> struct list_head *entry;
> @@ -570,6 +575,21 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci,
>
> struct xhci_dequeue_state deq_state;
>
> + if (unlikely(TRB_TO_SUSPEND_PORT(
> + xhci->cmd_ring->dequeue->generic.field[3]))) {
> + slot_id = TRB_TO_SLOT_ID(
> + xhci->cmd_ring->dequeue->generic.field[3]);
> + virt_dev = xhci->devs[slot_id];
> + if (virt_dev)
> + handle_cmd_in_cmd_wait_list(xhci, virt_dev,
> + event);
> + else
> + xhci_warn(xhci, "Stop endpoint command "
> + "completion for disabled slot %u\n",
> + slot_id);
> + return;
> + }
> +
> memset(&deq_state, 0, sizeof(deq_state));
> slot_id = TRB_TO_SLOT_ID(trb->generic.field[3]);
> ep_index = TRB_TO_EP_INDEX(trb->generic.field[3]);
> @@ -1021,7 +1041,7 @@ bandwidth_change:
> complete(&xhci->addr_dev);
> break;
> case TRB_TYPE(TRB_STOP_RING):
> - handle_stopped_endpoint(xhci, xhci->cmd_ring->dequeue);
> + handle_stopped_endpoint(xhci, xhci->cmd_ring->dequeue, event);
> break;
> case TRB_TYPE(TRB_SET_DEQ):
> handle_set_deq_completion(xhci, event, xhci->cmd_ring->dequeue);
> @@ -2334,15 +2354,20 @@ int xhci_queue_evaluate_context(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
> false);
> }
>
> +/*
> + * Suspend is set to indicate "Stop Endpoint Command" is being issued to stop
> + * activity on an endpoint that is about to be suspended.
> + */
> int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
> - unsigned int ep_index)
> + unsigned int ep_index, int suspend)
> {
> u32 trb_slot_id = SLOT_ID_FOR_TRB(slot_id);
> u32 trb_ep_index = EP_ID_FOR_TRB(ep_index);
> u32 type = TRB_TYPE(TRB_STOP_RING);
> + u32 trb_suspend = SUSPEND_PORT_FOR_TRB(suspend);
>
> return queue_command(xhci, 0, 0, 0,
> - trb_slot_id | trb_ep_index | type, false);
> + trb_slot_id | trb_ep_index | type | trb_suspend, false);
> }
>
> /* Set Transfer Ring Dequeue Pointer command.
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d5435c5..29da127 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -855,7 +855,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> ep->stop_cmd_timer.expires = jiffies +
> XHCI_STOP_EP_CMD_TIMEOUT * HZ;
> add_timer(&ep->stop_cmd_timer);
> - xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index);
> + xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index, 0);
> xhci_ring_cmd_db(xhci);
> }
> done:
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index fb3440b..351f464 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -269,6 +269,10 @@ struct xhci_op_regs {
> * A read gives the current link PM state of the port,
> * a write with Link State Write Strobe set sets the link state.
> */
> +#define PORT_PLS_MASK (0xf << 5)
> +#define XDEV_U0 (0x0 << 5)
> +#define XDEV_U3 (0x3 << 5)
> +#define XDEV_RESUME (0xf << 5)
> /* true: port has power (see HCC_PPC) */
> #define PORT_POWER (1 << 9)
> /* bits 10:13 indicate device speed:
> @@ -510,6 +514,7 @@ struct xhci_slot_ctx {
> #define MAX_EXIT (0xffff)
> /* Root hub port number that is needed to access the USB device */
> #define ROOT_HUB_PORT(p) (((p) & 0xff) << 16)
> +#define DEVINFO_TO_ROOT_HUB_PORT(p) (((p) >> 16) & 0xff)
> /* Maximum number of ports under a hub device */
> #define XHCI_MAX_PORTS(p) (((p) & 0xff) << 24)
>
> @@ -873,6 +878,10 @@ struct xhci_event_cmd {
> #define TRB_TO_EP_INDEX(p) ((((p) & (0x1f << 16)) >> 16) - 1)
> #define EP_ID_FOR_TRB(p) ((((p) + 1) & 0x1f) << 16)
>
> +#define SUSPEND_PORT_FOR_TRB(p) (((p) & 1) << 23)
> +#define TRB_TO_SUSPEND_PORT(p) (((p) & (1 << 23)) >> 23)
> +#define LAST_EP_INDEX 30
> +
> /* Set TR Dequeue Pointer command TRB fields */
> #define TRB_TO_STREAM_ID(p) ((((p) & (0xffff << 16)) >> 16))
> #define STREAM_ID_FOR_TRB(p) ((((p)) & 0xffff) << 16)
> @@ -1172,6 +1181,9 @@ struct xhci_hcd {
> unsigned int quirks;
> #define XHCI_LINK_TRB_QUIRK (1 << 0)
> #define XHCI_RESET_EP_QUIRK (1 << 1)
> + unsigned long port_c_suspend; /* port suspend change*/
> + unsigned long suspended_ports; /* which ports are
> + suspended */
> };
>
> /* For testing purposes */
> @@ -1380,7 +1392,7 @@ int xhci_queue_slot_control(struct xhci_hcd *xhci, u32 trb_type, u32 slot_id);
> int xhci_queue_address_device(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
> u32 slot_id);
> int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
> - unsigned int ep_index);
> + unsigned int ep_index, int suspend);
> int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags, struct urb *urb,
> int slot_id, unsigned int ep_index);
> int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags, struct urb *urb,
> @@ -1408,6 +1420,8 @@ void xhci_queue_config_ep_quirk(struct xhci_hcd *xhci,
> unsigned int slot_id, unsigned int ep_index,
> struct xhci_dequeue_state *deq_state);
> void xhci_stop_endpoint_command_watchdog(unsigned long arg);
> +void ring_ep_doorbell(struct xhci_hcd *xhci, unsigned int slot_id,
> + unsigned int ep_index, unsigned int stream_id);
>
> /* xHCI roothub code */
> int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
> --
> 1.6.3.3
>
>
>
--
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
