Patchwork [U-Boot,v3,2/6] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment

login
register
mail settings
Submitter Tom Rini
Date June 20, 2012, 4:21 p.m.
Message ID <1340209283-3404-3-git-send-email-trini@ti.com>
Download mbox | patch
Permalink /patch/166118/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Tom Rini - June 20, 2012, 4:21 p.m.
The USB spec says that 32 bytes is the minimum required alignment.
However on some platforms we have a larger minimum requirement for cache
coherency.  In those cases, use that value rather than the USB spec
minimum.  We add a cpp check to <usb.h> to define USB_DMA_MINALIGN and
make use of it in ehci-hcd.c and musb_core.h.  We cannot use MAX() here
as we are not allowed to have tests inside of align(...).

Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Tom Rini <trini@ti.com>

--
Changes in v2:
- Move test to <usb.h>, expand comment.
---
 drivers/usb/host/ehci-hcd.c  |   13 +++++++------
 drivers/usb/musb/musb_core.h |    2 +-
 include/usb.h                |   10 ++++++++++
 3 files changed, 18 insertions(+), 7 deletions(-)
Marek Vasut - June 20, 2012, 7 p.m.
Dear Tom Rini,

> The USB spec says that 32 bytes is the minimum required alignment.
> However on some platforms we have a larger minimum requirement for cache
> coherency.  In those cases, use that value rather than the USB spec
> minimum.  We add a cpp check to <usb.h> to define USB_DMA_MINALIGN and
> make use of it in ehci-hcd.c and musb_core.h.  We cannot use MAX() here
> as we are not allowed to have tests inside of align(...).
> 
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Tom Rini <trini@ti.com>
> 
> --
> Changes in v2:
> - Move test to <usb.h>, expand comment.
> ---
>  drivers/usb/host/ehci-hcd.c  |   13 +++++++------
>  drivers/usb/musb/musb_core.h |    2 +-
>  include/usb.h                |   10 ++++++++++
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 04300be..5a86117 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -29,12 +29,13 @@
> 
>  #include "ehci.h"
> 
> -int rootdev;
> -struct ehci_hccr *hccr;	/* R/O registers, not need for volatile */
> -volatile struct ehci_hcor *hcor;
> +int rootdev __attribute__((aligned(USB_DMA_MINALIGN)));
> +/* R/O registers, not need for volatile */
> +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN)));
> +volatile struct ehci_hcor *hcor
> __attribute__((aligned(USB_DMA_MINALIGN)));

^ these need to be aligned too?

> 
>  static uint16_t portreset;
> -static struct QH qh_list __attribute__((aligned(32)));
> +static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN)));
> 
>  static struct descriptor {
>  	struct usb_hub_descriptor hub;
> @@ -207,8 +208,8 @@ static int
>  ehci_submit_async(struct usb_device *dev, unsigned long pipe, void
> *buffer, int length, struct devrequest *req)
>  {
> -	static struct QH qh __attribute__((aligned(32)));
> -	static struct qTD qtd[3] __attribute__((aligned (32)));
> +	static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN)));
> +	static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));
>  	int qtd_counter = 0;
> 
>  	volatile struct qTD *vtd;
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index a8adcce..e914369 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -145,7 +145,7 @@ struct musb_regs {
>  		struct musb_epN_regs epN;
>  	} ep[16];
> 
> -} __attribute__((packed, aligned(32)));
> +} __attribute__((packed, aligned(USB_DMA_MINALIGN)));
>  #endif
> 
>  /*
> diff --git a/include/usb.h b/include/usb.h
> index 6da91e7..ba3d169 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -29,6 +29,16 @@
>  #include <usb_defs.h>
>  #include <usbdescriptors.h>
> 
> +/*
> + * The EHCI spec says that we must align to at least 32 bytes.  However,
> + * some platforms require larger alignment.
> + */
> +#if ARCH_DMA_MINALIGN > 32
> +#define USB_DMA_MINALIGN	ARCH_DMA_MINALIGN
> +#else
> +#define USB_DMA_MINALIGN	32
> +#endif
> +
>  /* Everything is aribtrary */
>  #define USB_ALTSETTINGALLOC		4
>  #define USB_MAXALTSETTING		128	/* Hard limit */

