Re: [PATCH v2 38/42] esp: convert ti_buf from array to Fifo8

[Mark Cave-Ayland]

On 23/02/2021 18:49, Philippe Mathieu-Daudé wrote:

> On 2/9/21 8:30 PM, Mark Cave-Ayland wrote:
> > Rename TI_BUFSZ to ESP_FIFO_SZ since this constant is really describing the size
> > of the FIFO and is not directly related to the TI size.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/scsi/esp.c         | 117 ++++++++++++++++++++++++++----------------
> >   include/hw/scsi/esp.h |   8 +--
> >   2 files changed, 79 insertions(+), 46 deletions(-)
>
> > @@ -806,11 +818,9 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
> > val)
> >               } else {
> >                   trace_esp_error_fifo_overrun();
> >               }
> > -        } else if (s->ti_wptr == TI_BUFSZ - 1) {
> > -            trace_esp_error_fifo_overrun();
> >           } else {
> >               s->ti_size++;
> > -            s->ti_buf[s->ti_wptr++] = val & 0xff;
> > +            esp_fifo_push(s, val & 0xff);
>
> Personally I'd drop the '& 0xff' part.

I left it as it was so that it was direct translation of the code it was replacing, 
but I can easily drop it.

> >           }
> >   
> >           /* Non-DMA transfers raise an interrupt after every byte */
> > @@ -839,8 +849,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
> > val)
> >           case CMD_FLUSH:
> >               trace_esp_mem_writeb_cmd_flush(val);
> >               /*s->ti_size = 0;*/
>
> Is this comment still meaningful?

This line can also be removed, so I will make this change for v3.

> > -            s->ti_wptr = 0;
> > -            s->ti_rptr = 0;
> > +            fifo8_reset(&s->fifo);
> >               break;
> >           case CMD_RESET:
> >               trace_esp_mem_writeb_cmd_reset(val);
> > @@ -958,11 +967,18 @@ static int esp_pre_save(void *opaque)
> >   static int esp_post_load(void *opaque, int version_id)
> >   {
> >       ESPState *s = ESP(opaque);
> > +    int len, i;
> >   
> >       version_id = MIN(version_id, s->mig_version_id);
> >   
> >       if (version_id < 5) {
> >           esp_set_tc(s, s->mig_dma_left);
> > +
> > +        /* Migrate ti_buf to fifo */
> > +        len = s->mig_ti_wptr - s->mig_ti_rptr;
> > +        for (i = 0; i < len; i++) {
> > +            fifo8_push(&s->fifo, s->mig_ti_buf[i]);
>
> Again I dare to add:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thank you :)


ATB,

Mark.