[PATCH v2 00/15] hw: Use QOM alias properties and few QOM/QDev cleanups

[PATCH v2 00/15] hw: Use QOM alias properties and few QOM/QDev cleanups

[Philippe Mathieu-Daudé]

Since v1:
- Addressed Markus & Zoltan review comments
- Introduce/use qdev_unrealize_and_unref()

These patches are extracted from a QOM/QDev refactor series,
so they are preliminary cleanups noticed while working on it:

- Use correct type when calling qdev_prop_set_xxx()
- Unify some qdev properties in MIPS models
- Replace intermediate properties by link properties
- Remove DEFINE_PROP_DMAADDR() macro which is used one time
- Use qdev_realize_and_unref() instead of open-coding it

Philippe Mathieu-Daudé (15):
  hw/pci/pcie_sriov: Replace fprintf(error_pretty) -> 
    warn_reportf_err()
  hw/qdev: Introduce qdev_unrealize_and_unref()
  linux-user/syscall: Do not open-code qdev_unrealize_and_unref()
  hw/pci/pcie_sriov: Do not open-code qdev_unrealize_and_unref()
  hw/i386/sgx: Do not open-code qdev_realize_and_unref()
  hw/ppc/sam460ex: Correctly set MAL properties
  hw/arm/nrf51: Alias 'flash-size' QOM property in SoC object
  hw/arm/fsl-imx: Alias 'phy-num' QOM property in SoC object
  hw/usb/hcd-ohci: Include missing 'sysbus.h' header
  hw/display/sm501: Embed OHCI QOM child in chipset
  hw/display/sm501: Alias 'dma-offset' QOM property in chipset object
  hw/display/sm501: Unify common QOM properties
  hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'
  hw/mips: Declare all length properties as unsigned
  hw/mips/itu: Pass SAAR using QOM link property

 hw/arm/fsl-imx25.c           |  3 +--
 hw/arm/fsl-imx6.c            |  3 +--
 hw/arm/fsl-imx6ul.c          |  8 ++++----
 hw/arm/fsl-imx7.c            | 12 ++++++------
 hw/arm/microbit.c            |  5 ++++-
 hw/arm/nrf51_soc.c           | 10 +---------
 hw/core/qdev.c               |  9 +++++++++
 hw/display/sm501.c           | 33 ++++++++++++++-------------------
 hw/i386/sgx.c                | 14 ++++++--------
 hw/intc/mips_gic.c           |  4 ++--
 hw/mips/boston.c             |  2 +-
 hw/mips/cps.c                | 35 ++++++++++++-----------------------
 hw/mips/malta.c              |  2 +-
 hw/misc/mips_cmgcr.c         |  2 +-
 hw/misc/mips_itu.c           | 30 ++++++++++++++++++++----------
 hw/nvram/nrf51_nvm.c         |  6 +++++-
 hw/pci/pcie_sriov.c          | 11 ++++-------
 hw/ppc/sam460ex.c            |  4 ++--
 hw/sh4/r2d.c                 |  2 +-
 hw/usb/hcd-ohci-pci.c        |  1 -
 hw/usb/hcd-ohci.c            |  3 +--
 hw/usb/hcd-ohci.h            |  1 +
 include/hw/arm/fsl-imx25.h   |  1 -
 include/hw/arm/fsl-imx6.h    |  1 -
 include/hw/arm/fsl-imx6ul.h  |  2 --
 include/hw/arm/fsl-imx7.h    |  1 -
 include/hw/arm/nrf51_soc.h   |  1 -
 include/hw/intc/mips_gic.h   |  4 ++--
 include/hw/misc/mips_cmgcr.h |  2 +-
 include/hw/misc/mips_itu.h   |  9 ++++-----
 include/hw/qdev-core.h       | 20 ++++++++++++++++++++
 include/hw/qdev-dma.h        | 16 ----------------
 linux-user/syscall.c         |  3 +--
 33 files changed, 125 insertions(+), 135 deletions(-)
 delete mode 100644 include/hw/qdev-dma.h

-- 
2.38.1

[PATCH v2 01/15] hw/pci/pcie_sriov: Replace fprintf(error_pretty) -> warn_reportf_err()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/pcie_sriov.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index f0bd72e069..93b0624599 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -196,19 +196,17 @@ static void register_vfs(PCIDevice *dev)
 
 static void unregister_vfs(PCIDevice *dev)
 {
-    Error *local_err = NULL;
     uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
     uint16_t i;
 
     trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
                                PCI_FUNC(dev->devfn), num_vfs);
     for (i = 0; i < num_vfs; i++) {
+        Error *errp = NULL;
         PCIDevice *vf = dev->exp.sriov_pf.vf[i];
-        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
-        if (local_err) {
-            fprintf(stderr, "Failed to unplug: %s\n",
-                    error_get_pretty(local_err));
-            error_free(local_err);
+        object_property_set_bool(OBJECT(vf), "realized", false, &errp);
+        if (errp) {
+            warn_reportf_err(errp, "Failed to unplug: ");
         }
         object_unparent(OBJECT(vf));
     }
-- 
2.38.1

Re: [PATCH v2 01/15] hw/pci/pcie_sriov: Replace fprintf(error_pretty) -> warn_reportf_err()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/pci/pcie_sriov.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index f0bd72e069..93b0624599 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -196,19 +196,17 @@ static void register_vfs(PCIDevice *dev)
>  
>  static void unregister_vfs(PCIDevice *dev)
>  {
> -    Error *local_err = NULL;
>      uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
>      uint16_t i;
>  
>      trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
>                                 PCI_FUNC(dev->devfn), num_vfs);
>      for (i = 0; i < num_vfs; i++) {
> +        Error *errp = NULL;
>          PCIDevice *vf = dev->exp.sriov_pf.vf[i];
> -        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
> -        if (local_err) {
> -            fprintf(stderr, "Failed to unplug: %s\n",
> -                    error_get_pretty(local_err));
> -            error_free(local_err);
> +        object_property_set_bool(OBJECT(vf), "realized", false, &errp);
> +        if (errp) {
> +            warn_reportf_err(errp, "Failed to unplug: ");
>          }
>          object_unparent(OBJECT(vf));
>      }

We use @errp for Error **, and @err, @local_err and similar for Error *.
Recommend to stick with @local_err here, i.e. just

  -            fprintf(stderr, "Failed to unplug: %s\n",
  -                    error_get_pretty(local_err));
  -            error_free(local_err);
  +            warn_reportf_err(local_err, "Failed to unplug: ");

[PATCH v2 02/15] hw/qdev: Introduce qdev_unrealize_and_unref()

[Philippe Mathieu-Daudé]

Inspired-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/qdev.c         |  9 +++++++++
 include/hw/qdev-core.h | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d759c4602c..b9ef793d51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -306,6 +306,15 @@ void qdev_unrealize(DeviceState *dev)
     object_property_set_bool(OBJECT(dev), "realized", false, &error_abort);
 }
 
+bool qdev_unrealize_and_unref(DeviceState *dev, Error **errp)
+{
+    bool ret;
+
+    ret = object_property_set_bool(OBJECT(dev), "realized", false, errp);
+    object_unref(OBJECT(dev));
+    return ret;
+}
+
 static int qdev_assert_realized_properly_cb(Object *obj, void *opaque)
 {
     DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index fd11e9a82f..20b9220023 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -374,6 +374,8 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
  * for the only reference to the child device to be held by the parent
  * via the child<> property, and so the reference-count-drop done here
  * would be incorrect. For that use case you want qdev_realize().
+ *
+ * Returns %true on success or %false if an error occured.
  */
 bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
 
@@ -397,6 +399,24 @@ bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
  * the life of the simulation and should not be unrealized and freed.
  */
 void qdev_unrealize(DeviceState *dev);
+
+/**
+ * qdev_unrealize_and_unref: Unrealize a device and drop a reference
+ * @dev: device to unrealize
+ * @errp: pointer to error object
+ *
+ * Unrealize @dev and drop a reference.
+ * This is like qdev_unrealize(), except the caller must hold a
+ * (private) reference, which is dropped on return regardless of
+ * success or failure.
+ *
+ * Warning: most devices in QEMU do not expect to be unrealized.  Only
+ * devices which are hot-unpluggable should be unrealized (as part of
+ * the unplugging process); all other devices are expected to last for
+ * the life of the simulation and should not be unrealized and freed.
+ */
+bool qdev_unrealize_and_unref(DeviceState *dev, Error **errp);
+
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
-- 
2.38.1

Re: [PATCH v2 02/15] hw/qdev: Introduce qdev_unrealize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Inspired-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 03/15] linux-user/syscall: Do not open-code qdev_unrealize_and_unref()

[Philippe Mathieu-Daudé]

(See commits dc3edf8d8a "Convert to qdev_unrealize() manually"
 and 981c3dcd94 "Convert to qdev_unrealize() with Coccinelle").

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 linux-user/syscall.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1f8c10f8ef..9299f71126 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8642,8 +8642,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
         if (CPU_NEXT(first_cpu)) {
             TaskState *ts = cpu->opaque;
 
-            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);
-            object_unref(OBJECT(cpu));
+            qdev_unrealize_and_unref(DEVICE(cpu), NULL);
             /*
              * At this point the CPU should be unrealized and removed
              * from cpu lists. We can clean-up the rest of the thread
-- 
2.38.1

Re: [PATCH v2 03/15] linux-user/syscall: Do not open-code qdev_unrealize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> (See commits dc3edf8d8a "Convert to qdev_unrealize() manually"
>  and 981c3dcd94 "Convert to qdev_unrealize() with Coccinelle").
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 04/15] hw/pci/pcie_sriov: Do not open-code qdev_unrealize_and_unref()

[Philippe Mathieu-Daudé]

(See commits dc3edf8d8a "Convert to qdev_unrealize() manually"
 and 981c3dcd94 "Convert to qdev_unrealize() with Coccinelle").

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/pcie_sriov.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 93b0624599..0b6101302b 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -204,11 +204,10 @@ static void unregister_vfs(PCIDevice *dev)
     for (i = 0; i < num_vfs; i++) {
         Error *errp = NULL;
         PCIDevice *vf = dev->exp.sriov_pf.vf[i];
-        object_property_set_bool(OBJECT(vf), "realized", false, &errp);
-        if (errp) {
+
+        if (!qdev_unrealize_and_unref(DEVICE(vf), &errp)) {
             warn_reportf_err(errp, "Failed to unplug: ");
         }
-        object_unparent(OBJECT(vf));
     }
     g_free(dev->exp.sriov_pf.vf);
     dev->exp.sriov_pf.vf = NULL;
-- 
2.38.1

Re: [PATCH v2 04/15] hw/pci/pcie_sriov: Do not open-code qdev_unrealize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> (See commits dc3edf8d8a "Convert to qdev_unrealize() manually"
>  and 981c3dcd94 "Convert to qdev_unrealize() with Coccinelle").
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/pci/pcie_sriov.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index 93b0624599..0b6101302b 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -204,11 +204,10 @@ static void unregister_vfs(PCIDevice *dev)
>      for (i = 0; i < num_vfs; i++) {
>          Error *errp = NULL;
>          PCIDevice *vf = dev->exp.sriov_pf.vf[i];
> -        object_property_set_bool(OBJECT(vf), "realized", false, &errp);
> -        if (errp) {
> +
> +        if (!qdev_unrealize_and_unref(DEVICE(vf), &errp)) {
>              warn_reportf_err(errp, "Failed to unplug: ");
>          }
> -        object_unparent(OBJECT(vf));
>      }
>      g_free(dev->exp.sriov_pf.vf);
>      dev->exp.sriov_pf.vf = NULL;

This replaces object_unref() by object_unparent().  Why is that okay?

[PATCH v2 05/15] hw/i386/sgx: Do not open-code qdev_realize_and_unref()

[Philippe Mathieu-Daudé]

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/sgx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index db004d17a6..6a729bff94 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -285,7 +285,6 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
     SGXEPCState *sgx_epc = &pcms->sgx_epc;
     X86MachineState *x86ms = X86_MACHINE(pcms);
     SgxEPCList *list = NULL;
-    Object *obj;
 
     memset(sgx_epc, 0, sizeof(SGXEPCState));
     if (!x86ms->sgx_epc_list) {
@@ -299,16 +298,15 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
                                 &sgx_epc->mr);
 
     for (list = x86ms->sgx_epc_list; list; list = list->next) {
-        obj = object_new("sgx-epc");
+        DeviceState *dev = qdev_new(TYPE_SGX_EPC);
 
         /* set the memdev link with memory backend */
-        object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
-                              &error_fatal);
+        object_property_parse(OBJECT(dev), SGX_EPC_MEMDEV_PROP,
+                              list->value->memdev, &error_fatal);
         /* set the numa node property for sgx epc object */
