[PATCH v1 0/3] Risc-V cache operations

[PATCH v1 0/3] Risc-V cache operations

[Mayuresh Chitale]

This patchset adds support for using the CBO instructions to perform the
dcache flush/inval operations for the qemu-riscv board when those are
enabled. The CBO instructions are defined in the Risc-V CMO specification
which can be found at the link below:
https://github.com/riscv/riscv-CMOs/blob/master/specifications/cmobase-v1.0.pdf

Mayuresh Chitale (3):
  riscv: Add support for defining instructions
  riscv: cache: Add CBO instructions
  board: qemu-riscv: Override enable_caches

 arch/riscv/Kconfig                      |  4 ++
 arch/riscv/include/asm/cache.h          |  3 +
 arch/riscv/include/asm/insn-def.h       | 42 ++++++++++++
 arch/riscv/lib/cache.c                  | 90 +++++++++++++++++++++++++
 board/emulation/qemu-riscv/qemu-riscv.c | 16 +++++
 5 files changed, 155 insertions(+)
 create mode 100644 arch/riscv/include/asm/insn-def.h

-- 
2.34.1

[PATCH v1 1/3] riscv: Add support for defining instructions

[Mayuresh Chitale]

Add insn-def.h which is similar to that in linux and contains the macros
to generate any instruction of type 'I' using the assembler's .insn
directive.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 arch/riscv/include/asm/insn-def.h | 42 +++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 arch/riscv/include/asm/insn-def.h

diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
new file mode 100644
index 0000000000..99ad5b8f6a
--- /dev/null
+++ b/arch/riscv/include/asm/insn-def.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2024 Ventana Micro Systems Ltd.
+ *
+ * Ported from linux insn-def.h.
+ */
+
+#ifndef _ASM_RISCV_BARRIER_H
+#define _ASM_RISCV_BARRIER_H
+
+#define INSN_I_SIMM12_SHIFT		20
+#define INSN_I_RS1_SHIFT		15
+#define INSN_I_FUNC3_SHIFT		12
+#define INSN_I_RD_SHIFT			 7
+#define INSN_I_OPCODE_SHIFT		 0
+
+#define RV_OPCODE(v)		__ASM_STR(v)
+#define RV_FUNC3(v)		__ASM_STR(v)
+#define RV_FUNC7(v)		__ASM_STR(v)
+#define RV_SIMM12(v)		__ASM_STR(v)
+#define RV_RD(v)		__ASM_STR(v)
+#define RV_RS1(v)		__ASM_STR(v)
+#define RV_RS2(v)		__ASM_STR(v)
+#define __RV_REG(v)		__ASM_STR(x ## v)
+#define RV___RD(v)		__RV_REG(v)
+#define RV___RS1(v)		__RV_REG(v)
+#define RV___RS2(v)		__RV_REG(v)
+
+#define RV_OPCODE_MISC_MEM	RV_OPCODE(15)
+#define RV_OPCODE_SYSTEM	RV_OPCODE(115)
+
+#define RV_OPCODE_MISC_MEM	RV_OPCODE(15)
+#define RV_OPCODE_SYSTEM	RV_OPCODE(115)
+
+#define __INSN_I(opcode, func3, rd, rs1, simm12)	\
+	".insn	i " opcode ", " func3 ", " rd ", " rs1 ", " simm12 "\n"
+
+#define INSN_I(opcode, func3, rd, rs1, simm12)			\
+	__INSN_I(RV_##opcode, RV_##func3, RV_##rd,		\
+		 RV_##rs1, RV_##simm12)
+
+#endif /* _ASM_RISCV_BARRIER_H */
-- 
2.34.1

[PATCH v1 2/3] riscv: cache: Add CBO instructions

[Mayuresh Chitale]

Define CBO inval and flush instructions and use those for the
dcache inval and flush operations respectively.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 arch/riscv/Kconfig             |  4 ++
 arch/riscv/include/asm/cache.h |  3 ++
 arch/riscv/lib/cache.c         | 90 ++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index fa3b016c52..0f89d07be7 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -310,6 +310,10 @@ endmenu
 config RISCV_ISA_A
 	def_bool y
 
+config RISCV_ISA_ZICBOM
+	bool "Zicbom support"
+	depends on !SYS_DISABLE_DCACHE_OPS
+
 config DMA_ADDR_T_64BIT
 	bool
 	default y if 64BIT
diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
index 874963d731..42dbce5b4f 100644
--- a/arch/riscv/include/asm/cache.h
+++ b/arch/riscv/include/asm/cache.h
@@ -9,6 +9,9 @@
 
 /* cache */
 void cache_flush(void);
+void riscv_zicbom_init(void);
+void cbo_flush(unsigned long start, unsigned long end);
+void cbo_inval(unsigned long start, unsigned long end);
 
 /*
  * The current upper bound for RISCV L1 data cache line sizes is 32 bytes.
diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
index afad7e117f..456353d9c1 100644
--- a/arch/riscv/lib/cache.c
+++ b/arch/riscv/lib/cache.c
@@ -5,6 +5,95 @@
  */
 
 #include <cpu_func.h>
+#include <dm.h>
+#include <asm/insn-def.h>
+#include <linux/const.h>
+
+#define CBO_INVAL(base)						\
+	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
+	       RS1(base), SIMM12(0))
+#define CBO_CLEAN(base)						\
+	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
+	       RS1(base), SIMM12(1))
+#define CBO_FLUSH(base)						\
+	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
+	       RS1(base), SIMM12(2))
+enum {
+	CBO_CLEAN,
+	CBO_FLUSH,
+	CBO_INVAL
+} riscv_cbo_ops;
+static int zicbom_block_size;
+
+static inline void do_cbo_clean(unsigned long base)
+{
+	asm volatile ("add a0, %0, zero\n" CBO_CLEAN(%0) ::
+		      "r"(base) : "memory");
+}
+
+static inline void do_cbo_flush(unsigned long base)
+{
+	asm volatile ("add a0, %0, zero\n" CBO_FLUSH(%0) ::
+		      "r"(base) : "memory");
+}
+
+static inline void do_cbo_inval(unsigned long base)
+{
+	asm volatile ("add a0, %0, zero\n" CBO_INVAL(%0) ::
+		      "r"(base) : "memory");
+}
+
+static void cbo_op(int op_type, unsigned long start,
+		   unsigned long end)
+{
+	unsigned long op_size = end - start, size = 0;
+	void (*fn)(unsigned long base);
+
+	switch (op_type) {
+	case CBO_CLEAN:
+		fn = do_cbo_clean;
+		break;
+	case CBO_FLUSH:
+		fn = do_cbo_flush;
+		break;
+	case CBO_INVAL:
+		fn = do_cbo_inval;
+		break;
+	}
+	start &= ~(UL(zicbom_block_size - 1));
+	while (size < op_size) {
+		fn(start + size);
+		size += zicbom_block_size;
+	}
+}
+
+void cbo_flush(unsigned long start, unsigned long end)
+{
+	if (zicbom_block_size)
+		cbo_op(CBO_FLUSH, start, end);
+}
+
+void cbo_inval(unsigned long start, unsigned long end)
+{
+	if (zicbom_block_size)
+		cbo_op(CBO_INVAL, start, end);
+}
+
+void riscv_zicbom_init(void)
+{
+	struct udevice *dev;
+
+	if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
+		return;
+
+	uclass_first_device(UCLASS_CPU, &dev);
+	if (!dev) {
+		log_err("Failed to get cpu device!\n");
+		return;
+	}
+
+	(void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
+}
 
 void invalidate_icache_all(void)
 {
@@ -72,4 +161,5 @@ __weak int dcache_status(void)
 
 __weak void enable_caches(void)
 {
+	puts("WARNING: Caches not enabled\n");
 }
-- 
2.34.1

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Heinrich Schuchardt]

On 20.08.24 11:37, Mayuresh Chitale wrote:
> Define CBO inval and flush instructions and use those for the
> dcache inval and flush operations respectively.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   arch/riscv/Kconfig             |  4 ++
>   arch/riscv/include/asm/cache.h |  3 ++
>   arch/riscv/lib/cache.c         | 90 ++++++++++++++++++++++++++++++++++
>   3 files changed, 97 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index fa3b016c52..0f89d07be7 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -310,6 +310,10 @@ endmenu
>   config RISCV_ISA_A
>   	def_bool y
>
> +config RISCV_ISA_ZICBOM
> +	bool "Zicbom support"
> +	depends on !SYS_DISABLE_DCACHE_OPS
> +
>   config DMA_ADDR_T_64BIT
>   	bool
>   	default y if 64BIT
> diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> index 874963d731..42dbce5b4f 100644
> --- a/arch/riscv/include/asm/cache.h
> +++ b/arch/riscv/include/asm/cache.h
> @@ -9,6 +9,9 @@
>
>   /* cache */
>   void cache_flush(void);
> +void riscv_zicbom_init(void);
> +void cbo_flush(unsigned long start, unsigned long end);
> +void cbo_inval(unsigned long start, unsigned long end);
>
>   /*
>    * The current upper bound for RISCV L1 data cache line sizes is 32 bytes.
> diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
> index afad7e117f..456353d9c1 100644
> --- a/arch/riscv/lib/cache.c
> +++ b/arch/riscv/lib/cache.c
> @@ -5,6 +5,95 @@
>    */
>
>   #include <cpu_func.h>
> +#include <dm.h>
> +#include <asm/insn-def.h>
> +#include <linux/const.h>
> +
> +#define CBO_INVAL(base)						\
> +	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
> +	       RS1(base), SIMM12(0))
> +#define CBO_CLEAN(base)						\
> +	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
> +	       RS1(base), SIMM12(1))
> +#define CBO_FLUSH(base)						\
> +	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
> +	       RS1(base), SIMM12(2))
> +enum {
> +	CBO_CLEAN,
> +	CBO_FLUSH,
> +	CBO_INVAL
> +} riscv_cbo_ops;
> +static int zicbom_block_size;
> +
> +static inline void do_cbo_clean(unsigned long base)
> +{
> +	asm volatile ("add a0, %0, zero\n" CBO_CLEAN(%0) ::
> +		      "r"(base) : "memory");
> +}
> +
> +static inline void do_cbo_flush(unsigned long base)
> +{
> +	asm volatile ("add a0, %0, zero\n" CBO_FLUSH(%0) ::
> +		      "r"(base) : "memory");
> +}
> +
> +static inline void do_cbo_inval(unsigned long base)
> +{
> +	asm volatile ("add a0, %0, zero\n" CBO_INVAL(%0) ::
> +		      "r"(base) : "memory");
> +}
> +
> +static void cbo_op(int op_type, unsigned long start,
> +		   unsigned long end)
> +{
> +	unsigned long op_size = end - start, size = 0;
> +	void (*fn)(unsigned long base);
> +
> +	switch (op_type) {
> +	case CBO_CLEAN:
> +		fn = do_cbo_clean;
> +		break;
> +	case CBO_FLUSH:
> +		fn = do_cbo_flush;
> +		break;
> +	case CBO_INVAL:
> +		fn = do_cbo_inval;
> +		break;
> +	}
> +	start &= ~(UL(zicbom_block_size - 1));
> +	while (size < op_size) {
> +		fn(start + size);
> +		size += zicbom_block_size;
> +	}
> +}
> +
> +void cbo_flush(unsigned long start, unsigned long end)
> +{
> +	if (zicbom_block_size)
> +		cbo_op(CBO_FLUSH, start, end);
> +}
> +
> +void cbo_inval(unsigned long start, unsigned long end)
> +{
> +	if (zicbom_block_size)
> +		cbo_op(CBO_INVAL, start, end);
> +}
> +
> +void riscv_zicbom_init(void)
> +{
> +	struct udevice *dev;
> +
> +	if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
> +		return;
> +
> +	uclass_first_device(UCLASS_CPU, &dev);
> +	if (!dev) {
> +		log_err("Failed to get cpu device!\n");
> +		return;

Please, return an error code.

> +	}
> +
> +	(void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);

Please, do not ignore errors.

Best regards

Heinrich

> +}
>
>   void invalidate_icache_all(void)
>   {
> @@ -72,4 +161,5 @@ __weak int dcache_status(void)
>
>   __weak void enable_caches(void)
>   {
> +	puts("WARNING: Caches not enabled\n");
>   }

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Mayuresh Chitale]

