| Message ID | 1313137442-8378-1-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Lukasz, On 12/08/2011 10:24, Lukasz Majewski wrote: > This commit adds support for reading the D cache line size for armv7 > architecture. > > The get_dcache_line_size() function is supposed to work in conjunction > with memalign call to provide D cache aligned DMA buffers. > > Signed-off-by: Lukasz Majewski<l.majewski@samsung.com> > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > CC: Aneesh V<aneesh@ti.com> > CC: Albert ARIBAUD<albert.u.boot@aribaud.net> > --- > arch/arm/cpu/armv7/cache_v7.c | 25 +++++++++++++++++++++++++ > include/common.h | 1 + > 2 files changed, 26 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c > index 3e1e1bf..bd0b1a8 100644 > --- a/arch/arm/cpu/armv7/cache_v7.c > +++ b/arch/arm/cpu/armv7/cache_v7.c > @@ -246,6 +246,21 @@ static void v7_inval_tlb(void) > CP15ISB; > } > > +/* Read the cache line size (bytes) */ > +static int v7_dcache_line_size(void) > +{ > + u32 ccsidr, log2_line_len; > + > + ccsidr = get_ccsidr(); > + > + log2_line_len = ((ccsidr& CCSIDR_LINE_SIZE_MASK)>> > + CCSIDR_LINE_SIZE_OFFSET) + 2; > + /* Converting from words to bytes */ > + log2_line_len += 2; > + > + return 1<< log2_line_len; > +} > + > void invalidate_dcache_all(void) > { > v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); > @@ -303,6 +318,11 @@ void flush_cache(unsigned long start, unsigned long size) > { > flush_dcache_range(start, start + size); > } > + > +int get_dcache_line_size(void) > +{ > + return v7_dcache_line_size(); > +} > #else /* #ifndef CONFIG_SYS_DCACHE_OFF */ > void invalidate_dcache_all(void) > { > @@ -327,6 +347,11 @@ void arm_init_before_mmu(void) > void flush_cache(unsigned long start, unsigned long size) > { > } > + > +int get_dcache_line_size(void) > +{ > + return 0; > +} > #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */ > > #ifndef CONFIG_SYS_ICACHE_OFF > diff --git a/include/common.h b/include/common.h > index 12a1074..b535300 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -622,6 +622,7 @@ void flush_dcache_range(unsigned long start, unsigned long stop); > void invalidate_dcache_range(unsigned long start, unsigned long stop); > void invalidate_dcache_all(void); > void invalidate_icache_all(void); > +int get_dcache_line_size(void); > > /* arch/$(ARCH)/lib/ticks.S */ > unsigned long long get_ticks(void); I like the idea of a global cache line accessor, but: 1) Does it have to be a function? I don't know cache implementations where line size is not a constant, and a function might be overkill. 2) If it has to be a function, then I'd rather have the function introduced in arch/arm/lib/cache.c as a weak function returning 32, with armv7 providing a replacement. Aneesh, that patch could affect your patch series as the magic 32 could then come from this weak function rather than be hard-coded. Amicalement,
On Friday 12 August 2011 01:54 PM, Lukasz Majewski wrote: > This commit adds support for reading the D cache line size for armv7 > architecture. > > The get_dcache_line_size() function is supposed to work in conjunction > with memalign call to provide D cache aligned DMA buffers. > > Signed-off-by: Lukasz Majewski<l.majewski@samsung.com> > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > CC: Aneesh V<aneesh@ti.com> > CC: Albert ARIBAUD<albert.u.boot@aribaud.net> > --- > arch/arm/cpu/armv7/cache_v7.c | 25 +++++++++++++++++++++++++ > include/common.h | 1 + > 2 files changed, 26 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c > index 3e1e1bf..bd0b1a8 100644 > --- a/arch/arm/cpu/armv7/cache_v7.c > +++ b/arch/arm/cpu/armv7/cache_v7.c > @@ -246,6 +246,21 @@ static void v7_inval_tlb(void) > CP15ISB; > } > > +/* Read the cache line size (bytes) */ > +static int v7_dcache_line_size(void) > +{ > + u32 ccsidr, log2_line_len; > + > + ccsidr = get_ccsidr(); > + > + log2_line_len = ((ccsidr& CCSIDR_LINE_SIZE_MASK)>> > + CCSIDR_LINE_SIZE_OFFSET) + 2; > + /* Converting from words to bytes */ > + log2_line_len += 2; > + > + return 1<< log2_line_len; > +} > + > void invalidate_dcache_all(void) > { > v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); > @@ -303,6 +318,11 @@ void flush_cache(unsigned long start, unsigned long size) > { > flush_dcache_range(start, start + size); > } > + > +int get_dcache_line_size(void) > +{ > + return v7_dcache_line_size(); > +} > #else /* #ifndef CONFIG_SYS_DCACHE_OFF */ > void invalidate_dcache_all(void) > { > @@ -327,6 +347,11 @@ void arm_init_before_mmu(void) > void flush_cache(unsigned long start, unsigned long size) > { > } > + > +int get_dcache_line_size(void) > +{ > + return 0; > +} Does memalign() take 0 as input and if so what is the output. The GNU C library seems to mandate only that it's a power of 2. But the following man page seems to says that "The value of alignment must be a power of two and must be greater than or equal to the size of a word." http://www.s-gms.ms.edus.si/cgi-bin/man-cgi?memalign+3C Also, you seem to be using get_dcache_line_size() in MMC driver. Is this function defined for all platforms. Maybe, you must implement a weakly linked default function for all platforms? best regards, Aneesh
Hi Albert, Lukasz, On Friday 12 August 2011 02:28 PM, Albert ARIBAUD wrote: > Hi Lukasz, > > On 12/08/2011 10:24, Lukasz Majewski wrote: >> This commit adds support for reading the D cache line size for armv7 >> architecture. >> >> The get_dcache_line_size() function is supposed to work in conjunction >> with memalign call to provide D cache aligned DMA buffers. >> >> Signed-off-by: Lukasz Majewski<l.majewski@samsung.com> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >> CC: Aneesh V<aneesh@ti.com> >> CC: Albert ARIBAUD<albert.u.boot@aribaud.net> >> --- >> arch/arm/cpu/armv7/cache_v7.c | 25 +++++++++++++++++++++++++ >> include/common.h | 1 + >> 2 files changed, 26 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/cache_v7.c >> b/arch/arm/cpu/armv7/cache_v7.c >> index 3e1e1bf..bd0b1a8 100644 >> --- a/arch/arm/cpu/armv7/cache_v7.c >> +++ b/arch/arm/cpu/armv7/cache_v7.c >> @@ -246,6 +246,21 @@ static void v7_inval_tlb(void) >> CP15ISB; >> } >> >> +/* Read the cache line size (bytes) */ >> +static int v7_dcache_line_size(void) >> +{ >> + u32 ccsidr, log2_line_len; >> + >> + ccsidr = get_ccsidr(); >> + >> + log2_line_len = ((ccsidr& CCSIDR_LINE_SIZE_MASK)>> >> + CCSIDR_LINE_SIZE_OFFSET) + 2; >> + /* Converting from words to bytes */ >> + log2_line_len += 2; >> + >> + return 1<< log2_line_len; >> +} >> + >> void invalidate_dcache_all(void) >> { >> v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); >> @@ -303,6 +318,11 @@ void flush_cache(unsigned long start, unsigned >> long size) >> { >> flush_dcache_range(start, start + size); >> } >> + >> +int get_dcache_line_size(void) >> +{ >> + return v7_dcache_line_size(); >> +} >> #else /* #ifndef CONFIG_SYS_DCACHE_OFF */ >> void invalidate_dcache_all(void) >> { >> @@ -327,6 +347,11 @@ void arm_init_before_mmu(void) >> void flush_cache(unsigned long start, unsigned long size) >> { >> } >> + >> +int get_dcache_line_size(void) >> +{ >> + return 0; >> +} >> #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */ >> >> #ifndef CONFIG_SYS_ICACHE_OFF >> diff --git a/include/common.h b/include/common.h >> index 12a1074..b535300 100644 >> --- a/include/common.h >> +++ b/include/common.h >> @@ -622,6 +622,7 @@ void flush_dcache_range(unsigned long start, >> unsigned long stop); >> void invalidate_dcache_range(unsigned long start, unsigned long stop); >> void invalidate_dcache_all(void); >> void invalidate_icache_all(void); >> +int get_dcache_line_size(void); >> >> /* arch/$(ARCH)/lib/ticks.S */ >> unsigned long long get_ticks(void); > > I like the idea of a global cache line accessor, but: > > 1) Does it have to be a function? I don't know cache implementations > where line size is not a constant, and a function might be overkill. > > 2) If it has to be a function, then I'd rather have the function > introduced in arch/arm/lib/cache.c as a weak function returning 32, with > armv7 providing a replacement. Since this is used from generic drivers I think this should be defined all ARCHs. How about something like this. int __weak get_dcache_line_size(void) { #if defined(CONFIG_SYS_CACHE_LINE_SIZE) && !defined(CONFIG_SYS_DCACHE_OFF) return CONFIG_SYS_CACHE_LINE_SIZE; #else return sizeof(int); /* or 0 - whatever is appropriate for memalign() */ #endif } Then over-ride this from the respective ARCH implementation if there is a cache-type registers. I see that armv5/6/7 all can find this at run-time from CP15 registers. > > Aneesh, that patch could affect your patch series as the magic 32 could > then come from this weak function rather than be hard-coded. I think you are referring to Hong's series. In armv7 series we are getting the size by reading ccsidr register just like Lukasz is doing here - no magic values. I think in Hong's series also this can be found from the cache type CP15 register. br, Aneesh
Hi Anessh, On Fri, 12 Aug 2011 14:32:52 +0530 Aneesh V <aneesh@ti.com> wrote: > > + > > +int get_dcache_line_size(void) > > +{ > > + return 0; > > +} > > Does memalign() take 0 as input and if so what is the output. The GNU > C library seems to mandate only that it's a power of 2. But the > following man page seems to says that "The value of alignment must be > a power of two and must be greater than or equal to the size of a > word." > > http://www.s-gms.ms.edus.si/cgi-bin/man-cgi?memalign+3C Documentation is one thing, and I was initially planning to return 8 when the CONFIG_DCACHE_OFF is defined (when dcache is disabled). But then I've looked in the implementation of this function (common/dlmalloc.c) and discover, that it's smallest alignment is 8 bytes. When number passed to memalign is smaller than 8, then anyway the alignment is set to this default value. On the other hand I've realized than it is more natural to return 0 than 8, and assumed that this will not harm the current implementation. For the sake of documentation conformance, the value of 8 shall be returned in this case. > > Also, you seem to be using get_dcache_line_size() in MMC driver. Is > this function defined for all platforms. Maybe, you must implement a > weakly linked default function for all platforms? > > best regards, > Aneesh Yes, you are right, now the get_dcache_line_size() is defined in the arch/arm/cpu/armv7/cache_v7.c, so other boards will be broken. Next version will use the weakly linked default function.
(Added Hong Xu) Hi Aneesh, On 12/08/2011 11:16, Aneesh V wrote: > Hi Albert, Lukasz, > > On Friday 12 August 2011 02:28 PM, Albert ARIBAUD wrote: >> Hi Lukasz, >> >> On 12/08/2011 10:24, Lukasz Majewski wrote: >>> This commit adds support for reading the D cache line size for armv7 >>> architecture. >>> >>> The get_dcache_line_size() function is supposed to work in conjunction >>> with memalign call to provide D cache aligned DMA buffers. >>> >>> Signed-off-by: Lukasz Majewski<l.majewski@samsung.com> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >>> CC: Aneesh V<aneesh@ti.com> >>> CC: Albert ARIBAUD<albert.u.boot@aribaud.net> >>> --- >>> arch/arm/cpu/armv7/cache_v7.c | 25 +++++++++++++++++++++++++ >>> include/common.h | 1 + >>> 2 files changed, 26 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv7/cache_v7.c >>> b/arch/arm/cpu/armv7/cache_v7.c >>> index 3e1e1bf..bd0b1a8 100644 >>> --- a/arch/arm/cpu/armv7/cache_v7.c >>> +++ b/arch/arm/cpu/armv7/cache_v7.c >>> @@ -246,6 +246,21 @@ static void v7_inval_tlb(void) >>> CP15ISB; >>> } >>> >>> +/* Read the cache line size (bytes) */ >>> +static int v7_dcache_line_size(void) >>> +{ >>> + u32 ccsidr, log2_line_len; >>> + >>> + ccsidr = get_ccsidr(); >>> + >>> + log2_line_len = ((ccsidr& CCSIDR_LINE_SIZE_MASK)>> >>> + CCSIDR_LINE_SIZE_OFFSET) + 2; >>> + /* Converting from words to bytes */ >>> + log2_line_len += 2; >>> + >>> + return 1<< log2_line_len; >>> +} >>> + >>> void invalidate_dcache_all(void) >>> { >>> v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); >>> @@ -303,6 +318,11 @@ void flush_cache(unsigned long start, unsigned >>> long size) >>> { >>> flush_dcache_range(start, start + size); >>> } >>> + >>> +int get_dcache_line_size(void) >>> +{ >>> + return v7_dcache_line_size(); >>> +} >>> #else /* #ifndef CONFIG_SYS_DCACHE_OFF */ >>> void invalidate_dcache_all(void) >>> { >>> @@ -327,6 +347,11 @@ void arm_init_before_mmu(void) >>> void flush_cache(unsigned long start, unsigned long size) >>> { >>> } >>> + >>> +int get_dcache_line_size(void) >>> +{ >>> + return 0; >>> +} >>> #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */ >>> >>> #ifndef CONFIG_SYS_ICACHE_OFF >>> diff --git a/include/common.h b/include/common.h >>> index 12a1074..b535300 100644 >>> --- a/include/common.h >>> +++ b/include/common.h >>> @@ -622,6 +622,7 @@ void flush_dcache_range(unsigned long start, >>> unsigned long stop); >>> void invalidate_dcache_range(unsigned long start, unsigned long stop); >>> void invalidate_dcache_all(void); >>> void invalidate_icache_all(void); >>> +int get_dcache_line_size(void); >>> >>> /* arch/$(ARCH)/lib/ticks.S */ >>> unsigned long long get_ticks(void); >> >> I like the idea of a global cache line accessor, but: >> >> 1) Does it have to be a function? I don't know cache implementations >> where line size is not a constant, and a function might be overkill. >> >> 2) If it has to be a function, then I'd rather have the function >> introduced in arch/arm/lib/cache.c as a weak function returning 32, with >> armv7 providing a replacement. > > Since this is used from generic drivers I think this should be defined > all ARCHs. How about something like this. > > int __weak get_dcache_line_size(void) > { > #if defined(CONFIG_SYS_CACHE_LINE_SIZE) && !defined(CONFIG_SYS_DCACHE_OFF) > return CONFIG_SYS_CACHE_LINE_SIZE; > #else > return sizeof(int); /* or 0 - whatever is appropriate for memalign() */ > #endif > } > > Then over-ride this from the respective ARCH implementation if there is > a cache-type registers. I see that armv5/6/7 all can find this at > run-time from CP15 registers. > >> >> Aneesh, that patch could affect your patch series as the magic 32 could >> then come from this weak function rather than be hard-coded. > > I think you are referring to Hong's series. In armv7 series we are > getting the size by reading ccsidr register just like Lukasz is doing > here - no magic values. I think in Hong's series also this can be found > from the cache type CP15 register. Correct, I misattributed the patch. As for the cache line size, I see one reason for using a constant rather than dynamically finding it: aligning non-dynamic (either static local or global) buffers. If there are any in the code, they'll need an __attribute__((aligned())) directive, which can only take a constant. OTOH, maybe they could be changed to dynamic buffers allocated at init time, which would make the handling of cache_aligned buffers homogeneous. Amicalement,
On Fri, Aug 12, 2011 at 2:16 AM, Aneesh V <aneesh@ti.com> wrote: > Hi Albert, Lukasz, > > On Friday 12 August 2011 02:28 PM, Albert ARIBAUD wrote: >> Hi Lukasz, >> >> On 12/08/2011 10:24, Lukasz Majewski wrote: >>> This commit adds support for reading the D cache line size for armv7 >>> architecture. >>> >>> The get_dcache_line_size() function is supposed to work in conjunction >>> with memalign call to provide D cache aligned DMA buffers. >>> >>> Signed-off-by: Lukasz Majewski<l.majewski@samsung.com> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >>> CC: Aneesh V<aneesh@ti.com> >>> CC: Albert ARIBAUD<albert.u.boot@aribaud.net> >>> --- >>> arch/arm/cpu/armv7/cache_v7.c | 25 +++++++++++++++++++++++++ >>> include/common.h | 1 + >>> 2 files changed, 26 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv7/cache_v7.c >>> b/arch/arm/cpu/armv7/cache_v7.c >>> index 3e1e1bf..bd0b1a8 100644 >>> --- a/arch/arm/cpu/armv7/cache_v7.c >>> +++ b/arch/arm/cpu/armv7/cache_v7.c >>> @@ -246,6 +246,21 @@ static void v7_inval_tlb(void) >>> CP15ISB; >>> } >>> >>> +/* Read the cache line size (bytes) */ >>> +static int v7_dcache_line_size(void) >>> +{ >>> + u32 ccsidr, log2_line_len; >>> + >>> + ccsidr = get_ccsidr(); >>> + >>> + log2_line_len = ((ccsidr& CCSIDR_LINE_SIZE_MASK)>> >>> + CCSIDR_LINE_SIZE_OFFSET) + 2; >>> + /* Converting from words to bytes */ >>> + log2_line_len += 2; >>> + >>> + return 1<< log2_line_len; >>> +} >>> + >>> void invalidate_dcache_all(void) >>> { >>> v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); >>> @@ -303,6 +318,11 @@ void flush_cache(unsigned long start, unsigned >>> long size) >>> { >>> flush_dcache_range(start, start + size); >>> } >>> + >>> +int get_dcache_line_size(void) >>> +{ >>> + return v7_dcache_line_size(); >>> +} >>> #else /* #ifndef CONFIG_SYS_DCACHE_OFF */ >>> void invalidate_dcache_all(void) >>> { >>> @@ -327,6 +347,11 @@ void arm_init_before_mmu(void) >>> void flush_cache(unsigned long start, unsigned long size) >>> { >>> } >>> + >>> +int get_dcache_line_size(void) >>> +{ >>> + return 0; >>> +} >>> #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */ >>> >>> #ifndef CONFIG_SYS_ICACHE_OFF >>> diff --git a/include/common.h b/include/common.h >>> index 12a1074..b535300 100644 >>> --- a/include/common.h >>> +++ b/include/common.h >>> @@ -622,6 +622,7 @@ void flush_dcache_range(unsigned long start, >>> unsigned long stop); >>> void invalidate_dcache_range(unsigned long start, unsigned long stop); >>> void invalidate_dcache_all(void); >>> void invalidate_icache_all(void); >>> +int get_dcache_line_size(void); >>> >>> /* arch/$(ARCH)/lib/ticks.S */ >>> unsigned long long get_ticks(void); >> >> I like the idea of a global cache line accessor, but: >> >> 1) Does it have to be a function? I don't know cache implementations >> where line size is not a constant, and a function might be overkill. >> >> 2) If it has to be a function, then I'd rather have the function >> introduced in arch/arm/lib/cache.c as a weak function returning 32, with >> armv7 providing a replacement. > > Since this is used from generic drivers I think this should be defined > all ARCHs. How about something like this. > > int __weak get_dcache_line_size(void) > { > #if defined(CONFIG_SYS_CACHE_LINE_SIZE) && !defined(CONFIG_SYS_DCACHE_OFF) > return CONFIG_SYS_CACHE_LINE_SIZE; > #else > return sizeof(int); /* or 0 - whatever is appropriate for memalign() */ It might be better here to return __BIGGEST_ALIGNMENT__ which will always be defined by GCC to be the largest alignment requirement for any data type on the current machine. -Anton > #endif > } > > Then over-ride this from the respective ARCH implementation if there is > a cache-type registers. I see that armv5/6/7 all can find this at > run-time from CP15 registers. > >> >> Aneesh, that patch could affect your patch series as the magic 32 could >> then come from this weak function rather than be hard-coded. > > I think you are referring to Hong's series. In armv7 series we are > getting the size by reading ccsidr register just like Lukasz is doing > here - no magic values. I think in Hong's series also this can be found > from the cache type CP15 register. > > br, > Aneesh > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c index 3e1e1bf..bd0b1a8 100644 --- a/arch/arm/cpu/armv7/cache_v7.c +++ b/arch/arm/cpu/armv7/cache_v7.c @@ -246,6 +246,21 @@ static void v7_inval_tlb(void) CP15ISB; } +/* Read the cache line size (bytes) */ +static int v7_dcache_line_size(void) +{ + u32 ccsidr, log2_line_len; + + ccsidr = get_ccsidr(); + + log2_line_len = ((ccsidr & CCSIDR_LINE_SIZE_MASK) >> + CCSIDR_LINE_SIZE_OFFSET) + 2; + /* Converting from words to bytes */ + log2_line_len += 2; + + return 1 << log2_line_len; +} + void invalidate_dcache_all(void) { v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL); @@ -303,6 +318,11 @@ void flush_cache(unsigned long start, unsigned long size) { flush_dcache_range(start, start + size); } + +int get_dcache_line_size(void) +{ + return v7_dcache_line_size(); +} #else /* #ifndef CONFIG_SYS_DCACHE_OFF */ void invalidate_dcache_all(void) { @@ -327,6 +347,11 @@ void arm_init_before_mmu(void) void flush_cache(unsigned long start, unsigned long size) { } + +int get_dcache_line_size(void) +{ + return 0; +} #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */ #ifndef CONFIG_SYS_ICACHE_OFF diff --git a/include/common.h b/include/common.h index 12a1074..b535300 100644 --- a/include/common.h +++ b/include/common.h @@ -622,6 +622,7 @@ void flush_dcache_range(unsigned long start, unsigned long stop); void invalidate_dcache_range(unsigned long start, unsigned long stop); void invalidate_dcache_all(void); void invalidate_icache_all(void); +int get_dcache_line_size(void); /* arch/$(ARCH)/lib/ticks.S */ unsigned long long get_ticks(void);