[RFC PATCH 0/2] lib: reset: thead: Correct dts naming convention

[RFC PATCH 0/2] lib: reset: thead: Correct dts naming convention

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

Correct the naming convention to fit Linux kernel upstream rule.

Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/

Guo Ren (2):
  lib: reset: thead: Remove unnecessary fence operations
  lib: reset: thead: Correct the naming convention of dts

 docs/platform/thead-c9xx.md           | 21 ++++------
 lib/utils/reset/fdt_reset_thead.c     | 58 +++++++++++++--------------
 lib/utils/reset/fdt_reset_thead_asm.S |  2 -
 3 files changed, 35 insertions(+), 46 deletions(-)

-- 
2.36.1

[RFC PATCH 1/2] lib: reset: thead: Remove unnecessary fence operations

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

There are no load/store operations to fence at the reset point.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
 lib/utils/reset/fdt_reset_thead.c     | 1 -
 lib/utils/reset/fdt_reset_thead_asm.S | 2 --
 2 files changed, 3 deletions(-)

diff --git a/lib/utils/reset/fdt_reset_thead.c b/lib/utils/reset/fdt_reset_thead.c
index e1d6885debae..ff7d2981b42f 100644
--- a/lib/utils/reset/fdt_reset_thead.c
+++ b/lib/utils/reset/fdt_reset_thead.c
@@ -27,7 +27,6 @@ static void clone_csrs(int cnt)
 
 		/* Mask csr BIT[31 - 20] */
 		*(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
-		smp_mb();
 
 		/* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
 		*(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
diff --git a/lib/utils/reset/fdt_reset_thead_asm.S b/lib/utils/reset/fdt_reset_thead_asm.S
index 8237951fd82d..650f8585cdd2 100644
--- a/lib/utils/reset/fdt_reset_thead_asm.S
+++ b/lib/utils/reset/fdt_reset_thead_asm.S
@@ -27,7 +27,6 @@ __thead_pre_start_warm:
 	 */
 	li	t1, 0x70013
 	csrw	0x7c2, t1
-	fence rw,rw
 
 	lla	t1, custom_csr
 
@@ -43,5 +42,4 @@ __reset_thead_csr_stub:
 	 */
 	li	t1, 0x70013
 	csrw	0x7c2, t1
-	fence rw,rw
 	j _start_warm
-- 
2.36.1

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

Correct the naming convention to fit Linux kernel upstream rule.

Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
Cc: Conor Dooley <conor.dooley@microchip.com>
Cc: Jisheng Zhang <jszhang@kernel.org>
Cc: Wei Fu <wefu@redhat.com>
---
 docs/platform/thead-c9xx.md       | 21 +++++-------
 lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
 2 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
index 8bb9e91f1a9b..35cca94b5bb9 100644
--- a/docs/platform/thead-c9xx.md
+++ b/docs/platform/thead-c9xx.md
@@ -19,9 +19,6 @@ because it uses generic platform.
 CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
 ```
 
-The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
-"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
-
 DTS Example1: (Single core, eg: Allwinner D1 - c906)
 ----------------------------------------------------
 
@@ -75,8 +72,8 @@ DTS Example1: (Single core, eg: Allwinner D1 - c906)
 	}
 ```
 
-DTS Example2: (Multi cores with soc reset-regs)
------------------------------------------------
+DTS Example2: (Multi cores, eg: T-HEAD th1520 - c910)
+-----------------------------------------------------
 
 ```
 	cpus {
@@ -143,13 +140,11 @@ DTS Example2: (Multi cores with soc reset-regs)
 		compatible = "simple-bus";
 		ranges;
 
-		reset: reset-sample {
-			compatible = "thead,reset-sample";
-			entry-reg = <0xff 0xff019050>;
-			entry-cnt = <4>;
-			control-reg = <0xff 0xff015004>;
-			control-val = <0x1c>;
-			csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
+		reset-controller at ffff019050 {
+			compatible = "thead,th1520-cpu-reset";
+			reg = <0xff 0xff019050 0x0 0x4>, <0xff 0xff015004 0x0 0x0>;
+			reset-ctrl-val = <0x1c>;
+			csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc 0x7ce>;
 		};
 
 		clint0: clint at ffdc000000 {
@@ -186,7 +181,7 @@ DTS Example2: (Multi cores with old reset csrs)
 -----------------------------------------------
 ```
 	reset: reset-sample {
-		compatible = "thead,reset-sample";
+		compatible = "thead,common-cpu-reset";
 		using-csr-reset;
 		csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
 			    0x3b0 0x3b1 0x3b2 0x3b3
diff --git a/lib/utils/reset/fdt_reset_thead.c b/lib/utils/reset/fdt_reset_thead.c
index ff7d2981b42f..760c117f6178 100644
--- a/lib/utils/reset/fdt_reset_thead.c
+++ b/lib/utils/reset/fdt_reset_thead.c
@@ -5,6 +5,8 @@
 #include <libfdt.h>
 #include <sbi/riscv_io.h>
 #include <sbi/sbi_bitops.h>
+#include <sbi/sbi_console.h>
+#include <sbi/sbi_error.h>
 #include <sbi/sbi_hart.h>
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_system.h>
@@ -58,11 +60,10 @@ extern void __thead_pre_start_warm(void);
 static int thead_reset_init(void *fdt, int nodeoff,
 				 const struct fdt_match *match)
 {
-	char *p;
-	const fdt64_t *val;
+	int rc, len, i;
+	u32 tmp;
 	const fdt32_t *val_w;
-	int len, i;
-	u32 t, tmp = 0;
+	uint64_t reg_addr, reg_size;
 
 	/* Prepare clone csrs */
 	val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
@@ -85,45 +86,41 @@ static int thead_reset_init(void *fdt, int nodeoff,
 	if (fdt_getprop(fdt, nodeoff, "using-csr-reset", &len)) {
 		csr_write(0x7c7, (ulong)&__thead_pre_start_warm);
 		csr_write(0x7c6, -1);
+		goto out;
 	}
 
 	/* Custom reset method for secondary harts */
-	val = fdt_getprop(fdt, nodeoff, "entry-reg", &len);
-	if (len > 0 && val) {
-          p = (char *)(ulong)fdt64_to_cpu(*val);
-
-		val_w = fdt_getprop(fdt, nodeoff, "entry-cnt", &len);
-		if (len > 0 && val_w) {
-			tmp = fdt32_to_cpu(*val_w);
-
-			for (i = 0; i < tmp; i++) {
-				t = (ulong)&__thead_pre_start_warm;
-				writel(t, p + (8 * i));
-				t = (u64)(ulong)&__thead_pre_start_warm >> 32;
-				writel(t, p + (8 * i) + 4);
-			}
-		}
+	rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &reg_addr, &reg_size);
+	if (rc < 0)
+		return SBI_ENODEV;
+
+	for (i = 0; i < reg_size; i++) {
+		tmp = (ulong)&__thead_pre_start_warm;
+		writel(tmp, (char *)(ulong)reg_addr + (i * 8));
+		tmp = (u64)(ulong)&__thead_pre_start_warm >> 32;
+		writel(tmp, (char *)(ulong)reg_addr + (i * 8) + 4);
+	}
 
-		val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
-		if (len > 0 && val) {
-			p = (void *)(ulong)fdt64_to_cpu(*val);
+	rc = fdt_get_node_addr_size(fdt, nodeoff, 1, &reg_addr, &reg_size);
+	if (rc < 0)
+		return SBI_ENODEV;
 
-			val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
-			if (len > 0 && val_w) {
-				tmp = fdt32_to_cpu(*val_w);
-				tmp |= readl(p);
-				writel(tmp, p);
-			}
-		}
+	val_w = fdt_getprop(fdt, nodeoff, "reset-ctrl-val", &len);
+	if (len > 0 && val_w) {
+		tmp = fdt32_to_cpu(*val_w);
+		tmp |= readl((char *)(ulong)reg_addr);
+		writel(tmp,  (char *)(ulong)reg_addr);
 	}
 
+out:
 	sbi_system_reset_add_device(&thead_reset);
 
 	return 0;
 }
 
 static const struct fdt_match thead_reset_match[] = {
-	{ .compatible = "thead,reset-sample" },
+	{ .compatible = "thead,common-cpu-reset" },
+	{ .compatible = "thead,th1520-cpu-reset" },
 	{ },
 };
 
-- 
2.36.1

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

Hey Guo Ren,

On Tue, May 23, 2023 at 05:46:49AM -0400, guoren at kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Correct the naming convention to fit Linux kernel upstream rule.

Understatement of the year contender? ;)

This is a backwards incompatible change, based on suggestions that I
made on LKML, with a view to creating a yaml binding for this thing.
Everyone else might disagree with me, but I think "proper" bindings
should be written for this stuff so that use would be common across SBI
providers etc *but* in doing so do you not want to keep OpenSBI
backwards compatible with the markdown "binding" in the process?
Skimming this patch, the old way of doing things would no longer work?

> Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Cc: Conor Dooley <conor.dooley@microchip.com>
> Cc: Jisheng Zhang <jszhang@kernel.org>
> Cc: Wei Fu <wefu@redhat.com>
> ---
>  docs/platform/thead-c9xx.md       | 21 +++++-------
>  lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
>  2 files changed, 35 insertions(+), 43 deletions(-)
> 
> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> index 8bb9e91f1a9b..35cca94b5bb9 100644
> --- a/docs/platform/thead-c9xx.md
> +++ b/docs/platform/thead-c9xx.md
> @@ -19,9 +19,6 @@ because it uses generic platform.
>  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
>  ```
>  
> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> -
>  DTS Example1: (Single core, eg: Allwinner D1 - c906)
>  ----------------------------------------------------
>  
> @@ -75,8 +72,8 @@ DTS Example1: (Single core, eg: Allwinner D1 - c906)
>  	}
>  ```
>  
> -DTS Example2: (Multi cores with soc reset-regs)
> ------------------------------------------------
> +DTS Example2: (Multi cores, eg: T-HEAD th1520 - c910)
> +-----------------------------------------------------
>  
>  ```
>  	cpus {
> @@ -143,13 +140,11 @@ DTS Example2: (Multi cores with soc reset-regs)
>  		compatible = "simple-bus";
>  		ranges;
>  
> -		reset: reset-sample {
> -			compatible = "thead,reset-sample";
> -			entry-reg = <0xff 0xff019050>;
> -			entry-cnt = <4>;
> -			control-reg = <0xff 0xff015004>;
> -			control-val = <0x1c>;
> -			csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> +		reset-controller at ffff019050 {
> +			compatible = "thead,th1520-cpu-reset";
> +			reg = <0xff 0xff019050 0x0 0x4>, <0xff 0xff015004 0x0 0x0>;
> +			reset-ctrl-val = <0x1c>;
> +			csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc 0x7ce>;

I also did not suggest either rest-ctrl-val or csr-copy, as both are
surely able to be determined from match data based on the compatible
string.

Thanks,
Conor.

>  		};
>  
>  		clint0: clint at ffdc000000 {
> @@ -186,7 +181,7 @@ DTS Example2: (Multi cores with old reset csrs)
>  -----------------------------------------------
>  ```
>  	reset: reset-sample {
> -		compatible = "thead,reset-sample";
> +		compatible = "thead,common-cpu-reset";
>  		using-csr-reset;
>  		csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
>  			    0x3b0 0x3b1 0x3b2 0x3b3
> diff --git a/lib/utils/reset/fdt_reset_thead.c b/lib/utils/reset/fdt_reset_thead.c
> index ff7d2981b42f..760c117f6178 100644
> --- a/lib/utils/reset/fdt_reset_thead.c
> +++ b/lib/utils/reset/fdt_reset_thead.c
> @@ -5,6 +5,8 @@
>  #include <libfdt.h>
>  #include <sbi/riscv_io.h>
>  #include <sbi/sbi_bitops.h>
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_error.h>
>  #include <sbi/sbi_hart.h>
>  #include <sbi/sbi_scratch.h>
>  #include <sbi/sbi_system.h>
> @@ -58,11 +60,10 @@ extern void __thead_pre_start_warm(void);
>  static int thead_reset_init(void *fdt, int nodeoff,
>  				 const struct fdt_match *match)
>  {
> -	char *p;
> -	const fdt64_t *val;
> +	int rc, len, i;
> +	u32 tmp;
>  	const fdt32_t *val_w;
> -	int len, i;
> -	u32 t, tmp = 0;
> +	uint64_t reg_addr, reg_size;
>  
>  	/* Prepare clone csrs */
>  	val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
> @@ -85,45 +86,41 @@ static int thead_reset_init(void *fdt, int nodeoff,
>  	if (fdt_getprop(fdt, nodeoff, "using-csr-reset", &len)) {
>  		csr_write(0x7c7, (ulong)&__thead_pre_start_warm);
>  		csr_write(0x7c6, -1);
> +		goto out;
>  	}
>  
>  	/* Custom reset method for secondary harts */
> -	val = fdt_getprop(fdt, nodeoff, "entry-reg", &len);
> -	if (len > 0 && val) {
> -          p = (char *)(ulong)fdt64_to_cpu(*val);
> -
> -		val_w = fdt_getprop(fdt, nodeoff, "entry-cnt", &len);
> -		if (len > 0 && val_w) {
> -			tmp = fdt32_to_cpu(*val_w);
> -
> -			for (i = 0; i < tmp; i++) {
> -				t = (ulong)&__thead_pre_start_warm;
> -				writel(t, p + (8 * i));
> -				t = (u64)(ulong)&__thead_pre_start_warm >> 32;
> -				writel(t, p + (8 * i) + 4);
> -			}
> -		}
> +	rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &reg_addr, &reg_size);
> +	if (rc < 0)
> +		return SBI_ENODEV;
> +
> +	for (i = 0; i < reg_size; i++) {
> +		tmp = (ulong)&__thead_pre_start_warm;
> +		writel(tmp, (char *)(ulong)reg_addr + (i * 8));
> +		tmp = (u64)(ulong)&__thead_pre_start_warm >> 32;
> +		writel(tmp, (char *)(ulong)reg_addr + (i * 8) + 4);
> +	}
>  
> -		val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
> -		if (len > 0 && val) {
> -			p = (void *)(ulong)fdt64_to_cpu(*val);
> +	rc = fdt_get_node_addr_size(fdt, nodeoff, 1, &reg_addr, &reg_size);
> +	if (rc < 0)
> +		return SBI_ENODEV;
>  
> -			val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
> -			if (len > 0 && val_w) {
> -				tmp = fdt32_to_cpu(*val_w);
> -				tmp |= readl(p);
> -				writel(tmp, p);
> -			}
> -		}
> +	val_w = fdt_getprop(fdt, nodeoff, "reset-ctrl-val", &len);
> +	if (len > 0 && val_w) {
> +		tmp = fdt32_to_cpu(*val_w);
> +		tmp |= readl((char *)(ulong)reg_addr);
> +		writel(tmp,  (char *)(ulong)reg_addr);
>  	}
>  
> +out:
>  	sbi_system_reset_add_device(&thead_reset);
>  
>  	return 0;
>  }
>  
>  static const struct fdt_match thead_reset_match[] = {
> -	{ .compatible = "thead,reset-sample" },
> +	{ .compatible = "thead,common-cpu-reset" },
> +	{ .compatible = "thead,th1520-cpu-reset" },
>  	{ },
>  };
>  
> -- 
> 2.36.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230523/10e8f96e/attachment-0001.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Tue, May 23, 2023 at 11:51?PM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Guo Ren,
>
> On Tue, May 23, 2023 at 05:46:49AM -0400, guoren at kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Correct the naming convention to fit Linux kernel upstream rule.
>
> Understatement of the year contender? ;)
>
> This is a backwards incompatible change, based on suggestions that I
> made on LKML, with a view to creating a yaml binding for this thing.
> Everyone else might disagree with me, but I think "proper" bindings
> should be written for this stuff so that use would be common across SBI
> providers etc *but* in doing so do you not want to keep OpenSBI
> backwards compatible with the markdown "binding" in the process?
> Skimming this patch, the old way of doing things would no longer work?
There is no upstreamed usage of the old way, which used in old frozen
version of opensbi & custom linux. So I can accept the change in the
new version opensbi & Linux.

>
> > Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > Cc: Conor Dooley <conor.dooley@microchip.com>
> > Cc: Jisheng Zhang <jszhang@kernel.org>
> > Cc: Wei Fu <wefu@redhat.com>
> > ---
> >  docs/platform/thead-c9xx.md       | 21 +++++-------
> >  lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
> >  2 files changed, 35 insertions(+), 43 deletions(-)
> >
> > diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> > index 8bb9e91f1a9b..35cca94b5bb9 100644
> > --- a/docs/platform/thead-c9xx.md
> > +++ b/docs/platform/thead-c9xx.md
> > @@ -19,9 +19,6 @@ because it uses generic platform.
> >  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> >  ```
> >
> > -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> > -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> > -
> >  DTS Example1: (Single core, eg: Allwinner D1 - c906)
> >  ----------------------------------------------------
> >
> > @@ -75,8 +72,8 @@ DTS Example1: (Single core, eg: Allwinner D1 - c906)
> >       }
> >  ```
> >
> > -DTS Example2: (Multi cores with soc reset-regs)
> > ------------------------------------------------
> > +DTS Example2: (Multi cores, eg: T-HEAD th1520 - c910)
> > +-----------------------------------------------------
> >
> >  ```
> >       cpus {
> > @@ -143,13 +140,11 @@ DTS Example2: (Multi cores with soc reset-regs)
> >               compatible = "simple-bus";
> >               ranges;
> >
> > -             reset: reset-sample {
> > -                     compatible = "thead,reset-sample";
> > -                     entry-reg = <0xff 0xff019050>;
> > -                     entry-cnt = <4>;
> > -                     control-reg = <0xff 0xff015004>;
> > -                     control-val = <0x1c>;
> > -                     csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > +             reset-controller at ffff019050 {
> > +                     compatible = "thead,th1520-cpu-reset";
> > +                     reg = <0xff 0xff019050 0x0 0x4>, <0xff 0xff015004 0x0 0x0>;
> > +                     reset-ctrl-val = <0x1c>;
> > +                     csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc 0x7ce>;
>
> I also did not suggest either rest-ctrl-val or csr-copy, as both are
> surely able to be determined from match data based on the compatible
> string.
I want to keep them in dts to be modified flexiable. This driver is
not only for th1520, but for bunches of  T-HEAD SoCs, th1520 is a
sample/example for us.

Of cause, some of vendors would develop their own chip reset flow
(Allwinner, Sopho), but this one would be easier for hardware guys to
modify dtb without compling some stuffs.

Does the "reset-ctrl-val & csr-copy" violate any rule of dts?

>
> Thanks,
> Conor.
>
> >               };
> >
> >               clint0: clint at ffdc000000 {
> > @@ -186,7 +181,7 @@ DTS Example2: (Multi cores with old reset csrs)
> >  -----------------------------------------------
> >  ```
> >       reset: reset-sample {
> > -             compatible = "thead,reset-sample";
> > +             compatible = "thead,common-cpu-reset";
> >               using-csr-reset;
> >               csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
> >                           0x3b0 0x3b1 0x3b2 0x3b3
> > diff --git a/lib/utils/reset/fdt_reset_thead.c b/lib/utils/reset/fdt_reset_thead.c
> > index ff7d2981b42f..760c117f6178 100644
> > --- a/lib/utils/reset/fdt_reset_thead.c
> > +++ b/lib/utils/reset/fdt_reset_thead.c
> > @@ -5,6 +5,8 @@
> >  #include <libfdt.h>
> >  #include <sbi/riscv_io.h>
> >  #include <sbi/sbi_bitops.h>
> > +#include <sbi/sbi_console.h>
> > +#include <sbi/sbi_error.h>
> >  #include <sbi/sbi_hart.h>
> >  #include <sbi/sbi_scratch.h>
> >  #include <sbi/sbi_system.h>
> > @@ -58,11 +60,10 @@ extern void __thead_pre_start_warm(void);
> >  static int thead_reset_init(void *fdt, int nodeoff,
> >                                const struct fdt_match *match)
> >  {
> > -     char *p;
> > -     const fdt64_t *val;
> > +     int rc, len, i;
> > +     u32 tmp;
> >       const fdt32_t *val_w;
> > -     int len, i;
> > -     u32 t, tmp = 0;
> > +     uint64_t reg_addr, reg_size;
> >
> >       /* Prepare clone csrs */
> >       val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
> > @@ -85,45 +86,41 @@ static int thead_reset_init(void *fdt, int nodeoff,
> >       if (fdt_getprop(fdt, nodeoff, "using-csr-reset", &len)) {
> >               csr_write(0x7c7, (ulong)&__thead_pre_start_warm);
> >               csr_write(0x7c6, -1);
> > +             goto out;
> >       }
> >
> >       /* Custom reset method for secondary harts */
> > -     val = fdt_getprop(fdt, nodeoff, "entry-reg", &len);
> > -     if (len > 0 && val) {
> > -          p = (char *)(ulong)fdt64_to_cpu(*val);
> > -
> > -             val_w = fdt_getprop(fdt, nodeoff, "entry-cnt", &len);
> > -             if (len > 0 && val_w) {
> > -                     tmp = fdt32_to_cpu(*val_w);
> > -
> > -                     for (i = 0; i < tmp; i++) {
> > -                             t = (ulong)&__thead_pre_start_warm;
> > -                             writel(t, p + (8 * i));
> > -                             t = (u64)(ulong)&__thead_pre_start_warm >> 32;
> > -                             writel(t, p + (8 * i) + 4);
> > -                     }
> > -             }
> > +     rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &reg_addr, &reg_size);
> > +     if (rc < 0)
> > +             return SBI_ENODEV;
> > +
> > +     for (i = 0; i < reg_size; i++) {
> > +             tmp = (ulong)&__thead_pre_start_warm;
> > +             writel(tmp, (char *)(ulong)reg_addr + (i * 8));
> > +             tmp = (u64)(ulong)&__thead_pre_start_warm >> 32;
> > +             writel(tmp, (char *)(ulong)reg_addr + (i * 8) + 4);
> > +     }
> >
> > -             val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
> > -             if (len > 0 && val) {
> > -                     p = (void *)(ulong)fdt64_to_cpu(*val);
> > +     rc = fdt_get_node_addr_size(fdt, nodeoff, 1, &reg_addr, &reg_size);
> > +     if (rc < 0)
> > +             return SBI_ENODEV;
> >
> > -                     val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
> > -                     if (len > 0 && val_w) {
> > -                             tmp = fdt32_to_cpu(*val_w);
> > -                             tmp |= readl(p);
> > -                             writel(tmp, p);
> > -                     }
> > -             }
> > +     val_w = fdt_getprop(fdt, nodeoff, "reset-ctrl-val", &len);
> > +     if (len > 0 && val_w) {
> > +             tmp = fdt32_to_cpu(*val_w);
> > +             tmp |= readl((char *)(ulong)reg_addr);
> > +             writel(tmp,  (char *)(ulong)reg_addr);
> >       }
> >
> > +out:
> >       sbi_system_reset_add_device(&thead_reset);
> >
> >       return 0;
> >  }
> >
> >  static const struct fdt_match thead_reset_match[] = {
> > -     { .compatible = "thead,reset-sample" },
> > +     { .compatible = "thead,common-cpu-reset" },
> > +     { .compatible = "thead,th1520-cpu-reset" },
> >       { },
> >  };
> >
> > --
> > 2.36.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi



-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On Wed, May 24, 2023 at 09:17:11AM +0800, Guo Ren wrote:
> On Tue, May 23, 2023 at 11:51?PM Conor Dooley <conor@kernel.org> wrote:
> > On Tue, May 23, 2023 at 05:46:49AM -0400, guoren at kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Correct the naming convention to fit Linux kernel upstream rule.
> >
> > Understatement of the year contender? ;)
> >
> > This is a backwards incompatible change, based on suggestions that I
> > made on LKML, with a view to creating a yaml binding for this thing.
> > Everyone else might disagree with me, but I think "proper" bindings
> > should be written for this stuff so that use would be common across SBI
> > providers etc *but* in doing so do you not want to keep OpenSBI
> > backwards compatible with the markdown "binding" in the process?
> > Skimming this patch, the old way of doing things would no longer work?
> There is no upstreamed usage of the old way, which used in old frozen
> version of opensbi & custom linux. So I can accept the change in the
> new version opensbi & Linux.

Okay, cool.

> > > Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > Cc: Conor Dooley <conor.dooley@microchip.com>
> > > Cc: Jisheng Zhang <jszhang@kernel.org>
> > > Cc: Wei Fu <wefu@redhat.com>
> > > ---
> > >  docs/platform/thead-c9xx.md       | 21 +++++-------
> > >  lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
> > >  2 files changed, 35 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> > > index 8bb9e91f1a9b..35cca94b5bb9 100644
> > > --- a/docs/platform/thead-c9xx.md
> > > +++ b/docs/platform/thead-c9xx.md
> > > @@ -19,9 +19,6 @@ because it uses generic platform.
> > >  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> > >  ```
> > >
> > > -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> > > -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> > > -
> > >  DTS Example1: (Single core, eg: Allwinner D1 - c906)
> > >  ----------------------------------------------------
> > >
> > > @@ -75,8 +72,8 @@ DTS Example1: (Single core, eg: Allwinner D1 - c906)
> > >       }
> > >  ```
> > >
> > > -DTS Example2: (Multi cores with soc reset-regs)
> > > ------------------------------------------------
> > > +DTS Example2: (Multi cores, eg: T-HEAD th1520 - c910)
> > > +-----------------------------------------------------
> > >
> > >  ```
> > >       cpus {
> > > @@ -143,13 +140,11 @@ DTS Example2: (Multi cores with soc reset-regs)
> > >               compatible = "simple-bus";
> > >               ranges;
> > >
> > > -             reset: reset-sample {
> > > -                     compatible = "thead,reset-sample";
> > > -                     entry-reg = <0xff 0xff019050>;
> > > -                     entry-cnt = <4>;
> > > -                     control-reg = <0xff 0xff015004>;
> > > -                     control-val = <0x1c>;
> > > -                     csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > > +             reset-controller at ffff019050 {
> > > +                     compatible = "thead,th1520-cpu-reset";
> > > +                     reg = <0xff 0xff019050 0x0 0x4>, <0xff 0xff015004 0x0 0x0>;
> > > +                     reset-ctrl-val = <0x1c>;
> > > +                     csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc 0x7ce>;
> >
> > I also did not suggest either rest-ctrl-val or csr-copy, as both are
> > surely able to be determined from match data based on the compatible
> > string.
> I want to keep them in dts to be modified flexiable. This driver is
> not only for th1520, but for bunches of  T-HEAD SoCs, th1520 is a
> sample/example for us.
> 
> Of cause, some of vendors would develop their own chip reset flow
> (Allwinner, Sopho), but this one would be easier for hardware guys to
> modify dtb without compling some stuffs.

I am not a maintainer of OpenSBI, but I would differentiate "people
doing SoC bringup work" who can totally have hacked together things
that read these values from properties in a dtb & what should be
upstreamed/shipped.
When you, or someone else, asks me to review some dt stuff I am going
to only consider the latter. You are always going to have hacked
together things that you use during SoC bringup - the dts is probably
only the start.

My understanding is that Allwinner and Sopho are SoC vendors, so all
they have to do is add a new compatible string when they settle on their
final magic values & pull the information out of match data. So Sopho
would do something like "sopho,magic-carpet-cpu-reset" etc etc.

> Does the "reset-ctrl-val & csr-copy" violate any rule of dts?

"reset-ctrl-val" certainly goes against "do not put register values in
the dts". Both of them go against adding special properties for things
that are known based on the compatible string.

Cheers,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230524/02eea703/attachment.sig>

[RFC PATCH 0/2] lib: reset: thead: Correct dts naming convention

[Anup Patel]

On Tue, May 23, 2023 at 3:17?PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> Correct the naming convention to fit Linux kernel upstream rule.
>
> Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/

PATCH1 fine so that can be taken but for PATCH2 we should
wait for  DT bindings to be accepted.

Regards,
Anup

>
> Guo Ren (2):
>   lib: reset: thead: Remove unnecessary fence operations
>   lib: reset: thead: Correct the naming convention of dts
>
>  docs/platform/thead-c9xx.md           | 21 ++++------
>  lib/utils/reset/fdt_reset_thead.c     | 58 +++++++++++++--------------
>  lib/utils/reset/fdt_reset_thead_asm.S |  2 -
>  3 files changed, 35 insertions(+), 46 deletions(-)
>
> --
> 2.36.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[RFC PATCH 0/2] lib: reset: thead: Correct dts naming convention

[Guo Ren]

On Wed, May 24, 2023 at 3:30?PM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, May 23, 2023 at 3:17?PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Correct the naming convention to fit Linux kernel upstream rule.
> >
> > Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/
>
> PATCH1 fine so that can be taken but for PATCH2 we should
> wait for  DT bindings to be accepted.
Okay, thx.

>
> Regards,
> Anup
>
> >
> > Guo Ren (2):
> >   lib: reset: thead: Remove unnecessary fence operations
> >   lib: reset: thead: Correct the naming convention of dts
> >
> >  docs/platform/thead-c9xx.md           | 21 ++++------
> >  lib/utils/reset/fdt_reset_thead.c     | 58 +++++++++++++--------------
> >  lib/utils/reset/fdt_reset_thead_asm.S |  2 -
> >  3 files changed, 35 insertions(+), 46 deletions(-)
> >
> > --
> > 2.36.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi



-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Wed, May 24, 2023 at 2:43?PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Wed, May 24, 2023 at 09:17:11AM +0800, Guo Ren wrote:
> > On Tue, May 23, 2023 at 11:51?PM Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, May 23, 2023 at 05:46:49AM -0400, guoren at kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > Correct the naming convention to fit Linux kernel upstream rule.
> > >
> > > Understatement of the year contender? ;)
> > >
> > > This is a backwards incompatible change, based on suggestions that I
> > > made on LKML, with a view to creating a yaml binding for this thing.
> > > Everyone else might disagree with me, but I think "proper" bindings
> > > should be written for this stuff so that use would be common across SBI
> > > providers etc *but* in doing so do you not want to keep OpenSBI
> > > backwards compatible with the markdown "binding" in the process?
> > > Skimming this patch, the old way of doing things would no longer work?
> > There is no upstreamed usage of the old way, which used in old frozen
> > version of opensbi & custom linux. So I can accept the change in the
> > new version opensbi & Linux.
>
> Okay, cool.
>
> > > > Link: https://lore.kernel.org/linux-riscv/CAJF2gTRG5edPnVmhMtv67OANpOynL0br0wcrQCave88_KkyZcg at mail.gmail.com/
> > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > Cc: Conor Dooley <conor.dooley@microchip.com>
> > > > Cc: Jisheng Zhang <jszhang@kernel.org>
> > > > Cc: Wei Fu <wefu@redhat.com>
> > > > ---
> > > >  docs/platform/thead-c9xx.md       | 21 +++++-------
> > > >  lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
> > > >  2 files changed, 35 insertions(+), 43 deletions(-)
> > > >
> > > > diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> > > > index 8bb9e91f1a9b..35cca94b5bb9 100644
> > > > --- a/docs/platform/thead-c9xx.md
> > > > +++ b/docs/platform/thead-c9xx.md
> > > > @@ -19,9 +19,6 @@ because it uses generic platform.
> > > >  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> > > >  ```
> > > >
> > > > -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> > > > -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> > > > -
> > > >  DTS Example1: (Single core, eg: Allwinner D1 - c906)
> > > >  ----------------------------------------------------
> > > >
> > > > @@ -75,8 +72,8 @@ DTS Example1: (Single core, eg: Allwinner D1 - c906)
> > > >       }
> > > >  ```
> > > >
> > > > -DTS Example2: (Multi cores with soc reset-regs)
> > > > ------------------------------------------------
> > > > +DTS Example2: (Multi cores, eg: T-HEAD th1520 - c910)
> > > > +-----------------------------------------------------
> > > >
> > > >  ```
> > > >       cpus {
> > > > @@ -143,13 +140,11 @@ DTS Example2: (Multi cores with soc reset-regs)
> > > >               compatible = "simple-bus";
> > > >               ranges;
> > > >
> > > > -             reset: reset-sample {
> > > > -                     compatible = "thead,reset-sample";
> > > > -                     entry-reg = <0xff 0xff019050>;
> > > > -                     entry-cnt = <4>;
> > > > -                     control-reg = <0xff 0xff015004>;
> > > > -                     control-val = <0x1c>;
> > > > -                     csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > > > +             reset-controller at ffff019050 {
> > > > +                     compatible = "thead,th1520-cpu-reset";
> > > > +                     reg = <0xff 0xff019050 0x0 0x4>, <0xff 0xff015004 0x0 0x0>;
> > > > +                     reset-ctrl-val = <0x1c>;
> > > > +                     csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc 0x7ce>;
> > >
> > > I also did not suggest either rest-ctrl-val or csr-copy, as both are
> > > surely able to be determined from match data based on the compatible
> > > string.
> > I want to keep them in dts to be modified flexiable. This driver is
> > not only for th1520, but for bunches of  T-HEAD SoCs, th1520 is a
> > sample/example for us.
> >
> > Of cause, some of vendors would develop their own chip reset flow
> > (Allwinner, Sopho), but this one would be easier for hardware guys to
> > modify dtb without compling some stuffs.
>
> I am not a maintainer of OpenSBI, but I would differentiate "people
> doing SoC bringup work" who can totally have hacked together things
> that read these values from properties in a dtb & what should be
> upstreamed/shipped.
> When you, or someone else, asks me to review some dt stuff I am going
> to only consider the latter. You are always going to have hacked
> together things that you use during SoC bringup - the dts is probably
> only the start.
>
> My understanding is that Allwinner and Sopho are SoC vendors, so all
> they have to do is add a new compatible string when they settle on their
> final magic values & pull the information out of match data. So Sopho
> would do something like "sopho,magic-carpet-cpu-reset" etc etc.
>
> > Does the "reset-ctrl-val & csr-copy" violate any rule of dts?
>
> "reset-ctrl-val" certainly goes against "do not put register values in
> the dts". Both of them go against adding special properties for things
> that are known based on the compatible string.
We remove reset-ctrl-val, but still keep csr-copy. Because it contains
the index, not value. It didn't viloate anything. okay?

               reset-controller at ffff019050 {
                       compatible = "thead,th1520-cpu-reset";
                       reg = <0xff 0xff019050 0x0 0x4>, <0xff
0xff015004 0x0 0x0>;
                       csr-copy-idx = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
0x7c5 0x7cc 0x7ce>;
                };


>
> Cheers,
> Conor.



-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On Wed, May 24, 2023 at 06:39:47PM +0800, Guo Ren wrote:

> We remove reset-ctrl-val, but still keep csr-copy. Because it contains
> the index, not value. It didn't viloate anything. okay?

If it is set per-soc, which apparently it is, then you don't need to
fill the values into a property to communicate it to software because
you already have a compatible that tells you exactly what implementation
you have. Just like it is normal for a driver to use #defines etc for
register access, since it knows those registers exist on a particular
implementation. I don't know what OpenSBI calls this, but in Linux it
is called match_data.
Either way - we are just going around in circles here :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230524/e2741170/attachment.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Jessica Clarke]

On 24 May 2023, at 15:46, Conor Dooley <conor@kernel.org> wrote:
> 
> On Wed, May 24, 2023 at 06:39:47PM +0800, Guo Ren wrote:
> 
>> We remove reset-ctrl-val, but still keep csr-copy. Because it contains
>> the index, not value. It didn't viloate anything. okay?
> 
> If it is set per-soc, which apparently it is, then you don't need to
> fill the values into a property to communicate it to software because
> you already have a compatible that tells you exactly what implementation
> you have. Just like it is normal for a driver to use #defines etc for
> register access, since it knows those registers exist on a particular
> implementation. I don't know what OpenSBI calls this, but in Linux it
> is called match_data.
> Either way - we are just going around in circles here :)

I don?t actually understand this opinion. As a driver author, I would
much prefer to be able to write a single parameterised driver where the
important parameters com from the FDT, rather than having to figure out
all the parameters myself and put them in a table that gets looked up.
Otherwise you could take this to the extreme and just have the
compatible, with everything else hard-coded in the driver, which gets
you much closer to ACPI?s model, which itself is borrowing from the FDT
model and now adding key-value pairs to _DSD. Take something like
gpio-restart, which has three configurable delays. Should those instead
be done based on a per-SoC compatible? Or take syscon-reboot, which has
configurable offset and mask. Should those be done based on a per-SoC
compatible? Having just the one compatible means the driver can be
written once and not need touching again as new SoCs appear, since the
parameters themselves are in the FDT, but if you instead have a
mycompany,mysoc-syscon-reboot for every SoC then each time a new SoC
appears you need to go add the offset+mask values to your big lookup
table in the driver and use a new kernel. This flexibility to put all
the parameters in the FDT was always something I saw as an advantage of
FDTs over ACPI.

The SoC-specific compatible does have value in case the specific IP
needs identifying to work around quirks, but IMO using that to derive
all the parameters rather than having them in the FDT makes drivers
worse.

Jess

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On Wed, May 24, 2023 at 04:55:04PM +0100, Jessica Clarke wrote:
> On 24 May 2023, at 15:46, Conor Dooley <conor@kernel.org> wrote:
> > 
> > On Wed, May 24, 2023 at 06:39:47PM +0800, Guo Ren wrote:
> > 
> >> We remove reset-ctrl-val, but still keep csr-copy. Because it contains
> >> the index, not value. It didn't viloate anything. okay?
> > 
> > If it is set per-soc, which apparently it is, then you don't need to
> > fill the values into a property to communicate it to software because
> > you already have a compatible that tells you exactly what implementation
> > you have. Just like it is normal for a driver to use #defines etc for
> > register access, since it knows those registers exist on a particular
> > implementation. I don't know what OpenSBI calls this, but in Linux it
> > is called match_data.
> > Either way - we are just going around in circles here :)

> The SoC-specific compatible does have value in case the specific IP
> needs identifying to work around quirks, but IMO using that to derive
> all the parameters rather than having them in the FDT makes drivers
> worse.

The other thing to consider is whether the csr copy property is actually
SoC specific, or varies more/less frequently than that. For example, is
it actually set by the cpu core, rather than the SoC, and a number of SoCs
would use the same values? Or would it vary depending on use-case (AMP
etc).
I tried to do some reading up on the documentation, although I struggled
to find detail in a language that I understand. The documentation that I
did find though, looked like you might want to use the same ones for all
c910s.
I'm certainly not diametrically opposed to this property & adding to a
fallback compatible (something like "thead,c910-cpu-reset"?).
If it actually is the case that the CSRs you want to copy is a property
of the cpu core, you wouldn't even need match data & could copy the
default set of CSRs always*.
And when a real use case comes along that needs a property to set
arbitrary values a specific property can always be added with a fallback
to the base cpu values?
It would desperately need an explanation that is better than "copy these
csrs" though, explaining the which & why, and the binding should enforce
that only valid CSRs for an SoC pass validation.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230524/f2a35f50/attachment-0001.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Thu, May 25, 2023 at 1:27?AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, May 24, 2023 at 04:55:04PM +0100, Jessica Clarke wrote:
> > On 24 May 2023, at 15:46, Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Wed, May 24, 2023 at 06:39:47PM +0800, Guo Ren wrote:
> > >
> > >> We remove reset-ctrl-val, but still keep csr-copy. Because it contains
> > >> the index, not value. It didn't viloate anything. okay?
> > >
> > > If it is set per-soc, which apparently it is, then you don't need to
> > > fill the values into a property to communicate it to software because
> > > you already have a compatible that tells you exactly what implementation
> > > you have. Just like it is normal for a driver to use #defines etc for
> > > register access, since it knows those registers exist on a particular
> > > implementation. I don't know what OpenSBI calls this, but in Linux it
> > > is called match_data.
> > > Either way - we are just going around in circles here :)
>
> > The SoC-specific compatible does have value in case the specific IP
> > needs identifying to work around quirks, but IMO using that to derive
> > all the parameters rather than having them in the FDT makes drivers
> > worse.
>
> The other thing to consider is whether the csr copy property is actually
> SoC specific, or varies more/less frequently than that. For example, is
> it actually set by the cpu core, rather than the SoC, and a number of SoCs
> would use the same values? Or would it vary depending on use-case (AMP
> etc).
> I tried to do some reading up on the documentation, although I struggled
> to find detail in a language that I understand. The documentation that I
> did find though, looked like you might want to use the same ones for all
> c910s.
> I'm certainly not diametrically opposed to this property & adding to a
> fallback compatible (something like "thead,c910-cpu-reset"?).
Yes, but not only for c910, but also for c907/c908/c910/c920/r910 ...

So it could be "thead,cpu-reset", okay?

> If it actually is the case that the CSRs you want to copy is a property
> of the cpu core, you wouldn't even need match data & could copy the
> default set of CSRs always*.
Every SoC has different CSRs arrary setting.

> And when a real use case comes along that needs a property to set
> arbitrary values a specific property can always be added with a fallback
> to the base cpu values?
> It would desperately need an explanation that is better than "copy these
> csrs" though, explaining the which & why, and the binding should enforce
> that only valid CSRs for an SoC pass validation.
The driver just help copy CSRs from primary core to the secondary
cores, the driver don't care about which CSR and what value is. This
is the function provided by this thead,cpu-reset driver.

What you mentioned is not ralted to this driver, it's should be
defined in zsbl or earlier boot loader.

Actually, our core could let SoC vendors define their own custom
CSRs/custom reset values of CSRs, so we don't know what would be added
in the future. Put a array in dts instead of hard-code table is much
more flexiblity.

>


-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Wed, May 24, 2023 at 11:55?PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 24 May 2023, at 15:46, Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, May 24, 2023 at 06:39:47PM +0800, Guo Ren wrote:
> >
> >> We remove reset-ctrl-val, but still keep csr-copy. Because it contains
> >> the index, not value. It didn't viloate anything. okay?
> >
> > If it is set per-soc, which apparently it is, then you don't need to
> > fill the values into a property to communicate it to software because
> > you already have a compatible that tells you exactly what implementation
> > you have. Just like it is normal for a driver to use #defines etc for
> > register access, since it knows those registers exist on a particular
> > implementation. I don't know what OpenSBI calls this, but in Linux it
> > is called match_data.
> > Either way - we are just going around in circles here :)
>
> I don?t actually understand this opinion. As a driver author, I would
> much prefer to be able to write a single parameterised driver where the
> important parameters com from the FDT, rather than having to figure out
> all the parameters myself and put them in a table that gets looked up.
> Otherwise you could take this to the extreme and just have the
> compatible, with everything else hard-coded in the driver, which gets
> you much closer to ACPI?s model, which itself is borrowing from the FDT
> model and now adding key-value pairs to _DSD. Take something like
> gpio-restart, which has three configurable delays. Should those instead
> be done based on a per-SoC compatible? Or take syscon-reboot, which has
> configurable offset and mask. Should those be done based on a per-SoC
> compatible? Having just the one compatible means the driver can be
> written once and not need touching again as new SoCs appear, since the
> parameters themselves are in the FDT, but if you instead have a
> mycompany,mysoc-syscon-reboot for every SoC then each time a new SoC
> appears you need to go add the offset+mask values to your big lookup
> table in the driver and use a new kernel. This flexibility to put all
> the parameters in the FDT was always something I saw as an advantage of
> FDTs over ACPI.
I prefer FDTs to ACPI so much :)

