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

[PATCH 00/36] 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>


Mark Cave-Ayland (36):
  next-cube: fix up compilation when DEBUG_NEXT is enabled
  next-cube: remove 0x14020 dummy value from next_mmio_read()
  next-cube: remove overlap between next.dma and next.mmio memory
    regions
  next-cube: remove cpu parameter from next_scsi_init()
  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 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 reset the rtc state machine
  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 | 1025 +++++++++++++++++++++++++++----------------
 2 files changed, 655 insertions(+), 371 deletions(-)

-- 
2.39.5

[PATCH 01/36] next-cube: fix up compilation when DEBUG_NEXT is enabled

[Mark Cave-Ayland]

These were accidentally introduced by my last series.

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

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 9832213e7e..7a503e0707 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -404,7 +404,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
 
     switch (addr) {
     case 0x14108:
-        DPRINTF("FDCSR Write: %x\n", value);
+        DPRINTF("FDCSR Write: %"PRIx64 "\n", val);
         if (val == 0x0) {
             /* qemu_irq_raise(s->fd_irq[0]); */
         }
@@ -468,15 +468,15 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
             /* int_mask |= 0x1000; */
             /* s->scsi_csr_1 |= 0x80; */
         }
-        DPRINTF("SCSICSR Write: %x\n", val);
+        DPRINTF("SCSICSR Write: %"PRIx64 "\n", val);
         /* s->scsi_csr_1 = val; */
         break;
 
     /* Hardware timer latch - not implemented yet */
     case 0x1a000:
     default:
-        DPRINTF("BMAP Write @ 0x%x with 0x%x size %u\n", (unsigned int)addr,
-                val, size);
+        DPRINTF("BMAP Write @ 0x%x with 0x%"PRIx64 " size %u\n",
+                (unsigned int)addr, val, size);
     }
 }
 
@@ -585,7 +585,7 @@ static void next_dma_write(void *opaque, hwaddr addr, uint64_t val,
         break;
 
     default:
-        DPRINTF("DMA write @ %x w/ %x\n", (unsigned)addr, (unsigned)value);
+        DPRINTF("DMA write @ %x w/ %x\n", (unsigned)addr, (unsigned)val);
     }
 }
 
-- 
2.39.5

[PATCH 02/36] 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>
---
 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 7a503e0707..4e8e55a8bd 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 03/36] 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, adjusting the address offsets in
next_mmio_read() and next_mmio_write() accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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 4e8e55a8bd..e1d94c1ce0 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:
         /* DPRINTF("Read INT status: %x\n", s->int_status); */
         val = s->int_status;
         break;
 
-    case 0x7800:
+    case 0x2800:
         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:
+        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:
+        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:
         DPRINTF("INT Status old: %x new: %x\n", s->int_status,
                 (unsigned int)val);
         s->int_status = val;
         break;
 
-    case 0x7800:
+    case 0x2800:
         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:
         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:
+        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 04/36] next-cube: remove cpu parameter from next_scsi_init()

[Mark Cave-Ayland]

The parameter is not used.

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

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index e1d94c1ce0..6bdd91c9c6 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -824,7 +824,7 @@ 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, M68kCPU *cpu)
+static void next_scsi_init(DeviceState *pcdev)
 {
     struct NeXTPC *next_pc = NEXT_PC(pcdev);
     DeviceState *dev;
@@ -1046,7 +1046,7 @@ static void next_cube_init(MachineState *machine)
     /* TODO: */
     /* Network */
     /* SCSI */
-    next_scsi_init(pcdev, cpu);
+    next_scsi_init(pcdev);
 
     /* DMA */
     memory_region_init_io(&m->dmamem, NULL, &next_dma_ops, machine,
-- 
2.39.5

[PATCH 05/36] 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 6bdd91c9c6..9e457fdf12 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 06/36] 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>
---
 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 9e457fdf12..0c3f8780a1 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 07/36] 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>
---
 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 0c3f8780a1..3b4c361156 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 08/36] 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>
---
 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 3b4c361156..266f57ac63 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 09/36] 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 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 266f57ac63..32466a425f 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 10/36] next-cube: move SCSI 4020 logic from next-pc device to next-scsi device

[Mark Cave-Ayland]

The SCSI 4020 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
now this has been resolved. This allows us to move the more complex prototype
logic into the next-scsi MMIO accessors.

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 32466a425f..22da777006 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 11/36] 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>
---
 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 22da777006..f1a50ec737 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 12/36] 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>
---
 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 f1a50ec737..7f714640da 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 13/36] 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 7f714640da..915dd80f6f 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -124,6 +124,8 @@ struct NeXTPC {
     qemu_irq scsi_reset;
     qemu_irq scsi_dma;
 
+    ESCCState escc;
+
     NextRtc rtc;
 };
 
@@ -978,31 +980,6 @@ static const MemoryRegionOps next_floppy_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void next_escc_init(DeviceState *pcdev)
-{
-    NeXTPC *next_pc = NEXT_PC(pcdev);
-    DeviceState *dev;
-    SysBusDevice *s;
-
-    dev = qdev_new(TYPE_ESCC);
-    qdev_prop_set_uint32(dev, "disabled", 0);
-    qdev_prop_set_uint32(dev, "frequency", 9600 * 384);
-    qdev_prop_set_uint32(dev, "it_shift", 0);
-    qdev_prop_set_bit(dev, "bit_swap", true);
-    qdev_prop_set_chr(dev, "chrB", serial_hd(1));
-    qdev_prop_set_chr(dev, "chrA", serial_hd(0));
-    qdev_prop_set_uint32(dev, "chnBtype", escc_serial);
-    qdev_prop_set_uint32(dev, "chnAtype", escc_serial);
-
-    s = SYS_BUS_DEVICE(dev);
-    sysbus_realize_and_unref(s, &error_fatal);
-    sysbus_connect_irq(s, 0, qdev_get_gpio_in(pcdev, NEXT_SCC_I));
-    sysbus_connect_irq(s, 1, qdev_get_gpio_in(pcdev, NEXT_SCC_DMA_I));
-
-    memory_region_add_subregion(&next_pc->scrmem, 0x18000,
-                                sysbus_mmio_get_region(s, 0));
-}
-
 static void next_pc_reset(DeviceState *dev)
 {
     NeXTPC *s = NEXT_PC(dev);
@@ -1043,6 +1020,28 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     /* Floppy */
     memory_region_add_subregion(&s->scrmem, 0x14108,
                                 &s->floppy_mem);
+
+    /* ESCC */
+    d = DEVICE(object_resolve_path_component(OBJECT(dev), "escc"));
+    qdev_prop_set_uint32(d, "disabled", 0);
+    qdev_prop_set_uint32(d, "frequency", 9600 * 384);
+    qdev_prop_set_uint32(d, "it_shift", 0);
+    qdev_prop_set_bit(d, "bit_swap", true);
+    qdev_prop_set_chr(d, "chrB", serial_hd(1));
+    qdev_prop_set_chr(d, "chrA", serial_hd(0));
+    qdev_prop_set_uint32(d, "chnBtype", escc_serial);
+    qdev_prop_set_uint32(d, "chnAtype", escc_serial);
+
+    sbd = SYS_BUS_DEVICE(d);
+    if (!sysbus_realize(sbd, errp)) {
+        return;
+    }
+    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
+    sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
+
+    memory_region_add_subregion(&s->scrmem, 0x18000,
+                                sysbus_mmio_get_region(sbd, 0));
+
 }
 
 static void next_pc_init(Object *obj)
