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

[Ben Dooks]

On Mon, Aug 01, 2022 at 12:37:33PM +0100, Peter Maydell wrote:
> 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.

ok, will check

> > @@ -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?

I think the compat[] is fine, I should have probably added I also fixed
up to just call qemu_fdt_setprop_string()

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

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.