On 03/13/2017 10:45 PM, vikas wrote: > 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.
Clearly, I'm not getting through here. Something like CACHE_V7M_REG_FOO is much easier to grep for than FOO. >>> +#define WAYS_SHIFT 30 >>> +#define SETS_SHIFT 5 >> >> Is this always 30 and 5 , on all CPUs ? > > Yes for all armv7m arch. OK >>> +/* 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. Eh? This is just a value, you can use it directly ... [...] >>> + 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. So basically this is a workaround to silence the compiler which correctly warns you about dead code ? I think you know what to do (hint: remove dead code ...) >> >> 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. Effective how ? >> >>> + 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. Could use better explanation / comment ... >> >>> + 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" ? It reduces indent ... >> >>> + 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. Most of which should come from DT, but OK ... >> >>> #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. And unlike the verbose comments above, which describe what the register actually does, they are totally useless ... -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot