[PATCH v2 00/34] next-cube: more tidy-ups and improvements

[PATCH v2 00/34] next-cube: more tidy-ups and improvements

[Mark Cave-Ayland]

This series contains a number of tidy-ups and improvements to the NeXTCube machine
which include:

  - Bringing the code up-to-date with our latest coding standards/APIs, in particular
    related to the board configuration and IRQ wiring

  - Remove the remaining overlapping memory regions and consolidating multiple
    register implementations into a single place

  - Add a new next-scsi device containing the ESP device and its associated
    CSRs

  - Adding the empty_slot device to fill unimplemented devices and removing
    the "catch-all" next.scr memory region

  - QOMifying the next-rtc device and wiring it up with gpios as required

The next-cube machine looks in fairly good shape now, the main remaining work is to
create a separate device for the DMA controller and update the wiring of the IRQs
(including to the CPU) accordingly.

There is no change to the behaviour of the next-cube machine with this series in
that the next-cube machine with a suitable ROM image can now load the kernel from
a pre-installed NeXTStep image and start executing it.

Note that due to the device model changes this is a migration break, however since
the next-cube machine is currently unable to boot anything useful, I don't see
this as an issue.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

[Patches still needing review: 3, 7, 8, 11, 17, 21, 25, 28, 29, 30, 31, 34]

v2:
- Rebase onto master
- Drop patches 1 and 4 (these have been merged separately as bugfixes)
- Add R-B tags from Thomas and Phil
- Update commit message in patch 2
- Add comments containing system register addresses as requested by Thomas in
  patch 2
- Update description for patch 7 explaining that the SCSI CSRs are also now
  modelled as standard registers (similar to Previous)
- Update description for patch 8 explaining why the previously hardcoded value
  has been dropped
- Add comments suggesting unimplemented devices in patch 14
- Remove next_rtc_vmstate from next_pc_vmstate in patch 25
- Fix cut/paste error in patch 30 headline


Mark Cave-Ayland (34):
  next-cube: remove 0x14020 dummy value from next_mmio_read()
  next-cube: remove overlap between next.dma and next.mmio memory
    regions
  next-cube: create new next.scsi container memory region
  next-cube: move next_scsi_init() to next_pc_realize()
  next-cube: introduce next_pc_init() object init function
  next-cube: introduce next-scsi device
  next-cube: move SCSI CSRs from next-pc to the next-scsi device
  next-cube: move SCSI 4020/4021 logic from next-pc device to next-scsi
    device
  next-cube: move floppy disk MMIO to separate memory region in next-pc
  next-cube: map ESCC registers as a subregion of the next.scr memory
    region
  next-cube: move ESCC to be QOM child of next-pc device
  next-cube: move timer MMIO to separate memory region on next-pc device
  next-cube: move en ethernet MMIO to separate memory region on next-pc
    device
  next-cube: add empty slots for unknown accesses to next.scr memory
    region
  next-cube: remove unused next.scr memory region
  next-cube: rearrange NeXTState declarations to improve readability
  next-cube: convert next-pc device to use Resettable interface
  next-cube: rename typedef struct NextRtc to NeXTRTC
  next-cube: use qemu_irq to drive int_status in next_scr2_rtc_update()
  next-cube: separate rtc read and write shift logic
  next-cube: always use retval to return rtc read values
  next-cube: use named gpio to set RTC data bit in scr2
  next-cube: use named gpio to read RTC data bit in scr2
  next-cube: don't use rtc phase value of -1
  next-cube: QOMify NeXTRTC
  next-cube: move reset of next-rtc fields from next-pc to next-rtc
  next-cube: move rtc-data-in gpio from next-pc to next-rtc device
  next-cube: use named gpio output for next-rtc data
  next-cube: add rtc-cmd-reset named gpio to reset the rtc state machine
  next-cube: add rtc-power-out named gpio to trigger the NEXT_PWR_I
    interrupt
  next-cube: move next_rtc_cmd_is_write() and next_rtc_data_in_irq()
    functions
  next-cube: rename old_scr2 and scr2_2 in next_scr2_rtc_update()
  next-cube: add my copyright to the top of the file
  next-cube: replace boiler-plate GPL 2.0 or later license text with
    SPDX identifier

 hw/m68k/Kconfig     |    1 +
 hw/m68k/next-cube.c | 1026 +++++++++++++++++++++++++++----------------
 2 files changed, 659 insertions(+), 368 deletions(-)

-- 
2.39.5

[PATCH v2 01/34] next-cube: remove 0x14020 dummy value from next_mmio_read()

[Mark Cave-Ayland]

This is a dummy value for the SCSI CSR which appears to have no effect when
removed. Eventually the reads/writes to this register will be directed
towards the WIP implementations in next_scr_readfn() and next_scr_writefn().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 08886d432c..0418fbc8aa 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -286,10 +286,6 @@ static uint64_t next_mmio_read(void *opaque, hwaddr addr, unsigned size)
                         size << 3);
         break;
 
-    case 0x14020:
-        val = 0x7f;
-        break;
-
     default:
         val = 0;
         DPRINTF("MMIO Read @ 0x%"HWADDR_PRIx" size %d\n", addr, size);
-- 
2.39.5

[PATCH v2 02/34] next-cube: remove overlap between next.dma and next.mmio memory regions

[Mark Cave-Ayland]

