All of lore.kernel.org
 help / color / mirror / Atom feed
* add qemu_fdt_setprop_strings() and use it in most places
@ 2022-07-27 22:39 Ben Dooks
  2022-07-27 22:39 ` [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper Ben Dooks
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair Francis, qemu-arm

Add a helper for qemu_fdt_setprop_strings() to take a set of strings
to put into a device-tree, which removes several open-coded methods
such as setting an char arr[] = {..} or setting char val[] = "str\0str2";

This is for hw/arm, hw/mips and hw/riscv as well as a couple of cores. It
is not fully tested over all of those, I may not have caught all places
this is to be replaced.




^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper
  2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
@ 2022-07-27 22:39 ` Ben Dooks
  2022-07-28  9:22   ` Andrew Jones
  2022-07-27 22:39 ` [PATCH v3 2/5] hw/riscv: use qemu_fdt_setprop_strings() for string arrays Ben Dooks
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair Francis, qemu-arm, Ben Dooks

Add a helper to set a property from a set of strings
to reduce the following code:

    static const char * const clint_compat[2] = {
        "sifive,clint0", "riscv,clint0"
    };

    qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
        (char **)&clint_compat, ARRAY_SIZE(clint_compat));

Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
---
v3;
 - fix return value for the call
 - add better help text
v2:
 - fix node/path in comment
---
 include/sysemu/device_tree.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index ef060a9759..83bdfe390e 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -87,6 +87,25 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
 int qemu_fdt_setprop_string_array(void *fdt, const char *node_path,
                                   const char *prop, char **array, int len);
 
