TYPE_TOSA_MISC_GPIO doesn't need to be a SysBus device,
make it a plain QDev.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because having to pass MachineState and call
object_property_add_child() simply makes things more
complex... but it seems to cleaner QOM design.
Cc: Markus Armbruster <armbru@redhat.com>
---
hw/arm/tosa.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index f23651fd775..524d5fcd10b 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -79,7 +79,7 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO)
typedef struct TosaMiscGPIOState {
- SysBusDevice parent_obj;
+ DeviceState parent_obj;
} TosaMiscGPIOState;
static void tosa_reset(void *opaque, int line, int level)
@@ -96,7 +96,7 @@ static void tosa_misc_gpio_init(Object *obj)
qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
}
-static void tosa_gpio_setup(PXA2xxState *cpu,
+static void tosa_gpio_setup(MachineState *machine, PXA2xxState *cpu,
DeviceState *scp0,
DeviceState *scp1,
TC6393xbState *tmio)
@@ -104,7 +104,10 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
DeviceState *misc_gpio;
LEDState *led[4];
- misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
+ misc_gpio = qdev_new(TYPE_TOSA_MISC_GPIO);
+ object_property_add_child(OBJECT(machine), "pcb-container",
+ OBJECT(misc_gpio));
+ qdev_realize_and_unref(misc_gpio, NULL, &error_fatal);
/* MMC/SD host */
pxa2xx_mmci_handlers(cpu->mmc,
@@ -253,7 +256,7 @@ static void tosa_init(MachineState *machine)
scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
- tosa_gpio_setup(mpu, scp0, scp1, tmio);
+ tosa_gpio_setup(machine, mpu, scp0, scp1, tmio);
tosa_microdrive_attach(mpu);
@@ -307,7 +310,7 @@ static const TypeInfo tosa_ssp_info = {
static const TypeInfo tosa_misc_gpio_info = {
.name = TYPE_TOSA_MISC_GPIO,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_DEVICE,
.instance_size = sizeof(TosaMiscGPIOState),
.instance_init = tosa_misc_gpio_init,
/*
--
2.26.2
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> TYPE_TOSA_MISC_GPIO doesn't need to be a SysBus device,
> make it a plain QDev.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because having to pass MachineState and call
> object_property_add_child() simply makes things more
> complex... but it seems to cleaner QOM design.
Well, what devices really *need* to be sysbus devices?
The question is trivial for "real" buses, such as PCI, USB, and so
forth: a device is a FOO device when it plugs into a FOO bus.
Sysbus is quite unlike these "real" buses. It exists because qdev
initially *required* qdevs to plug into a qbus, so we made up a qbus for
the devices that don't plug into any of our "real" buses[1].
I figure all sysbus devices could be coded as bus-less devices today.
So the answer to "what devices really *need* to be sysbus devices?" is
"none".
I think a more useful question is what devices *should* be coded as
sysbus devices vs. bus-less devices.
Sysbus is more than just a dummy qbus. It's a software interface that
provides useful stuff. To use it, the device needs to be a
SysBusDevice. This leads to a partial answer: if the device profits
from stuff we provide only to SysBusDevices, it should be one.
Perhaps the useful stuff could be separated from SysBusDevice. Then
this partial answer evaporates.
There is just one instance of TYPE_SYSTEM_BUS[2]. This leads to another
partial answer: if the device can be part of another device, it should
not be a SysBusDevice.
Sysbus also enables "dynamic" sysbus devices. Shoehorning them into
SysBusDevice may have been a mistake.
> Cc: Markus Armbruster <armbru@redhat.com>
Cc: QOM maintainers for actual QOM expertise :)
[1] sysbus.h describes itself as "Devices attached directly to the main
system bus". I think that's an (unconscious?) attempt to rationalize
away its peculiar role.
[2] Exception: TYPE_MACIO_BUS (used by g3beige and mac99) is a subtype
of TYPE_SYSTEM_BUS.
> ---
> hw/arm/tosa.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index f23651fd775..524d5fcd10b 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -79,7 +79,7 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
> OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO)
>
> typedef struct TosaMiscGPIOState {
> - SysBusDevice parent_obj;
> + DeviceState parent_obj;
> } TosaMiscGPIOState;
>
> static void tosa_reset(void *opaque, int line, int level)
> @@ -96,7 +96,7 @@ static void tosa_misc_gpio_init(Object *obj)
> qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
> }
>
> -static void tosa_gpio_setup(PXA2xxState *cpu,
> +static void tosa_gpio_setup(MachineState *machine, PXA2xxState *cpu,
> DeviceState *scp0,
> DeviceState *scp1,
> TC6393xbState *tmio)
> @@ -104,7 +104,10 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
> DeviceState *misc_gpio;
> LEDState *led[4];
>
> - misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
> + misc_gpio = qdev_new(TYPE_TOSA_MISC_GPIO);
> + object_property_add_child(OBJECT(machine), "pcb-container",
> + OBJECT(misc_gpio));
> + qdev_realize_and_unref(misc_gpio, NULL, &error_fatal);
>
> /* MMC/SD host */
> pxa2xx_mmci_handlers(cpu->mmc,
> @@ -253,7 +256,7 @@ static void tosa_init(MachineState *machine)
> scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
> scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
>
> - tosa_gpio_setup(mpu, scp0, scp1, tmio);
> + tosa_gpio_setup(machine, mpu, scp0, scp1, tmio);
>
> tosa_microdrive_attach(mpu);
>
> @@ -307,7 +310,7 @@ static const TypeInfo tosa_ssp_info = {
>
> static const TypeInfo tosa_misc_gpio_info = {
> .name = TYPE_TOSA_MISC_GPIO,
> - .parent = TYPE_SYS_BUS_DEVICE,
> + .parent = TYPE_DEVICE,
> .instance_size = sizeof(TosaMiscGPIOState),
> .instance_init = tosa_misc_gpio_init,
> /*
On Tue, 8 Sep 2020 at 08:54, Markus Armbruster <armbru@redhat.com> wrote: > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > > > TYPE_TOSA_MISC_GPIO doesn't need to be a SysBus device, > > make it a plain QDev. > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > RFC because having to pass MachineState and call > > object_property_add_child() simply makes things more > > complex... but it seems to cleaner QOM design. > > Well, what devices really *need* to be sysbus devices? > > The question is trivial for "real" buses, such as PCI, USB, and so > forth: a device is a FOO device when it plugs into a FOO bus. > > Sysbus is quite unlike these "real" buses. It exists because qdev > initially *required* qdevs to plug into a qbus, so we made up a qbus for > the devices that don't plug into any of our "real" buses[1]. > > I figure all sysbus devices could be coded as bus-less devices today. > So the answer to "what devices really *need* to be sysbus devices?" is > "none". The major thing sysbus being a bus gives you is reset: devices on buses get reset automatically, but devices not on buses don't. So in this particular case I'm not in favour of this change -- right now the TOSA_MISC_GPIO device doesn't happen to need a reset, but having devices floating around in the system which can't have a reset method is a beartrap for our future selves. It's bad enough that we have this issue today with CPU objects: I don't want us to extend that to anything else if we can avoid it. > I think a more useful question is what devices *should* be coded as > sysbus devices vs. bus-less devices. > > Sysbus is more than just a dummy qbus. It's a software interface that > provides useful stuff. To use it, the device needs to be a > SysBusDevice. This leads to a partial answer: if the device profits > from stuff we provide only to SysBusDevices, it should be one. > > Perhaps the useful stuff could be separated from SysBusDevice. Then > this partial answer evaporates. This also is true -- some pretty generic useful stuff like "I can have MMIO regions" and "I get automatically reset" is implemented in sysbus, and some (like "I have gpio lines") for DeviceState. I would be happy to see this cleaned up. For the code we have at the moment I prefer to treat SysBusDevice as the preferred parent class for devices, ie don't directly inherit from DeviceState unless you really know what you're doing. The reset stuff in particular is desperately in need of a cleanup but it's a swamp, as usual. Currently we reset along the qbus tree (which is why non-bus-connected devices don't get their qdev reset method called, and must fend for themselves via qemu_register_reset()). These days I feel like "resetting a device should reset all its QOM children" would be a more natural way to model things (with some sort of "this device needs to override that default behaviour" for SoCs with more complicated reset handling) but getting there from here feels like it would be very painful. > There is just one instance of TYPE_SYSTEM_BUS[2]. This leads to another > partial answer: if the device can be part of another device, it should > not be a SysBusDevice. > > Sysbus also enables "dynamic" sysbus devices. Shoehorning them into > SysBusDevice may have been a mistake. I was never much of a fan of dynamic sysbus devices at all :-) thanks -- PMM
© 2016 - 2026 Red Hat, Inc.