[PATCH v4 0/6] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[PATCH v4 0/6] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

This series fixes a bunch of bugs in current implementation of the imx
spi controller, including the following issues:

- chip select signal was not lower down when spi controller is disabled
- remove imx_spi_update_irq() in imx_spi_reset()
- transfer incorrect data when the burst length is larger than 32 bit
- spi controller tx and rx fifo endianness is incorrect

Tested with upstream U-Boot v2020.10 (polling mode) and VxWorks 7
(interrupt mode).

Changes in v4:
- adujst the patch 2,3 order
- rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
- s/normal/common/ in the commit message
- log the burst length value in the log message

Changes in v3:
- new patch: remove imx_spi_update_irq() in imx_spi_reset()
- Move the chip selects disable out of imx_spi_reset()
- new patch: log unimplemented burst length
- Simplify the tx fifo endianness handling

Changes in v2:
- Fix the "Fixes" tag in the commit message
- Use ternary operator as Philippe suggested

Bin Meng (5):
  hw/ssi: imx_spi: Use a macro for number of chip selects supported
  hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()
  hw/ssi: imx_spi: Log unimplemented burst length
  hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic
  hw/ssi: imx_spi: Correct tx and rx fifo endianness

Xuzhou Cheng (1):
  hw/ssi: imx_spi: Disable chip selects when controller is disabled

 include/hw/ssi/imx_spi.h |  5 ++++-
 hw/ssi/imx_spi.c         | 42 ++++++++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v4 1/6] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Avoid using a magic number (4) everywhere for the number of chip
selects supported.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 include/hw/ssi/imx_spi.h | 5 ++++-
 hw/ssi/imx_spi.c         | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/hw/ssi/imx_spi.h b/include/hw/ssi/imx_spi.h
index b82b17f364..eeaf49bbac 100644
--- a/include/hw/ssi/imx_spi.h
+++ b/include/hw/ssi/imx_spi.h
@@ -77,6 +77,9 @@
 
 #define EXTRACT(value, name) extract32(value, name##_SHIFT, name##_LENGTH)
 
+/* number of chip selects supported */
+#define ECSPI_NUM_CS 4
+
 #define TYPE_IMX_SPI "imx.spi"
 OBJECT_DECLARE_SIMPLE_TYPE(IMXSPIState, IMX_SPI)
 
@@ -89,7 +92,7 @@ struct IMXSPIState {
 
     qemu_irq irq;
 
-    qemu_irq cs_lines[4];
+    qemu_irq cs_lines[ECSPI_NUM_CS];
 
     SSIBus *bus;
 
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index d8885ae454..e605049a21 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -361,7 +361,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
 
             /* We are in master mode */
 
-            for (i = 0; i < 4; i++) {
+            for (i = 0; i < ECSPI_NUM_CS; i++) {
                 qemu_set_irq(s->cs_lines[i],
                              i == imx_spi_selected_channel(s) ? 0 : 1);
             }
@@ -424,7 +424,7 @@ static void imx_spi_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
 
-    for (i = 0; i < 4; ++i) {
+    for (i = 0; i < ECSPI_NUM_CS; ++i) {
         sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cs_lines[i]);
     }
 
-- 
2.25.1

[PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Usually the approach is that the device on the other end of the line
is going to reset its state anyway, so there's no need to actively
signal an irq line change during the reset hook.

Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
imx_spi_hard_reset() that is called when the controller is disabled.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v4:
- adujst the patch 2,3 order
- rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion

Changes in v3:
- new patch: remove imx_spi_update_irq() in imx_spi_reset()

 hw/ssi/imx_spi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index e605049a21..2c4c5ec1b8 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
     imx_spi_rxfifo_reset(s);
     imx_spi_txfifo_reset(s);
 
-    imx_spi_update_irq(s);
-
     s->burst_length = 0;
 }
 
+static void imx_spi_hard_reset(IMXSPIState *s)
+{
+    imx_spi_reset(DEVICE(s));
+
+    imx_spi_update_irq(s);
+}
+
 static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
 {
     uint32_t value = 0;
@@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
         s->regs[ECSPI_CONREG] = value;
 
         if (!imx_spi_is_enabled(s)) {
-            /* device is disabled, so this is a reset */
-            imx_spi_reset(DEVICE(s));
+            /* device is disabled, so this is a hard reset */
+            imx_spi_hard_reset(s);
+
             return;
         }
 
-- 
2.25.1

[PATCH v4 3/6] hw/ssi: imx_spi: Disable chip selects when controller is disabled

[Bin Meng]

From: Xuzhou Cheng <xuzhou.cheng@windriver.com>

When a write to ECSPI_CONREG register to disable the SPI controller,
imx_spi_reset() is called to reset the controller, but chip select
lines should have been disabled, otherwise the state machine of any
devices (e.g.: SPI flashes) connected to the SPI master is stuck to
its last state and responds incorrectly to any follow-up commands.

Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller")
Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

(no changes since v3)

Changes in v3:
- Move the chip selects disable out of imx_spi_reset()

Changes in v2:
- Fix the "Fixes" tag in the commit message

 hw/ssi/imx_spi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 2c4c5ec1b8..168ea95440 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -246,9 +246,15 @@ static void imx_spi_reset(DeviceState *dev)
 
 static void imx_spi_hard_reset(IMXSPIState *s)
 {
+    int i;
+
     imx_spi_reset(DEVICE(s));
 
     imx_spi_update_irq(s);
+
+    for (i = 0; i < ECSPI_NUM_CS; i++) {
+        qemu_set_irq(s->cs_lines[i], 1);
+    }
 }
 
 static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
-- 
2.25.1

[PATCH v4 4/6] hw/ssi: imx_spi: Log unimplemented burst length

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Current implementation of the imx spi controller expects the burst
length to be multiple of 8, which is the most common use case.

In case the burst length is not what we expect, log it to give user
a chance to notice it.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---

Changes in v4:
- s/normal/common/ in the commit message
- log the burst length value in the log message

Changes in v3:
- new patch: log unimplemented burst length

 hw/ssi/imx_spi.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 168ea95440..7f81b329a4 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -128,7 +128,16 @@ static uint8_t imx_spi_selected_channel(IMXSPIState *s)
 
 static uint32_t imx_spi_burst_length(IMXSPIState *s)
 {
-    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
+    uint32_t burst;
+
+    burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
+    if (burst % 8) {
+        qemu_log_mask(LOG_UNIMP,
+                      "[%s]%s: burst length (%d) not multiple of 8!\n",
+                      TYPE_IMX_SPI, __func__, burst);
+    }
+
+    return burst;
 }
 
 static bool imx_spi_is_enabled(IMXSPIState *s)
-- 
2.25.1

[PATCH v4 5/6] hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

For the ECSPIx_CONREG register BURST_LENGTH field, the manual says:

0x020 A SPI burst contains the 1 LSB in first word and all 32 bits in second word.
0x021 A SPI burst contains the 2 LSB in first word and all 32 bits in second word.

Current logic uses either s->burst_length or 32, whichever smaller,
to determine how many bits it should read from the tx fifo each time.
For example, for a 48 bit burst length, current logic transfers the
first 32 bit from the first word in the tx fifo, followed by a 16
bit from the second word in the tx fifo, which is wrong. The correct
logic should be: transfer the first 16 bit from the first word in
the tx fifo, followed by a 32 bit from the second word in the tx fifo.

With this change, SPI flash can be successfully probed by U-Boot on
imx6 sabrelite board.

  => sf probe
  SF: Detected sst25vf016b with page size 256 Bytes, erase size 4 KiB, total 2 MiB

Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---

(no changes since v2)

Changes in v2:
- Use ternary operator as Philippe suggested

 hw/ssi/imx_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 7f81b329a4..47c8a0f572 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -187,7 +187,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
 
         DPRINTF("data tx:0x%08x\n", tx);
 
-        tx_burst = MIN(s->burst_length, 32);
+        tx_burst = (s->burst_length % 32) ? : 32;
 
         rx = 0;
 
-- 
2.25.1

[PATCH v4 6/6] hw/ssi: imx_spi: Correct tx and rx fifo endianness

[Bin Meng]

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.

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>

---

(no changes since v3)

Changes in v3:
- Simplify the tx fifo endianness handling

 hw/ssi/imx_spi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 47c8a0f572..b5124a6426 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -171,7 +171,6 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
 
     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);
@@ -192,7 +191,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
         rx = 0;
 
         while (tx_burst > 0) {
-            uint8_t byte = tx & 0xff;
+            uint8_t byte = tx >> (tx_burst - 8);
 
             DPRINTF("writing 0x%02x\n", (uint32_t)byte);
 
@@ -201,13 +200,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.25.1

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Philippe Mathieu-Daudé]

On 1/10/21 9:14 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Usually the approach is that the device on the other end of the line
> is going to reset its state anyway, so there's no need to actively
> signal an irq line change during the reset hook.
> 
> Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
> imx_spi_hard_reset() that is called when the controller is disabled.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v4:
> - adujst the patch 2,3 order
> - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
> 
> Changes in v3:
> - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> 
>  hw/ssi/imx_spi.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index e605049a21..2c4c5ec1b8 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
>      imx_spi_rxfifo_reset(s);
>      imx_spi_txfifo_reset(s);
>  
> -    imx_spi_update_irq(s);
> -
>      s->burst_length = 0;
>  }
>  
> +static void imx_spi_hard_reset(IMXSPIState *s)
> +{
> +    imx_spi_reset(DEVICE(s));
> +
> +    imx_spi_update_irq(s);
> +}
> +
>  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      uint32_t value = 0;
> @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>          s->regs[ECSPI_CONREG] = value;
>  
>          if (!imx_spi_is_enabled(s)) {
> -            /* device is disabled, so this is a reset */
> -            imx_spi_reset(DEVICE(s));
> +            /* device is disabled, so this is a hard reset */
> +            imx_spi_hard_reset(s);
> +
>              return;
>          }
>  

Almost good :)

