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

[PATCH v3 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.
Thanks to Rob Herring for reporting these issues [1],
Conor.

Changes since v2:
- move the syscon subnodes back to the top level instead of into the
  syscon node
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         | 8 +++++---
 include/hw/riscv/virt.h | 1 +
 3 files changed, 7 insertions(+), 4 deletions(-)


base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819
-- 
2.37.1

[PATCH v3 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 v3 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 v3 3/4] hw/riscv: virt: fix syscon subnode paths

[Conor Dooley]

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

The reset and poweroff features of the syscon were originally added to
top level, which is a valid path for a syscon subnode. Subsequently a
reorganisation was carried out while implementing NUMA in which the
subnodes were moved into the /soc node. As /soc is a "simple-bus", this
path is invalid, and so dt-validate produces the following warnings:

/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

Move the syscon subnodes back to the top level and silence the warnings.

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")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I dropped your R-b Alistair intentionally.
Tested both Linux and FreeBSD:
[    0.073406] device: 'poweroff': device_add
[    0.073441] bus: 'platform': add device poweroff
[    0.502347] bus: 'platform': add driver syscon-poweroff
[    0.502379] bus: 'platform': __driver_probe_device: matched device poweroff with driver syscon-poweroff
[    0.502397] bus: 'platform': really_probe: probing driver syscon-poweroff with device poweroff
[    0.502423] syscon-poweroff poweroff: no pinctrl handle
[    0.502681] syscon-poweroff poweroff: pm_power_off already claimed for sbi_srst_power_off
[    0.503333] syscon-poweroff: probe of poweroff failed with error -16
[    0.073629] device: 'reboot': device_add
[    0.073664] bus: 'platform': add device reboot
[    0.500640] bus: 'platform': add driver syscon-reboot
[    0.500673] bus: 'platform': __driver_probe_device: matched device reboot with driver syscon-reboot
[    0.500694] bus: 'platform': really_probe: probing driver syscon-reboot with device reboot
[    0.500725] syscon-reboot reboot: no pinctrl handle
[    0.502168] driver: 'syscon-reboot': driver_bound: bound to device 'reboot'
[    0.502242] bus: 'platform': really_probe: bound device reboot to driver syscon-reboot

syscon_power0: <Syscon poweroff> on ofwbus0
syscon_power1: <Syscon reboot> on ofwbus0
---
 hw/riscv/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8b2978076e..6f0fd1541b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -896,7 +896,7 @@ 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("/reboot");
     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 +904,7 @@ 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("/poweroff");
     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

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

[Alistair Francis]

On Thu, Aug 11, 2022 at 5:06 AM Conor Dooley <mail@conchuod.ie> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> The reset and poweroff features of the syscon were originally added to
> top level, which is a valid path for a syscon subnode. Subsequently a
> reorganisation was carried out while implementing NUMA in which the
> subnodes were moved into the /soc node. As /soc is a "simple-bus", this
> path is invalid, and so dt-validate produces the following warnings:
>
> /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
>
> Move the syscon subnodes back to the top level and silence the warnings.
>
> 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")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> I dropped your R-b Alistair intentionally.
> Tested both Linux and FreeBSD:
> [    0.073406] device: 'poweroff': device_add
> [    0.073441] bus: 'platform': add device poweroff
> [    0.502347] bus: 'platform': add driver syscon-poweroff
> [    0.502379] bus: 'platform': __driver_probe_device: matched device poweroff with driver syscon-poweroff
> [    0.502397] bus: 'platform': really_probe: probing driver syscon-poweroff with device poweroff
> [    0.502423] syscon-poweroff poweroff: no pinctrl handle
> [    0.502681] syscon-poweroff poweroff: pm_power_off already claimed for sbi_srst_power_off
> [    0.503333] syscon-poweroff: probe of poweroff failed with error -16
> [    0.073629] device: 'reboot': device_add
> [    0.073664] bus: 'platform': add device reboot
> [    0.500640] bus: 'platform': add driver syscon-reboot
> [    0.500673] bus: 'platform': __driver_probe_device: matched device reboot with driver syscon-reboot
> [    0.500694] bus: 'platform': really_probe: probing driver syscon-reboot with device reboot
> [    0.500725] syscon-reboot reboot: no pinctrl handle
> [    0.502168] driver: 'syscon-reboot': driver_bound: bound to device 'reboot'
> [    0.502242] bus: 'platform': really_probe: bound device reboot to driver syscon-reboot
>
> syscon_power0: <Syscon poweroff> on ofwbus0
> syscon_power1: <Syscon reboot> on ofwbus0
> ---
>  hw/riscv/virt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 8b2978076e..6f0fd1541b 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -896,7 +896,7 @@ 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("/reboot");
>      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 +904,7 @@ 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("/poweroff");
>      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 v3 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 v3 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs

[Alistair Francis]

On Thu, Aug 11, 2022 at 5:09 AM Conor Dooley <mail@conchuod.ie> wrote:
>
> 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.
> Thanks to Rob Herring for reporting these issues [1],
> Conor.
>
> Changes since v2:
> - move the syscon subnodes back to the top level instead of into the
>   syscon node
> 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

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  hw/core/sysbus-fdt.c    | 2 +-
>  hw/riscv/virt.c         | 8 +++++---
>  include/hw/riscv/virt.h | 1 +
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
>
> base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819
> --
> 2.37.1
>
>