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

[Simon Glass]

Hi,

On Thu, Feb 23, 2012 at 6:25 AM, Puneet Saxena <puneets at 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 at nvidia.com>
> Signed-off-by: Jim Lin <jilin at 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?

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

>        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