armv7: cache maintenance operations

make default implementation of cache_flush() weakly linked so that sub-architectures can override it

Signed-off-by Aneesh V<[email protected]>

- Add a framework for layered cache maintenance
- separate out SOC specific outer cache maintenance from maintenance of caches known to CPU

- Add generic ARMv7 cache maintenance operations that affect all caches known to ARMv7 CPUs. For instance in Cortex-A8 these opertions will affect both L1 and L2 caches. In Cortex-A9 these will affect only L1 cache

- D-cache operations supported:
- Invalidate entire D-cache
- Invalidate D-cache range
- Flush(clean& invalidate) entire D-cache
- Flush D-cache range
- I-cache operations supported:
- Invalidate entire I-cache

- Add maintenance functions for TLB, branch predictor array etc.

- Enable -march=armv7-a so that armv7 assembly instructions can be used

Signed-off-by Aneesh V<[email protected]>

---
  arch/arm/cpu/armv7/Makefile   |    2 +-
  arch/arm/cpu/armv7/cache_v7.c |  359 +++++++++++++++++++++++++++++++++++++++++
  arch/arm/cpu/armv7/config.mk  |    2 +-
  arch/arm/include/asm/armv7.h  |   63 +++++++
  include/common.h              |    5 +-
  5 files changed, 428 insertions(+), 3 deletions(-)

create mode 100644 arch/arm/cpu/armv7/cache_v7.c create mode 100644 arch/arm/include/asm/armv7.h

arch/arm/cpu/armv7/Makefile +1 -1

index 8c0e915..299792a 100644
@@ -26,7 +26,7 @@ include $(TOPDIR)/config.mk
  LIB	= $(obj)lib$(CPU).o
 
  START	:= start.o
-COBJS	:= cpu.o
+COBJS	:= cpu.o cache_v7.o
  COBJS  += syslib.o
 
  SRCS	:= $(START:.o=.S) $(COBJS:.o=.c)

arch/arm/cpu/armv7/cache_v7.c new file +73 -0

new file mode 100644
index 0000000..0521d66
@@ -0,0 +1,359 @@
+/*
+ * (C) Copyright 2010
+ * Texas Instruments Incorporated - http://www.ti.com/
+ * Aneesh V<[email protected]>
+ *
+ * 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
+ */
+#include<linux/types.h>
+#include<common.h>
+#include<asm/armv7.h>
+
+#define ARMV7_DCACHE_INVAL_ALL		1
+#define ARMV7_DCACHE_CLEAN_INVAL_ALL	2
+#define ARMV7_DCACHE_INVAL_RANGE	3
+#define ARMV7_DCACHE_CLEAN_INVAL_RANGE	4
+
+struct v7_outer_cache_ops v7_outer_cache;
+
+#ifndef CONFIG_SYS_NO_DCACHE
+/*
+ * Write the level and type you want to Cache Size Selection Register(CSSELR)
+ * to get size details from Current Cache Size ID Register(CCSIDR)
+ */
+static void set_csselr(u32 level, u32 type)
+{	u32 csselr = level<<  1 | type;
+	/* Write to Cache Size Selection Register(CSSELR) */
+	asm volatile ("mcr p15, 2, %0, c0, c0, 0" : : "r" (csselr));
+}
+
+static u32 get_ccsidr(void)
+{
+	u32 ccsidr;
+	/* Read current CP15 Cache Size ID Register */
+	asm volatile ("mrc p15, 1, %0, c0, c0, 0" : "=r" (ccsidr));
+	return ccsidr;
+}
+
+static u32 get_clidr(void)
+{
+	u32 clidr;
+	/* Read current CP15 Cache Level ID Register */
+	asm volatile ("mrc p15,1,%0,c0,c0,1" : "=r" (clidr));
+	return clidr;
+}
+
+/* round up the input number to a power of 2 and get the log2 */
+static inline u32 log_2_round_up(u32 num)
+{
+	/* count leading zeros */
+	asm volatile ("CLZ %0, %0" : "+r" (num));
+
+	/* position of most significant 1 */
+	num = 31 - num;
+
+	return num;
+}
+

arch/arm/cpu/armv7/cache_v7.c new file +155 -0

+			setway = (level<<  1) | (set<<  log2_line_len) |
+				 (way<<  way_shift);
+			/* Invalidate data/unified cache line by set/way */
+			asm volatile ("	mcr p15, 0, %0, c7, c6, 2"
+					: : "r" (setway));
+		}
+	/* Make sure the operation is complete */
+	asm volatile ("DMB");
+}
+
+static void v7_clean_inval_dcache_level_setway(u32 level, u32 num_sets,
+					       u32 num_ways, u32 way_shift,
+					       u32 log2_line_len)
+{
+	int way, set, setway;
+	/*
+	 * For optimal assembly code:
+	 *	a. count down
+	 *	b. have bigger loop inside
+	 */
+	for (way = num_ways - 1; way>= 0 ; way--)
+		for (set = num_sets - 1; set>= 0; set--) {
+			setway = (level<<  1) | (set<<  log2_line_len) |
+				 (way<<  way_shift);
+			/*
+			 * Clean&  Invalidate data/unified
+			 * cache line by set/way
+			 */
+			asm volatile ("	mcr p15, 0, %0, c7, c14, 2"
+					: : "r" (setway));
+		}
+	/* Make sure the operation is complete */
+	asm volatile ("DMB");
+}
+
+static void v7_maint_dcache_level_setway(u32 level, u32 operation)
+{
+	u32 ccsidr;
+	u32 num_sets, num_ways, log2_line_len, log2_num_ways;
+	u32 way_shift;
+	set_csselr(level, ARMV7_CSSELR_IND_DATA_UNIFIED);
+
+	ccsidr = get_ccsidr();
+
+	log2_line_len = mask_n_get(ccsidr, 0, 2) + 2;
+	/* Converting from words to bytes */
+	log2_line_len += 2;
+
+	num_ways  = mask_n_get(ccsidr, 3, 12) + 1;
+	num_sets  = mask_n_get(ccsidr, 13, 27) + 1;
+	/*
+	 * According to ARMv7 ARM number of sets and number of ways need
+	 * not be a power of 2
+	 */
+	log2_num_ways = log_2_round_up(num_ways);
+
+	way_shift = (32 - log2_num_ways);
+	if (operation == ARMV7_DCACHE_INVAL_ALL)
+		v7_inval_dcache_level_setway(level, num_sets, num_ways,
+				      way_shift, log2_line_len);
+	else if (operation == ARMV7_DCACHE_CLEAN_INVAL_ALL)
+		v7_clean_inval_dcache_level_setway(level, num_sets, num_ways,
+						   way_shift, log2_line_len);
+}
+
+static void v7_maint_dcache_all(u32 operation)
+{
+	u32 level, cache_type, level_start_bit = 0;
+
+	u32 clidr = get_clidr();
+
+	for (level = 0; level<  7; level++) {
+		cache_type = mask_n_get(clidr, level_start_bit,
+					level_start_bit + 2);
+		if ((cache_type == ARMV7_CLIDR_CTYPE_DATA_ONLY) ||
+		    (cache_type == ARMV7_CLIDR_CTYPE_INSTRUCTION_DATA) ||
+		    (cache_type == ARMV7_CLIDR_CTYPE_UNIFIED))
+			v7_maint_dcache_level_setway(level, operation);
+		level_start_bit += 3;
+	}
+}
+
+static void v7_dcache_clean_inval_range(u32 start,
+					u32 stop, u32 line_len)
+{
+	u32 mva;
+	/* Align start to cache line boundary */
+	start&= ~(line_len - 1);
+	for (mva = start; mva<  stop; mva = mva + line_len)
+		/* DCCIMVAC - Clean&  Invalidate data cache by MVA to PoC */
+		asm volatile ("mcr p15, 0, %0, c7, c14, 1" : : "r" (mva));
+}
+
+static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len)
+{
+	u32 mva;
+
+	/*
+	 * If start address is not aligned to cache-line flush the first
+	 * line to prevent affecting somebody else's buffer
+	 */
+	if (start&  (line_len - 1)) {
+		v7_dcache_clean_inval_range(start, start + 1, line_len);
+		/* move to next cache line */
+		start = (start + line_len - 1)&  ~(line_len - 1);
+	}
+
+	/*
+	 * If stop address is not aligned to cache-line flush the last
+	 * line to prevent affecting somebody else's buffer
+	 */
+	if (stop&  (line_len - 1)) {
+		v7_dcache_clean_inval_range(stop, stop + 1, line_len);
+		/* align to the beginning of this cache line */
+		stop&= ~(line_len - 1);
+	}
+
+	for (mva = start; mva<  stop; mva = mva + line_len)
+		/* DCIMVAC - Invalidate data cache by MVA to PoC */
+		asm volatile ("mcr p15, 0, %0, c7, c6, 1" : : "r" (mva));
+}
+
+static void v7_dcache_maint_range(u32 start, u32 stop, u32 range_op)
+{
+	u32 line_len, ccsidr;
+	ccsidr = get_ccsidr();
+	line_len = mask_n_get(ccsidr, 0, 2) + 2;
+	/* Converting from words to bytes */
+	line_len += 2;
+	/* converting from log2(linelen) to linelen */
+	line_len = 1<<  line_len;
+
+	switch (range_op) {
+	case ARMV7_DCACHE_CLEAN_INVAL_RANGE:
+		v7_dcache_clean_inval_range(start, stop, line_len);
+		break;
+	case ARMV7_DCACHE_INVAL_RANGE:
+		v7_dcache_inval_range(start, stop, line_len);
+		break;
+	}
+
+	/* Make sure the operation is complete */
+	asm volatile ("DMB");
+}
+
+/* Invalidate TLB */
+static void v7_inval_tlb(void)
+{
+	/* Invalidate entire unified TLB */
+	asm volatile ("mcr p15, 0, %0, c8, c7, 0" : : "r" (0));
+	/* Invalidate entire data TLB */
+	asm volatile ("mcr p15, 0, %0, c8, c6, 0" : : "r" (0));
+	/* Invalidate entire instruction TLB */
+	asm volatile ("mcr p15, 0, %0, c8, c5, 0" : : "r" (0));
+	/* Full system DSB - make sure that the invalidation is complete */

arch/arm/cpu/armv7/cache_v7.c new file +99 -0

+}
+
+/*
+ * Performs a clean&  invalidation of the entire data cache
+ * at all levels
+ */
+void flush_dcache_all(void)
+{
+	v7_maint_dcache_all(ARMV7_DCACHE_CLEAN_INVAL_ALL);
+	if (v7_outer_cache.flush_all)
+		v7_outer_cache.flush_all();
+}
+
+/*
+ * Invalidates range in all levels of D-cache/unified cache used:
+ * Affects the range [start, stop - 1]
+ */
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+
+	v7_dcache_maint_range(start, stop, ARMV7_DCACHE_INVAL_RANGE);
+	if (v7_outer_cache.inval_range)
+		/* physical address is same as virtual address */
+		v7_outer_cache.inval_range(start, stop);
+}
+
+/*
+ * Flush range(clean&  invalidate) from all levels of D-cache/unified
+ * cache used:
+ * Affects the range [start, stop - 1]
+ */
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+	v7_dcache_maint_range(start, stop, ARMV7_DCACHE_CLEAN_INVAL_RANGE);
+	if (v7_outer_cache.flush_range)
+		/* physical address is same as virtual address */
+		v7_outer_cache.flush_range(start, stop);
+}
+static void __v7_setup_outer_cache_ops(void)
+{
+	puts("v7_setup_outer_cache_ops: dummy implementation! "
+	     "real implementation not available!!\n");
+}
+
+void v7_setup_outer_cache_ops(void)
+	__attribute__((weak, alias("__v7_setup_outer_cache_ops")));
+
+void arm_init_before_mmu(void)
+{
+	v7_setup_outer_cache_ops();
+	if (v7_outer_cache.enable)
+		v7_outer_cache.enable();
+	invalidate_dcache_all();
+	v7_inval_tlb();
+}
+#else
+void invalidate_dcache_all(void)
+{
+}
+
+void flush_dcache_all(void)
+{
+}
+
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void arm_init_before_mmu(void)
+{
+}
+#endif
+
+#ifndef CONFIG_SYS_NO_ICACHE
+/* Invalidate entire I-cache and branch predictor array */
+void invalidate_icache_all(void)
+{
+	/*
+	 * Invalidate all instruction caches to PoU.
+	 * Also flushes branch target cache.
+	 */
+	asm volatile ("mcr p15, 0, %0, c7, c5, 0" : : "r" (0));
+
+	/* Invalidate entire branch predictor array */
+	asm volatile ("mcr p15, 0, %0, c7, c5, 6" : : "r" (0));
+
+	/* Full system DSB - make sure that the invalidation is complete */
+	asm volatile ("DSB");
+	/* Full system ISB - make sure the instruction stream sees it */
+	asm volatile ("ISB");
+}
+#else
+void invalidate_icache_all(void)
+{
+}

