* [PATCH v6 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
2025-03-03 14:13 [PATCH v6 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and other improvements Chalapathi V
@ 2025-03-03 14:13 ` Chalapathi V
2025-03-03 14:13 ` [PATCH v6 2/4] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index() Chalapathi V
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Chalapathi V @ 2025-03-03 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, fbarrat, npiggin, clg, calebs, chalapathi.v,
chalapathi.v, saif.abrar, dantan, milesg, philmd, alistair
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 | 264 ++++++++++++++++-----------------------
2 files changed, 108 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..388b425157 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,24 @@ 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;
+ int i;
/*
* Processing here must handle:
@@ -144,13 +115,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 +147,23 @@ 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) {
+ /* pop out N1_bytes from rx_fifo if not already */
+ if (s->N1_rx == 0) {
+ for (i = 0; i < s->N1_bytes; i++) {
+ if (!fifo8_is_empty(&s->rx_fifo)) {
+ fifo8_pop(&s->rx_fifo);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: Reading empty"
+ " RX_FIFO\n");
+ }
+ }
+ }
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 +190,41 @@ 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) {
+ break;
+ }
+ 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);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RX_FIFO is full\n");
break;
}
- *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
- (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
}
}
- 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 +295,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 +311,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 +322,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 +331,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 +385,13 @@ 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 {
+ qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO is full\n");
+ break;
+ }
} else {
/*
* We hit a shift_n1 opcode TX but the TDR is empty, tell the
@@ -436,16 +412,17 @@ 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);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO is full\n");
+ break;
}
}
n1_count++;
@@ -486,15 +463,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 +527,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 +544,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 +562,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 +592,47 @@ 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 {
+ qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO is full\n");
+ break;
+ }
+ } 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);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO is full\n");
+ break;
}
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 +648,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 +665,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 +726,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 +789,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 +806,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 +814,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 +837,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 +932,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 +1152,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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v6 2/4] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
2025-03-03 14:13 [PATCH v6 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and other improvements Chalapathi V
2025-03-03 14:13 ` [PATCH v6 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
@ 2025-03-03 14:13 ` Chalapathi V
2025-03-03 14:13 ` [PATCH v6 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket Chalapathi V
2025-03-03 14:13 ` [PATCH v6 4/4] hw/ssi/pnv_spi: Put a limit to RDR match failures Chalapathi V
3 siblings, 0 replies; 5+ messages in thread
From: Chalapathi V @ 2025-03-03 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, fbarrat, npiggin, clg, calebs, chalapathi.v,
chalapathi.v, saif.abrar, dantan, milesg, philmd, alistair
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>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/ssi/pnv_spi.c | 97 ++++++++++++++++++++++++++----------------------
1 file changed, 52 insertions(+), 45 deletions(-)
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 388b425157..de33542c35 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -227,18 +227,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.
@@ -664,6 +652,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]
@@ -677,12 +666,17 @@ 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);
}
+ /*
+ * SPI_STS_SEQ_INDEX of status register is kept in seq_index variable and
+ * updated back to status register at the end of operation_sequencer().
+ */
+ 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);
/*
@@ -699,7 +693,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);
@@ -710,7 +704,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
@@ -721,8 +715,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 "
@@ -747,13 +740,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
@@ -785,8 +780,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);
@@ -806,9 +802,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
@@ -819,14 +814,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",
@@ -851,14 +848,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).
@@ -874,15 +873,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
@@ -901,12 +901,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
@@ -920,19 +921,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
@@ -945,19 +948,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 */
/*
@@ -965,10 +970,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);
@@ -979,6 +984,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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v6 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
2025-03-03 14:13 [PATCH v6 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and other improvements Chalapathi V
2025-03-03 14:13 ` [PATCH v6 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
2025-03-03 14:13 ` [PATCH v6 2/4] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index() Chalapathi V
@ 2025-03-03 14:13 ` Chalapathi V
2025-03-03 14:13 ` [PATCH v6 4/4] hw/ssi/pnv_spi: Put a limit to RDR match failures Chalapathi V
3 siblings, 0 replies; 5+ messages in thread
From: Chalapathi V @ 2025-03-03 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, fbarrat, npiggin, clg, calebs, chalapathi.v,
chalapathi.v, saif.abrar, dantan, milesg, philmd, alistair
Create a spi buses with distinct names on each socket so that responders
are attached to correct SPI controllers.
Change the bus name to chipX.spi.<busnum> where X = 0..<num_sockets>
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.spi.0 (SSI)
/xscom-spi[0] (memory-region)
/chip[1] (power10_v2.0-pnv-chip)
/pib_spic[0] (pnv-spi)
/chip1.spi.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..6adb72dbb2 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 "spi"
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 de33542c35..83221607c9 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -1147,14 +1147,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..600493c425 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.spi.2,cs=0,"
"drive=pib_spic2", tmp_path);
spi_seeprom_transaction(qts, chip);
qtest_quit(qts);
--
2.39.5
^ permalink raw reply related [flat|nested] 5+ messages in thread