* [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()
@ 2024-09-18 16:50 Chalapathi V
2024-09-18 16:50 ` [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section Chalapathi V
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Chalapathi V @ 2024-09-18 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, fbarrat, npiggin, clg, calebs, chalapathi.v,
chalapathi.v, saif.abrar, dantan, milesg, philmd, alistair
Hello,
v3:
1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
2. Use of PnvXferBuffer results in a additonal process overhead due to
frequent dynamic allocations and hence use an existing Fifo8 buffer.
3. Use a local variable seq_index and use it with in while loop instead
of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
overrun when seq_index is incremented.
Tested:
passed make check and make check-avocado
Supersedes: <20240807202804.56038-1-philmd@linaro.org>
Philippe Mathieu-Daudé (1):
MAINTAINERS: Cover PowerPC SPI model in PowerNV section
Chalapathi V (2):
hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
MAINTAINERS | 2 +
include/hw/ssi/pnv_spi.h | 3 +
hw/ssi/pnv_spi.c | 228 +++++++++++++++------------------------
3 files changed, 89 insertions(+), 144 deletions(-)
--
2.39.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section
2024-09-18 16:50 [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Chalapathi V
@ 2024-09-18 16:50 ` Chalapathi V
2024-10-08 7:44 ` Nicholas Piggin
2024-09-18 16:50 ` [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Chalapathi V @ 2024-09-18 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, fbarrat, npiggin, clg, calebs, chalapathi.v,
chalapathi.v, saif.abrar, dantan, milesg, philmd, alistair
From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
It is unfair to let the PowerNV SPI model to the SSI
maintainers. Also include the PowerNV ones.
Fixes: 29318db133 ("hw/ssi: Add SPI model")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index ffacd60f40..b11c4edaf0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1541,8 +1541,10 @@ F: hw/ppc/pnv*
F: hw/intc/pnv*
F: hw/intc/xics_pnv.c
F: hw/pci-host/pnv*
+F: hw/ssi/pnv_spi.c
F: include/hw/ppc/pnv*
F: include/hw/pci-host/pnv*
+F: hw/ssi/pnv_spi*
F: pc-bios/skiboot.lid
F: tests/qtest/pnv*
F: tests/functional/test_ppc64_powernv.py
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
2024-09-18 16:50 [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Chalapathi V
2024-09-18 16:50 ` [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section Chalapathi V
@ 2024-09-18 16:50 ` Chalapathi V
2024-10-08 7:59 ` Nicholas Piggin
2024-09-18 16:50 ` [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index() Chalapathi V
2024-10-08 7:43 ` [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Nicholas Piggin
3 siblings, 1 reply; 13+ messages in thread
From: Chalapathi V @ 2024-09-18 16:50 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 | 167 +++++++++++++--------------------------
2 files changed, 56 insertions(+), 114 deletions(-)
diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
index 8815f67d45..9878d9a25f 100644
--- a/include/hw/ssi/pnv_spi.h
+++ b/include/hw/ssi/pnv_spi.h
@@ -23,6 +23,7 @@
#include "hw/ssi/ssi.h"
#include "hw/sysbus.h"
+#include "qemu/fifo8.h"
#define TYPE_PNV_SPI "pnv-spi"
OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
@@ -37,6 +38,8 @@ typedef struct PnvSpi {
SSIBus *ssi_bus;
qemu_irq *cs_line;
MemoryRegion xscom_spic_regs;
+ Fifo8 tx_fifo;
+ Fifo8 rx_fifo;
/* SPI object number */
uint32_t spic_num;
uint8_t transfer_len;
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 9e7207bf7c..2fd5aa0a96 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -19,6 +19,7 @@
#define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
#define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
+#define PNV_SPI_FIFO_SIZE 16
/*
* Macro from include/hw/ppc/fdt.h
@@ -35,38 +36,6 @@
} \
} while (0)
-/* PnvXferBuffer */
-typedef struct PnvXferBuffer {
-
- uint32_t len;
- uint8_t *data;
-
-} PnvXferBuffer;
-
-/* pnv_spi_xfer_buffer_methods */
-static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
-{
- PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
-
- return payload;
-}
-
-static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
-{
- free(payload->data);
- free(payload);
-}
-
-static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
- uint32_t offset, uint32_t length)
-{
- if (payload->len < (offset + length)) {
- payload->len = offset + length;
- payload->data = g_realloc(payload->data, payload->len);
- }
- return &payload->data[offset];
-}
-
static bool does_rdr_match(PnvSpi *s)
{
/*
@@ -107,8 +76,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
return byte;
}
-static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
- uint8_t ecc_count, uint8_t shift_in_count)
+static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
+ uint8_t shift_in_count)
{
uint8_t byte;
int count = 0;
@@ -118,8 +87,8 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
if ((ecc_count != 0) &&
(shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
shift_in_count = 0;
- } else {
- byte = read_buf[count];
+ } else if (!fifo8_is_empty(&s->rx_fifo)) {
+ byte = fifo8_pop(&s->rx_fifo);
trace_pnv_spi_shift_rx(byte, count);
s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
}
@@ -128,7 +97,7 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
return shift_in_count;
}
-static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
+static void spi_response(PnvSpi *s)
{
uint8_t ecc_count;
uint8_t shift_in_count;
@@ -144,13 +113,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
* First check that the response payload is the exact same
* number of bytes as the request payload was
*/
- if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
+ if ((&s->rx_fifo)->num != (s->N1_bytes + s->N2_bytes)) {
qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
"bytes, expected %d, got %d\n",
- (s->N1_bytes + s->N2_bytes), rsp_payload->len);
+ (s->N1_bytes + s->N2_bytes), (&s->rx_fifo)->num);
} else {
uint8_t ecc_control;
- trace_pnv_spi_rx_received(rsp_payload->len);
+ trace_pnv_spi_rx_received((&s->rx_fifo)->num);
trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
/*
@@ -175,14 +144,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
/* Handle the N1 portion of the frame first */
if (s->N1_rx != 0) {
trace_pnv_spi_rx_read_N1frame();
- shift_in_count = read_from_frame(s, &rsp_payload->data[0],
- s->N1_bytes, ecc_count, shift_in_count);
+ shift_in_count = read_from_frame(s, s->N1_bytes,
+ ecc_count, shift_in_count);
}
/* Handle the N2 portion of the frame */
if (s->N2_rx != 0) {
trace_pnv_spi_rx_read_N2frame();
- shift_in_count = read_from_frame(s,
- &rsp_payload->data[s->N1_bytes], s->N2_bytes,
+ shift_in_count = read_from_frame(s, s->N2_bytes,
ecc_count, shift_in_count);
}
if ((s->N1_rx + s->N2_rx) > 0) {
@@ -210,34 +178,36 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
} /* end of else */
} /* end of spi_response() */
-static void transfer(PnvSpi *s, PnvXferBuffer *payload)
+static void transfer(PnvSpi *s)
{
- uint32_t tx;
- uint32_t rx;
- PnvXferBuffer *rsp_payload = NULL;
+ uint32_t tx, rx, payload_len;
+ uint8_t rx_byte;
- rsp_payload = pnv_spi_xfer_buffer_new();
- for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
+ payload_len = (&s->tx_fifo)->num;
+ for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
tx = 0;
for (int i = 0; i < s->transfer_len; i++) {
- if ((offset + i) >= payload->len) {
+ if ((offset + i) >= payload_len) {
tx <<= 8;
- } else {
- tx = (tx << 8) | payload->data[offset + i];
+ } else if (!fifo8_is_empty(&s->tx_fifo)) {
+ tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
}
}
rx = ssi_transfer(s->ssi_bus, tx);
for (int i = 0; i < s->transfer_len; i++) {
- if ((offset + i) >= payload->len) {
+ if ((offset + i) >= payload_len) {
break;
}
- *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
- (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
+ rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
+ if (!fifo8_is_full(&s->rx_fifo)) {
+ fifo8_push(&s->rx_fifo, rx_byte);
+ }
}
}
- if (rsp_payload != NULL) {
- spi_response(s, s->N1_bits, rsp_payload);
- }
+ spi_response(s);
+ /* Reset fifo for next frame */
+ fifo8_reset(&s->tx_fifo);
+ fifo8_reset(&s->rx_fifo);
}
static inline uint8_t get_seq_index(PnvSpi *s)
@@ -348,19 +318,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
/*
* Shift_N1 operation handler method
*/
-static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
- PnvXferBuffer **payload, bool send_n1_alone)
+static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
{
uint8_t n1_count;
bool stop = false;
-
- /*
- * If there isn't a current payload left over from a stopped sequence
- * create a new one.
- */
- if (*payload == NULL) {
- *payload = pnv_spi_xfer_buffer_new();
- }
/*
* Use a combination of N1 counters to build the N1 portion of the
* transmit payload.
@@ -411,9 +372,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
*/
uint8_t n1_byte = 0x00;
n1_byte = get_from_offset(s, n1_count);
- trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
- *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
- n1_byte;
+ if (!fifo8_is_full(&s->tx_fifo)) {
+ trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
+ fifo8_push(&s->tx_fifo, n1_byte);
+ }
} else {
/*
* We hit a shift_n1 opcode TX but the TDR is empty, tell the
@@ -440,10 +402,9 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
"set for receive but RDR is full");
stop = true;
break;
- } else {
+ } else if (!fifo8_is_full(&s->tx_fifo)) {
trace_pnv_spi_tx_append_FF("n1_byte");
- *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
- = 0xff;
+ fifo8_push(&s->tx_fifo, 0xff);
}
}
n1_count++;
@@ -484,15 +445,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
*/
if (send_n1_alone && !stop) {
/* We have a TX and a full TDR or an RX and an empty RDR */
- trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
- transfer(s, *payload);
+ trace_pnv_spi_tx_request("Shifting N1 frame", (&s->tx_fifo)->num);
+ transfer(s);
/* The N1 frame shift is complete so reset the N1 counters */
s->N2_bits = 0;
s->N2_bytes = 0;
s->N2_tx = 0;
s->N2_rx = 0;
- pnv_spi_xfer_buffer_free(*payload);
- *payload = NULL;
}
return stop;
} /* end of operation_shiftn1() */
@@ -588,19 +547,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
* Shift_N2 operation handler method
*/
-static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
- PnvXferBuffer **payload)
+static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
{
uint8_t n2_count;
bool stop = false;
-
- /*
- * If there isn't a current payload left over from a stopped sequence
- * create a new one.
- */
- if (*payload == NULL) {
- *payload = pnv_spi_xfer_buffer_new();
- }
/*
* Use a combination of N2 counters to build the N2 portion of the
* transmit payload.
@@ -639,25 +589,26 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
/* Always append data for the N2 segment if it is set for TX */
uint8_t n2_byte = 0x00;
n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
- trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
- *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
- = n2_byte;
- } else {
+ if (!fifo8_is_full(&s->tx_fifo)) {
+ trace_pnv_spi_tx_append("n2_byte", n2_byte,
+ (s->N1_tx + n2_count));
+ fifo8_push(&s->tx_fifo, n2_byte);
+ }
+ } else if (!fifo8_is_full(&s->tx_fifo)) {
/*
* Regardless of whether or not N2 is set for TX or RX, we need
* the number of bytes in the payload to match the overall length
* of the operation.
*/
trace_pnv_spi_tx_append_FF("n2_byte");
- *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
- = 0xff;
+ fifo8_push(&s->tx_fifo, 0xff);
}
n2_count++;
} /* end of while */
if (!stop) {
/* We have a TX and a full TDR or an RX and an empty RDR */
- trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
- transfer(s, *payload);
+ trace_pnv_spi_tx_request("Shifting N2 frame", (&s->tx_fifo)->num);
+ transfer(s);
/*
* If we are doing an N2 TX and the TDR is full we need to clear the
* TDR_full status. Do this here instead of up in the loop above so we
@@ -680,8 +631,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
s->N1_bytes = 0;
s->N1_tx = 0;
s->N1_rx = 0;
- pnv_spi_xfer_buffer_free(*payload);
- *payload = NULL;
}
return stop;
} /* end of operation_shiftn2()*/
@@ -699,19 +648,6 @@ static void operation_sequencer(PnvSpi *s)
uint8_t opcode = 0;
uint8_t masked_opcode = 0;
- /*
- * PnvXferBuffer for containing the payload of the SPI frame.
- * This is a static because there are cases where a sequence has to stop
- * and wait for the target application to unload the RDR. If this occurs
- * during a sequence where N1 is not sent alone and instead combined with
- * N2 since the N1 tx length + the N2 tx length is less than the size of
- * the TDR.
- */
- static PnvXferBuffer *payload;
-
- if (payload == NULL) {
- payload = pnv_spi_xfer_buffer_new();
- }
/*
* Clear the sequencer FSM error bit - general_SPI_status[3]
* before starting a sequence.
@@ -840,7 +776,7 @@ static void operation_sequencer(PnvSpi *s)
}
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
FSM_SHIFT_N1);
- stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
+ stop = operation_shiftn1(s, opcode, send_n1_alone);
if (stop) {
/*
* The operation code says to stop, this can occur if:
@@ -895,7 +831,7 @@ static void operation_sequencer(PnvSpi *s)
/* Ok to do a Shift_N2 */
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
FSM_SHIFT_N2);
- stop = operation_shiftn2(s, opcode, &payload);
+ stop = operation_shiftn2(s, opcode);
/*
* If the operation code says to stop set the shifter state to
* wait and stop
@@ -1208,6 +1144,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
s->cs_line = g_new0(qemu_irq, 1);
qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
+ fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
+ fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
+
/* spi scoms */
pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
2024-09-18 16:50 [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Chalapathi V
2024-09-18 16:50 ` [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section Chalapathi V
2024-09-18 16:50 ` [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
@ 2024-09-18 16:50 ` Chalapathi V
2024-10-08 8:13 ` Nicholas Piggin
2024-10-08 7:43 ` [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Nicholas Piggin
3 siblings, 1 reply; 13+ messages in thread
From: Chalapathi V @ 2024-09-18 16:50 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 caling
get_seq_index() method.
Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 2fd5aa0a96..962115f40f 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
fifo8_reset(&s->rx_fifo);
}
-static inline uint8_t get_seq_index(PnvSpi *s)
-{
- return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
-}
-
static inline void next_sequencer_fsm(PnvSpi *s)
{
- uint8_t seq_index = get_seq_index(s);
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
}
@@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
bool stop = false; /* Flag to stop the sequencer */
uint8_t opcode = 0;
uint8_t masked_opcode = 0;
+ uint8_t seq_index;
/*
* Clear the sequencer FSM error bit - general_SPI_status[3]
@@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
}
+ seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
/*
* There are only 8 possible operation IDs to iterate through though
* some operations may cause more than one frame to be sequenced.
*/
- while (get_seq_index(s) < NUM_SEQ_OPS) {
- opcode = s->seq_op[get_seq_index(s)];
+ while (seq_index < NUM_SEQ_OPS) {
+ opcode = s->seq_op[seq_index];
/* Set sequencer state to decode */
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
/*
@@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_STOP:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
/* A stop operation in any position stops the sequencer */
- trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("STOP", seq_index);
stop = true;
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
@@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_SELECT_SLAVE:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
/*
* This device currently only supports a single responder
* connection at position 0. De-selecting a responder is fine
@@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
if (s->responder_select == 0) {
trace_pnv_spi_shifter_done();
qemu_set_irq(s->cs_line[0], 1);
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- (get_seq_index(s) + 1));
+ seq_index++;
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
} else if (s->responder_select != 1) {
qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
@@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
* applies once a valid responder select has occurred.
*/
s->shift_n1_done = false;
+ seq_index++;
next_sequencer_fsm(s);
}
break;
case SEQ_OP_SHIFT_N1:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
/*
* Only allow a shift_n1 when the state is not IDLE or DONE.
* In either of those two cases the sequencer is not in a proper
@@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
* transmission to the responder without requiring a refill of
* the TDR between the two operations.
*/
- if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
- == SEQ_OP_SHIFT_N2) {
+ if ((seq_index != 7) &&
+ PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
+ SEQ_OP_SHIFT_N2) {
send_n1_alone = false;
}
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
@@ -793,8 +789,7 @@ static void operation_sequencer(PnvSpi *s)
s->shift_n1_done = true;
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
FSM_SHIFT_N2);
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- (get_seq_index(s) + 1));
+ seq_index++;
} else {
/*
* This is case (1) or (2) so the sequencer needs to
@@ -806,6 +801,7 @@ static void operation_sequencer(PnvSpi *s)
} else {
/* Ok to move on to the next index */
s->shift_n1_done = true;
+ seq_index++;
next_sequencer_fsm(s);
}
}
@@ -813,7 +809,7 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_SHIFT_N2:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("SHIFT_N2", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("SHIFT_N2", seq_index);
if (!s->shift_n1_done) {
qemu_log_mask(LOG_GUEST_ERROR, "Shift_N2 is not allowed if a "
"Shift_N1 is not done, shifter state = 0x%llx",
@@ -841,6 +837,7 @@ static void operation_sequencer(PnvSpi *s)
FSM_WAIT);
} else {
/* Ok to move on to the next index */
+ seq_index++;
next_sequencer_fsm(s);
}
}
@@ -848,7 +845,7 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_BRANCH_IFNEQ_RDR:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_RDR", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_RDR", seq_index);
/*
* The memory mapping register RDR match value is compared against
* the 16 rightmost bytes of the RDR (potentially with masking).
@@ -864,6 +861,7 @@ static void operation_sequencer(PnvSpi *s)
if (rdr_matched) {
trace_pnv_spi_RDR_match("success");
/* A match occurred, increment the sequencer index. */
+ seq_index++;
next_sequencer_fsm(s);
} else {
trace_pnv_spi_RDR_match("failed");
@@ -871,8 +869,7 @@ static void operation_sequencer(PnvSpi *s)
* Branch the sequencer to the index coded into the op
* code.
*/
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+ seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
}
/*
* Regardless of where the branch ended up we want the
@@ -891,12 +888,13 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_TRANSFER_TDR:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
qemu_log_mask(LOG_GUEST_ERROR, "Transfer TDR is not supported\n");
+ seq_index++;
next_sequencer_fsm(s);
break;
case SEQ_OP_BRANCH_IFNEQ_INC_1:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_1", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_1", seq_index);
/*
* The spec says the loop should execute count compare + 1 times.
* However we learned from engineering that we really only loop
@@ -910,18 +908,18 @@ static void operation_sequencer(PnvSpi *s)
* mask off all but the first three bits so we don't try to
* access beyond the sequencer_operation_reg boundary.
*/
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+ seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
s->loop_counter_1++;
} else {
/* Continue to next index if loop counter is reached */
+ seq_index++;
next_sequencer_fsm(s);
}
break;
case SEQ_OP_BRANCH_IFNEQ_INC_2:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", seq_index);
uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
s->regs[SPI_CTR_CFG_REG]);
/*
@@ -936,11 +934,11 @@ static void operation_sequencer(PnvSpi *s)
* mask off all but the first three bits so we don't try to
* access beyond the sequencer_operation_reg boundary.
*/
- s->status = SETFIELD(SPI_STS_SEQ_INDEX,
- s->status, PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+ seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
s->loop_counter_2++;
} else {
/* Continue to next index if loop counter is reached */
+ seq_index++;
next_sequencer_fsm(s);
}
break;
@@ -948,6 +946,7 @@ static void operation_sequencer(PnvSpi *s)
default:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
/* Ignore unsupported operations. */
+ seq_index++;
next_sequencer_fsm(s);
break;
} /* end of switch */
@@ -956,10 +955,10 @@ static void operation_sequencer(PnvSpi *s)
* we need to go ahead and end things as if there was a STOP at the
* end.
*/
- if (get_seq_index(s) == NUM_SEQ_OPS) {
+ if (seq_index == NUM_SEQ_OPS) {
/* All 8 opcodes completed, sequencer idling */
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
+ seq_index = 0;
s->loop_counter_1 = 0;
s->loop_counter_2 = 0;
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_IDLE);
@@ -970,6 +969,8 @@ static void operation_sequencer(PnvSpi *s)
break;
}
} /* end of while */
+ /* Update sequencer index field in status.*/
+ s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, seq_index);
return;
} /* end of operation_sequencer() */
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()
2024-09-18 16:50 [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Chalapathi V
` (2 preceding siblings ...)
2024-09-18 16:50 ` [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index() Chalapathi V
@ 2024-10-08 7:43 ` Nicholas Piggin
2024-10-08 12:44 ` Cédric Le Goater
3 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2024-10-08 7:43 UTC (permalink / raw)
To: Chalapathi V, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> Hello,
>
> v3:
> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
> 2. Use of PnvXferBuffer results in a additonal process overhead due to
> frequent dynamic allocations and hence use an existing Fifo8 buffer.
> 3. Use a local variable seq_index and use it with in while loop instead
> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
> overrun when seq_index is incremented.
>
> Tested:
> passed make check and make check-avocado
>
> Supersedes: <20240807202804.56038-1-philmd@linaro.org>
Hi Chalapathi,
To be clear, this fixes Coverity CID 1558831? A Resolves:
tag for the CID should be there, I guess it's patch 2?
I like patch 2, but since it is quite a significant change,
should we take the v2 series first which is much smaller,
then add this conversion on top of it?
If it was long-standing code that would be important (because
you don't want to introduce regressions or conflicts when
backporting fixes). Since this is a new model I guess there
is leeway to just take v3 as is.
Thanks,
Nick
>
> Philippe Mathieu-Daudé (1):
> MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>
> Chalapathi V (2):
> hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
> hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
>
> MAINTAINERS | 2 +
> include/hw/ssi/pnv_spi.h | 3 +
> hw/ssi/pnv_spi.c | 228 +++++++++++++++------------------------
> 3 files changed, 89 insertions(+), 144 deletions(-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section
2024-09-18 16:50 ` [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section Chalapathi V
@ 2024-10-08 7:44 ` Nicholas Piggin
2024-10-08 17:32 ` Chalapathi V
0 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2024-10-08 7:44 UTC (permalink / raw)
To: Chalapathi V, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>
> It is unfair to let the PowerNV SPI model to the SSI
> maintainers. Also include the PowerNV ones.
>
> Fixes: 29318db133 ("hw/ssi: Add SPI model")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
> MAINTAINERS | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ffacd60f40..b11c4edaf0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1541,8 +1541,10 @@ F: hw/ppc/pnv*
> F: hw/intc/pnv*
> F: hw/intc/xics_pnv.c
> F: hw/pci-host/pnv*
> +F: hw/ssi/pnv_spi.c
> F: include/hw/ppc/pnv*
> F: include/hw/pci-host/pnv*
> +F: hw/ssi/pnv_spi*
Should be include/hw/ssi/pnv_spi*
Otherwise,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> F: pc-bios/skiboot.lid
> F: tests/qtest/pnv*
> F: tests/functional/test_ppc64_powernv.py
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
2024-09-18 16:50 ` [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
@ 2024-10-08 7:59 ` Nicholas Piggin
2024-10-08 17:18 ` Chalapathi V
0 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2024-10-08 7:59 UTC (permalink / raw)
To: Chalapathi V, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> In PnvXferBuffer dynamically allocating and freeing is a
> process overhead. Hence used an existing Fifo8 buffer with
> capacity of 16 bytes.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
> include/hw/ssi/pnv_spi.h | 3 +
> hw/ssi/pnv_spi.c | 167 +++++++++++++--------------------------
> 2 files changed, 56 insertions(+), 114 deletions(-)
>
> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
> index 8815f67d45..9878d9a25f 100644
> --- a/include/hw/ssi/pnv_spi.h
> +++ b/include/hw/ssi/pnv_spi.h
> @@ -23,6 +23,7 @@
>
> #include "hw/ssi/ssi.h"
> #include "hw/sysbus.h"
> +#include "qemu/fifo8.h"
>
> #define TYPE_PNV_SPI "pnv-spi"
> OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
> @@ -37,6 +38,8 @@ typedef struct PnvSpi {
> SSIBus *ssi_bus;
> qemu_irq *cs_line;
> MemoryRegion xscom_spic_regs;
> + Fifo8 tx_fifo;
> + Fifo8 rx_fifo;
> /* SPI object number */
> uint32_t spic_num;
> uint8_t transfer_len;
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 9e7207bf7c..2fd5aa0a96 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -19,6 +19,7 @@
>
> #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
> #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
> +#define PNV_SPI_FIFO_SIZE 16
>
> /*
> * Macro from include/hw/ppc/fdt.h
> @@ -35,38 +36,6 @@
> } \
> } while (0)
>
> -/* PnvXferBuffer */
> -typedef struct PnvXferBuffer {
> -
> - uint32_t len;
> - uint8_t *data;
> -
> -} PnvXferBuffer;
> -
> -/* pnv_spi_xfer_buffer_methods */
> -static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
> -{
> - PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
> -
> - return payload;
> -}
> -
> -static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
> -{
> - free(payload->data);
> - free(payload);
> -}
> -
> -static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
> - uint32_t offset, uint32_t length)
> -{
> - if (payload->len < (offset + length)) {
> - payload->len = offset + length;
> - payload->data = g_realloc(payload->data, payload->len);
> - }
> - return &payload->data[offset];
> -}
> -
> static bool does_rdr_match(PnvSpi *s)
> {
> /*
> @@ -107,8 +76,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
> return byte;
> }
>
> -static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
> - uint8_t ecc_count, uint8_t shift_in_count)
> +static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
> + uint8_t shift_in_count)
> {
> uint8_t byte;
> int count = 0;
> @@ -118,8 +87,8 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
> if ((ecc_count != 0) &&
> (shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
> shift_in_count = 0;
> - } else {
> - byte = read_buf[count];
> + } else if (!fifo8_is_empty(&s->rx_fifo)) {
> + byte = fifo8_pop(&s->rx_fifo);
> trace_pnv_spi_shift_rx(byte, count);
> s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
> }
What happens to the else case here? Did the previous code underflow
read_buf, or is it a programming error?
> @@ -128,7 +97,7 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
> return shift_in_count;
> }
>
> -static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
> +static void spi_response(PnvSpi *s)
> {
> uint8_t ecc_count;
> uint8_t shift_in_count;
> @@ -144,13 +113,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
> * First check that the response payload is the exact same
> * number of bytes as the request payload was
> */
> - if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
> + if ((&s->rx_fifo)->num != (s->N1_bytes + s->N2_bytes)) {
fifo8_num_used()
> qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
> "bytes, expected %d, got %d\n",
> - (s->N1_bytes + s->N2_bytes), rsp_payload->len);
> + (s->N1_bytes + s->N2_bytes), (&s->rx_fifo)->num);
> } else {
> uint8_t ecc_control;
> - trace_pnv_spi_rx_received(rsp_payload->len);
> + trace_pnv_spi_rx_received((&s->rx_fifo)->num);
fifo8_num_used()
> trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
> s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
> /*
> @@ -175,14 +144,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
> /* Handle the N1 portion of the frame first */
> if (s->N1_rx != 0) {
> trace_pnv_spi_rx_read_N1frame();
> - shift_in_count = read_from_frame(s, &rsp_payload->data[0],
> - s->N1_bytes, ecc_count, shift_in_count);
> + shift_in_count = read_from_frame(s, s->N1_bytes,
> + ecc_count, shift_in_count);
Maybe indent this ^ line up to the ( of the function call operator
if possible (not sure it's a hard rule but it reads a bit better
unless you have really run out of columns)..
> }
> /* Handle the N2 portion of the frame */
> if (s->N2_rx != 0) {
> trace_pnv_spi_rx_read_N2frame();
> - shift_in_count = read_from_frame(s,
> - &rsp_payload->data[s->N1_bytes], s->N2_bytes,
> + shift_in_count = read_from_frame(s, s->N2_bytes,
> ecc_count, shift_in_count);
Same here, and for other code where you touch nearby lines.
> }
> if ((s->N1_rx + s->N2_rx) > 0) {
> @@ -210,34 +178,36 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
> } /* end of else */
> } /* end of spi_response() */
>
> -static void transfer(PnvSpi *s, PnvXferBuffer *payload)
> +static void transfer(PnvSpi *s)
> {
> - uint32_t tx;
> - uint32_t rx;
> - PnvXferBuffer *rsp_payload = NULL;
> + uint32_t tx, rx, payload_len;
> + uint8_t rx_byte;
>
> - rsp_payload = pnv_spi_xfer_buffer_new();
> - for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
> + payload_len = (&s->tx_fifo)->num;
fifo8_num_used()
(And several other cases below should use num_used).
> + for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
> tx = 0;
> for (int i = 0; i < s->transfer_len; i++) {
> - if ((offset + i) >= payload->len) {
> + if ((offset + i) >= payload_len) {
> tx <<= 8;
> - } else {
> - tx = (tx << 8) | payload->data[offset + i];
> + } else if (!fifo8_is_empty(&s->tx_fifo)) {
> + tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
> }
Similar question about underflow here. Is there an assert or error
message or trace event for all these over/under flow cases?
> }
> rx = ssi_transfer(s->ssi_bus, tx);
> for (int i = 0; i < s->transfer_len; i++) {
> - if ((offset + i) >= payload->len) {
> + if ((offset + i) >= payload_len) {
> break;
> }
> - *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
> - (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
> + rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
> + if (!fifo8_is_full(&s->rx_fifo)) {
> + fifo8_push(&s->rx_fifo, rx_byte);
> + }
> }
And overflow, this just gets lost? You can just put || fifo8_is_full()
in the break condition I think?.
> }
> - if (rsp_payload != NULL) {
> - spi_response(s, s->N1_bits, rsp_payload);
> - }
> + spi_response(s);
> + /* Reset fifo for next frame */
> + fifo8_reset(&s->tx_fifo);
> + fifo8_reset(&s->rx_fifo);
> }
>
> static inline uint8_t get_seq_index(PnvSpi *s)
> @@ -348,19 +318,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
> /*
> * Shift_N1 operation handler method
> */
> -static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
> - PnvXferBuffer **payload, bool send_n1_alone)
> +static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
> {
> uint8_t n1_count;
> bool stop = false;
> -
> - /*
> - * If there isn't a current payload left over from a stopped sequence
> - * create a new one.
> - */
> - if (*payload == NULL) {
> - *payload = pnv_spi_xfer_buffer_new();
> - }
> /*
> * Use a combination of N1 counters to build the N1 portion of the
> * transmit payload.
> @@ -411,9 +372,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
> */
> uint8_t n1_byte = 0x00;
> n1_byte = get_from_offset(s, n1_count);
> - trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
> - n1_byte;
> + if (!fifo8_is_full(&s->tx_fifo)) {
> + trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
> + fifo8_push(&s->tx_fifo, n1_byte);
> + }
> } else {
> /*
> * We hit a shift_n1 opcode TX but the TDR is empty, tell the
> @@ -440,10 +402,9 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
> "set for receive but RDR is full");
> stop = true;
> break;
> - } else {
> + } else if (!fifo8_is_full(&s->tx_fifo)) {
> trace_pnv_spi_tx_append_FF("n1_byte");
> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
> - = 0xff;
> + fifo8_push(&s->tx_fifo, 0xff);
> }
> }
> n1_count++;
> @@ -484,15 +445,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
> */
> if (send_n1_alone && !stop) {
> /* We have a TX and a full TDR or an RX and an empty RDR */
> - trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
> - transfer(s, *payload);
> + trace_pnv_spi_tx_request("Shifting N1 frame", (&s->tx_fifo)->num);
> + transfer(s);
> /* The N1 frame shift is complete so reset the N1 counters */
> s->N2_bits = 0;
> s->N2_bytes = 0;
> s->N2_tx = 0;
> s->N2_rx = 0;
> - pnv_spi_xfer_buffer_free(*payload);
> - *payload = NULL;
> }
> return stop;
> } /* end of operation_shiftn1() */
> @@ -588,19 +547,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
> * Shift_N2 operation handler method
> */
>
> -static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
> - PnvXferBuffer **payload)
> +static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
> {
> uint8_t n2_count;
> bool stop = false;
> -
> - /*
> - * If there isn't a current payload left over from a stopped sequence
> - * create a new one.
> - */
> - if (*payload == NULL) {
> - *payload = pnv_spi_xfer_buffer_new();
> - }
> /*
> * Use a combination of N2 counters to build the N2 portion of the
> * transmit payload.
> @@ -639,25 +589,26 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
> /* Always append data for the N2 segment if it is set for TX */
> uint8_t n2_byte = 0x00;
> n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
> - trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
> - = n2_byte;
> - } else {
> + if (!fifo8_is_full(&s->tx_fifo)) {
> + trace_pnv_spi_tx_append("n2_byte", n2_byte,
> + (s->N1_tx + n2_count));
> + fifo8_push(&s->tx_fifo, n2_byte);
> + }
> + } else if (!fifo8_is_full(&s->tx_fifo)) {
> /*
> * Regardless of whether or not N2 is set for TX or RX, we need
> * the number of bytes in the payload to match the overall length
> * of the operation.
> */
> trace_pnv_spi_tx_append_FF("n2_byte");
> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
> - = 0xff;
> + fifo8_push(&s->tx_fifo, 0xff);
> }
> n2_count++;
> } /* end of while */
> if (!stop) {
> /* We have a TX and a full TDR or an RX and an empty RDR */
> - trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
> - transfer(s, *payload);
> + trace_pnv_spi_tx_request("Shifting N2 frame", (&s->tx_fifo)->num);
> + transfer(s);
> /*
> * If we are doing an N2 TX and the TDR is full we need to clear the
> * TDR_full status. Do this here instead of up in the loop above so we
> @@ -680,8 +631,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
> s->N1_bytes = 0;
> s->N1_tx = 0;
> s->N1_rx = 0;
> - pnv_spi_xfer_buffer_free(*payload);
> - *payload = NULL;
> }
> return stop;
> } /* end of operation_shiftn2()*/
> @@ -699,19 +648,6 @@ static void operation_sequencer(PnvSpi *s)
> uint8_t opcode = 0;
> uint8_t masked_opcode = 0;
>
> - /*
> - * PnvXferBuffer for containing the payload of the SPI frame.
> - * This is a static because there are cases where a sequence has to stop
> - * and wait for the target application to unload the RDR. If this occurs
> - * during a sequence where N1 is not sent alone and instead combined with
> - * N2 since the N1 tx length + the N2 tx length is less than the size of
> - * the TDR.
> - */
> - static PnvXferBuffer *payload;
> -
> - if (payload == NULL) {
> - payload = pnv_spi_xfer_buffer_new();
> - }
> /*
> * Clear the sequencer FSM error bit - general_SPI_status[3]
> * before starting a sequence.
> @@ -840,7 +776,7 @@ static void operation_sequencer(PnvSpi *s)
> }
> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> FSM_SHIFT_N1);
> - stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
> + stop = operation_shiftn1(s, opcode, send_n1_alone);
> if (stop) {
> /*
> * The operation code says to stop, this can occur if:
> @@ -895,7 +831,7 @@ static void operation_sequencer(PnvSpi *s)
> /* Ok to do a Shift_N2 */
> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> FSM_SHIFT_N2);
> - stop = operation_shiftn2(s, opcode, &payload);
> + stop = operation_shiftn2(s, opcode);
> /*
> * If the operation code says to stop set the shifter state to
> * wait and stop
> @@ -1208,6 +1144,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
> s->cs_line = g_new0(qemu_irq, 1);
> qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>
> + fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
> + fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
> +
> /* spi scoms */
> pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
> s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);
Looks nice, simplifies the code and tricky pointer and memory handling.
Thanks,
Nick
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
2024-09-18 16:50 ` [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index() Chalapathi V
@ 2024-10-08 8:13 ` Nicholas Piggin
2024-10-08 17:34 ` Chalapathi V
0 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2024-10-08 8:13 UTC (permalink / raw)
To: Chalapathi V, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
> Use a local variable seq_index instead of repeatedly caling
> get_seq_index() method.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
> hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
> 1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 2fd5aa0a96..962115f40f 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
> fifo8_reset(&s->rx_fifo);
> }
>
> -static inline uint8_t get_seq_index(PnvSpi *s)
> -{
> - return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
> -}
> -
> static inline void next_sequencer_fsm(PnvSpi *s)
> {
> - uint8_t seq_index = get_seq_index(s);
> - s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
> }
>
> @@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
> bool stop = false; /* Flag to stop the sequencer */
> uint8_t opcode = 0;
> uint8_t masked_opcode = 0;
> + uint8_t seq_index;
>
> /*
> * Clear the sequencer FSM error bit - general_SPI_status[3]
> @@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
> if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
> s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
> }
> + seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
> /*
> * There are only 8 possible operation IDs to iterate through though
> * some operations may cause more than one frame to be sequenced.
> */
> - while (get_seq_index(s) < NUM_SEQ_OPS) {
> - opcode = s->seq_op[get_seq_index(s)];
> + while (seq_index < NUM_SEQ_OPS) {
> + opcode = s->seq_op[seq_index];
> /* Set sequencer state to decode */
> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
> /*
> @@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
> case SEQ_OP_STOP:
> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
> /* A stop operation in any position stops the sequencer */
> - trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
> + trace_pnv_spi_sequencer_op("STOP", seq_index);
>
> stop = true;
> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
> @@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
>
> case SEQ_OP_SELECT_SLAVE:
> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
> - trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
> + trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
> /*
> * This device currently only supports a single responder
> * connection at position 0. De-selecting a responder is fine
> @@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
> if (s->responder_select == 0) {
> trace_pnv_spi_shifter_done();
> qemu_set_irq(s->cs_line[0], 1);
> - s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
> - (get_seq_index(s) + 1));
> + seq_index++;
> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
> } else if (s->responder_select != 1) {
> qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
> @@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
> * applies once a valid responder select has occurred.
> */
> s->shift_n1_done = false;
> + seq_index++;
> next_sequencer_fsm(s);
Maybe could just open-code next_sequencer_fsm() now, since a bunch of
other FSM fields seem to be open-coded?
> }
> break;
>
> case SEQ_OP_SHIFT_N1:
> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
> - trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
> + trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
> /*
> * Only allow a shift_n1 when the state is not IDLE or DONE.
> * In either of those two cases the sequencer is not in a proper
> @@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
> * transmission to the responder without requiring a refill of
> * the TDR between the two operations.
> */
> - if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
> - == SEQ_OP_SHIFT_N2) {
> + if ((seq_index != 7) &&
> + PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
> + SEQ_OP_SHIFT_N2) {
> send_n1_alone = false;
> }
> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
The seq_index != 7 is a new test? Is that a separate fix, I'm not
seeing how it's related to the seq_index change.
Thanks,
Nick
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()
2024-10-08 7:43 ` [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Nicholas Piggin
@ 2024-10-08 12:44 ` Cédric Le Goater
2024-10-08 17:40 ` Chalapathi V
0 siblings, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2024-10-08 12:44 UTC (permalink / raw)
To: Nicholas Piggin, Chalapathi V, qemu-devel
Cc: qemu-ppc, fbarrat, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On 10/8/24 09:43, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> Hello,
>>
>> v3:
>> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
>> 2. Use of PnvXferBuffer results in a additonal process overhead due to
>> frequent dynamic allocations and hence use an existing Fifo8 buffer.
>> 3. Use a local variable seq_index and use it with in while loop instead
>> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
>> overrun when seq_index is incremented.
>>
>> Tested:
>> passed make check and make check-avocado
>>
>> Supersedes: <20240807202804.56038-1-philmd@linaro.org>
>
> Hi Chalapathi,
>
> To be clear, this fixes Coverity CID 1558831? A Resolves:
> tag for the CID should be there, I guess it's patch 2?
Patches should have a tag :
Resolves: Coverity CID XYZZY
Thanks,
C.
>
> I like patch 2, but since it is quite a significant change,
> should we take the v2 series first which is much smaller,
> then add this conversion on top of it?
>
> If it was long-standing code that would be important (because
> you don't want to introduce regressions or conflicts when
> backporting fixes). Since this is a new model I guess there
> is leeway to just take v3 as is.
>
> Thanks,
> Nick
>
>>
>> Philippe Mathieu-Daudé (1):
>> MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>>
>> Chalapathi V (2):
>> hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
>> hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
>>
>> MAINTAINERS | 2 +
>> include/hw/ssi/pnv_spi.h | 3 +
>> hw/ssi/pnv_spi.c | 228 +++++++++++++++------------------------
>> 3 files changed, 89 insertions(+), 144 deletions(-)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
2024-10-08 7:59 ` Nicholas Piggin
@ 2024-10-08 17:18 ` Chalapathi V
0 siblings, 0 replies; 13+ messages in thread
From: Chalapathi V @ 2024-10-08 17:18 UTC (permalink / raw)
To: Nicholas Piggin, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On 08-10-2024 13:29, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> In PnvXferBuffer dynamically allocating and freeing is a
>> process overhead. Hence used an existing Fifo8 buffer with
>> capacity of 16 bytes.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>> include/hw/ssi/pnv_spi.h | 3 +
>> hw/ssi/pnv_spi.c | 167 +++++++++++++--------------------------
>> 2 files changed, 56 insertions(+), 114 deletions(-)
>>
>> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
>> index 8815f67d45..9878d9a25f 100644
>> --- a/include/hw/ssi/pnv_spi.h
>> +++ b/include/hw/ssi/pnv_spi.h
>> @@ -23,6 +23,7 @@
>>
>> #include "hw/ssi/ssi.h"
>> #include "hw/sysbus.h"
>> +#include "qemu/fifo8.h"
>>
>> #define TYPE_PNV_SPI "pnv-spi"
>> OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>> @@ -37,6 +38,8 @@ typedef struct PnvSpi {
>> SSIBus *ssi_bus;
>> qemu_irq *cs_line;
>> MemoryRegion xscom_spic_regs;
>> + Fifo8 tx_fifo;
>> + Fifo8 rx_fifo;
>> /* SPI object number */
>> uint32_t spic_num;
>> uint8_t transfer_len;
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 9e7207bf7c..2fd5aa0a96 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -19,6 +19,7 @@
>>
>> #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>> #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>> +#define PNV_SPI_FIFO_SIZE 16
>>
>> /*
>> * Macro from include/hw/ppc/fdt.h
>> @@ -35,38 +36,6 @@
>> } \
>> } while (0)
>>
>> -/* PnvXferBuffer */
>> -typedef struct PnvXferBuffer {
>> -
>> - uint32_t len;
>> - uint8_t *data;
>> -
>> -} PnvXferBuffer;
>> -
>> -/* pnv_spi_xfer_buffer_methods */
>> -static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
>> -{
>> - PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
>> -
>> - return payload;
>> -}
>> -
>> -static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
>> -{
>> - free(payload->data);
>> - free(payload);
>> -}
>> -
>> -static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
>> - uint32_t offset, uint32_t length)
>> -{
>> - if (payload->len < (offset + length)) {
>> - payload->len = offset + length;
>> - payload->data = g_realloc(payload->data, payload->len);
>> - }
>> - return &payload->data[offset];
>> -}
>> -
>> static bool does_rdr_match(PnvSpi *s)
>> {
>> /*
>> @@ -107,8 +76,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
>> return byte;
>> }
>>
>> -static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>> - uint8_t ecc_count, uint8_t shift_in_count)
>> +static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
>> + uint8_t shift_in_count)
>> {
>> uint8_t byte;
>> int count = 0;
>> @@ -118,8 +87,8 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>> if ((ecc_count != 0) &&
>> (shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
>> shift_in_count = 0;
>> - } else {
>> - byte = read_buf[count];
>> + } else if (!fifo8_is_empty(&s->rx_fifo)) {
>> + byte = fifo8_pop(&s->rx_fifo);
>> trace_pnv_spi_shift_rx(byte, count);
>> s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
>> }
> What happens to the else case here? Did the previous code underflow
> read_buf, or is it a programming error?
read_buf should not underflow, I will add else with trace.
Thank You
>
>> @@ -128,7 +97,7 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>> return shift_in_count;
>> }
>>
>> -static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> +static void spi_response(PnvSpi *s)
>> {
>> uint8_t ecc_count;
>> uint8_t shift_in_count;
>> @@ -144,13 +113,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> * First check that the response payload is the exact same
>> * number of bytes as the request payload was
>> */
>> - if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
>> + if ((&s->rx_fifo)->num != (s->N1_bytes + s->N2_bytes)) {
> fifo8_num_used()
Sure. Thank You
>
>> qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
>> "bytes, expected %d, got %d\n",
>> - (s->N1_bytes + s->N2_bytes), rsp_payload->len);
>> + (s->N1_bytes + s->N2_bytes), (&s->rx_fifo)->num);
>> } else {
>> uint8_t ecc_control;
>> - trace_pnv_spi_rx_received(rsp_payload->len);
>> + trace_pnv_spi_rx_received((&s->rx_fifo)->num);
> fifo8_num_used()
>
>> trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
>> s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
>> /*
>> @@ -175,14 +144,13 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> /* Handle the N1 portion of the frame first */
>> if (s->N1_rx != 0) {
>> trace_pnv_spi_rx_read_N1frame();
>> - shift_in_count = read_from_frame(s, &rsp_payload->data[0],
>> - s->N1_bytes, ecc_count, shift_in_count);
>> + shift_in_count = read_from_frame(s, s->N1_bytes,
>> + ecc_count, shift_in_count);
> Maybe indent this ^ line up to the ( of the function call operator
> if possible (not sure it's a hard rule but it reads a bit better
> unless you have really run out of columns)..
This exceeds 80 characters limit, I will shorten the variable name and
check.
>
>> }
>> /* Handle the N2 portion of the frame */
>> if (s->N2_rx != 0) {
>> trace_pnv_spi_rx_read_N2frame();
>> - shift_in_count = read_from_frame(s,
>> - &rsp_payload->data[s->N1_bytes], s->N2_bytes,
>> + shift_in_count = read_from_frame(s, s->N2_bytes,
>> ecc_count, shift_in_count);
> Same here, and for other code where you touch nearby lines.
>
>> }
>> if ((s->N1_rx + s->N2_rx) > 0) {
>> @@ -210,34 +178,36 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> } /* end of else */
>> } /* end of spi_response() */
>>
>> -static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>> +static void transfer(PnvSpi *s)
>> {
>> - uint32_t tx;
>> - uint32_t rx;
>> - PnvXferBuffer *rsp_payload = NULL;
>> + uint32_t tx, rx, payload_len;
>> + uint8_t rx_byte;
>>
>> - rsp_payload = pnv_spi_xfer_buffer_new();
>> - for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
>> + payload_len = (&s->tx_fifo)->num;
> fifo8_num_used()
>
> (And several other cases below should use num_used).
Sure. Will update. Thank You
>
>> + for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
>> tx = 0;
>> for (int i = 0; i < s->transfer_len; i++) {
>> - if ((offset + i) >= payload->len) {
>> + if ((offset + i) >= payload_len) {
>> tx <<= 8;
>> - } else {
>> - tx = (tx << 8) | payload->data[offset + i];
>> + } else if (!fifo8_is_empty(&s->tx_fifo)) {
>> + tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
>> }
> Similar question about underflow here. Is there an assert or error
> message or trace event for all these over/under flow cases?
Will add the trace in else case.
>
>> }
>> rx = ssi_transfer(s->ssi_bus, tx);
>> for (int i = 0; i < s->transfer_len; i++) {
>> - if ((offset + i) >= payload->len) {
>> + if ((offset + i) >= payload_len) {
>> break;
>> }
>> - *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
>> - (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>> + rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>> + if (!fifo8_is_full(&s->rx_fifo)) {
>> + fifo8_push(&s->rx_fifo, rx_byte);
>> + }
>> }
> And overflow, this just gets lost? You can just put || fifo8_is_full()
> in the break condition I think?.
Sure.
>
>> }
>> - if (rsp_payload != NULL) {
>> - spi_response(s, s->N1_bits, rsp_payload);
>> - }
>> + spi_response(s);
>> + /* Reset fifo for next frame */
>> + fifo8_reset(&s->tx_fifo);
>> + fifo8_reset(&s->rx_fifo);
>> }
>>
>> static inline uint8_t get_seq_index(PnvSpi *s)
>> @@ -348,19 +318,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>> /*
>> * Shift_N1 operation handler method
>> */
>> -static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> - PnvXferBuffer **payload, bool send_n1_alone)
>> +static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
>> {
>> uint8_t n1_count;
>> bool stop = false;
>> -
>> - /*
>> - * If there isn't a current payload left over from a stopped sequence
>> - * create a new one.
>> - */
>> - if (*payload == NULL) {
>> - *payload = pnv_spi_xfer_buffer_new();
>> - }
>> /*
>> * Use a combination of N1 counters to build the N1 portion of the
>> * transmit payload.
>> @@ -411,9 +372,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> */
>> uint8_t n1_byte = 0x00;
>> n1_byte = get_from_offset(s, n1_count);
>> - trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
>> - n1_byte;
>> + if (!fifo8_is_full(&s->tx_fifo)) {
>> + trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
>> + fifo8_push(&s->tx_fifo, n1_byte);
>> + }
>> } else {
>> /*
>> * We hit a shift_n1 opcode TX but the TDR is empty, tell the
>> @@ -440,10 +402,9 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> "set for receive but RDR is full");
>> stop = true;
>> break;
>> - } else {
>> + } else if (!fifo8_is_full(&s->tx_fifo)) {
>> trace_pnv_spi_tx_append_FF("n1_byte");
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> - = 0xff;
>> + fifo8_push(&s->tx_fifo, 0xff);
>> }
>> }
>> n1_count++;
>> @@ -484,15 +445,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> */
>> if (send_n1_alone && !stop) {
>> /* We have a TX and a full TDR or an RX and an empty RDR */
>> - trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
>> - transfer(s, *payload);
>> + trace_pnv_spi_tx_request("Shifting N1 frame", (&s->tx_fifo)->num);
>> + transfer(s);
>> /* The N1 frame shift is complete so reset the N1 counters */
>> s->N2_bits = 0;
>> s->N2_bytes = 0;
>> s->N2_tx = 0;
>> s->N2_rx = 0;
>> - pnv_spi_xfer_buffer_free(*payload);
>> - *payload = NULL;
>> }
>> return stop;
>> } /* end of operation_shiftn1() */
>> @@ -588,19 +547,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>> * Shift_N2 operation handler method
>> */
>>
>> -static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> - PnvXferBuffer **payload)
>> +static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
>> {
>> uint8_t n2_count;
>> bool stop = false;
>> -
>> - /*
>> - * If there isn't a current payload left over from a stopped sequence
>> - * create a new one.
>> - */
>> - if (*payload == NULL) {
>> - *payload = pnv_spi_xfer_buffer_new();
>> - }
>> /*
>> * Use a combination of N2 counters to build the N2 portion of the
>> * transmit payload.
>> @@ -639,25 +589,26 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> /* Always append data for the N2 segment if it is set for TX */
>> uint8_t n2_byte = 0x00;
>> n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
>> - trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> - = n2_byte;
>> - } else {
>> + if (!fifo8_is_full(&s->tx_fifo)) {
>> + trace_pnv_spi_tx_append("n2_byte", n2_byte,
>> + (s->N1_tx + n2_count));
>> + fifo8_push(&s->tx_fifo, n2_byte);
>> + }
>> + } else if (!fifo8_is_full(&s->tx_fifo)) {
>> /*
>> * Regardless of whether or not N2 is set for TX or RX, we need
>> * the number of bytes in the payload to match the overall length
>> * of the operation.
>> */
>> trace_pnv_spi_tx_append_FF("n2_byte");
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> - = 0xff;
>> + fifo8_push(&s->tx_fifo, 0xff);
>> }
>> n2_count++;
>> } /* end of while */
>> if (!stop) {
>> /* We have a TX and a full TDR or an RX and an empty RDR */
>> - trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
>> - transfer(s, *payload);
>> + trace_pnv_spi_tx_request("Shifting N2 frame", (&s->tx_fifo)->num);
>> + transfer(s);
>> /*
>> * If we are doing an N2 TX and the TDR is full we need to clear the
>> * TDR_full status. Do this here instead of up in the loop above so we
>> @@ -680,8 +631,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> s->N1_bytes = 0;
>> s->N1_tx = 0;
>> s->N1_rx = 0;
>> - pnv_spi_xfer_buffer_free(*payload);
>> - *payload = NULL;
>> }
>> return stop;
>> } /* end of operation_shiftn2()*/
>> @@ -699,19 +648,6 @@ static void operation_sequencer(PnvSpi *s)
>> uint8_t opcode = 0;
>> uint8_t masked_opcode = 0;
>>
>> - /*
>> - * PnvXferBuffer for containing the payload of the SPI frame.
>> - * This is a static because there are cases where a sequence has to stop
>> - * and wait for the target application to unload the RDR. If this occurs
>> - * during a sequence where N1 is not sent alone and instead combined with
>> - * N2 since the N1 tx length + the N2 tx length is less than the size of
>> - * the TDR.
>> - */
>> - static PnvXferBuffer *payload;
>> -
>> - if (payload == NULL) {
>> - payload = pnv_spi_xfer_buffer_new();
>> - }
>> /*
>> * Clear the sequencer FSM error bit - general_SPI_status[3]
>> * before starting a sequence.
>> @@ -840,7 +776,7 @@ static void operation_sequencer(PnvSpi *s)
>> }
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> FSM_SHIFT_N1);
>> - stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
>> + stop = operation_shiftn1(s, opcode, send_n1_alone);
>> if (stop) {
>> /*
>> * The operation code says to stop, this can occur if:
>> @@ -895,7 +831,7 @@ static void operation_sequencer(PnvSpi *s)
>> /* Ok to do a Shift_N2 */
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> FSM_SHIFT_N2);
>> - stop = operation_shiftn2(s, opcode, &payload);
>> + stop = operation_shiftn2(s, opcode);
>> /*
>> * If the operation code says to stop set the shifter state to
>> * wait and stop
>> @@ -1208,6 +1144,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
>> s->cs_line = g_new0(qemu_irq, 1);
>> qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>>
>> + fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
>> + fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
>> +
>> /* spi scoms */
>> pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
>> s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);
>
> Looks nice, simplifies the code and tricky pointer and memory handling.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section
2024-10-08 7:44 ` Nicholas Piggin
@ 2024-10-08 17:32 ` Chalapathi V
0 siblings, 0 replies; 13+ messages in thread
From: Chalapathi V @ 2024-10-08 17:32 UTC (permalink / raw)
To: Nicholas Piggin, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On 08-10-2024 13:14, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>>
>> It is unfair to let the PowerNV SPI model to the SSI
>> maintainers. Also include the PowerNV ones.
>>
>> Fixes: 29318db133 ("hw/ssi: Add SPI model")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>> MAINTAINERS | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ffacd60f40..b11c4edaf0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1541,8 +1541,10 @@ F: hw/ppc/pnv*
>> F: hw/intc/pnv*
>> F: hw/intc/xics_pnv.c
>> F: hw/pci-host/pnv*
>> +F: hw/ssi/pnv_spi.c
>> F: include/hw/ppc/pnv*
>> F: include/hw/pci-host/pnv*
>> +F: hw/ssi/pnv_spi*
> Should be include/hw/ssi/pnv_spi*
Sure. Will update in next version.
Thank You,
Chalapathi
>
> Otherwise,
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>> F: pc-bios/skiboot.lid
>> F: tests/qtest/pnv*
>> F: tests/functional/test_ppc64_powernv.py
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
2024-10-08 8:13 ` Nicholas Piggin
@ 2024-10-08 17:34 ` Chalapathi V
0 siblings, 0 replies; 13+ messages in thread
From: Chalapathi V @ 2024-10-08 17:34 UTC (permalink / raw)
To: Nicholas Piggin, qemu-devel
Cc: qemu-ppc, fbarrat, clg, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On 08-10-2024 13:43, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> Use a local variable seq_index instead of repeatedly caling
>> get_seq_index() method.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>> hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
>> 1 file changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 2fd5aa0a96..962115f40f 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
>> fifo8_reset(&s->rx_fifo);
>> }
>>
>> -static inline uint8_t get_seq_index(PnvSpi *s)
>> -{
>> - return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>> -}
>> -
>> static inline void next_sequencer_fsm(PnvSpi *s)
>> {
>> - uint8_t seq_index = get_seq_index(s);
>> - s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
>> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
>> }
>>
>> @@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
>> bool stop = false; /* Flag to stop the sequencer */
>> uint8_t opcode = 0;
>> uint8_t masked_opcode = 0;
>> + uint8_t seq_index;
>>
>> /*
>> * Clear the sequencer FSM error bit - general_SPI_status[3]
>> @@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
>> if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
>> s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
>> }
>> + seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>> /*
>> * There are only 8 possible operation IDs to iterate through though
>> * some operations may cause more than one frame to be sequenced.
>> */
>> - while (get_seq_index(s) < NUM_SEQ_OPS) {
>> - opcode = s->seq_op[get_seq_index(s)];
>> + while (seq_index < NUM_SEQ_OPS) {
>> + opcode = s->seq_op[seq_index];
>> /* Set sequencer state to decode */
>> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
>> /*
>> @@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
>> case SEQ_OP_STOP:
>> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> /* A stop operation in any position stops the sequencer */
>> - trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
>> + trace_pnv_spi_sequencer_op("STOP", seq_index);
>>
>> stop = true;
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
>> @@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
>>
>> case SEQ_OP_SELECT_SLAVE:
>> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> - trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
>> + trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
>> /*
>> * This device currently only supports a single responder
>> * connection at position 0. De-selecting a responder is fine
>> @@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
>> if (s->responder_select == 0) {
>> trace_pnv_spi_shifter_done();
>> qemu_set_irq(s->cs_line[0], 1);
>> - s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
>> - (get_seq_index(s) + 1));
>> + seq_index++;
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
>> } else if (s->responder_select != 1) {
>> qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
>> @@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
>> * applies once a valid responder select has occurred.
>> */
>> s->shift_n1_done = false;
>> + seq_index++;
>> next_sequencer_fsm(s);
> Maybe could just open-code next_sequencer_fsm() now, since a bunch of
> other FSM fields seem to be open-coded?
Sure.
>
>> }
>> break;
>>
>> case SEQ_OP_SHIFT_N1:
>> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> - trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
>> + trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
>> /*
>> * Only allow a shift_n1 when the state is not IDLE or DONE.
>> * In either of those two cases the sequencer is not in a proper
>> @@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
>> * transmission to the responder without requiring a refill of
>> * the TDR between the two operations.
>> */
>> - if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
>> - == SEQ_OP_SHIFT_N2) {
>> + if ((seq_index != 7) &&
>> + PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
>> + SEQ_OP_SHIFT_N2) {
>> send_n1_alone = false;
>> }
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> The seq_index != 7 is a new test? Is that a separate fix, I'm not
> seeing how it's related to the seq_index change.
Not a new test but to make sure array index of seq_op doesn't overflow
due to seq_index + 1.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index()
2024-10-08 12:44 ` Cédric Le Goater
@ 2024-10-08 17:40 ` Chalapathi V
0 siblings, 0 replies; 13+ messages in thread
From: Chalapathi V @ 2024-10-08 17:40 UTC (permalink / raw)
To: Cédric Le Goater, Nicholas Piggin, qemu-devel
Cc: qemu-ppc, fbarrat, calebs, chalapathi.v, saif.abrar, dantan,
milesg, philmd, alistair
On 08-10-2024 18:14, Cédric Le Goater wrote:
> On 10/8/24 09:43, Nicholas Piggin wrote:
>> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>>> Hello,
>>>
>>> v3:
>>> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi*
>>> 2. Use of PnvXferBuffer results in a additonal process overhead due to
>>> frequent dynamic allocations and hence use an existing Fifo8 buffer.
>>> 3. Use a local variable seq_index and use it with in while loop instead
>>> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't
>>> overrun when seq_index is incremented.
>>>
>>> Tested:
>>> passed make check and make check-avocado
>>>
>>> Supersedes: <20240807202804.56038-1-philmd@linaro.org>
>>
>> Hi Chalapathi,
>>
>> To be clear, this fixes Coverity CID 1558831? A Resolves:
>> tag for the CID should be there, I guess it's patch 2?
>
>
> Patches should have a tag :
>
> Resolves: Coverity CID XYZZY
>
> Thanks,
>
> C.
Sure. Will update. Thank You
>
>
>>
>> I like patch 2, but since it is quite a significant change,
>> should we take the v2 series first which is much smaller,
>> then add this conversion on top of it?
>>
>> If it was long-standing code that would be important (because
>> you don't want to introduce regressions or conflicts when
>> backporting fixes). Since this is a new model I guess there
>> is leeway to just take v3 as is.
>>
>> Thanks,
>> Nick
>>
>>>
>>> Philippe Mathieu-Daudé (1):
>>> MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>>>
>>> Chalapathi V (2):
>>> hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
>>> hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().
>>>
>>> MAINTAINERS | 2 +
>>> include/hw/ssi/pnv_spi.h | 3 +
>>> hw/ssi/pnv_spi.c | 228
>>> +++++++++++++++------------------------
>>> 3 files changed, 89 insertions(+), 144 deletions(-)
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-10-08 17:41 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-18 16:50 [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Chalapathi V
2024-09-18 16:50 ` [PATCH v3 1/3] MAINTAINERS: Cover PowerPC SPI model in PowerNV section Chalapathi V
2024-10-08 7:44 ` Nicholas Piggin
2024-10-08 17:32 ` Chalapathi V
2024-09-18 16:50 ` [PATCH v3 2/3] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
2024-10-08 7:59 ` Nicholas Piggin
2024-10-08 17:18 ` Chalapathi V
2024-09-18 16:50 ` [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index() Chalapathi V
2024-10-08 8:13 ` Nicholas Piggin
2024-10-08 17:34 ` Chalapathi V
2024-10-08 7:43 ` [PATCH-for-9.2 v3 0/3] hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() Nicholas Piggin
2024-10-08 12:44 ` Cédric Le Goater
2024-10-08 17:40 ` Chalapathi V
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).