arch/arm/cpu/armv7/config.mk +0 -0

  # =========================================================================
  #
  # Supply options according to compiler version

arch/arm/include/asm/armv7.h new file +39 -0

new file mode 100644
index 0000000..57409b6
@@ -0,0 +1,63 @@
+/*
+ * (C) Copyright 2010
+ * Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Aneesh V<[email protected]>
+ *
+ * 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 ARMV7_H
+#define ARMV7_H
+
+#include<linux/types.h>
+
+/*
+ * Values for InD field in CSSELR
+ * Selects the type of cache
+ */
+#define ARMV7_CSSELR_IND_DATA_UNIFIED	0
+#define ARMV7_CSSELR_IND_INSTRUCTION	1
+
+/* Values for Ctype fields in CLIDR */
+#define ARMV7_CLIDR_CTYPE_NO_CACHE		0
+#define ARMV7_CLIDR_CTYPE_INSTRUCTION_ONLY	1

arch/arm/include/asm/armv7.h new file +13 -0

+struct v7_outer_cache_ops {
+	void (*enable)(void);
+	void (*disable)(void);
+	void (*flush_all)(void);
+	void (*inval_all)(void);
+	void (*flush_range)(u32 start, u32 end);
+	void (*inval_range)(u32 start, u32 end);
+};
+
+extern struct v7_outer_cache_ops v7_outer_cache;
+
+void v7_setup_outer_cache_ops(void);
+#endif

include/common.h +1 -0

index 189ad81..d750ff9 100644
@@ -411,6 +411,7 @@ void	icache_disable(void);
  int	dcache_status (void);
  void	dcache_enable (void);
  void	dcache_disable(void);
+void	mmu_disable(void);
  void	relocate_code (ulong, gd_t *, ulong) __attribute__ ((noreturn));
  ulong	get_endaddr   (void);
  void	trap_init     (ulong);

@@ -603,9 +604,11 @@ ulong	video_setmem (ulong);
 
  /* arch/$(ARCH)/lib/cache.c */

arch/arm/cpu/armv7/config.mk line 26

Quoted 8 lines

arch/arm/cpu/armv7/config.mk +1 -1

@@ -23,7 +23,7 @@
   PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float
 
   # Make ARMv5 to allow more compilers to work, even though its v7a.
-PLATFORM_CPPFLAGS += -march=armv5
+PLATFORM_CPPFLAGS += -march=armv7-a

Did you check that this does not break any board using armv7?

Hi Aneesh,

Le 22/12/2010 12:54, Aneesh V a ?crit :

Quoted 130 lines

- Add a framework for layered cache maintenance
- separate out SOC specific outer cache maintenance from maintenance of caches known to CPU

- Add generic ARMv7 cache maintenance operations that affect all caches known to ARMv7 CPUs. For instance in Cortex-A8 these opertions will affect both L1 and L2 caches. In Cortex-A9 these will affect only L1 cache

- D-cache operations supported:
- Invalidate entire D-cache
- Invalidate D-cache range
- Flush(clean& invalidate) entire D-cache
- Flush D-cache range
- I-cache operations supported:
- Invalidate entire I-cache

- Add maintenance functions for TLB, branch predictor array etc.

- Enable -march=armv7-a so that armv7 assembly instructions can be used

Signed-off-by Aneesh V<[email protected]>

---
   arch/arm/cpu/armv7/Makefile   |    2 +-
   arch/arm/cpu/armv7/cache_v7.c |  359 +++++++++++++++++++++++++++++++++++++++++
   arch/arm/cpu/armv7/config.mk  |    2 +-
   arch/arm/include/asm/armv7.h  |   63 +++++++
   include/common.h              |    5 +-
   5 files changed, 428 insertions(+), 3 deletions(-)

create mode 100644 arch/arm/cpu/armv7/cache_v7.c create mode 100644 arch/arm/include/asm/armv7.h

arch/arm/cpu/armv7/Makefile +1 -1

index 8c0e915..299792a 100644
@@ -26,7 +26,7 @@ include $(TOPDIR)/config.mk
   LIB	= $(obj)lib$(CPU).o
 
   START	:= start.o
-COBJS	:= cpu.o
+COBJS	:= cpu.o cache_v7.o
   COBJS  += syslib.o
 
   SRCS	:= $(START:.o=.S) $(COBJS:.o=.c)

arch/arm/cpu/armv7/cache_v7.c new file +78 -0