+/**
+ * qemu_fdt_setprop_strings: set a property from a set of strings
+ *
+ * @fdt: pointer to the dt blob
+ * @path: node name
+ * @prop: property array
+ *
+ * This is a helper for the qemu_fdt_setprop_string_array() function
+ * which takes a va-arg set of strings instead of having to setup a
+ * single use string array.
+ */
+#define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
+    ({ int __ret; do {                                          \
+        static const char * const __strs[] = { __VA_ARGS__ };   \
+        __ret = qemu_fdt_setprop_string_array(fdt, path, prop,  \
+                (char **)&__strs, ARRAY_SIZE(__strs));          \
+     } while(0); __ret; })
+
+
 int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
                              const char *property,
                              const char *target_node_path);
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 2/5] hw/riscv: use qemu_fdt_setprop_strings() for string arrays
  2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
  2022-07-27 22:39 ` [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper Ben Dooks
@ 2022-07-27 22:39 ` Ben Dooks
  2022-07-27 22:39 ` [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings() Ben Dooks
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair Francis, qemu-arm, Ben Dooks

Use the qemu_fdt_setprop_strings() in sifve_u.c to simplify the code.

Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
---
 hw/riscv/sifive_u.c | 18 +++++-------------
 hw/riscv/spike.c    |  7 ++-----
 hw/riscv/virt.c     | 32 ++++++++------------------------
 3 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index e4c814a3ea..dc112a253a 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -103,13 +103,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
     char *nodename;
     uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle = 1;
     uint32_t hfclk_phandle, rtcclk_phandle, phy_phandle;
-    static const char * const ethclk_names[2] = { "pclk", "hclk" };
-    static const char * const clint_compat[2] = {
-        "sifive,clint0", "riscv,clint0"
-    };
-    static const char * const plic_compat[2] = {
-        "sifive,plic-1.0.0", "riscv,plic0"
-    };
 
     if (ms->dtb) {
         fdt = s->fdt = load_device_tree(ms->dtb, &s->fdt_size);
@@ -221,11 +214,11 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
     nodename = g_strdup_printf("/soc/clint@%lx",
         (long)memmap[SIFIVE_U_DEV_CLINT].base);
     qemu_fdt_add_subnode(fdt, nodename);
-    qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
-        (char **)&clint_compat, ARRAY_SIZE(clint_compat));
     qemu_fdt_setprop_cells(fdt, nodename, "reg",
         0x0, memmap[SIFIVE_U_DEV_CLINT].base,
         0x0, memmap[SIFIVE_U_DEV_CLINT].size);
+    qemu_fdt_setprop_strings(fdt, nodename, "compatible",
+                             "sifive,clint0", "riscv,clint0");
     qemu_fdt_setprop(fdt, nodename, "interrupts-extended",
         cells, ms->smp.cpus * sizeof(uint32_t) * 4);
     g_free(cells);
@@ -279,8 +272,8 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
         (long)memmap[SIFIVE_U_DEV_PLIC].base);
     qemu_fdt_add_subnode(fdt, nodename);
     qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 1);
-    qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
-        (char **)&plic_compat, ARRAY_SIZE(plic_compat));
+    qemu_fdt_setprop_strings(fdt, nodename, "compatbile",
+                             "sifive,plic-1.0.0", "riscv,plic0");
     qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
     qemu_fdt_setprop(fdt, nodename, "interrupts-extended",
         cells, (ms->smp.cpus * 4 - 2) * sizeof(uint32_t));
@@ -426,8 +419,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
     qemu_fdt_setprop_cell(fdt, nodename, "interrupts", SIFIVE_U_GEM_IRQ);
     qemu_fdt_setprop_cells(fdt, nodename, "clocks",
         prci_phandle, PRCI_CLK_GEMGXLPLL, prci_phandle, PRCI_CLK_GEMGXLPLL);
-    qemu_fdt_setprop_string_array(fdt, nodename, "clock-names",
-        (char **)&ethclk_names, ARRAY_SIZE(ethclk_names));
+    qemu_fdt_setprop_strings(fdt, nodename, "clock-names", "pclk", "hclk");
     qemu_fdt_setprop(fdt, nodename, "local-mac-address",
         s->soc.gem.conf.macaddr.a, ETH_ALEN);
     qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 1);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index e41b6aa9f0..aa895779cd 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -59,9 +59,6 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
     uint32_t cpu_phandle, intc_phandle, phandle = 1;
     char *name, *mem_name, *clint_name, *clust_name;
     char *core_name, *cpu_name, *intc_name;
-    static const char * const clint_compat[2] = {
-        "sifive,clint0", "riscv,clint0"
-    };
 
     fdt = s->fdt = create_device_tree(&s->fdt_size);
     if (!fdt) {
@@ -159,8 +156,8 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
             (memmap[SPIKE_CLINT].size * socket);
         clint_name = g_strdup_printf("/soc/clint@%lx", clint_addr);
         qemu_fdt_add_subnode(fdt, clint_name);
-        qemu_fdt_setprop_string_array(fdt, clint_name, "compatible",
-            (char **)&clint_compat, ARRAY_SIZE(clint_compat));
+        qemu_fdt_setprop_strings(fdt, clint_name, "compatible",
+                                 "sifive,clint0", "riscv,clint0");
         qemu_fdt_setprop_cells(fdt, clint_name, "reg",
             0x0, clint_addr, 0x0, memmap[SPIKE_CLINT].size);
         qemu_fdt_setprop(fdt, clint_name, "interrupts-extended",
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..c6aaa611a6 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -261,11 +261,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
             intc_phandles[cpu]);
         if (riscv_feature(&s->soc[socket].harts[cpu].env,
                           RISCV_FEATURE_AIA)) {
-            static const char * const compat[2] = {
-                "riscv,cpu-intc-aia", "riscv,cpu-intc"
-            };
-            qemu_fdt_setprop_string_array(mc->fdt, intc_name, "compatible",
-                                      (char **)&compat, ARRAY_SIZE(compat));
+            qemu_fdt_setprop_strings(mc->fdt, intc_name, "compatible",
+                                     "riscv,cpu-intc-aia", "riscv,cpu-intc");
         } else {
             qemu_fdt_setprop_string(mc->fdt, intc_name, "compatible",
                 "riscv,cpu-intc");
@@ -310,9 +307,6 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
     uint32_t *clint_cells;
     unsigned long clint_addr;
     MachineState *mc = MACHINE(s);
-    static const char * const clint_compat[2] = {
-        "sifive,clint0", "riscv,clint0"
-    };
 
     clint_cells = g_new0(uint32_t, s->soc[socket].num_harts * 4);
 
@@ -326,9 +320,8 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
     clint_addr = memmap[VIRT_CLINT].base + (memmap[VIRT_CLINT].size * socket);
     clint_name = g_strdup_printf("/soc/clint@%lx", clint_addr);
     qemu_fdt_add_subnode(mc->fdt, clint_name);
-    qemu_fdt_setprop_string_array(mc->fdt, clint_name, "compatible",
-                                  (char **)&clint_compat,
-                                  ARRAY_SIZE(clint_compat));
+    qemu_fdt_setprop_strings(mc->fdt, clint_name, "compatible",
+                             "sifive,clint0", "riscv,clint0");
     qemu_fdt_setprop_cells(mc->fdt, clint_name, "reg",
         0x0, clint_addr, 0x0, memmap[VIRT_CLINT].size);
     qemu_fdt_setprop(mc->fdt, clint_name, "interrupts-extended",
@@ -437,9 +430,6 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     uint32_t *plic_cells;
     unsigned long plic_addr;
     MachineState *mc = MACHINE(s);
-    static const char * const plic_compat[2] = {
-        "sifive,plic-1.0.0", "riscv,plic0"
-    };
 
     if (kvm_enabled()) {
         plic_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2);
@@ -465,9 +455,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     qemu_fdt_add_subnode(mc->fdt, plic_name);
     qemu_fdt_setprop_cell(mc->fdt, plic_name,
         "#interrupt-cells", FDT_PLIC_INT_CELLS);
-    qemu_fdt_setprop_string_array(mc->fdt, plic_name, "compatible",
-                                  (char **)&plic_compat,
-                                  ARRAY_SIZE(plic_compat));
+    qemu_fdt_setprop_strings(mc->fdt, plic_name, "compatible",
+                             "sifive,plic-1.0.0", "riscv,plic0");
     qemu_fdt_setprop(mc->fdt, plic_name, "interrupt-controller", NULL, 0);
     qemu_fdt_setprop(mc->fdt, plic_name, "interrupts-extended",
         plic_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4);
@@ -881,13 +870,8 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
     name = g_strdup_printf("/soc/test@%lx",
         (long)memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(mc->fdt, name);
-    {
-        static const char * const compat[3] = {
-            "sifive,test1", "sifive,test0", "syscon"
-        };
-        qemu_fdt_setprop_string_array(mc->fdt, name, "compatible",
-                                      (char **)&compat, ARRAY_SIZE(compat));
-    }
+    qemu_fdt_setprop_strings(mc->fdt, name, "compatible",
+                             "sifive,test1", "sifive,test0", "syscon");
     qemu_fdt_setprop_cells(mc->fdt, name, "reg",
         0x0, memmap[VIRT_TEST].base, 0x0, memmap[VIRT_TEST].size);
     qemu_fdt_setprop_cell(mc->fdt, name, "phandle", test_phandle);
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings()
  2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
  2022-07-27 22:39 ` [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper Ben Dooks
  2022-07-27 22:39 ` [PATCH v3 2/5] hw/riscv: use qemu_fdt_setprop_strings() for string arrays Ben Dooks
@ 2022-07-27 22:39 ` Ben Dooks
  2022-08-01 11:30   ` Peter Maydell
  2022-07-27 22:39 ` [PATCH v3 4/5] hw/mips: " Ben Dooks
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair Francis, qemu-arm, Ben Dooks

Change to using the qemu_fdt_setprop_strings() helper in
hw/core code.

Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
---
 hw/core/guest-loader.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
index 391c875a29..203090503e 100644
--- a/hw/core/guest-loader.c
+++ b/hw/core/guest-loader.c
@@ -56,10 +56,8 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
     qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
 
     if (s->kernel) {
-        const char *compat[2] = { "multiboot,module", "multiboot,kernel" };
-        if (qemu_fdt_setprop_string_array(fdt, node, "compatible",
-                                          (char **) &compat,
-                                          ARRAY_SIZE(compat)) < 0) {
+        if (qemu_fdt_setprop_strings(fdt, node, "compatible",
+                                     "multiboot,module", "multiboot,kernel") < 0) {
             error_setg(errp, "couldn't set %s/compatible", node);
             return;
         }
@@ -69,10 +67,8 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
             }
         }
     } else if (s->initrd) {
-        const char *compat[2] = { "multiboot,module", "multiboot,ramdisk" };
-        if (qemu_fdt_setprop_string_array(fdt, node, "compatible",
-                                          (char **) &compat,
-                                          ARRAY_SIZE(compat)) < 0) {
+        if (qemu_fdt_setprop_strings(fdt, node, "compatible",
+                                     "multiboot,module", "multiboot,ramdisk") < 0) {
             error_setg(errp, "couldn't set %s/compatible", node);
             return;
         }
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 4/5] hw/mips: use qemu_fdt_setprop_strings()
  2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
                   ` (2 preceding siblings ...)
  2022-07-27 22:39 ` [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings() Ben Dooks
@ 2022-07-27 22:39 ` Ben Dooks
  2022-08-01 11:30   ` Peter Maydell
  2022-07-27 22:39 ` [PATCH v3 5/5] hw/arm: change to " Ben Dooks
  2022-07-27 22:48 ` add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
  5 siblings, 1 reply; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair Francis, qemu-arm, Ben Dooks

Change to using qemu_fdt_setprop_strings() helper in hw/mips.

Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
---
 hw/mips/boston.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index d2ab9da1a0..759f6daafe 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -515,9 +515,6 @@ static const void *create_fdt(BostonState *s,
     MachineState *mc = s->mach;
     uint32_t platreg_ph, gic_ph, clk_ph;
     char *name, *gic_name, *platreg_name, *stdout_name;
-    static const char * const syscon_compat[2] = {
-        "img,boston-platform-regs", "syscon"
-    };
 
     fdt = create_device_tree(dt_size);
     if (!fdt) {
@@ -608,9 +605,8 @@ static const void *create_fdt(BostonState *s,
     platreg_name = g_strdup_printf("/soc/system-controller@%" HWADDR_PRIx,
                                    memmap[BOSTON_PLATREG].base);
     qemu_fdt_add_subnode(fdt, platreg_name);
-    qemu_fdt_setprop_string_array(fdt, platreg_name, "compatible",
-                                 (char **)&syscon_compat,
-                                 ARRAY_SIZE(syscon_compat));
+    qemu_fdt_setprop_strings(fdt, platreg_name, "compatible",
+                             "img,boston-platform-regs", "syscon");
     qemu_fdt_setprop_cells(fdt, platreg_name, "reg",
                            memmap[BOSTON_PLATREG].base,
                            memmap[BOSTON_PLATREG].size);
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v3 5/5] hw/arm: change to use qemu_fdt_setprop_strings()
  2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
                   ` (3 preceding siblings ...)
  2022-07-27 22:39 ` [PATCH v3 4/5] hw/mips: " Ben Dooks
@ 2022-07-27 22:39 ` Ben Dooks
  2022-08-01 11:37   ` Peter Maydell
  2022-07-27 22:48 ` add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
  5 siblings, 1 reply; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair Francis, qemu-arm, Ben Dooks

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");     
         } else {
-            const char comp[] = "arm,psci-1.0\0arm,psci-0.2\0arm,psci";
-            qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
+            qemu_fdt_setprop_strings(fdt, "/psci", "compatible",
+                                     "arm,psci-1.0", "arm,psci-0.2", "arm,psci");
         }
 
         cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9633f822f3..2670c13ef1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -343,9 +343,8 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
 
     armcpu = ARM_CPU(qemu_get_cpu(0));
     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
-        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
-        qemu_fdt_setprop(ms->fdt, "/timer", "compatible",
-                         compat, sizeof(compat));
+        qemu_fdt_setprop_strings(ms->fdt, "/timer", "compatible",
+                                 "arm,armv8-timer", "arm,armv7-timer");
     } else {
         qemu_fdt_setprop_string(ms->fdt, "/timer", "compatible",
                                 "arm,armv7-timer");
@@ -843,8 +842,6 @@ static void create_uart(const VirtMachineState *vms, int uart,
     hwaddr base = vms->memmap[uart].base;
     hwaddr size = vms->memmap[uart].size;
     int irq = vms->irqmap[uart];
-    const char compat[] = "arm,pl011\0arm,primecell";
-    const char clocknames[] = "uartclk\0apb_pclk";
     DeviceState *dev = qdev_new(TYPE_PL011);
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     MachineState *ms = MACHINE(vms);
@@ -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 */
-    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",
@@ -867,8 +864,8 @@ static void create_uart(const VirtMachineState *vms, int uart,
                                GIC_FDT_IRQ_FLAGS_LEVEL_HI);
     qemu_fdt_setprop_cells(ms->fdt, nodename, "clocks",
                                vms->clock_phandle, vms->clock_phandle);
-    qemu_fdt_setprop(ms->fdt, nodename, "clock-names",
-                         clocknames, sizeof(clocknames));
+    qemu_fdt_setprop_strings(ms->fdt, nodename, "clock-names",
+                             "uartclk", "apb_pclk");
 
     if (uart == VIRT_UART) {
         qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", nodename);
@@ -890,14 +887,14 @@ static void create_rtc(const VirtMachineState *vms)
     hwaddr base = vms->memmap[VIRT_RTC].base;
     hwaddr size = vms->memmap[VIRT_RTC].size;
     int irq = vms->irqmap[VIRT_RTC];
-    const char compat[] = "arm,pl031\0arm,primecell";
     MachineState *ms = MACHINE(vms);
 
     sysbus_create_simple("pl031", base, qdev_get_gpio_in(vms->gic, irq));
 
     nodename = g_strdup_printf("/pl031@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
-    qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_strings(ms->fdt, nodename, "compatible",
+                             "arm,pl031", "arm,primecell");
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
     qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
@@ -983,7 +980,6 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     hwaddr base = vms->memmap[gpio].base;
     hwaddr size = vms->memmap[gpio].size;
     int irq = vms->irqmap[gpio];
-    const char compat[] = "arm,pl061\0arm,primecell";
     SysBusDevice *s;
     MachineState *ms = MACHINE(vms);
 
@@ -1001,7 +997,8 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
-    qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_strings(ms->fdt, nodename, "compatible",
+                             "arm,pl061", "arm,primecell");
     qemu_fdt_setprop_cell(ms->fdt, nodename, "#gpio-cells", 2);
     qemu_fdt_setprop(ms->fdt, nodename, "gpio-controller", NULL, 0);
     qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
@@ -1325,7 +1322,6 @@ static void create_smmu(const VirtMachineState *vms,
     int i;
     hwaddr base = vms->memmap[VIRT_SMMU].base;
     hwaddr size = vms->memmap[VIRT_SMMU].size;
-    const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
     DeviceState *dev;
     MachineState *ms = MACHINE(vms);
 
@@ -1355,8 +1351,8 @@ static void create_smmu(const VirtMachineState *vms,
             GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
             GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
 
-    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
-                     sizeof(irq_names));
+    qemu_fdt_setprop_strings(ms->fdt, node, "interrupt-names",
+                             "eventq", "priq", "cmdq-sync", "gerror");
 
     qemu_fdt_setprop_cell(ms->fdt, node, "clocks", vms->clock_phandle);
     qemu_fdt_setprop_string(ms->fdt, node, "clock-names", "apb_pclk");
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 37fc9b919c..2cfc11f0b2 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -153,7 +153,6 @@ static void fdt_add_timer_nodes(VersalVirt *s)
 
 static void fdt_add_usb_xhci_nodes(VersalVirt *s)
 {
-    const char clocknames[] = "bus_clk\0ref_clk";
     const char irq_name[] = "dwc_usb3";
     const char compatVersalDWC3[] = "xlnx,versal-dwc3";
     const char compatDWC3[] = "snps,dwc3";
@@ -165,8 +164,8 @@ static void fdt_add_usb_xhci_nodes(VersalVirt *s)
     qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
                                  2, MM_USB2_CTRL_REGS,
                                  2, MM_USB2_CTRL_REGS_SIZE);
-    qemu_fdt_setprop(s->fdt, name, "clock-names",
-                         clocknames, sizeof(clocknames));
+    qemu_fdt_setprop_strings(s->fdt, name, "clock-names",
+                             "bus_clk", "ref_clk");
     qemu_fdt_setprop_cells(s->fdt, name, "clocks",
                                s->phandle.clk_25Mhz, s->phandle.clk_125Mhz);
     qemu_fdt_setprop(s->fdt, name, "ranges", NULL, 0);
@@ -205,8 +204,6 @@ static void fdt_add_uart_nodes(VersalVirt *s)
 {
     uint64_t addrs[] = { MM_UART1, MM_UART0 };
     unsigned int irqs[] = { VERSAL_UART1_IRQ_0, VERSAL_UART0_IRQ_0 };
-    const char compat[] = "arm,pl011\0arm,sbsa-uart";
-    const char clocknames[] = "uartclk\0apb_pclk";
     int i;
 
     for (i = 0; i < ARRAY_SIZE(addrs); i++) {
@@ -215,16 +212,16 @@ static void fdt_add_uart_nodes(VersalVirt *s)
         qemu_fdt_setprop_cell(s->fdt, name, "current-speed", 115200);
         qemu_fdt_setprop_cells(s->fdt, name, "clocks",
                                s->phandle.clk_125Mhz, s->phandle.clk_125Mhz);
-        qemu_fdt_setprop(s->fdt, name, "clock-names",
-                         clocknames, sizeof(clocknames));
+        qemu_fdt_setprop_strings(s->fdt, name, "clock-names",
+                                 "uartclk", "apb_pclk");
 
         qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
                                GIC_FDT_IRQ_TYPE_SPI, irqs[i],
                                GIC_FDT_IRQ_FLAGS_LEVEL_HI);
         qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
                                      2, addrs[i], 2, 0x1000);
-        qemu_fdt_setprop(s->fdt, name, "compatible",
-                         compat, sizeof(compat));
+        qemu_fdt_setprop_strings(s->fdt, name, "compatible",
+                                 "arm,pl011", "arm,sbsa-uart");
         qemu_fdt_setprop(s->fdt, name, "u-boot,dm-pre-reloc", NULL, 0);
 
         if (addrs[i] == MM_UART0) {
@@ -251,8 +248,6 @@ static void fdt_add_gem_nodes(VersalVirt *s)
 {
     uint64_t addrs[] = { MM_GEM1, MM_GEM0 };
     unsigned int irqs[] = { VERSAL_GEM1_IRQ_0, VERSAL_GEM0_IRQ_0 };
-    const char clocknames[] = "pclk\0hclk\0tx_clk\0rx_clk";
-    const char compat_gem[] = "cdns,zynqmp-gem\0cdns,gem";
     int i;
 
     for (i = 0; i < ARRAY_SIZE(addrs); i++) {
@@ -266,8 +261,8 @@ static void fdt_add_gem_nodes(VersalVirt *s)
         qemu_fdt_setprop_cells(s->fdt, name, "clocks",
                                s->phandle.clk_25Mhz, s->phandle.clk_25Mhz,
                                s->phandle.clk_125Mhz, s->phandle.clk_125Mhz);
-        qemu_fdt_setprop(s->fdt, name, "clock-names",
-                         clocknames, sizeof(clocknames));
+        qemu_fdt_setprop_strings(s->fdt, name, "clock-names",
+                                 "pclk", "hclk","tx_clk", "rx_clk");
         qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
                                GIC_FDT_IRQ_TYPE_SPI, irqs[i],
                                GIC_FDT_IRQ_FLAGS_LEVEL_HI,
@@ -275,8 +270,8 @@ static void fdt_add_gem_nodes(VersalVirt *s)
                                GIC_FDT_IRQ_FLAGS_LEVEL_HI);
         qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
                                      2, addrs[i], 2, 0x1000);
-        qemu_fdt_setprop(s->fdt, name, "compatible",
-                         compat_gem, sizeof(compat_gem));
+        qemu_fdt_setprop_strings(s->fdt, name, "compatible",
+                                 "cdns,zynqmp-gem", "cdns,gem");
         qemu_fdt_setprop_cell(s->fdt, name, "#address-cells", 1);
         qemu_fdt_setprop_cell(s->fdt, name, "#size-cells", 0);
         g_free(name);
@@ -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";
     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";
     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";
-    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);
 }
 
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: add qemu_fdt_setprop_strings() and use it in most places
  2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
                   ` (4 preceding siblings ...)
  2022-07-27 22:39 ` [PATCH v3 5/5] hw/arm: change to " Ben Dooks
@ 2022-07-27 22:48 ` Ben Dooks
  5 siblings, 0 replies; 21+ messages in thread
From: Ben Dooks @ 2022-07-27 22:48 UTC (permalink / raw)
  To: Ben Dooks; +Cc: qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Wed, Jul 27, 2022 at 11:39:00PM +0100, Ben Dooks wrote:
> Add a helper for qemu_fdt_setprop_strings() to take a set of strings
> to put into a device-tree, which removes several open-coded methods
> such as setting an char arr[] = {..} or setting char val[] = "str\0str2";
> 
> This is for hw/arm, hw/mips and hw/riscv as well as a couple of cores. It
> is not fully tested over all of those, I may not have caught all places
> this is to be replaced.

I've added a branch at
     https://github.com/bendooks/qemu.git dev/fdt-string-array

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

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper
  2022-07-27 22:39 ` [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper Ben Dooks
@ 2022-07-28  9:22   ` Andrew Jones
  2022-07-28 10:41     ` Ben Dooks
  2022-08-10 12:22     ` Andrew Jones
  0 siblings, 2 replies; 21+ messages in thread
From: Andrew Jones @ 2022-07-28  9:22 UTC (permalink / raw)
  To: Ben Dooks; +Cc: qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Wed, Jul 27, 2022 at 11:39:01PM +0100, Ben Dooks wrote:
> Add a helper to set a property from a set of strings
> to reduce the following code:
> 
>     static const char * const clint_compat[2] = {
>         "sifive,clint0", "riscv,clint0"
>     };
> 
>     qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
>         (char **)&clint_compat, ARRAY_SIZE(clint_compat));
> 
> Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> ---
> v3;
>  - fix return value for the call
>  - add better help text
> v2:
>  - fix node/path in comment
> ---
>  include/sysemu/device_tree.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ef060a9759..83bdfe390e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -87,6 +87,25 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>  int qemu_fdt_setprop_string_array(void *fdt, const char *node_path,
>                                    const char *prop, char **array, int len);
>  
> +/**
> + * qemu_fdt_setprop_strings: set a property from a set of strings
> + *
> + * @fdt: pointer to the dt blob
> + * @path: node name
> + * @prop: property array
> + *
> + * This is a helper for the qemu_fdt_setprop_string_array() function
> + * which takes a va-arg set of strings instead of having to setup a
> + * single use string array.
> + */
> +#define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
> +    ({ int __ret; do {                                          \
> +        static const char * const __strs[] = { __VA_ARGS__ };   \
> +        __ret = qemu_fdt_setprop_string_array(fdt, path, prop,  \
> +                (char **)&__strs, ARRAY_SIZE(__strs));          \
> +     } while(0); __ret; })

The do { } while (0) shouldn't be necessary inside the ({}), but I
think we should change the places that are checking the return value
of qemu_fdt_setprop_string_array() to not check the return value,
because it will always be zero. qemu_fdt_setprop_string_array() calls
qemu_fdt_setprop() which exits QEMU on error. Then, after there are
no callers checking the return value we can change this macro to

#define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
    do {                                                       \
       static const char * const __strs[] = { __VA_ARGS__ };   \
       qemu_fdt_setprop_string_array(fdt, path, prop,          \
               (char **)&__strs, ARRAY_SIZE(__strs));          \
     } while(0)


Thanks,
drew


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper
  2022-07-28  9:22   ` Andrew Jones
@ 2022-07-28 10:41     ` Ben Dooks
  2022-08-10 12:22     ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Ben Dooks @ 2022-07-28 10:41 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Ben Dooks, qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Thu, Jul 28, 2022 at 11:22:27AM +0200, Andrew Jones wrote:
> On Wed, Jul 27, 2022 at 11:39:01PM +0100, Ben Dooks wrote:
> > Add a helper to set a property from a set of strings
> > to reduce the following code:
> > 
> >     static const char * const clint_compat[2] = {
> >         "sifive,clint0", "riscv,clint0"
> >     };
> > 
> >     qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
> >         (char **)&clint_compat, ARRAY_SIZE(clint_compat));
> > 
> > Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> > ---
> > v3;
> >  - fix return value for the call
> >  - add better help text
> > v2:
> >  - fix node/path in comment
> > ---
> >  include/sysemu/device_tree.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > index ef060a9759..83bdfe390e 100644
> > --- a/include/sysemu/device_tree.h
> > +++ b/include/sysemu/device_tree.h
> > @@ -87,6 +87,25 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
> >  int qemu_fdt_setprop_string_array(void *fdt, const char *node_path,
> >                                    const char *prop, char **array, int len);
> >  
> > +/**
> > + * qemu_fdt_setprop_strings: set a property from a set of strings
> > + *
> > + * @fdt: pointer to the dt blob
> > + * @path: node name
> > + * @prop: property array
> > + *
> > + * This is a helper for the qemu_fdt_setprop_string_array() function
> > + * which takes a va-arg set of strings instead of having to setup a
> > + * single use string array.
> > + */
> > +#define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
> > +    ({ int __ret; do {                                          \
> > +        static const char * const __strs[] = { __VA_ARGS__ };   \
> > +        __ret = qemu_fdt_setprop_string_array(fdt, path, prop,  \
> > +                (char **)&__strs, ARRAY_SIZE(__strs));          \
> > +     } while(0); __ret; })
> 
> The do { } while (0) shouldn't be necessary inside the ({}), but I
> think we should change the places that are checking the return value
> of qemu_fdt_setprop_string_array() to not check the return value,
> because it will always be zero. qemu_fdt_setprop_string_array() calls
> qemu_fdt_setprop() which exits QEMU on error. Then, after there are
> no callers checking the return value we can change this macro to

I think I did this due to the hw/ppc changes that /do/ check the
return code but are different enough to not be trivially changable.

I'll go back and make this the original do {} while() tongith and
post a v4 for people to look. The hw/ppc stuff can stay as is for
now.

Thank you for the review.

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

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings()
  2022-07-27 22:39 ` [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings() Ben Dooks
@ 2022-08-01 11:30   ` Peter Maydell
  2022-08-09 18:50     ` Ben Dooks
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2022-08-01 11:30 UTC (permalink / raw)
  To: Ben Dooks; +Cc: qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Wed, 27 Jul 2022 at 23:39, Ben Dooks <qemu@ben.fluff.org> wrote:
>
> Change to using the qemu_fdt_setprop_strings() helper in
> hw/core code.
>
> Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 4/5] hw/mips: use qemu_fdt_setprop_strings()
  2022-07-27 22:39 ` [PATCH v3 4/5] hw/mips: " Ben Dooks
@ 2022-08-01 11:30   ` Peter Maydell
  0 siblings, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2022-08-01 11:30 UTC (permalink / raw)
  To: Ben Dooks; +Cc: qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Wed, 27 Jul 2022 at 23:40, Ben Dooks <qemu@ben.fluff.org> wrote:
>
> Change to using qemu_fdt_setprop_strings() helper in hw/mips.
>
> Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/5] hw/arm: change to use qemu_fdt_setprop_strings()
  2022-07-27 22:39 ` [PATCH v3 5/5] hw/arm: change to " Ben Dooks
@ 2022-08-01 11:37   ` Peter Maydell
  2022-08-09 18:48     ` Ben Dooks
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2022-08-01 11:37 UTC (permalink / raw)
  To: Ben Dooks
  Cc: qemu-devel, qemu-riscv, Alistair Francis, qemu-arm, Edgar E. Iglesias

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/5] hw/arm: change to use qemu_fdt_setprop_strings()
  2022-08-01 11:37   ` Peter Maydell
@ 2022-08-09 18:48     ` Ben Dooks
  2022-08-09 19:22       ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: Ben Dooks @ 2022-08-09 18:48 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Ben Dooks, qemu-devel, qemu-riscv, Alistair Francis, qemu-arm,
	Edgar E. Iglesias

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.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings()
  2022-08-01 11:30   ` Peter Maydell
@ 2022-08-09 18:50     ` Ben Dooks
  2022-08-09 20:49       ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: Ben Dooks @ 2022-08-09 18:50 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Ben Dooks, qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Mon, Aug 01, 2022 at 12:30:22PM +0100, Peter Maydell wrote:
> On Wed, 27 Jul 2022 at 23:39, Ben Dooks <qemu@ben.fluff.org> wrote:
> >
> > Change to using the qemu_fdt_setprop_strings() helper in
> > hw/core code.
> >
> > Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> > ---
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I've had to make a second version, so assuming it'll need
reviewing again

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

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/5] hw/arm: change to use qemu_fdt_setprop_strings()
  2022-08-09 18:48     ` Ben Dooks
@ 2022-08-09 19:22       ` Peter Maydell
  2022-08-09 19:32         ` Mark Cave-Ayland
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2022-08-09 19:22 UTC (permalink / raw)
  To: Ben Dooks
  Cc: Ben Dooks, qemu-devel, qemu-riscv, Alistair Francis, qemu-arm,
	Edgar E. Iglesias

On Tue, 9 Aug 2022 at 19:48, Ben Dooks <ben@fluff.org> wrote:
>
> 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:
> > > @@ -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()

I guess if it's definitely supposed to be one string instead of two that's OK.

-- PMM


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 5/5] hw/arm: change to use qemu_fdt_setprop_strings()
  2022-08-09 19:22       ` Peter Maydell
