[PATCH v5 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and fix CID 1558827

[PATCH v5 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and fix CID 1558827

[Chalapathi V]

Hello,

In this revision two new commits are added to address multi chip SPI issues.

Updates in V5:
1. Use of PnvXferBuffer results in a additonal process overhead due to
frequent dynamic allocations and hence use an existing Fifo8 buffer.
2. 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.
3. Unique bus names are created for each controller in a socket so that
responders are attched to correct SPI controllers via CLI.
4. Enforce a limit on number RDR match failures so that SPI controller
doesn't get caught in an infinite execution loop querying the responder
for RDR match.

Tested:
passed make check and make check-avocado

Chalapathi V (4):
  hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
  hw/ssi/pnv_spi: Coverity CID 1558827: Use local var seq_index instead
    of get_seq_index().
  hw/ssi/pnv_spi: Make bus names distinct for each controllers of a
    socket
  hw/ssi/pnv_spi: Put a limit to RDR match failures

 include/hw/ssi/pnv_spi.h           |   6 +-
 hw/ppc/pnv.c                       |   2 +
 hw/ssi/pnv_spi.c                   | 336 ++++++++++++-----------------
 tests/qtest/pnv-spi-seeprom-test.c |   2 +-
 4 files changed, 143 insertions(+), 203 deletions(-)

-- 
2.39.5

[PATCH v5 1/4] 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         | 237 +++++++++++++--------------------------
 2 files changed, 81 insertions(+), 159 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 15e25bd1be..63d298980d 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,48 +36,14 @@
         }                                                          \
     } 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)
-{
-    g_free(payload->data);
-    g_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)
 {
     /*
      * According to spec, the mask bits that are 0 are compared and the
      * bits that are 1 are ignored.
      */
-    uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK,
-                                        s->regs[SPI_MM_REG]);
-    uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL,
-                                        s->regs[SPI_MM_REG]);
+    uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK, s->regs[SPI_MM_REG]);
+    uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL, s->regs[SPI_MM_REG]);
 
     if ((~rdr_match_mask & rdr_match_val) == ((~rdr_match_mask) &
             GETFIELD(PPC_BITMASK(48, 63), s->regs[SPI_RCV_DATA_REG]))) {
@@ -107,8 +74,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,20 +85,23 @@ 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;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: Reading empty RX_FIFO\n");
         }
         count++;
     } /* end of while */
     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;
+    uint32_t rx_len;
 
     /*
      * Processing here must handle:
@@ -144,13 +114,14 @@ 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)) {
+    rx_len = fifo8_num_used(&s->rx_fifo);
+    if (rx_len != (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), rx_len);
     } else {
         uint8_t ecc_control;
-        trace_pnv_spi_rx_received(rsp_payload->len);
+        trace_pnv_spi_rx_received(rx_len);
         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,15 +146,12 @@ 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,
-                            ecc_count, shift_in_count);
+            shift_in_count = read_from_frame(s, s->N2_bytes, ecc_count, shift_in_count);
         }
         if ((s->N1_rx + s->N2_rx) > 0) {
             /*
@@ -210,36 +178,38 @@ 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();
-    if (!rsp_payload) {
-        return;
-    }
-    for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
+    payload_len = fifo8_num_used(&s->tx_fifo);
+    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 if (!fifo8_is_empty(&s->tx_fifo)) {
+                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
             } else {
-                tx = (tx << 8) | payload->data[offset + i];
+                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
             }
         }
         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) || fifo8_is_full(&s->rx_fifo)) {
                 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);
+            }
         }
     }
-    spi_response(s, s->N1_bits, rsp_payload);
-    pnv_spi_xfer_buffer_free(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)
@@ -310,13 +280,11 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
              * If Forced Implicit mode and count control doesn't
              * indicate transmit then reset the tx count to 0
              */
-            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2,
-                                    s->regs[SPI_CTR_CFG_REG]) == 0) {
+            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 0) {
                 s->N1_tx = 0;
             }
             /* If rx count control for N1 is set, load the rx value */
-            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3,
-                                    s->regs[SPI_CTR_CFG_REG]) == 1) {
+            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 1) {
                 s->N1_rx = s->N1_bytes;
             }
         }
@@ -328,8 +296,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
      * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
      * error bit.
      */
-    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
-                                   s->regs[SPI_CLK_CFG_REG]);
+    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
     if (ecc_control == 0 || ecc_control == 2) {
         if (s->N1_bytes > (PNV_SPI_REG_SIZE + 1)) {
             qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size when "
@@ -340,8 +307,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
         }
     } else if (s->N1_bytes > PNV_SPI_REG_SIZE) {
         qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size, "
-                      "bytes = 0x%x, bits = 0x%x\n",
-                      s->N1_bytes, s->N1_bits);
+                      "bytes = 0x%x, bits = 0x%x\n", s->N1_bytes, s->N1_bits);
         s->N1_bytes = PNV_SPI_REG_SIZE;
         s->N1_bits = s->N1_bytes * 8;
     }
@@ -350,19 +316,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.
@@ -413,9 +370,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
@@ -436,16 +394,14 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
              * - we are receiving and the RDR is empty so we allow the operation
              *   to proceed.
              */
-            if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL,
-                                           s->status) == 1)) {
+            if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
                 trace_pnv_spi_sequencer_stop_requested("shift N1"
                                 "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++;
@@ -486,15 +442,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", fifo8_num_used(&s->tx_fifo));
+        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() */
@@ -552,13 +506,11 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
              * If Forced Implicit mode and count control doesn't
              * indicate a receive then reset the rx count to 0
              */
-            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3,
-                                    s->regs[SPI_CTR_CFG_REG]) == 0) {
+            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 0) {
                 s->N2_rx = 0;
             }
             /* If tx count control for N2 is set, load the tx value */
-            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2,
-                                    s->regs[SPI_CTR_CFG_REG]) == 1) {
+            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 1) {
                 s->N2_tx = s->N2_bytes;
             }
         }
@@ -571,8 +523,7 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
      * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
      * error bit.
      */
-    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
-                    s->regs[SPI_CLK_CFG_REG]);
+    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
     if (ecc_control == 0 || ecc_control == 2) {
         if (s->N2_bytes > (PNV_SPI_REG_SIZE + 1)) {
             /* Unsupported N2 shift size when ECC enabled */
@@ -590,19 +541,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.
@@ -629,44 +571,41 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
          * code continue will end up building the payload twice in the same
          * buffer since RDR full causes a sequence stop and restart.
          */
-        if ((s->N2_rx != 0) &&
-            (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
+        if ((s->N2_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
             trace_pnv_spi_sequencer_stop_requested("shift N2 set"
                             "for receive but RDR is full");
             stop = true;
             break;
         }
-        if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) <
-                                PNV_SPI_REG_SIZE)) {
+        if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) < PNV_SPI_REG_SIZE)) {
             /* 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", fifo8_num_used(&s->tx_fifo));
+        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
          * don't log the message in every loop iteration.
          */
-        if ((s->N2_tx != 0) &&
-            (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
+        if ((s->N2_tx != 0) && (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
             s->status = SETFIELD(SPI_STS_TDR_FULL, s->status, 0);
         }
         /*
@@ -682,8 +621,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()*/
@@ -701,19 +638,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.
@@ -775,10 +699,8 @@ static void operation_sequencer(PnvSpi *s)
                 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 "
-                              "not supported, select = 0x%x\n",
-                               s->responder_select);
-                trace_pnv_spi_sequencer_stop_requested("invalid "
-                                "responder select");
+                              "not supported, select = 0x%x\n", s->responder_select);
+                trace_pnv_spi_sequencer_stop_requested("invalid responder select");
                 s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
                 stop = true;
             } else {
@@ -840,9 +762,8 @@ static void operation_sequencer(PnvSpi *s)
                                 == SEQ_OP_SHIFT_N2) {
                     send_n1_alone = false;
                 }
-                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
-                                FSM_SHIFT_N1);
-                stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
+                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N1);
+                stop = operation_shiftn1(s, opcode, send_n1_alone);
                 if (stop) {
                     /*
                      *  The operation code says to stop, this can occur if:
@@ -858,7 +779,7 @@ static void operation_sequencer(PnvSpi *s)
                     if (GETFIELD(SPI_STS_TDR_UNDERRUN, s->status)) {
                         s->shift_n1_done = true;
                         s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
-                                                  FSM_SHIFT_N2);
+                                        FSM_SHIFT_N2);
                         s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
                                         (get_seq_index(s) + 1));
                     } else {
@@ -866,8 +787,7 @@ static void operation_sequencer(PnvSpi *s)
                          * This is case (1) or (2) so the sequencer needs to
                          * wait and NOT go to the next sequence yet.
                          */
-                        s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
-                                        FSM_WAIT);
+                        s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
                     }
                 } else {
                     /* Ok to move on to the next index */
@@ -890,21 +810,18 @@ static void operation_sequencer(PnvSpi *s)
                  * error bit 3 (general_SPI_status[3]) in status reg.
                  */
                 s->status = SETFIELD(SPI_STS_GEN_STATUS_B3, s->status, 1);
-                trace_pnv_spi_sequencer_stop_requested("shift_n2 "
-                                    "w/no shift_n1 done");
+                trace_pnv_spi_sequencer_stop_requested("shift_n2 w/no shift_n1 done");
                 stop = true;
             } else {
                 /* Ok to do a Shift_N2 */
-                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
-                                FSM_SHIFT_N2);
-                stop = operation_shiftn2(s, opcode, &payload);
+                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N2);
+                stop = operation_shiftn2(s, opcode);
                 /*
                  * If the operation code says to stop set the shifter state to
                  * wait and stop
                  */
                 if (stop) {
-                    s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
-                                    FSM_WAIT);
+                    s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
                 } else {
                     /* Ok to move on to the next index */
                     next_sequencer_fsm(s);
@@ -988,8 +905,7 @@ static void operation_sequencer(PnvSpi *s)
         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));
