Hi Libin,
My comments are below. Looks fine, aside from a couple places need
comments and xhci_stop_endpoints() needs some work. I haven't had a
chance to test.
Sarah Sharp
On Wed, Mar 03, 2010 at 06:02:42PM +0800, Libin wrote:
> >From 858d0e67059852385be474a32b08c72cf4adbe75 Mon Sep 17 00:00:00 2001
> From: Crane Cai <crane.cai@xxxxxxx>
> Date: Tue, 2 Mar 2010 14:16:07 +0800
> Subject: [PATCH 1/5] xhci: port power management implementation
>
> Add software trigger USB device suspend resume function hook.
> Do port suspend & resume in terms of xHCI spec 4.15.
> System trigger suspend/resume via:
> echo suspend > /sys/bus/usb/devices/../power/level
> echo on > /sys/bus/usb/devices/../power/level
> Tested with USB storage device & mouse.
>
> 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 | 144 +++++++++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci-ring.c | 2 +-
> drivers/usb/host/xhci.h | 9 +++
> 3 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 208b805..0f5f0db 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -128,6 +128,76 @@ static u32 xhci_port_state_to_neutral(u32 state)
> /* Save read-only status and port state */
> return (state & XHCI_PORT_RO) | (state & XHCI_PORT_RWS);
> }
> +/*
> + * stop endpoints before the port suspend, and ring the bell before resume.
> + * caller need xhci lock and may release it to finish commands.
> + */
> +static u32 xhci_stop_endpoints(struct xhci_hcd *xhci, u16 port, int stop,
> + unsigned long *flags)
> +{
> + int i, slot_id;
> + struct xhci_container_ctx *out_ctx;
> + struct xhci_slot_ctx *slot_ctx;
> + struct xhci_ep_ctx *ep_ctx;
> + struct xhci_virt_ep *ep;
> +
> + slot_id = 0;
> + if (!xhci->devs)
> + return 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 (GET_SLOT_STATE(slot_ctx->dev_state) == 3 &&
> + ROOT_HUB_PORT(slot_ctx->dev_info2) == port) {
> + slot_id = i;
> + break;
> + }
> + }
> +
> + if (!slot_id)
> + return 0;
> +
> + if (stop)
> + goto stop;
> + else
> + goto run;
Can you instead code this as:
if (run) {
> + for (i = 0; i < 31; i++) {
> + ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, i);
> + if ((ep_ctx->ep_info & EP_STATE_MASK) == EP_STATE_STOPPED)
> + ring_ep_doorbell(xhci, slot_id, i);
> + }
> + return 0;
}
Labels should really only be used in shared error handling code, where
you need to drop down. See Chapter 7 of Documentation/CodingStyle.
> + for (i = 0; i < 31; i++) {
> + ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, i);
> + if ((ep_ctx->ep_info & EP_STATE_MASK) != EP_STATE_RUNNING)
> + continue;
> + ep = &xhci->devs[slot_id]->eps[i];
> + if (!(ep->ep_state & EP_HALT_PENDING)) {
> + ep->ep_state |= EP_HALT_PENDING;
> + ep->stop_cmds_pending++;
> + ep->stop_cmd_timer.expires = jiffies +
> + XHCI_STOP_EP_CMD_TIMEOUT * HZ;
> + add_timer(&ep->stop_cmd_timer);
> + xhci_queue_stop_endpoint(xhci, slot_id, i);
> + xhci_ring_cmd_db(xhci);
> + }
> + }
> + spin_unlock_irqrestore(&xhci->lock, *flags);
> + for (i = 0; i < 31; i++) {
> + ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, i);
> + if ((ep_ctx->ep_info & EP_STATE_MASK) == EP_STATE_RUNNING) {
> + i = 0;
> + msleep(1);
> + }
> + }
> + spin_lock_irqsave(&xhci->lock, *flags);
Why not wait on a completion used in the last stop endpoint command?
Since commands must be completed in the order they're put on the ring,
you know when you get the last stop endpoint command completion you
should be done. Busy waiting is not generally good and this loop looks
ugly.
Also, I think you need to modify xhci_queue_stop_endpoint() to set the
Suspend (SP) bit in the Stop Endpoint Command TRB. That will let the
xHCI hardware know you're stopping an endpoint because you're suspending
the device.
> + return 0;
> +
> +}
>
> static void xhci_disable_port(struct xhci_hcd *xhci, u16 wIndex,
> u32 __iomem *addr, u32 port_status)
> @@ -162,6 +232,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;
> @@ -211,9 +285,17 @@ 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_PLS_MASK) == XDEV_RESUME)
> + status |= 1 << USB_PORT_FEAT_SUSPEND;
So XDEV_U3 means you've read a 3 from the port link state registers,
meaning the port is in U3 (device suspend). And XDEV_RESUME means the
device is "in the resume state". Does that mean the device is resuming,
or resumed? Can you add a comment to clarify this?
> + if ((temp & PORT_PLS_MASK) == XDEV_U0
> + && test_bit(wIndex, &xhci->suspended_ports)) {
> + clear_bit(wIndex, &xhci->suspended_ports);
> + set_bit(wIndex, &xhci->port_c_suspend);
> + }
> if (temp & PORT_CONNECT) {
> status |= 1 << USB_PORT_FEAT_CONNECTION;
> status |= xhci_port_speed(temp);
> @@ -226,6 +308,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 +322,27 @@ 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);
> + if ((temp & PORT_PE) == 0 || (temp & PORT_RESET)
> + || (temp & PORT_PLS_MASK) >= XDEV_U3) {
> + xhci_dbg(xhci, "goto error status = 0x%x\n",
> + temp);
> + goto error;
> + }
You need to go into detail in your commit message (or in a comment)
about what error handling you're working on here. Particularly, what
(temp & PORT_PLS_MASK) >= XDEV_U3) means, since some people don't have
access to the xHCI spec.
> + xhci_stop_endpoints(xhci, wIndex + 1, 1, &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 */
Isn't this wait handled by the USB core?
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_readl(xhci, addr);
> + set_bit(wIndex, &xhci->suspended_ports);
Do you want to set this inside of the lock? I'm not sure how you use
xhci->suspended_ports; do you need mutual exclusion?
> + break;
> case USB_PORT_FEAT_POWER:
> /*
> * Turn on ports, even if there isn't per-port switching.
> @@ -271,6 +376,43 @@ 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);
What's to stop some other part of the xHCI hub code from mucking with
your port registers here?
> + 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);
> + }
> + xhci_stop_endpoints(xhci, wIndex + 1, 0, &flags);
> + 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 6ba841b..71d0d5d 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -292,7 +292,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,
> unsigned int slot_id,
> unsigned int ep_index)
> {
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index e5eb09b..1a16c5d 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:
> @@ -1112,6 +1116,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 */
> @@ -1314,6 +1321,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);
>
> /* xHCI roothub code */
> int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
> --
> 1.6.0.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
