diff mbox series

[6/9] hw/display/sm501: QOM-alias 'dma-offset' property in chipset object

Message ID 20230203113650.78146-7-philmd@linaro.org
State New
Headers show
Series hw: Use QOM alias properties and few QOM/QDev cleanups | expand

Commit Message

Philippe Mathieu-Daudé Feb. 3, 2023, 11:36 a.m. UTC
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(-)

Comments

BALATON Zoltan Feb. 3, 2023, 1:05 p.m. UTC | #1
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);
>
Philippe Mathieu-Daudé Feb. 3, 2023, 1:21 p.m. UTC | #2
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.
BALATON Zoltan Feb. 3, 2023, 1:50 p.m. UTC | #3
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.
Philippe Mathieu-Daudé Feb. 3, 2023, 2:11 p.m. UTC | #4
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!
BALATON Zoltan Feb. 3, 2023, 2:12 p.m. UTC | #5
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 mbox series

Patch

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);