-        object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
-                             &error_fatal);
-        object_property_set_bool(obj, "realized", true, &error_fatal);
-        object_unref(obj);
+        object_property_set_uint(OBJECT(dev), SGX_EPC_NUMA_NODE_PROP,
+                                 list->value->node, &error_fatal);
+        qdev_realize_and_unref(dev, NULL, &error_fatal);
     }
 
     if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {
-- 
2.38.1

[PATCH v2 06/15] hw/ppc/sam460ex: Correctly set MAL properties

[Philippe Mathieu-Daudé]

MAL properties are declared as uint8_t:

  static Property ppc4xx_mal_properties[] = {
      DEFINE_PROP_UINT8("txc-num", Ppc4xxMalState, txcnum, 0),
      DEFINE_PROP_UINT8("rxc-num", Ppc4xxMalState, rxcnum, 0),
      DEFINE_PROP_END_OF_LIST(),
  };

Correct the API use by setting the property using
qdev_prop_set_uint8(). No behavioral change.

Fixes: da116a8aab ("ppc/ppc405: QOM'ify MAL")
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/sam460ex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4a22ce3761..cf065aae0e 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -389,8 +389,8 @@ static void sam460ex_init(MachineState *machine)
 
     /* MAL */
     dev = qdev_new(TYPE_PPC4xx_MAL);
-    qdev_prop_set_uint32(dev, "txc-num", 4);
-    qdev_prop_set_uint32(dev, "rxc-num", 16);
+    qdev_prop_set_uint8(dev, "txc-num", 4);
+    qdev_prop_set_uint8(dev, "rxc-num", 16);
     ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(dev), cpu, &error_fatal);
     object_unref(OBJECT(dev));
     sbdev = SYS_BUS_DEVICE(dev);
-- 
2.38.1

[PATCH v2 07/15] hw/arm/nrf51: Alias 'flash-size' QOM property in SoC object

[Philippe Mathieu-Daudé]

No need to use an intermediate 'flash-size' property in the
SoC object. Alias the property, so when the machine (here
microbit) sets the value on the SoC, it is propagated to
the flash object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/microbit.c          |  5 ++++-
 hw/arm/nrf51_soc.c         | 10 +---------
 hw/nvram/nrf51_nvm.c       |  6 +++++-
 include/hw/arm/nrf51_soc.h |  1 -
 4 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index 50df362088..79b5574884 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -36,6 +36,7 @@ static void microbit_init(MachineState *machine)
     MicrobitMachineState *s = MICROBIT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *mr;
+    int64_t flash_size;
 
     object_initialize_child(OBJECT(machine), "nrf51", &s->nrf51,
                             TYPE_NRF51_SOC);
@@ -43,6 +44,8 @@ static void microbit_init(MachineState *machine)
     object_property_set_link(OBJECT(&s->nrf51), "memory",
                              OBJECT(system_memory), &error_fatal);
     sysbus_realize(SYS_BUS_DEVICE(&s->nrf51), &error_fatal);
+    flash_size = object_property_get_int(OBJECT(&s->nrf51),
+                                         "flash-size", &error_abort);
 
     /*
      * Overlap the TWI stub device into the SoC.  This is a microbit-specific
@@ -57,7 +60,7 @@ static void microbit_init(MachineState *machine)
                                         mr, -1);
 
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
-                       0, s->nrf51.flash_size);
+                       0, flash_size);
 }
 
 static void microbit_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 34da0d62f0..cc4a636c51 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -24,9 +24,7 @@
  * are supported in the future, add a sub-class of NRF51SoC for
  * the specific variants
  */
-#define NRF51822_FLASH_PAGES    256
 #define NRF51822_SRAM_PAGES     16
-#define NRF51822_FLASH_SIZE     (NRF51822_FLASH_PAGES * NRF51_PAGE_SIZE)
 #define NRF51822_SRAM_SIZE      (NRF51822_SRAM_PAGES * NRF51_PAGE_SIZE)
 
 #define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
@@ -122,11 +120,6 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
                        BASE_TO_IRQ(NRF51_RNG_BASE)));
 
     /* UICR, FICR, NVMC, FLASH */
-    if (!object_property_set_uint(OBJECT(&s->nvm), "flash-size",
-                                  s->flash_size, errp)) {
-        return;
-    }
-
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->nvm), errp)) {
         return;
     }
