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

[Simon Glass]

Hi Marek,

On Mon, Feb 27, 2012 at 9:11 AM, Marek Vasut <marex at denx.de> wrote:
>> Hi Marek,
>>
>> On Mon, Feb 27, 2012 at 8:49 AM, Marek Vasut <marex at 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 at nvidia.com>
>> >> Signed-off-by: Jim Lin <jilin at 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.
>
> This is really weird solution :-(

It's not pretty under the hood but it solves the problem well for MMC.
Apart from the alignment of buffers, it is as efficient as the
original code.

>>
>> [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...
>
> Can you please point me to such discussion?

Start here perhaps and look at the whole thread:

http://lists.denx.de/pipermail/u-boot/2011-August/099152.html

>
> Thanks!

Regards,
Simon