[U-Boot] [PATCH v2 1/2] usb: align buffers at cacheline

Simon Glass
2012-02-27T17:03:15Z

Hi Marek,

On Mon, Feb 27, 2012 at 8:49 AM, Marek Vasut <marex@denx.de> wrote:
>> As DMA expects the buffers to be equal and larger then
>> cache lines, This aligns buffers at cacheline.
>>
>> Signed-off-by: Puneet Saxena <puneets@nvidia.com>
>> Signed-off-by: Jim Lin <jilin@nvidia.com>
>> ---
>>
>
> First of all, thanks for this patch, it really helps. But, there are a few
> things that concern me.
>
>> Changes for V2:
>> ? ? - Use "ARCH_DMA_MINALIGN" directly
>> ? ? - Use "ALIGN" to align size as cacheline
>> ? ? - Removed headers from usb.h
>> ? ? - Send 8 bytes of device descriptor size to read
>> ? ? ? Max packet size
>> scsi.h header is needed to avoid extra memcpy from local buffer
>> to global buffer.
>>
>> ?common/cmd_usb.c ? ? ? ? ? ?| ? ?3 +-
>> ?common/usb.c ? ? ? ? ? ? ? ?| ? 61
>> ++++++++++++++++++++++++------------------ common/usb_storage.c ? ? ? ?|
>> 59 +++++++++++++++++++---------------------- disk/part_dos.c ? ? ? ? ? ? |
>> ? ?2 +-
>> ?drivers/usb/host/ehci-hcd.c | ? ?8 +++++
>> ?include/scsi.h ? ? ? ? ? ? ?| ? ?4 ++-
>> ?6 files changed, 77 insertions(+), 60 deletions(-)
>>
>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
>> index 320667f..bca9d94 100644
>> --- a/common/cmd_usb.c
>> +++ b/common/cmd_usb.c
>> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass,
>> unsigned char subclass,
>>
>> ?void usb_display_string(struct usb_device *dev, int index)
>> ?{
>> - ? ? char buffer[256];
>> + ? ? ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256);
>
> Why not memalign(), do you want to allocate on stack so badly ?

This issue came up with MMC and there was a strong request not to
sprinkle the code with malloc. In fact the macro above was devised at
some cost just to solve this problem.

[snip]
>> ?{
>> ? ? ? int tries;
>> - ? ? struct usb_port_status portsts;
>> + ? ? ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>
> Looking at all this stuff, it's already allocated on stack. Ok, I already
> outlined first option -- memalign(), but that'd really put quite a strain on the
> memory allocator -- the other option (and maybe even better) is to use
> __attribute__((aligned)), which will allow the compiler to reorder data
> allocated on the stack so the array you create is aligned and not much space
> around on the stack is wasted. Apparently, ALLOC_CACHE_ALIGN_BUFFER() wastes a
> lot of space on the stack.

There was quite a bit of discussion about this related to MMC. I think
the current solution is the result of that discussion...

Regards,
Simon