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.manocha@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.manocha@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.
+#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.
[...]
- 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.
- 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 ...
dsb for same reason as above & isb to flush the instruction 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" ?
It reduces indent ...
thanks, i will invert it in v4.
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 ...
done in v3, i will send v4 with rest of modifications.
Cheers, Vikas