[PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()

[PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()

[Chalapathi V]

Hello,

v3:
1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
2. Use of PnvXferBuffer results in a additonal process overhead due to
frequent dynamic allocations and hence use an existing Fifo8 buffer.
3. Use a local variable seq_index and use it with in while loop instead
of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
overrun when seq_index is incremented.

Tested:
passed make check and make check-avocado

Supersedes: <20240807202804.56038-1-philmd@linaro.org>

Philippe Mathieu-Daudé (1):
  MAINTAINERS: Cover PowerPC SPI model in PowerNV section

Chalapathi V (2):
  hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
  hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().

 MAINTAINERS              |   2 +
 include/hw/ssi/pnv_spi.h |   3 +
 hw/ssi/pnv_spi.c         | 228 +++++++++++++++------------------------
 3 files changed, 89 insertions(+), 144 deletions(-)

-- 
2.39.5

[PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section

[Chalapathi V]

From: "Philippe Mathieu-Daudé" <philmd@linaro.org>

It is unfair to let the PowerNV SPI model to the SSI
maintainers. Also include the PowerNV ones.

Fixes: 29318db133 ("hw/ssi: Add SPI model")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ffacd60f40..b11c4edaf0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1541,8 +1541,10 @@ F: hw/ppc/pnv*
 F: hw/intc/pnv*
 F: hw/intc/xics_pnv.c
 F: hw/pci-host/pnv*
+F: hw/ssi/pnv_spi.c
 F: include/hw/ppc/pnv*
 F: include/hw/pci-host/pnv*
+F: hw/ssi/pnv_spi*
 F: pc-bios/skiboot.lid
 F: tests/qtest/pnv*
 F: tests/functional/test_ppc64_powernv.py
-- 
2.39.5

Re: [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section

[Nicholas Piggin]

On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>
> It is unfair to let the PowerNV SPI model to the SSI
> maintainers. Also include the PowerNV ones.
>
> Fixes: 29318db133 ("hw/ssi: Add SPI model")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ffacd60f40..b11c4edaf0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1541,8 +1541,10 @@ F: hw/ppc/pnv*
>  F: hw/intc/pnv*
>  F: hw/intc/xics_pnv.c
>  F: hw/pci-host/pnv*
> +F: hw/ssi/pnv_spi.c
>  F: include/hw/ppc/pnv*
>  F: include/hw/pci-host/pnv*
> +F: hw/ssi/pnv_spi*

Should be include/hw/ssi/pnv_spi*

Otherwise,

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>  F: pc-bios/skiboot.lid
>  F: tests/qtest/pnv*
>  F: tests/functional/test_ppc64_powernv.py

Re: [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section

[Chalapathi V]

On 08-10-2024 13:14, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>>
>> It is unfair to let the PowerNV SPI model to the SSI
>> maintainers. Also include the PowerNV ones.
>>
>> Fixes: 29318db133 ("hw/ssi: Add SPI model")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   MAINTAINERS | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ffacd60f40..b11c4edaf0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1541,8 +1541,10 @@ F: hw/ppc/pnv*
>>   F: hw/intc/pnv*
>>   F: hw/intc/xics_pnv.c
>>   F: hw/pci-host/pnv*
>> +F: hw/ssi/pnv_spi.c
>>   F: include/hw/ppc/pnv*
>>   F: include/hw/pci-host/pnv*
>> +F: hw/ssi/pnv_spi*
> Should be include/hw/ssi/pnv_spi*

Sure. Will update in next version.

Thank You,

Chalapathi

>
> Otherwise,
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>>   F: pc-bios/skiboot.lid
>>   F: tests/qtest/pnv*
>>   F: tests/functional/test_ppc64_powernv.py

[PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure

[Chalapathi V]

In PnvXferBuffer dynamically allocating and freeing is a
process overhead. Hence used an existing Fifo8 buffer with
capacity of 16 bytes.

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 include/hw/ssi/pnv_spi.h |   3 +
 hw/ssi/pnv_spi.c         | 167 +++++++++++++--------------------------
 2 files changed, 56 insertions(+), 114 deletions(-)

diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
index 8815f67d45..9878d9a25f 100644
--- a/include/hw/ssi/pnv_spi.h
+++ b/include/hw/ssi/pnv_spi.h
@@ -23,6 +23,7 @@
 
 #include "hw/ssi/ssi.h"
 #include "hw/sysbus.h"
+#include "qemu/fifo8.h"
 
 #define TYPE_PNV_SPI "pnv-spi"
 OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
@@ -37,6 +38,8 @@ typedef struct PnvSpi {
     SSIBus *ssi_bus;
     qemu_irq *cs_line;
     MemoryRegion    xscom_spic_regs;
+    Fifo8 tx_fifo;
+    Fifo8 rx_fifo;
     /* SPI object number */
     uint32_t        spic_num;
     uint8_t         transfer_len;
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 9e7207bf7c..2fd5aa0a96 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -19,6 +19,7 @@
 
 #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
 #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
+#define PNV_SPI_FIFO_SIZE 16
 
 /*
  * Macro from include/hw/ppc/fdt.h
@@ -35,38 +36,6 @@
         }                                                          \
     } while (0)
 
-/* PnvXferBuffer */
-typedef struct PnvXferBuffer {
-
-    uint32_t    len;
-    uint8_t    *data;
-
-} PnvXferBuffer;
-
-/* pnv_spi_xfer_buffer_methods */
-static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
-{
-    PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
-
-    return payload;
-}
-
-static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
-{
-    free(payload->data);
-    free(payload);
-}
-
-static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
-                uint32_t offset, uint32_t length)
-{
-    if (payload->len < (offset + length)) {
-        payload->len = offset + length;
-        payload->data = g_realloc(payload->data, payload->len);
-    }
-    return &payload->data[offset];
-}
-
 static bool does_rdr_match(PnvSpi *s)
 {
     /*
@@ -107,8 +76,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
     return byte;
 }
 
-static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
-                uint8_t ecc_count, uint8_t shift_in_count)
+static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
+                uint8_t shift_in_count)
 {
     uint8_t byte;
     int count = 0;
@@ -118,8 +87,8 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
         if ((ecc_count != 0) &&
             (shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
             shift_in_count = 0;
-        } else {
-            byte = read_buf[count];
+        } else if (!fifo8_is_empty(&s->rx_fifo)) {
+            byte = fifo8_pop(&s->rx_fifo);
             trace_pnv_spi_shift_rx(byte, count);
             s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
         }
@@ -128,7 +97,7 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
     return shift_in_count;
 }
 
-static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
+static void spi_response(PnvSpi *s)
 {
     uint8_t ecc_count;
     uint8_t shift_in_count;
@@ -144,13 +113,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
      * First check that the response payload is the exact same
      * number of bytes as the request payload was
      */
-    if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
+    if ((&s->rx_fifo)->num != (s->N1_bytes + s->N2_bytes)) {
         qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
                        "bytes, expected %d, got %d\n",
-                       (s->N1_bytes + s->N2_bytes), rsp_payload->len);
+                       (s->N1_bytes + s->N2_bytes), (&s->rx_fifo)->num);
     } else {
         uint8_t ecc_control;
-        trace_pnv_spi_rx_received(rsp_payload->len);
+        trace_pnv_spi_rx_received((&s->rx_fifo)->num);
         trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
                         s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
         /*
@@ -175,14 +144,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
         /* Handle the N1 portion of the frame first */
         if (s->N1_rx != 0) {
             trace_pnv_spi_rx_read_N1frame();
-            shift_in_count = read_from_frame(s, &rsp_payload->data[0],
-                            s->N1_bytes, ecc_count, shift_in_count);
+            shift_in_count = read_from_frame(s, s->N1_bytes,
+                            ecc_count, shift_in_count);
         }
         /* Handle the N2 portion of the frame */
         if (s->N2_rx != 0) {
             trace_pnv_spi_rx_read_N2frame();
-            shift_in_count = read_from_frame(s,
-                            &rsp_payload->data[s->N1_bytes], s->N2_bytes,
+            shift_in_count = read_from_frame(s, s->N2_bytes,
                             ecc_count, shift_in_count);
         }
         if ((s->N1_rx + s->N2_rx) > 0) {
@@ -210,34 +178,36 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
     } /* end of else */
 } /* end of spi_response() */
 
-static void transfer(PnvSpi *s, PnvXferBuffer *payload)
+static void transfer(PnvSpi *s)
 {
-    uint32_t tx;
-    uint32_t rx;
-    PnvXferBuffer *rsp_payload = NULL;
+    uint32_t tx, rx, payload_len;
+    uint8_t rx_byte;
 
-    rsp_payload = pnv_spi_xfer_buffer_new();
-    for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
+    payload_len = (&s->tx_fifo)->num;
+    for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
         tx = 0;
         for (int i = 0; i < s->transfer_len; i++) {
-            if ((offset + i) >= payload->len) {
+            if ((offset + i) >= payload_len) {
                 tx <<= 8;
-            } else {
-                tx = (tx << 8) | payload->data[offset + i];
+            } else if (!fifo8_is_empty(&s->tx_fifo)) {
+                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
             }
         }
         rx = ssi_transfer(s->ssi_bus, tx);
         for (int i = 0; i < s->transfer_len; i++) {
-            if ((offset + i) >= payload->len) {
+            if ((offset + i) >= payload_len) {
                 break;
             }
-            *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
-                    (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
+            rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
+            if (!fifo8_is_full(&s->rx_fifo)) {
+                fifo8_push(&s->rx_fifo, rx_byte);
+            }
         }
     }
-    if (rsp_payload != NULL) {
-        spi_response(s, s->N1_bits, rsp_payload);
-    }
+    spi_response(s);
+    /* Reset fifo for next frame */
+    fifo8_reset(&s->tx_fifo);
+    fifo8_reset(&s->rx_fifo);
 }
 
 static inline uint8_t get_seq_index(PnvSpi *s)
@@ -348,19 +318,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
 /*
  * Shift_N1 operation handler method
  */
-static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
-                       PnvXferBuffer **payload, bool send_n1_alone)
+static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
 {
     uint8_t n1_count;
     bool stop = false;
-
-    /*
-     * If there isn't a current payload left over from a stopped sequence
-     * create a new one.
-     */
-    if (*payload == NULL) {
-        *payload = pnv_spi_xfer_buffer_new();
-    }
     /*
      * Use a combination of N1 counters to build the N1 portion of the
      * transmit payload.
@@ -411,9 +372,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
                  */
                 uint8_t n1_byte = 0x00;
                 n1_byte = get_from_offset(s, n1_count);
-                trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
-                *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
-                        n1_byte;
+                if (!fifo8_is_full(&s->tx_fifo)) {
+                    trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
+                    fifo8_push(&s->tx_fifo, n1_byte);
+                }
             } else {
                 /*
                  * We hit a shift_n1 opcode TX but the TDR is empty, tell the
@@ -440,10 +402,9 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
                                 "set for receive but RDR is full");
                 stop = true;
                 break;
-            } else {
+            } else if (!fifo8_is_full(&s->tx_fifo)) {
                 trace_pnv_spi_tx_append_FF("n1_byte");
-                *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
-                        = 0xff;
+                fifo8_push(&s->tx_fifo, 0xff);
             }
         }
         n1_count++;
@@ -484,15 +445,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
      */
     if (send_n1_alone && !stop) {
         /* We have a TX and a full TDR or an RX and an empty RDR */
-        trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
-        transfer(s, *payload);
+        trace_pnv_spi_tx_request("Shifting N1 frame", (&s->tx_fifo)->num);
+        transfer(s);
         /* The N1 frame shift is complete so reset the N1 counters */
         s->N2_bits = 0;
         s->N2_bytes = 0;
         s->N2_tx = 0;
         s->N2_rx = 0;
-        pnv_spi_xfer_buffer_free(*payload);
-        *payload = NULL;
     }
     return stop;
 } /* end of operation_shiftn1() */
@@ -588,19 +547,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
  * Shift_N2 operation handler method
  */
 
-static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
-                       PnvXferBuffer **payload)
+static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
 {
     uint8_t n2_count;
     bool stop = false;
-
-    /*
-     * If there isn't a current payload left over from a stopped sequence
-     * create a new one.
-     */
-    if (*payload == NULL) {
-        *payload = pnv_spi_xfer_buffer_new();
-    }
     /*
      * Use a combination of N2 counters to build the N2 portion of the
      * transmit payload.
@@ -639,25 +589,26 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
             /* Always append data for the N2 segment if it is set for TX */
             uint8_t n2_byte = 0x00;
             n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
-            trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
-            *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
-                    = n2_byte;
-        } else {
+            if (!fifo8_is_full(&s->tx_fifo)) {
+                trace_pnv_spi_tx_append("n2_byte", n2_byte,
+                                (s->N1_tx + n2_count));
+                fifo8_push(&s->tx_fifo, n2_byte);
+            }
+        } else if (!fifo8_is_full(&s->tx_fifo)) {
             /*
              * Regardless of whether or not N2 is set for TX or RX, we need
              * the number of bytes in the payload to match the overall length
              * of the operation.
              */
             trace_pnv_spi_tx_append_FF("n2_byte");
-            *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
-                    = 0xff;
+            fifo8_push(&s->tx_fifo, 0xff);
         }
         n2_count++;
     } /* end of while */
     if (!stop) {
         /* We have a TX and a full TDR or an RX and an empty RDR */
-        trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
-        transfer(s, *payload);
+        trace_pnv_spi_tx_request("Shifting N2 frame", (&s->tx_fifo)->num);
+        transfer(s);
         /*
          * If we are doing an N2 TX and the TDR is full we need to clear the
          * TDR_full status. Do this here instead of up in the loop above so we
@@ -680,8 +631,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
         s->N1_bytes = 0;
         s->N1_tx = 0;
         s->N1_rx = 0;
-        pnv_spi_xfer_buffer_free(*payload);
-        *payload = NULL;
     }
     return stop;
 } /*  end of operation_shiftn2()*/
@@ -699,19 +648,6 @@ static void operation_sequencer(PnvSpi *s)
     uint8_t opcode = 0;
     uint8_t masked_opcode = 0;
 
-    /*
-     * PnvXferBuffer for containing the payload of the SPI frame.
-     * This is a static because there are cases where a sequence has to stop
-     * and wait for the target application to unload the RDR.  If this occurs
-     * during a sequence where N1 is not sent alone and instead combined with
-     * N2 since the N1 tx length + the N2 tx length is less than the size of
-     * the TDR.
-     */
-    static PnvXferBuffer *payload;
-
-    if (payload == NULL) {
-        payload = pnv_spi_xfer_buffer_new();
-    }
     /*
      * Clear the sequencer FSM error bit - general_SPI_status[3]
      * before starting a sequence.
@@ -840,7 +776,7 @@ static void operation_sequencer(PnvSpi *s)
                 }
                 s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
                                 FSM_SHIFT_N1);
-                stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
+                stop = operation_shiftn1(s, opcode, send_n1_alone);
                 if (stop) {
                     /*
                      *  The operation code says to stop, this can occur if:
@@ -895,7 +831,7 @@ static void operation_sequencer(PnvSpi *s)
                 /* Ok to do a Shift_N2 */
                 s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
                                 FSM_SHIFT_N2);
-                stop = operation_shiftn2(s, opcode, &payload);
+                stop = operation_shiftn2(s, opcode);
                 /*
                  * If the operation code says to stop set the shifter state to
                  * wait and stop
@@ -1208,6 +1144,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
     s->cs_line = g_new0(qemu_irq, 1);
     qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
 
+    fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
+    fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
+
     /* spi scoms */
     pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
                           s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);
-- 
2.39.5

Re: [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure

[Nicholas Piggin]

On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> In PnvXferBuffer dynamically allocating and freeing is a
> process overhead. Hence used an existing Fifo8 buffer with
> capacity of 16 bytes.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  include/hw/ssi/pnv_spi.h |   3 +
>  hw/ssi/pnv_spi.c         | 167 +++++++++++++--------------------------
>  2 files changed, 56 insertions(+), 114 deletions(-)
>
> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
> index 8815f67d45..9878d9a25f 100644
> --- a/include/hw/ssi/pnv_spi.h
> +++ b/include/hw/ssi/pnv_spi.h
> @@ -23,6 +23,7 @@
>  
>  #include "hw/ssi/ssi.h"
>  #include "hw/sysbus.h"
> +#include "qemu/fifo8.h"
>  
>  #define TYPE_PNV_SPI "pnv-spi"
>  OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
> @@ -37,6 +38,8 @@ typedef struct PnvSpi {
>      SSIBus *ssi_bus;
>      qemu_irq *cs_line;
>      MemoryRegion    xscom_spic_regs;
> +    Fifo8 tx_fifo;
> +    Fifo8 rx_fifo;
>      /* SPI object number */
>      uint32_t        spic_num;
>      uint8_t         transfer_len;
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 9e7207bf7c..2fd5aa0a96 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -19,6 +19,7 @@
>  
>  #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>  #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
> +#define PNV_SPI_FIFO_SIZE 16
>  
>  /*
>   * Macro from include/hw/ppc/fdt.h
> @@ -35,38 +36,6 @@
>          }                                                          \
>      } while (0)
>  
> -/* PnvXferBuffer */
> -typedef struct PnvXferBuffer {
> -
> -    uint32_t    len;
> -    uint8_t    *data;
> -
> -} PnvXferBuffer;
> -
> -/* pnv_spi_xfer_buffer_methods */
> -static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
> -{
> -    PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
> -
> -    return payload;
> -}
> -
> -static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
> -{
> -    free(payload->data);
> -    free(payload);
> -}
> -
> -static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
> -                uint32_t offset, uint32_t length)
> -{
> -    if (payload->len < (offset + length)) {
> -        payload->len = offset + length;
> -        payload->data = g_realloc(payload->data, payload->len);
> -    }
> -    return &payload->data[offset];
> -}
> -
>  static bool does_rdr_match(PnvSpi *s)
>  {
>      /*
> @@ -107,8 +76,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
>      return byte;
>  }
>  
> -static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
> -                uint8_t ecc_count, uint8_t shift_in_count)
> +static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
> +                uint8_t shift_in_count)
>  {
>      uint8_t byte;
>      int count = 0;
> @@ -118,8 +87,8 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>          if ((ecc_count != 0) &&
>              (shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
>              shift_in_count = 0;
> -        } else {
> -            byte = read_buf[count];
> +        } else if (!fifo8_is_empty(&s->rx_fifo)) {
> +            byte = fifo8_pop(&s->rx_fifo);
>              trace_pnv_spi_shift_rx(byte, count);
>              s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
>          }

What happens to the else case here? Did the previous code underflow
read_buf, or is it a programming error?

> @@ -128,7 +97,7 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>      return shift_in_count;
>  }
>  
> -static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
> +static void spi_response(PnvSpi *s)
>  {
>      uint8_t ecc_count;
>      uint8_t shift_in_count;
> @@ -144,13 +113,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>       * First check that the response payload is the exact same
>       * number of bytes as the request payload was
>       */
> -    if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
> +    if ((&s->rx_fifo)->num != (s->N1_bytes + s->N2_bytes)) {

fifo8_num_used()

>          qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
>                         "bytes, expected %d, got %d\n",
> -                       (s->N1_bytes + s->N2_bytes), rsp_payload->len);
> +                       (s->N1_bytes + s->N2_bytes), (&s->rx_fifo)->num);
>      } else {
>          uint8_t ecc_control;
> -        trace_pnv_spi_rx_received(rsp_payload->len);
> +        trace_pnv_spi_rx_received((&s->rx_fifo)->num);

fifo8_num_used()

>          trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
>                          s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
>          /*
> @@ -175,14 +144,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>          /* Handle the N1 portion of the frame first */
>          if (s->N1_rx != 0) {
>              trace_pnv_spi_rx_read_N1frame();
> -            shift_in_count = read_from_frame(s, &rsp_payload->data[0],
> -                            s->N1_bytes, ecc_count, shift_in_count);
> +            shift_in_count = read_from_frame(s, s->N1_bytes,
> +                            ecc_count, shift_in_count);

Maybe indent this ^ line up to the ( of the function call operator
if possible (not sure it's a hard rule  but it reads a bit better
unless you have really run out of columns)..

>          }
>          /* Handle the N2 portion of the frame */
>          if (s->N2_rx != 0) {
>              trace_pnv_spi_rx_read_N2frame();
> -            shift_in_count = read_from_frame(s,
> -                            &rsp_payload->data[s->N1_bytes], s->N2_bytes,
> +            shift_in_count = read_from_frame(s, s->N2_bytes,
>                              ecc_count, shift_in_count);

Same here, and for other code where you touch nearby lines.

>          }
>          if ((s->N1_rx + s->N2_rx) > 0) {
> @@ -210,34 +178,36 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>      } /* end of else */
>  } /* end of spi_response() */
>  
> -static void transfer(PnvSpi *s, PnvXferBuffer *payload)
> +static void transfer(PnvSpi *s)
>  {
> -    uint32_t tx;
> -    uint32_t rx;
> -    PnvXferBuffer *rsp_payload = NULL;
> +    uint32_t tx, rx, payload_len;
> +    uint8_t rx_byte;
>  
> -    rsp_payload = pnv_spi_xfer_buffer_new();
> -    for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
> +    payload_len = (&s->tx_fifo)->num;

fifo8_num_used()

(And several other cases below should use num_used).

> +    for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
>          tx = 0;
>          for (int i = 0; i < s->transfer_len; i++) {
> -            if ((offset + i) >= payload->len) {
> +            if ((offset + i) >= payload_len) {
>                  tx <<= 8;
> -            } else {
> -                tx = (tx << 8) | payload->data[offset + i];
> +            } else if (!fifo8_is_empty(&s->tx_fifo)) {
> +                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
>              }

Similar question about underflow here. Is there an assert or error
message or trace event for all these over/under flow cases?

>          }
>          rx = ssi_transfer(s->ssi_bus, tx);
>          for (int i = 0; i < s->transfer_len; i++) {
> -            if ((offset + i) >= payload->len) {
> +            if ((offset + i) >= payload_len) {
>                  break;
>              }
> -            *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
> -                    (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
> +            rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
> +            if (!fifo8_is_full(&s->rx_fifo)) {
> +                fifo8_push(&s->rx_fifo, rx_byte);
> +            }
>          }

And overflow, this just gets lost? You can just put || fifo8_is_full()
in the break condition I think?.

>      }
> -    if (rsp_payload != NULL) {
> -        spi_response(s, s->N1_bits, rsp_payload);
> -    }
> +    spi_response(s);
> +    /* Reset fifo for next frame */
> +    fifo8_reset(&s->tx_fifo);
> +    fifo8_reset(&s->rx_fifo);
>  }
>  
>  static inline uint8_t get_seq_index(PnvSpi *s)
> @@ -348,19 +318,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>  /*
>   * Shift_N1 operation handler method
>   */
> -static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
> -                       PnvXferBuffer **payload, bool send_n1_alone)
> +static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
>  {
>      uint8_t n1_count;
>      bool stop = false;
> -
> -    /*
> -     * If there isn't a current payload left over from a stopped sequence
> -     * create a new one.
> -     */
> -    if (*payload == NULL) {
> -        *payload = pnv_spi_xfer_buffer_new();
> -    }
>      /*
>       * Use a combination of N1 counters to build the N1 portion of the
>       * transmit payload.
> @@ -411,9 +372,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>                   */
>                  uint8_t n1_byte = 0x00;
>                  n1_byte = get_from_offset(s, n1_count);
> -                trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
> -                *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
> -                        n1_byte;
> +                if (!fifo8_is_full(&s->tx_fifo)) {
> +                    trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
> +                    fifo8_push(&s->tx_fifo, n1_byte);
> +                }
>              } else {
>                  /*
>                   * We hit a shift_n1 opcode TX but the TDR is empty, tell the
> @@ -440,10 +402,9 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>                                  "set for receive but RDR is full");
>                  stop = true;
>                  break;
> -            } else {
> +            } else if (!fifo8_is_full(&s->tx_fifo)) {
>                  trace_pnv_spi_tx_append_FF("n1_byte");
> -                *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
> -                        = 0xff;
> +                fifo8_push(&s->tx_fifo, 0xff);
>              }
>          }
>          n1_count++;
> @@ -484,15 +445,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>       */
>      if (send_n1_alone && !stop) {
>          /* We have a TX and a full TDR or an RX and an empty RDR */
> -        trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
> -        transfer(s, *payload);
> +        trace_pnv_spi_tx_request("Shifting N1 frame", (&s->tx_fifo)->num);

> +        transfer(s);
>          /* The N1 frame shift is complete so reset the N1 counters */
>          s->N2_bits = 0;
>          s->N2_bytes = 0;
>          s->N2_tx = 0;
>          s->N2_rx = 0;
> -        pnv_spi_xfer_buffer_free(*payload);
> -        *payload = NULL;
>      }
>      return stop;
>  } /* end of operation_shiftn1() */
> @@ -588,19 +547,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>   * Shift_N2 operation handler method
>   */
>  
> -static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
> -                       PnvXferBuffer **payload)
> +static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
>  {
>      uint8_t n2_count;
>      bool stop = false;
> -
> -    /*
> -     * If there isn't a current payload left over from a stopped sequence
> -     * create a new one.
> -     */
> -    if (*payload == NULL) {
> -        *payload = pnv_spi_xfer_buffer_new();
> -    }
>      /*
>       * Use a combination of N2 counters to build the N2 portion of the
>       * transmit payload.
> @@ -639,25 +589,26 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>              /* Always append data for the N2 segment if it is set for TX */
>              uint8_t n2_byte = 0x00;
>              n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
> -            trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
> -            *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
> -                    = n2_byte;
> -        } else {
> +            if (!fifo8_is_full(&s->tx_fifo)) {
> +                trace_pnv_spi_tx_append("n2_byte", n2_byte,
> +                                (s->N1_tx + n2_count));
> +                fifo8_push(&s->tx_fifo, n2_byte);
> +            }
> +        } else if (!fifo8_is_full(&s->tx_fifo)) {
>              /*
>               * Regardless of whether or not N2 is set for TX or RX, we need
>               * the number of bytes in the payload to match the overall length
>               * of the operation.
>               */
>              trace_pnv_spi_tx_append_FF("n2_byte");
> -            *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
> -                    = 0xff;
> +            fifo8_push(&s->tx_fifo, 0xff);
>          }
>          n2_count++;
>      } /* end of while */
>      if (!stop) {
>          /* We have a TX and a full TDR or an RX and an empty RDR */
> -        trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
> -        transfer(s, *payload);
> +        trace_pnv_spi_tx_request("Shifting N2 frame", (&s->tx_fifo)->num);
> +        transfer(s);
>          /*
>           * If we are doing an N2 TX and the TDR is full we need to clear the
>           * TDR_full status. Do this here instead of up in the loop above so we
> @@ -680,8 +631,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>          s->N1_bytes = 0;
>          s->N1_tx = 0;
>          s->N1_rx = 0;
> -        pnv_spi_xfer_buffer_free(*payload);
> -        *payload = NULL;
>      }
>      return stop;
>  } /*  end of operation_shiftn2()*/
> @@ -699,19 +648,6 @@ static void operation_sequencer(PnvSpi *s)
>      uint8_t opcode = 0;
>      uint8_t masked_opcode = 0;
>  
> -    /*
> -     * PnvXferBuffer for containing the payload of the SPI frame.
> -     * This is a static because there are cases where a sequence has to stop
> -     * and wait for the target application to unload the RDR.  If this occurs
> -     * during a sequence where N1 is not sent alone and instead combined with
> -     * N2 since the N1 tx length + the N2 tx length is less than the size of
> -     * the TDR.
> -     */
> -    static PnvXferBuffer *payload;
> -
> -    if (payload == NULL) {
> -        payload = pnv_spi_xfer_buffer_new();
> -    }
>      /*
>       * Clear the sequencer FSM error bit - general_SPI_status[3]
>       * before starting a sequence.
> @@ -840,7 +776,7 @@ static void operation_sequencer(PnvSpi *s)
>                  }
>                  s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>                                  FSM_SHIFT_N1);
> -                stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
> +                stop = operation_shiftn1(s, opcode, send_n1_alone);
>                  if (stop) {
>                      /*
>                       *  The operation code says to stop, this can occur if:
> @@ -895,7 +831,7 @@ static void operation_sequencer(PnvSpi *s)
>                  /* Ok to do a Shift_N2 */
>                  s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>                                  FSM_SHIFT_N2);
> -                stop = operation_shiftn2(s, opcode, &payload);
> +                stop = operation_shiftn2(s, opcode);
>                  /*
>                   * If the operation code says to stop set the shifter state to
>                   * wait and stop
> @@ -1208,6 +1144,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
>      s->cs_line = g_new0(qemu_irq, 1);
>      qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>  
> +    fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
> +    fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
> +
>      /* spi scoms */
>      pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
>                            s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);


Looks nice, simplifies the code and tricky pointer and memory handling.

Thanks,
Nick

Re: [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure

[Chalapathi V]

On 08-10-2024 13:29, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> In PnvXferBuffer dynamically allocating and freeing is a
>> process overhead. Hence used an existing Fifo8 buffer with
>> capacity of 16 bytes.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   include/hw/ssi/pnv_spi.h |   3 +
>>   hw/ssi/pnv_spi.c         | 167 +++++++++++++--------------------------
>>   2 files changed, 56 insertions(+), 114 deletions(-)
>>
>> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
>> index 8815f67d45..9878d9a25f 100644
>> --- a/include/hw/ssi/pnv_spi.h
>> +++ b/include/hw/ssi/pnv_spi.h
>> @@ -23,6 +23,7 @@
>>   
>>   #include "hw/ssi/ssi.h"
>>   #include "hw/sysbus.h"
>> +#include "qemu/fifo8.h"
>>   
>>   #define TYPE_PNV_SPI "pnv-spi"
>>   OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>> @@ -37,6 +38,8 @@ typedef struct PnvSpi {
>>       SSIBus *ssi_bus;
>>       qemu_irq *cs_line;
>>       MemoryRegion    xscom_spic_regs;
>> +    Fifo8 tx_fifo;
>> +    Fifo8 rx_fifo;
>>       /* SPI object number */
>>       uint32_t        spic_num;
>>       uint8_t         transfer_len;
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 9e7207bf7c..2fd5aa0a96 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -19,6 +19,7 @@
>>   
>>   #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>>   #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>> +#define PNV_SPI_FIFO_SIZE 16
>>   
>>   /*
>>    * Macro from include/hw/ppc/fdt.h
>> @@ -35,38 +36,6 @@
>>           }                                                          \
>>       } while (0)
>>   
>> -/* PnvXferBuffer */
>> -typedef struct PnvXferBuffer {
>> -
>> -    uint32_t    len;
>> -    uint8_t    *data;
>> -
>> -} PnvXferBuffer;
>> -
>> -/* pnv_spi_xfer_buffer_methods */
>> -static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
>> -{
>> -    PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
>> -
>> -    return payload;
>> -}
>> -
>> -static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
>> -{
>> -    free(payload->data);
>> -    free(payload);
>> -}
>> -
>> -static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
>> -                uint32_t offset, uint32_t length)
>> -{
>> -    if (payload->len < (offset + length)) {
>> -        payload->len = offset + length;
>> -        payload->data = g_realloc(payload->data, payload->len);
>> -    }
>> -    return &payload->data[offset];
>> -}
>> -
>>   static bool does_rdr_match(PnvSpi *s)
>>   {
>>       /*
>> @@ -107,8 +76,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
>>       return byte;
>>   }
>>   
>> -static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>> -                uint8_t ecc_count, uint8_t shift_in_count)
>> +static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
>> +                uint8_t shift_in_count)
>>   {
>>       uint8_t byte;
>>       int count = 0;
>> @@ -118,8 +87,8 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>>           if ((ecc_count != 0) &&
>>               (shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
>>               shift_in_count = 0;
>> -        } else {
>> -            byte = read_buf[count];
>> +        } else if (!fifo8_is_empty(&s->rx_fifo)) {
>> +            byte = fifo8_pop(&s->rx_fifo);
>>               trace_pnv_spi_shift_rx(byte, count);
>>               s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
>>           }
> What happens to the else case here? Did the previous code underflow
> read_buf, or is it a programming error?

read_buf should not underflow, I will add else with trace.

Thank You

>
>> @@ -128,7 +97,7 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>>       return shift_in_count;
>>   }
>>   
>> -static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> +static void spi_response(PnvSpi *s)
>>   {
>>       uint8_t ecc_count;
>>       uint8_t shift_in_count;
>> @@ -144,13 +113,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>>        * First check that the response payload is the exact same
>>        * number of bytes as the request payload was
>>        */
>> -    if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
>> +    if ((&s->rx_fifo)->num != (s->N1_bytes + s->N2_bytes)) {
> fifo8_num_used()
Sure. Thank You
>
>>           qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
>>                          "bytes, expected %d, got %d\n",
>> -                       (s->N1_bytes + s->N2_bytes), rsp_payload->len);
>> +                       (s->N1_bytes + s->N2_bytes), (&s->rx_fifo)->num);
>>       } else {
>>           uint8_t ecc_control;
>> -        trace_pnv_spi_rx_received(rsp_payload->len);
>> +        trace_pnv_spi_rx_received((&s->rx_fifo)->num);
> fifo8_num_used()
>
>>           trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
>>                           s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
>>           /*
>> @@ -175,14 +144,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>>           /* Handle the N1 portion of the frame first */
>>           if (s->N1_rx != 0) {
>>               trace_pnv_spi_rx_read_N1frame();
>> -            shift_in_count = read_from_frame(s, &rsp_payload->data[0],
>> -                            s->N1_bytes, ecc_count, shift_in_count);
>> +            shift_in_count = read_from_frame(s, s->N1_bytes,
>> +                            ecc_count, shift_in_count);
> Maybe indent this ^ line up to the ( of the function call operator
> if possible (not sure it's a hard rule  but it reads a bit better
> unless you have really run out of columns)..
This exceeds 80 characters limit, I will shorten the variable name and 
check.
>
>>           }
>>           /* Handle the N2 portion of the frame */
>>           if (s->N2_rx != 0) {
>>               trace_pnv_spi_rx_read_N2frame();
>> -            shift_in_count = read_from_frame(s,
>> -                            &rsp_payload->data[s->N1_bytes], s->N2_bytes,
>> +            shift_in_count = read_from_frame(s, s->N2_bytes,
>>                               ecc_count, shift_in_count);
> Same here, and for other code where you touch nearby lines.
>
>>           }
>>           if ((s->N1_rx + s->N2_rx) > 0) {
>> @@ -210,34 +178,36 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>>       } /* end of else */
>>   } /* end of spi_response() */
>>   
>> -static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>> +static void transfer(PnvSpi *s)
>>   {
>> -    uint32_t tx;
>> -    uint32_t rx;
>> -    PnvXferBuffer *rsp_payload = NULL;
>> +    uint32_t tx, rx, payload_len;
>> +    uint8_t rx_byte;
>>   
>> -    rsp_payload = pnv_spi_xfer_buffer_new();
>> -    for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
>> +    payload_len = (&s->tx_fifo)->num;
> fifo8_num_used()
>
> (And several other cases below should use num_used).
Sure. Will update. Thank You
>
>> +    for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
>>           tx = 0;
>>           for (int i = 0; i < s->transfer_len; i++) {
>> -            if ((offset + i) >= payload->len) {
>> +            if ((offset + i) >= payload_len) {
>>                   tx <<= 8;
>> -            } else {
>> -                tx = (tx << 8) | payload->data[offset + i];
>> +            } else if (!fifo8_is_empty(&s->tx_fifo)) {
>> +                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
>>               }
> Similar question about underflow here. Is there an assert or error
> message or trace event for all these over/under flow cases?
Will add the trace in else case.
>
>>           }
>>           rx = ssi_transfer(s->ssi_bus, tx);
>>           for (int i = 0; i < s->transfer_len; i++) {
>> -            if ((offset + i) >= payload->len) {
>> +            if ((offset + i) >= payload_len) {
>>                   break;
>>               }
>> -            *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
>> -                    (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>> +            rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>> +            if (!fifo8_is_full(&s->rx_fifo)) {
>> +                fifo8_push(&s->rx_fifo, rx_byte);
>> +            }
>>           }
> And overflow, this just gets lost? You can just put || fifo8_is_full()
> in the break condition I think?.
Sure.
>
>>       }
>> -    if (rsp_payload != NULL) {
>> -        spi_response(s, s->N1_bits, rsp_payload);
>> -    }
>> +    spi_response(s);
>> +    /* Reset fifo for next frame */
>> +    fifo8_reset(&s->tx_fifo);
>> +    fifo8_reset(&s->rx_fifo);
>>   }
>>   
>>   static inline uint8_t get_seq_index(PnvSpi *s)
>> @@ -348,19 +318,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>>   /*
>>    * Shift_N1 operation handler method
>>    */
>> -static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> -                       PnvXferBuffer **payload, bool send_n1_alone)
>> +static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
>>   {
>>       uint8_t n1_count;
>>       bool stop = false;
>> -
>> -    /*
>> -     * If there isn't a current payload left over from a stopped sequence
>> -     * create a new one.
>> -     */
>> -    if (*payload == NULL) {
>> -        *payload = pnv_spi_xfer_buffer_new();
>> -    }
>>       /*
>>        * Use a combination of N1 counters to build the N1 portion of the
>>        * transmit payload.
>> @@ -411,9 +372,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>>                    */
>>                   uint8_t n1_byte = 0x00;
>>                   n1_byte = get_from_offset(s, n1_count);
>> -                trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
>> -                *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
>> -                        n1_byte;
>> +                if (!fifo8_is_full(&s->tx_fifo)) {
>> +                    trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
>> +                    fifo8_push(&s->tx_fifo, n1_byte);
>> +                }
>>               } else {
>>                   /*
>>                    * We hit a shift_n1 opcode TX but the TDR is empty, tell the
>> @@ -440,10 +402,9 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>>                                   "set for receive but RDR is full");
>>                   stop = true;
>>                   break;
>> -            } else {
>> +            } else if (!fifo8_is_full(&s->tx_fifo)) {
>>                   trace_pnv_spi_tx_append_FF("n1_byte");
>> -                *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> -                        = 0xff;
>> +                fifo8_push(&s->tx_fifo, 0xff);
>>               }
>>           }
>>           n1_count++;
>> @@ -484,15 +445,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>>        */
>>       if (send_n1_alone && !stop) {
>>           /* We have a TX and a full TDR or an RX and an empty RDR */
>> -        trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
>> -        transfer(s, *payload);
>> +        trace_pnv_spi_tx_request("Shifting N1 frame", (&s->tx_fifo)->num);
>> +        transfer(s);
>>           /* The N1 frame shift is complete so reset the N1 counters */
>>           s->N2_bits = 0;
>>           s->N2_bytes = 0;
>>           s->N2_tx = 0;
>>           s->N2_rx = 0;
>> -        pnv_spi_xfer_buffer_free(*payload);
>> -        *payload = NULL;
>>       }
>>       return stop;
>>   } /* end of operation_shiftn1() */
>> @@ -588,19 +547,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>>    * Shift_N2 operation handler method
>>    */
>>   
>> -static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> -                       PnvXferBuffer **payload)
>> +static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
>>   {
>>       uint8_t n2_count;
>>       bool stop = false;
>> -
>> -    /*
>> -     * If there isn't a current payload left over from a stopped sequence
>> -     * create a new one.
>> -     */
>> -    if (*payload == NULL) {
>> -        *payload = pnv_spi_xfer_buffer_new();
>> -    }
>>       /*
>>        * Use a combination of N2 counters to build the N2 portion of the
>>        * transmit payload.
>> @@ -639,25 +589,26 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>>               /* Always append data for the N2 segment if it is set for TX */
>>               uint8_t n2_byte = 0x00;
>>               n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
>> -            trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
>> -            *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> -                    = n2_byte;
>> -        } else {
>> +            if (!fifo8_is_full(&s->tx_fifo)) {
>> +                trace_pnv_spi_tx_append("n2_byte", n2_byte,
>> +                                (s->N1_tx + n2_count));
>> +                fifo8_push(&s->tx_fifo, n2_byte);
>> +            }
>> +        } else if (!fifo8_is_full(&s->tx_fifo)) {
>>               /*
>>                * Regardless of whether or not N2 is set for TX or RX, we need
>>                * the number of bytes in the payload to match the overall length
>>                * of the operation.
>>                */
>>               trace_pnv_spi_tx_append_FF("n2_byte");
>> -            *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> -                    = 0xff;
>> +            fifo8_push(&s->tx_fifo, 0xff);
>>           }
>>           n2_count++;
>>       } /* end of while */
>>       if (!stop) {
>>           /* We have a TX and a full TDR or an RX and an empty RDR */
>> -        trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
>> -        transfer(s, *payload);
>> +        trace_pnv_spi_tx_request("Shifting N2 frame", (&s->tx_fifo)->num);
>> +        transfer(s);
>>           /*
>>            * If we are doing an N2 TX and the TDR is full we need to clear the
>>            * TDR_full status. Do this here instead of up in the loop above so we
>> @@ -680,8 +631,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>>           s->N1_bytes = 0;
>>           s->N1_tx = 0;
>>           s->N1_rx = 0;
>> -        pnv_spi_xfer_buffer_free(*payload);
>> -        *payload = NULL;
>>       }
>>       return stop;
>>   } /*  end of operation_shiftn2()*/
>> @@ -699,19 +648,6 @@ static void operation_sequencer(PnvSpi *s)
>>       uint8_t opcode = 0;
>>       uint8_t masked_opcode = 0;
>>   
>> -    /*
>> -     * PnvXferBuffer for containing the payload of the SPI frame.
>> -     * This is a static because there are cases where a sequence has to stop
>> -     * and wait for the target application to unload the RDR.  If this occurs
>> -     * during a sequence where N1 is not sent alone and instead combined with
>> -     * N2 since the N1 tx length + the N2 tx length is less than the size of
>> -     * the TDR.
>> -     */
>> -    static PnvXferBuffer *payload;
>> -
>> -    if (payload == NULL) {
>> -        payload = pnv_spi_xfer_buffer_new();
>> -    }
>>       /*
>>        * Clear the sequencer FSM error bit - general_SPI_status[3]
>>        * before starting a sequence.
>> @@ -840,7 +776,7 @@ static void operation_sequencer(PnvSpi *s)
>>                   }
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>>                                   FSM_SHIFT_N1);
>> -                stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
>> +                stop = operation_shiftn1(s, opcode, send_n1_alone);
>>                   if (stop) {
>>                       /*
>>                        *  The operation code says to stop, this can occur if:
>> @@ -895,7 +831,7 @@ static void operation_sequencer(PnvSpi *s)
>>                   /* Ok to do a Shift_N2 */
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>>                                   FSM_SHIFT_N2);
>> -                stop = operation_shiftn2(s, opcode, &payload);
>> +                stop = operation_shiftn2(s, opcode);
>>                   /*
>>                    * If the operation code says to stop set the shifter state to
>>                    * wait and stop
>> @@ -1208,6 +1144,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
>>       s->cs_line = g_new0(qemu_irq, 1);
>>       qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>>   
>> +    fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
>> +    fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
>> +
>>       /* spi scoms */
>>       pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
>>                             s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);
>
> Looks nice, simplifies the code and tricky pointer and memory handling.
>
> Thanks,
> Nick

[PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().

[Chalapathi V]

Use a local variable seq_index instead of repeatedly caling
get_seq_index() method.

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 2fd5aa0a96..962115f40f 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
     fifo8_reset(&s->rx_fifo);
 }
 
-static inline uint8_t get_seq_index(PnvSpi *s)
-{
-    return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
-}
-
 static inline void next_sequencer_fsm(PnvSpi *s)
 {
-    uint8_t seq_index = get_seq_index(s);
-    s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
     s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
 }
 
@@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
     bool stop = false; /* Flag to stop the sequencer */
     uint8_t opcode = 0;
     uint8_t masked_opcode = 0;
+    uint8_t seq_index;
 
     /*
      * Clear the sequencer FSM error bit - general_SPI_status[3]
@@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
     if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
         s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
     }
+    seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
     /*
      * There are only 8 possible operation IDs to iterate through though
      * some operations may cause more than one frame to be sequenced.
      */
-    while (get_seq_index(s) < NUM_SEQ_OPS) {
-        opcode = s->seq_op[get_seq_index(s)];
+    while (seq_index < NUM_SEQ_OPS) {
+        opcode = s->seq_op[seq_index];
         /* Set sequencer state to decode */
         s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
         /*
@@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
         case SEQ_OP_STOP:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
             /* A stop operation in any position stops the sequencer */
-            trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("STOP", seq_index);
 
             stop = true;
             s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
@@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
 
         case SEQ_OP_SELECT_SLAVE:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
-            trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
             /*
              * This device currently only supports a single responder
              * connection at position 0.  De-selecting a responder is fine
@@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
             if (s->responder_select == 0) {
                 trace_pnv_spi_shifter_done();
                 qemu_set_irq(s->cs_line[0], 1);
-                s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
-                                (get_seq_index(s) + 1));
+                seq_index++;
                 s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
             } else if (s->responder_select != 1) {
                 qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
@@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
                  * applies once a valid responder select has occurred.
                  */
                 s->shift_n1_done = false;
+                seq_index++;
                 next_sequencer_fsm(s);
             }
             break;
 
         case SEQ_OP_SHIFT_N1:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
-            trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
             /*
              * Only allow a shift_n1 when the state is not IDLE or DONE.
              * In either of those two cases the sequencer is not in a proper
@@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
                  * transmission to the responder without requiring a refill of
                  * the TDR between the two operations.
                  */
-                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
-                                == SEQ_OP_SHIFT_N2) {
+                if ((seq_index != 7) &&
+                    PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
+                    SEQ_OP_SHIFT_N2) {
                     send_n1_alone = false;
                 }
                 s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
@@ -793,8 +789,7 @@ static void operation_sequencer(PnvSpi *s)
                         s->shift_n1_done = true;
                         s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
                                                   FSM_SHIFT_N2);
-                        s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
-                                        (get_seq_index(s) + 1));
+                        seq_index++;
                     } else {
                         /*
                          * This is case (1) or (2) so the sequencer needs to
@@ -806,6 +801,7 @@ static void operation_sequencer(PnvSpi *s)
                 } else {
                     /* Ok to move on to the next index */
                     s->shift_n1_done = true;
+                    seq_index++;
                     next_sequencer_fsm(s);
                 }
             }
@@ -813,7 +809,7 @@ static void operation_sequencer(PnvSpi *s)
 
         case SEQ_OP_SHIFT_N2:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
-            trace_pnv_spi_sequencer_op("SHIFT_N2", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("SHIFT_N2", seq_index);
             if (!s->shift_n1_done) {
                 qemu_log_mask(LOG_GUEST_ERROR, "Shift_N2 is not allowed if a "
                               "Shift_N1 is not done, shifter state = 0x%llx",
@@ -841,6 +837,7 @@ static void operation_sequencer(PnvSpi *s)
                                     FSM_WAIT);
                 } else {
                     /* Ok to move on to the next index */
+                    seq_index++;
                     next_sequencer_fsm(s);
                 }
             }
@@ -848,7 +845,7 @@ static void operation_sequencer(PnvSpi *s)
 
         case SEQ_OP_BRANCH_IFNEQ_RDR:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
-            trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_RDR", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_RDR", seq_index);
             /*
              * The memory mapping register RDR match value is compared against
              * the 16 rightmost bytes of the RDR (potentially with masking).
@@ -864,6 +861,7 @@ static void operation_sequencer(PnvSpi *s)
                 if (rdr_matched) {
                     trace_pnv_spi_RDR_match("success");
                     /* A match occurred, increment the sequencer index. */
