On Wed, 2010-03-03 at 13:50 -0800, Sarah Sharp wrote:
> 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.
> >
> > +
> > + /* 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?
Just make sure host has saved the internal state. I will remove it.
> > + 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 y);
> > + return -ETIMEDOUT;
> > + }
> > + /* 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().
OK, I will set the it to xhci->cmd_ring->dequeue.
> 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?
OK, I will remove it.
>
> > + 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?
There are some codes in xhci_mem_init() not necessary. So to speed up
the reset, I just copy out some of the xhci_mem_init() code.
>
> > + 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?
At the beginning when I wrote the code, I did not zero the event ring. I
just set the internal event ring pointer to the next address. It fails.
So I reallocated the event ring later and it works. It is a good
suggestion to zero the ring. I will try later.
>
> > +
> > + 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.
OK, I will change code.
> > +
> > + if (!pci_set_mwi(pdev))
> > + xhci_dbg(xhci, "MWI active\n");
> > + 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?
I will remove it.
> > + return rc;
> > +}
> > +
--
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