@@ -199,6 +192,7 @@ static void nrf51_soc_init(Object *obj)
     object_initialize_child(obj, "rng", &s->rng, TYPE_NRF51_RNG);
 
     object_initialize_child(obj, "nvm", &s->nvm, TYPE_NRF51_NVM);
+    object_property_add_alias(obj, "flash-size", OBJECT(&s->nvm), "flash-size");
 
     object_initialize_child(obj, "gpio", &s->gpio, TYPE_NRF51_GPIO);
 
@@ -215,8 +209,6 @@ static Property nrf51_soc_properties[] = {
     DEFINE_PROP_LINK("memory", NRF51State, board_memory, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_UINT32("sram-size", NRF51State, sram_size, NRF51822_SRAM_SIZE),
-    DEFINE_PROP_UINT32("flash-size", NRF51State, flash_size,
-                       NRF51822_FLASH_SIZE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
index 7f1db8c423..bfae028fcd 100644
--- a/hw/nvram/nrf51_nvm.c
+++ b/hw/nvram/nrf51_nvm.c
@@ -26,6 +26,9 @@
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 
+#define NRF51822_FLASH_PAGES    256
+#define NRF51822_FLASH_SIZE     (NRF51822_FLASH_PAGES * NRF51_PAGE_SIZE)
+
 /*
  * FICR Registers Assignments
  * CODEPAGESIZE      0x010
@@ -358,7 +361,8 @@ static void nrf51_nvm_reset(DeviceState *dev)
 }
 
 static Property nrf51_nvm_properties[] = {
-    DEFINE_PROP_UINT32("flash-size", NRF51NVMState, flash_size, 0x40000),
+    DEFINE_PROP_UINT32("flash-size", NRF51NVMState,
+                       flash_size, NRF51822_FLASH_SIZE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e52a56e75e..8cf0c21614 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -45,7 +45,6 @@ struct NRF51State {
     MemoryRegion twi;
 
     uint32_t sram_size;
-    uint32_t flash_size;
 
     MemoryRegion *board_memory;
 
-- 
2.38.1

[PATCH v2 08/15] hw/arm/fsl-imx: Alias 'phy-num' QOM property in SoC object

[Philippe Mathieu-Daudé]

No need to use intermediate 'fec-phy-num' properties in the
SoC object. Alias the properties, so when the machines set
the value on the SoC, it is propagated to the network device
object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/fsl-imx25.c          |  3 +--
 hw/arm/fsl-imx6.c           |  3 +--
 hw/arm/fsl-imx6ul.c         |  8 ++++----
 hw/arm/fsl-imx7.c           | 12 ++++++------
 include/hw/arm/fsl-imx25.h  |  1 -
 include/hw/arm/fsl-imx6.h   |  1 -
 include/hw/arm/fsl-imx6ul.h |  2 --
 include/hw/arm/fsl-imx7.h   |  1 -
 8 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 24c4374590..eff58e1f7a 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -55,6 +55,7 @@ static void fsl_imx25_init(Object *obj)
     }
 
     object_initialize_child(obj, "fec", &s->fec, TYPE_IMX_FEC);
+    object_property_add_alias(obj, "fec-phy-num", OBJECT(&s->fec), "phy-num");
 
     object_initialize_child(obj, "rngc", &s->rngc, TYPE_IMX_RNGC);
 
@@ -169,7 +170,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
                                             epit_table[i].irq));
     }
 
-    object_property_set_uint(OBJECT(&s->fec), "phy-num", s->phy_num, &err);
     qdev_set_nic_properties(DEVICE(&s->fec), &nd_table[0]);
 
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->fec), errp)) {
@@ -315,7 +315,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx25_properties[] = {
-    DEFINE_PROP_UINT32("fec-phy-num", FslIMX25State, phy_num, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 00dafe3f62..4f870c928c 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -100,6 +100,7 @@ static void fsl_imx6_init(Object *obj)
 
 
     object_initialize_child(obj, "eth", &s->eth, TYPE_IMX_ENET);
+    object_property_add_alias(obj, "fec-phy-num", OBJECT(&s->eth), "phy-num");
 }
 
 static void fsl_imx6_realize(DeviceState *dev, Error **errp)
@@ -377,7 +378,6 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                             spi_table[i].irq));
     }
 
-    object_property_set_uint(OBJECT(&s->eth), "phy-num", s->phy_num, &err);
     qdev_set_nic_properties(DEVICE(&s->eth), &nd_table[0]);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->eth), errp)) {
         return;
@@ -451,7 +451,6 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx6_properties[] = {
-    DEFINE_PROP_UINT32("fec-phy-num", FslIMX6State, phy_num, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index d88d6cc1c5..8b3939e8c5 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -120,8 +120,12 @@ static void fsl_imx6ul_init(Object *obj)
      * Ethernet
      */
     for (i = 0; i < FSL_IMX6UL_NUM_ETHS; i++) {
+        g_autofree gchar *propname = g_strdup_printf("fec%d-phy-num", i + 1);
         snprintf(name, NAME_SIZE, "eth%d", i);
         object_initialize_child(obj, name, &s->eth[i], TYPE_IMX_ENET);
+        qdev_prop_set_uint32(DEVICE(&s->eth[i]), "phy-num", i);
+        object_property_add_alias(obj, propname,
+                                  OBJECT(&s->eth[i]), "phy-num");
     }
 
     /* USB */
@@ -424,8 +428,6 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
             FSL_IMX6UL_ENET2_TIMER_IRQ,
         };
 
-        object_property_set_uint(OBJECT(&s->eth[i]), "phy-num",
-                                 s->phy_num[i], &error_abort);
         object_property_set_uint(OBJECT(&s->eth[i]), "tx-ring-num",
                                  FSL_IMX6UL_ETH_NUM_TX_RINGS, &error_abort);
         qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
@@ -618,8 +620,6 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx6ul_properties[] = {
-    DEFINE_PROP_UINT32("fec1-phy-num", FslIMX6ULState, phy_num[0], 0),
-    DEFINE_PROP_UINT32("fec2-phy-num", FslIMX6ULState, phy_num[1], 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index afc7480799..df035c9314 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -102,8 +102,12 @@ static void fsl_imx7_init(Object *obj)
      * Ethernet
      */
     for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
-            snprintf(name, NAME_SIZE, "eth%d", i);
-            object_initialize_child(obj, name, &s->eth[i], TYPE_IMX_ENET);
+        g_autofree gchar *propname = g_strdup_printf("fec%d-phy-num", i + 1);
+        snprintf(name, NAME_SIZE, "eth%d", i);
+        object_initialize_child(obj, name, &s->eth[i], TYPE_IMX_ENET);
+        qdev_prop_set_uint32(DEVICE(&s->eth[i]), "phy-num", i);
+        object_property_add_alias(obj, propname,
+                                  OBJECT(&s->eth[i]), "phy-num");
     }
 
     /*
@@ -402,8 +406,6 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
             FSL_IMX7_ENET2_ADDR,
         };
 
-        object_property_set_uint(OBJECT(&s->eth[i]), "phy-num",
-                                 s->phy_num[i], &error_abort);
         object_property_set_uint(OBJECT(&s->eth[i]), "tx-ring-num",
                                  FSL_IMX7_ETH_NUM_TX_RINGS, &error_abort);
         qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
@@ -599,8 +601,6 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
 }
 
 static Property fsl_imx7_properties[] = {
-    DEFINE_PROP_UINT32("fec1-phy-num", FslIMX7State, phy_num[0], 0),
-    DEFINE_PROP_UINT32("fec2-phy-num", FslIMX7State, phy_num[1], 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
index 1b1086e945..e377f8e79a 100644
--- a/include/hw/arm/fsl-imx25.h
+++ b/include/hw/arm/fsl-imx25.h
@@ -66,7 +66,6 @@ struct FslIMX25State {
     MemoryRegion   rom[2];
     MemoryRegion   iram;
     MemoryRegion   iram_alias;
-    uint32_t       phy_num;
 };
 
 /**
diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 83291457cf..f7d1a94640 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -74,7 +74,6 @@ struct FslIMX6State {
     MemoryRegion   caam;
     MemoryRegion   ocram;
     MemoryRegion   ocram_alias;
-    uint32_t       phy_num;
 };
 
 
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index 7812e516a5..5217eeb8ff 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -88,8 +88,6 @@ struct FslIMX6ULState {
     MemoryRegion       caam;
     MemoryRegion       ocram;
     MemoryRegion       ocram_alias;
-
-    uint32_t           phy_num[FSL_IMX6UL_NUM_ETHS];
 };
 
 enum FslIMX6ULMemoryMap {
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 4e5e071864..16c68a4937 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -82,7 +82,6 @@ struct FslIMX7State {
     IMX7GPRState       gpr;
     ChipideaState      usb[FSL_IMX7_NUM_USBS];
     DesignwarePCIEHost pcie;
-    uint32_t           phy_num[FSL_IMX7_NUM_ETHS];
 };
 
 enum FslIMX7MemoryMap {
-- 
2.38.1

[PATCH v2 09/15] hw/usb/hcd-ohci: Include missing 'sysbus.h' header

[Philippe Mathieu-Daudé]

Avoid when including "hw/usb/hcd-ohci.h":

  hw/usb/hcd-ohci.h:100:5: error: unknown type name 'SysBusDevice'
      SysBusDevice parent_obj;
      ^

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/usb/hcd-ohci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
index 11ac57058d..e5e6b434fd 100644
--- a/hw/usb/hcd-ohci.h
+++ b/hw/usb/hcd-ohci.h
@@ -21,6 +21,7 @@
 #ifndef HCD_OHCI_H
 #define HCD_OHCI_H
 
+#include "hw/sysbus.h"
 #include "sysemu/dma.h"
 #include "hw/usb.h"
 #include "qom/object.h"
-- 
2.38.1

Re: [PATCH v2 09/15] hw/usb/hcd-ohci: Include missing 'sysbus.h' header

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Avoid when including "hw/usb/hcd-ohci.h":
>
>   hw/usb/hcd-ohci.h:100:5: error: unknown type name 'SysBusDevice'
>       SysBusDevice parent_obj;
>       ^
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

This has become commit 4713720a78c56ec2e00bcb4abcac71e99315e4ea.

[PATCH v2 10/15] hw/display/sm501: Embed OHCI QOM child in chipset

[Philippe Mathieu-Daudé]

Note this device doesn't implement unrealize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/sm501.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 52e42585af..0f7e09d7e2 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -28,6 +28,7 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "hw/usb/hcd-ohci.h"
 #include "hw/char/serial.h"
 #include "ui/console.h"
 #include "hw/sysbus.h"
@@ -1944,13 +1945,13 @@ struct SM501SysBusState {
     uint32_t vram_size;
     uint32_t base;
     SerialMM serial;
+    OHCISysBusState ohci;
 };
 
 static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
 {
     SM501SysBusState *s = SYSBUS_SM501(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    DeviceState *usb_dev;
     MemoryRegion *mr;
 
     sm501_init(&s->state, dev, s->vram_size);
@@ -1963,13 +1964,11 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->state.mmio_region);
 
     /* bridge to usb host emulation module */
-    usb_dev = qdev_new("sysbus-ohci");
-    qdev_prop_set_uint32(usb_dev, "num-ports", 2);
-    qdev_prop_set_uint64(usb_dev, "dma-offset", s->base);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal);
+    qdev_prop_set_uint64(DEVICE(&s->ohci), "dma-offset", s->base);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal);
     memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST,
-                       sysbus_mmio_get_region(SYS_BUS_DEVICE(usb_dev), 0));
-    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(usb_dev));
+                       sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0));
+    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->ohci));
 
     /* bridge to serial emulation module */
     sysbus_realize(SYS_BUS_DEVICE(&s->serial), &error_fatal);