@ 2022-08-09 19:32         ` Mark Cave-Ayland
  0 siblings, 0 replies; 21+ messages in thread
From: Mark Cave-Ayland @ 2022-08-09 19:32 UTC (permalink / raw)
  To: Peter Maydell, Ben Dooks
  Cc: Ben Dooks, qemu-devel, qemu-riscv, Alistair Francis, qemu-arm,
	Edgar E. Iglesias

On 09/08/2022 20:22, Peter Maydell wrote:

> On Tue, 9 Aug 2022 at 19:48, Ben Dooks <ben@fluff.org> wrote:
>>
>> 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:
>>>> @@ -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()
> 
> I guess if it's definitely supposed to be one string instead of two that's OK.

FWIW the compat strings look like they are using the older <manufacturer>,<device> 
notation similar to SUNW,tcx and SUNW,cgthree so if Xilinx is the manufacturer I 
think they are correct.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings()
  2022-08-09 18:50     ` Ben Dooks
@ 2022-08-09 20:49       ` Peter Maydell
  0 siblings, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2022-08-09 20:49 UTC (permalink / raw)
  To: Ben Dooks; +Cc: Ben Dooks, qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Tue, 9 Aug 2022 at 19:50, Ben Dooks <ben@fluff.org> wrote:
>
> On Mon, Aug 01, 2022 at 12:30:22PM +0100, Peter Maydell wrote:
> > On Wed, 27 Jul 2022 at 23:39, Ben Dooks <qemu@ben.fluff.org> wrote:
> > >
> > > Change to using the qemu_fdt_setprop_strings() helper in
> > > hw/core code.
> > >
> > > Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> > > ---
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I've had to make a second version, so assuming it'll need
> reviewing again

If you didn't change the specific patch that I gave a reviewed-by
tag for, then you should put that tag into the commit message
next to your signed-off-by line in the patch you send in the v2
series, to indicate it's already been reviewed and doesn't need
re-doing in v2.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper
  2022-07-28  9:22   ` Andrew Jones
  2022-07-28 10:41     ` Ben Dooks
@ 2022-08-10 12:22     ` Andrew Jones
  1 sibling, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2022-08-10 12:22 UTC (permalink / raw)
  To: Ben Dooks; +Cc: qemu-devel, qemu-riscv, Alistair Francis, qemu-arm

On Thu, Jul 28, 2022 at 11:22:27AM +0200, Andrew Jones wrote:
> On Wed, Jul 27, 2022 at 11:39:01PM +0100, Ben Dooks wrote:
> > Add a helper to set a property from a set of strings
> > to reduce the following code:
> > 
> >     static const char * const clint_compat[2] = {
> >         "sifive,clint0", "riscv,clint0"
> >     };
> > 
> >     qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
> >         (char **)&clint_compat, ARRAY_SIZE(clint_compat));
> > 
> > Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> > ---
> > v3;
> >  - fix return value for the call
> >  - add better help text
> > v2:
> >  - fix node/path in comment
> > ---
> >  include/sysemu/device_tree.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > index ef060a9759..83bdfe390e 100644
> > --- a/include/sysemu/device_tree.h
> > +++ b/include/sysemu/device_tree.h
> > @@ -87,6 +87,25 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
> >  int qemu_fdt_setprop_string_array(void *fdt, const char *node_path,
> >                                    const char *prop, char **array, int len);
> >  
> > +/**
> > + * qemu_fdt_setprop_strings: set a property from a set of strings
> > + *
> > + * @fdt: pointer to the dt blob
> > + * @path: node name
> > + * @prop: property array
> > + *
> > + * This is a helper for the qemu_fdt_setprop_string_array() function
> > + * which takes a va-arg set of strings instead of having to setup a
> > + * single use string array.
> > + */
> > +#define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
> > +    ({ int __ret; do {                                          \
> > +        static const char * const __strs[] = { __VA_ARGS__ };   \
> > +        __ret = qemu_fdt_setprop_string_array(fdt, path, prop,  \
> > +                (char **)&__strs, ARRAY_SIZE(__strs));          \
> > +     } while(0); __ret; })
> 
> The do { } while (0) shouldn't be necessary inside the ({}), but I
> think we should change the places that are checking the return value
> of qemu_fdt_setprop_string_array() to not check the return value,
> because it will always be zero. qemu_fdt_setprop_string_array() calls
> qemu_fdt_setprop() which exits QEMU on error. Then, after there are
> no callers checking the return value we can change this macro to
> 
> #define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
>     do {                                                       \
>        static const char * const __strs[] = { __VA_ARGS__ };   \

Eh, I just realized I didn't notice the static the first time I read this
and so I copy+pasted here in my suggestion. Sorry about that.

Thanks,
drew


>        qemu_fdt_setprop_string_array(fdt, path, prop,          \
>                (char **)&__strs, ARRAY_SIZE(__strs));          \
>      } while(0)
> 
> 
> Thanks,
> drew


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: add qemu_fdt_setprop_strings() and use it in most places
  2022-08-09 18:56 Ben Dooks
  2022-08-09 19:12 ` Ben Dooks
@ 2022-08-12 10:30 ` Peter Maydell
  1 sibling, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2022-08-12 10:30 UTC (permalink / raw)
  To: Ben Dooks; +Cc: qemu-devel, qemu-riscv, Alistair.Francis, qemu-arm

