[PATCH 0/2] aspeed/smc: fix data corruption on witherspoon machines

[PATCH 0/2] aspeed/smc: fix data corruption on witherspoon machines

[Cédric Le Goater]

Hello,

We have been seeing a squashfs corruption on the witherspoon-bmc
machine for quite some time. It boots but quickly after boot
corruption errors start to fill the console. This machine has two BMC
chips with UBIfs on them. The romulus-bmc machine has a similar
problem when both PNOR are enabled.

After some investigation, it appeared that the SPI transfers on the
two chips were getting mixed and after more investigation, the User
mode select/unselect scheme of the Aspeed SMC model proved to be
broken. It's been there since the right beginning of the model. Here
is a fix.

Kudos to Andrew J. for the time he spent on this and Andrew G. for his
patience.

I pushed the code on my github branch aspeed-5.0 for more testing.

Thanks,

C.

Cédric Le Goater (2):
  aspeed/smc: Add some tracing
  aspeed/smc: Fix User mode select/unselect scheme

 Makefile.objs       |  1 +
 hw/ssi/aspeed_smc.c | 56 ++++++++++++++++++++++++++++++++-------------
 hw/ssi/trace-events | 10 ++++++++
 3 files changed, 51 insertions(+), 16 deletions(-)
 create mode 100644 hw/ssi/trace-events

-- 
2.21.1

[PATCH 1/2] aspeed/smc: Add some tracing

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 Makefile.objs       |  1 +
 hw/ssi/aspeed_smc.c | 17 +++++++++++++++++
 hw/ssi/trace-events |  9 +++++++++
 3 files changed, 27 insertions(+)
 create mode 100644 hw/ssi/trace-events

diff --git a/Makefile.objs b/Makefile.objs
index 26b9cff95436..9e4ba95794e9 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -168,6 +168,7 @@ trace-events-subdirs += hw/scsi
 trace-events-subdirs += hw/sd
 trace-events-subdirs += hw/sparc
 trace-events-subdirs += hw/sparc64
+trace-events-subdirs += hw/ssi
 trace-events-subdirs += hw/timer
 trace-events-subdirs += hw/tpm
 trace-events-subdirs += hw/usb
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 23c8d2f06245..e5621bf728ca 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -31,6 +31,7 @@
 #include "qapi/error.h"
 #include "exec/address-spaces.h"
 #include "qemu/units.h"
+#include "trace.h"
 
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
@@ -513,6 +514,8 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
 
     s->ctrl->reg_to_segment(s, new, &seg);
 
+    trace_aspeed_smc_flash_set_segment(cs, new, seg.addr, seg.addr + seg.size);
+
     /* The start address of CS0 is read-only */
     if (cs == 0 && seg.addr != s->ctrl->flash_window_base) {
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -753,6 +756,8 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
                       __func__, aspeed_smc_flash_mode(fl));
     }
 
+    trace_aspeed_smc_flash_read(fl->id, addr, size, ret,
+                                aspeed_smc_flash_mode(fl));
     return ret;
 }
 
@@ -808,6 +813,9 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
     AspeedSMCState *s = fl->controller;
     uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
 