>
> The SoC-specific compatible does have value in case the specific IP
> needs identifying to work around quirks, but IMO using that to derive
> all the parameters rather than having them in the FDT makes drivers
> worse.
Thx sincerely. You've said what I want.

>
> Jess

>


--
Best Regards
 Guo Ren

Re: [RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:

>So it could be "thead,cpu-reset", okay?

As a generic fallback compatible.

>Actually, our core could let SoC vendors define their own custom
>CSRs/custom reset values of CSRs, so we don't know what would be added
>in the future. Put a array in dts instead of hard-code table is much
>more flexiblity.

If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.

Thanks,
Conor.

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
>
>
>
> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
>
> >So it could be "thead,cpu-reset", okay?
>
> As a generic fallback compatible.
>
> >Actually, our core could let SoC vendors define their own custom
> >CSRs/custom reset values of CSRs, so we don't know what would be added
> >in the future. Put a array in dts instead of hard-code table is much
> >more flexiblity.
>
> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
Yes, that is what we want. th1520 is a example.

Thanks for the review and correction! Your help is greatly appreciated
in improving th1520 upstream.

>
> Thanks,
> Conor.



-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

Hi Conor,

Jisheng Zhang would update the Linux yaml patch, here is the final dts
format of the reset controller:

               reset-controller at ffff019050 {
                       compatible = "thead,cpu-reset";
                       reg = <0xff 0xff019050 0x0 0x4>, <0xff
0xff015004 0x0 0x0>;
                       reset-ctrl = <0x1c>;
                       clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
0x7c5 0x7cc 0x7ce>;
                };

The reset-ctrl is used to control different parts of soc, generally, a
bit indicates a reset signal (a core/a interconnect/a subsystem).

On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
>
> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
> >
> >
> >
> > On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
> >
> > >So it could be "thead,cpu-reset", okay?
> >
> > As a generic fallback compatible.
> >
> > >Actually, our core could let SoC vendors define their own custom
> > >CSRs/custom reset values of CSRs, so we don't know what would be added
> > >in the future. Put a array in dts instead of hard-code table is much
> > >more flexiblity.
> >
> > If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
> Yes, that is what we want. th1520 is a example.
>
> Thanks for the review and correction! Your help is greatly appreciated
> in improving th1520 upstream.
>
> >
> > Thanks,
> > Conor.
>
>
>
> --
> Best Regards
>  Guo Ren



-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Jessica Clarke]

