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

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
>

Hi Philippe,

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?

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

> +}
> +
> +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.

> +    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.

Regards,
Bin

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.

Hi Bin,

On 9/23/21 12:29, Philippe Mathieu-Daudé 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

>>> +    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,

So the bug is here, the serial is at 0x00 and the other register
stubs at 0x20. I can see u-boot console doing s/0x20/0x00/.

>>> +                
>>> 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.
> 

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

On Thu, 23 Sept 2021 at 11:29, 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:
> >> +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.

Yes, this is only needed for backwards-compatibility of incoming
migration data with old versions of QEMU which implemented migration
of devices with hand-rolled code. If a device didn't previously
handle migration at all then it should not now be setting the
legacy instance ID.

Speaking of migration, I notice that this QOM conversion does
not add a vmstate, which usually we do as part of the QOM conversion.
The device is also missing a reset method.

thanks
-- PMM

On 9/23/21 12:41, Peter Maydell wrote:
> On Thu, 23 Sept 2021 at 11:29, 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:
>>>> +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.
> 
> Yes, this is only needed for backwards-compatibility of incoming
> migration data with old versions of QEMU which implemented migration
> of devices with hand-rolled code. If a device didn't previously
> handle migration at all then it should not now be setting the
> legacy instance ID.

Thanks. I'll try to add that in the documentation somewhere.

> Speaking of migration, I notice that this QOM conversion does
> not add a vmstate, which usually we do as part of the QOM conversion.
> The device is also missing a reset method.

OK, I'll add that in a previous patch.

Thanks,

Phil.

On 9/23/21 12:52, Philippe Mathieu-Daudé wrote:
> On 9/23/21 12:41, Peter Maydell wrote:
>> On Thu, 23 Sept 2021 at 11:29, 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:
>>>>> +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.
>>
>> Yes, this is only needed for backwards-compatibility of incoming
>> migration data with old versions of QEMU which implemented migration
>> of devices with hand-rolled code. If a device didn't previously
>> handle migration at all then it should not now be setting the
>> legacy instance ID.
> 
> Thanks. I'll try to add that in the documentation somewhere.
> 
>> Speaking of migration, I notice that this QOM conversion does
>> not add a vmstate, which usually we do as part of the QOM conversion.
>> The device is also missing a reset method.

Sigh, you reminded me of this series:
"hw: Mark devices with no migratable fields"
https://lore.kernel.org/qemu-devel/20210117192446.23753-19-f4bug@amsat.org/

> 
> OK, I'll add that in a previous patch.
> 
> Thanks,
> 
> Phil.
>