Re: [U-Boot] [PATCH v3 1/2] armv7m: add instruction & data cache support

Hi Vikas,

On 14 March 2017 at 11:27, Vikas Manocha vikas.manocha@st.com wrote:

This patch adds armv7m instruction & data cache support.

Signed-off-by: Vikas Manocha vikas.manocha@st.com cc: Christophe KERELLO christophe.kerello@st.com

---

Changed in v3:

• uint32 replcaed with u32.
• multiple read of hardware register replaced with single.
• pointers replaced with macros for base address.
• register names added as comment for system control block registers.

Changed in v2:

• changed strucures for memory mapped cache registers to macros
• added lines better readability.
• replaced magic numbers with macros.

arch/arm/cpu/armv7m/Makefile | 2 +- arch/arm/cpu/armv7m/cache.c | 291 ++++++++++++++++++++++++++++++++++++++++++ arch/arm/include/asm/armv7m.h | 26 +++- arch/arm/lib/Makefile | 2 + 4 files changed, 318 insertions(+), 3 deletions(-) create mode 100644 arch/arm/cpu/armv7m/cache.c

diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile index aff60e8..41efe11 100644 --- a/arch/arm/cpu/armv7m/Makefile +++ b/arch/arm/cpu/armv7m/Makefile @@ -6,4 +6,4 @@ #

extra-y := start.o -obj-y += cpu.o +obj-y += cpu.o cache.o diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c new file mode 100644 index 0000000..9021525 --- /dev/null +++ b/arch/arm/cpu/armv7m/cache.c @@ -0,0 +1,291 @@ +/*

• • (C) Copyright 2017
• • Vikas Manocha, ST Micoelectronics, vikas.manocha@st.com.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <asm/armv7m.h> +#include <asm/io.h> +#include <errno.h>

put this one below common.h

+/* Cache maintenance operation registers */

+#define IC_IALLU (V7M_CACHE_MAINT_BASE + 0x00) +#define INVAL_ICACHE_POU 0 +#define IC_IMVALU (V7M_CACHE_MAINT_BASE + 0x08) +#define DC_IMVAC (V7M_CACHE_MAINT_BASE + 0x0C) +#define DC_ISW (V7M_CACHE_MAINT_BASE + 0x10) +#define DC_CMVAU (V7M_CACHE_MAINT_BASE + 0x14) +#define DC_CMVAC (V7M_CACHE_MAINT_BASE + 0x18) +#define DC_CSW (V7M_CACHE_MAINT_BASE + 0x1C) +#define DC_CIMVAC (V7M_CACHE_MAINT_BASE + 0x20) +#define DC_CISW (V7M_CACHE_MAINT_BASE + 0x24) +#define WAYS_SHIFT 30 +#define SETS_SHIFT 5

+/* armv7m processor feature registers */

+#define CLIDR (V7M_PROC_FTR_BASE + 0x00) +#define CTR (V7M_PROC_FTR_BASE + 0x04) +#define CCSIDR (V7M_PROC_FTR_BASE + 0x08) +#define MASK_NUM_WAYS GENMASK(12, 3) +#define MASK_NUM_SETS GENMASK(27, 13) +#define CLINE_SIZE_MASK GENMASK(2, 0) +#define NUM_WAYS_SHIFT 3 +#define NUM_SETS_SHIFT 13 +#define CSSELR (V7M_PROC_FTR_BASE + 0x0C) +#define SEL_I_OR_D BIT(0)

+enum cache_type {

•   DCACHE = 0,
  

Do you need the =0 ?

•   ICACHE,
  

+};

+/* PoU : Point of Unification, Poc: Point of Coherency */ +enum cache_action {

•   INVALIDATE_POU,         /* for i-cache invalidate by address */
  

•   INVALIDATE_POC,         /* for d-cache invalidate by address */
  

•   INVALIDATE_SET_WAY,     /* for d-cache invalidate by sets/ways */
  

•   FLUSH_POU,
  

•   FLUSH_POC,
  

•   FLUSH_SET_WAY,
  

•   FLUSH_INVAL_POC,
  

•   FLUSH_INVAL_SET_WAY,
  

Can you add comments for the rest?

+};

+#ifndef CONFIG_SYS_DCACHE_OFF +struct dcache_config {

•   u32 ways;
  

•   u32 sets;
  

+};

+static void get_cache_ways_sets(struct dcache_config *cache) +{

•   u32 cache_size_id = readl(CCSIDR);
  

blank line here

•   cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
  

•   cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
  

+}

+static u32 *get_action_reg_set_ways(enum cache_action action)

Can you please add a function comment? What does this return?

+{

•   switch (action) {
  

•   case INVALIDATE_SET_WAY:
  

•           return (u32 *)DC_ISW;
  

Can you drop these casts by using a C structure or by putting thecast in the #define?

•   case FLUSH_SET_WAY:
  

•           return (u32 *)DC_CSW;
  

•   case FLUSH_INVAL_SET_WAY:
  

•           return (u32 *)DC_CISW;
  

•   default:
  

•           break;
  

•   };
  

•   return NULL;
  

+}

+static u32 *get_action_reg_range(enum cache_action action) +{

•   switch (action) {
  

•   case INVALIDATE_POU:
  

•           return (u32 *)IC_IMVALU;
  

•   case INVALIDATE_POC:
  

•           return (u32 *)DC_IMVAC;
  

•   case FLUSH_POU:
  

•           return (u32 *)DC_CMVAU;
  

•   case FLUSH_POC:
  

•           return (u32 *)DC_CMVAC;
  

•   case FLUSH_INVAL_POC:
  

•           return (u32 *)DC_CIMVAC;
  

•   default:
  

•           break;
  

•   }
  

•   return NULL;
  

+}

+static u32 get_cline_size(enum cache_type type)

Why u32? Should it be uint or ulong?

+{

•   u32 size;
  

•   if (type == DCACHE)
  

•           clrbits_le32(CSSELR, BIT(SEL_I_OR_D));
  

•   else if (type == ICACHE)
  

•           setbits_le32(CSSELR, BIT(SEL_I_OR_D));
  

•   dsb();
  

•   size = readl(CCSIDR) & CLINE_SIZE_MASK;
  

•   /* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
  

•   size = 1 << (size + 2);
  

•   debug("cache line size is %d\n", size);
  

•   return size;
  

+}

+static __attribute__((unused)) int action_cache_range(enum cache_action action,

•                u32 start_addr, int64_t size)
  

Function comment.

Can you use __used ?

+{

•   u32 cline_size;
  

•   u32 *action_reg;
  

•   enum cache_type type;
  

•   action_reg = get_action_reg_range(action);
  

•   if (!action_reg)
  

•           return -EINVAL;
  

•   if (action == INVALIDATE_POU)
  

•           type = ICACHE;
  

•   else
  

•           type = DCACHE;
  

•   /* Cache line size is minium size for the cache action */
  

•   cline_size = get_cline_size(type);
  

•   /* Align start address to cache line boundary */
  

•   start_addr &= ~(cline_size - 1);
  

•   do {
  

•           writel(start_addr, action_reg);
  

•           size -= cline_size;
  

•           start_addr += cline_size;
  

•   } while (size > cline_size);
  

•   dsb();
  

•   isb();
  

•   debug("cache action on range done\n");
  

•   return 0;
  

+}

+static int action_dcache_all(enum cache_action action)

Function comment.

Regards, Simon