Change the start of the next.mmio memory region so that it follows on directly
after the next.dma memory region. Increase the address offsets in
next_mmio_read() and next_mmio_write(), and reduce the size of the next.mmio
memory region accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/m68k/next-cube.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 0418fbc8aa..550e7f0b0a 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -266,23 +266,23 @@ static uint64_t next_mmio_read(void *opaque, hwaddr addr, unsigned size)
     uint64_t val;
 
     switch (addr) {
-    case 0x7000:
+    case 0x2000:    /* 0x2005000 */
         /* DPRINTF("Read INT status: %x\n", s->int_status); */
         val = s->int_status;
         break;
 
-    case 0x7800:
+    case 0x2800:    /* 0x2007800 */
         DPRINTF("MMIO Read INT mask: %x\n", s->int_mask);
         val = s->int_mask;
         break;
 
-    case 0xc000 ... 0xc003:
-        val = extract32(s->scr1, (4 - (addr - 0xc000) - size) << 3,
+    case 0x7000 ... 0x7003:    /* 0x200c000 */
+        val = extract32(s->scr1, (4 - (addr - 0x7000) - size) << 3,
                         size << 3);
         break;
 
-    case 0xd000 ... 0xd003:
-        val = extract32(s->scr2, (4 - (addr - 0xd000) - size) << 3,
+    case 0x8000 ... 0x8003:    /* 0x200d000 */
+        val = extract32(s->scr2, (4 - (addr - 0x8000) - size) << 3,
                         size << 3);
         break;
 
@@ -301,25 +301,25 @@ static void next_mmio_write(void *opaque, hwaddr addr, uint64_t val,
     NeXTPC *s = NEXT_PC(opaque);
 
     switch (addr) {
-    case 0x7000:
+    case 0x2000:    /* 0x2005000 */
         DPRINTF("INT Status old: %x new: %x\n", s->int_status,
                 (unsigned int)val);
         s->int_status = val;
         break;
 
-    case 0x7800:
+    case 0x2800:    /* 0x2007800 */
         DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, (unsigned int)val);
         s->int_mask  = val;
         break;
 
-    case 0xc000 ... 0xc003:
+    case 0x7000 ... 0x7003:    /* 0x200c000 */
         DPRINTF("SCR1 Write: %x\n", (unsigned int)val);
-        s->scr1 = deposit32(s->scr1, (4 - (addr - 0xc000) - size) << 3,
+        s->scr1 = deposit32(s->scr1, (4 - (addr - 0x7000) - size) << 3,
                             size << 3, val);
         break;
 
-    case 0xd000 ... 0xd003:
-        s->scr2 = deposit32(s->scr2, (4 - (addr - 0xd000) - size) << 3,
+    case 0x8000 ... 0x8003:    /* 0x200d000 */
+        s->scr2 = deposit32(s->scr2, (4 - (addr - 0x8000) - size) << 3,
                             size << 3, val);
         next_scr2_led_update(s);
         next_scr2_rtc_update(s);
@@ -897,7 +897,7 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     qdev_init_gpio_in(dev, next_irq, NEXT_NUM_IRQS);
 
     memory_region_init_io(&s->mmiomem, OBJECT(s), &next_mmio_ops, s,
-                          "next.mmio", 0xd0000);
+                          "next.mmio", 0x9000);
     memory_region_init_io(&s->scrmem, OBJECT(s), &next_scr_ops, s,
                           "next.scr", 0x20000);
     sysbus_init_mmio(sbd, &s->mmiomem);
@@ -1000,7 +1000,7 @@ static void next_cube_init(MachineState *machine)
     sysbus_create_simple(TYPE_NEXTFB, 0x0B000000, NULL);
 
     /* MMIO */
-    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 0, 0x02000000);
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 0, 0x02005000);
 
     /* BMAP IO - acts as a catch-all for now */
     sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
-- 
2.39.5

[PATCH v2 03/34] next-cube: create new next.scsi container memory region

[Mark Cave-Ayland]

Move the ESP SCSI and SCSI CSR registers to the new next.scsi container memory
region.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 550e7f0b0a..f95ed4a170 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -94,6 +94,7 @@ struct NeXTPC {
 
     MemoryRegion mmiomem;
     MemoryRegion scrmem;
+    MemoryRegion scsimem;
 
     uint32_t scr1;
     uint32_t scr2;
@@ -843,7 +844,12 @@ static void next_scsi_init(DeviceState *pcdev)
     sysbusdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(sysbusdev, &error_fatal);
     sysbus_connect_irq(sysbusdev, 0, qdev_get_gpio_in(pcdev, NEXT_SCSI_I));
-    sysbus_mmio_map(sysbusdev, 0, 0x2114000);
+
+    memory_region_init(&next_pc->scsimem, OBJECT(next_pc), "next.scsi", 0x40);
+    memory_region_add_subregion(&next_pc->scsimem, 0x0,
+                                sysbus_mmio_get_region(sysbusdev, 0));
+
+    memory_region_add_subregion(&next_pc->scrmem, 0x14000, &next_pc->scsimem);
 
     next_pc->scsi_reset = qdev_get_gpio_in(dev, 0);
     next_pc->scsi_dma = qdev_get_gpio_in(dev, 1);
-- 
2.39.5

[PATCH v2 04/34] next-cube: move next_scsi_init() to next_pc_realize()

[Mark Cave-Ayland]

This reflects that the SCSI interface exists within the NeXT Peripheral
Controller (PC).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index f95ed4a170..9b3578cd4f 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -908,6 +908,9 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
                           "next.scr", 0x20000);
     sysbus_init_mmio(sbd, &s->mmiomem);
     sysbus_init_mmio(sbd, &s->scrmem);
+
+    /* SCSI */
+    next_scsi_init(dev);
 }
 
 /*
@@ -1051,8 +1054,6 @@ static void next_cube_init(MachineState *machine)
 
     /* TODO: */
     /* Network */
-    /* SCSI */
-    next_scsi_init(pcdev);
 
     /* DMA */
     memory_region_init_io(&m->dmamem, NULL, &next_dma_ops, machine,
-- 
2.39.5

[PATCH v2 05/34] next-cube: introduce next_pc_init() object init function

[Mark Cave-Ayland]

Move initialisation of the memory regions and GPIOs from next_pc_realize() to
the new next_pc_init() function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 9b3578cd4f..fcd48f045e 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -897,20 +897,24 @@ static void next_pc_reset(DeviceState *dev)
 
 static void next_pc_realize(DeviceState *dev, Error **errp)
 {
-    NeXTPC *s = NEXT_PC(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    /* SCSI */
+    next_scsi_init(dev);
+}
+
+static void next_pc_init(Object *obj)
+{
+    NeXTPC *s = NEXT_PC(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    qdev_init_gpio_in(dev, next_irq, NEXT_NUM_IRQS);
+    qdev_init_gpio_in(DEVICE(obj), next_irq, NEXT_NUM_IRQS);
 
     memory_region_init_io(&s->mmiomem, OBJECT(s), &next_mmio_ops, s,
                           "next.mmio", 0x9000);
     memory_region_init_io(&s->scrmem, OBJECT(s), &next_scr_ops, s,
                           "next.scr", 0x20000);
+
     sysbus_init_mmio(sbd, &s->mmiomem);
     sysbus_init_mmio(sbd, &s->scrmem);
-
-    /* SCSI */
-    next_scsi_init(dev);
 }
 
 /*
@@ -972,6 +976,7 @@ static void next_pc_class_init(ObjectClass *klass, void *data)
 static const TypeInfo next_pc_info = {
     .name = TYPE_NEXT_PC,
     .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = next_pc_init,
     .instance_size = sizeof(NeXTPC),
     .class_init = next_pc_class_init,
 };
-- 
2.39.5

[PATCH v2 06/34] next-cube: introduce next-scsi device

[Mark Cave-Ayland]

This device is intended to hold the ESP SCSI controller and the NeXT SCSI CSRs.
Start by creating the device and moving the ESP SCSI controller to be an
embedded child device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 93 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 74 insertions(+), 19 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index fcd48f045e..1321c04520 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -83,6 +83,18 @@ struct NeXTState {
     next_dma dma[10];
 };
 
+#define TYPE_NEXT_SCSI "next-scsi"
+OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
+
+/* NeXT SCSI Controller */
+struct NeXTSCSI {
+    SysBusDevice parent_obj;
+
+    MemoryRegion scsi_mem;
+
+    SysBusESPState sysbus_esp;
+};
+
 #define TYPE_NEXT_PC "next-pc"
 OBJECT_DECLARE_SIMPLE_TYPE(NeXTPC, NEXT_PC)
 
@@ -94,7 +106,6 @@ struct NeXTPC {
 
     MemoryRegion mmiomem;
     MemoryRegion scrmem;
-    MemoryRegion scsimem;
 
     uint32_t scr1;
     uint32_t scr2;
@@ -102,6 +113,8 @@ struct NeXTPC {
     uint32_t int_mask;
     uint32_t int_status;
     uint32_t led;
+
+    NeXTSCSI next_scsi;
     uint8_t scsi_csr_1;
     uint8_t scsi_csr_2;
 
@@ -825,38 +838,61 @@ static void nextscsi_write(void *opaque, uint8_t *buf, int size)
     nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
 }
 
-static void next_scsi_init(DeviceState *pcdev)
+static void next_scsi_init(Object *obj)
 {
-    struct NeXTPC *next_pc = NEXT_PC(pcdev);
-    DeviceState *dev;
-    SysBusDevice *sysbusdev;
+    NeXTSCSI *s = NEXT_SCSI(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    object_initialize_child(obj, "esp", &s->sysbus_esp, TYPE_SYSBUS_ESP);
+
+    memory_region_init(&s->scsi_mem, obj, "next.scsi", 0x40);
+    sysbus_init_mmio(sbd, &s->scsi_mem);
+}
+
+static void next_scsi_realize(DeviceState *dev, Error **errp)
+{
+    NeXTSCSI *s = NEXT_SCSI(dev);
     SysBusESPState *sysbus_esp;
+    SysBusDevice *sbd;
     ESPState *esp;
+    NeXTPC *pcdev;
+
+    pcdev = NEXT_PC(container_of(s, NeXTPC, next_scsi));
 
-    dev = qdev_new(TYPE_SYSBUS_ESP);
-    sysbus_esp = SYSBUS_ESP(dev);
+    /* ESP */
+    sysbus_esp = SYSBUS_ESP(&s->sysbus_esp);
     esp = &sysbus_esp->esp;
     esp->dma_memory_read = nextscsi_read;
     esp->dma_memory_write = nextscsi_write;
     esp->dma_opaque = pcdev;
     sysbus_esp->it_shift = 0;
     esp->dma_enabled = 1;
-    sysbusdev = SYS_BUS_DEVICE(dev);
-    sysbus_realize_and_unref(sysbusdev, &error_fatal);
-    sysbus_connect_irq(sysbusdev, 0, qdev_get_gpio_in(pcdev, NEXT_SCSI_I));
-
-    memory_region_init(&next_pc->scsimem, OBJECT(next_pc), "next.scsi", 0x40);
-    memory_region_add_subregion(&next_pc->scsimem, 0x0,
-                                sysbus_mmio_get_region(sysbusdev, 0));
+    sbd = SYS_BUS_DEVICE(sysbus_esp);
+    if (!sysbus_realize(sbd, errp)) {
+        return;
+    }
+    memory_region_add_subregion(&s->scsi_mem, 0x0,
+                                sysbus_mmio_get_region(sbd, 0));
 
-    memory_region_add_subregion(&next_pc->scrmem, 0x14000, &next_pc->scsimem);
+    scsi_bus_legacy_handle_cmdline(&s->sysbus_esp.esp.bus);
+}
 
-    next_pc->scsi_reset = qdev_get_gpio_in(dev, 0);
-    next_pc->scsi_dma = qdev_get_gpio_in(dev, 1);
+static void next_scsi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
-    scsi_bus_legacy_handle_cmdline(&esp->bus);
+    dc->desc = "NeXT SCSI Controller";
+    dc->realize = next_scsi_realize;
 }
 
+static const TypeInfo next_scsi_info = {
+    .name = TYPE_NEXT_SCSI,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = next_scsi_init,
+    .instance_size = sizeof(NeXTSCSI),
+    .class_init = next_scsi_class_init,
+};
+
 static void next_escc_init(DeviceState *pcdev)
 {
     DeviceState *dev;
@@ -897,8 +933,24 @@ static void next_pc_reset(DeviceState *dev)
 
 static void next_pc_realize(DeviceState *dev, Error **errp)
 {
+    NeXTPC *s = NEXT_PC(dev);
+    SysBusDevice *sbd;
+    DeviceState *d;
+
     /* SCSI */
-    next_scsi_init(dev);
+    sbd = SYS_BUS_DEVICE(&s->next_scsi);
+    if (!sysbus_realize(sbd, errp)) {
+        return;
+    }
+    memory_region_add_subregion(&s->scrmem, 0x14000,
+                                sysbus_mmio_get_region(sbd, 0));
+
+    d = DEVICE(object_resolve_path_component(OBJECT(&s->next_scsi), "esp"));
+    sysbus_connect_irq(SYS_BUS_DEVICE(d), 0,
+                       qdev_get_gpio_in(DEVICE(s), NEXT_SCSI_I));
+
+    s->scsi_reset = qdev_get_gpio_in(d, 0);
+    s->scsi_dma = qdev_get_gpio_in(d, 1);
 }
 
 static void next_pc_init(Object *obj)
@@ -915,6 +967,8 @@ static void next_pc_init(Object *obj)
 
     sysbus_init_mmio(sbd, &s->mmiomem);
     sysbus_init_mmio(sbd, &s->scrmem);
+
+    object_initialize_child(obj, "next-scsi", &s->next_scsi, TYPE_NEXT_SCSI);
 }
 
 /*
@@ -1089,6 +1143,7 @@ static void next_register_type(void)
 {
     type_register_static(&next_typeinfo);
     type_register_static(&next_pc_info);
+    type_register_static(&next_scsi_info);
 }
 
 type_init(next_register_type)
-- 
2.39.5

[PATCH v2 07/34] next-cube: move SCSI CSRs from next-pc to the next-scsi device

[Mark Cave-Ayland]

The SCSI CSRs are located within the SCSI subsystem of the NeXT PC (Peripheral
Contoller) which is now modelled as a separate QEMU device. Add a new memory
region subregion to contain the SCSI CSRs that simply store and retrieve the
register values.

Add a new VMStateDescription for the next-scsi device to enable the SCSI CSRs
to be migrated.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 88 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 1321c04520..928740b281 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -93,6 +93,10 @@ struct NeXTSCSI {
     MemoryRegion scsi_mem;
 
     SysBusESPState sysbus_esp;
+
+    MemoryRegion scsi_csr_mem;
+    uint8_t scsi_csr_1;
+    uint8_t scsi_csr_2;
 };
 
 #define TYPE_NEXT_PC "next-pc"
@@ -115,8 +119,6 @@ struct NeXTPC {
     uint32_t led;
 
     NeXTSCSI next_scsi;
-    uint8_t scsi_csr_1;
-    uint8_t scsi_csr_2;
 
     qemu_irq scsi_reset;
     qemu_irq scsi_dma;
@@ -364,6 +366,7 @@ static const MemoryRegionOps next_mmio_ops = {
 static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
 {
     NeXTPC *s = NEXT_PC(opaque);
+    NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
     uint64_t val;
 
     switch (addr) {
@@ -373,12 +376,12 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case 0x14020:
-        DPRINTF("SCSI 4020  STATUS READ %X\n", s->scsi_csr_1);
-        val = s->scsi_csr_1;
+        DPRINTF("SCSI 4020  STATUS READ %X\n", ns->scsi_csr_1);
+        val = ns->scsi_csr_1;
         break;
 
     case 0x14021:
-        DPRINTF("SCSI 4021 STATUS READ %X\n", s->scsi_csr_2);
+        DPRINTF("SCSI 4021 STATUS READ %X\n", ns->scsi_csr_2);
         val = 0x40;
         break;
 
@@ -411,6 +414,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
                              unsigned size)
 {
     NeXTPC *s = NEXT_PC(opaque);
+    NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
 
     switch (addr) {
     case 0x14108:
@@ -445,7 +449,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
             DPRINTF("SCSICSR Reset\n");
             /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
             qemu_irq_raise(s->scsi_reset);
-            s->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
+            ns->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
             qemu_irq_lower(s->scsi_reset);
         }
         if (val & SCSICSR_DMADIR) {
@@ -838,6 +842,54 @@ static void nextscsi_write(void *opaque, uint8_t *buf, int size)
     nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
 }
 
+static void next_scsi_csr_write(void *opaque, hwaddr addr, uint64_t val,
+                                unsigned size)
+{
+    NeXTSCSI *s = NEXT_SCSI(opaque);
+
+    switch (addr) {
+    case 0:
+        s->scsi_csr_1 = val;
+        break;
+
+    case 1:
+        s->scsi_csr_2 = val;
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static uint64_t next_scsi_csr_read(void *opaque, hwaddr addr, unsigned size)
+{
+    NeXTSCSI *s = NEXT_SCSI(opaque);
+    uint64_t val;
+
+    switch (addr) {
+    case 0:
+        val = s->scsi_csr_1;
+        break;
+
+    case 1:
+        val = s->scsi_csr_2;
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+
+    return val;
+}
+
+static const MemoryRegionOps next_scsi_csr_ops = {
+    .read = next_scsi_csr_read,
+    .write = next_scsi_csr_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 1,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
 static void next_scsi_init(Object *obj)
 {
     NeXTSCSI *s = NEXT_SCSI(obj);
@@ -845,6 +897,9 @@ static void next_scsi_init(Object *obj)
 
     object_initialize_child(obj, "esp", &s->sysbus_esp, TYPE_SYSBUS_ESP);
 
+    memory_region_init_io(&s->scsi_csr_mem, obj, &next_scsi_csr_ops,
+                          s, "csrs", 2);
+
     memory_region_init(&s->scsi_mem, obj, "next.scsi", 0x40);
     sysbus_init_mmio(sbd, &s->scsi_mem);
 }
@@ -874,15 +929,30 @@ static void next_scsi_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->scsi_mem, 0x0,
                                 sysbus_mmio_get_region(sbd, 0));
 
+    /* SCSI CSRs */
+    memory_region_add_subregion(&s->scsi_mem, 0x20, &s->scsi_csr_mem);
+
     scsi_bus_legacy_handle_cmdline(&s->sysbus_esp.esp.bus);
 }
 
+static const VMStateDescription next_scsi_vmstate = {
+    .name = "next-scsi",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT8(scsi_csr_1, NeXTSCSI),
+        VMSTATE_UINT8(scsi_csr_2, NeXTSCSI),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static void next_scsi_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->desc = "NeXT SCSI Controller";
     dc->realize = next_scsi_realize;
+    dc->vmsd = &next_scsi_vmstate;
 }
 
 static const TypeInfo next_scsi_info = {
@@ -1000,8 +1070,8 @@ static const VMStateDescription next_rtc_vmstate = {
 
 static const VMStateDescription next_pc_vmstate = {
     .name = "next-pc",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(scr1, NeXTPC),
         VMSTATE_UINT32(scr2, NeXTPC),
@@ -1009,8 +1079,6 @@ static const VMStateDescription next_pc_vmstate = {
         VMSTATE_UINT32(int_mask, NeXTPC),
         VMSTATE_UINT32(int_status, NeXTPC),
         VMSTATE_UINT32(led, NeXTPC),
-        VMSTATE_UINT8(scsi_csr_1, NeXTPC),
-        VMSTATE_UINT8(scsi_csr_2, NeXTPC),
         VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
         VMSTATE_END_OF_LIST()
     },
-- 
2.39.5

[PATCH v2 08/34] next-cube: move SCSI 4020/4021 logic from next-pc device to next-scsi device

[Mark Cave-Ayland]

The SCSI 4020/4021 logic refers to the offset of the SCSI CSRs within the NeXTCube
address space. Due to the previously overlapping memory regions, there were
duplicate MMIO accessors in the next.scr memory region for these registers but
this has now been resolved.

Move the remaining SCSI 4020/4021 logic from the next-pc device to the next-scsi
device, with the exception that the SCSI 4021 register now returns its previous
value like a normal register instead of a hardcoded 0x40 value. This also matches
how the registers are implemented in the Previous emulator.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 139 ++++++++++++++++++++------------------------
 1 file changed, 62 insertions(+), 77 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 928740b281..ea1006efb3 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -365,8 +365,6 @@ static const MemoryRegionOps next_mmio_ops = {
 
 static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
 {
-    NeXTPC *s = NEXT_PC(opaque);
-    NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
     uint64_t val;
 
     switch (addr) {
@@ -375,16 +373,6 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
         val = 0x40 | 0x04 | 0x2 | 0x1;
         break;
 
-    case 0x14020:
-        DPRINTF("SCSI 4020  STATUS READ %X\n", ns->scsi_csr_1);
-        val = ns->scsi_csr_1;
-        break;
-
-    case 0x14021:
-        DPRINTF("SCSI 4021 STATUS READ %X\n", ns->scsi_csr_2);
-        val = 0x40;
-        break;
-
     /*
      * These 4 registers are the hardware timer, not sure which register
      * is the latch instead of data, but no problems so far.
@@ -413,9 +401,6 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
 static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
                              unsigned size)
 {
-    NeXTPC *s = NEXT_PC(opaque);
-    NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
-
     switch (addr) {
     case 0x14108:
         DPRINTF("FDCSR Write: %"PRIx64 "\n", val);
@@ -424,68 +409,6 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
         }
         break;
 
-    case 0x14020: /* SCSI Control Register */
-        if (val & SCSICSR_FIFOFL) {
-            DPRINTF("SCSICSR FIFO Flush\n");
-            /* will have to add another irq to the esp if this is needed */
-            /* esp_puflush_fifo(esp_g); */
-        }
-
-        if (val & SCSICSR_ENABLE) {
-            DPRINTF("SCSICSR Enable\n");
-            /*
-             * qemu_irq_raise(s->scsi_dma);
-             * s->scsi_csr_1 = 0xc0;
-             * s->scsi_csr_1 |= 0x1;
-             * qemu_irq_pulse(s->scsi_dma);
-             */
-        }
-        /*
-         * else
-         *     s->scsi_csr_1 &= ~SCSICSR_ENABLE;
-         */
-
-        if (val & SCSICSR_RESET) {
-            DPRINTF("SCSICSR Reset\n");
-            /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
-            qemu_irq_raise(s->scsi_reset);
-            ns->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
-            qemu_irq_lower(s->scsi_reset);
-        }
-        if (val & SCSICSR_DMADIR) {
-            DPRINTF("SCSICSR DMAdir\n");
-        }
-        if (val & SCSICSR_CPUDMA) {
-            DPRINTF("SCSICSR CPUDMA\n");
-            /* qemu_irq_raise(s->scsi_dma); */
-            s->int_status |= 0x4000000;
-        } else {
-            /* fprintf(stderr,"SCSICSR CPUDMA disabled\n"); */
-            s->int_status &= ~(0x4000000);
-            /* qemu_irq_lower(s->scsi_dma); */
-        }
-        if (val & SCSICSR_INTMASK) {
-            DPRINTF("SCSICSR INTMASK\n");
-            /*
-             * int_mask &= ~0x1000;
-             * s->scsi_csr_1 |= val;
-             * s->scsi_csr_1 &= ~SCSICSR_INTMASK;
-             * if (s->scsi_queued) {
-             *     s->scsi_queued = 0;
-             *     next_irq(s, NEXT_SCSI_I, level);
-             * }
-             */
-        } else {
-            /* int_mask |= 0x1000; */
-        }
-        if (val & 0x80) {
-            /* int_mask |= 0x1000; */
-            /* s->scsi_csr_1 |= 0x80; */
-        }
-        DPRINTF("SCSICSR Write: %"PRIx64 "\n", val);
-        /* s->scsi_csr_1 = val; */
-        break;
-
     /* Hardware timer latch - not implemented yet */
     case 0x1a000:
     default:
@@ -846,13 +769,73 @@ static void next_scsi_csr_write(void *opaque, hwaddr addr, uint64_t val,
                                 unsigned size)
 {
     NeXTSCSI *s = NEXT_SCSI(opaque);
+    NeXTPC *pc = NEXT_PC(container_of(s, NeXTPC, next_scsi));
 
     switch (addr) {
     case 0:
+        if (val & SCSICSR_FIFOFL) {
+            DPRINTF("SCSICSR FIFO Flush\n");
+            /* will have to add another irq to the esp if this is needed */
+            /* esp_puflush_fifo(esp_g); */
+        }
+
+        if (val & SCSICSR_ENABLE) {
+            DPRINTF("SCSICSR Enable\n");
+            /*
+             * qemu_irq_raise(s->scsi_dma);
+             * s->scsi_csr_1 = 0xc0;
+             * s->scsi_csr_1 |= 0x1;
+             * qemu_irq_pulse(s->scsi_dma);
+             */
+        }
+        /*
+         * else
+         *     s->scsi_csr_1 &= ~SCSICSR_ENABLE;
+         */
+
+        if (val & SCSICSR_RESET) {
+            DPRINTF("SCSICSR Reset\n");
+            /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
+            qemu_irq_raise(pc->scsi_reset);
+            s->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
+            qemu_irq_lower(pc->scsi_reset);
+        }
+        if (val & SCSICSR_DMADIR) {
+            DPRINTF("SCSICSR DMAdir\n");
+        }
+        if (val & SCSICSR_CPUDMA) {
+            DPRINTF("SCSICSR CPUDMA\n");
+            /* qemu_irq_raise(s->scsi_dma); */
+            pc->int_status |= 0x4000000;
+        } else {
+            /* fprintf(stderr,"SCSICSR CPUDMA disabled\n"); */
+            pc->int_status &= ~(0x4000000);
+            /* qemu_irq_lower(s->scsi_dma); */
+        }
+        if (val & SCSICSR_INTMASK) {
+            DPRINTF("SCSICSR INTMASK\n");
+            /*
+             * int_mask &= ~0x1000;
+             * s->scsi_csr_1 |= val;
+             * s->scsi_csr_1 &= ~SCSICSR_INTMASK;
+             * if (s->scsi_queued) {
+             *     s->scsi_queued = 0;
+             *     next_irq(s, NEXT_SCSI_I, level);
+             * }
+             */
+        } else {
+            /* int_mask |= 0x1000; */
+        }
+        if (val & 0x80) {
+            /* int_mask |= 0x1000; */
+            /* s->scsi_csr_1 |= 0x80; */
+        }
+        DPRINTF("SCSICSR1 Write: %"PRIx64 "\n", val);
         s->scsi_csr_1 = val;
         break;
 
     case 1:
+        DPRINTF("SCSICSR2 Write: %"PRIx64 "\n", val);
         s->scsi_csr_2 = val;
         break;
 
@@ -868,10 +851,12 @@ static uint64_t next_scsi_csr_read(void *opaque, hwaddr addr, unsigned size)
 
     switch (addr) {
     case 0:
+        DPRINTF("SCSI 4020  STATUS READ %X\n", s->scsi_csr_1);
         val = s->scsi_csr_1;
         break;
 
     case 1:
+        DPRINTF("SCSI 4021 STATUS READ %X\n", s->scsi_csr_2);
         val = s->scsi_csr_2;
         break;
 
-- 
2.39.5

[PATCH v2 09/34] next-cube: move floppy disk MMIO to separate memory region in next-pc

[Mark Cave-Ayland]

The dummy floppy disk device is part of the next-pc device, and not related to
the NeXTCube SCRs.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 61 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index ea1006efb3..d08026ccf9 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -108,6 +108,7 @@ struct NeXTPC {
 
     M68kCPU *cpu;
 
+    MemoryRegion floppy_mem;
     MemoryRegion mmiomem;
     MemoryRegion scrmem;
 
@@ -368,11 +369,6 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
     uint64_t val;
 
     switch (addr) {
-    case 0x14108:
-        DPRINTF("FD read @ %x\n", (unsigned int)addr);
-        val = 0x40 | 0x04 | 0x2 | 0x1;
-        break;
-
     /*
      * These 4 registers are the hardware timer, not sure which register
      * is the latch instead of data, but no problems so far.
@@ -402,13 +398,6 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
                              unsigned size)
 {
     switch (addr) {
-    case 0x14108:
-        DPRINTF("FDCSR Write: %"PRIx64 "\n", val);
-        if (val == 0x0) {
-            /* qemu_irq_raise(s->fd_irq[0]); */
-        }
-        break;
-
     /* Hardware timer latch - not implemented yet */
     case 0x1a000:
     default:
@@ -948,6 +937,47 @@ static const TypeInfo next_scsi_info = {
     .class_init = next_scsi_class_init,
 };
 
+static void next_floppy_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned size)
+{
+    switch (addr) {
+    case 0:
+        DPRINTF("FDCSR Write: %"PRIx64 "\n", val);
+        if (val == 0x0) {
+            /* qemu_irq_raise(s->fd_irq[0]); */
+        }
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static uint64_t next_floppy_read(void *opaque, hwaddr addr, unsigned size)
+{
+    uint64_t val;
+
+    switch (addr) {
+    case 0:
+        DPRINTF("FD read @ %x\n", (unsigned int)addr);
+        val = 0x40 | 0x04 | 0x2 | 0x1;
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+
+    return val;
+}
+
+static const MemoryRegionOps next_floppy_ops = {
+    .read = next_floppy_read,
+    .write = next_floppy_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
 static void next_escc_init(DeviceState *pcdev)
 {
     DeviceState *dev;
@@ -1006,6 +1036,10 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
 
     s->scsi_reset = qdev_get_gpio_in(d, 0);
     s->scsi_dma = qdev_get_gpio_in(d, 1);
+
+    /* Floppy */
+    memory_region_add_subregion(&s->scrmem, 0x14108,
+                                &s->floppy_mem);
 }
 
 static void next_pc_init(Object *obj)
@@ -1024,6 +1058,9 @@ static void next_pc_init(Object *obj)
     sysbus_init_mmio(sbd, &s->scrmem);
 
     object_initialize_child(obj, "next-scsi", &s->next_scsi, TYPE_NEXT_SCSI);
+
+    memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
+                          "next.floppy", 4);
 }
 
 /*
-- 
2.39.5

[PATCH v2 10/34] next-cube: map ESCC registers as a subregion of the next.scr memory region

[Mark Cave-Ayland]

Since the ESCC device exists within the memory range of the next.scr memory region, map
the ESCC device registers as a subregion of the next.scr memory region instead of
directly to the system address space.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index d08026ccf9..efbb11e74b 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -980,6 +980,7 @@ static const MemoryRegionOps next_floppy_ops = {
 
 static void next_escc_init(DeviceState *pcdev)
 {
+    NeXTPC *next_pc = NEXT_PC(pcdev);
     DeviceState *dev;
     SysBusDevice *s;
 
@@ -997,7 +998,9 @@ static void next_escc_init(DeviceState *pcdev)
     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));
-    sysbus_mmio_map(s, 0, 0x2118000);
+
+    memory_region_add_subregion(&next_pc->scrmem, 0x18000,
+                                sysbus_mmio_get_region(s, 0));
 }
 
 static void next_pc_reset(DeviceState *dev)
-- 
2.39.5

[PATCH v2 11/34] next-cube: move ESCC to be QOM child of next-pc device

[Mark Cave-Ayland]

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 efbb11e74b..20a0b073e1 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 */
 
-- 
2.39.5

[PATCH v2 12/34] next-cube: move timer MMIO to separate memory region on next-pc device

[Mark Cave-Ayland]

Move the timer MMIO accesses to a separate memory region on the next-pc device
instead of being part of the next.scr MMIO memory region.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 63 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 20a0b073e1..861d90024a 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -109,6 +109,7 @@ struct NeXTPC {
     M68kCPU *cpu;
 
     MemoryRegion floppy_mem;
+    MemoryRegion timer_mem;
     MemoryRegion mmiomem;
     MemoryRegion scrmem;
 
@@ -371,17 +372,6 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
     uint64_t val;
 
     switch (addr) {
-    /*
-     * These 4 registers are the hardware timer, not sure which register
-     * is the latch instead of data, but no problems so far.
-     *
-     * Hack: We need to have the LSB change consistently to make it work
-     */
-    case 0x1a000 ... 0x1a003:
-        val = extract32(clock(), (4 - (addr - 0x1a000) - size) << 3,
-                        size << 3);
-        break;
-
     /* For now return dummy byte to allow the Ethernet test to timeout */
     case 0x6000:
         val = 0xff;
@@ -400,8 +390,6 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
                              unsigned size)
 {
     switch (addr) {
-    /* Hardware timer latch - not implemented yet */
-    case 0x1a000:
     default:
         DPRINTF("BMAP Write @ 0x%x with 0x%"PRIx64 " size %u\n",
                 (unsigned int)addr, val, size);
@@ -980,6 +968,50 @@ static const MemoryRegionOps next_floppy_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void next_timer_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned size)
+{
+    switch (addr) {
+    case 0 ... 3:
+        /* Hardware timer latch - not implemented yet */
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static uint64_t next_timer_read(void *opaque, hwaddr addr, unsigned size)
+{
+    uint64_t val;
+
+    switch (addr) {
+    case 0 ... 3:
+        /*
+         * These 4 registers are the hardware timer, not sure which register
+         * is the latch instead of data, but no problems so far.
+         *
+         * Hack: We need to have the LSB change consistently to make it work
+         */
+        val = extract32(clock(), (4 - addr - size) << 3,
+                        size << 3);
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+
+    return val;
+}
+
+static const MemoryRegionOps next_timer_ops = {
+    .read = next_timer_read,
+    .write = next_timer_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
 static void next_pc_reset(DeviceState *dev)
 {
     NeXTPC *s = NEXT_PC(dev);
@@ -1042,6 +1074,8 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->scrmem, 0x18000,
                                 sysbus_mmio_get_region(sbd, 0));
 
+    /* Timer */
+    memory_region_add_subregion(&s->scrmem, 0x1a000, &s->timer_mem);
 }
 
 static void next_pc_init(Object *obj)
@@ -1065,6 +1099,9 @@ static void next_pc_init(Object *obj)
                           "next.floppy", 4);
 
     object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
+
+    memory_region_init_io(&s->timer_mem, OBJECT(s), &next_timer_ops, s,
+                          "next.timer", 4);
 }
 
 /*
-- 
2.39.5

[PATCH v2 13/34] next-cube: move en ethernet MMIO to separate memory region on next-pc device

[Mark Cave-Ayland]

Move the en ethernet MMIO accesses to a separate memory region on the next-pc
device instead of being part of the next.scr MMIO memory region.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 48 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 861d90024a..76ecf86ed0 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -110,6 +110,7 @@ struct NeXTPC {
 
     MemoryRegion floppy_mem;
     MemoryRegion timer_mem;
+    MemoryRegion dummyen_mem;
     MemoryRegion mmiomem;
     MemoryRegion scrmem;
 
@@ -372,11 +373,6 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
     uint64_t val;
 
     switch (addr) {
-    /* For now return dummy byte to allow the Ethernet test to timeout */
-    case 0x6000:
-        val = 0xff;
-        break;
-
     default:
         DPRINTF("BMAP Read @ 0x%x size %u\n", (unsigned int)addr, size);
         val = 0;
@@ -1012,6 +1008,38 @@ static const MemoryRegionOps next_timer_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void next_dummy_en_write(void *opaque, hwaddr addr, uint64_t val,
+                                unsigned size)
+{
+    /* Do nothing */
+    return;
+}
+
+static uint64_t next_dummy_en_read(void *opaque, hwaddr addr, unsigned size)
+{
+    uint64_t val;
+
+    switch (addr) {
+    case 0:
+        /* For now return dummy byte to allow the Ethernet test to timeout */
+        val = 0xff;
+        break;
+
+    default:
+        val = 0;
+    }
+
+    return val;
+}
+
+static const MemoryRegionOps next_dummy_en_ops = {
+    .read = next_dummy_en_read,
+    .write = next_dummy_en_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
 static void next_pc_reset(DeviceState *dev)
 {
     NeXTPC *s = NEXT_PC(dev);
@@ -1034,6 +1062,10 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd;
     DeviceState *d;
 
+    /* en network (dummy) */
+    memory_region_add_subregion(&s->scrmem, 0x6000,
+                                &s->dummyen_mem);
+
     /* SCSI */
     sbd = SYS_BUS_DEVICE(&s->next_scsi);
     if (!sysbus_realize(sbd, errp)) {
@@ -1093,6 +1125,9 @@ static void next_pc_init(Object *obj)
     sysbus_init_mmio(sbd, &s->mmiomem);
     sysbus_init_mmio(sbd, &s->scrmem);
 
+    memory_region_init_io(&s->dummyen_mem, OBJECT(s), &next_dummy_en_ops, s,
+                          "next.en", 0x20);
+
     object_initialize_child(obj, "next-scsi", &s->next_scsi, TYPE_NEXT_SCSI);
 
     memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
@@ -1239,9 +1274,6 @@ static void next_cube_init(MachineState *machine)
         }
     }
 
-    /* TODO: */
-    /* Network */
-
     /* DMA */
     memory_region_init_io(&m->dmamem, NULL, &next_dma_ops, machine,
                           "next.dma", 0x5000);
-- 
2.39.5

[PATCH v2 14/34] next-cube: add empty slots for unknown accesses to next.scr memory region

[Mark Cave-Ayland]

The next.scr memory is now effectively unused, however there are 3 separate region
accesses still logged that occur when booting a NeXTStep disk image. Use the
empty_slot device to capture and ignore memory accesses to these 3 memory regions.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/Kconfig     | 1 +
 hw/m68k/next-cube.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
index 0092cda4e9..aff769b30f 100644
--- a/hw/m68k/Kconfig
+++ b/hw/m68k/Kconfig
@@ -18,6 +18,7 @@ config NEXTCUBE
     depends on M68K
     select FRAMEBUFFER
     select ESCC
+    select EMPTY_SLOT
 
 config Q800
     bool
diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 76ecf86ed0..95d01030cd 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -22,6 +22,7 @@
 #include "qom/object.h"
 #include "hw/char/escc.h" /* ZILOG 8530 Serial Emulation */
 #include "hw/block/fdc.h"
+#include "hw/misc/empty_slot.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -1239,6 +1240,13 @@ static void next_cube_init(MachineState *machine)
     /* BMAP IO - acts as a catch-all for now */
     sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
 
+    /* unknown: Brightness control register? */
+    empty_slot_init("next.unknown", 0x02110000, 0x10);
+    /* unknown: Magneto-Optical drive controller? */
+    empty_slot_init("next.unknown", 0x02112000, 0x10);
+    /* unknown: Serial clock configuration register? */
+    empty_slot_init("next.unknown", 0x02118004, 0x10);
+
     /* BMAP memory */
     memory_region_init_ram_flags_nomigrate(&m->bmapm1, NULL, "next.bmapmem",
                                            64, RAM_SHARED, &error_fatal);
-- 
2.39.5

[PATCH v2 15/34] next-cube: remove unused next.scr memory region

[Mark Cave-Ayland]

Now that the next.scr memory region is unused it can be removed and the next-pc
devices mapped directly within the machine init function. This is the last
remaining overlapping memory region within the NeXTCube machine.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 73 +++++++++++++--------------------------------
 1 file changed, 21 insertions(+), 52 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 95d01030cd..86a0aaa511 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -369,38 +369,6 @@ static const MemoryRegionOps next_mmio_ops = {
 #define SCSICSR_CPUDMA  0x10  /* if set, dma enabled */
 #define SCSICSR_INTMASK 0x20  /* if set, interrupt enabled */
 
-static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
-{
-    uint64_t val;
-
-    switch (addr) {
-    default:
-        DPRINTF("BMAP Read @ 0x%x size %u\n", (unsigned int)addr, size);
-        val = 0;
-        break;
-    }
-
-    return val;
-}
-
-static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
-                             unsigned size)
-{
-    switch (addr) {
-    default:
-        DPRINTF("BMAP Write @ 0x%x with 0x%"PRIx64 " size %u\n",
-                (unsigned int)addr, val, size);
-    }
-}
-
-static const MemoryRegionOps next_scr_ops = {
-    .read = next_scr_readfn,
-    .write = next_scr_writefn,
-    .valid.min_access_size = 1,
-    .valid.max_access_size = 4,
-    .endianness = DEVICE_BIG_ENDIAN,
-};
-
 #define NEXTDMA_SCSI(x)      (0x10 + x)
 #define NEXTDMA_FD(x)        (0x10 + x)
 #define NEXTDMA_ENTX(x)      (0x110 + x)
@@ -1063,17 +1031,11 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd;
     DeviceState *d;
 
-    /* en network (dummy) */
-    memory_region_add_subregion(&s->scrmem, 0x6000,
-                                &s->dummyen_mem);
-
     /* SCSI */
     sbd = SYS_BUS_DEVICE(&s->next_scsi);
     if (!sysbus_realize(sbd, errp)) {
         return;
     }
-    memory_region_add_subregion(&s->scrmem, 0x14000,
-                                sysbus_mmio_get_region(sbd, 0));
 
     d = DEVICE(object_resolve_path_component(OBJECT(&s->next_scsi), "esp"));
     sysbus_connect_irq(SYS_BUS_DEVICE(d), 0,
@@ -1082,10 +1044,6 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     s->scsi_reset = qdev_get_gpio_in(d, 0);
     s->scsi_dma = qdev_get_gpio_in(d, 1);
 
-    /* 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);
@@ -1103,12 +1061,6 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     }
     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));
-
-    /* Timer */
-    memory_region_add_subregion(&s->scrmem, 0x1a000, &s->timer_mem);
 }
 
 static void next_pc_init(Object *obj)
@@ -1120,24 +1072,27 @@ static void next_pc_init(Object *obj)
 
     memory_region_init_io(&s->mmiomem, OBJECT(s), &next_mmio_ops, s,
                           "next.mmio", 0x9000);
-    memory_region_init_io(&s->scrmem, OBJECT(s), &next_scr_ops, s,
-                          "next.scr", 0x20000);
-
     sysbus_init_mmio(sbd, &s->mmiomem);
-    sysbus_init_mmio(sbd, &s->scrmem);
 
     memory_region_init_io(&s->dummyen_mem, OBJECT(s), &next_dummy_en_ops, s,
                           "next.en", 0x20);
+    sysbus_init_mmio(sbd, &s->dummyen_mem);
 
     object_initialize_child(obj, "next-scsi", &s->next_scsi, TYPE_NEXT_SCSI);
+    sysbus_init_mmio(sbd,
+                     sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->next_scsi), 0));
 
     memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
                           "next.floppy", 4);
+    sysbus_init_mmio(sbd, &s->floppy_mem);
 
     object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
+    sysbus_init_mmio(sbd,
+                     sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->escc), 0));
 
     memory_region_init_io(&s->timer_mem, OBJECT(s), &next_timer_ops, s,
                           "next.timer", 4);
+    sysbus_init_mmio(sbd, &s->timer_mem);
 }
 
 /*
@@ -1240,13 +1195,27 @@ static void next_cube_init(MachineState *machine)
     /* BMAP IO - acts as a catch-all for now */
     sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
 
+    /* en network (dummy) */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02106000);
+
     /* unknown: Brightness control register? */
     empty_slot_init("next.unknown", 0x02110000, 0x10);
     /* unknown: Magneto-Optical drive controller? */
     empty_slot_init("next.unknown", 0x02112000, 0x10);
+
+    /* SCSI */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 2, 0x02114000);
+    /* Floppy */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 3, 0x02114108);
+    /* ESCC */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 4, 0x02118000);
+
     /* unknown: Serial clock configuration register? */
     empty_slot_init("next.unknown", 0x02118004, 0x10);
 
+    /* Timer */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 5, 0x0211a000);
+
     /* BMAP memory */
     memory_region_init_ram_flags_nomigrate(&m->bmapm1, NULL, "next.bmapmem",
                                            64, RAM_SHARED, &error_fatal);
-- 
2.39.5

[PATCH v2 16/34] next-cube: rearrange NeXTState declarations to improve readability

[Mark Cave-Ayland]

Move the NeXTState, next_dma and TYPE_NEXT_MACHINE definition to the same area
at the top of next-cube.c.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 64 ++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 86a0aaa511..5333fc7bef 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -38,30 +38,10 @@
 #define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
-#define TYPE_NEXT_MACHINE MACHINE_TYPE_NAME("next-cube")
-OBJECT_DECLARE_SIMPLE_TYPE(NeXTState, NEXT_MACHINE)
-
 #define ENTRY       0x0100001e
 #define RAM_SIZE    0x4000000
 #define ROM_FILE    "Rev_2.5_v66.bin"
 
-typedef struct next_dma {
-    uint32_t csr;
-
-    uint32_t saved_next;
-    uint32_t saved_limit;
-    uint32_t saved_start;
-    uint32_t saved_stop;
-
-    uint32_t next;
-    uint32_t limit;
-    uint32_t start;
-    uint32_t stop;
-
-    uint32_t next_initbuf;
-    uint32_t size;
-} next_dma;
-
 typedef struct NextRtc {
     int8_t phase;
     uint8_t ram[32];
@@ -72,18 +52,6 @@ typedef struct NextRtc {
     uint8_t retval;
 } NextRtc;
 
-struct NeXTState {
-    MachineState parent;
-
-    MemoryRegion rom;
-    MemoryRegion rom2;
-    MemoryRegion dmamem;
-    MemoryRegion bmapm1;
-    MemoryRegion bmapm2;
-
-    next_dma dma[10];
-};
-
 #define TYPE_NEXT_SCSI "next-scsi"
 OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
 
@@ -132,6 +100,38 @@ struct NeXTPC {
     NextRtc rtc;
 };
 
+typedef struct next_dma {
+    uint32_t csr;
+
+    uint32_t saved_next;
+    uint32_t saved_limit;
+    uint32_t saved_start;
+    uint32_t saved_stop;
+
+    uint32_t next;
+    uint32_t limit;
+    uint32_t start;
+    uint32_t stop;
+
+    uint32_t next_initbuf;
+    uint32_t size;
+} next_dma;
+
+#define TYPE_NEXT_MACHINE MACHINE_TYPE_NAME("next-cube")
+OBJECT_DECLARE_SIMPLE_TYPE(NeXTState, NEXT_MACHINE)
+
+struct NeXTState {
+    MachineState parent;
+
+    MemoryRegion rom;
+    MemoryRegion rom2;
+    MemoryRegion dmamem;
+    MemoryRegion bmapm1;
+    MemoryRegion bmapm2;
+
+    next_dma dma[10];
+};
+
 /* Thanks to NeXT forums for this */
 /*
 static const uint8_t rtc_ram3[32] = {
-- 
2.39.5

[PATCH v2 17/34] next-cube: convert next-pc device to use Resettable interface

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 5333fc7bef..4a11fe6b39 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -1009,9 +1009,9 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void next_pc_reset(DeviceState *dev)
+static void next_pc_reset_hold(Object *obj, ResetType type)
 {
-    NeXTPC *s = NEXT_PC(dev);
+    NeXTPC *s = NEXT_PC(obj);
 
     /* Set internal registers to initial values */
     /*     0x0000XX00 << vital bits */
@@ -1141,12 +1141,13 @@ static const VMStateDescription next_pc_vmstate = {
 static void next_pc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     dc->desc = "NeXT Peripheral Controller";
     dc->realize = next_pc_realize;
-    device_class_set_legacy_reset(dc, next_pc_reset);
     device_class_set_props(dc, next_pc_properties);
     dc->vmsd = &next_pc_vmstate;
+    rc->phases.hold = next_pc_reset_hold;
 }
 
 static const TypeInfo next_pc_info = {
-- 
2.39.5

[PATCH v2 18/34] next-cube: rename typedef struct NextRtc to NeXTRTC

[Mark Cave-Ayland]

This brings the capitalisation in line with the other NeXTCube definitions.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 4a11fe6b39..0b07cd1a75 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -42,7 +42,7 @@
 #define RAM_SIZE    0x4000000
 #define ROM_FILE    "Rev_2.5_v66.bin"
 
-typedef struct NextRtc {
+typedef struct NeXTRTC {
     int8_t phase;
     uint8_t ram[32];
     uint8_t command;
@@ -50,7 +50,7 @@ typedef struct NextRtc {
     uint8_t status;
     uint8_t control;
     uint8_t retval;
-} NextRtc;
+} NeXTRTC;
 
 #define TYPE_NEXT_SCSI "next-scsi"
 OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
@@ -97,7 +97,7 @@ struct NeXTPC {
 
     ESCCState escc;
 
-    NextRtc rtc;
+    NeXTRTC rtc;
 };
 
 typedef struct next_dma {
@@ -167,7 +167,7 @@ static void next_scr2_led_update(NeXTPC *s)
 static void next_scr2_rtc_update(NeXTPC *s)
 {
     uint8_t old_scr2, scr2_2;
-    NextRtc *rtc = &s->rtc;
+    NeXTRTC *rtc = &s->rtc;
 
     old_scr2 = extract32(s->old_scr2, 8, 8);
     scr2_2 = extract32(s->scr2, 8, 8);
@@ -1111,13 +1111,13 @@ static const VMStateDescription next_rtc_vmstate = {
     .version_id = 2,
     .minimum_version_id = 2,
     .fields = (const VMStateField[]) {
-        VMSTATE_INT8(phase, NextRtc),
-        VMSTATE_UINT8_ARRAY(ram, NextRtc, 32),
-        VMSTATE_UINT8(command, NextRtc),
-        VMSTATE_UINT8(value, NextRtc),
-        VMSTATE_UINT8(status, NextRtc),
-        VMSTATE_UINT8(control, NextRtc),
-        VMSTATE_UINT8(retval, NextRtc),
+        VMSTATE_INT8(phase, NeXTRTC),
+        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
+        VMSTATE_UINT8(command, NeXTRTC),
+        VMSTATE_UINT8(value, NeXTRTC),
+        VMSTATE_UINT8(status, NeXTRTC),
+        VMSTATE_UINT8(control, NeXTRTC),
+        VMSTATE_UINT8(retval, NeXTRTC),
         VMSTATE_END_OF_LIST()
     },
 };
@@ -1133,7 +1133,7 @@ static const VMStateDescription next_pc_vmstate = {
         VMSTATE_UINT32(int_mask, NeXTPC),
         VMSTATE_UINT32(int_status, NeXTPC),
         VMSTATE_UINT32(led, NeXTPC),
-        VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
+        VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NeXTRTC),
         VMSTATE_END_OF_LIST()
     },
 };
-- 
2.39.5

[PATCH v2 19/34] next-cube: use qemu_irq to drive int_status in next_scr2_rtc_update()

[Mark Cave-Ayland]

Rather than directly clear bit 3 in int_status in next_scr2_rtc_update(), use
a qemu_irq to drive the equivalent NEXT_PWR_I signal.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 0b07cd1a75..7c6dc132fe 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -98,6 +98,7 @@ struct NeXTPC {
     ESCCState escc;
 
     NeXTRTC rtc;
+    qemu_irq rtc_power_irq;
 };
 
 typedef struct next_dma {
@@ -267,7 +268,7 @@ static void next_scr2_rtc_update(NeXTPC *s)
                     /* clear FTU */
                     if (rtc->value & 0x04) {
                         rtc->status = rtc->status & (~0x18);
-                        s->int_status = s->int_status & (~0x04);
+                        qemu_irq_lower(s->rtc_power_irq);
                     }
                 }
             }
@@ -1093,6 +1094,8 @@ static void next_pc_init(Object *obj)
     memory_region_init_io(&s->timer_mem, OBJECT(s), &next_timer_ops, s,
                           "next.timer", 4);
     sysbus_init_mmio(sbd, &s->timer_mem);
+
+    s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
 }
 
 /*
-- 
2.39.5

[PATCH v2 20/34] next-cube: separate rtc read and write shift logic

[Mark Cave-Ayland]

Introduce a new next_rtc_cmd_is_write() function to determine if an rtc command
is a read or write, and start by using it to avoid shifting the rtc input value
if a rtc read command is executed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 138 ++++++++++++++++++++++++--------------------
 1 file changed, 74 insertions(+), 64 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 7c6dc132fe..cc3dcad4e6 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -165,6 +165,12 @@ static void next_scr2_led_update(NeXTPC *s)
     }
 }
 
+static bool next_rtc_cmd_is_write(uint8_t cmd)
+{
+    return (cmd >= 0x80 && cmd <= 0x9f) ||
+           (cmd == 0xb1);
+}
+
 static void next_scr2_rtc_update(NeXTPC *s)
 {
     uint8_t old_scr2, scr2_2;
@@ -186,76 +192,80 @@ static void next_scr2_rtc_update(NeXTPC *s)
                                ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
             }
             if (rtc->phase >= 8 && rtc->phase < 16) {
-                rtc->value = (rtc->value << 1) |
-                             ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-
-                /* if we read RAM register, output RT_DATA bit */
-                if (rtc->command <= 0x1F) {
-                    scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                    if (rtc->ram[rtc->command] & (0x80 >> (rtc->phase - 8))) {
-                        scr2_2 |= SCR2_RTDATA;
-                    }
-
-                    rtc->retval = (rtc->retval << 1) |
-                                  ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                }
-                /* read the status 0x30 */
-                if (rtc->command == 0x30) {
-                    scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                    /* for now status = 0x98 (new rtc + FTU) */
-                    if (rtc->status & (0x80 >> (rtc->phase - 8))) {
-                        scr2_2 |= SCR2_RTDATA;
+                if (next_rtc_cmd_is_write(rtc->command)) {
+                    /* Shift in value to write */
+                    rtc->value = (rtc->value << 1) |
+                                 ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
+                } else {
+                    /* Shift out value to read */
+
+                    /* if we read RAM register, output RT_DATA bit */
+                    if (rtc->command <= 0x1F) {
+                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
+                        if (rtc->ram[rtc->command] &
+                            (0x80 >> (rtc->phase - 8))) {
+                                scr2_2 |= SCR2_RTDATA;
+                        }
+
+                        rtc->retval = (rtc->retval << 1) |
+                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
                     }
-
-                    rtc->retval = (rtc->retval << 1) |
-                                  ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                }
-                /* read the status 0x31 */
-                if (rtc->command == 0x31) {
-                    scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                    if (rtc->control & (0x80 >> (rtc->phase - 8))) {
-                        scr2_2 |= SCR2_RTDATA;
+                    /* read the status 0x30 */
+                    if (rtc->command == 0x30) {
+                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
+                        /* for now status = 0x98 (new rtc + FTU) */
+                        if (rtc->status & (0x80 >> (rtc->phase - 8))) {
+                            scr2_2 |= SCR2_RTDATA;
+                        }
+
+                        rtc->retval = (rtc->retval << 1) |
+                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
                     }
-                    rtc->retval = (rtc->retval << 1) |
-                                  ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                }
-
-                if ((rtc->command >= 0x20) && (rtc->command <= 0x2F)) {
-                    scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                    /* for now 0x00 */
-                    time_t time_h = time(NULL);
-                    struct tm *info = localtime(&time_h);
-                    int ret = 0;
-
-                    switch (rtc->command) {
-                    case 0x20:
-                        ret = SCR2_TOBCD(info->tm_sec);
-                        break;
-                    case 0x21:
-                        ret = SCR2_TOBCD(info->tm_min);
-                        break;
-                    case 0x22:
-                        ret = SCR2_TOBCD(info->tm_hour);
-                        break;
-                    case 0x24:
-                        ret = SCR2_TOBCD(info->tm_mday);
-                        break;
-                    case 0x25:
-                        ret = SCR2_TOBCD((info->tm_mon + 1));
-                        break;
-                    case 0x26:
-                        ret = SCR2_TOBCD((info->tm_year - 100));
-                        break;
-
+                    /* read the status 0x31 */
+                    if (rtc->command == 0x31) {
+                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
+                        if (rtc->control & (0x80 >> (rtc->phase - 8))) {
+                            scr2_2 |= SCR2_RTDATA;
+                        }
+                        rtc->retval = (rtc->retval << 1) |
+                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
                     }
 
-                    if (ret & (0x80 >> (rtc->phase - 8))) {
-                        scr2_2 |= SCR2_RTDATA;
+                    if ((rtc->command >= 0x20) && (rtc->command <= 0x2F)) {
+                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
+                        /* for now 0x00 */
+                        time_t time_h = time(NULL);
+                        struct tm *info = localtime(&time_h);
+                        int ret = 0;
+
+                        switch (rtc->command) {
+                        case 0x20:
+                            ret = SCR2_TOBCD(info->tm_sec);
+                            break;
+                        case 0x21:
+                            ret = SCR2_TOBCD(info->tm_min);
+                            break;
+                        case 0x22:
+                            ret = SCR2_TOBCD(info->tm_hour);
+                            break;
+                        case 0x24:
+                            ret = SCR2_TOBCD(info->tm_mday);
+                            break;
+                        case 0x25:
+                            ret = SCR2_TOBCD((info->tm_mon + 1));
+                            break;
+                        case 0x26:
+                            ret = SCR2_TOBCD((info->tm_year - 100));
+                            break;
+                        }
+
+                        if (ret & (0x80 >> (rtc->phase - 8))) {
+                            scr2_2 |= SCR2_RTDATA;
+                        }
+                        rtc->retval = (rtc->retval << 1) |
+                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
                     }
-                    rtc->retval = (rtc->retval << 1) |
-                                  ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
                 }
-
             }
 
             rtc->phase++;
-- 
2.39.5

[PATCH v2 21/34] next-cube: always use retval to return rtc read values

[Mark Cave-Ayland]

Instead of shifting out rtc read values from individual rtc registers, change
the logic so that rtc read commands are executed when the last bit of the rtc
command is received and the result stored in retval. This simplifies the rtc
read logic such that the shift out logic can be consolidated for rtc phases
between 8 and 16.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 99 ++++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 59 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index cc3dcad4e6..b5abc2b822 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -190,93 +190,74 @@ static void next_scr2_rtc_update(NeXTPC *s)
             if (rtc->phase < 8) {
                 rtc->command = (rtc->command << 1) |
                                ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-            }
-            if (rtc->phase >= 8 && rtc->phase < 16) {
-                if (next_rtc_cmd_is_write(rtc->command)) {
-                    /* Shift in value to write */
-                    rtc->value = (rtc->value << 1) |
-                                 ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                } else {
-                    /* Shift out value to read */
 
-                    /* if we read RAM register, output RT_DATA bit */
-                    if (rtc->command <= 0x1F) {
-                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                        if (rtc->ram[rtc->command] &
-                            (0x80 >> (rtc->phase - 8))) {
-                                scr2_2 |= SCR2_RTDATA;
-                        }
-
-                        rtc->retval = (rtc->retval << 1) |
-                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                    }
-                    /* read the status 0x30 */
-                    if (rtc->command == 0x30) {
-                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                        /* for now status = 0x98 (new rtc + FTU) */
-                        if (rtc->status & (0x80 >> (rtc->phase - 8))) {
-                            scr2_2 |= SCR2_RTDATA;
-                        }
-
-                        rtc->retval = (rtc->retval << 1) |
-                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                    }
-                    /* read the status 0x31 */
-                    if (rtc->command == 0x31) {
-                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                        if (rtc->control & (0x80 >> (rtc->phase - 8))) {
-                            scr2_2 |= SCR2_RTDATA;
-                        }
-                        rtc->retval = (rtc->retval << 1) |
-                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
+                if (rtc->phase == 7 && !next_rtc_cmd_is_write(rtc->command)) {
+                    if (rtc->command <= 0x1f) {
+                        /* RAM registers */
+                        rtc->retval = rtc->ram[rtc->command];
                     }
-
                     if ((rtc->command >= 0x20) && (rtc->command <= 0x2F)) {
-                        scr2_2 = scr2_2 & (~SCR2_RTDATA);
-                        /* for now 0x00 */
+                        /* RTC */
                         time_t time_h = time(NULL);
                         struct tm *info = localtime(&time_h);
-                        int ret = 0;
+                        rtc->retval = 0;
 
                         switch (rtc->command) {
                         case 0x20:
-                            ret = SCR2_TOBCD(info->tm_sec);
+                            rtc->retval = SCR2_TOBCD(info->tm_sec);
                             break;
                         case 0x21:
-                            ret = SCR2_TOBCD(info->tm_min);
+                            rtc->retval = SCR2_TOBCD(info->tm_min);
                             break;
                         case 0x22:
-                            ret = SCR2_TOBCD(info->tm_hour);
+                            rtc->retval = SCR2_TOBCD(info->tm_hour);
                             break;
                         case 0x24:
-                            ret = SCR2_TOBCD(info->tm_mday);
+                            rtc->retval = SCR2_TOBCD(info->tm_mday);
                             break;
                         case 0x25:
-                            ret = SCR2_TOBCD((info->tm_mon + 1));
+                            rtc->retval = SCR2_TOBCD((info->tm_mon + 1));
                             break;
                         case 0x26:
-                            ret = SCR2_TOBCD((info->tm_year - 100));
+                            rtc->retval = SCR2_TOBCD((info->tm_year - 100));
                             break;
                         }
-
-                        if (ret & (0x80 >> (rtc->phase - 8))) {
-                            scr2_2 |= SCR2_RTDATA;
-                        }
-                        rtc->retval = (rtc->retval << 1) |
-                                      ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
+                    }
+                    if (rtc->command == 0x30) {
+                        /* read the status 0x30 */
+                        rtc->retval = rtc->status;
+                    }
+                    if (rtc->command == 0x31) {
+                        /* read the control 0x31 */
+                        rtc->retval = rtc->control;
+                    }
+                }
+            }
+            if (rtc->phase >= 8 && rtc->phase < 16) {
+                if (next_rtc_cmd_is_write(rtc->command)) {
+                    /* Shift in value to write */
+                    rtc->value = (rtc->value << 1) |
+                                 ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
+                } else {
+                    /* Shift out value to read */
+                    if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
+                        scr2_2 |= SCR2_RTDATA;
+                    } else {
+                        scr2_2 &= ~SCR2_RTDATA;
                     }
                 }
             }
 
             rtc->phase++;
-            if (rtc->phase == 16) {
-                if (rtc->command >= 0x80 && rtc->command <= 0x9F) {
+            if (rtc->phase == 16 && next_rtc_cmd_is_write(rtc->command)) {
+                if (rtc->command >= 0x80 && rtc->command <= 0x9f) {
+                    /* RAM registers */
                     rtc->ram[rtc->command - 0x80] = rtc->value;
                 }
-                /* write to x30 register */
-                if (rtc->command == 0xB1) {
-                    /* clear FTU */
+                if (rtc->command == 0xb1) {
+                    /* write to 0x30 register */
                     if (rtc->value & 0x04) {
+                        /* clear FTU */
                         rtc->status = rtc->status & (~0x18);
                         qemu_irq_lower(s->rtc_power_irq);
                     }
-- 
2.39.5

[PATCH v2 22/34] next-cube: use named gpio to set RTC data bit in scr2

[Mark Cave-Ayland]

This is in preparation for moving NeXTRTC to its own separate device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index b5abc2b822..89dd9f5b85 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -240,10 +240,13 @@ static void next_scr2_rtc_update(NeXTPC *s)
                                  ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
                 } else {
                     /* Shift out value to read */
+                    qemu_irq rtc_data_in_irq = qdev_get_gpio_in_named(
+                        DEVICE(s), "pc-rtc-data-in", 0);
+
                     if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
-                        scr2_2 |= SCR2_RTDATA;
+                        qemu_irq_raise(rtc_data_in_irq);
                     } else {
-                        scr2_2 &= ~SCR2_RTDATA;
+                        qemu_irq_lower(rtc_data_in_irq);
                     }
                 }
             }
@@ -270,8 +273,6 @@ static void next_scr2_rtc_update(NeXTPC *s)
         rtc->command = 0;
         rtc->value = 0;
     }
-
-    s->scr2 = deposit32(s->scr2, 8, 8, scr2_2);
 }
 
 static uint64_t next_mmio_read(void *opaque, hwaddr addr, unsigned size)
@@ -1001,6 +1002,20 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void next_pc_rtc_data_in_irq(void *opaque, int n, int level)
+{
+    NeXTPC *s = NEXT_PC(opaque);
+    uint8_t scr2_2 = extract32(s->scr2, 8, 8);
+
+    if (level) {
+        scr2_2 |= SCR2_RTDATA;
+    } else {
+        scr2_2 &= ~SCR2_RTDATA;
+    }
+
+    s->scr2 = deposit32(s->scr2, 8, 8, scr2_2);
+}
+
 static void next_pc_reset_hold(Object *obj, ResetType type)
 {
     NeXTPC *s = NEXT_PC(obj);
@@ -1087,6 +1102,8 @@ static void next_pc_init(Object *obj)
     sysbus_init_mmio(sbd, &s->timer_mem);
 
     s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
+    qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
+                            "pc-rtc-data-in", 1);
 }
 
 /*
-- 
2.39.5

[PATCH v2 23/34] next-cube: use named gpio to read RTC data bit in scr2

[Mark Cave-Ayland]

This is in preparation for moving NeXTRTC to its own separate device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 169 ++++++++++++++++++++++++--------------------
 1 file changed, 92 insertions(+), 77 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 89dd9f5b85..e13f46add2 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -171,6 +171,90 @@ static bool next_rtc_cmd_is_write(uint8_t cmd)
            (cmd == 0xb1);
 }
 
+static void next_rtc_data_in_irq(void *opaque, int n, int level)
+{
+    NeXTPC *s = NEXT_PC(opaque);
+    NeXTRTC *rtc = &s->rtc;
+
+    if (rtc->phase < 8) {
+        rtc->command = (rtc->command << 1) | level;
+
+        if (rtc->phase == 7 && !next_rtc_cmd_is_write(rtc->command)) {
+            if (rtc->command <= 0x1f) {
+                /* RAM registers */
+                rtc->retval = rtc->ram[rtc->command];
+            }
+            if ((rtc->command >= 0x20) && (rtc->command <= 0x2f)) {
+                /* RTC */
+                time_t time_h = time(NULL);
+                struct tm *info = localtime(&time_h);
+                rtc->retval = 0;
+
+                switch (rtc->command) {
+                case 0x20:
+                    rtc->retval = SCR2_TOBCD(info->tm_sec);
+                    break;
+                case 0x21:
+                    rtc->retval = SCR2_TOBCD(info->tm_min);
+                    break;
+                case 0x22:
+                    rtc->retval = SCR2_TOBCD(info->tm_hour);
+                    break;
+                case 0x24:
+                    rtc->retval = SCR2_TOBCD(info->tm_mday);
+                    break;
+                case 0x25:
+                    rtc->retval = SCR2_TOBCD((info->tm_mon + 1));
+                    break;
+                case 0x26:
+                    rtc->retval = SCR2_TOBCD((info->tm_year - 100));
+                    break;
+                }
+            }
+            if (rtc->command == 0x30) {
+                /* read the status 0x30 */
+                rtc->retval = rtc->status;
+            }
+            if (rtc->command == 0x31) {
+                /* read the control 0x31 */
+                rtc->retval = rtc->control;
+            }
+        }
+    }
+    if (rtc->phase >= 8 && rtc->phase < 16) {
+        if (next_rtc_cmd_is_write(rtc->command)) {
+            /* Shift in value to write */
+            rtc->value = (rtc->value << 1) | level;
+        } else {
+            /* Shift out value to read */
+            qemu_irq rtc_data_in_irq = qdev_get_gpio_in_named(
+                DEVICE(s), "pc-rtc-data-in", 0);
+
+            if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
+                qemu_irq_raise(rtc_data_in_irq);
+            } else {
+                qemu_irq_lower(rtc_data_in_irq);
+            }
+        }
+    }
+
+    rtc->phase++;
+    if (rtc->phase == 16 && next_rtc_cmd_is_write(rtc->command)) {
+        if (rtc->command >= 0x80 && rtc->command <= 0x9f) {
+            /* RAM registers */
+            rtc->ram[rtc->command - 0x80] = rtc->value;
+        }
+        if (rtc->command == 0xb1) {
+            /* write to 0x30 register */
+            if (rtc->value & 0x04) {
+                /* clear FTU */
+                rtc->status = rtc->status & (~0x18);
+                qemu_irq_lower(s->rtc_power_irq);
+            }
+        }
+    }
+}
+
 static void next_scr2_rtc_update(NeXTPC *s)
 {
     uint8_t old_scr2, scr2_2;
@@ -187,84 +271,13 @@ static void next_scr2_rtc_update(NeXTPC *s)
         /* If we are in going down clock... do something */
         if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
                 ((scr2_2 & SCR2_RTCLK) == 0)) {
-            if (rtc->phase < 8) {
-                rtc->command = (rtc->command << 1) |
-                               ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-
-                if (rtc->phase == 7 && !next_rtc_cmd_is_write(rtc->command)) {
-                    if (rtc->command <= 0x1f) {
-                        /* RAM registers */
-                        rtc->retval = rtc->ram[rtc->command];
-                    }
-                    if ((rtc->command >= 0x20) && (rtc->command <= 0x2F)) {
-                        /* RTC */
-                        time_t time_h = time(NULL);
-                        struct tm *info = localtime(&time_h);
-                        rtc->retval = 0;
-
-                        switch (rtc->command) {
-                        case 0x20:
-                            rtc->retval = SCR2_TOBCD(info->tm_sec);
-                            break;
-                        case 0x21:
-                            rtc->retval = SCR2_TOBCD(info->tm_min);
-                            break;
-                        case 0x22:
-                            rtc->retval = SCR2_TOBCD(info->tm_hour);
-                            break;
-                        case 0x24:
-                            rtc->retval = SCR2_TOBCD(info->tm_mday);
-                            break;
-                        case 0x25:
-                            rtc->retval = SCR2_TOBCD((info->tm_mon + 1));
-                            break;
-                        case 0x26:
-                            rtc->retval = SCR2_TOBCD((info->tm_year - 100));
-                            break;
-                        }
-                    }
-                    if (rtc->command == 0x30) {
-                        /* read the status 0x30 */
-                        rtc->retval = rtc->status;
-                    }
-                    if (rtc->command == 0x31) {
-                        /* read the control 0x31 */
-                        rtc->retval = rtc->control;
-                    }
-                }
-            }
-            if (rtc->phase >= 8 && rtc->phase < 16) {
-                if (next_rtc_cmd_is_write(rtc->command)) {
-                    /* Shift in value to write */
-                    rtc->value = (rtc->value << 1) |
-                                 ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
-                } else {
-                    /* Shift out value to read */
-                    qemu_irq rtc_data_in_irq = qdev_get_gpio_in_named(
-                        DEVICE(s), "pc-rtc-data-in", 0);
-
-                    if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
-                        qemu_irq_raise(rtc_data_in_irq);
-                    } else {
-                        qemu_irq_lower(rtc_data_in_irq);
-                    }
-                }
-            }
+            qemu_irq rtc_data_in_irq = qdev_get_gpio_in_named(
+                DEVICE(s), "rtc-data-in", 0);
 
-            rtc->phase++;
-            if (rtc->phase == 16 && next_rtc_cmd_is_write(rtc->command)) {
-                if (rtc->command >= 0x80 && rtc->command <= 0x9f) {
-                    /* RAM registers */
-                    rtc->ram[rtc->command - 0x80] = rtc->value;
-                }
-                if (rtc->command == 0xb1) {
-                    /* write to 0x30 register */
-                    if (rtc->value & 0x04) {
-                        /* clear FTU */
-                        rtc->status = rtc->status & (~0x18);
-                        qemu_irq_lower(s->rtc_power_irq);
-                    }
-                }
+            if (scr2_2 & SCR2_RTDATA) {
+                qemu_irq_raise(rtc_data_in_irq);
+            } else {
+                qemu_irq_lower(rtc_data_in_irq);
             }
         }
     } else {
@@ -1104,6 +1117,8 @@ static void next_pc_init(Object *obj)
     s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
     qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
                             "pc-rtc-data-in", 1);
+    qdev_init_gpio_in_named(DEVICE(obj), next_rtc_data_in_irq,
+                            "rtc-data-in", 1);
 }
 
 /*
-- 
2.39.5

[PATCH v2 24/34] next-cube: don't use rtc phase value of -1

[Mark Cave-Ayland]

The rtc phase value of -1 is directly equivalent to using a phase value of 0 so
simplify the logic to use an initial rtc phase of 0.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index e13f46add2..9c91ee146a 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -265,9 +265,6 @@ static void next_scr2_rtc_update(NeXTPC *s)
 
     if (scr2_2 & 0x1) {
         /* DPRINTF("RTC %x phase %i\n", scr2_2, rtc->phase); */
-        if (rtc->phase == -1) {
-            rtc->phase = 0;
-        }
         /* If we are in going down clock... do something */
         if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
                 ((scr2_2 & SCR2_RTCLK) == 0)) {
@@ -282,7 +279,7 @@ static void next_scr2_rtc_update(NeXTPC *s)
         }
     } else {
         /* else end or abort */
-        rtc->phase = -1;
+        rtc->phase = 0;
         rtc->command = 0;
         rtc->value = 0;
     }
-- 
2.39.5

[PATCH v2 25/34] next-cube: QOMify NeXTRTC

[Mark Cave-Ayland]

This is to allow the RTC functionality to be maintained within its own separate
device rather than as part of the next-pc device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 71 +++++++++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 9c91ee146a..c947af65e2 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -42,7 +42,13 @@
 #define RAM_SIZE    0x4000000
 #define ROM_FILE    "Rev_2.5_v66.bin"
 
-typedef struct NeXTRTC {
+
+#define TYPE_NEXT_RTC "next-rtc"
+OBJECT_DECLARE_SIMPLE_TYPE(NeXTRTC, NEXT_RTC)
+
+struct NeXTRTC {
+    SysBusDevice parent_obj;
+
     int8_t phase;
     uint8_t ram[32];
     uint8_t command;
@@ -50,7 +56,7 @@ typedef struct NeXTRTC {
     uint8_t status;
     uint8_t control;
     uint8_t retval;
-} NeXTRTC;
+};
 
 #define TYPE_NEXT_SCSI "next-scsi"
 OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
@@ -1012,6 +1018,37 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static const VMStateDescription next_rtc_vmstate = {
+    .name = "next-rtc",
+    .version_id = 3,
+    .minimum_version_id = 3,
+    .fields = (const VMStateField[]) {
+        VMSTATE_INT8(phase, NeXTRTC),
+        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
+        VMSTATE_UINT8(command, NeXTRTC),
+        VMSTATE_UINT8(value, NeXTRTC),
+        VMSTATE_UINT8(status, NeXTRTC),
+        VMSTATE_UINT8(control, NeXTRTC),
+        VMSTATE_UINT8(retval, NeXTRTC),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void next_rtc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "NeXT RTC";
+    dc->vmsd = &next_rtc_vmstate;
+}
+
+static const TypeInfo next_rtc_info = {
+    .name = TYPE_NEXT_RTC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(NeXTRTC),
+    .class_init = next_rtc_class_init,
+};
+
 static void next_pc_rtc_data_in_irq(void *opaque, int n, int level)
 {
     NeXTPC *s = NEXT_PC(opaque);
@@ -1078,6 +1115,12 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     }
     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));
+
+    /* RTC */
+    d = DEVICE(object_resolve_path_component(OBJECT(dev), "rtc"));
+    if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
+        return;
+    }
 }
 
 static void next_pc_init(Object *obj)
@@ -1111,6 +1154,8 @@ static void next_pc_init(Object *obj)
                           "next.timer", 4);
     sysbus_init_mmio(sbd, &s->timer_mem);
 
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_NEXT_RTC);
+
     s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
     qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
                             "pc-rtc-data-in", 1);