new file mode 100644
index 0000000..0521d66
@@ -0,0 +1,359 @@
+/*
+ * (C) Copyright 2010
+ * Texas Instruments Incorporated - http://www.ti.com/
+ * Aneesh V<[email protected]>
+ *
+ * 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
+ */
+#include<linux/types.h>
+#include<common.h>
+#include<asm/armv7.h>
+
+#define ARMV7_DCACHE_INVAL_ALL		1
+#define ARMV7_DCACHE_CLEAN_INVAL_ALL	2
+#define ARMV7_DCACHE_INVAL_RANGE	3
+#define ARMV7_DCACHE_CLEAN_INVAL_RANGE	4
+
+struct v7_outer_cache_ops v7_outer_cache;
+
+#ifndef CONFIG_SYS_NO_DCACHE
+/*
+ * Write the level and type you want to Cache Size Selection Register(CSSELR)
+ * to get size details from Current Cache Size ID Register(CCSIDR)
+ */
+static void set_csselr(u32 level, u32 type)
+{	u32 csselr = level<<   1 | type;
+	/* Write to Cache Size Selection Register(CSSELR) */
+	asm volatile ("mcr p15, 2, %0, c0, c0, 0" : : "r" (csselr));
+}
+
+static u32 get_ccsidr(void)
+{
+	u32 ccsidr;
+	/* Read current CP15 Cache Size ID Register */
+	asm volatile ("mrc p15, 1, %0, c0, c0, 0" : "=r" (ccsidr));
+	return ccsidr;
+}
+
+static u32 get_clidr(void)
+{
+	u32 clidr;
+	/* Read current CP15 Cache Level ID Register */
+	asm volatile ("mrc p15,1,%0,c0,c0,1" : "=r" (clidr));
+	return clidr;
+}
+
+/* round up the input number to a power of 2 and get the log2 */
+static inline u32 log_2_round_up(u32 num)
+{
+	/* count leading zeros */
+	asm volatile ("CLZ %0, %0" : "+r" (num));
+
+	/* position of most significant 1 */
+	num = 31 - num;
+
+	return num;
+}
+
+static void v7_inval_dcache_level_setway(u32 level, u32 num_sets,
+					 u32 num_ways, u32 way_shift,
+					 u32 log2_line_len)
+{
+	int way, set, setway;

arch/arm/cpu/armv7/cache_v7.c line 83

Quoted 5 lines

arch/arm/cpu/armv7/cache_v7.c new file +5 -0

+	/*
+	 * For optimal assembly code:
+	 *	a. count down
+	 *	b. have bigger loop inside
+	 */

Out of curiosity, can you elaborate on why the compiler would optimize better in these cases?

arch/arm/cpu/armv7/cache_v7.c line 85

Quoted 3 lines

arch/arm/cpu/armv7/cache_v7.c new file +2 -0

+	for (way = num_ways - 1; way>= 0 ; way--)
+		for (set = num_sets - 1; set>= 0; set--) {

Please fix whitespacing around operators. The best way to ''catch'em all'' is to run Linux' checkpatch.pl (I do this with option --no-tree) on all patches that you submit to u-boot and, fix all warning and errors and if some are left that you think should not be fixed, mention them and explain why they're wrongly emitted.

Quoted 2 lines

Out of curiosity, can you elaborate on why the compiler would optimize better in these cases?

While counting down the termination condition check is against 0. So you can just decrement the loop count using a 'subs' and do a 'bne'. When you count up you have to do a comparison with a non-zero value. So you will need one 'cmp' instruction extra:-)

bigger loop inside because that reduces the frequency at which your outer parameter changes and hence the overall number of instructions executed. Consider this:
1. We encode both the loop counts along with other data into a register that is finally written to CP15 register.
2. outer loop has the code for shifting and ORing the outer variable to this register.
3. Inner loop has the code for shifting and ORing the inner variable. Step (3) has to be executed 'way x set' number of times anyways. But having bigger loop inside makes sure that 2 is executed fewer times!

With these tweaks the assembly code generated by this C code is as good as the original hand-written assembly code with my compiler.

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	for (way = num_ways - 1; way>= 0 ; way--)
+		for (set = num_sets - 1; set>= 0; set--) {

Please fix whitespacing around operators. The best way to ''catch'em all'' is to run Linux' checkpatch.pl (I do this with option --no-tree) on all patches that you submit to u-boot and, fix all warning and errors and if some are left that you think should not be fixed, mention them and explain why they're wrongly emitted.

I religiously do checkpatch whenever I send out a patch. Please note that my original mail seems to be fine. I saved it and ran checkpatch again. No errors, no warnings! Something amiss?

Best regards,
Aneesh

(I realize I did not answer the other ones)

Le 08/01/2011 11:06, Aneesh V a ?crit :

Quoted 3 lines

Quoted 2 lines

Out of curiosity, can you elaborate on why the compiler would optimize better in these cases?

Quoted 4 lines

While counting down the termination condition check is against 0. So you can just decrement the loop count using a 'subs' and do a 'bne'. When you count up you have to do a comparison with a non-zero value. So you will need one 'cmp' instruction extra:-)

I would not try to be too smart about what instructions are generated and how by a compiler such as gcc which has rather complex code generation optimizations.

Quoted 11 lines

bigger loop inside because that reduces the frequency at which your outer parameter changes and hence the overall number of instructions executed. Consider this:
1. We encode both the loop counts along with other data into a register that is finally written to CP15 register.
2. outer loop has the code for shifting and ORing the outer variable to this register.
3. Inner loop has the code for shifting and ORing the inner variable. Step (3) has to be executed 'way x set' number of times anyways. But having bigger loop inside makes sure that 2 is executed fewer times!

Here too it seems like you're underestimating the compiler's optimizing capabilities -- your explanation seems to amount to extracting a constant calculation from a loop, something that is rather usual in code optimizing.

Quoted 3 lines

With these tweaks the assembly code generated by this C code is as good as the original hand-written assembly code with my compiler.

How about other compilers?

Quoted 9 lines

Quoted 8 lines

Quoted 2 lines

+ for (way = num_ways - 1; way>= 0 ; way--)
+ for (set = num_sets - 1; set>= 0; set--) {

Please fix whitespacing around operators. The best way to ''catch'em all'' is to run Linux' checkpatch.pl (I do this with option --no-tree) on all patches that you submit to u-boot and, fix all warning and errors and if some are left that you think should not be fixed, mention them and explain why they're wrongly emitted.

Quoted 3 lines

I religiously do checkpatch whenever I send out a patch. Please note that my original mail seems to be fine. I saved it and ran checkpatch again. No errors, no warnings! Something amiss?

Well, something like "set>= 0" is quite surprising as it has inconsistent spacing around a binary operators. But you're right, checkpatch does not detect it. Can you fix them manually?

Quoted 3 lines

Best regards,
Aneesh

Amicalement,

<snip ..>

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Quoted 2 lines

+ for (way = num_ways - 1; way>= 0 ; way--)
+ for (set = num_sets - 1; set>= 0; set--) {

Please fix whitespacing around operators. The best way to ''catch'em all'' is to run Linux' checkpatch.pl (I do this with option --no-tree) on all patches that you submit to u-boot and, fix all warning and errors
and if some are left that you think should not be fixed, mention them and explain why they're wrongly emitted.

I religiously do checkpatch whenever I send out a patch. Please note that my original mail seems to be fine. I saved it and ran checkpatch again. No errors, no warnings! Something amiss?

Quoted 3 lines

Well, something like "set>= 0" is quite surprising as it has inconsistent spacing around a binary operators. But you're right, checkpatch does not detect it. Can you fix them manually?

Checkpatch does find such issues. I was trying to say that my original mail doesn't have any spacing issues. The problem seems to have appeared in your reply. Is your mail client doing something funny?

Hi Aneesh,

arch/arm/cpu/armv7/cache_v7.c line 250

Quoted 6 lines

arch/arm/cpu/armv7/cache_v7.c new file +6 -0

+
+void invalidate_dcache_all(void)
+{
+	v7_maint_dcache_all(ARMV7_DCACHE_INVAL_ALL);
+	if (v7_outer_cache.inval_all)
+		v7_outer_cache.inval_all();

Why use pointers here rather than weak functions?

Quoted 106 lines

arch/arm/cpu/armv7/cache_v7.c new file +105 -0

+}
+
+/*
+ * Performs a clean&   invalidation of the entire data cache
+ * at all levels
+ */
+void flush_dcache_all(void)
+{
+	v7_maint_dcache_all(ARMV7_DCACHE_CLEAN_INVAL_ALL);
+	if (v7_outer_cache.flush_all)
+		v7_outer_cache.flush_all();
+}
+
+/*
+ * Invalidates range in all levels of D-cache/unified cache used:
+ * Affects the range [start, stop - 1]
+ */
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+
+	v7_dcache_maint_range(start, stop, ARMV7_DCACHE_INVAL_RANGE);
+	if (v7_outer_cache.inval_range)
+		/* physical address is same as virtual address */
+		v7_outer_cache.inval_range(start, stop);
+}
+
+/*
+ * Flush range(clean&   invalidate) from all levels of D-cache/unified
+ * cache used:
+ * Affects the range [start, stop - 1]
+ */
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+	v7_dcache_maint_range(start, stop, ARMV7_DCACHE_CLEAN_INVAL_RANGE);
+	if (v7_outer_cache.flush_range)
+		/* physical address is same as virtual address */
+		v7_outer_cache.flush_range(start, stop);
+}
+static void __v7_setup_outer_cache_ops(void)
+{
+	puts("v7_setup_outer_cache_ops: dummy implementation! "
+	     "real implementation not available!!\n");
+}
+
+void v7_setup_outer_cache_ops(void)
+	__attribute__((weak, alias("__v7_setup_outer_cache_ops")));
+
+void arm_init_before_mmu(void)
+{
+	v7_setup_outer_cache_ops();
+	if (v7_outer_cache.enable)
+		v7_outer_cache.enable();
+	invalidate_dcache_all();
+	v7_inval_tlb();
+}
+#else
+void invalidate_dcache_all(void)
+{
+}
+
+void flush_dcache_all(void)
+{
+}
+
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void arm_init_before_mmu(void)
+{
+}
+#endif
+
+#ifndef CONFIG_SYS_NO_ICACHE
+/* Invalidate entire I-cache and branch predictor array */
+void invalidate_icache_all(void)
+{
+	/*
+	 * Invalidate all instruction caches to PoU.
+	 * Also flushes branch target cache.
+	 */
+	asm volatile ("mcr p15, 0, %0, c7, c5, 0" : : "r" (0));
+
+	/* Invalidate entire branch predictor array */
+	asm volatile ("mcr p15, 0, %0, c7, c5, 6" : : "r" (0));
+
+	/* Full system DSB - make sure that the invalidation is complete */
+	asm volatile ("DSB");
+	/* Full system ISB - make sure the instruction stream sees it */
+	asm volatile ("ISB");
+}
+#else
+void invalidate_icache_all(void)
+{
+}
+#endif
+
+/*
+ * Flush range from all levels of d-cache/unified-cache used:
+ * Affects the range [start, start + size - 1]
+ */

arch/arm/cpu/armv7/cache_v7.c line 359

Quoted 4 lines

arch/arm/cpu/armv7/cache_v7.c new file +4 -0

+void  flush_cache(unsigned long start, unsigned long size)
+{
+	flush_dcache_range(start, start + size);
+}

This function is the only one which is defined to something non-empty when CONFIG_SYS_NO_DCACHE is defined. Why is it not in the big #ifndef for dcache above ?

Quoted 8 lines

arch/arm/cpu/armv7/config.mk +0 -0

index 49ac9c7..7f9b171 100644
@@ -23,7 +23,7 @@
   PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float
 

arch/arm/cpu/armv7/config.mk line 26

Quoted 3 lines

arch/arm/cpu/armv7/config.mk +1 -1

   # Make ARMv5 to allow more compilers to work, even though its v7a.
-PLATFORM_CPPFLAGS += -march=armv5
+PLATFORM_CPPFLAGS += -march=armv7-a

Did you check that this does not break any board using armv7?

Quoted 53 lines

arch/arm/cpu/armv7/config.mk +0 -0

   # =========================================================================
   #
   # Supply options according to compiler version

arch/arm/include/asm/armv7.h new file +44 -0

new file mode 100644
index 0000000..57409b6
@@ -0,0 +1,63 @@
+/*
+ * (C) Copyright 2010
+ * Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Aneesh V<[email protected]>
+ *
+ * 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 ARMV7_H
+#define ARMV7_H
+
+#include<linux/types.h>
+
+/*
+ * Values for InD field in CSSELR
+ * Selects the type of cache
+ */
+#define ARMV7_CSSELR_IND_DATA_UNIFIED	0
+#define ARMV7_CSSELR_IND_INSTRUCTION	1
+
+/* Values for Ctype fields in CLIDR */
+#define ARMV7_CLIDR_CTYPE_NO_CACHE		0
+#define ARMV7_CLIDR_CTYPE_INSTRUCTION_ONLY	1
+#define ARMV7_CLIDR_CTYPE_DATA_ONLY		2
+#define ARMV7_CLIDR_CTYPE_INSTRUCTION_DATA	3
+#define ARMV7_CLIDR_CTYPE_UNIFIED		4
+
+/* some utility macros */

arch/arm/include/asm/armv7.h

Quoted 5 lines

arch/arm/include/asm/armv7.h new file +5 -0

+#define mask(start, end) \
+	(((1<<   ((end) - (start) + 1)) - 1)<<   (start))
+
+#define mask_n_get(reg, start, end) \
+	(((reg)&   mask(start, end))>>   (start))

Seeing as these functions are only used in the ARMv7 cache C file, they should be moved there.

Quoted 1 line

Why use pointers here rather than weak functions?

In fact, I hadn't thought about it. Maybe I was biased by the Linux implementation.The only reason I can think of is that pointer gives the flexibility of doing this assignment at run-time. Let's say we had a multi-platform u-boot that detects the SOC at run-time?

Quoted 8 lines

arch/arm/cpu/armv7/cache_v7.c line 359

Quoted 4 lines

arch/arm/cpu/armv7/cache_v7.c new file +4 -0

+void flush_cache(unsigned long start, unsigned long size)
+{
+ flush_dcache_range(start, start + size);
+}

This function is the only one which is defined to something non-empty when CONFIG_SYS_NO_DCACHE is defined. Why is it not in the big #ifndef for dcache above ?

Although this function is non-empty, flush_dcache_range() is in turn empty. Effect will be the same, right?

Quoted 7 lines

Quoted 7 lines

arch/arm/cpu/armv7/config.mk +0 -0

index 49ac9c7..7f9b171 100644
@@ -23,7 +23,7 @@

PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float

Quoted 5 lines

arch/arm/cpu/armv7/config.mk line 26

Quoted 3 lines

# Make ARMv5 to allow more compilers to work, even though its v7a.

arch/arm/cpu/armv7/config.mk +1 -1

-PLATFORM_CPPFLAGS += -march=armv5
+PLATFORM_CPPFLAGS += -march=armv7-a

Did you check that this does not break any board using armv7?

I checked only Codesourcery tool chain.
Linux kernel build for a v7 architecture processor uses armv7-a. Is it not fair to assume that the toolchain used for bootloader also supports it? Do we have to support those out-dated compilers?

Quoted 9 lines

arch/arm/cpu/armv7/config.mk

Quoted 6 lines

arch/arm/cpu/armv7/config.mk +6 -0

+/* some utility macros */
+#define mask(start, end) \
+ (((1<< ((end) - (start) + 1)) - 1)<< (start))
+
+#define mask_n_get(reg, start, end) \
+ (((reg)& mask(start, end))>> (start))

Seeing as these functions are only used in the ARMv7 cache C file, they should be moved there.

I plan to use a modified version of mask_n_get() and its set couterpart mask_n_set() in my subsequent works in more files.

Can I keep it here itself or should I move it to an OMAP specific header file or can I move it to a more generic header file? Please suggest.

Best regards,
Aneesh

I know we consider multi-board u-boot binaries when boards are variant of a given SoC, that's one reason why we wanted relocation. I'm not sure about multi-SoC when SoC is a variant of a given cpu, though. Wolfgang, your opinion?

Quoted 2 lines

Although this function is non-empty, flush_dcache_range() is in turn empty. Effect will be the same, right?

Yes the effect is the same, but your patch results in a non-trivially empty function; I'd prefer it to be visibly empty when we compile without cache support.

Just because Linux uses armv7-a for a v7 arch does not mean we must have it for u-boot. For starters, U-boot does not always boot Linux. :)

