On 03/16/2017 10:39 PM, vikas wrote: > Thanks Marek, > > On 03/16/2017 02:40 PM, Marek Vasut wrote: >> 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. > > ok, i will make them V7M_IC_* for instruction cache & V7M_DC_* for data cache > specific regs.
Which is still pretty cryptic ... >>>>> +#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 ... > > done in v3, i will send the v4 with rest of the modifications. Could you give the patch a few days on the list to gather feedback ? I believe I warned you about this before already, but the maintainers are already saturated by patches, sending one revision after the other does NOT help anyone and only congests the maintainers further. >> [...] >> >>>>> + 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 ...) > > ok. > >> >>>> >>>> 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 ? > > dsb barrier makes sure outstanding memory transactions are completed before > next instructions. > which means in this case the required cache memory is selected before > following action on sets/ways. Shouldn't IO accessors already contain such barrier instructions too ? This should be in a comment in the code anyway ... [...] -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot