[RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Philippe Mathieu-Daudé]

Hi,

As it is sometimes harder for me to express myself in plain
English, I found it easier to write the patches I was thinking
about. I know this doesn't scale.

So this is how I understand the ecSPI reset works, after
looking at the IMX6DQRM.pdf datasheet.

This is a respin of Ben's v5 series [*].
Tagged RFC because I have not tested it :)

Sometimes changing device reset to better match hardware gives
trouble when using '-kernel ...' because there is no bootloader
setting the device in the state Linux expects it.

Copy of Ben's v5 cover:

  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()
  - round up the tx burst length to be multiple of 8
  - transfer incorrect data when the burst length is larger than 32 bit
  - spi controller tx and rx fifo endianness is incorrect

[*] https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg02333.html

Diff with Ben's v5:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respective=
ly

001/11:[----] [--] 'hw/ssi: imx_spi: Use a macro for number of chip selects s=
upported'
002/11:[down] 'hw/ssi: imx_spi: Remove pointless variable initialization'
003/11:[down] 'hw/ssi: imx_spi: Convert some debug printf()s to trace events'
004/11:[down] 'hw/ssi: imx_spi: Reduce 'change_mask' variable scope'
005/11:[down] 'hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG registe=
r value'
006/11:[down] 'hw/ssi: imx_spi: Rework imx_spi_read() to handle block disable=
d'
007/11:[down] 'hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabl=
ed'
008/11:[0004] [FC] 'hw/ssi: imx_spi: Disable chip selects when controller is =
disabled'
009/11:[----] [--] 'hw/ssi: imx_spi: Round up the burst length to be multiple=
 of 8'
010/11:[----] [--] 'hw/ssi: imx_spi: Correct the burst length > 32 bit transf=
er logic'
011/11:[----] [--] 'hw/ssi: imx_spi: Correct tx and rx fifo endianness'

Bin Meng (4):
  hw/ssi: imx_spi: Use a macro for number of chip selects supported
  hw/ssi: imx_spi: Round up the burst length to be multiple of 8
  hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic
  hw/ssi: imx_spi: Correct tx and rx fifo endianness

Philippe Mathieu-Daud=C3=A9 (6):
  hw/ssi: imx_spi: Remove pointless variable initialization
  hw/ssi: imx_spi: Convert some debug printf()s to trace events
  hw/ssi: imx_spi: Reduce 'change_mask' variable scope
  hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value
  hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled
  hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

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         | 147 +++++++++++++++++++++++----------------
 hw/ssi/trace-events      |   7 ++
 3 files changed, 97 insertions(+), 62 deletions(-)

--=20
2.26.2

[RFC PATCH v6 01/11] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Philippe Mathieu-Daudé]

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>
Message-Id: <20210112145526.31095-2-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 b82b17f3643..eeaf49bbac3 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 d8885ae454e..e605049a213 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.26.2

Re: [RFC PATCH v6 01/11] hw/ssi: imx_spi: Use a macro for number of chip selects supported

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 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>
> Message-Id: <20210112145526.31095-2-bmeng.cn@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

A fast search don't show what resets cs_lines, but that is independent
of this patch.

[RFC PATCH v6 02/11] hw/ssi: imx_spi: Remove pointless variable initialization

[Philippe Mathieu-Daudé]

'burst_length' is cleared in imx_spi_reset(), which is called
after imx_spi_realize(). Remove the initialization to simplify.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index e605049a213..40f72c36b61 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -428,8 +428,6 @@ static void imx_spi_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cs_lines[i]);
     }
 
-    s->burst_length = 0;
-
     fifo32_create(&s->tx_fifo, ECSPI_FIFO_SIZE);
     fifo32_create(&s->rx_fifo, ECSPI_FIFO_SIZE);
 }
-- 
2.26.2

