On Fri, May 13, 2022 at 2:37 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Fri, 22 Apr 2022 at 01:40, Alistair Francis > <alistair.fran...@opensource.wdc.com> wrote: > > > > From: Wilfred Mallawa <wilfred.mall...@wdc.com> > > > > Adds the SPI_HOST device model for ibex. The device specification is as per > > [1]. The model has been tested on opentitan with spi_host unit tests > > written for TockOS. > > > > [1] https://docs.opentitan.org/hw/ip/spi_host/doc/ > > > Hi; Coverity points out a bug in this code (CID 1488107): > > > +REG32(STATUS, 0x14) > > + FIELD(STATUS, TXQD, 0, 8) > > + FIELD(STATUS, RXQD, 18, 8) > > RXQD isn't at the bottom of this register, so the R_STATUS_RXQD_MASK > define is a shifted-up mask... > > > +static void ibex_spi_host_transfer(IbexSPIHostState *s) > > +{ > > + uint32_t rx, tx; > > + /* Get num of one byte transfers */ > > + uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & > > R_COMMAND_LEN_MASK) > > + >> R_COMMAND_LEN_SHIFT); > > + while (segment_len > 0) { > > + if (fifo8_is_empty(&s->tx_fifo)) { > > + /* Assert Stall */ > > + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXSTALL_MASK; > > + break; > > + } else if (fifo8_is_full(&s->rx_fifo)) { > > + /* Assert Stall */ > > + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXSTALL_MASK; > > + break; > > + } else { > > + tx = fifo8_pop(&s->tx_fifo); > > + } > > + > > + rx = ssi_transfer(s->ssi, tx); > > + > > + trace_ibex_spi_host_transfer(tx, rx); > > + > > + if (!fifo8_is_full(&s->rx_fifo)) { > > + fifo8_push(&s->rx_fifo, rx); > > + } else { > > + /* Assert RXFULL */ > > + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXFULL_MASK; > > + } > > + --segment_len; > > + } > > + > > + /* Assert Ready */ > > + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > > + /* 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)); > > ...but here we don't shift div4_round_up(segment_len) up to the > right place before ORing it with the MASK, so Coverity points > out that the result is always zero. > > > + /* 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; > > This has the same issue, but avoids it by luck because TXQD > does start at bit 0. > > Since we're using the FIELD() macros, it would be clearer to > write all this to use FIELD_DP32() rather than manual > bit operations to clear the bits and then OR in the new ones. > (True here and also in what looks like several other places > through out the file, for deposit and extract operations.)
Thanks Peter, Wilfred is looking into it and should be sending patches soon. Alistair > > thanks > -- PMM
