On Wed, 20 May 2020 at 15:33, Eden Mikitas <e.miki...@gmail.com> wrote:
> When inserting the value retrieved (rx) from the spi slave, rx is pushed to
> rx_fifo after being cast to uint8_t. rx_fifo is a fifo32, and the rx
> register the driver uses is also 32 bit. This zeroes the 24 most
> significant bits of rx. This proved problematic with devices that expect to
> use the whole 32 bits of the rx register.
> I tested this change by running `make check` and by booting linux on
> sabrelite (which uses an spi flash device).
>
> Signed-off-by: Eden Mikitas <e.miki...@gmail.com>
> ---
> hw/ssi/imx_spi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 2dd9a631e1..43b2f14dd2 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -182,7 +182,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>
> rx = 0;
>
> - while (tx_burst) {
> + while (tx_burst > 0) {
> uint8_t byte = tx & 0xff;
When does this make a difference? Looking at the code
I don't think tx_burst can ever be negative at this
point, in which case the two conditions are equivalent.
What was the motivation for this change?
> DPRINTF("writing 0x%02x\n", (uint32_t)byte);
> @@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
> if (fifo32_is_full(&s->rx_fifo)) {
> s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
> } else {
> - fifo32_push(&s->rx_fifo, (uint8_t)rx);
> + fifo32_push(&s->rx_fifo, rx);
> }
>
> if (s->burst_length <= 0) {
This seems like a separate change to the first one;
in general unrelated changes should be each in their
own patch, rather than combined into a single patch.
The fifo32_push() part of this change looks correct to me.
thanks
-- PMM