@@ -2016,8 +2015,12 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
 static void sm501_sysbus_init(Object *o)
 {
     SM501SysBusState *sm501 = SYSBUS_SM501(o);
+    OHCISysBusState *ohci = &sm501->ohci;
     SerialMM *smm = &sm501->serial;
 
+    object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI);
+    qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2);
+
     object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM);
     qdev_set_legacy_instance_id(DEVICE(smm), SM501_UART0, 2);
     qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
-- 
2.38.1

Re: [PATCH v2 10/15] hw/display/sm501: Embed OHCI QOM child in chipset

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> Note this device doesn't implement unrealize().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/display/sm501.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 52e42585af..0f7e09d7e2 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -28,6 +28,7 @@
> #include "qapi/error.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> +#include "hw/usb/hcd-ohci.h"
> #include "hw/char/serial.h"
> #include "ui/console.h"
> #include "hw/sysbus.h"
> @@ -1944,13 +1945,13 @@ struct SM501SysBusState {
>     uint32_t vram_size;
>     uint32_t base;
>     SerialMM serial;
> +    OHCISysBusState ohci;
> };
>
> static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> {
>     SM501SysBusState *s = SYSBUS_SM501(dev);
>     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    DeviceState *usb_dev;
>     MemoryRegion *mr;
>
>     sm501_init(&s->state, dev, s->vram_size);
> @@ -1963,13 +1964,11 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>     sysbus_init_mmio(sbd, &s->state.mmio_region);
>
>     /* bridge to usb host emulation module */
> -    usb_dev = qdev_new("sysbus-ohci");
> -    qdev_prop_set_uint32(usb_dev, "num-ports", 2);
> -    qdev_prop_set_uint64(usb_dev, "dma-offset", s->base);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal);
> +    qdev_prop_set_uint64(DEVICE(&s->ohci), "dma-offset", s->base);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal);
>     memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST,
> -                       sysbus_mmio_get_region(SYS_BUS_DEVICE(usb_dev), 0));
> -    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(usb_dev));
> +                       sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0));
> +    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->ohci));
>
>     /* bridge to serial emulation module */
>     sysbus_realize(SYS_BUS_DEVICE(&s->serial), &error_fatal);
> @@ -2016,8 +2015,12 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
> static void sm501_sysbus_init(Object *o)
> {
>     SM501SysBusState *sm501 = SYSBUS_SM501(o);
> +    OHCISysBusState *ohci = &sm501->ohci;
>     SerialMM *smm = &sm501->serial;
>
> +    object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI);
> +    qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2);
> +
>     object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM);
>     qdev_set_legacy_instance_id(DEVICE(smm), SM501_UART0, 2);
>     qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
>

Re: [PATCH v2 10/15] hw/display/sm501: Embed OHCI QOM child in chipset

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Note this device doesn't implement unrealize().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

This has become commit 01c400ae435f911eaf575b016090922c074dc1e4.

[PATCH v2 11/15] hw/display/sm501: Alias 'dma-offset' QOM property in chipset object

[Philippe Mathieu-Daudé]

No need to use an intermediate 'dma-offset' property in the
chipset object. Alias the property, so when the machine (here
r2d-plus) sets the value on the chipset, it is propagated to
the OHCI object.

Note we can rename the chipset 'base' property as 'dma-offset'
since the object is a non-user-creatable sysbus type.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/sm501.c | 7 ++-----
 hw/sh4/r2d.c       | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 0f7e09d7e2..efebd93f1e 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1943,7 +1943,6 @@ struct SM501SysBusState {
     /*< public >*/
     SM501State state;
     uint32_t vram_size;
-    uint32_t base;
     SerialMM serial;
     OHCISysBusState ohci;
 };
@@ -1964,7 +1963,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->state.mmio_region);
 
     /* bridge to usb host emulation module */
-    qdev_prop_set_uint64(DEVICE(&s->ohci), "dma-offset", s->base);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal);
     memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST,
                        sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0));
@@ -1979,7 +1977,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
 
 static Property sm501_sysbus_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
-    DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2019,6 +2016,7 @@ static void sm501_sysbus_init(Object *o)
     SerialMM *smm = &sm501->serial;
 
     object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI);
+    object_property_add_alias(o, "dma-offset", OBJECT(ohci), "dma-offset");
     qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2);
 
     object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM);
@@ -2026,8 +2024,7 @@ static void sm501_sysbus_init(Object *o)
     qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
     qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
 
-    object_property_add_alias(o, "chardev",
-                              OBJECT(smm), "chardev");
+    object_property_add_alias(o, "chardev", OBJECT(smm), "chardev");
 }
 
 static const TypeInfo sm501_sysbus_info = {
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 39fc4f19d9..d7020d6d2b 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -274,7 +274,7 @@ static void r2d_init(MachineState *machine)
     dev = qdev_new("sysbus-sm501");
     busdev = SYS_BUS_DEVICE(dev);
     qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
-    qdev_prop_set_uint32(dev, "base", 0x10000000);
+    qdev_prop_set_uint64(dev, "dma-offset", 0x10000000);
     qdev_prop_set_chr(dev, "chardev", serial_hd(2));
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0x10000000);
-- 
2.38.1