Best regards,
Marek Vasut
Tom Rini - June 20, 2012, 7:15 p.m.
On Wed, Jun 20, 2012 at 09:00:45PM +0200, Marek Vasut wrote:
> Dear Tom Rini,
> 
> > The USB spec says that 32 bytes is the minimum required alignment.
> > However on some platforms we have a larger minimum requirement for cache
> > coherency.  In those cases, use that value rather than the USB spec
> > minimum.  We add a cpp check to <usb.h> to define USB_DMA_MINALIGN and
> > make use of it in ehci-hcd.c and musb_core.h.  We cannot use MAX() here
> > as we are not allowed to have tests inside of align(...).
> > 
> > Cc: Marek Vasut <marex@denx.de>
> > Signed-off-by: Tom Rini <trini@ti.com>
> > 
> > --
> > Changes in v2:
> > - Move test to <usb.h>, expand comment.
> > ---
> >  drivers/usb/host/ehci-hcd.c  |   13 +++++++------
> >  drivers/usb/musb/musb_core.h |    2 +-
> >  include/usb.h                |   10 ++++++++++
> >  3 files changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > index 04300be..5a86117 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -29,12 +29,13 @@
> > 
> >  #include "ehci.h"
> > 
> > -int rootdev;
> > -struct ehci_hccr *hccr;	/* R/O registers, not need for volatile */
> > -volatile struct ehci_hcor *hcor;
> > +int rootdev __attribute__((aligned(USB_DMA_MINALIGN)));
> > +/* R/O registers, not need for volatile */
> > +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN)));
> > +volatile struct ehci_hcor *hcor
> > __attribute__((aligned(USB_DMA_MINALIGN)));
> 
> ^ these need to be aligned too?

Yes, these were the first and easy to spot ones in the cache flush
messages, I would swear.
Marek Vasut - June 20, 2012, 9:15 p.m.
Dear Tom Rini,

> On Wed, Jun 20, 2012 at 09:00:45PM +0200, Marek Vasut wrote:
> > Dear Tom Rini,
> > 
> > > The USB spec says that 32 bytes is the minimum required alignment.
> > > However on some platforms we have a larger minimum requirement for
> > > cache coherency.  In those cases, use that value rather than the USB
> > > spec minimum.  We add a cpp check to <usb.h> to define
> > > USB_DMA_MINALIGN and make use of it in ehci-hcd.c and musb_core.h.  We
> > > cannot use MAX() here as we are not allowed to have tests inside of
> > > align(...).
> > > 
> > > Cc: Marek Vasut <marex@denx.de>
> > > Signed-off-by: Tom Rini <trini@ti.com>
> > > 
> > > --
> > > Changes in v2:
> > > - Move test to <usb.h>, expand comment.
> > > ---
> > > 
> > >  drivers/usb/host/ehci-hcd.c  |   13 +++++++------
> > >  drivers/usb/musb/musb_core.h |    2 +-
> > >  include/usb.h                |   10 ++++++++++
> > >  3 files changed, 18 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > > index 04300be..5a86117 100644
> > > --- a/drivers/usb/host/ehci-hcd.c
> > > +++ b/drivers/usb/host/ehci-hcd.c
> > > @@ -29,12 +29,13 @@
> > > 
> > >  #include "ehci.h"
> > > 
> > > -int rootdev;
> > > -struct ehci_hccr *hccr;	/* R/O registers, not need for volatile */
> > > -volatile struct ehci_hcor *hcor;
> > > +int rootdev __attribute__((aligned(USB_DMA_MINALIGN)));
> > > +/* R/O registers, not need for volatile */
> > > +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN)));
> > > +volatile struct ehci_hcor *hcor
> > > __attribute__((aligned(USB_DMA_MINALIGN)));
> > 
> > ^ these need to be aligned too?
> 
> Yes, these were the first and easy to spot ones in the cache flush
> messages, I would swear.

hcor and hccr are register locations though ... so it's pretty weird.