Re: [RFC PATCH v6 02/11] hw/ssi: imx_spi: Remove pointless variable initialization

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 'burst_length' is cleared in imx_spi_reset(), which is called
> after imx_spi_realize(). Remove the initialization to simplify.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[RFC PATCH v6 03/11] hw/ssi: imx_spi: Convert some debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert some DPRINTF() to trace events.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c    | 8 ++++----
 hw/ssi/trace-events | 7 +++++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 40f72c36b61..35ab33c0511 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -14,6 +14,7 @@
 #include "migration/vmstate.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 #ifndef DEBUG_IMX_SPI
 #define DEBUG_IMX_SPI 0
@@ -232,7 +233,7 @@ static void imx_spi_reset(DeviceState *dev)
 {
     IMXSPIState *s = IMX_SPI(dev);
 
-    DPRINTF("\n");
+    trace_imx_spi_reset();
 
     memset(s->regs, 0, sizeof(s->regs));
 
@@ -290,7 +291,7 @@ static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
         break;
     }
 
-    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
+    trace_imx_spi_read(index, imx_spi_reg_name(index), value);
 
     imx_spi_update_irq(s);
 
@@ -310,8 +311,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
         return;
     }
 
-    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
-            (uint32_t)value);
+    trace_imx_spi_write(index, imx_spi_reg_name(index), value);
 
     change_mask = s->regs[index] ^ value;
 
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
index 612d3d6087a..20fcaf32df6 100644
--- a/hw/ssi/trace-events
+++ b/hw/ssi/trace-events
@@ -1,3 +1,5 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
 # aspeed_smc.c
 
 aspeed_smc_flash_set_segment(int cs, uint64_t reg, uint64_t start, uint64_t end) "CS%d segreg=0x%"PRIx64" [ 0x%"PRIx64" - 0x%"PRIx64" ]"
@@ -10,6 +12,11 @@ aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t dram_addr, uint
 aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
 aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
 
+# imx_spi.c
+imx_spi_reset(void) ""
+imx_spi_read(uint32_t index, const char *name, uint32_t value) "index:%u (%s) value:0x%08x"
+imx_spi_write(uint32_t index, const char *name, uint32_t value) "index:%u (%s) value:0x%08x"
+
 # npcm7xx_fiu.c
 
 npcm7xx_fiu_enter_reset(const char *id, int reset_type) "%s reset type: %d"
-- 
2.26.2

Re: [RFC PATCH v6 03/11] hw/ssi: imx_spi: Convert some debug printf()s to trace events

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Convert some DPRINTF() to trace events.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> index 612d3d6087a..20fcaf32df6 100644
> --- a/hw/ssi/trace-events
> +++ b/hw/ssi/trace-events
> @@ -1,3 +1,5 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
>  # aspeed_smc.c
>  
>  aspeed_smc_flash_set_segment(int cs, uint64_t reg, uint64_t start, uint64_t end) "CS%d segreg=0x%"PRIx64" [ 0x%"PRIx64" - 0x%"PRIx64" ]"

Not that I am against the comment, but it looks spurious on this patch.

