Am Mon, 28 Oct 2024 22:28:58 +0000
schrieb Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>:
> On 28/10/2024 16:39, Thomas Huth wrote:
> > Am Wed, 23 Oct 2024 09:58:29 +0100
> > schrieb Mark Cave-Ayland <mark.cave-ayl...@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-ayl...@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...
Ok, fair point, then let's keep this patch as it is!
Thomas
