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

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 11:05 AM, Stephen Warren <swarren at 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
>