[RFC PATCH v6 04/11] hw/ssi: imx_spi: Reduce 'change_mask' variable scope

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 35ab33c0511..bcc535f2893 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -303,7 +303,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
 {
     IMXSPIState *s = opaque;
     uint32_t index = offset >> 2;
-    uint32_t change_mask;
 
     if (index >=  ECSPI_MAX) {
         qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
@@ -313,7 +312,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
 
     trace_imx_spi_write(index, imx_spi_reg_name(index), value);
 
-    change_mask = s->regs[index] ^ value;
 
     switch (index) {
     case ECSPI_RXDATA:
@@ -357,6 +355,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
         }
 
         if (imx_spi_channel_is_master(s)) {
+            uint32_t change_mask = s->regs[index] ^ value;
             int i;
 
             /* We are in master mode */
-- 
2.26.2

Re: [RFC PATCH v6 04/11] hw/ssi: imx_spi: Reduce 'change_mask' variable scope

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I think this one is wrong.


> ---
>  hw/ssi/imx_spi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 35ab33c0511..bcc535f2893 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -303,7 +303,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>  {
>      IMXSPIState *s = opaque;
>      uint32_t index = offset >> 2;
> -    uint32_t change_mask;
>  
>      if (index >=  ECSPI_MAX) {
>          qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> @@ -313,7 +312,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>  
>      trace_imx_spi_write(index, imx_spi_reg_name(index), value);
>  
> -    change_mask = s->regs[index] ^ value;
>  
>      switch (index) {
>      case ECSPI_RXDATA:
> @@ -357,6 +355,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>          }
>  
>          if (imx_spi_channel_is_master(s)) {
> +            uint32_t change_mask = s->regs[index] ^ value;
>              int i;
>  
>              /* We are in master mode */

The code does:

    change_mask = s->regs[index] ^ value;

    switch (index) {

    ...

    case ECSPI_CONREG:
        s->regs[ECSPI_CONREG] = value;  <<---- here

        if (!imx_spi_is_enabled(s)) {
            /* device is disabled, so this is a reset */
            imx_spi_reset(DEVICE(s));
            return;
        }

        if (imx_spi_channel_is_master(s)) {
            int i;
       >>>>>  You are setting change_mask here.

At this point, s->regs[index] has a new value in "here".

Later, Juan.

Re: [RFC PATCH v6 04/11] hw/ssi: imx_spi: Reduce 'change_mask' variable scope

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> I think this one is wrong.

Wrong is a strong word.  I mean that it changes behaviour and the commit
message don't talk about changing behaviour.

Later, Juan.

>
>
>> ---
>>  hw/ssi/imx_spi.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>> index 35ab33c0511..bcc535f2893 100644
>> --- a/hw/ssi/imx_spi.c
>> +++ b/hw/ssi/imx_spi.c
>> @@ -303,7 +303,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>>  {
>>      IMXSPIState *s = opaque;
>>      uint32_t index = offset >> 2;
>> -    uint32_t change_mask;
>>  
>>      if (index >=  ECSPI_MAX) {
>>          qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
>> @@ -313,7 +312,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>>  
>>      trace_imx_spi_write(index, imx_spi_reg_name(index), value);
>>  
>> -    change_mask = s->regs[index] ^ value;
>>  
>>      switch (index) {
>>      case ECSPI_RXDATA:
>> @@ -357,6 +355,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>>          }
>>  
>>          if (imx_spi_channel_is_master(s)) {
>> +            uint32_t change_mask = s->regs[index] ^ value;
>>              int i;
>>  
>>              /* We are in master mode */
>
> The code does:
>
>     change_mask = s->regs[index] ^ value;
>
>     switch (index) {
>
>     ...
>
>     case ECSPI_CONREG:
>         s->regs[ECSPI_CONREG] = value;  <<---- here
>
>         if (!imx_spi_is_enabled(s)) {
>             /* device is disabled, so this is a reset */
>             imx_spi_reset(DEVICE(s));
>             return;
>         }
>
>         if (imx_spi_channel_is_master(s)) {
>             int i;
>        >>>>>  You are setting change_mask here.
>
> At this point, s->regs[index] has a new value in "here".
>
> Later, Juan.

Re: [RFC PATCH v6 04/11] hw/ssi: imx_spi: Reduce 'change_mask' variable scope

[Philippe Mathieu-Daudé]

On 1/13/21 2:47 PM, Juan Quintela wrote:
> Juan Quintela <quintela@redhat.com> wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> I think this one is wrong.
> 
> Wrong is a strong word.  I mean that it changes behaviour and the commit
> message don't talk about changing behaviour.

Indeed. Well I'll simply drop this patch as it is not essential.

Thanks for reviewing the series!

Phil.

[RFC PATCH v6 05/11] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Philippe Mathieu-Daudé]

When the block is disabled, all registers are reset with the
exception of the ECSPI_CONREG. It is initialized to zero
when the instance is created.

Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
     chapter 21.7.3: Control Register (ECSPIx_CONREG)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index bcc535f2893..96aecc8fa28 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -232,12 +232,23 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
 static void imx_spi_reset(DeviceState *dev)
 {
     IMXSPIState *s = IMX_SPI(dev);
+    unsigned i;
 
     trace_imx_spi_reset();
 
-    memset(s->regs, 0, sizeof(s->regs));
-
-    s->regs[ECSPI_STATREG] = 0x00000003;
+    for (i = 0; i < ARRAY_SIZE(s->regs); i++) {
+        switch (i) {
+        case ECSPI_CONREG:
+            /* CONREG is not updated on reset */
+            break;
+        case ECSPI_STATREG:
+            s->regs[i] = 0x00000003;
+            break;
+        default:
+            s->regs[i] = 0;
+            break;
+        }
+    }
 
     imx_spi_rxfifo_reset(s);
     imx_spi_txfifo_reset(s);
-- 
2.26.2

Re: [RFC PATCH v6 05/11] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> When the block is disabled, all registers are reset with the
> exception of the ECSPI_CONREG. It is initialized to zero
> when the instance is created.
>
> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
>      chapter 21.7.3: Control Register (ECSPIx_CONREG)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I trust your reading of the documentation O:-)

[RFC PATCH v6 06/11] hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled

[Philippe Mathieu-Daudé]

When the block is disabled, it stay it is 'internal reset logic'
(internal clocks are gated off). Reading any register returns
its reset value. Only update this value if the device is enabled.

Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
     chapter 21.7.3: Control Register (ECSPIx_CONREG)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 60 +++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 96aecc8fa28..7ac9da0f1d2 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -270,42 +270,40 @@ static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
         return 0;
     }
 
-    switch (index) {
-    case ECSPI_RXDATA:
-        if (!imx_spi_is_enabled(s)) {
-            value = 0;
-        } else if (fifo32_is_empty(&s->rx_fifo)) {
-            /* value is undefined */
-            value = 0xdeadbeef;
-        } else {
-            /* read from the RX FIFO */
-            value = fifo32_pop(&s->rx_fifo);
+    value = s->regs[index];
+
+    if (imx_spi_is_enabled(s)) {
+        switch (index) {
+        case ECSPI_RXDATA:
+            if (fifo32_is_empty(&s->rx_fifo)) {
+                /* value is undefined */
+                value = 0xdeadbeef;
+            } else {
+                /* read from the RX FIFO */
+                value = fifo32_pop(&s->rx_fifo);
+            }
+            break;
+        case ECSPI_TXDATA:
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "[%s]%s: Trying to read from TX FIFO\n",
+                          TYPE_IMX_SPI, __func__);
+
+            /* Reading from TXDATA gives 0 */
+            break;
+        case ECSPI_MSGDATA:
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "[%s]%s: Trying to read from MSG FIFO\n",
+                          TYPE_IMX_SPI, __func__);
+            /* Reading from MSGDATA gives 0 */
+            break;
+        default:
+            break;
         }
 
-        break;
-    case ECSPI_TXDATA:
-        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from TX FIFO\n",
-                      TYPE_IMX_SPI, __func__);
-
-        /* Reading from TXDATA gives 0 */
-
-        break;
-    case ECSPI_MSGDATA:
-        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from MSG FIFO\n",
-                      TYPE_IMX_SPI, __func__);
-
-        /* Reading from MSGDATA gives 0 */
-
-        break;
-    default:
-        value = s->regs[index];
-        break;
+        imx_spi_update_irq(s);
     }
-
     trace_imx_spi_read(index, imx_spi_reg_name(index), value);
 
-    imx_spi_update_irq(s);
-
     return (uint64_t)value;
 }
 
-- 
2.26.2

Re: [RFC PATCH v6 06/11] hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> When the block is disabled, it stay it is 'internal reset logic'
> (internal clocks are gated off). Reading any register returns
> its reset value. Only update this value if the device is enabled.
>
> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
>      chapter 21.7.3: Control Register (ECSPIx_CONREG)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[RFC PATCH v6 07/11] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Philippe Mathieu-Daudé]

When the block is disabled, only the ECSPI_CONREG register can
be modified. Setting the EN bit enabled the device, clearing it
"disables the block and resets the internal logic with the
exception of the ECSPI_CONREG" register.

Move the imx_spi_is_enabled() check earlier.

Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM),
     chapter 21.7.3: Control Register (ECSPIx_CONREG)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 7ac9da0f1d2..801daa5cbfa 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -321,6 +321,20 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
 
     trace_imx_spi_write(index, imx_spi_reg_name(index), value);
 
+    if (!imx_spi_is_enabled(s)) {
+        /* Block is disabled */
+        if (index != ECSPI_CONREG) {
+            /* Ignore access */
+            return;
+        }
+        s->regs[ECSPI_CONREG] = value;
+        if (value & ECSPI_CONREG_EN) {
+            /* Keep disabled */
+            return;
+        }
+        /* Enable the block */
+        imx_spi_reset(DEVICE(s));
+    }
 
     switch (index) {
     case ECSPI_RXDATA:
@@ -328,10 +342,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
                       TYPE_IMX_SPI, __func__);
         break;
     case ECSPI_TXDATA:
-        if (!imx_spi_is_enabled(s)) {
-            /* Ignore writes if device is disabled */
-            break;
-        } else if (fifo32_is_full(&s->tx_fifo)) {
+        if (fifo32_is_full(&s->tx_fifo)) {
             /* Ignore writes if queue is full */
             break;
         }
@@ -357,12 +368,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
     case ECSPI_CONREG:
         s->regs[ECSPI_CONREG] = value;
 
-        if (!imx_spi_is_enabled(s)) {
-            /* device is disabled, so this is a reset */
-            imx_spi_reset(DEVICE(s));
-            return;
-        }
-
         if (imx_spi_channel_is_master(s)) {
             uint32_t change_mask = s->regs[index] ^ value;
             int i;
-- 
2.26.2

[RFC PATCH v6 08/11] hw/ssi: imx_spi: Disable chip selects when controller is disabled

[Philippe Mathieu-Daudé]

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210112145526.31095-4-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 801daa5cbfa..2f9e800dd3a 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -255,6 +255,10 @@ static void imx_spi_reset(DeviceState *dev)
 
     imx_spi_update_irq(s);
 
+    for (i = 0; i < ECSPI_NUM_CS; i++) {
+        qemu_set_irq(s->cs_lines[i], 1);
+    }
+
     s->burst_length = 0;
 }
 
-- 
2.26.2

[RFC PATCH v6 09/11] hw/ssi: imx_spi: Round up the burst length to be multiple of 8

[Philippe Mathieu-Daudé]

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, and round it up to be multiple of 8.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Message-Id: <20210112145526.31095-5-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/imx_spi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 2f9e800dd3a..638959daa08 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -129,7 +129,20 @@ 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);
+        burst = ROUND_UP(burst, 8);
+        qemu_log_mask(LOG_UNIMP,
+                      "[%s]%s: burst length rounded up to %d; this may not work.\n",
+                      TYPE_IMX_SPI, __func__, burst);
+    }
+
+    return burst;
 }
 
 static bool imx_spi_is_enabled(IMXSPIState *s)
-- 
2.26.2

[RFC PATCH v6 10/11] hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic

[Philippe Mathieu-Daudé]

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>
Message-Id: <20210112145526.31095-6-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 638959daa08..c4e2d2e1c97 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -192,7 +192,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.26.2

[RFC PATCH v6 11/11] hw/ssi: imx_spi: Correct tx and rx fifo endianness

[Philippe Mathieu-Daudé]

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>
Message-Id: <20210112145526.31095-7-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 c4e2d2e1c97..38892698918 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -176,7 +176,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);
@@ -197,7 +196,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);
 
@@ -206,13 +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.26.2

Re: [RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

Hi Philippe,

On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi,
>
> As it is sometimes harder for me to express myself in plain
> English, I found it easier to write the patches I was thinking
> about. I know this doesn't scale.
>
> So this is how I understand the ecSPI reset works, after
> looking at the IMX6DQRM.pdf datasheet.
>
> This is a respin of Ben's v5 series [*].
> Tagged RFC because I have not tested it :)

Unfortunately this series breaks SPI flash testing under both U-Boot
and VxWorks 7.

> Sometimes changing device reset to better match hardware gives
> trouble when using '-kernel ...' because there is no bootloader
> setting the device in the state Linux expects it.
>

Given most of the new changes in this RFC series are clean-ups, I
suggest we apply the v5 series unless there is anything seriously
wrong in v5, IOW, don't fix it unless it's broken.

Thoughts?

Regards,
Bin

Re: [RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Philippe Mathieu-Daudé]

Hi Ben,

On 1/13/21 4:29 AM, Bin Meng wrote:
> On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi,
>>
>> As it is sometimes harder for me to express myself in plain
>> English, I found it easier to write the patches I was thinking
>> about. I know this doesn't scale.
>>
>> So this is how I understand the ecSPI reset works, after
>> looking at the IMX6DQRM.pdf datasheet.
>>
>> This is a respin of Ben's v5 series [*].
>> Tagged RFC because I have not tested it :)
> 
> Unfortunately this series breaks SPI flash testing under both U-Boot
> and VxWorks 7.

Thanks for testing :) Can you provide the binary tested and the command
line used? At least one, so I can have a look.

