On Sat, 1 May 2010, Matthew Wilcox wrote:
> > > The patch you did to EHCI (as1300 / commit 40f8db8f8f) does look quite
> > > straightforward to port to the uhci-q.c and ohci-q.c file. On the one
> > > hand, that's a good thing, because I can just do that (er, if I can figure
> > > out a way to test either of them).
> >
> > Testing is easy -- just see if your mass storage devices still work.
> > usb-storage calls usb_sg_*, which will use the HCD's native SG support.
>
> Easy ... if you have the hardware. I need to check my hubs to see if
> I have one that's USB 1.1 only, so I can force an EHCI controller into
> UHCI mode.
You don't need an extra hub. Just rmmod ehci-hcd.
> > > On the other hand, it's not taking
> > > advantage of EHCI's native ability to do scatter-gather in a single qtd if
> > > the pages are virtually contiguous (see pages 86-88 of the EHCI 1.0 spec).
> > >
> > > So EHCI could be further enhanced ... if it's worth it.
> >
> > Hmm, maybe... If it's not too hard to do. I'll have to look at the
> > code.
>
> I think the cleanest way to do it is to split qtd_fill into qtd_fill_buf
> and qtd_fill_sg; use the former for the usb_pipecontrol and one_more
> cases; and the latter in the main loop. qtd_fill_buf could be quite simple
> since it's never going to use multiple pages. qtd_fill_sg would be pretty
> complex though.
>
> I have no idea how much improvement we'd see from using multiple pages
> in a qtd instead of using one qtd per page though.
You mean multiple scatterlist elements per qtd -- we already can have
multiple pages per qtd if a scatterlist element is more than one page
long.
I don't know how much difference it would make either, probably not
anything measurable. But if you want to play around with it, here's a
totally untested patch. It changes qtd_fill fairly drastically (IMO
the overall result is a simplification), but nothing else needed much
work. (It also fixes a bug in the toggle computation for control
transfers; apparently that pathway has never been used.)
Alan Stern
Index: usb-2.6/drivers/usb/host/ehci-q.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-q.c
+++ usb-2.6/drivers/usb/host/ehci-q.c
@@ -44,40 +44,59 @@
static int
qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
- size_t len, int token, int maxpacket)
+ size_t len, int token, int maxpacket, int *index)
{
- int i, count;
- u64 addr = buf;
+ int i;
+ int count, n;
+ u64 addr;
+
+ n = buf & 0x0fff; /* start position in the first page */
+ i = *index;
+ *index = 0; /* assume the next transfer can't be merged */
+ if (i == 0) {
+ qtd->hw_token = cpu_to_hc32(ehci, token);
+ } else if (unlikely(n)) {
+ /* Can't merge buffers if they aren't "virtually contiguous":
+ * This one must start at a page boundary (and the previous
+ * one must have ended at a page boundary) (4.10.6).
+ */
+ return 0;
+ }
/* one buffer entry per 4K ... first might be short or unaligned */
- qtd->hw_buf[0] = cpu_to_hc32(ehci, (u32)addr);
- qtd->hw_buf_hi[0] = cpu_to_hc32(ehci, (u32)(addr >> 32));
- count = 0x1000 - (buf & 0x0fff); /* rest of that page */
- if (likely (len < count)) /* ... iff needed */
- count = len;
- else {
- buf += 0x1000;
- buf &= ~0x0fff;
+ n = 0x1000 - n; /* rest of the first page */
+ count = 0;
- /* per-qtd limit: from 16K to 20K (best alignment) */
- for (i = 1; count < len && i < 5; i++) {
- addr = buf;
- qtd->hw_buf[i] = cpu_to_hc32(ehci, (u32)addr);
- qtd->hw_buf_hi[i] = cpu_to_hc32(ehci,
- (u32)(addr >> 32));
- buf += 0x1000;
- if ((count + 0x1000) < len)
- count += 0x1000;
- else
- count = len;
- }
-
- /* short packets may only terminate transfers */
- if (count != len)
- count -= (count % maxpacket);
+ /* per-qtd limit: from 16K to 20K (best alignment) */
+ while (i < 5) {
+ addr = buf;
+ qtd->hw_buf[i] = cpu_to_hc32(ehci, (u32)addr);
+ qtd->hw_buf_hi[i] = cpu_to_hc32(ehci, (u32)(addr >> 32));
+ ++i;
+ buf += n;
+ count += n;
+ if (count >= len) {
+ /* If this transfer ended at a page boundary,
+ * the next transfer may also go in this qtd.
+ */
+ if (count == len && i < 5)
+ *index = i;
+ count = len;
+ break;
+ }
+ n = 0x1000;
}
- qtd->hw_token = cpu_to_hc32(ehci, (count << 16) | token);
- qtd->length = count;
+
+ /* Short packets may only terminate transfers.
+ * This matters only if the buffer is > 16KB and not aligned
+ * on a maxpacket boundary -- so it should never matter.
+ */
+ if (count != len)
+ count -= (count % maxpacket);
+
+ qtd->length += count;
+ qtd->hw_token &= ~cpu_to_hc32(ehci, (0x7fff << 16));
+ qtd->hw_token |= cpu_to_hc32(ehci, (qtd->length << 16));
return count;
}
@@ -619,6 +638,7 @@ qh_urb_transaction (
int len, this_sg_len, maxpacket;
int is_input;
u32 token;
+ int index;
int i;
struct scatterlist *sg;
@@ -630,6 +650,7 @@ qh_urb_transaction (
return NULL;
list_add_tail (&qtd->qtd_list, head);
qtd->urb = urb;
+ index = 0;
token = QTD_STS_ACTIVE;
token |= (EHCI_TUNE_CERR << 10);
@@ -641,7 +662,7 @@ qh_urb_transaction (
/* SETUP pid */
qtd_fill(ehci, qtd, urb->setup_dma,
sizeof (struct usb_ctrlrequest),
- token | (2 /* "setup" */ << 8), 8);
+ token | (2 /* "setup" */ << 8), 8, &index);
/* ... and always at least one more pid */
token ^= QTD_TOGGLE;
@@ -652,6 +673,7 @@ qh_urb_transaction (
qtd->urb = urb;
qtd_prev->hw_next = QTD_NEXT(ehci, qtd->qtd_dma);
list_add_tail (&qtd->qtd_list, head);
+ index = 0;
/* for zero length DATA stages, STATUS is always IN */
if (len == 0)
@@ -691,7 +713,7 @@ qh_urb_transaction (
int this_qtd_len;
this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
- maxpacket);
+ maxpacket, &index);
this_sg_len -= this_qtd_len;
len -= this_qtd_len;
buf += this_qtd_len;
@@ -705,7 +727,7 @@ qh_urb_transaction (
qtd->hw_alt_next = ehci->async->hw->hw_alt_next;
/* qh makes control packets use qtd toggle; maybe switch it */
- if ((maxpacket & (this_qtd_len + (maxpacket - 1))) == 0)
+ if (maxpacket & (this_qtd_len + (maxpacket - 1)))
token ^= QTD_TOGGLE;
if (likely(this_sg_len <= 0)) {
@@ -716,13 +738,15 @@ qh_urb_transaction (
this_sg_len = min_t(int, sg_dma_len(sg), len);
}
- qtd_prev = qtd;
- qtd = ehci_qtd_alloc (ehci, flags);
- if (unlikely (!qtd))
- goto cleanup;
- qtd->urb = urb;
- qtd_prev->hw_next = QTD_NEXT(ehci, qtd->qtd_dma);
- list_add_tail (&qtd->qtd_list, head);
+ if (!index) {
+ qtd_prev = qtd;
+ qtd = ehci_qtd_alloc(ehci, flags);
+ if (unlikely(!qtd))
+ goto cleanup;
+ qtd->urb = urb;
+ qtd_prev->hw_next = QTD_NEXT(ehci, qtd->qtd_dma);
+ list_add_tail(&qtd->qtd_list, head);
+ }
}
/*
@@ -758,9 +782,10 @@ qh_urb_transaction (
qtd->urb = urb;
qtd_prev->hw_next = QTD_NEXT(ehci, qtd->qtd_dma);
list_add_tail (&qtd->qtd_list, head);
+ index = 0;
/* never any data in such packets */
- qtd_fill(ehci, qtd, 0, 0, token, 0);
+ qtd_fill(ehci, qtd, 0, 0, token, 0, &index);
}
}
--
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