DeviceReset handler is a hard reset, so you need:

  dc->reset = imx_spi_hard_reset;

Thus you also need this function prototype:

void imx_spi_hard_reset(DeviceState *dev)

Eventually renaming imx_spi_reset() -> imx_spi_soft_reset()
will make things even easier.

Regards,

Phil.

Re: [PATCH v4 3/6] hw/ssi: imx_spi: Disable chip selects when controller is disabled

[Philippe Mathieu-Daudé]

On 1/10/21 9:14 AM, Bin Meng wrote:
> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> 
> When a write to ECSPI_CONREG register to disable the SPI controller,
> imx_spi_reset() is called to reset the controller, but chip select
> lines should have been disabled, otherwise the state machine of any
> devices (e.g.: SPI flashes) connected to the SPI master is stuck to
> its last state and responds incorrectly to any follow-up commands.
> 
> Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller")
> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> (no changes since v3)
> 
> Changes in v3:
> - Move the chip selects disable out of imx_spi_reset()
> 
> Changes in v2:
> - Fix the "Fixes" tag in the commit message
> 
>  hw/ssi/imx_spi.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Bin Meng]

Hi Philippe,

On Sun, Jan 10, 2021 at 7:15 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/10/21 9:14 AM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Usually the approach is that the device on the other end of the line
> > is going to reset its state anyway, so there's no need to actively
> > signal an irq line change during the reset hook.
> >
> > Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
> > imx_spi_hard_reset() that is called when the controller is disabled.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> > Changes in v4:
> > - adujst the patch 2,3 order
> > - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
> >
> > Changes in v3:
> > - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> >
> >  hw/ssi/imx_spi.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index e605049a21..2c4c5ec1b8 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
> >      imx_spi_rxfifo_reset(s);
> >      imx_spi_txfifo_reset(s);
> >
> > -    imx_spi_update_irq(s);
> > -
> >      s->burst_length = 0;
> >  }
> >
> > +static void imx_spi_hard_reset(IMXSPIState *s)
> > +{
> > +    imx_spi_reset(DEVICE(s));
> > +
> > +    imx_spi_update_irq(s);
> > +}
> > +
> >  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> >      uint32_t value = 0;
> > @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> >          s->regs[ECSPI_CONREG] = value;
> >
> >          if (!imx_spi_is_enabled(s)) {
> > -            /* device is disabled, so this is a reset */
> > -            imx_spi_reset(DEVICE(s));
> > +            /* device is disabled, so this is a hard reset */
> > +            imx_spi_hard_reset(s);
> > +
> >              return;
> >          }
> >
>
> Almost good :)
>
> DeviceReset handler is a hard reset, so you need:
>
>   dc->reset = imx_spi_hard_reset;
>
> Thus you also need this function prototype:
>
> void imx_spi_hard_reset(DeviceState *dev)
>
> Eventually renaming imx_spi_reset() -> imx_spi_soft_reset()
> will make things even easier.

