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

[Peter Maydell <peter.maydell@linaro.org>]

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