On Tue, Aug 20, 2024 at 5:49 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 20.08.24 11:37, Mayuresh Chitale wrote:
> > Define CBO inval and flush instructions and use those for the
> > dcache inval and flush operations respectively.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   arch/riscv/Kconfig             |  4 ++
> >   arch/riscv/include/asm/cache.h |  3 ++
> >   arch/riscv/lib/cache.c         | 90 ++++++++++++++++++++++++++++++++++
> >   3 files changed, 97 insertions(+)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index fa3b016c52..0f89d07be7 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -310,6 +310,10 @@ endmenu
> >   config RISCV_ISA_A
> >       def_bool y
> >
> > +config RISCV_ISA_ZICBOM
> > +     bool "Zicbom support"
> > +     depends on !SYS_DISABLE_DCACHE_OPS
> > +
> >   config DMA_ADDR_T_64BIT
> >       bool
> >       default y if 64BIT
> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > index 874963d731..42dbce5b4f 100644
> > --- a/arch/riscv/include/asm/cache.h
> > +++ b/arch/riscv/include/asm/cache.h
> > @@ -9,6 +9,9 @@
> >
> >   /* cache */
> >   void cache_flush(void);
> > +void riscv_zicbom_init(void);
> > +void cbo_flush(unsigned long start, unsigned long end);
> > +void cbo_inval(unsigned long start, unsigned long end);
> >
> >   /*
> >    * The current upper bound for RISCV L1 data cache line sizes is 32 bytes.
> > diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
> > index afad7e117f..456353d9c1 100644
> > --- a/arch/riscv/lib/cache.c
> > +++ b/arch/riscv/lib/cache.c
> > @@ -5,6 +5,95 @@
> >    */
> >
> >   #include <cpu_func.h>
> > +#include <dm.h>
> > +#include <asm/insn-def.h>
> > +#include <linux/const.h>
> > +
> > +#define CBO_INVAL(base)                                              \
> > +     INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),              \
> > +            RS1(base), SIMM12(0))
> > +#define CBO_CLEAN(base)                                              \
> > +     INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),              \
> > +            RS1(base), SIMM12(1))
> > +#define CBO_FLUSH(base)                                              \
> > +     INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),              \
> > +            RS1(base), SIMM12(2))
> > +enum {
> > +     CBO_CLEAN,
> > +     CBO_FLUSH,
> > +     CBO_INVAL
> > +} riscv_cbo_ops;
> > +static int zicbom_block_size;
> > +
> > +static inline void do_cbo_clean(unsigned long base)
> > +{
> > +     asm volatile ("add a0, %0, zero\n" CBO_CLEAN(%0) ::
> > +                   "r"(base) : "memory");
> > +}
> > +
> > +static inline void do_cbo_flush(unsigned long base)
> > +{
> > +     asm volatile ("add a0, %0, zero\n" CBO_FLUSH(%0) ::
> > +                   "r"(base) : "memory");
> > +}
> > +
> > +static inline void do_cbo_inval(unsigned long base)
> > +{
> > +     asm volatile ("add a0, %0, zero\n" CBO_INVAL(%0) ::
> > +                   "r"(base) : "memory");
> > +}
> > +
> > +static void cbo_op(int op_type, unsigned long start,
> > +                unsigned long end)
> > +{
> > +     unsigned long op_size = end - start, size = 0;
> > +     void (*fn)(unsigned long base);
> > +
> > +     switch (op_type) {
> > +     case CBO_CLEAN:
> > +             fn = do_cbo_clean;
> > +             break;
> > +     case CBO_FLUSH:
> > +             fn = do_cbo_flush;
> > +             break;
> > +     case CBO_INVAL:
> > +             fn = do_cbo_inval;
> > +             break;
> > +     }
> > +     start &= ~(UL(zicbom_block_size - 1));
> > +     while (size < op_size) {
> > +             fn(start + size);
> > +             size += zicbom_block_size;
> > +     }
> > +}
> > +
> > +void cbo_flush(unsigned long start, unsigned long end)
> > +{
> > +     if (zicbom_block_size)
> > +             cbo_op(CBO_FLUSH, start, end);
> > +}
> > +
> > +void cbo_inval(unsigned long start, unsigned long end)
> > +{
> > +     if (zicbom_block_size)
> > +             cbo_op(CBO_INVAL, start, end);
> > +}
> > +
> > +void riscv_zicbom_init(void)
> > +{
> > +     struct udevice *dev;
> > +
> > +     if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
> > +             return;
> > +
> > +     uclass_first_device(UCLASS_CPU, &dev);
> > +     if (!dev) {
> > +             log_err("Failed to get cpu device!\n");
> > +             return;
>
> Please, return an error code.
>
> > +     }
> > +
> > +     (void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
>
> Please, do not ignore errors.
Ok.
>
> Best regards
>
> Heinrich
>
> > +}
> >
> >   void invalidate_icache_all(void)
> >   {
> > @@ -72,4 +161,5 @@ __weak int dcache_status(void)
> >
> >   __weak void enable_caches(void)
> >   {
> > +     puts("WARNING: Caches not enabled\n");
> >   }
>

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Tue, Aug 20, 2024 at 02:14:01PM +0200, Heinrich Schuchardt wrote:
> On 20.08.24 11:37, Mayuresh Chitale wrote:
> > +void riscv_zicbom_init(void)
> > +{
> > +	struct udevice *dev;
> > +
> > +	if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
> > +		return;
> > +
> > +	uclass_first_device(UCLASS_CPU, &dev);
> > +	if (!dev) {
> > +		log_err("Failed to get cpu device!\n");
> > +		return;
> 
> Please, return an error code.
> 
> > +	}
> > +
> > +	(void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
> 
> Please, do not ignore errors.

I'm curious what the policy of U-Boot is w.r.t. extension probing. Is it
okay to enable the option for Zicbom even if your hardware does not
support it, in which case riscv_zicbom_init() would be expected to fail
gracefully and no CMOs done?
Say, for example, you had two very similar chips, one with DMA non-coherent
peripherals and one that only differed by having DMA coherent ones, and you
wanted to run the same U-Boot binary on both devices using a devicetree
passed from firmware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Conor.Dooley]

On 21/08/2024 10:57, Conor Dooley wrote:
> On Tue, Aug 20, 2024 at 02:14:01PM +0200, Heinrich Schuchardt wrote:
>> On 20.08.24 11:37, Mayuresh Chitale wrote:
>>> +void riscv_zicbom_init(void)
>>> +{
>>> +	struct udevice *dev;
>>> +
>>> +	if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
>>> +		return;
>>> +
>>> +	uclass_first_device(UCLASS_CPU, &dev);
>>> +	if (!dev) {
>>> +		log_err("Failed to get cpu device!\n");
>>> +		return;
>>
>> Please, return an error code.
>>
>>> +	}
>>> +
>>> +	(void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
>>
>> Please, do not ignore errors.
> 
> I'm curious what the policy of U-Boot is w.r.t. extension probing. Is it
> okay to enable the option for Zicbom even if your hardware does not
> support it, in which case riscv_zicbom_init() would be expected to fail
> gracefully and no CMOs done?
> Say, for example, you had two very similar chips, one with DMA non-coherent
> peripherals and one that only differed by having DMA coherent ones, and you
> wanted to run the same U-Boot binary on both devices using a devicetree
> passed from firmware.

To be clear, I did notice that if the dev_read_u32() does not populate
zicbom_block_size no cache ops will be done. My question was about
the behaviour relating to the config option and or emitting warnings etc.

Cheers,
Conor.

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Mayuresh Chitale]

On Wed, Aug 21, 2024 at 3:33 PM <Conor.Dooley@microchip.com> wrote:
>
> On 21/08/2024 10:57, Conor Dooley wrote:
> > On Tue, Aug 20, 2024 at 02:14:01PM +0200, Heinrich Schuchardt wrote:
> >> On 20.08.24 11:37, Mayuresh Chitale wrote:
> >>> +void riscv_zicbom_init(void)
> >>> +{
> >>> +   struct udevice *dev;
> >>> +
> >>> +   if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
> >>> +           return;
> >>> +
> >>> +   uclass_first_device(UCLASS_CPU, &dev);
> >>> +   if (!dev) {
> >>> +           log_err("Failed to get cpu device!\n");
> >>> +           return;
> >>
> >> Please, return an error code.
> >>
> >>> +   }
> >>> +
> >>> +   (void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
> >>
> >> Please, do not ignore errors.
> >
> > I'm curious what the policy of U-Boot is w.r.t. extension probing. Is it
> > okay to enable the option for Zicbom even if your hardware does not
> > support it, in which case riscv_zicbom_init() would be expected to fail
> > gracefully and no CMOs done?
> > Say, for example, you had two very similar chips, one with DMA non-coherent
> > peripherals and one that only differed by having DMA coherent ones, and you
> > wanted to run the same U-Boot binary on both devices using a devicetree
> > passed from firmware.
>
> To be clear, I did notice that if the dev_read_u32() does not populate
> zicbom_block_size no cache ops will be done. My question was about
> the behaviour relating to the config option and or emitting warnings etc.
I am planning to add a info message if the config option is enabled
but the dt property is not found.
>
> Cheers,
> Conor.

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Mayuresh Chitale]

On Wed, Aug 21, 2024 at 3:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Tue, Aug 20, 2024 at 02:14:01PM +0200, Heinrich Schuchardt wrote:
> > On 20.08.24 11:37, Mayuresh Chitale wrote:
> > > +void riscv_zicbom_init(void)
> > > +{
> > > +   struct udevice *dev;
> > > +
> > > +   if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
> > > +           return;
> > > +
> > > +   uclass_first_device(UCLASS_CPU, &dev);
> > > +   if (!dev) {
> > > +           log_err("Failed to get cpu device!\n");
> > > +           return;
> >
> > Please, return an error code.
> >
> > > +   }
> > > +
> > > +   (void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
> >
> > Please, do not ignore errors.
>
> I'm curious what the policy of U-Boot is w.r.t. extension probing. Is it
> okay to enable the option for Zicbom even if your hardware does not
> support it, in which case riscv_zicbom_init() would be expected to fail
> gracefully and no CMOs done?
> Say, for example, you had two very similar chips, one with DMA non-coherent
> peripherals and one that only differed by having DMA coherent ones, and you
> wanted to run the same U-Boot binary on both devices using a devicetree
> passed from firmware.
I think in such a case the config option can always be enabled with
the proper dt being passed by the firmware.

Re: [PATCH v1 2/3] riscv: cache: Add CBO instructions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

On Wed, Aug 21, 2024 at 09:27:03PM +0530, Mayuresh Chitale wrote:
> On Wed, Aug 21, 2024 at 3:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Tue, Aug 20, 2024 at 02:14:01PM +0200, Heinrich Schuchardt wrote:
> > > On 20.08.24 11:37, Mayuresh Chitale wrote:
> > > > +void riscv_zicbom_init(void)
> > > > +{
> > > > +   struct udevice *dev;
> > > > +
> > > > +   if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
> > > > +           return;
> > > > +
> > > > +   uclass_first_device(UCLASS_CPU, &dev);
> > > > +   if (!dev) {
> > > > +           log_err("Failed to get cpu device!\n");
> > > > +           return;
> > >
> > > Please, return an error code.
> > >
> > > > +   }
> > > > +
> > > > +   (void)dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size);
> > >
> > > Please, do not ignore errors.
> >
> > I'm curious what the policy of U-Boot is w.r.t. extension probing. Is it
> > okay to enable the option for Zicbom even if your hardware does not
> > support it, in which case riscv_zicbom_init() would be expected to fail
> > gracefully and no CMOs done?
> > Say, for example, you had two very similar chips, one with DMA non-coherent
> > peripherals and one that only differed by having DMA coherent ones, and you
> > wanted to run the same U-Boot binary on both devices using a devicetree
> > passed from firmware.
> I think in such a case the config option can always be enabled with
> the proper dt being passed by the firmware.

I don't understand what you mean, sorry. Both cases I mention would have
a "proper" devicetree that matches the hardware. I'm interested in
whether or not U-Boot's maintainers think that enabling the config option
should warn on, or preclude use of, systems that do not not support
Zicbom.

Cheers,
Conor.

Also, technically there's no guarantee that "riscv,cbom-block-size" being
present means that zicbom also is. Realistically, it should never be the
case that it is present without zicbom in the isa string representation,
but correctness would require checking that it is. supports_extension()
only supports single-letter extensions at present, making it support
multi-letter extensions should not be too difficult.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 3/3] board: qemu-riscv: Override enable_caches

[Mayuresh Chitale]

Define enable_caches function for the qemu-riscv board which probes for
the cbom-block-size dt property when RISCV_ISA_ZICBOM is enabled. Also
add flush_dcache_range and invalidate_dcache_range functions which use
the corresponding CBO ops.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 board/emulation/qemu-riscv/qemu-riscv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c
index e5193e31e3..1795d2f831 100644
--- a/board/emulation/qemu-riscv/qemu-riscv.c
+++ b/board/emulation/qemu-riscv/qemu-riscv.c
@@ -14,6 +14,7 @@
 #include <usb.h>
 #include <virtio_types.h>
 #include <virtio.h>
+#include <asm/cache.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -70,3 +71,18 @@ void *board_fdt_blob_setup(int *err)
 	/* Stored the DTB address there during our init */
 	return (void *)(ulong)gd->arch.firmware_fdt_addr;
 }
+
+void enable_caches(void)
+{
+	riscv_zicbom_init();
+}
+
+void flush_dcache_range(unsigned long start, unsigned long end)
+{
+	cbo_flush(start, end);
+}
+
+void invalidate_dcache_range(unsigned long start, unsigned long end)
+{
+	cbo_inval(start, end);
+}
-- 
2.34.1

Re: [PATCH v1 3/3] board: qemu-riscv: Override enable_caches

[Heinrich Schuchardt]

On 20.08.24 11:37, Mayuresh Chitale wrote:
> Define enable_caches function for the qemu-riscv board which probes for
> the cbom-block-size dt property when RISCV_ISA_ZICBOM is enabled. Also
> add flush_dcache_range and invalidate_dcache_range functions which use
> the corresponding CBO ops.
> 
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   board/emulation/qemu-riscv/qemu-riscv.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c
> index e5193e31e3..1795d2f831 100644
> --- a/board/emulation/qemu-riscv/qemu-riscv.c
> +++ b/board/emulation/qemu-riscv/qemu-riscv.c
> @@ -14,6 +14,7 @@
>   #include <usb.h>
>   #include <virtio_types.h>
>   #include <virtio.h>
> +#include <asm/cache.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -70,3 +71,18 @@ void *board_fdt_blob_setup(int *err)
>   	/* Stored the DTB address there during our init */
>   	return (void *)(ulong)gd->arch.firmware_fdt_addr;
>   }
> +
> +void enable_caches(void)
> +{
> +	riscv_zicbom_init();
> +}

Enable caches may be called multiple times. But riscv_zicbom_init() only 
needs to be called once.

> +
> +void flush_dcache_range(unsigned long start, unsigned long end)
> +{
> +	cbo_flush(start, end);
> +}
> +
> +void invalidate_dcache_range(unsigned long start, unsigned long end)
> +{
> +	cbo_inval(start, end);
> +}

The ZICBOM extension is not specific to a single board. We should not 
add this code to board files but into the central place for cache 
operations, i.e. arch/riscv/lib/cache.c.

E.g.

static int get_zicbom_block_size()
{
	if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
		return 0;
	if (zicbom_block_size)
		return zicbom_block_size;

	uclass_first_device(UCLASS_CPU, &dev);
	if (!dev) {
		log_err("Failed to get CPU device!\n");
		return 0;
	}

	if (dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size))
		return 0;
	
	return zicbom_block_size
}

__weak void invalidate_icache_range(unsigned long start, unsigned long end)
{
	if (CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM) && get_zicbom_block_size())
		cbo_inval(start, end);
	else		
	        invalidate_icache_all();
}

Cc: Rick as one of the RISC-V maintainers.

Best regards

Heinrich

Re: [PATCH v1 3/3] board: qemu-riscv: Override enable_caches

[Mayuresh Chitale]

Hi Heinrich,

On Tue, Aug 20, 2024 at 5:43 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 20.08.24 11:37, Mayuresh Chitale wrote:
> > Define enable_caches function for the qemu-riscv board which probes for
> > the cbom-block-size dt property when RISCV_ISA_ZICBOM is enabled. Also
> > add flush_dcache_range and invalidate_dcache_range functions which use
> > the corresponding CBO ops.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   board/emulation/qemu-riscv/qemu-riscv.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c
> > index e5193e31e3..1795d2f831 100644
> > --- a/board/emulation/qemu-riscv/qemu-riscv.c
> > +++ b/board/emulation/qemu-riscv/qemu-riscv.c
> > @@ -14,6 +14,7 @@
> >   #include <usb.h>
> >   #include <virtio_types.h>
> >   #include <virtio.h>
> > +#include <asm/cache.h>
> >
> >   DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -70,3 +71,18 @@ void *board_fdt_blob_setup(int *err)
> >       /* Stored the DTB address there during our init */
> >       return (void *)(ulong)gd->arch.firmware_fdt_addr;
> >   }
> > +
> > +void enable_caches(void)
> > +{
> > +     riscv_zicbom_init();
> > +}
>
> Enable caches may be called multiple times. But riscv_zicbom_init() only
> needs to be called once.
Ok.
>
> > +
> > +void flush_dcache_range(unsigned long start, unsigned long end)
> > +{
> > +     cbo_flush(start, end);
> > +}
> > +
> > +void invalidate_dcache_range(unsigned long start, unsigned long end)
> > +{
> > +     cbo_inval(start, end);
> > +}
>
> The ZICBOM extension is not specific to a single board. We should not
> add this code to board files but into the central place for cache
> operations, i.e. arch/riscv/lib/cache.c.
Most of the code is already cache.c. I didn't know if it would suffice
for all platforms so I decided to override the weak functions like in
ARM.

> E.g.
>
> static int get_zicbom_block_size()
> {
>         if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
>                 return 0;
>         if (zicbom_block_size)
>                 return zicbom_block_size;
>
>         uclass_first_device(UCLASS_CPU, &dev);
>         if (!dev) {
>                 log_err("Failed to get CPU device!\n");
>                 return 0;
>         }
>
>         if (dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size))
>                 return 0;
>
>         return zicbom_block_size
> }
>
> __weak void invalidate_icache_range(unsigned long start, unsigned long end)
> {
>         if (CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM) && get_zicbom_block_size())
>                 cbo_inval(start, end);
>         else
>                 invalidate_icache_all();
> }
>
> Cc: Rick as one of the RISC-V maintainers.
Ok.
>
> Best regards
>
> Heinrich
>

