[SeaBIOS] Re: [PATCH 2/2] esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion

[Paul Menzel]

[Cc: +Paolo as original author of the driver]

Dear Mark,

Thank you for your patch.

Am 29.07.23 um 15:04 schrieb Mark Cave-Ayland:
> The ESP SELATN command used to send SCSI commands from the ESP to thes SCSI bus

s/thes/the/
> is not a DMA command and therefore does not affect the STAT_TC bit. The only
> reason this works at all is due to a bug in QEMU which (currently) always
> updates the STAT_TC bit in ESP_RSTAT regardless of the state of the ESP_CMD_DMA
> bit.

I’d appreciated a link to these QEMU patches, describing the problem in 
QEMU.
> According to the NCR datasheet the INTR_BS/INTR_FC bits are set when the SELATN

Could you please mention the full name and revision of the datasheet?
> command has completed, so update the existing logic to check for these bits in
> ESP_RINTR instead. Note that the read of ESP_RINTR needs to be restricted to
> state == 0 as reading ESP_RINTR resets the ESP_RSTAT register which breaks the
> STAT_TC check when state == 1.
>
> This commit also includes an extra read of ESP_INTR to clear all the interrupt
> bits before submitting the SELATN command to ensure that we don't accidentally
> immediately progress to the data phase handling logic where ESP_RINTR bits have
> already been set by a previous ESP command.

It’d be great, if you added the QEMU commands how to test your patch.

Lastly, will SeaBIOS built with your patches still work with older QEMU 
versions without your QEMU patches?
> Signed-off-by: Mark Cave-Ayland
>
> src/hw/esp-scsi.c | 36 ++++++++++++++++++++++--------------
>   1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/src/hw/esp-scsi.c b/src/hw/esp-scsi.c
> index e4815aa..2d2d915 100644
> --- a/src/hw/esp-scsi.c
> +++ b/src/hw/esp-scsi.c
> @@ -57,6 +57,8 @@
>   #define ESP_STAT_MSG     0x04
>   #define ESP_STAT_TC      0x10
>
> +#define ESP_INTR_FC      0x08
> +#define ESP_INTR_BS      0x10
>   #define ESP_INTR_DC      0x20
>
> struct esp_lun_s {
> @@ -97,8 +99,9 @@ esp_scsi_process_op(struct disk_op_s *op)
>   outb(target, iobase + ESP_WBUSID);
>
> /* Clear FIFO before sending command.  */
>
> /* Clear FIFO and interrupts before sending command.  */
>  outb(ESP_CMD_FLUSH, iobase + ESP_CMD);
>
> inb(iobase + ESP_RINTR);
>
> /*
> We need to pass the LUN at the beginning of the command, and the FIFO
>
> @@ -115,22 +118,27 @@ esp_scsi_process_op(struct disk_op_s *op)
>   for (state = 0;;) {
>       u8 stat = inb(iobase + ESP_RSTAT);
>    u8 intr;
>
>    /* Detect disconnected device.  */
>    if (state == 0 && (inb(iobase + ESP_RINTR) & ESP_INTR_DC)) {
>        return DISK_RET_ENOTREADY;
>    }
>    if (state == 0) {
>        intr = inb(iobase + ESP_RINTR);
>
>    /* HBA reads command, clears CD, sets TC -> do DMA if needed.  */
>    if (state == 0 && (stat & ESP_STAT_TC)) {
>        state++;
>        if (op->count && blocksize) {
>            /* Data phase.  */
>            u32 count = (u32)op->count * blocksize;
>            esp_scsi_dma(iobase, (u32)op->buf_fl, count, scsi_is_read(op));
>            outb(ESP_CMD_TI | ESP_CMD_DMA, iobase + ESP_CMD);
>            continue;
>        /* Detect disconnected device.  */
>        if (intr & ESP_INTR_DC) {
>            return DISK_RET_ENOTREADY;
>         }
>        /* HBA reads command, executes it, sets BS/FC -> do DMA if needed.  */
>        if (intr & (ESP_INTR_BS | ESP_INTR_FC)) {
>            state++;
>            if (op->count && blocksize) {
>                /* Data phase.  */
>                u32 count = (u32)op->count * blocksize;
>                esp_scsi_dma(iobase, (u32)op->buf_fl, count, scsi_is_read(op));
>                outb(ESP_CMD_TI | ESP_CMD_DMA, iobase + ESP_CMD);
>                continue;
>            }
>    }
>     }
>
>     /* At end of DMA TC is set again -> complete command.  */

Reviewed-by: Paul Menzel

Kind regards,

Paul