[U-Boot] [PATCH 0/6] Support Andes RISC-V l2cache on AE350 platform

[U-Boot] [PATCH 0/6] Support Andes RISC-V l2cache on AE350 platform

[Andes]

From: Rick Chen <rick@andestech.com>

Add a v5l2 cache controller driver that is usually found on
Andes RISC-V ae350 platform. It will parse and configure the
cache settings (data & instruction prefetch, data & tag latency)
from the device tree blob.

Also implement L2 cache flush and disable before jump to linux.
The sequence will be preferred as below:
L1 flush -> L1 disable -> L2 flush -> L2 disable

Rick Chen (6):
  dm: cache: add v5l2 cache controller driver
  riscv: ae350: use the v5l2 driver to configure the cache
  riscv: ae350: add imply v5l2 cache controller
  riscv: cache: Flush L2 cache before jump to linux
  riscv: dts: move out AE350 L2 node from cpus node
  riscv: ax25: use CCTL to flush d-cache

 arch/riscv/cpu/ax25/cache.c             |  22 ++++---
 arch/riscv/cpu/ax25/cpu.c               |   4 ++
 arch/riscv/dts/ae350_32.dts             |  17 ++++--
 arch/riscv/dts/ae350_64.dts             |  17 ++++--
 arch/riscv/include/asm/global_data.h    |   3 +
 arch/riscv/include/asm/v5l2cache.h      |  61 +++++++++++++++++++
 board/AndesTech/ax25-ae350/Kconfig      |   1 +
 board/AndesTech/ax25-ae350/ax25-ae350.c |  15 +++++
 drivers/cache/Kconfig                   |   9 +++
 drivers/cache/Makefile                  |   1 +
 drivers/cache/cache-v5l2.c              | 102 ++++++++++++++++++++++++++++++++
 11 files changed, 231 insertions(+), 21 deletions(-)
 create mode 100644 arch/riscv/include/asm/v5l2cache.h
 create mode 100644 drivers/cache/cache-v5l2.c

-- 
2.7.4

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Andes]

From: Rick Chen <rick@andestech.com>

Add a v5l2 cache controller driver that is usually found on
Andes RISC-V ae350 platform. It will parse the cache settings
from the dtb.

In this version tag and data ram control timing can be adjusted
by the requirement from the dtb.

Signed-off-by: Rick Chen <rick@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
---
 arch/riscv/include/asm/global_data.h |   3 ++
 arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
 drivers/cache/Kconfig                |   9 ++++
 drivers/cache/Makefile               |   1 +
 drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
 5 files changed, 176 insertions(+)
 create mode 100644 arch/riscv/include/asm/v5l2cache.h
 create mode 100644 drivers/cache/cache-v5l2.c

diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
index b74bd7e..6e52d5d 100644
--- a/arch/riscv/include/asm/global_data.h
+++ b/arch/riscv/include/asm/global_data.h
@@ -24,6 +24,9 @@ struct arch_global_data {
 #ifdef CONFIG_ANDES_PLMT
 	void __iomem *plmt;	/* plmt base address */
 #endif
+#ifdef CONFIG_V5L2_CACHE
+	void __iomem *v5l2;	/* v5l2 base address */
+#endif
 #ifdef CONFIG_SMP
 	struct ipi_data ipi[CONFIG_NR_CPUS];
 #endif
diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
new file mode 100644
index 0000000..8ed1c6c
--- /dev/null
+++ b/arch/riscv/include/asm/v5l2cache.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Andes Technology Corporation
+ * Rick Chen, Andes Technology Corporation <rick@andestech.com>
+ */
+
+#ifndef _ASM_V5_L2CACHE_H
+#define _ASM_V5_L2CACHE_H
+
+struct l2cache {
+	volatile u64	configure;
+	volatile u64	control;
+	volatile u64	hpm0;
+	volatile u64	hpm1;
+	volatile u64	hpm2;
+	volatile u64	hpm3;
+	volatile u64	error_status;
+	volatile u64	ecc_error;
+	volatile u64	cctl_command0;
+	volatile u64	cctl_access_line0;
+	volatile u64	cctl_command1;
+	volatile u64	cctl_access_line1;
+	volatile u64	cctl_command2;
+	volatile u64	cctl_access_line2;
+	volatile u64	cctl_command3;
+	volatile u64	cctl_access_line4;
+	volatile u64	cctl_status;
+};
+
+/* Control Register */
+#define L2_ENABLE	0x1
+/* prefetch */
+#define IPREPETCH_OFF	3
+#define DPREPETCH_OFF	5
+#define IPREPETCH_MSK	(3 << IPREPETCH_OFF)
+#define DPREPETCH_MSK	(3 << DPREPETCH_OFF)
+/* tag ram */
+#define TRAMOCTL_OFF	8
+#define TRAMICTL_OFF	10
+#define TRAMOCTL_MSK	(3 << TRAMOCTL_OFF)
+#define TRAMICTL_MSK	BIT(TRAMICTL_OFF)
+/* data ram */
+#define DRAMOCTL_OFF	11
+#define DRAMICTL_OFF	13
+#define DRAMOCTL_MSK	(3 << DRAMOCTL_OFF)
+#define DRAMICTL_MSK	BIT(DRAMICTL_OFF)
+
+/* CCTL Command Register */
+#define CCTL_CMD_REG(base, hart)	((ulong)(base) + 0x40 + (hart) * 0x10)
+#define L2_WBINVAL_ALL	0x12
+
+/* CCTL Status Register */
+#define CCTL_STATUS_MSK(hart)		(0xf << ((hart) * 4))
+#define CCTL_STATUS_IDLE(hart)		(0 << ((hart) * 4))
+#define CCTL_STATUS_PROCESS(hart)	(1 << ((hart) * 4))
+#define CCTL_STATUS_ILLEGAL(hart)	(2 << ((hart) * 4))
+
+void v5l2_enable(void);
+void v5l2_disable(void);
+
+#endif /* _ASM_V5_L2CACHE_H */
diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
index 24def7a..665689a 100644
--- a/drivers/cache/Kconfig
+++ b/drivers/cache/Kconfig
@@ -22,4 +22,13 @@ config L2X0_CACHE
 	  ARMv7(32-bit) devices. The driver configures the cache settings
 	  found in the device tree.
 
+config V5L2_CACHE
+	tristate "Andes V5L2 cache driver"
+	select CACHE
+	depends on RISCV_NDS_CACHE
+	help
+	  Support Andes V5L2 cache controller in AE350 platform.
+	  It will configure tag and data ram timing control from the
+	  device tree and enable L2 cache.
+
 endmenu
diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
index 9deb961..4a6458c 100644
--- a/drivers/cache/Makefile
+++ b/drivers/cache/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_CACHE) += cache-uclass.o
 obj-$(CONFIG_SANDBOX) += sandbox_cache.o
 obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
+obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
new file mode 100644
index 0000000..7022feb
--- /dev/null
+++ b/drivers/cache/cache-v5l2.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Andes Technology Corporation
+ * Rick Chen, Andes Technology Corporation <rick@andestech.com>
+ */
+
+#include <common.h>
+#include <command.h>
+#include <dm.h>
+#include <asm/io.h>
+#include <dm/ofnode.h>
+#include <asm/v5l2cache.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+void v5l2_enable(void)
+{
+	struct l2cache *regs = gd->arch.v5l2;
+
+	if (regs)
+		setbits_le32(&regs->control, L2_ENABLE);
+}
+
+void v5l2_disable(void)
+{
+	volatile struct l2cache *regs = gd->arch.v5l2;
+	u8 hart = gd->arch.boot_hart;
+	void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
+
+	if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
+		writel(L2_WBINVAL_ALL, cctlcmd);
+
+		while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
+			if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
+				printf("L2 flush illegal! hanging...");
+				hang();
+			}
+		}
+		clrbits_le32(&regs->control, L2_ENABLE);
+	}
+}
+
+static void v5l2_of_parse_and_init(struct udevice *dev)
+{
+	struct l2cache *regs;
+	u32 ctl_val, prefetch;
+	u32 tram_ctl[2];
+	u32 dram_ctl[2];
+
+	regs = (struct l2cache *)dev_read_addr(dev);
+	gd->arch.v5l2 = regs;
+	ctl_val = readl(&regs->control);
+
+	if (!(ctl_val & L2_ENABLE))
+		ctl_val |= L2_ENABLE;
+
+	/* Instruction and data fetch prefetch depth */
+	if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
+		ctl_val &= ~(IPREPETCH_MSK);
+		ctl_val |= (prefetch << IPREPETCH_OFF);
+	}
+
+	if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
+		ctl_val &= ~(DPREPETCH_MSK);
+		ctl_val |= (prefetch << DPREPETCH_OFF);
+	}
+
+	/* Set tag RAM and data RAM setup and output cycle */
+	if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
+		ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
+		ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
+		ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
+	}
+
+	if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
+		ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
+		ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
+		ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
+	}
+
+	writel(ctl_val, &regs->control);
+}
+
+static int v5l2_probe(struct udevice *dev)
+{
+	v5l2_of_parse_and_init(dev);
+
+	return 0;
+}
+
+static const struct udevice_id v5l2_cache_ids[] = {
+	{ .compatible = "cache" },
+	{}
+};
+
+U_BOOT_DRIVER(v5l2_cache) = {
+	.name   = "v5l2_cache",
+	.id     = UCLASS_CACHE,
+	.of_match = v5l2_cache_ids,
+	.probe	= v5l2_probe,
+	.flags  = DM_FLAG_PRE_RELOC,
+};
-- 
2.7.4

[U-Boot] [PATCH 2/6] riscv: ae350: use the v5l2 driver to configure the cache

[Andes]

From: Rick Chen <rick@andestech.com>

Find the UCLASS_CACHE driver to configure the cache controller's
settings.

Signed-off-by: Rick Chen <rick@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
---
 board/AndesTech/ax25-ae350/ax25-ae350.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c
index 3d65ce7..686ec4a 100644
--- a/board/AndesTech/ax25-ae350/ax25-ae350.c
+++ b/board/AndesTech/ax25-ae350/ax25-ae350.c
@@ -11,6 +11,7 @@
 #include <linux/io.h>
 #include <faraday/ftsmc020.h>
 #include <fdtdec.h>
+#include <dm.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -93,10 +94,24 @@ int smc_init(void)
 	return 0;
 }
 