@@ -1129,26 +1174,10 @@ static Property next_pc_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static const VMStateDescription next_rtc_vmstate = {
-    .name = "next-rtc",
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .fields = (const VMStateField[]) {
-        VMSTATE_INT8(phase, NeXTRTC),
-        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
-        VMSTATE_UINT8(command, NeXTRTC),
-        VMSTATE_UINT8(value, NeXTRTC),
-        VMSTATE_UINT8(status, NeXTRTC),
-        VMSTATE_UINT8(control, NeXTRTC),
-        VMSTATE_UINT8(retval, NeXTRTC),
-        VMSTATE_END_OF_LIST()
-    },
-};
-
 static const VMStateDescription next_pc_vmstate = {
     .name = "next-pc",
-    .version_id = 3,
-    .minimum_version_id = 3,
+    .version_id = 4,
+    .minimum_version_id = 4,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(scr1, NeXTPC),
         VMSTATE_UINT32(scr2, NeXTPC),
@@ -1156,7 +1185,6 @@ static const VMStateDescription next_pc_vmstate = {
         VMSTATE_UINT32(int_mask, NeXTPC),
         VMSTATE_UINT32(int_status, NeXTPC),
         VMSTATE_UINT32(led, NeXTPC),
-        VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NeXTRTC),
         VMSTATE_END_OF_LIST()
     },
 };
