Re: [PATCH v5 12/37] serial: start making SerialMM a sysbus device

[Marc-André Lureau]

On Fri, Dec 20, 2019 at 5:57 PM 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. In particular, "serial-mm: use sysbus facilities" will
> move internal serial realization to serial_mm_realize callback to
> follow qdev best practices.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/char/omap_uart.c      |  2 +-
>  hw/char/serial.c         | 66 +++++++++++++++++++++++++++++++---------
>  hw/mips/boston.c         |  2 +-
>  hw/mips/mips_malta.c     |  2 +-
>  include/hw/char/serial.h | 20 +++++++++---
>  5 files changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c
> index 13e4f43c4c..e8da933378 100644
> --- a/hw/char/omap_uart.c
> +++ b/hw/char/omap_uart.c
> @@ -27,7 +27,7 @@
>  struct omap_uart_s {
>      MemoryRegion iomem;
>      hwaddr base;
> -    SerialState *serial; /* TODO */
> +    SerialMM *serial; /* TODO */
>      struct omap_target_agent_s *ta;
>      omap_clk fclk;
>      qemu_irq irq;
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index ec388f3876..905b84c25f 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1032,16 +1032,28 @@ static const TypeInfo serial_info = {
>  static uint64_t serial_mm_read(void *opaque, hwaddr addr,
>                                 unsigned size)
>  {
> -    SerialState *s = opaque;
> -    return serial_ioport_read(s, addr >> s->it_shift, 1);
> +    SerialMM *s = SERIAL_MM(opaque);
> +    return serial_ioport_read(&s->serial, addr >> s->it_shift, 1);
>  }
>
>  static void serial_mm_write(void *opaque, hwaddr addr,
>                              uint64_t value, unsigned size)
>  {
> -    SerialState *s = opaque;
> +    SerialMM *s = SERIAL_MM(opaque);
>      value &= 255;
> -    serial_ioport_write(s, addr >> s->it_shift, value, 1);
> +    serial_ioport_write(&s->serial, addr >> s->it_shift, value, 1);
> +}
> +
> +static void serial_mm_realize(DeviceState *dev, Error **errp)
> +{
> +    SerialMM *s = SERIAL_MM(dev);
> +    Error *local_err = NULL;
> +
> +    object_property_set_bool(OBJECT(&s->serial), true, "realized", 
> &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>
>  static const MemoryRegionOps serial_mm_ops[3] = {
> @@ -1068,30 +1080,56 @@ static const MemoryRegionOps serial_mm_ops[3] = {
>      },
>  };
>
> -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 *smm = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
> +    SerialState *s = &smm->serial;
>
> -    s->it_shift = it_shift;
> +    smm->it_shift = it_shift;
>      s->irq = irq;
> -    qdev_prop_set_uint32(dev, "baudbase", baudbase);
> -    qdev_prop_set_chr(dev, "chardev", chr);
> -    qdev_set_legacy_instance_id(dev, base, 2);
> -    qdev_init_nofail(dev);
> +    qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
> +    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
> +    qdev_set_legacy_instance_id(DEVICE(s), base, 2);
>
> -    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
> +    qdev_init_nofail(DEVICE(smm));
> +
> +    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], smm,
>                            "serial", 8 << it_shift);
>      memory_region_add_subregion(address_space, base, &s->io);
> -    return s;
> +
> +    return smm;
> +}
> +
> +static void serial_mm_instance_init(Object *o)
> +{
> +    SerialMM *smm = SERIAL_MM(o);
> +
> +    object_initialize_child(o, "serial", &smm->serial, sizeof(smm->serial),
> +                            TYPE_SERIAL, &error_abort, NULL);
>  }
>
> +static void serial_mm_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = serial_mm_realize;
> +}
> +
> +static const TypeInfo serial_mm_info = {
> +    .name = TYPE_SERIAL_MM,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .class_init = serial_mm_class_init,
> +    .instance_init = serial_mm_instance_init,
> +    .instance_size = sizeof(SerialMM),
> +};
> +
>  static void serial_register_types(void)
>  {
>      type_register_static(&serial_info);
> +    type_register_static(&serial_mm_info);
>  }
>
>  type_init(serial_register_types)
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index ca7d813a52..23fdd5ec6a 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -50,7 +50,7 @@ typedef struct {
>
>      MachineState *mach;
>      MIPSCPSState cps;
> -    SerialState *uart;
> +    SerialMM *uart;
>
>      CharBackend lcd_display;
>      char lcd_content[8];
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 783cd99848..ea92e5e27d 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -83,7 +83,7 @@ typedef struct {
>      uint32_t i2csel;
>      CharBackend display;
>      char display_text[9];
> -    SerialState *uart;
> +    SerialMM *uart;
>      bool display_inited;
>  } MaltaFPGAState;
>
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 548944b06a..730165347c 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -57,7 +57,6 @@ typedef struct SerialState {
>      qemu_irq irq;
>      CharBackend chr;
>      int last_break_enable;
> -    int it_shift;
>      uint32_t baudbase;
>      uint32_t tsr_retry;
>      guint watch_tag;
> @@ -80,6 +79,14 @@ typedef struct SerialState {
>      MemoryRegion io;
>  } SerialState;
>
> +typedef struct SerialMM {
> +    SysBusDevice parent;
> +
> +    SerialState serial;
> +
> +    int it_shift;
> +} SerialMM;
> +
>  extern const VMStateDescription vmstate_serial;
>  extern const MemoryRegionOps serial_io_ops;
>
> @@ -88,12 +95,15 @@ void serial_set_frequency(SerialState *s, uint32_t 
> frequency);
>  #define TYPE_SERIAL "serial"
>  #define SERIAL(s) OBJECT_CHECK(SerialState, (s), TYPE_SERIAL)
>
> +#define TYPE_SERIAL_MM "serial-mm"
> +#define SERIAL_MM(s) OBJECT_CHECK(SerialMM, (s), TYPE_SERIAL_MM)
> +
>  SerialState *serial_init(int base, qemu_irq irq, int baudbase,
>                           Chardev *chr, MemoryRegion *system_io);
> -SerialState *serial_mm_init(MemoryRegion *address_space,
> -                            hwaddr base, int it_shift,
> -                            qemu_irq irq, int baudbase,
> -                            Chardev *chr, enum device_endian end);
> +SerialMM *serial_mm_init(MemoryRegion *address_space,
> +                         hwaddr base, int it_shift,
> +                         qemu_irq irq, int baudbase,
> +                         Chardev *chr, enum device_endian end);
>
>  /* serial-isa.c */
>
> --
> 2.24.0.308.g228f53135a
>
>

ping, thanks

-- 
Marc-André Lureau