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

Simon Glass
2012-02-24T12:42:29Z

Hi,

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