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

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

[Vikas Manocha]

This patchset adds armv7m instruction/data caches support &
enable it for stm32f7.

Changed in v4:
- invalidate_dcache_range() & flush_dcache_range() function added.
- blank lines added.
- comments added for registers, functions & barriers.
- register names changed for better clarity.
- typecasting moved to macro definitions.

Changed in v3:
- uint32 replcaed with u32.
- multiple read of hardware register replaced with single.
- pointers replaced with macros for base address.
- register names added as comment for system control block registers.

Changed in v2:
- changed strucures for memory mapped cache registers to macros
- added lines better readability.
- replaced magic numbers with macros.

Vikas Manocha (2):
  armv7m: add instruction & data cache support
  stm32f7: enable instruction & data cache

 arch/arm/cpu/armv7m/Makefile      |   3 +-
 arch/arm/cpu/armv7m/cache.c       | 336 ++++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/armv7m.h     |  26 ++-
 arch/arm/lib/Makefile             |   2 +
 arch/arm/mach-stm32/stm32f7/soc.c |   2 +
 include/configs/stm32f746-disco.h |   4 +-
 6 files changed, 366 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm/cpu/armv7m/cache.c

-- 
1.9.1

[U-Boot] [PATCH v4 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>
cc: Christophe KERELLO <christophe.kerello@st.com>
---

Changed in v4:
- invalidate_dcache_range() & flush_dcache_range() function added.
- blank lines added.
- comments added for registers, functions & barriers.
- register names changed for better clarity.
- typecasting moved to macro definitions.

Changed in v3:
- uint32 replcaed with u32.
- multiple read of hardware register replaced with single.
- pointers replaced with macros for base address.
- register names added as comment for system control block registers.

Changed in v2:
- changed strucures for memory mapped cache registers to macros
- added lines better readability.
- replaced magic numbers with macros.

 arch/arm/cpu/armv7m/Makefile  |   3 +-
 arch/arm/cpu/armv7m/cache.c   | 336 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/armv7m.h |  26 +++-
 arch/arm/lib/Makefile         |   2 +
 4 files changed, 363 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/cpu/armv7m/cache.c

diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile
index e1a6c40..93c9085 100644
--- a/arch/arm/cpu/armv7m/Makefile
+++ b/arch/arm/cpu/armv7m/Makefile
@@ -6,6 +6,5 @@
 #
 
 extra-y := start.o
-obj-y += cpu.o
-
+obj-y += cpu.o cache.o
 obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o
diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
new file mode 100644
index 0000000..162cfe3
--- /dev/null
+++ b/arch/arm/cpu/armv7m/cache.c
@@ -0,0 +1,336 @@
+/*
+ * (C) Copyright 2017
+ * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <asm/armv7m.h>
+#include <asm/io.h>
+
+/* Cache maintenance operation registers */
+
+#define V7M_CACHE_REG_ICIALLU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x00))
+#define INVAL_ICACHE_POU		0
+#define V7M_CACHE_REG_ICIMVALU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x08))
+#define V7M_CACHE_REG_DCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C))
+#define V7M_CACHE_REG_DCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x10))
+#define V7M_CACHE_REG_DCCMVAU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x14))
+#define V7M_CACHE_REG_DCCMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x18))
+#define V7M_CACHE_REG_DCCSW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C))
+#define V7M_CACHE_REG_DCCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x20))
+#define V7M_CACHE_REG_DCCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x24))
+#define WAYS_SHIFT			30
+#define SETS_SHIFT			5
+
+/* armv7m processor feature registers */
+
+#define V7M_PROC_REG_CLIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x00))
+#define V7M_PROC_REG_CTR		((u32 *)(V7M_PROC_FTR_BASE + 0x04))
+#define V7M_PROC_REG_CCSIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x08))
+#define MASK_NUM_WAYS			GENMASK(12, 3)
+#define MASK_NUM_SETS			GENMASK(27, 13)
+#define CLINE_SIZE_MASK			GENMASK(2, 0)
+#define NUM_WAYS_SHIFT			3
+#define NUM_SETS_SHIFT			13
+#define V7M_PROC_REG_CSSELR		((u32 *)(V7M_PROC_FTR_BASE + 0x0C))
+#define SEL_I_OR_D			BIT(0)
+
+enum cache_type {
+	DCACHE,
+	ICACHE,
+};
+
+/* PoU : Point of Unification, Poc: Point of Coherency */
+enum cache_action {
+	INVALIDATE_POU,		/* i-cache invalidate by address */
+	INVALIDATE_POC,		/* d-cache invalidate by address */
+	INVALIDATE_SET_WAY,	/* d-cache invalidate by sets/ways */
+	FLUSH_POU,		/* d-cache clean by address to the PoU */
+	FLUSH_POC,		/* d-cache clean by address to the PoC */
+	FLUSH_SET_WAY,		/* d-cache clean by sets/ways */
+	FLUSH_INVAL_POC,	/* d-cache clean & invalidate by addr to PoC */
+	FLUSH_INVAL_SET_WAY,	/* d-cache clean & invalidate by set/ways */
+};
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+struct dcache_config {
+	u32 ways;
+	u32 sets;
+};
+
+static void get_cache_ways_sets(struct dcache_config *cache)
+{
+	u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
+
+	cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
+	cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
+}
+
+/*
+ * Return the io register to perform required cache action like clean or clean
+ * & invalidate by sets/ways.
+ */
+static u32 *get_action_reg_set_ways(enum cache_action action)
+{
+	switch (action) {
+	case INVALIDATE_SET_WAY:
+		return V7M_CACHE_REG_DCISW;
+	case FLUSH_SET_WAY:
+		return V7M_CACHE_REG_DCCSW;
+	case FLUSH_INVAL_SET_WAY:
+		return V7M_CACHE_REG_DCCISW;
+	default:
+		break;
+	};
+
+	return NULL;
+}
+
+/*
+ * Return the io register to perform required cache action like clean or clean
+ * & invalidate by adddress or range.
+ */
+static u32 *get_action_reg_range(enum cache_action action)
+{
+	switch (action) {
+	case INVALIDATE_POU:
+		return V7M_CACHE_REG_ICIMVALU;
+	case INVALIDATE_POC:
+		return V7M_CACHE_REG_DCIMVAC;
+	case FLUSH_POU:
+		return V7M_CACHE_REG_DCCMVAU;
+	case FLUSH_POC:
+		return V7M_CACHE_REG_DCCMVAC;
+	case FLUSH_INVAL_POC:
+		return V7M_CACHE_REG_DCCIMVAC;
+	default:
+		break;
+	}
+
+	return NULL;
+}
+
+static u32 get_cline_size(enum cache_type type)
+{
+	u32 size;
+
+	if (type == DCACHE)
+		clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
+	else if (type == ICACHE)
+		setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
+	/* Make sure cache selection is effective for next memory access */
+	dsb();
+
+	size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
+	/* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
+	size = 1 << (size + 2);
+	debug("cache line size is %d\n", size);
+
+	return size;
+}
+
+/* Perform the action like invalidate/clean on a range of cache addresses */
+static int action_cache_range(enum cache_action action, u32 start_addr,
+			      int64_t size)
+{
+	u32 cline_size;
+	u32 *action_reg;
+	enum cache_type type;
+
+	action_reg = get_action_reg_range(action);
+	if (!action_reg)
+		return -EINVAL;
+	if (action == INVALIDATE_POU)
+		type = ICACHE;
+	else
+		type = DCACHE;
+
+	/* Cache line size is minium size for the cache action */
+	cline_size = get_cline_size(type);
+	/* Align start address to cache line boundary */
+	start_addr &= ~(cline_size - 1);
+	debug("total size for cache action = %llx\n", size);
+	do {
+		writel(start_addr, action_reg);
+		size -= cline_size;
+		start_addr += cline_size;
+	} while (size > cline_size);
+
+	/* Make sure cache action is effective for next memory access */
+	dsb();
+	isb();	/* Make sure instruction stream sees it */
+	debug("cache action on range done\n");
+
+	return 0;
+}
+
+/* Perform the action like invalidate/clean on all cached addresses */
+static int action_dcache_all(enum cache_action action)
+{
+	struct dcache_config cache;
+	u32 *action_reg;
+	int i, j;
+
+	action_reg = get_action_reg_set_ways(action);
+	if (!action_reg)
+		return -EINVAL;
+
+	clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
+	/* Make sure cache selection is effective for next memory access */
+	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);
+		}
+	}
+
+	/* Make sure cache action is effective for next memory access */
+	dsb();
+	isb();	/* Make sure instruction stream sees it */
+
+	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));
+
+	/* Make sure cache action is effective for next memory access */
+	dsb();
+	isb();	/* Make sure instruction stream sees it */
+}
+
+void dcache_disable(void)
+{
+	if (!dcache_status())
+		return;
+
+	/* if dcache is enabled-> dcache disable & then flush */
+	if (action_dcache_all(FLUSH_SET_WAY)) {
+		printf("ERR: D-cache not flushed\n");
+		return;
+	}
+
+	clrbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_DCACHE));
+
+	/* Make sure cache action is effective for next memory access */
+	dsb();
+	isb();	/* Make sure instruction stream sees it */
+}
+
+int dcache_status(void)
+{
+	return (readl(&V7M_SCB->ccr) & BIT(V7M_CCR_DCACHE)) != 0;
+}
+
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+	if (action_cache_range(INVALIDATE_POC, start, stop - start)) {
+		printf("ERR: D-cache not invalidated\n");
+		return;
+	}
+}
+
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+	if (action_cache_range(FLUSH_POC, start, stop - start)) {
+		printf("ERR: D-cache not flushed\n");
+		return;
+	}
+}
+#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_REG_ICIALLU);
+
+	/* Make sure cache action is effective for next memory access */
+	dsb();
+	isb();	/* Make sure instruction stream sees it */
+}
+
+void icache_enable(void)
+{
+	if (icache_status())
+		return;
+
+	invalidate_icache_all();
+	setbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_ICACHE));
+
+	/* Make sure cache action is effective for next memory access */
+	dsb();
+	isb();	/* Make sure instruction stream sees it */
+}
+
+int icache_status(void)
+{
+	return (readl(&V7M_SCB->ccr) & BIT(V7M_CCR_ICACHE)) != 0;
+}
+
+void icache_disable(void)
+{
+	if (!icache_status())
+		return;
+
+	isb();	/* flush pipeline */
+	clrbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_ICACHE));
+	isb();	/* subsequent instructions fetch see cache disable effect */
+}
+#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..ebf0f17 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: System Control Register */
+	uint32_t ccr;		/* offset 0x14: Config and Control Register */
+	uint32_t shpr1;		/* offset 0x18: System Handler Priority Reg 1 */
+	uint32_t shpr2;		/* offset 0x1c: System Handler Priority Reg 2 */
+	uint32_t shpr3;		/* offset 0x20: System Handler Priority Reg 3 */
+	uint32_t shcrs;		/* offset 0x24: System Handler Control State */
+	uint32_t cfsr;		/* offset 0x28: Configurable Fault Status Reg */
+	uint32_t hfsr;		/* offset 0x2C: HardFault Status Register */
+	uint32_t res;		/* offset 0x30: reserved */
+	uint32_t mmar;		/* offset 0x34: MemManage Fault Address Reg */
+	uint32_t bfar;		/* offset 0x38: BusFault Address Reg */
+	uint32_t afsr;		/* offset 0x3C: Auxiliary Fault Status Reg */
 };
 #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 v4 1/2] armv7m: add instruction & data cache support