-            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
-                              s->regs[SPI_CTR_CFG_REG]);
+            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2, s->regs[SPI_CTR_CFG_REG]);
             /*
              * The spec says the loop should execute count compare + 1 times.
              * However we learned from engineering that we really only loop
@@ -1209,6 +1125,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 v5 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure

[Nicholas Piggin]

On Sat Jan 4, 2025 at 2:18 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         | 237 +++++++++++++--------------------------
>  2 files changed, 81 insertions(+), 159 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 15e25bd1be..63d298980d 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,48 +36,14 @@
>          }                                                          \
>      } 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)
> -{
> -    g_free(payload->data);
> -    g_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)
>  {
>      /*
>       * According to spec, the mask bits that are 0 are compared and the
>       * bits that are 1 are ignored.
>       */
> -    uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK,
> -                                        s->regs[SPI_MM_REG]);
> -    uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL,
> -                                        s->regs[SPI_MM_REG]);
> +    uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK, s->regs[SPI_MM_REG]);
> +    uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL, s->regs[SPI_MM_REG]);
>  
>      if ((~rdr_match_mask & rdr_match_val) == ((~rdr_match_mask) &
>              GETFIELD(PPC_BITMASK(48, 63), s->regs[SPI_RCV_DATA_REG]))) {

Usually try to avoid unrelated / cleanup in the same patch that acually
changes things. In this case it's quite minor but it helps with review
and rebasing to avoid.

If it's on the same line or very close line to your change, or
occasional ones I don't mind so much, but you have quite a few
more further down the patch.

> @@ -107,8 +74,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,20 +85,23 @@ 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;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: Reading empty RX_FIFO\n");
>          }
>          count++;
>      } /* end of while */
>      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;
> +    uint32_t rx_len;
>  
>      /*
>       * Processing here must handle:
> @@ -144,13 +114,14 @@ 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)) {
> +    rx_len = fifo8_num_used(&s->rx_fifo);
> +    if (rx_len != (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), rx_len);
>      } else {
>          uint8_t ecc_control;
> -        trace_pnv_spi_rx_received(rsp_payload->len);
> +        trace_pnv_spi_rx_received(rx_len);
>          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,15 +146,12 @@ 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,
> -                            ecc_count, shift_in_count);
> +            shift_in_count = read_from_frame(s, s->N2_bytes, ecc_count, shift_in_count);
>          }
>          if ((s->N1_rx + s->N2_rx) > 0) {
>              /*
> @@ -210,36 +178,38 @@ 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();
> -    if (!rsp_payload) {
> -        return;
> -    }
> -    for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
> +    payload_len = fifo8_num_used(&s->tx_fifo);
> +    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 if (!fifo8_is_empty(&s->tx_fifo)) {
> +                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
>              } else {
> -                tx = (tx << 8) | payload->data[offset + i];
> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
>              }
>          }
>          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) || fifo8_is_full(&s->rx_fifo)) {
>                  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);
> +            }
>          }
>      }
> -    spi_response(s, s->N1_bits, rsp_payload);
> -    pnv_spi_xfer_buffer_free(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)
> @@ -310,13 +280,11 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>               * If Forced Implicit mode and count control doesn't
>               * indicate transmit then reset the tx count to 0
>               */
> -            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2,
> -                                    s->regs[SPI_CTR_CFG_REG]) == 0) {
> +            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 0) {
>                  s->N1_tx = 0;
>              }
>              /* If rx count control for N1 is set, load the rx value */
> -            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3,
> -                                    s->regs[SPI_CTR_CFG_REG]) == 1) {
> +            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 1) {
>                  s->N1_rx = s->N1_bytes;
>              }
>          }
> @@ -328,8 +296,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>       * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
>       * error bit.
>       */
> -    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
> -                                   s->regs[SPI_CLK_CFG_REG]);
> +    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
>      if (ecc_control == 0 || ecc_control == 2) {
>          if (s->N1_bytes > (PNV_SPI_REG_SIZE + 1)) {
>              qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size when "
> @@ -340,8 +307,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>          }
>      } else if (s->N1_bytes > PNV_SPI_REG_SIZE) {
>          qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size, "
> -                      "bytes = 0x%x, bits = 0x%x\n",
> -                      s->N1_bytes, s->N1_bits);
> +                      "bytes = 0x%x, bits = 0x%x\n", s->N1_bytes, s->N1_bits);
>          s->N1_bytes = PNV_SPI_REG_SIZE;
>          s->N1_bits = s->N1_bytes * 8;
>      }
> @@ -350,19 +316,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.
> @@ -413,9 +370,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
> @@ -436,16 +394,14 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>               * - we are receiving and the RDR is empty so we allow the operation
>               *   to proceed.
>               */
> -            if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL,
> -                                           s->status) == 1)) {
> +            if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>                  trace_pnv_spi_sequencer_stop_requested("shift N1"
>                                  "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);
>              }

Is there any problem or overrun that needs to be logged if the
fifo is full in the above two places?

>          }
>          n1_count++;
> @@ -486,15 +442,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", fifo8_num_used(&s->tx_fifo));
> +        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() */
> @@ -552,13 +506,11 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>               * If Forced Implicit mode and count control doesn't
>               * indicate a receive then reset the rx count to 0
>               */
> -            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3,
> -                                    s->regs[SPI_CTR_CFG_REG]) == 0) {
> +            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 0) {
>                  s->N2_rx = 0;
>              }
>              /* If tx count control for N2 is set, load the tx value */
> -            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2,
> -                                    s->regs[SPI_CTR_CFG_REG]) == 1) {
> +            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 1) {
>                  s->N2_tx = s->N2_bytes;
>              }
>          }
> @@ -571,8 +523,7 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>       * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
>       * error bit.
>       */
> -    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
> -                    s->regs[SPI_CLK_CFG_REG]);
> +    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
>      if (ecc_control == 0 || ecc_control == 2) {
>          if (s->N2_bytes > (PNV_SPI_REG_SIZE + 1)) {
>              /* Unsupported N2 shift size when ECC enabled */
> @@ -590,19 +541,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.
> @@ -629,44 +571,41 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>           * code continue will end up building the payload twice in the same
>           * buffer since RDR full causes a sequence stop and restart.
>           */
> -        if ((s->N2_rx != 0) &&
> -            (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
> +        if ((s->N2_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>              trace_pnv_spi_sequencer_stop_requested("shift N2 set"
>                              "for receive but RDR is full");
>              stop = true;
>              break;
>          }
> -        if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) <
> -                                PNV_SPI_REG_SIZE)) {
> +        if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) < PNV_SPI_REG_SIZE)) {
>              /* 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", fifo8_num_used(&s->tx_fifo));
> +        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
>           * don't log the message in every loop iteration.
>           */
> -        if ((s->N2_tx != 0) &&
> -            (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
> +        if ((s->N2_tx != 0) && (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
>              s->status = SETFIELD(SPI_STS_TDR_FULL, s->status, 0);
>          }
>          /*
> @@ -682,8 +621,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()*/
> @@ -701,19 +638,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.
> @@ -775,10 +699,8 @@ static void operation_sequencer(PnvSpi *s)
>                  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 "
> -                              "not supported, select = 0x%x\n",
> -                               s->responder_select);
> -                trace_pnv_spi_sequencer_stop_requested("invalid "
> -                                "responder select");
> +                              "not supported, select = 0x%x\n", s->responder_select);
> +                trace_pnv_spi_sequencer_stop_requested("invalid responder select");
>                  s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
>                  stop = true;
>              } else {
> @@ -840,9 +762,8 @@ static void operation_sequencer(PnvSpi *s)
>                                  == SEQ_OP_SHIFT_N2) {
>                      send_n1_alone = false;
>                  }
> -                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> -                                FSM_SHIFT_N1);
> -                stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
> +                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N1);
> +                stop = operation_shiftn1(s, opcode, send_n1_alone);
>                  if (stop) {
>                      /*
>                       *  The operation code says to stop, this can occur if:
> @@ -858,7 +779,7 @@ static void operation_sequencer(PnvSpi *s)
>                      if (GETFIELD(SPI_STS_TDR_UNDERRUN, s->status)) {
>                          s->shift_n1_done = true;
>                          s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> -                                                  FSM_SHIFT_N2);
> +                                        FSM_SHIFT_N2);
>                          s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
>                                          (get_seq_index(s) + 1));
>                      } else {
> @@ -866,8 +787,7 @@ static void operation_sequencer(PnvSpi *s)
>                           * This is case (1) or (2) so the sequencer needs to
>                           * wait and NOT go to the next sequence yet.
>                           */
> -                        s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> -                                        FSM_WAIT);
> +                        s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
>                      }
>                  } else {
>                      /* Ok to move on to the next index */
> @@ -890,21 +810,18 @@ static void operation_sequencer(PnvSpi *s)
>                   * error bit 3 (general_SPI_status[3]) in status reg.
>                   */
>                  s->status = SETFIELD(SPI_STS_GEN_STATUS_B3, s->status, 1);
> -                trace_pnv_spi_sequencer_stop_requested("shift_n2 "
> -                                    "w/no shift_n1 done");
> +                trace_pnv_spi_sequencer_stop_requested("shift_n2 w/no shift_n1 done");
>                  stop = true;
>              } else {
>                  /* Ok to do a Shift_N2 */
> -                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> -                                FSM_SHIFT_N2);
> -                stop = operation_shiftn2(s, opcode, &payload);
> +                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N2);
> +                stop = operation_shiftn2(s, opcode);
>                  /*
>                   * If the operation code says to stop set the shifter state to
>                   * wait and stop
>                   */
>                  if (stop) {
> -                    s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> -                                    FSM_WAIT);
> +                    s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
>                  } else {
>                      /* Ok to move on to the next index */
>                      next_sequencer_fsm(s);
> @@ -988,8 +905,7 @@ static void operation_sequencer(PnvSpi *s)
>          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));
> -            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
> -                              s->regs[SPI_CTR_CFG_REG]);
> +            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2, s->regs[SPI_CTR_CFG_REG]);
>              /*
>               * The spec says the loop should execute count compare + 1 times.
>               * However we learned from engineering that we really only loop
> @@ -1209,6 +1125,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);

Aside from those small nits, it looks like a good cleanup and
improvement with more error logging.

Thanks,
Nick

Re: [PATCH v5 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure

[Chalapathi V]

[-- Attachment #1: Type: text/plain, Size: 26753 bytes --]

Hello Nick,

Thank You for reviewing this series.

On 27-02-2025 07:09, Nicholas Piggin wrote:
> On Sat Jan 4, 2025 at 2:18 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         | 237 +++++++++++++--------------------------
>>   2 files changed, 81 insertions(+), 159 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 15e25bd1be..63d298980d 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,48 +36,14 @@
>>           }                                                          \
>>       } 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)
>> -{
>> -    g_free(payload->data);
>> -    g_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)
>>   {
>>       /*
>>        * According to spec, the mask bits that are 0 are compared and the
>>        * bits that are 1 are ignored.
>>        */
>> -    uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK,
>> -                                        s->regs[SPI_MM_REG]);
>> -    uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL,
>> -                                        s->regs[SPI_MM_REG]);
>> +    uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK, s->regs[SPI_MM_REG]);
>> +    uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL, s->regs[SPI_MM_REG]);
>>   
>>       if ((~rdr_match_mask & rdr_match_val) == ((~rdr_match_mask) &
>>               GETFIELD(PPC_BITMASK(48, 63), s->regs[SPI_RCV_DATA_REG]))) {
> Usually try to avoid unrelated / cleanup in the same patch that acually
> changes things. In this case it's quite minor but it helps with review
> and rebasing to avoid.
>
> If it's on the same line or very close line to your change, or
> occasional ones I don't mind so much, but you have quite a few
> more further down the patch.
Sorry about that!. Should I create a new patch in v6 or keep it as is 
for now?

Going forward will create new patches for distinct changes.

Thank You.

>> @@ -107,8 +74,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,20 +85,23 @@ 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;
>> +        } else {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: Reading empty RX_FIFO\n");
>>           }
>>           count++;
>>       } /* end of while */
>>       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;
>> +    uint32_t rx_len;
>>   
>>       /*
>>        * Processing here must handle:
>> @@ -144,13 +114,14 @@ 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)) {
>> +    rx_len = fifo8_num_used(&s->rx_fifo);
>> +    if (rx_len != (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), rx_len);
>>       } else {
>>           uint8_t ecc_control;
>> -        trace_pnv_spi_rx_received(rsp_payload->len);
>> +        trace_pnv_spi_rx_received(rx_len);
>>           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,15 +146,12 @@ 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,
>> -                            ecc_count, shift_in_count);
>> +            shift_in_count = read_from_frame(s, s->N2_bytes, ecc_count, shift_in_count);
>>           }
>>           if ((s->N1_rx + s->N2_rx) > 0) {
>>               /*
>> @@ -210,36 +178,38 @@ 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();
>> -    if (!rsp_payload) {
>> -        return;
>> -    }
>> -    for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
>> +    payload_len = fifo8_num_used(&s->tx_fifo);
>> +    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 if (!fifo8_is_empty(&s->tx_fifo)) {
>> +                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
>>               } else {
>> -                tx = (tx << 8) | payload->data[offset + i];
>> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
>>               }
>>           }
>>           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) || fifo8_is_full(&s->rx_fifo)) {
>>                   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);
>> +            }
>>           }
>>       }
>> -    spi_response(s, s->N1_bits, rsp_payload);
>> -    pnv_spi_xfer_buffer_free(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)
>> @@ -310,13 +280,11 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>>                * If Forced Implicit mode and count control doesn't
>>                * indicate transmit then reset the tx count to 0
>>                */
>> -            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2,
>> -                                    s->regs[SPI_CTR_CFG_REG]) == 0) {
>> +            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 0) {
>>                   s->N1_tx = 0;
>>               }
>>               /* If rx count control for N1 is set, load the rx value */
>> -            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3,
>> -                                    s->regs[SPI_CTR_CFG_REG]) == 1) {
>> +            if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 1) {
>>                   s->N1_rx = s->N1_bytes;
>>               }
>>           }
>> @@ -328,8 +296,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>>        * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
>>        * error bit.
>>        */
>> -    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
>> -                                   s->regs[SPI_CLK_CFG_REG]);
>> +    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
>>       if (ecc_control == 0 || ecc_control == 2) {
>>           if (s->N1_bytes > (PNV_SPI_REG_SIZE + 1)) {
>>               qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size when "
>> @@ -340,8 +307,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>>           }
>>       } else if (s->N1_bytes > PNV_SPI_REG_SIZE) {
>>           qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size, "
>> -                      "bytes = 0x%x, bits = 0x%x\n",
>> -                      s->N1_bytes, s->N1_bits);
>> +                      "bytes = 0x%x, bits = 0x%x\n", s->N1_bytes, s->N1_bits);
>>           s->N1_bytes = PNV_SPI_REG_SIZE;
>>           s->N1_bits = s->N1_bytes * 8;
>>       }
>> @@ -350,19 +316,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.
>> @@ -413,9 +370,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
>> @@ -436,16 +394,14 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>>                * - we are receiving and the RDR is empty so we allow the operation
>>                *   to proceed.
>>                */
>> -            if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL,
>> -                                           s->status) == 1)) {
>> +            if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>>                   trace_pnv_spi_sequencer_stop_requested("shift N1"
>>                                   "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);
>>               }
> Is there any problem or overrun that needs to be logged if the
> fifo is full in the above two places?
Will add a error log upon hitting the overflow. Thank You.
>>           }
>>           n1_count++;
>> @@ -486,15 +442,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", fifo8_num_used(&s->tx_fifo));
>> +        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() */
>> @@ -552,13 +506,11 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>>                * If Forced Implicit mode and count control doesn't
>>                * indicate a receive then reset the rx count to 0
>>                */
>> -            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3,
>> -                                    s->regs[SPI_CTR_CFG_REG]) == 0) {
>> +            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 0) {
>>                   s->N2_rx = 0;
>>               }
>>               /* If tx count control for N2 is set, load the tx value */
>> -            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2,
>> -                                    s->regs[SPI_CTR_CFG_REG]) == 1) {
>> +            if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 1) {
>>                   s->N2_tx = s->N2_bytes;
>>               }
>>           }
>> @@ -571,8 +523,7 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>>        * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
>>        * error bit.
>>        */
>> -    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
>> -                    s->regs[SPI_CLK_CFG_REG]);
>> +    uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
>>       if (ecc_control == 0 || ecc_control == 2) {
>>           if (s->N2_bytes > (PNV_SPI_REG_SIZE + 1)) {
>>               /* Unsupported N2 shift size when ECC enabled */
>> @@ -590,19 +541,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.
>> @@ -629,44 +571,41 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>>            * code continue will end up building the payload twice in the same
>>            * buffer since RDR full causes a sequence stop and restart.
>>            */
>> -        if ((s->N2_rx != 0) &&
>> -            (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>> +        if ((s->N2_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>>               trace_pnv_spi_sequencer_stop_requested("shift N2 set"
>>                               "for receive but RDR is full");
>>               stop = true;
>>               break;
>>           }
>> -        if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) <
>> -                                PNV_SPI_REG_SIZE)) {
>> +        if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) < PNV_SPI_REG_SIZE)) {
>>               /* 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", fifo8_num_used(&s->tx_fifo));
>> +        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
>>            * don't log the message in every loop iteration.
>>            */
>> -        if ((s->N2_tx != 0) &&
>> -            (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
>> +        if ((s->N2_tx != 0) && (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
>>               s->status = SETFIELD(SPI_STS_TDR_FULL, s->status, 0);
>>           }
>>           /*
>> @@ -682,8 +621,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()*/
>> @@ -701,19 +638,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.
>> @@ -775,10 +699,8 @@ static void operation_sequencer(PnvSpi *s)
>>                   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 "
>> -                              "not supported, select = 0x%x\n",
>> -                               s->responder_select);
>> -                trace_pnv_spi_sequencer_stop_requested("invalid "
>> -                                "responder select");
>> +                              "not supported, select = 0x%x\n", s->responder_select);
>> +                trace_pnv_spi_sequencer_stop_requested("invalid responder select");
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
>>                   stop = true;
>>               } else {
>> @@ -840,9 +762,8 @@ static void operation_sequencer(PnvSpi *s)
>>                                   == SEQ_OP_SHIFT_N2) {
>>                       send_n1_alone = false;
>>                   }
>> -                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> -                                FSM_SHIFT_N1);
>> -                stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
>> +                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N1);
>> +                stop = operation_shiftn1(s, opcode, send_n1_alone);
>>                   if (stop) {
>>                       /*
>>                        *  The operation code says to stop, this can occur if:
>> @@ -858,7 +779,7 @@ static void operation_sequencer(PnvSpi *s)
>>                       if (GETFIELD(SPI_STS_TDR_UNDERRUN, s->status)) {
>>                           s->shift_n1_done = true;
>>                           s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> -                                                  FSM_SHIFT_N2);
>> +                                        FSM_SHIFT_N2);
>>                           s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
>>                                           (get_seq_index(s) + 1));
>>                       } else {
>> @@ -866,8 +787,7 @@ static void operation_sequencer(PnvSpi *s)
>>                            * This is case (1) or (2) so the sequencer needs to
>>                            * wait and NOT go to the next sequence yet.
>>                            */
>> -                        s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> -                                        FSM_WAIT);
>> +                        s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
>>                       }
>>                   } else {
>>                       /* Ok to move on to the next index */
>> @@ -890,21 +810,18 @@ static void operation_sequencer(PnvSpi *s)
>>                    * error bit 3 (general_SPI_status[3]) in status reg.
>>                    */
>>                   s->status = SETFIELD(SPI_STS_GEN_STATUS_B3, s->status, 1);
>> -                trace_pnv_spi_sequencer_stop_requested("shift_n2 "
>> -                                    "w/no shift_n1 done");
>> +                trace_pnv_spi_sequencer_stop_requested("shift_n2 w/no shift_n1 done");
>>                   stop = true;
>>               } else {
>>                   /* Ok to do a Shift_N2 */
>> -                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> -                                FSM_SHIFT_N2);
>> -                stop = operation_shiftn2(s, opcode, &payload);
>> +                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N2);
>> +                stop = operation_shiftn2(s, opcode);
>>                   /*
>>                    * If the operation code says to stop set the shifter state to
>>                    * wait and stop
>>                    */
>>                   if (stop) {
>> -                    s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> -                                    FSM_WAIT);
>> +                    s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
>>                   } else {
>>                       /* Ok to move on to the next index */
>>                       next_sequencer_fsm(s);
>> @@ -988,8 +905,7 @@ static void operation_sequencer(PnvSpi *s)
>>           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));
>> -            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
>> -                              s->regs[SPI_CTR_CFG_REG]);
>> +            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2, s->regs[SPI_CTR_CFG_REG]);
>>               /*
>>                * The spec says the loop should execute count compare + 1 times.
>>                * However we learned from engineering that we really only loop
>> @@ -1209,6 +1125,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);
> Aside from those small nits, it looks like a good cleanup and
> improvement with more error logging.
>
> Thanks,
> Nick

[-- Attachment #2: Type: text/html, Size: 26894 bytes --]

[PATCH v5 2/4] hw/ssi/pnv_spi: Coverity CID 1558827: Use local var seq_index instead of get_seq_index().

[Chalapathi V]

Use a local variable seq_index instead of repeatedly calling
get_seq_index() method and open-code next_sequencer_fsm().

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

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 63d298980d..87eac666bb 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -212,18 +212,6 @@ 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);
-}
-
 /*
  * Calculate the N1 counters based on passed in opcode and
  * internal register values.
@@ -637,6 +625,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]
@@ -650,12 +639,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);
         /*
@@ -672,7 +662,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);
@@ -683,7 +673,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
@@ -694,8 +684,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 "
@@ -720,13 +709,15 @@ static void operation_sequencer(PnvSpi *s)
                  * applies once a valid responder select has occurred.
                  */
                 s->shift_n1_done = false;
