[U-Boot] [PATCH] usb: align buffers at cache boundary

[Mike Frysinger <vapier@gentoo.org>]

On Friday 17 February 2012 05:50:21 Puneet Saxena wrote:
> This aligns buffers,passed to usb controller at
> dcache boundary.
> 
> Buffer "stop" address = "start" address + size.
> Stop address can not be aligned even though
> "start" address is aligned as size does not fall
> at cache line boundary. Therefore "debug" in place of
> "printf" is added in "arch/arm/cpu/armv7/cache_v7.c".
> Cleaning and invalidating last cache line to avoid
> stale data to be reused.

the point is to align things based on the DMA constraints, not cacheline 
constraints (with the assumption that DMA constraints will always be equal or 
larger than cachelines and never smaller).  so using CONFIG_SYS_CACHELINE_SIZE 
is wrong.  this is exactly why we have ARCH_DMA_MINALIGN.

> Ignoring "checkpatch.pl" warnings to replace "__attribute__((packed))"
> by "__packed" and "__attribute__((aligned(size)))" by "__aligned(size)".
> As "__GNUC__" directive should be defined to use "__packed" and
> "__aligned(size)" and current patch is not supporting it, ignoring
> the "checkpatch.pl" warning.

i don't understand this.  we have linux/compiler.h which provides these macros 
so you should be able to use them.

so NAK to this implementation for the reasons above

> --- a/arch/arm/cpu/armv7/cache_v7.c
> +++ b/arch/arm/cpu/armv7/cache_v7.c
> 
>  	/*
> +	 * We can't avoid stop address not aligned with cache-line.
> +	 * Allocating cache -aligned buffer might waste memory.
>  	 * If stop address is not aligned to cache-line do not
> -	 * invalidate the last cache-line
> +	 * invalidate the last cache-line and flush the last
> +	 * line to prevent affecting somebody else's buffer.
>  	 */
>  	if (stop & (line_len - 1)) {
> -		printf("ERROR: %s - stop address is not aligned - 0x%08x\n",
> +		debug("ERROR: %s - stop address is not aligned - 0x%08x\n",
>  			__func__, stop);
> +		v7_dcache_clean_inval_range(stop, stop + 1, line_len);
>  		/* align to the beginning of this cache line */
>  		stop &= ~(line_len - 1);
>  	}

pretty sure this is ignoring the problem and so shouldn't get merged

> --- a/common/usb.c
> +++ b/common/usb.c
>
> -static struct devrequest setup_packet;
> +#ifdef CONFIG_SYS_CACHELINE_SIZE
> +	static struct devrequest setup_packet
> +		__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));
> +#else
> +	static struct devrequest setup_packet;
> +#endif

this is used in only one function, so it'd be better to simply move it to that 
one function rather than keeping it as a global.

> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> 
> -static unsigned char usb_stor_buf[512];
> +#ifdef CONFIG_SYS_CACHELINE_SIZE
> +	static unsigned char usb_stor_buf[512]
> +		__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));
> +#else
> +	static unsigned char usb_stor_buf[512];
> +#endif

Marek has already posted a patch fixing this

> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -132,8 +132,20 @@ static void cache_qtd(struct qTD *qtd, int flush)
>  	int len = (qtd->qt_token & 0x7fff0000) >> 16;
> 
>  	flush_invalidate((u32)qtd, sizeof(struct qTD), flush);
> +
> +	/*
> +	 * Invalidate cache for the aligned address
> +	 * and for the data length which is DMAed.
> +	 * Do not invalidate again intermidiate address
> +	 * as it may not be aligned.
> +	 */
> +#if CONFIG_SYS_CACHELINE_SIZE
> +	if (!((u32) ptr & (CONFIG_SYS_CACHELINE_SIZE - 1)) && len)
> +		flush_invalidate((u32)ptr, len, flush);
> +#else
>  	if (ptr && len)
>  		flush_invalidate((u32)ptr, len, flush);
> +#endif
>  }

ugh no.  existing code is correct.

> --- 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 CONFIG_SYS_CACHELINE_SIZE
> +	unsigned char		sense_buf[64]
> +		__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));
> +#else
> +	unsigned char		sense_buf[64];
> +#endif

pretty sure this is wrong

> --- 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 CONFIG_SYS_CACHELINE_SIZE
> +	struct usb_device_descriptor descriptor
> +		__attribute__((aligned(CONFIG_SYS_CACHELINE_SIZE)));
> +#else
> +	struct usb_device_descriptor descriptor;
> +#endif
>  	struct usb_config config; /* config descriptor */
> 
>  	int have_langid;		/* whether string_langid is valid yet */

as is this
-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/20120217/e26c721c/attachment.pgp>