[PATCH 00/10] hw/arm/virt: Fix dt-schema warnings

[PATCH 00/10] hw/arm/virt: Fix dt-schema warnings

[Jean-Philippe Brucker]

dt-validate throws a few warnings when checking the aarch64 virt DTB.
Ensure that the virt DTB conforms to devicetree schema.

Some schema changes were needed, and should be in next Linux version [1]

I'm testing by running various virt machine configurations with -M
dumpdtb=qemu.dtb, then running dt-validate [1] and dtc:

  dt-validate -s linux/Documentation/devicetree/bindings/ qemu.dtb
  dtc -O dts qemu.dtb -o qemu.dts

[1] https://lore.kernel.org/linux-devicetree/20220822152224.507497-1-jean-philippe@linaro.org/
[2] https://github.com/devicetree-org/dt-schema

Jean-Philippe Brucker (10):
  hw/arm/virt: Fix devicetree warning about the root node
  hw/arm/boot: Fix devicetree warning about the PSCI node
  hw/arm/virt: Fix devicetree warnings about the GIC node
  hw/arm/virt: Use "msi-map" devicetree property for PCI
  hw/arm/virt: Fix devicetree warning about the timer node
  hw/arm/virt: Fix devicetree warning about the gpio-key node
  hw/arm/virt: Fix devicetree warnings about node names
  hw/arm/virt: Fix devicetree warnings about the GPIO node
  hw/arm/virt: Fix devicetree warnings about the SMMU node
  hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

 hw/arm/boot.c |  2 +-
 hw/arm/virt.c | 53 ++++++++++++++++++++++++++++-----------------------
 2 files changed, 30 insertions(+), 25 deletions(-)

-- 
2.37.1

[PATCH 01/10] hw/arm/virt: Fix devicetree warning about the root node

[Jean-Philippe Brucker]

dt-validate warns that the 'model' property is missing from the
devicetree:

  /: 'model' is a required property
  From schema: dtschema/schemas/root-node.yaml

Use the same name for model as for compatible. The devicetree
specification recommends that 'compatible' follows the format
'manufacturer,model' and 'model' follows the format
'manufacturer,model-number'. Since our 'compatible' doesn't observe
this, 'model' doesn't really need to either.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
This could be an opportunity to improve the machine print by Linux, for
example make it display "qemu,virt-7.2". I was concerned about breaking
some automated testing that may use the stable virt-x.y machines, so I
kept the same as compatible.
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9633f822f3..abcf2716bc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -252,6 +252,7 @@ static void create_fdt(VirtMachineState *vms)
     qemu_fdt_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
     qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
     qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
+    qemu_fdt_setprop_string(fdt, "/", "model", "linux,dummy-virt");
 
     /* /chosen must exist for load_dtb to fill in necessary properties later */
     qemu_fdt_add_subnode(fdt, "/chosen");
-- 
2.37.1

[PATCH 02/10] hw/arm/boot: Fix devicetree warning about the PSCI node

[Jean-Philippe Brucker]

dt-validate warns that an implementation compatible with arm,psci-1.0
shouldn't have arm,psci in their compatible string.

  psci: compatible: 'oneOf' conditional failed, one must be fixed:
	['arm,psci-1.0', 'arm,psci-0.2', 'arm,psci'] is too long
  From schema: linux/Documentation/devicetree/bindings/arm/psci.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..527918227e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -493,7 +493,7 @@ static void fdt_add_psci_node(void *fdt)
             const char comp[] = "arm,psci-0.2\0arm,psci";
             qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
         } else {
-            const char comp[] = "arm,psci-1.0\0arm,psci-0.2\0arm,psci";
+            const char comp[] = "arm,psci-1.0\0arm,psci-0.2";
             qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
         }
 
-- 
2.37.1

Re: [PATCH 02/10] hw/arm/boot: Fix devicetree warning about the PSCI node

[Peter Maydell]

On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> dt-validate warns that an implementation compatible with arm,psci-1.0
> shouldn't have arm,psci in their compatible string.
>
>   psci: compatible: 'oneOf' conditional failed, one must be fixed:
>         ['arm,psci-1.0', 'arm,psci-0.2', 'arm,psci'] is too long
>   From schema: linux/Documentation/devicetree/bindings/arm/psci.yaml
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ada2717f76..527918227e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -493,7 +493,7 @@ static void fdt_add_psci_node(void *fdt)
>              const char comp[] = "arm,psci-0.2\0arm,psci";
>              qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
>          } else {
> -            const char comp[] = "arm,psci-1.0\0arm,psci-0.2\0arm,psci";
> +            const char comp[] = "arm,psci-1.0\0arm,psci-0.2";
>              qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
>          }

