[PATCH v7 0/9] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[PATCH v7 0/9] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Philippe Mathieu-Daudé]

Hi,

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 [*].

Since v6:
- Dropped "Reduce 'change_mask' variable scope" patch
- Fixed inverted reset logic
- Added Juan R-b tags
- Removed 'RFC' tag as tests pass

Based-on: <1608688825-81519-1-git-send-email-bmeng.cn@gmail.com>
(queued on riscv-next).

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 v6:

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/9:[----] [--] 'hw/ssi: imx_spi: Use a macro for number of chip selects su=
pported'
002/9:[----] [--] 'hw/ssi: imx_spi: Remove pointless variable initialization'
003/9:[----] [-C] 'hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG reg=
ister value'
004/9:[----] [-C] 'hw/ssi: imx_spi: Rework imx_spi_read() to handle block dis=
abled'
005/9:[0003] [FC] 'hw/ssi: imx_spi: Rework imx_spi_write() to handle block di=
sabled'
006/9:[----] [--] 'hw/ssi: imx_spi: Disable chip selects when controller is d=
isabled'
007/9:[----] [--] 'hw/ssi: imx_spi: Round up the burst length to be multiple =
of 8'
008/9:[----] [--] 'hw/ssi: imx_spi: Correct the burst length > 32 bit transfe=
r logic'
009/9:[----] [--] '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 (4):
  hw/ssi: imx_spi: Remove pointless variable initialization
  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         | 137 +++++++++++++++++++++++----------------
 2 files changed, 86 insertions(+), 56 deletions(-)

--=20
2.26.2

[PATCH v7 1/9] 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>
Reviewed-by: Juan Quintela <quintela@redhat.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

[PATCH v7 2/9] 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.

Reviewed-by: Juan Quintela <quintela@redhat.com>
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

[PATCH v7 3/9] 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)

Reviewed-by: Juan Quintela <quintela@redhat.com>
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 40f72c36b61..78b19c2eb91 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -231,12 +231,23 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
 static void imx_spi_reset(DeviceState *dev)
 {
     IMXSPIState *s = IMX_SPI(dev);
+    unsigned i;
 
     DPRINTF("\n");
 
-    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

[PATCH v7 4/9] 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)

Reviewed-by: Juan Quintela <quintela@redhat.com>
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 78b19c2eb91..ba7d3438d87 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -269,42 +269,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);
     }
-
     DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
 
-    imx_spi_update_irq(s);
-
     return (uint64_t)value;
 }
 
-- 
2.26.2

[PATCH v7 5/9] 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 | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index ba7d3438d87..f06bbf317e2 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
     DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
             (uint32_t)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));
+    }
+
     change_mask = s->regs[index] ^ value;
 
     switch (index) {
@@ -330,10 +345,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;
         }
@@ -359,12 +371,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)) {
             int i;
 
-- 
2.26.2

[PATCH v7 6/9] 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 f06bbf317e2..c132f99ba5b 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -254,6 +254,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

[PATCH v7 7/9] 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 c132f99ba5b..b79304d93d9 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -128,7 +128,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

[PATCH v7 8/9] 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 b79304d93d9..707defb8b3f 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -191,7 +191,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

[PATCH v7 9/9] 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 707defb8b3f..081b7e464ff 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -175,7 +175,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);
@@ -196,7 +195,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);
 
@@ -205,13 +204,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: [PATCH v7 4/9] hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled

[Bin Meng]

On Fri, Jan 15, 2021 at 11:37 PM 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)
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 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 78b19c2eb91..ba7d3438d87 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -269,42 +269,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 */

The new logic is a little bit non straight forward as the value 0
comes from s->regs[index] which was never written hence 0. While the
previous logic is returning explicitly zero. Perhaps a comment update
is needed.

> +            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 */

ditto

> +            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);
>      }
> -
>      DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
>
> -    imx_spi_update_irq(s);
> -
>      return (uint64_t)value;
>  }

Regards,
Bin

Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Bin Meng]

On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> When the block is disabled, only the ECSPI_CONREG register can
> be modified. Setting the EN bit enabled the device, clearing it

I don't know how this conclusion came out. The manual only says the
following 2 registers ignore the write when the block is disabled.

ECSPI_TXDATA, ECSPI_INTREG

> "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 | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index ba7d3438d87..f06bbf317e2 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
>              (uint32_t)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 */

So other bits except ECSPI_CONREG_EN are discarded? The manual does
not explicitly mention this but this looks suspicious.