[Marek Vasut]

On 03/27/2017 10:02 PM, Vikas Manocha wrote:
> This patch adds armv7m instruction & data cache support.
> 
> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> cc: Christophe KERELLO <christophe.kerello@st.com>
> ---
> 
> Changed in v4:
> - invalidate_dcache_range() & flush_dcache_range() function added.
> - blank lines added.
> - comments added for registers, functions & barriers.
> - register names changed for better clarity.
> - typecasting moved to macro definitions.
> 
> Changed in v3:
> - uint32 replcaed with u32.
> - multiple read of hardware register replaced with single.
> - pointers replaced with macros for base address.
> - register names added as comment for system control block registers.
> 
> Changed in v2:
> - changed strucures for memory mapped cache registers to macros
> - added lines better readability.
> - replaced magic numbers with macros.
> 
>  arch/arm/cpu/armv7m/Makefile  |   3 +-
>  arch/arm/cpu/armv7m/cache.c   | 336 ++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/armv7m.h |  26 +++-
>  arch/arm/lib/Makefile         |   2 +
>  4 files changed, 363 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm/cpu/armv7m/cache.c
> 
> diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile
> index e1a6c40..93c9085 100644
> --- a/arch/arm/cpu/armv7m/Makefile
> +++ b/arch/arm/cpu/armv7m/Makefile
> @@ -6,6 +6,5 @@
>  #
>  
>  extra-y := start.o
> -obj-y += cpu.o
> -
> +obj-y += cpu.o cache.o
>  obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o
> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> new file mode 100644
> index 0000000..162cfe3
> --- /dev/null
> +++ b/arch/arm/cpu/armv7m/cache.c
> @@ -0,0 +1,336 @@
> +/*
> + * (C) Copyright 2017
> + * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <errno.h>
> +#include <asm/armv7m.h>
> +#include <asm/io.h>
> +
> +/* Cache maintenance operation registers */
> +
> +#define V7M_CACHE_REG_ICIALLU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x00))
                                                 ^^^^^^^^^^^^^^^^^^^^^
                                                Drop this whole part,
it's a register offset, not address. Just calculate the address
in-place. The cast is also not needed.

> +#define INVAL_ICACHE_POU		0
> +#define V7M_CACHE_REG_ICIMVALU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x08))
> +#define V7M_CACHE_REG_DCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C))
> +#define V7M_CACHE_REG_DCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x10))
> +#define V7M_CACHE_REG_DCCMVAU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x14))
> +#define V7M_CACHE_REG_DCCMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x18))
> +#define V7M_CACHE_REG_DCCSW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C))
> +#define V7M_CACHE_REG_DCCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x20))
> +#define V7M_CACHE_REG_DCCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x24))
> +#define WAYS_SHIFT			30
> +#define SETS_SHIFT			5
> +
> +/* armv7m processor feature registers */
> +
> +#define V7M_PROC_REG_CLIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x00))
> +#define V7M_PROC_REG_CTR		((u32 *)(V7M_PROC_FTR_BASE + 0x04))
> +#define V7M_PROC_REG_CCSIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x08))
> +#define MASK_NUM_WAYS			GENMASK(12, 3)
> +#define MASK_NUM_SETS			GENMASK(27, 13)
> +#define CLINE_SIZE_MASK			GENMASK(2, 0)
> +#define NUM_WAYS_SHIFT			3
> +#define NUM_SETS_SHIFT			13

Well use those macros in MASK_* above ...

