Re: [PATCH v6 13/30] hw/char/sh_serial: QOM-ify

[Philippe Mathieu-Daudé]

On 10/29/21 23:02, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/char/sh_serial.c | 98 +++++++++++++++++++++++++++------------------
>  hw/sh4/sh7750.c     | 56 +++++++++++++++++++-------
>  include/hw/sh4/sh.h |  9 +----
>  3 files changed, 101 insertions(+), 62 deletions(-)

> -void sh_serial_init(MemoryRegion *sysmem,
> -                    hwaddr base, int feat,
> -                    uint32_t freq, Chardev *chr,
> -                    qemu_irq eri_source,
> -                    qemu_irq rxi_source,
> -                    qemu_irq txi_source,
> -                    qemu_irq tei_source,
> -                    qemu_irq bri_source)
> +static void sh_serial_realize(DeviceState *d, Error **errp)
>  {
> -    SHSerialState *s = g_malloc0(sizeof(*s));
> -
> -    s->feat = feat;
> -    sh_serial_reset(s);
> -
> -    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
> -                          "serial", 0x100000000ULL);
> -
> -    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4", &s->iomem,
> -                             0, 0x28);
> -    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
> -
> -    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7", &s->iomem,
> -                             0, 0x28);
> -    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);
> -
> -    if (chr) {
> -        qemu_chr_fe_init(&s->chr, chr, &error_abort);
> +    SHSerialState *s = SH_SERIAL(d);
> +    MemoryRegion *iomem = g_malloc(sizeof(*iomem));
> +
> +    assert(d->id);

DeviceRealize() takes a Error* parameter... But well since this is
not user_creatable I suppose it is understandable enough.

> +    memory_region_init_io(iomem, OBJECT(d), &sh_serial_ops, s, d->id, 0x28);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(d), iomem);
> +    qdev_init_gpio_out_named(d, &s->eri, "eri", 1);
> +    qdev_init_gpio_out_named(d, &s->rxi, "rxi", 1);
> +    qdev_init_gpio_out_named(d, &s->txi, "txi", 1);
> +    qdev_init_gpio_out_named(d, &s->tei, "tei", 1);
> +    qdev_init_gpio_out_named(d, &s->bri, "bri", 1);
> +
> +    if (qemu_chr_fe_backend_connected(&s->chr)) {
>          qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
>                                   sh_serial_receive1,
>                                   sh_serial_event, NULL, s, NULL, true);
> @@ -435,9 +432,32 @@ void sh_serial_init(MemoryRegion *sysmem,
>      timer_init_ns(&s->fifo_timeout_timer, QEMU_CLOCK_VIRTUAL,
>                    sh_serial_timeout_int, s);
>      s->etu = NANOSECONDS_PER_SECOND / 9600;
> -    s->eri = eri_source;
> -    s->rxi = rxi_source;
> -    s->txi = txi_source;
> -    s->tei = tei_source;
> -    s->bri = bri_source;
> +}
> +
> +static void sh_serial_finalize(Object *obj)
> +{
> +    SHSerialState *s = SH_SERIAL(obj);
> +
> +    timer_del(&s->fifo_timeout_timer);
> +}
> +
> +static void sh_serial_init(Object *obj)
> +{
> +}
> +
> +static Property sh_serial_properties[] = {
> +    DEFINE_PROP_CHR("chardev", SHSerialState, chr),
> +    DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void sh_serial_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    device_class_set_props(dc, sh_serial_properties);
> +    dc->realize = sh_serial_realize;
> +    dc->reset = sh_serial_reset;
> +    /* Reason: part of SuperH CPU/SoC, needs to be wired up */
> +    dc->user_creatable = false;
>  }
> diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
> index 6c702d627c..7b21f1ce44 100644
> --- a/hw/sh4/sh7750.c
> +++ b/hw/sh4/sh7750.c
> @@ -24,9 +24,13 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
>  #include "hw/irq.h"
>  #include "hw/sh4/sh.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>  #include "sh7750_regs.h"
>  #include "sh7750_regnames.h"
>  #include "hw/sh4/sh_intc.h"
> @@ -762,6 +766,9 @@ static const MemoryRegionOps sh7750_mmct_ops = {
>  SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
>  {
>      SH7750State *s;
> +    DeviceState *dev;
> +    SysBusDevice *sb;
> +    MemoryRegion *mr, *alias;
>  
>      s = g_malloc0(sizeof(SH7750State));
>      s->cpu = cpu;
> @@ -807,21 +814,40 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion 
> *sysmem)
>  
>      cpu->env.intc_handle = &s->intc;
>  
> -    sh_serial_init(sysmem, 0x1fe00000,
> -                   0, s->periph_freq, serial_hd(0),
> -                   s->intc.irqs[SCI1_ERI],
> -                   s->intc.irqs[SCI1_RXI],
> -                   s->intc.irqs[SCI1_TXI],
> -                   s->intc.irqs[SCI1_TEI],
> -                   NULL);
> -    sh_serial_init(sysmem, 0x1fe80000,
> -                   SH_SERIAL_FEAT_SCIF,
> -                   s->periph_freq, serial_hd(1),
> -                   s->intc.irqs[SCIF_ERI],
> -                   s->intc.irqs[SCIF_RXI],
> -                   s->intc.irqs[SCIF_TXI],
> -                   NULL,
> -                   s->intc.irqs[SCIF_BRI]);
> +    /* SCI */
> +    dev = qdev_new(TYPE_SH_SERIAL);
> +    dev->id = (char *)"sci";

No, this is g_strdup() (freed in device_finalize()).

> +    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
> +    sb = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(sb, &error_fatal);
> +    sysbus_mmio_map(sb, 0, 0xffe00000);
> +    alias = g_malloc(sizeof(*alias));
> +    mr = sysbus_mmio_get_region(sb, 0);
> +    memory_region_init_alias(alias, OBJECT(dev), "sci-a7", mr,
> +                             0, memory_region_size(mr));
> +    memory_region_add_subregion(sysmem, A7ADDR(0xffe00000), alias);
> +    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCI1_ERI]);
> +    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCI1_RXI]);
> +    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCI1_TXI]);
> +    qdev_connect_gpio_out_named(dev, "tei", 0, s->intc.irqs[SCI1_TEI]);
> +
> +    /* SCIF */
> +    dev = qdev_new(TYPE_SH_SERIAL);
> +    dev->id =  (char *)"scif";

g_strdup()

If you agree I can fix when applying.