| Message ID | 20200520143255.27235-1-e.mikitas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement | expand |
On Wed, 20 May 2020 at 15:33, Eden Mikitas <e.mikitas@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.mikitas@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
The ecspi controller is supposed to support burst length of 1 to 4096 bits, meaning it is possible to configure it to use a value that isn't a multiple of 8 (to my understanding). In that case, since tx_burst is always decremented by 8, it will underflow. Sorry I didn't include an explanation. Should I resubmit the patch? > > > 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. Should I resubmit this as a patch? > > The fifo32_push() part of this change looks correct to me. > > thanks > -- PMM >
On Thu, 21 May 2020 at 19:49, Eden Mikitas <e.mikitas@gmail.com> wrote: > > The ecspi controller is supposed to support burst length of 1 to 4096 bits, > meaning it is possible to configure it to use a value that isn't a multiple > of 8 (to my understanding). In that case, since tx_burst is always > decremented by 8, it will underflow. Sorry I didn't include an explanation. Ah, yes, I misread the code. On the first time into the loop tx_burst must be positive, but this is a while() condition and on subsequent loops we might end up negative. So all the code changes in this patch are correct, they just need to be split into two commits I think. > > 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. > > Should I resubmit this as a patch? Yes, if you could submit a 2-patch patch series where one patch is the fix for handling burst lengths which aren't multiples of 8, and the other is the fix for saving all the data into the rx fifo rather than just the low byte, that would be great. thanks -- PMM
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; 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) {
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.mikitas@gmail.com> --- hw/ssi/imx_spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)