Thanks Simon,
On 03/16/2017 03:06 PM, Simon Glass wrote:
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
oh yes, ok.
+/* 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 ?
No :-), thanks.
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?
sure.
+};
+#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
ok.
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?
this function returns the io register to perform required cache action like clean or clean & invalidate by sets/ways. The procedure to perform on these io register is same for cleaning & clean/invalidate.
I will add the comment in code.
+{
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?
C structures was the first choice but they were removed after v1 as per review comment & replaced with #defines. ok for the casting in #defines.
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?
armv7m is 32bit arch, cacheline size (32 bytes for cortex M7) can never be more than u32. Please let me know if i am missing something.
+{
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.
this function is to perform the required action like invalidate/clean on a range of cache addresses. I will add comment.
Can you use __used ?
I figured these attribute will not be required after using the cache prototypes of common.h like invalidate_dcache_range().
+{
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;elsetype = 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.
this function is to perform the required action like invalidate/clean on all cached addresses. I will add comment for it.
Cheers, Vikas
Regards, Simon .