[PATCH v2 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs

[PATCH v2 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The device trees produced automatically for the virt and spike machines
fail dt-validate on several grounds. Some of these need to be fixed in
the linux kernel's dt-bindings, but others are caused by bugs in QEMU.

I mostly opted for what appeared to be the smallest change that would
fix the warnings, partly due to my inexperience with the QEMU codebase.
A "sister" patchset for the kernel will clear the remaining warnings [0].
Thanks to Rob Herring for reporting these issues [1],
Conor.

Changes since v1:
- drop patch 1

To reproduce the errors:
./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
(The processed schema needs to be generated first)

0 - https://lore.kernel.org/linux-riscv/20220805162844.1554247-1-mail@conchuod.ie
1 - https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org

Conor Dooley (4):
  hw/riscv: virt: fix uart node name
  hw/riscv: virt: Fix the plic's address cells
  hw/riscv: virt: fix syscon subnode paths
  hw/core: fix platform bus node name

 hw/core/sysbus-fdt.c    |  2 +-
 hw/riscv/virt.c         | 10 +++++++---
 include/hw/riscv/virt.h |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)


base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The device trees produced automatically for the virt and spike machines
fail dt-validate on several grounds. Some of these need to be fixed in
the linux kernel's dt-bindings, but others are caused by bugs in QEMU.

I mostly opted for what appeared to be the smallest change that would
fix the warnings, partly due to my inexperience with the QEMU codebase.
A "sister" patchset for the kernel will clear the remaining warnings [0].
Thanks to Rob Herring for reporting these issues [1],
Conor.

Changes since v1:
- drop patch 1

To reproduce the errors:
./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
(The processed schema needs to be generated first)

0 - https://lore.kernel.org/linux-riscv/20220805162844.1554247-1-mail@conchuod.ie
1 - https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org

Conor Dooley (4):
  hw/riscv: virt: fix uart node name
  hw/riscv: virt: Fix the plic's address cells
  hw/riscv: virt: fix syscon subnode paths
  hw/core: fix platform bus node name

 hw/core/sysbus-fdt.c    |  2 +-
 hw/riscv/virt.c         | 10 +++++++---
 include/hw/riscv/virt.h |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)


base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819
-- 
2.37.1

[PATCH v2 1/4] hw/riscv: virt: fix uart node name

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

"uart" is not a node name that complies with the dt-schema.
Change the node name to "serial" to ix warnings seen during
dt-validate on a dtbdump of the virt machine such as:
/stuff/qemu/qemu.dtb: uart@10000000: $nodename:0: 'uart@10000000' does not match '^serial(@.*)?$'
        From schema: /stuff/linux/Documentation/devicetree/bindings/serial/8250.yaml

Reported-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
Fixes: 04331d0b56 ("RISC-V VirtIO Machine")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/riscv/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..6c61a406c4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -917,7 +917,7 @@ static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
     char *name;
     MachineState *mc = MACHINE(s);
 
-    name = g_strdup_printf("/soc/uart@%lx", (long)memmap[VIRT_UART0].base);
+    name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "ns16550a");
     qemu_fdt_setprop_cells(mc->fdt, name, "reg",
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 1/4] hw/riscv: virt: fix uart node name

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

"uart" is not a node name that complies with the dt-schema.
Change the node name to "serial" to ix warnings seen during
dt-validate on a dtbdump of the virt machine such as:
/stuff/qemu/qemu.dtb: uart@10000000: $nodename:0: 'uart@10000000' does not match '^serial(@.*)?$'
        From schema: /stuff/linux/Documentation/devicetree/bindings/serial/8250.yaml

Reported-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
Fixes: 04331d0b56 ("RISC-V VirtIO Machine")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/riscv/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..6c61a406c4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -917,7 +917,7 @@ static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
     char *name;
     MachineState *mc = MACHINE(s);
 
