[Qemu-devel] [PATCH 0/4] aspeed/smc: add fast read support under User command mode.

[Qemu-devel] [PATCH 0/4] aspeed/smc: add fast read support under User command mode.

[Cédric Le Goater]

Hello,

When in the User command mode, the Aspeed SMC controller driver
performs the dummy cycles of a fast read command using byte transfers,
that is ony byte for eight cycles. But, the QEMU m25p80 models one
dummy cycle with one byte transfer.

To restore the correct number of cycles, this series adds a function
snooping the SPI transfers to catch commands requiring dummy cycles
and replaces them with byte transfers compatible with the m25p80 model.

Thanks,

C.

Cédric Le Goater (4):
  aspeed/smc: fix default read value
  aspeed/smc: define registers for all possible CS
  aspeed/smc: Add dummy data register
  aspeed/smc: snoop transfers to fake dummy cycles

 include/hw/ssi/aspeed_smc.h |   3 +
 hw/ssi/aspeed_smc.c         | 128 +++++++++++++++++++++++++++++++++---
 2 files changed, 123 insertions(+), 8 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/4] aspeed/smc: fix default read value

[Cédric Le Goater]

0xFFFFFFFF should be returned for non implemented registers.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 1270842dcf0c..7af808c33c50 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -670,7 +670,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
-        return 0;
+        return -1;
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH 2/4] aspeed/smc: define registers for all possible CS

[Cédric Le Goater]

The model should expose one control register per possible CS. When
testing the validity of the register number in the read operation,
replace 's->num_cs' by 'ctrl->max_slaves' which represents the maximum
number of flash devices a controller can handle.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 7af808c33c50..6045ca11b969 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -665,7 +665,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == s->r_ce_ctrl ||
         addr == R_INTR_CTRL ||
         (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
-        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
+        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
         return s->regs[addr];
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
-- 
2.20.1

[Qemu-devel] [PATCH 3/4] aspeed/smc: Add dummy data register

[Cédric Le Goater]

The SMC controllers have a register containing the byte that will be
used as dummy output. It can be modified by software.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/aspeed_smc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 6045ca11b969..9f3b6f4b4501 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -98,8 +98,8 @@
 /* Misc Control Register #1 */
 #define R_MISC_CTRL1      (0x50 / 4)
 
-/* Misc Control Register #2 */
-#define R_MISC_CTRL2      (0x54 / 4)
+/* SPI dummy cycle data */
+#define R_DUMMY_DATA      (0x54 / 4)
 
 /* DMA Control/Status Register */
 #define R_DMA_CTRL        (0x80 / 4)
@@ -529,7 +529,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
      */
     if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
         for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
-            ssi_transfer(fl->controller->spi, 0xFF);
+            ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff);
         }
     }
 }
@@ -664,6 +664,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == s->r_timings ||
         addr == s->r_ce_ctrl ||
         addr == R_INTR_CTRL ||
