Thanks Marek, On 03/11/2017 10:02 PM, Marek Vasut wrote: > On 03/12/2017 01:13 AM, Vikas Manocha wrote: >> This patch adds armv7m instruction & data cache support. >> >> Signed-off-by: Vikas Manocha <vikas.mano...@st.com> >> --- >> >> Changed in v2: >> - changed strucures for memory mapped cache registers to MACROs > > Macro is written in lowercase, FYI ...
ok. > >> - added lines better readability. >> - replaced magic numbers with MACROs. >> >> arch/arm/cpu/armv7m/Makefile | 2 +- >> arch/arm/cpu/armv7m/cache.c | 294 >> ++++++++++++++++++++++++++++++++++++++++++ >> arch/arm/include/asm/armv7m.h | 26 +++- >> arch/arm/lib/Makefile | 2 + >> 4 files changed, 321 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..cc17366 >> --- /dev/null >> +++ b/arch/arm/cpu/armv7m/cache.c >> @@ -0,0 +1,294 @@ >> +/* >> + * (C) Copyright 2017 >> + * Vikas Manocha, ST Micoelectronics, vikas.mano...@st.com. >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <asm/armv7m.h> >> +#include <asm/io.h> >> +#include <errno.h> >> + >> +/* Cache maintenance operation registers */ >> + >> +#define IC_IALLU 0x00 >> +#define INVAL_ICACHE_POU 0 >> + >> +#define IC_IMVALU 0x08 >> +#define DC_IMVAC 0x0C >> +#define DC_ISW 0x10 >> +#define DC_CMVAU 0x14 >> +#define DC_CMVAC 0x18 >> +#define DC_CSW 0x1C >> +#define DC_CIMVAC 0x20 >> +#define DC_CISW 0x24 > > Would be nice to have some more distinguishing name here, so one can > easily git grep for those reg names and make sense of their name without > reading the datasheet . these names are consistent with the arch manual to help relating them with manual. > >> +#define WAYS_SHIFT 30 >> +#define SETS_SHIFT 5 > > Is this always 30 and 5 , on all CPUs ? Yes for all armv7m arch. > >> +/* armv7m processor feature registers */ >> + >> +#define CLIDR 0x00 >> +#define CTR 0x04 >> + >> +#define CCSIDR 0x08 >> +#define MASK_NUM_WAYS GENMASK(12, 3) >> +#define MASK_NUM_SETS GENMASK(27, 13) >> +#define NUM_WAYS_SHIFT 3 >> +#define NUM_SETS_SHIFT 13 >> + >> +#define CSSELR 0x0C >> +#define SEL_I_OR_D BIT(0) >> + >> +void *const v7m_cache_maint = (void *)V7M_CACHE_MAINT_BASE; >> +void *const v7m_processor = (void *)V7M_PROC_FTR_BASE; > > Needed ? Why don't you just use the macro directly ? Yes it is possible. I was trying to avoid typecasting macro to pointer each time before passing to functions required it as address pointer. > >> + >> + >> +enum cache_type { >> + DCACHE = 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, >> +}; >> + >> +#ifndef CONFIG_SYS_DCACHE_OFF >> +struct dcache_config { >> + uint32_t ways; >> + uint32_t sets; > > u32 , this is not userspace ... ok, i will change all. > >> +}; >> + >> +static void get_cache_ways_sets(struct dcache_config *cache) >> +{ >> + cache->ways = (readl(v7m_processor + CCSIDR) & MASK_NUM_WAYS) >> + >> NUM_WAYS_SHIFT; >> + cache->sets = (readl(v7m_processor + CCSIDR) & MASK_NUM_SETS) >> + >> NUM_SETS_SHIFT; > > Do you need to read the reg twice ? Good point, we can read once in temp & find out ways & sets. > >> +} >> + >> +static uint32_t *get_action_reg_set_ways(enum cache_action action) >> +{ >> + switch (action) { >> + case INVALIDATE_SET_WAY: >> + return v7m_cache_maint + DC_ISW; >> + case FLUSH_SET_WAY: >> + return v7m_cache_maint + DC_CSW; >> + case FLUSH_INVAL_SET_WAY: >> + return v7m_cache_maint + DC_CISW; >> + default: >> + break; >> + }; >> + >> + return NULL; >> +} >> + >> +static uint32_t *get_action_reg_range(enum cache_action action) >> +{ >> + switch (action) { >> + case INVALIDATE_POU: >> + return v7m_cache_maint + IC_IMVALU; >> + case INVALIDATE_POC: >> + return v7m_cache_maint + DC_IMVAC; >> + case FLUSH_POU: >> + return v7m_cache_maint + DC_CMVAU; >> + case FLUSH_POC: >> + return v7m_cache_maint + DC_CMVAC; >> + case FLUSH_INVAL_POC: >> + return v7m_cache_maint + DC_CIMVAC; >> + default: >> + break; >> + } >> + >> + return NULL; >> +} >> + >> +static uint32_t get_cline_size(enum cache_type type) >> +{ >> + uint32_t size; >> + >> + if (type == DCACHE) >> + clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D)); >> + else if (type == ICACHE) >> + setbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D)); >> + dsb(); >> + >> + size = readl(v7m_processor + CCSIDR) & GENMASK(2, 0); > > define the mask as some macro .... ok > >> + size = size * 4 + 4; /* 0 means 4, 1 means 8 & so on */ > > 2 means 12 or 16 ? The comment is useless ... > > Is the size basically 1 << (size + 2) ? agree, I will add better comment in v3. > >> + debug("cache line size is %d\n", size); >> + >> + return size; >> +} >> + >> +int action_cache_range(enum cache_action action, uint32_t start_addr, >> + int64_t size) > > static ? this function at present is not being used as we are invalidating/flushing all cache but helper function to flush/invalidate parts/range of cache. Making it static leads to "function not used" compilation warning. attribute "unused" can be used also but not sure... Please suggest. > > You're never checking if start_addr and size are cache-line aligned , > see arm926ejs and armv7a > >> +{ >> + uint32_t cline_size; >> + uint32_t *action_reg; > > u32 , fix globally > >> + 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); >> + do { >> + writel(start_addr, action_reg); >> + size -= cline_size; >> + start_addr += cline_size; >> + } while (size > cline_size); >> + debug("cache action on range done\n"); >> + dsb(); >> + isb(); >> + >> + return 0; >> +} >> + >> +static int action_dcache_all(enum cache_action action) >> +{ >> + struct dcache_config cache; >> + uint32_t *action_reg; >> + int i, j; >> + >> + action_reg = get_action_reg_set_ways(action); >> + if (!action_reg) >> + return -EINVAL; >> + >> + clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D)); >> + dsb(); > > Needed ? Yes to make cache selection effective. > >> + get_cache_ways_sets(&cache); /* Get number of ways & sets */ >> + debug("cache: ways= %d, sets= %d\n", cache.ways + 1, cache.sets + 1); >> + for (i = cache.sets; i >= 0; i--) { >> + for (j = cache.ways; j >= 0; j--) { >> + writel((j << WAYS_SHIFT) | (i << SETS_SHIFT), >> + action_reg); >> + } >> + } >> + dsb(); >> + isb(); > > Are all those barriers needed ? Yes, to make the write effective & flush the pipeline. > >> + return 0; >> +} >> + >> +void dcache_enable(void) >> +{ >> + if (dcache_status()) /* return if cache already enabled */ >> + return; >> + >> + if (action_dcache_all(INVALIDATE_SET_WAY)) { >> + printf("ERR: D-cache not enabled\n"); >> + return; >> + } >> + >> + setbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_DCACHE)); >> + dsb(); >> + isb(); >> +} >> + >> +void dcache_disable(void) >> +{ >> + /* if dcache is enabled-> dcache disable & then flush */ >> + if (dcache_status()) { > > Invert the condition here ... not sure if it is helpful to check "if cache is disabled" instead of present test of "if cache is enabled" ? > >> + if (action_dcache_all(FLUSH_SET_WAY)) { >> + printf("ERR: D-cahe not flushed\n"); >> + return; >> + } >> + >> + clrbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_DCACHE)); >> + dsb(); >> + isb(); >> + } >> +} >> + >> +int dcache_status(void) >> +{ >> + return (readl(&V7M_SCB->ccr) & BIT(V7M_CCR_DCACHE)) != 0; >> +} >> + >> +#else >> +void dcache_enable(void) >> +{ >> + return; >> +} >> + >> +void dcache_disable(void) >> +{ >> + return; >> +} >> + >> +int dcache_status(void) >> +{ >> + return 0; >> +} >> +#endif >> + >> +#ifndef CONFIG_SYS_ICACHE_OFF >> + >> +void invalidate_icache_all(void) >> +{ >> + writel(INVAL_ICACHE_POU, v7m_cache_maint + IC_IALLU); >> + dsb(); >> + isb(); >> +} >> + >> +void icache_enable(void) >> +{ >> + if (icache_status()) >> + return; >> + >> + invalidate_icache_all(); >> + setbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_ICACHE)); >> + dsb(); >> + isb(); >> +} >> + >> +int icache_status(void) >> +{ >> + return (readl(&V7M_SCB->ccr) & BIT(V7M_CCR_ICACHE)) != 0; >> +} >> + >> +void icache_disable(void) >> +{ >> + isb(); >> + clrbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_ICACHE)); >> + isb(); >> +} >> +#else >> +void icache_enable(void) >> +{ >> + return; >> +} >> + >> +void icache_disable(void) >> +{ >> + return; >> +} >> + >> +int icache_status(void) >> +{ >> + return 0; >> +} >> +#endif >> + >> +void enable_caches(void) >> +{ >> +#ifndef CONFIG_SYS_ICACHE_OFF >> + icache_enable(); >> +#endif >> +#ifndef CONFIG_SYS_DCACHE_OFF >> + dcache_enable(); >> +#endif >> +} >> diff --git a/arch/arm/include/asm/armv7m.h b/arch/arm/include/asm/armv7m.h >> index 54d8a2b..67cb0e4 100644 >> --- a/arch/arm/include/asm/armv7m.h >> +++ b/arch/arm/include/asm/armv7m.h >> @@ -16,8 +16,15 @@ >> .thumb >> #endif >> >> -#define V7M_SCB_BASE 0xE000ED00 >> -#define V7M_MPU_BASE 0xE000ED90 >> +/* armv7m fixed base addresses */ >> +#define V7M_SCS_BASE 0xE000E000 >> +#define V7M_NVIC_BASE (V7M_SCS_BASE + 0x0100) >> +#define V7M_SCB_BASE (V7M_SCS_BASE + 0x0D00) >> +#define V7M_PROC_FTR_BASE (V7M_SCS_BASE + 0x0D78) >> +#define V7M_MPU_BASE (V7M_SCS_BASE + 0x0D90) >> +#define V7M_FPU_BASE (V7M_SCS_BASE + 0x0F30) >> +#define V7M_CACHE_MAINT_BASE (V7M_SCS_BASE + 0x0F50) >> +#define V7M_ACCESS_CNTL_BASE (V7M_SCS_BASE + 0x0F90) > > Does all this stuff need to be in global namespace ? No. I added all armv7m architecture stuff at one place to have complete view just like soc's peripherals base addresses defines. > >> #define V7M_SCB_VTOR 0x08 >> >> @@ -27,6 +34,18 @@ struct v7m_scb { >> uint32_t icsr; /* Interrupt Control and State Register */ >> uint32_t vtor; /* Vector Table Offset Register */ >> uint32_t aircr; /* App Interrupt and Reset Control Register */ >> + uint32_t scr; /* offset 0x10 */ >> + uint32_t ccr; /* offset 0x14 */ >> + uint32_t shpr1; /* offset 0x18 */ >> + uint32_t shpr2; /* offset 0x1c */ >> + uint32_t shpr3; /* offset 0x20 */ >> + uint32_t shcrs; /* offset 0x24 */ >> + uint32_t cfsr; /* offset 0x28 */ >> + uint32_t hfsr; /* offset 0x2C */ >> + uint32_t res; /* offset 0x30 */ >> + uint32_t mmar; /* offset 0x34 */ >> + uint32_t bfar; /* offset 0x38 */ >> + uint32_t afsr; /* offset 0x3C */ > > The comments are real useless compared to the previous comments in this > block ... They provide the cross-check of address offsets & help in adding space of reserved area. I will add names of registers also in v3. Cheers, Vikas > >> }; >> #define V7M_SCB ((struct v7m_scb *)V7M_SCB_BASE) >> >> @@ -39,6 +58,9 @@ struct v7m_scb { >> >> #define V7M_ICSR_VECTACT_MSK 0xFF >> >> +#define V7M_CCR_DCACHE 16 >> +#define V7M_CCR_ICACHE 17 >> + >> struct v7m_mpu { >> uint32_t type; /* Type Register */ >> uint32_t ctrl; /* Control Register */ >> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >> index 166fa9e..52b36b3 100644 >> --- a/arch/arm/lib/Makefile >> +++ b/arch/arm/lib/Makefile >> @@ -55,8 +55,10 @@ endif >> >> obj-y += cache.o >> ifndef CONFIG_ARM64 >> +ifndef CONFIG_CPU_V7M >> obj-y += cache-cp15.o >> endif >> +endif >> >> obj-y += psci-dt.o >> >> > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot