I have questions about this patch, see below.
Sarah Sharp
On Wed, Mar 03, 2010 at 06:10:45PM +0800, Libin wrote:
> >From bf08f32104f0d4d13b3a61eea14133f0f4d1e3cc Mon Sep 17 00:00:00 2001
> From: Libin Yang <libin.yang@xxxxxxx>
> Date: Wed, 3 Mar 2010 16:49:57 +0800
> Subject: [PATCH 4/5] xHCI: PCI power management implementation
>
> This patch implements the PCI suspend/resume.
>
> Please refer xHCI spec for doing the suspend/resume operation.
>
> For S3, CSS/SRS in USBCMD is used to save/restore the internal state.
> However, an error maybe occurs while restoring the internal state.
> In this case, it means that HC internal state is wrong and HC reset is
> needed.
>
> Signed-off-by: Dong Nguyen <dong.nguyen@xxxxxxx>
> Signed-off-by: Libin Yang <libin.yang@xxxxxxx>
> ---
> drivers/usb/host/xhci-hcd.c | 272 ++++++++++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci-mem.c | 2 +-
> drivers/usb/host/xhci-pci.c | 45 +++++++-
> drivers/usb/host/xhci.h | 28 +++++-
> 4 files changed, 342 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hcd.c b/drivers/usb/host/xhci-hcd.c
> index fe6a113..adb29df 100644
> --- a/drivers/usb/host/xhci-hcd.c
> +++ b/drivers/usb/host/xhci-hcd.c
> @@ -20,6 +20,7 @@
> * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#include <linux/pci.h>
> #include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> @@ -48,7 +49,7 @@ MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
> * handshake done). There are two failure modes: "usec" have passed (major
> * hardware flakeout), or the register reads as all-ones (hardware removed).
> */
> -static int handshake(struct xhci_hcd *xhci, void __iomem *ptr,
> +int handshake(struct xhci_hcd *xhci, void __iomem *ptr,
> u32 mask, u32 done, int usec)
> {
> u32 result;
> @@ -531,6 +532,275 @@ void xhci_shutdown(struct usb_hcd *hcd)
> xhci_readl(xhci, &xhci->op_regs->status));
> }
>
> +/*
> + * Stop HC (not bus-specific)
> + *
> + * This is called when the machine transition into S3/S4 mode.
> + *
> + */
> +int xhci_suspend(struct xhci_hcd *xhci)
> +{
> + int rc = 0;
> + struct usb_hcd *hcd = xhci_to_hcd(xhci);
> + u32 command;
> +
> + if (hcd->state != HC_STATE_SUSPENDED) {
> + rc = -EINVAL;
> + goto bail;
> + }
> +
> + spin_lock_irq(&xhci->lock);
> + clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> + /* step 1: stop endpoint */
> + /* skipped assuming that port suspend has done */
> +
> + /* step 2: clear Run/Stop bit */
> + command = xhci_readl(xhci, &xhci->op_regs->command);
> + command &= ~CMD_RUN;
> + xhci_writel(xhci, command, &xhci->op_regs->command);
> + if (handshake(xhci, &xhci->op_regs->status,
> + STS_HALT, STS_HALT, 100*100)) {
> + xhci_dbg(xhci, "WARN: xHC CMD_RUN timeout\n");
> + spin_unlock_irq(&xhci->lock);
> + return -ETIMEDOUT;
> + }
> +
> + /* step 3: save registers */
> + xhci->s3.command = xhci_readl(xhci, &xhci->op_regs->command);
> + xhci->s3.dev_nt = xhci_readl(xhci, &xhci->op_regs->dev_notification);
> + xhci->s3.dcbaa_ptr = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
> + xhci->s3.config_reg = xhci_readl(xhci, &xhci->op_regs->config_reg);
> + xhci->s3.irq_pending = xhci_readl(xhci, &xhci->ir_set->irq_pending);
> + xhci->s3.irq_control = xhci_readl(xhci, &xhci->ir_set->irq_control);
> + xhci->s3.erst_size = xhci_readl(xhci, &xhci->ir_set->erst_size);
> + xhci->s3.erst_base = xhci_read_64(xhci, &xhci->ir_set->erst_base);
> + xhci->s3.erst_dequeue = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
> +
> + /* step 4: set CSS flag */
> + command = xhci_readl(xhci, &xhci->op_regs->command);
> + command |= CMD_CSS;
> + xhci_writel(xhci, command, &xhci->op_regs->command);
> + udelay(1000);
Why is there this delay here? It's not required by the xHCI
specification. There is a major effort to make Linux suspend very fast,
and it's likely this will impede that effort. Why do you need this?
> + if (handshake(xhci, &xhci->op_regs->status, STS_SAVE, 0, 10*100)) {
> + xhci_dbg(xhci, "WARN: xHC CMD_CSS timeout\n");
> + spin_unlock_irq(&xhci->lock);
> + return -ETIMEDOUT;
> + }
> + /* step 5: remove core well power */
> + spin_unlock_irq(&xhci->lock);
> +bail:
> + return rc;
> +}
> +
> +/*
> + * start xHC (not bus-specific)
> + *
> + * This is called when the machine transition from S3/S4 mode.
> + *
> + */
> +int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> +{
> + u32 command, temp = 0;
> + struct usb_hcd *hcd = xhci_to_hcd(xhci);
> + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> + u64 val_64;
> + int old_state, retval;
> + unsigned int val, val2;
> + struct xhci_segment *seg;
> + int i;
> +
> + old_state = hcd->state;
> + if (time_before(jiffies, xhci->next_statechange))
> + msleep(100);
> +
> + spin_lock_irq(&xhci->lock);
> +
> + if (!hibernated) {
> + /* step 1: restore register */
> + xhci_writel(xhci, xhci->s3.command, &xhci->op_regs->command);
> + xhci_writel(xhci, xhci->s3.dev_nt,
> + &xhci->op_regs->dev_notification);
> + xhci_write_64(xhci, xhci->s3.dcbaa_ptr,
> + &xhci->op_regs->dcbaa_ptr);
> + xhci_writel(xhci, xhci->s3.config_reg,
> + &xhci->op_regs->config_reg);
> + xhci_writel(xhci, xhci->s3.irq_pending,
> + &xhci->ir_set->irq_pending);
> + xhci_writel(xhci, xhci->s3.irq_control,
> + &xhci->ir_set->irq_control);
> + xhci_writel(xhci, xhci->s3.erst_size, &xhci->ir_set->erst_size);
> + xhci_write_64(xhci, xhci->s3.erst_base,
> + &xhci->ir_set->erst_base);
> + xhci_set_hc_event_deq(xhci);
> + /* step 2: initialize command ring buffer */
> + val_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
> + val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) |
> + (xhci->cmd_ring->first_seg->dma &
> + (u64) ~CMD_RING_RSVD_BITS) |
> + xhci->cmd_ring->cycle_state;
Um, what if you've enqueued some commands before you suspend the system?
If you set the command ring dequeue pointer to the first TRB in the
command ring, those commands will get replayed. Instead, you need to
set the command ring dequeue pointer to xhci->cmd_ring->dequeue. You
can get the DMA pointer from the virtual pointer with
xhci_trb_virt_to_dma().
Alternatively, you could mem_set() the command ring segments and move
xhci->cmd_ring->enqueue and xhci->cmd_ring->dequeue. But the xHCI
internal command ring pointers and the hardware pointers have to match.
> + xhci_dbg(xhci, "// Setting command ring address to 0x%llx\n",
> + (long unsigned long) val_64);
> + xhci_write_64(xhci, val_64, &xhci->op_regs->cmd_ring);
> + /* step 3: restore state and start state*/
> + /* step 3: set CRS flag */
> + command = xhci_readl(xhci, &xhci->op_regs->command);
> + command |= CMD_CRS;
> + xhci_writel(xhci, command, &xhci->op_regs->command);
> + udelay(1000);
Again, why the delay here?
> + if (handshake(xhci, &xhci->op_regs->status,
> + STS_RESTORE, 0, 10*100)) {
> + xhci_dbg(xhci, "WARN: xHC CMD_CSS timeout\n");
> + spin_unlock_irq(&xhci->lock);
> + return -ETIMEDOUT;
> + }
> + temp = xhci_readl(xhci, &xhci->op_regs->status);
> + }
> +
> + /* If restore operation fails, reset the HC during resume */
> + if ((temp & STS_SRE) || hibernated) {
> + usb_root_hub_lost_power(hcd->self.root_hub);
> +
> + retval = xhci_halt(xhci);
> + if (retval) {
> + spin_unlock_irq(&xhci->lock);
> + return retval;
> + }
> +
> + xhci_dbg(xhci, "Resetting HCD\n");
> + /* Reset the internal HC memory state and registers. */
> + retval = xhci_reset(xhci);
> + if (retval) {
> + spin_unlock_irq(&xhci->lock);
> + return retval;
> + }
> +
> + xhci_dbg(xhci, "Reset complete\n");
> +
> + /*
> + * Initialize HCD and host controller data structures.
> + */
Why not just call xhci_mem_cleanup() and then xhci_mem_init() here? Or
break out the shared code between xhci_mem_init() and this function
instead of copying and pasting it?
> + val = HCS_MAX_SLOTS(xhci_readl(xhci,
> + &xhci->cap_regs->hcs_params1));
> + val2 = xhci_readl(xhci, &xhci->op_regs->config_reg);
> + val |= (val2 & ~HCS_SLOTS_MASK);
> + xhci_writel(xhci, val, &xhci->op_regs->config_reg);
> +
> + xhci_write_64(xhci, xhci->dcbaa->dma,
> + &xhci->op_regs->dcbaa_ptr);
> +
> + /*
> + * Set the address in the Command Ring Control register
> + */
> + val_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
> + val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) |
> + (xhci->cmd_ring->first_seg->dma &
> + (u64) ~CMD_RING_RSVD_BITS) |
> + xhci->cmd_ring->cycle_state;
> + xhci_dbg(xhci,
> + "// Setting command ring address to 0x%x\n", val);
> + xhci_write_64(xhci, val_64, &xhci->op_regs->cmd_ring);
> +
> + xhci_dbg(xhci, "// Allocating event ring\n");
> + xhci_ring_free(xhci, xhci->event_ring);
> + xhci->event_ring = NULL;
> + xhci->event_ring = xhci_ring_alloc(xhci, ERST_NUM_SEGS,
> + false, GFP_KERNEL);
> + if (!xhci->event_ring)
> + goto fail;
Why are you reallocating the event ring? Why not just zero it and reset
the internal event ring pointers?
> +
> + memset(xhci->erst.entries, 0,
> + sizeof(struct xhci_erst_entry)*ERST_NUM_SEGS);
> +
> + for (val = 0, seg = xhci->event_ring->first_seg;
> + val < ERST_NUM_SEGS; val++) {
> + struct xhci_erst_entry *entry =
> + &xhci->erst.entries[val];
> + entry->seg_addr = seg->dma;
> + entry->seg_size = TRBS_PER_SEGMENT;
> + entry->rsvd = 0;
> + seg = seg->next;
> + }
> +
> + val = xhci_readl(xhci, &xhci->ir_set->erst_size);
> + val &= ERST_SIZE_MASK;
> + val |= ERST_NUM_SEGS;
> + xhci_writel(xhci, val, &xhci->ir_set->erst_size);
> +
> + xhci_dbg(xhci,
> + "// Set ERST entries to point to event ring.\n");
> + /* set the segment table base address */
> + xhci_dbg(xhci,
> + "// Set ERST base address for ir_set 0 = 0x%llx\n",
> + (unsigned long long)xhci->erst.erst_dma_addr);
> + val_64 = xhci_read_64(xhci, &xhci->ir_set->erst_base);
> + val_64 &= ERST_PTR_MASK;
> + val_64 |= (xhci->erst.erst_dma_addr &
> + (u64) ~ERST_PTR_MASK);
> + xhci_write_64(xhci, val_64, &xhci->ir_set->erst_base);
> +
> + /* Set the event ring dequeue address */
> + xhci_set_hc_event_deq(xhci);
> + xhci_dbg(xhci, "Wrote ERST address to ir_set 0.\n");
> +
> + for (i = 0; i < MAX_HC_SLOTS; ++i) {
> + xhci_free_virt_device(xhci, i);
> + xhci->resume_done[i] = 0;
> + }
> +
> + goto out;
> +fail:
> + xhci_warn(xhci, "Couldn't initialize memory\n");
> + spin_unlock_irq(&xhci->lock);
> + xhci_mem_cleanup(xhci);
> + return -ENOMEM;
> +out:
These goto statements are a little bit wacky. Can you rewrite the code
without them? The label fail is only used if the event ring didn't get
reallocated, so move it into that if statement instead. Then the code
will be cleaner looking.
> +
> + if (!pci_set_mwi(pdev))
> + xhci_dbg(xhci, "MWI active\n");
> +
> + hcd->uses_new_polling = 1;
> + hcd->poll_rh = 0;
> +
> + temp = xhci_readl(xhci, &xhci->ir_set->irq_control);
> + temp &= ~ER_IRQ_INTERVAL_MASK;
> + temp |= (u32) 160;
> + xhci_writel(xhci, temp, &xhci->ir_set->irq_control);
> +
> + temp = xhci_readl(xhci, &xhci->op_regs->command);
> + temp |= (CMD_EIE);
> + xhci_writel(xhci, temp, &xhci->op_regs->command);
> +
> + temp = xhci_readl(xhci, &xhci->ir_set->irq_pending);
> + xhci_writel(xhci, ER_IRQ_ENABLE(temp),
> + &xhci->ir_set->irq_pending);
> + }
> +
> + /* step 4: set Run/Stop bit */
> + command = xhci_readl(xhci, &xhci->op_regs->command);
> + command |= CMD_RUN;
> + xhci_writel(xhci, command, &xhci->op_regs->command);
> + handshake(xhci, &xhci->op_regs->status, STS_HALT,
> + 0, 250 * 1000);
> +
> + /* step 5: walk topology and initialize portsc,
> + * portpmsc and portli
> + */
> + /* this is done in bus_resume */
> +
> + /* step 6: restart each of the previously
> + * Running endpoints by ringing their doorbells
> + */
> +
> + set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> + if (!hibernated)
> + hcd->state = old_state;
> + else
> + hcd->state = HC_STATE_SUSPENDED;
> +
> + spin_unlock_irq(&xhci->lock);
> + return 0;
> +}
> +
> /*-------------------------------------------------------------------------*/
>
> /**
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index a840706..b6f625c 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -149,7 +149,7 @@ static void xhci_initialize_ring_info(struct xhci_ring *ring)
> * Set the end flag and the cycle toggle bit on the last segment.
> * See section 4.9.1 and figures 15 and 16.
> */
> -static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
> +struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
> unsigned int num_segs, bool link_trbs, gfp_t flags)
> {
> struct xhci_ring *ring;
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index b071d9b..f7037d1 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -31,7 +31,7 @@
> static const char hcd_name[] = "xhci_hcd";
>
> /* called after powerup, by probe or system-pm "wakeup" */
> -static int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev)
> +int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev)
> {
> /*
> * TODO: Implement finding debug ports later.
> @@ -103,6 +103,39 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
> return xhci_pci_reinit(xhci, pdev);
> }
>
> +#ifdef CONFIG_PM
> +static int xhci_pci_suspend(struct usb_hcd *hcd)
> +{
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + int rc = 0;
> +
> + if (hcd->state != HC_STATE_SUSPENDED) {
> + rc = -EINVAL;
> + goto bail;
> + }
> +
> + xhci_dbg(xhci, "TRACE: call xhci_suspend\n");
> + rc = xhci_suspend(xhci);
> + xhci_dbg(xhci, "TRACE: exit xhci_suspend\n");
> +
> +bail:
> +
> + xhci_dbg(xhci, "TRACE: xhci_pci_suspend: EXIT, rc = %d\n", rc);
Is there a reason these debug statements have TRACE at the beginning?
> + return rc;
> +}
> +
> +static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
> +{
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + int retval = 0;
> +
> + xhci_dbg(xhci, "TRACE: enter xhci_pci_resume\n");
> +
> + retval = xhci_resume(xhci, hibernated);
> + return retval;
> +}
> +#endif /* CONFIG_PM */
> +
> static const struct hc_driver xhci_pci_hc_driver = {
> .description = hcd_name,
> .product_desc = "xHCI Host Controller",
> @@ -119,7 +152,10 @@ static const struct hc_driver xhci_pci_hc_driver = {
> */
> .reset = xhci_pci_setup,
> .start = xhci_run,
> - /* suspend and resume implemented later */
> +#ifdef CONFIG_PM
> + .pci_suspend = xhci_pci_suspend,
> + .pci_resume = xhci_pci_resume,
> +#endif
> .stop = xhci_stop,
> .shutdown = xhci_shutdown,
>
> @@ -173,6 +209,11 @@ static struct pci_driver xhci_pci_driver = {
> /* suspend and resume implemented later */
>
> .shutdown = usb_hcd_pci_shutdown,
> +#ifdef CONFIG_PM_SLEEP
> + .driver = {
> + .pm = &usb_hcd_pci_pm_ops
> + },
> +#endif
> };
>
> int xhci_register_pci()
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index fd092ca..4a7e92b 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -31,6 +31,7 @@
> /* Code sharing between pci-quirks and xhci hcd */
> #include "xhci-ext-caps.h"
>
> +#define HCD_FLAG_HW_ACCESSIBLE 0x00000001
> /* xHCI PCI Configuration Registers */
> #define XHCI_SBRN_OFFSET (0x60)
>
> @@ -191,7 +192,7 @@ struct xhci_op_regs {
> /* bits 4:6 are reserved (and should be preserved on writes). */
> /* light reset (port status stays unchanged) - reset completed when this is 0 */
> #define CMD_LRESET (1 << 7)
> -/* FIXME: ignoring host controller save/restore state for now. */
> +/* host controller save/restore state. */
> #define CMD_CSS (1 << 8)
> #define CMD_CRS (1 << 9)
> /* Enable Wrap Event - '1' means xHC generates an event when MFINDEX wraps. */
> @@ -1037,6 +1038,17 @@ struct xhci_scratchpad {
> #define XHCI_STOP_EP_CMD_TIMEOUT 5
> /* XXX: Make these module parameters */
>
> +struct s3_save {
> + u32 command;
> + u32 dev_nt;
> + u64 dcbaa_ptr;
> + u32 config_reg;
> + u32 irq_pending;
> + u32 irq_control;
> + u32 erst_size;
> + u64 erst_base;
> + u64 erst_dequeue;
> +};
>
> /* There is one ehci_hci structure per controller */
> struct xhci_hcd {
> @@ -1103,6 +1115,9 @@ struct xhci_hcd {
> unsigned long next_statechange;
>
> u32 command;
> + u32 irq_control;
> + int pci_suspend;
> + struct s3_save s3;
> /* Host controller is dying - not responding to commands. "I'm not dead yet!"
> *
> * xHC interrupts have been disabled and a watchdog timer will (or has already)
> @@ -1123,6 +1138,9 @@ struct xhci_hcd {
> unsigned int quirks;
> #define XHCI_LINK_TRB_QUIRK (1 << 0)
> #define XHCI_RESET_EP_QUIRK (1 << 1)
> + /* power management */
> + struct xhci_op_regs pm_op_regs;
> + struct xhci_run_regs pm_run_regs;
> unsigned long port_c_suspend; /* port suspend change*/
> unsigned long suspended_ports; /* which ports are
> suspended */
> @@ -1233,6 +1251,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci);
> int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
> void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id);
> int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id, struct usb_device *udev, gfp_t flags);
> +struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
> + unsigned int num_segs, bool link_trbs, gfp_t flags);
> int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev);
> unsigned int xhci_get_endpoint_index(struct usb_endpoint_descriptor *desc);
> unsigned int xhci_get_endpoint_flag(struct usb_endpoint_descriptor *desc);
> @@ -1263,6 +1283,7 @@ void xhci_free_command(struct xhci_hcd *xhci,
> /* xHCI PCI glue */
> int xhci_register_pci(void);
> void xhci_unregister_pci(void);
> +int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev);
> #endif
>
> /* xHCI host controller glue */
> @@ -1274,6 +1295,11 @@ void xhci_work(struct xhci_hcd *xhci);
> int xhci_run(struct usb_hcd *hcd);
> void xhci_stop(struct usb_hcd *hcd);
> void xhci_shutdown(struct usb_hcd *hcd);
> +int xhci_suspend(struct xhci_hcd *xhci);
> +int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
> +void xhci_work(struct xhci_hcd *xhci);
> +int handshake(struct xhci_hcd *xhci, void __iomem *ptr,
> + u32 mask, u32 done, int usec);
> int xhci_get_frame(struct usb_hcd *hcd);
> irqreturn_t xhci_irq(struct usb_hcd *hcd);
> int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev);
> --
> 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