Best regards,
Marek Vasut
Tom Rini - June 20, 2012, 10:07 p.m.
On Wed, Jun 20, 2012 at 11:15:26PM +0200, Marek Vasut wrote:
> Dear Tom Rini,
> 
> > On Wed, Jun 20, 2012 at 09:00:45PM +0200, Marek Vasut wrote:
> > > Dear Tom Rini,
> > > 
> > > > The USB spec says that 32 bytes is the minimum required alignment.
> > > > However on some platforms we have a larger minimum requirement for
> > > > cache coherency.  In those cases, use that value rather than the USB
> > > > spec minimum.  We add a cpp check to <usb.h> to define
> > > > USB_DMA_MINALIGN and make use of it in ehci-hcd.c and musb_core.h.  We
> > > > cannot use MAX() here as we are not allowed to have tests inside of
> > > > align(...).
> > > > 
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Signed-off-by: Tom Rini <trini@ti.com>
> > > > 
> > > > --
> > > > Changes in v2:
> > > > - Move test to <usb.h>, expand comment.
> > > > ---
> > > > 
> > > >  drivers/usb/host/ehci-hcd.c  |   13 +++++++------
> > > >  drivers/usb/musb/musb_core.h |    2 +-
> > > >  include/usb.h                |   10 ++++++++++
> > > >  3 files changed, 18 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > > > index 04300be..5a86117 100644
> > > > --- a/drivers/usb/host/ehci-hcd.c
> > > > +++ b/drivers/usb/host/ehci-hcd.c
> > > > @@ -29,12 +29,13 @@
> > > > 
> > > >  #include "ehci.h"
> > > > 
> > > > -int rootdev;
> > > > -struct ehci_hccr *hccr;	/* R/O registers, not need for volatile */
> > > > -volatile struct ehci_hcor *hcor;
> > > > +int rootdev __attribute__((aligned(USB_DMA_MINALIGN)));
> > > > +/* R/O registers, not need for volatile */
> > > > +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN)));
> > > > +volatile struct ehci_hcor *hcor
> > > > __attribute__((aligned(USB_DMA_MINALIGN)));
> > > 
> > > ^ these need to be aligned too?
> > 
> > Yes, these were the first and easy to spot ones in the cache flush
> > messages, I would swear.
> 
> hcor and hccr are register locations though ... so it's pretty weird.

OK, now that I look harder at the messages, doing that is only masking a
problem.  At issue (from u-boot.map):
 .bss           0x801586c0      0x1c0 drivers/usb/host/libusb_host.o
                0x80158860                hcor
And a message of:
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x9ffbc860
Using 'bdinfo' to see reloc offset gives us hcor, but _end_ not start
means it's something within the anonymous part of the bss and padding
hcor out just masks the real problem.  I'll resubmit a v4 shortly.
Thanks!
Marek Vasut - June 21, 2012, 12:09 a.m.
Dear Tom Rini,