-                next_sequencer_fsm(s);
+                seq_index++;
+                s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
+                                SEQ_STATE_INDEX_INCREMENT);
             }
             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
@@ -758,8 +749,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, FSM_SHIFT_N1);
@@ -779,9 +771,8 @@ static void operation_sequencer(PnvSpi *s)
                     if (GETFIELD(SPI_STS_TDR_UNDERRUN, s->status)) {
                         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));
+                                                  FSM_SHIFT_N2);
+                        seq_index++;
                     } else {
                         /*
                          * This is case (1) or (2) so the sequencer needs to
@@ -792,14 +783,16 @@ static void operation_sequencer(PnvSpi *s)
                 } else {
                     /* Ok to move on to the next index */
                     s->shift_n1_done = true;
-                    next_sequencer_fsm(s);
+                    seq_index++;
+                    s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
+                                    SEQ_STATE_INDEX_INCREMENT);
                 }
             }
             break;
 
         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",
@@ -824,14 +817,16 @@ static void operation_sequencer(PnvSpi *s)
                     s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
                 } else {
                     /* Ok to move on to the next index */
-                    next_sequencer_fsm(s);
+                    seq_index++;
+                    s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
+                                    SEQ_STATE_INDEX_INCREMENT);
                 }
             }
             break;
 
         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).
