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

login
register
mail settings
Submitter Łukasz Majewski
Date Aug. 25, 2011, 8:37 a.m.
Message ID <1314261435-29789-1-git-send-email-l.majewski@samsung.com>
Download mbox | patch
Permalink /patch/111519/
State Superseded
Headers show

Comments

Łukasz Majewski - Aug. 25, 2011, 8:37 a.m.
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
Wolfgang Denk - Aug. 25, 2011, 9:34 a.m.
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
Mike Frysinger - Aug. 30, 2011, 3:44 p.m.
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
Anton Staaf - Aug. 30, 2011, 5:14 p.m.
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
>
Aneesh V - Sept. 1, 2011, 11:13 a.m.
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
Mike Frysinger - Sept. 1, 2011, 2:35 p.m.
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

Patch

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_ */