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

[Thomas Huth <huth@tuxfamily.org>]

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