lib: reset: thead: Correct dts naming convention

From: Guo Ren <[email protected]>

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 <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Cc Conor Dooley <[email protected]>

Cc Jisheng Zhang <[email protected]>

Cc Wei Fu <[email protected]>

---
 docs/platform/thead-c9xx.md       | 21 +++++-------
 lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
 2 files changed, 35 insertions(+), 43 deletions(-)

docs/platform/thead-c9xx.md +2 -7

index 8bb9e91f1a9b..35cca94b5bb9 100644
@@ -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";

docs/platform/thead-c9xx.md +1 -1

 		};
 
 		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

lib/utils/reset/fdt_reset_thead.c +27 -30

index ff7d2981b42f..760c117f6178 100644
@@ -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" },
 	{ },
 };
 

Signature · 7 lines

2.36.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Hey Guo Ren,

Quoted 3 lines · guoren at kernel.org

From: Guo Ren <[email protected]>

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?

Quoted 41 lines

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

Signed-off-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Cc Conor Dooley <[email protected]>

Cc Jisheng Zhang <[email protected]>

Cc Wei Fu <[email protected]>

---
 docs/platform/thead-c9xx.md       | 21 +++++-------
 lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
 2 files changed, 35 insertions(+), 43 deletions(-)

docs/platform/thead-c9xx.md +2 -5

index 8bb9e91f1a9b..35cca94b5bb9 100644
@@ -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;
 

docs/platform/thead-c9xx.md DTS Example2: (Multi cores with soc reset-regs) · line 147

Quoted 12 lines

docs/platform/thead-c9xx.md +5 -7

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

Quoted 116 lines

docs/platform/thead-c9xx.md +1 -1

              };
 
              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

lib/utils/reset/fdt_reset_thead.c +27 -31

index ff7d2981b42f..760c117f6178 100644
@@ -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

Quoted 7 lines · On Tue, May 23, 2023 at 11:51?PM Conor Dooley <[email protected]> wrote:

Quoted 3 lines · guoren at kernel.org

From: Guo Ren <[email protected]>

Correct the naming convention to fit Linux kernel upstream rule.

Understatement of the year contender? ;)

Quoted 7 lines

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.

Quoted 40 lines

Quoted 40 lines

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

Signed-off-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Cc Conor Dooley <[email protected]>

Cc Jisheng Zhang <[email protected]>

Cc Wei Fu <[email protected]>

---
 docs/platform/thead-c9xx.md       | 21 +++++-------
 lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
 2 files changed, 35 insertions(+), 43 deletions(-)

docs/platform/thead-c9xx.md +2 -5

index 8bb9e91f1a9b..35cca94b5bb9 100644
@@ -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;
 

Quoted 16 lines

docs/platform/thead-c9xx.md DTS Example2: (Multi cores with soc reset-regs) · line 147

Quoted 12 lines

docs/platform/thead-c9xx.md +5 -7

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

Quoted 15 lines · Guo Ren

Quoted 7 lines · On Tue, May 23, 2023 at 11:51?PM Conor Dooley <[email protected]> wrote:

Quoted 3 lines · guoren at kernel.org

From: Guo Ren <[email protected]>

Correct the naming convention to fit Linux kernel upstream rule.

Understatement of the year contender? ;)

Quoted 7 lines

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?

Quoted 3 lines

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.

Quoted 56 lines

Quoted 40 lines

Quoted 40 lines

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

Signed-off-by Guo Ren <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Cc Conor Dooley <[email protected]>

Cc Jisheng Zhang <[email protected]>

Cc Wei Fu <[email protected]>

---
 docs/platform/thead-c9xx.md       | 21 +++++-------
 lib/utils/reset/fdt_reset_thead.c | 57 +++++++++++++++----------------
 2 files changed, 35 insertions(+), 43 deletions(-)

docs/platform/thead-c9xx.md +2 -5

index 8bb9e91f1a9b..35cca94b5bb9 100644
@@ -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;
 

Quoted 16 lines

docs/platform/thead-c9xx.md DTS Example2: (Multi cores with soc reset-regs) · line 147

Quoted 12 lines

docs/platform/thead-c9xx.md +5 -7

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

Quoted 7 lines

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.

Quoted 1 line

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.

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

Quoted 2 lines · Guo Ren

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 :)

Quoted 6 lines · Conor Dooley

Quoted 2 lines · Guo Ren

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

Quoted 8 lines

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.

Quoted 16 lines · Jessica Clarke

Quoted 6 lines · Conor Dooley

Quoted 2 lines · Guo Ren

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

Quoted 8 lines

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 :)

Quoted 4 lines

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.

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

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.

Quoted 1 line · Guo Ren

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

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 3 lines

Thanks,
Conor.

--
Best Regards
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).

Quoted 31 lines · On Thu, May 25, 2023 at 2:06?PM Guo Ren <[email protected]> wrote:

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 3 lines

Thanks,
Conor.

--
Best Regards
Guo Ren

Signature · 2 lines

Best Regards
Guo Ren

Quoted 6 lines · Guo Ren

Hi Conor,

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

Quoted 6 lines

              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

Quoted 44 lines

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

Quoted 23 lines · On Thu, May 25, 2023 at 2:06?PM Guo Ren <[email protected]> wrote:

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Thanks,
Conor.

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

Quoted 15 lines · On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <[email protected]> wrote:

Quoted 6 lines · Guo Ren

Hi Conor,

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

Quoted 6 lines

             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

Quoted 2 lines

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.

Quoted 1 line

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 }

Quoted 48 lines

Jess

Quoted 44 lines

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

Quoted 23 lines · On Thu, May 25, 2023 at 2:06?PM Guo Ren <[email protected]> wrote:

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Thanks,
Conor.

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