On 12 Jun 2023, at 01:57, Guo Ren <guoren@kernel.org> wrote:
> 
> Hi Conor,
> 
> Jisheng Zhang would update the Linux yaml patch, here is the final dts
> format of the reset controller:
> 
>               reset-controller at ffff019050 {
>                       compatible = "thead,cpu-reset";
>                       reg = <0xff 0xff019050 0x0 0x4>, <0xff
> 0xff015004 0x0 0x0>;
>                       reset-ctrl = <0x1c>;
>                       clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3

This is pretty horrible, there?s no indirect CSR read/write
instruction, so what are you expecting drivers to do with this? Have a
big switch statement for every possible CSR?

Jess

> 0x7c5 0x7cc 0x7ce>;
>                };
> 
> The reset-ctrl is used to control different parts of soc, generally, a
> bit indicates a reset signal (a core/a interconnect/a subsystem).
> 
> On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
>> 
>> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
>>> 
>>> 
>>> 
>>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
>>> 
>>>> So it could be "thead,cpu-reset", okay?
>>> 
>>> As a generic fallback compatible.
>>> 
>>>> Actually, our core could let SoC vendors define their own custom
>>>> CSRs/custom reset values of CSRs, so we don't know what would be added
>>>> in the future. Put a array in dts instead of hard-code table is much
>>>> more flexiblity.
>>> 
>>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
>> Yes, that is what we want. th1520 is a example.
>> 
>> Thanks for the review and correction! Your help is greatly appreciated
>> in improving th1520 upstream.
>> 
>>> 
>>> Thanks,
>>> Conor.
>> 
>> 
>> 
>> --
>> Best Regards
>> Guo Ren
> 
> 
> 
> -- 
> Best Regards
> Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> Hi Conor,
> 
> Jisheng Zhang would update the Linux yaml patch, here is the final dts
> format of the reset controller:
> 
>                reset-controller at ffff019050 {
>                        compatible = "thead,cpu-reset";
>                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> 0xff015004 0x0 0x0>;
>                        reset-ctrl = <0x1c>;
>                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> 0x7c5 0x7cc 0x7ce>;
>                 };
> 
> The reset-ctrl is used to control different parts of soc, generally, a
> bit indicates a reset signal (a core/a interconnect/a subsystem).