> +            return;
> +        }
> +        /* Enable the block */
> +        imx_spi_reset(DEVICE(s));
> +    }
> +
>      change_mask = s->regs[index] ^ value;
>
>      switch (index) {
> @@ -330,10 +345,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;
>          }
> @@ -359,12 +371,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)) {
>              int i;
>

Regards,
Bin

Re: [PATCH v7 0/9] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

Hi Philippe,

On Fri, Jan 15, 2021 at 11:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi,
>
> 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 [*].
>
> Since v6:
> - Dropped "Reduce 'change_mask' variable scope" patch
> - Fixed inverted reset logic
> - Added Juan R-b tags
> - Removed 'RFC' tag as tests pass
>
> Based-on: <1608688825-81519-1-git-send-email-bmeng.cn@gmail.com>
> (queued on riscv-next).
>

This series dropped my imx_spi_soft_reset() change that has the
imx_spi_update_irq() moved from imx_spi_reset(). May I know why?

Regards,
Bin

Re: [PATCH v7 0/9] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Philippe Mathieu-Daudé]

On 1/16/21 3:03 PM, Bin Meng wrote:
> Hi Philippe,
> 
> On Fri, Jan 15, 2021 at 11:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi,
>>
>> 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 [*].
>>
>> Since v6:
>> - Dropped "Reduce 'change_mask' variable scope" patch
>> - Fixed inverted reset logic
>> - Added Juan R-b tags
>> - Removed 'RFC' tag as tests pass
>>
>> Based-on: <1608688825-81519-1-git-send-email-bmeng.cn@gmail.com>
>> (queued on riscv-next).
>>
> 
> This series dropped my imx_spi_soft_reset() change that has the
> imx_spi_update_irq() moved from imx_spi_reset(). May I know why?

Because we don't need it. My comment without looking at the datasheet
was incorrect: there is only one single reset on the block.

> 
> Regards,
> Bin
> 

Re: [PATCH v7 0/9] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

On Sat, Jan 16, 2021 at 11:07 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/16/21 3:03 PM, Bin Meng wrote:
> > Hi Philippe,
> >
> > On Fri, Jan 15, 2021 at 11:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi,
> >>
> >> 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 [*].
> >>
> >> Since v6:
> >> - Dropped "Reduce 'change_mask' variable scope" patch
> >> - Fixed inverted reset logic
> >> - Added Juan R-b tags
> >> - Removed 'RFC' tag as tests pass
> >>
> >> Based-on: <1608688825-81519-1-git-send-email-bmeng.cn@gmail.com>
> >> (queued on riscv-next).
> >>
> >
> > This series dropped my imx_spi_soft_reset() change that has the
> > imx_spi_update_irq() moved from imx_spi_reset(). May I know why?
>
> Because we don't need it. My comment without looking at the datasheet
> was incorrect: there is only one single reset on the block.

Oh, you must have missed Peter's comments.

See his comments here:
http://patchwork.ozlabs.org/project/qemu-devel/patch/20201202144523.24526-2-bmeng.cn@gmail.com/

Regards,
Bin

Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Philippe Mathieu-Daudé]

Hi Bin,

On 1/16/21 2:57 PM, Bin Meng wrote:
> On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> When the block is disabled, only the ECSPI_CONREG register can
>> be modified. Setting the EN bit enabled the device, clearing it
> 
> I don't know how this conclusion came out. The manual only says the
> following 2 registers ignore the write when the block is disabled.
> 
> ECSPI_TXDATA, ECSPI_INTREG

21.4.5 Reset

  Whenever a device reset occurs, a reset is performed on the
  ECSPI, resetting all registers to their default values.

My understanding is it is pointless to update them when the
device is in reset, as they will get their default value when
going out of reset.

> 
>> "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 | 26 ++++++++++++++++----------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>> index ba7d3438d87..f06bbf317e2 100644
>> --- a/hw/ssi/imx_spi.c
>> +++ b/hw/ssi/imx_spi.c
>> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
>>              (uint32_t)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 */
> 
> So other bits except ECSPI_CONREG_EN are discarded? The manual does
> not explicitly mention this but this looks suspicious.

See in [*], all bits from the register are updated. We simply check
ECSPI_CONREG_EN to see if we need to go out of reset.

See:

21.5 Initialization

  This section provides initialization information for ECSPI.

  To initialize the block:

  1. Clear the EN bit in ECSPI_CONREG to reset the block.
  2. Enable the clocks for ECSPI.
  3. Set the EN bit in ECSPI_CONREG to put ECSPI out of reset.
  4. Configure corresponding IOMUX for ECSPI external signals.
  5 Configure registers of ECSPI properly according to the
    specifications of the external SPI device.

