| Message ID | 1608182913-54603-2-git-send-email-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic | expand |
On Thu, 17 Dec 2020 at 05:28, Bin Meng <bmeng.cn@gmail.com> wrote: > > From: Bin Meng <bin.meng@windriver.com> > > The endianness of data exchange between tx and rx fifo is incorrect. > Earlier bytes are supposed to show up on MSB and later bytes on LSB, > ie: in big endian. The manual does not explicitly say this, but the > U-Boot and Linux driver codes have a swap on the data transferred > to tx fifo and from rx fifo. To check my understanding, if we have a burst length of 16 bits, say, when we do the fifo32_pop() of a 32 bit word, where in that word and which way round are the 2 bytes we are going to transfer ? > With this change, U-Boot read from / write to SPI flash tests pass. > > => sf test 1ff000 1000 > SPI flash test: > 0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps > 1 check: 3 ticks, 1333 KiB/s 10.664 Mbps > 2 write: 235 ticks, 17 KiB/s 0.136 Mbps > 3 read: 2 ticks, 2000 KiB/s 16.000 Mbps > Test passed > 0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps > 1 check: 3 ticks, 1333 KiB/s 10.664 Mbps > 2 write: 235 ticks, 17 KiB/s 0.136 Mbps > 3 read: 2 ticks, 2000 KiB/s 16.000 Mbps > > Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller") > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > hw/ssi/imx_spi.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c > index 509fb9f..71f0902 100644 > --- a/hw/ssi/imx_spi.c > +++ b/hw/ssi/imx_spi.c > @@ -156,13 +156,14 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) > { > uint32_t tx; > uint32_t rx; > + uint32_t data; > + uint8_t byte; > > DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n", > fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo)); > > while (!fifo32_is_empty(&s->tx_fifo)) { > int tx_burst = 0; > - int index = 0; > > if (s->burst_length <= 0) { > s->burst_length = imx_spi_burst_length(s); > @@ -183,10 +184,18 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) > tx_burst = 32; > } > > + data = 0; > + for (int i = 0; i < tx_burst / 8; i++) { > + byte = tx & 0xff; > + tx = tx >> 8; > + data = (data << 8) | byte; > + } > + tx = data; > + Why carefully reverse the order of bytes in the word and then take a byte at a time from the bottom of the word in the loop below, when you could change the loop to take bytes from the top of the word instead ? > rx = 0; > > while (tx_burst > 0) { > - uint8_t byte = tx & 0xff; > + byte = tx & 0xff; > > DPRINTF("writing 0x%02x\n", (uint32_t)byte); > > @@ -196,12 +205,11 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) > DPRINTF("0x%02x read\n", (uint32_t)byte); > > tx = tx >> 8; > - rx |= (byte << (index * 8)); > + rx = (rx << 8) | byte; > > /* Remove 8 bits from the actual burst */ > tx_burst -= 8; > s->burst_length -= 8; > - index++; > } > > DPRINTF("data rx:0x%08x\n", rx); > -- > 2.7.4 thanks -- PMM
Hi Peter, On Fri, Jan 8, 2021 at 10:49 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 17 Dec 2020 at 05:28, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > From: Bin Meng <bin.meng@windriver.com> > > > > The endianness of data exchange between tx and rx fifo is incorrect. > > Earlier bytes are supposed to show up on MSB and later bytes on LSB, > > ie: in big endian. The manual does not explicitly say this, but the > > U-Boot and Linux driver codes have a swap on the data transferred > > to tx fifo and from rx fifo. > > To check my understanding, if we have a burst length of 16 bits, say, > when we do the fifo32_pop() of a 32 bit word, where in that > word and which way round are the 2 bytes we are going to transfer ? Say the fifo was written with a value of 0x00001234 when the burst length is 16 bits, 0x12 will be transferred first then followed by 0x34. > > > With this change, U-Boot read from / write to SPI flash tests pass. > > > > => sf test 1ff000 1000 > > SPI flash test: > > 0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps > > 1 check: 3 ticks, 1333 KiB/s 10.664 Mbps > > 2 write: 235 ticks, 17 KiB/s 0.136 Mbps > > 3 read: 2 ticks, 2000 KiB/s 16.000 Mbps > > Test passed > > 0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps > > 1 check: 3 ticks, 1333 KiB/s 10.664 Mbps > > 2 write: 235 ticks, 17 KiB/s 0.136 Mbps > > 3 read: 2 ticks, 2000 KiB/s 16.000 Mbps > > > > Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller") > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > > > --- > > > > hw/ssi/imx_spi.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c > > index 509fb9f..71f0902 100644 > > --- a/hw/ssi/imx_spi.c > > +++ b/hw/ssi/imx_spi.c > > @@ -156,13 +156,14 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) > > { > > uint32_t tx; > > uint32_t rx; > > + uint32_t data; > > + uint8_t byte; > > > > DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n", > > fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo)); > > > > while (!fifo32_is_empty(&s->tx_fifo)) { > > int tx_burst = 0; > > - int index = 0; > > > > if (s->burst_length <= 0) { > > s->burst_length = imx_spi_burst_length(s); > > @@ -183,10 +184,18 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) > > tx_burst = 32; > > } > > > > + data = 0; > > + for (int i = 0; i < tx_burst / 8; i++) { > > + byte = tx & 0xff; > > + tx = tx >> 8; > > + data = (data << 8) | byte; > > + } > > + tx = data; > > + > > Why carefully reverse the order of bytes in the word and then > take a byte at a time from the bottom of the word in the loop below, > when you could change the loop to take bytes from the top of the word > instead ? Ah, yes, this can be rewritten to simplify a little. > > > rx = 0; > > > > while (tx_burst > 0) { > > - uint8_t byte = tx & 0xff; > > + byte = tx & 0xff; > > > > DPRINTF("writing 0x%02x\n", (uint32_t)byte); > > > > @@ -196,12 +205,11 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) > > DPRINTF("0x%02x read\n", (uint32_t)byte); > > > > tx = tx >> 8; > > - rx |= (byte << (index * 8)); > > + rx = (rx << 8) | byte; > > > > /* Remove 8 bits from the actual burst */ > > tx_burst -= 8; > > s->burst_length -= 8; > > - index++; > > } > > > > DPRINTF("data rx:0x%08x\n", rx); > > -- Regards, Bin
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c index 509fb9f..71f0902 100644 --- a/hw/ssi/imx_spi.c +++ b/hw/ssi/imx_spi.c @@ -156,13 +156,14 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) { uint32_t tx; uint32_t rx; + uint32_t data; + uint8_t byte; DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n", fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo)); while (!fifo32_is_empty(&s->tx_fifo)) { int tx_burst = 0; - int index = 0; if (s->burst_length <= 0) { s->burst_length = imx_spi_burst_length(s); @@ -183,10 +184,18 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) tx_burst = 32; } + data = 0; + for (int i = 0; i < tx_burst / 8; i++) { + byte = tx & 0xff; + tx = tx >> 8; + data = (data << 8) | byte; + } + tx = data; + rx = 0; while (tx_burst > 0) { - uint8_t byte = tx & 0xff; + byte = tx & 0xff; DPRINTF("writing 0x%02x\n", (uint32_t)byte); @@ -196,12 +205,11 @@ static void imx_spi_flush_txfifo(IMXSPIState *s) DPRINTF("0x%02x read\n", (uint32_t)byte); tx = tx >> 8; - rx |= (byte << (index * 8)); + rx = (rx << 8) | byte; /* Remove 8 bits from the actual burst */ tx_burst -= 8; s->burst_length -= 8; - index++; } DPRINTF("data rx:0x%08x\n", rx);