| Message ID | 20241023085852.1061031-14-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | next-cube: more tidy-ups and improvements | expand |
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? Thomas > } > > 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 */ >
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 */ >> >
Am Mon, 28 Oct 2024 22:28:58 +0000 schrieb Mark Cave-Ayland <mark.cave-ayland@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-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... Ok, fair point, then let's keep this patch as it is! Thomas
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)); + } 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 */
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(-)