[PATCH 00/14] Clean-ups: remove QDEV_PROP_PTR

[PATCH 00/14] Clean-ups: remove QDEV_PROP_PTR

[Marc-André Lureau]

Hi,

QDEV_PROP_PTR is marked in multiple places as "FIXME/TODO/remove
me". In most cases, it can be easily replaced with QDEV_PROP_LINK when
the pointer points to an Object.

There are a few places where such substitution isn't possible. For
those places, it seems reasonable to use a specific setter method
instead, and keep the user_creatable = false. It is the case for OMAP
clocks, and smbus-eeprom initial data pointer. Improving that
situation is left for a future improvement.

Marc-André Lureau (14):
  sm501: replace PROP_PTR with PROP_LINK
  vmmouse: replace PROP_PTR with PROP_LINK
  lance: replace PROP_PTR with PROP_LINK
  etraxfs: remove PROP_PTR usage
  dp8393x: replace PROP_PTR with PROP_LINK
  leon3: replace PROP_PTR with PROP_LINK
  RFC: mips/cps: fix setting saar property
  cris: replace PROP_PTR with PROP_LINK for interrupt vector
  smbus-eeprom: remove PROP_PTR
  omap-intc: remove PROP_PTR
  omap-i2c: remove PROP_PTR
  omap-gpio: remove PROP_PTR
  qdev: remove PROP_MEMORY_REGION
  Remove QDEV_PROP_PTR

 hw/arm/omap1.c               |  8 +++----
 hw/arm/omap2.c               | 25 ++++++++++-----------
 hw/core/qdev-properties.c    | 18 ----------------
 hw/cris/axis_dev88.c         |  4 +---
 hw/display/sm501.c           |  5 +++--
 hw/dma/sparc32_dma.c         |  2 +-
 hw/gpio/omap_gpio.c          | 42 +++++++++++++-----------------------
 hw/i2c/omap_i2c.c            | 19 ++++++++++------
 hw/i2c/smbus_eeprom.c        | 17 +++++++--------
 hw/i386/pc.c                 |  6 +++---
 hw/i386/vmmouse.c            |  8 +++----
 hw/input/pckbd.c             |  8 +++----
 hw/intc/Makefile.objs        |  2 +-
 hw/intc/etraxfs_pic.c        | 18 ++++++----------
 hw/intc/grlib_irqmp.c        | 20 ++++++-----------
 hw/intc/omap_intc.c          | 17 +++++++++------
 hw/mips/cps.c                |  2 +-
 hw/mips/mips_jazz.c          |  3 ++-
 hw/net/dp8393x.c             |  7 +++---
 hw/net/etraxfs_eth.c         | 35 ++++++++++++++++++++----------
 hw/net/lance.c               |  5 ++---
 hw/net/pcnet-pci.c           |  2 +-
 hw/net/pcnet.h               |  2 +-
 hw/sh4/r2d.c                 |  3 ++-
 hw/sparc/leon3.c             |  7 +++---
 include/hw/arm/omap.h        | 36 +++++++++++++++++++++++++++++++
 include/hw/cris/etraxfs.h    | 20 +++--------------
 include/hw/input/i8042.h     |  4 +++-
 include/hw/qdev-properties.h | 24 ---------------------
 target/sparc/cpu.h           |  1 +
 30 files changed, 172 insertions(+), 198 deletions(-)

-- 
2.23.0.606.g08da6496b6

[PATCH 01/14] sm501: replace PROP_PTR with PROP_LINK

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/sm501.c | 5 +++--
 hw/sh4/r2d.c       | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 1f33c87e65..a87d18efab 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1930,7 +1930,7 @@ typedef struct {
     SM501State state;
     uint32_t vram_size;
     uint32_t base;
-    void *chr_state;
+    Chardev *chr_state;
 } SM501SysBusState;
 
 static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
@@ -1968,7 +1968,8 @@ 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_PTR("chr-state", SM501SysBusState, chr_state),
+    DEFINE_PROP_LINK("chr-state", SM501SysBusState, chr_state,
+                     TYPE_CHARDEV, Chardev *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index ee0840f380..5780ee85d9 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -272,7 +272,8 @@ static void r2d_init(MachineState *machine)
     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_ptr(dev, "chr-state", serial_hd(2));
+    object_property_set_link(OBJECT(dev), OBJECT(serial_hd(2)),
+                             "chr-state", &error_abort);
     qdev_init_nofail(dev);
     sysbus_mmio_map(busdev, 0, 0x10000000);
     sysbus_mmio_map(busdev, 1, 0x13e00000);
-- 
2.23.0.606.g08da6496b6

[PATCH 02/14] vmmouse: replace PROP_PTR with PROP_LINK

[Marc-André Lureau]

While at it, use the expected type.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i386/pc.c             | 6 +++---
 hw/i386/vmmouse.c        | 8 +++-----
 hw/input/pckbd.c         | 8 +++-----
 include/hw/input/i8042.h | 4 +++-
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b1904237e..ada1ea8802 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1861,9 +1861,9 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport)
         vmmouse = NULL;
     }
     if (vmmouse) {
-        DeviceState *dev = DEVICE(vmmouse);
-        qdev_prop_set_ptr(dev, "ps2_mouse", i8042);
-        qdev_init_nofail(dev);
+        object_property_set_link(OBJECT(vmmouse), OBJECT(i8042),
+                                 "i8042", &error_abort);
+        qdev_init_nofail(DEVICE(vmmouse));
     }
     port92 = isa_create_simple(isa_bus, "port92");
 
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index 41ad91ad53..c0c329f817 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -66,7 +66,7 @@ typedef struct VMMouseState
     uint16_t status;
     uint8_t absolute;
     QEMUPutMouseEntry *entry;
-    void *ps2_mouse;
+    ISAKBDState *i8042;
 } VMMouseState;
 
 static uint32_t vmmouse_get_status(VMMouseState *s)
@@ -105,7 +105,7 @@ static void vmmouse_mouse_event(void *opaque, int x, int y, int dz, int buttons_
 
     /* need to still generate PS2 events to notify driver to
        read from queue */
-    i8042_isa_mouse_fake_event(s->ps2_mouse);
+    i8042_isa_mouse_fake_event(s->i8042);
 }
 
 static void vmmouse_remove_handler(VMMouseState *s)
@@ -275,7 +275,7 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp)
 }
 
 static Property vmmouse_properties[] = {
-    DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
+    DEFINE_PROP_LINK("i8042", VMMouseState, i8042, TYPE_I8042, ISAKBDState *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -287,8 +287,6 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
     dc->reset = vmmouse_reset;
     dc->vmsd = &vmstate_vmmouse;
     dc->props = vmmouse_properties;
-    /* Reason: pointer property "ps2_mouse" */
-    dc->user_creatable = false;
 }
 
 static const TypeInfo vmmouse_info = {
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index f0acfd86f7..9b641021c9 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -483,17 +483,15 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
 
 #define I8042(obj) OBJECT_CHECK(ISAKBDState, (obj), TYPE_I8042)
 
-typedef struct ISAKBDState {
+struct ISAKBDState {
     ISADevice parent_obj;
 
     KBDState kbd;
     MemoryRegion io[2];
-} ISAKBDState;
+};
 
-void i8042_isa_mouse_fake_event(void *opaque)
+void i8042_isa_mouse_fake_event(ISAKBDState *isa)
 {
-    ISADevice *dev = opaque;
-    ISAKBDState *isa = I8042(dev);
     KBDState *s = &isa->kbd;
 
     ps2_mouse_fake_event(s->mouse);
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index 246e6f3335..8eaebf50ce 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -14,10 +14,12 @@
 
 #define I8042_A20_LINE "a20"
 
+typedef struct ISAKBDState ISAKBDState;
+
 void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
                    MemoryRegion *region, ram_addr_t size,
                    hwaddr mask);
-void i8042_isa_mouse_fake_event(void *opaque);
+void i8042_isa_mouse_fake_event(ISAKBDState *isa);
 void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out);
 
 #endif /* HW_INPUT_I8042_H */
-- 
2.23.0.606.g08da6496b6

[PATCH 03/14] lance: replace PROP_PTR with PROP_LINK

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/dma/sparc32_dma.c | 2 +-
 hw/net/lance.c       | 5 ++---
 hw/net/pcnet-pci.c   | 2 +-
 hw/net/pcnet.h       | 2 +-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 0e5bbcdc7f..3e4da0c47f 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
     d = qdev_create(NULL, TYPE_LANCE);
     object_property_add_child(OBJECT(dev), "lance", OBJECT(d), errp);
     qdev_set_nic_properties(d, nd);
-    qdev_prop_set_ptr(d, "dma", dev);
+    object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp);
     qdev_init_nofail(d);
 }
 
diff --git a/hw/net/lance.c b/hw/net/lance.c
index 6631e2a4e0..4d96299041 100644
--- a/hw/net/lance.c
+++ b/hw/net/lance.c
@@ -138,7 +138,8 @@ static void lance_instance_init(Object *obj)
 }
 
 static Property lance_properties[] = {
-    DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque),
+    DEFINE_PROP_LINK("dma", SysBusPCNetState, state.dma_opaque,
+                     TYPE_DEVICE, DeviceState *),
     DEFINE_NIC_PROPERTIES(SysBusPCNetState, state.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -153,8 +154,6 @@ static void lance_class_init(ObjectClass *klass, void *data)
     dc->reset = lance_reset;
     dc->vmsd = &vmstate_lance;
     dc->props = lance_properties;
-    /* Reason: pointer property "dma" */
-    dc->user_creatable = false;
 }
 
 static const TypeInfo lance_info = {
diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 4723c30c79..d067d21e2c 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -231,7 +231,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
     s->irq = pci_allocate_irq(pci_dev);
     s->phys_mem_read = pci_physical_memory_read;
     s->phys_mem_write = pci_physical_memory_write;
-    s->dma_opaque = pci_dev;
+    s->dma_opaque = DEVICE(pci_dev);
 
     pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info);
 }
diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h
index 28d19a5c6f..f49b213c57 100644
--- a/hw/net/pcnet.h
+++ b/hw/net/pcnet.h
@@ -50,7 +50,7 @@ struct PCNetState_st {
                          uint8_t *buf, int len, int do_bswap);
     void (*phys_mem_write)(void *dma_opaque, hwaddr addr,
                           uint8_t *buf, int len, int do_bswap);
-    void *dma_opaque;
+    DeviceState *dma_opaque;
     int tx_busy;
     int looptest;
 };
-- 
2.23.0.606.g08da6496b6

[PATCH 04/14] etraxfs: remove PROP_PTR usage

[Marc-André Lureau]

etraxfs_dma_client are not Object, so can't be exposed to user with
QOM path. Let's remove property usage and move the constructor to the
.c unit, simplifying some code on the way.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/etraxfs_eth.c      | 35 ++++++++++++++++++++++++-----------
 include/hw/cris/etraxfs.h | 20 +++-----------------
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
index 4cfbf1135a..4726ad5298 100644
--- a/hw/net/etraxfs_eth.c
+++ b/hw/net/etraxfs_eth.c
@@ -338,14 +338,8 @@ typedef struct ETRAXFSEthState
     uint8_t macaddr[2][6];
     uint32_t regs[FS_ETH_MAX_REGS];
 
-    union {
-        void *vdma_out;
-        struct etraxfs_dma_client *dma_out;
-    };
-    union {
-        void *vdma_in;
-        struct etraxfs_dma_client *dma_in;
-    };
+    struct etraxfs_dma_client *dma_out;
+    struct etraxfs_dma_client *dma_in;
 
     /* MDIO bus.  */
     struct qemu_mdio mdio_bus;
@@ -635,8 +629,6 @@ static void etraxfs_eth_realize(DeviceState *dev, Error **errp)
 
 static Property etraxfs_eth_properties[] = {
     DEFINE_PROP_UINT32("phyaddr", ETRAXFSEthState, phyaddr, 1),
-    DEFINE_PROP_PTR("dma_out", ETRAXFSEthState, vdma_out),
-    DEFINE_PROP_PTR("dma_in", ETRAXFSEthState, vdma_in),
     DEFINE_NIC_PROPERTIES(ETRAXFSEthState, conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -648,10 +640,31 @@ static void etraxfs_eth_class_init(ObjectClass *klass, void *data)
     dc->realize = etraxfs_eth_realize;
     dc->reset = etraxfs_eth_reset;
     dc->props = etraxfs_eth_properties;
-    /* Reason: pointer properties "dma_out", "dma_in" */
+    /* Reason: dma_out, dma_in are not user settable */
     dc->user_creatable = false;
 }
 
+
+/* Instantiate an ETRAXFS Ethernet MAC.  */
+DeviceState *
+etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
+                 struct etraxfs_dma_client *dma_out,
+                 struct etraxfs_dma_client *dma_in)
+{
+    DeviceState *dev;
+    qemu_check_nic_model(nd, "fseth");
+
+    dev = qdev_create(NULL, "etraxfs-eth");
+    qdev_set_nic_properties(dev, nd);
+    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
+    ETRAX_FS_ETH(dev)->dma_out = dma_out;
+    ETRAX_FS_ETH(dev)->dma_in = dma_in;
+    qdev_init_nofail(dev);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+
+    return dev;
+}
+
 static const TypeInfo etraxfs_eth_info = {
     .name          = TYPE_ETRAX_FS_ETH,
     .parent        = TYPE_SYS_BUS_DEVICE,
diff --git a/include/hw/cris/etraxfs.h b/include/hw/cris/etraxfs.h
index aa146a2cd8..403e7f95e6 100644
--- a/include/hw/cris/etraxfs.h
+++ b/include/hw/cris/etraxfs.h
@@ -30,23 +30,9 @@
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 
-/* Instantiate an ETRAXFS Ethernet MAC.  */
-static inline DeviceState *
-etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
-                 void *dma_out, void *dma_in)
-{
-    DeviceState *dev;
-    qemu_check_nic_model(nd, "fseth");
-
-    dev = qdev_create(NULL, "etraxfs-eth");
-    qdev_set_nic_properties(dev, nd);
-    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
-    qdev_prop_set_ptr(dev, "dma_out", dma_out);
-    qdev_prop_set_ptr(dev, "dma_in", dma_in);
-    qdev_init_nofail(dev);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-    return dev;
-}
+DeviceState *etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
+                              struct etraxfs_dma_client *dma_out,
+                              struct etraxfs_dma_client *dma_in);
 
 static inline DeviceState *etraxfs_ser_create(hwaddr addr,
                                               qemu_irq irq,
-- 
2.23.0.606.g08da6496b6

[PATCH 05/14] dp8393x: replace PROP_PTR with PROP_LINK

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/mips/mips_jazz.c | 3 ++-
 hw/net/dp8393x.c    | 7 +++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 8d010a0b6e..878925a963 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -284,7 +284,8 @@ static void mips_jazz_init(MachineState *machine,
             dev = qdev_create(NULL, "dp8393x");
             qdev_set_nic_properties(dev, nd);
             qdev_prop_set_uint8(dev, "it_shift", 2);
-            qdev_prop_set_ptr(dev, "dma_mr", rc4030_dma_mr);
+            object_property_set_link(OBJECT(dev), OBJECT(rc4030_dma_mr),
+                                     "dma_mr", &error_abort);
             qdev_init_nofail(dev);
             sysbus = SYS_BUS_DEVICE(dev);
             sysbus_mmio_map(sysbus, 0, 0x80001000);
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index a5678e11fa..946c7a8f64 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -173,7 +173,7 @@ typedef struct dp8393xState {
     int loopback_packet;
 
     /* Memory access */
-    void *dma_mr;
+    MemoryRegion *dma_mr;
     AddressSpace as;
 } dp8393xState;
 
@@ -922,7 +922,8 @@ static const VMStateDescription vmstate_dp8393x = {
 
 static Property dp8393x_properties[] = {
     DEFINE_NIC_PROPERTIES(dp8393xState, conf),
-    DEFINE_PROP_PTR("dma_mr", dp8393xState, dma_mr),
+    DEFINE_PROP_LINK("dma_mr", dp8393xState, dma_mr,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
     DEFINE_PROP_UINT8("it_shift", dp8393xState, it_shift, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -936,8 +937,6 @@ static void dp8393x_class_init(ObjectClass *klass, void *data)
     dc->reset = dp8393x_reset;
     dc->vmsd = &vmstate_dp8393x;
     dc->props = dp8393x_properties;
-    /* Reason: dma_mr property can't be set */
-    dc->user_creatable = false;
 }
 
 static const TypeInfo dp8393x_info = {
-- 
2.23.0.606.g08da6496b6

[PATCH 06/14] leon3: replace PROP_PTR with PROP_LINK

[Marc-André Lureau]

"set_pil_in" and "set_pil_in" are used to set a callback, but have a
single user and cannot be modified by the user.

Simplify the code by calling directly into leon3_set_pil_in(), and use
a "cpu" link property.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/intc/grlib_irqmp.c | 20 ++++++--------------
 hw/sparc/leon3.c      |  7 +++----
 target/sparc/cpu.h    |  1 +
 3 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index bc78e1a14f..34b7e1b4e1 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -58,10 +58,8 @@ typedef struct IRQMP {
 
     MemoryRegion iomem;
 
-    void *set_pil_in;
-    void *set_pil_in_opaque;
-
     IRQMPState *state;
+    SPARCCPU *cpu;
 } IRQMP;
 
 struct IRQMPState {
@@ -82,7 +80,6 @@ static void grlib_irqmp_check_irqs(IRQMPState *state)
     uint32_t      pend   = 0;
     uint32_t      level0 = 0;
     uint32_t      level1 = 0;
-    set_pil_in_fn set_pil_in;
 
     assert(state != NULL);
     assert(state->parent != NULL);
@@ -97,13 +94,11 @@ static void grlib_irqmp_check_irqs(IRQMPState *state)
     trace_grlib_irqmp_check_irqs(state->pending, state->force[0],
                                  state->mask[0], level1, level0);
 
-    set_pil_in = (set_pil_in_fn)state->parent->set_pil_in;
-
     /* Trigger level1 interrupt first and level0 if there is no level1 */
     if (level1 != 0) {
-        set_pil_in(state->parent->set_pil_in_opaque, level1);
+        leon3_set_pil_in(state->parent->cpu, level1);
     } else {
-        set_pil_in(state->parent->set_pil_in_opaque, level0);
+        leon3_set_pil_in(state->parent->cpu, level0);
     }
 }
 
@@ -348,14 +343,13 @@ static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
     IRQMP *irqmp = GRLIB_IRQMP(dev);
 
         /* Check parameters */
-    if (irqmp->set_pil_in == NULL) {
-        error_setg(errp, "set_pil_in cannot be NULL.");
+    if (irqmp->cpu == NULL) {
+        error_setg(errp, "cpu cannot be NULL.");
     }
 }
 
 static Property grlib_irqmp_properties[] = {
-    DEFINE_PROP_PTR("set_pil_in", IRQMP, set_pil_in),
-    DEFINE_PROP_PTR("set_pil_in_opaque", IRQMP, set_pil_in_opaque),
+    DEFINE_PROP_LINK("cpu", IRQMP, cpu, TYPE_SPARC_CPU, SPARCCPU *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -365,8 +359,6 @@ static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
 
     dc->reset = grlib_irqmp_reset;
     dc->props = grlib_irqmp_properties;
-    /* Reason: pointer properties "set_pil_in", "set_pil_in_opaque" */
-    dc->user_creatable = false;
     dc->realize = grlib_irqmp_realize;
 }
 
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index c5f1b1ee72..fa32936ca4 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -143,9 +143,9 @@ void leon3_irq_ack(void *irq_manager, int intno)
     grlib_irqmp_ack((DeviceState *)irq_manager, intno);
 }
 
-static void leon3_set_pil_in(void *opaque, uint32_t pil_in)
+void leon3_set_pil_in(SPARCCPU *cpu, uint32_t pil_in)
 {
-    CPUSPARCState *env = (CPUSPARCState *)opaque;
+    CPUSPARCState *env = &cpu->env;
     CPUState *cs;
 
     assert(env != NULL);
@@ -225,8 +225,7 @@ static void leon3_generic_hw_init(MachineState *machine)
 
     /* Allocate IRQ manager */
     dev = qdev_create(NULL, TYPE_GRLIB_IRQMP);
-    qdev_prop_set_ptr(dev, "set_pil_in", leon3_set_pil_in);
-    qdev_prop_set_ptr(dev, "set_pil_in_opaque", env);
+    object_property_set_link(OBJECT(dev), OBJECT(cpu), "cpu", &error_abort);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_IRQMP_OFFSET);
     env->irq_manager = dev;
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 778aa8e073..5f8e6ec6e8 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -592,6 +592,7 @@ void cpu_check_irqs(CPUSPARCState *env);
 
 /* leon3.c */
 void leon3_irq_ack(void *irq_manager, int intno);
+void leon3_set_pil_in(SPARCCPU *cpu, uint32_t pil_in);
 
 #if defined (TARGET_SPARC64)
 
-- 
2.23.0.606.g08da6496b6

[PATCH 07/14] RFC: mips/cps: fix setting saar property

[Marc-André Lureau]

There is no "saar" property. Note: I haven't been able to test this
code. Help welcome.

May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips:
Update ITU to utilize SAARI and SAAR CP0 registers")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/mips/cps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 1660f86908..c49868d5da 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
         object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present",
                                  &err);
         if (saar_present) {
-            qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void *)&env->CP0_SAAR);
+            s->itu.saar = &env->CP0_SAAR;
         }
         object_property_set_bool(OBJECT(&s->itu), true, "realized", &err);
         if (err != NULL) {
-- 
2.23.0.606.g08da6496b6

[PATCH 08/14] cris: replace PROP_PTR with PROP_LINK for interrupt vector

[Marc-André Lureau]

Instead of using raw interrupt vector pointer, store the associated
CPU with a link property.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/cris/axis_dev88.c  |  4 +---
 hw/intc/Makefile.objs |  2 +-
 hw/intc/etraxfs_pic.c | 18 +++++++-----------
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
index 940c7dd122..cb7b1b58aa 100644
--- a/hw/cris/axis_dev88.c
+++ b/hw/cris/axis_dev88.c
@@ -253,7 +253,6 @@ void axisdev88_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     CRISCPU *cpu;
-    CPUCRISState *env;
     DeviceState *dev;
     SysBusDevice *s;
     DriveInfo *nand;
@@ -267,7 +266,6 @@ void axisdev88_init(MachineState *machine)
 
     /* init CPUs */
     cpu = CRIS_CPU(cpu_create(machine->cpu_type));
-    env = &cpu->env;
 
     /* allocate RAM */
     memory_region_allocate_system_memory(phys_ram, NULL, "axisdev88.ram",
@@ -298,7 +296,7 @@ void axisdev88_init(MachineState *machine)
 
     dev = qdev_create(NULL, "etraxfs,pic");
     /* FIXME: Is there a proper way to signal vectors to the CPU core?  */
-    qdev_prop_set_ptr(dev, "interrupt_vector", &env->interrupt_vector);
+    object_property_set_link(OBJECT(dev), OBJECT(cpu), "cpu", &error_abort);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(s, 0, 0x3001c000);
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index f726d87532..26a3cb36cb 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -5,7 +5,6 @@ common-obj-$(CONFIG_PUV3) += puv3_intc.o
 common-obj-$(CONFIG_XILINX) += xilinx_intc.o
 common-obj-$(CONFIG_XLNX_ZYNQMP_PMU) += xlnx-pmu-iomod-intc.o
 common-obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp-ipi.o
-common-obj-$(CONFIG_ETRAXFS) += etraxfs_pic.o
 common-obj-$(CONFIG_IMX) += imx_avic.o imx_gpcv2.o
 common-obj-$(CONFIG_LM32) += lm32_pic.o
 common-obj-$(CONFIG_REALVIEW) += realview_gic.o
@@ -49,3 +48,4 @@ obj-$(CONFIG_ARM_GIC) += arm_gicv3_cpuif.o
 obj-$(CONFIG_MIPS_CPS) += mips_gic.o
 obj-$(CONFIG_NIOS2) += nios2_iic.o
 obj-$(CONFIG_OMPIC) += ompic.o
+obj-$(CONFIG_ETRAXFS) += etraxfs_pic.o
diff --git a/hw/intc/etraxfs_pic.c b/hw/intc/etraxfs_pic.c
index 77f652acec..8065fc757e 100644
--- a/hw/intc/etraxfs_pic.c
+++ b/hw/intc/etraxfs_pic.c
@@ -27,8 +27,7 @@
 #include "qemu/module.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
-//#include "pc.h"
-//#include "etraxfs.h"
+#include "target/cris/cpu.h"
 
 #define D(x)
 
@@ -48,10 +47,10 @@ struct etrax_pic
     SysBusDevice parent_obj;
 
     MemoryRegion mmio;
-    void *interrupt_vector;
     qemu_irq parent_irq;
     qemu_irq parent_nmi;
     uint32_t regs[R_MAX];
+    CRISCPU *cpu;
 };
 
 static void pic_update(struct etrax_pic *fs)
@@ -79,9 +78,10 @@ static void pic_update(struct etrax_pic *fs)
         }
     }
 
-    if (fs->interrupt_vector) {
-        /* hack alert: ptr property */
-        *(uint32_t*)(fs->interrupt_vector) = vector;
+    if (fs->cpu) {
+        /* hack alert: cpu link property */
+        int32_t *int_vec = &fs->cpu->env.interrupt_vector;
+        *int_vec = (uint32_t)vector;
     }
     qemu_set_irq(fs->parent_irq, !!vector);
 }
@@ -164,7 +164,7 @@ static void etraxfs_pic_init(Object *obj)
 }
 
 static Property etraxfs_pic_properties[] = {
-    DEFINE_PROP_PTR("interrupt_vector", struct etrax_pic, interrupt_vector),
+    DEFINE_PROP_LINK("cpu", struct etrax_pic, cpu, TYPE_CRIS_CPU, CRISCPU *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -173,10 +173,6 @@ static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->props = etraxfs_pic_properties;
-    /*
-     * Note: pointer property "interrupt_vector" may remain null, thus
-     * no need for dc->user_creatable = false;
-     */
 }
 
 static const TypeInfo etraxfs_pic_info = {
-- 
2.23.0.606.g08da6496b6

[PATCH 09/14] smbus-eeprom: remove PROP_PTR

[Marc-André Lureau]

Instead, set the initial data field directly. Since it is only 256
bytes, let's simply copy it to avoid invalid pointers issues.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i2c/smbus_eeprom.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 54c86a0112..533c728b3b 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -44,7 +44,7 @@
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
     uint8_t data[SMBUS_EEPROM_SIZE];
-    void *init_data;
+    uint8_t *init_data;
     uint8_t offset;
     bool accessed;
 } SMBusEEPROMDevice;
@@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev)
 
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
+    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
+
     smbus_eeprom_reset(dev);
+    if (eeprom->init_data == NULL) {
+        error_setg(errp, "init_data cannot be NULL");
+    }
 }
 
