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

[Thomas Huth <huth@tuxfamily.org>]

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