@@ -847,15 +842,16 @@ static void operation_sequencer(PnvSpi *s)
                 if (rdr_matched) {
                     trace_pnv_spi_RDR_match("success");
                     /* A match occurred, increment the sequencer index. */
-                    next_sequencer_fsm(s);
+                    seq_index++;
+                    s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
+                                    SEQ_STATE_INDEX_INCREMENT);
                 } else {
                     trace_pnv_spi_RDR_match("failed");
                     /*
                      * 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
@@ -874,12 +870,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");
-            next_sequencer_fsm(s);
+            seq_index++;
+            s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
             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
@@ -893,19 +890,21 @@ 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 */
-                next_sequencer_fsm(s);
+                seq_index++;
+                s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
+                                SEQ_STATE_INDEX_INCREMENT);
             }
             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));
-            uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2, s->regs[SPI_CTR_CFG_REG]);
+            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]);
             /*
              * The spec says the loop should execute count compare + 1 times.
              * However we learned from engineering that we really only loop
@@ -918,19 +917,21 @@ 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 */
-                next_sequencer_fsm(s);
+                seq_index++;
+                s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
+                                SEQ_STATE_INDEX_INCREMENT);
             }
             break;
 
         default:
             s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
             /* Ignore unsupported operations. */
-            next_sequencer_fsm(s);
+            seq_index++;
+            s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
             break;
         } /* end of switch */
         /*
@@ -938,10 +939,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);
@@ -952,6 +953,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 v5 2/4] hw/ssi/pnv_spi: Coverity CID 1558827: Use local var seq_index instead of get_seq_index().

[Nicholas Piggin]

On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
> Use a local variable seq_index instead of repeatedly calling
> get_seq_index() method and open-code next_sequencer_fsm().
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  hw/ssi/pnv_spi.c | 93 +++++++++++++++++++++++++-----------------------
>  1 file changed, 48 insertions(+), 45 deletions(-)
>
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 63d298980d..87eac666bb 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -212,18 +212,6 @@ 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);
> -}
> -
>  /*
>   * Calculate the N1 counters based on passed in opcode and
>   * internal register values.
> @@ -637,6 +625,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]
> @@ -650,12 +639,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);

You could add a comment that this field is kept in seq_index and not
updated in place until the end of the function.

Otherwise looks good.

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

Re: [PATCH v5 2/4] hw/ssi/pnv_spi: Coverity CID 1558827: Use local var seq_index instead of get_seq_index().

[Nicholas Piggin]

On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
> Use a local variable seq_index instead of repeatedly calling
> get_seq_index() method and open-code next_sequencer_fsm().

Oh... one other thing, this is no longer a fix for CID
1558827, right? Just need to change the subject to remove
that.

Thanks,
Nick

[PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Chalapathi V]

Create a spi buses with distict names on each socket so that responders
are attached to correct SPI controllers.

QOM tree on a 2 socket machine:
(qemu) info qom-tree
/machine (powernv10-machine)
  /chip[0] (power10_v2.0-pnv-chip)
    /pib_spic[0] (pnv-spi)
      /chip0.pnv.spi.bus.0 (SSI)
      /xscom-spi[0] (memory-region)
  /chip[1] (power10_v2.0-pnv-chip)
    /pib_spic[0] (pnv-spi)
      /chip1.pnv.spi.bus.0 (SSI)
      /xscom-spi[0] (memory-region)

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 include/hw/ssi/pnv_spi.h           | 3 ++-
 hw/ppc/pnv.c                       | 2 ++
 hw/ssi/pnv_spi.c                   | 5 +++--
 tests/qtest/pnv-spi-seeprom-test.c | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
index 9878d9a25f..7fc5da1f84 100644
--- a/include/hw/ssi/pnv_spi.h
+++ b/include/hw/ssi/pnv_spi.h
@@ -31,7 +31,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
 #define PNV_SPI_REG_SIZE 8
 #define PNV_SPI_REGS 7
 
-#define TYPE_PNV_SPI_BUS "pnv-spi-bus"
+#define TYPE_PNV_SPI_BUS "pnv.spi.bus"
 typedef struct PnvSpi {
     SysBusDevice parent_obj;
 
@@ -42,6 +42,7 @@ typedef struct PnvSpi {
     Fifo8 rx_fifo;
     /* SPI object number */
     uint32_t        spic_num;