-    name = g_strdup_printf("/soc/uart@%lx", (long)memmap[VIRT_UART0].base);
+    name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "ns16550a");
     qemu_fdt_setprop_cells(mc->fdt, name, "reg",
-- 
2.37.1

[PATCH v2 2/4] hw/riscv: virt: Fix the plic's address cells

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

When optional AIA PLIC support was added the to the virt machine, the
address cells property was removed leading the issues with dt-validate
on a dump from the virt machine:
/stuff/qemu/qemu.dtb: plic@c000000: '#address-cells' is a required property
        From schema: /stuff/linux/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
Add back the property to suppress the warning.

Reported-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
Fixes: e6faee6585 ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/riscv/virt.c         | 2 ++
 include/hw/riscv/virt.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6c61a406c4..8b2978076e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -465,6 +465,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     qemu_fdt_add_subnode(mc->fdt, plic_name);
     qemu_fdt_setprop_cell(mc->fdt, plic_name,
         "#interrupt-cells", FDT_PLIC_INT_CELLS);
+    qemu_fdt_setprop_cell(mc->fdt, plic_name,
+        "#address-cells", FDT_PLIC_ADDR_CELLS);
     qemu_fdt_setprop_string_array(mc->fdt, plic_name, "compatible",
                                   (char **)&plic_compat,
                                   ARRAY_SIZE(plic_compat));
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 984e55c77f..be4ab8fe7f 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -111,6 +111,7 @@ enum {
 
 #define FDT_PCI_ADDR_CELLS    3
 #define FDT_PCI_INT_CELLS     1
+#define FDT_PLIC_ADDR_CELLS   0
 #define FDT_PLIC_INT_CELLS    1
 #define FDT_APLIC_INT_CELLS   2
 #define FDT_IMSIC_INT_CELLS   0
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 2/4] hw/riscv: virt: Fix the plic's address cells

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

When optional AIA PLIC support was added the to the virt machine, the
address cells property was removed leading the issues with dt-validate
on a dump from the virt machine:
/stuff/qemu/qemu.dtb: plic@c000000: '#address-cells' is a required property
        From schema: /stuff/linux/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
Add back the property to suppress the warning.

Reported-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
Fixes: e6faee6585 ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/riscv/virt.c         | 2 ++
 include/hw/riscv/virt.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6c61a406c4..8b2978076e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -465,6 +465,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     qemu_fdt_add_subnode(mc->fdt, plic_name);
     qemu_fdt_setprop_cell(mc->fdt, plic_name,
         "#interrupt-cells", FDT_PLIC_INT_CELLS);
+    qemu_fdt_setprop_cell(mc->fdt, plic_name,
+        "#address-cells", FDT_PLIC_ADDR_CELLS);
     qemu_fdt_setprop_string_array(mc->fdt, plic_name, "compatible",
                                   (char **)&plic_compat,
                                   ARRAY_SIZE(plic_compat));
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 984e55c77f..be4ab8fe7f 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -111,6 +111,7 @@ enum {
 
 #define FDT_PCI_ADDR_CELLS    3
 #define FDT_PCI_INT_CELLS     1
+#define FDT_PLIC_ADDR_CELLS   0
 #define FDT_PLIC_INT_CELLS    1
 #define FDT_APLIC_INT_CELLS   2
 #define FDT_IMSIC_INT_CELLS   0
-- 
2.37.1

[PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The subnodes of the syscon have been added to the incorrect paths.
Rather than add them as subnodes, they were originally added to "/foo"
and a later patch moved them to "/soc/foo". Both are incorrect & they
should have been added as "/soc/test@###/foo" as "/soc/test" is the
syscon node. Fix both the reboot and poweroff subnodes to avoid errors
such as:

/stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
/stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml

Reported-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/riscv/virt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8b2978076e..a98b054545 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
     g_free(name);
 
-    name = g_strdup_printf("/soc/reboot");
+    name = g_strdup_printf("/soc/test@%lx/reboot",
+        (long)memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
@@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
     g_free(name);
 
-    name = g_strdup_printf("/soc/poweroff");
+    name = g_strdup_printf("/soc/test@%lx/poweroff",
+        (long)memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The subnodes of the syscon have been added to the incorrect paths.
Rather than add them as subnodes, they were originally added to "/foo"
and a later patch moved them to "/soc/foo". Both are incorrect & they
should have been added as "/soc/test@###/foo" as "/soc/test" is the
syscon node. Fix both the reboot and poweroff subnodes to avoid errors
such as:

/stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
/stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml

Reported-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/riscv/virt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8b2978076e..a98b054545 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
     g_free(name);
 
-    name = g_strdup_printf("/soc/reboot");
+    name = g_strdup_printf("/soc/test@%lx/reboot",
+        (long)memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
@@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
     g_free(name);
 
-    name = g_strdup_printf("/soc/poweroff");
+    name = g_strdup_printf("/soc/test@%lx/poweroff",
+        (long)memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
-- 
2.37.1

[PATCH v2 4/4] hw/core: fix platform bus node name

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

"platform" is not a valid name for a bus node in dt-schema, so warnings
can be see in dt-validate on a dump of the riscv virt dtb:

/stuff/qemu/qemu.dtb: platform@4000000: $nodename:0: 'platform@4000000' does not match '^([a-z][a-z0-9\\-]+-bus|bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
"platform-bus" is a valid name, so use that instead.

CC: Rob Herring <robh@kernel.org>
Fixes: 11d306b9df ("hw/arm/sysbus-fdt: helpers for platform bus nodes addition")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/core/sysbus-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index 19d22cbe73..edb0c49b19 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -539,7 +539,7 @@ void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
 
     assert(fdt);
 
-    node = g_strdup_printf("/platform@%"PRIx64, addr);
+    node = g_strdup_printf("/platform-bus@%"PRIx64, addr);
 
     /* Create a /platform node that we can put all devices into */
     qemu_fdt_add_subnode(fdt, node);
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 4/4] hw/core: fix platform bus node name

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

"platform" is not a valid name for a bus node in dt-schema, so warnings
can be see in dt-validate on a dump of the riscv virt dtb:

/stuff/qemu/qemu.dtb: platform@4000000: $nodename:0: 'platform@4000000' does not match '^([a-z][a-z0-9\\-]+-bus|bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
"platform-bus" is a valid name, so use that instead.

CC: Rob Herring <robh@kernel.org>
Fixes: 11d306b9df ("hw/arm/sysbus-fdt: helpers for platform bus nodes addition")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 hw/core/sysbus-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index 19d22cbe73..edb0c49b19 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -539,7 +539,7 @@ void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
 
     assert(fdt);
 
-    node = g_strdup_printf("/platform@%"PRIx64, addr);
+    node = g_strdup_printf("/platform-bus@%"PRIx64, addr);
 
     /* Create a /platform node that we can put all devices into */
     qemu_fdt_add_subnode(fdt, node);
-- 
2.37.1

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Jessica Clarke]

On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
> 
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The subnodes of the syscon have been added to the incorrect paths.
> Rather than add them as subnodes, they were originally added to "/foo"
> and a later patch moved them to "/soc/foo". Both are incorrect & they
> should have been added as "/soc/test@###/foo" as "/soc/test" is the
> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
> such as:
> 
> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> 
> Reported-by: Rob Herring <robh@kernel.org>
> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
is written to handle syscon-poweroff/reboot existing as a child of a
simplebus not a syscon. Moreover, what is the point of regmap in this
case? Its existence suggests the point is for them to *not* be children
of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
just look at the parent. Moving the nodes whilst keeping the property
doesn’t make sense to me.

Jess

> ---
> hw/riscv/virt.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 8b2978076e..a98b054545 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
>     g_free(name);
> 
> -    name = g_strdup_printf("/soc/reboot");
> +    name = g_strdup_printf("/soc/test@%lx/reboot",
> +        (long)memmap[VIRT_TEST].base);
>     qemu_fdt_add_subnode(mc->fdt, name);
>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
> @@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
>     g_free(name);
> 
> -    name = g_strdup_printf("/soc/poweroff");
> +    name = g_strdup_printf("/soc/test@%lx/poweroff",
> +        (long)memmap[VIRT_TEST].base);
>     qemu_fdt_add_subnode(mc->fdt, name);
>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
> -- 
> 2.37.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Jessica Clarke]

On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
> 
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The subnodes of the syscon have been added to the incorrect paths.
> Rather than add them as subnodes, they were originally added to "/foo"
> and a later patch moved them to "/soc/foo". Both are incorrect & they
> should have been added as "/soc/test@###/foo" as "/soc/test" is the
> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
> such as:
> 
> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> 
> Reported-by: Rob Herring <robh@kernel.org>
> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
is written to handle syscon-poweroff/reboot existing as a child of a
simplebus not a syscon. Moreover, what is the point of regmap in this
case? Its existence suggests the point is for them to *not* be children
of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
just look at the parent. Moving the nodes whilst keeping the property
doesn’t make sense to me.

Jess

> ---
> hw/riscv/virt.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 8b2978076e..a98b054545 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
>     g_free(name);
> 
> -    name = g_strdup_printf("/soc/reboot");
> +    name = g_strdup_printf("/soc/test@%lx/reboot",
> +        (long)memmap[VIRT_TEST].base);
>     qemu_fdt_add_subnode(mc->fdt, name);
>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
> @@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
>     g_free(name);
> 
> -    name = g_strdup_printf("/soc/poweroff");
> +    name = g_strdup_printf("/soc/test@%lx/poweroff",
> +        (long)memmap[VIRT_TEST].base);
>     qemu_fdt_add_subnode(mc->fdt, name);
>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
> -- 
> 2.37.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor.Dooley]

On 08/08/2022 22:28, Jessica Clarke wrote:
> On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
>>
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> The subnodes of the syscon have been added to the incorrect paths.
>> Rather than add them as subnodes, they were originally added to "/foo"
>> and a later patch moved them to "/soc/foo". Both are incorrect & they
>> should have been added as "/soc/test@###/foo" as "/soc/test" is the
>> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
>> such as:
>>
>> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
>>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
>>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>>
>> Reported-by: Rob Herring <robh@kernel.org>
>> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
>> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
>> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
> is written to handle syscon-poweroff/reboot existing as a child of a
> simplebus not a syscon.

I know next to nothing about FreeBSD unfortunately or how it handles
buses. My understanding of simple-bus was that it is supposed to
represent a bus with "things" mapped to addresses, relying on the "reg"
property. And then syscon is used when there is some multifunction
register region that controls multiple features of the hardware.
Since simple-bus defines a reg property and the function nodes do not
define one, I'd like to know how FreeBSD's driver handles that.

> Moreover, what is the point of regmap in this
> case? Its existence suggests the point is for them to *not* be children
> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
> just look at the parent. Moving the nodes whilst keeping the property
> doesn’t make sense to me.

That's how syscon bindings are constructed, makes it easier to follow
I suppose if they functions are children of the syscon node. Strictly
I think they don't need to be under the syscon itself, I think they can
also go at the top level - they just aren't valid under the /soc node
as it has been defined as a "simple-bus".

It would appear that the original patch 0e404da007 ("riscv/virt: Add
syscon reboot and poweroff DT nodes") that added them put them at the
top level and it was in the refactor that they got moved to the soc bus.*
Maybe the solution would be to put them back at the top level?

dt-validate will consider it valid, but what does the FreeBSD driver
think of that?

Thanks for your input Jess,
Conor.

* On reflection it looks like my fixes tags are not correct and that
0e404da007 was actually correct but the refactor broke things.

> 
> Jess
> 
>> ---
>> hw/riscv/virt.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 8b2978076e..a98b054545 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>>     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
>>     g_free(name);
>>
>> -    name = g_strdup_printf("/soc/reboot");
>> +    name = g_strdup_printf("/soc/test@%lx/reboot",
>> +        (long)memmap[VIRT_TEST].base);
>>     qemu_fdt_add_subnode(mc->fdt, name);
>>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
>>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> @@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>>     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
>>     g_free(name);
>>
>> -    name = g_strdup_printf("/soc/poweroff");
>> +    name = g_strdup_printf("/soc/test@%lx/poweroff",
>> +        (long)memmap[VIRT_TEST].base);
>>     qemu_fdt_add_subnode(mc->fdt, name);
>>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
>>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> -- 
>> 2.37.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor.Dooley]

On 08/08/2022 22:28, Jessica Clarke wrote:
> On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
>>
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> The subnodes of the syscon have been added to the incorrect paths.
>> Rather than add them as subnodes, they were originally added to "/foo"
>> and a later patch moved them to "/soc/foo". Both are incorrect & they
>> should have been added as "/soc/test@###/foo" as "/soc/test" is the
>> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
>> such as:
>>
>> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
>>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
>>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
>>
>> Reported-by: Rob Herring <robh@kernel.org>
>> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
>> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
>> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
> is written to handle syscon-poweroff/reboot existing as a child of a
> simplebus not a syscon.

I know next to nothing about FreeBSD unfortunately or how it handles
buses. My understanding of simple-bus was that it is supposed to
represent a bus with "things" mapped to addresses, relying on the "reg"
property. And then syscon is used when there is some multifunction
register region that controls multiple features of the hardware.
Since simple-bus defines a reg property and the function nodes do not
define one, I'd like to know how FreeBSD's driver handles that.

> Moreover, what is the point of regmap in this
> case? Its existence suggests the point is for them to *not* be children
> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
> just look at the parent. Moving the nodes whilst keeping the property
> doesn’t make sense to me.

That's how syscon bindings are constructed, makes it easier to follow
I suppose if they functions are children of the syscon node. Strictly
I think they don't need to be under the syscon itself, I think they can
also go at the top level - they just aren't valid under the /soc node
as it has been defined as a "simple-bus".

It would appear that the original patch 0e404da007 ("riscv/virt: Add
syscon reboot and poweroff DT nodes") that added them put them at the
top level and it was in the refactor that they got moved to the soc bus.*
Maybe the solution would be to put them back at the top level?

dt-validate will consider it valid, but what does the FreeBSD driver
think of that?

Thanks for your input Jess,
Conor.

* On reflection it looks like my fixes tags are not correct and that
0e404da007 was actually correct but the refactor broke things.

> 
> Jess
> 
>> ---
>> hw/riscv/virt.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 8b2978076e..a98b054545 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -896,7 +896,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>>     test_phandle = qemu_fdt_get_phandle(mc->fdt, name);
>>     g_free(name);
>>
>> -    name = g_strdup_printf("/soc/reboot");
>> +    name = g_strdup_printf("/soc/test@%lx/reboot",
>> +        (long)memmap[VIRT_TEST].base);
>>     qemu_fdt_add_subnode(mc->fdt, name);
>>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-reboot");
>>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> @@ -904,7 +905,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
>>     qemu_fdt_setprop_cell(mc->fdt, name, "value", FINISHER_RESET);
>>     g_free(name);
>>
>> -    name = g_strdup_printf("/soc/poweroff");
>> +    name = g_strdup_printf("/soc/test@%lx/poweroff",
>> +        (long)memmap[VIRT_TEST].base);
>>     qemu_fdt_add_subnode(mc->fdt, name);
>>     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "syscon-poweroff");
>>     qemu_fdt_setprop_cell(mc->fdt, name, "regmap", test_phandle);
>> -- 
>> 2.37.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Rob Herring]

On Mon, Aug 8, 2022 at 4:10 PM <Conor.Dooley@microchip.com> wrote:
>
> On 08/08/2022 22:28, Jessica Clarke wrote:
> > On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
> >>
> >> From: Conor Dooley <conor.dooley@microchip.com>
> >>
> >> The subnodes of the syscon have been added to the incorrect paths.
> >> Rather than add them as subnodes, they were originally added to "/foo"
> >> and a later patch moved them to "/soc/foo". Both are incorrect & they
> >> should have been added as "/soc/test@###/foo" as "/soc/test" is the
> >> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
> >> such as:
> >>
> >> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
> >>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> >> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
> >>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> >>
> >> Reported-by: Rob Herring <robh@kernel.org>
> >> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
> >> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
> >> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
> > is written to handle syscon-poweroff/reboot existing as a child of a
> > simplebus not a syscon.

It probably breaks Linux, too.

> I know next to nothing about FreeBSD unfortunately or how it handles
> buses. My understanding of simple-bus was that it is supposed to
> represent a bus with "things" mapped to addresses, relying on the "reg"
> property. And then syscon is used when there is some multifunction
> register region that controls multiple features of the hardware.
> Since simple-bus defines a reg property and the function nodes do not
> define one, I'd like to know how FreeBSD's driver handles that.
>
> > Moreover, what is the point of regmap in this
> > case? Its existence suggests the point is for them to *not* be children
> > of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
> > just look at the parent. Moving the nodes whilst keeping the property
> > doesn’t make sense to me.
>
> That's how syscon bindings are constructed, makes it easier to follow
> I suppose if they functions are children of the syscon node. Strictly
> I think they don't need to be under the syscon itself, I think they can
> also go at the top level - they just aren't valid under the /soc node
> as it has been defined as a "simple-bus".
>
> It would appear that the original patch 0e404da007 ("riscv/virt: Add
> syscon reboot and poweroff DT nodes") that added them put them at the
> top level and it was in the refactor that they got moved to the soc bus.*
> Maybe the solution would be to put them back at the top level?

Perhaps.

The other option is adding 'simple-mfd' to the 'test' node compatible.
That would work for Linux. Not sure for FreeBSD.

Rob

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Rob Herring]

On Mon, Aug 8, 2022 at 4:10 PM <Conor.Dooley@microchip.com> wrote:
>
> On 08/08/2022 22:28, Jessica Clarke wrote:
> > On 8 Aug 2022, at 22:06, Conor Dooley <mail@conchuod.ie> wrote:
> >>
> >> From: Conor Dooley <conor.dooley@microchip.com>
> >>
> >> The subnodes of the syscon have been added to the incorrect paths.
> >> Rather than add them as subnodes, they were originally added to "/foo"
> >> and a later patch moved them to "/soc/foo". Both are incorrect & they
> >> should have been added as "/soc/test@###/foo" as "/soc/test" is the
> >> syscon node. Fix both the reboot and poweroff subnodes to avoid errors
> >> such as:
> >>
> >> /stuff/qemu/qemu.dtb: soc: poweroff: {'value': [[21845]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-poweroff']} should not be valid under {'type': 'object'}
> >>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> >> /stuff/qemu/qemu.dtb: soc: reboot: {'value': [[30583]], 'offset': [[0]], 'regmap': [[4]], 'compatible': ['syscon-reboot']} should not be valid under {'type': 'object'}
> >>        From schema: /home/conor/.local/lib/python3.9/site-packages/dtschema/schemas/simple-bus.yaml
> >>
> >> Reported-by: Rob Herring <robh@kernel.org>
> >> Link: https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/
> >> Fixes: 18df0b4695 ("hw/riscv: virt: Allow creating multiple NUMA sockets")
> >> Fixes: 0e404da007 ("riscv/virt: Add syscon reboot and poweroff DT nodes")
> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > This breaks FreeBSD’s driver (well, it just won’t probe/attach), which
> > is written to handle syscon-poweroff/reboot existing as a child of a
> > simplebus not a syscon.

It probably breaks Linux, too.

> I know next to nothing about FreeBSD unfortunately or how it handles
> buses. My understanding of simple-bus was that it is supposed to
> represent a bus with "things" mapped to addresses, relying on the "reg"
> property. And then syscon is used when there is some multifunction
> register region that controls multiple features of the hardware.
> Since simple-bus defines a reg property and the function nodes do not
> define one, I'd like to know how FreeBSD's driver handles that.
>
> > Moreover, what is the point of regmap in this
> > case? Its existence suggests the point is for them to *not* be children
> > of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
> > just look at the parent. Moving the nodes whilst keeping the property
> > doesn’t make sense to me.
>
> That's how syscon bindings are constructed, makes it easier to follow
> I suppose if they functions are children of the syscon node. Strictly
> I think they don't need to be under the syscon itself, I think they can
> also go at the top level - they just aren't valid under the /soc node
> as it has been defined as a "simple-bus".
>
> It would appear that the original patch 0e404da007 ("riscv/virt: Add
> syscon reboot and poweroff DT nodes") that added them put them at the
> top level and it was in the refactor that they got moved to the soc bus.*
> Maybe the solution would be to put them back at the top level?

Perhaps.

The other option is adding 'simple-mfd' to the 'test' node compatible.
That would work for Linux. Not sure for FreeBSD.

Rob

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor.Dooley]

On 09/08/2022 00:10, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Aug 8, 2022 at 4:10 PM <Conor.Dooley@microchip.com> wrote:
>>
>> On 08/08/2022 22:28, Jessica Clarke wrote:
>>> Moreover, what is the point of regmap in this
>>> case? Its existence suggests the point is for them to *not* be children
>>> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
>>> just look at the parent. Moving the nodes whilst keeping the property
>>> doesn’t make sense to me.
>>
>> That's how syscon bindings are constructed, makes it easier to follow
>> I suppose if they functions are children of the syscon node. Strictly
>> I think they don't need to be under the syscon itself, I think they can
>> also go at the top level - they just aren't valid under the /soc node
>> as it has been defined as a "simple-bus".
>>
>> It would appear that the original patch 0e404da007 ("riscv/virt: Add
>> syscon reboot and poweroff DT nodes") that added them put them at the
>> top level and it was in the refactor that they got moved to the soc bus.*
>> Maybe the solution would be to put them back at the top level?
> 
> Perhaps.
> 
> The other option is adding 'simple-mfd' to the 'test' node compatible.
> That would work for Linux. Not sure for FreeBSD.

Right, of course I was missing something in my understanding. The probe
flow on Linux came back to me on the bike this morning after reading this
and I felt like an idiot for missing that in the devicetrees I looked at!

@Jess, which does FreeBSD prefer? top level or add the extra compatible?

Thanks,
Conor.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor.Dooley]

On 09/08/2022 00:10, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Aug 8, 2022 at 4:10 PM <Conor.Dooley@microchip.com> wrote:
>>
>> On 08/08/2022 22:28, Jessica Clarke wrote:
>>> Moreover, what is the point of regmap in this
>>> case? Its existence suggests the point is for them to *not* be children
>>> of the syscon, otherwise you wouldn’t need an explicit phandle, you’d
>>> just look at the parent. Moving the nodes whilst keeping the property
>>> doesn’t make sense to me.
>>
>> That's how syscon bindings are constructed, makes it easier to follow
>> I suppose if they functions are children of the syscon node. Strictly
>> I think they don't need to be under the syscon itself, I think they can
>> also go at the top level - they just aren't valid under the /soc node
>> as it has been defined as a "simple-bus".
>>
>> It would appear that the original patch 0e404da007 ("riscv/virt: Add
>> syscon reboot and poweroff DT nodes") that added them put them at the
>> top level and it was in the refactor that they got moved to the soc bus.*
>> Maybe the solution would be to put them back at the top level?
> 
> Perhaps.
> 
> The other option is adding 'simple-mfd' to the 'test' node compatible.
> That would work for Linux. Not sure for FreeBSD.

Right, of course I was missing something in my understanding. The probe
flow on Linux came back to me on the bike this morning after reading this
and I felt like an idiot for missing that in the devicetrees I looked at!

@Jess, which does FreeBSD prefer? top level or add the extra compatible?

Thanks,
Conor.