@@ -1305,6 +1333,7 @@ static void next_register_type(void)
     type_register_static(&next_typeinfo);
     type_register_static(&next_pc_info);
     type_register_static(&next_scsi_info);
+    type_register_static(&next_rtc_info);
 }
 
 type_init(next_register_type)
-- 
2.39.5

[PATCH v2 26/34] next-cube: move reset of next-rtc fields from next-pc to next-rtc

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index c947af65e2..a7a8b3cbbf 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -1018,6 +1018,16 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void next_rtc_reset_hold(Object *obj, ResetType type)
+{
+    NeXTRTC *rtc = NEXT_RTC(obj);
+
+    rtc->status = 0x90;
+
+    /* Load RTC RAM - TODO: provide possibility to load contents from file */
+    memcpy(rtc->ram, rtc_ram2, 32);
+}
+
 static const VMStateDescription next_rtc_vmstate = {
     .name = "next-rtc",
     .version_id = 3,
@@ -1037,9 +1047,11 @@ static const VMStateDescription next_rtc_vmstate = {
 static void next_rtc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     dc->desc = "NeXT RTC";
     dc->vmsd = &next_rtc_vmstate;
+    rc->phases.hold = next_rtc_reset_hold;
 }
 
 static const TypeInfo next_rtc_info = {
@@ -1072,11 +1084,6 @@ static void next_pc_reset_hold(Object *obj, ResetType type)
     s->scr1 = 0x00011102;
     s->scr2 = 0x00ff0c80;
     s->old_scr2 = s->scr2;
-
-    s->rtc.status = 0x90;
-
-    /* Load RTC RAM - TODO: provide possibility to load contents from file */
-    memcpy(s->rtc.ram, rtc_ram2, 32);
 }
 
 static void next_pc_realize(DeviceState *dev, Error **errp)
-- 
2.39.5

[PATCH v2 27/34] next-cube: move rtc-data-in gpio from next-pc to next-rtc device

[Mark Cave-Ayland]

Add a new rtc-data-out gpio to the next-pc device and wire it up to the next-rtc
rtc-data-in gpio using the standard qdev gpio APIs.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index a7a8b3cbbf..35eeeb0d59 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -105,6 +105,7 @@ struct NeXTPC {
 
     NeXTRTC rtc;
     qemu_irq rtc_power_irq;
+    qemu_irq rtc_data_irq;
 };
 
 typedef struct next_dma {
@@ -179,8 +180,8 @@ static bool next_rtc_cmd_is_write(uint8_t cmd)
 
 static void next_rtc_data_in_irq(void *opaque, int n, int level)
 {
-    NeXTPC *s = NEXT_PC(opaque);
-    NeXTRTC *rtc = &s->rtc;
+    NeXTRTC *rtc = NEXT_RTC(opaque);
+    NeXTPC *s = NEXT_PC(container_of(rtc, NeXTPC, rtc));
 
     if (rtc->phase < 8) {
         rtc->command = (rtc->command << 1) | level;
@@ -274,13 +275,10 @@ static void next_scr2_rtc_update(NeXTPC *s)
         /* If we are in going down clock... do something */
         if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
                 ((scr2_2 & SCR2_RTCLK) == 0)) {
-            qemu_irq rtc_data_in_irq = qdev_get_gpio_in_named(
-                DEVICE(s), "rtc-data-in", 0);
-
             if (scr2_2 & SCR2_RTDATA) {
-                qemu_irq_raise(rtc_data_in_irq);
+                qemu_irq_raise(s->rtc_data_irq);
             } else {
-                qemu_irq_lower(rtc_data_in_irq);
+                qemu_irq_lower(s->rtc_data_irq);
             }
         }
     } else {
@@ -1028,6 +1026,12 @@ static void next_rtc_reset_hold(Object *obj, ResetType type)
     memcpy(rtc->ram, rtc_ram2, 32);
 }
 
+static void next_rtc_init(Object *obj)
+{
+    qdev_init_gpio_in_named(DEVICE(obj), next_rtc_data_in_irq,
+                            "rtc-data-in", 1);
+}
+
 static const VMStateDescription next_rtc_vmstate = {
     .name = "next-rtc",
     .version_id = 3,
@@ -1057,6 +1061,7 @@ static void next_rtc_class_init(ObjectClass *klass, void *data)
 static const TypeInfo next_rtc_info = {
     .name = TYPE_NEXT_RTC,
     .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = next_rtc_init,
     .instance_size = sizeof(NeXTRTC),
     .class_init = next_rtc_class_init,
 };
@@ -1128,6 +1133,9 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
         return;
     }
+    /* Data from NeXTPC to RTC */
+    qdev_connect_gpio_out_named(dev, "rtc-data-out", 0,
+                                qdev_get_gpio_in_named(d, "rtc-data-in", 0));
 }
 
 static void next_pc_init(Object *obj)
@@ -1166,8 +1174,8 @@ static void next_pc_init(Object *obj)
     s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
     qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
                             "pc-rtc-data-in", 1);