>> Sometimes changing device reset to better match hardware gives
>> trouble when using '-kernel ...' because there is no bootloader
>> setting the device in the state Linux expects it.
>>
> 
> Given most of the new changes in this RFC series are clean-ups, I
> suggest we apply the v5 series unless there is anything seriously
> wrong in v5, IOW, don't fix it unless it's broken.
> 
> Thoughts?

Up to the maintainer :)

The IMX6DQRM datasheet is available here:
https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-1/ta-p/1115983
https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-2/ta-p/1118510

Regards,

Phil.

Re: [RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

Hi Philippe,

On Wed, Jan 13, 2021 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Ben,
>
> On 1/13/21 4:29 AM, Bin Meng wrote:
> > On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi,
> >>
> >> As it is sometimes harder for me to express myself in plain
> >> English, I found it easier to write the patches I was thinking
> >> about. I know this doesn't scale.
> >>
> >> So this is how I understand the ecSPI reset works, after
> >> looking at the IMX6DQRM.pdf datasheet.
> >>
> >> This is a respin of Ben's v5 series [*].
> >> Tagged RFC because I have not tested it :)
> >
> > Unfortunately this series breaks SPI flash testing under both U-Boot
> > and VxWorks 7.
>
> Thanks for testing :) Can you provide the binary tested and the command
> line used? At least one, so I can have a look.