+    trace_aspeed_smc_do_snoop(fl->id, s->snoop_index, s->snoop_dummies,
+                              (uint8_t) data & 0xff);
+
     if (s->snoop_index == SNOOP_OFF) {
         return false; /* Do nothing */
 
@@ -858,6 +866,9 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
     AspeedSMCState *s = fl->controller;
     int i;
 
+    trace_aspeed_smc_flash_write(fl->id, addr, size, data,
+                                 aspeed_smc_flash_mode(fl));
+
     if (!aspeed_smc_is_writable(fl)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
                       HWADDR_PRIx "\n", __func__, addr);
@@ -972,6 +983,9 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         (s->ctrl->has_dma && addr == R_DMA_CHECKSUM) ||
         (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
         (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
+
+        trace_aspeed_smc_read(addr, size, s->regs[addr]);
+
         return s->regs[addr];
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
@@ -1091,6 +1105,7 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
                           __func__, s->regs[R_DMA_FLASH_ADDR]);
             return;
         }
+        trace_aspeed_smc_dma_checksum(s->regs[R_DMA_FLASH_ADDR], data);
 
         /*
          * When the DMA is on-going, the DMA registers are updated
@@ -1225,6 +1240,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
 
     addr >>= 2;
 
+    trace_aspeed_smc_write(addr, size, data);
+
     if (addr == s->r_conf ||
         (addr >= s->r_timings &&
          addr < s->r_timings + s->ctrl->nregs_timings) ||
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
new file mode 100644
index 000000000000..ffe531a500aa
--- /dev/null
+++ b/hw/ssi/trace-events
@@ -0,0 +1,9 @@
+# aspeed_smc.c
+
+aspeed_smc_flash_set_segment(int cs, uint64_t reg, uint64_t start, uint64_t end) "CS%d segreg=0x%"PRIx64" [ 0x%"PRIx64" - 0x%"PRIx64" ]"
+aspeed_smc_flash_read(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
+aspeed_smc_do_snoop(int cs, int index, int dummies, int data) "CS%d index:0x%x dummies:%d data:0x%x"
+aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
+aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
+aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
+aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
-- 
2.21.1

Re: [PATCH 1/2] aspeed/smc: Add some tracing

[Andrew Jeffery]

On Thu, 6 Feb 2020, at 21:56, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Re: [PATCH 1/2] aspeed/smc: Add some tracing

[Joel Stanley]

n

On Thu, 6 Feb 2020 at 11:27, Cédric Le Goater <clg@kaod.org> wrote:
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>


> ---
>  Makefile.objs       |  1 +
>  hw/ssi/aspeed_smc.c | 17 +++++++++++++++++
>  hw/ssi/trace-events |  9 +++++++++
>  3 files changed, 27 insertions(+)
>  create mode 100644 hw/ssi/trace-events
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 26b9cff95436..9e4ba95794e9 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -168,6 +168,7 @@ trace-events-subdirs += hw/scsi
>  trace-events-subdirs += hw/sd
>  trace-events-subdirs += hw/sparc
>  trace-events-subdirs += hw/sparc64
> +trace-events-subdirs += hw/ssi
>  trace-events-subdirs += hw/timer
>  trace-events-subdirs += hw/tpm
>  trace-events-subdirs += hw/usb
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 23c8d2f06245..e5621bf728ca 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -31,6 +31,7 @@
>  #include "qapi/error.h"
>  #include "exec/address-spaces.h"
>  #include "qemu/units.h"
> +#include "trace.h"
>
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
> @@ -513,6 +514,8 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
>
>      s->ctrl->reg_to_segment(s, new, &seg);
>
> +    trace_aspeed_smc_flash_set_segment(cs, new, seg.addr, seg.addr + seg.size);
> +
>      /* The start address of CS0 is read-only */
>      if (cs == 0 && seg.addr != s->ctrl->flash_window_base) {
>          qemu_log_mask(LOG_GUEST_ERROR,
> @@ -753,6 +756,8 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>                        __func__, aspeed_smc_flash_mode(fl));
>      }
>
> +    trace_aspeed_smc_flash_read(fl->id, addr, size, ret,
> +                                aspeed_smc_flash_mode(fl));
>      return ret;
>  }
>
> @@ -808,6 +813,9 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
>      AspeedSMCState *s = fl->controller;
>      uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
>
> +    trace_aspeed_smc_do_snoop(fl->id, s->snoop_index, s->snoop_dummies,
> +                              (uint8_t) data & 0xff);
> +
>      if (s->snoop_index == SNOOP_OFF) {
>          return false; /* Do nothing */
>
> @@ -858,6 +866,9 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>      AspeedSMCState *s = fl->controller;
>      int i;
>
> +    trace_aspeed_smc_flash_write(fl->id, addr, size, data,
> +                                 aspeed_smc_flash_mode(fl));
> +
>      if (!aspeed_smc_is_writable(fl)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
>                        HWADDR_PRIx "\n", __func__, addr);
> @@ -972,6 +983,9 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>          (s->ctrl->has_dma && addr == R_DMA_CHECKSUM) ||
>          (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
>          (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
> +
> +        trace_aspeed_smc_read(addr, size, s->regs[addr]);
> +
>          return s->regs[addr];
>      } else {
>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
> @@ -1091,6 +1105,7 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>                            __func__, s->regs[R_DMA_FLASH_ADDR]);
>              return;
>          }
> +        trace_aspeed_smc_dma_checksum(s->regs[R_DMA_FLASH_ADDR], data);
>
>          /*
>           * When the DMA is on-going, the DMA registers are updated
> @@ -1225,6 +1240,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>
>      addr >>= 2;
>
> +    trace_aspeed_smc_write(addr, size, data);
> +
>      if (addr == s->r_conf ||
>          (addr >= s->r_timings &&
>           addr < s->r_timings + s->ctrl->nregs_timings) ||
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> new file mode 100644
> index 000000000000..ffe531a500aa
> --- /dev/null
> +++ b/hw/ssi/trace-events
> @@ -0,0 +1,9 @@
> +# aspeed_smc.c
> +
> +aspeed_smc_flash_set_segment(int cs, uint64_t reg, uint64_t start, uint64_t end) "CS%d segreg=0x%"PRIx64" [ 0x%"PRIx64" - 0x%"PRIx64" ]"
> +aspeed_smc_flash_read(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> +aspeed_smc_do_snoop(int cs, int index, int dummies, int data) "CS%d index:0x%x dummies:%d data:0x%x"
> +aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> +aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
> +aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
> +aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
> --
> 2.21.1
>

Re: [PATCH 1/2] aspeed/smc: Add some tracing

[Philippe Mathieu-Daudé]

On 2/6/20 12:26 PM, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   Makefile.objs       |  1 +
>   hw/ssi/aspeed_smc.c | 17 +++++++++++++++++
>   hw/ssi/trace-events |  9 +++++++++
>   3 files changed, 27 insertions(+)
>   create mode 100644 hw/ssi/trace-events
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 26b9cff95436..9e4ba95794e9 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -168,6 +168,7 @@ trace-events-subdirs += hw/scsi
>   trace-events-subdirs += hw/sd
>   trace-events-subdirs += hw/sparc
>   trace-events-subdirs += hw/sparc64
> +trace-events-subdirs += hw/ssi
>   trace-events-subdirs += hw/timer
>   trace-events-subdirs += hw/tpm
>   trace-events-subdirs += hw/usb
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 23c8d2f06245..e5621bf728ca 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -31,6 +31,7 @@
>   #include "qapi/error.h"
>   #include "exec/address-spaces.h"
>   #include "qemu/units.h"
> +#include "trace.h"
>   
>   #include "hw/irq.h"
>   #include "hw/qdev-properties.h"
> @@ -513,6 +514,8 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
>   
>       s->ctrl->reg_to_segment(s, new, &seg);
>   
> +    trace_aspeed_smc_flash_set_segment(cs, new, seg.addr, seg.addr + seg.size);
> +
>       /* The start address of CS0 is read-only */
>       if (cs == 0 && seg.addr != s->ctrl->flash_window_base) {
>           qemu_log_mask(LOG_GUEST_ERROR,
> @@ -753,6 +756,8 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>                         __func__, aspeed_smc_flash_mode(fl));
>       }
>   
> +    trace_aspeed_smc_flash_read(fl->id, addr, size, ret,
> +                                aspeed_smc_flash_mode(fl));
>       return ret;
>   }
>   
> @@ -808,6 +813,9 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
>       AspeedSMCState *s = fl->controller;
>       uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
>   
> +    trace_aspeed_smc_do_snoop(fl->id, s->snoop_index, s->snoop_dummies,
> +                              (uint8_t) data & 0xff);
> +
>       if (s->snoop_index == SNOOP_OFF) {
>           return false; /* Do nothing */
>   
> @@ -858,6 +866,9 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>       AspeedSMCState *s = fl->controller;
>       int i;
>   
> +    trace_aspeed_smc_flash_write(fl->id, addr, size, data,
> +                                 aspeed_smc_flash_mode(fl));
> +
>       if (!aspeed_smc_is_writable(fl)) {
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
>                         HWADDR_PRIx "\n", __func__, addr);
> @@ -972,6 +983,9 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>           (s->ctrl->has_dma && addr == R_DMA_CHECKSUM) ||
>           (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
>           (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
> +
> +        trace_aspeed_smc_read(addr, size, s->regs[addr]);
> +
>           return s->regs[addr];
>       } else {
>           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
> @@ -1091,6 +1105,7 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>                             __func__, s->regs[R_DMA_FLASH_ADDR]);
>               return;
>           }
> +        trace_aspeed_smc_dma_checksum(s->regs[R_DMA_FLASH_ADDR], data);
>   
>           /*
>            * When the DMA is on-going, the DMA registers are updated
> @@ -1225,6 +1240,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>   
>       addr >>= 2;
>   
> +    trace_aspeed_smc_write(addr, size, data);
> +
>       if (addr == s->r_conf ||
>           (addr >= s->r_timings &&
>            addr < s->r_timings + s->ctrl->nregs_timings) ||
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> new file mode 100644
> index 000000000000..ffe531a500aa
> --- /dev/null
> +++ b/hw/ssi/trace-events
> @@ -0,0 +1,9 @@
> +# aspeed_smc.c
> +
> +aspeed_smc_flash_set_segment(int cs, uint64_t reg, uint64_t start, uint64_t end) "CS%d segreg=0x%"PRIx64" [ 0x%"PRIx64" - 0x%"PRIx64" ]"
> +aspeed_smc_flash_read(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> +aspeed_smc_do_snoop(int cs, int index, int dummies, int data) "CS%d index:0x%x dummies:%d data:0x%x"
> +aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> +aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
> +aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
> +aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 2/2] aspeed/smc: Fix User mode select/unselect scheme

[Cédric Le Goater]

The Aspeed SMC Controller can operate in different modes : Read, Fast
Read, Write and User modes. When the User mode is configured, it
selects automatically the SPI slave device until the CE_STOP_ACTIVE
bit is set to 1. When any other modes are configured the device is
unselected. The HW logic handles the chip select automatically when
the flash is accessed through its AHB window.

When configuring the CEx Control Register, the User mode logic to
select and unselect the slave is incorrect and data corruption can be
seen on machines using two chips, witherspoon and romulus.

Rework the handler setting the CEx Control Register to fix this issue.

Fixes: 7c1c69bca43c ("ast2400: add SMC controllers (FMC and SPI)")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 39 +++++++++++++++++++++++----------------
 hw/ssi/trace-events |  1 +
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index e5621bf728ca..32be2a02b0e4 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -639,27 +639,23 @@ static inline int aspeed_smc_flash_is_4byte(const AspeedSMCFlash *fl)
     }
 }
 
-static inline bool aspeed_smc_is_ce_stop_active(const AspeedSMCFlash *fl)
+static void aspeed_smc_flash_do_select(AspeedSMCFlash *fl, bool unselect)
 {
-    const AspeedSMCState *s = fl->controller;
+    AspeedSMCState *s = fl->controller;
+
+    trace_aspeed_smc_flash_select(fl->id, unselect ? "un" : "");
 
-    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE;
+    qemu_set_irq(s->cs_lines[fl->id], unselect);
 }
 
 static void aspeed_smc_flash_select(AspeedSMCFlash *fl)
 {
-    AspeedSMCState *s = fl->controller;
-
-    s->regs[s->r_ctrl0 + fl->id] &= ~CTRL_CE_STOP_ACTIVE;
-    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
+    aspeed_smc_flash_do_select(fl, false);
 }
 
 static void aspeed_smc_flash_unselect(AspeedSMCFlash *fl)
 {
-    AspeedSMCState *s = fl->controller;
-
-    s->regs[s->r_ctrl0 + fl->id] |= CTRL_CE_STOP_ACTIVE;
-    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
+    aspeed_smc_flash_do_select(fl, true);
 }
 
 static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
@@ -911,13 +907,25 @@ static const MemoryRegionOps aspeed_smc_flash_ops = {
     },
 };
 
-static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl)
+static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value)
 {
     AspeedSMCState *s = fl->controller;
+    bool unselect;
+
+    /* User mode selects the CS, other modes unselect */
+    unselect = (value & CTRL_CMD_MODE_MASK) != CTRL_USERMODE;
+
+    /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */
+    if (!(s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE) &&
+        value & CTRL_CE_STOP_ACTIVE) {
+        unselect = true;
+    }
+
+    s->regs[s->r_ctrl0 + fl->id] = value;
 
-    s->snoop_index = aspeed_smc_is_ce_stop_active(fl) ? SNOOP_OFF : SNOOP_START;
+    s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START;
 
-    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
+    aspeed_smc_flash_do_select(fl, unselect);
 }
 
 static void aspeed_smc_reset(DeviceState *d)
