Re: [PATCH v3 13/33] serial: start making SerialMM a sysbus device

[Peter Maydell]

On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau
<address@hidden> wrote:
>
> Memory mapped serial device is in fact a sysbus device. The following
> patches will make use of sysbus facilities for resource and
> registration.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/char/omap_uart.c      |  2 +-
>  hw/char/serial.c         | 47 ++++++++++++++++++++++++++++------------
>  hw/mips/boston.c         |  2 +-
>  hw/mips/mips_malta.c     |  2 +-
>  include/hw/char/serial.h | 20 ++++++++++++-----
>  5 files changed, 51 insertions(+), 22 deletions(-)


> -SerialState *serial_mm_init(MemoryRegion *address_space,
> +SerialMM *serial_mm_init(MemoryRegion *address_space,
>                              hwaddr base, int it_shift,
>                              qemu_irq irq, int baudbase,
>                              Chardev *chr, enum device_endian end)
>  {
> -    DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
> -    SerialState *s = SERIAL(dev);
> +    SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
> +    SerialState *s = &self->serial;
>
> -    s->it_shift = it_shift;
> +    self->it_shift = it_shift;
>      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_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
> +    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
> +    qdev_prop_set_int32(DEVICE(s), "instance-id", base);
> +
> +    qdev_init_nofail(DEVICE(s));
> +    qdev_init_nofail(DEVICE(self));

Something odd is going on here. This is a convenience
wrapper around creating the SERIAL_MM device, so it's
correct that it has to init DEVICE(self). But it should
not be doing anything with the internals of 'self'.
It's the instance_init/realize of the SERIAL_MM object that should
instance_init/realize the 'self->serial' object. You have the
code below to do the 'instance_init' in the serial_mm_instance_init
function, but are missing the equivalent realize code.

> -    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
> +    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self,
>                            "serial", 8 << it_shift);
>      memory_region_add_subregion(address_space, base, &s->io);
> -    return s;
> +
> +    return self;
> +}
> +
> +static void serial_mm_instance_init(Object *o)
> +{
> +    SerialMM *self = SERIAL_MM(o);

'self' is not idiomatic for the name of the variable containing
the pointer to the object in QOM code ("git grep '\Wself\W' hw"
shows no uses of it at all, which is quite unusual for us --
usually the codebase has at least a few uses of any non-standard
way of writing something ;-))

Usually we use something approximating to the abbreviation
of the type name, so here 'smm' would do.

> +
> +    object_initialize_child(o, "serial", &self->serial, sizeof(self->serial),
> +                            TYPE_SERIAL, &error_abort, NULL);
>  }

thanks
-- PMM