diff mbox series

[v2,5/8] hw/arm/virt: Fix devicetree warnings about the GPIO node

Message ID 20220927100347.176606-6-jean-philippe@linaro.org
State New
Headers show
Series hw/arm/virt: Fix dt-schema warnings | expand

Commit Message

Jean-Philippe Brucker Sept. 27, 2022, 10:03 a.m. UTC
Since the pl061 device can be used as interrupt controller, its node
should contain "interrupt-controller" and "#interrupt-cells" properties.
Fix the corresponding dt-validate warnings:

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

Comments

Peter Maydell Sept. 27, 2022, 11:25 a.m. UTC | #1
On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Since the pl061 device can be used as interrupt controller, its node
> should contain "interrupt-controller" and "#interrupt-cells" properties.

It *can* be, but this PL061 is *not* an interrupt controller.
I don't see any reason why we should claim so in the DT.

thanks
-- PMM
Rob Herring (Arm) Nov. 29, 2022, 8:55 p.m. UTC | #2
On Tue, Sep 27, 2022 at 6:25 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > Since the pl061 device can be used as interrupt controller, its node
> > should contain "interrupt-controller" and "#interrupt-cells" properties.
>
> It *can* be, but this PL061 is *not* an interrupt controller.
> I don't see any reason why we should claim so in the DT.

Taking another look, it is an interrupt controller. The GPIOs are
connected to the 'gpio-keys' node which is interrupt based (there's a
polled version too). That binding happens to be pretty lax and allows
the GPIO to be specified either with 'gpios' or 'interrupts' property.
The Linux PL061 driver happens to work only because it always
registers an interrupt controller regardless of having
"interrupt-controller" and "#interrupt-cells" properties or not.

Rob
Peter Maydell Nov. 30, 2022, 12:27 p.m. UTC | #3
On Tue, 29 Nov 2022 at 20:56, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Sep 27, 2022 at 6:25 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> > <jean-philippe@linaro.org> wrote:
> > >
> > > Since the pl061 device can be used as interrupt controller, its node
> > > should contain "interrupt-controller" and "#interrupt-cells" properties.
> >
> > It *can* be, but this PL061 is *not* an interrupt controller.
> > I don't see any reason why we should claim so in the DT.
>
> Taking another look, it is an interrupt controller. The GPIOs are
> connected to the 'gpio-keys' node which is interrupt based (there's a
> polled version too). That binding happens to be pretty lax and allows
> the GPIO to be specified either with 'gpios' or 'interrupts' property.
> The Linux PL061 driver happens to work only because it always
> registers an interrupt controller regardless of having
> "interrupt-controller" and "#interrupt-cells" properties or not.

No, it really isn't an interrupt controller. The interrupt controller
in this system is the GIC. The PL061 is a GPIO controller. It *has*
an interrupt line, which it connects to the GIC, but that doesn't make
it an interrupt controller any more than it makes a UART an interrupt
controller. It just means you can use the GPIOs in either a polled
or an interrupt-driven mode, same as you can use a PL011 UART in
either polled or interrupt-driven mode. And the guest knows it
can do that because we've told it "this is a PL061" and that's part
of the PL061's functionality.

The gpio-keys stuff is just "here is a wire which is tracking
the state of an emulated power button". This isn't an interrupt,
it's just a wire that has some status the guest probably
cares about.

The second PL061 which this bit of dtb-generation code also
applies to happens to currently be being used purely for
output GPIOs, so calling that one an interrupt controller
makes even less sense.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6805c57530..10ce66c722 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 */