Re: [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev

[Markus Armbruster]

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,
>      /*