| Message ID | 20191018154212.13458-5-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: remove QDEV_PROP_PTR | expand |
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
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 >
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
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
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,
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(-)