[PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

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

This v8 series is based on the following 2 versions:

- v5 series sent from Bin
  http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223919
- v7 series sent from Philippe
  http://patchwork.ozlabs.org/project/qemu-devel/list/?series=224612

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

- remove imx_spi_update_irq() in imx_spi_reset()
- chip select signal was not lower down when spi controller is disabled
- 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
- remove pointless variable (s->burst_length) initialization (Philippe)
- rework imx_spi_reset() to keep CONREG register value (Philippe)
- rework imx_spi_read() to handle block disabled (Philippe)
- rework imx_spi_write() to handle block disabled (Philippe)

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

Changes in v8:
- keep the controller disable logic in the ECSPI_CONREG case
  in imx_spi_write()

Changes in v7:
- remove the RFC tag

Changes in v6:
- new patch: [RFC] remove pointless variable initialization
- new patch: [RFC] rework imx_spi_reset() to keep CONREG register value
- new patch: [RFC] rework imx_spi_read() to handle block disabled
- new patch: [RFC] rework imx_spi_write() to handle block disabled

Changes in v5:
- rename imx_spi_hard_reset() to imx_spi_soft_reset()
- round up the burst length to be multiple of 8

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: 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é (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         | 138 +++++++++++++++++++++++++++++------------------
 2 files changed, 90 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH v8 01/10] 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>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---

(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 b82b17f..eeaf49b 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 d8885ae..e605049 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.7.4

[PATCH v8 02/10] 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_soft_reset() that is called when the controller is disabled.

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

---

(no changes since v5)

Changes in v5:
- rename imx_spi_hard_reset() to imx_spi_soft_reset()

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 e605049..4d488b1 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_soft_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 soft reset */
+            imx_spi_soft_reset(s);
+
             return;
         }
 
-- 
2.7.4

[PATCH v8 03/10] hw/ssi: imx_spi: Remove pointless variable initialization

[Bin Meng]

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

'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>
Message-Id: <20210115153049.3353008-3-f4bug@amsat.org>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

(no changes since v7)

Changes in v7:
- remove the RFC tag

Changes in v6:
- new patch: [RFC] remove pointless variable initialization

 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 4d488b1..8fb3c9b 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -434,8 +434,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.7.4

[PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Bin Meng]

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

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>
Message-Id: <20210115153049.3353008-4-f4bug@amsat.org>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

(no changes since v7)

Changes in v7:
- remove the RFC tag

Changes in v6:
- new patch: [RFC] rework imx_spi_reset() to keep CONREG register value

 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 8fb3c9b..c952a3d 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);
+    int 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.7.4

[PATCH v8 05/10] hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled

[Bin Meng]

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

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>
Message-Id: <20210115153049.3353008-5-f4bug@amsat.org>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

(no changes since v7)

Changes in v7:
- remove the RFC tag

Changes in v6:
- new patch: [RFC] rework imx_spi_read() to handle block disabled

 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 c952a3d..277b936 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -274,42 +274,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);
