| Message ID | 1322106896-23054-10-git-send-email-sjg@chromium.org |
|---|---|
| State | New, archived |
| Headers | show |
On 11/23/2011 08:54 PM, Simon Glass wrote: > CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of data for > USB packets (e.g. 4 means word-aligned). This is required for Tegra > to operate. > > CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning > field in the EHCI controller on reset. Shouldn't that be two separate patches? > diff --git a/README b/README ... > + CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of > + data for USB packets (e.g. 4 means word-aligned). This is > + required for Tegra to operate. > + > + CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the > + txfilltuning field in the EHCI controller on reset. > + > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c ... > @@ -322,6 +329,27 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, > int timeout; > int ret = 0; > > +#ifdef CONFIG_USB_EHCI_DATA_ALIGN > + /* In case ehci host requires alignment for buffers */ > + void *align_buf = NULL; > + void *orig_buf = buffer; > + int unaligned = ((int)buffer & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) != 0; This uses "!= 0". > + > + if (unaligned) { > + align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN); > + if (!align_buf) > + return -1; > + if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) This doesn't use "!= 0". Probably drop the "!= 0" above? > + buffer = (void *)((int)align_buf + > + CONFIG_USB_EHCI_DATA_ALIGN - > + ((int)align_buf & > + (CONFIG_USB_EHCI_DATA_ALIGN - 1))); > + else > + buffer = align_buf; Why not jsut do the following unconditionally; it seems much simpler even if very marginally less efficient: buffer = (align_buf + CONFIG_USB_EHCI_DATA_ALIGN - 1) & ~(CONFIG_USB_EHCI_DATA_ALIGN - 1); IIRC, in the kernel, we had to use this technique because arbitrary callers could have allocated "buffer" however they pleased. I assume the same is true in U-Boot; there isn't some way that this alignment restriction could be implemented in some core USB buffer allocation routine instead, and thus avoid all the copying? Do you know how often unaligned buffers actually occur in practice, and hence how much of a performance impact the copying will have?
Hi Stephen, On Mon, Nov 28, 2011 at 11:05 AM, Stephen Warren <swarren@nvidia.com> wrote: > On 11/23/2011 08:54 PM, Simon Glass wrote: >> CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of data for >> USB packets (e.g. 4 means word-aligned). This is required for Tegra >> to operate. >> >> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning >> field in the EHCI controller on reset. Sorry I am getting very little time for this this week. Hope to update the series tomorrow. > > Shouldn't that be two separate patches? Yes, will split. > >> diff --git a/README b/README > ... >> + CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of >> + data for USB packets (e.g. 4 means word-aligned). This is >> + required for Tegra to operate. >> + >> + CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the >> + txfilltuning field in the EHCI controller on reset. >> + > >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > ... >> @@ -322,6 +329,27 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, >> int timeout; >> int ret = 0; >> >> +#ifdef CONFIG_USB_EHCI_DATA_ALIGN >> + /* In case ehci host requires alignment for buffers */ >> + void *align_buf = NULL; >> + void *orig_buf = buffer; >> + int unaligned = ((int)buffer & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) != 0; > > This uses "!= 0". > >> + >> + if (unaligned) { >> + align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN); >> + if (!align_buf) >> + return -1; >> + if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) > > This doesn't use "!= 0". Probably drop the "!= 0" above? Dropped. > >> + buffer = (void *)((int)align_buf + >> + CONFIG_USB_EHCI_DATA_ALIGN - >> + ((int)align_buf & >> + (CONFIG_USB_EHCI_DATA_ALIGN - 1))); >> + else >> + buffer = align_buf; > > Why not jsut do the following unconditionally; it seems much simpler > even if very marginally less efficient: > > buffer = (align_buf + CONFIG_USB_EHCI_DATA_ALIGN - 1) & > ~(CONFIG_USB_EHCI_DATA_ALIGN - 1); OK done (adding casts back in). > > IIRC, in the kernel, we had to use this technique because arbitrary > callers could have allocated "buffer" however they pleased. I assume the > same is true in U-Boot; there isn't some way that this alignment > restriction could be implemented in some core USB buffer allocation > routine instead, and thus avoid all the copying? Do you know how often > unaligned buffers actually occur in practice, and hence how much of a > performance impact the copying will have? No I don't but the same exercise has happened with MMC, so things will be better now than they were. We might be able to turn this into an assert perhaps in the future. Regards, Simon > > -- > nvpublic >
diff --git a/README b/README index 07f1d11..d3289d2 100644 --- a/README +++ b/README @@ -1096,6 +1096,13 @@ The following options need to be configured: May be defined to allow interrupt polling instead of using asynchronous interrupts + CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of + data for USB packets (e.g. 4 means word-aligned). This is + required for Tegra to operate. + + CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the + txfilltuning field in the EHCI controller on reset. + - USB Device: Define the below if you wish to use the USB console. Once firmware is rebuilt from a serial console issue the diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 2197119..d3eeefe 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -247,6 +247,13 @@ static int ehci_reset(void) #endif ehci_writel(reg_ptr, tmp); } + +#ifdef CONFIG_USB_EHCI_TXFIFO_THRESH + cmd = ehci_readl(&hcor->or_txfilltuning); + cmd &= ~TXFIFO_THRESH(0x3f); + cmd |= TXFIFO_THRESH(CONFIG_USB_EHCI_TXFIFO_THRESH); + ehci_writel(&hcor->or_txfilltuning, cmd); +#endif out: return ret; } @@ -322,6 +329,27 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int timeout; int ret = 0; +#ifdef CONFIG_USB_EHCI_DATA_ALIGN + /* In case ehci host requires alignment for buffers */ + void *align_buf = NULL; + void *orig_buf = buffer; + int unaligned = ((int)buffer & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) != 0; + + if (unaligned) { + align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN); + if (!align_buf) + return -1; + if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) + buffer = (void *)((int)align_buf + + CONFIG_USB_EHCI_DATA_ALIGN - + ((int)align_buf & + (CONFIG_USB_EHCI_DATA_ALIGN - 1))); + else + buffer = align_buf; + if (usb_pipeout(pipe)) + memcpy(buffer, orig_buf, length); + } +#endif debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", dev, pipe, buffer, length, req); if (req != NULL) @@ -513,9 +541,20 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, ehci_readl(&hcor->or_portsc[1])); } +#ifdef CONFIG_USB_EHCI_DATA_ALIGN + if (unaligned) { + if (usb_pipein(pipe) && dev->act_len) + memcpy(orig_buf, buffer, length); + free(align_buf); + } +#endif return (dev->status != USB_ST_NOT_PROC) ? 0 : -1; fail: +#ifdef CONFIG_USB_EHCI_DATA_ALIGN + if (unaligned) + free(align_buf); +#endif td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next); while (td != (void *)QT_NEXT_TERMINATE) { qh->qh_overlay.qt_next = td->qt_next; diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index 3d0ad0c..cc00ce4 100644 --- a/drivers/usb/host/ehci.h +++ b/drivers/usb/host/ehci.h @@ -80,7 +80,11 @@ struct ehci_hcor { uint32_t or_ctrldssegment; uint32_t or_periodiclistbase; uint32_t or_asynclistaddr; - uint32_t _reserved_[9]; + uint32_t _reserved_0_; + uint32_t or_burstsize; + uint32_t or_txfilltuning; +#define TXFIFO_THRESH(p) ((p & 0x3f) << 16) + uint32_t _reserved_1_[6]; uint32_t or_configflag; #define FLAG_CF (1 << 0) /* true: we'll support "high speed" */ uint32_t or_portsc[CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS];
CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of data for USB packets (e.g. 4 means word-aligned). This is required for Tegra to operate. CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning field in the EHCI controller on reset. Signed-off-by: Simon Glass <sjg@chromium.org> --- README | 7 +++++++ drivers/usb/host/ehci-hcd.c | 39 +++++++++++++++++++++++++++++++++++++++ drivers/usb/host/ehci.h | 6 +++++- 3 files changed, 51 insertions(+), 1 deletions(-)