-static Property smbus_eeprom_properties[] = {
-    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     dc->reset = smbus_eeprom_reset;
     sc->receive_byte = eeprom_receive_byte;
     sc->write_data = eeprom_write_data;
-    dc->props = smbus_eeprom_properties;
     dc->vmsd = &vmstate_smbus_eeprom;
-    /* Reason: pointer property "data" */
+    /* Reason: init_data */
     dc->user_creatable = false;
 }
 
@@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
 
     dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
     qdev_prop_set_uint8(dev, "address", address);
-    qdev_prop_set_ptr(dev, "data", eeprom_buf);
+    SMBUS_EEPROM(dev)->init_data = eeprom_buf;
     qdev_init_nofail(dev);
 }
 
-- 
2.23.0.606.g08da6496b6

[PATCH 10/14] omap-intc: remove PROP_PTR

[Marc-André Lureau]

Since clock are not QOM objects, replace PROP_PTR of clocks with
setters methods.

(in theory there should probably be different methods for omap1 &
omap2 intc, but this is left as a future improvement)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/omap1.c        |  4 ++--
 hw/arm/omap2.c        |  4 ++--
 hw/intc/omap_intc.c   | 17 ++++++++++-------
 include/hw/arm/omap.h | 10 ++++++++++
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 0400593805..c5583b10e0 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -3891,7 +3891,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
 
     s->ih[0] = qdev_create(NULL, "omap-intc");
     qdev_prop_set_uint32(s->ih[0], "size", 0x100);