-        }
-
-        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__);
+    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 MSGDATA gives 0 */
+            /* 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;
-    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.7.4

[PATCH v8 06/10] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Bin Meng]

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

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.

Ignore all other registers write except ECSPI_CONREG when the
block is disabled.

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>
Message-Id: <20210115153049.3353008-6-f4bug@amsat.org>
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v8:
- keep the controller disable logic in the ECSPI_CONREG case
  in imx_spi_write()

Changes in v7:
- remove the RFC tag

Changes in v6:
- new patch: [RFC] rework imx_spi_write() to handle block disabled

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

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 277b936..23f9f9d 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -327,6 +327,14 @@ 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;
+        }
+    }
+
     change_mask = s->regs[index] ^ value;
 
     switch (index) {
@@ -335,10 +343,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;
         }
-- 
2.7.4

[PATCH v8 07/10] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---

(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 23f9f9d..5838bb0 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -257,9 +257,15 @@ static void imx_spi_reset(DeviceState *dev)
 
 static void imx_spi_soft_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.7.4

[PATCH v8 08/10] hw/ssi: imx_spi: Round up the burst length to be multiple of 8

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

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

---

(no changes since v5)

Changes in v5:
- round up the burst length to be multiple of 8

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 5838bb0..3c80725 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.7.4

[PATCH v8 09/10] 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 3c80725..de0c481 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.7.4

[PATCH v8 10/10] 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 de0c481..dee7368 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.7.4

Re: [PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

On Tue, Jan 19, 2021 at 9:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> This v8 series is based on the following 2 versions:
>
> - v5 series sent from Bin
>   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223919
> - v7 series sent from Philippe
>   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=224612
>
> This series fixes a bunch of bugs in current implementation of the imx
> spi controller, including the following issues:
>
> - remove imx_spi_update_irq() in imx_spi_reset()
> - chip select signal was not lower down when spi controller is disabled
> - 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
> - remove pointless variable (s->burst_length) initialization (Philippe)
> - rework imx_spi_reset() to keep CONREG register value (Philippe)
> - rework imx_spi_read() to handle block disabled (Philippe)
> - rework imx_spi_write() to handle block disabled (Philippe)
>
> Tested with upstream U-Boot v2020.10 (polling mode) and VxWorks 7
> (interrupt mode).
>
> Changes in v8:
> - keep the controller disable logic in the ECSPI_CONREG case
>   in imx_spi_write()

Ping?

Re: [PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Bin Meng]

On Fri, Jan 22, 2021 at 9:36 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 9:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > This v8 series is based on the following 2 versions:
> >
> > - v5 series sent from Bin
> >   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223919
> > - v7 series sent from Philippe
> >   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=224612
> >
> > This series fixes a bunch of bugs in current implementation of the imx
> > spi controller, including the following issues:
> >
> > - remove imx_spi_update_irq() in imx_spi_reset()
> > - chip select signal was not lower down when spi controller is disabled
> > - 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
> > - remove pointless variable (s->burst_length) initialization (Philippe)
> > - rework imx_spi_reset() to keep CONREG register value (Philippe)
> > - rework imx_spi_read() to handle block disabled (Philippe)
> > - rework imx_spi_write() to handle block disabled (Philippe)
> >
> > Tested with upstream U-Boot v2020.10 (polling mode) and VxWorks 7
> > (interrupt mode).
> >
> > Changes in v8:
> > - keep the controller disable logic in the ECSPI_CONREG case
> >   in imx_spi_write()
>
> Ping?

Could we get this applied soon if no more comments?

Regards,
Bin

Re: [PATCH v8 10/10] hw/ssi: imx_spi: Correct tx and rx fifo endianness

[Peter Maydell]

On Tue, 19 Jan 2021 at 13:40, 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v8 06/10] hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

[Peter Maydell]

On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> 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.
>
> Ignore all other registers write except ECSPI_CONREG when the
> block is disabled.
>
> 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>
> Message-Id: <20210115153049.3353008-6-f4bug@amsat.org>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v8 02/10] hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()

[Peter Maydell]

On Tue, 19 Jan 2021 at 13:40, 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_soft_reset() that is called when the controller is disabled.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Peter Maydell]

On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> 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)

> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 8fb3c9b..c952a3d 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);
> +    int 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;
> +        }
> +    }

This retains the CONREG register value for both:
 * 'soft' reset caused by write to device register to disable
   the block
   -- this is corrcet as per the datasheet quote
 * 'power on' reset via TYPE_DEVICE's reset method
   -- but in this case we should reset CONREG, because the Device
   reset method is like a complete device powercycle and should
   return the device state to what it was when QEMU was first
   started.

thanks
-- PMM

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Bin Meng]

On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > 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)
>
> > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > index 8fb3c9b..c952a3d 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);
> > +    int 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;
> > +        }
> > +    }
>
> This retains the CONREG register value for both:
>  * 'soft' reset caused by write to device register to disable
>    the block
>    -- this is corrcet as per the datasheet quote
>  * 'power on' reset via TYPE_DEVICE's reset method
>    -- but in this case we should reset CONREG, because the Device
>    reset method is like a complete device powercycle and should
>    return the device state to what it was when QEMU was first
>    started.

The POR value of CONREG is zero, which should be the default value, no?

Regards,
Bin

Re: [PATCH v8 08/10] hw/ssi: imx_spi: Round up the burst length to be multiple of 8

[Peter Maydell]

On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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>

> @@ -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);

It's friendlier to the user to do the LOG_UNIMP when the
unsupported CONREG value is written, rather than here
where it is used. That way the warning happens once, rather
than every time the device transmits data.

Also, you could squash the warning down into one line, something
like:
   "[%s]%s: burst length %d not supported: rounding up to next multiple of 8\n"
rather than logging twice.

thanks
-- PMM

Re: [PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

[Peter Maydell]

On Thu, 28 Jan 2021 at 07:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 9:36 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Tue, Jan 19, 2021 at 9:40 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > This v8 series is based on the following 2 versions:
> > >
> > > - v5 series sent from Bin
> > >   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223919
> > > - v7 series sent from Philippe
> > >   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=224612
> > >
> > > This series fixes a bunch of bugs in current implementation of the imx
> > > spi controller, including the following issues:
> > >
> > > - remove imx_spi_update_irq() in imx_spi_reset()
> > > - chip select signal was not lower down when spi controller is disabled
> > > - 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
> > > - remove pointless variable (s->burst_length) initialization (Philippe)
> > > - rework imx_spi_reset() to keep CONREG register value (Philippe)
> > > - rework imx_spi_read() to handle block disabled (Philippe)
> > > - rework imx_spi_write() to handle block disabled (Philippe)
> > >
> > > Tested with upstream U-Boot v2020.10 (polling mode) and VxWorks 7
> > > (interrupt mode).
> > >
> > > Changes in v8:
> > > - keep the controller disable logic in the ECSPI_CONREG case
> > >   in imx_spi_write()
> >
> > Ping?
>
> Could we get this applied soon if no more comments?

Sorry, I think I missed this re-send. I've reviewed or left
comments on the patches that were still unreviewed.

thanks
-- PMM

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Peter Maydell]

On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >
> > > 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)
> >
> > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > > index 8fb3c9b..c952a3d 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);
> > > +    int 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;
> > > +        }
> > > +    }
> >
> > This retains the CONREG register value for both:
> >  * 'soft' reset caused by write to device register to disable
> >    the block
> >    -- this is corrcet as per the datasheet quote
> >  * 'power on' reset via TYPE_DEVICE's reset method
> >    -- but in this case we should reset CONREG, because the Device
> >    reset method is like a complete device powercycle and should
> >    return the device state to what it was when QEMU was first
> >    started.
>
> The POR value of CONREG is zero, which should be the default value, no?

But you're not setting it to zero here, you're leaving it with
whatever value it had before. (That's correct for soft reset,
but wrong for power-on.)

thanks
-- PMM

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Bin Meng]

On Thu, Jan 28, 2021 at 9:54 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > >
> > > > 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)
> > >
> > > > diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> > > > index 8fb3c9b..c952a3d 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);
> > > > +    int 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;
> > > > +        }
> > > > +    }
> > >
> > > This retains the CONREG register value for both:
> > >  * 'soft' reset caused by write to device register to disable
> > >    the block
> > >    -- this is corrcet as per the datasheet quote
> > >  * 'power on' reset via TYPE_DEVICE's reset method
> > >    -- but in this case we should reset CONREG, because the Device
> > >    reset method is like a complete device powercycle and should
> > >    return the device state to what it was when QEMU was first
> > >    started.
> >
> > The POR value of CONREG is zero, which should be the default value, no?
>
> But you're not setting it to zero here, you're leaving it with
> whatever value it had before. (That's correct for soft reset,
> but wrong for power-on.)

I think that's ensured by object_initialize_with_type().

Regards,
Bin

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Philippe Mathieu-Daudé]

On 1/28/21 2:54 PM, Peter Maydell wrote:
> On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>
>>>> 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)
>>>
>>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>>>> index 8fb3c9b..c952a3d 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);
>>>> +    int 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;
>>>> +        }
>>>> +    }
>>>
>>> This retains the CONREG register value for both:
>>>  * 'soft' reset caused by write to device register to disable
>>>    the block
>>>    -- this is corrcet as per the datasheet quote
>>>  * 'power on' reset via TYPE_DEVICE's reset method
>>>    -- but in this case we should reset CONREG, because the Device
>>>    reset method is like a complete device powercycle and should
>>>    return the device state to what it was when QEMU was first
>>>    started.
>>
>> The POR value of CONREG is zero, which should be the default value, no?
> 
> But you're not setting it to zero here, you're leaving it with
> whatever value it had before. (That's correct for soft reset,
> but wrong for power-on.)

zero value on power-on is what I tried to describe as
"It is initialized to zero when the instance is created."

Most of the codebase assumes QOM provides a zero-initialized
instance state. Do you think it should be explicit?

Regards,

Phil.

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Peter Maydell]

On Thu, 28 Jan 2021 at 14:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/28/21 2:54 PM, Peter Maydell wrote:
> > On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> This retains the CONREG register value for both:
> >>>  * 'soft' reset caused by write to device register to disable
> >>>    the block
> >>>    -- this is corrcet as per the datasheet quote
> >>>  * 'power on' reset via TYPE_DEVICE's reset method
> >>>    -- but in this case we should reset CONREG, because the Device
> >>>    reset method is like a complete device powercycle and should
> >>>    return the device state to what it was when QEMU was first
> >>>    started.
> >>
> >> The POR value of CONREG is zero, which should be the default value, no?
> >
> > But you're not setting it to zero here, you're leaving it with
> > whatever value it had before. (That's correct for soft reset,
> > but wrong for power-on.)
>
> zero value on power-on is what I tried to describe as
> "It is initialized to zero when the instance is created."

Yes, but QOM device reset does not happen just once at startup and
not thereafter. Consider:

 * user starts QEMU
 * QOM devices are created and realized
 * QOM device reset happens
     -- CONREG is zero here because QOM structs are zero-initialized
 * guest runs
 * guest modifies CONREG from its initial value
 * system reset is requested (perhaps by user, perhaps by
   guest writing some register or another)
 * QOM device reset happens
     -- CONREG is not zero here, so reset must clear it

thanks
-- PMM

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Philippe Mathieu-Daudé]

On 1/28/21 3:22 PM, Peter Maydell wrote:
> On Thu, 28 Jan 2021 at 14:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 1/28/21 2:54 PM, Peter Maydell wrote:
>>> On Thu, 28 Jan 2021 at 13:47, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> On Thu, Jan 28, 2021 at 9:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> This retains the CONREG register value for both:
>>>>>  * 'soft' reset caused by write to device register to disable
>>>>>    the block
>>>>>    -- this is corrcet as per the datasheet quote
>>>>>  * 'power on' reset via TYPE_DEVICE's reset method
>>>>>    -- but in this case we should reset CONREG, because the Device
>>>>>    reset method is like a complete device powercycle and should
>>>>>    return the device state to what it was when QEMU was first
>>>>>    started.
>>>>
>>>> The POR value of CONREG is zero, which should be the default value, no?
>>>
>>> But you're not setting it to zero here, you're leaving it with
>>> whatever value it had before. (That's correct for soft reset,
>>> but wrong for power-on.)
>>
>> zero value on power-on is what I tried to describe as
>> "It is initialized to zero when the instance is created."
> 
> Yes, but QOM device reset does not happen just once at startup and
> not thereafter. Consider:
> 
>  * user starts QEMU
>  * QOM devices are created and realized
>  * QOM device reset happens
>      -- CONREG is zero here because QOM structs are zero-initialized
>  * guest runs
>  * guest modifies CONREG from its initial value
>  * system reset is requested (perhaps by user, perhaps by
>    guest writing some register or another)
>  * QOM device reset happens
>      -- CONREG is not zero here, so reset must clear it

Oh I totally missed that :S

Bin, I'd correct this as:

- extract imx_spi_soft_reset(IMXSPIState *s) from imx_spi_reset()
- zero-initialize CONREG in imx_spi_reset().

static void imx_spi_soft_reset(IMXSPIState *s)
{
 ...
}

static void imx_spi_reset(DeviceState *dev)
{
    IMXSPIState *s = IMX_SPI(dev);

    s->regs[ECSPI_CONREG] = 0;
    imx_spi_soft_reset(s);
}

What do you think?

Phil.

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Peter Maydell]

On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Oh I totally missed that :S
>
> Bin, I'd correct this as:
>
> - extract imx_spi_soft_reset(IMXSPIState *s) from imx_spi_reset()
> - zero-initialize CONREG in imx_spi_reset().
>
> static void imx_spi_soft_reset(IMXSPIState *s)
> {
>  ...
> }
>
> static void imx_spi_reset(DeviceState *dev)
> {
>     IMXSPIState *s = IMX_SPI(dev);
>
>     s->regs[ECSPI_CONREG] = 0;
>     imx_spi_soft_reset(s);
> }
>
> What do you think?

That doesn't give you anywhere to put the imx_spi_update_irq()
call, which must happen only on soft reset and not on DeviceState
reset. You could do one of:
 * have a 'common reset' function that does most of this,
   plus an imx_spi_reset which clears CONREG and calls
   common reset and an imx_spi_soft_reset which calls
   common reset and imx_spi_update_irq()
 * have imx_spi_soft_reset save the old CONREG in a local
   variable before calling imx_spi_reset and then restore it
   to s->regs

thanks
-- PMM

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Philippe Mathieu-Daudé]

On 1/28/21 3:41 PM, Peter Maydell wrote:
> On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Oh I totally missed that :S
>>
>> Bin, I'd correct this as:
>>
>> - extract imx_spi_soft_reset(IMXSPIState *s) from imx_spi_reset()
>> - zero-initialize CONREG in imx_spi_reset().
>>
>> static void imx_spi_soft_reset(IMXSPIState *s)
>> {
>>  ...
>> }
>>
>> static void imx_spi_reset(DeviceState *dev)
>> {
>>     IMXSPIState *s = IMX_SPI(dev);
>>
>>     s->regs[ECSPI_CONREG] = 0;
>>     imx_spi_soft_reset(s);
>> }
>>
>> What do you think?
> 
> That doesn't give you anywhere to put the imx_spi_update_irq()
> call, which must happen only on soft reset and not on DeviceState
> reset. You could do one of:
>  * have a 'common reset' function that does most of this,
>    plus an imx_spi_reset which clears CONREG and calls
>    common reset and an imx_spi_soft_reset which calls
>    common reset and imx_spi_update_irq()
>  * have imx_spi_soft_reset save the old CONREG in a local
>    variable before calling imx_spi_reset and then restore it
>    to s->regs

Long term maintenance I'd prefer the 'common reset' approach
(but this is probably subjective to my view on the hardware,
since this is software, the 2nd approach is also valid but
harder to represent thinking of hardware).

Bin, can you send a v9? (using the approach you prefer)

Thanks,

Phil.

Re: [PATCH v8 04/10] hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value

[Bin Meng]

On Thu, Jan 28, 2021 at 10:49 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/28/21 3:41 PM, Peter Maydell wrote:
> > On Thu, 28 Jan 2021 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >> Oh I totally missed that :S
> >>
> >> Bin, I'd correct this as:
> >>
> >> - extract imx_spi_soft_reset(IMXSPIState *s) from imx_spi_reset()
> >> - zero-initialize CONREG in imx_spi_reset().
> >>
> >> static void imx_spi_soft_reset(IMXSPIState *s)
> >> {
> >>  ...
> >> }
> >>
> >> static void imx_spi_reset(DeviceState *dev)
> >> {
> >>     IMXSPIState *s = IMX_SPI(dev);
> >>
> >>     s->regs[ECSPI_CONREG] = 0;
> >>     imx_spi_soft_reset(s);
> >> }
> >>
> >> What do you think?
> >
> > That doesn't give you anywhere to put the imx_spi_update_irq()
> > call, which must happen only on soft reset and not on DeviceState
> > reset. You could do one of:
> >  * have a 'common reset' function that does most of this,
> >    plus an imx_spi_reset which clears CONREG and calls
> >    common reset and an imx_spi_soft_reset which calls
> >    common reset and imx_spi_update_irq()
> >  * have imx_spi_soft_reset save the old CONREG in a local
> >    variable before calling imx_spi_reset and then restore it
> >    to s->regs
>
> Long term maintenance I'd prefer the 'common reset' approach
> (but this is probably subjective to my view on the hardware,
> since this is software, the 2nd approach is also valid but
> harder to represent thinking of hardware).
>
> Bin, can you send a v9? (using the approach you prefer)
>

Yes, I will send a v9.

Thanks Peter for the explanation on the QOM device reset scenarios.

Regards,
Bin