Re: [PATCH v3 5/5] hw/arm: change to use qemu_fdt_setprop_strings()

[Peter Maydell]

On Wed, 27 Jul 2022 at 23:44, Ben Dooks <qemu@ben.fluff.org> wrote:
>
> Change to using qemu_fdt_setprop_strings() instead of using
> \0 separated string arrays.
>
> Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> ---
>  hw/arm/boot.c             |  8 +++---
>  hw/arm/virt.c             | 28 +++++++++------------
>  hw/arm/xlnx-versal-virt.c | 51 ++++++++++++++++-----------------------
>  3 files changed, 37 insertions(+), 50 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ada2717f76..bf29b7ae60 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -490,11 +490,11 @@ static void fdt_add_psci_node(void *fdt)
>      qemu_fdt_add_subnode(fdt, "/psci");
>      if (armcpu->psci_version >= QEMU_PSCI_VERSION_0_2) {
>          if (armcpu->psci_version < QEMU_PSCI_VERSION_1_0) {
> -            const char comp[] = "arm,psci-0.2\0arm,psci";
> -            qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
> +            qemu_fdt_setprop_strings(fdt, "/psci", "compatible",
> +                                     "arm,psci-0.2", "arm,psci");

I think you may have some stray trailing whitespace here.
checkpatch should be able to tell you.

> @@ -858,8 +855,8 @@ static void create_uart(const VirtMachineState *vms, int 
> uart,
>      nodename = g_strdup_printf("/pl011@%" PRIx64, base);
>      qemu_fdt_add_subnode(ms->fdt, nodename);
>      /* Note that we can't use setprop_string because of the embedded NUL */

With this change, this comment becomes obsolete, and we should delete it too.

> -    qemu_fdt_setprop(ms->fdt, nodename, "compatible",
> -                         compat, sizeof(compat));
> +    qemu_fdt_setprop_strings(ms->fdt, nodename, "compatible",
> +                             "arm,pl011", "arm,primecell");
>      qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
>                                       2, base, 2, size);
>      qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",



> @@ -285,8 +280,6 @@ static void fdt_add_gem_nodes(VersalVirt *s)
>
>  static void fdt_add_zdma_nodes(VersalVirt *s)
>  {
> -    const char clocknames[] = "clk_main\0clk_apb";
> -    const char compat[] = "xlnx,zynqmp-dma-1.0";

This looks suspiciously like a pre-existing bug to me.
Alaistair, Edgar -- shouldn't this be a NUL-separated
'compatible' string, rather than a comma-separated one?

>      int i;
>
>      for (i = XLNX_VERSAL_NR_ADMAS - 1; i >= 0; i--) {
> @@ -298,22 +291,21 @@ static void fdt_add_zdma_nodes(VersalVirt *s)
>          qemu_fdt_setprop_cell(s->fdt, name, "xlnx,bus-width", 64);
>          qemu_fdt_setprop_cells(s->fdt, name, "clocks",
>                                 s->phandle.clk_25Mhz, s->phandle.clk_25Mhz);
> -        qemu_fdt_setprop(s->fdt, name, "clock-names",
> -                         clocknames, sizeof(clocknames));
> +        qemu_fdt_setprop_strings(s->fdt, name, "clock-names",
> +                                 "clk_main", "clk_apb");
>          qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
>                                 GIC_FDT_IRQ_TYPE_SPI, VERSAL_ADMA_IRQ_0 + i,
>                                 GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>          qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
>                                       2, addr, 2, 0x1000);
> -        qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat));
> +        qemu_fdt_setprop_string(s->fdt, name, "compatible",
> +                                "xlnx,zynqmp-dma-1.0");
>          g_free(name);
>      }
>  }
>
>  static void fdt_add_sd_nodes(VersalVirt *s)
>  {
> -    const char clocknames[] = "clk_xin\0clk_ahb";
> -    const char compat[] = "arasan,sdhci-8.9a";

Ditto here...

>      int i;
>
>      for (i = ARRAY_SIZE(s->soc.pmc.iou.sd) - 1; i >= 0; i--) {
> @@ -324,22 +316,21 @@ static void fdt_add_sd_nodes(VersalVirt *s)
>
>          qemu_fdt_setprop_cells(s->fdt, name, "clocks",
>                                 s->phandle.clk_25Mhz, s->phandle.clk_25Mhz);
> -        qemu_fdt_setprop(s->fdt, name, "clock-names",
> -                         clocknames, sizeof(clocknames));
> +        qemu_fdt_setprop_strings(s->fdt, name, "clock-names",
> +                                 "clk_xin", "clk_ahb");
>          qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
>                                 GIC_FDT_IRQ_TYPE_SPI, VERSAL_SD0_IRQ_0 + i * 
> 2,
>                                 GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>          qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
>                                       2, addr, 2, MM_PMC_SD0_SIZE);
> -        qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat));
> +        qemu_fdt_setprop_string(s->fdt, name, "compatible",
> +                                "arasan,sdhci-8.9a");
>          g_free(name);
>      }
>  }
>
>  static void fdt_add_rtc_node(VersalVirt *s)
>  {
> -    const char compat[] = "xlnx,zynqmp-rtc";

...and here.

> -    const char interrupt_names[] = "alarm\0sec";
>      char *name = g_strdup_printf("/rtc@%x", MM_PMC_RTC);
>
>      qemu_fdt_add_subnode(s->fdt, name);
> @@ -349,11 +340,11 @@ static void fdt_add_rtc_node(VersalVirt *s)
>                             GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>                             GIC_FDT_IRQ_TYPE_SPI, VERSAL_RTC_SECONDS_IRQ,
>                             GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> -    qemu_fdt_setprop(s->fdt, name, "interrupt-names",
> -                     interrupt_names, sizeof(interrupt_names));
> +    qemu_fdt_setprop_strings(s->fdt, name, "interrupt-names",
> +                             "alarm", "sec");
>      qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
>                                   2, MM_PMC_RTC, 2, MM_PMC_RTC_SIZE);
> -    qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_string(s->fdt, name, "compatible", "xlnx,zynqmp-rtc");
>      g_free(name);
>  }

thanks
-- PMM