aspeed/smc: Fix User mode select/unselect scheme

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>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Message-id: 20200206112645.21275-3-clg@kaod.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

2 files changed, 24 insertions, 16 deletions

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index e5621bf728..32be2a02b0 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 ffe531a500..0a70629801 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"