+int v5l2_init(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	ret = uclass_get_device(UCLASS_CACHE, 0, &dev);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 #ifdef CONFIG_BOARD_EARLY_INIT_F
 int board_early_init_f(void)
 {
 	smc_init();
+	v5l2_init();
 
 	return 0;
 }
-- 
2.7.4

[U-Boot] [PATCH 3/6] riscv: ae350: add imply v5l2 cache controller

[Andes]

From: Rick Chen <rick@andestech.com>

Select the v5l2 UCLASS_CACHE driver for AE350.

Signed-off-by: Rick Chen <rick@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
---
 board/AndesTech/ax25-ae350/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/board/AndesTech/ax25-ae350/Kconfig b/board/AndesTech/ax25-ae350/Kconfig
index 5e682b6..dd299d9 100644
--- a/board/AndesTech/ax25-ae350/Kconfig
+++ b/board/AndesTech/ax25-ae350/Kconfig
@@ -25,5 +25,6 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	def_bool y
 	select RISCV_NDS
 	imply SMP
+	imply V5L2_CACHE
 
 endif
-- 
2.7.4

[U-Boot] [PATCH 4/6] riscv: cache: Flush L2 cache before jump to linux

[Andes]

From: Rick Chen <rick@andestech.com>

Flush and disable cache in cleanup_before_linux()
which will be called before jump to linux.

The sequence will be preferred as below:
L1 flush -> L1 disable -> L2 flush -> L2 disable

Signed-off-by: Rick Chen <rick@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
---
 arch/riscv/cpu/ax25/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/cpu/ax25/cpu.c b/arch/riscv/cpu/ax25/cpu.c
index 76689b2..9e7579a 100644
--- a/arch/riscv/cpu/ax25/cpu.c
+++ b/arch/riscv/cpu/ax25/cpu.c
@@ -7,6 +7,7 @@
 /* CPU specific code */
 #include <common.h>
 #include <asm/cache.h>
+#include <asm/v5l2cache.h>
 
 /*
  * cleanup_before_linux() is called just before we call linux
@@ -22,6 +23,9 @@ int cleanup_before_linux(void)
 	cache_flush();
 	icache_disable();
 	dcache_disable();
+#ifdef CONFIG_RISCV_NDS_CACHE
+	v5l2_disable();
+#endif
 
 	return 0;
 }
-- 
2.7.4

[U-Boot] [PATCH 5/6] riscv: dts: move out AE350 L2 node from cpus node

[Andes]

From: Rick Chen <rick@andestech.com>

When L2 node exists inside cpus node, uclass_get_device
can not parse L2 node successfully. So move it outside
from cpus node.

Also add tag-ram-ctl and data-ram-ctl attributes for
v5l2 cache controller driver. This can adjust timing
by requirement from dtb to improve performance.

Signed-off-by: Rick Chen <rick@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
---
 arch/riscv/dts/ae350_32.dts | 17 +++++++++++------
 arch/riscv/dts/ae350_64.dts | 17 +++++++++++------
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/dts/ae350_32.dts b/arch/riscv/dts/ae350_32.dts
index cb6ee13..83abfcb 100644
--- a/arch/riscv/dts/ae350_32.dts
+++ b/arch/riscv/dts/ae350_32.dts
@@ -62,13 +62,18 @@
 				compatible = "riscv,cpu-intc";
 			};
 		};
+	};
 
-		L2: l2-cache at e0500000 {
-			compatible = "cache";
-			cache-level = <2>;
-			cache-size = <0x40000>;
-			reg = <0x0 0xe0500000 0x0 0x40000>;
-		};
+	L2: l2-cache at e0500000 {
+		compatible = "cache";
+		cache-level = <2>;
+		cache-size = <0x40000>;
+		reg = <0xe0500000 0x40000>;
+		andes,inst-prefetch = <3>;
+		andes,data-prefetch = <3>;
+		// The value format is <XRAMOCTL XRAMICTL>
+		andes,tag-ram-ctl = <0 0>;
+		andes,data-ram-ctl = <0 0>;
 	};
 
 	memory at 0 {
diff --git a/arch/riscv/dts/ae350_64.dts b/arch/riscv/dts/ae350_64.dts
index 705491a..7009bdc 100644
--- a/arch/riscv/dts/ae350_64.dts
+++ b/arch/riscv/dts/ae350_64.dts
@@ -62,13 +62,18 @@
 				compatible = "riscv,cpu-intc";
 			};
 		};
+	};
 
-		L2: l2-cache at e0500000 {
-			compatible = "cache";
-			cache-level = <2>;
-			cache-size = <0x40000>;
-			reg = <0x0 0xe0500000 0x0 0x40000>;
-		};
+	L2: l2-cache at e0500000 {
+		compatible = "cache";
+		cache-level = <2>;
+		cache-size = <0x40000>;
+		reg = <0x0 0xe0500000 0x0 0x40000>;
+		andes,inst-prefetch = <3>;
+		andes,data-prefetch = <3>;
+		// The value format is <XRAMOCTL XRAMICTL>
+		andes,tag-ram-ctl = <0 0>;
+		andes,data-ram-ctl = <0 0>;
 	};
 
 	memory at 0 {
-- 
2.7.4

[U-Boot] [PATCH 6/6] riscv: ax25: use CCTL to flush d-cache

[Andes]

From: Rick Chen <rick@andestech.com>

Use CCTL command to do d-cache write back and invalidate
instead of fence.

Signed-off-by: Rick Chen <rick@andestech.com>
Cc: Greentime Hu <greentime@andestech.com>
---
 arch/riscv/cpu/ax25/cache.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/cpu/ax25/cache.c b/arch/riscv/cpu/ax25/cache.c
index 228fc55..d30071e 100644
--- a/arch/riscv/cpu/ax25/cache.c
+++ b/arch/riscv/cpu/ax25/cache.c
@@ -5,17 +5,21 @@
  */
 
 #include <common.h>
+#include <asm/csr.h>
+
+#ifdef CONFIG_RISCV_NDS_CACHE
+/* mcctlcommand */
+#define CCTL_REG_MCCTLCOMMAND_NUM      0x7cc
+
+/* D-cache operation */
+#define CCTL_L1D_WBINVAL_ALL	6
+#endif
 
 void flush_dcache_all(void)
 {
-	/*
-	 * Andes' AX25 does not have a coherence agent. U-Boot must use data
-	 * cache flush and invalidate functions to keep data in the system
-	 * coherent.
-	 * The implementation of the fence instruction in the AX25 flushes the
-	 * data cache and is used for this purpose.
-	 */
-	asm volatile ("fence" ::: "memory");
+#ifdef CONFIG_RISCV_NDS_CACHE
+	csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
+#endif
 }
 
 void flush_dcache_range(unsigned long start, unsigned long end)
@@ -72,8 +76,8 @@ void dcache_disable(void)
 {
 #ifndef CONFIG_SYS_DCACHE_OFF
 #ifdef CONFIG_RISCV_NDS_CACHE
+	csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
 	asm volatile (
-		"fence\n\t"
 		"csrr t1, mcache_ctl\n\t"
 		"andi t0, t1, ~0x2\n\t"
 		"csrw mcache_ctl, t0\n\t"
-- 
2.7.4

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Bin Meng]

Hi Rick,

On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
>
> From: Rick Chen <rick@andestech.com>
>
> Add a v5l2 cache controller driver that is usually found on
> Andes RISC-V ae350 platform. It will parse the cache settings
> from the dtb.
>
> In this version tag and data ram control timing can be adjusted
> by the requirement from the dtb.
>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> ---
>  arch/riscv/include/asm/global_data.h |   3 ++
>  arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
>  drivers/cache/Kconfig                |   9 ++++
>  drivers/cache/Makefile               |   1 +
>  drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
>  5 files changed, 176 insertions(+)
>  create mode 100644 arch/riscv/include/asm/v5l2cache.h
>  create mode 100644 drivers/cache/cache-v5l2.c
>
> diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> index b74bd7e..6e52d5d 100644
> --- a/arch/riscv/include/asm/global_data.h
> +++ b/arch/riscv/include/asm/global_data.h
> @@ -24,6 +24,9 @@ struct arch_global_data {
>  #ifdef CONFIG_ANDES_PLMT
>         void __iomem *plmt;     /* plmt base address */
>  #endif
> +#ifdef CONFIG_V5L2_CACHE
> +       void __iomem *v5l2;     /* v5l2 base address */
> +#endif

Please do not expose this to global data if it is only used inside a
driver. Anything that is here is for "global" usage.

>  #ifdef CONFIG_SMP
>         struct ipi_data ipi[CONFIG_NR_CPUS];
>  #endif
> diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> new file mode 100644
> index 0000000..8ed1c6c
> --- /dev/null
> +++ b/arch/riscv/include/asm/v5l2cache.h

Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
of the name, would it be more readable to name it as v5_l2cache.h? Or
even add more information to v5, for me, I don't know what v5 stands
for :)

> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2019 Andes Technology Corporation
> + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> + */
> +
> +#ifndef _ASM_V5_L2CACHE_H
> +#define _ASM_V5_L2CACHE_H
> +
> +struct l2cache {
> +       volatile u64    configure;

checkpatch.pl will report warnings against volatile usage. I think we
should drop these.

> +       volatile u64    control;
> +       volatile u64    hpm0;
> +       volatile u64    hpm1;
> +       volatile u64    hpm2;
> +       volatile u64    hpm3;
> +       volatile u64    error_status;
> +       volatile u64    ecc_error;
> +       volatile u64    cctl_command0;
> +       volatile u64    cctl_access_line0;
> +       volatile u64    cctl_command1;
> +       volatile u64    cctl_access_line1;
> +       volatile u64    cctl_command2;
> +       volatile u64    cctl_access_line2;
> +       volatile u64    cctl_command3;
> +       volatile u64    cctl_access_line4;
> +       volatile u64    cctl_status;
> +};
> +
> +/* Control Register */
> +#define L2_ENABLE      0x1
> +/* prefetch */
> +#define IPREPETCH_OFF  3
> +#define DPREPETCH_OFF  5
> +#define IPREPETCH_MSK  (3 << IPREPETCH_OFF)
> +#define DPREPETCH_MSK  (3 << DPREPETCH_OFF)
> +/* tag ram */
> +#define TRAMOCTL_OFF   8
> +#define TRAMICTL_OFF   10
> +#define TRAMOCTL_MSK   (3 << TRAMOCTL_OFF)
> +#define TRAMICTL_MSK   BIT(TRAMICTL_OFF)
> +/* data ram */
> +#define DRAMOCTL_OFF   11
> +#define DRAMICTL_OFF   13
> +#define DRAMOCTL_MSK   (3 << DRAMOCTL_OFF)
> +#define DRAMICTL_MSK   BIT(DRAMICTL_OFF)
> +
> +/* CCTL Command Register */
> +#define CCTL_CMD_REG(base, hart)       ((ulong)(base) + 0x40 + (hart) * 0x10)
> +#define L2_WBINVAL_ALL 0x12
> +
> +/* CCTL Status Register */
> +#define CCTL_STATUS_MSK(hart)          (0xf << ((hart) * 4))
> +#define CCTL_STATUS_IDLE(hart)         (0 << ((hart) * 4))
> +#define CCTL_STATUS_PROCESS(hart)      (1 << ((hart) * 4))
> +#define CCTL_STATUS_ILLEGAL(hart)      (2 << ((hart) * 4))
> +
> +void v5l2_enable(void);
> +void v5l2_disable(void);
> +
> +#endif /* _ASM_V5_L2CACHE_H */
> diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> index 24def7a..665689a 100644
> --- a/drivers/cache/Kconfig
> +++ b/drivers/cache/Kconfig
> @@ -22,4 +22,13 @@ config L2X0_CACHE
>           ARMv7(32-bit) devices. The driver configures the cache settings
>           found in the device tree.
>
> +config V5L2_CACHE
> +       tristate "Andes V5L2 cache driver"

This should be bool. U-Boot does not support "tristate"

> +       select CACHE
> +       depends on RISCV_NDS_CACHE
> +       help
> +         Support Andes V5L2 cache controller in AE350 platform.
> +         It will configure tag and data ram timing control from the
> +         device tree and enable L2 cache.
> +
>  endmenu
> diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> index 9deb961..4a6458c 100644
> --- a/drivers/cache/Makefile
> +++ b/drivers/cache/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_CACHE) += cache-uclass.o
>  obj-$(CONFIG_SANDBOX) += sandbox_cache.o
>  obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> new file mode 100644
> index 0000000..7022feb
> --- /dev/null
> +++ b/drivers/cache/cache-v5l2.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Andes Technology Corporation
> + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <dm.h>
> +#include <asm/io.h>
> +#include <dm/ofnode.h>
> +#include <asm/v5l2cache.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +void v5l2_enable(void)

This should really be avoided. It looks current DM cache uclass driver
is lacking of ops to do cache enable/disable. Please improve the DM
cache uclass driver first.

> +{
> +       struct l2cache *regs = gd->arch.v5l2;
> +
> +       if (regs)
> +               setbits_le32(&regs->control, L2_ENABLE);
> +}
> +
> +void v5l2_disable(void)
> +{
> +       volatile struct l2cache *regs = gd->arch.v5l2;
> +       u8 hart = gd->arch.boot_hart;
> +       void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> +
> +       if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
> +               writel(L2_WBINVAL_ALL, cctlcmd);
> +
> +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> +                               printf("L2 flush illegal! hanging...");
> +                               hang();
> +                       }
> +               }
> +               clrbits_le32(&regs->control, L2_ENABLE);
> +       }
> +}
> +
> +static void v5l2_of_parse_and_init(struct udevice *dev)

The entire function below should really be created as the driver's
ofdata_to_platdata() function, inside which all DT properties are
parsed and saved to driver's platdata. After that, there is no need to
get register base from global data.

> +{
> +       struct l2cache *regs;
> +       u32 ctl_val, prefetch;
> +       u32 tram_ctl[2];
> +       u32 dram_ctl[2];
> +
> +       regs = (struct l2cache *)dev_read_addr(dev);
> +       gd->arch.v5l2 = regs;
> +       ctl_val = readl(&regs->control);
> +
> +       if (!(ctl_val & L2_ENABLE))
> +               ctl_val |= L2_ENABLE;
> +
> +       /* Instruction and data fetch prefetch depth */
> +       if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> +               ctl_val &= ~(IPREPETCH_MSK);
> +               ctl_val |= (prefetch << IPREPETCH_OFF);
> +       }
> +
> +       if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> +               ctl_val &= ~(DPREPETCH_MSK);
> +               ctl_val |= (prefetch << DPREPETCH_OFF);
> +       }
> +
> +       /* Set tag RAM and data RAM setup and output cycle */
> +       if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> +               ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> +               ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> +               ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> +       }
> +
> +       if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> +               ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> +               ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> +               ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> +       }
> +
> +       writel(ctl_val, &regs->control);
> +}
> +
> +static int v5l2_probe(struct udevice *dev)
> +{
> +       v5l2_of_parse_and_init(dev);
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id v5l2_cache_ids[] = {
> +       { .compatible = "cache" },

I suspect this compatible string is too generic. Has this been
reviewed by DT community upstream?

> +       {}
> +};
> +
> +U_BOOT_DRIVER(v5l2_cache) = {
> +       .name   = "v5l2_cache",
> +       .id     = UCLASS_CACHE,
> +       .of_match = v5l2_cache_ids,
> +       .probe  = v5l2_probe,
> +       .flags  = DM_FLAG_PRE_RELOC,
> +};
> --

Regards,
Bin

[U-Boot] [PATCH 2/6] riscv: ae350: use the v5l2 driver to configure the cache

[Bin Meng]

Hi Rick,

On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
>
> From: Rick Chen <rick@andestech.com>
>
> Find the UCLASS_CACHE driver to configure the cache controller's
> settings.
>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> ---
>  board/AndesTech/ax25-ae350/ax25-ae350.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c
> index 3d65ce7..686ec4a 100644
> --- a/board/AndesTech/ax25-ae350/ax25-ae350.c
> +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c
> @@ -11,6 +11,7 @@
>  #include <linux/io.h>
>  #include <faraday/ftsmc020.h>
>  #include <fdtdec.h>
> +#include <dm.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -93,10 +94,24 @@ int smc_init(void)
>         return 0;
>  }
>
> +int v5l2_init(void)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       ret = uclass_get_device(UCLASS_CACHE, 0, &dev);
> +
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
>  #ifdef CONFIG_BOARD_EARLY_INIT_F
>  int board_early_init_f(void)
>  {
>         smc_init();
> +       v5l2_init();

Please check the return value here, or you can make v512_init() returns void.

>
>         return 0;
>  }
> --

Regards,
Bin

[U-Boot] [PATCH 3/6] riscv: ae350: add imply v5l2 cache controller

[Bin Meng]

Hi Rick,

On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
>
> From: Rick Chen <rick@andestech.com>
>
> Select the v5l2 UCLASS_CACHE driver for AE350.
>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> ---
>  board/AndesTech/ax25-ae350/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/board/AndesTech/ax25-ae350/Kconfig b/board/AndesTech/ax25-ae350/Kconfig
> index 5e682b6..dd299d9 100644
> --- a/board/AndesTech/ax25-ae350/Kconfig
> +++ b/board/AndesTech/ax25-ae350/Kconfig
> @@ -25,5 +25,6 @@ config BOARD_SPECIFIC_OPTIONS # dummy
>         def_bool y
>         select RISCV_NDS
>         imply SMP
> +       imply V5L2_CACHE