On Tue, 9 Aug 2022 at 19:57, Ben Dooks <qemu@ben.fluff.org> wrote:
>
> Add a helper for qemu_fdt_setprop_strings() to take a set of strings
> to put into a device-tree, which removes several open-coded methods
> such as setting an char arr[] = {..} or setting char val[] = "str\0str2";
>
> This is for hw/arm, hw/mips and hw/riscv as well as a couple of cores. It
> is not fully tested over all of those, I may not have caught all places
> this is to be replaced.

Hi; just as a minor process thing for next time you send a
patchset: the usual convention is that the 'cover letter'
email has a subject starting with a tag like "[PATCH v4 0/6]".
This makes it easier to pick the cover letter out when eyeballing
a set of subject lines, and also helps some of the automated
tooling (eg I think this is why https://patchew.org/QEMU/ didn't
notice this series). Usually 'git format-patch --cover-letter'
will get this right automatically for you.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: add qemu_fdt_setprop_strings() and use it in most places
  2022-08-09 18:56 Ben Dooks
@ 2022-08-09 19:12 ` Ben Dooks
  2022-08-12 10:30 ` Peter Maydell
  1 sibling, 0 replies; 21+ messages in thread
From: Ben Dooks @ 2022-08-09 19:12 UTC (permalink / raw)
  To: Ben Dooks
  Cc: qemu-devel, qemu-riscv, Alistair.Francis, peter.maydell, qemu-arm