Signature · 2 lines

Best Regards
Guo Ren

Quoted 52 lines · Guo Ren

Quoted 15 lines · On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <[email protected]> wrote:

Quoted 6 lines · Guo Ren

Hi Conor,

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

Quoted 6 lines

             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

Quoted 2 lines

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.

Quoted 1 line

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();

Quoted 4 lines

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

Quoted 78 lines

}
==========

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

Quoted 48 lines

Jess

Quoted 44 lines

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

Quoted 23 lines · On Thu, May 25, 2023 at 2:06?PM Guo Ren <[email protected]> wrote:

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Thanks,
Conor.

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

Quoted 59 lines · On Tue, Jun 13, 2023 at 10:43?AM Jessica Clarke <[email protected]> wrote:

Quoted 56 lines · Guo Ren

Quoted 15 lines · On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <[email protected]> wrote:

Quoted 6 lines · Guo Ren

Hi Conor,

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

Quoted 6 lines

            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

Quoted 2 lines

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.

Quoted 1 line

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();
}

Quoted 3 lines

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.

Quoted 75 lines

Quoted 75 lines

}
==========

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

Quoted 48 lines

Jess

Quoted 44 lines

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

Quoted 23 lines · On Thu, May 25, 2023 at 2:06?PM Guo Ren <[email protected]> wrote:

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Thanks,
Conor.

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

Quoted 5 lines

Quoted 3 lines

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

Quoted 61 lines · Guo Ren

Quoted 59 lines · On Tue, Jun 13, 2023 at 10:43?AM Jessica Clarke <[email protected]> wrote:

Quoted 56 lines · Guo Ren

Quoted 15 lines · On Mon, Jun 12, 2023 at 9:05?AM Jessica Clarke <[email protected]> wrote:

Quoted 6 lines · Guo Ren

Hi Conor,

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

Quoted 6 lines

            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

Quoted 2 lines

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.

Quoted 1 line

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();
}

Quoted 6 lines

Quoted 3 lines

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

Quoted 85 lines

Quoted 75 lines

Quoted 75 lines

}
==========

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

Quoted 48 lines

Jess

Quoted 44 lines

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

Quoted 23 lines · On Thu, May 25, 2023 at 2:06?PM Guo Ren <[email protected]> wrote:

Quoted 7 lines · On Thu, May 25, 2023 at 1:33?PM Conor Dooley <[email protected]> wrote:

Quoted 1 line · Guo Ren

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

Quoted 8 lines

As a generic fallback compatible.

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Thanks,
Conor.

--
Best Regards
Guo Ren

--
Best Regards
Guo Ren

Quoted 5 lines

Quoted 3 lines

--
Best Regards
Guo Ren

--
Best Regards
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).

Quoted 14 lines · 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>;
                };

Quoted 2 lines

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.

Quoted 6 lines · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

Quoted 5 lines · Guo Ren

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";

Quoted 5 lines

Quoted 5 lines

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

0x7c5 0x7cc 0x7ce>;

Quoted 5 lines

Quoted 2 lines

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.

Quoted 5 lines

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);
}
}

Hey Guo Ren,

Quoted 7 lines · Guo Ren

Quoted 6 lines · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

Quoted 5 lines · Guo Ren

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

                       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.

Quoted 15 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 14 lines

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);
}
}

Quoted 5 lines · On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <[email protected]> wrote:

Quoted 4 lines · Guo Ren

Quoted 3 lines · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

Quoted 2 lines · Guo Ren

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

Quoted 12 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

               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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

0x7c5 0x7cc 0x7ce>;

Quoted 7 lines

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

Quoted 2 lines · Guo Ren

Quoted 1 line · On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <[email protected]> wrote:

On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

Quoted 3 lines · Guo Ren

Quoted 2 lines

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.

Quoted 21 lines

Quoted 13 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

               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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

0x7c5 0x7cc 0x7ce>;

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 4 lines · On Wed, Jun 14, 2023 at 2:56?PM Conor Dooley <[email protected]> wrote:

Quoted 2 lines · Guo Ren

Quoted 1 line · On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <[email protected]> wrote:

On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

Quoted 3 lines · Guo Ren

Quoted 2 lines

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.

Quoted 30 lines

Quoted 21 lines

Quoted 13 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

               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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

0x7c5 0x7cc 0x7ce>;

Quoted 7 lines

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.

Quoted 2 lines

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?

(quoted diff) +4 -0

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

Quoted 59 lines · Guo Ren

Quoted 4 lines · On Wed, Jun 14, 2023 at 2:56?PM Conor Dooley <[email protected]> wrote:

Quoted 2 lines · Guo Ren

Quoted 1 line · On Wed, Jun 14, 2023 at 3:36?AM Conor Dooley <[email protected]> wrote:

On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

Quoted 3 lines · Guo Ren

Quoted 2 lines

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.

Quoted 30 lines

Quoted 21 lines

Quoted 13 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

               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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

0x7c5 0x7cc 0x7ce>;

Quoted 7 lines

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.

Quoted 2 lines

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?

(quoted diff) +4 -0

+               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

Quoted 8 lines

        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.

Hey Guo Ren,

Quoted 2 lines · Guo Ren

Quoted 1 line · On Mon, Jun 12, 2023 at 11:39?PM Conor Dooley <[email protected]> wrote:

On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

                       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.

Quoted 13 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 14 lines

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);
}
}

Quoted 6 lines · Conor Dooley

Quoted 2 lines · Guo Ren

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

Quoted 8 lines

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

From: Guo Ren <[email protected]>

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

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Quoted 6 lines · On Tue, May 23, 2023 at 3:17?PM <[email protected]> wrote:

From: Guo Ren <[email protected]>

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

Quoted 18 lines

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