I believe L2 cache is a CPU specific feature, hence this should be
implied from arch/riscv/cpu/ax25/Kconfig

Regards,
Bin

[U-Boot] [PATCH 4/6] riscv: cache: Flush L2 cache before jump to linux

[Bin Meng]

Hi Rick,

On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
>
> From: Rick Chen <rick@andestech.com>
>
> Flush and disable cache in cleanup_before_linux()
> which will be called before jump to linux.
>
> The sequence will be preferred as below:
> L1 flush -> L1 disable -> L2 flush -> L2 disable
>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> ---
>  arch/riscv/cpu/ax25/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/cpu/ax25/cpu.c b/arch/riscv/cpu/ax25/cpu.c
> index 76689b2..9e7579a 100644
> --- a/arch/riscv/cpu/ax25/cpu.c
> +++ b/arch/riscv/cpu/ax25/cpu.c
> @@ -7,6 +7,7 @@
>  /* CPU specific code */
>  #include <common.h>
>  #include <asm/cache.h>
> +#include <asm/v5l2cache.h>
>
>  /*
>   * cleanup_before_linux() is called just before we call linux
> @@ -22,6 +23,9 @@ int cleanup_before_linux(void)
>         cache_flush();
>         icache_disable();
>         dcache_disable();
> +#ifdef CONFIG_RISCV_NDS_CACHE
> +       v5l2_disable();
> +#endif

The direct call into a driver should be avoided. Instead, use a proper
DM cache uclass driver API (see my review comments in patch [1/6])

Regards,
Bin

[U-Boot] [PATCH 5/6] riscv: dts: move out AE350 L2 node from cpus node

[Bin Meng]

Hi Rick,

On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
>
> From: Rick Chen <rick@andestech.com>
>
> When L2 node exists inside cpus node, uclass_get_device
> can not parse L2 node successfully. So move it outside
> from cpus node.
>
> Also add tag-ram-ctl and data-ram-ctl attributes for
> v5l2 cache controller driver. This can adjust timing
> by requirement from dtb to improve performance.
>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> ---
>  arch/riscv/dts/ae350_32.dts | 17 +++++++++++------
>  arch/riscv/dts/ae350_64.dts | 17 +++++++++++------
>  2 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/arch/riscv/dts/ae350_32.dts b/arch/riscv/dts/ae350_32.dts
> index cb6ee13..83abfcb 100644
> --- a/arch/riscv/dts/ae350_32.dts
> +++ b/arch/riscv/dts/ae350_32.dts
> @@ -62,13 +62,18 @@
>                                 compatible = "riscv,cpu-intc";
>                         };
>                 };
> +       };
>
> -               L2: l2-cache at e0500000 {
> -                       compatible = "cache";
> -                       cache-level = <2>;
> -                       cache-size = <0x40000>;
> -                       reg = <0x0 0xe0500000 0x0 0x40000>;
> -               };
> +       L2: l2-cache at e0500000 {
> +               compatible = "cache";

too generic compatible string (see my previous comments in patch [1/6])

> +               cache-level = <2>;
> +               cache-size = <0x40000>;
> +               reg = <0xe0500000 0x40000>;
> +               andes,inst-prefetch = <3>;
> +               andes,data-prefetch = <3>;
> +               // The value format is <XRAMOCTL XRAMICTL>

nits: no //, use /* */

> +               andes,tag-ram-ctl = <0 0>;
> +               andes,data-ram-ctl = <0 0>;
>         };
>
>         memory at 0 {
> diff --git a/arch/riscv/dts/ae350_64.dts b/arch/riscv/dts/ae350_64.dts
> index 705491a..7009bdc 100644
> --- a/arch/riscv/dts/ae350_64.dts
> +++ b/arch/riscv/dts/ae350_64.dts
> @@ -62,13 +62,18 @@
>                                 compatible = "riscv,cpu-intc";
>                         };
>                 };
> +       };
>
> -               L2: l2-cache at e0500000 {
> -                       compatible = "cache";
> -                       cache-level = <2>;
> -                       cache-size = <0x40000>;
> -                       reg = <0x0 0xe0500000 0x0 0x40000>;
> -               };
> +       L2: l2-cache at e0500000 {
> +               compatible = "cache";
> +               cache-level = <2>;
> +               cache-size = <0x40000>;
> +               reg = <0x0 0xe0500000 0x0 0x40000>;
> +               andes,inst-prefetch = <3>;
> +               andes,data-prefetch = <3>;
> +               // The value format is <XRAMOCTL XRAMICTL>

nits: no //, use /* */

> +               andes,tag-ram-ctl = <0 0>;
> +               andes,data-ram-ctl = <0 0>;
>         };
>
>         memory at 0 {
> --

Regards,
Bin

[U-Boot] [PATCH 6/6] riscv: ax25: use CCTL to flush d-cache

[Bin Meng]

Hi Rick,

On Tue, May 28, 2019 at 5:45 PM Andes <uboot@andestech.com> wrote:
>
> From: Rick Chen <rick@andestech.com>
>
> Use CCTL command to do d-cache write back and invalidate
> instead of fence.
>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Cc: Greentime Hu <greentime@andestech.com>
> ---
>  arch/riscv/cpu/ax25/cache.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/cpu/ax25/cache.c b/arch/riscv/cpu/ax25/cache.c
> index 228fc55..d30071e 100644
> --- a/arch/riscv/cpu/ax25/cache.c
> +++ b/arch/riscv/cpu/ax25/cache.c
> @@ -5,17 +5,21 @@
>   */
>
>  #include <common.h>
> +#include <asm/csr.h>
> +
> +#ifdef CONFIG_RISCV_NDS_CACHE
> +/* mcctlcommand */
> +#define CCTL_REG_MCCTLCOMMAND_NUM      0x7cc
> +
> +/* D-cache operation */
> +#define CCTL_L1D_WBINVAL_ALL   6
> +#endif
>
>  void flush_dcache_all(void)
>  {
> -       /*
> -        * Andes' AX25 does not have a coherence agent. U-Boot must use data
> -        * cache flush and invalidate functions to keep data in the system
> -        * coherent.
> -        * The implementation of the fence instruction in the AX25 flushes the
> -        * data cache and is used for this purpose.
> -        */
> -       asm volatile ("fence" ::: "memory");
> +#ifdef CONFIG_RISCV_NDS_CACHE
> +       csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);

I think CCTL_REG_MCCTLCOMMAND_NUM is a vendor specific CSR. Does
upstream GCC support this CSR?

> +#endif
>  }
>
>  void flush_dcache_range(unsigned long start, unsigned long end)
> @@ -72,8 +76,8 @@ void dcache_disable(void)
>  {
>  #ifndef CONFIG_SYS_DCACHE_OFF
>  #ifdef CONFIG_RISCV_NDS_CACHE
> +       csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
>         asm volatile (
> -               "fence\n\t"
>                 "csrr t1, mcache_ctl\n\t"
>                 "andi t0, t1, ~0x2\n\t"
>                 "csrw mcache_ctl, t0\n\t"
> --

Regards,
Bin

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Rick Chen]

Hi Bin

Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > Add a v5l2 cache controller driver that is usually found on
> > Andes RISC-V ae350 platform. It will parse the cache settings
> > from the dtb.
> >
> > In this version tag and data ram control timing can be adjusted
> > by the requirement from the dtb.
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  arch/riscv/include/asm/global_data.h |   3 ++
> >  arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
> >  drivers/cache/Kconfig                |   9 ++++
> >  drivers/cache/Makefile               |   1 +
> >  drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 176 insertions(+)
> >  create mode 100644 arch/riscv/include/asm/v5l2cache.h
> >  create mode 100644 drivers/cache/cache-v5l2.c
> >
> > diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> > index b74bd7e..6e52d5d 100644
> > --- a/arch/riscv/include/asm/global_data.h
> > +++ b/arch/riscv/include/asm/global_data.h
> > @@ -24,6 +24,9 @@ struct arch_global_data {
> >  #ifdef CONFIG_ANDES_PLMT
> >         void __iomem *plmt;     /* plmt base address */
> >  #endif
> > +#ifdef CONFIG_V5L2_CACHE
> > +       void __iomem *v5l2;     /* v5l2 base address */
> > +#endif
>
> Please do not expose this to global data if it is only used inside a
> driver. Anything that is here is for "global" usage.

OK.
I will remove it.

>
> >  #ifdef CONFIG_SMP
> >         struct ipi_data ipi[CONFIG_NR_CPUS];
> >  #endif
> > diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> > new file mode 100644
> > index 0000000..8ed1c6c
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/v5l2cache.h
>
> Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
> of the name, would it be more readable to name it as v5_l2cache.h? Or
> even add more information to v5, for me, I don't know what v5 stands
> for :)

OK
I will create arch/riscv/include/asm/arch-ax25/v5_l2cache.h

>
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2019 Andes Technology Corporation
> > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > + */
> > +
> > +#ifndef _ASM_V5_L2CACHE_H
> > +#define _ASM_V5_L2CACHE_H
> > +
> > +struct l2cache {
> > +       volatile u64    configure;
>
> checkpatch.pl will report warnings against volatile usage. I think we
> should drop these.

I know that checkpatch.pl will report this warning.
But I still need to add volatile. It help to fix some unpredictable problem.
Without this some driver code flows will be optimized and go wrong somehow.

>
> > +       volatile u64    control;
> > +       volatile u64    hpm0;
> > +       volatile u64    hpm1;
> > +       volatile u64    hpm2;
> > +       volatile u64    hpm3;
> > +       volatile u64    error_status;
> > +       volatile u64    ecc_error;
> > +       volatile u64    cctl_command0;
> > +       volatile u64    cctl_access_line0;
> > +       volatile u64    cctl_command1;
> > +       volatile u64    cctl_access_line1;
> > +       volatile u64    cctl_command2;
> > +       volatile u64    cctl_access_line2;
> > +       volatile u64    cctl_command3;
> > +       volatile u64    cctl_access_line4;
> > +       volatile u64    cctl_status;
> > +};
> > +
> > +/* Control Register */
> > +#define L2_ENABLE      0x1
> > +/* prefetch */
> > +#define IPREPETCH_OFF  3
> > +#define DPREPETCH_OFF  5
> > +#define IPREPETCH_MSK  (3 << IPREPETCH_OFF)
> > +#define DPREPETCH_MSK  (3 << DPREPETCH_OFF)
> > +/* tag ram */
> > +#define TRAMOCTL_OFF   8
> > +#define TRAMICTL_OFF   10
> > +#define TRAMOCTL_MSK   (3 << TRAMOCTL_OFF)
> > +#define TRAMICTL_MSK   BIT(TRAMICTL_OFF)
> > +/* data ram */
> > +#define DRAMOCTL_OFF   11
> > +#define DRAMICTL_OFF   13
> > +#define DRAMOCTL_MSK   (3 << DRAMOCTL_OFF)
> > +#define DRAMICTL_MSK   BIT(DRAMICTL_OFF)
> > +
> > +/* CCTL Command Register */
> > +#define CCTL_CMD_REG(base, hart)       ((ulong)(base) + 0x40 + (hart) * 0x10)
> > +#define L2_WBINVAL_ALL 0x12
> > +
> > +/* CCTL Status Register */
> > +#define CCTL_STATUS_MSK(hart)          (0xf << ((hart) * 4))
> > +#define CCTL_STATUS_IDLE(hart)         (0 << ((hart) * 4))
> > +#define CCTL_STATUS_PROCESS(hart)      (1 << ((hart) * 4))
> > +#define CCTL_STATUS_ILLEGAL(hart)      (2 << ((hart) * 4))
> > +
> > +void v5l2_enable(void);
> > +void v5l2_disable(void);
> > +
> > +#endif /* _ASM_V5_L2CACHE_H */
> > diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> > index 24def7a..665689a 100644
> > --- a/drivers/cache/Kconfig
> > +++ b/drivers/cache/Kconfig
> > @@ -22,4 +22,13 @@ config L2X0_CACHE
> >           ARMv7(32-bit) devices. The driver configures the cache settings
> >           found in the device tree.
> >
> > +config V5L2_CACHE
> > +       tristate "Andes V5L2 cache driver"
>
> This should be bool. U-Boot does not support "tristate"

I will modify it as bool.

>
> > +       select CACHE
> > +       depends on RISCV_NDS_CACHE
> > +       help
> > +         Support Andes V5L2 cache controller in AE350 platform.
> > +         It will configure tag and data ram timing control from the
> > +         device tree and enable L2 cache.
> > +
> >  endmenu
> > diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> > index 9deb961..4a6458c 100644
> > --- a/drivers/cache/Makefile
> > +++ b/drivers/cache/Makefile
> > @@ -2,3 +2,4 @@
> >  obj-$(CONFIG_CACHE) += cache-uclass.o
> >  obj-$(CONFIG_SANDBOX) += sandbox_cache.o
> >  obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> > +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> > diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> > new file mode 100644
> > index 0000000..7022feb
> > --- /dev/null
> > +++ b/drivers/cache/cache-v5l2.c
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Andes Technology Corporation
> > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <dm.h>
> > +#include <asm/io.h>
> > +#include <dm/ofnode.h>
> > +#include <asm/v5l2cache.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +void v5l2_enable(void)
>
> This should really be avoided. It looks current DM cache uclass driver
> is lacking of ops to do cache enable/disable. Please improve the DM
> cache uclass driver first.

OK
I will improve the DM cache uclass driver.

>
> > +{
> > +       struct l2cache *regs = gd->arch.v5l2;
> > +
> > +       if (regs)
> > +               setbits_le32(&regs->control, L2_ENABLE);
> > +}
> > +
> > +void v5l2_disable(void)
> > +{
> > +       volatile struct l2cache *regs = gd->arch.v5l2;
> > +       u8 hart = gd->arch.boot_hart;
> > +       void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> > +
> > +       if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
> > +               writel(L2_WBINVAL_ALL, cctlcmd);
> > +
> > +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> > +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> > +                               printf("L2 flush illegal! hanging...");
> > +                               hang();
> > +                       }
> > +               }
> > +               clrbits_le32(&regs->control, L2_ENABLE);
> > +       }
> > +}
> > +
> > +static void v5l2_of_parse_and_init(struct udevice *dev)
>
> The entire function below should really be created as the driver's
> ofdata_to_platdata() function, inside which all DT properties are
> parsed and saved to driver's platdata. After that, there is no need to
> get register base from global data.