This doesn't look right.
Documentation/devicetree/bindings/arm/psci.yaml says
"arm,psci-1.0" means "complies to PSCI 1.0",
"arm,psci-0.2" means "complies to PSCI 0.2",
and "arm,psci" means "complies to pre-0.2 PSCI"

If you want to drop "arm,psci" then you should be arguing why
we're not compliant with pre-0.2 PSCI. Maybe we aren't and we
shouldn't be advertising it, but you need more rationale than
"dt-validate complained".

thanks
-- PMM

Re: [PATCH 02/10] hw/arm/boot: Fix devicetree warning about the PSCI node

[Jean-Philippe Brucker]

On Wed, Aug 24, 2022 at 08:33:54PM +0100, Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > dt-validate warns that an implementation compatible with arm,psci-1.0
> > shouldn't have arm,psci in their compatible string.
> >
> >   psci: compatible: 'oneOf' conditional failed, one must be fixed:
> >         ['arm,psci-1.0', 'arm,psci-0.2', 'arm,psci'] is too long
> >   From schema: linux/Documentation/devicetree/bindings/arm/psci.yaml
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  hw/arm/boot.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index ada2717f76..527918227e 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -493,7 +493,7 @@ static void fdt_add_psci_node(void *fdt)
> >              const char comp[] = "arm,psci-0.2\0arm,psci";
> >              qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
> >          } else {
> > -            const char comp[] = "arm,psci-1.0\0arm,psci-0.2\0arm,psci";
> > +            const char comp[] = "arm,psci-1.0\0arm,psci-0.2";
> >              qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
> >          }
> 
> This doesn't look right.
> Documentation/devicetree/bindings/arm/psci.yaml says
> "arm,psci-1.0" means "complies to PSCI 1.0",
> "arm,psci-0.2" means "complies to PSCI 0.2",
> and "arm,psci" means "complies to pre-0.2 PSCI"
> 
> If you want to drop "arm,psci" then you should be arguing why
> we're not compliant with pre-0.2 PSCI. Maybe we aren't and we
> shouldn't be advertising it, but you need more rationale than
> "dt-validate complained".

Yes I agree, and that's my mistake. Rob already relaxed the bindings
https://lore.kernel.org/all/20220803201639.2552581-1-robh@kernel.org/
but that's queued for v6.1 and I was validating against mainline.
I'll drop the patch

Thanks,
Jean

[PATCH 03/10] hw/arm/virt: Fix devicetree warnings about the GIC node

[Jean-Philippe Brucker]

Fix three dt-validate warnings about the GIC node due to invalid names
and missing property:

  intc@8000000: $nodename:0: 'intc@8000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
  intc@8000000: 'its@8080000' does not match any of the regexes: '^(msi-controller|gic-its|interrupt-controller)@[0-9a-f]+$', '^gic-its@', '^interrupt-controller@[0-9a-f]+$', 'pinctrl-[0-9]+'

  interrupt-controller@8000000: msi-controller@8080000: '#msi-cells' is a required property
  From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index abcf2716bc..b6aa311d8c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -481,12 +481,13 @@ static void fdt_add_its_gic_node(VirtMachineState *vms)
     MachineState *ms = MACHINE(vms);
 
     vms->msi_phandle = qemu_fdt_alloc_phandle(ms->fdt);
-    nodename = g_strdup_printf("/intc/its@%" PRIx64,
+    nodename = g_strdup_printf("/interrupt-controller/msi-controller@%" PRIx64,
                                vms->memmap[VIRT_GIC_ITS].base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                             "arm,gic-v3-its");
     qemu_fdt_setprop(ms->fdt, nodename, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#msi-cells", 1);
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_ITS].base,
                                  2, vms->memmap[VIRT_GIC_ITS].size);
@@ -499,7 +500,7 @@ static void fdt_add_v2m_gic_node(VirtMachineState *vms)
     MachineState *ms = MACHINE(vms);
     char *nodename;
 
