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

[Bin Meng]

Hi Philippe,

On Thu, Sep 23, 2021 at 6:29 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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...
>

These are really helpful. Thanks a lot!

Do you think if we find a place in docs/develop to document such best
practice (qom.rst)?

> > 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 :)
>

Regards,
Bin