+                    seq_index++;
                     next_sequencer_fsm(s);
                 } else {
                     trace_pnv_spi_RDR_match("failed");
@@ -871,8 +869,7 @@ static void operation_sequencer(PnvSpi *s)
                      * Branch the sequencer to the index coded into the op
                      * code.
                      */
-                    s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
-                                    PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+                    seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
                 }
                 /*
                  * Regardless of where the branch ended up we want the
@@ -891,12 +888,13 @@ static void operation_sequencer(PnvSpi *s)
         case SEQ_OP_TRANSFER_TDR:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
             qemu_log_mask(LOG_GUEST_ERROR, "Transfer TDR is not supported\n");
+            seq_index++;
             next_sequencer_fsm(s);
             break;
 
         case SEQ_OP_BRANCH_IFNEQ_INC_1:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
-            trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_1", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_1", seq_index);
             /*
              * The spec says the loop should execute count compare + 1 times.
              * However we learned from engineering that we really only loop
@@ -910,18 +908,18 @@ static void operation_sequencer(PnvSpi *s)
                  * mask off all but the first three bits so we don't try to
                  * access beyond the sequencer_operation_reg boundary.
                  */
-                s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
-                                PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+                seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
                 s->loop_counter_1++;
             } else {
                 /* Continue to next index if loop counter is reached */
+                seq_index++;
                 next_sequencer_fsm(s);
             }
             break;
 
         case SEQ_OP_BRANCH_IFNEQ_INC_2:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