+        addr == R_DUMMY_DATA ||
         (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
         (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
         return s->regs[addr];
@@ -697,6 +698,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         if (value != s->regs[R_SEG_ADDR0 + cs]) {
             aspeed_smc_flash_set_segment(s, cs, value);
         }
+    } else if (addr == R_DUMMY_DATA) {
+        s->regs[addr] = value & 0xff;
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
-- 
2.20.1

[Qemu-devel] [PATCH 4/4] aspeed/smc: snoop SPI transfers to fake dummy cycles

[Cédric Le Goater]

The m25p80 models dummy cycles using byte transfers. This works well
when the transfers are initiated by the QEMU model of a SPI controller
but when these are initiated by the OS, it breaks emulation.

Snoop the SPI transfer to catch commands requiring dummy cycles and
replace them with byte transfers compatible with the m25p80 model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
---
 include/hw/ssi/aspeed_smc.h |   3 +
 hw/ssi/aspeed_smc.c         | 115 +++++++++++++++++++++++++++++++++++-
 2 files changed, 115 insertions(+), 3 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 1f557313fa93..3b1e7fce6c86 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -98,6 +98,9 @@ typedef struct AspeedSMCState {
     uint8_t conf_enable_w0;
 
     AspeedSMCFlash *flashes;
+
+    uint8_t snoop_index;
+    uint8_t snoop_dummies;
 } AspeedSMCState;
 
 #endif /* ASPEED_SMC_H */
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 9f3b6f4b4501..f1e66870d71f 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -145,6 +145,9 @@
 /* Flash opcodes. */
 #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
 
+#define SNOOP_OFF         0xFF
+#define SNOOP_START       0x0
+
 /*
  * Default segments mapping addresses and size for each slave per
  * controller. These can be changed when board is initialized with the
@@ -566,6 +569,101 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
     return ret;
 }
 
+/*
+ * TODO (clg@kaod.org): stolen from xilinx_spips.c. Should move to a
+ * common include header.
+ */
+typedef enum {
+    READ = 0x3,         READ_4 = 0x13,
+    FAST_READ = 0xb,    FAST_READ_4 = 0x0c,
+    DOR = 0x3b,         DOR_4 = 0x3c,
+    QOR = 0x6b,         QOR_4 = 0x6c,
+    DIOR = 0xbb,        DIOR_4 = 0xbc,
+    QIOR = 0xeb,        QIOR_4 = 0xec,
+
+    PP = 0x2,           PP_4 = 0x12,
+    DPP = 0xa2,
+    QPP = 0x32,         QPP_4 = 0x34,
+} FlashCMD;
+
+static int aspeed_smc_num_dummies(uint8_t command)
+{
+    switch (command) { /* check for dummies */
+    case READ: /* no dummy bytes/cycles */
+    case PP:
+    case DPP:
+    case QPP:
+    case READ_4:
+    case PP_4:
+    case QPP_4:
+        return 0;
+    case FAST_READ:
+    case DOR:
+    case QOR:
+    case DOR_4:
+    case QOR_4:
+        return 1;
+    case DIOR:
+    case FAST_READ_4:
+    case DIOR_4:
+        return 2;
+    case QIOR:
+    case QIOR_4:
+        return 4;
+    default:
+        return -1;
+    }
+}
+
+static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
+                                unsigned size)
+{
+    AspeedSMCState *s = fl->controller;
+    uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
+
+    if (s->snoop_index == SNOOP_OFF) {
+        return false; /* Do nothing */
+
+    } else if (s->snoop_index == SNOOP_START) {
+        uint8_t cmd = data & 0xff;
+        int ndummies = aspeed_smc_num_dummies(cmd);
+
+        /*
+         * No dummy cycles are expected with the current command. Turn
+         * off snooping and let the transfer proceed normally.
+         */
+        if (ndummies <= 0) {
+            s->snoop_index = SNOOP_OFF;
+            return false;
+        }
+
+        s->snoop_dummies = ndummies * 8;
+
+    } else if (s->snoop_index >= addr_width + 1) {
+
+        /* The SPI transfer has reached the dummy cycles sequence */
+        for (; s->snoop_dummies; s->snoop_dummies--) {
+            ssi_transfer(s->spi, s->regs[R_DUMMY_DATA] & 0xff);
+        }
+
+        /* If no more dummy cycles are expected, turn off snooping */
+        if (!s->snoop_dummies) {
+            s->snoop_index = SNOOP_OFF;
+        } else {
+            s->snoop_index += size;
+        }
+
+        /*
+         * Dummy cycles have been faked already. Ignore the current
+         * SPI transfer
+         */
+        return true;
+    }
+
+    s->snoop_index += size;
+    return false;
+}
+
 static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
                                    unsigned size)
 {
@@ -581,6 +679,10 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
 
     switch (aspeed_smc_flash_mode(fl)) {
     case CTRL_USERMODE:
+        if (aspeed_smc_do_snoop(fl, data, size)) {
+            break;
+        }
+
         for (i = 0; i < size; i++) {
             ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
         }
@@ -613,7 +715,9 @@ static const MemoryRegionOps aspeed_smc_flash_ops = {
 
 static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl)
 {
-    const AspeedSMCState *s = fl->controller;
+    AspeedSMCState *s = fl->controller;
+
+    s->snoop_index = aspeed_smc_is_ce_stop_active(fl) ? SNOOP_OFF : SNOOP_START;
 
     qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
 }
@@ -652,6 +756,9 @@ static void aspeed_smc_reset(DeviceState *d)
     if (s->ctrl->segments == aspeed_segments_fmc) {
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
     }
+
+    s->snoop_index = SNOOP_OFF;
+    s->snoop_dummies = 0;
 }
 
 static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
@@ -793,10 +900,12 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_aspeed_smc = {
     .name = "aspeed.smc",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX),
+        VMSTATE_UINT8(snoop_index, AspeedSMCState),
+        VMSTATE_UINT8(snoop_dummies, AspeedSMCState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/4] aspeed/smc: fix default read value

[Joel Stanley]

On Fri, 25 Jan 2019 at 01:08, Cédric Le Goater <clg@kaod.org> wrote:
>
> 0xFFFFFFFF should be returned for non implemented registers.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

Re: [Qemu-devel] [PATCH 2/4] aspeed/smc: define registers for all possible CS

[Joel Stanley]

On Fri, 25 Jan 2019 at 01:08, Cédric Le Goater <clg@kaod.org> wrote:
>
> The model should expose one control register per possible CS. When
> testing the validity of the register number in the read operation,
> replace 's->num_cs' by 'ctrl->max_slaves' which represents the maximum
> number of flash devices a controller can handle.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

Re: [Qemu-devel] [PATCH 3/4] aspeed/smc: Add dummy data register

[Joel Stanley]

On Fri, 25 Jan 2019 at 01:05, Cédric Le Goater <clg@kaod.org> wrote:
>
> The SMC controllers have a register containing the byte that will be
> used as dummy output. It can be modified by software.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Re: [Qemu-devel] [PATCH 3/4] aspeed/smc: Add dummy data register

[Alistair Francis]

On Thu, Jan 24, 2019 at 6:06 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> The SMC controllers have a register containing the byte that will be
> used as dummy output. It can be modified by software.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/aspeed_smc.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 6045ca11b969..9f3b6f4b4501 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -98,8 +98,8 @@
>  /* Misc Control Register #1 */
>  #define R_MISC_CTRL1      (0x50 / 4)
>
> -/* Misc Control Register #2 */
> -#define R_MISC_CTRL2      (0x54 / 4)
> +/* SPI dummy cycle data */
> +#define R_DUMMY_DATA      (0x54 / 4)
>
>  /* DMA Control/Status Register */
>  #define R_DMA_CTRL        (0x80 / 4)
> @@ -529,7 +529,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>       */
>      if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
>          for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
> -            ssi_transfer(fl->controller->spi, 0xFF);
> +            ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff);
>          }
>      }
>  }
> @@ -664,6 +664,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>          addr == s->r_timings ||
>          addr == s->r_ce_ctrl ||
>          addr == R_INTR_CTRL ||
> +        addr == R_DUMMY_DATA ||
>          (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
>          (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
>          return s->regs[addr];
> @@ -697,6 +698,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>          if (value != s->regs[R_SEG_ADDR0 + cs]) {
>              aspeed_smc_flash_set_segment(s, cs, value);
>          }
> +    } else if (addr == R_DUMMY_DATA) {
> +        s->regs[addr] = value & 0xff;
>      } else {
>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                        __func__, addr);
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH 0/4] aspeed/smc: add fast read support under User command mode.

[Peter Maydell]

On Thu, 24 Jan 2019 at 14:08, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello,
>
> When in the User command mode, the Aspeed SMC controller driver
> performs the dummy cycles of a fast read command using byte transfers,
> that is ony byte for eight cycles. But, the QEMU m25p80 models one
> dummy cycle with one byte transfer.
>
> To restore the correct number of cycles, this series adds a function
> snooping the SPI transfers to catch commands requiring dummy cycles
> and replaces them with byte transfers compatible with the m25p80 model.
>
> Thanks,
>
> C.
>
> Cédric Le Goater (4):
>   aspeed/smc: fix default read value
>   aspeed/smc: define registers for all possible CS
>   aspeed/smc: Add dummy data register
>   aspeed/smc: snoop transfers to fake dummy cycles


Applied to target-arm.next, thanks.

-- PMM