| Message ID | 1340209283-3404-3-git-send-email-trini@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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
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.
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
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!
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
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 */
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(-)