-    qdev_init_gpio_in_named(DEVICE(obj), next_rtc_data_in_irq,
-                            "rtc-data-in", 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->rtc_data_irq,
+                             "rtc-data-out", 1);
 }
 
 /*
-- 
2.39.5

[PATCH v2 28/34] next-cube: use named gpio output for next-rtc data

[Mark Cave-Ayland]

Add a named gpio output for the next-rtc data and then update
next_rtc_data_in_irq() to drive the IRQ directly. This enables the next-rtc to
next-pc data to be wired up using the standard qdev gpio APIs.

At the same time rename the pc-rtc-data-in gpio to rtc-data-in which is possible
now that the previous rtc-data-in gpio has been moved to the next-rtc device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 35eeeb0d59..5d3ffebb93 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -56,6 +56,8 @@ struct NeXTRTC {
     uint8_t status;
     uint8_t control;
     uint8_t retval;
+
+    qemu_irq data_out_irq;
 };
 
 #define TYPE_NEXT_SCSI "next-scsi"
@@ -234,13 +236,10 @@ static void next_rtc_data_in_irq(void *opaque, int n, int level)
             rtc->value = (rtc->value << 1) | level;
         } else {
             /* Shift out value to read */
-            qemu_irq rtc_data_in_irq = qdev_get_gpio_in_named(
-                DEVICE(s), "pc-rtc-data-in", 0);
-
             if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