-    qdev_prop_set_ptr(s->ih[0], "clk", omap_findclk(s, "arminth_ck"));
+    omap_intc_set_iclk(OMAP_INTC(s->ih[0]), omap_findclk(s, "arminth_ck"));
     qdev_init_nofail(s->ih[0]);
     busdev = SYS_BUS_DEVICE(s->ih[0]);
     sysbus_connect_irq(busdev, 0,
@@ -3901,7 +3901,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
     sysbus_mmio_map(busdev, 0, 0xfffecb00);
     s->ih[1] = qdev_create(NULL, "omap-intc");
     qdev_prop_set_uint32(s->ih[1], "size", 0x800);
-    qdev_prop_set_ptr(s->ih[1], "clk", omap_findclk(s, "arminth_ck"));
+    omap_intc_set_iclk(OMAP_INTC(s->ih[1]), omap_findclk(s, "arminth_ck"));
     qdev_init_nofail(s->ih[1]);
     busdev = SYS_BUS_DEVICE(s->ih[1]);
     sysbus_connect_irq(busdev, 0,
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index bd7ddff983..726a628e64 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -2311,8 +2311,8 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
     /* Actually mapped at any 2K boundary in the ARM11 private-peripheral if */
     s->ih[0] = qdev_create(NULL, "omap2-intc");
     qdev_prop_set_uint8(s->ih[0], "revision", 0x21);
-    qdev_prop_set_ptr(s->ih[0], "fclk", omap_findclk(s, "mpu_intc_fclk"));
-    qdev_prop_set_ptr(s->ih[0], "iclk", omap_findclk(s, "mpu_intc_iclk"));
+    omap_intc_set_fclk(OMAP_INTC(s->ih[0]), omap_findclk(s, "mpu_intc_fclk"));
+    omap_intc_set_iclk(OMAP_INTC(s->ih[0]), omap_findclk(s, "mpu_intc_iclk"));
     qdev_init_nofail(s->ih[0]);
     busdev = SYS_BUS_DEVICE(s->ih[0]);
     sysbus_connect_irq(busdev, 0,
diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
index 854b709ca0..73bb1c2af4 100644
--- a/hw/intc/omap_intc.c
+++ b/hw/intc/omap_intc.c
@@ -38,10 +38,6 @@ struct omap_intr_handler_bank_s {
     unsigned char priority[32];
 };
 
-#define TYPE_OMAP_INTC "common-omap-intc"
-#define OMAP_INTC(obj) \
-    OBJECT_CHECK(struct omap_intr_handler_s, (obj), TYPE_OMAP_INTC)
-
 struct omap_intr_handler_s {
     SysBusDevice parent_obj;
 
@@ -391,9 +387,18 @@ static void omap_intc_realize(DeviceState *dev, Error **errp)
     }
 }
 
+void omap_intc_set_iclk(omap_intr_handler *intc, omap_clk clk)
+{
+    intc->iclk = clk;
+}
+
+void omap_intc_set_fclk(omap_intr_handler *intc, omap_clk clk)
+{
+    intc->fclk = clk;
+}
+
 static Property omap_intc_properties[] = {
     DEFINE_PROP_UINT32("size", struct omap_intr_handler_s, size, 0x100),
-    DEFINE_PROP_PTR("clk", struct omap_intr_handler_s, iclk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -647,8 +652,6 @@ static void omap2_intc_realize(DeviceState *dev, Error **errp)
 static Property omap2_intc_properties[] = {
     DEFINE_PROP_UINT8("revision", struct omap_intr_handler_s,
     revision, 0x21),
-    DEFINE_PROP_PTR("iclk", struct omap_intr_handler_s, iclk),
-    DEFINE_PROP_PTR("fclk", struct omap_intr_handler_s, fclk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 2fda996648..6b7897ba27 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -67,6 +67,16 @@ void omap_clk_setrate(omap_clk clk, int divide, int multiply);
 int64_t omap_clk_getrate(omap_clk clk);
 void omap_clk_reparent(omap_clk clk, omap_clk parent);
 
+/* omap_intc.c */
+#define TYPE_OMAP_INTC "common-omap-intc"
+#define OMAP_INTC(obj)                                              \
+    OBJECT_CHECK(omap_intr_handler, (obj), TYPE_OMAP_INTC)
+
+typedef struct omap_intr_handler_s omap_intr_handler;
+
+void omap_intc_set_iclk(omap_intr_handler *intc, omap_clk clk);
+void omap_intc_set_fclk(omap_intr_handler *intc, omap_clk clk);
+
 /* OMAP2 l4 Interconnect */
 struct omap_l4_s;
 struct omap_l4_region_s {
-- 
2.23.0.606.g08da6496b6

[PATCH 11/14] omap-i2c: remove PROP_PTR

[Marc-André Lureau]

Since clock are not QOM objects, replace PROP_PTR of clocks with
setters methods.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/omap1.c        |  2 +-
 hw/arm/omap2.c        |  8 ++++----
 hw/i2c/omap_i2c.c     | 19 ++++++++++++-------
 include/hw/arm/omap.h |  9 +++++++++
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index c5583b10e0..fe55c44c7e 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -4032,7 +4032,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
 
     s->i2c[0] = qdev_create(NULL, "omap_i2c");
     qdev_prop_set_uint8(s->i2c[0], "revision", 0x11);
-    qdev_prop_set_ptr(s->i2c[0], "fclk", omap_findclk(s, "mpuper_ck"));
+    omap_i2c_set_fclk(OMAP_I2C(s->i2c[0]), omap_findclk(s, "mpuper_ck"));
     qdev_init_nofail(s->i2c[0]);
     busdev = SYS_BUS_DEVICE(s->i2c[0]);
     sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(s->ih[1], OMAP_INT_I2C));
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index 726a628e64..046fb6ffb5 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -2428,8 +2428,8 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
 
     s->i2c[0] = qdev_create(NULL, "omap_i2c");
     qdev_prop_set_uint8(s->i2c[0], "revision", 0x34);
-    qdev_prop_set_ptr(s->i2c[0], "iclk", omap_findclk(s, "i2c1.iclk"));
-    qdev_prop_set_ptr(s->i2c[0], "fclk", omap_findclk(s, "i2c1.fclk"));
+    omap_i2c_set_iclk(OMAP_I2C(s->i2c[0]), omap_findclk(s, "i2c1.iclk"));
+    omap_i2c_set_fclk(OMAP_I2C(s->i2c[0]), omap_findclk(s, "i2c1.fclk"));
     qdev_init_nofail(s->i2c[0]);
     busdev = SYS_BUS_DEVICE(s->i2c[0]);
     sysbus_connect_irq(busdev, 0,
@@ -2440,8 +2440,8 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
 
     s->i2c[1] = qdev_create(NULL, "omap_i2c");
     qdev_prop_set_uint8(s->i2c[1], "revision", 0x34);
-    qdev_prop_set_ptr(s->i2c[1], "iclk", omap_findclk(s, "i2c2.iclk"));
-    qdev_prop_set_ptr(s->i2c[1], "fclk", omap_findclk(s, "i2c2.fclk"));
+    omap_i2c_set_iclk(OMAP_I2C(s->i2c[1]), omap_findclk(s, "i2c2.iclk"));
+    omap_i2c_set_fclk(OMAP_I2C(s->i2c[1]), omap_findclk(s, "i2c2.fclk"));
     qdev_init_nofail(s->i2c[1]);
     busdev = SYS_BUS_DEVICE(s->i2c[1]);
     sysbus_connect_irq(busdev, 0,
diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
index 3ba965a58f..3ccbd5cc2c 100644
--- a/hw/i2c/omap_i2c.c
+++ b/hw/i2c/omap_i2c.c
@@ -28,10 +28,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 
-#define TYPE_OMAP_I2C "omap_i2c"
-#define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
-
-typedef struct OMAPI2CState {
+struct OMAPI2CState {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
@@ -56,7 +53,7 @@ typedef struct OMAPI2CState {
     uint8_t divider;
     uint8_t times[2];
     uint16_t test;
-} OMAPI2CState;
+};
 
 #define OMAP2_INTR_REV	0x34
 #define OMAP2_GC_REV	0x34
@@ -504,10 +501,18 @@ static void omap_i2c_realize(DeviceState *dev, Error **errp)
     }
 }
 
+void omap_i2c_set_iclk(OMAPI2CState *i2c, omap_clk clk)
+{
+    i2c->iclk = clk;
+}
+
+void omap_i2c_set_fclk(OMAPI2CState *i2c, omap_clk clk)
+{
+    i2c->fclk = clk;
+}
+
 static Property omap_i2c_properties[] = {
     DEFINE_PROP_UINT8("revision", OMAPI2CState, revision, 0),
-    DEFINE_PROP_PTR("iclk", OMAPI2CState, iclk),
-    DEFINE_PROP_PTR("fclk", OMAPI2CState, fclk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 6b7897ba27..08ee0c7702 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -77,6 +77,15 @@ typedef struct omap_intr_handler_s omap_intr_handler;
 void omap_intc_set_iclk(omap_intr_handler *intc, omap_clk clk);
 void omap_intc_set_fclk(omap_intr_handler *intc, omap_clk clk);
 
+/* omap_i2c.c */
+#define TYPE_OMAP_I2C "omap_i2c"
+#define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
+
+typedef struct OMAPI2CState OMAPI2CState;
+
+void omap_i2c_set_iclk(OMAPI2CState *i2c, omap_clk clk);
+void omap_i2c_set_fclk(OMAPI2CState *i2c, omap_clk clk);
+
 /* OMAP2 l4 Interconnect */
 struct omap_l4_s;
 struct omap_l4_region_s {
-- 
2.23.0.606.g08da6496b6

[PATCH 12/14] omap-gpio: remove PROP_PTR

[Marc-André Lureau]

Since clock are not QOM objects, replace PROP_PTR of clocks with
setters methods.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/omap1.c        |  2 +-
 hw/arm/omap2.c        | 13 +++++++------
 hw/gpio/omap_gpio.c   | 42 +++++++++++++++---------------------------
 include/hw/arm/omap.h | 17 +++++++++++++++++
 4 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index fe55c44c7e..f13d46afaa 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -4014,7 +4014,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
 
     s->gpio = qdev_create(NULL, "omap-gpio");
     qdev_prop_set_int32(s->gpio, "mpu_model", s->mpu_model);
-    qdev_prop_set_ptr(s->gpio, "clk", omap_findclk(s, "arm_gpio_ck"));
+    omap_gpio_set_clk(OMAP1_GPIO(s->gpio), omap_findclk(s, "arm_gpio_ck"));
     qdev_init_nofail(s->gpio);
     sysbus_connect_irq(SYS_BUS_DEVICE(s->gpio), 0,
                        qdev_get_gpio_in(s->ih[0], OMAP_INT_GPIO_BANK1));
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index 046fb6ffb5..d5c20f6486 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -2452,13 +2452,14 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
 
     s->gpio = qdev_create(NULL, "omap2-gpio");
     qdev_prop_set_int32(s->gpio, "mpu_model", s->mpu_model);
-    qdev_prop_set_ptr(s->gpio, "iclk", omap_findclk(s, "gpio_iclk"));
-    qdev_prop_set_ptr(s->gpio, "fclk0", omap_findclk(s, "gpio1_dbclk"));
-    qdev_prop_set_ptr(s->gpio, "fclk1", omap_findclk(s, "gpio2_dbclk"));
-    qdev_prop_set_ptr(s->gpio, "fclk2", omap_findclk(s, "gpio3_dbclk"));
-    qdev_prop_set_ptr(s->gpio, "fclk3", omap_findclk(s, "gpio4_dbclk"));
+    omap2_gpio_set_iclk(OMAP2_GPIO(s->gpio), omap_findclk(s, "gpio_iclk"));
+    omap2_gpio_set_fclk(OMAP2_GPIO(s->gpio), 0, omap_findclk(s, "gpio1_dbclk"));
+    omap2_gpio_set_fclk(OMAP2_GPIO(s->gpio), 1, omap_findclk(s, "gpio2_dbclk"));
+    omap2_gpio_set_fclk(OMAP2_GPIO(s->gpio), 2, omap_findclk(s, "gpio3_dbclk"));
+    omap2_gpio_set_fclk(OMAP2_GPIO(s->gpio), 3, omap_findclk(s, "gpio4_dbclk"));
     if (s->mpu_model == omap2430) {
-        qdev_prop_set_ptr(s->gpio, "fclk4", omap_findclk(s, "gpio5_dbclk"));
+        omap2_gpio_set_fclk(OMAP2_GPIO(s->gpio), 4,
+                            omap_findclk(s, "gpio5_dbclk"));
     }
     qdev_init_nofail(s->gpio);
     busdev = SYS_BUS_DEVICE(s->gpio);
diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
index 41e1aa798c..85c16897ae 100644
--- a/hw/gpio/omap_gpio.c
+++ b/hw/gpio/omap_gpio.c
@@ -40,10 +40,6 @@ struct omap_gpio_s {
     uint16_t pins;
 };
 
-#define TYPE_OMAP1_GPIO "omap-gpio"
-#define OMAP1_GPIO(obj) \
-    OBJECT_CHECK(struct omap_gpif_s, (obj), TYPE_OMAP1_GPIO)
-
 struct omap_gpif_s {
     SysBusDevice parent_obj;
 
@@ -212,10 +208,6 @@ struct omap2_gpio_s {
     uint8_t delay;
 };
 
-#define TYPE_OMAP2_GPIO "omap2-gpio"
-#define OMAP2_GPIO(obj) \
-    OBJECT_CHECK(struct omap2_gpif_s, (obj), TYPE_OMAP2_GPIO)
-
 struct omap2_gpif_s {
     SysBusDevice parent_obj;
 
@@ -747,21 +739,13 @@ static void omap2_gpio_realize(DeviceState *dev, Error **errp)
     }
 }
 
-/* Using qdev pointer properties for the clocks is not ideal.
- * qdev should support a generic means of defining a 'port' with
- * an arbitrary interface for connecting two devices. Then we
- * could reframe the omap clock API in terms of clock ports,
- * and get some type safety. For now the best qdev provides is
- * passing an arbitrary pointer.
- * (It's not possible to pass in the string which is the clock
- * name, because this device does not have the necessary information
- * (ie the struct omap_mpu_state_s*) to do the clockname to pointer
- * translation.)
- */
+void omap_gpio_set_clk(omap_gpif *gpio, omap_clk clk)
+{
+    gpio->clk = clk;
+}
 
 static Property omap_gpio_properties[] = {
     DEFINE_PROP_INT32("mpu_model", struct omap_gpif_s, mpu_model, 0),
-    DEFINE_PROP_PTR("clk", struct omap_gpif_s, clk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -784,15 +768,19 @@ static const TypeInfo omap_gpio_info = {
     .class_init    = omap_gpio_class_init,
 };
 
+void omap2_gpio_set_iclk(omap2_gpif *gpio, omap_clk clk)
+{
+    gpio->iclk = clk;
+}
+
+void omap2_gpio_set_fclk(omap2_gpif *gpio, uint8_t i, omap_clk clk)
+{
+    assert(i <= 5);
+    gpio->fclk[i] = clk;
+}
+
 static Property omap2_gpio_properties[] = {
     DEFINE_PROP_INT32("mpu_model", struct omap2_gpif_s, mpu_model, 0),
-    DEFINE_PROP_PTR("iclk", struct omap2_gpif_s, iclk),
-    DEFINE_PROP_PTR("fclk0", struct omap2_gpif_s, fclk[0]),
-    DEFINE_PROP_PTR("fclk1", struct omap2_gpif_s, fclk[1]),
-    DEFINE_PROP_PTR("fclk2", struct omap2_gpif_s, fclk[2]),
-    DEFINE_PROP_PTR("fclk3", struct omap2_gpif_s, fclk[3]),
-    DEFINE_PROP_PTR("fclk4", struct omap2_gpif_s, fclk[4]),
-    DEFINE_PROP_PTR("fclk5", struct omap2_gpif_s, fclk[5]),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 08ee0c7702..97115891f8 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -86,6 +86,23 @@ typedef struct OMAPI2CState OMAPI2CState;
 void omap_i2c_set_iclk(OMAPI2CState *i2c, omap_clk clk);
 void omap_i2c_set_fclk(OMAPI2CState *i2c, omap_clk clk);
 
+/* omap_gpio.c */
+#define TYPE_OMAP1_GPIO "omap-gpio"
+#define OMAP1_GPIO(obj)                                         \
+    OBJECT_CHECK(struct omap_gpif_s, (obj), TYPE_OMAP1_GPIO)
+
+#define TYPE_OMAP2_GPIO "omap2-gpio"
+#define OMAP2_GPIO(obj)                                         \
+    OBJECT_CHECK(struct omap2_gpif_s, (obj), TYPE_OMAP2_GPIO)
+
+typedef struct omap_gpif_s omap_gpif;
+typedef struct omap2_gpif_s omap2_gpif;
+
+void omap_gpio_set_clk(omap_gpif *gpio, omap_clk clk);
+
+void omap2_gpio_set_iclk(omap2_gpif *gpio, omap_clk clk);
+void omap2_gpio_set_fclk(omap2_gpif *gpio, uint8_t i, omap_clk clk);
+
 /* OMAP2 l4 Interconnect */
 struct omap_l4_s;
 struct omap_l4_region_s {
-- 
2.23.0.606.g08da6496b6

[PATCH 13/14] qdev: remove PROP_MEMORY_REGION

[Marc-André Lureau]

PROP_MEMORY_REGION was a derivative of PROP_PTR, added in commit
ed03d749f3f513b8fb0287757cfda2cb6825f063 (qdev: add MemoryRegion
property) and thankfully no longer needed since commit
3eff40dbf44896a8180c86c84dbdefb2eb173fbe (hw/misc: Remove
mmio_interface device).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/qdev-properties.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index c6a8cb5516..16837ab5dd 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -216,8 +216,6 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
     DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
 #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
-#define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
-    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, MemoryRegion *)
 #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
                         OffAutoPCIBAR)
-- 
2.23.0.606.g08da6496b6

[PATCH 14/14] Remove QDEV_PROP_PTR

[Marc-André Lureau]

No longer used in the tree. The comment about user_creatable is still
quite relevant, but there is already a similar comment in qdev-core.h.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/core/qdev-properties.c    | 18 ------------------
 include/hw/qdev-properties.h | 22 ----------------------
 2 files changed, 40 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index ac28890e5a..6ca7697599 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -501,13 +501,6 @@ const PropertyInfo qdev_prop_string = {
     .set   = set_string,
 };
 
-/* --- pointer --- */
-
-/* Not a proper property, just for dirty hacks.  TODO Remove it!  */
-const PropertyInfo qdev_prop_ptr = {
-    .name  = "ptr",
-};
-
 /* --- mac address --- */
 
 /*
@@ -1165,17 +1158,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
                             name, &error_abort);
 }
 
-void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
-{
-    Property *prop;
-    void **ptr;
-
-    prop = qdev_prop_find(dev, name);
-    assert(prop && prop->info == &qdev_prop_ptr);
-    ptr = qdev_get_prop_ptr(dev, prop);
-    *ptr = value;
-}
-
 static GPtrArray *global_props(void)
 {
     static GPtrArray *gp;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 16837ab5dd..a90a9cec80 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -18,7 +18,6 @@ extern const PropertyInfo qdev_prop_size;
 extern const PropertyInfo qdev_prop_string;
 extern const PropertyInfo qdev_prop_chr;
 extern const PropertyInfo qdev_prop_tpm;
-extern const PropertyInfo qdev_prop_ptr;
 extern const PropertyInfo qdev_prop_macaddr;
 extern const PropertyInfo qdev_prop_on_off_auto;
 extern const PropertyInfo qdev_prop_losttickpolicy;
@@ -171,25 +170,6 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
 
-/*
- * Please avoid pointer properties.  If you must use them, you must
- * cover them in their device's class init function as follows:
- *
- * - If the property must be set, the device cannot be used with
- *   device_add, so add code like this:
- *   |* Reason: pointer property "NAME-OF-YOUR-PROP" *|
- *   DeviceClass *dc = DEVICE_CLASS(class);
- *   dc->user_creatable = false;
- *
- * - If the property may safely remain null, document it like this:
- *   |*
- *    * Note: pointer property "interrupt_vector" may remain null, thus
- *    * no need for dc->user_creatable = false;
- *    *|
- */
-#define DEFINE_PROP_PTR(_n, _s, _f)             \
-    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)
-
 #define DEFINE_PROP_CHR(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_chr, CharBackend)
 #define DEFINE_PROP_STRING(_n, _s, _f)             \
@@ -262,8 +242,6 @@ void qdev_prop_set_drive(DeviceState *dev, const char *name,
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
                            const uint8_t *value);
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
-/* FIXME: Remove opaque pointer properties.  */
-void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
 void qdev_prop_register_global(GlobalProperty *prop);
 int qdev_prop_check_globals(void);
-- 
2.23.0.606.g08da6496b6

Re: [PATCH 04/14] etraxfs: remove PROP_PTR usage

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> etraxfs_dma_client are not Object, so can't be exposed to user with
> QOM path. Let's remove property usage and move the constructor to the
> .c unit, simplifying some code on the way.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +
> +/* Instantiate an ETRAXFS Ethernet MAC.  */
> +DeviceState *
> +etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> +                 struct etraxfs_dma_client *dma_out,
> +                 struct etraxfs_dma_client *dma_in)
> +{
> +    DeviceState *dev;
> +    qemu_check_nic_model(nd, "fseth");
> +
> +    dev = qdev_create(NULL, "etraxfs-eth");
> +    qdev_set_nic_properties(dev, nd);
> +    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> +    ETRAX_FS_ETH(dev)->dma_out = dma_out;
> +    ETRAX_FS_ETH(dev)->dma_in = dma_in;
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +
> +    return dev;
> +}
> +
>  static const TypeInfo etraxfs_eth_info = {
>      .name          = TYPE_ETRAX_FS_ETH,
>      .parent        = TYPE_SYS_BUS_DEVICE,
> diff --git a/include/hw/cris/etraxfs.h b/include/hw/cris/etraxfs.h
> index aa146a2cd8..403e7f95e6 100644
> --- a/include/hw/cris/etraxfs.h
> +++ b/include/hw/cris/etraxfs.h
> @@ -30,23 +30,9 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/sysbus.h"
>
> -/* Instantiate an ETRAXFS Ethernet MAC.  */
> -static inline DeviceState *
> -etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> -                 void *dma_out, void *dma_in)
> -{
> -    DeviceState *dev;
> -    qemu_check_nic_model(nd, "fseth");
> -
> -    dev = qdev_create(NULL, "etraxfs-eth");
> -    qdev_set_nic_properties(dev, nd);
> -    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> -    qdev_prop_set_ptr(dev, "dma_out", dma_out);
> -    qdev_prop_set_ptr(dev, "dma_in", dma_in);
> -    qdev_init_nofail(dev);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> -    return dev;
> -}
> +DeviceState *etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> +                              struct etraxfs_dma_client *dma_out,
> +                              struct etraxfs_dma_client *dma_in);


I don't think this is an improvement -- it's taking a step
back in the direction of "you need to call a funny _init
function to initialize a device". You should be able to
create devices using generic qdev functions.

What we're actually connecting here is 'etraxfs_dma_client'
struct pointers between the devices like this ethernet
device and the DMA controller. The connection is currently
done via a pointer property because we don't have a more
QOM-like way to do it, but if we want to get rid of the
pointer property we need to actually implement the more
QOM-like mechanism, not go backwards from having devices
connected via properties.

(Similar comments apply for the omap clock connections.
In that case the answer might be Damien's clock framework
API, eventually.)

thanks
-- PMM

Re: [PATCH 04/14] etraxfs: remove PROP_PTR usage

[Marc-André Lureau]

Hi

On Fri, Oct 18, 2019 at 5:59 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > etraxfs_dma_client are not Object, so can't be exposed to user with
> > QOM path. Let's remove property usage and move the constructor to the
> > .c unit, simplifying some code on the way.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> > +
> > +/* Instantiate an ETRAXFS Ethernet MAC.  */
> > +DeviceState *
> > +etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> > +                 struct etraxfs_dma_client *dma_out,
> > +                 struct etraxfs_dma_client *dma_in)
> > +{
> > +    DeviceState *dev;
> > +    qemu_check_nic_model(nd, "fseth");
> > +
> > +    dev = qdev_create(NULL, "etraxfs-eth");
> > +    qdev_set_nic_properties(dev, nd);
> > +    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> > +    ETRAX_FS_ETH(dev)->dma_out = dma_out;
> > +    ETRAX_FS_ETH(dev)->dma_in = dma_in;
> > +    qdev_init_nofail(dev);
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> > +
> > +    return dev;
> > +}
> > +
> >  static const TypeInfo etraxfs_eth_info = {
> >      .name          = TYPE_ETRAX_FS_ETH,
> >      .parent        = TYPE_SYS_BUS_DEVICE,
> > diff --git a/include/hw/cris/etraxfs.h b/include/hw/cris/etraxfs.h
> > index aa146a2cd8..403e7f95e6 100644
> > --- a/include/hw/cris/etraxfs.h
> > +++ b/include/hw/cris/etraxfs.h
> > @@ -30,23 +30,9 @@
> >  #include "hw/qdev-properties.h"
> >  #include "hw/sysbus.h"
> >
> > -/* Instantiate an ETRAXFS Ethernet MAC.  */
> > -static inline DeviceState *
> > -etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> > -                 void *dma_out, void *dma_in)
> > -{
> > -    DeviceState *dev;
> > -    qemu_check_nic_model(nd, "fseth");
> > -
> > -    dev = qdev_create(NULL, "etraxfs-eth");
> > -    qdev_set_nic_properties(dev, nd);
> > -    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> > -    qdev_prop_set_ptr(dev, "dma_out", dma_out);
> > -    qdev_prop_set_ptr(dev, "dma_in", dma_in);
> > -    qdev_init_nofail(dev);
> > -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> > -    return dev;
> > -}
> > +DeviceState *etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> > +                              struct etraxfs_dma_client *dma_out,
> > +                              struct etraxfs_dma_client *dma_in);
>
>
> I don't think this is an improvement -- it's taking a step
> back in the direction of "you need to call a funny _init
> function to initialize a device". You should be able to
> create devices using generic qdev functions.
>

Is there really much difference between:

dev = qdev_create()
qdev_prop_set_ptr(dev, "prop", ptr);
qdev_init_nofail()

and

dev = qdev_create(MYDEV)
MYDEV(dev)->prop = ptr;
qdev_init_nofail()


As "prop" can only be set from code, and those objects are usually
very tightly coupled with the parent/owner.

I don't think it's worth to keep PROP_PTR for this, it just adds complexity.

> What we're actually connecting here is 'etraxfs_dma_client'
> struct pointers between the devices like this ethernet
> device and the DMA controller. The connection is currently
> done via a pointer property because we don't have a more
> QOM-like way to do it, but if we want to get rid of the
> pointer property we need to actually implement the more
> QOM-like mechanism, not go backwards from having devices
> connected via properties.
>
> (Similar comments apply for the omap clock connections.
> In that case the answer might be Damien's clock framework
> API, eventually.)
>
> thanks
> -- PMM
>


-- 
Marc-André Lureau

Re: [PATCH 01/14] sm501: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/sm501.c | 5 +++--
>  hw/sh4/r2d.c       | 3 ++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 1f33c87e65..a87d18efab 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1930,7 +1930,7 @@ typedef struct {
>      SM501State state;
>      uint32_t vram_size;
>      uint32_t base;
> -    void *chr_state;
> +    Chardev *chr_state;
>  } SM501SysBusState;
>
>  static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> @@ -1968,7 +1968,8 @@ 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_PTR("chr-state", SM501SysBusState, chr_state),
> +    DEFINE_PROP_LINK("chr-state", SM501SysBusState, chr_state,
> +                     TYPE_CHARDEV, Chardev *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index ee0840f380..5780ee85d9 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -272,7 +272,8 @@ static void r2d_init(MachineState *machine)
>      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_ptr(dev, "chr-state", serial_hd(2));
> +    object_property_set_link(OBJECT(dev), OBJECT(serial_hd(2)),
> +                             "chr-state", &error_abort);
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(busdev, 0, 0x10000000);
>      sysbus_mmio_map(busdev, 1, 0x13e00000);

We have a typed way of passing a Chardev to devices:
use qdev_prop_set_chr(). Unfortunately it's not trivially
easy to drop in here, because it sets a property defined
with DEFINE_PROP_CHR to set a field of type CharBackend
(note, not Chardev, and not a pointer) in the device struct.
But serial_mm_init() wants a Chardev*, because it is a
non-QOM interface to the serial device and is manually
doing the qemu_chr_fe_init() that connects the Chardev
to its own CharBackend. The QOM CHR property setter wants
to do that qemu_chr_fe_init() itself.

So I think the right fix here is to properly QOMify the
code which is not QOMified, ie hw/char/serial.c, in a
way that means that the various "memory mapped 16650-ish
devices" we have can use it and can define a
TYPE_CHARDEV property.

In general I think our uses of PROP_PTR are code smells
that indicate places where we have not properly converted
code over to the general approach that the QOM/qdev
design desires; but we should be getting rid of PROP_PTR
by actually doing all those (difficult) conversions.
Merely removing PROP_PTR itself by rephrasing the dubious
inter-device connections in a way that makes them harder
to grep for doesn't seem to me to be necessarily worth
doing. Is the existence of PROP_PTR getting in your way
for a change you want to make ?

thanks
-- PMM

Re: [PATCH 04/14] etraxfs: remove PROP_PTR usage

[Peter Maydell]

On Fri, 18 Oct 2019 at 17:11, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Is there really much difference between:
>
> dev = qdev_create()
> qdev_prop_set_ptr(dev, "prop", ptr);
> qdev_init_nofail()
>
> and
>
> dev = qdev_create(MYDEV)
> MYDEV(dev)->prop = ptr;
> qdev_init_nofail()

One is easier to grep for than the other if you're
looking for things we need to fix :-)

I've just replied to patch 1 of this set with a reply that
gives my general objections which are the same for this patch
as that one; it's probably less confusing if we keep the
conversation in that thread rather than conducting it along
two parallel tracks (my fault for not reading the whole
series first; I should probably have replied to the cover
letter).

thanks
-- PMM

Re: [PATCH 01/14] sm501: replace PROP_PTR with PROP_LINK

[Marc-André Lureau]

Hi

On Fri, Oct 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/display/sm501.c | 5 +++--
> >  hw/sh4/r2d.c       | 3 ++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 1f33c87e65..a87d18efab 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -1930,7 +1930,7 @@ typedef struct {
> >      SM501State state;
> >      uint32_t vram_size;
> >      uint32_t base;
> > -    void *chr_state;
> > +    Chardev *chr_state;
> >  } SM501SysBusState;
> >
> >  static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> > @@ -1968,7 +1968,8 @@ 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_PTR("chr-state", SM501SysBusState, chr_state),
> > +    DEFINE_PROP_LINK("chr-state", SM501SysBusState, chr_state,
> > +                     TYPE_CHARDEV, Chardev *),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> > index ee0840f380..5780ee85d9 100644
> > --- a/hw/sh4/r2d.c
> > +++ b/hw/sh4/r2d.c
> > @@ -272,7 +272,8 @@ static void r2d_init(MachineState *machine)
> >      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_ptr(dev, "chr-state", serial_hd(2));
> > +    object_property_set_link(OBJECT(dev), OBJECT(serial_hd(2)),
> > +                             "chr-state", &error_abort);
> >      qdev_init_nofail(dev);
> >      sysbus_mmio_map(busdev, 0, 0x10000000);
> >      sysbus_mmio_map(busdev, 1, 0x13e00000);
>
> We have a typed way of passing a Chardev to devices:
> use qdev_prop_set_chr(). Unfortunately it's not trivially
> easy to drop in here, because it sets a property defined
> with DEFINE_PROP_CHR to set a field of type CharBackend
> (note, not Chardev, and not a pointer) in the device struct.
> But serial_mm_init() wants a Chardev*, because it is a
> non-QOM interface to the serial device and is manually
> doing the qemu_chr_fe_init() that connects the Chardev
> to its own CharBackend. The QOM CHR property setter wants
> to do that qemu_chr_fe_init() itself.
>
> So I think the right fix here is to properly QOMify the
> code which is not QOMified, ie hw/char/serial.c, in a
> way that means that the various "memory mapped 16650-ish
> devices" we have can use it and can define a
> TYPE_CHARDEV property.

I see, I can look at that.

> In general I think our uses of PROP_PTR are code smells
> that indicate places where we have not properly converted
> code over to the general approach that the QOM/qdev
> design desires; but we should be getting rid of PROP_PTR
> by actually doing all those (difficult) conversions.

I think all user_creatable = false are smelly in that regard.

> Merely removing PROP_PTR itself by rephrasing the dubious
> inter-device connections in a way that makes them harder
> to grep for doesn't seem to me to be necessarily worth

grep for user_creatable = false

> doing. Is the existence of PROP_PTR getting in your way
> for a change you want to make ?

Yes, I am looking at improving the qdev vs object and class vs
instance properties. I have a larger series of wip refactoring. This
initial series is preliminary cleanup that would help.

Re: [PATCH 01/14] sm501: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 17:36, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> On Fri, Oct 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > So I think the right fix here is to properly QOMify the
> > code which is not QOMified, ie hw/char/serial.c, in a
> > way that means that the various "memory mapped 16650-ish
> > devices" we have can use it and can define a
> > TYPE_CHARDEV property.
>
> I see, I can look at that.

It would certainly be a nice cleanup/refactoring to have;
it's likely quite painful though as so many things use
serial_mm_init, many of them not properly QOMified themselves;
and because SerialState is used by x86 it's likely to be
tricky to handle for migration-compat reasons.

> > In general I think our uses of PROP_PTR are code smells
> > that indicate places where we have not properly converted
> > code over to the general approach that the QOM/qdev
> > design desires; but we should be getting rid of PROP_PTR
> > by actually doing all those (difficult) conversions.
>
> I think all user_creatable = false are smelly in that regard.

I dunno, I tend to think that "not user creatable" is the
default state of things. Almost all devices aren't user
creatable, because they need to be wired up and put at
suitable addresses and IRQ lines. Pluggable devices like
PCI, ISA, etc are the weird exceptions that are usefully
user-creatable.

> > Merely removing PROP_PTR itself by rephrasing the dubious
> > inter-device connections in a way that makes them harder
> > to grep for doesn't seem to me to be necessarily worth
>
> grep for user_creatable = false
>
> > doing. Is the existence of PROP_PTR getting in your way
> > for a change you want to make ?
>
> Yes, I am looking at improving the qdev vs object and class vs
> instance properties. I have a larger series of wip refactoring. This
> initial series is preliminary cleanup that would help.

OK, that makes sense. I'll have a look through the series
again; if we don't want to do the nice cleanups now then
probably these changes are ok, possibly with some TODO
comments noting what we ideally ought to do instead.

thanks
-- PMM

Re: [PATCH 10/14] omap-intc: remove PROP_PTR

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Since clock are not QOM objects, replace PROP_PTR of clocks with
> setters methods.
>
> (in theory there should probably be different methods for omap1 &
> omap2 intc, but this is left as a future improvement)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
> index 2fda996648..6b7897ba27 100644
> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -67,6 +67,16 @@ void omap_clk_setrate(omap_clk clk, int divide, int multiply);
>  int64_t omap_clk_getrate(omap_clk clk);
>  void omap_clk_reparent(omap_clk clk, omap_clk parent);
>
> +/* omap_intc.c */
> +#define TYPE_OMAP_INTC "common-omap-intc"
> +#define OMAP_INTC(obj)                                              \
> +    OBJECT_CHECK(omap_intr_handler, (obj), TYPE_OMAP_INTC)
> +
> +typedef struct omap_intr_handler_s omap_intr_handler;
> +

If you add
/*
 * TODO: Ideally we should have a clock framework that
 * let us wire these clocks up with QOM properties or links.
 */

> +void omap_intc_set_iclk(omap_intr_handler *intc, omap_clk clk);
> +void omap_intc_set_fclk(omap_intr_handler *intc, omap_clk clk);
> +

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

thanks
-- PMM

Re: [PATCH 11/14] omap-i2c: remove PROP_PTR

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Since clock are not QOM objects, replace PROP_PTR of clocks with
> setters methods.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -77,6 +77,15 @@ typedef struct omap_intr_handler_s omap_intr_handler;
>  void omap_intc_set_iclk(omap_intr_handler *intc, omap_clk clk);
>  void omap_intc_set_fclk(omap_intr_handler *intc, omap_clk clk);
>
> +/* omap_i2c.c */
> +#define TYPE_OMAP_I2C "omap_i2c"
> +#define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
> +
> +typedef struct OMAPI2CState OMAPI2CState;
> +
> +void omap_i2c_set_iclk(OMAPI2CState *i2c, omap_clk clk);
> +void omap_i2c_set_fclk(OMAPI2CState *i2c, omap_clk clk);
> +

With a similar TODO header comment (or perhaps this is close
enough to the one from the previous patch that it doesn't
need repeating)

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

thanks
-- PMM

Re: [PATCH 12/14] omap-gpio: remove PROP_PTR

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Since clock are not QOM objects, replace PROP_PTR of clocks with
> setters methods.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> -/* Using qdev pointer properties for the clocks is not ideal.
> - * qdev should support a generic means of defining a 'port' with
> - * an arbitrary interface for connecting two devices. Then we
> - * could reframe the omap clock API in terms of clock ports,
> - * and get some type safety. For now the best qdev provides is
> - * passing an arbitrary pointer.
> - * (It's not possible to pass in the string which is the clock
> - * name, because this device does not have the necessary information
> - * (ie the struct omap_mpu_state_s*) to do the clockname to pointer
> - * translation.)
> - */

Ah, here's what's effectively a todo comment that you could
repurpose/adapt/move.

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

thanks
-- PMM

Re: [PATCH 13/14] qdev: remove PROP_MEMORY_REGION

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:44, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> PROP_MEMORY_REGION was a derivative of PROP_PTR, added in commit
> ed03d749f3f513b8fb0287757cfda2cb6825f063 (qdev: add MemoryRegion
> property) and thankfully no longer needed since commit
> 3eff40dbf44896a8180c86c84dbdefb2eb173fbe (hw/misc: Remove
> mmio_interface device).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/qdev-properties.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index c6a8cb5516..16837ab5dd 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -216,8 +216,6 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>      DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
>  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> -#define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
> -    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, MemoryRegion *)
>  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
>                          OffAutoPCIBAR)
> --
> 2.23.0.606.g08da6496b6

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

thanks
-- PMM

Re: [PATCH 14/14] Remove QDEV_PROP_PTR

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:44, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> No longer used in the tree. The comment about user_creatable is still
> quite relevant, but there is already a similar comment in qdev-core.h.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 18 ------------------
>  include/hw/qdev-properties.h | 22 ----------------------
>  2 files changed, 40 deletions(-)

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

thanks
-- PMM

Re: [PATCH 07/14] RFC: mips/cps: fix setting saar property

[Philippe Mathieu-Daudé]

On 10/18/19 5:42 PM, Marc-André Lureau wrote:
> There is no "saar" property. Note: I haven't been able to test this
> code. Help welcome. >
> May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips:
> Update ITU to utilize SAARI and SAAR CP0 registers")

This seems a MIPS feature that is not fully mainstreamed, I suppose we 
miss the DSPRAM patches, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg638373.html

This code is currently not reachable:

$ git grep saarp
hw/mips/cps.c:98:    saar_present = (bool)env->saarp;
target/mips/cpu.h:1103:    int saarp;

Unfortunately the author email is bouncing:

host mips-com.mail.protection.outlook.com[104.47.46.36] said:
550 5.4.1 [yongbok.kim@mips.com]: Recipient address rejected:
Access denied [BN3NAM04FT027.eop-NAM04.prod.protection.outlook.com]
(in reply to RCPT TO command)

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/mips/cps.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 1660f86908..c49868d5da 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>           object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present",
>                                    &err);
>           if (saar_present) {
> -            qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void *)&env->CP0_SAAR);
> +            s->itu.saar = &env->CP0_SAAR;
>           }
>           object_property_set_bool(OBJECT(&s->itu), true, "realized", &err);
>           if (err != NULL) {
> 

Re: [PATCH 09/14] smbus-eeprom: remove PROP_PTR

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Instead, set the initial data field directly. Since it is only 256
> bytes, let's simply copy it to avoid invalid pointers issues.

(Commit message says you copy the 256 bytes, but the code
doesn't seem to do any copying?)

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Ah, the smbus code. Corey had a go at cleaning this up
a while back; I can't remember if we found a reason why
we had to keep the weird use of a pointer property here, or
if we just wanted to avoid dragging yet another thing into
an already large patchset.

What we're actually trying to set up here is always 256
bytes of data. What's the right way to pass a QOM device
a small chunk of data like that?

> ---
>  hw/i2c/smbus_eeprom.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 54c86a0112..533c728b3b 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -44,7 +44,7 @@
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
>      uint8_t data[SMBUS_EEPROM_SIZE];
> -    void *init_data;
> +    uint8_t *init_data;
>      uint8_t offset;
>      bool accessed;
>  } SMBusEEPROMDevice;
> @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev)
>
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
> +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> +
>      smbus_eeprom_reset(dev);
> +    if (eeprom->init_data == NULL) {
> +        error_setg(errp, "init_data cannot be NULL");
> +    }
>  }
>
> -static Property smbus_eeprom_properties[] = {
> -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      dc->reset = smbus_eeprom_reset;
>      sc->receive_byte = eeprom_receive_byte;
>      sc->write_data = eeprom_write_data;
> -    dc->props = smbus_eeprom_properties;
>      dc->vmsd = &vmstate_smbus_eeprom;
> -    /* Reason: pointer property "data" */
> +    /* Reason: init_data */
>      dc->user_creatable = false;
>  }
>
> @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>
>      dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
>      qdev_prop_set_uint8(dev, "address", address);
> -    qdev_prop_set_ptr(dev, "data", eeprom_buf);
> +    SMBUS_EEPROM(dev)->init_data = eeprom_buf;
>      qdev_init_nofail(dev);
>  }
>
> --
> 2.23.0.606.g08da6496b6