> On Wed, Jun 20, 2012 at 11:15:26PM +0200, Marek Vasut wrote:
> > Dear Tom Rini,
> > 
> > > On Wed, Jun 20, 2012 at 09:00:45PM +0200, Marek Vasut wrote:
> > > > Dear Tom Rini,
> > > > 
> > > > > The USB spec says that 32 bytes is the minimum required alignment.
> > > > > However on some platforms we have a larger minimum requirement for
> > > > > cache coherency.  In those cases, use that value rather than the
> > > > > USB spec minimum.  We add a cpp check to <usb.h> to define
> > > > > USB_DMA_MINALIGN and make use of it in ehci-hcd.c and musb_core.h. 
> > > > > We cannot use MAX() here as we are not allowed to have tests
> > > > > inside of align(...).
> > > > > 
> > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > Signed-off-by: Tom Rini <trini@ti.com>
> > > > > 
> > > > > --
> > > > > Changes in v2:
> > > > > - Move test to <usb.h>, expand comment.
> > > > > ---
> > > > > 
> > > > >  drivers/usb/host/ehci-hcd.c  |   13 +++++++------
> > > > >  drivers/usb/musb/musb_core.h |    2 +-
> > > > >  include/usb.h                |   10 ++++++++++
> > > > >  3 files changed, 18 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/host/ehci-hcd.c
> > > > > b/drivers/usb/host/ehci-hcd.c index 04300be..5a86117 100644
> > > > > --- a/drivers/usb/host/ehci-hcd.c
> > > > > +++ b/drivers/usb/host/ehci-hcd.c
> > > > > @@ -29,12 +29,13 @@
> > > > > 
> > > > >  #include "ehci.h"
> > > > > 
> > > > > -int rootdev;
> > > > > -struct ehci_hccr *hccr;	/* R/O registers, not need for volatile 
*/
> > > > > -volatile struct ehci_hcor *hcor;
> > > > > +int rootdev __attribute__((aligned(USB_DMA_MINALIGN)));
> > > > > +/* R/O registers, not need for volatile */
> > > > > +struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN)));
> > > > > +volatile struct ehci_hcor *hcor
> > > > > __attribute__((aligned(USB_DMA_MINALIGN)));
> > > > 
> > > > ^ these need to be aligned too?
> > > 
> > > Yes, these were the first and easy to spot ones in the cache flush
> > > messages, I would swear.
> > 
> > hcor and hccr are register locations though ... so it's pretty weird.
> 
> OK, now that I look harder at the messages, doing that is only masking a
> problem.  At issue (from u-boot.map):
>  .bss           0x801586c0      0x1c0 drivers/usb/host/libusb_host.o
>                 0x80158860                hcor
> And a message of:
> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x9ffbc860
> Using 'bdinfo' to see reloc offset gives us hcor, but _end_ not start
> means it's something within the anonymous part of the bss and padding
> hcor out just masks the real problem.  I'll resubmit a v4 shortly.
> Thanks!

You're welcome, Add my Acked-by: Marek Vasut <marex@denx.de> and push through 
omap since it's part of a bigger patchset.

Best regards,
Marek Vasut

Patch

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 04300be..5a86117 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -29,12 +29,13 @@ 
 
 #include "ehci.h"
 
-int rootdev;
-struct ehci_hccr *hccr;	/* R/O registers, not need for volatile */
-volatile struct ehci_hcor *hcor;
+int rootdev __attribute__((aligned(USB_DMA_MINALIGN)));
+/* R/O registers, not need for volatile */
+struct ehci_hccr *hccr __attribute__((aligned(USB_DMA_MINALIGN)));
+volatile struct ehci_hcor *hcor __attribute__((aligned(USB_DMA_MINALIGN)));
 
 static uint16_t portreset;
-static struct QH qh_list __attribute__((aligned(32)));
+static struct QH qh_list __attribute__((aligned(USB_DMA_MINALIGN)));
 
 static struct descriptor {
 	struct usb_hub_descriptor hub;
@@ -207,8 +208,8 @@  static int
 ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		   int length, struct devrequest *req)
 {
-	static struct QH qh __attribute__((aligned(32)));
-	static struct qTD qtd[3] __attribute__((aligned (32)));
+	static struct QH qh __attribute__((aligned(USB_DMA_MINALIGN)));
+	static struct qTD qtd[3] __attribute__((aligned(USB_DMA_MINALIGN)));
 	int qtd_counter = 0;
 
 	volatile struct qTD *vtd;
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index a8adcce..e914369 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -145,7 +145,7 @@  struct musb_regs {
 		struct musb_epN_regs epN;
 	} ep[16];
 
-} __attribute__((packed, aligned(32)));
+} __attribute__((packed, aligned(USB_DMA_MINALIGN)));
 #endif
 
 /*
diff --git a/include/usb.h b/include/usb.h
index 6da91e7..ba3d169 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -29,6 +29,16 @@ 
 #include <usb_defs.h>
 #include <usbdescriptors.h>
 
+/*
+ * The EHCI spec says that we must align to at least 32 bytes.  However,
+ * some platforms require larger alignment.
+ */
+#if ARCH_DMA_MINALIGN > 32
+#define USB_DMA_MINALIGN	ARCH_DMA_MINALIGN
+#else
+#define USB_DMA_MINALIGN	32
+#endif
+
 /* Everything is aribtrary */
 #define USB_ALTSETTINGALLOC		4
 #define USB_MAXALTSETTING		128	/* Hard limit */