Am Mon, 28 Oct 2024 22:21:20 +0000
schrieb Mark Cave-Ayland <mark.cave-ayl...@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-ayl...@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-ayl...@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