Sure, will send you offline.

>
> >> Sometimes changing device reset to better match hardware gives
> >> trouble when using '-kernel ...' because there is no bootloader
> >> setting the device in the state Linux expects it.
> >>
> >
> > Given most of the new changes in this RFC series are clean-ups, I
> > suggest we apply the v5 series unless there is anything seriously
> > wrong in v5, IOW, don't fix it unless it's broken.
> >
> > Thoughts?
>
> Up to the maintainer :)
>
> The IMX6DQRM datasheet is available here:
> https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-1/ta-p/1115983
> https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-2/ta-p/1118510

Regards,
Bin

Re: [RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

Hi Philippe,

On Wed, Jan 13, 2021 at 9:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Philippe,
>
> On Wed, Jan 13, 2021 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > Hi Ben,
> >
> > On 1/13/21 4:29 AM, Bin Meng wrote:
> > > On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >>
> > >> Hi,
> > >>
> > >> As it is sometimes harder for me to express myself in plain
> > >> English, I found it easier to write the patches I was thinking
> > >> about. I know this doesn't scale.
> > >>
> > >> So this is how I understand the ecSPI reset works, after
> > >> looking at the IMX6DQRM.pdf datasheet.
> > >>
> > >> This is a respin of Ben's v5 series [*].
> > >> Tagged RFC because I have not tested it :)
> > >
> > > Unfortunately this series breaks SPI flash testing under both U-Boot
> > > and VxWorks 7.
> >
> > Thanks for testing :) Can you provide the binary tested and the command
> > line used? At least one, so I can have a look.
>
> Sure, will send you offline.