@@ -1064,6 +1063,8 @@ static void next_pc_init(Object *obj)
 
     memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
                           "next.floppy", 4);
+
+    object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
 }
 
 /*
@@ -1201,9 +1202,6 @@ static void next_cube_init(MachineState *machine)
         }
     }
 
-    /* Serial */
-    next_escc_init(pcdev);
-
     /* TODO: */
     /* Network */
 
-- 
2.39.5

[PATCH 14/36] 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>
---
 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 915dd80f6f..402aa7ea8e 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 15/36] 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>
---
 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 402aa7ea8e..3278970890 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 16/36] 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>
---
 hw/m68k/Kconfig     | 1 +
 hw/m68k/next-cube.c | 6 ++++++
 2 files changed, 7 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 3278970890..ac6d3cb634 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,11 @@ 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 */
+    empty_slot_init("next.unknown", 0x02110000, 0x10);
+    empty_slot_init("next.unknown", 0x02112000, 0x10);
+    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 17/36] 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>
---
 hw/m68k/next-cube.c | 73 +++++++++++----------------------------------
 1 file changed, 18 insertions(+), 55 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index ac6d3cb634..9541f88dca 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);
 }
 
 /*
@@ -1237,13 +1192,21 @@ static void next_cube_init(MachineState *machine)
     /* MMIO */
     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);
-
+    /* en network (dummy) */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02106000);
     /* unknown */
     empty_slot_init("next.unknown", 0x02110000, 0x10);
     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 */
     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",
-- 
2.39.5

[PATCH 18/36] 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>
---
 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 9541f88dca..9829f49387 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 19/36] 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>
---
 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 9829f49387..c44a1c6f0c 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 20/36] 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>
---
 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 c44a1c6f0c..33297f01b8 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 21/36] 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>
---
 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 33297f01b8..bb375b63ca 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 22/36] 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>
---
 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 bb375b63ca..4aed80c9fe 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 23/36] 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 4aed80c9fe..bca975065f 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 24/36] 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>
---
 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 bca975065f..2a2e970830 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 25/36] 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>
---
 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 2a2e970830..43b2c775c0 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 26/36] 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>
---
 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 43b2c775c0..e4d0083eb0 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 27/36] next-cube: QOMify NeXTRTC

[Mark Cave-Ayland]

This is to allow the RTC functionality to be maintained within its own separate
device.

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

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index e4d0083eb0..6b574d39cf 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,22 +1174,6 @@ 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,
@@ -1297,6 +1326,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 28/36] 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>
---
 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 6b574d39cf..24f59480c5 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 29/36] 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>
---
 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 24f59480c5..111a2c0311 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 30/36] 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 111a2c0311..bd24359913 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 31/36] 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 bd24359913..16b16e9956 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 32/36] next-cube: add rtc-power-out 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_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 16b16e9956..0a8b899515 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 33/36] 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 0a8b899515..076c9d1f3a 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 34/36] 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>
---
 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 076c9d1f3a..ece63f20b1 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 35/36] 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>
---
 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 ece63f20b1..eefb372dca 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 36/36] 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 eefb372dca..c961d5fef6 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 36/36] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Daniel P. Berrangé]

On Wed, Oct 23, 2024 at 09:58:52AM +0100, 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 eefb372dca..c961d5fef6 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

Although the Kernel folks did such replacements, IMHO we should be
wary about removing existing license boilerplate text, as it is
potentially in conflict with the requirements in clause 1 of the
license to keep all existing notices intact, depending on your
interpretation.

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 36/36] next-cube: replace boiler-plate GPL 2.0 or later license text with SPDX identifier

[Mark Cave-Ayland]

On 23/10/2024 10:30, Daniel P. Berrangé wrote:

> On Wed, Oct 23, 2024 at 09:58:52AM +0100, 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 eefb372dca..c961d5fef6 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
> 
> Although the Kernel folks did such replacements, IMHO we should be
> wary about removing existing license boilerplate text, as it is
> potentially in conflict with the requirements in clause 1 of the
> license to keep all existing notices intact, depending on your
> interpretation.

Hmmm. Do we have a specific list of criteria where this may or may not be 
appropriate? FWIW the NeXTCube machine was written as part of GSoC back in 2011 
(https://wiki.qemu.org/Google_Summer_of_Code_2011#NeXT_machines_system_emulation) so 
I'm confident that the code was written with the intention of being upstreamed and 
covered by the standard GPL-v2.0 or later license used by QEMU.


ATB,

Mark.

Re: [PATCH 26/36] next-cube: don't use rtc phase value of -1

[BALATON Zoltan]

On Wed, 23 Oct 2024, Mark Cave-Ayland wrote:
> 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>
> ---
> 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 43b2c775c0..e4d0083eb0 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;
>     }

Additionally, maybe it would be simpler to invert the if condition and 
move this else branch up there to the beginning so you can return early 
after this reset (the deposit at the end does nothing after the else case 
as it's just storing back the unmodified value) and then you can deindent 
the big if where most of the functionality is now.

Regards,
BALATON Zoltan

Re: [PATCH 03/36] next-cube: remove overlap between next.dma and next.mmio memory regions

[Philippe Mathieu-Daudé]

On 23/10/24 05:58, Mark Cave-Ayland wrote:
> Change the start of the next.mmio memory region so that it follows on directly
> after the next.dma memory region, adjusting the address offsets in
> next_mmio_read() and next_mmio_write() accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/next-cube.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)


> @@ -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);

Please mention 0xd0000 was incorrect, otherwise:

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

Re: [PATCH 27/36] next-cube: QOMify NeXTRTC

[Philippe Mathieu-Daudé]

Hi Mark,

On 23/10/24 05:58, Mark Cave-Ayland wrote:
> This is to allow the RTC functionality to be maintained within its own separate
> device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>   1 file changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index e4d0083eb0..6b574d39cf 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;

Since it was not explicitly reset, maybe QDev parent is enough?

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

Re: [PATCH 26/36] next-cube: don't use rtc phase value of -1

[Mark Cave-Ayland]

On 23/10/2024 11:37, BALATON Zoltan wrote:

> On Wed, 23 Oct 2024, Mark Cave-Ayland wrote:
>> 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>
>> ---
>> 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 43b2c775c0..e4d0083eb0 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;
>>     }
> 
> Additionally, maybe it would be simpler to invert the if condition and move this else 
> branch up there to the beginning so you can return early after this reset (the 
> deposit at the end does nothing after the else case as it's just storing back the 
> unmodified value) and then you can deindent the big if where most of the 
> functionality is now.

I didn't change the layout of the outer if() mainly because Bryce reversed engineered 
a lot of this from Mach/NetBSD, and wasn't 100% confident that there was any other 
logic that needed to be applied to bit 0. It's also worth pointing out that by the 
end of the series all of the functionality has been moved to gpios, so all the 
complexity within the big if() disappears anyway.


ATB,

Mark.

Re: [PATCH 03/36] next-cube: remove overlap between next.dma and next.mmio memory regions

[Mark Cave-Ayland]

On 24/10/2024 03:42, Philippe Mathieu-Daudé wrote:

> On 23/10/24 05:58, Mark Cave-Ayland wrote:
>> Change the start of the next.mmio memory region so that it follows on directly
>> after the next.dma memory region, adjusting the address offsets in
>> next_mmio_read() and next_mmio_write() accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> 
>> @@ -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);
> 
> Please mention 0xd0000 was incorrect, otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

This one is a little more fuzzy, since I'm not even sure that the new value 0x9000 is 
correct (there isn't much in the way of documentation to support this) - effectively 
the purpose of truncating this memory region is to remove the overlap with another so 
that MMIO accesses are consistently directed to the same handler.

I'll have a think about how I can re-word this better for a v2.


ATB,

Mark.

Re: [PATCH 27/36] next-cube: QOMify NeXTRTC

[Mark Cave-Ayland]

On 24/10/2024 03:44, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 23/10/24 05:58, Mark Cave-Ayland wrote:
>> This is to allow the RTC functionality to be maintained within its own separate
>> device.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 48 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index e4d0083eb0..6b574d39cf 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;
> 
> Since it was not explicitly reset, maybe QDev parent is enough?

Hi Phil,

This is deliberate, since the next-pc device resets a couple of fields directly in 
the NeXTRTC struct in next_pc_reset_hold(), and these are moved to a separate 
next_rtc_reset_hold() in the next patch.

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


ATB,

Mark.

Re: [PATCH 32/36] next-cube: add rtc-power-out named gpio to reset the rtc state machine

[Mark Cave-Ayland]

On 23/10/2024 09:58, Mark Cave-Ayland wrote:

> 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 16b16e9956..0a8b899515 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,

(replying to myself)

I've just noticed the headline summary is wrong and should instead read: "next-cube: 
add rtc-power-out named gpio to trigger the NEXT_PWR_I interrupt".


ATB,

Mark.

Re: [PATCH 01/36] next-cube: fix up compilation when DEBUG_NEXT is enabled

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:17 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> These were accidentally introduced by my last series.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

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

Re: [PATCH 02/36] next-cube: remove 0x14020 dummy value from next_mmio_read()

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:18 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 4 ----
>  1 file changed, 4 deletions(-)

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

Re: [PATCH 03/36] next-cube: remove overlap between next.dma and next.mmio memory regions

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:19 +0100
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, adjusting the address offsets in
> next_mmio_read() and next_mmio_write() accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  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 4e8e55a8bd..e1d94c1ce0 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:
>          /* DPRINTF("Read INT status: %x\n", s->int_status); */
>          val = s->int_status;
>          break;
>  
> -    case 0x7800:
> +    case 0x2800:
>          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:
> +        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:
> +        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:
>          DPRINTF("INT Status old: %x new: %x\n", s->int_status,
>                  (unsigned int)val);
>          s->int_status = val;
>          break;
>  
> -    case 0x7800:
> +    case 0x2800:
>          DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, (unsigned int)val);
>          s->int_mask  = val;
>          break;

Could you please add comments at the end of the "case" lines, stating which
mmio addresses are handled in each case? Otherwise, it's harder to grep for
certain addresses later. E.g:

    case 0x2800:     /* 0x2007800 */

> @@ -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);

Why 0x02005000 and not directly 0x02007000 ?

I think there is another range at 0x02006000 related to the ethernet
controller, so directly going with 0x02007000 might cause less friction
later when we add the NIC?

 Thanks,
  Thomas

Re: [PATCH 04/36] next-cube: remove cpu parameter from next_scsi_init()

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:20 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

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

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

Re: [PATCH 03/36] next-cube: remove overlap between next.dma and next.mmio memory regions

[Mark Cave-Ayland]

On 26/10/2024 08:56, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:19 +0100
> 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, adjusting the address offsets in
>> next_mmio_read() and next_mmio_write() accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   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 4e8e55a8bd..e1d94c1ce0 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:
>>           /* DPRINTF("Read INT status: %x\n", s->int_status); */
>>           val = s->int_status;
>>           break;
>>   
>> -    case 0x7800:
>> +    case 0x2800:
>>           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:
>> +        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:
>> +        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:
>>           DPRINTF("INT Status old: %x new: %x\n", s->int_status,
>>                   (unsigned int)val);
>>           s->int_status = val;
>>           break;
>>   
>> -    case 0x7800:
>> +    case 0x2800:
>>           DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, (unsigned int)val);
>>           s->int_mask  = val;
>>           break;
> 
> Could you please add comments at the end of the "case" lines, stating which
> mmio addresses are handled in each case? Otherwise, it's harder to grep for
> certain addresses later. E.g:
> 
>      case 0x2800:     /* 0x2007800 */

If you think it will help? Presumably this is to aid with comparing with other source 
code/documentation?

>> @@ -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);
> 
> Why 0x02005000 and not directly 0x02007000 ?

Before this patch the output of "info mtree" shows as follows:

(qemu) info mtree
address-space: cpu-memory-0
address-space: memory
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000000001ffff (prio 0, rom): alias next.rom2 @next.rom 
0000000000000000-000000000001ffff
     0000000001000000-000000000101ffff (prio 0, rom): next.rom
     0000000002000000-0000000002004fff (prio 0, i/o): next.dma
     0000000002000000-00000000020cffff (prio 0, i/o): next.mmio
     000000000200e000-000000000200efff (prio 0, i/o): next.kbd
     00000000020c0000-00000000020c003f (prio 0, ram): next.bmapmem
     0000000002100000-000000000211ffff (prio 0, i/o): next.scr
     0000000002114000-000000000211400f (prio 0, i/o): esp-regs
     0000000002118000-0000000002118003 (prio 0, i/o): escc
     0000000004000000-0000000007ffffff (prio 0, ram): next.ram
     000000000b000000-000000000b1cb0ff (prio 0, ram): next-video
     00000000820c0000-00000000820c003f (prio 0, ram): alias next.bmapmem2 
@next.bmapmem 0000000000000000-000000000000003f

All this patch does is move the start of next.mmio to 0x2005000 to avoid the overlap.

> I think there is another range at 0x02006000 related to the ethernet
> controller, so directly going with 0x02007000 might cause less friction
> later when we add the NIC?

By the end of the series, everything except the "global" registers in next.mmio have 
their own memory region (or empty-slot if the target is unknown) so that "info mtree" 
output looks like this:

(qemu) info mtree
address-space: cpu-memory-0
address-space: memory
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000000001ffff (prio 0, rom): alias next.rom2 @next.rom 
0000000000000000-000000000001ffff
     0000000001000000-000000000101ffff (prio 0, rom): next.rom
     0000000002000000-0000000002004fff (prio 0, i/o): next.dma
     0000000002005000-000000000200dfff (prio 0, i/o): next.mmio
     000000000200e000-000000000200efff (prio 0, i/o): next.kbd
     00000000020c0000-00000000020c003f (prio 0, ram): next.bmapmem
     0000000002106000-000000000210601f (prio 0, i/o): next.en
     0000000002110000-000000000211000f (prio -10000, i/o): empty-slot
     0000000002112000-000000000211200f (prio -10000, i/o): empty-slot
     0000000002114000-000000000211403f (prio 0, i/o): next.scsi
       0000000002114000-000000000211400f (prio 0, i/o): esp-regs
       0000000002114020-0000000002114021 (prio 0, i/o): csrs
     0000000002114108-000000000211410b (prio 0, i/o): next.floppy
     0000000002118000-0000000002118003 (prio 0, i/o): escc
     0000000002118004-0000000002118013 (prio -10000, i/o): empty-slot
     000000000211a000-000000000211a003 (prio 0, i/o): next.timer
     0000000004000000-0000000007ffffff (prio 0, ram): next.ram
     000000000b000000-000000000b1cb0ff (prio 0, ram): next-video
     00000000820c0000-00000000820c003f (prio 0, ram): alias next.bmapmem2 
@next.bmapmem 0000000000000000-000000000000003f

In this case next.en is a dummy memory region which can easily be replaced with a 
proper device implementation: see the final version of next-cube.c after the series 
at https://gitlab.com/mcayland/qemu/-/blob/next-cube-improvements/hw/m68k/next-cube.c.


ATB,

Mark.

Re: [PATCH 06/36] next-cube: move next_scsi_init() to next_pc_realize()

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:22 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

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

Re: [PATCH 07/36] next-cube: introduce next_pc_init() object init function

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:23 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  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 0c3f8780a1..3b4c361156 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,
>  };

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

Re: [PATCH 03/36] next-cube: remove overlap between next.dma and next.mmio memory regions

[Thomas Huth]

Am Sat, 26 Oct 2024 22:13:25 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> On 26/10/2024 08:56, Thomas Huth wrote:
> 
> > Am Wed, 23 Oct 2024 09:58:19 +0100
> > 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, adjusting the address offsets in
> >> next_mmio_read() and next_mmio_write() accordingly.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>   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 4e8e55a8bd..e1d94c1ce0 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:
> >>           /* DPRINTF("Read INT status: %x\n", s->int_status); */
> >>           val = s->int_status;
> >>           break;
> >>   
> >> -    case 0x7800:
> >> +    case 0x2800:
> >>           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:
> >> +        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:
> >> +        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:
> >>           DPRINTF("INT Status old: %x new: %x\n", s->int_status,
> >>                   (unsigned int)val);
> >>           s->int_status = val;
> >>           break;
> >>   
> >> -    case 0x7800:
> >> +    case 0x2800:
> >>           DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, (unsigned int)val);
> >>           s->int_mask  = val;
> >>           break;  
> > 
> > Could you please add comments at the end of the "case" lines, stating which
> > mmio addresses are handled in each case? Otherwise, it's harder to grep for
> > certain addresses later. E.g:
> > 
> >      case 0x2800:     /* 0x2007800 */  
> 
> If you think it will help? Presumably this is to aid with comparing with other source 
> code/documentation?

Yes, it will help with 1) debugging code that is running in the guest (so
you can find IO addresses that it is accessing more easily) and 2) help
when comparing the code with "Previous":

 https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/ioMemTabNEXT.c#l36

> >> @@ -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);  
> > 
> > Why 0x02005000 and not directly 0x02007000 ?  
> 
> Before this patch the output of "info mtree" shows as follows:
> 
> (qemu) info mtree
> address-space: cpu-memory-0
> address-space: memory
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-000000000001ffff (prio 0, rom): alias next.rom2 @next.rom 
> 0000000000000000-000000000001ffff
>      0000000001000000-000000000101ffff (prio 0, rom): next.rom
>      0000000002000000-0000000002004fff (prio 0, i/o): next.dma
>      0000000002000000-00000000020cffff (prio 0, i/o): next.mmio
>      000000000200e000-000000000200efff (prio 0, i/o): next.kbd
>      00000000020c0000-00000000020c003f (prio 0, ram): next.bmapmem
>      0000000002100000-000000000211ffff (prio 0, i/o): next.scr
>      0000000002114000-000000000211400f (prio 0, i/o): esp-regs
>      0000000002118000-0000000002118003 (prio 0, i/o): escc
>      0000000004000000-0000000007ffffff (prio 0, ram): next.ram
>      000000000b000000-000000000b1cb0ff (prio 0, ram): next-video
>      00000000820c0000-00000000820c003f (prio 0, ram): alias next.bmapmem2 
> @next.bmapmem 0000000000000000-000000000000003f
> 
> All this patch does is move the start of next.mmio to 0x2005000 to avoid the overlap.
> 
> > I think there is another range at 0x02006000 related to the ethernet
> > controller, so directly going with 0x02007000 might cause less friction
> > later when we add the NIC?  
> 
> By the end of the series, everything except the "global" registers in next.mmio have 
> their own memory region (or empty-slot if the target is unknown) so that "info mtree" 
> output looks like this:
> 
> (qemu) info mtree
> address-space: cpu-memory-0
> address-space: memory
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-000000000001ffff (prio 0, rom): alias next.rom2 @next.rom 
> 0000000000000000-000000000001ffff
>      0000000001000000-000000000101ffff (prio 0, rom): next.rom
>      0000000002000000-0000000002004fff (prio 0, i/o): next.dma
>      0000000002005000-000000000200dfff (prio 0, i/o): next.mmio
>      000000000200e000-000000000200efff (prio 0, i/o): next.kbd
>      00000000020c0000-00000000020c003f (prio 0, ram): next.bmapmem
>      0000000002106000-000000000210601f (prio 0, i/o): next.en
>      0000000002110000-000000000211000f (prio -10000, i/o): empty-slot
>      0000000002112000-000000000211200f (prio -10000, i/o): empty-slot
>      0000000002114000-000000000211403f (prio 0, i/o): next.scsi
>        0000000002114000-000000000211400f (prio 0, i/o): esp-regs
>        0000000002114020-0000000002114021 (prio 0, i/o): csrs
>      0000000002114108-000000000211410b (prio 0, i/o): next.floppy
>      0000000002118000-0000000002118003 (prio 0, i/o): escc
>      0000000002118004-0000000002118013 (prio -10000, i/o): empty-slot
>      000000000211a000-000000000211a003 (prio 0, i/o): next.timer
>      0000000004000000-0000000007ffffff (prio 0, ram): next.ram
>      000000000b000000-000000000b1cb0ff (prio 0, ram): next-video
>      00000000820c0000-00000000820c003f (prio 0, ram): alias next.bmapmem2 
> @next.bmapmem 0000000000000000-000000000000003f
> 
> In this case next.en is a dummy memory region which can easily be replaced with a 
> proper device implementation: see the final version of next-cube.c after the series 
> at https://gitlab.com/mcayland/qemu/-/blob/next-cube-improvements/hw/m68k/next-cube.c.

If I get it right, the IO memory is mirrored at 0x2000000 and 0x2100000,
so the ethernet region should show up in both, 0x2006000 and 0x2106000 in
the end? ... those memory regions on the NeXT are very confusing, but at
least this is what I get from 
 https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/cpu/memory.c#l1167
(IO_bank is mapped twice, unless it's the 030 Cube)

So I think we should make sure that we can mirror the ethernet registers at
0x2006000, too?

 Thomas

Re: [PATCH 08/36] next-cube: introduce next-scsi device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:24 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 93 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 19 deletions(-)

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

Re: [PATCH 09/36] next-cube: move SCSI CSRs from next-pc to the next-scsi device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:25 +0100
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 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 266f57ac63..32466a425f 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;

The old code never set the scsi_csr_x directly like this, so I'm not sure
whether this is right?

Also, maybe best squash this patch together with the next patch, otherwise
this is temporary change in behaviour, isn't it?

 Thomas


> +    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()
>      },

Re: [PATCH 10/36] next-cube: move SCSI 4020 logic from next-pc device to next-scsi device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:26 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> The SCSI 4020 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
> now this has been resolved. This allows us to move the more complex prototype
> logic into the next-scsi MMIO accessors.
> 
> 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 32466a425f..22da777006 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;

Where is that hard-coded 0x40 gone now? Please mention this in the commit
description, otherwise this looks like a mistake?

 Thomas


> -        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;
>  

Re: [PATCH 11/36] next-cube: move floppy disk MMIO to separate memory region in next-pc

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:27 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 61 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 12 deletions(-)

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

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

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:28 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

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

Re: [PATCH 13/36] next-cube: move ESCC to be QOM child of next-pc device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:29 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> Since the ESCC is part of the next-pc device, move the ESCC to be a QOM child
> of the next-pc device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 54 ++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 7f714640da..915dd80f6f 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -124,6 +124,8 @@ struct NeXTPC {
>      qemu_irq scsi_reset;
>      qemu_irq scsi_dma;
>  
> +    ESCCState escc;
> +
>      NextRtc rtc;
>  };
>  
> @@ -978,31 +980,6 @@ static const MemoryRegionOps next_floppy_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> -static void next_escc_init(DeviceState *pcdev)
> -{
> -    NeXTPC *next_pc = NEXT_PC(pcdev);
> -    DeviceState *dev;
> -    SysBusDevice *s;
> -
> -    dev = qdev_new(TYPE_ESCC);
> -    qdev_prop_set_uint32(dev, "disabled", 0);
> -    qdev_prop_set_uint32(dev, "frequency", 9600 * 384);
> -    qdev_prop_set_uint32(dev, "it_shift", 0);
> -    qdev_prop_set_bit(dev, "bit_swap", true);
> -    qdev_prop_set_chr(dev, "chrB", serial_hd(1));
> -    qdev_prop_set_chr(dev, "chrA", serial_hd(0));
> -    qdev_prop_set_uint32(dev, "chnBtype", escc_serial);
> -    qdev_prop_set_uint32(dev, "chnAtype", escc_serial);
> -
> -    s = SYS_BUS_DEVICE(dev);
> -    sysbus_realize_and_unref(s, &error_fatal);
> -    sysbus_connect_irq(s, 0, qdev_get_gpio_in(pcdev, NEXT_SCC_I));
> -    sysbus_connect_irq(s, 1, qdev_get_gpio_in(pcdev, NEXT_SCC_DMA_I));
> -
> -    memory_region_add_subregion(&next_pc->scrmem, 0x18000,
> -                                sysbus_mmio_get_region(s, 0));
> -}
> -
>  static void next_pc_reset(DeviceState *dev)
>  {
>      NeXTPC *s = NEXT_PC(dev);
> @@ -1043,6 +1020,28 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>      /* Floppy */
>      memory_region_add_subregion(&s->scrmem, 0x14108,
>                                  &s->floppy_mem);
> +
> +    /* ESCC */
> +    d = DEVICE(object_resolve_path_component(OBJECT(dev), "escc"));
> +    qdev_prop_set_uint32(d, "disabled", 0);
> +    qdev_prop_set_uint32(d, "frequency", 9600 * 384);
> +    qdev_prop_set_uint32(d, "it_shift", 0);
> +    qdev_prop_set_bit(d, "bit_swap", true);
> +    qdev_prop_set_chr(d, "chrB", serial_hd(1));
> +    qdev_prop_set_chr(d, "chrA", serial_hd(0));
> +    qdev_prop_set_uint32(d, "chnBtype", escc_serial);
> +    qdev_prop_set_uint32(d, "chnAtype", escc_serial);
> +
> +    sbd = SYS_BUS_DEVICE(d);
> +    if (!sysbus_realize(sbd, errp)) {
> +        return;
> +    }
> +    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
> +    sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
> +
> +    memory_region_add_subregion(&s->scrmem, 0x18000,
> +                                sysbus_mmio_get_region(sbd, 0));

You could also keep the next_escc_init() function, and call next_escc_init()
here?

 Thomas


>  }
>  
>  static void next_pc_init(Object *obj)
> @@ -1064,6 +1063,8 @@ static void next_pc_init(Object *obj)
>  
>      memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
>                            "next.floppy", 4);
> +
> +    object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
>  }
>  
>  /*
> @@ -1201,9 +1202,6 @@ static void next_cube_init(MachineState *machine)
>          }
>      }
>  
> -    /* Serial */
> -    next_escc_init(pcdev);
> -
>      /* TODO: */
>      /* Network */
>  

Re: [PATCH 03/36] next-cube: remove overlap between next.dma and next.mmio memory regions

[Mark Cave-Ayland]

On 27/10/2024 11:24, Thomas Huth wrote:

> Am Sat, 26 Oct 2024 22:13:25 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> On 26/10/2024 08:56, Thomas Huth wrote:
>>
>>> Am Wed, 23 Oct 2024 09:58:19 +0100
>>> 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, adjusting the address offsets in
>>>> next_mmio_read() and next_mmio_write() accordingly.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>    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 4e8e55a8bd..e1d94c1ce0 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:
>>>>            /* DPRINTF("Read INT status: %x\n", s->int_status); */
>>>>            val = s->int_status;
>>>>            break;
>>>>    
>>>> -    case 0x7800:
>>>> +    case 0x2800:
>>>>            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:
>>>> +        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:
>>>> +        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:
>>>>            DPRINTF("INT Status old: %x new: %x\n", s->int_status,
>>>>                    (unsigned int)val);
>>>>            s->int_status = val;
>>>>            break;
>>>>    
>>>> -    case 0x7800:
>>>> +    case 0x2800:
>>>>            DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, (unsigned int)val);
>>>>            s->int_mask  = val;
>>>>            break;
>>>
>>> Could you please add comments at the end of the "case" lines, stating which
>>> mmio addresses are handled in each case? Otherwise, it's harder to grep for
>>> certain addresses later. E.g:
>>>
>>>       case 0x2800:     /* 0x2007800 */
>>
>> If you think it will help? Presumably this is to aid with comparing with other source
>> code/documentation?
> 
> Yes, it will help with 1) debugging code that is running in the guest (so
> you can find IO addresses that it is accessing more easily) and 2) help
> when comparing the code with "Previous":
> 
>   https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/ioMemTabNEXT.c#l36
> 
>>>> @@ -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);
>>>
>>> Why 0x02005000 and not directly 0x02007000 ?
>>
>> Before this patch the output of "info mtree" shows as follows:
>>
>> (qemu) info mtree
>> address-space: cpu-memory-0
>> address-space: memory
>>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>       0000000000000000-000000000001ffff (prio 0, rom): alias next.rom2 @next.rom
>> 0000000000000000-000000000001ffff
>>       0000000001000000-000000000101ffff (prio 0, rom): next.rom
>>       0000000002000000-0000000002004fff (prio 0, i/o): next.dma
>>       0000000002000000-00000000020cffff (prio 0, i/o): next.mmio
>>       000000000200e000-000000000200efff (prio 0, i/o): next.kbd
>>       00000000020c0000-00000000020c003f (prio 0, ram): next.bmapmem
>>       0000000002100000-000000000211ffff (prio 0, i/o): next.scr
>>       0000000002114000-000000000211400f (prio 0, i/o): esp-regs
>>       0000000002118000-0000000002118003 (prio 0, i/o): escc
>>       0000000004000000-0000000007ffffff (prio 0, ram): next.ram
>>       000000000b000000-000000000b1cb0ff (prio 0, ram): next-video
>>       00000000820c0000-00000000820c003f (prio 0, ram): alias next.bmapmem2
>> @next.bmapmem 0000000000000000-000000000000003f
>>
>> All this patch does is move the start of next.mmio to 0x2005000 to avoid the overlap.
>>
>>> I think there is another range at 0x02006000 related to the ethernet
>>> controller, so directly going with 0x02007000 might cause less friction
>>> later when we add the NIC?
>>
>> By the end of the series, everything except the "global" registers in next.mmio have
>> their own memory region (or empty-slot if the target is unknown) so that "info mtree"
>> output looks like this:
>>
>> (qemu) info mtree
>> address-space: cpu-memory-0
>> address-space: memory
>>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>       0000000000000000-000000000001ffff (prio 0, rom): alias next.rom2 @next.rom
>> 0000000000000000-000000000001ffff
>>       0000000001000000-000000000101ffff (prio 0, rom): next.rom
>>       0000000002000000-0000000002004fff (prio 0, i/o): next.dma
>>       0000000002005000-000000000200dfff (prio 0, i/o): next.mmio
>>       000000000200e000-000000000200efff (prio 0, i/o): next.kbd
>>       00000000020c0000-00000000020c003f (prio 0, ram): next.bmapmem
>>       0000000002106000-000000000210601f (prio 0, i/o): next.en
>>       0000000002110000-000000000211000f (prio -10000, i/o): empty-slot
>>       0000000002112000-000000000211200f (prio -10000, i/o): empty-slot
>>       0000000002114000-000000000211403f (prio 0, i/o): next.scsi
>>         0000000002114000-000000000211400f (prio 0, i/o): esp-regs
>>         0000000002114020-0000000002114021 (prio 0, i/o): csrs
>>       0000000002114108-000000000211410b (prio 0, i/o): next.floppy
>>       0000000002118000-0000000002118003 (prio 0, i/o): escc
>>       0000000002118004-0000000002118013 (prio -10000, i/o): empty-slot
>>       000000000211a000-000000000211a003 (prio 0, i/o): next.timer
>>       0000000004000000-0000000007ffffff (prio 0, ram): next.ram
>>       000000000b000000-000000000b1cb0ff (prio 0, ram): next-video
>>       00000000820c0000-00000000820c003f (prio 0, ram): alias next.bmapmem2
>> @next.bmapmem 0000000000000000-000000000000003f
>>
>> In this case next.en is a dummy memory region which can easily be replaced with a
>> proper device implementation: see the final version of next-cube.c after the series
>> at https://gitlab.com/mcayland/qemu/-/blob/next-cube-improvements/hw/m68k/next-cube.c.
> 
> If I get it right, the IO memory is mirrored at 0x2000000 and 0x2100000,
> so the ethernet region should show up in both, 0x2006000 and 0x2106000 in
> the end? ... those memory regions on the NeXT are very confusing, but at
> least this is what I get from
>   https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/cpu/memory.c#l1167
> (IO_bank is mapped twice, unless it's the 030 Cube)

That's definitely not something I've seen before, but that certainly looks like the 
case in Previous. I wonder whether this is something that has been measured on real 
hardware?

> So I think we should make sure that we can mirror the ethernet registers at
> 0x2006000, too?

Yes, that's possible. I haven't really given a great deal of thought to how the 
series reflects real hardware, but I can confirm that each commit is structured such 
that it always boots the kernel to the same point as current master.

The address decoding looks odd, but if we were to decide to do this then I'd be 
inclined to create a memory subregion for the next-pc device, move everything from 
0x2000000-0x20fffff into it, and then create an alias from the 0x2100000-0x21fffff 
region back onto it.

Would it be okay to leave such a change to a follow-up series? There is already a 
huge amount of value in this series in terms of detangling the memory regions and 
sorting out the IRQ wiring, and I think the final decision as to how to implement it 
would depend a lot upon what happens once the DMA controller has been reworked.


ATB,

Mark.

Re: [PATCH 09/36] next-cube: move SCSI CSRs from next-pc to the next-scsi device

[Mark Cave-Ayland]

On 28/10/2024 16:21, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:25 +0100
> 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 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 266f57ac63..32466a425f 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;
> 
> The old code never set the scsi_csr_x directly like this, so I'm not sure
> whether this is right?

Well I initially did this on a hunch that something had gone wrong with an earlier 
refactoring, but I just did a quick check with Previous and it also treats them as 
normal registers (see 
https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/esp.c#l160). So I think 
this should be fine for now?

> Also, maybe best squash this patch together with the next patch, otherwise
> this is temporary change in behaviour, isn't it?

If possible could I keep it as-is? It just means there is separation between the 
change of the memory region topology and then consolidating the SCSI CSR access 
routines in the following patch.

>> +    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()
>>       },


ATB,

Mark.

Re: [PATCH 10/36] next-cube: move SCSI 4020 logic from next-pc device to next-scsi device

[Mark Cave-Ayland]

On 28/10/2024 16:22, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:26 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> The SCSI 4020 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
>> now this has been resolved. This allows us to move the more complex prototype
>> logic into the next-scsi MMIO accessors.
>>
>> 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 32466a425f..22da777006 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;
> 
> Where is that hard-coded 0x40 gone now? Please mention this in the commit
> description, otherwise this looks like a mistake?

Heh I guess the part about the duplicate MMIO accessors was a little bit too cryptic? 
I'll have a think as to how to improve the commit message for v2.


ATB,

Mark.

>> -        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;
>>   

Re: [PATCH 13/36] next-cube: move ESCC to be QOM child of next-pc device

[Mark Cave-Ayland]

On 28/10/2024 16:39, Thomas Huth wrote:
> Am Wed, 23 Oct 2024 09:58:29 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> Since the ESCC is part of the next-pc device, move the ESCC to be a QOM child
>> of the next-pc device.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 54 ++++++++++++++++++++++-----------------------
>>   1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 7f714640da..915dd80f6f 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -124,6 +124,8 @@ struct NeXTPC {
>>       qemu_irq scsi_reset;
>>       qemu_irq scsi_dma;
>>   
>> +    ESCCState escc;
>> +
>>       NextRtc rtc;
>>   };
>>   
>> @@ -978,31 +980,6 @@ static const MemoryRegionOps next_floppy_ops = {
>>       .endianness = DEVICE_BIG_ENDIAN,
>>   };
>>   
>> -static void next_escc_init(DeviceState *pcdev)
>> -{
>> -    NeXTPC *next_pc = NEXT_PC(pcdev);
>> -    DeviceState *dev;
>> -    SysBusDevice *s;
>> -
>> -    dev = qdev_new(TYPE_ESCC);
>> -    qdev_prop_set_uint32(dev, "disabled", 0);
>> -    qdev_prop_set_uint32(dev, "frequency", 9600 * 384);
>> -    qdev_prop_set_uint32(dev, "it_shift", 0);
>> -    qdev_prop_set_bit(dev, "bit_swap", true);
>> -    qdev_prop_set_chr(dev, "chrB", serial_hd(1));
>> -    qdev_prop_set_chr(dev, "chrA", serial_hd(0));
>> -    qdev_prop_set_uint32(dev, "chnBtype", escc_serial);
>> -    qdev_prop_set_uint32(dev, "chnAtype", escc_serial);
>> -
>> -    s = SYS_BUS_DEVICE(dev);
>> -    sysbus_realize_and_unref(s, &error_fatal);
>> -    sysbus_connect_irq(s, 0, qdev_get_gpio_in(pcdev, NEXT_SCC_I));
>> -    sysbus_connect_irq(s, 1, qdev_get_gpio_in(pcdev, NEXT_SCC_DMA_I));
>> -
>> -    memory_region_add_subregion(&next_pc->scrmem, 0x18000,
>> -                                sysbus_mmio_get_region(s, 0));
>> -}
>> -
>>   static void next_pc_reset(DeviceState *dev)
>>   {
>>       NeXTPC *s = NEXT_PC(dev);
>> @@ -1043,6 +1020,28 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>>       /* Floppy */
>>       memory_region_add_subregion(&s->scrmem, 0x14108,
>>                                   &s->floppy_mem);
>> +
>> +    /* ESCC */
>> +    d = DEVICE(object_resolve_path_component(OBJECT(dev), "escc"));
>> +    qdev_prop_set_uint32(d, "disabled", 0);
>> +    qdev_prop_set_uint32(d, "frequency", 9600 * 384);
>> +    qdev_prop_set_uint32(d, "it_shift", 0);
>> +    qdev_prop_set_bit(d, "bit_swap", true);
>> +    qdev_prop_set_chr(d, "chrB", serial_hd(1));
>> +    qdev_prop_set_chr(d, "chrA", serial_hd(0));
>> +    qdev_prop_set_uint32(d, "chnBtype", escc_serial);
>> +    qdev_prop_set_uint32(d, "chnAtype", escc_serial);
>> +
>> +    sbd = SYS_BUS_DEVICE(d);
>> +    if (!sysbus_realize(sbd, errp)) {
>> +        return;
>> +    }
>> +    sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
>> +    sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
>> +
>> +    memory_region_add_subregion(&s->scrmem, 0x18000,
>> +                                sysbus_mmio_get_region(sbd, 0));
> 
> You could also keep the next_escc_init() function, and call next_escc_init()
> here?

Normally a non-QOM _init() function suffix is used to both init() and realize() a 
device, whereas here since the ESCC device is a child of the next-pc device these 
operations must be separate. I think I can see why this convention is used elsewhere 
in the codebase, as otherwise you end up calling a function with a _init() prefix 
from _realize() which can get confusing with respect to the QOM model...


ATB,

Mark.

>>   }
>>   
>>   static void next_pc_init(Object *obj)
>> @@ -1064,6 +1063,8 @@ static void next_pc_init(Object *obj)
>>   
>>       memory_region_init_io(&s->floppy_mem, OBJECT(s), &next_floppy_ops, s,
>>                             "next.floppy", 4);
>> +
>> +    object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
>>   }
>>   
>>   /*
>> @@ -1201,9 +1202,6 @@ static void next_cube_init(MachineState *machine)
>>           }
>>       }
>>   
>> -    /* Serial */
>> -    next_escc_init(pcdev);
>> -
>>       /* TODO: */
>>       /* Network */
>>   
> 

Re: [PATCH 00/36] next-cube: more tidy-ups and improvements

[Peter Maydell]

On Wed, 23 Oct 2024 at 09:59, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> 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.

Would you have time to consider updating hw/m68k/next-kbd.c
to stop using qemu_add_kbd_event_handler()? It's the only
user left in the codebase of that input-legacy.c API.
Generally using qemu_input_handler_register() should simplify
the code because it will no longer need to decode multi-byte
PS2 code sequences back into "what is the single key event
that this corresponds to?". Commit ff7888dcc6c701 is an
example of the conversion (though that input device
is somewhat simpler than a full real keyboard).

thanks
-- PMM

Re: [PATCH 00/36] next-cube: more tidy-ups and improvements

[Mark Cave-Ayland]

On 29/10/2024 11:22, Peter Maydell wrote:

> On Wed, 23 Oct 2024 at 09:59, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> 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.
> 
> Would you have time to consider updating hw/m68k/next-kbd.c
> to stop using qemu_add_kbd_event_handler()? It's the only
> user left in the codebase of that input-legacy.c API.
> Generally using qemu_input_handler_register() should simplify
> the code because it will no longer need to decode multi-byte
> PS2 code sequences back into "what is the single key event
> that this corresponds to?". Commit ff7888dcc6c701 is an
> example of the conversion (though that input device
> is somewhat simpler than a full real keyboard).

Sure, I'll have a look and see if I can manage it - removing legacy APIs is always a 
good thing :)


ATB,

Mark.

Re: [PATCH 13/36] next-cube: move ESCC to be QOM child of next-pc device

[Thomas Huth]

Am Mon, 28 Oct 2024 22:28:58 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

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

Ok, fair point, then let's keep this patch as it is!

 Thomas

Re: [PATCH 09/36] next-cube: move SCSI CSRs from next-pc to the next-scsi device

[Thomas Huth]

Am Mon, 28 Oct 2024 22:21:20 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> On 28/10/2024 16:21, Thomas Huth wrote:
> 
> > Am Wed, 23 Oct 2024 09:58:25 +0100
> > 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 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 266f57ac63..32466a425f 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;  
> > 
> > The old code never set the scsi_csr_x directly like this, so I'm not sure
> > whether this is right?  
> 
> Well I initially did this on a hunch that something had gone wrong with an earlier 
> refactoring, but I just did a quick check with Previous and it also treats them as 
> normal registers (see 
> https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/esp.c#l160). So I think 
> this should be fine for now?

Ok, fine for me, but then please mention it in the commit description!

> > Also, maybe best squash this patch together with the next patch, otherwise
> > this is temporary change in behaviour, isn't it?  
> 
> If possible could I keep it as-is? It just means there is separation between the 
> change of the memory region topology and then consolidating the SCSI CSR access 
> routines in the following patch.

Sure, it was just a suggestion! Keep it separate if you prefer it that way.

 Thomas

Re: [PATCH 14/36] next-cube: move timer MMIO to separate memory region on next-pc device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:30 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 63 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 13 deletions(-)

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

Re: [PATCH 15/36] next-cube: move en ethernet MMIO to separate memory region on next-pc device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:31 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 48 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 8 deletions(-)

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

Re: [PATCH 17/36] next-cube: remove unused next.scr memory region

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:33 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 73 +++++++++++----------------------------------
>  1 file changed, 18 insertions(+), 55 deletions(-)

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

Re: [PATCH 18/36] next-cube: rearrange NeXTState declarations to improve readability

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:34 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 64 ++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)

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

Re: [PATCH 19/36] next-cube: convert next-pc device to use Resettable interface

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:35 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

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

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

Re: [PATCH 20/36] next-cube: rename typedef struct NextRtc to NeXTRTC

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:36 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> This brings the capitalisation in line with the other NeXTCube definitions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

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

Re: [PATCH 21/36] next-cube: use qemu_irq to drive int_status in next_scr2_rtc_update()

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:37 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

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

Re: [PATCH 22/36] next-cube: separate rtc read and write shift logic

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:38 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 138 ++++++++++++++++++++++++--------------------
>  1 file changed, 74 insertions(+), 64 deletions(-)

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

Re: [PATCH 24/36] next-cube: use named gpio to set RTC data bit in scr2

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:40 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> This is in preparation for moving NeXTRTC to its own separate device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)

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

Re: [PATCH 25/36] next-cube: use named gpio to read RTC data bit in scr2

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:41 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> This is in preparation for moving NeXTRTC to its own separate device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 169 ++++++++++++++++++++++++--------------------
>  1 file changed, 92 insertions(+), 77 deletions(-)

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

Re: [PATCH 26/36] next-cube: don't use rtc phase value of -1

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:42 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

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

Re: [PATCH 27/36] next-cube: QOMify NeXTRTC

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:43 +0100
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.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index e4d0083eb0..6b574d39cf 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,22 +1174,6 @@ 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,
> @@ -1297,6 +1326,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)

Shouldn't the next_rtc_vmstate get removed from next_pc_vmstate now?

Also, should we finally move the RTC code to a separate file?

 Thomas

Re: [PATCH 28/36] next-cube: move reset of next-rtc fields from next-pc to next-rtc

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:44 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

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

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

Re: [PATCH 29/36] next-cube: move rtc-data-in gpio from next-pc to next-rtc device

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:45 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  hw/m68k/next-cube.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

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

Re: [PATCH 31/36] next-cube: add rtc-cmd-reset named gpio to reset the rtc state machine

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:47 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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 bd24359913..16b16e9956 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);
>      }
>  }

Don't we also need a spot where the gpio gets lowered again?

 Thomas

Re: [PATCH 33/36] next-cube: move next_rtc_cmd_is_write() and next_rtc_data_in_irq() functions

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:49 +0100
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(-)

Alternatively, move the rtc code to a separate file?

 Thomas

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

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:50 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  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 076c9d1f3a..ece63f20b1 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);

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

Re: [PATCH 35/36] next-cube: add my copyright to the top of the file

[Thomas Huth]

Am Wed, 23 Oct 2024 09:58:51 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> 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>
> ---
>  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 ece63f20b1..eefb372dca 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

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

Re: [PATCH 27/36] next-cube: QOMify NeXTRTC

[Mark Cave-Ayland]

On 09/11/2024 08:14, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:43 +0100
> 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.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 48 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index e4d0083eb0..6b574d39cf 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,22 +1174,6 @@ 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,
>> @@ -1297,6 +1326,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)
> 
> Shouldn't the next_rtc_vmstate get removed from next_pc_vmstate now?

Indeed, yes it should. I'll fix that up for v2.

> Also, should we finally move the RTC code to a separate file?

I was thinking about whether it was worth splitting up next-cube.c whilst writing 
this series, but I didn't come up with a solution I was completely happy with. It is 
certainly something to reconsider in future though.


ATB,

Mark.

Re: [PATCH 31/36] next-cube: add rtc-cmd-reset named gpio to reset the rtc state machine

[Mark Cave-Ayland]

On 09/11/2024 08:24, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:47 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> 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 bd24359913..16b16e9956 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);
>>       }
>>   }
> 
> Don't we also need a spot where the gpio gets lowered again?

It's not strictly necessary in this particular case because the IRQ handler 
implements the reset directly when the gpio is raised, as opposed to it being a 
stateful signal like the others.


ATB,

Mark.

Re: [PATCH 33/36] next-cube: move next_rtc_cmd_is_write() and next_rtc_data_in_irq() functions

[Mark Cave-Ayland]

On 09/11/2024 08:25, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:49 +0100
> 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(-)
> 
> Alternatively, move the rtc code to a separate file?

I believe I've already responded to this in a reply to one of your earlier comments :)


ATB,

Mark.