From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vikas MANOCHA Date: Thu, 30 Mar 2017 16:43:40 +0000 Subject: [U-Boot] [PATCH v4 1/2] armv7m: add instruction & data cache support In-Reply-To: <2b699d64-c3c3-8b94-19ee-0cfcc63bbcb4@st.com> References: <1490644970-19266-1-git-send-email-vikas.manocha@st.com> <1490644970-19266-2-git-send-email-vikas.manocha@st.com> <0fb0a37f-68d5-9aa5-7ce5-cdbaf0cc398a@denx.de> <2b699d64-c3c3-8b94-19ee-0cfcc63bbcb4@st.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Marek, > -----Original Message----- > From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Vikas Manocha > Sent: Monday, March 27, 2017 2:33 PM > To: Marek Vasut ; u-boot at lists.denx.de > Cc: Christophe KERELLO ; Toshifumi NISHINAGA ; Stephen Warren > > Subject: Re: [U-Boot] [PATCH v4 1/2] armv7m: add instruction & data cache support > > Hi Marek, > > On 03/27/2017 02:27 PM, Marek Vasut wrote: > > 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 ? > > So, no need to calculate in-plcae. It was your suggestion also. Let me know if there is some open point for this patch. Cheers, Vikas > > > > >> 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 ... > > moving these addresses...its fixed by architecture. > > > > >> 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 ... > > hardcoded in GENMASK...really ? > > > > >>>> +#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 ... > > which bug ? user passing unaligned address ? > others are doing the same. > > > > >>> 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 ... > > page size... its armv7m. > > > > >>>> + 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 ... > > thats what i said, not needed all for now. They are there for sake of completeness, what's the harm ? > > Cheers, > Vikas > > > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > https://lists.denx.de/listinfo/u-boot