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

[puneets <puneets@nvidia.com>]

Hi Mike,
On Thursday 23 February 2012 11:45 PM, Mike Frysinger wrote:
> * PGP Signed by an unknown key
>
> On Thursday 23 February 2012 09:25:25 Puneet Saxena wrote:
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>>
>> -static unsigned char usb_stor_buf[512];
>> -static ccb usb_ccb;
>> +#ifdef ARCH_DMA_MINALIGN
>> +	static ccb usb_ccb __attribute__((aligned(ARCH_DMA_MINALIGN)));
>> +#else
>> +	static ccb usb_ccb;
>> +#endif
> don't use ifdef's.  you may assume that ARCH_DMA_MINALIGN is always defined.
> after all, the ALLOC_CACHE_ALIGN_BUFFER() helper does.
>
>> int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
>> 				block_dev_desc_t *dev_desc)
>>   {
>>   	unsigned char perq, modi;
>> -	unsigned long cap[2];
>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned long, cap, 2);
>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, usb_stor_buf, 36);
>>   	unsigned long *capacity, *blksz;
>>   	ccb *pccb =&usb_ccb;
>>
>> +	/* GJ */
>> +	memset(usb_stor_buf, 0, sizeof(usb_stor_buf));
> you shrunk the buffer to 36 bytes, so that's good.  but the memset() should be
> dropped.  see what i posted to Marek when he tried moving this buffer locally
> if you want background.
>
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>>
>>   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;
> i don't think this logic should exist at all.  the arch is responsible for
> flushing enough of its cache to cover the requested size.
>
The patches under review is used to avoid the warnings -

1) ERROR: v7_dcache_inval_range - start address is not aligned - 0x3ffbd5d0

2) ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3ffbd5f0

This piece of code assures that second warning should not appear.

By allocating Buffers cache line aligned we are making sure that "start 
address"
will always be aligned. Arch specific code is taking care of 
flushing/invalidating the
cache as per the requested size but 2nd warning appears before doing that.
As you see in code: stop address = start address + size, so we are 
required to assure
size to be multiple of cacheline(start address we are aligning already). 
The above piece
of code is meant for this.
other option could be to add code in arch specific file to align stop 
address. e.g in "v7_dcache_maint_range@

Cache_v7.c (arch\arm\cpu\armv7)"

>> --- a/include/scsi.h
>> +++ b/include/scsi.h
>>
>>   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
>>
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> 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
> i don't think either of these headers should be changed.  find&  fix the code
> that is causing a problem.
>
IMHO, scsi.h should be changed otherwise we may need to memcpy aligned 
local buffer to
this global buffer in "usb_request_sense @ Usb_storage.c (common).

> wrt the other random ALLOC_CACHE_ALIGN_BUFFER() changes, they look OK to me.
> -mike
>
> * Unknown Key
> * 0xE837F581


-----------------------------------------------------------------------------------
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.
-----------------------------------------------------------------------------------