And ECSPI_CONREG_EN bit description:

  SPI Block Enable Control. This bit enables the ECSPI. This bit
  must be set before writing to other registers or initiating an
  exchange. Writing zero to this bit disables the block and resets
  the internal logic with the exception of the ECSPI_CONREG. The
  block's internal clocks are gated off whenever the block is
  disabled.


I simply wanted to help you. I don't want to delay your work, so
if you think my approach is incorrect, suggest Peter to queue your
v5 or resend it (once riscv-next is merged) as v8.

Regards,

Phil.

Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Bin Meng]

Hi Philippe,

On Sat, Jan 16, 2021 at 11:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Bin,
>
> On 1/16/21 2:57 PM, Bin Meng wrote:
> > On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> When the block is disabled, only the ECSPI_CONREG register can
> >> be modified. Setting the EN bit enabled the device, clearing it
> >
> > I don't know how this conclusion came out. The manual only says the
> > following 2 registers ignore the write when the block is disabled.
> >
> > ECSPI_TXDATA, ECSPI_INTREG
>
> 21.4.5 Reset
>
>   Whenever a device reset occurs, a reset is performed on the
>   ECSPI, resetting all registers to their default values.
>
> My understanding is it is pointless to update them when the
> device is in reset, as they will get their default value when
> going out of reset.

I have a different understanding. When ECSPI_CONREG[EN] is cleared,
it's like a hardware reset, and the ECSPI takes the following action:

    "Whenever a device reset occurs, a reset is performed on the
ECSPI, resetting all registers to their default values."

Chapter 21.4.5 Reset does not mention what's the hardware behavior afterwards.

So my understanding is: afterwards, the software can still write to
various registers, unless the register description tells us it's
ignored.

>
> >
> >> "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 | 26 ++++++++++++++++----------
> >>  1 file changed, 16 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> >> index ba7d3438d87..f06bbf317e2 100644
> >> --- a/hw/ssi/imx_spi.c
> >> +++ b/hw/ssi/imx_spi.c
> >> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> >>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
> >>              (uint32_t)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 */
> >
> > So other bits except ECSPI_CONREG_EN are discarded? The manual does
> > not explicitly mention this but this looks suspicious.
>
> See in [*], all bits from the register are updated. We simply check
> ECSPI_CONREG_EN to see if we need to go out of reset.

Oops, I missed the [*] line. Now I have read this carefully, and found
there is one problem:

Now with the new logic the device reset activity has been postponed
until next time a device register is written. This is wrong.

>
> See:
>
> 21.5 Initialization
>
>   This section provides initialization information for ECSPI.
>
>   To initialize the block:
>
>   1. Clear the EN bit in ECSPI_CONREG to reset the block.
>   2. Enable the clocks for ECSPI.
>   3. Set the EN bit in ECSPI_CONREG to put ECSPI out of reset.
>   4. Configure corresponding IOMUX for ECSPI external signals.
>   5 Configure registers of ECSPI properly according to the
>     specifications of the external SPI device.
>
> And ECSPI_CONREG_EN bit description:
>
>   SPI Block Enable Control. This bit enables the ECSPI. This bit
>   must be set before writing to other registers or initiating an
>   exchange. Writing zero to this bit disables the block and resets
>   the internal logic with the exception of the ECSPI_CONREG. The
>   block's internal clocks are gated off whenever the block is
>   disabled.
>
>
> I simply wanted to help you. I don't want to delay your work, so
> if you think my approach is incorrect, suggest Peter to queue your
> v5 or resend it (once riscv-next is merged) as v8.

Thank you for the help. I mentioned in an earlier thread before, that
my view was not to fix it until it's broken as the v5 series can
satisfy my work. But since you pointed out various spec violation
stuff related to device reset, I do think your findings make sense. So
let's improve this model together. :)

Regards,
Bin

Re: [PATCH v7 4/9] hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled

[Philippe Mathieu-Daudé]

On 1/16/21 2:35 PM, Bin Meng wrote:
> On Fri, Jan 15, 2021 at 11:37 PM 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)
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> 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 78b19c2eb91..ba7d3438d87 100644
>> --- a/hw/ssi/imx_spi.c
>> +++ b/hw/ssi/imx_spi.c
>> @@ -269,42 +269,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 */
> 
> The new logic is a little bit non straight forward as the value 0
> comes from s->regs[index] which was never written hence 0. While the
> previous logic is returning explicitly zero. Perhaps a comment update
> is needed.