Re: [PATCH v2 11/15] hw/display/sm501: Alias 'dma-offset' QOM property in chipset object

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> No need to use an intermediate 'dma-offset' property in the
> chipset object. Alias the property, so when the machine (here
> r2d-plus) sets the value on the chipset, it is propagated to
> the OHCI object.
>
> Note we can rename the chipset 'base' property as 'dma-offset'
> since the object is a non-user-creatable sysbus type.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/display/sm501.c | 7 ++-----
> hw/sh4/r2d.c       | 2 +-
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 0f7e09d7e2..efebd93f1e 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1943,7 +1943,6 @@ struct SM501SysBusState {
>     /*< public >*/
>     SM501State state;
>     uint32_t vram_size;
> -    uint32_t base;
>     SerialMM serial;
>     OHCISysBusState ohci;
> };
> @@ -1964,7 +1963,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>     sysbus_init_mmio(sbd, &s->state.mmio_region);
>
>     /* bridge to usb host emulation module */
> -    qdev_prop_set_uint64(DEVICE(&s->ohci), "dma-offset", s->base);
>     sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal);
>     memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST,
>                        sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0));
> @@ -1979,7 +1977,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>
> static Property sm501_sysbus_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> -    DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2019,6 +2016,7 @@ static void sm501_sysbus_init(Object *o)
>     SerialMM *smm = &sm501->serial;
>
>     object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI);
> +    object_property_add_alias(o, "dma-offset", OBJECT(ohci), "dma-offset");
>     qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2);
>
>     object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM);
> @@ -2026,8 +2024,7 @@ static void sm501_sysbus_init(Object *o)
>     qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
>     qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
>
> -    object_property_add_alias(o, "chardev",
> -                              OBJECT(smm), "chardev");
> +    object_property_add_alias(o, "chardev", OBJECT(smm), "chardev");
> }
>
> static const TypeInfo sm501_sysbus_info = {
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index 39fc4f19d9..d7020d6d2b 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -274,7 +274,7 @@ static void r2d_init(MachineState *machine)
>     dev = qdev_new("sysbus-sm501");
>     busdev = SYS_BUS_DEVICE(dev);
>     qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
> -    qdev_prop_set_uint32(dev, "base", 0x10000000);
> +    qdev_prop_set_uint64(dev, "dma-offset", 0x10000000);
>     qdev_prop_set_chr(dev, "chardev", serial_hd(2));
>     sysbus_realize_and_unref(busdev, &error_fatal);
>     sysbus_mmio_map(busdev, 0, 0x10000000);
>

Re: [PATCH v2 11/15] hw/display/sm501: Alias 'dma-offset' QOM property in chipset object

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> No need to use an intermediate 'dma-offset' property in the
> chipset object. Alias the property, so when the machine (here
> r2d-plus) sets the value on the chipset, it is propagated to
> the OHCI object.
>
> Note we can rename the chipset 'base' property as 'dma-offset'
> since the object is a non-user-creatable sysbus type.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

This has become commit 6a015046606ebf260950605ec48fc6420422f43c.

[PATCH v2 12/15] hw/display/sm501: Unify common QOM properties

[Philippe Mathieu-Daudé]

Since now PCI and SysBus properties are identical, unify them.

Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/sm501.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index efebd93f1e..98375138b2 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1975,7 +1975,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
     /* TODO : chain irq to IRL */
 }
 
-static Property sm501_sysbus_properties[] = {
+static Property sm501_common_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -2004,7 +2004,7 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
     dc->realize = sm501_realize_sysbus;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
     dc->desc = "SM501 Multimedia Companion";
-    device_class_set_props(dc, sm501_sysbus_properties);
+    device_class_set_props(dc, sm501_common_properties);
     dc->reset = sm501_reset_sysbus;
     dc->vmsd = &vmstate_sm501_sysbus;
 }
@@ -2062,11 +2062,6 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
                      &s->state.mmio_region);
 }
 
-static Property sm501_pci_properties[] = {
-    DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void sm501_reset_pci(DeviceState *dev)
 {
     SM501PCIState *s = PCI_SM501(dev);
@@ -2098,7 +2093,7 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_DISPLAY_OTHER;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
     dc->desc = "SM501 Display Controller";
-    device_class_set_props(dc, sm501_pci_properties);
+    device_class_set_props(dc, sm501_common_properties);
     dc->reset = sm501_reset_pci;
     dc->hotpluggable = false;
     dc->vmsd = &vmstate_sm501_pci;
-- 
2.38.1

Re: [PATCH v2 12/15] hw/display/sm501: Unify common QOM properties

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote:
> Since now PCI and SysBus properties are identical, unify them.
>
> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/display/sm501.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index efebd93f1e..98375138b2 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1975,7 +1975,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>     /* TODO : chain irq to IRL */
> }
>
> -static Property sm501_sysbus_properties[] = {
> +static Property sm501_common_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>     DEFINE_PROP_END_OF_LIST(),
> };
> @@ -2004,7 +2004,7 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
>     dc->realize = sm501_realize_sysbus;
>     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>     dc->desc = "SM501 Multimedia Companion";
> -    device_class_set_props(dc, sm501_sysbus_properties);
> +    device_class_set_props(dc, sm501_common_properties);
>     dc->reset = sm501_reset_sysbus;
>     dc->vmsd = &vmstate_sm501_sysbus;
> }
> @@ -2062,11 +2062,6 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>                      &s->state.mmio_region);
> }
>
> -static Property sm501_pci_properties[] = {
> -    DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> static void sm501_reset_pci(DeviceState *dev)
> {
>     SM501PCIState *s = PCI_SM501(dev);
> @@ -2098,7 +2093,7 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
>     k->class_id = PCI_CLASS_DISPLAY_OTHER;
>     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>     dc->desc = "SM501 Display Controller";
> -    device_class_set_props(dc, sm501_pci_properties);
> +    device_class_set_props(dc, sm501_common_properties);
>     dc->reset = sm501_reset_pci;
>     dc->hotpluggable = false;
>     dc->vmsd = &vmstate_sm501_pci;
>

Re: [PATCH v2 12/15] hw/display/sm501: Unify common QOM properties

[Philippe Mathieu-Daudé]

On 3/2/23 15:55, Philippe Mathieu-Daudé wrote:
> Since now PCI and SysBus properties are identical, unify them.
> 
> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/display/sm501.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index efebd93f1e..98375138b2 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1975,7 +1975,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>       /* TODO : chain irq to IRL */
>   }
>   
> -static Property sm501_sysbus_properties[] = {
> +static Property sm501_common_properties[] = {
>       DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };


> @@ -2062,11 +2062,6 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>                        &s->state.mmio_region);
>   }
>   
> -static Property sm501_pci_properties[] = {
> -    DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),

Patch is invalid:

$ ./qemu-system-ppc -S -M sam460ex
qemu-system-ppc: Invalid VRAM size, nearest valid size is 2097152

> -    DEFINE_PROP_END_OF_LIST(),
> -};

Re: [PATCH v2 12/15] hw/display/sm501: Unify common QOM properties

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

On Mon, 27 Feb 2023, Philippe Mathieu-Daudé wrote:
> On 3/2/23 15:55, Philippe Mathieu-Daudé wrote:
>> Since now PCI and SysBus properties are identical, unify them.
>> 
>> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/display/sm501.c | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index efebd93f1e..98375138b2 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -1975,7 +1975,7 @@ static void sm501_realize_sysbus(DeviceState *dev, 
>> Error **errp)
>>       /* TODO : chain irq to IRL */
>>   }
>>   -static Property sm501_sysbus_properties[] = {
>> +static Property sm501_common_properties[] = {
>>       DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>
>
>> @@ -2062,11 +2062,6 @@ static void sm501_realize_pci(PCIDevice *dev, Error 
>> **errp)
>>                        &s->state.mmio_region);
>>   }
>>   -static Property sm501_pci_properties[] = {
>> -    DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),

Oops, indeed the defaults are different so maybe just drop this patch for 
now.

Regards,
BALATON Zoltan

>
> Patch is invalid:
>
> $ ./qemu-system-ppc -S -M sam460ex
> qemu-system-ppc: Invalid VRAM size, nearest valid size is 2097152
>
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>
>
>
>