-                qemu_irq_raise(rtc_data_in_irq);
+                qemu_irq_raise(rtc->data_out_irq);
             } else {
-                qemu_irq_lower(rtc_data_in_irq);
+                qemu_irq_lower(rtc->data_out_irq);
             }
         }
     }
@@ -1028,8 +1027,12 @@ static void next_rtc_reset_hold(Object *obj, ResetType type)
 
 static void next_rtc_init(Object *obj)
 {
+    NeXTRTC *rtc = NEXT_RTC(obj);
+
     qdev_init_gpio_in_named(DEVICE(obj), next_rtc_data_in_irq,
                             "rtc-data-in", 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &rtc->data_out_irq,
+                             "rtc-data-out", 1);
 }
 
 static const VMStateDescription next_rtc_vmstate = {
@@ -1136,6 +1139,10 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     /* Data from NeXTPC to RTC */
     qdev_connect_gpio_out_named(dev, "rtc-data-out", 0,
                                 qdev_get_gpio_in_named(d, "rtc-data-in", 0));
+    /* Data from RTC to NeXTPC */
+    qdev_connect_gpio_out_named(d, "rtc-data-out", 0,
+                                qdev_get_gpio_in_named(dev,
+                                                       "rtc-data-in", 0));
 }
 
 static void next_pc_init(Object *obj)
@@ -1173,7 +1180,7 @@ static void next_pc_init(Object *obj)
 
     s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
     qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
-                            "pc-rtc-data-in", 1);
+                            "rtc-data-in", 1);
     qdev_init_gpio_out_named(DEVICE(obj), &s->rtc_data_irq,
                              "rtc-data-out", 1);
 }
-- 
2.39.5

[PATCH v2 29/34] next-cube: add rtc-cmd-reset named gpio to reset the rtc state machine

[Mark Cave-Ayland]

This allows us to decouple the next-pc and next-rtc devices from each
other in next_scr2_rtc_update().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 5d3ffebb93..d61deda6e0 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -108,6 +108,7 @@ struct NeXTPC {
     NeXTRTC rtc;
     qemu_irq rtc_power_irq;
     qemu_irq rtc_data_irq;
+    qemu_irq rtc_cmd_reset_irq;
 };
 
 typedef struct next_dma {
@@ -264,7 +265,6 @@ static void next_rtc_data_in_irq(void *opaque, int n, int level)
 static void next_scr2_rtc_update(NeXTPC *s)
 {
     uint8_t old_scr2, scr2_2;
-    NeXTRTC *rtc = &s->rtc;
 
     old_scr2 = extract32(s->old_scr2, 8, 8);
     scr2_2 = extract32(s->scr2, 8, 8);
@@ -282,9 +282,7 @@ static void next_scr2_rtc_update(NeXTPC *s)
         }
     } else {
         /* else end or abort */
-        rtc->phase = 0;
-        rtc->command = 0;
-        rtc->value = 0;
+        qemu_irq_raise(s->rtc_cmd_reset_irq);
     }
 }
 
@@ -1015,6 +1013,17 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static void next_rtc_cmd_reset_irq(void *opaque, int n, int level)
+{
+    NeXTRTC *rtc = NEXT_RTC(opaque);
+
+    if (level) {
+        rtc->phase = 0;
+        rtc->command = 0;
+        rtc->value = 0;
+    }
+}
+
 static void next_rtc_reset_hold(Object *obj, ResetType type)
 {
     NeXTRTC *rtc = NEXT_RTC(obj);
@@ -1033,6 +1042,8 @@ static void next_rtc_init(Object *obj)
                             "rtc-data-in", 1);
     qdev_init_gpio_out_named(DEVICE(obj), &rtc->data_out_irq,
                              "rtc-data-out", 1);
+    qdev_init_gpio_in_named(DEVICE(obj), next_rtc_cmd_reset_irq,
+                            "rtc-cmd-reset", 1);
 }
 
 static const VMStateDescription next_rtc_vmstate = {
@@ -1143,6 +1154,8 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     qdev_connect_gpio_out_named(d, "rtc-data-out", 0,
                                 qdev_get_gpio_in_named(dev,
                                                        "rtc-data-in", 0));
+    qdev_connect_gpio_out_named(dev, "rtc-cmd-reset", 0,
+                                qdev_get_gpio_in_named(d, "rtc-cmd-reset", 0));
 }
 
 static void next_pc_init(Object *obj)
@@ -1183,6 +1196,8 @@ static void next_pc_init(Object *obj)
                             "rtc-data-in", 1);
     qdev_init_gpio_out_named(DEVICE(obj), &s->rtc_data_irq,
                              "rtc-data-out", 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->rtc_cmd_reset_irq,
+                             "rtc-cmd-reset", 1);
 }
 
 /*
-- 
2.39.5

[PATCH v2 30/34] next-cube: add rtc-power-out named gpio to trigger the NEXT_PWR_I interrupt

[Mark Cave-Ayland]

This allows us to decouple the next-pc and next-rtc devices from each
other in next_rtc_data_in_irq().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index d61deda6e0..94a9b90171 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -58,6 +58,7 @@ struct NeXTRTC {
     uint8_t retval;
 
     qemu_irq data_out_irq;
+    qemu_irq power_irq;
 };
 
 #define TYPE_NEXT_SCSI "next-scsi"
@@ -106,7 +107,6 @@ struct NeXTPC {
     ESCCState escc;
 
     NeXTRTC rtc;
-    qemu_irq rtc_power_irq;
     qemu_irq rtc_data_irq;
     qemu_irq rtc_cmd_reset_irq;
 };
@@ -184,7 +184,6 @@ static bool next_rtc_cmd_is_write(uint8_t cmd)
 static void next_rtc_data_in_irq(void *opaque, int n, int level)
 {
     NeXTRTC *rtc = NEXT_RTC(opaque);
-    NeXTPC *s = NEXT_PC(container_of(rtc, NeXTPC, rtc));
 
     if (rtc->phase < 8) {
         rtc->command = (rtc->command << 1) | level;
@@ -256,7 +255,7 @@ static void next_rtc_data_in_irq(void *opaque, int n, int level)
             if (rtc->value & 0x04) {
                 /* clear FTU */
                 rtc->status = rtc->status & (~0x18);
-                qemu_irq_lower(s->rtc_power_irq);
+                qemu_irq_lower(rtc->power_irq);
             }
         }
     }
@@ -1044,6 +1043,8 @@ static void next_rtc_init(Object *obj)
                              "rtc-data-out", 1);
     qdev_init_gpio_in_named(DEVICE(obj), next_rtc_cmd_reset_irq,
                             "rtc-cmd-reset", 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &rtc->power_irq,
+                             "rtc-power-out", 1);
 }
 
 static const VMStateDescription next_rtc_vmstate = {
@@ -1156,6 +1157,8 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
                                                        "rtc-data-in", 0));
     qdev_connect_gpio_out_named(dev, "rtc-cmd-reset", 0,
                                 qdev_get_gpio_in_named(d, "rtc-cmd-reset", 0));
+    qdev_connect_gpio_out_named(d, "rtc-power-out", 0,
+                                qdev_get_gpio_in(dev, NEXT_PWR_I));
 }
 
 static void next_pc_init(Object *obj)
@@ -1191,7 +1194,6 @@ static void next_pc_init(Object *obj)
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_NEXT_RTC);
 
-    s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
     qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
                             "rtc-data-in", 1);
     qdev_init_gpio_out_named(DEVICE(obj), &s->rtc_data_irq,
-- 
2.39.5

[PATCH v2 31/34] next-cube: move next_rtc_cmd_is_write() and next_rtc_data_in_irq() functions

[Mark Cave-Ayland]

Move these functions in next-cube.c so that they are with the rest of the
next-rtc functions.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 172 ++++++++++++++++++++++----------------------
 1 file changed, 86 insertions(+), 86 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 94a9b90171..12d31dfaad 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -175,92 +175,6 @@ static void next_scr2_led_update(NeXTPC *s)
     }
 }
 
