Re: [PATCH v14 1/9] esp: add pseudo-DMA as used by Macintosh

[Laurent Vivier]

Le 22/10/2019 à 14:21, Philippe Mathieu-Daudé a écrit :
> Hi Laurent,
> 
> On 10/22/19 1:17 PM, Laurent Vivier wrote:
>> 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
>>
>> Acked-by: Dr. David Alan Gilbert <address@hidden>
>> Co-developed-by: Mark Cave-Ayland <address@hidden>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> Signed-off-by: Laurent Vivier <address@hidden>
>> ---
>>   include/hw/scsi/esp.h |  15 ++
>>   hw/scsi/esp.c         | 338 ++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 324 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
>> index adab63d1c9..6ba47dac41 100644
>> --- a/include/hw/scsi/esp.h
>> +++ b/include/hw/scsi/esp.h
>> @@ -14,10 +14,18 @@ typedef void (*ESPDMAMemoryReadWriteFunc)(void
>> *opaque, uint8_t *buf, int len);
>>     typedef struct ESPState ESPState;
>>   +enum pdma_origin_id {
>> +    PDMA,
>> +    TI,
>> +    CMD,
>> +    ASYNC,
>> +};
>> +
>>   struct ESPState {
>>       uint8_t rregs[ESP_REGS];
>>       uint8_t wregs[ESP_REGS];
>>       qemu_irq irq;
>> +    qemu_irq irq_data;
>>       uint8_t chip_id;
>>       bool tchi_written;
>>       int32_t ti_size;
>> @@ -48,6 +56,12 @@ struct ESPState {
>>       ESPDMAMemoryReadWriteFunc dma_memory_write;
>>       void *dma_opaque;
>>       void (*dma_cb)(ESPState *s);
>> +    uint8_t pdma_buf[32];
>> +    int pdma_origin;
>> +    uint32_t pdma_len;
>> +    uint32_t pdma_start;
>> +    uint32_t pdma_cur;
>> +    void (*pdma_cb)(ESPState *s);
>>   };
>>     #define TYPE_ESP "esp"
>> @@ -59,6 +73,7 @@ typedef struct {
>>       /*< public >*/
>>         MemoryRegion iomem;
>> +    MemoryRegion pdma;
>>       uint32_t it_shift;
>>       ESPState esp;
>>   } SysBusESPState;
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 841d79b60e..90b40c4cb5 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -38,6 +38,8 @@
>>    *
>> http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/NCR89C100.txt
>>
>>    * and
>>    *
>> http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/NCR53C9X.txt
>>
>> + *
>> + * On Macintosh Quadra it is a NCR53C96.
>>    */
>>     static void esp_raise_irq(ESPState *s)
>> @@ -58,6 +60,16 @@ static void esp_lower_irq(ESPState *s)
>>       }
>>   }
>>   +static void esp_raise_drq(ESPState *s)
>> +{
>> +    qemu_irq_raise(s->irq_data);
>> +}
>> +
>> +static void esp_lower_drq(ESPState *s)
>> +{
>> +    qemu_irq_lower(s->irq_data);
>> +}
>> +
>>   void esp_dma_enable(ESPState *s, int irq, int level)
>>   {
>>       if (level) {
>> @@ -84,29 +96,35 @@ void esp_request_cancelled(SCSIRequest *req)
>>       }
>>   }
>>   -static uint32_t get_cmd(ESPState *s, uint8_t *buf, uint8_t buflen)
>> +static void set_pdma(ESPState *s, enum pdma_origin_id origin,
>> +                     uint32_t index, uint32_t len)
>> +{
>> +    s->pdma_origin = origin;
>> +    s->pdma_start = index;
>> +    s->pdma_cur = index;
>> +    s->pdma_len = len;
> 
> Can you pass the pdma_cb to this function, and:
> 
>        s->pdma_cb = pdma_cb;

I agree but I tried that in the past, but in get_cmd() we call
set_pdma() without setting pdma_cb, so no.

>> @@ -252,10 +397,25 @@ static void esp_do_dma(ESPState *s)
>>           trace_esp_do_dma(s->cmdlen, len);
>>           assert (s->cmdlen <= sizeof(s->cmdbuf) &&
>>                   len <= sizeof(s->cmdbuf) - s->cmdlen);
>> -        s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
>> +        if (s->dma_memory_read) {
>> +            s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen],
>> len);
>> +        } else {
>> +            set_pdma(s, CMD, s->cmdlen, len);
>> +            s->pdma_cb = do_dma_pdma_cb;
>> +            esp_raise_drq(s);
>> +            return;
>> +        }
>> +        trace_esp_handle_ti_cmd(s->cmdlen);
>> +        s->ti_size = 0;
>> +        s->cmdlen = 0;
>> +        s->do_cmd = 0;
>> +        do_cmd(s, s->cmdbuf);
>>           return;
>>       }
>>       if (s->async_len == 0) {
>> +        if (s->dma_left == 0) {
>> +            esp_dma_done(s);
>> +        }
> 
> This change seems unrelated to PDMA, is this a fix?

All changes are related to PDMA. It's complicated enough to not mix
several things.

This change is related to your following comment:

>> @@ -373,8 +547,7 @@ 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) {
> 
> Hmmm, unrelated too?

It's needed, see my answer to Thomas question for the v6 of the patch:

https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg06028.html

"The "else" is needed because this code has been duplicated inside
esp_do_dma() to be executed only in the case of "real" dma and not for
pseudo-dma."

Thanks,
Laurent
[1] I wrote this patch 7 years ago, so no more obvious or detailed
comments come to my mind at the moment.