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

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

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.

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

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.

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>

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.