| Message ID | 20230203113650.78146-7-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Use QOM alias properties and few QOM/QDev cleanups | expand |
On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: > No need to use an intermediate 'dma-offset' property in the > chipset object. Alias the property, so when the machine (here > r2d-plus) sets the value on the chipset, it is propagated to > the OHCI object. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/display/sm501.c | 22 +++++++++++----------- > hw/sh4/r2d.c | 2 +- > 2 files changed, 12 insertions(+), 12 deletions(-) It does not seem to be any simpler by the number of lines but maybe a bit cleaner. I wonder if it would worth renaming the property to dma-offset to match that of ohci so it's less confusing what it refers to. It's only used by r2d and this patch already changing that so would be an easy change. Regards, BALATON Zoltan > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index 52e42585af..49a648e952 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -28,6 +28,7 @@ > #include "qapi/error.h" > #include "qemu/log.h" > #include "qemu/module.h" > +#include "hw/usb/hcd-ohci.h" > #include "hw/char/serial.h" > #include "ui/console.h" > #include "hw/sysbus.h" > @@ -1942,7 +1943,7 @@ struct SM501SysBusState { > /*< public >*/ > SM501State state; > uint32_t vram_size; > - uint32_t base; > + OHCISysBusState ohci; > SerialMM serial; > }; > > @@ -1950,7 +1951,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) > { > SM501SysBusState *s = SYSBUS_SM501(dev); > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > - DeviceState *usb_dev; > MemoryRegion *mr; > > sm501_init(&s->state, dev, s->vram_size); > @@ -1963,13 +1963,10 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) > sysbus_init_mmio(sbd, &s->state.mmio_region); > > /* bridge to usb host emulation module */ > - usb_dev = qdev_new("sysbus-ohci"); > - qdev_prop_set_uint32(usb_dev, "num-ports", 2); > - qdev_prop_set_uint64(usb_dev, "dma-offset", s->base); > - sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal); > + sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal); > memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST, > - sysbus_mmio_get_region(SYS_BUS_DEVICE(usb_dev), 0)); > - sysbus_pass_irq(sbd, SYS_BUS_DEVICE(usb_dev)); > + sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0)); > + sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->ohci)); > > /* bridge to serial emulation module */ > sysbus_realize(SYS_BUS_DEVICE(&s->serial), &error_fatal); > @@ -1980,7 +1977,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) > > static Property sm501_sysbus_properties[] = { > DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0), > - DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2016,15 +2012,19 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data) > static void sm501_sysbus_init(Object *o) > { > SM501SysBusState *sm501 = SYSBUS_SM501(o); > + OHCISysBusState *ohci = &sm501->ohci; > SerialMM *smm = &sm501->serial; > > + object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI); > + object_property_add_alias(o, "base", OBJECT(ohci), "dma-offset"); > + qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2); > + > object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM); > qdev_set_legacy_instance_id(DEVICE(smm), SM501_UART0, 2); > qdev_prop_set_uint8(DEVICE(smm), "regshift", 2); > qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN); > > - object_property_add_alias(o, "chardev", > - OBJECT(smm), "chardev"); > + object_property_add_alias(o, "chardev", OBJECT(smm), "chardev"); > } > > static const TypeInfo sm501_sysbus_info = { > diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c > index 39fc4f19d9..279724ffbb 100644 > --- a/hw/sh4/r2d.c > +++ b/hw/sh4/r2d.c > @@ -274,7 +274,7 @@ static void r2d_init(MachineState *machine) > dev = qdev_new("sysbus-sm501"); > busdev = SYS_BUS_DEVICE(dev); > qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE); > - qdev_prop_set_uint32(dev, "base", 0x10000000); > + qdev_prop_set_uint64(dev, "base", 0x10000000); > qdev_prop_set_chr(dev, "chardev", serial_hd(2)); > sysbus_realize_and_unref(busdev, &error_fatal); > sysbus_mmio_map(busdev, 0, 0x10000000); >
On 3/2/23 14:05, BALATON Zoltan wrote: > On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: >> No need to use an intermediate 'dma-offset' property in the >> chipset object. Alias the property, so when the machine (here >> r2d-plus) sets the value on the chipset, it is propagated to >> the OHCI object. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/display/sm501.c | 22 +++++++++++----------- >> hw/sh4/r2d.c | 2 +- >> 2 files changed, 12 insertions(+), 12 deletions(-) > > It does not seem to be any simpler by the number of lines but maybe a > bit cleaner. Well it also moves to the "Embed QOM objects" pattern which Peter prefers. Note this device doesn't implement unrealize(). > I wonder if it would worth renaming the property to > dma-offset to match that of ohci so it's less confusing what it refers > to. It's only used by r2d and this patch already changing that so would > be an easy change. We can't because TYPE_PCI_SM501 is user-creatable, so we need to go thru the whole deprecation process and we don't have any API to deprecate QOM properties yet. I'll add these comments to the description.
On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: > On 3/2/23 14:05, BALATON Zoltan wrote: >> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: >>> No need to use an intermediate 'dma-offset' property in the >>> chipset object. Alias the property, so when the machine (here >>> r2d-plus) sets the value on the chipset, it is propagated to >>> the OHCI object. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/display/sm501.c | 22 +++++++++++----------- >>> hw/sh4/r2d.c | 2 +- >>> 2 files changed, 12 insertions(+), 12 deletions(-) >> >> It does not seem to be any simpler by the number of lines but maybe a bit >> cleaner. > > Well it also moves to the "Embed QOM objects" pattern which Peter prefers. > Note this device doesn't implement unrealize(). True. Maybe worth mentioning in the commit message to make this more explicit. I saw it in the patch but did not think about that. >> I wonder if it would worth renaming the property to dma-offset to match >> that of ohci so it's less confusing what it refers to. It's only used by >> r2d and this patch already changing that so would be an easy change. > > We can't because TYPE_PCI_SM501 is user-creatable, so we need to > go thru the whole deprecation process and we don't have any API to > deprecate QOM properties yet. But the sm501 PCI device only creates the display part hence it has no base option only vram-size (see sm501_pci_properties) so only the sysbus version has this property. Is this still a problem in that case? Regards, BALATON Zoltan > I'll add these comments to the description.
On 3/2/23 14:50, BALATON Zoltan wrote: > On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: >> On 3/2/23 14:05, BALATON Zoltan wrote: >>> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: >>>> No need to use an intermediate 'dma-offset' property in the >>>> chipset object. Alias the property, so when the machine (here >>>> r2d-plus) sets the value on the chipset, it is propagated to >>>> the OHCI object. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> --- >>>> hw/display/sm501.c | 22 +++++++++++----------- >>>> hw/sh4/r2d.c | 2 +- >>>> 2 files changed, 12 insertions(+), 12 deletions(-) >>> >>> It does not seem to be any simpler by the number of lines but maybe a >>> bit cleaner. >> >> Well it also moves to the "Embed QOM objects" pattern which Peter >> prefers. >> Note this device doesn't implement unrealize(). > > True. Maybe worth mentioning in the commit message to make this more > explicit. I saw it in the patch but did not think about that. > >>> I wonder if it would worth renaming the property to dma-offset to >>> match that of ohci so it's less confusing what it refers to. It's >>> only used by r2d and this patch already changing that so would be an >>> easy change. >> >> We can't because TYPE_PCI_SM501 is user-creatable, so we need to >> go thru the whole deprecation process and we don't have any API to >> deprecate QOM properties yet. > > But the sm501 PCI device only creates the display part hence it has no > base option only vram-size (see sm501_pci_properties) so only the sysbus > version has this property. Is this still a problem in that case? Oh you are right, I misread the PCI/sysbus functions. Lucky me, thanks!
On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: > On 3/2/23 14:50, BALATON Zoltan wrote: >> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: >>> On 3/2/23 14:05, BALATON Zoltan wrote: >>>> On Fri, 3 Feb 2023, Philippe Mathieu-Daudé wrote: >>>>> No need to use an intermediate 'dma-offset' property in the >>>>> chipset object. Alias the property, so when the machine (here >>>>> r2d-plus) sets the value on the chipset, it is propagated to >>>>> the OHCI object. >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> hw/display/sm501.c | 22 +++++++++++----------- >>>>> hw/sh4/r2d.c | 2 +- >>>>> 2 files changed, 12 insertions(+), 12 deletions(-) >>>> >>>> It does not seem to be any simpler by the number of lines but maybe a bit >>>> cleaner. >>> >>> Well it also moves to the "Embed QOM objects" pattern which Peter prefers. >>> Note this device doesn't implement unrealize(). >> >> True. Maybe worth mentioning in the commit message to make this more >> explicit. I saw it in the patch but did not think about that. >> >>>> I wonder if it would worth renaming the property to dma-offset to match >>>> that of ohci so it's less confusing what it refers to. It's only used by >>>> r2d and this patch already changing that so would be an easy change. >>> >>> We can't because TYPE_PCI_SM501 is user-creatable, so we need to >>> go thru the whole deprecation process and we don't have any API to >>> deprecate QOM properties yet. >> >> But the sm501 PCI device only creates the display part hence it has no base >> option only vram-size (see sm501_pci_properties) so only the sysbus version >> has this property. Is this still a problem in that case? > > Oh you are right, I misread the PCI/sysbus functions. Lucky me, thanks! And I've just realized then we also don't need separate sm501_sysbus_properties[] and sm501_pci_properties[] so you can just keep one and call it sm501_properties. Regards, BALATON Zoltan
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index 52e42585af..49a648e952 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -28,6 +28,7 @@ #include "qapi/error.h" #include "qemu/log.h" #include "qemu/module.h" +#include "hw/usb/hcd-ohci.h" #include "hw/char/serial.h" #include "ui/console.h" #include "hw/sysbus.h" @@ -1942,7 +1943,7 @@ struct SM501SysBusState { /*< public >*/ SM501State state; uint32_t vram_size; - uint32_t base; + OHCISysBusState ohci; SerialMM serial; }; @@ -1950,7 +1951,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) { SM501SysBusState *s = SYSBUS_SM501(dev); SysBusDevice *sbd = SYS_BUS_DEVICE(dev); - DeviceState *usb_dev; MemoryRegion *mr; sm501_init(&s->state, dev, s->vram_size); @@ -1963,13 +1963,10 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) sysbus_init_mmio(sbd, &s->state.mmio_region); /* bridge to usb host emulation module */ - usb_dev = qdev_new("sysbus-ohci"); - qdev_prop_set_uint32(usb_dev, "num-ports", 2); - qdev_prop_set_uint64(usb_dev, "dma-offset", s->base); - sysbus_realize_and_unref(SYS_BUS_DEVICE(usb_dev), &error_fatal); + sysbus_realize_and_unref(SYS_BUS_DEVICE(&s->ohci), &error_fatal); memory_region_add_subregion(&s->state.mmio_region, SM501_USB_HOST, - sysbus_mmio_get_region(SYS_BUS_DEVICE(usb_dev), 0)); - sysbus_pass_irq(sbd, SYS_BUS_DEVICE(usb_dev)); + sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ohci), 0)); + sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->ohci)); /* bridge to serial emulation module */ sysbus_realize(SYS_BUS_DEVICE(&s->serial), &error_fatal); @@ -1980,7 +1977,6 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) static Property sm501_sysbus_properties[] = { DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0), - DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0), DEFINE_PROP_END_OF_LIST(), }; @@ -2016,15 +2012,19 @@ static void sm501_sysbus_class_init(ObjectClass *klass, void *data) static void sm501_sysbus_init(Object *o) { SM501SysBusState *sm501 = SYSBUS_SM501(o); + OHCISysBusState *ohci = &sm501->ohci; SerialMM *smm = &sm501->serial; + object_initialize_child(o, "ohci", ohci, TYPE_SYSBUS_OHCI); + object_property_add_alias(o, "base", OBJECT(ohci), "dma-offset"); + qdev_prop_set_uint32(DEVICE(ohci), "num-ports", 2); + object_initialize_child(o, "serial", smm, TYPE_SERIAL_MM); qdev_set_legacy_instance_id(DEVICE(smm), SM501_UART0, 2); qdev_prop_set_uint8(DEVICE(smm), "regshift", 2); qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN); - object_property_add_alias(o, "chardev", - OBJECT(smm), "chardev"); + object_property_add_alias(o, "chardev", OBJECT(smm), "chardev"); } static const TypeInfo sm501_sysbus_info = { diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c index 39fc4f19d9..279724ffbb 100644 --- a/hw/sh4/r2d.c +++ b/hw/sh4/r2d.c @@ -274,7 +274,7 @@ static void r2d_init(MachineState *machine) dev = qdev_new("sysbus-sm501"); busdev = SYS_BUS_DEVICE(dev); qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE); - qdev_prop_set_uint32(dev, "base", 0x10000000); + qdev_prop_set_uint64(dev, "base", 0x10000000); qdev_prop_set_chr(dev, "chardev", serial_hd(2)); sysbus_realize_and_unref(busdev, &error_fatal); sysbus_mmio_map(busdev, 0, 0x10000000);
No need to use an intermediate 'dma-offset' property in the chipset object. Alias the property, so when the machine (here r2d-plus) sets the value on the chipset, it is propagated to the OHCI object. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/display/sm501.c | 22 +++++++++++----------- hw/sh4/r2d.c | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-)