+    uint32_t        chip_id;
     uint8_t         transfer_len;
     uint8_t         responder_select;
     /* To verify if shift_n1 happens prior to shift_n2 */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 11fd477b71..ce23892fdf 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2226,6 +2226,8 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
         /* pib_spic[2] connected to 25csm04 which implements 1 byte transfer */
         object_property_set_int(OBJECT(&chip10->pib_spic[i]), "transfer_len",
                                 (i == 2) ? 1 : 4, &error_fatal);
+        object_property_set_int(OBJECT(&chip10->pib_spic[i]), "chip-id",
+                                chip->chip_id, &error_fatal);
         if (!sysbus_realize(SYS_BUS_DEVICE(OBJECT
                                         (&chip10->pib_spic[i])), errp)) {
             return;
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 87eac666bb..41beb559c6 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -1116,14 +1116,15 @@ static const MemoryRegionOps pnv_spi_xscom_ops = {
 
 static const Property pnv_spi_properties[] = {
     DEFINE_PROP_UINT32("spic_num", PnvSpi, spic_num, 0),
+    DEFINE_PROP_UINT32("chip-id", PnvSpi, chip_id, 0),
     DEFINE_PROP_UINT8("transfer_len", PnvSpi, transfer_len, 4),
 };
 
 static void pnv_spi_realize(DeviceState *dev, Error **errp)
 {
     PnvSpi *s = PNV_SPI(dev);
-    g_autofree char *name = g_strdup_printf(TYPE_PNV_SPI_BUS ".%d",
-                    s->spic_num);
+    g_autofree char *name = g_strdup_printf("chip%d." TYPE_PNV_SPI_BUS ".%d",
+                    s->chip_id, s->spic_num);
     s->ssi_bus = ssi_create_bus(dev, name);
     s->cs_line = g_new0(qemu_irq, 1);
     qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
diff --git a/tests/qtest/pnv-spi-seeprom-test.c b/tests/qtest/pnv-spi-seeprom-test.c
index 57f20af76e..ef1005a926 100644
--- a/tests/qtest/pnv-spi-seeprom-test.c
+++ b/tests/qtest/pnv-spi-seeprom-test.c
@@ -92,7 +92,7 @@ static void test_spi_seeprom(const void *data)
     qts = qtest_initf("-machine powernv10 -smp 2,cores=2,"
                       "threads=1 -accel tcg,thread=single -nographic "
                       "-blockdev node-name=pib_spic2,driver=file,"
-                      "filename=%s -device 25csm04,bus=pnv-spi-bus.2,cs=0,"
+                      "filename=%s -device 25csm04,bus=chip0.pnv.spi.bus.2,cs=0,"
                       "drive=pib_spic2", tmp_path);
     spi_seeprom_transaction(qts, chip);
     qtest_quit(qts);
-- 
2.39.5

Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Nicholas Piggin]

On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
> Create a spi buses with distict names on each socket so that responders
> are attached to correct SPI controllers.
>
> QOM tree on a 2 socket machine:
> (qemu) info qom-tree
> /machine (powernv10-machine)
>   /chip[0] (power10_v2.0-pnv-chip)
>     /pib_spic[0] (pnv-spi)
>       /chip0.pnv.spi.bus.0 (SSI)
>       /xscom-spi[0] (memory-region)
>   /chip[1] (power10_v2.0-pnv-chip)
>     /pib_spic[0] (pnv-spi)
>       /chip1.pnv.spi.bus.0 (SSI)
>       /xscom-spi[0] (memory-region)

Mechanics of the patch looks fine. I don't know about the name
though.

I think "pnv-spi-bus" is the right name for the bus. Using dots as
with chip0. makes it seem like each element is part of a topology.

Would chip0.pnv-spi-bus be better?

I don't suppose there is a good way to create an alias so existing
cmdline works and refers to the bus on chip0? Maybe the chip0 bus
could just not have the chip0. prefix?

Thanks,
Nick

>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  include/hw/ssi/pnv_spi.h           | 3 ++-
>  hw/ppc/pnv.c                       | 2 ++
>  hw/ssi/pnv_spi.c                   | 5 +++--
>  tests/qtest/pnv-spi-seeprom-test.c | 2 +-
>  4 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
> index 9878d9a25f..7fc5da1f84 100644
> --- a/include/hw/ssi/pnv_spi.h
> +++ b/include/hw/ssi/pnv_spi.h
> @@ -31,7 +31,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>  #define PNV_SPI_REG_SIZE 8
>  #define PNV_SPI_REGS 7
>  
> -#define TYPE_PNV_SPI_BUS "pnv-spi-bus"
> +#define TYPE_PNV_SPI_BUS "pnv.spi.bus"
>  typedef struct PnvSpi {
>      SysBusDevice parent_obj;
>  
> @@ -42,6 +42,7 @@ typedef struct PnvSpi {
>      Fifo8 rx_fifo;
>      /* SPI object number */
>      uint32_t        spic_num;
> +    uint32_t        chip_id;
>      uint8_t         transfer_len;
>      uint8_t         responder_select;
>      /* To verify if shift_n1 happens prior to shift_n2 */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 11fd477b71..ce23892fdf 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -2226,6 +2226,8 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>          /* pib_spic[2] connected to 25csm04 which implements 1 byte transfer */
>          object_property_set_int(OBJECT(&chip10->pib_spic[i]), "transfer_len",
>                                  (i == 2) ? 1 : 4, &error_fatal);
> +        object_property_set_int(OBJECT(&chip10->pib_spic[i]), "chip-id",
> +                                chip->chip_id, &error_fatal);
>          if (!sysbus_realize(SYS_BUS_DEVICE(OBJECT
>                                          (&chip10->pib_spic[i])), errp)) {
>              return;
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 87eac666bb..41beb559c6 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -1116,14 +1116,15 @@ static const MemoryRegionOps pnv_spi_xscom_ops = {
>  
>  static const Property pnv_spi_properties[] = {
>      DEFINE_PROP_UINT32("spic_num", PnvSpi, spic_num, 0),
> +    DEFINE_PROP_UINT32("chip-id", PnvSpi, chip_id, 0),
>      DEFINE_PROP_UINT8("transfer_len", PnvSpi, transfer_len, 4),
>  };
>  
>  static void pnv_spi_realize(DeviceState *dev, Error **errp)
>  {
>      PnvSpi *s = PNV_SPI(dev);
> -    g_autofree char *name = g_strdup_printf(TYPE_PNV_SPI_BUS ".%d",
> -                    s->spic_num);
> +    g_autofree char *name = g_strdup_printf("chip%d." TYPE_PNV_SPI_BUS ".%d",
> +                    s->chip_id, s->spic_num);
>      s->ssi_bus = ssi_create_bus(dev, name);
>      s->cs_line = g_new0(qemu_irq, 1);
>      qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
> diff --git a/tests/qtest/pnv-spi-seeprom-test.c b/tests/qtest/pnv-spi-seeprom-test.c
> index 57f20af76e..ef1005a926 100644
> --- a/tests/qtest/pnv-spi-seeprom-test.c
> +++ b/tests/qtest/pnv-spi-seeprom-test.c
> @@ -92,7 +92,7 @@ static void test_spi_seeprom(const void *data)
>      qts = qtest_initf("-machine powernv10 -smp 2,cores=2,"
>                        "threads=1 -accel tcg,thread=single -nographic "
>                        "-blockdev node-name=pib_spic2,driver=file,"
> -                      "filename=%s -device 25csm04,bus=pnv-spi-bus.2,cs=0,"
> +                      "filename=%s -device 25csm04,bus=chip0.pnv.spi.bus.2,cs=0,"


>                        "drive=pib_spic2", tmp_path);
>      spi_seeprom_transaction(qts, chip);
>      qtest_quit(qts);

Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Chalapathi V]

[-- Attachment #1: Type: text/plain, Size: 4816 bytes --]


On 27-02-2025 07:24, Nicholas Piggin wrote:
> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>> Create a spi buses with distict names on each socket so that responders
>> are attached to correct SPI controllers.
>>
>> QOM tree on a 2 socket machine:
>> (qemu) info qom-tree
>> /machine (powernv10-machine)
>>    /chip[0] (power10_v2.0-pnv-chip)
>>      /pib_spic[0] (pnv-spi)
>>        /chip0.pnv.spi.bus.0 (SSI)
>>        /xscom-spi[0] (memory-region)
>>    /chip[1] (power10_v2.0-pnv-chip)
>>      /pib_spic[0] (pnv-spi)
>>        /chip1.pnv.spi.bus.0 (SSI)
>>        /xscom-spi[0] (memory-region)
> Mechanics of the patch looks fine. I don't know about the name
> though.
>
> I think "pnv-spi-bus" is the right name for the bus. Using dots as
> with chip0. makes it seem like each element is part of a topology.
>
> Would chip0.pnv-spi-bus be better?
Will rename the bus name to chip0.pnv-spi-bus . Thank You
>
> I don't suppose there is a good way to create an alias so existing
> cmdline works and refers to the bus on chip0? Maybe the chip0 bus
> could just not have the chip0. prefix?
>
> Thanks,
> Nick
Would it be best to keep the chip0 prefix to have uniformity?
>> Signed-off-by: Chalapathi V<chalapathi.v@linux.ibm.com>
>> ---
>>   include/hw/ssi/pnv_spi.h           | 3 ++-
>>   hw/ppc/pnv.c                       | 2 ++
>>   hw/ssi/pnv_spi.c                   | 5 +++--
>>   tests/qtest/pnv-spi-seeprom-test.c | 2 +-
>>   4 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
>> index 9878d9a25f..7fc5da1f84 100644
>> --- a/include/hw/ssi/pnv_spi.h
>> +++ b/include/hw/ssi/pnv_spi.h
>> @@ -31,7 +31,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>>   #define PNV_SPI_REG_SIZE 8
>>   #define PNV_SPI_REGS 7
>>   
>> -#define TYPE_PNV_SPI_BUS "pnv-spi-bus"
>> +#define TYPE_PNV_SPI_BUS "pnv.spi.bus"
>>   typedef struct PnvSpi {
>>       SysBusDevice parent_obj;
>>   
>> @@ -42,6 +42,7 @@ typedef struct PnvSpi {
>>       Fifo8 rx_fifo;
>>       /* SPI object number */
>>       uint32_t        spic_num;
>> +    uint32_t        chip_id;
>>       uint8_t         transfer_len;
>>       uint8_t         responder_select;
>>       /* To verify if shift_n1 happens prior to shift_n2 */
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 11fd477b71..ce23892fdf 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -2226,6 +2226,8 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>>           /* pib_spic[2] connected to 25csm04 which implements 1 byte transfer */
>>           object_property_set_int(OBJECT(&chip10->pib_spic[i]), "transfer_len",
>>                                   (i == 2) ? 1 : 4, &error_fatal);
>> +        object_property_set_int(OBJECT(&chip10->pib_spic[i]), "chip-id",
>> +                                chip->chip_id, &error_fatal);
>>           if (!sysbus_realize(SYS_BUS_DEVICE(OBJECT
>>                                           (&chip10->pib_spic[i])), errp)) {
>>               return;
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 87eac666bb..41beb559c6 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -1116,14 +1116,15 @@ static const MemoryRegionOps pnv_spi_xscom_ops = {
>>   
>>   static const Property pnv_spi_properties[] = {
>>       DEFINE_PROP_UINT32("spic_num", PnvSpi, spic_num, 0),
>> +    DEFINE_PROP_UINT32("chip-id", PnvSpi, chip_id, 0),
>>       DEFINE_PROP_UINT8("transfer_len", PnvSpi, transfer_len, 4),
>>   };
>>   
>>   static void pnv_spi_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvSpi *s = PNV_SPI(dev);
>> -    g_autofree char *name = g_strdup_printf(TYPE_PNV_SPI_BUS ".%d",
>> -                    s->spic_num);
>> +    g_autofree char *name = g_strdup_printf("chip%d." TYPE_PNV_SPI_BUS ".%d",
>> +                    s->chip_id, s->spic_num);
>>       s->ssi_bus = ssi_create_bus(dev, name);
>>       s->cs_line = g_new0(qemu_irq, 1);
>>       qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>> diff --git a/tests/qtest/pnv-spi-seeprom-test.c b/tests/qtest/pnv-spi-seeprom-test.c
>> index 57f20af76e..ef1005a926 100644
>> --- a/tests/qtest/pnv-spi-seeprom-test.c
>> +++ b/tests/qtest/pnv-spi-seeprom-test.c
>> @@ -92,7 +92,7 @@ static void test_spi_seeprom(const void *data)
>>       qts = qtest_initf("-machine powernv10 -smp 2,cores=2,"
>>                         "threads=1 -accel tcg,thread=single -nographic "
>>                         "-blockdev node-name=pib_spic2,driver=file,"
>> -                      "filename=%s -device 25csm04,bus=pnv-spi-bus.2,cs=0,"
>> +                      "filename=%s -device 25csm04,bus=chip0.pnv.spi.bus.2,cs=0,"
>
>>                         "drive=pib_spic2", tmp_path);
>>       spi_seeprom_transaction(qts, chip);
>>       qtest_quit(qts);

[-- Attachment #2: Type: text/html, Size: 5771 bytes --]

Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Cédric Le Goater]

On 2/28/25 04:03, Chalapathi V wrote:
> 
> On 27-02-2025 07:24, Nicholas Piggin wrote:
>> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>>> Create a spi buses with distict names on each socket so that responders
>>> are attached to correct SPI controllers.
>>>
>>> QOM tree on a 2 socket machine:
>>> (qemu) info qom-tree
>>> /machine (powernv10-machine)
>>>    /chip[0] (power10_v2.0-pnv-chip)
>>>      /pib_spic[0] (pnv-spi)
>>>        /chip0.pnv.spi.bus.0 (SSI)
>>>        /xscom-spi[0] (memory-region)
>>>    /chip[1] (power10_v2.0-pnv-chip)
>>>      /pib_spic[0] (pnv-spi)
>>>        /chip1.pnv.spi.bus.0 (SSI)
>>>        /xscom-spi[0] (memory-region)
>> Mechanics of the patch looks fine. I don't know about the name
>> though.
>>
>> I think "pnv-spi-bus" is the right name for the bus. Using dots as
>> with chip0. makes it seem like each element is part of a topology.
>>
>> Would chip0.pnv-spi-bus be better?
> Will rename the bus name to chip0.pnv-spi-bus . Thank You

Yep. I don't think the bus suffix is useful (minor).

Will you be attaching flash devices from the command line ? Can you provide
an example if so ?

Thanks,

C.


>> I don't suppose there is a good way to create an alias so existing
>> cmdline works and refers to the bus on chip0? Maybe the chip0 bus
>> could just not have the chip0. prefix?
>>
>> Thanks,
>> Nick
> Would it be best to keep the chip0 prefix to have uniformity?
>>> Signed-off-by: Chalapathi V<chalapathi.v@linux.ibm.com>
>>> ---
>>>   include/hw/ssi/pnv_spi.h           | 3 ++-
>>>   hw/ppc/pnv.c                       | 2 ++
>>>   hw/ssi/pnv_spi.c                   | 5 +++--
>>>   tests/qtest/pnv-spi-seeprom-test.c | 2 +-
>>>   4 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
>>> index 9878d9a25f..7fc5da1f84 100644
>>> --- a/include/hw/ssi/pnv_spi.h
>>> +++ b/include/hw/ssi/pnv_spi.h
>>> @@ -31,7 +31,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>>>   #define PNV_SPI_REG_SIZE 8
>>>   #define PNV_SPI_REGS 7
>>>   
>>> -#define TYPE_PNV_SPI_BUS "pnv-spi-bus"
>>> +#define TYPE_PNV_SPI_BUS "pnv.spi.bus"
>>>   typedef struct PnvSpi {
>>>       SysBusDevice parent_obj;
>>>   
>>> @@ -42,6 +42,7 @@ typedef struct PnvSpi {
>>>       Fifo8 rx_fifo;
>>>       /* SPI object number */
>>>       uint32_t        spic_num;
>>> +    uint32_t        chip_id;
>>>       uint8_t         transfer_len;
>>>       uint8_t         responder_select;
>>>       /* To verify if shift_n1 happens prior to shift_n2 */
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 11fd477b71..ce23892fdf 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -2226,6 +2226,8 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>>>           /* pib_spic[2] connected to 25csm04 which implements 1 byte transfer */
>>>           object_property_set_int(OBJECT(&chip10->pib_spic[i]), "transfer_len",
>>>                                   (i == 2) ? 1 : 4, &error_fatal);
>>> +        object_property_set_int(OBJECT(&chip10->pib_spic[i]), "chip-id",
>>> +                                chip->chip_id, &error_fatal);
>>>           if (!sysbus_realize(SYS_BUS_DEVICE(OBJECT
>>>                                           (&chip10->pib_spic[i])), errp)) {
>>>               return;
>>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>>> index 87eac666bb..41beb559c6 100644
>>> --- a/hw/ssi/pnv_spi.c
>>> +++ b/hw/ssi/pnv_spi.c
>>> @@ -1116,14 +1116,15 @@ static const MemoryRegionOps pnv_spi_xscom_ops = {
>>>   
>>>   static const Property pnv_spi_properties[] = {
>>>       DEFINE_PROP_UINT32("spic_num", PnvSpi, spic_num, 0),
>>> +    DEFINE_PROP_UINT32("chip-id", PnvSpi, chip_id, 0),
>>>       DEFINE_PROP_UINT8("transfer_len", PnvSpi, transfer_len, 4),
>>>   };
>>>   
>>>   static void pnv_spi_realize(DeviceState *dev, Error **errp)
>>>   {
>>>       PnvSpi *s = PNV_SPI(dev);
>>> -    g_autofree char *name = g_strdup_printf(TYPE_PNV_SPI_BUS ".%d",
>>> -                    s->spic_num);
>>> +    g_autofree char *name = g_strdup_printf("chip%d." TYPE_PNV_SPI_BUS ".%d",
>>> +                    s->chip_id, s->spic_num);
>>>       s->ssi_bus = ssi_create_bus(dev, name);
>>>       s->cs_line = g_new0(qemu_irq, 1);
>>>       qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>>> diff --git a/tests/qtest/pnv-spi-seeprom-test.c b/tests/qtest/pnv-spi-seeprom-test.c
>>> index 57f20af76e..ef1005a926 100644
>>> --- a/tests/qtest/pnv-spi-seeprom-test.c
>>> +++ b/tests/qtest/pnv-spi-seeprom-test.c
>>> @@ -92,7 +92,7 @@ static void test_spi_seeprom(const void *data)
>>>       qts = qtest_initf("-machine powernv10 -smp 2,cores=2,"
>>>                         "threads=1 -accel tcg,thread=single -nographic "
>>>                         "-blockdev node-name=pib_spic2,driver=file,"
>>> -                      "filename=%s -device 25csm04,bus=pnv-spi-bus.2,cs=0,"
>>> +                      "filename=%s -device 25csm04,bus=chip0.pnv.spi.bus.2,cs=0,"
>>>                         "drive=pib_spic2", tmp_path);
>>>       spi_seeprom_transaction(qts, chip);
>>>       qtest_quit(qts);

Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Chalapathi V]

On 28-02-2025 13:15, Cédric Le Goater wrote:
> On 2/28/25 04:03, Chalapathi V wrote:
>>
>> On 27-02-2025 07:24, Nicholas Piggin wrote:
>>> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>>>> Create a spi buses with distict names on each socket so that 
>>>> responders
>>>> are attached to correct SPI controllers.
>>>>
>>>> QOM tree on a 2 socket machine:
>>>> (qemu) info qom-tree
>>>> /machine (powernv10-machine)
>>>>    /chip[0] (power10_v2.0-pnv-chip)
>>>>      /pib_spic[0] (pnv-spi)
>>>>        /chip0.pnv.spi.bus.0 (SSI)
>>>>        /xscom-spi[0] (memory-region)
>>>>    /chip[1] (power10_v2.0-pnv-chip)
>>>>      /pib_spic[0] (pnv-spi)
>>>>        /chip1.pnv.spi.bus.0 (SSI)
>>>>        /xscom-spi[0] (memory-region)
>>> Mechanics of the patch looks fine. I don't know about the name
>>> though.
>>>
>>> I think "pnv-spi-bus" is the right name for the bus. Using dots as
>>> with chip0. makes it seem like each element is part of a topology.
>>>
>>> Would chip0.pnv-spi-bus be better?
>> Will rename the bus name to chip0.pnv-spi-bus . Thank You
>
> Yep. I don't think the bus suffix is useful (minor).
>
> Will you be attaching flash devices from the command line ? Can you 
> provide
> an example if so ?
>
> Thanks,
>
> C.
>
Yes, I am attaching seeprom and TPM device from command line.
"-blockdev node-name=pib_spic2,driver=file,filename=%s -device 
25csm04,bus=chip0.pnv-spi-bus.2,cs=0,drive=pib_spic2"

"-chardev 
socket,id=chrtpm,path=/tmp/swtpm.chalap1.250227212039.rOWBH/swtpm-sock 
-tpmdev emulator,id=tpm0,chardev=chrtpm -device 
tpm-tis-spi,tpmdev=tpm0,bus=chip0.pnv-spi-bus.4"

Thank You,

Chalapathi

>
>>> I don't suppose there is a good way to create an alias so existing
>>> cmdline works and refers to the bus on chip0? Maybe the chip0 bus
>>> could just not have the chip0. prefix?
>>>
>>> Thanks,
>>> Nick
>> Would it be best to keep the chip0 prefix to have uniformity?
>>>> Signed-off-by: Chalapathi V<chalapathi.v@linux.ibm.com>
>>>> ---
>>>>   include/hw/ssi/pnv_spi.h           | 3 ++-
>>>>   hw/ppc/pnv.c                       | 2 ++
>>>>   hw/ssi/pnv_spi.c                   | 5 +++--
>>>>   tests/qtest/pnv-spi-seeprom-test.c | 2 +-
>>>>   4 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
>>>> index 9878d9a25f..7fc5da1f84 100644
>>>> --- a/include/hw/ssi/pnv_spi.h
>>>> +++ b/include/hw/ssi/pnv_spi.h
>>>> @@ -31,7 +31,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>>>>   #define PNV_SPI_REG_SIZE 8
>>>>   #define PNV_SPI_REGS 7
>>>>   -#define TYPE_PNV_SPI_BUS "pnv-spi-bus"
>>>> +#define TYPE_PNV_SPI_BUS "pnv.spi.bus"
>>>>   typedef struct PnvSpi {
>>>>       SysBusDevice parent_obj;
>>>>   @@ -42,6 +42,7 @@ typedef struct PnvSpi {
>>>>       Fifo8 rx_fifo;
>>>>       /* SPI object number */
>>>>       uint32_t        spic_num;
>>>> +    uint32_t        chip_id;
>>>>       uint8_t         transfer_len;
>>>>       uint8_t         responder_select;
>>>>       /* To verify if shift_n1 happens prior to shift_n2 */
>>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>>> index 11fd477b71..ce23892fdf 100644
>>>> --- a/hw/ppc/pnv.c
>>>> +++ b/hw/ppc/pnv.c
>>>> @@ -2226,6 +2226,8 @@ static void 
>>>> pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>>>>           /* pib_spic[2] connected to 25csm04 which implements 1 
>>>> byte transfer */
>>>> object_property_set_int(OBJECT(&chip10->pib_spic[i]), "transfer_len",
>>>>                                   (i == 2) ? 1 : 4, &error_fatal);
>>>> + object_property_set_int(OBJECT(&chip10->pib_spic[i]), "chip-id",
>>>> +                                chip->chip_id, &error_fatal);
>>>>           if (!sysbus_realize(SYS_BUS_DEVICE(OBJECT
>>>> (&chip10->pib_spic[i])), errp)) {
>>>>               return;
>>>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>>>> index 87eac666bb..41beb559c6 100644
>>>> --- a/hw/ssi/pnv_spi.c
>>>> +++ b/hw/ssi/pnv_spi.c
>>>> @@ -1116,14 +1116,15 @@ static const MemoryRegionOps 
>>>> pnv_spi_xscom_ops = {
>>>>     static const Property pnv_spi_properties[] = {
>>>>       DEFINE_PROP_UINT32("spic_num", PnvSpi, spic_num, 0),
>>>> +    DEFINE_PROP_UINT32("chip-id", PnvSpi, chip_id, 0),
>>>>       DEFINE_PROP_UINT8("transfer_len", PnvSpi, transfer_len, 4),
>>>>   };
>>>>     static void pnv_spi_realize(DeviceState *dev, Error **errp)
>>>>   {
>>>>       PnvSpi *s = PNV_SPI(dev);
>>>> -    g_autofree char *name = g_strdup_printf(TYPE_PNV_SPI_BUS ".%d",
>>>> -                    s->spic_num);
>>>> +    g_autofree char *name = g_strdup_printf("chip%d." 
>>>> TYPE_PNV_SPI_BUS ".%d",
>>>> +                    s->chip_id, s->spic_num);
>>>>       s->ssi_bus = ssi_create_bus(dev, name);
>>>>       s->cs_line = g_new0(qemu_irq, 1);
>>>>       qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>>>> diff --git a/tests/qtest/pnv-spi-seeprom-test.c 
>>>> b/tests/qtest/pnv-spi-seeprom-test.c
>>>> index 57f20af76e..ef1005a926 100644
>>>> --- a/tests/qtest/pnv-spi-seeprom-test.c
>>>> +++ b/tests/qtest/pnv-spi-seeprom-test.c
>>>> @@ -92,7 +92,7 @@ static void test_spi_seeprom(const void *data)
>>>>       qts = qtest_initf("-machine powernv10 -smp 2,cores=2,"
>>>>                         "threads=1 -accel tcg,thread=single 
>>>> -nographic "
>>>>                         "-blockdev node-name=pib_spic2,driver=file,"
>>>> -                      "filename=%s -device 
>>>> 25csm04,bus=pnv-spi-bus.2,cs=0,"
>>>> +                      "filename=%s -device 
>>>> 25csm04,bus=chip0.pnv.spi.bus.2,cs=0,"
>>>>                         "drive=pib_spic2", tmp_path);
>>>>       spi_seeprom_transaction(qts, chip);
>>>>       qtest_quit(qts);
>

Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Cédric Le Goater]

On 2/28/25 12:00, Chalapathi V wrote:
> 
> On 28-02-2025 13:15, Cédric Le Goater wrote:
>> On 2/28/25 04:03, Chalapathi V wrote:
>>>
>>> On 27-02-2025 07:24, Nicholas Piggin wrote:
>>>> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>>>>> Create a spi buses with distict names on each socket so that responders
>>>>> are attached to correct SPI controllers.
>>>>>
>>>>> QOM tree on a 2 socket machine:
>>>>> (qemu) info qom-tree
>>>>> /machine (powernv10-machine)
>>>>>    /chip[0] (power10_v2.0-pnv-chip)
>>>>>      /pib_spic[0] (pnv-spi)
>>>>>        /chip0.pnv.spi.bus.0 (SSI)
>>>>>        /xscom-spi[0] (memory-region)
>>>>>    /chip[1] (power10_v2.0-pnv-chip)
>>>>>      /pib_spic[0] (pnv-spi)
>>>>>        /chip1.pnv.spi.bus.0 (SSI)
>>>>>        /xscom-spi[0] (memory-region)
>>>> Mechanics of the patch looks fine. I don't know about the name
>>>> though.
>>>>
>>>> I think "pnv-spi-bus" is the right name for the bus. Using dots as
>>>> with chip0. makes it seem like each element is part of a topology.
>>>>
>>>> Would chip0.pnv-spi-bus be better?
>>> Will rename the bus name to chip0.pnv-spi-bus . Thank You
>>
>> Yep. I don't think the bus suffix is useful (minor).
>>
>> Will you be attaching flash devices from the command line ? Can you provide
>> an example if so ?
>>
>> Thanks,
>>
>> C.
>>
> Yes, I am attaching seeprom and TPM device from command line.
> "-blockdev node-name=pib_spic2,driver=file,filename=%s -device 25csm04,bus=chip0.pnv-spi-bus.2,cs=0,drive=pib_spic2"
> 
> "-chardev socket,id=chrtpm,path=/tmp/swtpm.chalap1.250227212039.rOWBH/swtpm-sock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-tis-spi,tpmdev=tpm0,bus=chip0.pnv-spi-bus.4"
  
"chipX.spi.4" should be enough to identify the bus IMO.

Thanks,

C.

[PATCH v5 4/4] hw/ssi/pnv_spi: Put a limit to RDR match failures

[Chalapathi V]

There is a possibility that SPI controller can get into loop due to indefinite
RDR match failures. Hence put a limit to failures and stop the sequencer.

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

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 41beb559c6..d605fa8b46 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -20,6 +20,7 @@
 #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
 #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
 #define PNV_SPI_FIFO_SIZE 16
+#define RDR_MATCH_FAILURE_LIMIT 16
 
 /*
  * Macro from include/hw/ppc/fdt.h
@@ -838,21 +839,31 @@ static void operation_sequencer(PnvSpi *s)
              */
             if (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1) {
                 bool rdr_matched = false;
+                static int fail_count;
                 rdr_matched = does_rdr_match(s);
                 if (rdr_matched) {
                     trace_pnv_spi_RDR_match("success");
+                    fail_count = 0;
                     /* A match occurred, increment the sequencer index. */
                     seq_index++;
                     s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
                                     SEQ_STATE_INDEX_INCREMENT);
                 } else {
                     trace_pnv_spi_RDR_match("failed");
+                    fail_count++;
                     /*
                      * Branch the sequencer to the index coded into the op
                      * code.
                      */
                     seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
                 }
+                if (fail_count >= RDR_MATCH_FAILURE_LIMIT) {
+                    qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RDR match failure"
+                                  " limit crossed %d times hence requesting "
+                                  "sequencer to stop.\n",
+                                  RDR_MATCH_FAILURE_LIMIT);
+                    stop = true;
+                }
                 /*
                  * Regardless of where the branch ended up we want the
                  * sequencer to continue shifting so we have to clear
-- 
2.39.5

Re: [PATCH v5 4/4] hw/ssi/pnv_spi: Put a limit to RDR match failures

[Nicholas Piggin]

On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
> There is a possibility that SPI controller can get into loop due to indefinite
> RDR match failures. Hence put a limit to failures and stop the sequencer.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  hw/ssi/pnv_spi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 41beb559c6..d605fa8b46 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -20,6 +20,7 @@
>  #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>  #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>  #define PNV_SPI_FIFO_SIZE 16
> +#define RDR_MATCH_FAILURE_LIMIT 16
>  
>  /*
>   * Macro from include/hw/ppc/fdt.h
> @@ -838,21 +839,31 @@ static void operation_sequencer(PnvSpi *s)
>               */
>              if (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1) {
>                  bool rdr_matched = false;
> +                static int fail_count;

This will be shared by SPI instances, is that okay or should it be
in PnvSpi?

Other than that, looks good.

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

Re: [PATCH v5 4/4] hw/ssi/pnv_spi: Put a limit to RDR match failures

[Chalapathi V]

On 27-02-2025 07:26, Nicholas Piggin wrote:
> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>> There is a possibility that SPI controller can get into loop due to indefinite
>> RDR match failures. Hence put a limit to failures and stop the sequencer.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   hw/ssi/pnv_spi.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 41beb559c6..d605fa8b46 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -20,6 +20,7 @@
>>   #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>>   #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>>   #define PNV_SPI_FIFO_SIZE 16
>> +#define RDR_MATCH_FAILURE_LIMIT 16
>>   
>>   /*
>>    * Macro from include/hw/ppc/fdt.h
>> @@ -838,21 +839,31 @@ static void operation_sequencer(PnvSpi *s)
>>                */
>>               if (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1) {
>>                   bool rdr_matched = false;
>> +                static int fail_count;
> This will be shared by SPI instances, is that okay or should it be
> in PnvSpi?
>
> Other than that, looks good.
This should be in PnvSpi. Will update in V6. Thank You.
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>