-            trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", get_seq_index(s));
+            trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", seq_index);
             uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
                               s->regs[SPI_CTR_CFG_REG]);
             /*
@@ -936,11 +934,11 @@ static void operation_sequencer(PnvSpi *s)
                  * mask off all but the first three bits so we don't try to
                  * access beyond the sequencer_operation_reg boundary.
                  */
-                s->status = SETFIELD(SPI_STS_SEQ_INDEX,
-                                s->status, PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+                seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
                 s->loop_counter_2++;
             } else {
                 /* Continue to next index if loop counter is reached */
+                seq_index++;
                 next_sequencer_fsm(s);
             }
             break;
@@ -948,6 +946,7 @@ static void operation_sequencer(PnvSpi *s)
         default:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
             /* Ignore unsupported operations. */
+            seq_index++;
             next_sequencer_fsm(s);
             break;
         } /* end of switch */
@@ -956,10 +955,10 @@ static void operation_sequencer(PnvSpi *s)
          * we need to go ahead and end things as if there was a STOP at the
          * end.
          */
-        if (get_seq_index(s) == NUM_SEQ_OPS) {
+        if (seq_index == NUM_SEQ_OPS) {
             /* All 8 opcodes completed, sequencer idling */
             s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
-            s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
+            seq_index = 0;
             s->loop_counter_1 = 0;
             s->loop_counter_2 = 0;
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_IDLE);
@@ -970,6 +969,8 @@ static void operation_sequencer(PnvSpi *s)
             break;
         }
     } /* end of while */