Now I am confused. The v3 patch did the above already, but you
mentioned that usually the hard reset includes the soft reset. But in
the hard reset the imx_spi_update_irq() does not need to be called.

Regards,
Bin

Re: [PATCH v4 6/6] hw/ssi: imx_spi: Correct tx and rx fifo endianness

[Peter Maydell]

On Sun, 10 Jan 2021 at 08:15, 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.
>
> 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>
>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Simplify the tx fifo endianness handling
>
>  hw/ssi/imx_spi.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 47c8a0f572..b5124a6426 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -171,7 +171,6 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>
>      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);
> @@ -192,7 +191,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>          rx = 0;
>
>          while (tx_burst > 0) {
> -            uint8_t byte = tx & 0xff;
> +            uint8_t byte = tx >> (tx_burst - 8);
>
>              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
>
> @@ -201,13 +200,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++;
>          }

This version of the loop definitely looks a lot neater. However,
looking at the code I don't think there's anything that forces the
guest to set a burst length that's a multiple of 8, so you need
to handle that somehow. Otherwise on the last time through the
loop (tx_burst - 8) can be negative, which is undefined behaviour
when you try to shift by it.

I think just rounding tx_burst up to a multiple of 8 before
the start of the loop would do the right thing ?

thanks
-- PMM

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Peter Maydell]

On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Usually the approach is that the device on the other end of the line
> is going to reset its state anyway, so there's no need to actively
> signal an irq line change during the reset hook.
>
> Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
> imx_spi_hard_reset() that is called when the controller is disabled.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>
> ---
>
> Changes in v4:
> - adujst the patch 2,3 order
> - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
>
> Changes in v3:
> - new patch: remove imx_spi_update_irq() in imx_spi_reset()
>
>  hw/ssi/imx_spi.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index e605049a21..2c4c5ec1b8 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
>      imx_spi_rxfifo_reset(s);
>      imx_spi_txfifo_reset(s);
>
> -    imx_spi_update_irq(s);
> -
>      s->burst_length = 0;
>  }
>
> +static void imx_spi_hard_reset(IMXSPIState *s)
> +{
> +    imx_spi_reset(DEVICE(s));
> +
> +    imx_spi_update_irq(s);
> +}
> +
>  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      uint32_t value = 0;
> @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>          s->regs[ECSPI_CONREG] = value;
>
>          if (!imx_spi_is_enabled(s)) {
> -            /* device is disabled, so this is a reset */
> -            imx_spi_reset(DEVICE(s));
> +            /* device is disabled, so this is a hard reset */
> +            imx_spi_hard_reset(s);
> +
>              return;
>          }

The function of the code is correct, but you seem to have the function
naming backwards here. Generally:
 * soft reset == the reset triggered by the register write
 * hard reset == power-on reset == the dc->reset function

I think this is what Philippe was trying to say.

thanks
-- PMM

Re: [PATCH v4 6/6] hw/ssi: imx_spi: Correct tx and rx fifo endianness

[Bin Meng]

On Tue, Jan 12, 2021 at 6:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 10 Jan 2021 at 08:15, 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.
> >
> > 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>
> >
> > ---
> >
> > (no changes since v3)
> >
> > Changes in v3:
> > - Simplify the tx fifo endianness handling
> >
> >  hw/ssi/imx_spi.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index 47c8a0f572..b5124a6426 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -171,7 +171,6 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
> >
> >      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);
> > @@ -192,7 +191,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
> >          rx = 0;
> >
> >          while (tx_burst > 0) {
> > -            uint8_t byte = tx & 0xff;
> > +            uint8_t byte = tx >> (tx_burst - 8);
> >
> >              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
> >
> > @@ -201,13 +200,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++;
> >          }
>
> This version of the loop definitely looks a lot neater. However,
> looking at the code I don't think there's anything that forces the
> guest to set a burst length that's a multiple of 8, so you need
> to handle that somehow. Otherwise on the last time through the
> loop (tx_burst - 8) can be negative, which is undefined behaviour
> when you try to shift by it.