As for out-dated compilers, that's the question I'm asking: do we consider e.g. ELDK 4.2 as outdated or not? It won't accept armv7-a.

Dear Albert ARIBAUD,

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

I know we consider multi-board u-boot binaries when boards are variant of a given SoC, that's one reason why we wanted relocation. I'm not sure about multi-SoC when SoC is a variant of a given cpu, though. Wolfgang, your opinion?

Unless we see a specific example which uses this feature, we should not add provisions that make the code more complicated than needed.

And when we start supporting such a feature, we should probably do this based on a device tree approach.

Quoted 6 lines

Quoted 2 lines

Although this function is non-empty, flush_dcache_range() is in turn empty. Effect will be the same, right?

Yes the effect is the same, but your patch results in a non-trivially empty function; I'd prefer it to be visibly empty when we compile without cache support.

Yes, that's my opinion, too.

Quoted 7 lines

Just because Linux uses armv7-a for a v7 arch does not mean we must have it for u-boot. For starters, U-boot does not always boot Linux. :)

As for out-dated compilers, that's the question I'm asking: do we consider e.g. ELDK 4.2 as outdated or not? It won't accept armv7-a.

That's a catch question.

Yes, ELDK 4.2 is outdated. But it is still one of the most stable versions of a tool chain known to me, especially when it comes to using the very same tool versions across several architectures.