Re: [PATCH v1 3/3] board: qemu-riscv: Override enable_caches

[Heinrich Schuchardt]

On 21.08.24 11:11, Mayuresh Chitale wrote:
> Hi Heinrich,
> 
> On Tue, Aug 20, 2024 at 5:43 PM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 20.08.24 11:37, Mayuresh Chitale wrote:
>>> Define enable_caches function for the qemu-riscv board which probes for
>>> the cbom-block-size dt property when RISCV_ISA_ZICBOM is enabled. Also
>>> add flush_dcache_range and invalidate_dcache_range functions which use
>>> the corresponding CBO ops.
>>>
>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>> ---
>>>    board/emulation/qemu-riscv/qemu-riscv.c | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c
>>> index e5193e31e3..1795d2f831 100644
>>> --- a/board/emulation/qemu-riscv/qemu-riscv.c
>>> +++ b/board/emulation/qemu-riscv/qemu-riscv.c
>>> @@ -14,6 +14,7 @@
>>>    #include <usb.h>
>>>    #include <virtio_types.h>
>>>    #include <virtio.h>
>>> +#include <asm/cache.h>
>>>
>>>    DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -70,3 +71,18 @@ void *board_fdt_blob_setup(int *err)
>>>        /* Stored the DTB address there during our init */
>>>        return (void *)(ulong)gd->arch.firmware_fdt_addr;
>>>    }
>>> +
>>> +void enable_caches(void)
>>> +{
>>> +     riscv_zicbom_init();
>>> +}
>>
>> Enable caches may be called multiple times. But riscv_zicbom_init() only
>> needs to be called once.
> Ok.
>>
>>> +
>>> +void flush_dcache_range(unsigned long start, unsigned long end)
>>> +{
>>> +     cbo_flush(start, end);
>>> +}
>>> +
>>> +void invalidate_dcache_range(unsigned long start, unsigned long end)
>>> +{
>>> +     cbo_inval(start, end);
>>> +}
>>
>> The ZICBOM extension is not specific to a single board. We should not
>> add this code to board files but into the central place for cache
>> operations, i.e. arch/riscv/lib/cache.c.
> Most of the code is already cache.c. I didn't know if it would suffice
> for all platforms so I decided to override the weak functions like in
> ARM.


With the suggestion below boards needing a board specific solution can 
still override the weak functions. But I would expect that in future new 
boards converge on zicbom.

Best regards

Heinrich

> 
>> E.g.
>>
>> static int get_zicbom_block_size()
>> {
>>          if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
>>                  return 0;
>>          if (zicbom_block_size)
>>                  return zicbom_block_size;
>>
>>          uclass_first_device(UCLASS_CPU, &dev);
>>          if (!dev) {
>>                  log_err("Failed to get CPU device!\n");
>>                  return 0;
>>          }
>>
>>          if (dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size))
>>                  return 0;
>>
>>          return zicbom_block_size
>> }
>>
>> __weak void invalidate_icache_range(unsigned long start, unsigned long end)
>> {
>>          if (CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM) && get_zicbom_block_size())
>>                  cbo_inval(start, end);
>>          else
>>                  invalidate_icache_all();
>> }
>>
>> Cc: Rick as one of the RISC-V maintainers.
> Ok.
>>
>> Best regards
>>
>> Heinrich
>>