Re: [PATCH] hw/arm/virt: Fix PL061 node name and properties

[Peter Maydell]

On Tue, 19 May 2020 at 09:49, Geert Uytterhoeven
<address@hidden> wrote:
> Make the created node comply with the PL061 Device Tree bindings:
>   - Use generic node name "gpio" instead of "pl061",
>   - Add missing "#interrupt-cells" and "interrupt-controller"
>     properties.

Where have these properties come from? They must be optional,
because in the version of the binding documentation from Linux
5.0 they're not described:
https://elixir.bootlin.com/linux/v5.0/source/Documentation/devicetree/bindings/gpio/pl061-gpio.txt

They seem to have magically appeared in kernel commit
910f38bed9439e765f7e, which purports to only be a change
of format from plain text to yaml but has added some
extra properties and claimed them to be mandatory.

Since the devicetree spec says that the interrupt-controller
property "defines a node as an interrupt controller node"
and a GPIO chip isn't an interrupt controller, this seems
like some kind of error in the dtb binding. Maybe I'm
missing something...

What actually goes wrong if QEMU doesn't specify these
properties?

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7dc96abf72cf2b9a..99593d7bce4d85cb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -818,13 +818,15 @@ static void create_gpio(const VirtMachineState *vms)
>                                       qdev_get_gpio_in(vms->gic, irq));
>
>      uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
> -    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
> +    nodename = g_strdup_printf("/gpio@%" PRIx64, base);

Does the devicetree binding really mandate what the node name is?
I thought that finding the right device was doe via the
'compatible' string and the nodename could be whatever the
device tree creator wanted.

thanks
-- PMM