On Tue, Aug 09, 2022 at 07:56:34PM +0100, Ben Dooks wrote:
> Add a helper for qemu_fdt_setprop_strings() to take a set of strings
> to put into a device-tree, which removes several open-coded methods
> such as setting an char arr[] = {..} or setting char val[] = "str\0str2";       
> 
> This is for hw/arm, hw/mips and hw/riscv as well as a couple of cores. It
> is not fully tested over all of those, I may not have caught all places
> this is to be replaced.                                                  

I've put a github repo up with this and possibly some other bits I am
working on:

https://github.com/bendooks/qemu/tree/dev/fdt-string-array-v4

> Changes:
> 
> v4:
>  - fix checkpatch issues
>  - remove error checking in hw/core/guest-loader.c
> v3:
>  - fix return value for the call                                                
>  - add better help text                                                         
> v2:                                                                             
>  - fix node/path in comment                                                     
> 
> 
> 

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

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* add qemu_fdt_setprop_strings() and use it in most places
@ 2022-08-09 18:56 Ben Dooks
  2022-08-09 19:12 ` Ben Dooks
  2022-08-12 10:30 ` Peter Maydell
  0 siblings, 2 replies; 21+ messages in thread
From: Ben Dooks @ 2022-08-09 18:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, Alistair.Francis, peter.maydell, qemu-arm