@@ -1249,8 +1257,7 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         s->regs[addr] = value;
     } else if (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs) {
         int cs = addr - s->r_ctrl0;
-        s->regs[addr] = value;
-        aspeed_smc_flash_update_cs(&s->flashes[cs]);
+        aspeed_smc_flash_update_ctrl(&s->flashes[cs], value);
     } else if (addr >= R_SEG_ADDR0 &&
                addr < R_SEG_ADDR0 + s->ctrl->max_slaves) {
         int cs = addr - R_SEG_ADDR0;
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
index ffe531a500aa..0a70629801a9 100644
--- a/hw/ssi/trace-events
+++ b/hw/ssi/trace-events
@@ -7,3 +7,4 @@ aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int
 aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
 aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
 aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
+aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
-- 
2.21.1

Re: [PATCH 2/2] aspeed/smc: Fix User mode select/unselect scheme

[Andrew Jeffery]

On Thu, 6 Feb 2020, at 21:56, Cédric Le Goater wrote:
> The Aspeed SMC Controller can operate in different modes : Read, Fast
> Read, Write and User modes. When the User mode is configured, it
> selects automatically the SPI slave device until the CE_STOP_ACTIVE
> bit is set to 1. When any other modes are configured the device is
> unselected. The HW logic handles the chip select automatically when
> the flash is accessed through its AHB window.
> 
> When configuring the CEx Control Register, the User mode logic to
> select and unselect the slave is incorrect and data corruption can be
> seen on machines using two chips, witherspoon and romulus.
> 
> Rework the handler setting the CEx Control Register to fix this issue.
> 
> Fixes: 7c1c69bca43c ("ast2400: add SMC controllers (FMC and SPI)")
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Champion!

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Re: [PATCH 0/2] aspeed/smc: fix data corruption on witherspoon machines

[Cédric Le Goater]

On 2/6/20 12:26 PM, Cédric Le Goater wrote:
> Hello,
> 
> We have been seeing a squashfs corruption on the witherspoon-bmc
> machine for quite some time. It boots but quickly after boot
> corruption errors start to fill the console. This machine has two BMC
> chips with UBIfs on them. The romulus-bmc machine has a similar
> problem when both PNOR are enabled.
> 
> After some investigation, it appeared that the SPI transfers on the
> two chips were getting mixed and after more investigation, the User
> mode select/unselect scheme of the Aspeed SMC model proved to be
> broken. It's been there since the right beginning of the model. Here
> is a fix.
> 
> Kudos to Andrew J. for the time he spent on this and Andrew G. for his
> patience.
> 
> I pushed the code on my github branch aspeed-5.0 for more testing.
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (2):
>   aspeed/smc: Add some tracing
>   aspeed/smc: Fix User mode select/unselect scheme
> 
>  Makefile.objs       |  1 +
>  hw/ssi/aspeed_smc.c | 56 ++++++++++++++++++++++++++++++++-------------
>  hw/ssi/trace-events | 10 ++++++++
>  3 files changed, 51 insertions(+), 16 deletions(-)
>  create mode 100644 hw/ssi/trace-events

Hello Peter, 

If you have some time to take a look, it would be nice to get this 
fix in QEMU 5.0. 

Thanks,

C.

Re: [PATCH 0/2] aspeed/smc: fix data corruption on witherspoon machines

[Peter Maydell]

On Mon, 9 Mar 2020 at 16:32, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 2/6/20 12:26 PM, Cédric Le Goater wrote:
> > Hello,
> >
> > We have been seeing a squashfs corruption on the witherspoon-bmc
> > machine for quite some time. It boots but quickly after boot
> > corruption errors start to fill the console. This machine has two BMC
> > chips with UBIfs on them. The romulus-bmc machine has a similar
> > problem when both PNOR are enabled.


> Hello Peter,
>
> If you have some time to take a look, it would be nice to get this
> fix in QEMU 5.0.

Oops, I think this just fell off my list somehow.


Applied to target-arm.next, thanks.

-- PMM