-    nodename = g_strdup_printf("/intc/v2m@%" PRIx64,
+    nodename = g_strdup_printf("/interrupt-controller/v2m@%" PRIx64,
                                vms->memmap[VIRT_GIC_V2M].base);
     vms->msi_phandle = qemu_fdt_alloc_phandle(ms->fdt);
     qemu_fdt_add_subnode(ms->fdt, nodename);
@@ -521,7 +522,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
     vms->gic_phandle = qemu_fdt_alloc_phandle(ms->fdt);
     qemu_fdt_setprop_cell(ms->fdt, "/", "interrupt-parent", vms->gic_phandle);
 
-    nodename = g_strdup_printf("/intc@%" PRIx64,
+    nodename = g_strdup_printf("/interrupt-controller@%" PRIx64,
                                vms->memmap[VIRT_GIC_DIST].base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_cell(ms->fdt, nodename, "#interrupt-cells", 3);
@@ -1651,7 +1652,7 @@ void virt_machine_done(Notifier *notifier, void *data)
      * while qemu takes charge of the qom stuff.
      */
     if (info->dtb_filename == NULL) {
-        platform_bus_add_all_fdt_nodes(ms->fdt, "/intc",
+        platform_bus_add_all_fdt_nodes(ms->fdt, "/interrupt-controller",
                                        vms->memmap[VIRT_PLATFORM_BUS].base,
                                        vms->memmap[VIRT_PLATFORM_BUS].size,
                                        vms->irqmap[VIRT_PLATFORM_BUS]);
-- 
2.37.1

Re: [PATCH 03/10] hw/arm/virt: Fix devicetree warnings about the GIC node

[Peter Maydell]

On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Fix three dt-validate warnings about the GIC node due to invalid names
> and missing property:
>
>   intc@8000000: $nodename:0: 'intc@8000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>   intc@8000000: 'its@8080000' does not match any of the regexes: '^(msi-controller|gic-its|interrupt-controller)@[0-9a-f]+$', '^gic-its@', '^interrupt-controller@[0-9a-f]+$', 'pinctrl-[0-9]+'
>
>   interrupt-controller@8000000: msi-controller@8080000: '#msi-cells' is a required property
>   From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml

Why is dt-validate complaining about the node names? Surely
anything looking for the ITS in the DT should be looking for
it by the "compatible" string ?

thanks
-- PMM

Re: [PATCH 03/10] hw/arm/virt: Fix devicetree warnings about the GIC node

[Jean-Philippe Brucker]

On Wed, Aug 24, 2022 at 08:36:33PM +0100, Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > Fix three dt-validate warnings about the GIC node due to invalid names
> > and missing property:
> >
> >   intc@8000000: $nodename:0: 'intc@8000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
> >   intc@8000000: 'its@8080000' does not match any of the regexes: '^(msi-controller|gic-its|interrupt-controller)@[0-9a-f]+$', '^gic-its@', '^interrupt-controller@[0-9a-f]+$', 'pinctrl-[0-9]+'
> >
> >   interrupt-controller@8000000: msi-controller@8080000: '#msi-cells' is a required property
> >   From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
> 
> Why is dt-validate complaining about the node names? Surely
> anything looking for the ITS in the DT should be looking for
> it by the "compatible" string ?

The device-tree specification, in 2.2.2 Generic Name Recommendation [1],
provides the node names. Given that the guest will look at compatible
strings, changing the name is safe.

Thanks,
Jean

[1] http://devicetree-org.github.io/devicetree-specification/index.html#generic-names-recommendation 

[PATCH 04/10] hw/arm/virt: Use "msi-map" devicetree property for PCI

[Jean-Philippe Brucker]

The "msi-parent" property can be used on the PCI node when MSIs do not
contain sideband data (a device ID) [1]. In QEMU, MSI transactions
contain the requester ID, so the PCI node should use the "msi-map"
property instead of "msi-parent". In our case the property describes an
identity map between requester ID and sideband data.

This fixes a warning when passing the DTB generated by QEMU to dtc,
following a recent change to the GICv3 node:

  Warning (msi_parent_property): /pcie@10000000:msi-parent: property size (4) too small for cell size 1

[1] linux/Documentation/devicetree/bindings/pci/pci-msi.txt

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b6aa311d8c..ca5d213895 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1489,8 +1489,8 @@ static void create_pcie(VirtMachineState *vms)
     qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
 
     if (vms->msi_phandle) {
-        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-parent",
-                               vms->msi_phandle);
+        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-map",
+                               0, vms->msi_phandle, 0, 0x10000);
     }
 
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
-- 
2.37.1

[PATCH 05/10] hw/arm/virt: Fix devicetree warning about the timer node

[Jean-Philippe Brucker]

The compatible property of the Arm timer should contain either
"arm,armv7-timer" or "arm,armv8-timer", not both.

  timer: compatible: 'oneOf' conditional failed, one must be fixed:
  	['arm,armv8-timer', 'arm,armv7-timer'] is too long
  From schema: linux/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ca5d213895..5935f32a44 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -344,7 +344,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
 
     armcpu = ARM_CPU(qemu_get_cpu(0));
     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
-        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
+        const char compat[] = "arm,armv8-timer";
         qemu_fdt_setprop(ms->fdt, "/timer", "compatible",
                          compat, sizeof(compat));
     } else {
-- 
2.37.1

Re: [PATCH 05/10] hw/arm/virt: Fix devicetree warning about the timer node

[Peter Maydell]

On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The compatible property of the Arm timer should contain either
> "arm,armv7-timer" or "arm,armv8-timer", not both.
>
>   timer: compatible: 'oneOf' conditional failed, one must be fixed:
>         ['arm,armv8-timer', 'arm,armv7-timer'] is too long
>   From schema: linux/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ca5d213895..5935f32a44 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -344,7 +344,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
>
>      armcpu = ARM_CPU(qemu_get_cpu(0));
>      if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> -        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
> +        const char compat[] = "arm,armv8-timer";
>          qemu_fdt_setprop(ms->fdt, "/timer", "compatible",
>                           compat, sizeof(compat));
>      } else {

Are we really sure there are no existing guests out there that are
looking for this device under "armv7-timer" ?

This used to be valid DT before Linux kernel commit 4d2bb3e65035954,
which changed from "should at least contain one of" to requiring
exactly one-of, and that was only in 2018.

thanks
-- PMM

Re: [PATCH 05/10] hw/arm/virt: Fix devicetree warning about the timer node

[Jean-Philippe Brucker]

On Wed, Aug 24, 2022 at 08:40:21PM +0100, Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > The compatible property of the Arm timer should contain either
> > "arm,armv7-timer" or "arm,armv8-timer", not both.
> >
> >   timer: compatible: 'oneOf' conditional failed, one must be fixed:
> >         ['arm,armv8-timer', 'arm,armv7-timer'] is too long
> >   From schema: linux/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  hw/arm/virt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ca5d213895..5935f32a44 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -344,7 +344,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
> >
> >      armcpu = ARM_CPU(qemu_get_cpu(0));
> >      if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> > -        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
> > +        const char compat[] = "arm,armv8-timer";
> >          qemu_fdt_setprop(ms->fdt, "/timer", "compatible",
> >                           compat, sizeof(compat));
> >      } else {
> 
> Are we really sure there are no existing guests out there that are
> looking for this device under "armv7-timer" ?

It's highly unlikely. It would take for example a 32-bit Linux from before
2013 or a 32-bit FreeBSD from before 2015, running on a machine with
ARM_FEATURE_V8. But I can't say for sure that no one is running such a
config, so I'll ask about relaxing the binding.

> 
> This used to be valid DT before Linux kernel commit 4d2bb3e65035954,
> which changed from "should at least contain one of" to requiring
> exactly one-of, and that was only in 2018.

Yes the text bindings weren't always exact.

Thanks,
Jean

[PATCH 06/10] hw/arm/virt: Fix devicetree warning about the gpio-key node

[Jean-Philippe Brucker]

The node name of the gpio-key devicetree node should be "key-poweroff":

  gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
  From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5935f32a44..3d460f3686 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -932,12 +932,12 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
     qemu_fdt_add_subnode(fdt, "/gpio-keys");
     qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
 
-    qemu_fdt_add_subnode(fdt, "/gpio-keys/poweroff");
-    qemu_fdt_setprop_string(fdt, "/gpio-keys/poweroff",
+    qemu_fdt_add_subnode(fdt, "/gpio-keys/key-poweroff");
+    qemu_fdt_setprop_string(fdt, "/gpio-keys/key-poweroff",
                             "label", "GPIO Key Poweroff");
-    qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
+    qemu_fdt_setprop_cell(fdt, "/gpio-keys/key-poweroff", "linux,code",
                           KEY_POWER);
-    qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
+    qemu_fdt_setprop_cells(fdt, "/gpio-keys/key-poweroff",
                            "gpios", phandle, 3, 0);
 }
 
-- 
2.37.1

[PATCH 07/10] hw/arm/virt: Fix devicetree warnings about node names

[Jean-Philippe Brucker]

dt-validate reports that several nodes have non-compliant names:

  pl061@9030000: $nodename:0: 'pl061@9030000' does not match '^gpio@[0-9a-f]+$'
  From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
  pl031@9010000: $nodename:0: 'pl031@9010000' does not match '^rtc(@.*|-[0-9a-f])*$'
  From schema: linux/Documentation/devicetree/bindings/rtc/arm,pl031.yaml
  pl011@9000000: $nodename:0: 'pl011@9000000' does not match '^serial(@.*)?$'
  From schema: linux/Documentation/devicetree/bindings/serial/pl011.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3d460f3686..952af37935 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -857,7 +857,7 @@ static void create_uart(const VirtMachineState *vms, int uart,
                                 sysbus_mmio_get_region(s, 0));
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
 
-    nodename = g_strdup_printf("/pl011@%" PRIx64, base);
+    nodename = g_strdup_printf("/serial@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     /* Note that we can't use setprop_string because of the embedded NUL */
     qemu_fdt_setprop(ms->fdt, nodename, "compatible",
@@ -897,7 +897,7 @@ static void create_rtc(const VirtMachineState *vms)
 
     sysbus_create_simple("pl031", base, qdev_get_gpio_in(vms->gic, irq));
 
-    nodename = g_strdup_printf("/pl031@%" PRIx64, base);
+    nodename = g_strdup_printf("/rtc@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
@@ -999,7 +999,7 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
 
     uint32_t phandle = qemu_fdt_alloc_phandle(ms->fdt);
-    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
+    nodename = g_strdup_printf("/gpio@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
-- 
2.37.1

[PATCH 08/10] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Jean-Philippe Brucker]

The GPIO devicetree node is missing "interrupt-controller" and
"#interrupt-cells" properties:

  pl061@9030000: 'interrupt-controller' is a required property
  From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
  pl061@9030000: '#interrupt-cells' is a required property
  From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 952af37935..779eb5ea31 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1012,6 +1012,8 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     qemu_fdt_setprop_cell(ms->fdt, nodename, "clocks", vms->clock_phandle);
     qemu_fdt_setprop_string(ms->fdt, nodename, "clock-names", "apb_pclk");
     qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle", phandle);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#interrupt-cells", 2);
+    qemu_fdt_setprop(ms->fdt, nodename, "interrupt-controller", NULL, 0);
 
     if (gpio != VIRT_GPIO) {
         /* Mark as not usable by the normal world */
-- 
2.37.1

Re: [PATCH 08/10] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Peter Maydell]

On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The GPIO devicetree node is missing "interrupt-controller" and
> "#interrupt-cells" properties:
>
>   pl061@9030000: 'interrupt-controller' is a required property
>   From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
>   pl061@9030000: '#interrupt-cells' is a required property
>   From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml

Why? This is a GPIO controller, not an interrupt controller.
It seems wrong to be advertising in the dtb that it is.

thanks
-- PMM

Re: [PATCH 08/10] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Jean-Philippe Brucker]

On Wed, Aug 24, 2022 at 08:48:26PM +0100, Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > The GPIO devicetree node is missing "interrupt-controller" and
> > "#interrupt-cells" properties:
> >
> >   pl061@9030000: 'interrupt-controller' is a required property
> >   From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
> >   pl061@9030000: '#interrupt-cells' is a required property
> >   From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
> 
> Why? This is a GPIO controller, not an interrupt controller.
> It seems wrong to be advertising in the dtb that it is.

pl061 can be used as an interrupt controller, and I think it's wired that
way, with GPIO key acting as an interrupt source. I'll add this to the
commit message.

Thanks,
Jean

[PATCH 09/10] hw/arm/virt: Fix devicetree warnings about the SMMU node

[Jean-Philippe Brucker]

dt-validate reports three issues in the SMMU device-tree node:

  smmuv3@9050000: $nodename:0: 'smmuv3@9050000' does not match '^iommu@[0-9a-f]*'
  smmuv3@9050000: interrupt-names: 'oneOf' conditional failed, one must be fixed:
  	['eventq', 'priq', 'cmdq-sync', 'gerror'] is too long
  	'combined' was expected
  	'gerror' was expected
  	'gerror' is not one of ['cmdq-sync', 'priq']
  smmuv3@9050000: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
  From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml

Fix them by:
* changing the node name
* reordering the IRQs
* removing the clock properties which are not expected for the SMMU node

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 779eb5ea31..de508d5329 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1329,7 +1329,9 @@ static void create_smmu(const VirtMachineState *vms,
     int i;
     hwaddr base = vms->memmap[VIRT_SMMU].base;
     hwaddr size = vms->memmap[VIRT_SMMU].size;
-    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
+    uint32_t irq_type = GIC_FDT_IRQ_TYPE_SPI;
+    uint32_t irq_trigger = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
+    const char irq_names[] = "eventq\0gerror\0priq\0cmdq-sync";
     DeviceState *dev;
     MachineState *ms = MACHINE(vms);
 
@@ -1348,22 +1350,20 @@ static void create_smmu(const VirtMachineState *vms,
                            qdev_get_gpio_in(vms->gic, irq + i));
     }
 
-    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
+    node = g_strdup_printf("/iommu@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, node);
     qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
 
     qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
-            GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
-            GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
-            GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
-            GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+            irq_type, irq + SMMU_IRQ_EVTQ,      irq_trigger,
+            irq_type, irq + SMMU_IRQ_GERROR,    irq_trigger,
+            irq_type, irq + SMMU_IRQ_PRIQ,      irq_trigger,
+            irq_type, irq + SMMU_IRQ_CMD_SYNC,  irq_trigger);
 
     qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
                      sizeof(irq_names));
 
-    qemu_fdt_setprop_cell(ms->fdt, node, "clocks", vms->clock_phandle);
-    qemu_fdt_setprop_string(ms->fdt, node, "clock-names", "apb_pclk");
     qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
 
     qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
-- 
2.37.1

Re: [PATCH 09/10] hw/arm/virt: Fix devicetree warnings about the SMMU node

[Peter Maydell]

On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> dt-validate reports three issues in the SMMU device-tree node:
>
>   smmuv3@9050000: $nodename:0: 'smmuv3@9050000' does not match '^iommu@[0-9a-f]*'
>   smmuv3@9050000: interrupt-names: 'oneOf' conditional failed, one must be fixed:
>         ['eventq', 'priq', 'cmdq-sync', 'gerror'] is too long
>         'combined' was expected
>         'gerror' was expected
>         'gerror' is not one of ['cmdq-sync', 'priq']
>   smmuv3@9050000: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
>
> Fix them by:
> * changing the node name
> * reordering the IRQs
> * removing the clock properties which are not expected for the SMMU node

Why does dt-validate insist on a fixed interrupt order here?

thanks
-- PMM

Re: [PATCH 09/10] hw/arm/virt: Fix devicetree warnings about the SMMU node

[Jean-Philippe Brucker]

On Wed, Aug 24, 2022 at 08:45:05PM +0100, Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > dt-validate reports three issues in the SMMU device-tree node:
> >
> >   smmuv3@9050000: $nodename:0: 'smmuv3@9050000' does not match '^iommu@[0-9a-f]*'
> >   smmuv3@9050000: interrupt-names: 'oneOf' conditional failed, one must be fixed:
> >         ['eventq', 'priq', 'cmdq-sync', 'gerror'] is too long
> >         'combined' was expected
> >         'gerror' was expected
> >         'gerror' is not one of ['cmdq-sync', 'priq']
> >   smmuv3@9050000: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
> >   From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
> >
> > Fix them by:
> > * changing the node name
> > * reordering the IRQs
> > * removing the clock properties which are not expected for the SMMU node
> 
> Why does dt-validate insist on a fixed interrupt order here?

I think the binding can be relaxed, since the driver must always look at
interrupt-names and can't assume a specific order (given that all except
gerror are now optional).

Thanks,
Jean

[PATCH 10/10] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Jean-Philippe Brucker]

dt-validate and dtc throw a few warnings when parsing the virtio-iommu
node:

  pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
  pcie@10000000: Unevaluated properties are not allowed (... 'virtio_iommu@16' were unexpected)
  From schema: linux/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
  pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
  From schema: dtschema/schemas/pci/pci-bus.yaml

  Warning (pci_device_reg): /pcie@10000000/virtio_iommu@16: PCI unit address format error, expected "2,0"

The compatible property for a PCI child node should follow the rules
from "PCI Bus Binding to: IEEE Std 1275-1994". It should contain the
Vendor ID and Device ID (or class code).

The unit-name should be "device,function".

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
Note that this doesn't follow
linux/Documentation/devicetree/bindings/virtio/iommu.txt, I'll update
that document when converting it to yaml, hopefully this Linux cycle.
The "virtio,pci-iommu" compatible string is not actually used by any
driver and only QEMU implements it, so we can get rid of it.
---
 hw/arm/virt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index de508d5329..08b79592eb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1374,14 +1374,15 @@ static void create_smmu(const VirtMachineState *vms,
 
 static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
 {
-    const char compat[] = "virtio,pci-iommu";
+    const char compat[] = "pci1af4,1057";
     uint16_t bdf = vms->virtio_iommu_bdf;
     MachineState *ms = MACHINE(vms);
     char *node;
 
     vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
 
-    node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf);
+    node = g_strdup_printf("%s/virtio_iommu@%x,%x", vms->pciehb_nodename,
+                           PCI_SLOT(bdf), PCI_FUNC(bdf));
     qemu_fdt_add_subnode(ms->fdt, node);
     qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg",
-- 
2.37.1

Re: [PATCH 10/10] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Peter Maydell]

On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> dt-validate and dtc throw a few warnings when parsing the virtio-iommu
> node:
>
>   pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
>   pcie@10000000: Unevaluated properties are not allowed (... 'virtio_iommu@16' were unexpected)
>   From schema: linux/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
>   pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
>   From schema: dtschema/schemas/pci/pci-bus.yaml
>
>   Warning (pci_device_reg): /pcie@10000000/virtio_iommu@16: PCI unit address format error, expected "2,0"
>
> The compatible property for a PCI child node should follow the rules
> from "PCI Bus Binding to: IEEE Std 1275-1994". It should contain the
> Vendor ID and Device ID (or class code).
>
> The unit-name should be "device,function".
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> Note that this doesn't follow
> linux/Documentation/devicetree/bindings/virtio/iommu.txt, I'll update
> that document when converting it to yaml, hopefully this Linux cycle.
> The "virtio,pci-iommu" compatible string is not actually used by any
> driver and only QEMU implements it, so we can get rid of it.

I'm not sure you can just change the compat string like that,
unless you can guarantee that nobody anywhere has ever
looked for it in a dtb. Also, "virtio,pci-iommu" is much
clearer than "pci1af4,1057"...

-- PMM

Re: [PATCH 10/10] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Jean-Philippe Brucker]