+    /* Update sequencer index field in status.*/
+    s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, seq_index);
     return;
 } /* end of operation_sequencer() */
 
-- 
2.39.5

Re: [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().

[Nicholas Piggin]

On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> Use a local variable seq_index instead of repeatedly caling
> get_seq_index() method.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 2fd5aa0a96..962115f40f 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
>      fifo8_reset(&s->rx_fifo);
>  }
>  
> -static inline uint8_t get_seq_index(PnvSpi *s)
> -{
> -    return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
> -}
> -
>  static inline void next_sequencer_fsm(PnvSpi *s)
>  {
> -    uint8_t seq_index = get_seq_index(s);
> -    s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
>      s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
>  }
>  
> @@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
>      bool stop = false; /* Flag to stop the sequencer */
>      uint8_t opcode = 0;
>      uint8_t masked_opcode = 0;
> +    uint8_t seq_index;
>  
>      /*
>       * Clear the sequencer FSM error bit - general_SPI_status[3]
> @@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
>      if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
>          s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
>      }
> +    seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>      /*
>       * There are only 8 possible operation IDs to iterate through though
>       * some operations may cause more than one frame to be sequenced.
>       */
> -    while (get_seq_index(s) < NUM_SEQ_OPS) {
> -        opcode = s->seq_op[get_seq_index(s)];
> +    while (seq_index < NUM_SEQ_OPS) {
> +        opcode = s->seq_op[seq_index];
>          /* Set sequencer state to decode */
>          s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
>          /*
> @@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
>          case SEQ_OP_STOP:
>              s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>              /* A stop operation in any position stops the sequencer */
> -            trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
> +            trace_pnv_spi_sequencer_op("STOP", seq_index);
>  
>              stop = true;
>              s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
> @@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
>  
>          case SEQ_OP_SELECT_SLAVE:
>              s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
> -            trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
> +            trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
>              /*
>               * This device currently only supports a single responder
>               * connection at position 0.  De-selecting a responder is fine
> @@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
>              if (s->responder_select == 0) {
>                  trace_pnv_spi_shifter_done();
>                  qemu_set_irq(s->cs_line[0], 1);
> -                s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
> -                                (get_seq_index(s) + 1));
> +                seq_index++;
>                  s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
>              } else if (s->responder_select != 1) {
>                  qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
> @@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
>                   * applies once a valid responder select has occurred.
>                   */
>                  s->shift_n1_done = false;
> +                seq_index++;
>                  next_sequencer_fsm(s);

Maybe could just open-code next_sequencer_fsm() now, since a bunch of
other FSM fields seem to be open-coded?

>              }
>              break;
>  
>          case SEQ_OP_SHIFT_N1:
>              s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
> -            trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
> +            trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
>              /*
>               * Only allow a shift_n1 when the state is not IDLE or DONE.
>               * In either of those two cases the sequencer is not in a proper
> @@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
>                   * transmission to the responder without requiring a refill of
>                   * the TDR between the two operations.
>                   */
> -                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
> -                                == SEQ_OP_SHIFT_N2) {
> +                if ((seq_index != 7) &&
> +                    PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
> +                    SEQ_OP_SHIFT_N2) {
>                      send_n1_alone = false;
>                  }
>                  s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,

The seq_index != 7 is a new test? Is that a separate fix, I'm not
seeing how it's related to the seq_index change.

Thanks,
Nick

Re: [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().

[Chalapathi V]

On 08-10-2024 13:43, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> Use a local variable seq_index instead of repeatedly caling
>> get_seq_index() method.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
>>   1 file changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 2fd5aa0a96..962115f40f 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
>>       fifo8_reset(&s->rx_fifo);
>>   }
>>   
>> -static inline uint8_t get_seq_index(PnvSpi *s)
>> -{
>> -    return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>> -}
>> -
>>   static inline void next_sequencer_fsm(PnvSpi *s)
>>   {
>> -    uint8_t seq_index = get_seq_index(s);
>> -    s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
>>       s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
>>   }
>>   
>> @@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
>>       bool stop = false; /* Flag to stop the sequencer */
>>       uint8_t opcode = 0;
>>       uint8_t masked_opcode = 0;
>> +    uint8_t seq_index;
>>   
>>       /*
>>        * Clear the sequencer FSM error bit - general_SPI_status[3]
>> @@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
>>       if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
>>           s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
>>       }
>> +    seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>>       /*
>>        * There are only 8 possible operation IDs to iterate through though
>>        * some operations may cause more than one frame to be sequenced.
>>        */
>> -    while (get_seq_index(s) < NUM_SEQ_OPS) {
>> -        opcode = s->seq_op[get_seq_index(s)];
>> +    while (seq_index < NUM_SEQ_OPS) {
>> +        opcode = s->seq_op[seq_index];
>>           /* Set sequencer state to decode */
>>           s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
>>           /*
>> @@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
>>           case SEQ_OP_STOP:
>>               s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>>               /* A stop operation in any position stops the sequencer */
>> -            trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
>> +            trace_pnv_spi_sequencer_op("STOP", seq_index);
>>   
>>               stop = true;
>>               s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
>> @@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
>>   
>>           case SEQ_OP_SELECT_SLAVE:
>>               s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> -            trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
>> +            trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
>>               /*
>>                * This device currently only supports a single responder
>>                * connection at position 0.  De-selecting a responder is fine
>> @@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
>>               if (s->responder_select == 0) {
>>                   trace_pnv_spi_shifter_done();
>>                   qemu_set_irq(s->cs_line[0], 1);
>> -                s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
>> -                                (get_seq_index(s) + 1));
>> +                seq_index++;
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
>>               } else if (s->responder_select != 1) {
>>                   qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
>> @@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
>>                    * applies once a valid responder select has occurred.
>>                    */
>>                   s->shift_n1_done = false;
>> +                seq_index++;
>>                   next_sequencer_fsm(s);
> Maybe could just open-code next_sequencer_fsm() now, since a bunch of
> other FSM fields seem to be open-coded?
Sure.
>
>>               }
>>               break;
>>   
>>           case SEQ_OP_SHIFT_N1:
>>               s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> -            trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
>> +            trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
>>               /*
>>                * Only allow a shift_n1 when the state is not IDLE or DONE.
>>                * In either of those two cases the sequencer is not in a proper
>> @@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
>>                    * transmission to the responder without requiring a refill of
>>                    * the TDR between the two operations.
>>                    */
>> -                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
>> -                                == SEQ_OP_SHIFT_N2) {
>> +                if ((seq_index != 7) &&
>> +                    PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
>> +                    SEQ_OP_SHIFT_N2) {
>>                       send_n1_alone = false;
>>                   }
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> The seq_index != 7 is a new test? Is that a separate fix, I'm not
> seeing how it's related to the seq_index change.
Not a new test but to make sure array index of seq_op doesn't overflow 
due to seq_index + 1.
>
> Thanks,
> Nick

Re: [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()

[Nicholas Piggin]

On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> Hello,
>
> v3:
> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
> 2. Use of PnvXferBuffer results in a additonal process overhead due to
> frequent dynamic allocations and hence use an existing Fifo8 buffer.
> 3. Use a local variable seq_index and use it with in while loop instead
> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
> overrun when seq_index is incremented.
>
> Tested:
> passed make check and make check-avocado
>
> Supersedes: <20240807202804.56038-1-philmd@linaro.org>

Hi Chalapathi,

To be clear, this fixes Coverity CID 1558831? A Resolves:
tag for the CID should be there, I guess it's patch 2?

I like patch 2, but since it is quite a significant change,
should we take the v2 series first which is much smaller,
then add this conversion on top of it?

If it was long-standing code that would be important (because
you don't want to introduce regressions or conflicts when
backporting fixes). Since this is a new model I guess there
is leeway to just take v3 as is.

Thanks,
Nick

>
> Philippe Mathieu-Daudé (1):
>   MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>
> Chalapathi V (2):
>   hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
>   hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
>
>  MAINTAINERS              |   2 +
>  include/hw/ssi/pnv_spi.h |   3 +
>  hw/ssi/pnv_spi.c         | 228 +++++++++++++++------------------------
>  3 files changed, 89 insertions(+), 144 deletions(-)

Re: [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()

[Cédric Le Goater]

On 10/8/24 09:43, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> Hello,
>>
>> v3:
>> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
>> 2. Use of PnvXferBuffer results in a additonal process overhead due to
>> frequent dynamic allocations and hence use an existing Fifo8 buffer.
>> 3. Use a local variable seq_index and use it with in while loop instead
>> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
>> overrun when seq_index is incremented.
>>
>> Tested:
>> passed make check and make check-avocado
>>
>> Supersedes: <20240807202804.56038-1-philmd@linaro.org>
> 
> Hi Chalapathi,
> 
> To be clear, this fixes Coverity CID 1558831? A Resolves:
> tag for the CID should be there, I guess it's patch 2?


Patches should have a tag :

Resolves: Coverity CID XYZZY

Thanks,

C.


> 
> I like patch 2, but since it is quite a significant change,
> should we take the v2 series first which is much smaller,
> then add this conversion on top of it?
> 
> If it was long-standing code that would be important (because
> you don't want to introduce regressions or conflicts when
> backporting fixes). Since this is a new model I guess there
> is leeway to just take v3 as is.
> 
> Thanks,
> Nick
> 
>>
>> Philippe Mathieu-Daudé (1):
>>    MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>>
>> Chalapathi V (2):
>>    hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
>>    hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
>>
>>   MAINTAINERS              |   2 +
>>   include/hw/ssi/pnv_spi.h |   3 +
>>   hw/ssi/pnv_spi.c         | 228 +++++++++++++++------------------------
>>   3 files changed, 89 insertions(+), 144 deletions(-)
> 

Re: [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()

[Chalapathi V]

On 08-10-2024 18:14, Cédric Le Goater wrote:
> On 10/8/24 09:43, Nicholas Piggin wrote:
>> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>>> Hello,
>>>
>>> v3:
>>> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
>>> 2. Use of PnvXferBuffer results in a additonal process overhead due to
>>> frequent dynamic allocations and hence use an existing Fifo8 buffer.
>>> 3. Use a local variable seq_index and use it with in while loop instead
>>> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
>>> overrun when seq_index is incremented.
>>>
>>> Tested:
>>> passed make check and make check-avocado
>>>
>>> Supersedes: <20240807202804.56038-1-philmd@linaro.org>
>>
>> Hi Chalapathi,
>>
>> To be clear, this fixes Coverity CID 1558831? A Resolves:
>> tag for the CID should be there, I guess it's patch 2?
>
>
> Patches should have a tag :
>
> Resolves: Coverity CID XYZZY
>
> Thanks,
>
> C.
Sure. Will update. Thank You
>
>
>>
>> I like patch 2, but since it is quite a significant change,
>> should we take the v2 series first which is much smaller,
>> then add this conversion on top of it?
>>
>> If it was long-standing code that would be important (because
>> you don't want to introduce regressions or conflicts when
>> backporting fixes). Since this is a new model I guess there
>> is leeway to just take v3 as is.
>>
>> Thanks,
>> Nick
>>
>>>
>>> Philippe Mathieu-Daudé (1):
>>>    MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>>>
>>> Chalapathi V (2):
>>>    hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
>>>    hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
>>>
>>>   MAINTAINERS              |   2 +
>>>   include/hw/ssi/pnv_spi.h |   3 +
>>>   hw/ssi/pnv_spi.c         | 228 
>>> +++++++++++++++------------------------
>>>   3 files changed, 89 insertions(+), 144 deletions(-)
>>
>