-static bool next_rtc_cmd_is_write(uint8_t cmd)
-{
-    return (cmd >= 0x80 && cmd <= 0x9f) ||
-           (cmd == 0xb1);
-}
-
-static void next_rtc_data_in_irq(void *opaque, int n, int level)
-{
-    NeXTRTC *rtc = NEXT_RTC(opaque);
-
-    if (rtc->phase < 8) {
-        rtc->command = (rtc->command << 1) | level;
-
-        if (rtc->phase == 7 && !next_rtc_cmd_is_write(rtc->command)) {
-            if (rtc->command <= 0x1f) {
-                /* RAM registers */
-                rtc->retval = rtc->ram[rtc->command];
-            }
-            if ((rtc->command >= 0x20) && (rtc->command <= 0x2f)) {
-                /* RTC */
-                time_t time_h = time(NULL);
-                struct tm *info = localtime(&time_h);
-                rtc->retval = 0;
-
-                switch (rtc->command) {
-                case 0x20:
-                    rtc->retval = SCR2_TOBCD(info->tm_sec);
-                    break;
-                case 0x21:
-                    rtc->retval = SCR2_TOBCD(info->tm_min);
-                    break;
-                case 0x22:
-                    rtc->retval = SCR2_TOBCD(info->tm_hour);
-                    break;
-                case 0x24:
-                    rtc->retval = SCR2_TOBCD(info->tm_mday);
-                    break;
-                case 0x25:
-                    rtc->retval = SCR2_TOBCD((info->tm_mon + 1));
-                    break;
-                case 0x26:
-                    rtc->retval = SCR2_TOBCD((info->tm_year - 100));
-                    break;
-                }
-            }
-            if (rtc->command == 0x30) {
-                /* read the status 0x30 */
-                rtc->retval = rtc->status;
-            }
-            if (rtc->command == 0x31) {
-                /* read the control 0x31 */
-                rtc->retval = rtc->control;
-            }
-        }
-    }
-    if (rtc->phase >= 8 && rtc->phase < 16) {
-        if (next_rtc_cmd_is_write(rtc->command)) {
-            /* Shift in value to write */
-            rtc->value = (rtc->value << 1) | level;
-        } else {
-            /* Shift out value to read */
-            if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
-                qemu_irq_raise(rtc->data_out_irq);
-            } else {
-                qemu_irq_lower(rtc->data_out_irq);
-            }
-        }
-    }
-
-    rtc->phase++;
-    if (rtc->phase == 16 && next_rtc_cmd_is_write(rtc->command)) {
-        if (rtc->command >= 0x80 && rtc->command <= 0x9f) {
-            /* RAM registers */
-            rtc->ram[rtc->command - 0x80] = rtc->value;
-        }
-        if (rtc->command == 0xb1) {
-            /* write to 0x30 register */
-            if (rtc->value & 0x04) {
-                /* clear FTU */
-                rtc->status = rtc->status & (~0x18);
-                qemu_irq_lower(rtc->power_irq);
-            }
-        }
-    }
-}
-
 static void next_scr2_rtc_update(NeXTPC *s)
 {
     uint8_t old_scr2, scr2_2;
@@ -1012,6 +926,92 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static bool next_rtc_cmd_is_write(uint8_t cmd)
+{
+    return (cmd >= 0x80 && cmd <= 0x9f) ||
+           (cmd == 0xb1);
+}
+
+static void next_rtc_data_in_irq(void *opaque, int n, int level)
+{
+    NeXTRTC *rtc = NEXT_RTC(opaque);
+
+    if (rtc->phase < 8) {
+        rtc->command = (rtc->command << 1) | level;
+
+        if (rtc->phase == 7 && !next_rtc_cmd_is_write(rtc->command)) {
+            if (rtc->command <= 0x1f) {
+                /* RAM registers */
+                rtc->retval = rtc->ram[rtc->command];
+            }
+            if ((rtc->command >= 0x20) && (rtc->command <= 0x2f)) {
+                /* RTC */
+                time_t time_h = time(NULL);
+                struct tm *info = localtime(&time_h);
+                rtc->retval = 0;
+
+                switch (rtc->command) {
+                case 0x20:
+                    rtc->retval = SCR2_TOBCD(info->tm_sec);
+                    break;
+                case 0x21:
+                    rtc->retval = SCR2_TOBCD(info->tm_min);
+                    break;
+                case 0x22:
+                    rtc->retval = SCR2_TOBCD(info->tm_hour);
+                    break;
+                case 0x24:
+                    rtc->retval = SCR2_TOBCD(info->tm_mday);
+                    break;
+                case 0x25:
+                    rtc->retval = SCR2_TOBCD((info->tm_mon + 1));
+                    break;
+                case 0x26:
+                    rtc->retval = SCR2_TOBCD((info->tm_year - 100));
+                    break;
+                }
+            }
+            if (rtc->command == 0x30) {
+                /* read the status 0x30 */
+                rtc->retval = rtc->status;
+            }
+            if (rtc->command == 0x31) {
+                /* read the control 0x31 */
+                rtc->retval = rtc->control;
+            }
+        }
+    }
+    if (rtc->phase >= 8 && rtc->phase < 16) {
+        if (next_rtc_cmd_is_write(rtc->command)) {
+            /* Shift in value to write */
+            rtc->value = (rtc->value << 1) | level;
+        } else {
+            /* Shift out value to read */
+            if (rtc->retval & (0x80 >> (rtc->phase - 8))) {
+                qemu_irq_raise(rtc->data_out_irq);
+            } else {
+                qemu_irq_lower(rtc->data_out_irq);
+            }
+        }
+    }
+
+    rtc->phase++;
+    if (rtc->phase == 16 && next_rtc_cmd_is_write(rtc->command)) {
+        if (rtc->command >= 0x80 && rtc->command <= 0x9f) {
+            /* RAM registers */
+            rtc->ram[rtc->command - 0x80] = rtc->value;
+        }
+        if (rtc->command == 0xb1) {
+            /* write to 0x30 register */
+            if (rtc->value & 0x04) {
+                /* clear FTU */
+                rtc->status = rtc->status & (~0x18);
+                qemu_irq_lower(rtc->power_irq);
+            }
+        }
+    }
+}
+
 static void next_rtc_cmd_reset_irq(void *opaque, int n, int level)
 {
     NeXTRTC *rtc = NEXT_RTC(opaque);
-- 
2.39.5

[PATCH v2 32/34] next-cube: rename old_scr2 and scr2_2 in next_scr2_rtc_update()

[Mark Cave-Ayland]

Rename them to old_scr2_rtc and scr2_rtc to reflect that they contain the previous
and current values of the SCR2 RTC bits.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 12d31dfaad..7b0769c0d3 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -177,17 +177,17 @@ static void next_scr2_led_update(NeXTPC *s)
 
 static void next_scr2_rtc_update(NeXTPC *s)
 {
-    uint8_t old_scr2, scr2_2;
+    uint8_t old_scr2_rtc, scr2_rtc;
 
-    old_scr2 = extract32(s->old_scr2, 8, 8);
-    scr2_2 = extract32(s->scr2, 8, 8);
+    old_scr2_rtc = extract32(s->old_scr2, 8, 8);
+    scr2_rtc = extract32(s->scr2, 8, 8);
 
-    if (scr2_2 & 0x1) {
+    if (scr2_rtc & 0x1) {
         /* DPRINTF("RTC %x phase %i\n", scr2_2, rtc->phase); */
         /* If we are in going down clock... do something */
-        if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
-                ((scr2_2 & SCR2_RTCLK) == 0)) {
-            if (scr2_2 & SCR2_RTDATA) {
+        if (((old_scr2_rtc & SCR2_RTCLK) != (scr2_rtc & SCR2_RTCLK)) &&
+                ((scr2_rtc & SCR2_RTCLK) == 0)) {
+            if (scr2_rtc & SCR2_RTDATA) {
                 qemu_irq_raise(s->rtc_data_irq);
             } else {
                 qemu_irq_lower(s->rtc_data_irq);
-- 
2.39.5

[PATCH v2 33/34] next-cube: add my copyright to the top of the file

[Mark Cave-Ayland]

This series has involved rewriting and/or updating a considerable part of the
next-cube emulation so update the copyright in next-cube.c to reflect this.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 7b0769c0d3..1e96bb02f8 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -2,6 +2,7 @@
  * NeXT Cube System Driver
  *
  * Copyright (c) 2011 Bryce Lanham
+ * Copyright (c) 2024 Mark Cave-Ayland
  *
  * This code is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published
-- 
2.39.5

[PATCH v2 34/34] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 1e96bb02f8..3c2f3e295c 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -4,10 +4,7 @@
  * Copyright (c) 2011 Bryce Lanham
  * Copyright (c) 2024 Mark Cave-Ayland
  *
- * This code is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published
- * by the Free Software Foundation; either version 2 of the License,
- * or (at your option) any later version.
+ * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
 #include "qemu/osdep.h"
-- 
2.39.5

Re: [PATCH v2 34/34] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Daniel P. Berrangé]

On Thu, Dec 12, 2024 at 11:46:20AM +0000, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 1e96bb02f8..3c2f3e295c 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -4,10 +4,7 @@
>   * Copyright (c) 2011 Bryce Lanham
>   * Copyright (c) 2024 Mark Cave-Ayland
>   *
> - * This code is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published
> - * by the Free Software Foundation; either version 2 of the License,
> - * or (at your option) any later version.
> + * SPDX-License-Identifier: GPL-2.0-or-later
>   */

While adding a SPDX-License-Identifier alongside existing header text
is acceptable, my view is that we should stay away from removing existing
license headers. There are some difficult questions wrt interpretation
of the GPL in this area & avoiding opening that can of worms would be
nice.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 02/34] next-cube: remove overlap between next.dma and next.mmio memory regions

[Thomas Huth]

 Hi Mark!

Am Thu, 12 Dec 2024 11:45:48 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> Change the start of the next.mmio memory region so that it follows on directly
> after the next.dma memory region. Increase the address offsets in
> next_mmio_read() and next_mmio_write(), and reduce the size of the next.mmio
> memory region accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/m68k/next-cube.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 0418fbc8aa..550e7f0b0a 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -266,23 +266,23 @@ static uint64_t next_mmio_read(void *opaque, hwaddr addr, unsigned size)
>      uint64_t val;
>  
>      switch (addr) {
> -    case 0x7000:
> +    case 0x2000:    /* 0x2005000 */
>          /* DPRINTF("Read INT status: %x\n", s->int_status); */
>          val = s->int_status;
>          break;
>  
> -    case 0x7800:
> +    case 0x2800:    /* 0x2007800 */
>          DPRINTF("MMIO Read INT mask: %x\n", s->int_mask);
>          val = s->int_mask;
>          break;
>  
> -    case 0xc000 ... 0xc003:
> -        val = extract32(s->scr1, (4 - (addr - 0xc000) - size) << 3,
> +    case 0x7000 ... 0x7003:    /* 0x200c000 */
> +        val = extract32(s->scr1, (4 - (addr - 0x7000) - size) << 3,
>                          size << 3);
>          break;
>  
> -    case 0xd000 ... 0xd003:
> -        val = extract32(s->scr2, (4 - (addr - 0xd000) - size) << 3,
> +    case 0x8000 ... 0x8003:    /* 0x200d000 */
> +        val = extract32(s->scr2, (4 - (addr - 0x8000) - size) << 3,
>                          size << 3);
>          break;
>  
> @@ -301,25 +301,25 @@ static void next_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>      NeXTPC *s = NEXT_PC(opaque);
>  
>      switch (addr) {
> -    case 0x7000:
> +    case 0x2000:    /* 0x2005000 */

Comment should be /* 0x2007000 */ instead.

With that fixed:

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 03/34] next-cube: create new next.scsi container memory region

[Thomas Huth]

Am Thu, 12 Dec 2024 11:45:49 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> Move the ESP SCSI and SCSI CSR registers to the new next.scsi container memory
> region.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 07/34] next-cube: move SCSI CSRs from next-pc to the next-scsi device

[Thomas Huth]

Am Thu, 12 Dec 2024 11:45:53 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> The SCSI CSRs are located within the SCSI subsystem of the NeXT PC (Peripheral
> Contoller) which is now modelled as a separate QEMU device. Add a new memory
> region subregion to contain the SCSI CSRs that simply store and retrieve the
> register values.
> 
> Add a new VMStateDescription for the next-scsi device to enable the SCSI CSRs
> to be migrated.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 88 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 78 insertions(+), 10 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 08/34] next-cube: move SCSI 4020/4021 logic from next-pc device to next-scsi device

[Thomas Huth]

Am Thu, 12 Dec 2024 11:45:54 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> The SCSI 4020/4021 logic refers to the offset of the SCSI CSRs within the NeXTCube
> address space. Due to the previously overlapping memory regions, there were
> duplicate MMIO accessors in the next.scr memory region for these registers but
> this has now been resolved.
> 
> Move the remaining SCSI 4020/4021 logic from the next-pc device to the next-scsi
> device, with the exception that the SCSI 4021 register now returns its previous
> value like a normal register instead of a hardcoded 0x40 value. This also matches
> how the registers are implemented in the Previous emulator.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 139 ++++++++++++++++++++------------------------
>  1 file changed, 62 insertions(+), 77 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 11/34] next-cube: move ESCC to be QOM child of next-pc device

[Thomas Huth]

Am Thu, 12 Dec 2024 11:45:57 +0000
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(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 25/34] next-cube: QOMify NeXTRTC

[Thomas Huth]

Am Thu, 12 Dec 2024 11:46:11 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> This is to allow the RTC functionality to be maintained within its own separate
> device rather than as part of the next-pc device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 71 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 9c91ee146a..c947af65e2 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
...
> @@ -1078,6 +1115,12 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>      }
>      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));
> +
> +    /* RTC */
> +    d = DEVICE(object_resolve_path_component(OBJECT(dev), "rtc"));
> +    if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
> +        return;
> +    }
>  }

Would it be easier to directly use s->rtc instead of taking the detour via
object_resolve_path_component?

 Thomas

Re: [PATCH v2 11/34] next-cube: move ESCC to be QOM child of next-pc device

[Thomas Huth]

Am Thu, 12 Dec 2024 11:45:57 +0000
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 efbb11e74b..20a0b073e1 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"));

Can't you use s->escc directly here instead of taking the detour via
object_resolve_path_component() ?

 Thomas

> +    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));
> +
>  }

Re: [PATCH v2 31/34] next-cube: move next_rtc_cmd_is_write() and next_rtc_data_in_irq() functions

[Thomas Huth]

Am Thu, 12 Dec 2024 11:46:17 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> Move these functions in next-cube.c so that they are with the rest of the
> next-rtc functions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 172 ++++++++++++++++++++++----------------------
>  1 file changed, 86 insertions(+), 86 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 03/34] next-cube: create new next.scsi container memory region

[Philippe Mathieu-Daudé]

On 12/12/24 12:45, Mark Cave-Ayland wrote:
> Move the ESP SCSI and SCSI CSR registers to the new next.scsi container memory
> region.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/next-cube.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 10/34] next-cube: map ESCC registers as a subregion of the next.scr memory region

[Philippe Mathieu-Daudé]

On 12/12/24 12:45, Mark Cave-Ayland wrote:
> Since the ESCC device exists within the memory range of the next.scr memory region, map
> the ESCC device registers as a subregion of the next.scr memory region instead of
> directly to the system address space.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/next-cube.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 11/34] next-cube: move ESCC to be QOM child of next-pc device

[Philippe Mathieu-Daudé]

On 14/12/24 07:10, Thomas Huth wrote:
> Am Thu, 12 Dec 2024 11:45:57 +0000
> 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 efbb11e74b..20a0b073e1 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;
>>   };

>>   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"));
> 
> Can't you use s->escc directly here instead of taking the detour via
> object_resolve_path_component() ?

Indeed.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 12/34] next-cube: move timer MMIO to separate memory region on next-pc device

[Philippe Mathieu-Daudé]

On 12/12/24 12:45, Mark Cave-Ayland wrote:
> Move the timer MMIO accesses to a separate memory region on the next-pc device
> instead of being part of the next.scr MMIO memory region.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/next-cube.c | 63 +++++++++++++++++++++++++++++++++++----------
>   1 file changed, 50 insertions(+), 13 deletions(-)


> +static uint64_t next_timer_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    uint64_t val;
> +
> +    switch (addr) {
> +    case 0 ... 3:
> +        /*
> +         * These 4 registers are the hardware timer, not sure which register
> +         * is the latch instead of data, but no problems so far.
> +         *
> +         * Hack: We need to have the LSB change consistently to make it work
> +         */
> +        val = extract32(clock(), (4 - addr - size) << 3,
> +                        size << 3);

Does this mean ...

> +        break;
> +
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return val;
> +}
> +
> +static const MemoryRegionOps next_timer_ops = {
> +    .read = next_timer_read,
> +    .write = next_timer_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_BIG_ENDIAN,

... this should be in little endianness?

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +};

Re: [PATCH v2 14/34] next-cube: add empty slots for unknown accesses to next.scr memory region

[Philippe Mathieu-Daudé]

On 12/12/24 12:46, Mark Cave-Ayland wrote:
> The next.scr memory is now effectively unused, however there are 3 separate region
> accesses still logged that occur when booting a NeXTStep disk image. Use the
> empty_slot device to capture and ignore memory accesses to these 3 memory regions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/Kconfig     | 1 +
>   hw/m68k/next-cube.c | 8 ++++++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
> index 0092cda4e9..aff769b30f 100644
> --- a/hw/m68k/Kconfig
> +++ b/hw/m68k/Kconfig
> @@ -18,6 +18,7 @@ config NEXTCUBE
>       depends on M68K
>       select FRAMEBUFFER
>       select ESCC
> +    select EMPTY_SLOT
>   
>   config Q800
>       bool
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 76ecf86ed0..95d01030cd 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -22,6 +22,7 @@
>   #include "qom/object.h"
>   #include "hw/char/escc.h" /* ZILOG 8530 Serial Emulation */
>   #include "hw/block/fdc.h"
> +#include "hw/misc/empty_slot.h"
>   #include "hw/qdev-properties.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
> @@ -1239,6 +1240,13 @@ static void next_cube_init(MachineState *machine)
>       /* BMAP IO - acts as a catch-all for now */
>       sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
>   
> +    /* unknown: Brightness control register? */
> +    empty_slot_init("next.unknown", 0x02110000, 0x10);
> +    /* unknown: Magneto-Optical drive controller? */
> +    empty_slot_init("next.unknown", 0x02112000, 0x10);
> +    /* unknown: Serial clock configuration register? */
> +    empty_slot_init("next.unknown", 0x02118004, 0x10);

IIRC the 'name' argument of empty_slot_init() was to log accesses,
but apparently the patch has never been merged.

Maybe use "next.unknown0/1/2" in preparation?

> +
>       /* BMAP memory */
>       memory_region_init_ram_flags_nomigrate(&m->bmapm1, NULL, "next.bmapmem",
>                                              64, RAM_SHARED, &error_fatal);

Re: [PATCH v2 16/34] next-cube: rearrange NeXTState declarations to improve readability

[Philippe Mathieu-Daudé]

On 12/12/24 12:46, Mark Cave-Ayland wrote:
> Move the NeXTState, next_dma and TYPE_NEXT_MACHINE definition to the same area
> at the top of next-cube.c.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/next-cube.c | 64 ++++++++++++++++++++++-----------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 17/34] next-cube: convert next-pc device to use Resettable interface

[Philippe Mathieu-Daudé]

On 12/12/24 12:46, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Acked-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/next-cube.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 26/34] next-cube: move reset of next-rtc fields from next-pc to next-rtc

[Philippe Mathieu-Daudé]

On 12/12/24 12:46, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/next-cube.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)


> +static void next_rtc_reset_hold(Object *obj, ResetType type)
> +{
> +    NeXTRTC *rtc = NEXT_RTC(obj);
> +
> +    rtc->status = 0x90;
> +
> +    /* Load RTC RAM - TODO: provide possibility to load contents from file */
> +    memcpy(rtc->ram, rtc_ram2, 32);
> +}

>   static const TypeInfo next_rtc_info = {
> @@ -1072,11 +1084,6 @@ static void next_pc_reset_hold(Object *obj, ResetType type)
>       s->scr1 = 0x00011102;
>       s->scr2 = 0x00ff0c80;
>       s->old_scr2 = s->scr2;
> -
> -    s->rtc.status = 0x90;
> -
> -    /* Load RTC RAM - TODO: provide possibility to load contents from file */
> -    memcpy(s->rtc.ram, rtc_ram2, 32);

Pre-existing, this looks suspicious to reset RTC device state
during device reset.

>   }
>   
>   static void next_pc_realize(DeviceState *dev, Error **errp)

