[U-Boot] [PATCH v2 0/2] add armv7m cache support

[U-Boot] [PATCH v2 0/2] add armv7m cache support

[Vikas Manocha]

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

-- 
1.9.1

[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

[Vikas Manocha]

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 at 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
 
-- 
1.9.1

[U-Boot] [PATCH v2 2/2] stm32f7: enable instruction & data cache

[Vikas Manocha]

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 */
-- 
1.9.1

[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

[Marek Vasut]

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 at 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
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

[vikas]

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 at 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
>>  
>>
> 
> 

[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

[vikas]

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 at 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

> 

[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

[Marek Vasut]

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 at 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 ...

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

[Marek Vasut]

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 at 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] [PATCH v2 1/2] armv7m: add instruction & data cache support

[Vikas Manocha]

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 at 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

> 
> [...]
>