Add a helper for qemu_fdt_setprop_strings() to take a set of strings
to put into a device-tree, which removes several open-coded methods
such as setting an char arr[] = {..} or setting char val[] = "str\0str2";       

This is for hw/arm, hw/mips and hw/riscv as well as a couple of cores. It
is not fully tested over all of those, I may not have caught all places
this is to be replaced.                                                  

Changes:

v4:
 - fix checkpatch issues
 - remove error checking in hw/core/guest-loader.c
v3:
 - fix return value for the call                                                
 - add better help text                                                         
v2:                                                                             
 - fix node/path in comment                                                     




^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-08-12 10:35 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-27 22:39 add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
2022-07-27 22:39 ` [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper Ben Dooks
2022-07-28  9:22   ` Andrew Jones
2022-07-28 10:41     ` Ben Dooks
2022-08-10 12:22     ` Andrew Jones
2022-07-27 22:39 ` [PATCH v3 2/5] hw/riscv: use qemu_fdt_setprop_strings() for string arrays Ben Dooks
2022-07-27 22:39 ` [PATCH v3 3/5] hw/core: use qemu_fdt_setprop_strings() Ben Dooks
2022-08-01 11:30   ` Peter Maydell
2022-08-09 18:50     ` Ben Dooks
2022-08-09 20:49       ` Peter Maydell
2022-07-27 22:39 ` [PATCH v3 4/5] hw/mips: " Ben Dooks
2022-08-01 11:30   ` Peter Maydell
2022-07-27 22:39 ` [PATCH v3 5/5] hw/arm: change to " Ben Dooks
2022-08-01 11:37   ` Peter Maydell
2022-08-09 18:48     ` Ben Dooks
2022-08-09 19:22       ` Peter Maydell
2022-08-09 19:32         ` Mark Cave-Ayland
2022-07-27 22:48 ` add qemu_fdt_setprop_strings() and use it in most places Ben Dooks
2022-08-09 18:56 Ben Dooks
2022-08-09 19:12 ` Ben Dooks
2022-08-12 10:30 ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.