Re: [PATCH v2 12/15] hw/display/sm501: Unify common QOM properties

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 27 Feb 2023, Philippe Mathieu-Daudé wrote:
>> On 3/2/23 15:55, Philippe Mathieu-Daudé wrote:
>>> Since now PCI and SysBus properties are identical, unify them.
>>> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/display/sm501.c | 11 +++--------
>>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index efebd93f1e..98375138b2 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -1975,7 +1975,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>>>       /* TODO : chain irq to IRL */
>>>   }
>>>   -static Property sm501_sysbus_properties[] = {
>>> +static Property sm501_common_properties[] = {
>>>       DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>
>>
>>> @@ -2062,11 +2062,6 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>>>                        &s->state.mmio_region);
>>>   }
>>>   -static Property sm501_pci_properties[] = {
>>> -    DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
>
> Oops, indeed the defaults are different so maybe just drop this patch for now.

Okay, I'm ignoring this patch.

[PATCH v2 13/15] hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'

[Philippe Mathieu-Daudé]

DEFINE_PROP_DMAADDR() is only used once. Since it doesn't
add much value, simply remove it, along with the header
defining it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/usb/hcd-ohci-pci.c |  1 -
 hw/usb/hcd-ohci.c     |  3 +--
 include/hw/qdev-dma.h | 16 ----------------
 3 files changed, 1 insertion(+), 19 deletions(-)
 delete mode 100644 include/hw/qdev-dma.h

diff --git a/hw/usb/hcd-ohci-pci.c b/hw/usb/hcd-ohci-pci.c
index 6b630d35a7..92cc151264 100644
--- a/hw/usb/hcd-ohci-pci.c
+++ b/hw/usb/hcd-ohci-pci.c
@@ -25,7 +25,6 @@
 #include "migration/vmstate.h"
 #include "hw/pci/pci_device.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-dma.h"
 #include "hw/qdev-properties.h"
 #include "trace.h"
 #include "hcd-ohci.h"
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 9d68036d23..26c377bf1b 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -33,7 +33,6 @@
 #include "hw/usb.h"
 #include "migration/vmstate.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-dma.h"
 #include "hw/qdev-properties.h"
 #include "trace.h"
 #include "hcd-ohci.h"
@@ -2008,7 +2007,7 @@ static Property ohci_sysbus_properties[] = {
     DEFINE_PROP_STRING("masterbus", OHCISysBusState, masterbus),
     DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3),
     DEFINE_PROP_UINT32("firstport", OHCISysBusState, firstport, 0),
-    DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 0),
+    DEFINE_PROP_UINT64("dma-offset", OHCISysBusState, dma_offset, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/qdev-dma.h b/include/hw/qdev-dma.h
deleted file mode 100644
index b00391aa0c..0000000000
--- a/include/hw/qdev-dma.h
+++ /dev/null
@@ -1,16 +0,0 @@
-/*
- * Support for dma_addr_t typed properties
- *
- * Copyright (C) 2012 David Gibson, IBM Corporation.
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#ifndef HW_QDEV_DMA_H
-#define HW_QDEV_DMA_H
-
-#define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)                               \
-    DEFINE_PROP_UINT64(_n, _s, _f, _d)
-
-#endif
-- 
2.38.1

Re: [PATCH v2 13/15] hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> DEFINE_PROP_DMAADDR() is only used once. Since it doesn't
> add much value, simply remove it, along with the header
> defining it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

DEFINE_PROP_DMAADDR() lets you wrap a property around a dma_addr_t
member without assuming anything about dma_addr_t.

If we use its (trivial) expansion instead, we assume dma_addr_t is
uint64_t.

Whether that's worth avoiding I can't say.  Depends on how much the
abstraction leaks in other ways.  Thoughts?

Re: [PATCH v2 13/15] hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'

[Markus Armbruster]

+David

Markus Armbruster <armbru@redhat.com> writes:

> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
>> DEFINE_PROP_DMAADDR() is only used once. Since it doesn't
>> add much value, simply remove it, along with the header
>> defining it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> DEFINE_PROP_DMAADDR() lets you wrap a property around a dma_addr_t
> member without assuming anything about dma_addr_t.
>
> If we use its (trivial) expansion instead, we assume dma_addr_t is
> uint64_t.
>
> Whether that's worth avoiding I can't say.  Depends on how much the
> abstraction leaks in other ways.  Thoughts?

Re: [PATCH v2 13/15] hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'

[Paolo Bonzini]

On 9/25/23 13:03, Markus Armbruster wrote:
> +David
> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> DEFINE_PROP_DMAADDR() is only used once. Since it doesn't
>>> add much value, simply remove it, along with the header
>>> defining it.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> DEFINE_PROP_DMAADDR() lets you wrap a property around a dma_addr_t
>> member without assuming anything about dma_addr_t.
>>
>> Whether that's worth avoiding I can't say.  Depends on how much the
>> abstraction leaks in other ways.  Thoughts?

I think it's okay to simplify things.  If anybody ever has a reason to 
make dma_addr_t variable sized (probably a bad idea because many of its 
users are compiled once only) they have one extra place to fix.  Tough 
luck. :)

Paolo

Re: [PATCH v2 13/15] hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 9/25/23 13:03, Markus Armbruster wrote:
>> +David
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>>
>>>> DEFINE_PROP_DMAADDR() is only used once. Since it doesn't
>>>> add much value, simply remove it, along with the header
>>>> defining it.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>> DEFINE_PROP_DMAADDR() lets you wrap a property around a dma_addr_t
>>> member without assuming anything about dma_addr_t.
>>>
>>> Whether that's worth avoiding I can't say.  Depends on how much the
>>> abstraction leaks in other ways.  Thoughts?
>
> I think it's okay to simplify things.  If anybody ever has a reason to make dma_addr_t variable sized (probably a bad idea because many of its users are compiled once only) they have one extra place to fix.  Tough luck. :)

Makes sense.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 14/15] hw/mips: Declare all length properties as unsigned

[Philippe Mathieu-Daudé]

Some length properties are signed, other unsigned:

  hw/mips/cps.c:183:    DEFINE_PROP_UINT32("num-vp", MIPSCPSState, num_vp, 1),
  hw/mips/cps.c:184:    DEFINE_PROP_UINT32("num-irq", MIPSCPSState, num_irq, 256),
  hw/misc/mips_cmgcr.c:215:    DEFINE_PROP_INT32("num-vp", MIPSGCRState, num_vps, 1),
  hw/misc/mips_cpc.c:167:    DEFINE_PROP_UINT32("num-vp", MIPSCPCState, num_vp, 0x1),
  hw/misc/mips_itu.c:552:    DEFINE_PROP_INT32("num-fifo", MIPSITUState, num_fifo,
  hw/misc/mips_itu.c:554:    DEFINE_PROP_INT32("num-semaphores", MIPSITUState,

Since negative values are not used (the minimum is '0'),
unify by declaring all properties as unsigned.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/mips_gic.c           |  4 ++--
 hw/mips/boston.c             |  2 +-
 hw/mips/cps.c                | 12 ++++++------
 hw/mips/malta.c              |  2 +-
 hw/misc/mips_cmgcr.c         |  2 +-
 hw/misc/mips_itu.c           |  4 ++--
 include/hw/intc/mips_gic.h   |  4 ++--
 include/hw/misc/mips_cmgcr.h |  2 +-
 include/hw/misc/mips_itu.h   |  4 ++--
 9 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/intc/mips_gic.c b/hw/intc/mips_gic.c
index bda4549925..4bdc3b1bd1 100644
--- a/hw/intc/mips_gic.c
+++ b/hw/intc/mips_gic.c
@@ -439,8 +439,8 @@ static void mips_gic_realize(DeviceState *dev, Error **errp)
 }
 
 static Property mips_gic_properties[] = {
-    DEFINE_PROP_INT32("num-vp", MIPSGICState, num_vps, 1),
-    DEFINE_PROP_INT32("num-irq", MIPSGICState, num_irq, 256),
+    DEFINE_PROP_UINT32("num-vp", MIPSGICState, num_vps, 1),
+    DEFINE_PROP_UINT32("num-irq", MIPSGICState, num_irq, 256),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index a9d87f3437..21ad844519 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -702,7 +702,7 @@ static void boston_mach_init(MachineState *machine)
     object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS);
     object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
                             &error_fatal);
-    object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
+    object_property_set_uint(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
                             &error_fatal);
     qdev_connect_clock_in(DEVICE(&s->cps), "clk-in",
                           qdev_get_clock_out(dev, "cpu-refclk"));
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 2b436700ce..38acc57468 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -114,9 +114,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     /* Inter-Thread Communication Unit */
     if (itu_present) {
         object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
-        object_property_set_int(OBJECT(&s->itu), "num-fifo", 16,
+        object_property_set_uint(OBJECT(&s->itu), "num-fifo", 16,
                                 &error_abort);
-        object_property_set_int(OBJECT(&s->itu), "num-semaphores", 16,
+        object_property_set_uint(OBJECT(&s->itu), "num-semaphores", 16,
                                 &error_abort);
         object_property_set_bool(OBJECT(&s->itu), "saar-present", saar_present,
                                  &error_abort);
@@ -133,7 +133,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 
     /* Cluster Power Controller */
     object_initialize_child(OBJECT(dev), "cpc", &s->cpc, TYPE_MIPS_CPC);
-    object_property_set_int(OBJECT(&s->cpc), "num-vp", s->num_vp,
+    object_property_set_uint(OBJECT(&s->cpc), "num-vp", s->num_vp,
                             &error_abort);
     object_property_set_int(OBJECT(&s->cpc), "vp-start-running", 1,
                             &error_abort);
@@ -146,9 +146,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 
     /* Global Interrupt Controller */
     object_initialize_child(OBJECT(dev), "gic", &s->gic, TYPE_MIPS_GIC);
-    object_property_set_int(OBJECT(&s->gic), "num-vp", s->num_vp,
+    object_property_set_uint(OBJECT(&s->gic), "num-vp", s->num_vp,
                             &error_abort);
-    object_property_set_int(OBJECT(&s->gic), "num-irq", 128,
+    object_property_set_uint(OBJECT(&s->gic), "num-irq", 128,
                             &error_abort);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gic), errp)) {
         return;
@@ -161,7 +161,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     gcr_base = env->CP0_CMGCRBase << 4;
 
     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
-    object_property_set_int(OBJECT(&s->gcr), "num-vp", s->num_vp,
+    object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
                             &error_abort);
     object_property_set_int(OBJECT(&s->gcr), "gcr-rev", 0x800,
                             &error_abort);
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index ec172b111a..af9021316d 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1066,7 +1066,7 @@ static void create_cps(MachineState *ms, MaltaState *s,
     object_initialize_child(OBJECT(s), "cps", &s->cps, TYPE_MIPS_CPS);
     object_property_set_str(OBJECT(&s->cps), "cpu-type", ms->cpu_type,
                             &error_fatal);
-    object_property_set_int(OBJECT(&s->cps), "num-vp", ms->smp.cpus,
+    object_property_set_uint(OBJECT(&s->cps), "num-vp", ms->smp.cpus,
                             &error_fatal);
     qdev_connect_clock_in(DEVICE(&s->cps), "clk-in", s->cpuclk);
     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
diff --git a/hw/misc/mips_cmgcr.c b/hw/misc/mips_cmgcr.c
index 3c8b37f700..66eb11662c 100644
--- a/hw/misc/mips_cmgcr.c
+++ b/hw/misc/mips_cmgcr.c
@@ -212,7 +212,7 @@ static const VMStateDescription vmstate_mips_gcr = {
 };
 
 static Property mips_gcr_properties[] = {
-    DEFINE_PROP_INT32("num-vp", MIPSGCRState, num_vps, 1),
+    DEFINE_PROP_UINT32("num-vp", MIPSGCRState, num_vps, 1),
     DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
     DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
     DEFINE_PROP_LINK("gic", MIPSGCRState, gic_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/mips_itu.c b/hw/misc/mips_itu.c
index badef5c214..a06cdd10ea 100644
--- a/hw/misc/mips_itu.c
+++ b/hw/misc/mips_itu.c
@@ -549,9 +549,9 @@ static void mips_itu_reset(DeviceState *dev)
 }
 
 static Property mips_itu_properties[] = {
-    DEFINE_PROP_INT32("num-fifo", MIPSITUState, num_fifo,
+    DEFINE_PROP_UINT32("num-fifo", MIPSITUState, num_fifo,
                       ITC_FIFO_NUM_MAX),
-    DEFINE_PROP_INT32("num-semaphores", MIPSITUState, num_semaphores,
+    DEFINE_PROP_UINT32("num-semaphores", MIPSITUState, num_semaphores,
                       ITC_SEMAPH_NUM_MAX),
     DEFINE_PROP_BOOL("saar-present", MIPSITUState, saar_present, false),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/intc/mips_gic.h b/include/hw/intc/mips_gic.h
index eeb136e261..5e4c71edd4 100644
--- a/include/hw/intc/mips_gic.h
+++ b/include/hw/intc/mips_gic.h
@@ -211,8 +211,8 @@ struct MIPSGICState {
     /* GIC VP Timer */
     MIPSGICTimerState *gic_timer;
 
-    int32_t num_vps;
-    int32_t num_irq;
+    uint32_t num_vps;
+    uint32_t num_irq;
 };
 
 #endif /* MIPS_GIC_H */
diff --git a/include/hw/misc/mips_cmgcr.h b/include/hw/misc/mips_cmgcr.h
index 9fa58942d7..db4bf5f449 100644
--- a/include/hw/misc/mips_cmgcr.h
+++ b/include/hw/misc/mips_cmgcr.h
@@ -75,7 +75,7 @@ struct MIPSGCRState {
     SysBusDevice parent_obj;
 
     int32_t gcr_rev;
-    int32_t num_vps;
+    uint32_t num_vps;
     hwaddr gcr_base;
     MemoryRegion iomem;
     MemoryRegion *cpc_mr;
diff --git a/include/hw/misc/mips_itu.h b/include/hw/misc/mips_itu.h
index 50d961106d..ab6d286c38 100644
--- a/include/hw/misc/mips_itu.h
+++ b/include/hw/misc/mips_itu.h
@@ -57,8 +57,8 @@ struct MIPSITUState {
     SysBusDevice parent_obj;
     /*< public >*/
 
-    int32_t num_fifo;
-    int32_t num_semaphores;
+    uint32_t num_fifo;
+    uint32_t num_semaphores;
 
     /* ITC Storage */
     ITCStorageCell *cell;
-- 
2.38.1

[RFC PATCH v2 15/15] hw/mips/itu: Pass SAAR using QOM link property

[Philippe Mathieu-Daudé]

QOM objects shouldn't access each other internals fields
except using the QOM API.

mips_cps_realize() instantiates a TYPE_MIPS_ITU object, and
directly sets the 'saar' pointer:

   if (saar_present) {
       s->itu.saar = &env->CP0_SAAR;
   }

In order to avoid that, pass the MIPS_CPU object via a QOM
link property, and set the 'saar' pointer in mips_itu_realize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC because not tested.
---
 hw/mips/cps.c              | 23 ++++++-----------------
 hw/misc/mips_itu.c         | 26 ++++++++++++++++++--------
 include/hw/misc/mips_itu.h |  5 ++---
 3 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 38acc57468..2b5269ebf1 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -66,20 +66,17 @@ static bool cpu_mips_itu_supported(CPUMIPSState *env)
 static void mips_cps_realize(DeviceState *dev, Error **errp)
 {
     MIPSCPSState *s = MIPS_CPS(dev);
-    CPUMIPSState *env;
-    MIPSCPU *cpu;
-    int i;
     target_ulong gcr_base;
     bool itu_present = false;
-    bool saar_present = false;
 
     if (!clock_get(s->clock)) {
         error_setg(errp, "CPS input clock is not connected to an output clock");
         return;
     }
 
-    for (i = 0; i < s->num_vp; i++) {
-        cpu = MIPS_CPU(object_new(s->cpu_type));
+    for (int i = 0; i < s->num_vp; i++) {
+        MIPSCPU *cpu = MIPS_CPU(object_new(s->cpu_type));
+        CPUMIPSState *env = &cpu->env;
 
         /* All VPs are halted on reset. Leave powering up to CPC. */
         if (!object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
@@ -97,7 +94,6 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
         cpu_mips_irq_init_cpu(cpu);
         cpu_mips_clock_init(cpu);
 
-        env = &cpu->env;
         if (cpu_mips_itu_supported(env)) {
             itu_present = true;
             /* Attach ITC Tag to the VP */
@@ -107,22 +103,15 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
         qemu_register_reset(main_cpu_reset, cpu);
     }
 
-    cpu = MIPS_CPU(first_cpu);
-    env = &cpu->env;
-    saar_present = (bool)env->saarp;
-
     /* Inter-Thread Communication Unit */
     if (itu_present) {
         object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
+        object_property_set_link(OBJECT(&s->itu), "cpu[0]",
+                                 OBJECT(first_cpu), &error_abort);
         object_property_set_uint(OBJECT(&s->itu), "num-fifo", 16,
                                 &error_abort);
         object_property_set_uint(OBJECT(&s->itu), "num-semaphores", 16,
                                 &error_abort);
-        object_property_set_bool(OBJECT(&s->itu), "saar-present", saar_present,
-                                 &error_abort);
-        if (saar_present) {
-            s->itu.saar = &env->CP0_SAAR;
-        }
         if (!sysbus_realize(SYS_BUS_DEVICE(&s->itu), errp)) {
             return;
         }
@@ -158,7 +147,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
                             sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gic), 0));
 
     /* Global Configuration Registers */
-    gcr_base = env->CP0_CMGCRBase << 4;
+    gcr_base = MIPS_CPU(first_cpu)->env.CP0_CMGCRBase << 4;
 
     object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
     object_property_set_uint(OBJECT(&s->gcr), "num-vp", s->num_vp,
diff --git a/hw/misc/mips_itu.c b/hw/misc/mips_itu.c
index a06cdd10ea..0eda302db4 100644
--- a/hw/misc/mips_itu.c
+++ b/hw/misc/mips_itu.c
@@ -93,10 +93,10 @@ void itc_reconfigure(MIPSITUState *tag)
     uint64_t size = (1 * KiB) + (am[1] & ITC_AM1_ADDR_MASK_MASK);
     bool is_enabled = (am[0] & ITC_AM0_EN_MASK) != 0;
 
-    if (tag->saar_present) {
-        address = ((*(uint64_t *) tag->saar) & 0xFFFFFFFFE000ULL) << 4;
-        size = 1ULL << ((*(uint64_t *) tag->saar >> 1) & 0x1f);
-        is_enabled = *(uint64_t *) tag->saar & 1;
+    if (tag->saar) {
+        address = (tag->saar[0] & 0xFFFFFFFFE000ULL) << 4;
+        size = 1ULL << ((tag->saar[0] >> 1) & 0x1f);
+        is_enabled = tag->saar[0] & 1;
     }
 
     memory_region_transaction_begin();
@@ -157,7 +157,7 @@ static inline ITCView get_itc_view(hwaddr addr)
 static inline int get_cell_stride_shift(const MIPSITUState *s)
 {
     /* Minimum interval (for EntryGain = 0) is 128 B */
-    if (s->saar_present) {
+    if (s->saar) {
         return 7 + ((s->icr0 >> ITC_ICR0_BLK_GRAIN) &
                     ITC_ICR0_BLK_GRAIN_MASK);
     } else {
@@ -515,6 +515,7 @@ static void mips_itu_init(Object *obj)
 static void mips_itu_realize(DeviceState *dev, Error **errp)
 {
     MIPSITUState *s = MIPS_ITU(dev);
+    CPUMIPSState *env;
 
     if (s->num_fifo > ITC_FIFO_NUM_MAX) {
         error_setg(errp, "Exceed maximum number of FIFO cells: %d",
@@ -526,6 +527,15 @@ static void mips_itu_realize(DeviceState *dev, Error **errp)
                    s->num_semaphores);
         return;
     }
+    if (!s->cpu0) {
+        error_setg(errp, "Missing 'cpu[0]' property");
+        return;
+    }
+
+    env = &s->cpu0->env;
+    if (env->saarp) {
+        s->saar = env->CP0_SAAR;
+    }
 
     s->cell = g_new(ITCStorageCell, get_num_cells(s));
 }
@@ -534,8 +544,8 @@ static void mips_itu_reset(DeviceState *dev)
 {
     MIPSITUState *s = MIPS_ITU(dev);
 
-    if (s->saar_present) {
-        *(uint64_t *) s->saar = 0x11 << 1;
+    if (s->saar) {
+        s->saar[0] = 0x11 << 1;
         s->icr0 = get_num_cells(s) << ITC_ICR0_CELL_NUM;
     } else {
         s->ITCAddressMap[0] = 0;
@@ -553,7 +563,7 @@ static Property mips_itu_properties[] = {
                       ITC_FIFO_NUM_MAX),
     DEFINE_PROP_UINT32("num-semaphores", MIPSITUState, num_semaphores,
                       ITC_SEMAPH_NUM_MAX),
-    DEFINE_PROP_BOOL("saar-present", MIPSITUState, saar_present, false),
+    DEFINE_PROP_LINK("cpu[0]", MIPSITUState, cpu0, TYPE_MIPS_CPU, MIPSCPU *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/misc/mips_itu.h b/include/hw/misc/mips_itu.h
index ab6d286c38..35218b2d14 100644
--- a/include/hw/misc/mips_itu.h
+++ b/include/hw/misc/mips_itu.h
@@ -72,9 +72,8 @@ struct MIPSITUState {
     uint64_t icr0;
 
     /* SAAR */
-    bool saar_present;
-    void *saar;
-
+    uint64_t *saar;
+    MIPSCPU *cpu0;
 };
 
 /* Get ITC Configuration Tag memory region. */
-- 
2.38.1

Re: [PATCH v2 00/15] hw: Use QOM alias properties and few QOM/QDev cleanups

[Philippe Mathieu-Daudé]

On 3/2/23 15:55, Philippe Mathieu-Daudé wrote:
> Since v1:
> - Addressed Markus & Zoltan review comments
> - Introduce/use qdev_unrealize_and_unref()
> 
> These patches are extracted from a QOM/QDev refactor series,
> so they are preliminary cleanups noticed while working on it:
> 
> - Use correct type when calling qdev_prop_set_xxx()
> - Unify some qdev properties in MIPS models
> - Replace intermediate properties by link properties
> - Remove DEFINE_PROP_DMAADDR() macro which is used one time
> - Use qdev_realize_and_unref() instead of open-coding it
> 
> Philippe Mathieu-Daudé (15):
>    hw/pci/pcie_sriov: Replace fprintf(error_pretty) ->
>      warn_reportf_err()
>    hw/qdev: Introduce qdev_unrealize_and_unref()
>    linux-user/syscall: Do not open-code qdev_unrealize_and_unref()
>    hw/pci/pcie_sriov: Do not open-code qdev_unrealize_and_unref()
>    hw/i386/sgx: Do not open-code qdev_realize_and_unref()
>    hw/ppc/sam460ex: Correctly set MAL properties
>    hw/arm/nrf51: Alias 'flash-size' QOM property in SoC object
>    hw/arm/fsl-imx: Alias 'phy-num' QOM property in SoC object
>    hw/usb/hcd-ohci: Include missing 'sysbus.h' header
>    hw/display/sm501: Embed OHCI QOM child in chipset
>    hw/display/sm501: Alias 'dma-offset' QOM property in chipset object
>    hw/display/sm501: Unify common QOM properties
>    hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'
>    hw/mips: Declare all length properties as unsigned
>    hw/mips/itu: Pass SAAR using QOM link property

Ping for review?

Re: [PATCH v2 00/15] hw: Use QOM alias properties and few QOM/QDev cleanups

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 3/2/23 15:55, Philippe Mathieu-Daudé wrote:
>> Since v1:
>> - Addressed Markus & Zoltan review comments
>> - Introduce/use qdev_unrealize_and_unref()
>> These patches are extracted from a QOM/QDev refactor series,
>> so they are preliminary cleanups noticed while working on it:
>> - Use correct type when calling qdev_prop_set_xxx()
>> - Unify some qdev properties in MIPS models
>> - Replace intermediate properties by link properties
>> - Remove DEFINE_PROP_DMAADDR() macro which is used one time
>> - Use qdev_realize_and_unref() instead of open-coding it
>> Philippe Mathieu-Daudé (15):
>>    hw/pci/pcie_sriov: Replace fprintf(error_pretty) ->
>>      warn_reportf_err()
>>    hw/qdev: Introduce qdev_unrealize_and_unref()
>>    linux-user/syscall: Do not open-code qdev_unrealize_and_unref()
>>    hw/pci/pcie_sriov: Do not open-code qdev_unrealize_and_unref()
>>    hw/i386/sgx: Do not open-code qdev_realize_and_unref()
>>    hw/ppc/sam460ex: Correctly set MAL properties
>>    hw/arm/nrf51: Alias 'flash-size' QOM property in SoC object
>>    hw/arm/fsl-imx: Alias 'phy-num' QOM property in SoC object
>>    hw/usb/hcd-ohci: Include missing 'sysbus.h' header
>>    hw/display/sm501: Embed OHCI QOM child in chipset
>>    hw/display/sm501: Alias 'dma-offset' QOM property in chipset object
>>    hw/display/sm501: Unify common QOM properties
>>    hw/qdev: Remove DEFINE_PROP_DMAADDR() and 'hw/qdev-dma.h'
>>    hw/mips: Declare all length properties as unsigned
>>    hw/mips/itu: Pass SAAR using QOM link property
>
> Ping for review?

Needs a rebase.  I reviewed some all the same.  If you want more from me
before you respin, let me know.