> +#define V7M_PROC_REG_CSSELR		((u32 *)(V7M_PROC_FTR_BASE + 0x0C))
> +#define SEL_I_OR_D			BIT(0)
> +
> +enum cache_type {
> +	DCACHE,
> +	ICACHE,
> +};
> +
> +/* PoU : Point of Unification, Poc: Point of Coherency */
> +enum cache_action {
> +	INVALIDATE_POU,		/* i-cache invalidate by address */
> +	INVALIDATE_POC,		/* d-cache invalidate by address */
> +	INVALIDATE_SET_WAY,	/* d-cache invalidate by sets/ways */
> +	FLUSH_POU,		/* d-cache clean by address to the PoU */
> +	FLUSH_POC,		/* d-cache clean by address to the PoC */
> +	FLUSH_SET_WAY,		/* d-cache clean by sets/ways */
> +	FLUSH_INVAL_POC,	/* d-cache clean & invalidate by addr to PoC */
> +	FLUSH_INVAL_SET_WAY,	/* d-cache clean & invalidate by set/ways */
> +};
> +
> +#ifndef CONFIG_SYS_DCACHE_OFF
> +struct dcache_config {
> +	u32 ways;
> +	u32 sets;
> +};
> +
> +static void get_cache_ways_sets(struct dcache_config *cache)
> +{
> +	u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
> +
> +	cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
> +	cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
> +}
> +
> +/*
> + * Return the io register to perform required cache action like clean or clean
> + * & invalidate by sets/ways.
> + */
> +static u32 *get_action_reg_set_ways(enum cache_action action)
> +{
> +	switch (action) {
> +	case INVALIDATE_SET_WAY:
> +		return V7M_CACHE_REG_DCISW;
> +	case FLUSH_SET_WAY:
> +		return V7M_CACHE_REG_DCCSW;
> +	case FLUSH_INVAL_SET_WAY:
> +		return V7M_CACHE_REG_DCCISW;
> +	default:
> +		break;
> +	};
> +
> +	return NULL;
> +}
> +
> +/*
> + * Return the io register to perform required cache action like clean or clean
> + * & invalidate by adddress or range.
> + */
> +static u32 *get_action_reg_range(enum cache_action action)
> +{
> +	switch (action) {
> +	case INVALIDATE_POU:
> +		return V7M_CACHE_REG_ICIMVALU;
> +	case INVALIDATE_POC:
> +		return V7M_CACHE_REG_DCIMVAC;
> +	case FLUSH_POU:
> +		return V7M_CACHE_REG_DCCMVAU;
> +	case FLUSH_POC:
> +		return V7M_CACHE_REG_DCCMVAC;
> +	case FLUSH_INVAL_POC:
> +		return V7M_CACHE_REG_DCCIMVAC;
> +	default:
> +		break;
> +	}
> +
> +	return NULL;
> +}
> +
> +static u32 get_cline_size(enum cache_type type)
> +{
> +	u32 size;
> +
> +	if (type == DCACHE)
> +		clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
> +	else if (type == ICACHE)
> +		setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
> +	/* Make sure cache selection is effective for next memory access */
> +	dsb();
> +
> +	size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
> +	/* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
> +	size = 1 << (size + 2);
> +	debug("cache line size is %d\n", size);
> +
> +	return size;
> +}
> +
> +/* Perform the action like invalidate/clean on a range of cache addresses */
> +static int action_cache_range(enum cache_action action, u32 start_addr,
> +			      int64_t size)
> +{
> +	u32 cline_size;
> +	u32 *action_reg;
> +	enum cache_type type;

Does this ever check whether start_addr and size is unaligned ?
Also, you use u32 all over the place, but size is signed-i64 ? Why?

> +	action_reg = get_action_reg_range(action);
> +	if (!action_reg)
> +		return -EINVAL;
> +	if (action == INVALIDATE_POU)
> +		type = ICACHE;
> +	else
> +		type = DCACHE;
> +
> +	/* Cache line size is minium size for the cache action */
> +	cline_size = get_cline_size(type);
> +	/* Align start address to cache line boundary */
> +	start_addr &= ~(cline_size - 1);
> +	debug("total size for cache action = %llx\n", size);
> +	do {
> +		writel(start_addr, action_reg);
> +		size -= cline_size;
> +		start_addr += cline_size;
> +	} while (size > cline_size);
> +
> +	/* Make sure cache action is effective for next memory access */
> +	dsb();
> +	isb();	/* Make sure instruction stream sees it */
> +	debug("cache action on range done\n");
> +
> +	return 0;
> +}

[...]

>  
> -#define V7M_SCB_BASE		0xE000ED00
> -#define V7M_MPU_BASE		0xE000ED90
> +/* armv7m fixed base addresses */
> +#define V7M_SCS_BASE		0xE000E000

Is all of this really used in the code ?

> +#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: System Control Register */
> +	uint32_t ccr;		/* offset 0x14: Config and Control Register */
> +	uint32_t shpr1;		/* offset 0x18: System Handler Priority Reg 1 */
> +	uint32_t shpr2;		/* offset 0x1c: System Handler Priority Reg 2 */
> +	uint32_t shpr3;		/* offset 0x20: System Handler Priority Reg 3 */
> +	uint32_t shcrs;		/* offset 0x24: System Handler Control State */
> +	uint32_t cfsr;		/* offset 0x28: Configurable Fault Status Reg */
> +	uint32_t hfsr;		/* offset 0x2C: HardFault Status Register */
> +	uint32_t res;		/* offset 0x30: reserved */
> +	uint32_t mmar;		/* offset 0x34: MemManage Fault Address Reg */
> +	uint32_t bfar;		/* offset 0x38: BusFault Address Reg */
> +	uint32_t afsr;		/* offset 0x3C: Auxiliary Fault Status Reg */
>  };
>  #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 v4 1/2] armv7m: add instruction & data cache support

[Vikas Manocha]

Hi Marek,

On 03/27/2017 01:34 PM, Marek Vasut wrote:
> On 03/27/2017 10:02 PM, Vikas Manocha wrote:
>> This patch adds armv7m instruction & data cache support.
>>
>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>> cc: Christophe KERELLO <christophe.kerello@st.com>
>> ---
>>
>> Changed in v4:
>> - invalidate_dcache_range() & flush_dcache_range() function added.
>> - blank lines added.
>> - comments added for registers, functions & barriers.
>> - register names changed for better clarity.
>> - typecasting moved to macro definitions.
>>
>> Changed in v3:
>> - uint32 replcaed with u32.
>> - multiple read of hardware register replaced with single.
>> - pointers replaced with macros for base address.
>> - register names added as comment for system control block registers.
>>
>> Changed in v2:
>> - changed strucures for memory mapped cache registers to macros
>> - added lines better readability.
>> - replaced magic numbers with macros.
>>
>>  arch/arm/cpu/armv7m/Makefile  |   3 +-
>>  arch/arm/cpu/armv7m/cache.c   | 336 ++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/include/asm/armv7m.h |  26 +++-
>>  arch/arm/lib/Makefile         |   2 +
>>  4 files changed, 363 insertions(+), 4 deletions(-)
>>  create mode 100644 arch/arm/cpu/armv7m/cache.c
>>
>> diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile
>> index e1a6c40..93c9085 100644
>> --- a/arch/arm/cpu/armv7m/Makefile
>> +++ b/arch/arm/cpu/armv7m/Makefile
>> @@ -6,6 +6,5 @@
>>  #
>>  
>>  extra-y := start.o
>> -obj-y += cpu.o
>> -
>> +obj-y += cpu.o cache.o
>>  obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o
>> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
>> new file mode 100644
>> index 0000000..162cfe3
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7m/cache.c
>> @@ -0,0 +1,336 @@
>> +/*
>> + * (C) Copyright 2017
>> + * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com.
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <errno.h>
>> +#include <asm/armv7m.h>
>> +#include <asm/io.h>
>> +
>> +/* Cache maintenance operation registers */
>> +
>> +#define V7M_CACHE_REG_ICIALLU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x00))
>                                                  ^^^^^^^^^^^^^^^^^^^^^
>                                                 Drop this whole part,
> it's a register offset, not address. Just calculate the address
> in-place. The cast is also not needed.

These are fixed addresses in armv7m architecture. In place calculations were replaced by
this fixed address macro as per your comment.
casting was moved here from code as per Simon's suggestion on on v3.

> 
>> +#define INVAL_ICACHE_POU		0
>> +#define V7M_CACHE_REG_ICIMVALU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x08))
>> +#define V7M_CACHE_REG_DCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C))
>> +#define V7M_CACHE_REG_DCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x10))
>> +#define V7M_CACHE_REG_DCCMVAU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x14))
>> +#define V7M_CACHE_REG_DCCMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x18))
>> +#define V7M_CACHE_REG_DCCSW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C))
>> +#define V7M_CACHE_REG_DCCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x20))
>> +#define V7M_CACHE_REG_DCCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x24))
>> +#define WAYS_SHIFT			30
>> +#define SETS_SHIFT			5
>> +
>> +/* armv7m processor feature registers */
>> +
>> +#define V7M_PROC_REG_CLIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x00))
>> +#define V7M_PROC_REG_CTR		((u32 *)(V7M_PROC_FTR_BASE + 0x04))
>> +#define V7M_PROC_REG_CCSIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x08))
>> +#define MASK_NUM_WAYS			GENMASK(12, 3)
>> +#define MASK_NUM_SETS			GENMASK(27, 13)
>> +#define CLINE_SIZE_MASK			GENMASK(2, 0)
>> +#define NUM_WAYS_SHIFT			3
>> +#define NUM_SETS_SHIFT			13
> 
> Well use those macros in MASK_* above ...

macros are being used, shifts are required after masking.