Yes, that's why I added a patch to log the unimplemented behavior to
notify the user.

> I think just rounding tx_burst up to a multiple of 8 before
> the start of the loop would do the right thing ?

Probably. Given all flash transfers are normally multiple of 8-bits I
am not sure what the real hardware behavior is when it is not multiple
of 8, but I will try to add something in the next version.

Regards,
Bin

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Bin Meng]

On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Usually the approach is that the device on the other end of the line
> > is going to reset its state anyway, so there's no need to actively
> > signal an irq line change during the reset hook.
> >
> > Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
> > imx_spi_hard_reset() that is called when the controller is disabled.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> > Changes in v4:
> > - adujst the patch 2,3 order
> > - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
> >
> > Changes in v3:
> > - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> >
> >  hw/ssi/imx_spi.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index e605049a21..2c4c5ec1b8 100644
> > --- a/hw/ssi/imx_spi.c
> > +++ b/hw/ssi/imx_spi.c
> > @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
> >      imx_spi_rxfifo_reset(s);
> >      imx_spi_txfifo_reset(s);
> >
> > -    imx_spi_update_irq(s);
> > -
> >      s->burst_length = 0;
> >  }
> >
> > +static void imx_spi_hard_reset(IMXSPIState *s)
> > +{
> > +    imx_spi_reset(DEVICE(s));
> > +
> > +    imx_spi_update_irq(s);
> > +}
> > +
> >  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> >      uint32_t value = 0;
> > @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> >          s->regs[ECSPI_CONREG] = value;
> >
> >          if (!imx_spi_is_enabled(s)) {
> > -            /* device is disabled, so this is a reset */
> > -            imx_spi_reset(DEVICE(s));
> > +            /* device is disabled, so this is a hard reset */
> > +            imx_spi_hard_reset(s);
> > +
> >              return;
> >          }
>
> The function of the code is correct, but you seem to have the function
> naming backwards here. Generally:
>  * soft reset == the reset triggered by the register write
>  * hard reset == power-on reset == the dc->reset function
>
> I think this is what Philippe was trying to say.

Philippe said: "Hmm usually hard reset include soft reset."

Since we are moving imx_spi_update_irq() out of imx_spi_reset() to a
new function called imx_spi_soft_reset() (what I did in v3), that
confused him (and I felt the same thing), so I renamed
imx_spi_soft_reset() to imx_spi_hard_reset() in v4..

Regards,
Bin

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Peter Maydell]

On Tue, 12 Jan 2021 at 12:54, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Usually the approach is that the device on the other end of the line
> > > is going to reset its state anyway, so there's no need to actively
> > > signal an irq line change during the reset hook.
> > >
> > > Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
> > > imx_spi_hard_reset() that is called when the controller is disabled.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - adujst the patch 2,3 order
> > > - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
> > >
> > > Changes in v3:
> > > - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> > >
> > >  hw/ssi/imx_spi.c | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > > index e605049a21..2c4c5ec1b8 100644
> > > --- a/hw/ssi/imx_spi.c
> > > +++ b/hw/ssi/imx_spi.c
> > > @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
> > >      imx_spi_rxfifo_reset(s);
> > >      imx_spi_txfifo_reset(s);
> > >
> > > -    imx_spi_update_irq(s);
> > > -
> > >      s->burst_length = 0;
> > >  }
> > >
> > > +static void imx_spi_hard_reset(IMXSPIState *s)
> > > +{
> > > +    imx_spi_reset(DEVICE(s));
> > > +
> > > +    imx_spi_update_irq(s);
> > > +}
> > > +
> > >  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> > >  {
> > >      uint32_t value = 0;
> > > @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> > >          s->regs[ECSPI_CONREG] = value;
> > >
> > >          if (!imx_spi_is_enabled(s)) {
> > > -            /* device is disabled, so this is a reset */
> > > -            imx_spi_reset(DEVICE(s));
> > > +            /* device is disabled, so this is a hard reset */
> > > +            imx_spi_hard_reset(s);
> > > +
> > >              return;
> > >          }
> >
> > The function of the code is correct, but you seem to have the function
> > naming backwards here. Generally:
> >  * soft reset == the reset triggered by the register write
> >  * hard reset == power-on reset == the dc->reset function
> >
> > I think this is what Philippe was trying to say.
>
> Philippe said: "Hmm usually hard reset include soft reset."

True in hardware, but for QEMU there are some things we don't
want to do in what we would call a hard or power-on reset.

> Since we are moving imx_spi_update_irq() out of imx_spi_reset() to a
> new function called imx_spi_soft_reset() (what I did in v3), that
> confused him (and I felt the same thing), so I renamed
> imx_spi_soft_reset() to imx_spi_hard_reset() in v4..

thanks
-- PMM

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Bin Meng]

