* [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
* [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 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 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 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 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
* 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, ajones; +Cc: qemu-riscv, Alistair Francis, qemu-devel
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-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, bmeng.cn; +Cc: qemu-riscv, Alistair Francis, qemu-devel
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-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, qemu-riscv, Alistair Francis, qemu-devel
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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.