thanks
-- PMM

Re: [PATCH 08/14] cris: replace PROP_PTR with PROP_LINK for interrupt vector

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Instead of using raw interrupt vector pointer, store the associated
> CPU with a link property.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

A link property is reasonable for a tightly coupled CPU and
interrupt-controller. But in this case the binding is not
actually very tight, and we can avoid it.

> @@ -298,7 +296,7 @@ void axisdev88_init(MachineState *machine)
>
>      dev = qdev_create(NULL, "etraxfs,pic");
>      /* FIXME: Is there a proper way to signal vectors to the CPU core?  */
> -    qdev_prop_set_ptr(dev, "interrupt_vector", &env->interrupt_vector);
> +    object_property_set_link(OBJECT(dev), OBJECT(cpu), "cpu", &error_abort);

Rather than using a link property like this, we could
instead make use of the fact that a qemu_irq line is
actually capable of passing an arbitrary "int" value,
not merely a bool. To do this we would:

 * remove the FIXME comment and the ptr prop/link prop code here
 * remove the definition of the property from the PIC device

> @@ -79,9 +78,10 @@ static void pic_update(struct etrax_pic *fs)
>          }
>      }
>
> -    if (fs->interrupt_vector) {
> -        /* hack alert: ptr property */
> -        *(uint32_t*)(fs->interrupt_vector) = vector;
> +    if (fs->cpu) {
> +        /* hack alert: cpu link property */
> +        int32_t *int_vec = &fs->cpu->env.interrupt_vector;
> +        *int_vec = (uint32_t)vector;
>      }
>      qemu_set_irq(fs->parent_irq, !!vector);

 * here, instead of setting the CPU interrupt_vector field
   and passing !!vector to qemu_set_irq, we just pass "vector",
   so the other end gets the whole integer

 * in target/cris/cpu.c:cris_cpu_set_irq(), we add something like
   if (irq == CRIS_CPU_IRQ) {
       /*
        * The PIC passes us the vector for the IRQ as the value it sends
        * over the qemu_irq line
        */
       cpu->interrupt_vector = value;
   }
   at the top of the function.

It would also be nice somewhere to have a comment documenting
that this is the semantics the CPU expects for its incoming
IRQ line. Unless anybody has a better place, then perhaps
in the part of target/cris/cpu.h that defines CRIS_CPU_IRQ.
(If the PIC followed the just-recently-invented qdev convention
of having a .h file with a comment defining the "QEMU interface"
to the device, as eg include/hw/misc/armsse-mhu.h, then that
comment would be a good place to note that its sysbus IRQ 0
has these value-is-the-vector semantics. But it doesn't.)

>  }
> @@ -164,7 +164,7 @@ static void etraxfs_pic_init(Object *obj)
>  }
>
>  static Property etraxfs_pic_properties[] = {
> -    DEFINE_PROP_PTR("interrupt_vector", struct etrax_pic, interrupt_vector),
> +    DEFINE_PROP_LINK("cpu", struct etrax_pic, cpu, TYPE_CRIS_CPU, CRISCPU *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -173,10 +173,6 @@ static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->props = etraxfs_pic_properties;
> -    /*
> -     * Note: pointer property "interrupt_vector" may remain null, thus
> -     * no need for dc->user_creatable = false;
> -     */
>  }

Incidentally this is a sysbus device, so it's not user
creatable anyway.

thanks
-- PMM

Re: [PATCH 07/14] RFC: mips/cps: fix setting saar property

[Aleksandar Markovic]

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

On Friday, October 18, 2019, Marc-André Lureau <marcandre.lureau@redhat.com>
wrote:

> There is no "saar" property. Note: I haven't been able to test this
> code. Help welcome.
>
> May fix commit 043715d1e0fbb3e3411be3f898c5b77b7f90327a ("target/mips:
> Update ITU to utilize SAARI and SAAR CP0 registers")
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/mips/cps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>
This was a part of larger body code that is still in process of upstreaming
(and testing). I'll try to take a closer look in next few days. But, for
4.2, only minor changes are acceptable (like this patch). I'll get back to
you when I get more info on all options. Thank you anyway!

Aleksandar



> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 1660f86908..c49868d5da 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -106,7 +106,7 @@ static void mips_cps_realize(DeviceState *dev, Error
> **errp)
>          object_property_set_bool(OBJECT(&s->itu), saar_present,
> "saar-present",
>                                   &err);
>          if (saar_present) {
> -            qdev_prop_set_ptr(DEVICE(&s->itu), "saar", (void
> *)&env->CP0_SAAR);
> +            s->itu.saar = &env->CP0_SAAR;
>          }
>          object_property_set_bool(OBJECT(&s->itu), true, "realized",
> &err);
>          if (err != NULL) {
> --
> 2.23.0.606.g08da6496b6
>
>
>

[-- Attachment #2: Type: text/html, Size: 2105 bytes --]

Re: [PATCH 06/14] leon3: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> "set_pil_in" and "set_pil_in" are used to set a callback, but have a
> single user and cannot be modified by the user.
>
> Simplify the code by calling directly into leon3_set_pil_in(), and use
> a "cpu" link property.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/intc/grlib_irqmp.c | 20 ++++++--------------
>  hw/sparc/leon3.c      |  7 +++----
>  target/sparc/cpu.h    |  1 +
>  3 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
> index bc78e1a14f..34b7e1b4e1 100644
> --- a/hw/intc/grlib_irqmp.c
> +++ b/hw/intc/grlib_irqmp.c
> @@ -58,10 +58,8 @@ typedef struct IRQMP {
>
>      MemoryRegion iomem;
>
> -    void *set_pil_in;
> -    void *set_pil_in_opaque;
> -
>      IRQMPState *state;
> +    SPARCCPU *cpu;
>  } IRQMP;
>
>  struct IRQMPState {
> @@ -82,7 +80,6 @@ static void grlib_irqmp_check_irqs(IRQMPState *state)
>      uint32_t      pend   = 0;
>      uint32_t      level0 = 0;
>      uint32_t      level1 = 0;
> -    set_pil_in_fn set_pil_in;
>
>      assert(state != NULL);
>      assert(state->parent != NULL);
> @@ -97,13 +94,11 @@ static void grlib_irqmp_check_irqs(IRQMPState *state)
>      trace_grlib_irqmp_check_irqs(state->pending, state->force[0],
>                                   state->mask[0], level1, level0);
>
> -    set_pil_in = (set_pil_in_fn)state->parent->set_pil_in;
> -
>      /* Trigger level1 interrupt first and level0 if there is no level1 */
>      if (level1 != 0) {
> -        set_pil_in(state->parent->set_pil_in_opaque, level1);
> +        leon3_set_pil_in(state->parent->cpu, level1);
>      } else {
> -        set_pil_in(state->parent->set_pil_in_opaque, level0);
> +        leon3_set_pil_in(state->parent->cpu, level0);
>      }
>  }
>
> @@ -348,14 +343,13 @@ static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
>      IRQMP *irqmp = GRLIB_IRQMP(dev);
>
>          /* Check parameters */
> -    if (irqmp->set_pil_in == NULL) {
> -        error_setg(errp, "set_pil_in cannot be NULL.");
> +    if (irqmp->cpu == NULL) {
> +        error_setg(errp, "cpu cannot be NULL.");
>      }
>  }

>  static Property grlib_irqmp_properties[] = {
> -    DEFINE_PROP_PTR("set_pil_in", IRQMP, set_pil_in),
> -    DEFINE_PROP_PTR("set_pil_in_opaque", IRQMP, set_pil_in_opaque),
> +    DEFINE_PROP_LINK("cpu", IRQMP, cpu, TYPE_SPARC_CPU, SPARCCPU *),
>      DEFINE_PROP_END_OF_LIST(),
>  };

This is using ptr properties to define a callback
mechanism where the device says "call the callback
function, passing it an opaque cookie and a
32-bit value". We already have a generic mechanism
for doing that, which is the qemu_irq. So we should
just use that instead of adding a link property between
the interrupt controller and the CPU.

(Bonus further cleanup: the code currently in
leon3_set_pil_in() should probably be part of the
SPARC CPU object itself, as a handler for an inbound
gpio line, as then you could just wire the qemu_irq
from the interrupt controller to the CPU. But you
can leave it as ad-hoc code in leon3.c for now.)

thanks
-- PMM

Re: [PATCH 05/14] dp8393x: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/mips/mips_jazz.c | 3 ++-
>  hw/net/dp8393x.c    | 7 +++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index 8d010a0b6e..878925a963 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -284,7 +284,8 @@ static void mips_jazz_init(MachineState *machine,
>              dev = qdev_create(NULL, "dp8393x");
>              qdev_set_nic_properties(dev, nd);
>              qdev_prop_set_uint8(dev, "it_shift", 2);
> -            qdev_prop_set_ptr(dev, "dma_mr", rc4030_dma_mr);
> +            object_property_set_link(OBJECT(dev), OBJECT(rc4030_dma_mr),
> +                                     "dma_mr", &error_abort);
>              qdev_init_nofail(dev);
>              sysbus = SYS_BUS_DEVICE(dev);
>              sysbus_mmio_map(sysbus, 0, 0x80001000);
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a5678e11fa..946c7a8f64 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -173,7 +173,7 @@ typedef struct dp8393xState {
>      int loopback_packet;
>
>      /* Memory access */
> -    void *dma_mr;
> +    MemoryRegion *dma_mr;
>      AddressSpace as;
>  } dp8393xState;
>
> @@ -922,7 +922,8 @@ static const VMStateDescription vmstate_dp8393x = {
>
>  static Property dp8393x_properties[] = {
>      DEFINE_NIC_PROPERTIES(dp8393xState, conf),
> -    DEFINE_PROP_PTR("dma_mr", dp8393xState, dma_mr),
> +    DEFINE_PROP_LINK("dma_mr", dp8393xState, dma_mr,
> +                     TYPE_MEMORY_REGION, MemoryRegion *),
>      DEFINE_PROP_UINT8("it_shift", dp8393xState, it_shift, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Link property is the correct way to pass a MemoryRegion to
a device for DMA purposes, so this is a good cleanup.

> @@ -936,8 +937,6 @@ static void dp8393x_class_init(ObjectClass *klass, void *data)
>      dc->reset = dp8393x_reset;
>      dc->vmsd = &vmstate_dp8393x;
>      dc->props = dp8393x_properties;
> -    /* Reason: dma_mr property can't be set */
> -    dc->user_creatable = false;
>  }

Sidenote: as a sysbus device, this remains non-usercreatable
even though we can drop the specific flag here.


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

thanks
-- PMM

Re: [PATCH 05/14] dp8393x: replace PROP_PTR with PROP_LINK

[Aleksandar Markovic]

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

On Friday, October 18, 2019, Marc-André Lureau <marcandre.lureau@redhat.com>
wrote:

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/mips/mips_jazz.c | 3 ++-
>  hw/net/dp8393x.c    | 7 +++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
>
Marc-Andre,

Can you put together a paragraph on why we need this patch, and place it in
the commit message?

Thanks,

Aleksandar





> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index 8d010a0b6e..878925a963 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -284,7 +284,8 @@ static void mips_jazz_init(MachineState *machine,
>              dev = qdev_create(NULL, "dp8393x");
>              qdev_set_nic_properties(dev, nd);
>              qdev_prop_set_uint8(dev, "it_shift", 2);
> -            qdev_prop_set_ptr(dev, "dma_mr", rc4030_dma_mr);
> +            object_property_set_link(OBJECT(dev), OBJECT(rc4030_dma_mr),
> +                                     "dma_mr", &error_abort);
>              qdev_init_nofail(dev);
>              sysbus = SYS_BUS_DEVICE(dev);
>              sysbus_mmio_map(sysbus, 0, 0x80001000);
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index a5678e11fa..946c7a8f64 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -173,7 +173,7 @@ typedef struct dp8393xState {
>      int loopback_packet;
>
>      /* Memory access */
> -    void *dma_mr;
> +    MemoryRegion *dma_mr;
>      AddressSpace as;
>  } dp8393xState;
>
> @@ -922,7 +922,8 @@ static const VMStateDescription vmstate_dp8393x = {
>
>  static Property dp8393x_properties[] = {
>      DEFINE_NIC_PROPERTIES(dp8393xState, conf),
> -    DEFINE_PROP_PTR("dma_mr", dp8393xState, dma_mr),
> +    DEFINE_PROP_LINK("dma_mr", dp8393xState, dma_mr,
> +                     TYPE_MEMORY_REGION, MemoryRegion *),
>      DEFINE_PROP_UINT8("it_shift", dp8393xState, it_shift, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -936,8 +937,6 @@ static void dp8393x_class_init(ObjectClass *klass,
> void *data)
>      dc->reset = dp8393x_reset;
>      dc->vmsd = &vmstate_dp8393x;
>      dc->props = dp8393x_properties;
> -    /* Reason: dma_mr property can't be set */
> -    dc->user_creatable = false;
>  }
>
>  static const TypeInfo dp8393x_info = {
> --
> 2.23.0.606.g08da6496b6
>
>
>

[-- Attachment #2: Type: text/html, Size: 3203 bytes --]

Re: [PATCH 03/14] lance: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/dma/sparc32_dma.c | 2 +-
>  hw/net/lance.c       | 5 ++---
>  hw/net/pcnet-pci.c   | 2 +-
>  hw/net/pcnet.h       | 2 +-
>  4 files changed, 5 insertions(+), 6 deletions(-)

I feel like there's probably a way to be more type-safe here
(eg for the lance.c property we know the linked device must
be TYPE_SPARC32_LEDMA_DEVICE, not just TYPE_DEVICE), but I guess
this will do.

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

thanks
-- PMM

Re: [PATCH 02/14] vmmouse: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> While at it, use the expected type.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

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

thanks
-- PMM

Re: [PATCH 04/14] etraxfs: remove PROP_PTR usage

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> etraxfs_dma_client are not Object, so can't be exposed to user with
> QOM path. Let's remove property usage and move the constructor to the
> .c unit, simplifying some code on the way.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +/* Instantiate an ETRAXFS Ethernet MAC.  */
> +DeviceState *
> +etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
> +                 struct etraxfs_dma_client *dma_out,
> +                 struct etraxfs_dma_client *dma_in)
> +{
> +    DeviceState *dev;
> +    qemu_check_nic_model(nd, "fseth");
> +
> +    dev = qdev_create(NULL, "etraxfs-eth");
> +    qdev_set_nic_properties(dev, nd);
> +    qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
> +    ETRAX_FS_ETH(dev)->dma_out = dma_out;
> +    ETRAX_FS_ETH(dev)->dma_in = dma_in;
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +
> +    return dev;
> +}

I think the right way to do this in a QOM design would be
to define a QOM interface for "I am an etraxfs DMA client"
(which replaces the current 'struct etraxfs_dma_client'
ad-hoc interface), implement it on the ethernet device,
and then have QOM link properties on the DMA controller
device so that you can pass the interface implementations
to it.

If that seems like too much hassle right now, I guess we
can add a TODO comment here explaining what we ought to
be doing instead.

thanks
-- PMM

Re: [PATCH 09/14] smbus-eeprom: remove PROP_PTR

[Corey Minyard]

On Fri, Oct 18, 2019 at 06:20:40PM +0100, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Instead, set the initial data field directly. Since it is only 256
> > bytes, let's simply copy it to avoid invalid pointers issues.
> 
> (Commit message says you copy the 256 bytes, but the code
> doesn't seem to do any copying?)
> 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Ah, the smbus code. Corey had a go at cleaning this up
> a while back; I can't remember if we found a reason why
> we had to keep the weird use of a pointer property here, or
> if we just wanted to avoid dragging yet another thing into
> an already large patchset.
> 
> What we're actually trying to set up here is always 256
> bytes of data. What's the right way to pass a QOM device
> a small chunk of data like that?

Yeah, we discussed this at the time, and IIRC we decided to leave it
like it is until someone came up with a use case.  My guess is that
the intent was to be able to pass in a buffer from the command line
or something like that, but that was never completed.

I'm ok with this change, the code is strange as it is, and I don't see
any uses of this propery in a quick scan of the code.

-corey

> 
> > ---
> >  hw/i2c/smbus_eeprom.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> > index 54c86a0112..533c728b3b 100644
> > --- a/hw/i2c/smbus_eeprom.c
> > +++ b/hw/i2c/smbus_eeprom.c
> > @@ -44,7 +44,7 @@
> >  typedef struct SMBusEEPROMDevice {
> >      SMBusDevice smbusdev;
> >      uint8_t data[SMBUS_EEPROM_SIZE];
> > -    void *init_data;
> > +    uint8_t *init_data;
> >      uint8_t offset;
> >      bool accessed;
> >  } SMBusEEPROMDevice;
> > @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev)
> >
> >  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> >  {
> > +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> > +
> >      smbus_eeprom_reset(dev);
> > +    if (eeprom->init_data == NULL) {
> > +        error_setg(errp, "init_data cannot be NULL");
> > +    }
> >  }
> >
> > -static Property smbus_eeprom_properties[] = {
> > -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> > -    DEFINE_PROP_END_OF_LIST(),
> > -};
> > -
> >  static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >      dc->reset = smbus_eeprom_reset;
> >      sc->receive_byte = eeprom_receive_byte;
> >      sc->write_data = eeprom_write_data;
> > -    dc->props = smbus_eeprom_properties;
> >      dc->vmsd = &vmstate_smbus_eeprom;
> > -    /* Reason: pointer property "data" */
> > +    /* Reason: init_data */
> >      dc->user_creatable = false;
> >  }
> >
> > @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
> >
> >      dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
> >      qdev_prop_set_uint8(dev, "address", address);
> > -    qdev_prop_set_ptr(dev, "data", eeprom_buf);
> > +    SMBUS_EEPROM(dev)->init_data = eeprom_buf;
> >      qdev_init_nofail(dev);
> >  }
> >
> > --
> > 2.23.0.606.g08da6496b6
> 
> thanks
> -- PMM

Re: [PATCH 11/14] omap-i2c: remove PROP_PTR

[Corey Minyard]

On Fri, Oct 18, 2019 at 05:42:09PM +0200, Marc-André Lureau wrote:
> Since clock are not QOM objects, replace PROP_PTR of clocks with
> setters methods.

I agree with Peter's comment, a clock framework might be something
useful someday (not sure), a comment might be nice.

Other than that, looks good to me.

Reviewed-by: Corey Minyard <cminyard@mvista.com>

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/arm/omap1.c        |  2 +-
>  hw/arm/omap2.c        |  8 ++++----
>  hw/i2c/omap_i2c.c     | 19 ++++++++++++-------
>  include/hw/arm/omap.h |  9 +++++++++
>  4 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
> index c5583b10e0..fe55c44c7e 100644
> --- a/hw/arm/omap1.c
> +++ b/hw/arm/omap1.c
> @@ -4032,7 +4032,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
>  
>      s->i2c[0] = qdev_create(NULL, "omap_i2c");
>      qdev_prop_set_uint8(s->i2c[0], "revision", 0x11);
> -    qdev_prop_set_ptr(s->i2c[0], "fclk", omap_findclk(s, "mpuper_ck"));
> +    omap_i2c_set_fclk(OMAP_I2C(s->i2c[0]), omap_findclk(s, "mpuper_ck"));
>      qdev_init_nofail(s->i2c[0]);
>      busdev = SYS_BUS_DEVICE(s->i2c[0]);
>      sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(s->ih[1], OMAP_INT_I2C));
> diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
> index 726a628e64..046fb6ffb5 100644
> --- a/hw/arm/omap2.c
> +++ b/hw/arm/omap2.c
> @@ -2428,8 +2428,8 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
>  
>      s->i2c[0] = qdev_create(NULL, "omap_i2c");
>      qdev_prop_set_uint8(s->i2c[0], "revision", 0x34);
> -    qdev_prop_set_ptr(s->i2c[0], "iclk", omap_findclk(s, "i2c1.iclk"));
> -    qdev_prop_set_ptr(s->i2c[0], "fclk", omap_findclk(s, "i2c1.fclk"));
> +    omap_i2c_set_iclk(OMAP_I2C(s->i2c[0]), omap_findclk(s, "i2c1.iclk"));
> +    omap_i2c_set_fclk(OMAP_I2C(s->i2c[0]), omap_findclk(s, "i2c1.fclk"));
>      qdev_init_nofail(s->i2c[0]);
>      busdev = SYS_BUS_DEVICE(s->i2c[0]);
>      sysbus_connect_irq(busdev, 0,
> @@ -2440,8 +2440,8 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
>  
>      s->i2c[1] = qdev_create(NULL, "omap_i2c");
>      qdev_prop_set_uint8(s->i2c[1], "revision", 0x34);
> -    qdev_prop_set_ptr(s->i2c[1], "iclk", omap_findclk(s, "i2c2.iclk"));
> -    qdev_prop_set_ptr(s->i2c[1], "fclk", omap_findclk(s, "i2c2.fclk"));
> +    omap_i2c_set_iclk(OMAP_I2C(s->i2c[1]), omap_findclk(s, "i2c2.iclk"));
> +    omap_i2c_set_fclk(OMAP_I2C(s->i2c[1]), omap_findclk(s, "i2c2.fclk"));
>      qdev_init_nofail(s->i2c[1]);
>      busdev = SYS_BUS_DEVICE(s->i2c[1]);
>      sysbus_connect_irq(busdev, 0,
> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
> index 3ba965a58f..3ccbd5cc2c 100644
> --- a/hw/i2c/omap_i2c.c
> +++ b/hw/i2c/omap_i2c.c
> @@ -28,10 +28,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  
> -#define TYPE_OMAP_I2C "omap_i2c"
> -#define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
> -
> -typedef struct OMAPI2CState {
> +struct OMAPI2CState {
>      SysBusDevice parent_obj;
>  
>      MemoryRegion iomem;
> @@ -56,7 +53,7 @@ typedef struct OMAPI2CState {
>      uint8_t divider;
>      uint8_t times[2];
>      uint16_t test;
> -} OMAPI2CState;
> +};
>  
>  #define OMAP2_INTR_REV	0x34
>  #define OMAP2_GC_REV	0x34
> @@ -504,10 +501,18 @@ static void omap_i2c_realize(DeviceState *dev, Error **errp)
>      }
>  }
>  
> +void omap_i2c_set_iclk(OMAPI2CState *i2c, omap_clk clk)
> +{
> +    i2c->iclk = clk;
> +}
> +
> +void omap_i2c_set_fclk(OMAPI2CState *i2c, omap_clk clk)
> +{
> +    i2c->fclk = clk;
> +}
> +
>  static Property omap_i2c_properties[] = {
>      DEFINE_PROP_UINT8("revision", OMAPI2CState, revision, 0),
> -    DEFINE_PROP_PTR("iclk", OMAPI2CState, iclk),
> -    DEFINE_PROP_PTR("fclk", OMAPI2CState, fclk),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
> index 6b7897ba27..08ee0c7702 100644
> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -77,6 +77,15 @@ typedef struct omap_intr_handler_s omap_intr_handler;
>  void omap_intc_set_iclk(omap_intr_handler *intc, omap_clk clk);
>  void omap_intc_set_fclk(omap_intr_handler *intc, omap_clk clk);
>  
> +/* omap_i2c.c */
> +#define TYPE_OMAP_I2C "omap_i2c"
> +#define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
> +
> +typedef struct OMAPI2CState OMAPI2CState;
> +
> +void omap_i2c_set_iclk(OMAPI2CState *i2c, omap_clk clk);
> +void omap_i2c_set_fclk(OMAPI2CState *i2c, omap_clk clk);
> +
>  /* OMAP2 l4 Interconnect */
>  struct omap_l4_s;
>  struct omap_l4_region_s {
> -- 
> 2.23.0.606.g08da6496b6
> 

Re: [PATCH 09/14] smbus-eeprom: remove PROP_PTR

[Marc-André Lureau]

Hi

On Fri, Oct 18, 2019 at 5:43 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Instead, set the initial data field directly. Since it is only 256
> bytes, let's simply copy it to avoid invalid pointers issues.

Actually, the commit message is wrong. The patch used to introduce a
init_data[256] array, and copy initial data there, but I changed my
mind because of the risk to introduce regression as the original
buffer may have changed.

I will rephrase.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/i2c/smbus_eeprom.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 54c86a0112..533c728b3b 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -44,7 +44,7 @@
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
>      uint8_t data[SMBUS_EEPROM_SIZE];
> -    void *init_data;
> +    uint8_t *init_data;
>      uint8_t offset;
>      bool accessed;
>  } SMBusEEPROMDevice;
> @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev)
>
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
> +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> +
>      smbus_eeprom_reset(dev);
> +    if (eeprom->init_data == NULL) {
> +        error_setg(errp, "init_data cannot be NULL");
> +    }
>  }
>
> -static Property smbus_eeprom_properties[] = {
> -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      dc->reset = smbus_eeprom_reset;
>      sc->receive_byte = eeprom_receive_byte;
>      sc->write_data = eeprom_write_data;
> -    dc->props = smbus_eeprom_properties;
>      dc->vmsd = &vmstate_smbus_eeprom;
> -    /* Reason: pointer property "data" */
> +    /* Reason: init_data */
>      dc->user_creatable = false;
>  }
>
> @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>
>      dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
>      qdev_prop_set_uint8(dev, "address", address);
> -    qdev_prop_set_ptr(dev, "data", eeprom_buf);
> +    SMBUS_EEPROM(dev)->init_data = eeprom_buf;
>      qdev_init_nofail(dev);
>  }
>
> --
> 2.23.0.606.g08da6496b6
>