dcache: Dcache line size aligned stack buffer allocation

PATCH Superseded Lukasz Majewski 5 participants 10 messages Aug 25, 2011 – Sep 2, 2011

Share & tools

Lukasz Majewski

author

2011-08-25 08:37 UTC #0 lore raw

PATCH 1/1 dcache: Dcache line size aligned stack buffer allocation

This commit is defining new include/cache.h file, which defines macro needed for cache aligned buffers.
ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using stack allocated buffers for DMA transfers.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

CC Albert ARIBAUD <[email protected]>

---
 include/cache.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 include/cache.h

include/cache.h new file +42 -0

new file mode 100644
index 0000000..d06a0ac
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics
+ * ?ukasz Majewski <[email protected]>
+ *
+ * Configuation settings for the SAMSUNG Universal (s5pc100) board.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef __GENERIC_CACHE_H_
+#define __GENERIC_CACHE_H_
+
+#if defined(CONFIG_SYS_CACHELINE_SIZE) && !defined(CONFIG_SYS_DCACHE_OFF)
+#define ALIGN_ADDR(addr) ((void *)(((unsigned long) addr + \
+				    CONFIG_SYS_CACHELINE_SIZE - 1)  \
+				   & ~(CONFIG_SYS_CACHELINE_SIZE - 1)))
+
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) \
+	char *__##name[size + CONFIG_SYS_CACHELINE_SIZE]; \
+	type *name = ALIGN_ADDR(__##name);
+#else
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) \
+	type name[size];
+#endif
+
+#endif  /* __GENERIC_CACHE_H_ */

Signature · 1 line

1.7.2.3

Wolfgang Denk

2011-08-25 09:34 UTC #1 lore raw

PATCH 1/1 dcache: Dcache line size aligned stack buffer allocation

Dear Lukasz Majewski,

Quoted 5 lines · In message <[email protected]> you wrote:

This commit is defining new include/cache.h file, which defines macro needed for cache aligned buffers.
ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using stack allocated buffers for DMA transfers.

Quoted 7 lines

Signed-off-by Lukasz Majewski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
CC Albert ARIBAUD <[email protected]>
---
 include/cache.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 include/cache.h

I don't think it makes sense to create a new header file just for this macro. Please add this to an existing header file instead; if no better place is found even to common.h

Quoted 1 line

+#if defined(CONFIG_SYS_CACHELINE_SIZE) && !defined(CONFIG_SYS_DCACHE_OFF)

Please omit this #ifdef.

CONFIG_SYS_CACHELINE_SIZE is a mandatory #define, and it's OK that a build breaks when it's missing. On the other hand I don;t se why this macro needs top be removed when the data cache is off.

include/cache.h line 33

Quoted 4 lines

include/cache.h new file +4 -0

+#define ALIGN_ADDR(addr) ((void *)(((unsigned long) addr + \
+				    CONFIG_SYS_CACHELINE_SIZE - 1)  \
+				   & ~(CONFIG_SYS_CACHELINE_SIZE - 1)))
+

This is not needed. common.h defines ALIGN() which should be sufficient here.

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of Microsoft crippleware systems is a sin that  carries  with
it its own punishment.
         -- Tom Christiansen in <[email protected]>

Mike Frysinger

2011-08-30 15:44 UTC #2 lore raw

PATCH 1/1 dcache: Dcache line size aligned stack buffer allocation

Quoted 3 lines · On Thursday, August 25, 2011 05:34:00 Wolfgang Denk wrote:

CONFIG_SYS_CACHELINE_SIZE is a mandatory #define, and it's OK that a build breaks when it's missing. On the other hand I don;t se why this macro needs top be removed when the data cache is off.

i guess a lot of arch people will need to post updates. this seems to be available for all ppc and mips peeps, and one arm soc. everyone else gets a fun build fail.

however, cacheline size is an aspect of the cpu core and doesnt really make sense as a board config. even the ppc header hints at this:
/*
* For compatibility reasons support the CONFIG_SYS_CACHELINE_SIZE too */
#ifndef CONFIG_SYS_CACHELINE_SIZE
#define CONFIG_SYS_CACHELINE_SIZE L1_CACHE_BYTES
#endif

so my proposal is to migrate away from CONFIG_SYS_CACHELINE_SIZE and to the API that Linux has adopted:
asm/cache.h: define L1_CACHE_BYTES, L1_CACHE_SHIFT, and ARCH_DMA_MINALIGN

