* [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host
@ 2022-08-10 23:01 Wilfred Mallawa
2022-08-10 23:02 ` [PATCH 2/3] hw/ssi: fixup coverity issue Wilfred Mallawa
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Wilfred Mallawa @ 2022-08-10 23:01 UTC (permalink / raw)
To: Alistair.Francis, qemu-riscv; +Cc: qemu-devel, Wilfred Mallawa
From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
This patch fixes up minor typos in ibex_spi_host
Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
---
hw/ssi/ibex_spi_host.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
index d14580b409..601041d719 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -172,7 +172,7 @@ static void ibex_spi_host_irq(IbexSPIHostState *s)
& R_INTR_STATE_SPI_EVENT_MASK;
int err_irq = 0, event_irq = 0;
- /* Error IRQ enabled and Error IRQ Cleared*/
+ /* Error IRQ enabled and Error IRQ Cleared */
if (error_en && !err_pending) {
/* Event enabled, Interrupt Test Error */
if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) {
@@ -434,7 +434,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
case IBEX_SPI_HOST_TXDATA:
/*
* This is a hardware `feature` where
- * the first word written TXDATA after init is omitted entirely
+ * the first word written to TXDATA after init is omitted entirely
*/
if (s->init_status) {
s->init_status = false;
@@ -487,7 +487,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
break;
case IBEX_SPI_HOST_ERROR_STATUS:
/*
- * Indicates that any errors that have occurred.
+ * Indicates any errors that have occurred.
* When an error occurs, the corresponding bit must be cleared
* here before issuing any further commands
*/
--
2.37.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/3] hw/ssi: fixup coverity issue 2022-08-10 23:01 [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Wilfred Mallawa @ 2022-08-10 23:02 ` Wilfred Mallawa 2022-08-11 2:55 ` Bin Meng 2022-08-11 14:23 ` Andrew Jones 2022-08-10 23:02 ` [PATCH 3/3] hw/ssi: fixup/add rw1c functionality Wilfred Mallawa ` (2 subsequent siblings) 3 siblings, 2 replies; 11+ messages in thread From: Wilfred Mallawa @ 2022-08-10 23:02 UTC (permalink / raw) To: Alistair.Francis, qemu-riscv; +Cc: qemu-devel, Wilfred Mallawa From: Wilfred Mallawa <wilfred.mallawa@wdc.com> This patch addresses the coverity issues specified in [1], as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been implemented to clean up the code. Additionally, the `EVENT_ENABLE` register is correctly updated to addr of `0x34`. [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html Fixes: Coverity CID 1488107 Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> --- hw/ssi/ibex_spi_host.c | 141 +++++++++++++++++++++++------------------ 1 file changed, 78 insertions(+), 63 deletions(-) diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c index 601041d719..8c35bfa95f 100644 --- a/hw/ssi/ibex_spi_host.c +++ b/hw/ssi/ibex_spi_host.c @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) FIELD(ERROR_STATUS, CMDINVAL, 3, 1) FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) -REG32(EVENT_ENABLE, 0x30) +REG32(EVENT_ENABLE, 0x34) FIELD(EVENT_ENABLE, RXFULL, 0, 1) FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) FIELD(EVENT_ENABLE, RXWM, 2, 1) @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t dividend) static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) { + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; /* Empty the RX FIFO and assert RXEMPTY */ fifo8_reset(&s->rx_fifo); - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); + s->regs[IBEX_SPI_HOST_STATUS] = data; } static void ibex_spi_txfifo_reset(IbexSPIHostState *s) { + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; /* Empty the TX FIFO and assert TXEMPTY */ fifo8_reset(&s->tx_fifo); - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); + s->regs[IBEX_SPI_HOST_STATUS] = data; } static void ibex_spi_host_reset(DeviceState *dev) @@ -162,37 +164,41 @@ static void ibex_spi_host_reset(DeviceState *dev) */ static void ibex_spi_host_irq(IbexSPIHostState *s) { - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] - & R_INTR_ENABLE_ERROR_MASK; - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] - & R_INTR_ENABLE_SPI_EVENT_MASK; - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] - & R_INTR_STATE_ERROR_MASK; - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] - & R_INTR_STATE_SPI_EVENT_MASK; + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], + INTR_ENABLE, ERROR); + + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], + INTR_ENABLE, SPI_EVENT); + + bool err_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], + INTR_STATE, ERROR); + + bool status_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], + INTR_STATE, SPI_EVENT); + int err_irq = 0, event_irq = 0; /* Error IRQ enabled and Error IRQ Cleared */ if (error_en && !err_pending) { /* Event enabled, Interrupt Test Error */ - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) { + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], INTR_TEST, ERROR)) { err_irq = 1; - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] - & R_ERROR_ENABLE_CMDBUSY_MASK) && - s->regs[IBEX_SPI_HOST_ERROR_STATUS] - & R_ERROR_STATUS_CMDBUSY_MASK) { + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], + ERROR_ENABLE, CMDBUSY) && + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], + ERROR_STATUS, CMDBUSY)) { /* Wrote to COMMAND when not READY */ err_irq = 1; - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] - & R_ERROR_ENABLE_CMDINVAL_MASK) && - s->regs[IBEX_SPI_HOST_ERROR_STATUS] - & R_ERROR_STATUS_CMDINVAL_MASK) { + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], + ERROR_ENABLE, CMDINVAL) && + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], + ERROR_STATUS, CMDINVAL)) { /* Invalid command segment */ err_irq = 1; - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] - & R_ERROR_ENABLE_CSIDINVAL_MASK) && - s->regs[IBEX_SPI_HOST_ERROR_STATUS] - & R_ERROR_STATUS_CSIDINVAL_MASK) { + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], + ERROR_ENABLE, CSIDINVAL) && + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], + ERROR_STATUS, CSIDINVAL)) { /* Invalid value for CSID */ err_irq = 1; } @@ -204,22 +210,26 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) /* Event IRQ Enabled and Event IRQ Cleared */ if (event_en && !status_pending) { - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_SPI_EVENT_MASK) { + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], + INTR_STATE, SPI_EVENT)) { /* Event enabled, Interrupt Test Event */ event_irq = 1; - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] - & R_EVENT_ENABLE_READY_MASK) && - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], + EVENT_ENABLE, READY) && + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], + STATUS, READY)) { /* SPI Host ready for next command */ event_irq = 1; - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] - & R_EVENT_ENABLE_TXEMPTY_MASK) && - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_TXEMPTY_MASK)) { + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], + EVENT_ENABLE, TXEMPTY) && + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], + STATUS, TXEMPTY)) { /* SPI TXEMPTY, TXFIFO drained */ event_irq = 1; - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] - & R_EVENT_ENABLE_RXFULL_MASK) && - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_RXFULL_MASK)) { + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], + EVENT_ENABLE, RXFULL) && + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], + STATUS, RXFULL)) { /* SPI RXFULL, RXFIFO full */ event_irq = 1; } @@ -232,10 +242,11 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) static void ibex_spi_host_transfer(IbexSPIHostState *s) { - uint32_t rx, tx; + uint32_t rx, tx, data; /* Get num of one byte transfers */ - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & R_COMMAND_LEN_MASK) - >> R_COMMAND_LEN_SHIFT); + uint8_t segment_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_COMMAND], + COMMAND, LEN); + while (segment_len > 0) { if (fifo8_is_empty(&s->tx_fifo)) { /* Assert Stall */ @@ -262,22 +273,23 @@ static void ibex_spi_host_transfer(IbexSPIHostState *s) --segment_len; } + data = s->regs[IBEX_SPI_HOST_STATUS]; /* Assert Ready */ - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; + data = FIELD_DP32(data, STATUS, READY, 1); /* Set RXQD */ - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK - & div4_round_up(segment_len)); + data = FIELD_DP32(data, STATUS, RXQD, div4_round_up(segment_len)); /* Set TXQD */ - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) / 4) - & R_STATUS_TXQD_MASK; + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s->tx_fifo) / 4); /* Clear TXFULL */ - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; - /* Assert TXEMPTY and drop remaining bytes that exceed segment_len */ - ibex_spi_txfifo_reset(s); + data = FIELD_DP32(data, STATUS, TXFULL, 0); /* Reset RXEMPTY */ - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); + /* Set TXEMPTY */ + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); + /* Drop remaining bytes that exceed segment_len */ + ibex_spi_txfifo_reset(s); + /* Update register status */ + s->regs[IBEX_SPI_HOST_STATUS] = data; ibex_spi_host_irq(s); } @@ -340,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, { IbexSPIHostState *s = opaque; uint32_t val32 = val64; - uint32_t shift_mask = 0xff; + uint32_t shift_mask = 0xff, data; uint8_t txqd_len; trace_ibex_spi_host_write(addr, size, val64); @@ -397,21 +409,23 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, s->regs[addr] = val32; /* STALL, IP not enabled */ - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & R_CONTROL_SPIEN_MASK)) { + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], + CONTROL, SPIEN))) { return; } /* SPI not ready, IRQ Error */ - if (!(s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], + STATUS, READY))) { s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= R_ERROR_STATUS_CMDBUSY_MASK; ibex_spi_host_irq(s); return; } + /* Assert Not Ready */ s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; - if (((val32 & R_COMMAND_DIRECTION_MASK) >> R_COMMAND_DIRECTION_SHIFT) - != BIDIRECTIONAL_TRANSFER) { + if (FIELD_EX32(val32, COMMAND, DIRECTION) != BIDIRECTIONAL_TRANSFER) { qemu_log_mask(LOG_UNIMP, "%s: Rx Only/Tx Only are not supported\n", __func__); } @@ -452,8 +466,8 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, return; } /* Byte ordering is set by the IP */ - if ((s->regs[IBEX_SPI_HOST_STATUS] & - R_STATUS_BYTEORDER_MASK) == 0) { + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, + BYTEORDER) == 0) { /* LE: LSB transmitted first (default for ibex processor) */ shift_mask = 0xff << (i * 8); } else { @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * 8)); } + data = s->regs[IBEX_SPI_HOST_STATUS]; /* Reset TXEMPTY */ - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK; + data = FIELD_DP32(data, STATUS, TXEMPTY, 0); /* Update TXQD */ - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] & - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT; + txqd_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, TXQD); /* Partial bytes (size < 4) are padded, in words. */ txqd_len += 1; - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len; + data = FIELD_DP32(data, STATUS, TXQD, txqd_len); /* Assert Ready */ - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; + data = FIELD_DP32(data, STATUS, READY, 1); + /* Update register status */ + s->regs[IBEX_SPI_HOST_STATUS] = data; break; case IBEX_SPI_HOST_ERROR_ENABLE: s->regs[addr] = val32; -- 2.37.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/ssi: fixup coverity issue 2022-08-10 23:02 ` [PATCH 2/3] hw/ssi: fixup coverity issue Wilfred Mallawa @ 2022-08-11 2:55 ` Bin Meng 2022-08-12 2:23 ` Wilfred Mallawa 2022-08-11 14:23 ` Andrew Jones 1 sibling, 1 reply; 11+ messages in thread From: Bin Meng @ 2022-08-11 2:55 UTC (permalink / raw) To: Wilfred Mallawa Cc: Alistair Francis, open list:RISC-V, qemu-devel@nongnu.org Developers, Wilfred Mallawa On Thu, Aug 11, 2022 at 8:58 AM Wilfred Mallawa <wilfred.mallawa@opensource.wdc.com> wrote: > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > This patch addresses the coverity issues specified in [1], > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > implemented to clean up the code. > > Additionally, the `EVENT_ENABLE` register is correctly updated > to addr of `0x34`. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > Fixes: Coverity CID 1488107 > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> nits: please add "ibex_spi" to the tag, like hw/ssi: ibex_spi: > --- > hw/ssi/ibex_spi_host.c | 141 +++++++++++++++++++++++------------------ > 1 file changed, 78 insertions(+), 63 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 601041d719..8c35bfa95f 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > -REG32(EVENT_ENABLE, 0x30) > +REG32(EVENT_ENABLE, 0x34) > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > FIELD(EVENT_ENABLE, RXWM, 2, 1) > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t dividend) > > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) > { > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Empty the RX FIFO and assert RXEMPTY */ > fifo8_reset(&s->rx_fifo); > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); > + s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_txfifo_reset(IbexSPIHostState *s) > { > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Empty the TX FIFO and assert TXEMPTY */ > fifo8_reset(&s->tx_fifo); > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > + s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_host_reset(DeviceState *dev) > @@ -162,37 +164,41 @@ static void ibex_spi_host_reset(DeviceState *dev) > */ > static void ibex_spi_host_irq(IbexSPIHostState *s) > { > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > - & R_INTR_ENABLE_ERROR_MASK; > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > - & R_INTR_ENABLE_SPI_EVENT_MASK; > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > - & R_INTR_STATE_ERROR_MASK; > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > - & R_INTR_STATE_SPI_EVENT_MASK; > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, ERROR); > + > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, SPI_EVENT); > + > + bool err_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, ERROR); > + > + bool status_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, SPI_EVENT); > + > int err_irq = 0, event_irq = 0; > > /* Error IRQ enabled and Error IRQ Cleared */ > if (error_en && !err_pending) { > /* Event enabled, Interrupt Test Error */ > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) { > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], INTR_TEST, ERROR)) { > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CMDBUSY_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CMDBUSY_MASK) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDBUSY) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CMDBUSY)) { > /* Wrote to COMMAND when not READY */ > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CMDINVAL_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CMDINVAL_MASK) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDINVAL) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CMDINVAL)) { > /* Invalid command segment */ > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CSIDINVAL_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CSIDINVAL_MASK) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CSIDINVAL) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CSIDINVAL)) { > /* Invalid value for CSID */ > err_irq = 1; > } > @@ -204,22 +210,26 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > > /* Event IRQ Enabled and Event IRQ Cleared */ > if (event_en && !status_pending) { > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_SPI_EVENT_MASK) { > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > + INTR_STATE, SPI_EVENT)) { > /* Event enabled, Interrupt Test Event */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_READY_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > + EVENT_ENABLE, READY) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, READY)) { > /* SPI Host ready for next command */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_TXEMPTY_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_TXEMPTY_MASK)) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > + EVENT_ENABLE, TXEMPTY) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, TXEMPTY)) { > /* SPI TXEMPTY, TXFIFO drained */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_RXFULL_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_RXFULL_MASK)) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > + EVENT_ENABLE, RXFULL) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, RXFULL)) { > /* SPI RXFULL, RXFIFO full */ > event_irq = 1; > } > @@ -232,10 +242,11 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > > static void ibex_spi_host_transfer(IbexSPIHostState *s) > { > - uint32_t rx, tx; > + uint32_t rx, tx, data; > /* Get num of one byte transfers */ > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & R_COMMAND_LEN_MASK) > - >> R_COMMAND_LEN_SHIFT); > + uint8_t segment_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_COMMAND], > + COMMAND, LEN); > + > while (segment_len > 0) { > if (fifo8_is_empty(&s->tx_fifo)) { > /* Assert Stall */ > @@ -262,22 +273,23 @@ static void ibex_spi_host_transfer(IbexSPIHostState *s) > --segment_len; > } > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Assert Ready */ > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > + data = FIELD_DP32(data, STATUS, READY, 1); > /* Set RXQD */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK > - & div4_round_up(segment_len)); > + data = FIELD_DP32(data, STATUS, RXQD, div4_round_up(segment_len)); > /* Set TXQD */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) / 4) > - & R_STATUS_TXQD_MASK; > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s->tx_fifo) / 4); > /* Clear TXFULL */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > - /* Assert TXEMPTY and drop remaining bytes that exceed segment_len */ > - ibex_spi_txfifo_reset(s); > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > /* Reset RXEMPTY */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); > + /* Set TXEMPTY */ > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > + /* Drop remaining bytes that exceed segment_len */ > + ibex_spi_txfifo_reset(s); > + /* Update register status */ > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > ibex_spi_host_irq(s); > } > @@ -340,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > { > IbexSPIHostState *s = opaque; > uint32_t val32 = val64; > - uint32_t shift_mask = 0xff; > + uint32_t shift_mask = 0xff, data; > uint8_t txqd_len; > > trace_ibex_spi_host_write(addr, size, val64); > @@ -397,21 +409,23 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > s->regs[addr] = val32; > > /* STALL, IP not enabled */ > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & R_CONTROL_SPIEN_MASK)) { > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], > + CONTROL, SPIEN))) { > return; > } > > /* SPI not ready, IRQ Error */ > - if (!(s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, READY))) { > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= R_ERROR_STATUS_CMDBUSY_MASK; > ibex_spi_host_irq(s); > return; > } > + > /* Assert Not Ready */ > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; > > - if (((val32 & R_COMMAND_DIRECTION_MASK) >> R_COMMAND_DIRECTION_SHIFT) > - != BIDIRECTIONAL_TRANSFER) { > + if (FIELD_EX32(val32, COMMAND, DIRECTION) != BIDIRECTIONAL_TRANSFER) { > qemu_log_mask(LOG_UNIMP, > "%s: Rx Only/Tx Only are not supported\n", __func__); > } > @@ -452,8 +466,8 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > return; > } > /* Byte ordering is set by the IP */ > - if ((s->regs[IBEX_SPI_HOST_STATUS] & > - R_STATUS_BYTEORDER_MASK) == 0) { > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, > + BYTEORDER) == 0) { > /* LE: LSB transmitted first (default for ibex processor) */ > shift_mask = 0xff << (i * 8); > } else { > @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * 8)); > } > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Reset TXEMPTY */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, TXEMPTY, 0); > /* Update TXQD */ > - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] & > - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT; > + txqd_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, TXQD); > /* Partial bytes (size < 4) are padded, in words. */ > txqd_len += 1; > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len; > + data = FIELD_DP32(data, STATUS, TXQD, txqd_len); > /* Assert Ready */ > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > + data = FIELD_DP32(data, STATUS, READY, 1); > + /* Update register status */ > + s->regs[IBEX_SPI_HOST_STATUS] = data; > break; > case IBEX_SPI_HOST_ERROR_ENABLE: > s->regs[addr] = val32; > -- Regards, Bin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/ssi: fixup coverity issue 2022-08-11 2:55 ` Bin Meng @ 2022-08-12 2:23 ` Wilfred Mallawa 0 siblings, 0 replies; 11+ messages in thread From: Wilfred Mallawa @ 2022-08-12 2:23 UTC (permalink / raw) To: wilfred.mallawa@opensource.wdc.com, bmeng.cn@gmail.com Cc: qemu-riscv@nongnu.org, Alistair Francis, qemu-devel@nongnu.org On Thu, 2022-08-11 at 10:55 +0800, Bin Meng wrote: > On Thu, Aug 11, 2022 at 8:58 AM Wilfred Mallawa > <wilfred.mallawa@opensource.wdc.com> wrote: > > > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > > > This patch addresses the coverity issues specified in [1], > > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > > implemented to clean up the code. > > > > Additionally, the `EVENT_ENABLE` register is correctly updated > > to addr of `0x34`. > > > > [1] > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > > > Fixes: Coverity CID 1488107 > > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > nits: please add "ibex_spi" to the tag, like hw/ssi: ibex_spi: will add in v2 :) > > > --- > > hw/ssi/ibex_spi_host.c | 141 +++++++++++++++++++++++-------------- > > ---- > > 1 file changed, 78 insertions(+), 63 deletions(-) > > > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > > index 601041d719..8c35bfa95f 100644 > > --- a/hw/ssi/ibex_spi_host.c > > +++ b/hw/ssi/ibex_spi_host.c > > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > > -REG32(EVENT_ENABLE, 0x30) > > +REG32(EVENT_ENABLE, 0x34) > > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > > FIELD(EVENT_ENABLE, RXWM, 2, 1) > > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t > > dividend) > > > > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) > > { > > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Empty the RX FIFO and assert RXEMPTY */ > > fifo8_reset(&s->rx_fifo); > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > } > > > > static void ibex_spi_txfifo_reset(IbexSPIHostState *s) > > { > > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Empty the TX FIFO and assert TXEMPTY */ > > fifo8_reset(&s->tx_fifo); > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > } > > > > static void ibex_spi_host_reset(DeviceState *dev) > > @@ -162,37 +164,41 @@ static void ibex_spi_host_reset(DeviceState > > *dev) > > */ > > static void ibex_spi_host_irq(IbexSPIHostState *s) > > { > > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > > - & R_INTR_ENABLE_ERROR_MASK; > > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > > - & R_INTR_ENABLE_SPI_EVENT_MASK; > > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > > - & R_INTR_STATE_ERROR_MASK; > > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > > - & R_INTR_STATE_SPI_EVENT_MASK; > > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > + INTR_ENABLE, ERROR); > > + > > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > + INTR_ENABLE, SPI_EVENT); > > + > > + bool err_pending = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_INTR_STATE], > > + INTR_STATE, ERROR); > > + > > + bool status_pending = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_INTR_STATE], > > + INTR_STATE, SPI_EVENT); > > + > > int err_irq = 0, event_irq = 0; > > > > /* Error IRQ enabled and Error IRQ Cleared */ > > if (error_en && !err_pending) { > > /* Event enabled, Interrupt Test Error */ > > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & > > R_INTR_TEST_ERROR_MASK) { > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > > INTR_TEST, ERROR)) { > > err_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > - & R_ERROR_ENABLE_CMDBUSY_MASK) && > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > - & R_ERROR_STATUS_CMDBUSY_MASK) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CMDBUSY) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + ERROR_STATUS, CMDBUSY)) { > > /* Wrote to COMMAND when not READY */ > > err_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > - & R_ERROR_ENABLE_CMDINVAL_MASK) && > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > - & R_ERROR_STATUS_CMDINVAL_MASK) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CMDINVAL) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + ERROR_STATUS, CMDINVAL)) { > > /* Invalid command segment */ > > err_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > - & R_ERROR_ENABLE_CSIDINVAL_MASK) && > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > - & R_ERROR_STATUS_CSIDINVAL_MASK) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CSIDINVAL) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + ERROR_STATUS, CSIDINVAL)) { > > /* Invalid value for CSID */ > > err_irq = 1; > > } > > @@ -204,22 +210,26 @@ static void > > ibex_spi_host_irq(IbexSPIHostState *s) > > > > /* Event IRQ Enabled and Event IRQ Cleared */ > > if (event_en && !status_pending) { > > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & > > R_INTR_TEST_SPI_EVENT_MASK) { > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > > + INTR_STATE, SPI_EVENT)) { > > /* Event enabled, Interrupt Test Event */ > > event_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > - & R_EVENT_ENABLE_READY_MASK) && > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_READY_MASK)) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > + EVENT_ENABLE, READY) && > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, READY)) { > > /* SPI Host ready for next command */ > > event_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > - & R_EVENT_ENABLE_TXEMPTY_MASK) && > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_TXEMPTY_MASK)) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > + EVENT_ENABLE, TXEMPTY) && > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, TXEMPTY)) { > > /* SPI TXEMPTY, TXFIFO drained */ > > event_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > - & R_EVENT_ENABLE_RXFULL_MASK) && > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_RXFULL_MASK)) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > + EVENT_ENABLE, RXFULL) && > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, RXFULL)) { > > /* SPI RXFULL, RXFIFO full */ > > event_irq = 1; > > } > > @@ -232,10 +242,11 @@ static void > > ibex_spi_host_irq(IbexSPIHostState *s) > > > > static void ibex_spi_host_transfer(IbexSPIHostState *s) > > { > > - uint32_t rx, tx; > > + uint32_t rx, tx, data; > > /* Get num of one byte transfers */ > > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & > > R_COMMAND_LEN_MASK) > > - >> R_COMMAND_LEN_SHIFT); > > + uint8_t segment_len = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_COMMAND], > > + COMMAND, LEN); > > + > > while (segment_len > 0) { > > if (fifo8_is_empty(&s->tx_fifo)) { > > /* Assert Stall */ > > @@ -262,22 +273,23 @@ static void > > ibex_spi_host_transfer(IbexSPIHostState *s) > > --segment_len; > > } > > > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Assert Ready */ > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > > + data = FIELD_DP32(data, STATUS, READY, 1); > > /* Set RXQD */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK > > - & div4_round_up(segment_len)); > > + data = FIELD_DP32(data, STATUS, RXQD, > > div4_round_up(segment_len)); > > /* Set TXQD */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) > > / 4) > > - & R_STATUS_TXQD_MASK; > > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s- > > >tx_fifo) / 4); > > /* Clear TXFULL */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > > - /* Assert TXEMPTY and drop remaining bytes that exceed > > segment_len */ > > - ibex_spi_txfifo_reset(s); > > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > > /* Reset RXEMPTY */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); > > + /* Set TXEMPTY */ > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > > + /* Drop remaining bytes that exceed segment_len */ > > + ibex_spi_txfifo_reset(s); > > + /* Update register status */ > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > > > ibex_spi_host_irq(s); > > } > > @@ -340,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > { > > IbexSPIHostState *s = opaque; > > uint32_t val32 = val64; > > - uint32_t shift_mask = 0xff; > > + uint32_t shift_mask = 0xff, data; > > uint8_t txqd_len; > > > > trace_ibex_spi_host_write(addr, size, val64); > > @@ -397,21 +409,23 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > s->regs[addr] = val32; > > > > /* STALL, IP not enabled */ > > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & > > R_CONTROL_SPIEN_MASK)) { > > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], > > + CONTROL, SPIEN))) { > > return; > > } > > > > /* SPI not ready, IRQ Error */ > > - if (!(s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_READY_MASK)) { > > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, READY))) { > > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= > > R_ERROR_STATUS_CMDBUSY_MASK; > > ibex_spi_host_irq(s); > > return; > > } > > + > > /* Assert Not Ready */ > > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; > > > > - if (((val32 & R_COMMAND_DIRECTION_MASK) >> > > R_COMMAND_DIRECTION_SHIFT) > > - != BIDIRECTIONAL_TRANSFER) { > > + if (FIELD_EX32(val32, COMMAND, DIRECTION) != > > BIDIRECTIONAL_TRANSFER) { > > qemu_log_mask(LOG_UNIMP, > > "%s: Rx Only/Tx Only are not > > supported\n", __func__); > > } > > @@ -452,8 +466,8 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > return; > > } > > /* Byte ordering is set by the IP */ > > - if ((s->regs[IBEX_SPI_HOST_STATUS] & > > - R_STATUS_BYTEORDER_MASK) == 0) { > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, > > + BYTEORDER) == 0) { > > /* LE: LSB transmitted first (default for ibex > > processor) */ > > shift_mask = 0xff << (i * 8); > > } else { > > @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * > > 8)); > > } > > > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Reset TXEMPTY */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 0); > > /* Update TXQD */ > > - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] & > > - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT; > > + txqd_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > STATUS, TXQD); > > /* Partial bytes (size < 4) are padded, in words. */ > > txqd_len += 1; > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len; > > + data = FIELD_DP32(data, STATUS, TXQD, txqd_len); > > /* Assert Ready */ > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > > + data = FIELD_DP32(data, STATUS, READY, 1); > > + /* Update register status */ > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > break; > > case IBEX_SPI_HOST_ERROR_ENABLE: > > s->regs[addr] = val32; > > -- > > Regards, > Bin Thanks, Wilfred ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/ssi: fixup coverity issue 2022-08-10 23:02 ` [PATCH 2/3] hw/ssi: fixup coverity issue Wilfred Mallawa 2022-08-11 2:55 ` Bin Meng @ 2022-08-11 14:23 ` Andrew Jones 2022-08-12 2:21 ` Wilfred Mallawa 1 sibling, 1 reply; 11+ messages in thread From: Andrew Jones @ 2022-08-11 14:23 UTC (permalink / raw) To: Wilfred Mallawa; +Cc: Alistair.Francis, qemu-riscv, qemu-devel, Wilfred Mallawa On Thu, Aug 11, 2022 at 09:02:00AM +1000, Wilfred Mallawa wrote: > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > This patch addresses the coverity issues specified in [1], > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > implemented to clean up the code. > > Additionally, the `EVENT_ENABLE` register is correctly updated > to addr of `0x34`. This sounds like separate patch material. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > Fixes: Coverity CID 1488107 > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > --- > hw/ssi/ibex_spi_host.c | 141 +++++++++++++++++++++++------------------ > 1 file changed, 78 insertions(+), 63 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 601041d719..8c35bfa95f 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > -REG32(EVENT_ENABLE, 0x30) > +REG32(EVENT_ENABLE, 0x34) > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > FIELD(EVENT_ENABLE, RXWM, 2, 1) > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t dividend) > > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) > { > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Empty the RX FIFO and assert RXEMPTY */ > fifo8_reset(&s->rx_fifo); > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); > + s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_txfifo_reset(IbexSPIHostState *s) > { > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Empty the TX FIFO and assert TXEMPTY */ > fifo8_reset(&s->tx_fifo); > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > + s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_host_reset(DeviceState *dev) > @@ -162,37 +164,41 @@ static void ibex_spi_host_reset(DeviceState *dev) > */ > static void ibex_spi_host_irq(IbexSPIHostState *s) > { > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > - & R_INTR_ENABLE_ERROR_MASK; > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > - & R_INTR_ENABLE_SPI_EVENT_MASK; > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > - & R_INTR_STATE_ERROR_MASK; > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > - & R_INTR_STATE_SPI_EVENT_MASK; > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, ERROR); > + > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, SPI_EVENT); > + > + bool err_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, ERROR); > + > + bool status_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, SPI_EVENT); uint32_t reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE]; bool error_en = FIELD_EX32(reg, INTR_ENABLE, ERROR); bool event_en = FIELD_EX32(reg, INTR_ENABLE, SPI_EVENT); ... would like nicer, IMHO. > + > int err_irq = 0, event_irq = 0; > > /* Error IRQ enabled and Error IRQ Cleared */ > if (error_en && !err_pending) { > /* Event enabled, Interrupt Test Error */ > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) { > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], INTR_TEST, ERROR)) { > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CMDBUSY_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CMDBUSY_MASK) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDBUSY) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CMDBUSY)) { > /* Wrote to COMMAND when not READY */ > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CMDINVAL_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CMDINVAL_MASK) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDINVAL) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CMDINVAL)) { > /* Invalid command segment */ > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CSIDINVAL_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CSIDINVAL_MASK) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CSIDINVAL) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CSIDINVAL)) { Same comment as above. If we set local variables for IBEX_SPI_HOST_ERROR_ENABLE and IBEX_SPI_HOST_ERROR_STATUS at the top of the ladder, then it should be easier to read. > /* Invalid value for CSID */ > err_irq = 1; > } > @@ -204,22 +210,26 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > > /* Event IRQ Enabled and Event IRQ Cleared */ > if (event_en && !status_pending) { > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_SPI_EVENT_MASK) { > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > + INTR_STATE, SPI_EVENT)) { > /* Event enabled, Interrupt Test Event */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_READY_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > + EVENT_ENABLE, READY) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, READY)) { > /* SPI Host ready for next command */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_TXEMPTY_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_TXEMPTY_MASK)) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > + EVENT_ENABLE, TXEMPTY) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, TXEMPTY)) { > /* SPI TXEMPTY, TXFIFO drained */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_RXFULL_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_RXFULL_MASK)) { > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > + EVENT_ENABLE, RXFULL) && > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, RXFULL)) { same comment > /* SPI RXFULL, RXFIFO full */ > event_irq = 1; > } > @@ -232,10 +242,11 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > > static void ibex_spi_host_transfer(IbexSPIHostState *s) > { > - uint32_t rx, tx; > + uint32_t rx, tx, data; > /* Get num of one byte transfers */ > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & R_COMMAND_LEN_MASK) > - >> R_COMMAND_LEN_SHIFT); > + uint8_t segment_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_COMMAND], > + COMMAND, LEN); > + > while (segment_len > 0) { > if (fifo8_is_empty(&s->tx_fifo)) { > /* Assert Stall */ > @@ -262,22 +273,23 @@ static void ibex_spi_host_transfer(IbexSPIHostState *s) > --segment_len; > } > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Assert Ready */ > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > + data = FIELD_DP32(data, STATUS, READY, 1); > /* Set RXQD */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK > - & div4_round_up(segment_len)); > + data = FIELD_DP32(data, STATUS, RXQD, div4_round_up(segment_len)); > /* Set TXQD */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) / 4) > - & R_STATUS_TXQD_MASK; > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s->tx_fifo) / 4); > /* Clear TXFULL */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > - /* Assert TXEMPTY and drop remaining bytes that exceed segment_len */ > - ibex_spi_txfifo_reset(s); > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > /* Reset RXEMPTY */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); > + /* Set TXEMPTY */ > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); Why add this here? This is still done in ibex_spi_txfifo_reset() > + /* Drop remaining bytes that exceed segment_len */ > + ibex_spi_txfifo_reset(s); > + /* Update register status */ > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > ibex_spi_host_irq(s); > } > @@ -340,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > { > IbexSPIHostState *s = opaque; > uint32_t val32 = val64; > - uint32_t shift_mask = 0xff; > + uint32_t shift_mask = 0xff, data; > uint8_t txqd_len; > > trace_ibex_spi_host_write(addr, size, val64); > @@ -397,21 +409,23 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > s->regs[addr] = val32; > > /* STALL, IP not enabled */ > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & R_CONTROL_SPIEN_MASK)) { > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], > + CONTROL, SPIEN))) { > return; > } > > /* SPI not ready, IRQ Error */ > - if (!(s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, READY))) { > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= R_ERROR_STATUS_CMDBUSY_MASK; > ibex_spi_host_irq(s); > return; > } > + > /* Assert Not Ready */ > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; > > - if (((val32 & R_COMMAND_DIRECTION_MASK) >> R_COMMAND_DIRECTION_SHIFT) > - != BIDIRECTIONAL_TRANSFER) { > + if (FIELD_EX32(val32, COMMAND, DIRECTION) != BIDIRECTIONAL_TRANSFER) { > qemu_log_mask(LOG_UNIMP, > "%s: Rx Only/Tx Only are not supported\n", __func__); > } > @@ -452,8 +466,8 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > return; > } > /* Byte ordering is set by the IP */ > - if ((s->regs[IBEX_SPI_HOST_STATUS] & > - R_STATUS_BYTEORDER_MASK) == 0) { > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, > + BYTEORDER) == 0) { Can use local variable for IBEX_SPI_HOST_STATUS to avoid these three wrapped if conditions > /* LE: LSB transmitted first (default for ibex processor) */ > shift_mask = 0xff << (i * 8); > } else { > @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * 8)); > } > > + data = s->regs[IBEX_SPI_HOST_STATUS]; Could probably change the name of data to status or whatever and use it all the way through. > /* Reset TXEMPTY */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, TXEMPTY, 0); > /* Update TXQD */ > - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] & > - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT; > + txqd_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, TXQD); > /* Partial bytes (size < 4) are padded, in words. */ > txqd_len += 1; > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len; > + data = FIELD_DP32(data, STATUS, TXQD, txqd_len); > /* Assert Ready */ > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > + data = FIELD_DP32(data, STATUS, READY, 1); > + /* Update register status */ > + s->regs[IBEX_SPI_HOST_STATUS] = data; > break; > case IBEX_SPI_HOST_ERROR_ENABLE: > s->regs[addr] = val32; > -- > 2.37.1 > > Thanks, drew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/ssi: fixup coverity issue 2022-08-11 14:23 ` Andrew Jones @ 2022-08-12 2:21 ` Wilfred Mallawa 2022-08-12 9:21 ` Andrew Jones 0 siblings, 1 reply; 11+ messages in thread From: Wilfred Mallawa @ 2022-08-12 2:21 UTC (permalink / raw) To: wilfred.mallawa@opensource.wdc.com, ajones@ventanamicro.com Cc: qemu-riscv@nongnu.org, Alistair Francis, qemu-devel@nongnu.org On Thu, 2022-08-11 at 16:23 +0200, Andrew Jones wrote: > On Thu, Aug 11, 2022 at 09:02:00AM +1000, Wilfred Mallawa wrote: > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > > > This patch addresses the coverity issues specified in [1], > > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > > implemented to clean up the code. > > > > Additionally, the `EVENT_ENABLE` register is correctly updated > > to addr of `0x34`. > > This sounds like separate patch material. > > > > > [1] > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > > > Fixes: Coverity CID 1488107 > > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > --- > > hw/ssi/ibex_spi_host.c | 141 +++++++++++++++++++++++-------------- > > ---- > > 1 file changed, 78 insertions(+), 63 deletions(-) > > > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > > index 601041d719..8c35bfa95f 100644 > > --- a/hw/ssi/ibex_spi_host.c > > +++ b/hw/ssi/ibex_spi_host.c > > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > > -REG32(EVENT_ENABLE, 0x30) > > +REG32(EVENT_ENABLE, 0x34) > > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > > FIELD(EVENT_ENABLE, RXWM, 2, 1) > > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t > > dividend) > > > > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) > > { > > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Empty the RX FIFO and assert RXEMPTY */ > > fifo8_reset(&s->rx_fifo); > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > } > > > > static void ibex_spi_txfifo_reset(IbexSPIHostState *s) > > { > > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Empty the TX FIFO and assert TXEMPTY */ > > fifo8_reset(&s->tx_fifo); > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > } > > > > static void ibex_spi_host_reset(DeviceState *dev) > > @@ -162,37 +164,41 @@ static void ibex_spi_host_reset(DeviceState > > *dev) > > */ > > static void ibex_spi_host_irq(IbexSPIHostState *s) > > { > > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > > - & R_INTR_ENABLE_ERROR_MASK; > > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > > - & R_INTR_ENABLE_SPI_EVENT_MASK; > > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > > - & R_INTR_STATE_ERROR_MASK; > > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > > - & R_INTR_STATE_SPI_EVENT_MASK; > > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > + INTR_ENABLE, ERROR); > > + > > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > + INTR_ENABLE, SPI_EVENT); > > + > > + bool err_pending = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_INTR_STATE], > > + INTR_STATE, ERROR); > > + > > + bool status_pending = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_INTR_STATE], > > + INTR_STATE, SPI_EVENT); > > uint32_t reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE]; > bool error_en = FIELD_EX32(reg, INTR_ENABLE, ERROR); > bool event_en = FIELD_EX32(reg, INTR_ENABLE, SPI_EVENT); > ... > > would like nicer, IMHO. as per the comment below, do you mean to make all the conditions variables here? and substitute those for the if statements instead of the `FIELD_..` macros? > > > > + > > int err_irq = 0, event_irq = 0; > > > > /* Error IRQ enabled and Error IRQ Cleared */ > > if (error_en && !err_pending) { > > /* Event enabled, Interrupt Test Error */ > > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & > > R_INTR_TEST_ERROR_MASK) { > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > > INTR_TEST, ERROR)) { > > err_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > - & R_ERROR_ENABLE_CMDBUSY_MASK) && > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > - & R_ERROR_STATUS_CMDBUSY_MASK) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CMDBUSY) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + ERROR_STATUS, CMDBUSY)) { > > /* Wrote to COMMAND when not READY */ > > err_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > - & R_ERROR_ENABLE_CMDINVAL_MASK) && > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > - & R_ERROR_STATUS_CMDINVAL_MASK) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CMDINVAL) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + ERROR_STATUS, CMDINVAL)) { > > /* Invalid command segment */ > > err_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > - & R_ERROR_ENABLE_CSIDINVAL_MASK) && > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > - & R_ERROR_STATUS_CSIDINVAL_MASK) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CSIDINVAL) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + ERROR_STATUS, CSIDINVAL)) { > > Same comment as above. If we set local variables for > IBEX_SPI_HOST_ERROR_ENABLE and IBEX_SPI_HOST_ERROR_STATUS at the top > of > the ladder, then it should be easier to read. > > > > /* Invalid value for CSID */ > > err_irq = 1; > > } > > @@ -204,22 +210,26 @@ static void > > ibex_spi_host_irq(IbexSPIHostState *s) > > > > /* Event IRQ Enabled and Event IRQ Cleared */ > > if (event_en && !status_pending) { > > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & > > R_INTR_TEST_SPI_EVENT_MASK) { > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > > + INTR_STATE, SPI_EVENT)) { > > /* Event enabled, Interrupt Test Event */ > > event_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > - & R_EVENT_ENABLE_READY_MASK) && > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_READY_MASK)) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > + EVENT_ENABLE, READY) && > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, READY)) { > > /* SPI Host ready for next command */ > > event_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > - & R_EVENT_ENABLE_TXEMPTY_MASK) && > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_TXEMPTY_MASK)) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > + EVENT_ENABLE, TXEMPTY) && > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, TXEMPTY)) { > > /* SPI TXEMPTY, TXFIFO drained */ > > event_irq = 1; > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > - & R_EVENT_ENABLE_RXFULL_MASK) && > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_RXFULL_MASK)) { > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > + EVENT_ENABLE, RXFULL) && > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, RXFULL)) { > > same comment > > > /* SPI RXFULL, RXFIFO full */ > > event_irq = 1; > > } > > @@ -232,10 +242,11 @@ static void > > ibex_spi_host_irq(IbexSPIHostState *s) > > > > static void ibex_spi_host_transfer(IbexSPIHostState *s) > > { > > - uint32_t rx, tx; > > + uint32_t rx, tx, data; > > /* Get num of one byte transfers */ > > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & > > R_COMMAND_LEN_MASK) > > - >> R_COMMAND_LEN_SHIFT); > > + uint8_t segment_len = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_COMMAND], > > + COMMAND, LEN); > > + > > while (segment_len > 0) { > > if (fifo8_is_empty(&s->tx_fifo)) { > > /* Assert Stall */ > > @@ -262,22 +273,23 @@ static void > > ibex_spi_host_transfer(IbexSPIHostState *s) > > --segment_len; > > } > > > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > > /* Assert Ready */ > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > > + data = FIELD_DP32(data, STATUS, READY, 1); > > /* Set RXQD */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK > > - & div4_round_up(segment_len)); > > + data = FIELD_DP32(data, STATUS, RXQD, > > div4_round_up(segment_len)); > > /* Set TXQD */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) > > / 4) > > - & R_STATUS_TXQD_MASK; > > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s- > > >tx_fifo) / 4); > > /* Clear TXFULL */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > > - /* Assert TXEMPTY and drop remaining bytes that exceed > > segment_len */ > > - ibex_spi_txfifo_reset(s); > > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > > /* Reset RXEMPTY */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); > > + /* Set TXEMPTY */ > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > > Why add this here? This is still done in ibex_spi_txfifo_reset() good catch! will fixup. > > > + /* Drop remaining bytes that exceed segment_len */ > > + ibex_spi_txfifo_reset(s); > > + /* Update register status */ > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > > > ibex_spi_host_irq(s); > > } > > @@ -340,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > { > > IbexSPIHostState *s = opaque; > > uint32_t val32 = val64; > > - uint32_t shift_mask = 0xff; > > + uint32_t shift_mask = 0xff, data; > > uint8_t txqd_len; > > > > trace_ibex_spi_host_write(addr, size, val64); > > @@ -397,21 +409,23 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > s->regs[addr] = val32; > > > > /* STALL, IP not enabled */ > > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & > > R_CONTROL_SPIEN_MASK)) { > > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], > > + CONTROL, SPIEN))) { > > return; > > } > > > > /* SPI not ready, IRQ Error */ > > - if (!(s->regs[IBEX_SPI_HOST_STATUS] & > > R_STATUS_READY_MASK)) { > > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > + STATUS, READY))) { > > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= > > R_ERROR_STATUS_CMDBUSY_MASK; > > ibex_spi_host_irq(s); > > return; > > } > > + > > /* Assert Not Ready */ > > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; > > > > - if (((val32 & R_COMMAND_DIRECTION_MASK) >> > > R_COMMAND_DIRECTION_SHIFT) > > - != BIDIRECTIONAL_TRANSFER) { > > + if (FIELD_EX32(val32, COMMAND, DIRECTION) != > > BIDIRECTIONAL_TRANSFER) { > > qemu_log_mask(LOG_UNIMP, > > "%s: Rx Only/Tx Only are not > > supported\n", __func__); > > } > > @@ -452,8 +466,8 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > return; > > } > > /* Byte ordering is set by the IP */ > > - if ((s->regs[IBEX_SPI_HOST_STATUS] & > > - R_STATUS_BYTEORDER_MASK) == 0) { > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, > > + BYTEORDER) == 0) { > > Can use local variable for IBEX_SPI_HOST_STATUS to avoid these three > wrapped > if conditions Not sure if I'm following, do you mean to use a local var that holds the condition? something like: bool byte_order_le = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, BYTEORDER) == 0; if (bo_le) {..} else {...} In which case we would still need a conditional to check for byteorder? > > > /* LE: LSB transmitted first (default for ibex > > processor) */ > > shift_mask = 0xff << (i * 8); > > } else { > > @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * > > 8)); > > } > > > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > > Could probably change the name of data to status or whatever and use > it > all the way through. `data` here is ambiguous as it is used for other registers at the top of the function and not just the status ones. We could create a new var `status` just for this bottom part...if it helps readibility? > > > /* Reset TXEMPTY */ > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK; > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 0); > > /* Update TXQD */ > > - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] & > > - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT; > > + txqd_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > STATUS, TXQD); > > /* Partial bytes (size < 4) are padded, in words. */ > > txqd_len += 1; > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > > - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len; > > + data = FIELD_DP32(data, STATUS, TXQD, txqd_len); > > /* Assert Ready */ > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > > + data = FIELD_DP32(data, STATUS, READY, 1); > > + /* Update register status */ > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > break; > > case IBEX_SPI_HOST_ERROR_ENABLE: > > s->regs[addr] = val32; > > -- > > 2.37.1 > > > > > > Thanks, > drew Thanks! Wilfred ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/ssi: fixup coverity issue 2022-08-12 2:21 ` Wilfred Mallawa @ 2022-08-12 9:21 ` Andrew Jones 0 siblings, 0 replies; 11+ messages in thread From: Andrew Jones @ 2022-08-12 9:21 UTC (permalink / raw) To: Wilfred Mallawa Cc: wilfred.mallawa@opensource.wdc.com, qemu-riscv@nongnu.org, Alistair Francis, qemu-devel@nongnu.org On Fri, Aug 12, 2022 at 02:21:40AM +0000, Wilfred Mallawa wrote: > On Thu, 2022-08-11 at 16:23 +0200, Andrew Jones wrote: > > On Thu, Aug 11, 2022 at 09:02:00AM +1000, Wilfred Mallawa wrote: > > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > > > > > This patch addresses the coverity issues specified in [1], > > > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > > > implemented to clean up the code. > > > > > > Additionally, the `EVENT_ENABLE` register is correctly updated > > > to addr of `0x34`. > > > > This sounds like separate patch material. > > > > > > > > [1] > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > > > > > Fixes: Coverity CID 1488107 > > > > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > > --- > > > hw/ssi/ibex_spi_host.c | 141 +++++++++++++++++++++++-------------- > > > ---- > > > 1 file changed, 78 insertions(+), 63 deletions(-) > > > > > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > > > index 601041d719..8c35bfa95f 100644 > > > --- a/hw/ssi/ibex_spi_host.c > > > +++ b/hw/ssi/ibex_spi_host.c > > > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > > > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > > > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > > > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > > > -REG32(EVENT_ENABLE, 0x30) > > > +REG32(EVENT_ENABLE, 0x34) > > > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > > > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > > > FIELD(EVENT_ENABLE, RXWM, 2, 1) > > > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t > > > dividend) > > > > > > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) > > > { > > > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > > > /* Empty the RX FIFO and assert RXEMPTY */ > > > fifo8_reset(&s->rx_fifo); > > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; > > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; > > > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); > > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > > } > > > > > > static void ibex_spi_txfifo_reset(IbexSPIHostState *s) > > > { > > > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > > > /* Empty the TX FIFO and assert TXEMPTY */ > > > fifo8_reset(&s->tx_fifo); > > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; > > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > > } > > > > > > static void ibex_spi_host_reset(DeviceState *dev) > > > @@ -162,37 +164,41 @@ static void ibex_spi_host_reset(DeviceState > > > *dev) > > > */ > > > static void ibex_spi_host_irq(IbexSPIHostState *s) > > > { > > > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > > > - & R_INTR_ENABLE_ERROR_MASK; > > > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > > > - & R_INTR_ENABLE_SPI_EVENT_MASK; > > > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > > > - & R_INTR_STATE_ERROR_MASK; > > > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > > > - & R_INTR_STATE_SPI_EVENT_MASK; > > > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > > + INTR_ENABLE, ERROR); > > > + > > > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > > + INTR_ENABLE, SPI_EVENT); > > > + > > > + bool err_pending = FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_INTR_STATE], > > > + INTR_STATE, ERROR); > > > + > > > + bool status_pending = FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_INTR_STATE], > > > + INTR_STATE, SPI_EVENT); > > > > uint32_t reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE]; > > bool error_en = FIELD_EX32(reg, INTR_ENABLE, ERROR); > > bool event_en = FIELD_EX32(reg, INTR_ENABLE, SPI_EVENT); > > ... > > > > would like nicer, IMHO. > as per the comment below, do you mean to make all the conditions > variables here? and substitute those for the if statements instead of > the `FIELD_..` macros? For the above code, the suggestion I gave is what I'm thinking. For the below code, keep the FIELD macros in place, but shrink the length of the line by doing uint32_t enable = s->regs[IBEX_SPI_HOST_ERROR_ENABLE]; uint32_t status = s->regs[IBEX_SPI_HOST_ERROR_STATUS]; at the top, and then } else if (FIELD_EX32(enable, ERROR_ENABLE, CMDBUSY) && FIELD_EX32(status, ERROR_STATUS, CMDBUSY)) { ... > > > > > > > + > > > int err_irq = 0, event_irq = 0; > > > > > > /* Error IRQ enabled and Error IRQ Cleared */ > > > if (error_en && !err_pending) { > > > /* Event enabled, Interrupt Test Error */ > > > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & > > > R_INTR_TEST_ERROR_MASK) { > > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > > > INTR_TEST, ERROR)) { > > > err_irq = 1; > > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > > - & R_ERROR_ENABLE_CMDBUSY_MASK) && > > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > > - & R_ERROR_STATUS_CMDBUSY_MASK) { > > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > > + ERROR_ENABLE, CMDBUSY) && > > > + FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > > + ERROR_STATUS, CMDBUSY)) { > > > /* Wrote to COMMAND when not READY */ > > > err_irq = 1; > > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > > - & R_ERROR_ENABLE_CMDINVAL_MASK) && > > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > > - & R_ERROR_STATUS_CMDINVAL_MASK) { > > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > > + ERROR_ENABLE, CMDINVAL) && > > > + FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > > + ERROR_STATUS, CMDINVAL)) { > > > /* Invalid command segment */ > > > err_irq = 1; > > > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > > > - & R_ERROR_ENABLE_CSIDINVAL_MASK) && > > > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > > > - & R_ERROR_STATUS_CSIDINVAL_MASK) { > > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > > + ERROR_ENABLE, CSIDINVAL) && > > > + FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > > + ERROR_STATUS, CSIDINVAL)) { > > > > Same comment as above. If we set local variables for > > IBEX_SPI_HOST_ERROR_ENABLE and IBEX_SPI_HOST_ERROR_STATUS at the top > > of > > the ladder, then it should be easier to read. > > > > > > > /* Invalid value for CSID */ > > > err_irq = 1; > > > } > > > @@ -204,22 +210,26 @@ static void > > > ibex_spi_host_irq(IbexSPIHostState *s) > > > > > > /* Event IRQ Enabled and Event IRQ Cleared */ > > > if (event_en && !status_pending) { > > > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & > > > R_INTR_TEST_SPI_EVENT_MASK) { > > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_TEST], > > > + INTR_STATE, SPI_EVENT)) { > > > /* Event enabled, Interrupt Test Event */ > > > event_irq = 1; > > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > > - & R_EVENT_ENABLE_READY_MASK) && > > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > > R_STATUS_READY_MASK)) { > > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > > + EVENT_ENABLE, READY) && > > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > > + STATUS, READY)) { > > > /* SPI Host ready for next command */ > > > event_irq = 1; > > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > > - & R_EVENT_ENABLE_TXEMPTY_MASK) && > > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > > R_STATUS_TXEMPTY_MASK)) { > > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > > + EVENT_ENABLE, TXEMPTY) && > > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > > + STATUS, TXEMPTY)) { > > > /* SPI TXEMPTY, TXFIFO drained */ > > > event_irq = 1; > > > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > > > - & R_EVENT_ENABLE_RXFULL_MASK) && > > > - (s->regs[IBEX_SPI_HOST_STATUS] & > > > R_STATUS_RXFULL_MASK)) { > > > + } else if (FIELD_EX32(s->regs[IBEX_SPI_HOST_EVENT_ENABLE], > > > + EVENT_ENABLE, RXFULL) && > > > + FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > > + STATUS, RXFULL)) { > > > > same comment > > > > > /* SPI RXFULL, RXFIFO full */ > > > event_irq = 1; > > > } > > > @@ -232,10 +242,11 @@ static void > > > ibex_spi_host_irq(IbexSPIHostState *s) > > > > > > static void ibex_spi_host_transfer(IbexSPIHostState *s) > > > { > > > - uint32_t rx, tx; > > > + uint32_t rx, tx, data; > > > /* Get num of one byte transfers */ > > > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & > > > R_COMMAND_LEN_MASK) > > > - >> R_COMMAND_LEN_SHIFT); > > > + uint8_t segment_len = FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_COMMAND], > > > + COMMAND, LEN); > > > + > > > while (segment_len > 0) { > > > if (fifo8_is_empty(&s->tx_fifo)) { > > > /* Assert Stall */ > > > @@ -262,22 +273,23 @@ static void > > > ibex_spi_host_transfer(IbexSPIHostState *s) > > > --segment_len; > > > } > > > > > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > > > /* Assert Ready */ > > > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > > > + data = FIELD_DP32(data, STATUS, READY, 1); > > > /* Set RXQD */ > > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; > > > - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK > > > - & div4_round_up(segment_len)); > > > + data = FIELD_DP32(data, STATUS, RXQD, > > > div4_round_up(segment_len)); > > > /* Set TXQD */ > > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > > > - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) > > > / 4) > > > - & R_STATUS_TXQD_MASK; > > > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s- > > > >tx_fifo) / 4); > > > /* Clear TXFULL */ > > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > > > - /* Assert TXEMPTY and drop remaining bytes that exceed > > > segment_len */ > > > - ibex_spi_txfifo_reset(s); > > > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > > > /* Reset RXEMPTY */ > > > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; > > > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); > > > + /* Set TXEMPTY */ > > > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > > > > Why add this here? This is still done in ibex_spi_txfifo_reset() > good catch! will fixup. > > > > > + /* Drop remaining bytes that exceed segment_len */ > > > + ibex_spi_txfifo_reset(s); > > > + /* Update register status */ > > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > > > > > ibex_spi_host_irq(s); > > > } > > > @@ -340,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, > > > hwaddr addr, > > > { > > > IbexSPIHostState *s = opaque; > > > uint32_t val32 = val64; > > > - uint32_t shift_mask = 0xff; > > > + uint32_t shift_mask = 0xff, data; > > > uint8_t txqd_len; > > > > > > trace_ibex_spi_host_write(addr, size, val64); > > > @@ -397,21 +409,23 @@ static void ibex_spi_host_write(void *opaque, > > > hwaddr addr, > > > s->regs[addr] = val32; > > > > > > /* STALL, IP not enabled */ > > > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & > > > R_CONTROL_SPIEN_MASK)) { > > > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], > > > + CONTROL, SPIEN))) { > > > return; > > > } > > > > > > /* SPI not ready, IRQ Error */ > > > - if (!(s->regs[IBEX_SPI_HOST_STATUS] & > > > R_STATUS_READY_MASK)) { > > > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > > > + STATUS, READY))) { > > > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= > > > R_ERROR_STATUS_CMDBUSY_MASK; > > > ibex_spi_host_irq(s); > > > return; > > > } > > > + > > > /* Assert Not Ready */ > > > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; > > > > > > - if (((val32 & R_COMMAND_DIRECTION_MASK) >> > > > R_COMMAND_DIRECTION_SHIFT) > > > - != BIDIRECTIONAL_TRANSFER) { > > > + if (FIELD_EX32(val32, COMMAND, DIRECTION) != > > > BIDIRECTIONAL_TRANSFER) { > > > qemu_log_mask(LOG_UNIMP, > > > "%s: Rx Only/Tx Only are not > > > supported\n", __func__); > > > } > > > @@ -452,8 +466,8 @@ static void ibex_spi_host_write(void *opaque, > > > hwaddr addr, > > > return; > > > } > > > /* Byte ordering is set by the IP */ > > > - if ((s->regs[IBEX_SPI_HOST_STATUS] & > > > - R_STATUS_BYTEORDER_MASK) == 0) { > > > + if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS, > > > + BYTEORDER) == 0) { > > > > Can use local variable for IBEX_SPI_HOST_STATUS to avoid these three > > wrapped > > if conditions > Not sure if I'm following, do you mean to use a local var that holds > the condition? something like: > > bool byte_order_le = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > STATUS, BYTEORDER) == 0; > if (bo_le) {..} else {...} > > In which case we would still need a conditional to check for > byteorder? No, I meant the same thing as above. Basically just make the lines shorter by putting the long 's->regs[...]' into a variable. > > > > > /* LE: LSB transmitted first (default for ibex > > > processor) */ > > > shift_mask = 0xff << (i * 8); > > > } else { > > > @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque, > > > hwaddr addr, > > > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * > > > 8)); > > > } > > > > > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > > > > Could probably change the name of data to status or whatever and use > > it > > all the way through. > `data` here is ambiguous as it is used for other registers at the top > of the function and not just the status ones. We could create a new > var `status` just for this bottom part...if it helps readibility? Yup, in summary, I suggest generously applying well-named local variables to wherever it makes sense in order to shorten the lines. Thanks, drew ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] hw/ssi: fixup/add rw1c functionality 2022-08-10 23:01 [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Wilfred Mallawa 2022-08-10 23:02 ` [PATCH 2/3] hw/ssi: fixup coverity issue Wilfred Mallawa @ 2022-08-10 23:02 ` Wilfred Mallawa 2022-08-14 21:56 ` Alistair Francis 2022-08-11 7:12 ` [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Alistair Francis 2022-08-11 14:24 ` Andrew Jones 3 siblings, 1 reply; 11+ messages in thread From: Wilfred Mallawa @ 2022-08-10 23:02 UTC (permalink / raw) To: Alistair.Francis, qemu-riscv; +Cc: qemu-devel, Wilfred Mallawa From: Wilfred Mallawa <wilfred.mallawa@wdc.com> This patch adds the `rw1c` functionality to the respective registers. The status fields are cleared when the respective field is set. Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> --- hw/ssi/ibex_spi_host.c | 36 +++++++++++++++++++++++++++++++--- include/hw/ssi/ibex_spi_host.h | 4 ++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c index 8c35bfa95f..935372506c 100644 --- a/hw/ssi/ibex_spi_host.c +++ b/hw/ssi/ibex_spi_host.c @@ -352,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, { IbexSPIHostState *s = opaque; uint32_t val32 = val64; - uint32_t shift_mask = 0xff, data; + uint32_t shift_mask = 0xff, data = 0; uint8_t txqd_len; trace_ibex_spi_host_write(addr, size, val64); @@ -362,7 +362,17 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, switch (addr) { /* Skipping any R/O registers */ - case IBEX_SPI_HOST_INTR_STATE...IBEX_SPI_HOST_INTR_ENABLE: + case IBEX_SPI_HOST_INTR_STATE: + /* rw1c status register */ + if (FIELD_EX32(val32, INTR_STATE, ERROR)) { + data = FIELD_DP32(data, INTR_STATE, ERROR, 0); + } + if (FIELD_EX32(val32, INTR_STATE, SPI_EVENT)) { + data = FIELD_DP32(data, INTR_STATE, SPI_EVENT, 0); + } + s->regs[addr] = data; + break; + case IBEX_SPI_HOST_INTR_ENABLE: s->regs[addr] = val32; break; case IBEX_SPI_HOST_INTR_TEST: @@ -506,7 +516,27 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, * When an error occurs, the corresponding bit must be cleared * here before issuing any further commands */ - s->regs[addr] = val32; + data = s->regs[addr]; + /* rw1c status register */ + if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) { + data = FIELD_DP32(data, ERROR_STATUS, CMDBUSY, 0); + } + if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) { + data = FIELD_DP32(data, ERROR_STATUS, OVERFLOW, 0); + } + if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) { + data = FIELD_DP32(data, ERROR_STATUS, UNDERFLOW, 0); + } + if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) { + data = FIELD_DP32(data, ERROR_STATUS, CMDINVAL, 0); + } + if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) { + data = FIELD_DP32(data, ERROR_STATUS, CSIDINVAL, 0); + } + if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) { + data = FIELD_DP32(data, ERROR_STATUS, ACCESSINVAL, 0); + } + s->regs[addr] = data; break; case IBEX_SPI_HOST_EVENT_ENABLE: /* Controls which classes of SPI events raise an interrupt. */ diff --git a/include/hw/ssi/ibex_spi_host.h b/include/hw/ssi/ibex_spi_host.h index 3fedcb6805..1f6d077766 100644 --- a/include/hw/ssi/ibex_spi_host.h +++ b/include/hw/ssi/ibex_spi_host.h @@ -40,7 +40,7 @@ OBJECT_CHECK(IbexSPIHostState, (obj), TYPE_IBEX_SPI_HOST) /* SPI Registers */ -#define IBEX_SPI_HOST_INTR_STATE (0x00 / 4) /* rw */ +#define IBEX_SPI_HOST_INTR_STATE (0x00 / 4) /* rw1c */ #define IBEX_SPI_HOST_INTR_ENABLE (0x04 / 4) /* rw */ #define IBEX_SPI_HOST_INTR_TEST (0x08 / 4) /* wo */ #define IBEX_SPI_HOST_ALERT_TEST (0x0c / 4) /* wo */ @@ -54,7 +54,7 @@ #define IBEX_SPI_HOST_TXDATA (0x28 / 4) #define IBEX_SPI_HOST_ERROR_ENABLE (0x2c / 4) /* rw */ -#define IBEX_SPI_HOST_ERROR_STATUS (0x30 / 4) /* rw */ +#define IBEX_SPI_HOST_ERROR_STATUS (0x30 / 4) /* rw1c */ #define IBEX_SPI_HOST_EVENT_ENABLE (0x34 / 4) /* rw */ /* FIFO Len in Bytes */ -- 2.37.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] hw/ssi: fixup/add rw1c functionality 2022-08-10 23:02 ` [PATCH 3/3] hw/ssi: fixup/add rw1c functionality Wilfred Mallawa @ 2022-08-14 21:56 ` Alistair Francis 0 siblings, 0 replies; 11+ messages in thread From: Alistair Francis @ 2022-08-14 21:56 UTC (permalink / raw) To: Wilfred Mallawa Cc: Alistair Francis, open list:RISC-V, qemu-devel@nongnu.org Developers, Wilfred Mallawa On Thu, Aug 11, 2022 at 11:05 AM Wilfred Mallawa <wilfred.mallawa@opensource.wdc.com> wrote: > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > This patch adds the `rw1c` functionality to the respective > registers. The status fields are cleared when the respective > field is set. > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/ssi/ibex_spi_host.c | 36 +++++++++++++++++++++++++++++++--- > include/hw/ssi/ibex_spi_host.h | 4 ++-- > 2 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 8c35bfa95f..935372506c 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -352,7 +352,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > { > IbexSPIHostState *s = opaque; > uint32_t val32 = val64; > - uint32_t shift_mask = 0xff, data; > + uint32_t shift_mask = 0xff, data = 0; > uint8_t txqd_len; > > trace_ibex_spi_host_write(addr, size, val64); > @@ -362,7 +362,17 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > > switch (addr) { > /* Skipping any R/O registers */ > - case IBEX_SPI_HOST_INTR_STATE...IBEX_SPI_HOST_INTR_ENABLE: > + case IBEX_SPI_HOST_INTR_STATE: > + /* rw1c status register */ > + if (FIELD_EX32(val32, INTR_STATE, ERROR)) { > + data = FIELD_DP32(data, INTR_STATE, ERROR, 0); > + } > + if (FIELD_EX32(val32, INTR_STATE, SPI_EVENT)) { > + data = FIELD_DP32(data, INTR_STATE, SPI_EVENT, 0); > + } > + s->regs[addr] = data; > + break; > + case IBEX_SPI_HOST_INTR_ENABLE: > s->regs[addr] = val32; > break; > case IBEX_SPI_HOST_INTR_TEST: > @@ -506,7 +516,27 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > * When an error occurs, the corresponding bit must be cleared > * here before issuing any further commands > */ > - s->regs[addr] = val32; > + data = s->regs[addr]; > + /* rw1c status register */ > + if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) { > + data = FIELD_DP32(data, ERROR_STATUS, CMDBUSY, 0); > + } > + if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) { > + data = FIELD_DP32(data, ERROR_STATUS, OVERFLOW, 0); > + } > + if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) { > + data = FIELD_DP32(data, ERROR_STATUS, UNDERFLOW, 0); > + } > + if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) { > + data = FIELD_DP32(data, ERROR_STATUS, CMDINVAL, 0); > + } > + if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) { > + data = FIELD_DP32(data, ERROR_STATUS, CSIDINVAL, 0); > + } > + if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) { > + data = FIELD_DP32(data, ERROR_STATUS, ACCESSINVAL, 0); > + } > + s->regs[addr] = data; > break; > case IBEX_SPI_HOST_EVENT_ENABLE: > /* Controls which classes of SPI events raise an interrupt. */ > diff --git a/include/hw/ssi/ibex_spi_host.h b/include/hw/ssi/ibex_spi_host.h > index 3fedcb6805..1f6d077766 100644 > --- a/include/hw/ssi/ibex_spi_host.h > +++ b/include/hw/ssi/ibex_spi_host.h > @@ -40,7 +40,7 @@ > OBJECT_CHECK(IbexSPIHostState, (obj), TYPE_IBEX_SPI_HOST) > > /* SPI Registers */ > -#define IBEX_SPI_HOST_INTR_STATE (0x00 / 4) /* rw */ > +#define IBEX_SPI_HOST_INTR_STATE (0x00 / 4) /* rw1c */ > #define IBEX_SPI_HOST_INTR_ENABLE (0x04 / 4) /* rw */ > #define IBEX_SPI_HOST_INTR_TEST (0x08 / 4) /* wo */ > #define IBEX_SPI_HOST_ALERT_TEST (0x0c / 4) /* wo */ > @@ -54,7 +54,7 @@ > #define IBEX_SPI_HOST_TXDATA (0x28 / 4) > > #define IBEX_SPI_HOST_ERROR_ENABLE (0x2c / 4) /* rw */ > -#define IBEX_SPI_HOST_ERROR_STATUS (0x30 / 4) /* rw */ > +#define IBEX_SPI_HOST_ERROR_STATUS (0x30 / 4) /* rw1c */ > #define IBEX_SPI_HOST_EVENT_ENABLE (0x34 / 4) /* rw */ > > /* FIFO Len in Bytes */ > -- > 2.37.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host 2022-08-10 23:01 [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Wilfred Mallawa 2022-08-10 23:02 ` [PATCH 2/3] hw/ssi: fixup coverity issue Wilfred Mallawa 2022-08-10 23:02 ` [PATCH 3/3] hw/ssi: fixup/add rw1c functionality Wilfred Mallawa @ 2022-08-11 7:12 ` Alistair Francis 2022-08-11 14:24 ` Andrew Jones 3 siblings, 0 replies; 11+ messages in thread From: Alistair Francis @ 2022-08-11 7:12 UTC (permalink / raw) To: Wilfred Mallawa Cc: Alistair Francis, open list:RISC-V, qemu-devel@nongnu.org Developers, Wilfred Mallawa On Thu, Aug 11, 2022 at 11:02 AM Wilfred Mallawa <wilfred.mallawa@opensource.wdc.com> wrote: > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > This patch fixes up minor typos in ibex_spi_host > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/ssi/ibex_spi_host.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index d14580b409..601041d719 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -172,7 +172,7 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > & R_INTR_STATE_SPI_EVENT_MASK; > int err_irq = 0, event_irq = 0; > > - /* Error IRQ enabled and Error IRQ Cleared*/ > + /* Error IRQ enabled and Error IRQ Cleared */ > if (error_en && !err_pending) { > /* Event enabled, Interrupt Test Error */ > if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) { > @@ -434,7 +434,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > case IBEX_SPI_HOST_TXDATA: > /* > * This is a hardware `feature` where > - * the first word written TXDATA after init is omitted entirely > + * the first word written to TXDATA after init is omitted entirely > */ > if (s->init_status) { > s->init_status = false; > @@ -487,7 +487,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > break; > case IBEX_SPI_HOST_ERROR_STATUS: > /* > - * Indicates that any errors that have occurred. > + * Indicates any errors that have occurred. > * When an error occurs, the corresponding bit must be cleared > * here before issuing any further commands > */ > -- > 2.37.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host 2022-08-10 23:01 [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Wilfred Mallawa ` (2 preceding siblings ...) 2022-08-11 7:12 ` [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Alistair Francis @ 2022-08-11 14:24 ` Andrew Jones 3 siblings, 0 replies; 11+ messages in thread From: Andrew Jones @ 2022-08-11 14:24 UTC (permalink / raw) To: Wilfred Mallawa; +Cc: Alistair.Francis, qemu-riscv, qemu-devel, Wilfred Mallawa On Thu, Aug 11, 2022 at 09:01:58AM +1000, Wilfred Mallawa wrote: > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > This patch fixes up minor typos in ibex_spi_host > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > --- > hw/ssi/ibex_spi_host.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index d14580b409..601041d719 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -172,7 +172,7 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > & R_INTR_STATE_SPI_EVENT_MASK; > int err_irq = 0, event_irq = 0; > > - /* Error IRQ enabled and Error IRQ Cleared*/ > + /* Error IRQ enabled and Error IRQ Cleared */ > if (error_en && !err_pending) { > /* Event enabled, Interrupt Test Error */ > if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) { > @@ -434,7 +434,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > case IBEX_SPI_HOST_TXDATA: > /* > * This is a hardware `feature` where > - * the first word written TXDATA after init is omitted entirely > + * the first word written to TXDATA after init is omitted entirely > */ > if (s->init_status) { > s->init_status = false; > @@ -487,7 +487,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > break; > case IBEX_SPI_HOST_ERROR_STATUS: > /* > - * Indicates that any errors that have occurred. > + * Indicates any errors that have occurred. > * When an error occurs, the corresponding bit must be cleared > * here before issuing any further commands > */ > -- > 2.37.1 > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-08-14 21:58 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-10 23:01 [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Wilfred Mallawa 2022-08-10 23:02 ` [PATCH 2/3] hw/ssi: fixup coverity issue Wilfred Mallawa 2022-08-11 2:55 ` Bin Meng 2022-08-12 2:23 ` Wilfred Mallawa 2022-08-11 14:23 ` Andrew Jones 2022-08-12 2:21 ` Wilfred Mallawa 2022-08-12 9:21 ` Andrew Jones 2022-08-10 23:02 ` [PATCH 3/3] hw/ssi: fixup/add rw1c functionality Wilfred Mallawa 2022-08-14 21:56 ` Alistair Francis 2022-08-11 7:12 ` [PATCH 1/3] hw/ssi: fixup typos in ibex_spi_host Alistair Francis 2022-08-11 14:24 ` Andrew Jones
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).