Re: [PATCH] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Alistair Francis <alistair23@gmail.com>]

On Sun, Sep 19, 2021 at 4:07 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> - Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
> - Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
> - Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
> - Keep mchp_pfsoc_mmuart_create() behavior
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h | 16 ++++--
>  hw/char/mchp_pfsoc_mmuart.c         | 77 +++++++++++++++++++++++------
>  2 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index f61990215f0..b484b7ea5e4 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -28,16 +28,22 @@
>  #ifndef HW_MCHP_PFSOC_MMUART_H
>  #define HW_MCHP_PFSOC_MMUART_H
>
> +#include "hw/sysbus.h"
>  #include "hw/char/serial.h"
>
>  #define MCHP_PFSOC_MMUART_REG_SIZE  52
>
> -typedef struct MchpPfSoCMMUartState {
> -    MemoryRegion iomem;
> -    hwaddr base;
> -    qemu_irq irq;
> +#define TYPE_MCHP_PFSOC_UART "mchp.pfsoc.uart"
> +OBJECT_DECLARE_SIMPLE_TYPE(MchpPfSoCMMUartState, MCHP_PFSOC_UART)
>
> -    SerialMM *serial;
> +typedef struct MchpPfSoCMMUartState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion iomem;
> +
> +    SerialMM serial_mm;
>
>      uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
>  } MchpPfSoCMMUartState;
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 2facf85c2d8..74404e047d4 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -22,8 +22,9 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> -#include "chardev/char.h"
> +#include "qapi/error.h"
>  #include "hw/char/mchp_pfsoc_mmuart.h"
> +#include "hw/qdev-properties.h"
>
>  static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
> @@ -63,23 +64,69 @@ static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>      },
>  };
>
> -MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
> -    hwaddr base, qemu_irq irq, Chardev *chr)
> +static void mchp_pfsoc_mmuart_init(Object *obj)
>  {
> -    MchpPfSoCMMUartState *s;
> -
> -    s = g_new0(MchpPfSoCMMUartState, 1);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(obj);
>
>      memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
>                            "mchp.pfsoc.mmuart", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
>
> -    s->base = base;
> -    s->irq = irq;
> -
> -    s->serial = serial_mm_init(sysmem, base, 2, irq, 399193, chr,
> -                               DEVICE_LITTLE_ENDIAN);
> -
> -    memory_region_add_subregion(sysmem, base + 0x20, &s->iomem);
> -
> -    return s;
> +    object_initialize_child(obj, "serial-mm", &s->serial_mm, TYPE_SERIAL_MM);
> +    object_property_add_alias(obj, "chardev", OBJECT(&s->serial_mm), "chardev");
> +}
> +
> +static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
> +{
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
> +
> +    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "regshift", 2);
> +    qdev_prop_set_uint32(DEVICE(&s->serial_mm), "baudbase", 399193);
> +    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "endianness",
> +                        DEVICE_LITTLE_ENDIAN);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->serial_mm), errp)) {
> +        return;
> +    }
> +    sysbus_pass_irq(SYS_BUS_DEVICE(dev), SYS_BUS_DEVICE(&s->serial_mm));
> +    memory_region_add_subregion(&s->iomem, 0x20,
> +                sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->serial_mm), 0));
> +}
> +
> +static void mchp_pfsoc_mmuart_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = mchp_pfsoc_mmuart_realize;
> +}
> +
> +static const TypeInfo mchp_pfsoc_mmuart_info = {
> +    .name          = TYPE_MCHP_PFSOC_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MchpPfSoCMMUartState),
> +    .instance_init = mchp_pfsoc_mmuart_init,
> +    .class_init    = mchp_pfsoc_mmuart_class_init,
> +};
> +
> +static void mchp_pfsoc_mmuart_register_types(void)
> +{
> +    type_register_static(&mchp_pfsoc_mmuart_info);
> +}
> +
> +type_init(mchp_pfsoc_mmuart_register_types)
> +
> +MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
> +                                               hwaddr base,
> +                                               qemu_irq irq, Chardev *chr)
> +{
> +    DeviceState *dev = qdev_new(TYPE_MCHP_PFSOC_UART);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +    sysbus_realize(sbd, &error_fatal);
> +
> +    memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(sbd, 0));
> +    sysbus_connect_irq(sbd, 0, irq);
> +
> +    return MCHP_PFSOC_UART(dev);
>  }
> --
> 2.31.1
>