From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Mon, 27 Mar 2017 23:27:08 +0200 Subject: [U-Boot] [PATCH v4 1/2] armv7m: add instruction & data cache support In-Reply-To: References: <1490644970-19266-1-git-send-email-vikas.manocha@st.com> <1490644970-19266-2-git-send-email-vikas.manocha@st.com> Message-ID: <0fb0a37f-68d5-9aa5-7ce5-cdbaf0cc398a@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 03/27/2017 10:41 PM, Vikas Manocha wrote: > Hi Marek, > > On 03/27/2017 01:34 PM, Marek Vasut wrote: >> On 03/27/2017 10:02 PM, Vikas Manocha wrote: >>> This patch adds armv7m instruction & data cache support. >>> >>> Signed-off-by: Vikas Manocha >>> cc: Christophe KERELLO >>> --- >>> >>> Changed in v4: >>> - invalidate_dcache_range() & flush_dcache_range() function added. >>> - blank lines added. >>> - comments added for registers, functions & barriers. >>> - register names changed for better clarity. >>> - typecasting moved to macro definitions. >>> >>> 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 | 3 +- >>> arch/arm/cpu/armv7m/cache.c | 336 ++++++++++++++++++++++++++++++++++++++++++ >>> arch/arm/include/asm/armv7m.h | 26 +++- >>> arch/arm/lib/Makefile | 2 + >>> 4 files changed, 363 insertions(+), 4 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 e1a6c40..93c9085 100644 >>> --- a/arch/arm/cpu/armv7m/Makefile >>> +++ b/arch/arm/cpu/armv7m/Makefile >>> @@ -6,6 +6,5 @@ >>> # >>> >>> extra-y := start.o >>> -obj-y += cpu.o >>> - >>> +obj-y += cpu.o cache.o >>> obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o >>> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c >>> new file mode 100644 >>> index 0000000..162cfe3 >>> --- /dev/null >>> +++ b/arch/arm/cpu/armv7m/cache.c >>> @@ -0,0 +1,336 @@ >>> +/* >>> + * (C) Copyright 2017 >>> + * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com. >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +/* Cache maintenance operation registers */ >>> + >>> +#define V7M_CACHE_REG_ICIALLU ((u32 *)(V7M_CACHE_MAINT_BASE + 0x00)) >> ^^^^^^^^^^^^^^^^^^^^^ >> Drop this whole part, >> it's a register offset, not address. Just calculate the address >> in-place. The cast is also not needed. > > These are fixed addresses in armv7m architecture. So what ? > In place calculations were replaced by > this fixed address macro as per your comment. And then one of the two things will happen: a) these addresses will be moved b) someone will look at the surrounding code, where _REG_ means register offset all over the place and will screw things up ... > casting was moved here from code as per Simon's suggestion on on v3. > >> >>> +#define INVAL_ICACHE_POU 0 >>> +#define V7M_CACHE_REG_ICIMVALU ((u32 *)(V7M_CACHE_MAINT_BASE + 0x08)) >>> +#define V7M_CACHE_REG_DCIMVAC ((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C)) >>> +#define V7M_CACHE_REG_DCISW ((u32 *)(V7M_CACHE_MAINT_BASE + 0x10)) >>> +#define V7M_CACHE_REG_DCCMVAU ((u32 *)(V7M_CACHE_MAINT_BASE + 0x14)) >>> +#define V7M_CACHE_REG_DCCMVAC ((u32 *)(V7M_CACHE_MAINT_BASE + 0x18)) >>> +#define V7M_CACHE_REG_DCCSW ((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C)) >>> +#define V7M_CACHE_REG_DCCIMVAC ((u32 *)(V7M_CACHE_MAINT_BASE + 0x20)) >>> +#define V7M_CACHE_REG_DCCISW ((u32 *)(V7M_CACHE_MAINT_BASE + 0x24)) >>> +#define WAYS_SHIFT 30 >>> +#define SETS_SHIFT 5 >>> + >>> +/* armv7m processor feature registers */ >>> + >>> +#define V7M_PROC_REG_CLIDR ((u32 *)(V7M_PROC_FTR_BASE + 0x00)) >>> +#define V7M_PROC_REG_CTR ((u32 *)(V7M_PROC_FTR_BASE + 0x04)) >>> +#define V7M_PROC_REG_CCSIDR ((u32 *)(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 >> >> Well use those macros in MASK_* above ... > > macros are being used, shifts are required after masking. Then look at the GENMASK() above and the hardcoded values ... >>> +#define V7M_PROC_REG_CSSELR ((u32 *)(V7M_PROC_FTR_BASE + 0x0C)) >>> +#define SEL_I_OR_D BIT(0) >>> + >>> +enum cache_type { >>> + DCACHE, >>> + ICACHE, >>> +}; >>> + >>> +/* PoU : Point of Unification, Poc: Point of Coherency */ >>> +enum cache_action { >>> + INVALIDATE_POU, /* i-cache invalidate by address */ >>> + INVALIDATE_POC, /* d-cache invalidate by address */ >>> + INVALIDATE_SET_WAY, /* d-cache invalidate by sets/ways */ >>> + FLUSH_POU, /* d-cache clean by address to the PoU */ >>> + FLUSH_POC, /* d-cache clean by address to the PoC */ >>> + FLUSH_SET_WAY, /* d-cache clean by sets/ways */ >>> + FLUSH_INVAL_POC, /* d-cache clean & invalidate by addr to PoC */ >>> + FLUSH_INVAL_SET_WAY, /* d-cache clean & invalidate by set/ways */ >>> +}; >>> + >>> +#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(V7M_PROC_REG_CCSIDR); >>> + >>> + cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT; >>> + cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT; >>> +} >>> + >>> +/* >>> + * Return the io register to perform required cache action like clean or clean >>> + * & invalidate by sets/ways. >>> + */ >>> +static u32 *get_action_reg_set_ways(enum cache_action action) >>> +{ >>> + switch (action) { >>> + case INVALIDATE_SET_WAY: >>> + return V7M_CACHE_REG_DCISW; >>> + case FLUSH_SET_WAY: >>> + return V7M_CACHE_REG_DCCSW; >>> + case FLUSH_INVAL_SET_WAY: >>> + return V7M_CACHE_REG_DCCISW; >>> + default: >>> + break; >>> + }; >>> + >>> + return NULL; >>> +} >>> + >>> +/* >>> + * Return the io register to perform required cache action like clean or clean >>> + * & invalidate by adddress or range. >>> + */ >>> +static u32 *get_action_reg_range(enum cache_action action) >>> +{ >>> + switch (action) { >>> + case INVALIDATE_POU: >>> + return V7M_CACHE_REG_ICIMVALU; >>> + case INVALIDATE_POC: >>> + return V7M_CACHE_REG_DCIMVAC; >>> + case FLUSH_POU: >>> + return V7M_CACHE_REG_DCCMVAU; >>> + case FLUSH_POC: >>> + return V7M_CACHE_REG_DCCMVAC; >>> + case FLUSH_INVAL_POC: >>> + return V7M_CACHE_REG_DCCIMVAC; >>> + default: >>> + break; >>> + } >>> + >>> + return NULL; >>> +} >>> + >>> +static u32 get_cline_size(enum cache_type type) >>> +{ >>> + u32 size; >>> + >>> + if (type == DCACHE) >>> + clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D)); >>> + else if (type == ICACHE) >>> + setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D)); >>> + /* Make sure cache selection is effective for next memory access */ >>> + dsb(); >>> + >>> + size = readl(V7M_PROC_REG_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; >>> +} >>> + >>> +/* Perform the action like invalidate/clean on a range of cache addresses */ >>> +static int action_cache_range(enum cache_action action, u32 start_addr, >>> + int64_t size) >>> +{ >>> + u32 cline_size; >>> + u32 *action_reg; >>> + enum cache_type type; >> >> Does this ever check whether start_addr and size is unaligned ? > > address alignment is being done below before cache action. Ah, so we hide bugs which are introduced by unaligned accesses ? Well please remove that and add address alignment check like the rest ... >> Also, you use u32 all over the place, but size is signed-i64 ? Why? > > Yes it is for condition "size > cline_size" when range/size is 4GB. Divide that by page size and you no longer need to use 64bit type on 32bit system ... >>> + 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); >>> + debug("total size for cache action = %llx\n", size); >>> + do { >>> + writel(start_addr, action_reg); >>> + size -= cline_size; >>> + start_addr += cline_size; >>> + } while (size > cline_size); >>> + >>> + /* Make sure cache action is effective for next memory access */ >>> + dsb(); >>> + isb(); /* Make sure instruction stream sees it */ >>> + debug("cache action on range done\n"); >>> + >>> + return 0; >>> +} >> >> [...] >> >>> >>> -#define V7M_SCB_BASE 0xE000ED00 >>> -#define V7M_MPU_BASE 0xE000ED90 >>> +/* armv7m fixed base addresses */ >>> +#define V7M_SCS_BASE 0xE000E000 >> >> Is all of this really used in the code ? > > Not all of them, it is for the sake of armv7m address space completeness. Not sure we need them all then ... -- Best regards, Marek Vasut