On Thu, May 06, 2010 at 11:11:06AM -0700, John Youn wrote:
> Change transfer ring behavior to not follow/activate link TRBs
> until active TRBs are queued after it. This change affects
> the behavior when a TD ends just before a link TRB.
Hi John,
Thanks for the patch! I've been working on the ring expansion patch, so
I can finally test it now.
However, I have two issues with this patch.
First, it seems like you're giving the link TRB to the hardware before the
driver writes the TD at the top of the ring. Won't you run into the
same issue you had with streams processing? Or will the host controller
not notice the link TRB until the driver rings the doorbell?
Second, I don't think you're handling hardware that has the link TRB
quirk correctly. For that 0.95 hardware, the xHCI driver has to make
sure never to clear the chain bit on the link TRB. (This is because one
part of the 0.95 spec said "the chain bit shall always be set", and then
the stop endpoint rules talked about the chain bit being cleared.)
Suggestions for handling the 0.95 hardware are inline:
> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
> ---
>
> Sarah, here is the patch finally. I was able to do some testing on
> NEC and our controller as well. Thanks.
>
> drivers/usb/host/xhci-ring.c | 73 ++++++++++++++++++++++++++++++++++-------
> 1 files changed, 60 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index b648efa..389fd9c 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -112,6 +112,12 @@ static inline int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> return (trb->link.control & TRB_TYPE_BITMASK) == TRB_TYPE(TRB_LINK);
> }
>
> +static inline int enqueue_is_link_trb(struct xhci_ring *ring)
> +{
> + struct xhci_link_trb *link = &ring->enqueue->link;
> + return ((link->control & TRB_TYPE_BITMASK) == TRB_TYPE(TRB_LINK));
> +}
> +
> /* Updates trb to point to the next TRB in the ring, and updates seg if the next
> * TRB is in a new segment. This does not skip over link TRBs, and it does not
> * effect the ring dequeue or enqueue pointers.
> @@ -193,20 +199,25 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer
> while (last_trb(xhci, ring, ring->enq_seg, next)) {
> if (!consumer) {
> if (ring != xhci->event_ring) {
> - /* If we're not dealing with 0.95 hardware,
> - * carry over the chain bit of the previous TRB
> - * (which may mean the chain bit is cleared).
> - */
> - if (!xhci_link_trb_quirk(xhci)) {
> - next->link.control &= ~TRB_CHAIN;
> - next->link.control |= chain;
> + if (chain) {
> + xhci_dbg(xhci, "link_trb with chain bit set (part of prev trb td)\n");
> + /* If we're not dealing with 0.95 hardware,
> + * carry over the chain bit of the previous TRB
> + * (which may mean the chain bit is cleared).
> + */
> + if (!xhci_link_trb_quirk(xhci)) {
> + next->link.control &= ~TRB_CHAIN;
> + next->link.control |= chain;
> + }
Don't check xhci_link_trb_quirk() here. You already know chain = 1.
xhci_link_trb_quirk() only needs to be checked if you're going to
possibly clear the chain bit.
> + /* Give this link TRB to the hardware */
> + wmb();
> + if (next->link.control & TRB_CYCLE)
> + next->link.control &= (u32) ~TRB_CYCLE;
> + else
> + next->link.control |= (u32) TRB_CYCLE;
> + } else {
> + break;
> }
> - /* Give this link TRB to the hardware */
> - wmb();
> - if (next->link.control & TRB_CYCLE)
> - next->link.control &= (u32) ~TRB_CYCLE;
> - else
> - next->link.control |= (u32) TRB_CYCLE;
> }
> /* Toggle the cycle bit after the last ring segment. */
> if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) {
> @@ -243,6 +254,13 @@ static int room_on_ring(struct xhci_hcd *xhci, struct xhci_ring *ring,
> union xhci_trb *enq = ring->enqueue;
> struct xhci_segment *enq_seg = ring->enq_seg;
>
> + /* If we are currently pointing to a link TRB, advance the
> + * enqueue pointer before checking for space */
> + while (last_trb(xhci, ring, enq_seg, enq)) {
> + enq_seg = enq_seg->next;
> + enq = enq_seg->trbs;
> + }
> +
> /* Check if ring is empty */
> if (enq == ring->dequeue)
> return 1;
> @@ -1703,6 +1721,35 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
> xhci_err(xhci, "ERROR no room on ep ring\n");
> return -ENOMEM;
> }
> +
> + if (enqueue_is_link_trb(ep_ring)) {
> + struct xhci_ring *ring = ep_ring;
> + union xhci_trb *next;
> + unsigned long long addr;
> +
> + xhci_dbg(xhci, "prepare_ring: pointing to link trb\n");
> + next = ring->enqueue;
> +
> + while (last_trb(xhci, ring, ring->enq_seg, next)) {
You need to check xhci_link_trb_quirk() before executing the next line,
since it clears the chain bit.
> + next->link.control &= ~TRB_CHAIN;
> + wmb();
> + next->link.control ^= (u32) TRB_CYCLE;
> +
> + /* Toggle the cycle bit after the last ring segment. */
> + if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) {
> + ring->cycle_state = (ring->cycle_state ? 0 : 1);
> + if (!in_interrupt()) {
> + xhci_dbg(xhci, "queue_trb: Toggle cycle "
> + "state for ring %p = %i\n",
> + ring, (unsigned int)ring->cycle_state);
> + }
> + }
> + ring->enq_seg = ring->enq_seg->next;
> + ring->enqueue = ring->enq_seg->trbs;
> + next = ring->enqueue;
> + }
> + }
> +
> return 0;
> }
>
> --
> 1.6.6.1
>
--
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
