Hi Philippe,
On Thu, Sep 23, 2021 at 6:29 PM Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
>
> On 9/23/21 07:16, Bin Meng wrote:> On Sun, Sep 19, 2021 at 2:07 AM
> Philippe Mathieu-Daudé <f4...@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é <f4...@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
