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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Peter,

On Thu, May 21, 2020 at 6:59 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Tue, 19 May 2020 at 09:49, Geert Uytterhoeven
> <geert+renesas@glider.be> 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

Many old DT bindings are incomplete.

> 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.

The main motivation behind the conversion from plain text to yaml is to
do automatic validation of DTS files, based on the bindings.  During the
conversion process, many issues are detected, and fixed; not only in the
DTS files, but also in the bindings (e.g. missing properties, like in
this case).

When running the validation on a device tree passed to the guest
(extracted from /sys/firmware/devicetree/base, converted to dts, and
 manually fixed up the phandles), the following is reported about the
pl061 node:

    virt.dt.yaml: pl061@9030000: {'reg': [[0, 151191552, 0, 4096]],
'gpio-controller': True, 'phandle': [[32771]], '#gpio-cells': [[2]],
'clocks': [[32768]], '#interrupt-cells': [[2]], 'compatible':
['arm,pl061', 'arm,primecell'], 'clock-names': ['apb_pclk'],
'$nodename': ['pl061@9030000']} is not valid under any of the given
schemas
    [...]
            virt.dt.yaml: pl061@9030000: 'interrupts' is a required property

    virt.dt.yaml: pl061@9030000: $nodename:0: 'pl061@9030000' does not
match '^gpio@[0-9a-f]+$'
    virt.dt.yaml: pl061@9030000: 'interrupt-controller' is a required property

> 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...

PL061 is an interrupt controller, as it can assert its interrupt output
based on activity on GPIO input lines.

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

It means that other devices that have their interrupt output connected
to a PL061 GPIO input won't work, as their driver in the guest OS cannot
find the interrupt.  Note that arm/virt.c currently doesn't instantiate
such devices.

> > 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.

Matching is indeed done based on compatible value.

For node names, please see
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3

   "2.2.2 Generic Names Recommendation

    The name of a node should be somewhat generic, reflecting the
    function of the device and not its precise programming model. If
    appropriate, the name should be one of the following choices:

    [...]

    - gpio

    [...]"

While many new generic names have been added recently, "gpio" was
already documented in the Power.orgTM Standard for Embedded Power
ArchitectureTM Platform Requirements (ePAPR).

I hope the above explains the rationale behind these change better.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds