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

[Thomas Huth <huth@tuxfamily.org>]

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