* [PATCH v5 1/2] hw/ssi: ibex_spi: fixup coverity issue
2022-09-30 3:32 [PATCH v5 0/2] hw/ssi/ibex_spi: bug fixes Wilfred Mallawa
@ 2022-09-30 3:32 ` Wilfred Mallawa
2022-09-30 3:32 ` [PATCH v5 2/2] hw/ssi: ibex_spi: fixup/add rw1c functionality Wilfred Mallawa
2022-10-11 22:42 ` [PATCH v5 0/2] hw/ssi/ibex_spi: bug fixes Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Wilfred Mallawa @ 2022-09-30 3:32 UTC (permalink / raw)
To: Alistair.Francis, qemu-riscv
Cc: qemu-devel, Wilfred Mallawa, Alistair Francis, Andrew Jones
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.
[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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
hw/ssi/ibex_spi_host.c | 132 +++++++++++++++++++++--------------------
1 file changed, 68 insertions(+), 64 deletions(-)
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
index 94d7da9cc2..d145a4cdbd 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -108,18 +108,22 @@ 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, RXFULL, 0);
+ 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, TXFULL, 0);
+ data = FIELD_DP32(data, STATUS, TXEMPTY, 1);
+ s->regs[IBEX_SPI_HOST_STATUS] = data;
}
static void ibex_spi_host_reset(DeviceState *dev)
@@ -162,37 +166,38 @@ 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;
+ uint32_t intr_test_reg = s->regs[IBEX_SPI_HOST_INTR_TEST];
+ uint32_t intr_en_reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE];
+ uint32_t intr_state_reg = s->regs[IBEX_SPI_HOST_INTR_STATE];
+
+ uint32_t err_en_reg = s->regs[IBEX_SPI_HOST_ERROR_ENABLE];
+ uint32_t event_en_reg = s->regs[IBEX_SPI_HOST_EVENT_ENABLE];
+ uint32_t err_status_reg = s->regs[IBEX_SPI_HOST_ERROR_STATUS];
+ uint32_t status_reg = s->regs[IBEX_SPI_HOST_STATUS];
+
+
+ bool error_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, ERROR);
+ bool event_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, SPI_EVENT);
+ bool err_pending = FIELD_EX32(intr_state_reg, INTR_STATE, ERROR);
+ bool status_pending = FIELD_EX32(intr_state_reg, 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(intr_test_reg, 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(err_en_reg, ERROR_ENABLE, CMDBUSY) &&
+ FIELD_EX32(err_status_reg, 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(err_en_reg, ERROR_ENABLE, CMDINVAL) &&
+ FIELD_EX32(err_status_reg, 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(err_en_reg, ERROR_ENABLE, CSIDINVAL) &&
+ FIELD_EX32(err_status_reg, ERROR_STATUS, CSIDINVAL)) {
/* Invalid value for CSID */
err_irq = 1;
}
@@ -204,22 +209,19 @@ 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(intr_test_reg, 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(event_en_reg, EVENT_ENABLE, READY) &&
+ FIELD_EX32(status_reg, 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(event_en_reg, EVENT_ENABLE, TXEMPTY) &&
+ FIELD_EX32(status_reg, 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(event_en_reg, EVENT_ENABLE, RXFULL) &&
+ FIELD_EX32(status_reg, STATUS, RXFULL)) {
/* SPI RXFULL, RXFIFO full */
event_irq = 1;
}
@@ -232,10 +234,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 +265,21 @@ 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);
+ /* Update register status */
+ s->regs[IBEX_SPI_HOST_STATUS] = data;
+ /* Drop remaining bytes that exceed segment_len */
+ ibex_spi_txfifo_reset(s);
ibex_spi_host_irq(s);
}
@@ -340,7 +342,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, status = 0;
uint8_t txqd_len;
trace_ibex_spi_host_write(addr, size, val64);
@@ -397,22 +399,24 @@ 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) {
- qemu_log_mask(LOG_UNIMP,
+ 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 +456,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) {
+ status = s->regs[IBEX_SPI_HOST_STATUS];
+ if (FIELD_EX32(status, STATUS, BYTEORDER) == 0) {
/* LE: LSB transmitted first (default for ibex processor) */
shift_mask = 0xff << (i * 8);
} else {
@@ -464,18 +468,18 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * 8));
}
-
+ status = s->regs[IBEX_SPI_HOST_STATUS];
/* Reset TXEMPTY */
- s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK;
+ status = FIELD_DP32(status, 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(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;
+ status = FIELD_DP32(status, STATUS, TXQD, txqd_len);
/* Assert Ready */
- s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK;
+ status = FIELD_DP32(status, STATUS, READY, 1);
+ /* Update register status */
+ s->regs[IBEX_SPI_HOST_STATUS] = status;
break;
case IBEX_SPI_HOST_ERROR_ENABLE:
s->regs[addr] = val32;
--
2.37.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v5 2/2] hw/ssi: ibex_spi: fixup/add rw1c functionality
2022-09-30 3:32 [PATCH v5 0/2] hw/ssi/ibex_spi: bug fixes Wilfred Mallawa
2022-09-30 3:32 ` [PATCH v5 1/2] hw/ssi: ibex_spi: fixup coverity issue Wilfred Mallawa
@ 2022-09-30 3:32 ` Wilfred Mallawa
2022-10-11 22:42 ` [PATCH v5 0/2] hw/ssi/ibex_spi: bug fixes Alistair Francis
2 siblings, 0 replies; 4+ messages in thread
From: Wilfred Mallawa @ 2022-09-30 3:32 UTC (permalink / raw)
To: Alistair.Francis, qemu-riscv
Cc: qemu-devel, Wilfred Mallawa, Alistair Francis
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>
---
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 d145a4cdbd..57df462e3c 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -342,7 +342,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
{
IbexSPIHostState *s = opaque;
uint32_t val32 = val64;
- uint32_t shift_mask = 0xff, status = 0;
+ uint32_t shift_mask = 0xff, status = 0, data = 0;
uint8_t txqd_len;
trace_ibex_spi_host_write(addr, size, val64);
@@ -352,7 +352,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:
@@ -495,7 +505,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;
+ status = s->regs[addr];
+ /* rw1c status register */
+ if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) {
+ status = FIELD_DP32(status, ERROR_STATUS, CMDBUSY, 0);
+ }
+ if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) {
+ status = FIELD_DP32(status, ERROR_STATUS, OVERFLOW, 0);
+ }
+ if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) {
+ status = FIELD_DP32(status, ERROR_STATUS, UNDERFLOW, 0);
+ }
+ if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) {
+ status = FIELD_DP32(status, ERROR_STATUS, CMDINVAL, 0);
+ }
+ if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) {
+ status = FIELD_DP32(status, ERROR_STATUS, CSIDINVAL, 0);
+ }
+ if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) {
+ status = FIELD_DP32(status, ERROR_STATUS, ACCESSINVAL, 0);
+ }
+ s->regs[addr] = status;
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.3
^ permalink raw reply related [flat|nested] 4+ messages in thread