then we build ALLOC_CACHE_ALIGN_BUFFER() on top of the ARCH_DMA_MINALIGN define (since that's the point of that define in the first place)
-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/20110830/a2b7ea45/attachment.pgp

Anton Staaf

2011-08-30 17:14 UTC #3 lore raw

PATCH 1/1 dcache: Dcache line size aligned stack buffer allocation

Quoted 18 lines · Mike Frysinger

Quoted 3 lines · On Thursday, August 25, 2011 05:34:00 Wolfgang Denk wrote:
CONFIG_SYS_CACHELINE_SIZE is a mandatory #define, and it's OK that a build breaks when it's missing. ?On the other hand I don;t se why this macro needs top be removed when the data cache is off.
i guess a lot of arch people will need to post updates. ?this seems to be available for all ppc and mips peeps, and one arm soc. ?everyone else gets a fun build fail.
however, cacheline size is an aspect of the cpu core and doesnt really make sense as a board config. ?even the ppc header hints at this:
/*
?* For compatibility reasons support the CONFIG_SYS_CACHELINE_SIZE too ?*/
#ifndef CONFIG_SYS_CACHELINE_SIZE
#define CONFIG_SYS_CACHELINE_SIZE ? L1_CACHE_BYTES
#endif

Quoted 6 lines

so my proposal is to migrate away from CONFIG_SYS_CACHELINE_SIZE and to the API that Linux has adopted:
asm/cache.h: define L1_CACHE_BYTES, L1_CACHE_SHIFT, and ARCH_DMA_MINALIGN
then we build ALLOC_CACHE_ALIGN_BUFFER() on top of the ARCH_DMA_MINALIGN define (since that's the point of that define in the first place)

This seems like a good idea.

Thanks,
Anton

Quoted 2 lines

-mike

Aneesh V

2011-09-01 11:13 UTC #4 lore raw

PATCH 1/1 dcache: Dcache line size aligned stack buffer allocation

In reply to Mike Frysinger

Hi Mike,

Quoted 8 lines · Mike Frysinger

Quoted 3 lines · On Thursday, August 25, 2011 05:34:00 Wolfgang Denk wrote:
CONFIG_SYS_CACHELINE_SIZE is a mandatory #define, and it's OK that a build breaks when it's missing. On the other hand I don;t se why this macro needs top be removed when the data cache is off.
i guess a lot of arch people will need to post updates. this seems to be available for all ppc and mips peeps, and one arm soc. everyone else gets a fun build fail.

That's indeed a problem.

Quoted 10 lines

however, cacheline size is an aspect of the cpu core and doesnt really make sense as a board config. even the ppc header hints at this:
/*
* For compatibility reasons support the CONFIG_SYS_CACHELINE_SIZE too */
#ifndef CONFIG_SYS_CACHELINE_SIZE
#define CONFIG_SYS_CACHELINE_SIZE L1_CACHE_BYTES
#endif

Quoted 3 lines

so my proposal is to migrate away from CONFIG_SYS_CACHELINE_SIZE and to the API that Linux has adopted:
asm/cache.h: define L1_CACHE_BYTES, L1_CACHE_SHIFT, and ARCH_DMA_MINALIGN

Not sure how this will work though. Cache-line is not same for ARM architectures or even sub-architectures. For instance Cortex-A8 and Cortex-A9(both armv7) have different cache-line sizes. So, asm/cache.h should probably have something like:

#ifdef CONFIG_CORTEXA8
#define L1_CACHE_BYTES x
...
#elif CONFIG_CORTEXA9
...
#endif

Even this wouldn't work straight away because use of flags such as CONFIG_CORTEXA8 is also not standard. But, that may be a better thing to fix than adding CONFIG_SYS_CACHELINE_SIZE in all the board config files.

Am I missing something?

best regards,
Aneesh

Mike Frysinger

2011-09-01 14:35 UTC #5 lore raw

PATCH 1/1 dcache: Dcache line size aligned stack buffer allocation

Quoted 12 lines · On Thursday, September 01, 2011 07:13:36 Aneesh V wrote:

Quoted 11 lines · Mike Frysinger
however, cacheline size is an aspect of the cpu core and doesnt really make sense as a board config. even the ppc header hints at this:
/*
* For compatibility reasons support the CONFIG_SYS_CACHELINE_SIZE too */
#ifndef CONFIG_SYS_CACHELINE_SIZE
#define CONFIG_SYS_CACHELINE_SIZE L1_CACHE_BYTES
#endif

Quoted 6 lines

Quoted 3 lines
so my proposal is to migrate away from CONFIG_SYS_CACHELINE_SIZE and to the API that Linux has adopted:
asm/cache.h: define L1_CACHE_BYTES, L1_CACHE_SHIFT, and ARCH_DMA_MINALIGN
Not sure how this will work though. Cache-line is not same for ARM architectures or even sub-architectures.

each arch is responsible for making sure the right value bubbles up. if that means they have to tail into asm/arch/cache.h, that's the arch's problem.

keep in mind, this is the API already in use by Linux, so they must have solved the issue there for the pile of SoC's they support (and i'm fairly certain they support just as many as us if not more).
-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/20110901/49ec29d8/attachment.pgp

Lukasz Majewski

author

2011-08-30 11:45 UTC #6 lore raw

PATCH v2 1/1 dcache: Dcache line size aligned stack buffer allocation

In reply to Lukasz Majewski

ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using stack allocated buffers for DMA transfers.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

CC Albert ARIBAUD <[email protected]>

---
Changes for v2:
- ./include/cache.h has been removed and replaced with simpler macro added to ./include/common.h

---
 include/common.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

include/common.h +5 -0

index 12a1074..5bc3af8 100644
@@ -767,6 +767,11 @@ int cpu_release(int nr, int argc, char * const argv[]);
 #define ALIGN(x,a)		__ALIGN_MASK((x),(typeof(x))(a)-1)
 #define __ALIGN_MASK(x,mask)	(((x)+(mask))&~(mask))
 
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) \
+	char *__##name[size + CONFIG_SYS_CACHELINE_SIZE]; \
+	type *name = ALIGN(((typeof(CONFIG_SYS_CACHELINE_SIZE))(__##name)),\
+			   (CONFIG_SYS_CACHELINE_SIZE));
+
 /* Pull in stuff for the build system */
 #ifdef DO_DEPS_ONLY
 # include <environment.h>

Signature · 1 line

1.7.2.3

Anton Staaf

2011-08-30 17:15 UTC #7 lore raw

PATCH v2 1/1 dcache: Dcache line size aligned stack buffer allocation

Quoted 19 lines · Lukasz Majewski

ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using stack allocated buffers for DMA transfers.
Signed-off-by Lukasz Majewski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
CC Albert ARIBAUD <[email protected]>
---
Changes for v2:
? ? ? ?- ./include/cache.h has been removed and replaced with ? ? ? ?simpler macro added to ./include/common.h
---
?include/common.h | ? ?5 +++++
?1 files changed, 5 insertions(+), 0 deletions(-)
include/common.h +0 -0
index 12a1074..5bc3af8 100644

include/common.h int cpu_release(int nr, int argc, char * const argv[]); · line 771

Quoted 5 lines

include/common.h +2 -0

@@ -767,6 +767,11 @@ int cpu_release(int nr, int argc, char * const argv[]);
 ?#define ALIGN(x,a) ? ? ? ? ? ? __ALIGN_MASK((x),(typeof(x))(a)-1)
 ?#define __ALIGN_MASK(x,mask) ? (((x)+(mask))&~(mask))
 
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) \
+ ? ? ? char *__##name[size + CONFIG_SYS_CACHELINE_SIZE]; \

This should be a "char" array, not a "char *" array. And the size should be "size + CONFIG_SYS_CACHELINE_SIZE - 1".

Thanks,
Anton

Quoted 10 lines

include/common.h +3 -0
+ ? ? ? type *name = ALIGN(((typeof(CONFIG_SYS_CACHELINE_SIZE))(__##name)),\
+ ? ? ? ? ? ? ? ? ? ? ? ? ?(CONFIG_SYS_CACHELINE_SIZE));
+
?/* Pull in stuff for the build system */
?#ifdef DO_DEPS_ONLY
?# include <environment.h>
--
1.7.2.3

Lukasz Majewski

author

2011-09-01 10:30 UTC #8 lore raw

PATCH v4 1/1 dcache: Dcache line size aligned stack buffer allocation

In reply to Lukasz Majewski

ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using stack allocated buffers for DMA transfers.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

CC Albert ARIBAUD <[email protected]>

---
Changes for v2:
- ./include/cache.h has been removed and replaced with simpler macro added to ./include/common.h
Changes for v3:
- change char * to char
- defined table size definition
Changes for v4:
- (type*) added for compiler warning fix

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 include/common.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

include/common.h +5 -0

index 12a1074..a74c6e8 100644
@@ -767,6 +767,11 @@ int cpu_release(int nr, int argc, char * const argv[]);
 #define ALIGN(x,a)		__ALIGN_MASK((x),(typeof(x))(a)-1)
 #define __ALIGN_MASK(x,mask)	(((x)+(mask))&~(mask))
 
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) \
+	char __##name[size + CONFIG_SYS_CACHELINE_SIZE - 1]; \
+	type *name = (type *)  ALIGN(((typeof(CONFIG_SYS_CACHELINE_SIZE))\
+				     (__##name)), (CONFIG_SYS_CACHELINE_SIZE));
+
 /* Pull in stuff for the build system */
 #ifdef DO_DEPS_ONLY
 # include <environment.h>

Signature · 1 line

1.7.2.3

Anton Staaf

2011-09-02 23:29 UTC #9 lore raw

PATCH v4 1/1 dcache: Dcache line size aligned stack buffer allocation

Quoted 28 lines · Lukasz Majewski

ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using stack allocated buffers for DMA transfers.
Signed-off-by Lukasz Majewski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
CC Albert ARIBAUD <[email protected]>
---
Changes for v2:
? ? ? ?- ./include/cache.h has been removed and replaced with ? ? ? ?simpler macro added to ./include/common.h Changes for v3:
? ? ? ?- change char * to char
? ? ? ?- defined table size definition
Changes for v4:
? ? ? ?- (type*) added for compiler warning fix
Signed-off-by Lukasz Majewski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
?include/common.h | ? ?5 +++++
?1 files changed, 5 insertions(+), 0 deletions(-)
include/common.h +0 -0
index 12a1074..a74c6e8 100644

include/common.h int cpu_release(int nr, int argc, char * const argv[]); · line 771

Quoted 5 lines

include/common.h +2 -0

@@ -767,6 +767,11 @@ int cpu_release(int nr, int argc, char * const argv[]);
 ?#define ALIGN(x,a) ? ? ? ? ? ? __ALIGN_MASK((x),(typeof(x))(a)-1)
 ?#define __ALIGN_MASK(x,mask) ? (((x)+(mask))&~(mask))
 
+#define ALLOC_CACHE_ALIGN_BUFFER(type, name, size) \
+ ? ? ? char __##name[size + CONFIG_SYS_CACHELINE_SIZE - 1]; \

It was pointed out to me that we need to make sure that both ends of the resulting buffer are cache line aligned. ?Or put another way, that the __##name array has enough padding at the beginning and end that an invalidate will be both aligned to the cache line and not effect anything defined after the array on the stack. ?So the above definition needs to change to something like:

char __##name[ROUND(size, CONFIG_SYS_CACHELINE_SIZE) ?+ CONFIG_SYS_CACHELINE_SIZE - 1];

Another thing that concerns me is that the macro takes a type, but the size parameter is specified in bytes, not units of the size of the type. ?Would it make sense to specify the size in units of the type? It would make almost no sense to specify a size that wasn't a multiple of the size of the type anyway. If we want to do that the the array definition becomes:

char __##name[ROUND(size * sizeof(type), CONFIG_SYS_CACHELINE_SIZE) ?+ CONFIG_SYS_CACHELINE_SIZE - 1];

And finally, the ROUND macro is written such that it will always return a value that is larger than it's first parameter. Thus ROUND(CONFIG_SYS_CACHELINE_SIZE, CONFIG_SYS_CACHELINE_SIZE) withh not equal CONFIG_SYS_CACHELINE_SIZE, but actually 2 * CONFIG_SYS_CACHELINE_SIZE. I'm not sure if this is intentional. In fact, the only use of ROUND that is not to round the value of CONFIG_SYS_MALLOC_LEN to a multiple of 4096 is in the common/cmd_sf.c implementation. And there it looks like the author worked around the behavior of ROUND by passing "len_arg - 1", instead of len_arg. So, it looks like a patch to fix ROUND might be in order as well. I'll try and send one today.

-Anton

Quoted 10 lines

include/common.h +3 -0
+ ? ? ? type *name = (type *) ?ALIGN(((typeof(CONFIG_SYS_CACHELINE_SIZE))\
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(__##name)), (CONFIG_SYS_CACHELINE_SIZE));
+
?/* Pull in stuff for the build system */
?#ifdef DO_DEPS_ONLY
?# include <environment.h>
--
1.7.2.3