| Message ID | 1314261435-29789-1-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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 >
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
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
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_ */