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

[Philippe Mathieu-Daudé]

On 9/23/21 07:16, Bin Meng wrote:> On Sun, Sep 19, 2021 at 2: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
>
> Thanks for taking care of the updates!
>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   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");
>
> Do we have a common convention for what needs to be done in the
> instance_init() call and what in the realize() call?

No official convention IFAIK, but Peter & Markus described it in a
thread 2 years ago, IIRC it was about the TYPE_ARMV7M model.

See armv7m_instance_init() and armv7m_realize().

stellaris_init() does:

    nvic = qdev_new(TYPE_ARMV7M);
    qdev_connect_clock_in(nvic, "cpuclk",
                          qdev_get_clock_out(ssys_dev, "SYSCLK"));
(1) qdev_prop_set_uint32(nvic, "num-irq", NUM_IRQ_LINES);
(2) object_property_set_link(OBJECT(nvic), "memory",
                             OBJECT(get_system_memory()), &error_abort);
(3) sysbus_realize_and_unref(SYS_BUS_DEVICE(nvic), &error_fatal);

static void armv7m_instance_init(Object *obj)
{
    ...
    object_initialize_child(obj, "nvic", &s->nvic, TYPE_NVIC);
    object_property_add_alias(obj, "num-irq",
                              OBJECT(&s->nvic), "num-irq");

For (1) to set the "num-irq" property (aliased to TYPE_NVIC)
before realization (3), it has to be available (aliased) first,
thus has to be in the instance_init() handler.

When the child creation depends on a property which is set by
the parent, the property must be set before the realization,
then is available in the realize() handler. For example with (2):

static void armv7m_realize(DeviceState *dev, Error **errp)
{
    ...
    if (!s->board_memory) {
        error_setg(errp, "memory property was not set");
        return;
    }
    memory_region_add_subregion_overlap(&s->container, 0,
                                        s->board_memory, -1);

If your model only provides link/aliased properties, then it
requires a instance_init() handler. If no property is consumed
during realization, then to keep it simple you can avoid
implementing a realize() handler, having all setup in instance_init().

If your model only consumes properties (no link/alias), it must
implement a realize() handler, and similarly you can skip the
instance_init(), having all setup in realize().

Now instance_init() is always called, and can never fail.
The resources it allocates are freed later in instance_finalize().

realize() can however fails and return error. If it succeeds,
the resources are released in unrealize().

If you have to implement realize() and it might fail, then it is
cheaper to check the failing conditions first, then allocate
resources. So in that case we prefer to avoid instance_init(),
otherwise on failure we'd have allocated and released resources
we know we'll not use. Pointless.

Hope this is not too unclear and helps...

> For example, I see some devices put memory_region_init_io() and
> sysbus_init_mmio() in their realize().

Following my previous explanation, it is better (as cheaper) to
have them in realize() indeed :)

> > +}
> > +
> > +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);
>
> It looks like serial_mm_init() does one more thing:
>
>      qdev_set_legacy_instance_id(DEVICE(smm), base, 2);
>
> I am not sure what that is.

I'll defer on Paolo / Marc-André for that part, I think this is for
migrating legacy (x86?) machines, which is not the case.

> > +    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);
> >   }
>
> This patch unfortunately breaks the polarfire machine that no serial
> output is seen. I did not take a further look yet.

Doh, it passed the CI... Ah, now I see, this machine is not covered
by CI, only manual testing per 
docs/system/riscv/microchip-icicle-kit.rst... I'll have a look during 
the week-end.

Regards,

Phil.