[U-Boot] [PATCH v2 0/2] add armv7m cache support
This patchset adds armv7m instruction/data caches support & enable it for stm32f7.
Changed in v2: - changed strucures for memory mapped cache registers to MACROs - added lines better readability. - replaced magic numbers with MACROs.
Vikas Manocha (2): armv7m: add instruction & data cache support stm32f7: enable instruction & data cache
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 + arch/arm/mach-stm32/stm32f7/soc.c | 2 + include/configs/stm32f746-disco.h | 4 +- 6 files changed, 324 insertions(+), 6 deletions(-) create mode 100644 arch/arm/cpu/armv7m/cache.c
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 - 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 +#define WAYS_SHIFT 30 +#define SETS_SHIFT 5 + +/* 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; + + +enum cache_type { + DCACHE = 0, + ICACHE, +}; + +/* PoU : Point of Unification, Poc: Point of Coherency */ +enum cache_action { + INVALIDATE_POU, /* for i-cache invalidate by address */ + INVALIDATE_POC, /* for d-cache invalidate by address */ + INVALIDATE_SET_WAY, /* for d-cache invalidate by sets/ways */ + FLUSH_POU, + FLUSH_POC, + FLUSH_SET_WAY, + FLUSH_INVAL_POC, + FLUSH_INVAL_SET_WAY, +}; + +#ifndef CONFIG_SYS_DCACHE_OFF +struct dcache_config { + uint32_t ways; + uint32_t sets; +}; + +static void get_cache_ways_sets(struct dcache_config *cache) +{ + cache->ways = (readl(v7m_processor + CCSIDR) & MASK_NUM_WAYS) + >> NUM_WAYS_SHIFT; + cache->sets = (readl(v7m_processor + CCSIDR) & MASK_NUM_SETS) + >> NUM_SETS_SHIFT; +} + +static uint32_t *get_action_reg_set_ways(enum cache_action action) +{ + switch (action) { + case INVALIDATE_SET_WAY: + return v7m_cache_maint + DC_ISW; + case FLUSH_SET_WAY: + return v7m_cache_maint + DC_CSW; + case FLUSH_INVAL_SET_WAY: + return v7m_cache_maint + DC_CISW; + default: + break; + }; + + return NULL; +} + +static uint32_t *get_action_reg_range(enum cache_action action) +{ + switch (action) { + case INVALIDATE_POU: + return v7m_cache_maint + IC_IMVALU; + case INVALIDATE_POC: + return v7m_cache_maint + DC_IMVAC; + case FLUSH_POU: + return v7m_cache_maint + DC_CMVAU; + case FLUSH_POC: + return v7m_cache_maint + DC_CMVAC; + case FLUSH_INVAL_POC: + return v7m_cache_maint + DC_CIMVAC; + default: + break; + } + + return NULL; +} + +static uint32_t get_cline_size(enum cache_type type) +{ + uint32_t size; + + if (type == DCACHE) + clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D)); + else if (type == ICACHE) + setbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D)); + dsb(); + + size = readl(v7m_processor + CCSIDR) & GENMASK(2, 0); + size = size * 4 + 4; /* 0 means 4, 1 means 8 & so on */ + 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) +{ + uint32_t cline_size; + uint32_t *action_reg; + 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(); + 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(); + + 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()) { + 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)
#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 */ }; #define V7M_SCB ((struct v7m_scb *)V7M_SCB_BASE)
@@ -39,6 +58,9 @@ struct v7m_scb {
#define V7M_ICSR_VECTACT_MSK 0xFF
+#define V7M_CCR_DCACHE 16 +#define V7M_CCR_ICACHE 17 + struct v7m_mpu { uint32_t type; /* Type Register */ uint32_t ctrl; /* Control Register */ diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 166fa9e..52b36b3 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -55,8 +55,10 @@ endif
obj-y += cache.o ifndef CONFIG_ARM64 +ifndef CONFIG_CPU_V7M obj-y += cache-cp15.o endif +endif
obj-y += psci-dt.o
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 ...
- 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 .
+#define WAYS_SHIFT 30 +#define SETS_SHIFT 5
Is this always 30 and 5 , on all CPUs ?
+/* 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 ?
+enum cache_type {
- DCACHE = 0,
- ICACHE,
+};
+/* PoU : Point of Unification, Poc: Point of Coherency */ +enum cache_action {
- INVALIDATE_POU, /* for i-cache invalidate by address */
- INVALIDATE_POC, /* for d-cache invalidate by address */
- INVALIDATE_SET_WAY, /* for d-cache invalidate by sets/ways */
- FLUSH_POU,
- FLUSH_POC,
- FLUSH_SET_WAY,
- FLUSH_INVAL_POC,
- FLUSH_INVAL_SET_WAY,
+};
+#ifndef CONFIG_SYS_DCACHE_OFF +struct dcache_config {
- uint32_t ways;
- uint32_t sets;
u32 , this is not userspace ...
+};
+static void get_cache_ways_sets(struct dcache_config *cache) +{
- cache->ways = (readl(v7m_processor + CCSIDR) & MASK_NUM_WAYS)
>> NUM_WAYS_SHIFT;- cache->sets = (readl(v7m_processor + CCSIDR) & MASK_NUM_SETS)
>> NUM_SETS_SHIFT;
Do you need to read the reg twice ?
+}
+static uint32_t *get_action_reg_set_ways(enum cache_action action) +{
- switch (action) {
- case INVALIDATE_SET_WAY:
return v7m_cache_maint + DC_ISW;- case FLUSH_SET_WAY:
return v7m_cache_maint + DC_CSW;- case FLUSH_INVAL_SET_WAY:
return v7m_cache_maint + DC_CISW;- default:
break;- };
- return NULL;
+}
+static uint32_t *get_action_reg_range(enum cache_action action) +{
- switch (action) {
- case INVALIDATE_POU:
return v7m_cache_maint + IC_IMVALU;- case INVALIDATE_POC:
return v7m_cache_maint + DC_IMVAC;- case FLUSH_POU:
return v7m_cache_maint + DC_CMVAU;- case FLUSH_POC:
return v7m_cache_maint + DC_CMVAC;- case FLUSH_INVAL_POC:
return v7m_cache_maint + DC_CIMVAC;- default:
break;- }
- return NULL;
+}
+static uint32_t get_cline_size(enum cache_type type) +{
- uint32_t size;
- if (type == DCACHE)
clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));- else if (type == ICACHE)
setbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));- dsb();
- size = readl(v7m_processor + CCSIDR) & GENMASK(2, 0);
define the mask as some macro ....
- size = size * 4 + 4; /* 0 means 4, 1 means 8 & so on */
2 means 12 or 16 ? The comment is useless ...
Is the size basically 1 << (size + 2) ?
- 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 ?
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 ?
- 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 ?
- 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 ...
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 ?
#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 ...
}; #define V7M_SCB ((struct v7m_scb *)V7M_SCB_BASE)
@@ -39,6 +58,9 @@ struct v7m_scb {
#define V7M_ICSR_VECTACT_MSK 0xFF
+#define V7M_CCR_DCACHE 16 +#define V7M_CCR_ICACHE 17
struct v7m_mpu { uint32_t type; /* Type Register */ uint32_t ctrl; /* Control Register */ diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 166fa9e..52b36b3 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -55,8 +55,10 @@ endif
obj-y += cache.o ifndef CONFIG_ARM64 +ifndef CONFIG_CPU_V7M obj-y += cache-cp15.o endif +endif
obj-y += psci-dt.o
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.
+#define WAYS_SHIFT 30 +#define SETS_SHIFT 5
Is this always 30 and 5 , on all CPUs ?
Yes for all armv7m arch.
+/* 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.
+enum cache_type {
- DCACHE = 0,
- ICACHE,
+};
+/* PoU : Point of Unification, Poc: Point of Coherency */ +enum cache_action {
- INVALIDATE_POU, /* for i-cache invalidate by address */
- INVALIDATE_POC, /* for d-cache invalidate by address */
- INVALIDATE_SET_WAY, /* for d-cache invalidate by sets/ways */
- FLUSH_POU,
- FLUSH_POC,
- FLUSH_SET_WAY,
- FLUSH_INVAL_POC,
- FLUSH_INVAL_SET_WAY,
+};
+#ifndef CONFIG_SYS_DCACHE_OFF +struct dcache_config {
- uint32_t ways;
- uint32_t sets;
u32 , this is not userspace ...
ok, i will change all.
+};
+static void get_cache_ways_sets(struct dcache_config *cache) +{
- cache->ways = (readl(v7m_processor + CCSIDR) & MASK_NUM_WAYS)
>> NUM_WAYS_SHIFT;- cache->sets = (readl(v7m_processor + CCSIDR) & MASK_NUM_SETS)
>> NUM_SETS_SHIFT;Do you need to read the reg twice ?
Good point, we can read once in temp & find out ways & sets.
+}
+static uint32_t *get_action_reg_set_ways(enum cache_action action) +{
- switch (action) {
- case INVALIDATE_SET_WAY:
return v7m_cache_maint + DC_ISW;- case FLUSH_SET_WAY:
return v7m_cache_maint + DC_CSW;- case FLUSH_INVAL_SET_WAY:
return v7m_cache_maint + DC_CISW;- default:
break;- };
- return NULL;
+}
+static uint32_t *get_action_reg_range(enum cache_action action) +{
- switch (action) {
- case INVALIDATE_POU:
return v7m_cache_maint + IC_IMVALU;- case INVALIDATE_POC:
return v7m_cache_maint + DC_IMVAC;- case FLUSH_POU:
return v7m_cache_maint + DC_CMVAU;- case FLUSH_POC:
return v7m_cache_maint + DC_CMVAC;- case FLUSH_INVAL_POC:
return v7m_cache_maint + DC_CIMVAC;- default:
break;- }
- return NULL;
+}
+static uint32_t get_cline_size(enum cache_type type) +{
- uint32_t size;
- if (type == DCACHE)
clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));- else if (type == ICACHE)
setbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));- dsb();
- size = readl(v7m_processor + CCSIDR) & GENMASK(2, 0);
define the mask as some macro ....
ok
- size = size * 4 + 4; /* 0 means 4, 1 means 8 & so on */
2 means 12 or 16 ? The comment is useless ...
Is the size basically 1 << (size + 2) ?
agree, I will add better comment in v3.
- 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.
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.
- 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.
- 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" ?
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.
#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.
Cheers, Vikas
}; #define V7M_SCB ((struct v7m_scb *)V7M_SCB_BASE)
@@ -39,6 +58,9 @@ struct v7m_scb {
#define V7M_ICSR_VECTACT_MSK 0xFF
+#define V7M_CCR_DCACHE 16 +#define V7M_CCR_ICACHE 17
struct v7m_mpu { uint32_t type; /* Type Register */ uint32_t ctrl; /* Control Register */ diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 166fa9e..52b36b3 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -55,8 +55,10 @@ endif
obj-y += cache.o ifndef CONFIG_ARM64 +ifndef CONFIG_CPU_V7M obj-y += cache-cp15.o endif +endif
obj-y += psci-dt.o
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.
+#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 ...
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
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.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.
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 ...
[...]
Hi Marek,
On 03/16/2017 03:06 PM, Marek Vasut wrote:
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.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.
Which is still pretty cryptic ...
ok, will make them V7M_CACHE_REG_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 ...
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.
ok.
[...]
- 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 ...)
Ah, A different prototype is there in common.h for cache range support like invalidate_dcache_range(). I will correct it in next version.
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 ?
I don't think so, esp as per cache requirements. armv7 cache driver is also using like this.
This should be in a comment in the code anyway ...
sure, i will add the comment.
Cheers, Vikas
[...]
It also enables commands for cache enable/disable/status.
Signed-off-by: Vikas Manocha vikas.manocha@st.com ---
Changed in v2: None
arch/arm/mach-stm32/stm32f7/soc.c | 2 ++ include/configs/stm32f746-disco.h | 4 +--- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-stm32/stm32f7/soc.c b/arch/arm/mach-stm32/stm32f7/soc.c index 8baee99..ca54603 100644 --- a/arch/arm/mach-stm32/stm32f7/soc.c +++ b/arch/arm/mach-stm32/stm32f7/soc.c @@ -60,6 +60,8 @@ int arch_cpu_init(void) (V7M_MPU_RASR_XN_ENABLE | V7M_MPU_RASR_AP_RW_RW | 0x01 << V7M_MPU_RASR_TEX_SHIFT + | 0x01 << V7M_MPU_RASR_B_SHIFT + | 0x01 << V7M_MPU_RASR_C_SHIFT | V7M_MPU_RASR_SIZE_8MB | V7M_MPU_RASR_EN) , &V7M_MPU->rasr diff --git a/include/configs/stm32f746-disco.h b/include/configs/stm32f746-disco.h index ae3211a..9e9406a 100644 --- a/include/configs/stm32f746-disco.h +++ b/include/configs/stm32f746-disco.h @@ -14,9 +14,6 @@ #define CONFIG_SYS_INIT_SP_ADDR 0x20050000 #define CONFIG_SYS_TEXT_BASE 0x08000000
-#define CONFIG_SYS_ICACHE_OFF -#define CONFIG_SYS_DCACHE_OFF - /* * Configuration of the external SDRAM memory */ @@ -82,4 +79,5 @@ #define CONFIG_CMDLINE_EDITING
#define CONFIG_CMD_MEM +#define CONFIG_CMD_CACHE #endif /* __CONFIG_H */
participants (3)
-
Marek Vasut -
vikas -
Vikas Manocha