I cannot see any benefits of this code change that would justiify such a breakage.

Best regards,

Wolfgang Denk

Quoted 3 lines

Unless we see a specific example which uses this feature, we should not add provisions that make the code more complicated than needed.

Agree. But do you think the pointer based approach makes it overly complex?

Le 08/01/2011 14:17, Aneesh V a ?crit :

Quoted 13 lines

Quoted 9 lines

arch/arm/include/asm/armv7.h

Quoted 6 lines

arch/arm/include/asm/armv7.h new file +6 -0

+/* some utility macros */
+#define mask(start, end) \
+ (((1<< ((end) - (start) + 1)) - 1)<< (start))
+
+#define mask_n_get(reg, start, end) \
+ (((reg)& mask(start, end))>> (start))

Seeing as these functions are only used in the ARMv7 cache C file, they should be moved there.

I plan to use a modified version of mask_n_get() and its set couterpart mask_n_set() in my subsequent works in more files.

Quoted 3 lines

Can I keep it here itself or should I move it to an OMAP specific header file or can I move it to a more generic header file? Please suggest.

They're very generic actually. I think they should go to a gereric bit manipulation header, and be named a... bit... more explicitly. For instance, the name 'mask' does not show that the macro creates a range of 'one' bits from start to end.

Quoted 2 lines

Le 08/01/2011 14:17, Aneesh V a ?crit :

<snip..>

Quoted 13 lines

Quoted 13 lines

Quoted 9 lines

arch/arm/include/asm/armv7.h

Quoted 6 lines

arch/arm/include/asm/armv7.h new file +6 -0

+/* some utility macros */
+#define mask(start, end) \
+ (((1<< ((end) - (start) + 1)) - 1)<< (start))
+
+#define mask_n_get(reg, start, end) \
+ (((reg)& mask(start, end))>> (start))

Seeing as these functions are only used in the ARMv7 cache C file, they should be moved there.

I plan to use a modified version of mask_n_get() and its set couterpart mask_n_set() in my subsequent works in more files.

Quoted 8 lines

Quoted 3 lines

Can I keep it here itself or should I move it to an OMAP specific header file or can I move it to a more generic header file? Please suggest.

They're very generic actually. I think they should go to a gereric bit manipulation header, and be named a... bit... more explicitly. For instance, the name 'mask' does not show that the macro creates a range of 'one' bits from start to end.

What I need is something like below:

#define get_bit_field(nr, start, mask)\
(((nr) & (mask)) >> (start))

#define set_bit_field(nr, start, mask, val)\
(nr) = ((nr) & ~(mask)) | (((val) << (start)) & (mask))

Can these go in a generic header? If so, can I add them to "include/linux/bitops.h"

Best regards,
Aneesh

Le 12/01/2011 10:08, Aneesh V a ?crit :

Quoted 36 lines · Albert ARIBAUD

Quoted 2 lines

Le 08/01/2011 14:17, Aneesh V a ?crit :

<snip..>

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

arch/arm/include/asm/armv7.h

Quoted 6 lines

arch/arm/include/asm/armv7.h new file +6 -0

+/* some utility macros */
+#define mask(start, end) \
+ (((1<< ((end) - (start) + 1)) - 1)<< (start))
+
+#define mask_n_get(reg, start, end) \
+ (((reg)& mask(start, end))>> (start))

Seeing as these functions are only used in the ARMv7 cache C file, they
should be moved there.

I plan to use a modified version of mask_n_get() and its set couterpart mask_n_set() in my subsequent works in more files.

Quoted 8 lines

Quoted 3 lines

Can I keep it here itself or should I move it to an OMAP specific header file or can I move it to a more generic header file? Please suggest.

They're very generic actually. I think they should go to a gereric bit manipulation header, and be named a... bit... more explicitly. For instance, the name 'mask' does not show that the macro creates a range of 'one' bits from start to end.

What I need is something like below:

#define get_bit_field(nr, start, mask)\
(((nr) & (mask)) >> (start))

#define set_bit_field(nr, start, mask, val)\
(nr) = ((nr) & ~(mask)) | (((val) << (start)) & (mask))

Quoted 2 lines

Can these go in a generic header? If so, can I add them to "include/linux/bitops.h"

After some more thought, I am wondering if a *generic* field setting and getting macro is really useful. So far everyone is fine with at most defining field-specific macros.

Quoted 8 lines

Quoted 8 lines

What I need is something like below:

#define get_bit_field(nr, start, mask)\
(((nr) & (mask)) >> (start))

#define set_bit_field(nr, start, mask, val)\
(nr) = ((nr) & ~(mask)) | (((val) << (start)) & (mask))

Quoted 6 lines

Quoted 2 lines

Can these go in a generic header? If so, can I add them to "include/linux/bitops.h"

After some more thought, I am wondering if a *generic* field setting and getting macro is really useful. So far everyone is fine with at most defining field-specific macros.

Is it going to be easy if you have many fields to deal with?

However, I agree that the above may be specific to our needs.

What may be of more generic interest may be something like this with the mask automatically generated:
#define get_bit_field(nr, start, end)
#define set_bit_field(nr, start, end, val)

However, in our case I am already given the mask and start position for each field (automatically generated from hw database). So, I prefer the former versions.

If that doesn't look useful for generic use I will put them in OMAP specific headers.

Best regards,
Aneesh

Le 13/01/2011 13:05, Aneesh V a ?crit :

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

What I need is something like below:

#define get_bit_field(nr, start, mask)\
(((nr) & (mask)) >> (start))

#define set_bit_field(nr, start, mask, val)\
(nr) = ((nr) & ~(mask)) | (((val) << (start)) & (mask))

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Can these go in a generic header? If so, can I add them to "include/linux/bitops.h"

After some more thought, I am wondering if a *generic* field setting and getting macro is really useful. So far everyone is fine with at most defining field-specific macros.

Is it going to be easy if you have many fields to deal with?

I don't see how the generic macros ease anything. Instead of defining say

#define get_field_F(x) ((x >> F_start) & F_mask) #define set_field_F(x,v) { x = (x ~ F_mask ) | (v << F_start) }

You'd have

#define get_field_F(x) get_bit_field(x, F_start, F_mask) #define set_field_F(x,v) set_bit_field(x, F_start, F_mask, v);

Which does not seem to bring any simplicity to me.

Perhaps my requirement is different. If this scheme is not used by many, I shall put these macros in OMAP specific headers.

best regards,
Aneesh

Hi Aneesh,

arch/arm/include/asm/armv7.h

Quoted 6 lines

arch/arm/include/asm/armv7.h new file +6 -0

+/* some utility macros */
+#define mask(start, end) \
+	(((1<<   ((end) - (start) + 1)) - 1)<<   (start))
+
+#define mask_n_get(reg, start, end) \
+	(((reg)&   mask(start, end))>>   (start))

Seeing as these functions are only used in the ARMv7 cache C file, they should be moved there.

Hi Albert,

Quoted 1 line · Albert ARIBAUD

Hi Aneesh,

[snip ..]

Quoted 10 lines

arch/arm/include/asm/armv7.h

Quoted 6 lines

arch/arm/include/asm/armv7.h new file +6 -0

+/* some utility macros */
+#define mask(start, end) \
+ (((1<< ((end) - (start) + 1)) - 1)<< (start))
+
+#define mask_n_get(reg, start, end) \
+ (((reg)& mask(start, end))>> (start))

Seeing as these functions are only used in the ARMv7 cache C file, they should be moved there.

I am working on v2 of this series.

We had aligned on moving these macros to omap specific headers. But I now realize that I want to use them from cache_v7.c, so it can not be in omap header. I have used them in other places too in my recent work so it needs to be in a header file. I am putting these functions and some other utils I need in a new file "arch/arm/include/asm/utils.h" Is that fine?

Best regards,
Aneesh

- Enable I-cache on bootup
- Enable MMU and D-cache immediately after relocation
- Do necessary initialization before enabling d-cache and MMU
- Changes to cleanup_before_linux()
- Make changes according to the new framework

Signed-off-by Aneesh V<[email protected]>

---
  arch/arm/cpu/armv7/cpu.c   |   47 +++++++++++++++++++------------------------
  arch/arm/cpu/armv7/start.S |   18 +++++++++++++++-
  arch/arm/lib/board.c       |    6 +++++
  arch/arm/lib/cache-cp15.c  |    9 ++++++++
  4 files changed, 53 insertions(+), 27 deletions(-)

arch/arm/cpu/armv7/cpu.c +1 -4

index a01e0d6..b418304 100644
@@ -38,13 +38,10 @@
  #ifndef CONFIG_L2_OFF
  #include<asm/arch/sys_proto.h>
  #endif
-
-static void cache_flush(void);
+#include<asm/armv7.h>
 
  int cleanup_before_linux(void)
  {
-	unsigned int i;
-
  	/*
  	 * this function is called just before we call linux
  	 * it prepares the processor for linux

arch/arm/cpu/armv7/cpu.c +0 -5

-
-#ifndef CONFIG_L2_OFF
-	/* turn off L2 cache */
-	l2_cache_disable();
-	/* invalidate L2 cache also */

arch/arm/cpu/armv7/cpu.c +1 -3

-#ifndef CONFIG_L2_OFF
-	l2_cache_enable();
-#endif
+	/*

arch/arm/cpu/armv7/cpu.c +0 -5

  	return 0;
  }
-
-static void cache_flush(void)
-{
-	asm ("mcr p15, 0, %0, c7, c5, 0": :"r" (0));
-}

arch/arm/cpu/armv7/start.S +17 -1

index 684f2d2..7d17f1e 100644
@@ -241,6 +241,14 @@ clbss_l:str	r2, [r0]		/* clear loop...		    */
   * initialization, now running from RAM.
   */
  jump_2_ram:
+/*
+ * If I-cache is enabled invalidate it
+ */
+#ifndef CONFIG_SYS_NO_ICACHE
+	mcr	p15, 0, r0, c7, c5, 0	@ invalidate icache
+	dsb
+	isb
+#endif
  	ldr	r0, _board_init_r_ofs
  	adr	r1, _start
  	add	lr, r0, r1

@@ -276,6 +284,9 @@ cpu_init_crit:
  	mov	r0, #0			@ set up for MCR
  	mcr	p15, 0, r0, c8, c7, 0	@ invalidate TLBs
  	mcr	p15, 0, r0, c7, c5, 0	@ invalidate icache
+	mcr	p15, 0, r0, c7, c5, 6  /* invalidate BP array */
+	dsb
+	isb
 
  	/*
  	 * disable MMU stuff and caches

@@ -284,7 +295,12 @@ cpu_init_crit:
  	bic	r0, r0, #0x00002000	@ clear bits 13 (--V-)
  	bic	r0, r0, #0x00000007	@ clear bits 2:0 (-CAM)
  	orr	r0, r0, #0x00000002	@ set bit 1 (--A-) Align
-	orr	r0, r0, #0x00000800	@ set bit 12 (Z---) BTB
+	orr	r0, r0, #0x00000800	@ set bit 11 (Z---) BTB
+#ifdef CONFIG_SYS_NO_ICACHE
+	bic	r0, r0, #0x00001000	@ clear bit 12 (I) I-cache
+#else
+	orr	r0, r0, #0x00001000	@ set bit 12 (I) I-cache
+#endif
  	mcr	p15, 0, r0, c1, c0, 0
 
  	/*

arch/arm/lib/board.c +6 -0

index 7266381..bef32a6 100644
@@ -459,6 +459,12 @@ void board_init_r (gd_t *id, ulong dest_addr)
 
  	gd->flags |= GD_FLG_RELOC;	/* tell others: relocation done */
 
+	/*
+	 * Enable D$:
+	 * I$, if needed, must be already enabled in start.S
+	 */
+	dcache_enable();
+
  	monitor_flash_len = _bss_start_ofs;
  	debug ("monitor flash len: %08lX\n", monitor_flash_len);
  	board_init();	/* Setup chipselects */

arch/arm/lib/cache-cp15.c +0 -0

index d9175f0..ca526fb 100644

arch/arm/lib/cache-cp15.c +3 -0

+void arm_init_before_mmu(void)
+	__attribute__((weak, alias("__arm_init_before_mmu")));
+
  static void cp_delay (void)
  {
  	volatile int i;

Hi Aneesh,

Le 22/12/2010 12:54, Aneesh V a ?crit :

Quoted 34 lines

- Enable I-cache on bootup
- Enable MMU and D-cache immediately after relocation
- Do necessary initialization before enabling d-cache and MMU
- Changes to cleanup_before_linux()
- Make changes according to the new framework

Signed-off-by Aneesh V<[email protected]>

---
   arch/arm/cpu/armv7/cpu.c   |   47 +++++++++++++++++++------------------------
   arch/arm/cpu/armv7/start.S |   18 +++++++++++++++-
   arch/arm/lib/board.c       |    6 +++++
   arch/arm/lib/cache-cp15.c  |    9 ++++++++
   4 files changed, 53 insertions(+), 27 deletions(-)

arch/arm/cpu/armv7/cpu.c +1 -4

index a01e0d6..b418304 100644
@@ -38,13 +38,10 @@
   #ifndef CONFIG_L2_OFF
   #include<asm/arch/sys_proto.h>
   #endif
-
-static void cache_flush(void);
+#include<asm/armv7.h>
 
   int cleanup_before_linux(void)
   {
-	unsigned int i;
-
   	/*
   	 * this function is called just before we call linux
   	 * it prepares the processor for linux

arch/arm/cpu/armv7/cpu.c int cleanup_before_linux(void) · line 58

Quoted 9 lines

arch/arm/cpu/armv7/cpu.c +3 -2

@@ -53,31 +50,29 @@ int cleanup_before_linux(void)
   	 */
   	disable_interrupts();
 
-	/* turn off I/D-cache */
+	/*
+	 * Turn off I-cache and invalidate it
+	 */
   	icache_disable();
-	dcache_disable();

(1) -- see below

arch/arm/cpu/armv7/cpu.c int cleanup_before_linux(void) · line 61

Quoted 4 lines

arch/arm/cpu/armv7/cpu.c +1 -2

+	invalidate_icache_all();
 
-	/* invalidate I-cache */
-	cache_flush();

(2) -- see below

Quoted 5 lines

arch/arm/cpu/armv7/cpu.c +0 -5

-
-#ifndef CONFIG_L2_OFF
-	/* turn off L2 cache */
-	l2_cache_disable();
-	/* invalidate L2 cache also */

arch/arm/cpu/armv7/cpu.c int cleanup_before_linux(void) · line 63

Quoted 10 lines

arch/arm/cpu/armv7/cpu.c +5 -5

-	invalidate_dcache(get_device_type());
-#endif
-	i = 0;
-	/* mem barrier to sync up things */
-	asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i));
+	/*
+	 * turn off D-cache
+	 * dcache_disable() in turn flushes the d-cache and disables MMU
+	 */
+	dcache_disable();

(3) -- see below

Quoted 4 lines

arch/arm/cpu/armv7/cpu.c +1 -3

-#ifndef CONFIG_L2_OFF
-	l2_cache_enable();
-#endif
+	/*

arch/arm/cpu/armv7/cpu.c int cleanup_before_linux(void) · line 75

Quoted 10 lines

arch/arm/cpu/armv7/cpu.c +10 -0

+	 * After D-cache is flushed and before it is disabled there may
+	 * be some new valid entries brought into the cache. We are sure
+	 * that these lines are not dirty and will not affect our execution.
+	 * (because unwinding the call-stack and setting a bit in CP15 SCTRL
+	 * is all we did during this. We have not pushed anything on to the
+	 * stack. Neither have we affected any static data)
+	 * So just invalidate the entire d-cache again to avoid coherency
+	 * problems for kernel
+	 */
+	invalidate_dcache_all();

I'm not sure about the logic here, but I am no cache guru, so don't hesitate to make me... flush... with shame. If dcache_disable stayed in
(1) instead of being moved to (3), wouldn't the cache_flush in (2) ensure the no new valid entries would appear in the dcache? Put another way, I'd have naively thought the sequence would be:

- disable L2 cache
- disable L1 I and D cache
- invalidate L1 I cache and flush L1 D cache
- flush L2 D cache

Quoted 76 lines

arch/arm/cpu/armv7/cpu.c +0 -5

   	return 0;
   }
-
-static void cache_flush(void)
-{
-	asm ("mcr p15, 0, %0, c7, c5, 0": :"r" (0));
-}

arch/arm/cpu/armv7/start.S +17 -1

index 684f2d2..7d17f1e 100644
@@ -241,6 +241,14 @@ clbss_l:str	r2, [r0]		/* clear loop...		    */
    * initialization, now running from RAM.
    */
   jump_2_ram:
+/*
+ * If I-cache is enabled invalidate it
+ */
+#ifndef CONFIG_SYS_NO_ICACHE
+	mcr	p15, 0, r0, c7, c5, 0	@ invalidate icache
+	dsb
+	isb
+#endif
   	ldr	r0, _board_init_r_ofs
   	adr	r1, _start
   	add	lr, r0, r1

@@ -276,6 +284,9 @@ cpu_init_crit:
   	mov	r0, #0			@ set up for MCR
   	mcr	p15, 0, r0, c8, c7, 0	@ invalidate TLBs
   	mcr	p15, 0, r0, c7, c5, 0	@ invalidate icache
+	mcr	p15, 0, r0, c7, c5, 6  /* invalidate BP array */
+	dsb
+	isb
 
   	/*
   	 * disable MMU stuff and caches

@@ -284,7 +295,12 @@ cpu_init_crit:
   	bic	r0, r0, #0x00002000	@ clear bits 13 (--V-)
   	bic	r0, r0, #0x00000007	@ clear bits 2:0 (-CAM)
   	orr	r0, r0, #0x00000002	@ set bit 1 (--A-) Align
-	orr	r0, r0, #0x00000800	@ set bit 12 (Z---) BTB
+	orr	r0, r0, #0x00000800	@ set bit 11 (Z---) BTB
+#ifdef CONFIG_SYS_NO_ICACHE
+	bic	r0, r0, #0x00001000	@ clear bit 12 (I) I-cache
+#else
+	orr	r0, r0, #0x00001000	@ set bit 12 (I) I-cache
+#endif
   	mcr	p15, 0, r0, c1, c0, 0
 
   	/*

arch/arm/lib/board.c +6 -0

index 7266381..bef32a6 100644
@@ -459,6 +459,12 @@ void board_init_r (gd_t *id, ulong dest_addr)
 
   	gd->flags |= GD_FLG_RELOC;	/* tell others: relocation done */
 
+	/*
+	 * Enable D$:
+	 * I$, if needed, must be already enabled in start.S
+	 */
+	dcache_enable();
+
   	monitor_flash_len = _bss_start_ofs;
   	debug ("monitor flash len: %08lX\n", monitor_flash_len);
   	board_init();	/* Setup chipselects */

arch/arm/lib/cache-cp15.c +0 -0

index d9175f0..ca526fb 100644
@@ -34,6 +34,14 @@
 
   DECLARE_GLOBAL_DATA_PTR;
 

arch/arm/lib/cache-cp15.c line 41

Quoted 5 lines

arch/arm/lib/cache-cp15.c +5 -0

+void __arm_init_before_mmu(void)
+{
+	puts("__arm_init_before_mmu: dummy implementation! "
+	     "real implementation missing!!\n");
+}

If the function is absolutely needed, please make sure booting does not continue. If the function is not absolutely needed, please rewrite message since nothing would actually be 'missing', but rather 'could be added'.

Quoted 7 lines

arch/arm/lib/cache-cp15.c +3 -0

+void arm_init_before_mmu(void)
+	__attribute__((weak, alias("__arm_init_before_mmu")));
+
   static void cp_delay (void)
   {
   	volatile int i;

arch/arm/lib/cache-cp15.c static inline void mmu_setup(void) · line 79

Quoted 7 lines

arch/arm/lib/cache-cp15.c +1 -0

@@ -65,6 +73,7 @@ static inline void mmu_setup(void)
   	int i;
   	u32 reg;
 
+	arm_init_before_mmu();
   	/* Set up an identity-mapping for all 4GB, rw for everyone */
   	for (i = 0; i<   4096; i++)
   		page_table[i] = i<<   20 | (3<<   10) | 0x12;

Amicalement,

1. make sure that page table setup is not done multiple times
2. flush_dcache_all() is more appropriate while disabling cache than a range flush on the entire memory(flush_cache())

Provide a default implementation for flush_dcache_all() for backward compatibility and to avoid build issues.

Signed-off-by Aneesh V<[email protected]>

---
  arch/arm/lib/cache-cp15.c |    9 +++++++--
  arch/arm/lib/cache.c      |   13 ++++++++++++-
  2 files changed, 19 insertions(+), 3 deletions(-)

arch/arm/lib/cache-cp15.c +5 -0

index ca526fb..20aa993 100644
@@ -94,13 +94,18 @@ static inline void mmu_setup(void)
  	set_cr(reg | CR_M);
  }
 
+static int mmu_enabled(void)
+{
+	return get_cr()&  CR_M;
+}
+
  /* cache_bit must be either CR_I or CR_C */

arch/arm/lib/cache-cp15.c +1 -1

@@ -119,7 +124,7 @@ static void cache_disable(uint32_t cache_bit)
  			return;
  		/* if disabling data cache, disable mmu too */
  		cache_bit |= CR_M;
-		flush_cache(0, ~0);
+		flush_dcache_all();
  	}
  	reg = get_cr();
  	cp_delay();

arch/arm/lib/cache.c +1 -1

index 275b6e1..363609a 100644
@@ -25,7 +25,7 @@
 
  #include<common.h>
 
-void  __flush_cache(unsigned long dummy1, unsigned long dummy2)
+void  __flush_cache(unsigned long start, unsigned long size)
  {
  #if defined(CONFIG_OMAP2420) || defined(CONFIG_ARM1136)
  	void arm1136_cache_flush(void);

Hi Aneesh,

Le 22/12/2010 12:54, Aneesh V a ?crit :

Quoted 31 lines

1. make sure that page table setup is not done multiple times
2. flush_dcache_all() is more appropriate while disabling cache than a range flush on the entire memory(flush_cache())

Provide a default implementation for flush_dcache_all() for backward compatibility and to avoid build issues.

Signed-off-by Aneesh V<[email protected]>

---
   arch/arm/lib/cache-cp15.c |    9 +++++++--
   arch/arm/lib/cache.c      |   13 ++++++++++++-
   2 files changed, 19 insertions(+), 3 deletions(-)

arch/arm/lib/cache-cp15.c +5 -0

index ca526fb..20aa993 100644
@@ -94,13 +94,18 @@ static inline void mmu_setup(void)
   	set_cr(reg | CR_M);
   }
 
