On 28/10/2024 16:39, Thomas Huth wrote:
> Am Wed, 23 Oct 2024 09:58:29 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>
>> Since the ESCC is part of the next-pc device, move the ESCC to be a QOM child
>> of the next-pc device.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/m68k/next-cube.c | 54 ++++++++++++++++++++++-----------------------
>> 1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 7f714640da..915dd80f6f 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -124,6 +124,8 @@ struct NeXTPC {
>> qemu_irq scsi_reset;
>> qemu_irq scsi_dma;
>>
>> + ESCCState escc;
>> +
>> NextRtc rtc;
>> };
>>
>> @@ -978,31 +980,6 @@ static const MemoryRegionOps next_floppy_ops = {
>> .endianness = DEVICE_BIG_ENDIAN,
>> };
>>
>> -static void next_escc_init(DeviceState *pcdev)
>> -{
>> - NeXTPC *next_pc = NEXT_PC(pcdev);
>> - DeviceState *dev;
>> - SysBusDevice *s;
>> -
>> - dev = qdev_new(TYPE_ESCC);
>> - qdev_prop_set_uint32(dev, "disabled", 0);
>> - qdev_prop_set_uint32(dev, "frequency", 9600 * 384);
>> - qdev_prop_set_uint32(dev, "it_shift", 0);
>> - qdev_prop_set_bit(dev, "bit_swap", true);
>> - qdev_prop_set_chr(dev, "chrB", serial_hd(1));
>> - qdev_prop_set_chr(dev, "chrA", serial_hd(0));
>> - qdev_prop_set_uint32(dev, "chnBtype", escc_serial);
>> - qdev_prop_set_uint32(dev, "chnAtype", escc_serial);
>> -
>> - s = SYS_BUS_DEVICE(dev);
>> - sysbus_realize_and_unref(s, &error_fatal);
>> - sysbus_connect_irq(s, 0, qdev_get_gpio_in(pcdev, NEXT_SCC_I));
>> - sysbus_connect_irq(s, 1, qdev_get_gpio_in(pcdev, NEXT_SCC_DMA_I));
>> -
>> - memory_region_add_subregion(&next_pc->scrmem, 0x18000,
>> - sysbus_mmio_get_region(s, 0));
>> -}
>> -
>> static void next_pc_reset(DeviceState *dev)
>> {
>> NeXTPC *s = NEXT_PC(dev);
>> @@ -1043,6 +1020,28 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>> /* Floppy */
>> memory_region_add_subregion(&s->scrmem, 0x14108,
>> &s->floppy_mem);
>> +
>> + /* ESCC */
>> + d = DEVICE(object_resolve_path_component(OBJECT(dev), "escc"));
>> + qdev_prop_set_uint32(d, "disabled", 0);
>> + qdev_prop_set_uint32(d, "frequency", 9600 * 384);
>> + qdev_prop_set_uint32(d, "it_shift", 0);
>> + qdev_prop_set_bit(d, "bit_swap", true);
>> + qdev_prop_set_chr(d, "chrB", serial_hd(1));
>> + qdev_prop_set_chr(d, "chrA", serial_hd(0));
>> + qdev_prop_set_uint32(d, "chnBtype", escc_serial);
>> + qdev_prop_set_uint32(d, "chnAtype", escc_serial);
>> +
>> + sbd = SYS_BUS_DEVICE(d);
>> + if (!sysbus_realize(sbd, errp)) {
>> + return;
>> + }
>> + sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
>> + sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
>> +
>> + memory_region_add_subregion(&s->scrmem, 0x18000,
>> + sysbus_mmio_get_region(sbd, 0));
>
> You could also keep the next_escc_init() function, and call next_escc_init()
> here?
Normally a non-QOM _init() function suffix is used to both init() and realize() a
device, whereas here since the ESCC device is a child of the next-pc device these
operations must be separate. I think I can see why this convention is used elsewhere
in the codebase, as otherwise you end up calling a function with a _init() prefix
from _realize() which can get confusing with respect to the QOM model...
ATB,
Mark.
>> }
>>
>> static void next_pc_init(Object *obj)
>> @@ -1064,6 +1063,8 @@ static void next_pc_init(Object *obj)
>>
>> memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
>> "next.floppy", 4);
>> +
>> + object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
>> }
>>
>> /*
>> @@ -1201,9 +1202,6 @@ static void next_cube_init(MachineState *machine)
>> }
>> }
>>
>> - /* Serial */
>> - next_escc_init(pcdev);
>> -
>> /* TODO: */
>> /* Network */
>>
>