You are right, if the device is in reset, it will return the reset
values (eventually 0, I haven't checked). Simple fix could be to
better place the imx_spi_reset() call in imx_spi_write().

Since we are discussing the reset bit of this device, I wonder if
it wouldn't be clearer to use the the 3-phase-reset API then...

> 
>> +            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 */
> 
> ditto
> 
>> +            break;
>> +        default:
>> +            break;
>>          }

Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Philippe Mathieu-Daudé]

On 1/16/21 4:59 PM, Bin Meng wrote:
> Hi Philippe,
> 
> On Sat, Jan 16, 2021 at 11:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Bin,
>>
>> On 1/16/21 2:57 PM, Bin Meng wrote:
>>> On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> When the block is disabled, only the ECSPI_CONREG register can
>>>> be modified. Setting the EN bit enabled the device, clearing it
>>>
>>> I don't know how this conclusion came out. The manual only says the
>>> following 2 registers ignore the write when the block is disabled.
>>>
>>> ECSPI_TXDATA, ECSPI_INTREG
>>
>> 21.4.5 Reset
>>
>>   Whenever a device reset occurs, a reset is performed on the
>>   ECSPI, resetting all registers to their default values.
>>
>> My understanding is it is pointless to update them when the
>> device is in reset, as they will get their default value when
>> going out of reset.
> 
> I have a different understanding. When ECSPI_CONREG[EN] is cleared,
> it's like a hardware reset, and the ECSPI takes the following action:
> 
>     "Whenever a device reset occurs, a reset is performed on the
> ECSPI, resetting all registers to their default values."
> 
> Chapter 21.4.5 Reset does not mention what's the hardware behavior afterwards.
> 
> So my understanding is: afterwards, the software can still write to
> various registers, unless the register description tells us it's
> ignored.
> 
>>
>>>
>>>> "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 | 26 ++++++++++++++++----------
>>>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>>>> index ba7d3438d87..f06bbf317e2 100644
>>>> --- a/hw/ssi/imx_spi.c
>>>> +++ b/hw/ssi/imx_spi.c
>>>> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
>>>>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
>>>>              (uint32_t)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 */
>>>
>>> So other bits except ECSPI_CONREG_EN are discarded? The manual does
>>> not explicitly mention this but this looks suspicious.
>>
>> See in [*], all bits from the register are updated. We simply check
>> ECSPI_CONREG_EN to see if we need to go out of reset.
> 
> Oops, I missed the [*] line. Now I have read this carefully, and found
> there is one problem:
> 
> Now with the new logic the device reset activity has been postponed
> until next time a device register is written. This is wrong.

Yes, I just realized that in the imx_spi_read() function.

> 
>>
>> See:
>>
>> 21.5 Initialization
>>
>>   This section provides initialization information for ECSPI.
>>
>>   To initialize the block:
>>
>>   1. Clear the EN bit in ECSPI_CONREG to reset the block.
>>   2. Enable the clocks for ECSPI.
>>   3. Set the EN bit in ECSPI_CONREG to put ECSPI out of reset.
>>   4. Configure corresponding IOMUX for ECSPI external signals.
>>   5 Configure registers of ECSPI properly according to the
>>     specifications of the external SPI device.
>>
>> And ECSPI_CONREG_EN bit description:
>>
>>   SPI Block Enable Control. This bit enables the ECSPI. This bit
>>   must be set before writing to other registers or initiating an
>>   exchange. Writing zero to this bit disables the block and resets
>>   the internal logic with the exception of the ECSPI_CONREG. The
>>   block's internal clocks are gated off whenever the block is
>>   disabled.
>>
>>
>> I simply wanted to help you. I don't want to delay your work, so
>> if you think my approach is incorrect, suggest Peter to queue your
>> v5 or resend it (once riscv-next is merged) as v8.
> 
> Thank you for the help. I mentioned in an earlier thread before, that
> my view was not to fix it until it's broken as the v5 series can
> satisfy my work. But since you pointed out various spec violation
> stuff related to device reset, I do think your findings make sense. So
> let's improve this model together. :)

I'm not mad, just I'm doing too many things and I should rather review
your ssi-sd series. I don't have the physical hardware (neither know the
firmware using it) so it is a bit dumb of me to code blindly with no
possibility of testing. If you think this series is going the good way,
it would be great if you can give it another try, and I will be happy
to review.