So what is "reset-ctrl"? Values to write into a register? A register
address?
Should this stuff be represented as a proper reset controller, with a
"resets" property in each of the controlled nodes w/ a phandle + index?
It's hard to say with your explanation here :/ Is there some
documentation in English that I could look at? Unfortunately that's only
language I speak that anyone writes technical docs in.

Thanks,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230612/7b279039/attachment-0001.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 12 Jun 2023, at 01:57, Guo Ren <guoren@kernel.org> wrote:
> >
> > Hi Conor,
> >
> > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > format of the reset controller:
> >
> >               reset-controller at ffff019050 {
> >                       compatible = "thead,cpu-reset";
> >                       reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > 0xff015004 0x0 0x0>;
> >                       reset-ctrl = <0x1c>;
> >                       clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
>
> This is pretty horrible, there?s no indirect CSR read/write
> instruction, so what are you expecting drivers to do with this? Have a
We've done it in the opensbi:
https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c

A lot of SoC customers would intergrate their own CSRs though our rtl
code interface, these CSRs are initilized in their primary hart in
early boot stage / jtag gdbinit script, the secondary harts just copy
the CSRs' values from boot hart. That means we could provide a unified
way to all customer.

> big switch statement for every possible CSR?
We needn't big switch statement, boot hart store the csrs setting
codes in the memory, and secondary harts excute them to set the csrs.
Here is the code snippet:

struct custom_csr custom_csr[MAX_CUSTOM_CSR];

#define CSR_OPCODE 0x39073
static void clone_csrs(int cnt)
{
unsigned long i;

for (i = 0; i < cnt; i++) {
/* Write csr BIT[31 - 20] to stub */
__reset_thead_csr_stub[3*i + 1] = CSR_OPCODE | (custom_csr[i].index << 20);

/* Mask csr BIT[31 - 20] */
*(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
smp_mb();

/* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
*(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
smp_mb();

RISCV_FENCE_I;

custom_csr[i].value = __fdt_reset_thead_csrr();
}

}
==========

/* Prepare clone csrs */
val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
if (len > 0 && val_w) {
int cnt;

cnt = len / sizeof(fdt32_t);
if (cnt > MAX_CUSTOM_CSR)
sbi_hart_hang();

for (i = 0; i < cnt; i++) {
custom_csr[i].index = fdt32_to_cpu(val_w[i]);
}

if (cnt)
clone_csrs(cnt); // prepare the secondary harts' excution code
}






>
> Jess
>
> > 0x7c5 0x7cc 0x7ce>;
> >                };
> >
> > The reset-ctrl is used to control different parts of soc, generally, a
> > bit indicates a reset signal (a core/a interconnect/a subsystem).
> >
> > On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
> >>
> >> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
> >>>
> >>>
> >>>
> >>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
> >>>
> >>>> So it could be "thead,cpu-reset", okay?
> >>>
> >>> As a generic fallback compatible.
> >>>
> >>>> Actually, our core could let SoC vendors define their own custom
> >>>> CSRs/custom reset values of CSRs, so we don't know what would be added
> >>>> in the future. Put a array in dts instead of hard-code table is much
> >>>> more flexiblity.
> >>>
> >>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
> >> Yes, that is what we want. th1520 is a example.
> >>
> >> Thanks for the review and correction! Your help is greatly appreciated
> >> in improving th1520 upstream.
> >>
> >>>
> >>> Thanks,
> >>> Conor.
> >>
> >>
> >>
> >> --
> >> Best Regards
> >> Guo Ren
> >
> >
> >
> > --
> > Best Regards
> > Guo Ren
>


-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > Hi Conor,
> >
> > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > format of the reset controller:
> >
> >                reset-controller at ffff019050 {
> >                        compatible = "thead,cpu-reset";
> >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > 0xff015004 0x0 0x0>;
> >                        reset-ctrl = <0x1c>;
> >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > 0x7c5 0x7cc 0x7ce>;
> >                 };
> >
> > The reset-ctrl is used to control different parts of soc, generally, a
> > bit indicates a reset signal (a core/a interconnect/a subsystem).
>
> So what is "reset-ctrl"? Values to write into a register? A register
> address?
It's a values, and every bit indicate a reset signal.

> Should this stuff be represented as a proper reset controller, with a
> "resets" property in each of the controlled nodes w/ a phandle + index?
> It's hard to say with your explanation here :/ Is there some
> documentation in English that I could look at? Unfortunately that's only
> language I speak that anyone writes technical docs in.
Sorry, there is no exact document about reset-ctrl, becasue every SoC
customers has some little difference here.

For SoC intergration, every t-head CPU would give a reset signal, and
SoC customer could intergrate them into their own reset control
register, generally, every bit indicate a reset signal. Some of them
would combine it with their SoC bus reset signal, so we provide a
flexiable/undefined setting value here.

This has been implemented in opensbi, and we have used it many years :)
https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c

val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
if (len > 0 && val) {
p = (void *)(ulong)fdt64_to_cpu(*val);

val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
if (len > 0 && val_w) {
tmp = fdt32_to_cpu(*val_w);
tmp |= readl(p);
writel(tmp, p);
}
}


>
> Thanks,
> Conor.



-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Jessica Clarke]

On 13 Jun 2023, at 03:21, Guo Ren <guoren@kernel.org> wrote:
> 
> On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> 
>> On 12 Jun 2023, at 01:57, Guo Ren <guoren@kernel.org> wrote:
>>> 
>>> Hi Conor,
>>> 
>>> Jisheng Zhang would update the Linux yaml patch, here is the final dts
>>> format of the reset controller:
>>> 
>>>              reset-controller at ffff019050 {
>>>                      compatible = "thead,cpu-reset";
>>>                      reg = <0xff 0xff019050 0x0 0x4>, <0xff
>>> 0xff015004 0x0 0x0>;
>>>                      reset-ctrl = <0x1c>;
>>>                      clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
>> 
>> This is pretty horrible, there?s no indirect CSR read/write
>> instruction, so what are you expecting drivers to do with this? Have a
> We've done it in the opensbi:
> https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> 
> A lot of SoC customers would intergrate their own CSRs though our rtl
> code interface, these CSRs are initilized in their primary hart in
> early boot stage / jtag gdbinit script, the secondary harts just copy
> the CSRs' values from boot hart. That means we could provide a unified
> way to all customer.
> 
>> big switch statement for every possible CSR?
> We needn't big switch statement, boot hart store the csrs setting
> codes in the memory, and secondary harts excute them to set the csrs.
> Here is the code snippet:
> 
> struct custom_csr custom_csr[MAX_CUSTOM_CSR];
> 
> #define CSR_OPCODE 0x39073
> static void clone_csrs(int cnt)
> {
> unsigned long i;
> 
> for (i = 0; i < cnt; i++) {
> /* Write csr BIT[31 - 20] to stub */
> __reset_thead_csr_stub[3*i + 1] = CSR_OPCODE | (custom_csr[i].index << 20);
> 
> /* Mask csr BIT[31 - 20] */
> *(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
> smp_mb();
> 
> /* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
> *(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
> smp_mb();
> 
> RISCV_FENCE_I;
> 
> custom_csr[i].value = __fdt_reset_thead_csrr();
> }

So you want your highly-privileged firmware to be dynamically
generating code, with the ability to have a WX mapping in the
first place?

Jess

> }
> ==========
> 
> /* Prepare clone csrs */
> val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
> if (len > 0 && val_w) {
> int cnt;
> 
> cnt = len / sizeof(fdt32_t);
> if (cnt > MAX_CUSTOM_CSR)
> sbi_hart_hang();
> 
> for (i = 0; i < cnt; i++) {
> custom_csr[i].index = fdt32_to_cpu(val_w[i]);
> }
> 
> if (cnt)
> clone_csrs(cnt); // prepare the secondary harts' excution code
> }
> 
> 
> 
> 
> 
> 
>> 
>> Jess
>> 
>>> 0x7c5 0x7cc 0x7ce>;
>>>               };
>>> 
>>> The reset-ctrl is used to control different parts of soc, generally, a
>>> bit indicates a reset signal (a core/a interconnect/a subsystem).
>>> 
>>> On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
>>>> 
>>>> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
>>>>> 
>>>>>> So it could be "thead,cpu-reset", okay?
>>>>> 
>>>>> As a generic fallback compatible.
>>>>> 
>>>>>> Actually, our core could let SoC vendors define their own custom
>>>>>> CSRs/custom reset values of CSRs, so we don't know what would be added
>>>>>> in the future. Put a array in dts instead of hard-code table is much
>>>>>> more flexiblity.
>>>>> 
>>>>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
>>>> Yes, that is what we want. th1520 is a example.
>>>> 
>>>> Thanks for the review and correction! Your help is greatly appreciated
>>>> in improving th1520 upstream.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Conor.
>>>> 
>>>> 
>>>> 
>>>> --
>>>> Best Regards
>>>> Guo Ren
>>> 
>>> 
>>> 
>>> --
>>> Best Regards
>>> Guo Ren
>> 
> 
> 
> -- 
> Best Regards
> Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

Hey Guo Ren,

On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > format of the reset controller:
> > >
> > >                reset-controller at ffff019050 {
> > >                        compatible = "thead,cpu-reset";

Also, please just add the soc-specific compatibles even if the driver
will bind against the most common form of it.

> > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > 0xff015004 0x0 0x0>;
> > >                        reset-ctrl = <0x1c>;
> > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > 0x7c5 0x7cc 0x7ce>;

To be honest, I don't actually understand why "clone-csrs" is even part
of this "cpu-reset" node. It seems to serve a different purpose to
something that could take various parts of the SoC out of reset.
IMO, the "clone-csrs" stuff is something that should be done based on
the soc-level compatible, and not related to the "cpu-reset" node at
all, which (given the below) sounds more and more like a regular reset
controller.


> > > The reset-ctrl is used to control different parts of soc, generally, a
> > > bit indicates a reset signal (a core/a interconnect/a subsystem).
> >
> > So what is "reset-ctrl"? Values to write into a register? A register
> > address?
> 
> It's a values, and every bit indicate a reset signal.
> 
> > Should this stuff be represented as a proper reset controller, with a
> > "resets" property in each of the controlled nodes w/ a phandle + index?
> > It's hard to say with your explanation here :/ Is there some
> > documentation in English that I could look at? Unfortunately that's only
> > language I speak that anyone writes technical docs in.
> Sorry, there is no exact document about reset-ctrl, becasue every SoC
> customers has some little difference here.
> 
> For SoC intergration, every t-head CPU would give a reset signal, and
> SoC customer could intergrate them into their own reset control
> register, generally, every bit indicate a reset signal. Some of them
> would combine it with their SoC bus reset signal, so we provide a
> flexiable/undefined setting value here.

So TL;DR, highly per-soc specific value, that you cannot explain since
it could mean completely different things, with completely different
behaviour (what registers it even writes to, which buses are affected)

I'm sorry, but I don't see why this shouldn't be split away from the csr
stuff completely, and become a "real" reset controller - especially if
different SoCs using your cores are going to be doing wildly different
things.

Cheers,
Conor.

> This has been implemented in opensbi, and we have used it many years :)
> https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> 
> val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
> if (len > 0 && val) {
> p = (void *)(ulong)fdt64_to_cpu(*val);
> 
> val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
> if (len > 0 && val_w) {
> tmp = fdt32_to_cpu(*val_w);
> tmp |= readl(p);
> writel(tmp, p);
> }
> }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230613/aafd8ad0/attachment.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Tue, Jun 13, 2023 at 10:43?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 13 Jun 2023, at 03:21, Guo Ren <guoren@kernel.org> wrote:
> >
> > On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>
> >> On 12 Jun 2023, at 01:57, Guo Ren <guoren@kernel.org> wrote:
> >>>
> >>> Hi Conor,
> >>>
> >>> Jisheng Zhang would update the Linux yaml patch, here is the final dts
> >>> format of the reset controller:
> >>>
> >>>              reset-controller at ffff019050 {
> >>>                      compatible = "thead,cpu-reset";
> >>>                      reg = <0xff 0xff019050 0x0 0x4>, <0xff
> >>> 0xff015004 0x0 0x0>;
> >>>                      reset-ctrl = <0x1c>;
> >>>                      clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> >>
> >> This is pretty horrible, there?s no indirect CSR read/write
> >> instruction, so what are you expecting drivers to do with this? Have a
> > We've done it in the opensbi:
> > https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> >
> > A lot of SoC customers would intergrate their own CSRs though our rtl
> > code interface, these CSRs are initilized in their primary hart in
> > early boot stage / jtag gdbinit script, the secondary harts just copy
> > the CSRs' values from boot hart. That means we could provide a unified
> > way to all customer.
> >
> >> big switch statement for every possible CSR?
> > We needn't big switch statement, boot hart store the csrs setting
> > codes in the memory, and secondary harts excute them to set the csrs.
> > Here is the code snippet:
> >
> > struct custom_csr custom_csr[MAX_CUSTOM_CSR];
> >
> > #define CSR_OPCODE 0x39073
> > static void clone_csrs(int cnt)
> > {
> > unsigned long i;
> >
> > for (i = 0; i < cnt; i++) {
> > /* Write csr BIT[31 - 20] to stub */
> > __reset_thead_csr_stub[3*i + 1] = CSR_OPCODE | (custom_csr[i].index << 20);
> >
> > /* Mask csr BIT[31 - 20] */
> > *(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
> > smp_mb();
> >
> > /* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
> > *(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
> > smp_mb();
> >
> > RISCV_FENCE_I;
> >
> > custom_csr[i].value = __fdt_reset_thead_csrr();
> > }
>
> So you want your highly-privileged firmware to be dynamically
> generating code, with the ability to have a WX mapping in the
> first place?
We don't expect WX mapping, but the current fdt_reset_init is after
the pmp config. I need to move it into early_init. Thx for pointing it
out.



>
> Jess
>
> > }
> > ==========
> >
> > /* Prepare clone csrs */
> > val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
> > if (len > 0 && val_w) {
> > int cnt;
> >
> > cnt = len / sizeof(fdt32_t);
> > if (cnt > MAX_CUSTOM_CSR)
> > sbi_hart_hang();
> >
> > for (i = 0; i < cnt; i++) {
> > custom_csr[i].index = fdt32_to_cpu(val_w[i]);
> > }
> >
> > if (cnt)
> > clone_csrs(cnt); // prepare the secondary harts' excution code
> > }
> >
> >
> >
> >
> >
> >
> >>
> >> Jess
> >>
> >>> 0x7c5 0x7cc 0x7ce>;
> >>>               };
> >>>
> >>> The reset-ctrl is used to control different parts of soc, generally, a
> >>> bit indicates a reset signal (a core/a interconnect/a subsystem).
> >>>
> >>> On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
> >>>>
> >>>> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
> >>>>>
> >>>>>> So it could be "thead,cpu-reset", okay?
> >>>>>
> >>>>> As a generic fallback compatible.
> >>>>>
> >>>>>> Actually, our core could let SoC vendors define their own custom
> >>>>>> CSRs/custom reset values of CSRs, so we don't know what would be added
> >>>>>> in the future. Put a array in dts instead of hard-code table is much
> >>>>>> more flexiblity.
> >>>>>
> >>>>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
> >>>> Yes, that is what we want. th1520 is a example.
> >>>>
> >>>> Thanks for the review and correction! Your help is greatly appreciated
> >>>> in improving th1520 upstream.
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Conor.
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Best Regards
> >>>> Guo Ren
> >>>
> >>>
> >>>
> >>> --
> >>> Best Regards
> >>> Guo Ren
> >>
> >
> >
> > --
> > Best Regards
> > Guo Ren
>
>


--
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Guo Ren,
>
> On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> > On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > > format of the reset controller:
> > > >
> > > >                reset-controller at ffff019050 {
> > > >                        compatible = "thead,cpu-reset";
>
> Also, please just add the soc-specific compatibles even if the driver
> will bind against the most common form of it.
>
> > > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > > 0xff015004 0x0 0x0>;
> > > >                        reset-ctrl = <0x1c>;
> > > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > > 0x7c5 0x7cc 0x7ce>;
>
> To be honest, I don't actually understand why "clone-csrs" is even part
> of this "cpu-reset" node. It seems to serve a different purpose to
> something that could take various parts of the SoC out of reset.
> IMO, the "clone-csrs" stuff is something that should be done based on
> the soc-level compatible, and not related to the "cpu-reset" node at
> all, which (given the below) sounds more and more like a regular reset
> controller.
We need to put clone-csrs' execution entry into the reset entry (the
first reg address).

        /* Custom reset method for secondary harts */
        val = fdt_getprop(fdt, nodeoff, "entry-reg", &len);
        if (len > 0 && val) {
          p = (char *)(ulong)fdt64_to_cpu(*val);

                val_w = fdt_getprop(fdt, nodeoff, "entry-cnt", &len);
                if (len > 0 && val_w) {
                        tmp = fdt32_to_cpu(*val_w);

                        for (i = 0; i < tmp; i++) {
                                t = (ulong)&__thead_pre_start_warm;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                writel(t, p + (8 * i));
                                t = (u64)(ulong)&__thead_pre_start_warm >> 32;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                writel(t, p + (8 * i) + 4);
                                ^^^^^^^^^^^^^^^^^^^^^
                        }
                }

                val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
                if (len > 0 && val) {
                        p = (void *)(ulong)fdt64_to_cpu(*val);

                        val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
                        if (len > 0 && val_w) {
                                tmp = fdt32_to_cpu(*val_w);
                                tmp |= readl(p);
                                writel(tmp, p);
                        }
                }
        }

I don't think we need to separate it, which made maintenance more complex.

>
>
> > > > The reset-ctrl is used to control different parts of soc, generally, a
> > > > bit indicates a reset signal (a core/a interconnect/a subsystem).
> > >
> > > So what is "reset-ctrl"? Values to write into a register? A register
> > > address?
> >
> > It's a values, and every bit indicate a reset signal.
> >
> > > Should this stuff be represented as a proper reset controller, with a
> > > "resets" property in each of the controlled nodes w/ a phandle + index?
> > > It's hard to say with your explanation here :/ Is there some
> > > documentation in English that I could look at? Unfortunately that's only
> > > language I speak that anyone writes technical docs in.
> > Sorry, there is no exact document about reset-ctrl, becasue every SoC
> > customers has some little difference here.
> >
> > For SoC intergration, every t-head CPU would give a reset signal, and
> > SoC customer could intergrate them into their own reset control
> > register, generally, every bit indicate a reset signal. Some of them
> > would combine it with their SoC bus reset signal, so we provide a
> > flexiable/undefined setting value here.
>
> So TL;DR, highly per-soc specific value, that you cannot explain since
> it could mean completely different things, with completely different
> behaviour (what registers it even writes to, which buses are affected)
>
> I'm sorry, but I don't see why this shouldn't be split away from the csr
> stuff completely, and become a "real" reset controller - especially if
> different SoCs using your cores are going to be doing wildly different
> things.
>
> Cheers,
> Conor.
>
> > This has been implemented in opensbi, and we have used it many years :)
> > https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> >
> > val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
> > if (len > 0 && val) {
> > p = (void *)(ulong)fdt64_to_cpu(*val);
> >
> > val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
> > if (len > 0 && val_w) {
> > tmp = fdt32_to_cpu(*val_w);
> > tmp |= readl(p);
> > writel(tmp, p);
> > }
> > }
>


-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Guo Ren,
>
> On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> > On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > > format of the reset controller:
> > > >
> > > >                reset-controller at ffff019050 {
> > > >                        compatible = "thead,cpu-reset";
>
> Also, please just add the soc-specific compatibles even if the driver
> will bind against the most common form of it.
compatible = "thead,th1520-cpu-reset";

Correct?

>
> > > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > > 0xff015004 0x0 0x0>;
> > > >                        reset-ctrl = <0x1c>;
> > > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > > 0x7c5 0x7cc 0x7ce>;
>
> To be honest, I don't actually understand why "clone-csrs" is even part
> of this "cpu-reset" node. It seems to serve a different purpose to
> something that could take various parts of the SoC out of reset.
> IMO, the "clone-csrs" stuff is something that should be done based on
> the soc-level compatible, and not related to the "cpu-reset" node at
> all, which (given the below) sounds more and more like a regular reset
> controller.
>
>
> > > > The reset-ctrl is used to control different parts of soc, generally, a
> > > > bit indicates a reset signal (a core/a interconnect/a subsystem).
> > >
> > > So what is "reset-ctrl"? Values to write into a register? A register
> > > address?
> >
> > It's a values, and every bit indicate a reset signal.
> >
> > > Should this stuff be represented as a proper reset controller, with a
> > > "resets" property in each of the controlled nodes w/ a phandle + index?
> > > It's hard to say with your explanation here :/ Is there some
> > > documentation in English that I could look at? Unfortunately that's only
> > > language I speak that anyone writes technical docs in.
> > Sorry, there is no exact document about reset-ctrl, becasue every SoC
> > customers has some little difference here.
> >
> > For SoC intergration, every t-head CPU would give a reset signal, and
> > SoC customer could intergrate them into their own reset control
> > register, generally, every bit indicate a reset signal. Some of them
> > would combine it with their SoC bus reset signal, so we provide a
> > flexiable/undefined setting value here.
>
> So TL;DR, highly per-soc specific value, that you cannot explain since
> it could mean completely different things, with completely different
> behaviour (what registers it even writes to, which buses are affected)
>
> I'm sorry, but I don't see why this shouldn't be split away from the csr
> stuff completely, and become a "real" reset controller - especially if
> different SoCs using your cores are going to be doing wildly different
> things.
>
> Cheers,
> Conor.
>
> > This has been implemented in opensbi, and we have used it many years :)
> > https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> >
> > val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
> > if (len > 0 && val) {
> > p = (void *)(ulong)fdt64_to_cpu(*val);
> >
> > val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
> > if (len > 0 && val_w) {
> > tmp = fdt32_to_cpu(*val_w);
> > tmp |= readl(p);
> > writel(tmp, p);
> > }
> > }
>


-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Jessica Clarke]

On 14 Jun 2023, at 02:07, Guo Ren <guoren@kernel.org> wrote:
> 
> On Tue, Jun 13, 2023 at 10:43?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> 
>> On 13 Jun 2023, at 03:21, Guo Ren <guoren@kernel.org> wrote:
>>> 
>>> On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>>> 
>>>> On 12 Jun 2023, at 01:57, Guo Ren <guoren@kernel.org> wrote:
>>>>> 
>>>>> Hi Conor,
>>>>> 
>>>>> Jisheng Zhang would update the Linux yaml patch, here is the final dts
>>>>> format of the reset controller:
>>>>> 
>>>>>             reset-controller at ffff019050 {
>>>>>                     compatible = "thead,cpu-reset";
>>>>>                     reg = <0xff 0xff019050 0x0 0x4>, <0xff
>>>>> 0xff015004 0x0 0x0>;
>>>>>                     reset-ctrl = <0x1c>;
>>>>>                     clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
>>>> 
>>>> This is pretty horrible, there?s no indirect CSR read/write
>>>> instruction, so what are you expecting drivers to do with this? Have a
>>> We've done it in the opensbi:
>>> https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
>>> 
>>> A lot of SoC customers would intergrate their own CSRs though our rtl
>>> code interface, these CSRs are initilized in their primary hart in
>>> early boot stage / jtag gdbinit script, the secondary harts just copy
>>> the CSRs' values from boot hart. That means we could provide a unified
>>> way to all customer.
>>> 
>>>> big switch statement for every possible CSR?
>>> We needn't big switch statement, boot hart store the csrs setting
>>> codes in the memory, and secondary harts excute them to set the csrs.
>>> Here is the code snippet:
>>> 
>>> struct custom_csr custom_csr[MAX_CUSTOM_CSR];
>>> 
>>> #define CSR_OPCODE 0x39073
>>> static void clone_csrs(int cnt)
>>> {
>>> unsigned long i;
>>> 
>>> for (i = 0; i < cnt; i++) {
>>> /* Write csr BIT[31 - 20] to stub */
>>> __reset_thead_csr_stub[3*i + 1] = CSR_OPCODE | (custom_csr[i].index << 20);
>>> 
>>> /* Mask csr BIT[31 - 20] */
>>> *(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
>>> smp_mb();
>>> 
>>> /* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
>>> *(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
>>> smp_mb();
>>> 
>>> RISCV_FENCE_I;
>>> 
>>> custom_csr[i].value = __fdt_reset_thead_csrr();
>>> }
>> 
>> So you want your highly-privileged firmware to be dynamically
>> generating code, with the ability to have a WX mapping in the
>> first place?
> We don't expect WX mapping, but the current fdt_reset_init is after
> the pmp config. I need to move it into early_init. Thx for pointing it
> out.

So you didn?t test your code?

Besides, my point was not about correctness, but about the security
concerns that come with generating code at run time. If you don?t need
to do it, don?t.

Jess

>>> }
>>> ==========
>>> 
>>> /* Prepare clone csrs */
>>> val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
>>> if (len > 0 && val_w) {
>>> int cnt;
>>> 
>>> cnt = len / sizeof(fdt32_t);
>>> if (cnt > MAX_CUSTOM_CSR)
>>> sbi_hart_hang();
>>> 
>>> for (i = 0; i < cnt; i++) {
>>> custom_csr[i].index = fdt32_to_cpu(val_w[i]);
>>> }
>>> 
>>> if (cnt)
>>> clone_csrs(cnt); // prepare the secondary harts' excution code
>>> }
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>>> 
>>>> Jess
>>>> 
>>>>> 0x7c5 0x7cc 0x7ce>;
>>>>>              };
>>>>> 
>>>>> The reset-ctrl is used to control different parts of soc, generally, a
>>>>> bit indicates a reset signal (a core/a interconnect/a subsystem).
>>>>> 
>>>>> On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
>>>>>> 
>>>>>> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
>>>>>>> 
>>>>>>>> So it could be "thead,cpu-reset", okay?
>>>>>>> 
>>>>>>> As a generic fallback compatible.
>>>>>>> 
>>>>>>>> Actually, our core could let SoC vendors define their own custom
>>>>>>>> CSRs/custom reset values of CSRs, so we don't know what would be added
>>>>>>>> in the future. Put a array in dts instead of hard-code table is much
>>>>>>>> more flexiblity.
>>>>>>> 
>>>>>>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
>>>>>> Yes, that is what we want. th1520 is a example.
>>>>>> 
>>>>>> Thanks for the review and correction! Your help is greatly appreciated
>>>>>> in improving th1520 upstream.
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Conor.
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> --
>>>>>> Best Regards
>>>>>> Guo Ren
>>>>> 
>>>>> 
>>>>> 
>>>>> --
>>>>> Best Regards
>>>>> Guo Ren
>>>> 
>>> 
>>> 
>>> --
>>> Best Regards
>>> Guo Ren
>> 
>> 
> 
> 
> --
> Best Regards
> Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On Wed, Jun 14, 2023 at 09:11:01AM +0800, Guo Ren wrote:
> On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <conor@kernel.org> wrote:
> > On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> > > On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > > > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > > > format of the reset controller:

@Jisheng, iirc this was the only real outstanding thing for your Linux
patchset, if you re-submit without the reset controller this week it
should still be in time for v6.5.

> > > > >                reset-controller at ffff019050 {
> > > > >                        compatible = "thead,cpu-reset";
> >
> > Also, please just add the soc-specific compatibles even if the driver
> > will bind against the most common form of it.
> >
> > > > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > > > 0xff015004 0x0 0x0>;
> > > > >                        reset-ctrl = <0x1c>;
> > > > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > > > 0x7c5 0x7cc 0x7ce>;
> >
> > To be honest, I don't actually understand why "clone-csrs" is even part
> > of this "cpu-reset" node. It seems to serve a different purpose to
> > something that could take various parts of the SoC out of reset.
> > IMO, the "clone-csrs" stuff is something that should be done based on
> > the soc-level compatible, and not related to the "cpu-reset" node at
> > all, which (given the below) sounds more and more like a regular reset
> > controller.

> We need to put clone-csrs' execution entry into the reset entry (the
> first reg address).

So, take the first reg out too then, with the clone-csrs bits?

That'd leave you with a "regular" reset controller for bits that reset
various parts of the SoC & the clone-csrs stuff can be its own thing.

What am I missing?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230614/257e6f15/attachment.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Wed, Jun 14, 2023 at 9:25?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 14 Jun 2023, at 02:07, Guo Ren <guoren@kernel.org> wrote:
> >
> > On Tue, Jun 13, 2023 at 10:43?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>
> >> On 13 Jun 2023, at 03:21, Guo Ren <guoren@kernel.org> wrote:
> >>>
> >>> On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>>>
> >>>> On 12 Jun 2023, at 01:57, Guo Ren <guoren@kernel.org> wrote:
> >>>>>
> >>>>> Hi Conor,
> >>>>>
> >>>>> Jisheng Zhang would update the Linux yaml patch, here is the final dts
> >>>>> format of the reset controller:
> >>>>>
> >>>>>             reset-controller at ffff019050 {
> >>>>>                     compatible = "thead,cpu-reset";
> >>>>>                     reg = <0xff 0xff019050 0x0 0x4>, <0xff
> >>>>> 0xff015004 0x0 0x0>;
> >>>>>                     reset-ctrl = <0x1c>;
> >>>>>                     clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> >>>>
> >>>> This is pretty horrible, there?s no indirect CSR read/write
> >>>> instruction, so what are you expecting drivers to do with this? Have a
> >>> We've done it in the opensbi:
> >>> https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> >>>
> >>> A lot of SoC customers would intergrate their own CSRs though our rtl
> >>> code interface, these CSRs are initilized in their primary hart in
> >>> early boot stage / jtag gdbinit script, the secondary harts just copy
> >>> the CSRs' values from boot hart. That means we could provide a unified
> >>> way to all customer.
> >>>
> >>>> big switch statement for every possible CSR?
> >>> We needn't big switch statement, boot hart store the csrs setting
> >>> codes in the memory, and secondary harts excute them to set the csrs.
> >>> Here is the code snippet:
> >>>
> >>> struct custom_csr custom_csr[MAX_CUSTOM_CSR];
> >>>
> >>> #define CSR_OPCODE 0x39073
> >>> static void clone_csrs(int cnt)
> >>> {
> >>> unsigned long i;
> >>>
> >>> for (i = 0; i < cnt; i++) {
> >>> /* Write csr BIT[31 - 20] to stub */
> >>> __reset_thead_csr_stub[3*i + 1] = CSR_OPCODE | (custom_csr[i].index << 20);
> >>>
> >>> /* Mask csr BIT[31 - 20] */
> >>> *(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
> >>> smp_mb();
> >>>
> >>> /* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
> >>> *(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
> >>> smp_mb();
> >>>
> >>> RISCV_FENCE_I;
> >>>
> >>> custom_csr[i].value = __fdt_reset_thead_csrr();
> >>> }
> >>
> >> So you want your highly-privileged firmware to be dynamically
> >> generating code, with the ability to have a WX mapping in the
> >> first place?
> > We don't expect WX mapping, but the current fdt_reset_init is after
> > the pmp config. I need to move it into early_init. Thx for pointing it
> > out.
>
> So you didn?t test your code?
We've used it for 2 years, it could satifies us to deliver our CPU
core to the SoC vendors.

Current pmp didn't support WX mapping for m-mode, we force ARWX for
all memory regions for m-mode. See:

Priv-isa-spec
3.7.1.2. Locking and Privilege Mode
When the L bit is clear, any M-mode access matching the PMP entry will
succeed; the R/W/X permissions apply only to S and U modes.

>
> Besides, my point was not about correctness, but about the security
> concerns that come with generating code at run time. If you don?t need
> to do it, don?t.
I agree to prevent generating code at run time, but this advice is not
related to my case.
I found my reset_fdt_init is after pmp_configure, and I plan to move
it before it. Next, when we update to ehanced PMP ISA, the root domain
region1 could be real READ & EXECUTE permission, we can froze the text
region.

>
> Jess
>
> >>> }
> >>> ==========
> >>>
> >>> /* Prepare clone csrs */
> >>> val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
> >>> if (len > 0 && val_w) {
> >>> int cnt;
> >>>
> >>> cnt = len / sizeof(fdt32_t);
> >>> if (cnt > MAX_CUSTOM_CSR)
> >>> sbi_hart_hang();
> >>>
> >>> for (i = 0; i < cnt; i++) {
> >>> custom_csr[i].index = fdt32_to_cpu(val_w[i]);
> >>> }
> >>>
> >>> if (cnt)
> >>> clone_csrs(cnt); // prepare the secondary harts' excution code
> >>> }
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>>
> >>>> Jess
> >>>>
> >>>>> 0x7c5 0x7cc 0x7ce>;
> >>>>>              };
> >>>>>
> >>>>> The reset-ctrl is used to control different parts of soc, generally, a
> >>>>> bit indicates a reset signal (a core/a interconnect/a subsystem).
> >>>>>
> >>>>> On Thu, May 25, 2023 at 2:06?PM Guo Ren <guoren@kernel.org> wrote:
> >>>>>>
> >>>>>> On Thu, May 25, 2023 at 1:33?PM Conor Dooley <conor@kernel.org> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren@kernel.org> wrote:
> >>>>>>>
> >>>>>>>> So it could be "thead,cpu-reset", okay?
> >>>>>>>
> >>>>>>> As a generic fallback compatible.
> >>>>>>>
> >>>>>>>> Actually, our core could let SoC vendors define their own custom
> >>>>>>>> CSRs/custom reset values of CSRs, so we don't know what would be added
> >>>>>>>> in the future. Put a array in dts instead of hard-code table is much
> >>>>>>>> more flexiblity.
> >>>>>>>
> >>>>>>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
> >>>>>> Yes, that is what we want. th1520 is a example.
> >>>>>>
> >>>>>> Thanks for the review and correction! Your help is greatly appreciated
> >>>>>> in improving th1520 upstream.
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Conor.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Best Regards
> >>>>>> Guo Ren
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Best Regards
> >>>>> Guo Ren
> >>>>
> >>>
> >>>
> >>> --
> >>> Best Regards
> >>> Guo Ren
> >>
> >>
> >
> >
> > --
> > Best Regards
> > Guo Ren
>
>


--
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Wed, Jun 14, 2023 at 2:56?PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Wed, Jun 14, 2023 at 09:11:01AM +0800, Guo Ren wrote:
> > On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> > > > On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > > > > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > > > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > > > > format of the reset controller:
>
> @Jisheng, iirc this was the only real outstanding thing for your Linux
> patchset, if you re-submit without the reset controller this week it
> should still be in time for v6.5.
I also agree let others go head first, thx Conor.

>
> > > > > >                reset-controller at ffff019050 {
> > > > > >                        compatible = "thead,cpu-reset";
> > >
> > > Also, please just add the soc-specific compatibles even if the driver
> > > will bind against the most common form of it.
> > >
> > > > > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > > > > 0xff015004 0x0 0x0>;
> > > > > >                        reset-ctrl = <0x1c>;
> > > > > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > > > > 0x7c5 0x7cc 0x7ce>;
> > >
> > > To be honest, I don't actually understand why "clone-csrs" is even part
> > > of this "cpu-reset" node. It seems to serve a different purpose to
> > > something that could take various parts of the SoC out of reset.
> > > IMO, the "clone-csrs" stuff is something that should be done based on
> > > the soc-level compatible, and not related to the "cpu-reset" node at
> > > all, which (given the below) sounds more and more like a regular reset
> > > controller.
>
> > We need to put clone-csrs' execution entry into the reset entry (the
> > first reg address).
>
> So, take the first reg out too then, with the clone-csrs bits?
>
> That'd leave you with a "regular" reset controller for bits that reset
> various parts of the SoC & the clone-csrs stuff can be its own thing.
>
> What am I missing?

+               tmp = (ulong)&__thead_pre_start_warm;
+               writel(tmp, (char *)(ulong)reg_addr + (i * 8));
+               tmp = (u64)(ulong)&__thead_pre_start_warm >> 32;
+               writel(tmp, (char *)(ulong)reg_addr + (i * 8) + 4);
We write __thead_pre_start_warm into entry-reg, and the
__thead_pre_start_warm is:
        .align 3
        .global __thead_pre_start_warm
__thead_pre_start_warm:
...
        .global __reset_thead_csr_stub
__reset_thead_csr_stub:
.rept   MAX_CUSTOM_CSR
        REG_L   t2, 8(t1)
        CSR_STUB
        addi    t1, t1, 16
.endr
...

The CSR_STUB array contains all clone-csrs array. We modify it during
the early boot.

>


-- 
Best Regards
 Guo Ren

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

On Thu, Jun 15, 2023 at 11:54:38PM +0800, Guo Ren wrote:
> On Wed, Jun 14, 2023 at 2:56?PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Wed, Jun 14, 2023 at 09:11:01AM +0800, Guo Ren wrote:
> > > On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <conor@kernel.org> wrote:
> > > > On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> > > > > On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > > > > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > > > > > format of the reset controller:
> >
> > @Jisheng, iirc this was the only real outstanding thing for your Linux
> > patchset, if you re-submit without the reset controller this week it
> > should still be in time for v6.5.
> I also agree let others go head first, thx Conor.
> 
> >
> > > > > > >                reset-controller at ffff019050 {
> > > > > > >                        compatible = "thead,cpu-reset";
> > > >
> > > > Also, please just add the soc-specific compatibles even if the driver
> > > > will bind against the most common form of it.
> > > >
> > > > > > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > > > > > 0xff015004 0x0 0x0>;
> > > > > > >                        reset-ctrl = <0x1c>;
> > > > > > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > > > > > 0x7c5 0x7cc 0x7ce>;
> > > >
> > > > To be honest, I don't actually understand why "clone-csrs" is even part
> > > > of this "cpu-reset" node. It seems to serve a different purpose to
> > > > something that could take various parts of the SoC out of reset.
> > > > IMO, the "clone-csrs" stuff is something that should be done based on
> > > > the soc-level compatible, and not related to the "cpu-reset" node at
> > > > all, which (given the below) sounds more and more like a regular reset
> > > > controller.
> >
> > > We need to put clone-csrs' execution entry into the reset entry (the
> > > first reg address).
> >
> > So, take the first reg out too then, with the clone-csrs bits?
> >
> > That'd leave you with a "regular" reset controller for bits that reset
> > various parts of the SoC & the clone-csrs stuff can be its own thing.
> >
> > What am I missing?
> 
> +               tmp = (ulong)&__thead_pre_start_warm;
> +               writel(tmp, (char *)(ulong)reg_addr + (i * 8));
> +               tmp = (u64)(ulong)&__thead_pre_start_warm >> 32;
> +               writel(tmp, (char *)(ulong)reg_addr + (i * 8) + 4);
> We write __thead_pre_start_warm into entry-reg, and the
> __thead_pre_start_warm is:
>         .align 3
>         .global __thead_pre_start_warm
> __thead_pre_start_warm:
> ...
>         .global __reset_thead_csr_stub
> __reset_thead_csr_stub:
> .rept   MAX_CUSTOM_CSR
>         REG_L   t2, 8(t1)
>         CSR_STUB
>         addi    t1, t1, 16
> .endr
> ...
> 
> The CSR_STUB array contains all clone-csrs array. We modify it during
> the early boot.

I don't see how this answers my question. That all seems to be about
what you originally called "entry-reg", "entry-cnt" & "csr-copy".
That does not seem like it has almost nothing to do with what were
originally called "control-reg" & "control-val".

Per your descriptions, there appears to be a normal reset controller
(the "control-" bits) and the CSR/hart entry point stuff crammed into
one DT node. I am asking why you do not split what seems to be a regular
reset controller away from the CSR/entry point stuff.

Thanks,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230615/5c41acc6/attachment.sig>

[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Guo Ren]

On Fri, Jun 16, 2023 at 12:46?AM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Jun 15, 2023 at 11:54:38PM +0800, Guo Ren wrote:
> > On Wed, Jun 14, 2023 at 2:56?PM Conor Dooley <conor.dooley@microchip.com> wrote:
> > >
> > > On Wed, Jun 14, 2023 at 09:11:01AM +0800, Guo Ren wrote:
> > > > On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <conor@kernel.org> wrote:
> > > > > On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> > > > > > On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > > > > > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > > > > > > format of the reset controller:
> > >
> > > @Jisheng, iirc this was the only real outstanding thing for your Linux
> > > patchset, if you re-submit without the reset controller this week it
> > > should still be in time for v6.5.
> > I also agree let others go head first, thx Conor.
> >
> > >
> > > > > > > >                reset-controller at ffff019050 {
> > > > > > > >                        compatible = "thead,cpu-reset";
> > > > >
> > > > > Also, please just add the soc-specific compatibles even if the driver
> > > > > will bind against the most common form of it.
> > > > >
> > > > > > > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > > > > > > 0xff015004 0x0 0x0>;
> > > > > > > >                        reset-ctrl = <0x1c>;
> > > > > > > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > > > > > > 0x7c5 0x7cc 0x7ce>;
> > > > >
> > > > > To be honest, I don't actually understand why "clone-csrs" is even part
> > > > > of this "cpu-reset" node. It seems to serve a different purpose to
> > > > > something that could take various parts of the SoC out of reset.
> > > > > IMO, the "clone-csrs" stuff is something that should be done based on
> > > > > the soc-level compatible, and not related to the "cpu-reset" node at
> > > > > all, which (given the below) sounds more and more like a regular reset
> > > > > controller.
> > >
> > > > We need to put clone-csrs' execution entry into the reset entry (the
> > > > first reg address).
> > >
> > > So, take the first reg out too then, with the clone-csrs bits?
> > >
> > > That'd leave you with a "regular" reset controller for bits that reset
> > > various parts of the SoC & the clone-csrs stuff can be its own thing.
> > >
> > > What am I missing?
> >
> > +               tmp = (ulong)&__thead_pre_start_warm;
> > +               writel(tmp, (char *)(ulong)reg_addr + (i * 8));
> > +               tmp = (u64)(ulong)&__thead_pre_start_warm >> 32;
> > +               writel(tmp, (char *)(ulong)reg_addr + (i * 8) + 4);
> > We write __thead_pre_start_warm into entry-reg, and the
> > __thead_pre_start_warm is:
> >         .align 3
> >         .global __thead_pre_start_warm
> > __thead_pre_start_warm:
> > ...
> >         .global __reset_thead_csr_stub
> > __reset_thead_csr_stub:
> > .rept   MAX_CUSTOM_CSR
> >         REG_L   t2, 8(t1)
> >         CSR_STUB
> >         addi    t1, t1, 16
> > .endr
> > ...
> >
> > The CSR_STUB array contains all clone-csrs array. We modify it during
> > the early boot.
>
> I don't see how this answers my question. That all seems to be about
> what you originally called "entry-reg", "entry-cnt" & "csr-copy".
> That does not seem like it has almost nothing to do with what were
> originally called "control-reg" & "control-val".
>
> Per your descriptions, there appears to be a normal reset controller
> (the "control-" bits) and the CSR/hart entry point stuff crammed into
> one DT node. I am asking why you do not split what seems to be a regular
> reset controller away from the CSR/entry point stuff.
They are continuous operating processes for cold boot:

1. prepare __thead_pre_start_warm code with csr-copy
2. write __thead_pre_start_warm into entry-reg
3. write control-val bits into control-reg

Then other secondary harts would reset from __thead_pre_start_warm.

So I really don't know why I need to split them, because no other
modules would use __thead_pre_start_warm.

>
> Thanks,
> Conor.



-- 
Best Regards
 Guo Ren