> 
>> +#define V7M_PROC_REG_CSSELR		((u32 *)(V7M_PROC_FTR_BASE + 0x0C))
>> +#define SEL_I_OR_D			BIT(0)
>> +
>> +enum cache_type {
>> +	DCACHE,
>> +	ICACHE,
>> +};
>> +
>> +/* PoU : Point of Unification, Poc: Point of Coherency */
>> +enum cache_action {
>> +	INVALIDATE_POU,		/* i-cache invalidate by address */
>> +	INVALIDATE_POC,		/* d-cache invalidate by address */
>> +	INVALIDATE_SET_WAY,	/* d-cache invalidate by sets/ways */
>> +	FLUSH_POU,		/* d-cache clean by address to the PoU */
>> +	FLUSH_POC,		/* d-cache clean by address to the PoC */
>> +	FLUSH_SET_WAY,		/* d-cache clean by sets/ways */
>> +	FLUSH_INVAL_POC,	/* d-cache clean & invalidate by addr to PoC */
>> +	FLUSH_INVAL_SET_WAY,	/* d-cache clean & invalidate by set/ways */
>> +};
>> +
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>> +struct dcache_config {
>> +	u32 ways;
>> +	u32 sets;
>> +};
>> +
>> +static void get_cache_ways_sets(struct dcache_config *cache)
>> +{
>> +	u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
>> +
>> +	cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
>> +	cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
>> +}
>> +
>> +/*
>> + * Return the io register to perform required cache action like clean or clean
>> + * & invalidate by sets/ways.
>> + */
>> +static u32 *get_action_reg_set_ways(enum cache_action action)
>> +{
>> +	switch (action) {
>> +	case INVALIDATE_SET_WAY:
>> +		return V7M_CACHE_REG_DCISW;
>> +	case FLUSH_SET_WAY:
>> +		return V7M_CACHE_REG_DCCSW;
>> +	case FLUSH_INVAL_SET_WAY:
>> +		return V7M_CACHE_REG_DCCISW;
>> +	default:
>> +		break;
>> +	};
>> +
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * Return the io register to perform required cache action like clean or clean
>> + * & invalidate by adddress or range.
>> + */
>> +static u32 *get_action_reg_range(enum cache_action action)
>> +{
>> +	switch (action) {
>> +	case INVALIDATE_POU:
>> +		return V7M_CACHE_REG_ICIMVALU;
>> +	case INVALIDATE_POC:
>> +		return V7M_CACHE_REG_DCIMVAC;
>> +	case FLUSH_POU:
>> +		return V7M_CACHE_REG_DCCMVAU;
>> +	case FLUSH_POC:
>> +		return V7M_CACHE_REG_DCCMVAC;
>> +	case FLUSH_INVAL_POC:
>> +		return V7M_CACHE_REG_DCCIMVAC;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static u32 get_cline_size(enum cache_type type)
>> +{
>> +	u32 size;
>> +
>> +	if (type == DCACHE)
>> +		clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>> +	else if (type == ICACHE)
>> +		setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>> +	/* Make sure cache selection is effective for next memory access */
>> +	dsb();
>> +
>> +	size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
>> +	/* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
>> +	size = 1 << (size + 2);
>> +	debug("cache line size is %d\n", size);
>> +
>> +	return size;
>> +}
>> +
>> +/* Perform the action like invalidate/clean on a range of cache addresses */
>> +static int action_cache_range(enum cache_action action, u32 start_addr,
>> +			      int64_t size)
>> +{
>> +	u32 cline_size;
>> +	u32 *action_reg;
>> +	enum cache_type type;
> 
> Does this ever check whether start_addr and size is unaligned ?

address alignment is being done below before cache action.

> Also, you use u32 all over the place, but size is signed-i64 ? Why?

Yes it is for condition "size > cline_size" when range/size is 4GB.

> 
>> +	action_reg = get_action_reg_range(action);
>> +	if (!action_reg)
>> +		return -EINVAL;
>> +	if (action == INVALIDATE_POU)
>> +		type = ICACHE;
>> +	else
>> +		type = DCACHE;
>> +
>> +	/* Cache line size is minium size for the cache action */
>> +	cline_size = get_cline_size(type);
>> +	/* Align start address to cache line boundary */
>> +	start_addr &= ~(cline_size - 1);
>> +	debug("total size for cache action = %llx\n", size);
>> +	do {
>> +		writel(start_addr, action_reg);
>> +		size -= cline_size;
>> +		start_addr += cline_size;
>> +	} while (size > cline_size);
>> +
>> +	/* Make sure cache action is effective for next memory access */
>> +	dsb();
>> +	isb();	/* Make sure instruction stream sees it */
>> +	debug("cache action on range done\n");
>> +
>> +	return 0;
>> +}
> 
> [...]
> 
>>  
>> -#define V7M_SCB_BASE		0xE000ED00
>> -#define V7M_MPU_BASE		0xE000ED90
>> +/* armv7m fixed base addresses */
>> +#define V7M_SCS_BASE		0xE000E000
> 
> Is all of this really used in the code ?

Not all of them, it is for the sake of armv7m address space completeness.

Cheers,
Vikas

> 
>> +#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: System Control Register */
>> +	uint32_t ccr;		/* offset 0x14: Config and Control Register */
>> +	uint32_t shpr1;		/* offset 0x18: System Handler Priority Reg 1 */
>> +	uint32_t shpr2;		/* offset 0x1c: System Handler Priority Reg 2 */
>> +	uint32_t shpr3;		/* offset 0x20: System Handler Priority Reg 3 */
>> +	uint32_t shcrs;		/* offset 0x24: System Handler Control State */
>> +	uint32_t cfsr;		/* offset 0x28: Configurable Fault Status Reg */
>> +	uint32_t hfsr;		/* offset 0x2C: HardFault Status Register */
>> +	uint32_t res;		/* offset 0x30: reserved */
>> +	uint32_t mmar;		/* offset 0x34: MemManage Fault Address Reg */
>> +	uint32_t bfar;		/* offset 0x38: BusFault Address Reg */
>> +	uint32_t afsr;		/* offset 0x3C: Auxiliary Fault Status Reg */
>>  };
>>  #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 v4 1/2] armv7m: add instruction & data cache support

[Marek Vasut]

On 03/27/2017 10:41 PM, Vikas Manocha wrote:
> Hi Marek,
> 
> On 03/27/2017 01:34 PM, Marek Vasut wrote:
>> On 03/27/2017 10:02 PM, Vikas Manocha wrote:
>>> This patch adds armv7m instruction & data cache support.
>>>
>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>>> cc: Christophe KERELLO <christophe.kerello@st.com>
>>> ---
>>>
>>> Changed in v4:
>>> - invalidate_dcache_range() & flush_dcache_range() function added.
>>> - blank lines added.
>>> - comments added for registers, functions & barriers.
>>> - register names changed for better clarity.
>>> - typecasting moved to macro definitions.
>>>
>>> Changed in v3:
>>> - uint32 replcaed with u32.
>>> - multiple read of hardware register replaced with single.
>>> - pointers replaced with macros for base address.
>>> - register names added as comment for system control block registers.
>>>
>>> Changed in v2:
>>> - changed strucures for memory mapped cache registers to macros
>>> - added lines better readability.
>>> - replaced magic numbers with macros.
>>>
>>>  arch/arm/cpu/armv7m/Makefile  |   3 +-
>>>  arch/arm/cpu/armv7m/cache.c   | 336 ++++++++++++++++++++++++++++++++++++++++++
>>>  arch/arm/include/asm/armv7m.h |  26 +++-
>>>  arch/arm/lib/Makefile         |   2 +
>>>  4 files changed, 363 insertions(+), 4 deletions(-)
>>>  create mode 100644 arch/arm/cpu/armv7m/cache.c
>>>
>>> diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile
>>> index e1a6c40..93c9085 100644
>>> --- a/arch/arm/cpu/armv7m/Makefile
>>> +++ b/arch/arm/cpu/armv7m/Makefile
>>> @@ -6,6 +6,5 @@
>>>  #
>>>  
>>>  extra-y := start.o
>>> -obj-y += cpu.o
>>> -
>>> +obj-y += cpu.o cache.o
>>>  obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o
>>> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
>>> new file mode 100644
>>> index 0000000..162cfe3
>>> --- /dev/null
>>> +++ b/arch/arm/cpu/armv7m/cache.c
>>> @@ -0,0 +1,336 @@
>>> +/*
>>> + * (C) Copyright 2017
>>> + * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com.
>>> + *
>>> + * SPDX-License-Identifier:	GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <errno.h>
>>> +#include <asm/armv7m.h>
>>> +#include <asm/io.h>
>>> +
>>> +/* Cache maintenance operation registers */
>>> +
>>> +#define V7M_CACHE_REG_ICIALLU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x00))
>>                                                  ^^^^^^^^^^^^^^^^^^^^^
>>                                                 Drop this whole part,
>> it's a register offset, not address. Just calculate the address
>> in-place. The cast is also not needed.
> 
> These are fixed addresses in armv7m architecture.

So what ?

> In place calculations were replaced by
> this fixed address macro as per your comment.

And then one of the two things will happen:
a) these addresses will be moved
b) someone will look at the surrounding code, where _REG_ means register
offset all over the place and will screw things up ...

> casting was moved here from code as per Simon's suggestion on on v3.
> 
>>
>>> +#define INVAL_ICACHE_POU		0
>>> +#define V7M_CACHE_REG_ICIMVALU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x08))
>>> +#define V7M_CACHE_REG_DCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C))
>>> +#define V7M_CACHE_REG_DCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x10))
>>> +#define V7M_CACHE_REG_DCCMVAU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x14))
>>> +#define V7M_CACHE_REG_DCCMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x18))
>>> +#define V7M_CACHE_REG_DCCSW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C))
>>> +#define V7M_CACHE_REG_DCCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x20))
>>> +#define V7M_CACHE_REG_DCCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x24))
>>> +#define WAYS_SHIFT			30
>>> +#define SETS_SHIFT			5
>>> +
>>> +/* armv7m processor feature registers */
>>> +
>>> +#define V7M_PROC_REG_CLIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x00))
>>> +#define V7M_PROC_REG_CTR		((u32 *)(V7M_PROC_FTR_BASE + 0x04))
>>> +#define V7M_PROC_REG_CCSIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x08))
>>> +#define MASK_NUM_WAYS			GENMASK(12, 3)
>>> +#define MASK_NUM_SETS			GENMASK(27, 13)
>>> +#define CLINE_SIZE_MASK			GENMASK(2, 0)
>>> +#define NUM_WAYS_SHIFT			3
>>> +#define NUM_SETS_SHIFT			13
>>
>> Well use those macros in MASK_* above ...
> 
> macros are being used, shifts are required after masking.

Then look at the GENMASK() above and the hardcoded values ...

>>> +#define V7M_PROC_REG_CSSELR		((u32 *)(V7M_PROC_FTR_BASE + 0x0C))
>>> +#define SEL_I_OR_D			BIT(0)
>>> +
>>> +enum cache_type {
>>> +	DCACHE,
>>> +	ICACHE,
>>> +};
>>> +
>>> +/* PoU : Point of Unification, Poc: Point of Coherency */
>>> +enum cache_action {
>>> +	INVALIDATE_POU,		/* i-cache invalidate by address */
>>> +	INVALIDATE_POC,		/* d-cache invalidate by address */
>>> +	INVALIDATE_SET_WAY,	/* d-cache invalidate by sets/ways */
>>> +	FLUSH_POU,		/* d-cache clean by address to the PoU */
>>> +	FLUSH_POC,		/* d-cache clean by address to the PoC */
>>> +	FLUSH_SET_WAY,		/* d-cache clean by sets/ways */
>>> +	FLUSH_INVAL_POC,	/* d-cache clean & invalidate by addr to PoC */
>>> +	FLUSH_INVAL_SET_WAY,	/* d-cache clean & invalidate by set/ways */
>>> +};
>>> +
>>> +#ifndef CONFIG_SYS_DCACHE_OFF
>>> +struct dcache_config {
>>> +	u32 ways;
>>> +	u32 sets;
>>> +};
>>> +
>>> +static void get_cache_ways_sets(struct dcache_config *cache)
>>> +{
>>> +	u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
>>> +
>>> +	cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
>>> +	cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
>>> +}
>>> +
>>> +/*
>>> + * Return the io register to perform required cache action like clean or clean
>>> + * & invalidate by sets/ways.
>>> + */
>>> +static u32 *get_action_reg_set_ways(enum cache_action action)
>>> +{
>>> +	switch (action) {
>>> +	case INVALIDATE_SET_WAY:
>>> +		return V7M_CACHE_REG_DCISW;
>>> +	case FLUSH_SET_WAY:
>>> +		return V7M_CACHE_REG_DCCSW;
>>> +	case FLUSH_INVAL_SET_WAY:
>>> +		return V7M_CACHE_REG_DCCISW;
>>> +	default:
>>> +		break;
>>> +	};
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +/*
>>> + * Return the io register to perform required cache action like clean or clean
>>> + * & invalidate by adddress or range.
>>> + */
>>> +static u32 *get_action_reg_range(enum cache_action action)
>>> +{
>>> +	switch (action) {
>>> +	case INVALIDATE_POU:
>>> +		return V7M_CACHE_REG_ICIMVALU;
>>> +	case INVALIDATE_POC:
>>> +		return V7M_CACHE_REG_DCIMVAC;
>>> +	case FLUSH_POU:
>>> +		return V7M_CACHE_REG_DCCMVAU;
>>> +	case FLUSH_POC:
>>> +		return V7M_CACHE_REG_DCCMVAC;
>>> +	case FLUSH_INVAL_POC:
>>> +		return V7M_CACHE_REG_DCCIMVAC;
>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static u32 get_cline_size(enum cache_type type)
>>> +{
>>> +	u32 size;
>>> +
>>> +	if (type == DCACHE)
>>> +		clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>>> +	else if (type == ICACHE)
>>> +		setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>>> +	/* Make sure cache selection is effective for next memory access */
>>> +	dsb();
>>> +
>>> +	size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
>>> +	/* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
>>> +	size = 1 << (size + 2);
>>> +	debug("cache line size is %d\n", size);
>>> +
>>> +	return size;
>>> +}
>>> +
>>> +/* Perform the action like invalidate/clean on a range of cache addresses */
>>> +static int action_cache_range(enum cache_action action, u32 start_addr,
>>> +			      int64_t size)
>>> +{
>>> +	u32 cline_size;
>>> +	u32 *action_reg;
>>> +	enum cache_type type;
>>
>> Does this ever check whether start_addr and size is unaligned ?
> 
> address alignment is being done below before cache action.

Ah, so we hide bugs which are introduced by unaligned accesses ? Well
please remove that and add address alignment check like the rest ...

>> Also, you use u32 all over the place, but size is signed-i64 ? Why?
> 
> Yes it is for condition "size > cline_size" when range/size is 4GB.

Divide that by page size and you no longer need to use 64bit type on
32bit system ...

>>> +	action_reg = get_action_reg_range(action);
>>> +	if (!action_reg)
>>> +		return -EINVAL;
>>> +	if (action == INVALIDATE_POU)
>>> +		type = ICACHE;
>>> +	else
>>> +		type = DCACHE;
>>> +
>>> +	/* Cache line size is minium size for the cache action */
>>> +	cline_size = get_cline_size(type);
>>> +	/* Align start address to cache line boundary */
>>> +	start_addr &= ~(cline_size - 1);
>>> +	debug("total size for cache action = %llx\n", size);
>>> +	do {
>>> +		writel(start_addr, action_reg);
>>> +		size -= cline_size;
>>> +		start_addr += cline_size;
>>> +	} while (size > cline_size);
>>> +
>>> +	/* Make sure cache action is effective for next memory access */
>>> +	dsb();
>>> +	isb();	/* Make sure instruction stream sees it */
>>> +	debug("cache action on range done\n");
>>> +
>>> +	return 0;
>>> +}
>>
>> [...]
>>
>>>  
>>> -#define V7M_SCB_BASE		0xE000ED00
>>> -#define V7M_MPU_BASE		0xE000ED90
>>> +/* armv7m fixed base addresses */
>>> +#define V7M_SCS_BASE		0xE000E000
>>
>> Is all of this really used in the code ?
> 
> Not all of them, it is for the sake of armv7m address space completeness.

Not sure we need them all then ...

-- 
Best regards,
Marek Vasut

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

[Vikas Manocha]

Hi Marek,

On 03/27/2017 02:27 PM, Marek Vasut wrote:
> On 03/27/2017 10:41 PM, Vikas Manocha wrote:
>> Hi Marek,
>>
>> On 03/27/2017 01:34 PM, Marek Vasut wrote:
>>> On 03/27/2017 10:02 PM, Vikas Manocha wrote:
>>>> This patch adds armv7m instruction & data cache support.
>>>>
>>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>>>> cc: Christophe KERELLO <christophe.kerello@st.com>
>>>> ---
>>>>
>>>> Changed in v4:
>>>> - invalidate_dcache_range() & flush_dcache_range() function added.
>>>> - blank lines added.
>>>> - comments added for registers, functions & barriers.
>>>> - register names changed for better clarity.
>>>> - typecasting moved to macro definitions.
>>>>
>>>> Changed in v3:
>>>> - uint32 replcaed with u32.
>>>> - multiple read of hardware register replaced with single.
>>>> - pointers replaced with macros for base address.
>>>> - register names added as comment for system control block registers.
>>>>
>>>> Changed in v2:
>>>> - changed strucures for memory mapped cache registers to macros
>>>> - added lines better readability.
>>>> - replaced magic numbers with macros.
>>>>
>>>>  arch/arm/cpu/armv7m/Makefile  |   3 +-
>>>>  arch/arm/cpu/armv7m/cache.c   | 336 ++++++++++++++++++++++++++++++++++++++++++
>>>>  arch/arm/include/asm/armv7m.h |  26 +++-
>>>>  arch/arm/lib/Makefile         |   2 +
>>>>  4 files changed, 363 insertions(+), 4 deletions(-)
>>>>  create mode 100644 arch/arm/cpu/armv7m/cache.c
>>>>
>>>> diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile
>>>> index e1a6c40..93c9085 100644
>>>> --- a/arch/arm/cpu/armv7m/Makefile
>>>> +++ b/arch/arm/cpu/armv7m/Makefile
>>>> @@ -6,6 +6,5 @@
>>>>  #
>>>>  
>>>>  extra-y := start.o
>>>> -obj-y += cpu.o
>>>> -
>>>> +obj-y += cpu.o cache.o
>>>>  obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o
>>>> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
>>>> new file mode 100644
>>>> index 0000000..162cfe3
>>>> --- /dev/null
>>>> +++ b/arch/arm/cpu/armv7m/cache.c
>>>> @@ -0,0 +1,336 @@
>>>> +/*
>>>> + * (C) Copyright 2017
>>>> + * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com.
>>>> + *
>>>> + * SPDX-License-Identifier:	GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <errno.h>
>>>> +#include <asm/armv7m.h>
>>>> +#include <asm/io.h>
>>>> +
>>>> +/* Cache maintenance operation registers */
>>>> +
>>>> +#define V7M_CACHE_REG_ICIALLU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x00))
>>>                                                  ^^^^^^^^^^^^^^^^^^^^^
>>>                                                 Drop this whole part,
>>> it's a register offset, not address. Just calculate the address
>>> in-place. The cast is also not needed.
>>
>> These are fixed addresses in armv7m architecture.
> 
> So what ?

So, no need to calculate in-plcae. It was your suggestion also.

> 
>> In place calculations were replaced by
>> this fixed address macro as per your comment.
> 
> And then one of the two things will happen:
> a) these addresses will be moved
> b) someone will look at the surrounding code, where _REG_ means register
> offset all over the place and will screw things up ...

moving these addresses...its fixed by architecture.

> 
>> casting was moved here from code as per Simon's suggestion on on v3.
>>
>>>
>>>> +#define INVAL_ICACHE_POU		0
>>>> +#define V7M_CACHE_REG_ICIMVALU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x08))
>>>> +#define V7M_CACHE_REG_DCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C))
>>>> +#define V7M_CACHE_REG_DCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x10))
>>>> +#define V7M_CACHE_REG_DCCMVAU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x14))
>>>> +#define V7M_CACHE_REG_DCCMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x18))
>>>> +#define V7M_CACHE_REG_DCCSW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C))
>>>> +#define V7M_CACHE_REG_DCCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x20))
>>>> +#define V7M_CACHE_REG_DCCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x24))
>>>> +#define WAYS_SHIFT			30
>>>> +#define SETS_SHIFT			5
>>>> +
>>>> +/* armv7m processor feature registers */
>>>> +
>>>> +#define V7M_PROC_REG_CLIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x00))
>>>> +#define V7M_PROC_REG_CTR		((u32 *)(V7M_PROC_FTR_BASE + 0x04))
>>>> +#define V7M_PROC_REG_CCSIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x08))
>>>> +#define MASK_NUM_WAYS			GENMASK(12, 3)
>>>> +#define MASK_NUM_SETS			GENMASK(27, 13)
>>>> +#define CLINE_SIZE_MASK			GENMASK(2, 0)
>>>> +#define NUM_WAYS_SHIFT			3
>>>> +#define NUM_SETS_SHIFT			13
>>>
>>> Well use those macros in MASK_* above ...
>>
>> macros are being used, shifts are required after masking.
> 
> Then look at the GENMASK() above and the hardcoded values ...

hardcoded in GENMASK...really ?

> 
>>>> +#define V7M_PROC_REG_CSSELR		((u32 *)(V7M_PROC_FTR_BASE + 0x0C))
>>>> +#define SEL_I_OR_D			BIT(0)
>>>> +
>>>> +enum cache_type {
>>>> +	DCACHE,
>>>> +	ICACHE,
>>>> +};
>>>> +
>>>> +/* PoU : Point of Unification, Poc: Point of Coherency */
>>>> +enum cache_action {
>>>> +	INVALIDATE_POU,		/* i-cache invalidate by address */
>>>> +	INVALIDATE_POC,		/* d-cache invalidate by address */
>>>> +	INVALIDATE_SET_WAY,	/* d-cache invalidate by sets/ways */
>>>> +	FLUSH_POU,		/* d-cache clean by address to the PoU */
>>>> +	FLUSH_POC,		/* d-cache clean by address to the PoC */
>>>> +	FLUSH_SET_WAY,		/* d-cache clean by sets/ways */
>>>> +	FLUSH_INVAL_POC,	/* d-cache clean & invalidate by addr to PoC */
>>>> +	FLUSH_INVAL_SET_WAY,	/* d-cache clean & invalidate by set/ways */
>>>> +};
>>>> +
>>>> +#ifndef CONFIG_SYS_DCACHE_OFF
>>>> +struct dcache_config {
>>>> +	u32 ways;
>>>> +	u32 sets;
>>>> +};
>>>> +
>>>> +static void get_cache_ways_sets(struct dcache_config *cache)
>>>> +{
>>>> +	u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
>>>> +
>>>> +	cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
>>>> +	cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return the io register to perform required cache action like clean or clean
>>>> + * & invalidate by sets/ways.
>>>> + */
>>>> +static u32 *get_action_reg_set_ways(enum cache_action action)
>>>> +{
>>>> +	switch (action) {
>>>> +	case INVALIDATE_SET_WAY:
>>>> +		return V7M_CACHE_REG_DCISW;
>>>> +	case FLUSH_SET_WAY:
>>>> +		return V7M_CACHE_REG_DCCSW;
>>>> +	case FLUSH_INVAL_SET_WAY:
>>>> +		return V7M_CACHE_REG_DCCISW;
>>>> +	default:
>>>> +		break;
>>>> +	};
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return the io register to perform required cache action like clean or clean
>>>> + * & invalidate by adddress or range.
>>>> + */
>>>> +static u32 *get_action_reg_range(enum cache_action action)
>>>> +{
>>>> +	switch (action) {
>>>> +	case INVALIDATE_POU:
>>>> +		return V7M_CACHE_REG_ICIMVALU;
>>>> +	case INVALIDATE_POC:
>>>> +		return V7M_CACHE_REG_DCIMVAC;
>>>> +	case FLUSH_POU:
>>>> +		return V7M_CACHE_REG_DCCMVAU;
>>>> +	case FLUSH_POC:
>>>> +		return V7M_CACHE_REG_DCCMVAC;
>>>> +	case FLUSH_INVAL_POC:
>>>> +		return V7M_CACHE_REG_DCCIMVAC;
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static u32 get_cline_size(enum cache_type type)
>>>> +{
>>>> +	u32 size;
>>>> +
>>>> +	if (type == DCACHE)
>>>> +		clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>>>> +	else if (type == ICACHE)
>>>> +		setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>>>> +	/* Make sure cache selection is effective for next memory access */
>>>> +	dsb();
>>>> +
>>>> +	size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
>>>> +	/* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
>>>> +	size = 1 << (size + 2);
>>>> +	debug("cache line size is %d\n", size);
>>>> +
>>>> +	return size;
>>>> +}
>>>> +
>>>> +/* Perform the action like invalidate/clean on a range of cache addresses */
>>>> +static int action_cache_range(enum cache_action action, u32 start_addr,
>>>> +			      int64_t size)
>>>> +{
>>>> +	u32 cline_size;
>>>> +	u32 *action_reg;
>>>> +	enum cache_type type;
>>>
>>> Does this ever check whether start_addr and size is unaligned ?
>>
>> address alignment is being done below before cache action.
> 
> Ah, so we hide bugs which are introduced by unaligned accesses ? Well
> please remove that and add address alignment check like the rest ...

which bug ? user passing unaligned address ?
others are doing the same.

> 
>>> Also, you use u32 all over the place, but size is signed-i64 ? Why?
>>
>> Yes it is for condition "size > cline_size" when range/size is 4GB.
> 
> Divide that by page size and you no longer need to use 64bit type on
> 32bit system ...

page size... its armv7m.

> 
>>>> +	action_reg = get_action_reg_range(action);
>>>> +	if (!action_reg)
>>>> +		return -EINVAL;
>>>> +	if (action == INVALIDATE_POU)
>>>> +		type = ICACHE;
>>>> +	else
>>>> +		type = DCACHE;
>>>> +
>>>> +	/* Cache line size is minium size for the cache action */
>>>> +	cline_size = get_cline_size(type);
>>>> +	/* Align start address to cache line boundary */
>>>> +	start_addr &= ~(cline_size - 1);
>>>> +	debug("total size for cache action = %llx\n", size);
>>>> +	do {
>>>> +		writel(start_addr, action_reg);
>>>> +		size -= cline_size;
>>>> +		start_addr += cline_size;
>>>> +	} while (size > cline_size);
>>>> +
>>>> +	/* Make sure cache action is effective for next memory access */
>>>> +	dsb();
>>>> +	isb();	/* Make sure instruction stream sees it */
>>>> +	debug("cache action on range done\n");
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> [...]
>>>
>>>>  
>>>> -#define V7M_SCB_BASE		0xE000ED00
>>>> -#define V7M_MPU_BASE		0xE000ED90
>>>> +/* armv7m fixed base addresses */
>>>> +#define V7M_SCS_BASE		0xE000E000
>>>
>>> Is all of this really used in the code ?
>>
>> Not all of them, it is for the sake of armv7m address space completeness.
> 
> Not sure we need them all then ...

thats what i said, not needed all for now. They are there for sake of completeness, what's the harm ?

Cheers,
Vikas

> 

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

[Vikas MANOCHA]

Hi Marek,

> -----Original Message-----
> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Vikas Manocha
> Sent: Monday, March 27, 2017 2:33 PM
> To: Marek Vasut <marex@denx.de>; u-boot at lists.denx.de
> Cc: Christophe KERELLO <christophe.kerello@st.com>; Toshifumi NISHINAGA <tnishinaga.dev@gmail.com>; Stephen Warren
> <swarren@nvidia.com>
> Subject: Re: [U-Boot] [PATCH v4 1/2] armv7m: add instruction & data cache support
> 
> Hi Marek,
> 
> On 03/27/2017 02:27 PM, Marek Vasut wrote:
> > On 03/27/2017 10:41 PM, Vikas Manocha wrote:
> >> Hi Marek,
> >>
> >> On 03/27/2017 01:34 PM, Marek Vasut wrote:
> >>> On 03/27/2017 10:02 PM, Vikas Manocha wrote:
> >>>> This patch adds armv7m instruction & data cache support.
> >>>>
> >>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> >>>> cc: Christophe KERELLO <christophe.kerello@st.com>
> >>>> ---
> >>>>
> >>>> Changed in v4:
> >>>> - invalidate_dcache_range() & flush_dcache_range() function added.
> >>>> - blank lines added.
> >>>> - comments added for registers, functions & barriers.
> >>>> - register names changed for better clarity.
> >>>> - typecasting moved to macro definitions.
> >>>>
> >>>> Changed in v3:
> >>>> - uint32 replcaed with u32.
> >>>> - multiple read of hardware register replaced with single.
> >>>> - pointers replaced with macros for base address.
> >>>> - register names added as comment for system control block registers.
> >>>>
> >>>> Changed in v2:
> >>>> - changed strucures for memory mapped cache registers to macros
> >>>> - added lines better readability.
> >>>> - replaced magic numbers with macros.
> >>>>
> >>>>  arch/arm/cpu/armv7m/Makefile  |   3 +-
> >>>>  arch/arm/cpu/armv7m/cache.c   | 336 ++++++++++++++++++++++++++++++++++++++++++
> >>>>  arch/arm/include/asm/armv7m.h |  26 +++-
> >>>>  arch/arm/lib/Makefile         |   2 +
> >>>>  4 files changed, 363 insertions(+), 4 deletions(-)  create mode
> >>>> 100644 arch/arm/cpu/armv7m/cache.c
> >>>>
> >>>> diff --git a/arch/arm/cpu/armv7m/Makefile
> >>>> b/arch/arm/cpu/armv7m/Makefile index e1a6c40..93c9085 100644
> >>>> --- a/arch/arm/cpu/armv7m/Makefile
> >>>> +++ b/arch/arm/cpu/armv7m/Makefile
> >>>> @@ -6,6 +6,5 @@
> >>>>  #
> >>>>
> >>>>  extra-y := start.o
> >>>> -obj-y += cpu.o
> >>>> -
> >>>> +obj-y += cpu.o cache.o
> >>>>  obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o diff --git
> >>>> a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c new
> >>>> file mode 100644 index 0000000..162cfe3
> >>>> --- /dev/null
> >>>> +++ b/arch/arm/cpu/armv7m/cache.c
> >>>> @@ -0,0 +1,336 @@
> >>>> +/*
> >>>> + * (C) Copyright 2017
> >>>> + * Vikas Manocha, ST Micoelectronics, vikas.manocha at st.com.
> >>>> + *
> >>>> + * SPDX-License-Identifier:	GPL-2.0+
> >>>> + */
> >>>> +
> >>>> +#include <common.h>
> >>>> +#include <errno.h>
> >>>> +#include <asm/armv7m.h>
> >>>> +#include <asm/io.h>
> >>>> +
> >>>> +/* Cache maintenance operation registers */
> >>>> +
> >>>> +#define V7M_CACHE_REG_ICIALLU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x00))
> >>>                                                  ^^^^^^^^^^^^^^^^^^^^^
> >>>                                                 Drop this whole
> >>> part, it's a register offset, not address. Just calculate the
> >>> address in-place. The cast is also not needed.
> >>
> >> These are fixed addresses in armv7m architecture.
> >
> > So what ?
> 
> So, no need to calculate in-plcae. It was your suggestion also.

Let me know if there is some open point for this patch.

Cheers,
Vikas

> 
> >
> >> In place calculations were replaced by this fixed address macro as
> >> per your comment.
> >
> > And then one of the two things will happen:
> > a) these addresses will be moved
> > b) someone will look at the surrounding code, where _REG_ means
> > register offset all over the place and will screw things up ...
> 
> moving these addresses...its fixed by architecture.
> 
> >
> >> casting was moved here from code as per Simon's suggestion on on v3.
> >>
> >>>
> >>>> +#define INVAL_ICACHE_POU		0
> >>>> +#define V7M_CACHE_REG_ICIMVALU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x08))
> >>>> +#define V7M_CACHE_REG_DCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x0C))
> >>>> +#define V7M_CACHE_REG_DCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x10))
> >>>> +#define V7M_CACHE_REG_DCCMVAU		((u32 *)(V7M_CACHE_MAINT_BASE + 0x14))
> >>>> +#define V7M_CACHE_REG_DCCMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x18))
> >>>> +#define V7M_CACHE_REG_DCCSW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x1C))
> >>>> +#define V7M_CACHE_REG_DCCIMVAC		((u32 *)(V7M_CACHE_MAINT_BASE + 0x20))
> >>>> +#define V7M_CACHE_REG_DCCISW		((u32 *)(V7M_CACHE_MAINT_BASE + 0x24))
> >>>> +#define WAYS_SHIFT			30
> >>>> +#define SETS_SHIFT			5
> >>>> +
> >>>> +/* armv7m processor feature registers */
> >>>> +
> >>>> +#define V7M_PROC_REG_CLIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x00))
> >>>> +#define V7M_PROC_REG_CTR		((u32 *)(V7M_PROC_FTR_BASE + 0x04))
> >>>> +#define V7M_PROC_REG_CCSIDR		((u32 *)(V7M_PROC_FTR_BASE + 0x08))
> >>>> +#define MASK_NUM_WAYS			GENMASK(12, 3)
> >>>> +#define MASK_NUM_SETS			GENMASK(27, 13)
> >>>> +#define CLINE_SIZE_MASK			GENMASK(2, 0)
> >>>> +#define NUM_WAYS_SHIFT			3
> >>>> +#define NUM_SETS_SHIFT			13
> >>>
> >>> Well use those macros in MASK_* above ...
> >>
> >> macros are being used, shifts are required after masking.
> >
> > Then look at the GENMASK() above and the hardcoded values ...
> 
> hardcoded in GENMASK...really ?
> 
> >
> >>>> +#define V7M_PROC_REG_CSSELR		((u32 *)(V7M_PROC_FTR_BASE + 0x0C))
> >>>> +#define SEL_I_OR_D			BIT(0)
> >>>> +
> >>>> +enum cache_type {
> >>>> +	DCACHE,
> >>>> +	ICACHE,
> >>>> +};
> >>>> +
> >>>> +/* PoU : Point of Unification, Poc: Point of Coherency */ enum
> >>>> +cache_action {
> >>>> +	INVALIDATE_POU,		/* i-cache invalidate by address */
> >>>> +	INVALIDATE_POC,		/* d-cache invalidate by address */
> >>>> +	INVALIDATE_SET_WAY,	/* d-cache invalidate by sets/ways */
> >>>> +	FLUSH_POU,		/* d-cache clean by address to the PoU */
> >>>> +	FLUSH_POC,		/* d-cache clean by address to the PoC */
> >>>> +	FLUSH_SET_WAY,		/* d-cache clean by sets/ways */
> >>>> +	FLUSH_INVAL_POC,	/* d-cache clean & invalidate by addr to PoC */
> >>>> +	FLUSH_INVAL_SET_WAY,	/* d-cache clean & invalidate by set/ways */
> >>>> +};
> >>>> +
> >>>> +#ifndef CONFIG_SYS_DCACHE_OFF
> >>>> +struct dcache_config {
> >>>> +	u32 ways;
> >>>> +	u32 sets;
> >>>> +};
> >>>> +
> >>>> +static void get_cache_ways_sets(struct dcache_config *cache) {
> >>>> +	u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
> >>>> +
> >>>> +	cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
> >>>> +	cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Return the io register to perform required cache action like
> >>>> +clean or clean
> >>>> + * & invalidate by sets/ways.
> >>>> + */
> >>>> +static u32 *get_action_reg_set_ways(enum cache_action action) {
> >>>> +	switch (action) {
> >>>> +	case INVALIDATE_SET_WAY:
> >>>> +		return V7M_CACHE_REG_DCISW;
> >>>> +	case FLUSH_SET_WAY:
> >>>> +		return V7M_CACHE_REG_DCCSW;
> >>>> +	case FLUSH_INVAL_SET_WAY:
> >>>> +		return V7M_CACHE_REG_DCCISW;
> >>>> +	default:
> >>>> +		break;
> >>>> +	};
> >>>> +
> >>>> +	return NULL;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Return the io register to perform required cache action like
> >>>> +clean or clean
> >>>> + * & invalidate by adddress or range.
> >>>> + */
> >>>> +static u32 *get_action_reg_range(enum cache_action action) {
> >>>> +	switch (action) {
> >>>> +	case INVALIDATE_POU:
> >>>> +		return V7M_CACHE_REG_ICIMVALU;
> >>>> +	case INVALIDATE_POC:
> >>>> +		return V7M_CACHE_REG_DCIMVAC;
> >>>> +	case FLUSH_POU:
> >>>> +		return V7M_CACHE_REG_DCCMVAU;
> >>>> +	case FLUSH_POC:
> >>>> +		return V7M_CACHE_REG_DCCMVAC;
> >>>> +	case FLUSH_INVAL_POC:
> >>>> +		return V7M_CACHE_REG_DCCIMVAC;
> >>>> +	default:
> >>>> +		break;
> >>>> +	}
> >>>> +
> >>>> +	return NULL;
> >>>> +}
> >>>> +
> >>>> +static u32 get_cline_size(enum cache_type type) {
> >>>> +	u32 size;
> >>>> +
> >>>> +	if (type == DCACHE)
> >>>> +		clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
> >>>> +	else if (type == ICACHE)
> >>>> +		setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
> >>>> +	/* Make sure cache selection is effective for next memory access */
> >>>> +	dsb();
> >>>> +
> >>>> +	size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
> >>>> +	/* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
> >>>> +	size = 1 << (size + 2);
> >>>> +	debug("cache line size is %d\n", size);
> >>>> +
> >>>> +	return size;
> >>>> +}
> >>>> +
> >>>> +/* Perform the action like invalidate/clean on a range of cache
> >>>> +addresses */ static int action_cache_range(enum cache_action action, u32 start_addr,
> >>>> +			      int64_t size)
> >>>> +{
> >>>> +	u32 cline_size;
> >>>> +	u32 *action_reg;
> >>>> +	enum cache_type type;
> >>>
> >>> Does this ever check whether start_addr and size is unaligned ?
> >>
> >> address alignment is being done below before cache action.
> >
> > Ah, so we hide bugs which are introduced by unaligned accesses ? Well
> > please remove that and add address alignment check like the rest ...
> 
> which bug ? user passing unaligned address ?
> others are doing the same.
> 
> >
> >>> Also, you use u32 all over the place, but size is signed-i64 ? Why?
> >>
> >> Yes it is for condition "size > cline_size" when range/size is 4GB.
> >
> > Divide that by page size and you no longer need to use 64bit type on
> > 32bit system ...
> 
> page size... its armv7m.
> 
> >
> >>>> +	action_reg = get_action_reg_range(action);
> >>>> +	if (!action_reg)
> >>>> +		return -EINVAL;
> >>>> +	if (action == INVALIDATE_POU)
> >>>> +		type = ICACHE;
> >>>> +	else
> >>>> +		type = DCACHE;
> >>>> +
> >>>> +	/* Cache line size is minium size for the cache action */
> >>>> +	cline_size = get_cline_size(type);
> >>>> +	/* Align start address to cache line boundary */
> >>>> +	start_addr &= ~(cline_size - 1);
> >>>> +	debug("total size for cache action = %llx\n", size);
> >>>> +	do {
> >>>> +		writel(start_addr, action_reg);
> >>>> +		size -= cline_size;
> >>>> +		start_addr += cline_size;
> >>>> +	} while (size > cline_size);
> >>>> +
> >>>> +	/* Make sure cache action is effective for next memory access */
> >>>> +	dsb();
> >>>> +	isb();	/* Make sure instruction stream sees it */
> >>>> +	debug("cache action on range done\n");
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>
> >>> [...]
> >>>
> >>>>
> >>>> -#define V7M_SCB_BASE		0xE000ED00
> >>>> -#define V7M_MPU_BASE		0xE000ED90
> >>>> +/* armv7m fixed base addresses */
> >>>> +#define V7M_SCS_BASE		0xE000E000
> >>>
> >>> Is all of this really used in the code ?
> >>
> >> Not all of them, it is for the sake of armv7m address space completeness.
> >
> > Not sure we need them all then ...
> 
> thats what i said, not needed all for now. They are there for sake of completeness, what's the harm ?
> 
> Cheers,
> Vikas
> 
> >
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Tom Rini]

On Mon, Mar 27, 2017 at 01:02:44PM -0700, Vikas Manocha wrote:

> This patch adds armv7m instruction & data cache support.
> 
> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> cc: Christophe KERELLO <christophe.kerello@st.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170408/8e89610d/attachment.sig>

[U-Boot] [PATCH v4 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>
cc: Christophe KERELLO <christophe.kerello@st.com>
---

Changed in v4: None
Changed in v3: None
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 06af631..6f9704a 100644
--- a/arch/arm/mach-stm32/stm32f7/soc.c
+++ b/arch/arm/mach-stm32/stm32f7/soc.c
@@ -58,6 +58,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] [U-Boot, v4, 2/2] stm32f7: enable instruction & data cache

[Tom Rini]

On Mon, Mar 27, 2017 at 01:02:45PM -0700, Vikas Manocha wrote:

> It also enables commands for cache enable/disable/status.
> 
> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> cc: Christophe KERELLO <christophe.kerello@st.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170408/26610911/attachment.sig>