+static int mmu_enabled(void)
+{
+	return get_cr()&   CR_M;
+}
+
   /* cache_bit must be either CR_I or CR_C */
   static void cache_enable(uint32_t cache_bit)
   {
   	uint32_t reg;
 

arch/arm/lib/cache-cp15.c static inline void mmu_setup(void) · line 111

Quoted 6 lines

arch/arm/lib/cache-cp15.c +1 -1

   	/* The data cache is not active unless the mmu is enabled too */
-	if (cache_bit == CR_C)
+	if ((cache_bit == CR_C)&&   !mmu_enabled())
   		mmu_setup();
   	reg = get_cr();	/* get control reg. */
   	cp_delay();

Do you know why double MMU setups happen? Can we not fix the execution path and remove the second MMU setup call there, rather that catching t on the fly to ignore it?

Add support for some of the key maintenance operations
- Invalidate all
- Invalidate range
- Flush(clean & invalidate) all
- Flush range

arch/arm/include/asm/pl310.h new file +0 -0

Dear Aneesh V,

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

Add support for some of the key maintenance operations
- Invalidate all
- Invalidate range
- Flush(clean& invalidate) all
- Flush range

Can you please use a more descriptive subject, and commit message?

I have no idea what a "PL310" might be - is this a new board? Or a new SoC? or a new Ethernet controller?

And what exactly are "key maintenance operations"? Looks as if you were talking about basic cache operations?

Quoted 3 lines

arch/arm/include/asm/pl310.h new file +0 -0

...

arch/arm/include/asm/pl310.h line 39

Quoted 10 lines

arch/arm/include/asm/pl310.h new file +10 -0

+/* Register offsets */
+#define PL310_CACHE_TYPE		0x004
+#define PL310_AUX_CTRL			0x104
+
+#define PL310_CACHE_SYNC		0x730
+#define PL310_INVAL_LINE_PA		0x770
+#define PL310_INVAL_WAY			0x77C
+#define PL310_CLEAN_LINE_PA		0x7B0
+#define PL310_CLEAN_INVAL_WAY		0x7FC
+#define PL310_CLEAN_INVAL_LINE_PA	0x7F0

NAK. Please use a C struct instead.

arch/arm/lib/Makefile COBJS-y += cache.o · line 47

Quoted 11 lines

arch/arm/lib/Makefile +1 -0

@@ -42,6 +42,7 @@ COBJS-y	+= cache.o
  ifndef CONFIG_SYS_NO_CP15_CACHE
  COBJS-y	+= cache-cp15.o
  endif
+COBJS-$(CONFIG_SYS_USE_PL310) += cache-pl310.o
  COBJS-y	+= interrupts.o
  COBJS-y	+= reset.o

There is no documentation for CONFIG_SYS_USE_PL310, and there is no use of this variable.

Also, it seems CONFIG_SYS_PL310 would be more appropriate.

...

arch/arm/lib/Makefile

Quoted 4 lines

arch/arm/lib/Makefile +4 -0

+static void pl310_cache_sync(void)
+{
+	__raw_writel(0, CONFIG_SYS_PL310_BASE + PL310_CACHE_SYNC);
+}

Please use a proper C struct instead of base address plus offset. Please fix globally.

...

arch/arm/lib/Makefile

Quoted 3 lines

arch/arm/lib/Makefile +3 -0

+	for (pa = start; pa<  stop; pa = pa + line_size)
+		__raw_writel(pa, CONFIG_SYS_PL310_BASE +
+			     PL310_CLEAN_INVAL_LINE_PA);

Please use braces for multiline statements.

Best regards,

Wolfgang Denk

Dear Aneesh V,

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

adapt omap4 to the new layered cache maintenance framework

Signed-off-by Aneesh V<[email protected]>

arch/arm/cpu/armv7/omap4/board.c int arch_cpu_init(void) · line 135

Quoted 4 lines

arch/arm/cpu/armv7/omap4/board.c +4 -0

+/*
+ * Outer cache related functions
+ */
+#ifndef CONFIG_SYS_NO_DCACHE

Do we really need such a #define? Where is CONFIG_SYS_NO_DCACHE documented?

arch/arm/cpu/armv7/omap4/board.c int arch_cpu_init(void) · line 138

Quoted 4 lines

arch/arm/cpu/armv7/omap4/board.c +3 -0

+void v7_setup_outer_cache_ops(void)
+{
+#ifndef CONFIG_L2_OFF

Should this be CONFIG_L2_OFF or CONFIG_SYS_L2_OFF? Where is CONFIG_SYS_L2_OFF documented?

arch/arm/cpu/armv7/omap4/lowlevel_init.S lowlevel_init: · line 56

Quoted 13 lines

arch/arm/cpu/armv7/omap4/lowlevel_init.S +7 -0

@@ -45,3 +47,18 @@ lowlevel_init:
  	 */
  	bl	s_init
  	pop	{ip, pc}
+
+set_pl310_ctrl_reg:
+        PUSH	{r4-r11, lr}	@ save registers - ROM code may pollute
+				@ our registers
+        LDR	r12, =0x102	@ Set PL310 control register - value in R0
+        SMC	#0		@ call ROM Code API to set control register
+        POP	{r4-r11, pc}

Indentation by TAB, please.

Best regards,

Wolfgang Denk

A handful of board config files are using it, but in actual code it's only referenced by armv7 code.

Shall I change them all to CONFIG_SYS_L2_OFF

I shall add documentation.

Sure. I will take care in future. BTW, that was not caught by checkpatch.pl. Is there anyway to catch such errors using some tool?

adapt omap3 to the new layered cache maintenance framework

+/* Declarations */

Dear Aneesh V,

Quoted 1 line · In message<[email protected]> you wrote:

adapt omap3 to the new layered cache maintenance framework

...

Quoted 1 line

+/* Declarations */

Please drop this comment. Everybody sees what this is.

arch/arm/cpu/armv7/omap3/board.c void s_init(void) · line 179

Quoted 8 lines

arch/arm/cpu/armv7/omap3/board.c +2 -3

+#ifndef CONFIG_L2_OFF
  	/*
-	 * Writing to AuxCR in U-boot using SMI for GP DEV
-	 * Currently SMI in Kernel on ES2 devices seems to have an issue
-	 * Once that is resolved, we can postpone this config to kernel
+	 * Invalidate L2-cache from secure mode
  	 */

Why not change this into simple

	/* Invalidate L2-cache from secure mode */

?

...

arch/arm/cpu/armv7/omap3/board.c int checkboard (void) · line 300

Quoted 13 lines

arch/arm/cpu/armv7/omap3/board.c +13 -0

+static void omap3_emu_romcode_call(u32 service_id, u32 *parameters)
+{
+	u32 i, num_params = *parameters;
+	u32 *sram_scratch_space = (u32 *)OMAP3_PUBLIC_SRAM_SCRATCH_AREA;
+	/*
+	 * copy the parameters to an un-cached area to avoid coherency
+	 * issues
+	 */
+	for (i = 0; i<  num_params; i++) {
+		__raw_writel(*parameters, sram_scratch_space);
+		parameters++;
+		sram_scratch_space++;
+	}

Do you have unlimited storage there? Or should you add some check not to exceed some maximum size?

arch/arm/cpu/armv7/omap3/board.c int checkboard (void) · line 321

Quoted 7 lines

arch/arm/cpu/armv7/omap3/board.c +6 -0

+	} else {
+		struct emu_hal_params emu_romcode_params;
+		emu_romcode_params.num_params = 1;
+		emu_romcode_params.param1 = acr;
+		omap3_emu_romcode_call(OMAP3_EMU_HAL_API_WRITE_ACR,
+				       (u32 *)&emu_romcode_params);

Please add a blank line between declarations and code (fix globally).

arch/arm/cpu/armv7/omap3/board.c int checkboard (void) · line 339

Quoted 5 lines

arch/arm/cpu/armv7/omap3/board.c +4 -0

+static void omap3_setup_aux_cr(void)
+{
+	/* Workaround for Cortex-A8 errata: #454179 #430973
+	 *	Set "IBE" bit

...
Incorrect multiline comment style.

...

Quoted 8 lines

arch/arm/include/asm/arch-omap3/sys_proto.h +0 -0

index 4a28ba1..25f54ea 100644
@@ -27,6 +27,11 @@ typedef struct {
  	char *nand_string;
  } omap3_sysinfo;
 

arch/arm/include/asm/arch-omap3/sys_proto.h typedef struct { · line 33

Quoted 4 lines

arch/arm/include/asm/arch-omap3/sys_proto.h +4 -0

+struct __attribute__ ((__packed__)) emu_hal_params {
+	u32 num_params;
+	u32 param1;
+};

Why exactly do we need the "__attribute__ ((__packed__))" here?

Best regards,

Wolfgang Denk

Quoted 7 lines

arch/arm/include/asm/arch-omap3/sys_proto.h typedef struct { · line 33

Quoted 5 lines

arch/arm/include/asm/arch-omap3/sys_proto.h +4 -0

+struct __attribute__ ((__packed__)) emu_hal_params {
+	u32 num_params;
+	u32 param1;
+};

Why exactly do we need the "__attribute__ ((__packed__))" here?

Because a pointer to it has to be passed to ROM code and ROM code wouldn't expect any padding.

Dear Aneesh V,

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

Quoted 7 lines

arch/arm/include/asm/arch-omap3/sys_proto.h typedef struct { · line 33

Quoted 5 lines

arch/arm/include/asm/arch-omap3/sys_proto.h +4 -0

+struct __attribute__ ((__packed__)) emu_hal_params {
+	u32 num_params;
+	u32 param1;
+};

Why exactly do we need the "__attribute__ ((__packed__))" here?

Quoted 2 lines

Because a pointer to it has to be passed to ROM code and ROM code wouldn't expect any padding.

But padding would only be needed if there were any alignment requirements requiring it. There are none here.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

arch/arm/include/asm/arch-omap3/sys_proto.h typedef struct { · line 33

Quoted 5 lines

arch/arm/include/asm/arch-omap3/sys_proto.h +4 -0

+struct __attribute__ ((__packed__)) emu_hal_params {
+	u32 num_params;
+	u32 param1;
+};

Why exactly do we need the "__attribute__ ((__packed__))" here?

Quoted 5 lines

Quoted 2 lines

Because a pointer to it has to be passed to ROM code and ROM code wouldn't expect any padding.

But padding would only be needed if there were any alignment requirements requiring it. There are none here.

Yes. But, is that guaranteed? I didn't want to make it compiler

dependent. Isn't it safer to use '__attribute__ ((__packed__))'

adapt s5pc1xx to the new layered cache maintenance framework

Signed-off-by Aneesh V <[email protected]>

---

?arch/arm/cpu/armv7/s5pc1xx/cache.S ? ? ? ? ? ?| ? 86 +-----------------------
?arch/arm/cpu/armv7/s5pc1xx/clock.c ? ? ? ? ? ?| ? 12 ++++
?arch/arm/include/asm/arch-s5pc1xx/sys_proto.h | ? ?4 +-

?3 files changed, 18 insertions(+), 84 deletions(-)

arch/arm/cpu/armv7/s5pc1xx/clock.c +0 -0

index 98a27e5..da8b2d7 100644
@@ -26,6 +26,8 @@

?#include <asm/io.h>
?#include <asm/arch/clock.h>
?#include <asm/arch/clk.h>

arch/arm/cpu/armv7/s5pc1xx/clock.c +2 -0

+#include <asm/arch/sys_proto.h>
+#include <asm/armv7.h>

?#define CLK_M ?0
?#define CLK_D ?1

arch/arm/cpu/armv7/s5pc1xx/clock.c +0 -0

@@ -328,3 +330,13 @@ void s5p_clock_init(void)
 ? ? ? ?get_uart_clk = s5pc1xx_get_uart_clk;
 ? ? ? ?get_pwm_clk = s5pc1xx_get_pwm_clk;
 ?}

Quoted 5 lines

arch/arm/cpu/armv7/s5pc1xx/clock.c +5 -0

+
+#ifndef CONFIG_SYS_NO_DCACHE
+void v7_setup_outer_cache_ops(void)
+{
+#ifndef CONFIG_L2_OFF