Regards,

Phil.

Re: [PATCH v7 5/9] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Bin Meng]

Hi Philippe,

On Sun, Jan 17, 2021 at 12:12 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/16/21 4:59 PM, Bin Meng wrote:
> > Hi Philippe,
> >
> > On Sat, Jan 16, 2021 at 11:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi Bin,
> >>
> >> On 1/16/21 2:57 PM, Bin Meng wrote:
> >>> On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>>>
> >>>> When the block is disabled, only the ECSPI_CONREG register can
> >>>> be modified. Setting the EN bit enabled the device, clearing it
> >>>
> >>> I don't know how this conclusion came out. The manual only says the
> >>> following 2 registers ignore the write when the block is disabled.
> >>>
> >>> ECSPI_TXDATA, ECSPI_INTREG
> >>
> >> 21.4.5 Reset
> >>
> >>   Whenever a device reset occurs, a reset is performed on the
> >>   ECSPI, resetting all registers to their default values.
> >>
> >> My understanding is it is pointless to update them when the
> >> device is in reset, as they will get their default value when
> >> going out of reset.
> >
> > I have a different understanding. When ECSPI_CONREG[EN] is cleared,
> > it's like a hardware reset, and the ECSPI takes the following action:
> >
> >     "Whenever a device reset occurs, a reset is performed on the
> > ECSPI, resetting all registers to their default values."
> >
> > Chapter 21.4.5 Reset does not mention what's the hardware behavior afterwards.
> >
> > So my understanding is: afterwards, the software can still write to
> > various registers, unless the register description tells us it's
> > ignored.
> >
> >>
> >>>
> >>>> "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 | 26 ++++++++++++++++----------
> >>>>  1 file changed, 16 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> >>>> index ba7d3438d87..f06bbf317e2 100644
> >>>> --- a/hw/ssi/imx_spi.c
> >>>> +++ b/hw/ssi/imx_spi.c
> >>>> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> >>>>      DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
> >>>>              (uint32_t)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 */
> >>>
> >>> So other bits except ECSPI_CONREG_EN are discarded? The manual does
> >>> not explicitly mention this but this looks suspicious.
> >>
> >> See in [*], all bits from the register are updated. We simply check
> >> ECSPI_CONREG_EN to see if we need to go out of reset.
> >
> > Oops, I missed the [*] line. Now I have read this carefully, and found
> > there is one problem:
> >
> > Now with the new logic the device reset activity has been postponed
> > until next time a device register is written. This is wrong.
>
> Yes, I just realized that in the imx_spi_read() function.
>
> >
> >>
> >> See:
> >>
> >> 21.5 Initialization
> >>
> >>   This section provides initialization information for ECSPI.
> >>
> >>   To initialize the block:
> >>
> >>   1. Clear the EN bit in ECSPI_CONREG to reset the block.
> >>   2. Enable the clocks for ECSPI.
> >>   3. Set the EN bit in ECSPI_CONREG to put ECSPI out of reset.
> >>   4. Configure corresponding IOMUX for ECSPI external signals.
> >>   5 Configure registers of ECSPI properly according to the
> >>     specifications of the external SPI device.
> >>
> >> And ECSPI_CONREG_EN bit description:
> >>
> >>   SPI Block Enable Control. This bit enables the ECSPI. This bit
> >>   must be set before writing to other registers or initiating an
> >>   exchange. Writing zero to this bit disables the block and resets
> >>   the internal logic with the exception of the ECSPI_CONREG. The
> >>   block's internal clocks are gated off whenever the block is
> >>   disabled.
> >>
> >>
> >> I simply wanted to help you. I don't want to delay your work, so
> >> if you think my approach is incorrect, suggest Peter to queue your
> >> v5 or resend it (once riscv-next is merged) as v8.
> >
> > Thank you for the help. I mentioned in an earlier thread before, that
> > my view was not to fix it until it's broken as the v5 series can
> > satisfy my work. But since you pointed out various spec violation
> > stuff related to device reset, I do think your findings make sense. So
> > let's improve this model together. :)
>
> I'm not mad, just I'm doing too many things and I should rather review
> your ssi-sd series. I don't have the physical hardware (neither know the
> firmware using it) so it is a bit dumb of me to code blindly with no
> possibility of testing. If you think this series is going the good way,
> it would be great if you can give it another try, and I will be happy
> to review.

Sure I will see if I can find a hardware to verify the register write
behavior when ECSPI is disabled.

Regards,
Bin