On Tue, Jan 12, 2021 at 9:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 12 Jan 2021 at 12:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > Usually the approach is that the device on the other end of the line
> > > > is going to reset its state anyway, so there's no need to actively
> > > > signal an irq line change during the reset hook.
> > > >
> > > > Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
> > > > imx_spi_hard_reset() that is called when the controller is disabled.
> > > >
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > ---
> > > >
> > > > Changes in v4:
> > > > - adujst the patch 2,3 order
> > > > - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
> > > >
> > > > Changes in v3:
> > > > - new patch: remove imx_spi_update_irq() in imx_spi_reset()
> > > >
> > > >  hw/ssi/imx_spi.c | 14 ++++++++++----
> > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > > > index e605049a21..2c4c5ec1b8 100644
> > > > --- a/hw/ssi/imx_spi.c
> > > > +++ b/hw/ssi/imx_spi.c
> > > > @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
> > > >      imx_spi_rxfifo_reset(s);
> > > >      imx_spi_txfifo_reset(s);
> > > >
> > > > -    imx_spi_update_irq(s);
> > > > -
> > > >      s->burst_length = 0;
> > > >  }
> > > >
> > > > +static void imx_spi_hard_reset(IMXSPIState *s)
> > > > +{
> > > > +    imx_spi_reset(DEVICE(s));
> > > > +
> > > > +    imx_spi_update_irq(s);
> > > > +}
> > > > +
> > > >  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> > > >  {
> > > >      uint32_t value = 0;
> > > > @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> > > >          s->regs[ECSPI_CONREG] = value;
> > > >
> > > >          if (!imx_spi_is_enabled(s)) {
> > > > -            /* device is disabled, so this is a reset */
> > > > -            imx_spi_reset(DEVICE(s));
> > > > +            /* device is disabled, so this is a hard reset */
> > > > +            imx_spi_hard_reset(s);
> > > > +
> > > >              return;
> > > >          }
> > >
> > > The function of the code is correct, but you seem to have the function
> > > naming backwards here. Generally:
> > >  * soft reset == the reset triggered by the register write
> > >  * hard reset == power-on reset == the dc->reset function
> > >
> > > I think this is what Philippe was trying to say.
> >
> > Philippe said: "Hmm usually hard reset include soft reset."
>
> True in hardware, but for QEMU there are some things we don't
> want to do in what we would call a hard or power-on reset.
>

OK, will revert to use imx_spi_soft_reset().

> > Since we are moving imx_spi_update_irq() out of imx_spi_reset() to a
> > new function called imx_spi_soft_reset() (what I did in v3), that
> > confused him (and I felt the same thing), so I renamed
> > imx_spi_soft_reset() to imx_spi_hard_reset() in v4..

Regards,
Bin

Re: [PATCH v4 2/6] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Philippe Mathieu-Daudé]

Hi Ben,

