Re: [PATCH v3 17/33] serial: make SerialIO a sysbus device

[Peter Maydell]

On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau
<address@hidden> wrote:
>
> Make serial IO a proper sysbus device, similar to serial MM.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/char/serial.c         | 58 ++++++++++++++++++++++++++++++++--------
>  include/hw/char/serial.h | 13 +++++++--
>  2 files changed, 58 insertions(+), 13 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index a40bc3ab81..84de2740a7 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -986,22 +986,57 @@ const MemoryRegionOps serial_io_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
> -SerialState *serial_init(int base, qemu_irq irq, int baudbase,
> -                         Chardev *chr, MemoryRegion *system_io)
> +static void serial_io_realize(DeviceState *dev, Error **errp)
>  {
> -    DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
> -    SerialState *s = SERIAL(dev);
> +    SerialIO *self = SERIAL_IO(dev);

"sio" or something rather than "self".

> +    SerialState *s = &self->serial;
>
> -    s->irq = irq;
> -    qdev_prop_set_uint32(dev, "baudbase", baudbase);
> -    qdev_prop_set_chr(dev, "chardev", chr);
> -    qdev_prop_set_int32(dev, "instance-id", base);
> -    qdev_init_nofail(dev);
> +    qdev_init_nofail(DEVICE(s));
>
>      memory_region_init_io(&s->io, NULL, &serial_io_ops, s, "serial", 8);
> -    memory_region_add_subregion(system_io, base, &s->io);
> +    sysbus_init_irq(SYS_BUS_DEVICE(self), &self->serial.irq);

You could say '&s->irq' here, since you have the local variable.

> +}
> +
> +static void serial_io_class_init(ObjectClass *klass, void* data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = serial_io_realize;

For class methods where the class has no data that needs
to be migrated it's helpful to have a comment
  /* No dc->vmsd: class has no migratable state */
(which lets us know that it's intentional and not a forgotten
thing). Some day I will get round to writing a patch so you
can say "dc->vmsd = no_migratable_state;" ...

> +}
> +
> +static void serial_io_instance_init(Object *o)
> +{
> +    SerialIO *self = SERIAL_IO(o);
> +
> +    object_initialize_child(o, "serial", &self->serial, sizeof(self->serial),
> +                            TYPE_SERIAL, &error_abort, NULL);
> +
> +    qdev_alias_all_properties(DEVICE(&self->serial), o);
> +}
> +
> +
> +static const TypeInfo serial_io_info = {
> +    .name = TYPE_SERIAL_IO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SerialIO),
> +    .instance_init = serial_io_instance_init,
> +    .class_init = serial_io_class_init,
> +};
> +
> +SerialIO *serial_init(int base, qemu_irq irq, int baudbase,
> +                         Chardev *chr, MemoryRegion *system_io)
> +{
> +    SerialIO *self = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO));
>
> -    return s;
> +    qdev_prop_set_uint32(DEVICE(self), "baudbase", baudbase);
> +    qdev_prop_set_chr(DEVICE(self), "chardev", chr);
> +    qdev_prop_set_int32(DEVICE(self), "instance-id", base);
> +    qdev_init_nofail(DEVICE(self));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq);
> +    memory_region_add_subregion(system_io, base, &self->serial.io);
> +
> +    return self;
>  }

thanks
-- PMM