I will use ofdata_to_platdata() to parse dtb information and save it
into platdata instead of saving in global data.

>
> > +{
> > +       struct l2cache *regs;
> > +       u32 ctl_val, prefetch;
> > +       u32 tram_ctl[2];
> > +       u32 dram_ctl[2];
> > +
> > +       regs = (struct l2cache *)dev_read_addr(dev);
> > +       gd->arch.v5l2 = regs;
> > +       ctl_val = readl(&regs->control);
> > +
> > +       if (!(ctl_val & L2_ENABLE))
> > +               ctl_val |= L2_ENABLE;
> > +
> > +       /* Instruction and data fetch prefetch depth */
> > +       if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> > +               ctl_val &= ~(IPREPETCH_MSK);
> > +               ctl_val |= (prefetch << IPREPETCH_OFF);
> > +       }
> > +
> > +       if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> > +               ctl_val &= ~(DPREPETCH_MSK);
> > +               ctl_val |= (prefetch << DPREPETCH_OFF);
> > +       }
> > +
> > +       /* Set tag RAM and data RAM setup and output cycle */
> > +       if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> > +               ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> > +               ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> > +               ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> > +       }
> > +
> > +       if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> > +               ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> > +               ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> > +               ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> > +       }
> > +
> > +       writel(ctl_val, &regs->control);
> > +}
> > +
> > +static int v5l2_probe(struct udevice *dev)
> > +{
> > +       v5l2_of_parse_and_init(dev);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id v5l2_cache_ids[] = {
> > +       { .compatible = "cache" },
>
> I suspect this compatible string is too generic. Has this been
> reviewed by DT community upstream?
>

It refer to many dts examples from arm,
For example :
arch/arm/dts/fsl-imx8-ca35.dtsi
     A35_L2: l2-cache0 {
       compatible = "cache";
     };


Thanks
Rick

> > +       {}
> > +};
> > +
> > +U_BOOT_DRIVER(v5l2_cache) = {
> > +       .name   = "v5l2_cache",
> > +       .id     = UCLASS_CACHE,
> > +       .of_match = v5l2_cache_ids,
> > +       .probe  = v5l2_probe,
> > +       .flags  = DM_FLAG_PRE_RELOC,
> > +};
> > --
>
> Regards,
> Bin

[U-Boot] [PATCH 2/6] riscv: ae350: use the v5l2 driver to configure the cache

[Rick Chen]

Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > Find the UCLASS_CACHE driver to configure the cache controller's
> > settings.
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  board/AndesTech/ax25-ae350/ax25-ae350.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c
> > index 3d65ce7..686ec4a 100644
> > --- a/board/AndesTech/ax25-ae350/ax25-ae350.c
> > +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/io.h>
> >  #include <faraday/ftsmc020.h>
> >  #include <fdtdec.h>
> > +#include <dm.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -93,10 +94,24 @@ int smc_init(void)
> >         return 0;
> >  }
> >
> > +int v5l2_init(void)
> > +{
> > +       struct udevice *dev;
> > +       int ret;
> > +
> > +       ret = uclass_get_device(UCLASS_CACHE, 0, &dev);
> > +
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> >  #ifdef CONFIG_BOARD_EARLY_INIT_F
> >  int board_early_init_f(void)
> >  {
> >         smc_init();
> > +       v5l2_init();
>
> Please check the return value here, or you can make v512_init() returns void.
>
> >
> >         return 0;
> >  }
> > --
>
> Regards,
> Bin

[U-Boot] [PATCH 2/6] riscv: ae350: use the v5l2 driver to configure the cache

[Rick Chen]

Hi Bin

Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > Find the UCLASS_CACHE driver to configure the cache controller's
> > settings.
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  board/AndesTech/ax25-ae350/ax25-ae350.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/board/AndesTech/ax25-ae350/ax25-ae350.c b/board/AndesTech/ax25-ae350/ax25-ae350.c
> > index 3d65ce7..686ec4a 100644
> > --- a/board/AndesTech/ax25-ae350/ax25-ae350.c
> > +++ b/board/AndesTech/ax25-ae350/ax25-ae350.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/io.h>
> >  #include <faraday/ftsmc020.h>
> >  #include <fdtdec.h>
> > +#include <dm.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -93,10 +94,24 @@ int smc_init(void)
> >         return 0;
> >  }
> >
> > +int v5l2_init(void)
> > +{
> > +       struct udevice *dev;
> > +       int ret;
> > +
> > +       ret = uclass_get_device(UCLASS_CACHE, 0, &dev);
> > +
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> >  #ifdef CONFIG_BOARD_EARLY_INIT_F
> >  int board_early_init_f(void)
> >  {
> >         smc_init();
> > +       v5l2_init();
>
> Please check the return value here, or you can make v512_init() returns void.

OK.
I will check the return value here.

>
> >
> >         return 0;
> >  }
> > --
>
> Regards,
> Bin

[U-Boot] [PATCH 4/6] riscv: cache: Flush L2 cache before jump to linux

[Rick Chen]

Hi Bin

>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > Flush and disable cache in cleanup_before_linux()
> > which will be called before jump to linux.
> >
> > The sequence will be preferred as below:
> > L1 flush -> L1 disable -> L2 flush -> L2 disable
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  arch/riscv/cpu/ax25/cpu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/ax25/cpu.c b/arch/riscv/cpu/ax25/cpu.c
> > index 76689b2..9e7579a 100644
> > --- a/arch/riscv/cpu/ax25/cpu.c
> > +++ b/arch/riscv/cpu/ax25/cpu.c
> > @@ -7,6 +7,7 @@
> >  /* CPU specific code */
> >  #include <common.h>
> >  #include <asm/cache.h>
> > +#include <asm/v5l2cache.h>
> >
> >  /*
> >   * cleanup_before_linux() is called just before we call linux
> > @@ -22,6 +23,9 @@ int cleanup_before_linux(void)
> >         cache_flush();
> >         icache_disable();
> >         dcache_disable();
> > +#ifdef CONFIG_RISCV_NDS_CACHE
> > +       v5l2_disable();
> > +#endif
>
> The direct call into a driver should be avoided. Instead, use a proper
> DM cache uclass driver API (see my review comments in patch [1/6])

OK
I will use DM cache uclass driver API to disable L2 driver.

Thanks
Rick

>
> Regards,
> Bin

[U-Boot] [PATCH 3/6] riscv: ae350: add imply v5l2 cache controller

[Rick Chen]

Hi Bin

>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > Select the v5l2 UCLASS_CACHE driver for AE350.
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  board/AndesTech/ax25-ae350/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/board/AndesTech/ax25-ae350/Kconfig b/board/AndesTech/ax25-ae350/Kconfig
> > index 5e682b6..dd299d9 100644
> > --- a/board/AndesTech/ax25-ae350/Kconfig
> > +++ b/board/AndesTech/ax25-ae350/Kconfig
> > @@ -25,5 +25,6 @@ config BOARD_SPECIFIC_OPTIONS # dummy
> >         def_bool y
> >         select RISCV_NDS
> >         imply SMP
> > +       imply V5L2_CACHE
>
> I believe L2 cache is a CPU specific feature, hence this should be
> implied from arch/riscv/cpu/ax25/Kconfig

OK
I will move it to arch/riscv/cpu/ax25/Kconfig

Thanks
Rick

>
> Regards,
> Bin

[U-Boot] [PATCH 5/6] riscv: dts: move out AE350 L2 node from cpus node

[Rick Chen]

Hi Bin

Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > When L2 node exists inside cpus node, uclass_get_device
> > can not parse L2 node successfully. So move it outside
> > from cpus node.
> >
> > Also add tag-ram-ctl and data-ram-ctl attributes for
> > v5l2 cache controller driver. This can adjust timing
> > by requirement from dtb to improve performance.
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  arch/riscv/dts/ae350_32.dts | 17 +++++++++++------
> >  arch/riscv/dts/ae350_64.dts | 17 +++++++++++------
> >  2 files changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/riscv/dts/ae350_32.dts b/arch/riscv/dts/ae350_32.dts
> > index cb6ee13..83abfcb 100644
> > --- a/arch/riscv/dts/ae350_32.dts
> > +++ b/arch/riscv/dts/ae350_32.dts
> > @@ -62,13 +62,18 @@
> >                                 compatible = "riscv,cpu-intc";
> >                         };
> >                 };
> > +       };
> >
> > -               L2: l2-cache at e0500000 {
> > -                       compatible = "cache";
> > -                       cache-level = <2>;
> > -                       cache-size = <0x40000>;
> > -                       reg = <0x0 0xe0500000 0x0 0x40000>;
> > -               };
> > +       L2: l2-cache at e0500000 {
> > +               compatible = "cache";
>
> too generic compatible string (see my previous comments in patch [1/6])

Same replying  in patch [1/6]
>
> > +               cache-level = <2>;
> > +               cache-size = <0x40000>;
> > +               reg = <0xe0500000 0x40000>;
> > +               andes,inst-prefetch = <3>;
> > +               andes,data-prefetch = <3>;
> > +               // The value format is <XRAMOCTL XRAMICTL>
>
> nits: no //, use /* */

OK
I will use /* */ instead of //

>
> > +               andes,tag-ram-ctl = <0 0>;
> > +               andes,data-ram-ctl = <0 0>;
> >         };
> >
> >         memory at 0 {
> > diff --git a/arch/riscv/dts/ae350_64.dts b/arch/riscv/dts/ae350_64.dts
> > index 705491a..7009bdc 100644
> > --- a/arch/riscv/dts/ae350_64.dts
> > +++ b/arch/riscv/dts/ae350_64.dts
> > @@ -62,13 +62,18 @@
> >                                 compatible = "riscv,cpu-intc";
> >                         };
> >                 };
> > +       };
> >
> > -               L2: l2-cache at e0500000 {
> > -                       compatible = "cache";
> > -                       cache-level = <2>;
> > -                       cache-size = <0x40000>;
> > -                       reg = <0x0 0xe0500000 0x0 0x40000>;
> > -               };
> > +       L2: l2-cache at e0500000 {
> > +               compatible = "cache";
> > +               cache-level = <2>;
> > +               cache-size = <0x40000>;
> > +               reg = <0x0 0xe0500000 0x0 0x40000>;
> > +               andes,inst-prefetch = <3>;
> > +               andes,data-prefetch = <3>;
> > +               // The value format is <XRAMOCTL XRAMICTL>
>
> nits: no //, use /* */

I will use /* */ instead of //

Thanks
Rick

>
> > +               andes,tag-ram-ctl = <0 0>;
> > +               andes,data-ram-ctl = <0 0>;
> >         };
> >
> >         memory at 0 {
> > --
>
> Regards,
> Bin

[U-Boot] [PATCH 6/6] riscv: ax25: use CCTL to flush d-cache

[Rick Chen]

Hi Bin

>
> Hi Rick,
>
> On Tue, May 28, 2019 at 5:45 PM Andes <uboot@andestech.com> wrote:
> >
> > From: Rick Chen <rick@andestech.com>
> >
> > Use CCTL command to do d-cache write back and invalidate
> > instead of fence.
> >
> > Signed-off-by: Rick Chen <rick@andestech.com>
> > Cc: Greentime Hu <greentime@andestech.com>
> > ---
> >  arch/riscv/cpu/ax25/cache.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/cpu/ax25/cache.c b/arch/riscv/cpu/ax25/cache.c
> > index 228fc55..d30071e 100644
> > --- a/arch/riscv/cpu/ax25/cache.c
> > +++ b/arch/riscv/cpu/ax25/cache.c
> > @@ -5,17 +5,21 @@
> >   */
> >
> >  #include <common.h>
> > +#include <asm/csr.h>
> > +
> > +#ifdef CONFIG_RISCV_NDS_CACHE
> > +/* mcctlcommand */
> > +#define CCTL_REG_MCCTLCOMMAND_NUM      0x7cc
> > +
> > +/* D-cache operation */
> > +#define CCTL_L1D_WBINVAL_ALL   6
> > +#endif
> >
> >  void flush_dcache_all(void)
> >  {
> > -       /*
> > -        * Andes' AX25 does not have a coherence agent. U-Boot must use data
> > -        * cache flush and invalidate functions to keep data in the system
> > -        * coherent.
> > -        * The implementation of the fence instruction in the AX25 flushes the
> > -        * data cache and is used for this purpose.
> > -        */
> > -       asm volatile ("fence" ::: "memory");
> > +#ifdef CONFIG_RISCV_NDS_CACHE
> > +       csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
>
> I think CCTL_REG_MCCTLCOMMAND_NUM is a vendor specific CSR. Does
> upstream GCC support this CSR?

Yes. It is a vendor specific CSR. Upstream GCC shall not support it.
So I isolate it by CONFIG_RISCV_NDS_CACHE.

Thanks
Rick

>
> > +#endif
> >  }
> >
> >  void flush_dcache_range(unsigned long start, unsigned long end)
> > @@ -72,8 +76,8 @@ void dcache_disable(void)
> >  {
> >  #ifndef CONFIG_SYS_DCACHE_OFF
> >  #ifdef CONFIG_RISCV_NDS_CACHE
> > +       csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
> >         asm volatile (
> > -               "fence\n\t"
> >                 "csrr t1, mcache_ctl\n\t"
> >                 "andi t0, t1, ~0x2\n\t"
> >                 "csrw mcache_ctl, t0\n\t"
> > --
>
> Regards,
> Bin

[U-Boot] [PATCH 6/6] riscv: ax25: use CCTL to flush d-cache

[Bin Meng]

Hi Rick,

On Wed, Jun 5, 2019 at 5:38 PM Rick Chen <rickchen36@gmail.com> wrote:
>
> Hi Bin
>
> >
> > Hi Rick,
> >
> > On Tue, May 28, 2019 at 5:45 PM Andes <uboot@andestech.com> wrote:
> > >
> > > From: Rick Chen <rick@andestech.com>
> > >
> > > Use CCTL command to do d-cache write back and invalidate
> > > instead of fence.
> > >
> > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > Cc: Greentime Hu <greentime@andestech.com>
> > > ---
> > >  arch/riscv/cpu/ax25/cache.c | 22 +++++++++++++---------
> > >  1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/riscv/cpu/ax25/cache.c b/arch/riscv/cpu/ax25/cache.c
> > > index 228fc55..d30071e 100644
> > > --- a/arch/riscv/cpu/ax25/cache.c
> > > +++ b/arch/riscv/cpu/ax25/cache.c
> > > @@ -5,17 +5,21 @@
> > >   */
> > >
> > >  #include <common.h>
> > > +#include <asm/csr.h>
> > > +
> > > +#ifdef CONFIG_RISCV_NDS_CACHE
> > > +/* mcctlcommand */
> > > +#define CCTL_REG_MCCTLCOMMAND_NUM      0x7cc
> > > +
> > > +/* D-cache operation */
> > > +#define CCTL_L1D_WBINVAL_ALL   6
> > > +#endif
> > >
> > >  void flush_dcache_all(void)
> > >  {
> > > -       /*
> > > -        * Andes' AX25 does not have a coherence agent. U-Boot must use data
> > > -        * cache flush and invalidate functions to keep data in the system
> > > -        * coherent.
> > > -        * The implementation of the fence instruction in the AX25 flushes the
> > > -        * data cache and is used for this purpose.
> > > -        */
> > > -       asm volatile ("fence" ::: "memory");
> > > +#ifdef CONFIG_RISCV_NDS_CACHE
> > > +       csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
> >
> > I think CCTL_REG_MCCTLCOMMAND_NUM is a vendor specific CSR. Does
> > upstream GCC support this CSR?
>
> Yes. It is a vendor specific CSR. Upstream GCC shall not support it.
> So I isolate it by CONFIG_RISCV_NDS_CACHE.
>

OK, but I suspect you will need do something for the travis build
since upstream gcc is used?

Regards,
Bin

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Auer, Lukas]

Hi Rick,

On Wed, 2019-06-05 at 16:58 +0800, Rick Chen wrote:
> Hi Bin
> 
> Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
> > Hi Rick,
> > 
> > On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> > > From: Rick Chen <rick@andestech.com>
> > > 
> > > Add a v5l2 cache controller driver that is usually found on
> > > Andes RISC-V ae350 platform. It will parse the cache settings
> > > from the dtb.
> > > 
> > > In this version tag and data ram control timing can be adjusted
> > > by the requirement from the dtb.
> > > 
> > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > Cc: Greentime Hu <greentime@andestech.com>
> > > ---
> > >  arch/riscv/include/asm/global_data.h |   3 ++
> > >  arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
> > >  drivers/cache/Kconfig                |   9 ++++
> > >  drivers/cache/Makefile               |   1 +
> > >  drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
> > >  5 files changed, 176 insertions(+)
> > >  create mode 100644 arch/riscv/include/asm/v5l2cache.h
> > >  create mode 100644 drivers/cache/cache-v5l2.c
> > > 
> > > diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> > > index b74bd7e..6e52d5d 100644
> > > --- a/arch/riscv/include/asm/global_data.h
> > > +++ b/arch/riscv/include/asm/global_data.h
> > > @@ -24,6 +24,9 @@ struct arch_global_data {
> > >  #ifdef CONFIG_ANDES_PLMT
> > >         void __iomem *plmt;     /* plmt base address */
> > >  #endif
> > > +#ifdef CONFIG_V5L2_CACHE
> > > +       void __iomem *v5l2;     /* v5l2 base address */
> > > +#endif
> > 
> > Please do not expose this to global data if it is only used inside a
> > driver. Anything that is here is for "global" usage.
> 
> OK.
> I will remove it.
> 
> > >  #ifdef CONFIG_SMP
> > >         struct ipi_data ipi[CONFIG_NR_CPUS];
> > >  #endif
> > > diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> > > new file mode 100644
> > > index 0000000..8ed1c6c
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/v5l2cache.h
> > 
> > Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
> > of the name, would it be more readable to name it as v5_l2cache.h? Or
> > even add more information to v5, for me, I don't know what v5 stands
> > for :)
> 
> OK
> I will create arch/riscv/include/asm/arch-ax25/v5_l2cache.h
> 
> > > @@ -0,0 +1,61 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * Copyright (C) 2019 Andes Technology Corporation
> > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > + */
> > > +
> > > +#ifndef _ASM_V5_L2CACHE_H
> > > +#define _ASM_V5_L2CACHE_H
> > > +
> > > +struct l2cache {
> > > +       volatile u64    configure;
> > 
> > checkpatch.pl will report warnings against volatile usage. I think we
> > should drop these.
> 
> I know that checkpatch.pl will report this warning.
> But I still need to add volatile. It help to fix some unpredictable problem.
> Without this some driver code flows will be optimized and go wrong somehow.
> 
> > > +       volatile u64    control;
> > > +       volatile u64    hpm0;
> > > +       volatile u64    hpm1;
> > > +       volatile u64    hpm2;
> > > +       volatile u64    hpm3;
> > > +       volatile u64    error_status;
> > > +       volatile u64    ecc_error;
> > > +       volatile u64    cctl_command0;
> > > +       volatile u64    cctl_access_line0;
> > > +       volatile u64    cctl_command1;
> > > +       volatile u64    cctl_access_line1;
> > > +       volatile u64    cctl_command2;
> > > +       volatile u64    cctl_access_line2;
> > > +       volatile u64    cctl_command3;
> > > +       volatile u64    cctl_access_line4;
> > > +       volatile u64    cctl_status;
> > > +};
> > > +
> > > +/* Control Register */
> > > +#define L2_ENABLE      0x1
> > > +/* prefetch */
> > > +#define IPREPETCH_OFF  3
> > > +#define DPREPETCH_OFF  5
> > > +#define IPREPETCH_MSK  (3 << IPREPETCH_OFF)
> > > +#define DPREPETCH_MSK  (3 << DPREPETCH_OFF)
> > > +/* tag ram */
> > > +#define TRAMOCTL_OFF   8
> > > +#define TRAMICTL_OFF   10
> > > +#define TRAMOCTL_MSK   (3 << TRAMOCTL_OFF)
> > > +#define TRAMICTL_MSK   BIT(TRAMICTL_OFF)
> > > +/* data ram */
> > > +#define DRAMOCTL_OFF   11
> > > +#define DRAMICTL_OFF   13
> > > +#define DRAMOCTL_MSK   (3 << DRAMOCTL_OFF)
> > > +#define DRAMICTL_MSK   BIT(DRAMICTL_OFF)
> > > +
> > > +/* CCTL Command Register */
> > > +#define CCTL_CMD_REG(base, hart)       ((ulong)(base) + 0x40 + (hart) * 0x10)
> > > +#define L2_WBINVAL_ALL 0x12
> > > +
> > > +/* CCTL Status Register */
> > > +#define CCTL_STATUS_MSK(hart)          (0xf << ((hart) * 4))
> > > +#define CCTL_STATUS_IDLE(hart)         (0 << ((hart) * 4))
> > > +#define CCTL_STATUS_PROCESS(hart)      (1 << ((hart) * 4))
> > > +#define CCTL_STATUS_ILLEGAL(hart)      (2 << ((hart) * 4))
> > > +
> > > +void v5l2_enable(void);
> > > +void v5l2_disable(void);
> > > +
> > > +#endif /* _ASM_V5_L2CACHE_H */
> > > diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> > > index 24def7a..665689a 100644
> > > --- a/drivers/cache/Kconfig
> > > +++ b/drivers/cache/Kconfig
> > > @@ -22,4 +22,13 @@ config L2X0_CACHE
> > >           ARMv7(32-bit) devices. The driver configures the cache settings
> > >           found in the device tree.
> > > 
> > > +config V5L2_CACHE
> > > +       tristate "Andes V5L2 cache driver"
> > 
> > This should be bool. U-Boot does not support "tristate"
> 
> I will modify it as bool.
> 
> > > +       select CACHE
> > > +       depends on RISCV_NDS_CACHE
> > > +       help
> > > +         Support Andes V5L2 cache controller in AE350 platform.
> > > +         It will configure tag and data ram timing control from the
> > > +         device tree and enable L2 cache.
> > > +
> > >  endmenu
> > > diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> > > index 9deb961..4a6458c 100644
> > > --- a/drivers/cache/Makefile
> > > +++ b/drivers/cache/Makefile
> > > @@ -2,3 +2,4 @@
> > >  obj-$(CONFIG_CACHE) += cache-uclass.o
> > >  obj-$(CONFIG_SANDBOX) += sandbox_cache.o
> > >  obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> > > +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> > > diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> > > new file mode 100644
> > > index 0000000..7022feb
> > > --- /dev/null
> > > +++ b/drivers/cache/cache-v5l2.c
> > > @@ -0,0 +1,102 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2019 Andes Technology Corporation
> > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <command.h>
> > > +#include <dm.h>
> > > +#include <asm/io.h>
> > > +#include <dm/ofnode.h>
> > > +#include <asm/v5l2cache.h>
> > > +
> > > +DECLARE_GLOBAL_DATA_PTR;
> > > +
> > > +void v5l2_enable(void)
> > 
> > This should really be avoided. It looks current DM cache uclass driver
> > is lacking of ops to do cache enable/disable. Please improve the DM
> > cache uclass driver first.
> 
> OK
> I will improve the DM cache uclass driver.
> 
> > > +{
> > > +       struct l2cache *regs = gd->arch.v5l2;
> > > +
> > > +       if (regs)
> > > +               setbits_le32(&regs->control, L2_ENABLE);
> > > +}
> > > +
> > > +void v5l2_disable(void)
> > > +{
> > > +       volatile struct l2cache *regs = gd->arch.v5l2;
> > > +       u8 hart = gd->arch.boot_hart;
> > > +       void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> > > +
> > > +       if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
> > > +               writel(L2_WBINVAL_ALL, cctlcmd);
> > > +
> > > +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> > > +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> > > +                               printf("L2 flush illegal! hanging...");
> > > +                               hang();
> > > +                       }
> > > +               }
> > > +               clrbits_le32(&regs->control, L2_ENABLE);
> > > +       }
> > > +}
> > > +
> > > +static void v5l2_of_parse_and_init(struct udevice *dev)
> > 
> > The entire function below should really be created as the driver's
> > ofdata_to_platdata() function, inside which all DT properties are
> > parsed and saved to driver's platdata. After that, there is no need to
> > get register base from global data.
> 
> I will use ofdata_to_platdata() to parse dtb information and save it
> into platdata instead of saving in global data.
> 
> > > +{
> > > +       struct l2cache *regs;
> > > +       u32 ctl_val, prefetch;
> > > +       u32 tram_ctl[2];
> > > +       u32 dram_ctl[2];
> > > +
> > > +       regs = (struct l2cache *)dev_read_addr(dev);
> > > +       gd->arch.v5l2 = regs;
> > > +       ctl_val = readl(&regs->control);
> > > +
> > > +       if (!(ctl_val & L2_ENABLE))
> > > +               ctl_val |= L2_ENABLE;
> > > +
> > > +       /* Instruction and data fetch prefetch depth */
> > > +       if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> > > +               ctl_val &= ~(IPREPETCH_MSK);
> > > +               ctl_val |= (prefetch << IPREPETCH_OFF);
> > > +       }
> > > +
> > > +       if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> > > +               ctl_val &= ~(DPREPETCH_MSK);
> > > +               ctl_val |= (prefetch << DPREPETCH_OFF);
> > > +       }
> > > +
> > > +       /* Set tag RAM and data RAM setup and output cycle */
> > > +       if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> > > +               ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> > > +               ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> > > +               ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> > > +       }
> > > +
> > > +       if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> > > +               ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> > > +               ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> > > +               ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> > > +       }
> > > +
> > > +       writel(ctl_val, &regs->control);
> > > +}
> > > +
> > > +static int v5l2_probe(struct udevice *dev)
> > > +{
> > > +       v5l2_of_parse_and_init(dev);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct udevice_id v5l2_cache_ids[] = {
> > > +       { .compatible = "cache" },
> > 
> > I suspect this compatible string is too generic. Has this been
> > reviewed by DT community upstream?
> > 
> 
> It refer to many dts examples from arm,
> For example :
> arch/arm/dts/fsl-imx8-ca35.dtsi
>      A35_L2: l2-cache0 {
>        compatible = "cache";
>      };
> 

None of these have a driver for the cache controller, which is why it
is sufficient to just use a generic compatible string.

I agree with Bin that you should choose a more specific compatible
string. This is likely to cause problems in the future otherwise. For
example, if Andes develops a new L2 cache controller, it must be
differentiated from this one using the compatible string. The new
controller would therefore have to use a different compatible string.
It is good practice to already use a more specific one now to avoid the
headache later. :)

Thanks,
Lukas

[U-Boot] [PATCH 6/6] riscv: ax25: use CCTL to flush d-cache

[Auer, Lukas]

Hi Rick,

On Wed, 2019-06-05 at 17:39 +0800, Bin Meng wrote:
> Hi Rick,
> 
> On Wed, Jun 5, 2019 at 5:38 PM Rick Chen <rickchen36@gmail.com> wrote:
> > Hi Bin
> > 
> > > Hi Rick,
> > > 
> > > On Tue, May 28, 2019 at 5:45 PM Andes <uboot@andestech.com> wrote:
> > > > From: Rick Chen <rick@andestech.com>
> > > > 
> > > > Use CCTL command to do d-cache write back and invalidate
> > > > instead of fence.
> > > > 
> > > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > > Cc: Greentime Hu <greentime@andestech.com>
> > > > ---
> > > >  arch/riscv/cpu/ax25/cache.c | 22 +++++++++++++---------
> > > >  1 file changed, 13 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/arch/riscv/cpu/ax25/cache.c b/arch/riscv/cpu/ax25/cache.c
> > > > index 228fc55..d30071e 100644
> > > > --- a/arch/riscv/cpu/ax25/cache.c
> > > > +++ b/arch/riscv/cpu/ax25/cache.c
> > > > @@ -5,17 +5,21 @@
> > > >   */
> > > > 
> > > >  #include <common.h>
> > > > +#include <asm/csr.h>
> > > > +
> > > > +#ifdef CONFIG_RISCV_NDS_CACHE
> > > > +/* mcctlcommand */
> > > > +#define CCTL_REG_MCCTLCOMMAND_NUM      0x7cc
> > > > +
> > > > +/* D-cache operation */
> > > > +#define CCTL_L1D_WBINVAL_ALL   6
> > > > +#endif
> > > > 
> > > >  void flush_dcache_all(void)
> > > >  {
> > > > -       /*
> > > > -        * Andes' AX25 does not have a coherence agent. U-Boot must use data
> > > > -        * cache flush and invalidate functions to keep data in the system
> > > > -        * coherent.
> > > > -        * The implementation of the fence instruction in the AX25 flushes the
> > > > -        * data cache and is used for this purpose.
> > > > -        */
> > > > -       asm volatile ("fence" ::: "memory");
> > > > +#ifdef CONFIG_RISCV_NDS_CACHE
> > > > +       csr_write(CCTL_REG_MCCTLCOMMAND_NUM, CCTL_L1D_WBINVAL_ALL);
> > > 
> > > I think CCTL_REG_MCCTLCOMMAND_NUM is a vendor specific CSR. Does
> > > upstream GCC support this CSR?
> > 
> > Yes. It is a vendor specific CSR. Upstream GCC shall not support it.
> > So I isolate it by CONFIG_RISCV_NDS_CACHE.
> > 
> 
> OK, but I suspect you will need do something for the travis build
> since upstream gcc is used?
> 

How about using the CSR address instead of the name? This way the board
can be built in Travis.

Thanks,
Lukas

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Rick Chen]

Hi Lukas

>
> Hi Rick,
>
> On Wed, 2019-06-05 at 16:58 +0800, Rick Chen wrote:
> > Hi Bin
> >
> > Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
> > > Hi Rick,
> > >
> > > On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> > > > From: Rick Chen <rick@andestech.com>
> > > >
> > > > Add a v5l2 cache controller driver that is usually found on
> > > > Andes RISC-V ae350 platform. It will parse the cache settings
> > > > from the dtb.
> > > >
> > > > In this version tag and data ram control timing can be adjusted
> > > > by the requirement from the dtb.
> > > >
> > > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > > Cc: Greentime Hu <greentime@andestech.com>
> > > > ---
> > > >  arch/riscv/include/asm/global_data.h |   3 ++
> > > >  arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
> > > >  drivers/cache/Kconfig                |   9 ++++
> > > >  drivers/cache/Makefile               |   1 +
> > > >  drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
> > > >  5 files changed, 176 insertions(+)
> > > >  create mode 100644 arch/riscv/include/asm/v5l2cache.h
> > > >  create mode 100644 drivers/cache/cache-v5l2.c
> > > >
> > > > diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> > > > index b74bd7e..6e52d5d 100644
> > > > --- a/arch/riscv/include/asm/global_data.h
> > > > +++ b/arch/riscv/include/asm/global_data.h
> > > > @@ -24,6 +24,9 @@ struct arch_global_data {
> > > >  #ifdef CONFIG_ANDES_PLMT
> > > >         void __iomem *plmt;     /* plmt base address */
> > > >  #endif
> > > > +#ifdef CONFIG_V5L2_CACHE
> > > > +       void __iomem *v5l2;     /* v5l2 base address */
> > > > +#endif
> > >
> > > Please do not expose this to global data if it is only used inside a
> > > driver. Anything that is here is for "global" usage.
> >
> > OK.
> > I will remove it.
> >
> > > >  #ifdef CONFIG_SMP
> > > >         struct ipi_data ipi[CONFIG_NR_CPUS];
> > > >  #endif
> > > > diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> > > > new file mode 100644
> > > > index 0000000..8ed1c6c
> > > > --- /dev/null
> > > > +++ b/arch/riscv/include/asm/v5l2cache.h
> > >
> > > Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
> > > of the name, would it be more readable to name it as v5_l2cache.h? Or
> > > even add more information to v5, for me, I don't know what v5 stands
> > > for :)
> >
> > OK
> > I will create arch/riscv/include/asm/arch-ax25/v5_l2cache.h
> >
> > > > @@ -0,0 +1,61 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > > +/*
> > > > + * Copyright (C) 2019 Andes Technology Corporation
> > > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > > + */
> > > > +
> > > > +#ifndef _ASM_V5_L2CACHE_H
> > > > +#define _ASM_V5_L2CACHE_H
> > > > +
> > > > +struct l2cache {
> > > > +       volatile u64    configure;
> > >
> > > checkpatch.pl will report warnings against volatile usage. I think we
> > > should drop these.
> >
> > I know that checkpatch.pl will report this warning.
> > But I still need to add volatile. It help to fix some unpredictable problem.
> > Without this some driver code flows will be optimized and go wrong somehow.
> >
> > > > +       volatile u64    control;
> > > > +       volatile u64    hpm0;
> > > > +       volatile u64    hpm1;
> > > > +       volatile u64    hpm2;
> > > > +       volatile u64    hpm3;
> > > > +       volatile u64    error_status;
> > > > +       volatile u64    ecc_error;
> > > > +       volatile u64    cctl_command0;
> > > > +       volatile u64    cctl_access_line0;
> > > > +       volatile u64    cctl_command1;
> > > > +       volatile u64    cctl_access_line1;
> > > > +       volatile u64    cctl_command2;
> > > > +       volatile u64    cctl_access_line2;
> > > > +       volatile u64    cctl_command3;
> > > > +       volatile u64    cctl_access_line4;
> > > > +       volatile u64    cctl_status;
> > > > +};
> > > > +
> > > > +/* Control Register */
> > > > +#define L2_ENABLE      0x1
> > > > +/* prefetch */
> > > > +#define IPREPETCH_OFF  3
> > > > +#define DPREPETCH_OFF  5
> > > > +#define IPREPETCH_MSK  (3 << IPREPETCH_OFF)
> > > > +#define DPREPETCH_MSK  (3 << DPREPETCH_OFF)
> > > > +/* tag ram */
> > > > +#define TRAMOCTL_OFF   8
> > > > +#define TRAMICTL_OFF   10
> > > > +#define TRAMOCTL_MSK   (3 << TRAMOCTL_OFF)
> > > > +#define TRAMICTL_MSK   BIT(TRAMICTL_OFF)
> > > > +/* data ram */
> > > > +#define DRAMOCTL_OFF   11
> > > > +#define DRAMICTL_OFF   13
> > > > +#define DRAMOCTL_MSK   (3 << DRAMOCTL_OFF)
> > > > +#define DRAMICTL_MSK   BIT(DRAMICTL_OFF)
> > > > +
> > > > +/* CCTL Command Register */
> > > > +#define CCTL_CMD_REG(base, hart)       ((ulong)(base) + 0x40 + (hart) * 0x10)
> > > > +#define L2_WBINVAL_ALL 0x12
> > > > +
> > > > +/* CCTL Status Register */
> > > > +#define CCTL_STATUS_MSK(hart)          (0xf << ((hart) * 4))
> > > > +#define CCTL_STATUS_IDLE(hart)         (0 << ((hart) * 4))
> > > > +#define CCTL_STATUS_PROCESS(hart)      (1 << ((hart) * 4))
> > > > +#define CCTL_STATUS_ILLEGAL(hart)      (2 << ((hart) * 4))
> > > > +
> > > > +void v5l2_enable(void);
> > > > +void v5l2_disable(void);
> > > > +
> > > > +#endif /* _ASM_V5_L2CACHE_H */
> > > > diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> > > > index 24def7a..665689a 100644
> > > > --- a/drivers/cache/Kconfig
> > > > +++ b/drivers/cache/Kconfig
> > > > @@ -22,4 +22,13 @@ config L2X0_CACHE
> > > >           ARMv7(32-bit) devices. The driver configures the cache settings
> > > >           found in the device tree.
> > > >
> > > > +config V5L2_CACHE
> > > > +       tristate "Andes V5L2 cache driver"
> > >
> > > This should be bool. U-Boot does not support "tristate"
> >
> > I will modify it as bool.
> >
> > > > +       select CACHE
> > > > +       depends on RISCV_NDS_CACHE
> > > > +       help
> > > > +         Support Andes V5L2 cache controller in AE350 platform.
> > > > +         It will configure tag and data ram timing control from the
> > > > +         device tree and enable L2 cache.
> > > > +
> > > >  endmenu
> > > > diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> > > > index 9deb961..4a6458c 100644
> > > > --- a/drivers/cache/Makefile
> > > > +++ b/drivers/cache/Makefile
> > > > @@ -2,3 +2,4 @@
> > > >  obj-$(CONFIG_CACHE) += cache-uclass.o
> > > >  obj-$(CONFIG_SANDBOX) += sandbox_cache.o
> > > >  obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> > > > +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> > > > diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> > > > new file mode 100644
> > > > index 0000000..7022feb
> > > > --- /dev/null
> > > > +++ b/drivers/cache/cache-v5l2.c
> > > > @@ -0,0 +1,102 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2019 Andes Technology Corporation
> > > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <command.h>
> > > > +#include <dm.h>
> > > > +#include <asm/io.h>
> > > > +#include <dm/ofnode.h>
> > > > +#include <asm/v5l2cache.h>
> > > > +
> > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > +
> > > > +void v5l2_enable(void)
> > >
> > > This should really be avoided. It looks current DM cache uclass driver
> > > is lacking of ops to do cache enable/disable. Please improve the DM
> > > cache uclass driver first.
> >
> > OK
> > I will improve the DM cache uclass driver.
> >
> > > > +{
> > > > +       struct l2cache *regs = gd->arch.v5l2;
> > > > +
> > > > +       if (regs)
> > > > +               setbits_le32(&regs->control, L2_ENABLE);
> > > > +}
> > > > +
> > > > +void v5l2_disable(void)
> > > > +{
> > > > +       volatile struct l2cache *regs = gd->arch.v5l2;
> > > > +       u8 hart = gd->arch.boot_hart;
> > > > +       void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> > > > +
> > > > +       if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
> > > > +               writel(L2_WBINVAL_ALL, cctlcmd);
> > > > +
> > > > +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> > > > +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> > > > +                               printf("L2 flush illegal! hanging...");
> > > > +                               hang();
> > > > +                       }
> > > > +               }
> > > > +               clrbits_le32(&regs->control, L2_ENABLE);
> > > > +       }
> > > > +}
> > > > +
> > > > +static void v5l2_of_parse_and_init(struct udevice *dev)
> > >
> > > The entire function below should really be created as the driver's
> > > ofdata_to_platdata() function, inside which all DT properties are
> > > parsed and saved to driver's platdata. After that, there is no need to
> > > get register base from global data.
> >
> > I will use ofdata_to_platdata() to parse dtb information and save it
> > into platdata instead of saving in global data.
> >
> > > > +{
> > > > +       struct l2cache *regs;
> > > > +       u32 ctl_val, prefetch;
> > > > +       u32 tram_ctl[2];
> > > > +       u32 dram_ctl[2];
> > > > +
> > > > +       regs = (struct l2cache *)dev_read_addr(dev);
> > > > +       gd->arch.v5l2 = regs;
> > > > +       ctl_val = readl(&regs->control);
> > > > +
> > > > +       if (!(ctl_val & L2_ENABLE))
> > > > +               ctl_val |= L2_ENABLE;
> > > > +
> > > > +       /* Instruction and data fetch prefetch depth */
> > > > +       if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> > > > +               ctl_val &= ~(IPREPETCH_MSK);
> > > > +               ctl_val |= (prefetch << IPREPETCH_OFF);
> > > > +       }
> > > > +
> > > > +       if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> > > > +               ctl_val &= ~(DPREPETCH_MSK);
> > > > +               ctl_val |= (prefetch << DPREPETCH_OFF);
> > > > +       }
> > > > +
> > > > +       /* Set tag RAM and data RAM setup and output cycle */
> > > > +       if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> > > > +               ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> > > > +               ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> > > > +               ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> > > > +       }
> > > > +
> > > > +       if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> > > > +               ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> > > > +               ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> > > > +               ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> > > > +       }
> > > > +
> > > > +       writel(ctl_val, &regs->control);
> > > > +}
> > > > +
> > > > +static int v5l2_probe(struct udevice *dev)
> > > > +{
> > > > +       v5l2_of_parse_and_init(dev);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static const struct udevice_id v5l2_cache_ids[] = {
> > > > +       { .compatible = "cache" },
> > >
> > > I suspect this compatible string is too generic. Has this been
> > > reviewed by DT community upstream?
> > >
> >
> > It refer to many dts examples from arm,
> > For example :
> > arch/arm/dts/fsl-imx8-ca35.dtsi
> >      A35_L2: l2-cache0 {
> >        compatible = "cache";
> >      };
> >
>
> None of these have a driver for the cache controller, which is why it
> is sufficient to just use a generic compatible string.
>
> I agree with Bin that you should choose a more specific compatible
> string. This is likely to cause problems in the future otherwise. For
> example, if Andes develops a new L2 cache controller, it must be
> differentiated from this one using the compatible string. The new
> controller would therefore have to use a different compatible string.
> It is good practice to already use a more specific one now to avoid the
> headache later. :)

You are right.
I will modify it as compatible = "v5l2cache"

Thanks
Rick

>
> Thanks,
> Lukas

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Bin Meng]

