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

[puneets <puneets@nvidia.com>]

Hi,
On Friday 24 February 2012 06:12 PM, Simon Glass wrote:
> Hi,
>
> On Thu, Feb 23, 2012 at 6:25 AM, Puneet Saxena<puneets@nvidia.com>  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>
>> ---
>> Changes for v2:
>>    - Split the commit in to 2 commits
>>    - "ARCH_DMA_MINALIGN" replacement
>>    - Making stop address cache line aligned by
>>      making size as multiple of cache line
>>    - incorporated Marek and Mike's comment
>>
>>   common/cmd_usb.c            |    3 +-
>>   common/usb.c                |   53 ++++++++++++++++++----------------
>>   common/usb_storage.c        |   66 ++++++++++++++++++++++--------------------
>>   drivers/usb/host/ehci-hcd.c |    9 ++++++
>>   include/scsi.h              |    8 ++++-
>>   include/usb.h               |    8 ++++-
>>   6 files changed, 88 insertions(+), 59 deletions(-)
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index d893b2a..82652f5 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -120,6 +120,15 @@ static struct descriptor {
>>   */
>>   static void flush_invalidate(u32 addr, int size, int flush)
>>   {
>> +       /*
>> +        * Size is the bytes actually moved during transaction,
>> +        * which may not equal to the cache line. This results
>> +        * stop address passed for invalidating cache may not be aligned.
>> +        * Therfore making size as nultiple of cache line size.
>> +        */
>> +       if (size&  (ARCH_DMA_MINALIGN - 1))
>> +                       size = ((size / ARCH_DMA_MINALIGN) + 1)
>> +                               * ARCH_DMA_MINALIGN;
> Can we just use:
>
> size = ALIGN(size, ARCH_DMA_MINALIGN)
>
> here or does it increase size even if already aligned?
size = ALIGN(size, ARCH_DMA_MINALIGN) can be used in place of this.
The code in patch does not increase the size if size is already aligned.
>>         if (flush)
>>                 flush_dcache_range(addr, addr + size);
>>         else
>> diff --git a/include/scsi.h b/include/scsi.h
>> index c52759c..c1f573e 100644
>> --- a/include/scsi.h
>> +++ b/include/scsi.h
>> @@ -26,7 +26,13 @@
>>
>>   typedef struct SCSI_cmd_block{
>>         unsigned char           cmd[16];                                        /* command                                 */
>> -       unsigned char           sense_buf[64];          /* for request sense */
>> +       /* for request sense */
>> +#ifdef ARCH_DMA_MINALIGN
>> +       unsigned char           sense_buf[64]
>> +               __attribute__((aligned(ARCH_DMA_MINALIGN)));
>> +#else
>> +       unsigned char           sense_buf[64];
>> +#endif
> I think Mike said this, but I thought ARCH_DMA_MINALIGN should always
> be defined.
>
> Is it possible to align something in the middle of a structure? How
> does that work? I'm suppose you have this working, I would just like
> to understand what the resulting code does in this case.
>
I verified that ARCH_DMA_MINALIGN is already defined so I will not check 
it in next patch.

Yes it would align the variable in middle at the time of instantiating 
the structure.
Compiler generates the addresses of this variable and structure 
variable, as 32 __BYTE__ aligned(cache line is 32).
It try to accommodate all the variables in these two addresses till 32 byte.
Another solution could be, Align whole structure of cacheline and keep 
the buffer as first element.
However in case more than one buffers are in a structure, only option is 
to align individual buffer in place of
whole structure.

>>         unsigned char           status;                                         /* SCSI Status                   */
>>         unsigned char           target;                                         /* Target ID                             */
>>         unsigned char           lun;                                                    /* Target LUN        */
>> diff --git a/include/usb.h b/include/usb.h
>> index 06170cd..d38817a 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -109,7 +109,13 @@ struct usb_device {
>>         int epmaxpacketout[16];         /* OUTput endpoint specific maximums */
>>
>>         int configno;                   /* selected config number */
>> -       struct usb_device_descriptor descriptor; /* Device Descriptor */
>> +        /* Device Descriptor */
>> +#ifdef ARCH_DMA_MINALIGN
>> +       struct usb_device_descriptor descriptor
>> +               __attribute__((aligned(ARCH_DMA_MINALIGN)));
>> +#else
>> +       struct usb_device_descriptor descriptor;
>> +#endif
> Same question here, it seems even more exotic - maybe you will need a
> memcpy somewhere after all?
>
>>         struct usb_config config; /* config descriptor */
>>
>>         int have_langid;                /* whether string_langid is valid yet */
>> --
>> 1.7.1
>>
> Regards,
> Simon


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------