From: Chalapathi V <chalapathi.v@linux.ibm.com>
To: Nicholas Piggin <npiggin@gmail.com>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, fbarrat@linux.ibm.com, clg@kaod.org,
calebs@linux.ibm.com, chalapathi.v@ibm.com,
saif.abrar@linux.ibm.com, dantan@linux.vnet.ibm.com,
milesg@linux.ibm.com, philmd@linaro.org, alistair@alistair23.me
Subject: Re: [PATCH v5 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure
Date: Fri, 28 Feb 2025 08:29:48 +0530 [thread overview]
Message-ID: <a4e1e393-4f12-4242-848b-e67a9a55dc0f@linux.ibm.com> (raw)
In-Reply-To: <D82UFOB22C3K.31271R5VWD0LU@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 26753 bytes --]
Hello Nick,
Thank You for reviewing this series.
On 27-02-2025 07:09, Nicholas Piggin wrote:
> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>> In PnvXferBuffer dynamically allocating and freeing is a
>> process overhead. Hence used an existing Fifo8 buffer with
>> capacity of 16 bytes.
>>
>> Signed-off-by: Chalapathi V<chalapathi.v@linux.ibm.com>
>> ---
>> include/hw/ssi/pnv_spi.h | 3 +
>> hw/ssi/pnv_spi.c | 237 +++++++++++++--------------------------
>> 2 files changed, 81 insertions(+), 159 deletions(-)
>>
>> diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
>> index 8815f67d45..9878d9a25f 100644
>> --- a/include/hw/ssi/pnv_spi.h
>> +++ b/include/hw/ssi/pnv_spi.h
>> @@ -23,6 +23,7 @@
>>
>> #include "hw/ssi/ssi.h"
>> #include "hw/sysbus.h"
>> +#include "qemu/fifo8.h"
>>
>> #define TYPE_PNV_SPI "pnv-spi"
>> OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
>> @@ -37,6 +38,8 @@ typedef struct PnvSpi {
>> SSIBus *ssi_bus;
>> qemu_irq *cs_line;
>> MemoryRegion xscom_spic_regs;
>> + Fifo8 tx_fifo;
>> + Fifo8 rx_fifo;
>> /* SPI object number */
>> uint32_t spic_num;
>> uint8_t transfer_len;
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 15e25bd1be..63d298980d 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -19,6 +19,7 @@
>>
>> #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>> #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>> +#define PNV_SPI_FIFO_SIZE 16
>>
>> /*
>> * Macro from include/hw/ppc/fdt.h
>> @@ -35,48 +36,14 @@
>> } \
>> } while (0)
>>
>> -/* PnvXferBuffer */
>> -typedef struct PnvXferBuffer {
>> -
>> - uint32_t len;
>> - uint8_t *data;
>> -
>> -} PnvXferBuffer;
>> -
>> -/* pnv_spi_xfer_buffer_methods */
>> -static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
>> -{
>> - PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
>> -
>> - return payload;
>> -}
>> -
>> -static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
>> -{
>> - g_free(payload->data);
>> - g_free(payload);
>> -}
>> -
>> -static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
>> - uint32_t offset, uint32_t length)
>> -{
>> - if (payload->len < (offset + length)) {
>> - payload->len = offset + length;
>> - payload->data = g_realloc(payload->data, payload->len);
>> - }
>> - return &payload->data[offset];
>> -}
>> -
>> static bool does_rdr_match(PnvSpi *s)
>> {
>> /*
>> * According to spec, the mask bits that are 0 are compared and the
>> * bits that are 1 are ignored.
>> */
>> - uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK,
>> - s->regs[SPI_MM_REG]);
>> - uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL,
>> - s->regs[SPI_MM_REG]);
>> + uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK, s->regs[SPI_MM_REG]);
>> + uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL, s->regs[SPI_MM_REG]);
>>
>> if ((~rdr_match_mask & rdr_match_val) == ((~rdr_match_mask) &
>> GETFIELD(PPC_BITMASK(48, 63), s->regs[SPI_RCV_DATA_REG]))) {
> Usually try to avoid unrelated / cleanup in the same patch that acually
> changes things. In this case it's quite minor but it helps with review
> and rebasing to avoid.
>
> If it's on the same line or very close line to your change, or
> occasional ones I don't mind so much, but you have quite a few
> more further down the patch.
Sorry about that!. Should I create a new patch in v6 or keep it as is
for now?
Going forward will create new patches for distinct changes.
Thank You.
>> @@ -107,8 +74,8 @@ static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
>> return byte;
>> }
>>
>> -static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>> - uint8_t ecc_count, uint8_t shift_in_count)
>> +static uint8_t read_from_frame(PnvSpi *s, uint8_t nr_bytes, uint8_t ecc_count,
>> + uint8_t shift_in_count)
>> {
>> uint8_t byte;
>> int count = 0;
>> @@ -118,20 +85,23 @@ static uint8_t read_from_frame(PnvSpi *s, uint8_t *read_buf, uint8_t nr_bytes,
>> if ((ecc_count != 0) &&
>> (shift_in_count == (PNV_SPI_REG_SIZE + ecc_count))) {
>> shift_in_count = 0;
>> - } else {
>> - byte = read_buf[count];
>> + } else if (!fifo8_is_empty(&s->rx_fifo)) {
>> + byte = fifo8_pop(&s->rx_fifo);
>> trace_pnv_spi_shift_rx(byte, count);
>> s->regs[SPI_RCV_DATA_REG] = (s->regs[SPI_RCV_DATA_REG] << 8) | byte;
>> + } else {
>> + qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: Reading empty RX_FIFO\n");
>> }
>> count++;
>> } /* end of while */
>> return shift_in_count;
>> }
>>
>> -static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> +static void spi_response(PnvSpi *s)
>> {
>> uint8_t ecc_count;
>> uint8_t shift_in_count;
>> + uint32_t rx_len;
>>
>> /*
>> * Processing here must handle:
>> @@ -144,13 +114,14 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> * First check that the response payload is the exact same
>> * number of bytes as the request payload was
>> */
>> - if (rsp_payload->len != (s->N1_bytes + s->N2_bytes)) {
>> + rx_len = fifo8_num_used(&s->rx_fifo);
>> + if (rx_len != (s->N1_bytes + s->N2_bytes)) {
>> qemu_log_mask(LOG_GUEST_ERROR, "Invalid response payload size in "
>> "bytes, expected %d, got %d\n",
>> - (s->N1_bytes + s->N2_bytes), rsp_payload->len);
>> + (s->N1_bytes + s->N2_bytes), rx_len);
>> } else {
>> uint8_t ecc_control;
>> - trace_pnv_spi_rx_received(rsp_payload->len);
>> + trace_pnv_spi_rx_received(rx_len);
>> trace_pnv_spi_log_Ncounts(s->N1_bits, s->N1_bytes, s->N1_tx,
>> s->N1_rx, s->N2_bits, s->N2_bytes, s->N2_tx, s->N2_rx);
>> /*
>> @@ -175,15 +146,12 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> /* Handle the N1 portion of the frame first */
>> if (s->N1_rx != 0) {
>> trace_pnv_spi_rx_read_N1frame();
>> - shift_in_count = read_from_frame(s, &rsp_payload->data[0],
>> - s->N1_bytes, ecc_count, shift_in_count);
>> + shift_in_count = read_from_frame(s, s->N1_bytes, ecc_count, shift_in_count);
>> }
>> /* Handle the N2 portion of the frame */
>> if (s->N2_rx != 0) {
>> trace_pnv_spi_rx_read_N2frame();
>> - shift_in_count = read_from_frame(s,
>> - &rsp_payload->data[s->N1_bytes], s->N2_bytes,
>> - ecc_count, shift_in_count);
>> + shift_in_count = read_from_frame(s, s->N2_bytes, ecc_count, shift_in_count);
>> }
>> if ((s->N1_rx + s->N2_rx) > 0) {
>> /*
>> @@ -210,36 +178,38 @@ static void spi_response(PnvSpi *s, int bits, PnvXferBuffer *rsp_payload)
>> } /* end of else */
>> } /* end of spi_response() */
>>
>> -static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>> +static void transfer(PnvSpi *s)
>> {
>> - uint32_t tx;
>> - uint32_t rx;
>> - PnvXferBuffer *rsp_payload = NULL;
>> + uint32_t tx, rx, payload_len;
>> + uint8_t rx_byte;
>>
>> - rsp_payload = pnv_spi_xfer_buffer_new();
>> - if (!rsp_payload) {
>> - return;
>> - }
>> - for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
>> + payload_len = fifo8_num_used(&s->tx_fifo);
>> + for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
>> tx = 0;
>> for (int i = 0; i < s->transfer_len; i++) {
>> - if ((offset + i) >= payload->len) {
>> + if ((offset + i) >= payload_len) {
>> tx <<= 8;
>> + } else if (!fifo8_is_empty(&s->tx_fifo)) {
>> + tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
>> } else {
>> - tx = (tx << 8) | payload->data[offset + i];
>> + qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
>> }
>> }
>> rx = ssi_transfer(s->ssi_bus, tx);
>> for (int i = 0; i < s->transfer_len; i++) {
>> - if ((offset + i) >= payload->len) {
>> + if (((offset + i) >= payload_len) || fifo8_is_full(&s->rx_fifo)) {
>> break;
>> }
>> - *(pnv_spi_xfer_buffer_write_ptr(rsp_payload, rsp_payload->len, 1)) =
>> - (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>> + rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>> + if (!fifo8_is_full(&s->rx_fifo)) {
>> + fifo8_push(&s->rx_fifo, rx_byte);
>> + }
>> }
>> }
>> - spi_response(s, s->N1_bits, rsp_payload);
>> - pnv_spi_xfer_buffer_free(rsp_payload);
>> + spi_response(s);
>> + /* Reset fifo for next frame */
>> + fifo8_reset(&s->tx_fifo);
>> + fifo8_reset(&s->rx_fifo);
>> }
>>
>> static inline uint8_t get_seq_index(PnvSpi *s)
>> @@ -310,13 +280,11 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>> * If Forced Implicit mode and count control doesn't
>> * indicate transmit then reset the tx count to 0
>> */
>> - if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2,
>> - s->regs[SPI_CTR_CFG_REG]) == 0) {
>> + if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 0) {
>> s->N1_tx = 0;
>> }
>> /* If rx count control for N1 is set, load the rx value */
>> - if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3,
>> - s->regs[SPI_CTR_CFG_REG]) == 1) {
>> + if (GETFIELD(SPI_CTR_CFG_N1_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 1) {
>> s->N1_rx = s->N1_bytes;
>> }
>> }
>> @@ -328,8 +296,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>> * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
>> * error bit.
>> */
>> - uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
>> - s->regs[SPI_CLK_CFG_REG]);
>> + uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
>> if (ecc_control == 0 || ecc_control == 2) {
>> if (s->N1_bytes > (PNV_SPI_REG_SIZE + 1)) {
>> qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size when "
>> @@ -340,8 +307,7 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>> }
>> } else if (s->N1_bytes > PNV_SPI_REG_SIZE) {
>> qemu_log_mask(LOG_GUEST_ERROR, "Unsupported N1 shift size, "
>> - "bytes = 0x%x, bits = 0x%x\n",
>> - s->N1_bytes, s->N1_bits);
>> + "bytes = 0x%x, bits = 0x%x\n", s->N1_bytes, s->N1_bits);
>> s->N1_bytes = PNV_SPI_REG_SIZE;
>> s->N1_bits = s->N1_bytes * 8;
>> }
>> @@ -350,19 +316,10 @@ static void calculate_N1(PnvSpi *s, uint8_t opcode)
>> /*
>> * Shift_N1 operation handler method
>> */
>> -static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> - PnvXferBuffer **payload, bool send_n1_alone)
>> +static bool operation_shiftn1(PnvSpi *s, uint8_t opcode, bool send_n1_alone)
>> {
>> uint8_t n1_count;
>> bool stop = false;
>> -
>> - /*
>> - * If there isn't a current payload left over from a stopped sequence
>> - * create a new one.
>> - */
>> - if (*payload == NULL) {
>> - *payload = pnv_spi_xfer_buffer_new();
>> - }
>> /*
>> * Use a combination of N1 counters to build the N1 portion of the
>> * transmit payload.
>> @@ -413,9 +370,10 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> */
>> uint8_t n1_byte = 0x00;
>> n1_byte = get_from_offset(s, n1_count);
>> - trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1)) =
>> - n1_byte;
>> + if (!fifo8_is_full(&s->tx_fifo)) {
>> + trace_pnv_spi_tx_append("n1_byte", n1_byte, n1_count);
>> + fifo8_push(&s->tx_fifo, n1_byte);
>> + }
>> } else {
>> /*
>> * We hit a shift_n1 opcode TX but the TDR is empty, tell the
>> @@ -436,16 +394,14 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> * - we are receiving and the RDR is empty so we allow the operation
>> * to proceed.
>> */
>> - if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL,
>> - s->status) == 1)) {
>> + if ((s->N1_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>> trace_pnv_spi_sequencer_stop_requested("shift N1"
>> "set for receive but RDR is full");
>> stop = true;
>> break;
>> - } else {
>> + } else if (!fifo8_is_full(&s->tx_fifo)) {
>> trace_pnv_spi_tx_append_FF("n1_byte");
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> - = 0xff;
>> + fifo8_push(&s->tx_fifo, 0xff);
>> }
> Is there any problem or overrun that needs to be logged if the
> fifo is full in the above two places?
Will add a error log upon hitting the overflow. Thank You.
>> }
>> n1_count++;
>> @@ -486,15 +442,13 @@ static bool operation_shiftn1(PnvSpi *s, uint8_t opcode,
>> */
>> if (send_n1_alone && !stop) {
>> /* We have a TX and a full TDR or an RX and an empty RDR */
>> - trace_pnv_spi_tx_request("Shifting N1 frame", (*payload)->len);
>> - transfer(s, *payload);
>> + trace_pnv_spi_tx_request("Shifting N1 frame", fifo8_num_used(&s->tx_fifo));
>> + transfer(s);
>> /* The N1 frame shift is complete so reset the N1 counters */
>> s->N2_bits = 0;
>> s->N2_bytes = 0;
>> s->N2_tx = 0;
>> s->N2_rx = 0;
>> - pnv_spi_xfer_buffer_free(*payload);
>> - *payload = NULL;
>> }
>> return stop;
>> } /* end of operation_shiftn1() */
>> @@ -552,13 +506,11 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>> * If Forced Implicit mode and count control doesn't
>> * indicate a receive then reset the rx count to 0
>> */
>> - if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3,
>> - s->regs[SPI_CTR_CFG_REG]) == 0) {
>> + if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B3, s->regs[SPI_CTR_CFG_REG]) == 0) {
>> s->N2_rx = 0;
>> }
>> /* If tx count control for N2 is set, load the tx value */
>> - if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2,
>> - s->regs[SPI_CTR_CFG_REG]) == 1) {
>> + if (GETFIELD(SPI_CTR_CFG_N2_CTRL_B2, s->regs[SPI_CTR_CFG_REG]) == 1) {
>> s->N2_tx = s->N2_bytes;
>> }
>> }
>> @@ -571,8 +523,7 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>> * cap the size at a max of 64 bits or 72 bits and set the sequencer FSM
>> * error bit.
>> */
>> - uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL,
>> - s->regs[SPI_CLK_CFG_REG]);
>> + uint8_t ecc_control = GETFIELD(SPI_CLK_CFG_ECC_CTRL, s->regs[SPI_CLK_CFG_REG]);
>> if (ecc_control == 0 || ecc_control == 2) {
>> if (s->N2_bytes > (PNV_SPI_REG_SIZE + 1)) {
>> /* Unsupported N2 shift size when ECC enabled */
>> @@ -590,19 +541,10 @@ static void calculate_N2(PnvSpi *s, uint8_t opcode)
>> * Shift_N2 operation handler method
>> */
>>
>> -static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> - PnvXferBuffer **payload)
>> +static bool operation_shiftn2(PnvSpi *s, uint8_t opcode)
>> {
>> uint8_t n2_count;
>> bool stop = false;
>> -
>> - /*
>> - * If there isn't a current payload left over from a stopped sequence
>> - * create a new one.
>> - */
>> - if (*payload == NULL) {
>> - *payload = pnv_spi_xfer_buffer_new();
>> - }
>> /*
>> * Use a combination of N2 counters to build the N2 portion of the
>> * transmit payload.
>> @@ -629,44 +571,41 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> * code continue will end up building the payload twice in the same
>> * buffer since RDR full causes a sequence stop and restart.
>> */
>> - if ((s->N2_rx != 0) &&
>> - (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>> + if ((s->N2_rx != 0) && (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1)) {
>> trace_pnv_spi_sequencer_stop_requested("shift N2 set"
>> "for receive but RDR is full");
>> stop = true;
>> break;
>> }
>> - if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) <
>> - PNV_SPI_REG_SIZE)) {
>> + if ((s->N2_tx != 0) && ((s->N1_tx + n2_count) < PNV_SPI_REG_SIZE)) {
>> /* Always append data for the N2 segment if it is set for TX */
>> uint8_t n2_byte = 0x00;
>> n2_byte = get_from_offset(s, (s->N1_tx + n2_count));
>> - trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> - = n2_byte;
>> - } else {
>> + if (!fifo8_is_full(&s->tx_fifo)) {
>> + trace_pnv_spi_tx_append("n2_byte", n2_byte, (s->N1_tx + n2_count));
>> + fifo8_push(&s->tx_fifo, n2_byte);
>> + }
>> + } else if (!fifo8_is_full(&s->tx_fifo)) {
>> /*
>> * Regardless of whether or not N2 is set for TX or RX, we need
>> * the number of bytes in the payload to match the overall length
>> * of the operation.
>> */
>> trace_pnv_spi_tx_append_FF("n2_byte");
>> - *(pnv_spi_xfer_buffer_write_ptr(*payload, (*payload)->len, 1))
>> - = 0xff;
>> + fifo8_push(&s->tx_fifo, 0xff);
>> }
>> n2_count++;
>> } /* end of while */
>> if (!stop) {
>> /* We have a TX and a full TDR or an RX and an empty RDR */
>> - trace_pnv_spi_tx_request("Shifting N2 frame", (*payload)->len);
>> - transfer(s, *payload);
>> + trace_pnv_spi_tx_request("Shifting N2 frame", fifo8_num_used(&s->tx_fifo));
>> + transfer(s);
>> /*
>> * If we are doing an N2 TX and the TDR is full we need to clear the
>> * TDR_full status. Do this here instead of up in the loop above so we
>> * don't log the message in every loop iteration.
>> */
>> - if ((s->N2_tx != 0) &&
>> - (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
>> + if ((s->N2_tx != 0) && (GETFIELD(SPI_STS_TDR_FULL, s->status) == 1)) {
>> s->status = SETFIELD(SPI_STS_TDR_FULL, s->status, 0);
>> }
>> /*
>> @@ -682,8 +621,6 @@ static bool operation_shiftn2(PnvSpi *s, uint8_t opcode,
>> s->N1_bytes = 0;
>> s->N1_tx = 0;
>> s->N1_rx = 0;
>> - pnv_spi_xfer_buffer_free(*payload);
>> - *payload = NULL;
>> }
>> return stop;
>> } /* end of operation_shiftn2()*/
>> @@ -701,19 +638,6 @@ static void operation_sequencer(PnvSpi *s)
>> uint8_t opcode = 0;
>> uint8_t masked_opcode = 0;
>>
>> - /*
>> - * PnvXferBuffer for containing the payload of the SPI frame.
>> - * This is a static because there are cases where a sequence has to stop
>> - * and wait for the target application to unload the RDR. If this occurs
>> - * during a sequence where N1 is not sent alone and instead combined with
>> - * N2 since the N1 tx length + the N2 tx length is less than the size of
>> - * the TDR.
>> - */
>> - static PnvXferBuffer *payload;
>> -
>> - if (payload == NULL) {
>> - payload = pnv_spi_xfer_buffer_new();
>> - }
>> /*
>> * Clear the sequencer FSM error bit - general_SPI_status[3]
>> * before starting a sequence.
>> @@ -775,10 +699,8 @@ static void operation_sequencer(PnvSpi *s)
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
>> } else if (s->responder_select != 1) {
>> qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
>> - "not supported, select = 0x%x\n",
>> - s->responder_select);
>> - trace_pnv_spi_sequencer_stop_requested("invalid "
>> - "responder select");
>> + "not supported, select = 0x%x\n", s->responder_select);
>> + trace_pnv_spi_sequencer_stop_requested("invalid responder select");
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
>> stop = true;
>> } else {
>> @@ -840,9 +762,8 @@ static void operation_sequencer(PnvSpi *s)
>> == SEQ_OP_SHIFT_N2) {
>> send_n1_alone = false;
>> }
>> - s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> - FSM_SHIFT_N1);
>> - stop = operation_shiftn1(s, opcode, &payload, send_n1_alone);
>> + s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N1);
>> + stop = operation_shiftn1(s, opcode, send_n1_alone);
>> if (stop) {
>> /*
>> * The operation code says to stop, this can occur if:
>> @@ -858,7 +779,7 @@ static void operation_sequencer(PnvSpi *s)
>> if (GETFIELD(SPI_STS_TDR_UNDERRUN, s->status)) {
>> s->shift_n1_done = true;
>> s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> - FSM_SHIFT_N2);
>> + FSM_SHIFT_N2);
>> s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
>> (get_seq_index(s) + 1));
>> } else {
>> @@ -866,8 +787,7 @@ static void operation_sequencer(PnvSpi *s)
>> * This is case (1) or (2) so the sequencer needs to
>> * wait and NOT go to the next sequence yet.
>> */
>> - s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> - FSM_WAIT);
>> + s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
>> }
>> } else {
>> /* Ok to move on to the next index */
>> @@ -890,21 +810,18 @@ static void operation_sequencer(PnvSpi *s)
>> * error bit 3 (general_SPI_status[3]) in status reg.
>> */
>> s->status = SETFIELD(SPI_STS_GEN_STATUS_B3, s->status, 1);
>> - trace_pnv_spi_sequencer_stop_requested("shift_n2 "
>> - "w/no shift_n1 done");
>> + trace_pnv_spi_sequencer_stop_requested("shift_n2 w/no shift_n1 done");
>> stop = true;
>> } else {
>> /* Ok to do a Shift_N2 */
>> - s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> - FSM_SHIFT_N2);
>> - stop = operation_shiftn2(s, opcode, &payload);
>> + s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_SHIFT_N2);
>> + stop = operation_shiftn2(s, opcode);
>> /*
>> * If the operation code says to stop set the shifter state to
>> * wait and stop
>> */
>> if (stop) {
>> - s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
>> - FSM_WAIT);
>> + s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_WAIT);
>> } else {
>> /* Ok to move on to the next index */
>> next_sequencer_fsm(s);
>> @@ -988,8 +905,7 @@ static void operation_sequencer(PnvSpi *s)
>> case SEQ_OP_BRANCH_IFNEQ_INC_2:
>> s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", get_seq_index(s));
>> - uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
>> - s->regs[SPI_CTR_CFG_REG]);
>> + uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2, s->regs[SPI_CTR_CFG_REG]);
>> /*
>> * The spec says the loop should execute count compare + 1 times.
>> * However we learned from engineering that we really only loop
>> @@ -1209,6 +1125,9 @@ static void pnv_spi_realize(DeviceState *dev, Error **errp)
>> s->cs_line = g_new0(qemu_irq, 1);
>> qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
>>
>> + fifo8_create(&s->tx_fifo, PNV_SPI_FIFO_SIZE);
>> + fifo8_create(&s->rx_fifo, PNV_SPI_FIFO_SIZE);
>> +
>> /* spi scoms */
>> pnv_xscom_region_init(&s->xscom_spic_regs, OBJECT(s), &pnv_spi_xscom_ops,
>> s, "xscom-spi", PNV10_XSCOM_PIB_SPIC_SIZE);
> Aside from those small nits, it looks like a good cleanup and
> improvement with more error logging.
>
> Thanks,
> Nick
[-- Attachment #2: Type: text/html, Size: 26894 bytes --]
next prev parent reply other threads:[~2025-02-28 3:01 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-03 16:18 [PATCH v5 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and fix CID 1558827 Chalapathi V
2025-01-03 16:18 ` [PATCH v5 1/4] hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure Chalapathi V
2025-02-27 1:39 ` Nicholas Piggin
2025-02-28 2:59 ` Chalapathi V [this message]
2025-01-03 16:18 ` [PATCH v5 2/4] hw/ssi/pnv_spi: Coverity CID 1558827: Use local var seq_index instead of get_seq_index() Chalapathi V
2025-02-27 1:43 ` Nicholas Piggin
2025-02-27 1:44 ` Nicholas Piggin
2025-01-03 16:18 ` [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket Chalapathi V
2025-02-27 1:54 ` Nicholas Piggin
2025-02-28 3:03 ` Chalapathi V
2025-02-28 7:45 ` Cédric Le Goater
2025-02-28 11:00 ` Chalapathi V
2025-02-28 16:15 ` Cédric Le Goater
2025-01-03 16:18 ` [PATCH v5 4/4] hw/ssi/pnv_spi: Put a limit to RDR match failures Chalapathi V
2025-02-27 1:56 ` Nicholas Piggin
2025-02-28 3:04 ` Chalapathi V
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a4e1e393-4f12-4242-848b-e67a9a55dc0f@linux.ibm.com \
--to=chalapathi.v@linux.ibm.com \
--cc=alistair@alistair23.me \
--cc=calebs@linux.ibm.com \
--cc=chalapathi.v@ibm.com \
--cc=clg@kaod.org \
--cc=dantan@linux.vnet.ibm.com \
--cc=fbarrat@linux.ibm.com \
--cc=milesg@linux.ibm.com \
--cc=npiggin@gmail.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=saif.abrar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).