Hi Rick,

On Mon, Jun 10, 2019 at 10:26 AM Rick Chen <rickchen36@gmail.com> wrote:
>
> Hi Lukas
>
> >
> > Hi Rick,
> >
> > On Wed, 2019-06-05 at 16:58 +0800, Rick Chen wrote:
> > > Hi Bin
> > >
> > > Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
> > > > Hi Rick,
> > > >
> > > > On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> > > > > From: Rick Chen <rick@andestech.com>
> > > > >
> > > > > Add a v5l2 cache controller driver that is usually found on
> > > > > Andes RISC-V ae350 platform. It will parse the cache settings
> > > > > from the dtb.
> > > > >
> > > > > In this version tag and data ram control timing can be adjusted
> > > > > by the requirement from the dtb.
> > > > >
> > > > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > > > Cc: Greentime Hu <greentime@andestech.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/global_data.h |   3 ++
> > > > >  arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
> > > > >  drivers/cache/Kconfig                |   9 ++++
> > > > >  drivers/cache/Makefile               |   1 +
> > > > >  drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
> > > > >  5 files changed, 176 insertions(+)
> > > > >  create mode 100644 arch/riscv/include/asm/v5l2cache.h
> > > > >  create mode 100644 drivers/cache/cache-v5l2.c
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> > > > > index b74bd7e..6e52d5d 100644
> > > > > --- a/arch/riscv/include/asm/global_data.h
> > > > > +++ b/arch/riscv/include/asm/global_data.h
> > > > > @@ -24,6 +24,9 @@ struct arch_global_data {
> > > > >  #ifdef CONFIG_ANDES_PLMT
> > > > >         void __iomem *plmt;     /* plmt base address */
> > > > >  #endif
> > > > > +#ifdef CONFIG_V5L2_CACHE
> > > > > +       void __iomem *v5l2;     /* v5l2 base address */
> > > > > +#endif
> > > >
> > > > Please do not expose this to global data if it is only used inside a
> > > > driver. Anything that is here is for "global" usage.
> > >
> > > OK.
> > > I will remove it.
> > >
> > > > >  #ifdef CONFIG_SMP
> > > > >         struct ipi_data ipi[CONFIG_NR_CPUS];
> > > > >  #endif
> > > > > diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> > > > > new file mode 100644
> > > > > index 0000000..8ed1c6c
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/include/asm/v5l2cache.h
> > > >
> > > > Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
> > > > of the name, would it be more readable to name it as v5_l2cache.h? Or
> > > > even add more information to v5, for me, I don't know what v5 stands
> > > > for :)
> > >
> > > OK
> > > I will create arch/riscv/include/asm/arch-ax25/v5_l2cache.h
> > >
> > > > > @@ -0,0 +1,61 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > > > +/*
> > > > > + * Copyright (C) 2019 Andes Technology Corporation
> > > > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > > > + */
> > > > > +
> > > > > +#ifndef _ASM_V5_L2CACHE_H
> > > > > +#define _ASM_V5_L2CACHE_H
> > > > > +
> > > > > +struct l2cache {
> > > > > +       volatile u64    configure;
> > > >
> > > > checkpatch.pl will report warnings against volatile usage. I think we
> > > > should drop these.
> > >
> > > I know that checkpatch.pl will report this warning.
> > > But I still need to add volatile. It help to fix some unpredictable problem.
> > > Without this some driver code flows will be optimized and go wrong somehow.
> > >
> > > > > +       volatile u64    control;
> > > > > +       volatile u64    hpm0;
> > > > > +       volatile u64    hpm1;
> > > > > +       volatile u64    hpm2;
> > > > > +       volatile u64    hpm3;
> > > > > +       volatile u64    error_status;
> > > > > +       volatile u64    ecc_error;
> > > > > +       volatile u64    cctl_command0;
> > > > > +       volatile u64    cctl_access_line0;
> > > > > +       volatile u64    cctl_command1;
> > > > > +       volatile u64    cctl_access_line1;
> > > > > +       volatile u64    cctl_command2;
> > > > > +       volatile u64    cctl_access_line2;
> > > > > +       volatile u64    cctl_command3;
> > > > > +       volatile u64    cctl_access_line4;
> > > > > +       volatile u64    cctl_status;
> > > > > +};
> > > > > +
> > > > > +/* Control Register */
> > > > > +#define L2_ENABLE      0x1
> > > > > +/* prefetch */
> > > > > +#define IPREPETCH_OFF  3
> > > > > +#define DPREPETCH_OFF  5
> > > > > +#define IPREPETCH_MSK  (3 << IPREPETCH_OFF)
> > > > > +#define DPREPETCH_MSK  (3 << DPREPETCH_OFF)
> > > > > +/* tag ram */
> > > > > +#define TRAMOCTL_OFF   8
> > > > > +#define TRAMICTL_OFF   10
> > > > > +#define TRAMOCTL_MSK   (3 << TRAMOCTL_OFF)
> > > > > +#define TRAMICTL_MSK   BIT(TRAMICTL_OFF)
> > > > > +/* data ram */
> > > > > +#define DRAMOCTL_OFF   11
> > > > > +#define DRAMICTL_OFF   13
> > > > > +#define DRAMOCTL_MSK   (3 << DRAMOCTL_OFF)
> > > > > +#define DRAMICTL_MSK   BIT(DRAMICTL_OFF)
> > > > > +
> > > > > +/* CCTL Command Register */
> > > > > +#define CCTL_CMD_REG(base, hart)       ((ulong)(base) + 0x40 + (hart) * 0x10)
> > > > > +#define L2_WBINVAL_ALL 0x12
> > > > > +
> > > > > +/* CCTL Status Register */
> > > > > +#define CCTL_STATUS_MSK(hart)          (0xf << ((hart) * 4))
> > > > > +#define CCTL_STATUS_IDLE(hart)         (0 << ((hart) * 4))
> > > > > +#define CCTL_STATUS_PROCESS(hart)      (1 << ((hart) * 4))
> > > > > +#define CCTL_STATUS_ILLEGAL(hart)      (2 << ((hart) * 4))
> > > > > +
> > > > > +void v5l2_enable(void);
> > > > > +void v5l2_disable(void);
> > > > > +
> > > > > +#endif /* _ASM_V5_L2CACHE_H */
> > > > > diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> > > > > index 24def7a..665689a 100644
> > > > > --- a/drivers/cache/Kconfig
> > > > > +++ b/drivers/cache/Kconfig
> > > > > @@ -22,4 +22,13 @@ config L2X0_CACHE
> > > > >           ARMv7(32-bit) devices. The driver configures the cache settings
> > > > >           found in the device tree.
> > > > >
> > > > > +config V5L2_CACHE
> > > > > +       tristate "Andes V5L2 cache driver"
> > > >
> > > > This should be bool. U-Boot does not support "tristate"
> > >
> > > I will modify it as bool.
> > >
> > > > > +       select CACHE
> > > > > +       depends on RISCV_NDS_CACHE
> > > > > +       help
> > > > > +         Support Andes V5L2 cache controller in AE350 platform.
> > > > > +         It will configure tag and data ram timing control from the
> > > > > +         device tree and enable L2 cache.
> > > > > +
> > > > >  endmenu
> > > > > diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> > > > > index 9deb961..4a6458c 100644
> > > > > --- a/drivers/cache/Makefile
> > > > > +++ b/drivers/cache/Makefile
> > > > > @@ -2,3 +2,4 @@
> > > > >  obj-$(CONFIG_CACHE) += cache-uclass.o
> > > > >  obj-$(CONFIG_SANDBOX) += sandbox_cache.o
> > > > >  obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> > > > > +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> > > > > diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> > > > > new file mode 100644
> > > > > index 0000000..7022feb
> > > > > --- /dev/null
> > > > > +++ b/drivers/cache/cache-v5l2.c
> > > > > @@ -0,0 +1,102 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2019 Andes Technology Corporation
> > > > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <command.h>
> > > > > +#include <dm.h>
> > > > > +#include <asm/io.h>
> > > > > +#include <dm/ofnode.h>
> > > > > +#include <asm/v5l2cache.h>
> > > > > +
> > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > +
> > > > > +void v5l2_enable(void)
> > > >
> > > > This should really be avoided. It looks current DM cache uclass driver
> > > > is lacking of ops to do cache enable/disable. Please improve the DM
> > > > cache uclass driver first.
> > >
> > > OK
> > > I will improve the DM cache uclass driver.
> > >
> > > > > +{
> > > > > +       struct l2cache *regs = gd->arch.v5l2;
> > > > > +
> > > > > +       if (regs)
> > > > > +               setbits_le32(&regs->control, L2_ENABLE);
> > > > > +}
> > > > > +
> > > > > +void v5l2_disable(void)
> > > > > +{
> > > > > +       volatile struct l2cache *regs = gd->arch.v5l2;
> > > > > +       u8 hart = gd->arch.boot_hart;
> > > > > +       void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> > > > > +
> > > > > +       if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
> > > > > +               writel(L2_WBINVAL_ALL, cctlcmd);
> > > > > +
> > > > > +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> > > > > +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> > > > > +                               printf("L2 flush illegal! hanging...");
> > > > > +                               hang();
> > > > > +                       }
> > > > > +               }
> > > > > +               clrbits_le32(&regs->control, L2_ENABLE);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > +static void v5l2_of_parse_and_init(struct udevice *dev)
> > > >
> > > > The entire function below should really be created as the driver's
> > > > ofdata_to_platdata() function, inside which all DT properties are
> > > > parsed and saved to driver's platdata. After that, there is no need to
> > > > get register base from global data.
> > >
> > > I will use ofdata_to_platdata() to parse dtb information and save it
> > > into platdata instead of saving in global data.
> > >
> > > > > +{
> > > > > +       struct l2cache *regs;
> > > > > +       u32 ctl_val, prefetch;
> > > > > +       u32 tram_ctl[2];
> > > > > +       u32 dram_ctl[2];
> > > > > +
> > > > > +       regs = (struct l2cache *)dev_read_addr(dev);
> > > > > +       gd->arch.v5l2 = regs;
> > > > > +       ctl_val = readl(&regs->control);
> > > > > +
> > > > > +       if (!(ctl_val & L2_ENABLE))
> > > > > +               ctl_val |= L2_ENABLE;
> > > > > +
> > > > > +       /* Instruction and data fetch prefetch depth */
> > > > > +       if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> > > > > +               ctl_val &= ~(IPREPETCH_MSK);
> > > > > +               ctl_val |= (prefetch << IPREPETCH_OFF);
> > > > > +       }
> > > > > +
> > > > > +       if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> > > > > +               ctl_val &= ~(DPREPETCH_MSK);
> > > > > +               ctl_val |= (prefetch << DPREPETCH_OFF);
> > > > > +       }
> > > > > +
> > > > > +       /* Set tag RAM and data RAM setup and output cycle */
> > > > > +       if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> > > > > +               ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> > > > > +               ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> > > > > +               ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> > > > > +       }
> > > > > +
> > > > > +       if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> > > > > +               ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> > > > > +               ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> > > > > +               ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> > > > > +       }
> > > > > +
> > > > > +       writel(ctl_val, &regs->control);
> > > > > +}
> > > > > +
> > > > > +static int v5l2_probe(struct udevice *dev)
> > > > > +{
> > > > > +       v5l2_of_parse_and_init(dev);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct udevice_id v5l2_cache_ids[] = {
> > > > > +       { .compatible = "cache" },
> > > >
> > > > I suspect this compatible string is too generic. Has this been
> > > > reviewed by DT community upstream?
> > > >
> > >
> > > It refer to many dts examples from arm,
> > > For example :
> > > arch/arm/dts/fsl-imx8-ca35.dtsi
> > >      A35_L2: l2-cache0 {
> > >        compatible = "cache";
> > >      };
> > >
> >
> > None of these have a driver for the cache controller, which is why it
> > is sufficient to just use a generic compatible string.
> >
> > I agree with Bin that you should choose a more specific compatible
> > string. This is likely to cause problems in the future otherwise. For
> > example, if Andes develops a new L2 cache controller, it must be
> > differentiated from this one using the compatible string. The new
> > controller would therefore have to use a different compatible string.
> > It is good practice to already use a more specific one now to avoid the
> > headache later. :)
>
> You are right.
> I will modify it as compatible = "v5l2cache"

Before you do that, could you please check whether this L2 cache
driver will be supported in the Linux kernel too? If yes, I think you
need go through the kernel upstream process, during which the
compatible string is to be discussed and approved.

Regards,
Bin

[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

[Rick Chen]

Hi Bin

>
> Hi Rick,
>
> On Mon, Jun 10, 2019 at 10:26 AM Rick Chen <rickchen36@gmail.com> wrote:
> >
> > Hi Lukas
> >
> > >
> > > Hi Rick,
> > >
> > > On Wed, 2019-06-05 at 16:58 +0800, Rick Chen wrote:
> > > > Hi Bin
> > > >
> > > > Bin Meng <bmeng.cn@gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
> > > > > Hi Rick,
> > > > >
> > > > > On Tue, May 28, 2019 at 5:44 PM Andes <uboot@andestech.com> wrote:
> > > > > > From: Rick Chen <rick@andestech.com>
> > > > > >
> > > > > > Add a v5l2 cache controller driver that is usually found on
> > > > > > Andes RISC-V ae350 platform. It will parse the cache settings
> > > > > > from the dtb.
> > > > > >
> > > > > > In this version tag and data ram control timing can be adjusted
> > > > > > by the requirement from the dtb.
> > > > > >
> > > > > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > > > > Cc: Greentime Hu <greentime@andestech.com>
> > > > > > ---
> > > > > >  arch/riscv/include/asm/global_data.h |   3 ++
> > > > > >  arch/riscv/include/asm/v5l2cache.h   |  61 +++++++++++++++++++++
> > > > > >  drivers/cache/Kconfig                |   9 ++++
> > > > > >  drivers/cache/Makefile               |   1 +
> > > > > >  drivers/cache/cache-v5l2.c           | 102 +++++++++++++++++++++++++++++++++++
> > > > > >  5 files changed, 176 insertions(+)
> > > > > >  create mode 100644 arch/riscv/include/asm/v5l2cache.h
> > > > > >  create mode 100644 drivers/cache/cache-v5l2.c
> > > > > >
> > > > > > diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> > > > > > index b74bd7e..6e52d5d 100644
> > > > > > --- a/arch/riscv/include/asm/global_data.h
> > > > > > +++ b/arch/riscv/include/asm/global_data.h
> > > > > > @@ -24,6 +24,9 @@ struct arch_global_data {
> > > > > >  #ifdef CONFIG_ANDES_PLMT
> > > > > >         void __iomem *plmt;     /* plmt base address */
> > > > > >  #endif
> > > > > > +#ifdef CONFIG_V5L2_CACHE
> > > > > > +       void __iomem *v5l2;     /* v5l2 base address */
> > > > > > +#endif
> > > > >
> > > > > Please do not expose this to global data if it is only used inside a
> > > > > driver. Anything that is here is for "global" usage.
> > > >
> > > > OK.
> > > > I will remove it.
> > > >
> > > > > >  #ifdef CONFIG_SMP
> > > > > >         struct ipi_data ipi[CONFIG_NR_CPUS];
> > > > > >  #endif
> > > > > > diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> > > > > > new file mode 100644
> > > > > > index 0000000..8ed1c6c
> > > > > > --- /dev/null
> > > > > > +++ b/arch/riscv/include/asm/v5l2cache.h
> > > > >
> > > > > Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
> > > > > of the name, would it be more readable to name it as v5_l2cache.h? Or
> > > > > even add more information to v5, for me, I don't know what v5 stands
> > > > > for :)
> > > >
> > > > OK
> > > > I will create arch/riscv/include/asm/arch-ax25/v5_l2cache.h
> > > >
> > > > > > @@ -0,0 +1,61 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > > > > +/*
> > > > > > + * Copyright (C) 2019 Andes Technology Corporation
> > > > > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > > > > + */
> > > > > > +
> > > > > > +#ifndef _ASM_V5_L2CACHE_H
> > > > > > +#define _ASM_V5_L2CACHE_H
> > > > > > +
> > > > > > +struct l2cache {
> > > > > > +       volatile u64    configure;
> > > > >
> > > > > checkpatch.pl will report warnings against volatile usage. I think we
> > > > > should drop these.
> > > >
> > > > I know that checkpatch.pl will report this warning.
> > > > But I still need to add volatile. It help to fix some unpredictable problem.
> > > > Without this some driver code flows will be optimized and go wrong somehow.
> > > >
> > > > > > +       volatile u64    control;
> > > > > > +       volatile u64    hpm0;
> > > > > > +       volatile u64    hpm1;
> > > > > > +       volatile u64    hpm2;
> > > > > > +       volatile u64    hpm3;
> > > > > > +       volatile u64    error_status;
> > > > > > +       volatile u64    ecc_error;
> > > > > > +       volatile u64    cctl_command0;
> > > > > > +       volatile u64    cctl_access_line0;
> > > > > > +       volatile u64    cctl_command1;
> > > > > > +       volatile u64    cctl_access_line1;
> > > > > > +       volatile u64    cctl_command2;
> > > > > > +       volatile u64    cctl_access_line2;
> > > > > > +       volatile u64    cctl_command3;
> > > > > > +       volatile u64    cctl_access_line4;
> > > > > > +       volatile u64    cctl_status;
> > > > > > +};
> > > > > > +
> > > > > > +/* Control Register */
> > > > > > +#define L2_ENABLE      0x1
> > > > > > +/* prefetch */
> > > > > > +#define IPREPETCH_OFF  3
> > > > > > +#define DPREPETCH_OFF  5
> > > > > > +#define IPREPETCH_MSK  (3 << IPREPETCH_OFF)
> > > > > > +#define DPREPETCH_MSK  (3 << DPREPETCH_OFF)
> > > > > > +/* tag ram */
> > > > > > +#define TRAMOCTL_OFF   8
> > > > > > +#define TRAMICTL_OFF   10
> > > > > > +#define TRAMOCTL_MSK   (3 << TRAMOCTL_OFF)
> > > > > > +#define TRAMICTL_MSK   BIT(TRAMICTL_OFF)
> > > > > > +/* data ram */
> > > > > > +#define DRAMOCTL_OFF   11
> > > > > > +#define DRAMICTL_OFF   13
> > > > > > +#define DRAMOCTL_MSK   (3 << DRAMOCTL_OFF)
> > > > > > +#define DRAMICTL_MSK   BIT(DRAMICTL_OFF)
> > > > > > +
> > > > > > +/* CCTL Command Register */
> > > > > > +#define CCTL_CMD_REG(base, hart)       ((ulong)(base) + 0x40 + (hart) * 0x10)
> > > > > > +#define L2_WBINVAL_ALL 0x12
> > > > > > +
> > > > > > +/* CCTL Status Register */
> > > > > > +#define CCTL_STATUS_MSK(hart)          (0xf << ((hart) * 4))
> > > > > > +#define CCTL_STATUS_IDLE(hart)         (0 << ((hart) * 4))
> > > > > > +#define CCTL_STATUS_PROCESS(hart)      (1 << ((hart) * 4))
> > > > > > +#define CCTL_STATUS_ILLEGAL(hart)      (2 << ((hart) * 4))
> > > > > > +
> > > > > > +void v5l2_enable(void);
> > > > > > +void v5l2_disable(void);
> > > > > > +
> > > > > > +#endif /* _ASM_V5_L2CACHE_H */
> > > > > > diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> > > > > > index 24def7a..665689a 100644
> > > > > > --- a/drivers/cache/Kconfig
> > > > > > +++ b/drivers/cache/Kconfig
> > > > > > @@ -22,4 +22,13 @@ config L2X0_CACHE
> > > > > >           ARMv7(32-bit) devices. The driver configures the cache settings
> > > > > >           found in the device tree.
> > > > > >
> > > > > > +config V5L2_CACHE
> > > > > > +       tristate "Andes V5L2 cache driver"
> > > > >
> > > > > This should be bool. U-Boot does not support "tristate"
> > > >
> > > > I will modify it as bool.
> > > >
> > > > > > +       select CACHE
> > > > > > +       depends on RISCV_NDS_CACHE
> > > > > > +       help
> > > > > > +         Support Andes V5L2 cache controller in AE350 platform.
> > > > > > +         It will configure tag and data ram timing control from the
> > > > > > +         device tree and enable L2 cache.
> > > > > > +
> > > > > >  endmenu
> > > > > > diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> > > > > > index 9deb961..4a6458c 100644
> > > > > > --- a/drivers/cache/Makefile
> > > > > > +++ b/drivers/cache/Makefile
> > > > > > @@ -2,3 +2,4 @@
> > > > > >  obj-$(CONFIG_CACHE) += cache-uclass.o
> > > > > >  obj-$(CONFIG_SANDBOX) += sandbox_cache.o
> > > > > >  obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> > > > > > +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> > > > > > diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> > > > > > new file mode 100644
> > > > > > index 0000000..7022feb
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/cache/cache-v5l2.c
> > > > > > @@ -0,0 +1,102 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (C) 2019 Andes Technology Corporation
> > > > > > + * Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > > > > > + */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <command.h>
> > > > > > +#include <dm.h>
> > > > > > +#include <asm/io.h>
> > > > > > +#include <dm/ofnode.h>
> > > > > > +#include <asm/v5l2cache.h>
> > > > > > +
> > > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > > +
> > > > > > +void v5l2_enable(void)
> > > > >
> > > > > This should really be avoided. It looks current DM cache uclass driver
> > > > > is lacking of ops to do cache enable/disable. Please improve the DM
> > > > > cache uclass driver first.
> > > >
> > > > OK
> > > > I will improve the DM cache uclass driver.
> > > >
> > > > > > +{
> > > > > > +       struct l2cache *regs = gd->arch.v5l2;
> > > > > > +
> > > > > > +       if (regs)
> > > > > > +               setbits_le32(&regs->control, L2_ENABLE);
> > > > > > +}
> > > > > > +
> > > > > > +void v5l2_disable(void)
> > > > > > +{
> > > > > > +       volatile struct l2cache *regs = gd->arch.v5l2;
> > > > > > +       u8 hart = gd->arch.boot_hart;
> > > > > > +       void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> > > > > > +
> > > > > > +       if ((regs) && (readl(&regs->control) & L2_ENABLE)) {
> > > > > > +               writel(L2_WBINVAL_ALL, cctlcmd);
> > > > > > +
> > > > > > +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> > > > > > +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> > > > > > +                               printf("L2 flush illegal! hanging...");
> > > > > > +                               hang();
> > > > > > +                       }
> > > > > > +               }
> > > > > > +               clrbits_le32(&regs->control, L2_ENABLE);
> > > > > > +       }
> > > > > > +}
> > > > > > +
> > > > > > +static void v5l2_of_parse_and_init(struct udevice *dev)
> > > > >
> > > > > The entire function below should really be created as the driver's
> > > > > ofdata_to_platdata() function, inside which all DT properties are
> > > > > parsed and saved to driver's platdata. After that, there is no need to
> > > > > get register base from global data.
> > > >
> > > > I will use ofdata_to_platdata() to parse dtb information and save it
> > > > into platdata instead of saving in global data.
> > > >
> > > > > > +{
> > > > > > +       struct l2cache *regs;
> > > > > > +       u32 ctl_val, prefetch;
> > > > > > +       u32 tram_ctl[2];
> > > > > > +       u32 dram_ctl[2];
> > > > > > +
> > > > > > +       regs = (struct l2cache *)dev_read_addr(dev);
> > > > > > +       gd->arch.v5l2 = regs;
> > > > > > +       ctl_val = readl(&regs->control);
> > > > > > +
> > > > > > +       if (!(ctl_val & L2_ENABLE))
> > > > > > +               ctl_val |= L2_ENABLE;
> > > > > > +
> > > > > > +       /* Instruction and data fetch prefetch depth */
> > > > > > +       if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> > > > > > +               ctl_val &= ~(IPREPETCH_MSK);
> > > > > > +               ctl_val |= (prefetch << IPREPETCH_OFF);
> > > > > > +       }
> > > > > > +
> > > > > > +       if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> > > > > > +               ctl_val &= ~(DPREPETCH_MSK);
> > > > > > +               ctl_val |= (prefetch << DPREPETCH_OFF);
> > > > > > +       }
> > > > > > +
> > > > > > +       /* Set tag RAM and data RAM setup and output cycle */
> > > > > > +       if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> > > > > > +               ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> > > > > > +               ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> > > > > > +               ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> > > > > > +       }
> > > > > > +
> > > > > > +       if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> > > > > > +               ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> > > > > > +               ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> > > > > > +               ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> > > > > > +       }
> > > > > > +
> > > > > > +       writel(ctl_val, &regs->control);
> > > > > > +}
> > > > > > +
> > > > > > +static int v5l2_probe(struct udevice *dev)
> > > > > > +{
> > > > > > +       v5l2_of_parse_and_init(dev);
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct udevice_id v5l2_cache_ids[] = {
> > > > > > +       { .compatible = "cache" },
> > > > >
> > > > > I suspect this compatible string is too generic. Has this been
> > > > > reviewed by DT community upstream?
> > > > >
> > > >
> > > > It refer to many dts examples from arm,
> > > > For example :
> > > > arch/arm/dts/fsl-imx8-ca35.dtsi
> > > >      A35_L2: l2-cache0 {
> > > >        compatible = "cache";
> > > >      };
> > > >
> > >
> > > None of these have a driver for the cache controller, which is why it
> > > is sufficient to just use a generic compatible string.
> > >
> > > I agree with Bin that you should choose a more specific compatible
> > > string. This is likely to cause problems in the future otherwise. For
> > > example, if Andes develops a new L2 cache controller, it must be
> > > differentiated from this one using the compatible string. The new
> > > controller would therefore have to use a different compatible string.
> > > It is good practice to already use a more specific one now to avoid the
> > > headache later. :)
> >
> > You are right.
> > I will modify it as compatible = "v5l2cache"
>
> Before you do that, could you please check whether this L2 cache
> driver will be supported in the Linux kernel too? If yes, I think you
> need go through the kernel upstream process, during which the
> compatible string is to be discussed and approved.
>

No, we only support this L2 cache in U-Boot about upstream.

Rick

> Regards,
> Bin