Re: [Qemu-devel] [PATCH v7 02/10] esp: add pseudo-DMA as used by Macintosh

From: Thomas Huth <huth@tuxfamily.org>
To: Laurent Vivier <laurent@vivier.eu>
Cc: "Fam Zheng" <fam@euphon.net>, "Kevin Wolf" <kwolf@redhat.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	qemu-block@nongnu.org, "Jason Wang" <jasowang@redhat.com>,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	qemu-devel@nongnu.org, "Max Reitz" <mreitz@redhat.com>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [Qemu-devel] [PATCH v7 02/10] esp: add pseudo-DMA as used by Macintosh
Date: Sun, 26 May 2019 18:20:41 +0200	[thread overview]
Message-ID: <20190526182041.3a41e568@thl530.multi.box> (raw)
In-Reply-To: <20190525225013.13916-3-laurent@vivier.eu>

Am Sun, 26 May 2019 00:50:05 +0200
schrieb Laurent Vivier <laurent@vivier.eu>:

> There is no DMA in Quadra 800, so the CPU reads/writes the data from
> the PDMA register (offset 0x100, ESP_PDMA in hw/m68k/q800.c) and
> copies them to/from the memory.
> 
> There is a nice assembly loop in the kernel to do that, see
> linux/drivers/scsi/mac_esp.c:MAC_ESP_PDMA_LOOP().
> 
> The start of the transfer is triggered by the DREQ interrupt (see
> linux mac_esp_send_pdma_cmd()), the CPU polls on the IRQ flag to
> start the transfer after a SCSI command has been sent (in Quadra 800
> it goes through the VIA2, the via2-irq line and the vIFR register)
> 
> The Macintosh hardware includes hardware handshaking to prevent the
> CPU from reading invalid data or writing data faster than the
> peripheral device can accept it.
> 
> This is the "blind mode", and from the doc:
> "Approximate maximum SCSI transfer rates within a blocks are 1.4 MB
> per second for blind transfers in the Macintosh II"
> 
> Some references can be found in:
>   Apple Macintosh Family Hardware Reference, ISBN 0-201-19255-1
>   Guide to the Macintosh Family Hardware, ISBN-0-201-52405-8
> 
> Co-developed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  hw/scsi/esp.c         | 293
> +++++++++++++++++++++++++++++++++++++----- include/hw/scsi/esp.h |
> 7 + 2 files changed, 271 insertions(+), 29 deletions(-)
[...]
> @@ -162,6 +196,15 @@ static void do_cmd(ESPState *s, uint8_t *buf)
>      do_busid_cmd(s, &buf[1], busid);
>  }
>  
> +static void satn_pdma_cb(ESPState *s)
> +{
> +    if (get_cmd_cb(s) < 0) {
> +        return;
> +    }
> +    if (s->pdma_cur != s->pdma_start)
> +        do_cmd(s, s->pdma_start);

Curly braces, please.

> +}
> +
>  static void handle_satn(ESPState *s)
>  {
>      uint8_t buf[32];
> @@ -171,11 +214,21 @@ static void handle_satn(ESPState *s)
>          s->dma_cb = handle_satn;
>          return;
>      }
> +    s->pdma_cb = satn_pdma_cb;
>      len = get_cmd(s, buf, sizeof(buf));
>      if (len)
>          do_cmd(s, buf);
>  }
>  
> +static void s_without_satn_pdma_cb(ESPState *s)
> +{
> +    if (get_cmd_cb(s) < 0) {
> +        return;
> +    }
> +    if (s->pdma_cur != s->pdma_start)
> +        do_busid_cmd(s, s->pdma_start, 0);

dito.

> +}
> +
>  static void handle_s_without_atn(ESPState *s)
>  {
>      uint8_t buf[32];
[...]
> @@ -370,14 +526,14 @@ static void handle_ti(ESPState *s)
>          s->dma_left = minlen;
>          s->rregs[ESP_RSTAT] &= ~STAT_TC;
>          esp_do_dma(s);
> -    }
> -    if (s->do_cmd) {
> +    } else if (s->do_cmd) {
>          trace_esp_handle_ti_cmd(s->cmdlen);
>          s->ti_size = 0;
>          s->cmdlen = 0;
>          s->do_cmd = 0;
>          do_cmd(s, s->cmdbuf);
>      }
> +
>  }

Superfluous empty line.

Apart from those nits (which can be fixed when you apply the patches),
the patch looks fine to me.

 Thomas