On 1/12/21 2:22 PM, Bin Meng wrote:
> On Tue, Jan 12, 2021 at 9:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 12 Jan 2021 at 12:54, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Tue, Jan 12, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Sun, 10 Jan 2021 at 08:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>
>>>>> From: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> Usually the approach is that the device on the other end of the line
>>>>> is going to reset its state anyway, so there's no need to actively
>>>>> signal an irq line change during the reset hook.
>>>>>
>>>>> Move imx_spi_update_irq() out of imx_spi_reset(), to a new function
>>>>> imx_spi_hard_reset() that is called when the controller is disabled.
>>>>>
>>>>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v4:
>>>>> - adujst the patch 2,3 order
>>>>> - rename imx_spi_soft_reset() to imx_spi_hard_reset() to avoid confusion
>>>>>
>>>>> Changes in v3:
>>>>> - new patch: remove imx_spi_update_irq() in imx_spi_reset()
>>>>>
>>>>>  hw/ssi/imx_spi.c | 14 ++++++++++----
>>>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>>>>> index e605049a21..2c4c5ec1b8 100644
>>>>> --- a/hw/ssi/imx_spi.c
>>>>> +++ b/hw/ssi/imx_spi.c
>>>>> @@ -241,11 +241,16 @@ static void imx_spi_reset(DeviceState *dev)
>>>>>      imx_spi_rxfifo_reset(s);
>>>>>      imx_spi_txfifo_reset(s);
>>>>>
>>>>> -    imx_spi_update_irq(s);
>>>>> -
>>>>>      s->burst_length = 0;
>>>>>  }
>>>>>
>>>>> +static void imx_spi_hard_reset(IMXSPIState *s)
>>>>> +{
>>>>> +    imx_spi_reset(DEVICE(s));
>>>>> +
>>>>> +    imx_spi_update_irq(s);
>>>>> +}
>>>>> +
>>>>>  static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
>>>>>  {
>>>>>      uint32_t value = 0;
>>>>> @@ -351,8 +356,9 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>>>>>          s->regs[ECSPI_CONREG] = value;
>>>>>
>>>>>          if (!imx_spi_is_enabled(s)) {
>>>>> -            /* device is disabled, so this is a reset */
>>>>> -            imx_spi_reset(DEVICE(s));
>>>>> +            /* device is disabled, so this is a hard reset */
>>>>> +            imx_spi_hard_reset(s);
>>>>> +
>>>>>              return;
>>>>>          }
>>>>
>>>> The function of the code is correct, but you seem to have the function
>>>> naming backwards here. Generally:
>>>>  * soft reset == the reset triggered by the register write
>>>>  * hard reset == power-on reset == the dc->reset function
>>>>
>>>> I think this is what Philippe was trying to say.
>>>
>>> Philippe said: "Hmm usually hard reset include soft reset."
>>
>> True in hardware, but for QEMU there are some things we don't
>> want to do in what we would call a hard or power-on reset.

Sorry for the confusion. I guess you understood me well, but
I was wrong. Anyhow I'll try to sort this discussion out with
my English teacher so the next time such confusion doesn't
happen again.

Thanks,

Phil.

> OK, will revert to use imx_spi_soft_reset().
> 
>>> Since we are moving imx_spi_update_irq() out of imx_spi_reset() to a
>>> new function called imx_spi_soft_reset() (what I did in v3), that
>>> confused him (and I felt the same thing), so I renamed
>>> imx_spi_soft_reset() to imx_spi_hard_reset() in v4..
> 
> Regards,
> Bin
> 

Re: [PATCH v4 6/6] hw/ssi: imx_spi: Correct tx and rx fifo endianness

[Philippe Mathieu-Daudé]

On 1/12/21 1:48 PM, Bin Meng wrote:
> On Tue, Jan 12, 2021 at 6:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Sun, 10 Jan 2021 at 08:15, 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.
[...]
>>
>> This version of the loop definitely looks a lot neater. However,
>> looking at the code I don't think there's anything that forces the
>> guest to set a burst length that's a multiple of 8, so you need
>> to handle that somehow. Otherwise on the last time through the
>> loop (tx_burst - 8) can be negative, which is undefined behaviour
>> when you try to shift by it.
> 
> Yes, that's why I added a patch to log the unimplemented behavior to
> notify the user.
> 
>> I think just rounding tx_burst up to a multiple of 8 before
>> the start of the loop would do the right thing ?
> 
> Probably. Given all flash transfers are normally multiple of 8-bits I
> am not sure what the real hardware behavior is when it is not multiple
> of 8, but I will try to add something in the next version.

FWIW not multiple of 8 use is not that uncommon, see:
https://guruce.com/blogpost/freescale-imx53-and-imx6-ecspi-silicon-bug