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

Mike Frysinger
2012-02-23T18:15:51Z

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>