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

[Stephen Warren <swarren@nvidia.com>]

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?

-- 
nvpublic