Please use attached u-boot image to test.

You will also need the following additional QEMU patches:

http://patchwork.ozlabs.org/project/qemu-devel/patch/1606704602-59435-1-git-send-email-bmeng.cn@gmail.com/
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=221754

$ qemu-system-arm -display none -serial null -serial stdio -M
sabrelite -m 1G -kernel u-boot

=> sf probe
SF: Detected sst25vf016b with page size 256 Bytes, erase size 4 KiB, total 2 MiB
=> sf test 1ff000 1000
SPI flash test:
0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps
1 check: 2 ticks, 2000 KiB/s 16.000 Mbps
2 write: 187 ticks, 21 KiB/s 0.168 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: 2 ticks, 2000 KiB/s 16.000 Mbps
2 write: 187 ticks, 21 KiB/s 0.168 Mbps
3 read: 2 ticks, 2000 KiB/s 16.000 Mbps

Regards,
Bin

[-- Attachment #2: u-boot --]
[-- Type: application/octet-stream, Size: 616136 bytes --]

Re: [RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Philippe Mathieu-Daudé]

On 1/13/21 2:27 PM, Bin Meng wrote:
> Hi Philippe,
> 
> On Wed, Jan 13, 2021 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Ben,
>>
>> On 1/13/21 4:29 AM, Bin Meng wrote:
>>> On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> As it is sometimes harder for me to express myself in plain
>>>> English, I found it easier to write the patches I was thinking
>>>> about. I know this doesn't scale.
>>>>
>>>> So this is how I understand the ecSPI reset works, after
>>>> looking at the IMX6DQRM.pdf datasheet.
>>>>
>>>> This is a respin of Ben's v5 series [*].
>>>> Tagged RFC because I have not tested it :)
>>>
>>> Unfortunately this series breaks SPI flash testing under both U-Boot
>>> and VxWorks 7.
>>
>> Thanks for testing :) Can you provide the binary tested and the command
>> line used? At least one, so I can have a look.
> 
> Sure, will send you offline.

