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

[Mike Frysinger]

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.

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

wrt the other random ALLOC_CACHE_ALIGN_BUFFER() changes, they look OK to me.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120223/42b044b1/attachment.pgp>