On Wed, Aug 24, 2022 at 08:51:18PM +0100, Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:51, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > dt-validate and dtc throw a few warnings when parsing the virtio-iommu
> > node:
> >
> >   pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
> >   pcie@10000000: Unevaluated properties are not allowed (... 'virtio_iommu@16' were unexpected)
> >   From schema: linux/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
> >   pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
> >   From schema: dtschema/schemas/pci/pci-bus.yaml
> >
> >   Warning (pci_device_reg): /pcie@10000000/virtio_iommu@16: PCI unit address format error, expected "2,0"
> >
> > The compatible property for a PCI child node should follow the rules
> > from "PCI Bus Binding to: IEEE Std 1275-1994". It should contain the
> > Vendor ID and Device ID (or class code).
> >
> > The unit-name should be "device,function".
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > Note that this doesn't follow
> > linux/Documentation/devicetree/bindings/virtio/iommu.txt, I'll update
> > that document when converting it to yaml, hopefully this Linux cycle.
> > The "virtio,pci-iommu" compatible string is not actually used by any
> > driver and only QEMU implements it, so we can get rid of it.
> 
> I'm not sure you can just change the compat string like that,
> unless you can guarantee that nobody anywhere has ever
> looked for it in a dtb. Also, "virtio,pci-iommu" is much
> clearer than "pci1af4,1057"...

I'm pretty sure nobody ever looked for it, but can't guarantee it. And yes
the PCI notation is hideous but that's what the standard requires. So I
think changing this to 'compatible = "virtio,pci-iommu", "pci1af4,1057"'
would be better.

Thanks,
Jean