| Message ID | 20200519084904.1069-1-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Fix PL061 node name and properties | expand |
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 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
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
On Fri, 22 May 2020 at 09:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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. Yeah, but production QEMU is out there in the world based on the old DT binding documentation. So you can't unilaterally make a part of the binding that wasn't documented and that QEMU didn't emit mandatory. It might be preferable for new QEMU to emit it, of course. > 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 This is just saying "the yaml says these things are mandatory". You could equally get rid of them by marking them optional in the yaml, right? Also, complaining about the nodename seems like a bug in the validation: it is not a mandatory part of the spec, just a recommendation. > > 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. By that logic the PL011 UART is an interrupt controller, because it can assert its interrupt output based on activity on the serial port input lines. A GPIO controller is not an interrupt controller inherently. Maybe you can use it in a system design as an interrupt controller if you want to, and in that system's dtb perhaps it would make sense to label it as one, but the virt board's PL061 is in no way an interrupt controller -- it's just a GPIO controller. > > 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. OK. But why would we want to run an interrupt line through the GPIO controller when we have a perfectly good interrupt controller in the system already? It might be reasonable to add the properties now to avoid setting a bear trap for ourselves in future; on the other hand if running interrupt lines through the GPIO controller doesn't work then it acts as a nudge to stop people adding devices that are wired up in a weird way. > > > 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. OK, then we don't need to change the node name here. Lots of the other devices on the virt board have node names that happen to use the device name rather than being more generic. thanks -- PMM
On 5/22/20 11:30 AM, Peter Maydell wrote: > On Fri, 22 May 2020 at 09:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> 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. >>> [...] >>> 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. > > By that logic the PL011 UART is an interrupt controller, because > it can assert its interrupt output based on activity on the serial > port input lines. Yes :) > A GPIO controller is not an interrupt controller inherently. > Maybe you can use it in a system design as an interrupt > controller if you want to, and in that system's dtb perhaps > it would make sense to label it as one, but the virt board's > PL061 is in no way an interrupt controller -- it's just a GPIO > controller. > >>> 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. > > OK. But why would we want to run an interrupt line through the GPIO > controller when we have a perfectly good interrupt controller in > the system already? This is sometimes done on embedded devices when all the INTC lines are already wired. You'd use extra lines on free peripherals. Usually the peripheral offer a limited GPIO mode as passthru interrupt, else you use nasty hacks... > > It might be reasonable to add the properties now to avoid setting > a bear trap for ourselves in future; on the other hand if running > interrupt lines through the GPIO controller doesn't work then it > acts as a nudge to stop people adding devices that are wired > up in a weird way. [...]
Hi Peter, CC Rob, who made these properties mandatory in the pl061 DT bindings. On Fri, May 22, 2020 at 11:30 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Fri, 22 May 2020 at 09:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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. > > Yeah, but production QEMU is out there in the world based on > the old DT binding documentation. So you can't unilaterally > make a part of the binding that wasn't documented and that QEMU > didn't emit mandatory. It might be preferable for new QEMU to > emit it, of course. Indeed, else QEMU will differ (more) from real hardware. Most PL061 GPIO controllers in DTS files in upstream Linux are marked as interrupt controllers (a few haven't been updated yet). > > 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 > > This is just saying "the yaml says these things are mandatory". > You could equally get rid of them by marking them optional in > the yaml, right? > > Also, complaining about the nodename seems like a bug in the > validation: it is not a mandatory part of the spec, just a > recommendation. I'll defer that one to Rob, too. > > > 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. > > By that logic the PL011 UART is an interrupt controller, because > it can assert its interrupt output based on activity on the serial > port input lines. Doh, bad wording on my side :-) > A GPIO controller is not an interrupt controller inherently. > Maybe you can use it in a system design as an interrupt > controller if you want to, and in that system's dtb perhaps > it would make sense to label it as one, but the virt board's > PL061 is in no way an interrupt controller -- it's just a GPIO > controller. Actually it is: the virt board has a gpio-keys node that ties the Poweroff key to a PL061 GPIO. The Linux gpio-keys driver relies on interrupts to detect key events (for IRQ incapable GPIOs you have to use "gpio-keys-polled" instead). This can easily be verified by looking at /proc/interrupts on the guest: 48: 0 9030000.gpio 3 Edge GPIO Key Poweroff (Unfortunately I don't know how to trigger a virtual key press in the guest) > > > 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. Seems I was wrong: arm/virt does have the Poweroff key, and Linux does find the interrupt. > OK. But why would we want to run an interrupt line through the GPIO > controller when we have a perfectly good interrupt controller in > the system already? Because the GIC (usually?) does not have its interrupt lines brought outside the SoC, but defers external interrupt handling to a specialized external IRQ handling block, or to a GPIO controller with interrupt capabilities (or to PCI MSI). If you have an external (non-PCI) device that needs to trigger an interrupt (e.g. an I2C sensor), its to be tied to the specialized block, or to a GPIO controller. > It might be reasonable to add the properties now to avoid setting > a bear trap for ourselves in future; on the other hand if running > interrupt lines through the GPIO controller doesn't work then it > acts as a nudge to stop people adding devices that are wired > up in a weird way. There are plenty of working examples for this. > > > > 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. > > OK, then we don't need to change the node name here. Lots > of the other devices on the virt board have node names that > happen to use the device name rather than being more generic. Obviously these can (and IMHO should) be fixed, too, together with all other validation issues. Thanks! Rob: full thread at https://lore.kernel.org/qemu-devel/20200519084904.1069-1-geert+renesas@glider.be/ Gr{oetje,eeting}s, Geert
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); qemu_fdt_add_subnode(vms->fdt, nodename); qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg", 2, base, 2, size); qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat)); qemu_fdt_setprop_cell(vms->fdt, nodename, "#gpio-cells", 2); qemu_fdt_setprop(vms->fdt, nodename, "gpio-controller", NULL, 0); + qemu_fdt_setprop_cell(vms->fdt, nodename, "#interrupt-cells", 2); + qemu_fdt_setprop(vms->fdt, nodename, "interrupt-controller", NULL, 0); qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts", GIC_FDT_IRQ_TYPE_SPI, irq, GIC_FDT_IRQ_FLAGS_LEVEL_HI);
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. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Split off from "[PATCH QEMU v2 2/5] ARM: PL061: Extract pl061_create_fdt()" (https://lore.kernel.org/r/20200423090118.11199-3-geert+renesas@glider.be). --- hw/arm/virt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)