Hi David,
I've been thinking about this some more, and I'd like to propose a much
simpler solution.
The TD fragment rules didn't go into the xHCI specification until the
1.0 revision. The code that follows those rules only seems to trigger
issues for 0.96 ASMedia hosts, while Intel 1.0 hosts benefit from the
code when USB ethernet devices are attached. The patch that changed the
link TRB behavior (commit 6c12db90f19727c76990e7f4801c67a148b30111) was
submitted in 2010, and the xHCI 1.0 spec didn't come out until later
that year, so it's unlikely that Synopsys host is a 1.0 host either.
So why not just limit your code (and the sg table entry limit) to only
trigger for 1.0 hosts? That fix would be much less complex than this
one. I would still consider the other cleanup patches on to of it for
inclusion in 3.15, although it looks like patches 4 and 5 rely on this
patch.
Walt, can you turn on xHCI debugging and look for whether the NEC host
that worked with David's patch is a 1.0 host? You'll see a line like:
// @%p = 0x%x (CAPLENGTH AND HCIVERSION)
That should allow me to decode the host version number.
Sarah Sharp
On Tue, Jan 21, 2014 at 05:12:54PM +0000, David Laight wrote:
> Some xhci controllers (eg ASMedia) don't like processing a LINK TRB and
> then finding that they can't process the next TRB.
>
> Instead of flipping the cycle bit on the first data TRB, remember the
> real first TRB in prepare_ring() and flip that in giveback_first_trb().
>
> In queue_trb() ignore the cycle bit value set by xhci_queue_bulk_tx() etc,
> and changes the ownership of all but the first TRB.
>
> If prepare_ring() adds any NOP TRB, the ownership of all but the first
> and the LINK are changed. The wmb() is no longer needed before changing
> the ownership of a LINK trb since is is preceeded by a NOP.
>
> Since the 'first trb' might be a LINK trb queue_command() must also now
> call giveback_first_trb(). Don't ring the doorbell here though.
>
> The isoc code calls prepare_ring() right at the start, this ensures that
> there is enough space for all the TRB and advances the write-ptr past
> any LINK TRB. It then calls prepare_transfer() multiple times.
> However the prepare_ring() calls must now be matched with those to
> giveback_first_trb().
> Don't call prepare_ring() from prepare_transfer() for isoc rings.
> Set 'more_trbs_coming' (it means 'advance past a LINK TRB) on all
> the queue_trb() calls except the very last one.
>
> Signed-off-by: David Laight <david.laight@xxxxxxxxxx>
> ---
> Note that this differs to any previous similar patches I've sent
> because it contains a fix to the isoc code.
> However I've only tsted it with USB3 ethernet.
>
> drivers/usb/host/xhci-ring.c | 84 +++++++++++++++++++++++++++++---------------
> drivers/usb/host/xhci.h | 1 +
> 2 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index a0b248c..62049e5 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2920,6 +2920,16 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> struct xhci_generic_trb *trb;
>
> trb = &ring->enqueue->generic;
> +
> + /*
> + * Ignore the caller's CYCLE bit.
> + * The caller doesn't know whether the real first TRB is
> + * actually a LINK (or NOP) TRB.
> + */
> + field4 = (field4 & ~TRB_CYCLE) | ring->cycle_state;
> + if (trb == &ring->enqueue_first->generic)
> + field4 ^= TRB_CYCLE;
> +
> trb->field[0] = cpu_to_le32(field1);
> trb->field[1] = cpu_to_le32(field2);
> trb->field[2] = cpu_to_le32(field3);
> @@ -2964,6 +2974,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
> return -EINVAL;
> }
>
> + /* Save entry whose cycle bit needs flipping at the end */
> + ep_ring->enqueue_first = ep_ring->enqueue;
> while (1) {
> if (room_on_ring(xhci, ep_ring, num_trbs)) {
> union xhci_trb *trb = ep_ring->enqueue;
> @@ -3006,13 +3018,18 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
> nop_cmd = cpu_to_le32(TRB_TYPE(TRB_TR_NOOP) |
> ep_ring->cycle_state);
> ep_ring->num_trbs_free -= usable;
> - do {
> + /* Leave TRB_CYCLE unchanged on first NOP */
> + trb->generic.field[3] = nop_cmd ^
> + cpu_to_le32(TRB_CYCLE);
> + for (;;) {
> trb->generic.field[0] = 0;
> trb->generic.field[1] = 0;
> trb->generic.field[2] = 0;
> - trb->generic.field[3] = nop_cmd;
> trb++;
> - } while (--usable);
> + if (!--usable)
> + break;
> + trb->generic.field[3] = nop_cmd;
> + }
> ep_ring->enqueue = trb;
> if (room_on_ring(xhci, ep_ring, num_trbs))
> break;
> @@ -3050,8 +3067,9 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
> else
> next->link.control |= cpu_to_le32(TRB_CHAIN);
>
> - wmb();
> - next->link.control ^= cpu_to_le32(TRB_CYCLE);
> + /* If LINK follows a NOP, invert cycle */
> + if (next != ep_ring->enqueue_first)
> + next->link.control ^= cpu_to_le32(TRB_CYCLE);
>
> /* Toggle the cycle bit after the last ring segment. */
> if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) {
> @@ -3088,11 +3106,14 @@ static int prepare_transfer(struct xhci_hcd *xhci,
> return -EINVAL;
> }
>
> - ret = prepare_ring(xhci, ep_ring,
> - le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
> - num_trbs, mem_flags);
> - if (ret)
> - return ret;
> + /* xhci_queue_isoc_tx_prepare() called prepare ring earlier. */
> + if (ep_ring->type != TYPE_ISOC) {
> + ret = prepare_ring(xhci, ep_ring,
> + le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
> + num_trbs, mem_flags);
> + if (ret)
> + return ret;
> + }
>
> urb_priv = urb->hcpriv;
> td = urb_priv->td[td_index];
> @@ -3167,19 +3188,24 @@ static void check_trb_math(struct urb *urb, int num_trbs, int running_total)
> }
>
> static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
> - unsigned int ep_index, unsigned int stream_id, int start_cycle,
> - struct xhci_generic_trb *start_trb)
> + unsigned int ep_index, struct xhci_ring *ring)
> {
> /*
> * Pass all the TRBs to the hardware at once and make sure this write
> * isn't reordered.
> */
> wmb();
> - if (start_cycle)
> - start_trb->field[3] |= cpu_to_le32(start_cycle);
> - else
> - start_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
> - xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
> + ring->enqueue_first->generic.field[3] ^= cpu_to_le32(TRB_CYCLE);
> +
> + if (ring->type == TYPE_COMMAND)
> + return;
> +
> + /*
> + * Make sure the hardware doesn't read the ring before the write
> + * above completes.
> + */
> + wmb();
> + xhci_ring_ep_doorbell(xhci, slot_id, ep_index, ring->stream_id);
> }
>
> /*
> @@ -3420,8 +3446,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> } while (running_total < urb->transfer_buffer_length);
>
> check_trb_math(urb, num_trbs, running_total);
> - giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
> - start_cycle, start_trb);
> + giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
> return 0;
> }
>
> @@ -3559,8 +3584,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> } while (running_total < urb->transfer_buffer_length);
>
> check_trb_math(urb, num_trbs, running_total);
> - giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
> - start_cycle, start_trb);
> + giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
> return 0;
> }
>
> @@ -3676,8 +3700,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> /* Event on completion */
> field | TRB_IOC | TRB_TYPE(TRB_STATUS) | ep_ring->cycle_state);
>
> - giveback_first_trb(xhci, slot_id, ep_index, 0,
> - start_cycle, start_trb);
> + giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
> return 0;
> }
>
> @@ -3770,7 +3793,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> int running_total, trb_buff_len, td_len, td_remain_len, ret;
> u64 start_addr, addr;
> int i, j;
> - bool more_trbs_coming;
> + bool more_trbs_coming = true;
>
> ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
>
> @@ -3851,7 +3874,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> */
> if (j < trbs_per_td - 1) {
> field |= TRB_CHAIN;
> - more_trbs_coming = true;
> } else {
> td->last_trb = ep_ring->enqueue;
> field |= TRB_IOC;
> @@ -3862,7 +3884,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> if (i < num_tds - 1)
> field |= TRB_BEI;
> }
> - more_trbs_coming = false;
> + /*
> + * We want to advance past LINK TRBs until the
> + * very last TRB.
> + */
> + if (i == num_tds - 1)
> + more_trbs_coming = false;
> }
>
> /* Calculate TRB length */
> @@ -3910,8 +3937,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> }
> xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs++;
>
> - giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
> - start_cycle, start_trb);
> + giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
> return 0;
> cleanup:
> /* Clean up a partially enqueued isoc transfer. */
> @@ -4036,6 +4062,8 @@ static int queue_command(struct xhci_hcd *xhci, u32 field1, u32 field2,
> }
> queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
> field4 | xhci->cmd_ring->cycle_state);
> + /* The 'first' TRB might be a LINK TRB... */
> + giveback_first_trb(xhci, 0, 0, xhci->cmd_ring);
> return 0;
> }
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index f8416639..6973c56 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1334,6 +1334,7 @@ struct xhci_ring {
> struct xhci_segment *first_seg;
> struct xhci_segment *last_seg;
> union xhci_trb *enqueue;
> + union xhci_trb *enqueue_first;
> struct xhci_segment *enq_seg;
> unsigned int enq_updates;
> union xhci_trb *dequeue;
> --
> 1.8.1.2
>
>
>
--
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
