Re: [PATCH] mac_via: fix init() and realize() behaviour

[Philippe Mathieu-Daudé]

On 10/13/20 6:26 PM, Mark Cave-Ayland wrote:
> The mac_via device does not currently follow the rules for init() and realize() 
> in
> regard to the mos6522 child devices. These child devices must be initialised 
> using
> object_initialize_child() within the mac_via init() function and then realized 
> as
> part of the mac_via realize() function. Move object_initialize_child() from
> realize() to init() which is where the iniitalisation of child devices should 
> occur.

Typo "initialisation".

> Similarly the realize() function creates alias properties to allow the VIA input
> and output IRQs to be wired up to the interrupt controller during machine init, 
> but
> realize() should never alter object properties. Remove these aliases and instead
> use object_resolve_path_component() to access the child objects from the mac_via
> device.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/q800.c    | 12 ++++++++----
>   hw/misc/mac_via.c | 36 ++++++++++++------------------------
>   2 files changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index ce4b47c3e3..773d75c1f8 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -174,6 +174,7 @@ static void q800_init(MachineState *machine)
>       SysBusESPState *sysbus_esp;
>       ESPState *esp;
>       SysBusDevice *sysbus;
> +    MOS6522State *ms;

As we often use 'ms' for MachineState* variable, maybe name
this 'mos' or 'via'?
Also you can use simply a Object*.

>       BusState *adb_bus;
>       NubusBus *nubus;
>       GLUEState *irq;
> @@ -226,9 +227,11 @@ static void q800_init(MachineState *machine)
>       sysbus = SYS_BUS_DEVICE(via_dev);
>       sysbus_realize_and_unref(sysbus, &error_fatal);
>       sysbus_mmio_map(sysbus, 0, VIA_BASE);
> -    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 0, pic[0]);
> -    qdev_connect_gpio_out_named(DEVICE(sysbus), "irq", 1, pic[1]);
>   
> +    ms = MOS6522(object_resolve_path_component(OBJECT(via_dev), "via1"));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(ms), 0, pic[0]);
> +    ms = MOS6522(object_resolve_path_component(OBJECT(via_dev), "via2"));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(ms), 0, pic[1]);
>   
>       adb_bus = qdev_get_child_bus(via_dev, "adb.0");
>       dev = qdev_new(TYPE_ADB_KEYBOARD);
> @@ -300,11 +303,12 @@ static void q800_init(MachineState *machine)
>   
>       sysbus = SYS_BUS_DEVICE(dev);
>       sysbus_realize_and_unref(sysbus, &error_fatal);
> -    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in_named(via_dev,
> +    ms = MOS6522(object_resolve_path_component(OBJECT(via_dev), "via2"));
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in_named(DEVICE(ms),
>                                                            "via2-irq",
>                                                            VIA2_IRQ_SCSI_BIT));
>       sysbus_connect_irq(sysbus, 1,
> -                       qdev_get_gpio_in_named(via_dev, "via2-irq",
> +                       qdev_get_gpio_in_named(DEVICE(ms), "via2-irq",
>                                                 VIA2_IRQ_SCSI_DATA_BIT));
>       sysbus_mmio_map(sysbus, 0, ESP_BASE);
>       sysbus_mmio_map(sysbus, 1, ESP_PDMA);
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 6db62dab7d..7c2c943d3f 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -1016,40 +1016,21 @@ static void mac_via_realize(DeviceState *dev, Error 
> **errp)
>       struct tm tm;
>       int ret;
>   
> -    /* Init VIAs 1 and 2 */
> -    object_initialize_child(OBJECT(dev), "via1", &m->mos6522_via1,
> -                            TYPE_MOS6522_Q800_VIA1);
> -
> -    object_initialize_child(OBJECT(dev), "via2", &m->mos6522_via2,
> -                            TYPE_MOS6522_Q800_VIA2);
> -
> -    /* Pass through mos6522 output IRQs */
> -    ms = MOS6522(&m->mos6522_via1);
> -    object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> -                              SYSBUS_DEVICE_GPIO_IRQ "[0]");
> -    ms = MOS6522(&m->mos6522_via2);
> -    object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> -                              SYSBUS_DEVICE_GPIO_IRQ "[0]");
> -
> +    /* Realize VIAs */
>       sysbus_realize(SYS_BUS_DEVICE(&m->mos6522_via1), &error_abort);
>       sysbus_realize(SYS_BUS_DEVICE(&m->mos6522_via2), &error_abort);
>   
> -    /* Pass through mos6522 input IRQs */
> -    qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq");
> -    qdev_pass_gpios(DEVICE(&m->mos6522_via2), dev, "via2-irq");
> -
>       /* VIA 1 */
> +    ms = MOS6522(&m->mos6522_via1);
>       m->mos6522_via1.one_second_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> -                                                     via1_one_second,
> -                                                     &m->mos6522_via1);
> -    m->mos6522_via1.VBL_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, via1_VBL,
> -                                              &m->mos6522_via1);
> +                                                     via1_one_second, ms);
> +    m->mos6522_via1.VBL_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, via1_VBL, ms);

Unrelated to your patch, but doesn't this belong
to mos6522_q800_via1_init()?

>   
>       qemu_get_timedate(&tm, 0);
>       m->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
>   
>       adb_register_autopoll_callback(adb_bus, adb_via_poll, m);
> -    m->adb_data_ready = qdev_get_gpio_in_named(dev, "via1-irq",
> +    m->adb_data_ready = qdev_get_gpio_in_named(DEVICE(ms), "via1-irq",
>                                                  VIA1_IRQ_ADB_READY_BIT);
>   
>       if (m->blk) {
> @@ -1080,6 +1061,13 @@ static void mac_via_init(Object *obj)
>       SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>       MacVIAState *m = MAC_VIA(obj);
>   
> +    /* Init VIAs 1 and 2 */
> +    object_initialize_child(obj, "via1", &m->mos6522_via1,
> +                            TYPE_MOS6522_Q800_VIA1);
> +
> +    object_initialize_child(obj, "via2", &m->mos6522_via2,
> +                            TYPE_MOS6522_Q800_VIA2);
> +
>       /* MMIO */
>       memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE);
>       sysbus_init_mmio(sbd, &m->mmio);