Re: [PATCH v2 33/34] next-cube: add my copyright to the top of the file

[Philippe Mathieu-Daudé]

On 12/12/24 12:46, Mark Cave-Ayland wrote:
> This series has involved rewriting and/or updating a considerable part of the
> next-cube emulation so update the copyright in next-cube.c to reflect this.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> ---
>   hw/m68k/next-cube.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 34/34] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Mark Cave-Ayland]

On 12/12/2024 17:28, Daniel P. Berrangé wrote:

> On Thu, Dec 12, 2024 at 11:46:20AM +0000, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 1e96bb02f8..3c2f3e295c 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -4,10 +4,7 @@
>>    * Copyright (c) 2011 Bryce Lanham
>>    * Copyright (c) 2024 Mark Cave-Ayland
>>    *
>> - * This code is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published
>> - * by the Free Software Foundation; either version 2 of the License,
>> - * or (at your option) any later version.
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>    */
> 
> While adding a SPDX-License-Identifier alongside existing header text
> is acceptable, my view is that we should stay away from removing existing
> license headers. There are some difficult questions wrt interpretation
> of the GPL in this area & avoiding opening that can of worms would be
> nice.

I remember you mentioned this before, but I wasn't sure if this would be mitigated by 
the fact that the code originated from GSoC? I'm sure I've seen at least one recent 
patch that made a similar change, but if there really are legal reasons not to allow 
changes of this type then I shall drop it from the series.


ATB,

Mark.

Re: [PATCH v2 02/34] next-cube: remove overlap between next.dma and next.mmio memory regions

[Mark Cave-Ayland]

On 13/12/2024 19:37, Thomas Huth wrote:

>   Hi Mark!
> 
> Am Thu, 12 Dec 2024 11:45:48 +0000
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> Change the start of the next.mmio memory region so that it follows on directly
>> after the next.dma memory region. Increase the address offsets in
>> next_mmio_read() and next_mmio_write(), and reduce the size of the next.mmio
>> memory region accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/m68k/next-cube.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 0418fbc8aa..550e7f0b0a 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -266,23 +266,23 @@ static uint64_t next_mmio_read(void *opaque, hwaddr addr, unsigned size)
>>       uint64_t val;
>>   
>>       switch (addr) {
>> -    case 0x7000:
>> +    case 0x2000:    /* 0x2005000 */
>>           /* DPRINTF("Read INT status: %x\n", s->int_status); */
>>           val = s->int_status;
>>           break;
>>   
>> -    case 0x7800:
>> +    case 0x2800:    /* 0x2007800 */
>>           DPRINTF("MMIO Read INT mask: %x\n", s->int_mask);
>>           val = s->int_mask;
>>           break;
>>   
>> -    case 0xc000 ... 0xc003:
>> -        val = extract32(s->scr1, (4 - (addr - 0xc000) - size) << 3,
>> +    case 0x7000 ... 0x7003:    /* 0x200c000 */
>> +        val = extract32(s->scr1, (4 - (addr - 0x7000) - size) << 3,
>>                           size << 3);
>>           break;
>>   
>> -    case 0xd000 ... 0xd003:
>> -        val = extract32(s->scr2, (4 - (addr - 0xd000) - size) << 3,
>> +    case 0x8000 ... 0x8003:    /* 0x200d000 */
>> +        val = extract32(s->scr2, (4 - (addr - 0x8000) - size) << 3,
>>                           size << 3);
>>           break;
>>   
>> @@ -301,25 +301,25 @@ static void next_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>>       NeXTPC *s = NEXT_PC(opaque);
>>   
>>       switch (addr) {
>> -    case 0x7000:
>> +    case 0x2000:    /* 0x2005000 */
> 
> Comment should be /* 0x2007000 */ instead.
> 
> With that fixed:
> 
> Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Ooops yes, thanks for spotting this. I'll fix it in v3.


ATB,

Mark.

Re: [PATCH v2 25/34] next-cube: QOMify NeXTRTC

[Mark Cave-Ayland]

On 14/12/2024 06:08, Thomas Huth wrote:

> Am Thu, 12 Dec 2024 11:46:11 +0000
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> This is to allow the RTC functionality to be maintained within its own separate
>> device rather than as part of the next-pc device.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 71 +++++++++++++++++++++++++++++++--------------
>>   1 file changed, 50 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 9c91ee146a..c947af65e2 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
> ...
>> @@ -1078,6 +1115,12 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>>       }
>>       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));
>> +
>> +    /* RTC */
>> +    d = DEVICE(object_resolve_path_component(OBJECT(dev), "rtc"));
>> +    if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
>> +        return;
>> +    }
>>   }
> 
> Would it be easier to directly use s->rtc instead of taking the detour via
> object_resolve_path_component?
> 
>   Thomas

That would also work. FWIW the object_resolve_path_component() version of the device 
lookup comes from a follow-up series that wires up the interrupts outside of the 
next-pc device, but it's easy enough for me to change for v3.


ATB,

Mark.

Re: [PATCH v2 11/34] next-cube: move ESCC to be QOM child of next-pc device

[Mark Cave-Ayland]

On 14/12/2024 06:10, Thomas Huth wrote:

> Am Thu, 12 Dec 2024 11:45:57 +0000
> 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 efbb11e74b..20a0b073e1 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"));
> 
> Can't you use s->escc directly here instead of taking the detour via
> object_resolve_path_component() ?

Indeed, its only there for the same reason as my previous email. I'll update it for v3.

>> +    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));
>> +
>>   }
> 

ATB,

Mark.

Re: [PATCH v2 12/34] next-cube: move timer MMIO to separate memory region on next-pc device

[Mark Cave-Ayland]

On 14/12/2024 13:29, Philippe Mathieu-Daudé wrote:

> On 12/12/24 12:45, Mark Cave-Ayland wrote:
>> Move the timer MMIO accesses to a separate memory region on the next-pc device
>> instead of being part of the next.scr MMIO memory region.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
>> ---
>>   hw/m68k/next-cube.c | 63 +++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 50 insertions(+), 13 deletions(-)
> 
> 
>> +static uint64_t next_timer_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    uint64_t val;
>> +
>> +    switch (addr) {
>> +    case 0 ... 3:
>> +        /*
>> +         * These 4 registers are the hardware timer, not sure which register
>> +         * is the latch instead of data, but no problems so far.
>> +         *
>> +         * Hack: We need to have the LSB change consistently to make it work
>> +         */
>> +        val = extract32(clock(), (4 - addr - size) << 3,
>> +                        size << 3);
> 
> Does this mean ...
> 
>> +        break;
>> +
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    return val;
>> +}
>> +
>> +static const MemoryRegionOps next_timer_ops = {
>> +    .read = next_timer_read,
>> +    .write = next_timer_write,
>> +    .valid.min_access_size = 1,
>> +    .valid.max_access_size = 4,
>> +    .endianness = DEVICE_BIG_ENDIAN,
> 
> ... this should be in little endianness?
> 
> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> +};

That's a good point re: endian. I don't have any documentation for the timer device, 
so the patch is focused to moving its registers to a separate memory region. It's 
definitely something to consider in the future though.


ATB,

Mark.

Re: [PATCH v2 14/34] next-cube: add empty slots for unknown accesses to next.scr memory region

[Mark Cave-Ayland]

On 14/12/2024 13:32, Philippe Mathieu-Daudé wrote:

> On 12/12/24 12:46, Mark Cave-Ayland wrote:
>> The next.scr memory is now effectively unused, however there are 3 separate region
>> accesses still logged that occur when booting a NeXTStep disk image. Use the
>> empty_slot device to capture and ignore memory accesses to these 3 memory regions.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Thomas Huth <huth@tuxfamily.org>
>> ---
>>   hw/m68k/Kconfig     | 1 +
>>   hw/m68k/next-cube.c | 8 ++++++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>> index 0092cda4e9..aff769b30f 100644
>> --- a/hw/m68k/Kconfig
>> +++ b/hw/m68k/Kconfig
>> @@ -18,6 +18,7 @@ config NEXTCUBE
>>       depends on M68K
>>       select FRAMEBUFFER
>>       select ESCC
>> +    select EMPTY_SLOT
>>   config Q800
>>       bool
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 76ecf86ed0..95d01030cd 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -22,6 +22,7 @@
>>   #include "qom/object.h"
>>   #include "hw/char/escc.h" /* ZILOG 8530 Serial Emulation */
>>   #include "hw/block/fdc.h"
>> +#include "hw/misc/empty_slot.h"
>>   #include "hw/qdev-properties.h"
>>   #include "qapi/error.h"
>>   #include "qemu/error-report.h"
>> @@ -1239,6 +1240,13 @@ static void next_cube_init(MachineState *machine)
>>       /* BMAP IO - acts as a catch-all for now */
>>       sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
>> +    /* unknown: Brightness control register? */
>> +    empty_slot_init("next.unknown", 0x02110000, 0x10);
>> +    /* unknown: Magneto-Optical drive controller? */
>> +    empty_slot_init("next.unknown", 0x02112000, 0x10);
>> +    /* unknown: Serial clock configuration register? */
>> +    empty_slot_init("next.unknown", 0x02118004, 0x10);
> 
> IIRC the 'name' argument of empty_slot_init() was to log accesses,
> but apparently the patch has never been merged.
> 
> Maybe use "next.unknown0/1/2" in preparation?
> 
>> +
>>       /* BMAP memory */
>>       memory_region_init_ram_flags_nomigrate(&m->bmapm1, NULL, "next.bmapmem",
>>                                              64, RAM_SHARED, &error_fatal);
> 
> 

That's a good idea: I'll use a numerical suffix for v3.


ATB,

Mark.

Re: [PATCH v2 34/34] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Daniel P. Berrangé]

On Sat, Dec 14, 2024 at 08:38:06PM +0000, Mark Cave-Ayland wrote:
> On 12/12/2024 17:28, Daniel P. Berrangé wrote:
> 
> > On Thu, Dec 12, 2024 at 11:46:20AM +0000, Mark Cave-Ayland wrote:
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > >   hw/m68k/next-cube.c | 5 +----
> > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> > > index 1e96bb02f8..3c2f3e295c 100644
> > > --- a/hw/m68k/next-cube.c
> > > +++ b/hw/m68k/next-cube.c
> > > @@ -4,10 +4,7 @@
> > >    * Copyright (c) 2011 Bryce Lanham
> > >    * Copyright (c) 2024 Mark Cave-Ayland
> > >    *
> > > - * This code is free software; you can redistribute it and/or modify
> > > - * it under the terms of the GNU General Public License as published
> > > - * by the Free Software Foundation; either version 2 of the License,
> > > - * or (at your option) any later version.
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > >    */
> > 
> > While adding a SPDX-License-Identifier alongside existing header text
> > is acceptable, my view is that we should stay away from removing existing
> > license headers. There are some difficult questions wrt interpretation
> > of the GPL in this area & avoiding opening that can of worms would be
> > nice.
> 
> I remember you mentioned this before, but I wasn't sure if this would be
> mitigated by the fact that the code originated from GSoC? I'm sure I've seen
> at least one recent patch that made a similar change, but if there really
> are legal reasons not to allow changes of this type then I shall drop it
> from the series.

GSoC isn't really important. The challenging problem here is GPL clause 1
which says

  "keep intact all the notices that refer to this License and to the
   absence of any warranty"

there are differing opinions on how strictly to interpret the "keep intact"
language there.

While we could have a debate over this and come to some project opinion
IMHO it is a better use of our time to just not remove existing notices.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 34/34] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Thomas Huth]

On 16/12/2024 11.38, Daniel P. Berrangé wrote:
> On Sat, Dec 14, 2024 at 08:38:06PM +0000, Mark Cave-Ayland wrote:
>> On 12/12/2024 17:28, Daniel P. Berrangé wrote:
>>
>>> On Thu, Dec 12, 2024 at 11:46:20AM +0000, Mark Cave-Ayland wrote:
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>    hw/m68k/next-cube.c | 5 +----
>>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>>>> index 1e96bb02f8..3c2f3e295c 100644
>>>> --- a/hw/m68k/next-cube.c
>>>> +++ b/hw/m68k/next-cube.c
>>>> @@ -4,10 +4,7 @@
>>>>     * Copyright (c) 2011 Bryce Lanham
>>>>     * Copyright (c) 2024 Mark Cave-Ayland
>>>>     *
>>>> - * This code is free software; you can redistribute it and/or modify
>>>> - * it under the terms of the GNU General Public License as published
>>>> - * by the Free Software Foundation; either version 2 of the License,
>>>> - * or (at your option) any later version.
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>     */
>>>
>>> While adding a SPDX-License-Identifier alongside existing header text
>>> is acceptable, my view is that we should stay away from removing existing
>>> license headers. There are some difficult questions wrt interpretation
>>> of the GPL in this area & avoiding opening that can of worms would be
>>> nice.
>>
>> I remember you mentioned this before, but I wasn't sure if this would be
>> mitigated by the fact that the code originated from GSoC? I'm sure I've seen
>> at least one recent patch that made a similar change, but if there really
>> are legal reasons not to allow changes of this type then I shall drop it
>> from the series.
> 
> GSoC isn't really important. The challenging problem here is GPL clause 1
> which says
> 
>    "keep intact all the notices that refer to this License and to the
>     absence of any warranty"
> 
> there are differing opinions on how strictly to interpret the "keep intact"
> language there.
> 
> While we could have a debate over this and come to some project opinion
> IMHO it is a better use of our time to just not remove existing notices.

I generally agree with Daniel here ... but in this special case, I should 
maybe mention that Bryce's original file only had a "This code is licensed 
under the GPL" statement in it:

  https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-cube.c#L12

IIRC it was me who replaced that with the usual boilerplate when I picked up 
his work to get it included in the upstream QEMU. And for me, it's fine if 
we switch to SPDX here, so in this special case, it might be OK to replace it?

  Thomas

Re: [PATCH v2 34/34] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Mark Cave-Ayland]

On 16/12/2024 11:17, Thomas Huth wrote:

> On 16/12/2024 11.38, Daniel P. Berrangé wrote:
>> On Sat, Dec 14, 2024 at 08:38:06PM +0000, Mark Cave-Ayland wrote:
>>> On 12/12/2024 17:28, Daniel P. Berrangé wrote:
>>>
>>>> On Thu, Dec 12, 2024 at 11:46:20AM +0000, Mark Cave-Ayland wrote:
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>>    hw/m68k/next-cube.c | 5 +----
>>>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>>>>> index 1e96bb02f8..3c2f3e295c 100644
>>>>> --- a/hw/m68k/next-cube.c
>>>>> +++ b/hw/m68k/next-cube.c
>>>>> @@ -4,10 +4,7 @@
>>>>>     * Copyright (c) 2011 Bryce Lanham
>>>>>     * Copyright (c) 2024 Mark Cave-Ayland
>>>>>     *
>>>>> - * This code is free software; you can redistribute it and/or modify
>>>>> - * it under the terms of the GNU General Public License as published
>>>>> - * by the Free Software Foundation; either version 2 of the License,
>>>>> - * or (at your option) any later version.
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>     */
>>>>
>>>> While adding a SPDX-License-Identifier alongside existing header text
>>>> is acceptable, my view is that we should stay away from removing existing
>>>> license headers. There are some difficult questions wrt interpretation
>>>> of the GPL in this area & avoiding opening that can of worms would be
>>>> nice.
>>>
>>> I remember you mentioned this before, but I wasn't sure if this would be
>>> mitigated by the fact that the code originated from GSoC? I'm sure I've seen
>>> at least one recent patch that made a similar change, but if there really
>>> are legal reasons not to allow changes of this type then I shall drop it
>>> from the series.
>>
>> GSoC isn't really important. The challenging problem here is GPL clause 1
>> which says
>>
>>    "keep intact all the notices that refer to this License and to the
>>     absence of any warranty"
>>
>> there are differing opinions on how strictly to interpret the "keep intact"
>> language there.
>>
>> While we could have a debate over this and come to some project opinion
>> IMHO it is a better use of our time to just not remove existing notices.
> 
> I generally agree with Daniel here ... but in this special case, I should maybe 
> mention that Bryce's original file only had a "This code is licensed under the GPL" 
> statement in it:
> 
>   https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-cube.c#L12
> 
> IIRC it was me who replaced that with the usual boilerplate when I picked up his work 
> to get it included in the upstream QEMU. And for me, it's fine if we switch to SPDX 
> here, so in this special case, it might be OK to replace it?

Seeing as there still seems to be some questions over this patch, I'll drop it from 
the v3 series.


ATB,

Mark.