* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER
@ 2012-07-08 3:08 Marek Vasut
2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Marek Vasut @ 2012-07-08 3:08 UTC (permalink / raw)
To: u-boot
This is the out-of-function-scope counterpart of
ALLOC_CACHE_ALIGN_BUFFER.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Tom Rini <trini@ti.com>
Cc: Ilya Yanok <ilya.yanok@cogentembedded.com>
---
include/common.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/common.h b/include/common.h
index 17c64b0..06d278f 100644
--- a/include/common.h
+++ b/include/common.h
@@ -965,6 +965,17 @@ int cpu_release(int nr, int argc, char * const argv[]);
\
type *name = (type *) ALIGN((uintptr_t)__##name, ARCH_DMA_MINALIGN)
+/*
+ * DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's
+ * purpose is to allow allocating aligned buffers outside of function scope.
+ * Usage of this macro shall be avoided or used with extreme care!
+ */
+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) \
+ static char __##name[roundup(size * sizeof(type), ARCH_DMA_MINALIGN)] \
+ __aligned(ARCH_DMA_MINALIGN); \
+ \
+ static type *name = (type *)__##name;
+
/* Pull in stuff for the build system */
#ifdef DO_DEPS_ONLY
# include <environment.h>
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 3:08 [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Marek Vasut @ 2012-07-08 3:08 ` Marek Vasut 2012-07-08 10:04 ` Ilya Yanok ` (2 more replies) 2012-07-08 12:23 ` [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Ilya Yanok 2012-07-20 4:01 ` Mike Frysinger 2 siblings, 3 replies; 21+ messages in thread From: Marek Vasut @ 2012-07-08 3:08 UTC (permalink / raw) To: u-boot From: Tom Rini <trini@ti.com> 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(...). Signed-off-by: Tom Rini <trini@ti.com> [ilya.yanok]: fix size alignment, drop (incorrect) rounding when invalidating the buffer. If we got unaligned buffer from the upper layer -- that's definetely a bug so it's good to buzz about it. But we have to align the buffer length -- upper layers should take care to reserve enough space. Signed-off-by: Ilya Yanok <ilya.yanok@cogentembedded.com> [marek.vasut]: introduce some crazy macro voodoo Signed-off-by: Marek Vasut <marex@denx.de> --- drivers/usb/host/ehci-hcd.c | 81 ++++++++++++++++++++++++------------------ drivers/usb/musb/musb_core.h | 2 +- include/usb.h | 10 ++++++ 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 04300be..5199560 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -34,7 +34,10 @@ struct ehci_hccr *hccr; /* R/O registers, not need for volatile */ volatile struct ehci_hcor *hcor; static uint16_t portreset; -static struct QH qh_list __attribute__((aligned(32))); +DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1); + +#define ALIGN_END_ADDR(type, ptr, size) \ + ((uint32_t)(ptr) + roundup((size) * sizeof(type), USB_DMA_MINALIGN)) static struct descriptor { struct usb_hub_descriptor hub; @@ -172,13 +175,13 @@ static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz) { uint32_t delta, next; uint32_t addr = (uint32_t)buf; - size_t rsz = roundup(sz, 32); + size_t rsz = roundup(sz, USB_DMA_MINALIGN); int idx; if (sz != rsz) debug("EHCI-HCD: Misaligned buffer size (%08x)\n", sz); - if (addr & 31) + if (addr != ALIGN(addr, USB_DMA_MINALIGN)) debug("EHCI-HCD: Misaligned buffer address (%p)\n", buf); idx = 0; @@ -207,8 +210,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))); + ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1); + ALLOC_CACHE_ALIGN_BUFFER(struct qTD, qtd, 3); int qtd_counter = 0; volatile struct qTD *vtd; @@ -229,8 +232,8 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, le16_to_cpu(req->value), le16_to_cpu(req->value), le16_to_cpu(req->index)); - memset(&qh, 0, sizeof(struct QH)); - memset(qtd, 0, sizeof(qtd)); + memset(qh, 0, sizeof(struct QH)); + memset(qtd, 0, 3 * sizeof(*qtd)); toggle = usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe)); @@ -244,7 +247,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, * qh_overlay.qt_next ...... 13-10 H * - qh_overlay.qt_altnext */ - qh.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH); + qh->qh_link = cpu_to_hc32((uint32_t)qh_list | QH_LINK_TYPE_QH); c = (usb_pipespeed(pipe) != USB_SPEED_HIGH && usb_pipeendpoint(pipe) == 0) ? 1 : 0; endpt = (8 << 28) | @@ -255,14 +258,14 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, (usb_pipespeed(pipe) << 12) | (usb_pipeendpoint(pipe) << 8) | (0 << 7) | (usb_pipedevice(pipe) << 0); - qh.qh_endpt1 = cpu_to_hc32(endpt); + qh->qh_endpt1 = cpu_to_hc32(endpt); endpt = (1 << 30) | (dev->portnr << 23) | (dev->parent->devnum << 16) | (0 << 8) | (0 << 0); - qh.qh_endpt2 = cpu_to_hc32(endpt); - qh.qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE); + qh->qh_endpt2 = cpu_to_hc32(endpt); + qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE); - tdp = &qh.qh_overlay.qt_next; + tdp = &qh->qh_overlay.qt_next; if (req != NULL) { /* @@ -340,13 +343,13 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, tdp = &qtd[qtd_counter++].qt_next; } - qh_list.qh_link = cpu_to_hc32((uint32_t)&qh | QH_LINK_TYPE_QH); + qh_list->qh_link = cpu_to_hc32((uint32_t)qh | QH_LINK_TYPE_QH); /* Flush dcache */ - flush_dcache_range((uint32_t)&qh_list, - (uint32_t)&qh_list + sizeof(struct QH)); - flush_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct QH)); - flush_dcache_range((uint32_t)qtd, (uint32_t)qtd + sizeof(qtd)); + flush_dcache_range((uint32_t)qh_list, + ALIGN_END_ADDR(struct QH, qh_list, 1)); + flush_dcache_range((uint32_t)qh, ALIGN_END_ADDR(struct QH, qh, 1)); + flush_dcache_range((uint32_t)qtd, ALIGN_END_ADDR(struct qTD, qtd, 3)); usbsts = ehci_readl(&hcor->or_usbsts); ehci_writel(&hcor->or_usbsts, (usbsts & 0x3f)); @@ -369,12 +372,12 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, timeout = USB_TIMEOUT_MS(pipe); do { /* Invalidate dcache */ - invalidate_dcache_range((uint32_t)&qh_list, - (uint32_t)&qh_list + sizeof(struct QH)); - invalidate_dcache_range((uint32_t)&qh, - (uint32_t)&qh + sizeof(struct QH)); + invalidate_dcache_range((uint32_t)qh_list, + ALIGN_END_ADDR(struct QH, qh_list, 1)); + invalidate_dcache_range((uint32_t)qh, + ALIGN_END_ADDR(struct QH, qh, 1)); invalidate_dcache_range((uint32_t)qtd, - (uint32_t)qtd + sizeof(qtd)); + ALIGN_END_ADDR(struct qTD, qtd, 3)); token = hc32_to_cpu(vtd->qt_token); if (!(token & 0x80)) @@ -382,9 +385,17 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, WATCHDOG_RESET(); } while (get_timer(ts) < timeout); - /* Invalidate the memory area occupied by buffer */ - invalidate_dcache_range(((uint32_t)buffer & ~31), - ((uint32_t)buffer & ~31) + roundup(length, 32)); + /* + * Invalidate the memory area occupied by buffer + * Don't try to fix the buffer alignment, if it isn't properly + * aligned it's upper layer's fault so let invalidate_dcache_range() + * vow about it. But we have to fix the length as it's actual + * transfer length and can be unaligned. This is potentially + * dangerous operation, it's responsibility of the calling + * code to make sure enough space is reserved. + */ + invalidate_dcache_range((uint32_t)buffer, + ALIGN_END_ADDR(u8, buffer, length)); /* Check that the TD processing happened */ if (token & 0x80) { @@ -403,9 +414,9 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, goto fail; } - qh_list.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH); + qh_list->qh_link = cpu_to_hc32((uint32_t)qh_list | QH_LINK_TYPE_QH); - token = hc32_to_cpu(qh.qh_overlay.qt_token); + token = hc32_to_cpu(qh->qh_overlay.qt_token); if (!(token & 0x80)) { debug("TOKEN=%#x\n", token); switch (token & 0xfc) { @@ -733,16 +744,16 @@ int usb_lowlevel_init(void) #endif /* Set head of reclaim list */ - memset(&qh_list, 0, sizeof(qh_list)); - qh_list.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH); - qh_list.qh_endpt1 = cpu_to_hc32((1 << 15) | (USB_SPEED_HIGH << 12)); - qh_list.qh_curtd = cpu_to_hc32(QT_NEXT_TERMINATE); - qh_list.qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE); - qh_list.qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE); - qh_list.qh_overlay.qt_token = cpu_to_hc32(0x40); + memset(qh_list, 0, sizeof(*qh_list)); + qh_list->qh_link = cpu_to_hc32((uint32_t)qh_list | QH_LINK_TYPE_QH); + qh_list->qh_endpt1 = cpu_to_hc32((1 << 15) | (USB_SPEED_HIGH << 12)); + qh_list->qh_curtd = cpu_to_hc32(QT_NEXT_TERMINATE); + qh_list->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE); + qh_list->qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE); + qh_list->qh_overlay.qt_token = cpu_to_hc32(0x40); /* Set async. queue head pointer. */ - ehci_writel(&hcor->or_asynclistaddr, (uint32_t)&qh_list); + ehci_writel(&hcor->or_asynclistaddr, (uint32_t)qh_list); reg = ehci_readl(&hccr->cr_hcsparams); descriptor.hub.bNbrPorts = HCS_N_PORTS(reg); 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 */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut @ 2012-07-08 10:04 ` Ilya Yanok 2012-07-08 18:51 ` Marek Vasut 2012-07-09 18:37 ` Ilya Yanok 2012-07-14 22:08 ` Ilya Yanok 2 siblings, 1 reply; 21+ messages in thread From: Ilya Yanok @ 2012-07-08 10:04 UTC (permalink / raw) To: u-boot Hi Marek, On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: [...] diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > index 04300be..5199560 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -34,7 +34,10 @@ struct ehci_hccr *hccr; /* R/O registers, not need > for volatile */ > volatile struct ehci_hcor *hcor; > > static uint16_t portreset; > -static struct QH qh_list __attribute__((aligned(32))); > +DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1); > This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by USB spec. The same with other buffers. Otherwise looks great. Regards, Ilya. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 10:04 ` Ilya Yanok @ 2012-07-08 18:51 ` Marek Vasut 2012-07-08 20:52 ` Tom Rini 0 siblings, 1 reply; 21+ messages in thread From: Marek Vasut @ 2012-07-08 18:51 UTC (permalink / raw) To: u-boot Dear Ilya Yanok, > Hi Marek, > > On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > > [...] > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > > > index 04300be..5199560 100644 > > --- a/drivers/usb/host/ehci-hcd.c > > +++ b/drivers/usb/host/ehci-hcd.c > > @@ -34,7 +34,10 @@ struct ehci_hccr *hccr; /* R/O registers, not > > need for volatile */ > > > > volatile struct ehci_hcor *hcor; > > > > static uint16_t portreset; > > > > -static struct QH qh_list __attribute__((aligned(32))); > > +DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1); > > This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of > ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by USB > spec. That's true -- maybe we should create ALLOC_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() to be a special case of it ? > The same with other buffers. Otherwise looks great. > > Regards, Ilya. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 18:51 ` Marek Vasut @ 2012-07-08 20:52 ` Tom Rini 2012-07-08 21:36 ` Marek Vasut 0 siblings, 1 reply; 21+ messages in thread From: Tom Rini @ 2012-07-08 20:52 UTC (permalink / raw) To: u-boot On Sun, Jul 8, 2012 at 11:51 AM, Marek Vasut <marex@denx.de> wrote: > Dear Ilya Yanok, > >> Hi Marek, >> >> On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: >> >> [...] >> >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c >> >> > index 04300be..5199560 100644 >> > --- a/drivers/usb/host/ehci-hcd.c >> > +++ b/drivers/usb/host/ehci-hcd.c >> > @@ -34,7 +34,10 @@ struct ehci_hccr *hccr; /* R/O registers, not >> > need for volatile */ >> > >> > volatile struct ehci_hcor *hcor; >> > >> > static uint16_t portreset; >> > >> > -static struct QH qh_list __attribute__((aligned(32))); >> > +DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1); >> >> This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of >> ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by USB >> spec. > > That's true -- maybe we should create ALLOC_ALIGN_BUFFER() and > ALLOC_CACHE_ALIGN_BUFFER() to be a special case of it ? Lets think. USB says "32byte min". Do we have other buses today that apply similar constraints? If so, we should probably go about abstracting into a __ALIGN_FOO(size, what) and do a general one and a USB one that does 32 or cachline. If it's just USB that might be overkill however... -- Tom ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 20:52 ` Tom Rini @ 2012-07-08 21:36 ` Marek Vasut 0 siblings, 0 replies; 21+ messages in thread From: Marek Vasut @ 2012-07-08 21:36 UTC (permalink / raw) To: u-boot Dear Tom Rini, > On Sun, Jul 8, 2012 at 11:51 AM, Marek Vasut <marex@denx.de> wrote: > > Dear Ilya Yanok, > > > >> Hi Marek, > >> > >> On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > >> > >> [...] > >> > >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > >> > >> > index 04300be..5199560 100644 > >> > --- a/drivers/usb/host/ehci-hcd.c > >> > +++ b/drivers/usb/host/ehci-hcd.c > >> > @@ -34,7 +34,10 @@ struct ehci_hccr *hccr; /* R/O registers, not > >> > need for volatile */ > >> > > >> > volatile struct ehci_hcor *hcor; > >> > > >> > static uint16_t portreset; > >> > > >> > -static struct QH qh_list __attribute__((aligned(32))); > >> > +DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1); > >> > >> This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of > >> ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by > >> USB spec. > > > > That's true -- maybe we should create ALLOC_ALIGN_BUFFER() and > > ALLOC_CACHE_ALIGN_BUFFER() to be a special case of it ? > > Lets think. USB says "32byte min". Do we have other buses today that > apply similar constraints? If so, we should probably go about > abstracting into a __ALIGN_FOO(size, what) and do a general one and a > USB one that does 32 or cachline. If it's just USB that might be > overkill however... Let's do this, we dunno what craziness can befall us in the future. And having the macro handy won't hurt anyway. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut 2012-07-08 10:04 ` Ilya Yanok @ 2012-07-09 18:37 ` Ilya Yanok 2012-07-10 1:55 ` Marek Vasut 2012-07-14 22:08 ` Ilya Yanok 2 siblings, 1 reply; 21+ messages in thread From: Ilya Yanok @ 2012-07-09 18:37 UTC (permalink / raw) To: u-boot Hi Marek, On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > - /* Invalidate the memory area occupied by buffer */ > - invalidate_dcache_range(((uint32_t)buffer & ~31), > - ((uint32_t)buffer & ~31) + roundup(length, 32)); > + /* > + * Invalidate the memory area occupied by buffer > + * Don't try to fix the buffer alignment, if it isn't properly > + * aligned it's upper layer's fault so let > invalidate_dcache_range() > + * vow about it. But we have to fix the length as it's actual > + * transfer length and can be unaligned. This is potentially > + * dangerous operation, it's responsibility of the calling > + * code to make sure enough space is reserved. > + */ > + invalidate_dcache_range((uint32_t)buffer, > + ALIGN_END_ADDR(u8, buffer, length)); > We shouldn't use ALIGN_END_ADDR here. This can lead to strange results on systems with ARCH_DMA_MINALIGN < 32. Regards, Ilya. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-09 18:37 ` Ilya Yanok @ 2012-07-10 1:55 ` Marek Vasut 2012-07-14 22:11 ` Ilya Yanok 0 siblings, 1 reply; 21+ messages in thread From: Marek Vasut @ 2012-07-10 1:55 UTC (permalink / raw) To: u-boot Dear Ilya Yanok, > Hi Marek, > > On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > > - /* Invalidate the memory area occupied by buffer */ > > - invalidate_dcache_range(((uint32_t)buffer & ~31), > > - ((uint32_t)buffer & ~31) + roundup(length, 32)); > > + /* > > + * Invalidate the memory area occupied by buffer > > + * Don't try to fix the buffer alignment, if it isn't properly > > + * aligned it's upper layer's fault so let > > invalidate_dcache_range() > > + * vow about it. But we have to fix the length as it's actual > > + * transfer length and can be unaligned. This is potentially > > + * dangerous operation, it's responsibility of the calling > > + * code to make sure enough space is reserved. > > + */ > > + invalidate_dcache_range((uint32_t)buffer, > > + ALIGN_END_ADDR(u8, buffer, length)); > > We shouldn't use ALIGN_END_ADDR here. This can lead to strange results on > systems with ARCH_DMA_MINALIGN < 32. Good point, ALIGN_END_ADDR should align it to max(32, length) ? > Regards, Ilya. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-10 1:55 ` Marek Vasut @ 2012-07-14 22:11 ` Ilya Yanok 0 siblings, 0 replies; 21+ messages in thread From: Ilya Yanok @ 2012-07-14 22:11 UTC (permalink / raw) To: u-boot Dear Marek, On Tue, Jul 10, 2012 at 5:55 AM, Marek Vasut <marex@denx.de> wrote: > > > > + invalidate_dcache_range((uint32_t)buffer, > > > + ALIGN_END_ADDR(u8, buffer, length)); > > > > We shouldn't use ALIGN_END_ADDR here. This can lead to strange results on > > systems with ARCH_DMA_MINALIGN < 32. > > > Good point, ALIGN_END_ADDR should align it to max(32, length) ? > > Hm.. no, actually we should align buffer length to ARCH_DMA_MINALIGN. Regards, Ilya. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut 2012-07-08 10:04 ` Ilya Yanok 2012-07-09 18:37 ` Ilya Yanok @ 2012-07-14 22:08 ` Ilya Yanok 2012-07-15 8:07 ` Marek Vasut 2 siblings, 1 reply; 21+ messages in thread From: Ilya Yanok @ 2012-07-14 22:08 UTC (permalink / raw) To: u-boot Hi, On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > @@ -207,8 +210,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))); > + ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1); > Somehow this doesn't work being allocated on stack... I assumed that this was declared as static just to use __attribute__((aligned))... but it seems that current code doesn't like qh address being changed (I'm getting "Timed out on TD" messages). Any ideas? Changing it to DEFINE_... seems to help but it looks like hiding another bug. Regards, Ilya. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-14 22:08 ` Ilya Yanok @ 2012-07-15 8:07 ` Marek Vasut 2012-07-15 8:55 ` Ilya Yanok 0 siblings, 1 reply; 21+ messages in thread From: Marek Vasut @ 2012-07-15 8:07 UTC (permalink / raw) To: u-boot Dear Ilya Yanok, > Hi, > > On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > > @@ -207,8 +210,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))); > > + ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1); > > Somehow this doesn't work being allocated on stack... I assumed that this > was declared as static just to use __attribute__((aligned))... Not really ... but then, we have indeed a problem here. It was probably declared static to preserve the contents of these variables across the the calls of this function. Sigh, the USB code is really bloody :-/ Ilya, can you try pulling these out of the function? > but it seems > that current code doesn't like qh address being changed (I'm getting "Timed > out on TD" messages). Any ideas? Changing it to DEFINE_... seems to help > but it looks like hiding another bug. > > Regards, Ilya. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-15 8:07 ` Marek Vasut @ 2012-07-15 8:55 ` Ilya Yanok 2012-07-15 9:42 ` Marek Vasut 0 siblings, 1 reply; 21+ messages in thread From: Ilya Yanok @ 2012-07-15 8:55 UTC (permalink / raw) To: u-boot Dear Marek, On Sun, Jul 15, 2012 at 12:07 PM, Marek Vasut <marex@denx.de> wrote: > > > @@ -207,8 +210,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))); > > > + ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1); > > > > Somehow this doesn't work being allocated on stack... I assumed that this > > was declared as static just to use __attribute__((aligned))... > > Not really ... but then, we have indeed a problem here. It was probably > declared > static to preserve the contents of these variables across the the calls of > this > function. Sigh, the USB code is really bloody :-/ > Hm.. no, I don't think it's about contents... Look, there are memset() calls straight in the beginning of the function. It looks like it's about qh address: I tried debugging it a bit and it seems to work until qh has the same address and break when the address eventually changes. > Ilya, can you try pulling these out of the function? > I can but what for? Are there any differences (except for scope) declaring statics inside or outside function? As I said declaring qh static (with DEFINE_CACHE_ALIGN_BUFFER) works but... Regards, Ilya. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment 2012-07-15 8:55 ` Ilya Yanok @ 2012-07-15 9:42 ` Marek Vasut 0 siblings, 0 replies; 21+ messages in thread From: Marek Vasut @ 2012-07-15 9:42 UTC (permalink / raw) To: u-boot Dear Ilya Yanok, > Dear Marek, > > On Sun, Jul 15, 2012 at 12:07 PM, Marek Vasut <marex@denx.de> wrote: > > > > @@ -207,8 +210,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))); > > > > + ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1); > > > > > > Somehow this doesn't work being allocated on stack... I assumed that > > > this was declared as static just to use __attribute__((aligned))... > > > > Not really ... but then, we have indeed a problem here. It was probably > > declared > > static to preserve the contents of these variables across the the calls > > of this > > function. Sigh, the USB code is really bloody :-/ > > Hm.. no, I don't think it's about contents... Look, there are memset() > calls straight in the beginning of the function. It looks like it's about > qh address: I tried debugging it a bit and it seems to work until qh has > the same address and break when the address eventually changes. I see, stupid me. So it's only the address? And I take it the controller doesn't use those after it leaves the function, right (maybe that might be it) ? > > Ilya, can you try pulling these out of the function? > > I can but what for? Are there any differences (except for scope) declaring > statics inside or outside function? As I said declaring qh static (with > DEFINE_CACHE_ALIGN_BUFFER) works but... > > Regards, Ilya. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-08 3:08 [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Marek Vasut 2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut @ 2012-07-08 12:23 ` Ilya Yanok 2012-07-08 18:55 ` Marek Vasut 2012-07-20 4:01 ` Mike Frysinger 2 siblings, 1 reply; 21+ messages in thread From: Ilya Yanok @ 2012-07-08 12:23 UTC (permalink / raw) To: u-boot Hi Marek, On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > This is the out-of-function-scope counterpart of > ALLOC_CACHE_ALIGN_BUFFER. > +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) \ > + static char __##name[roundup(size * sizeof(type), > ARCH_DMA_MINALIGN)] \ > + __aligned(ARCH_DMA_MINALIGN); \ > We need linux/compiler.h (not included from common.h) for __aligned. Regards, Ilya. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-08 12:23 ` [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Ilya Yanok @ 2012-07-08 18:55 ` Marek Vasut 0 siblings, 0 replies; 21+ messages in thread From: Marek Vasut @ 2012-07-08 18:55 UTC (permalink / raw) To: u-boot Dear Ilya Yanok, > Hi Marek, > > On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > > This is the out-of-function-scope counterpart of > > ALLOC_CACHE_ALIGN_BUFFER. > > +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) \ > > + static char __##name[roundup(size * sizeof(type), > > ARCH_DMA_MINALIGN)] \ > > + __aligned(ARCH_DMA_MINALIGN); \ > > We need linux/compiler.h (not included from common.h) for __aligned. Correct, but is it a good idea to include it in common.h ? Ilya, can you re-do this patchset and repost? > Regards, Ilya. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-08 3:08 [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Marek Vasut 2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut 2012-07-08 12:23 ` [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Ilya Yanok @ 2012-07-20 4:01 ` Mike Frysinger 2012-07-20 11:31 ` Marek Vasut 2 siblings, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2012-07-20 4:01 UTC (permalink / raw) To: u-boot On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > +/* > + * DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's > + * purpose is to allow allocating aligned buffers outside of function scope. > + * Usage of this macro shall be avoided or used with extreme care! > + */ > +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) \ > + static char __##name[roundup(size * sizeof(type), ARCH_DMA_MINALIGN)] \ > + __aligned(ARCH_DMA_MINALIGN); \ > + \ > + static type *name = (type *)__##name; how is this any different from doing: static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN); -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/20120720/017e9eec/attachment.pgp> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-20 4:01 ` Mike Frysinger @ 2012-07-20 11:31 ` Marek Vasut 2012-07-20 21:47 ` Mike Frysinger 0 siblings, 1 reply; 21+ messages in thread From: Marek Vasut @ 2012-07-20 11:31 UTC (permalink / raw) To: u-boot Dear Mike Frysinger, > On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > > +/* > > + * DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, > > but it's + * purpose is to allow allocating aligned buffers outside of > > function scope. + * Usage of this macro shall be avoided or used with > > extreme care! + */ > > +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) \ > > + static char __##name[roundup(size * sizeof(type), ARCH_DMA_MINALIGN)] \ > > + __aligned(ARCH_DMA_MINALIGN); \ > > + \ > > + static type *name = (type *)__##name; > > how is this any different from doing: > static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN); > -mike Does __aligned() align both start of the buffer downwards and end of it upwards ? Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-20 11:31 ` Marek Vasut @ 2012-07-20 21:47 ` Mike Frysinger 2012-07-20 21:50 ` Tom Rini 0 siblings, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2012-07-20 21:47 UTC (permalink / raw) To: u-boot On Friday 20 July 2012 07:31:47 Marek Vasut wrote: > Dear Mike Frysinger, > > On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > > > +/* > > > DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, > > > but it's purpose is to allow allocating aligned buffers outside of > > > function scope. Usage of this macro shall be avoided or used with > > > extreme care! */ > > > +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) > > > + static char __##name[roundup(size * sizeof(type), ARCH_DMA_MINALIGN)] > > > + __aligned(ARCH_DMA_MINALIGN); > > > + static type *name = (type *)__##name; > > > > how is this any different from doing: > > static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN); > > Does __aligned() align both start of the buffer downwards and end of it > upwards ? it guarantees the start is aligned. i don't believe it does any tail padding. that said, you've added just 1 consumer, but it uses in function scope, so i don't see why you had to define a new helper in the first place. the existing one would work fine shouldn't it ? you should probably add a const to the 2nd part so gcc is more likely to optimize away the storage: static type * const name = (type *)__##name; otherwise older gcc versions will create a pointer to go through rather than having things directly access the buffer. w/out const: $ readelf -s a.out | grep foo 11: 00000000004005d0 8 OBJECT LOCAL DEFAULT 13 foo.1592 12: 0000000000402080 96 OBJECT LOCAL DEFAULT 25 __foo.1591 w/const: $ readelf -s a.out | grep foo 11: 0000000000402080 96 OBJECT LOCAL DEFAULT 25 __foo.1591 -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/20120720/83b5b67c/attachment.pgp> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-20 21:47 ` Mike Frysinger @ 2012-07-20 21:50 ` Tom Rini 2012-07-21 17:22 ` Mike Frysinger 0 siblings, 1 reply; 21+ messages in thread From: Tom Rini @ 2012-07-20 21:50 UTC (permalink / raw) To: u-boot -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/20/2012 02:47 PM, Mike Frysinger wrote: > On Friday 20 July 2012 07:31:47 Marek Vasut wrote: >> Dear Mike Frysinger, >>> On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: >>>> +/* DEFINE_CACHE_ALIGN_BUFFER() is similar to >>>> ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow >>>> allocating aligned buffers outside of function scope. Usage >>>> of this macro shall be avoided or used with extreme care! */ >>>> +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) + static >>>> char __##name[roundup(size * sizeof(type), >>>> ARCH_DMA_MINALIGN)] + __aligned(ARCH_DMA_MINALIGN); + >>>> static type *name = (type *)__##name; >>> >>> how is this any different from doing: static __u8 foo[1234] >>> __aligned(ARCH_DMA_MINALIGN); >> >> Does __aligned() align both start of the buffer downwards and end >> of it upwards ? > > it guarantees the start is aligned. i don't believe it does any > tail padding. > > that said, you've added just 1 consumer, but it uses in function > scope, so i don't see why you had to define a new helper in the > first place. the existing one would work fine shouldn't it ? The rough outline of the problems are: - - We need to have buffers that are multiple of cache size, for clearing. - - Today we know ehci-hcd had a problem. We also know other drivers / layers have problems, but they aren't as readily breakable. That's why we put the macro in <common.h> rather than a USB header. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJQCdKoAAoJENk4IS6UOR1WuzUP/0ndUzaq6U2/8X8E7VIhLqAV AQ+5rmJy5seJAhhvzRLbNgYv7KrLJRayMd4bj4DT8WAmAyUDo1BPvIcKNuQ8Il3M un3Kjo9+qXusfyvbs1V2wOaXL1pmwcOSQe4n+/8xKlmJ3Jh9v1AIBoXIJQqXPrcL zhaN0ilxTcLeblFWVzXSOWPAWo4ufJZ1cd43lGXxtjW890GEUCRTNXod9Bpivi2P ShycGvEcvo80mR7xGKMjZYl9Zf5OQ5QP0Xcvul7X4rjGepEuOsn941wE0zrP1I+s zXfruOklqWBckiP+aBprX2lzsaBUE33hHZidfPtvuB9rTFk735snaDcrZsL/5K25 17Bczpqqb6RXP1yb/tpc1hWkzpfCZ+eqpg2pN8bIp0P8XZ5apMTvq7L9IMG90lN2 FPwOC2VH9gnOK34Est/iTkp6QnW15r/ZayD9DMBUoxelhx38VJHR8OVyu56QXk2K 10zb6lwKQ3dE8u24ki2TybycVgPVoOq35vUjMw7TWZwiwXSBwgcfHMRukpTSdHA6 wNzw+VsurehKdkqRBeG5tOeOf0tcFrKfg1tfyDQqlBJUn9E6Usj43IXfF23DOQ11 VMcMeizuHP6oTmBk571XrgOKczCaUej1UIrLhnfpXNmFsQ7YsIi9PlQcndFiMBLC yo+rfRjqLqrPxJdrP5h4 =alD6 -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-20 21:50 ` Tom Rini @ 2012-07-21 17:22 ` Mike Frysinger 2012-07-23 15:24 ` Tom Rini 0 siblings, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2012-07-21 17:22 UTC (permalink / raw) To: u-boot On Friday 20 July 2012 17:50:33 Tom Rini wrote: > On 07/20/2012 02:47 PM, Mike Frysinger wrote: > > On Friday 20 July 2012 07:31:47 Marek Vasut wrote: > >> Dear Mike Frysinger, > >> > >>> On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > >>>> +/* DEFINE_CACHE_ALIGN_BUFFER() is similar to > >>>> ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow > >>>> allocating aligned buffers outside of function scope. Usage > >>>> of this macro shall be avoided or used with extreme care! */ > >>>> +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) + static > >>>> char __##name[roundup(size * sizeof(type), > >>>> ARCH_DMA_MINALIGN)] + __aligned(ARCH_DMA_MINALIGN); + > >>>> static type *name = (type *)__##name; > >>> > >>> how is this any different from doing: static __u8 foo[1234] > >>> __aligned(ARCH_DMA_MINALIGN); > >> > >> Does __aligned() align both start of the buffer downwards and end > >> of it upwards ? > > > > it guarantees the start is aligned. i don't believe it does any > > tail padding. > > > > that said, you've added just 1 consumer, but it uses in function > > scope, so i don't see why you had to define a new helper in the > > first place. the existing one would work fine shouldn't it ? > > The rough outline of the problems are: > - We need to have buffers that are multiple of cache size, for clearing. > - Today we know ehci-hcd had a problem. We also know other drivers / > layers have problems, but they aren't as readily breakable. > > That's why we put the macro in <common.h> rather than a USB header. that wasn't the question. no one in the tree needs the new macro at all, regardless of what header it lives in. i guess the answer is that some code in the future (which hasn't been merged) might use it. -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/20120721/18a27507/attachment.pgp> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER 2012-07-21 17:22 ` Mike Frysinger @ 2012-07-23 15:24 ` Tom Rini 0 siblings, 0 replies; 21+ messages in thread From: Tom Rini @ 2012-07-23 15:24 UTC (permalink / raw) To: u-boot On Sat, Jul 21, 2012 at 01:22:40PM -0400, Mike Frysinger wrote: > On Friday 20 July 2012 17:50:33 Tom Rini wrote: > > On 07/20/2012 02:47 PM, Mike Frysinger wrote: > > > On Friday 20 July 2012 07:31:47 Marek Vasut wrote: > > >> Dear Mike Frysinger, > > >> > > >>> On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > > >>>> +/* DEFINE_CACHE_ALIGN_BUFFER() is similar to > > >>>> ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow > > >>>> allocating aligned buffers outside of function scope. Usage > > >>>> of this macro shall be avoided or used with extreme care! */ > > >>>> +#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) + static > > >>>> char __##name[roundup(size * sizeof(type), > > >>>> ARCH_DMA_MINALIGN)] + __aligned(ARCH_DMA_MINALIGN); + > > >>>> static type *name = (type *)__##name; > > >>> > > >>> how is this any different from doing: static __u8 foo[1234] > > >>> __aligned(ARCH_DMA_MINALIGN); > > >> > > >> Does __aligned() align both start of the buffer downwards and end > > >> of it upwards ? > > > > > > it guarantees the start is aligned. i don't believe it does any > > > tail padding. > > > > > > that said, you've added just 1 consumer, but it uses in function > > > scope, so i don't see why you had to define a new helper in the > > > first place. the existing one would work fine shouldn't it ? > > > > The rough outline of the problems are: > > - We need to have buffers that are multiple of cache size, for clearing. > > - Today we know ehci-hcd had a problem. We also know other drivers / > > layers have problems, but they aren't as readily breakable. > > > > That's why we put the macro in <common.h> rather than a USB header. > > that wasn't the question. no one in the tree needs the new macro at all, > regardless of what header it lives in. i guess the answer is that some code > in the future (which hasn't been merged) might use it. Er, between drivers/usb/host/ehci-hcd.c and drivers/usb/eth/smsc95xx.c the three new macros are used today. -- Tom ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-07-23 15:24 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-08 3:08 [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Marek Vasut 2012-07-08 3:08 ` [U-Boot] [PATCH 2/2] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment Marek Vasut 2012-07-08 10:04 ` Ilya Yanok 2012-07-08 18:51 ` Marek Vasut 2012-07-08 20:52 ` Tom Rini 2012-07-08 21:36 ` Marek Vasut 2012-07-09 18:37 ` Ilya Yanok 2012-07-10 1:55 ` Marek Vasut 2012-07-14 22:11 ` Ilya Yanok 2012-07-14 22:08 ` Ilya Yanok 2012-07-15 8:07 ` Marek Vasut 2012-07-15 8:55 ` Ilya Yanok 2012-07-15 9:42 ` Marek Vasut 2012-07-08 12:23 ` [U-Boot] [PATCH 1/2] common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER Ilya Yanok 2012-07-08 18:55 ` Marek Vasut 2012-07-20 4:01 ` Mike Frysinger 2012-07-20 11:31 ` Marek Vasut 2012-07-20 21:47 ` Mike Frysinger 2012-07-20 21:50 ` Tom Rini 2012-07-21 17:22 ` Mike Frysinger 2012-07-23 15:24 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.