[v2] esp: avoid explicit setting of DRQ within ESP state machine

[PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()

Posted by Mark Cave-Ayland 1 year, 11 months ago

The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
underlying Fifo8 functions directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f8230c74b3..100560244b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -309,6 +309,8 @@ static void do_command_phase(ESPState *s)
 
 static void do_message_phase(ESPState *s)
 {
+    uint32_t n;
+
     if (s->cmdfifo_cdb_offset) {
         uint8_t message = esp_fifo_pop(&s->cmdfifo);
 
@@ -320,7 +322,10 @@ static void do_message_phase(ESPState *s)
     /* Ignore extended messages for now */
     if (s->cmdfifo_cdb_offset) {
         int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
-        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
+
+        if (len) {
+            fifo8_pop_buf(&s->cmdfifo, len, &n);
+        }
         s->cmdfifo_cdb_offset = 0;
     }
 }
-- 
2.39.2

Re: [PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()

Posted by Philippe Mathieu-Daudé 1 year, 11 months ago

On 13/3/24 09:57, Mark Cave-Ayland wrote:
> The aim is to restrict the esp_fifo_*() functions so that they only operate on
> the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
> underlying Fifo8 functions directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index f8230c74b3..100560244b 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -309,6 +309,8 @@ static void do_command_phase(ESPState *s)
>   
>   static void do_message_phase(ESPState *s)
>   {
> +    uint32_t n;
> +
>       if (s->cmdfifo_cdb_offset) {
>           uint8_t message = esp_fifo_pop(&s->cmdfifo);
>   
> @@ -320,7 +322,10 @@ static void do_message_phase(ESPState *s)
>       /* Ignore extended messages for now */
>       if (s->cmdfifo_cdb_offset) {
>           int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
> -        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
> +
> +        if (len) {
> +            fifo8_pop_buf(&s->cmdfifo, len, &n);

'n' is unused, use NULL?

> +        }
>           s->cmdfifo_cdb_offset = 0;
>       }
>   }

Re: [PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()

Posted by Philippe Mathieu-Daudé 1 year, 11 months ago

On 13/3/24 12:03, Philippe Mathieu-Daudé wrote:
> On 13/3/24 09:57, Mark Cave-Ayland wrote:
>> The aim is to restrict the esp_fifo_*() functions so that they only 
>> operate on
>> the hardware FIFO. When reading from cmdfifo in do_message_phase() use 
>> the
>> underlying Fifo8 functions directly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index f8230c74b3..100560244b 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -309,6 +309,8 @@ static void do_command_phase(ESPState *s)
>>   static void do_message_phase(ESPState *s)
>>   {
>> +    uint32_t n;
>> +
>>       if (s->cmdfifo_cdb_offset) {
>>           uint8_t message = esp_fifo_pop(&s->cmdfifo);
>> @@ -320,7 +322,10 @@ static void do_message_phase(ESPState *s)
>>       /* Ignore extended messages for now */
>>       if (s->cmdfifo_cdb_offset) {
>>           int len = MIN(s->cmdfifo_cdb_offset, 
>> fifo8_num_used(&s->cmdfifo));
>> -        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
>> +
>> +        if (len) {
>> +            fifo8_pop_buf(&s->cmdfifo, len, &n);
> 
> 'n' is unused, use NULL?

Using NULL:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>