Patchwork [U-Boot,09/14] tegra: usb: Add support for data alignment and txfifo threshold

login
register
mail settings
Submitter Simon Glass
Date Nov. 24, 2011, 3:54 a.m.
Message ID <1322106896-23054-10-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/127432/
State New, archived
Headers show

Comments

Simon Glass - Nov. 24, 2011, 3:54 a.m.
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(-)
Stephen Warren - Nov. 28, 2011, 7:05 p.m.
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?
Simon Glass - Dec. 2, 2011, 1:42 a.m.
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
>

Patch

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];