Arf, stupid mistake in patch 7 :) With this diff I can run your
test:

-- >8 --
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -343,7 +343,7 @@ static void imx_spi_write(void *opaque, hwaddr
offset, uint64_t value,
             return;
         }
         s->regs[ECSPI_CONREG] = value;
-        if (value & ECSPI_CONREG_EN) {
+        if (!(value & ECSPI_CONREG_EN)) {
             /* Keep disabled */
             return;
         }
---

Regards,

Phil.

Re: [RFC PATCH v6 00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Philippe Mathieu-Daudé]

Hi Ben,

On 1/13/21 6:56 PM, Philippe Mathieu-Daudé wrote:
> On 1/13/21 2:27 PM, Bin Meng wrote:
>> Hi Philippe,
>>
>>>> Unfortunately this series breaks SPI flash testing under both U-Boot
>>>> and VxWorks 7.
>>>
>>> Thanks for testing :) Can you provide the binary tested and the command
>>> line used? At least one, so I can have a look.
>>
>> Sure, will send you offline.
> 
> Arf, stupid mistake in patch 7 :) With this diff I can run your
> test:
> 
> -- >8 --
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -343,7 +343,7 @@ static void imx_spi_write(void *opaque, hwaddr
> offset, uint64_t value,
>              return;
>          }
>          s->regs[ECSPI_CONREG] = value;
> -        if (value & ECSPI_CONREG_EN) {
> +        if (!(value & ECSPI_CONREG_EN)) {
>              /* Keep disabled */
>              return;
>          }
> ---

Could you have a try at this? Do you prefer I resubmit
the whole series?

Thanks,

Phil.