[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[Lukasz Majewski]

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 <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
 include/cache.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 include/cache.h

diff --git a/include/cache.h b/include/cache.h
new file mode 100644
index 0000000..d06a0ac
--- /dev/null
+++ b/include/cache.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics
+ * ?ukasz Majewski <l.majewski@samsung.com>
+ *
+ * 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_ */
-- 
1.7.2.3

[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[Wolfgang Denk]

Dear Lukasz Majewski,

In message <1314261435-29789-1-git-send-email-l.majewski@samsung.com> 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.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>  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

> +#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.

> +#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

-- 
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 <6bo3fr$pj8$5@csnews.cs.colorado.edu>

[U-Boot] [PATCH v2] dcache: Dcache line size aligned stack buffer allocation

[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 <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
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(-)

diff --git a/include/common.h b/include/common.h
index 12a1074..5bc3af8 100644
--- a/include/common.h
+++ b/include/common.h
@@ -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>
-- 
1.7.2.3

[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[Mike Frysinger]

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 

[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[Anton Staaf]

On Tue, Aug 30, 2011 at 8:44 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> 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)

This seems like a good idea.

Thanks,
    Anton

> -mike
>

[U-Boot] [PATCH v2] dcache: Dcache line size aligned stack buffer allocation

[Anton Staaf]

On Tue, Aug 30, 2011 at 4:45 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using
> stack allocated buffers for DMA transfers.
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> 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(-)
>
> diff --git a/include/common.h b/include/common.h
> index 12a1074..5bc3af8 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -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

> + ? ? ? 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
>
>

[U-Boot] [PATCH v4] dcache: Dcache line size aligned stack buffer allocation

[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 <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
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 <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/common.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/common.h b/include/common.h
index 12a1074..a74c6e8 100644
--- a/include/common.h
+++ b/include/common.h
@@ -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>
-- 
1.7.2.3

[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[Aneesh V]

Hi Mike,

On Tuesday 30 August 2011 09:14 PM, Mike Frysinger wrote:
> 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.

> 
> 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

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

[U-Boot] [PATCH] dcache: Dcache line size aligned stack buffer allocation

[Mike Frysinger]

On Thursday, September 01, 2011 07:13:36 Aneesh V wrote:
> On Tuesday 30 August 2011 09:14 PM, Mike Frysinger wrote:
> > 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
> 
> 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 

[U-Boot] [PATCH v4] dcache: Dcache line size aligned stack buffer allocation

[Anton Staaf]

On Thu, Sep 1, 2011 at 3:30 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>
> ALLOC_CACHE_ALIGN_BUFFER shall be used in functions, which are using
> stack allocated buffers for DMA transfers.
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> 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 <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> ?include/common.h | ? ?5 +++++
> ?1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/include/common.h b/include/common.h
> index 12a1074..a74c6e8 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -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

>
> + ? ? ? 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
>