[Qemu-devel] [PATCH v2 00/11] Aspeed SMC controller fixes and improvements

[Qemu-devel] [PATCH v2 00/11] Aspeed SMC controller fixes and improvements

[Cédric Le Goater]

Hello,

I have reduced the patchset size to focus on some improvements of the
SMC (Flash) controller model only and will address the watchdog and
network models in other patchset.

The main benefit of this series is to enable booting directly from a
flash image containing U-Boot. It adds :

 - some minor fixes and code rearrangements 
 - Command mode support. Flash contents is accessed directly on the
   AHB bus
 - auto strapping of configuration for boot flash
 - use CE0 as a boot ROM. Today, qemu can not boot from a MMIO region.
   As this is complex to do (TCG layer modification), we use a ROM
   region in which we copy the flash contents. Hopefully, I got it
   right this time.
 - dummy bytes support for Command mode only

Still in the pipe for the SMC controller are :

 - DMA support
 - dummy bytes support in user mode (hacky)

which I will send later.

Thanks,

C.

Cédric Le Goater (11):
  aspeed/smc: remove call to reset in realize function
  aspeed/smc: remove call to aspeed_smc_update_cs() in reset function
  aspeed/smc: rework the prototype of the AspeedSMCFlash helper routines
  aspeed/smc: autostrap CE0/1 configuration
  aspeed/smc: unfold the AspeedSMCController array
  aspeed/smc: adjust the size of the register region
  aspeed/smc: handle SPI flash Command mode
  aspeed/smc: reset flash after each test
  aspeed/smc: extend tests for Command mode
  aspeed: use first SPI flash as a boot ROM
  aspeed/smc: handle dummy bytes when doing fast reads in command mode

 hw/arm/aspeed.c             |  41 ++++++
 hw/ssi/aspeed_smc.c         | 352 +++++++++++++++++++++++++++++++++++---------
 include/hw/ssi/aspeed_smc.h |   4 +-
 tests/m25p80-test.c         | 133 +++++++++++++++++
 4 files changed, 456 insertions(+), 74 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 01/11] aspeed/smc: remove call to reset in realize function

[Cédric Le Goater]

This is useless as reset will be called later on.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 78f5aed53247..8a7217d4df6c 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -541,8 +541,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(sbd, &s->cs_lines[i]);
     }
 
-    aspeed_smc_reset(dev);
-
     /* The memory region for the controller registers */
     memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
                           s->ctrl->name, ASPEED_SMC_R_MAX * 4);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 01/11] aspeed/smc: remove call to reset in realize function

[mar.krzeminski]

W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
> This is useless as reset will be called later on.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>
> ---
>   hw/ssi/aspeed_smc.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 78f5aed53247..8a7217d4df6c 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -541,8 +541,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
>           sysbus_init_irq(sbd, &s->cs_lines[i]);
>       }
>   
> -    aspeed_smc_reset(dev);
> -
>       /* The memory region for the controller registers */
>       memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
>                             s->ctrl->name, ASPEED_SMC_R_MAX * 4);

[Qemu-devel] [PATCH v2 02/11] aspeed/smc: remove call to aspeed_smc_update_cs() in reset function

[Cédric Le Goater]

Instead, we can simply set the irq level when unselecting the slave
devices. This change prepares ground for a subsequent cleanup of the
aspeed_smc_update_cs() routine which uselessly loops on all slaves to
update their status.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 8a7217d4df6c..205c0abfae98 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -424,6 +424,7 @@ static void aspeed_smc_reset(DeviceState *d)
     /* Unselect all slaves */
     for (i = 0; i < s->num_cs; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
+        qemu_set_irq(s->cs_lines[i], true);
     }
 
     /* setup default segment register values for all */
@@ -431,8 +432,6 @@ static void aspeed_smc_reset(DeviceState *d)
         s->regs[R_SEG_ADDR0 + i] =
             aspeed_smc_segment_to_reg(&s->ctrl->segments[i]);
     }
-
-    aspeed_smc_update_cs(s);
 }
 
 static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 02/11] aspeed/smc: remove call to aspeed_smc_update_cs() in reset function

[mar.krzeminski]

W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
> Instead, we can simply set the irq level when unselecting the slave
> devices. This change prepares ground for a subsequent cleanup of the
> aspeed_smc_update_cs() routine which uselessly loops on all slaves to
> update their status.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ssi/aspeed_smc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 8a7217d4df6c..205c0abfae98 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -424,6 +424,7 @@ static void aspeed_smc_reset(DeviceState *d)
>       /* Unselect all slaves */
>       for (i = 0; i < s->num_cs; ++i) {
>           s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
> +        qemu_set_irq(s->cs_lines[i], true);
Does SMC support only flash chips with CS active LOW?
If yes:
Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>
>       }
>   
>       /* setup default segment register values for all */
> @@ -431,8 +432,6 @@ static void aspeed_smc_reset(DeviceState *d)
>           s->regs[R_SEG_ADDR0 + i] =
>               aspeed_smc_segment_to_reg(&s->ctrl->segments[i]);
>       }
> -
> -    aspeed_smc_update_cs(s);
>   }
>   
>   static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)

Re: [Qemu-devel] [PATCH v2 02/11] aspeed/smc: remove call to aspeed_smc_update_cs() in reset function

[Cédric Le Goater]

On 01/16/2017 06:23 PM, mar.krzeminski wrote:
> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>> Instead, we can simply set the irq level when unselecting the slave
>> devices. This change prepares ground for a subsequent cleanup of the
>> aspeed_smc_update_cs() routine which uselessly loops on all slaves to
>> update their status.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ssi/aspeed_smc.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 8a7217d4df6c..205c0abfae98 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -424,6 +424,7 @@ static void aspeed_smc_reset(DeviceState *d)
>>      /* Unselect all slaves */
>>      for (i = 0; i < s->num_cs; ++i) {
>>          s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
>> +        qemu_set_irq(s->cs_lines[i], true);
> Does SMC support only flash chips with CS active LOW?

yes. This is the case.

Thanks,


C. 

> If yes:
> Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>
>>      }
>>  
>>      /* setup default segment register values for all */
>> @@ -431,8 +432,6 @@ static void aspeed_smc_reset(DeviceState *d)
>>          s->regs[R_SEG_ADDR0 + i] =
>>              aspeed_smc_segment_to_reg(&s->ctrl->segments[i]);
>>      }
>> -
>> -    aspeed_smc_update_cs(s);
>>  }
>>  
>>  static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
> 

[Qemu-devel] [PATCH v2 03/11] aspeed/smc: rework the prototype of the AspeedSMCFlash helper routines

[Cédric Le Goater]

Change the routines prototype to use a 'AspeedSMCFlash *' instead of
'AspeedSMCState *'. The result will help in making future changes
clearer.

Also change aspeed_smc_update_cs() which uselessly loops on all slave
devices to update their status.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/ssi/aspeed_smc.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

 Changes since v1:

 - moved reset changes in preliminary patches
 - merged in changes in aspeed_smc_update_cs()

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 205c0abfae98..9b31d5d27012 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -328,19 +328,23 @@ static const MemoryRegionOps aspeed_smc_flash_default_ops = {
     },
 };
 
-static inline int aspeed_smc_flash_mode(const AspeedSMCState *s, int cs)
+static inline int aspeed_smc_flash_mode(const AspeedSMCFlash *fl)
 {
-    return s->regs[s->r_ctrl0 + cs] & CTRL_CMD_MODE_MASK;
+    const AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CMD_MODE_MASK;
 }
 
-static inline bool aspeed_smc_is_usermode(const AspeedSMCState *s, int cs)
+static inline bool aspeed_smc_is_usermode(const AspeedSMCFlash *fl)
 {
-    return aspeed_smc_flash_mode(s, cs) == CTRL_USERMODE;
+    return aspeed_smc_flash_mode(fl) == CTRL_USERMODE;
 }
 
-static inline bool aspeed_smc_is_writable(const AspeedSMCState *s, int cs)
+static inline bool aspeed_smc_is_writable(const AspeedSMCFlash *fl)
 {
-    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
+    const AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + fl->id));
 }
 
 static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
@@ -350,7 +354,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
     uint64_t ret = 0;
     int i;
 
-    if (aspeed_smc_is_usermode(s, fl->id)) {
+    if (aspeed_smc_is_usermode(fl)) {
         for (i = 0; i < size; i++) {
             ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
         }
@@ -370,13 +374,13 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
     const AspeedSMCState *s = fl->controller;
     int i;
 
-    if (!aspeed_smc_is_writable(s, fl->id)) {
+    if (!aspeed_smc_is_writable(fl)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
                       HWADDR_PRIx "\n", __func__, addr);
         return;
     }
 
-    if (!aspeed_smc_is_usermode(s, fl->id)) {
+    if (!aspeed_smc_is_usermode(fl)) {
         qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
                       __func__);
         return;
@@ -397,18 +401,18 @@ static const MemoryRegionOps aspeed_smc_flash_ops = {
     },
 };
 
-static bool aspeed_smc_is_ce_stop_active(const AspeedSMCState *s, int cs)
+static inline bool aspeed_smc_is_ce_stop_active(const AspeedSMCFlash *fl)
 {
-    return s->regs[s->r_ctrl0 + cs] & CTRL_CE_STOP_ACTIVE;
+    const AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE;
 }
 
-static void aspeed_smc_update_cs(const AspeedSMCState *s)
+static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl)
 {
-    int i;
+    const AspeedSMCState *s = fl->controller;
 
-    for (i = 0; i < s->num_cs; ++i) {
-        qemu_set_irq(s->cs_lines[i], aspeed_smc_is_ce_stop_active(s, i));
-    }
+    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
 }
 
 static void aspeed_smc_reset(DeviceState *d)
@@ -481,8 +485,9 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         addr == s->r_ce_ctrl) {
         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_update_cs(s);
+        aspeed_smc_flash_update_cs(&s->flashes[cs]);
     } else if (addr >= R_SEG_ADDR0 &&
                addr < R_SEG_ADDR0 + s->ctrl->max_slaves) {
         int cs = addr - R_SEG_ADDR0;
-- 
2.7.4

[Qemu-devel] [PATCH v2 04/11] aspeed/smc: autostrap CE0/1 configuration

[Cédric Le Goater]

On the AST2500 SoC, the FMC controller flash type is fixed to SPI for
CE0 and CE1 and 4BYTE mode is autodetected for CE0.

On the AST2400 SoC, the FMC controller flash type and 4BYTE mode are
strapped with register SCU70. We use the default settings from the
palmetto-bmc machine for now.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/ssi/aspeed_smc.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

 Changes since v1:

 - splitted ast2400 from ast2500 strapping as they should be handled
   in a different way. ast2400 strapping should be derived from SCU70
   but it seems a little overly complex to do it now.

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 9b31d5d27012..3bd381b13bc2 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -39,11 +39,14 @@
 #define   CONF_ENABLE_W2       18
 #define   CONF_ENABLE_W1       17
 #define   CONF_ENABLE_W0       16
-#define   CONF_FLASH_TYPE4     9
-#define   CONF_FLASH_TYPE3     7
-#define   CONF_FLASH_TYPE2     5
-#define   CONF_FLASH_TYPE1     3
-#define   CONF_FLASH_TYPE0     1
+#define   CONF_FLASH_TYPE4     8
+#define   CONF_FLASH_TYPE3     6
+#define   CONF_FLASH_TYPE2     4
+#define   CONF_FLASH_TYPE1     2
+#define   CONF_FLASH_TYPE0     0
+#define      CONF_FLASH_TYPE_NOR   0x0
+#define      CONF_FLASH_TYPE_NAND  0x1
+#define      CONF_FLASH_TYPE_SPI   0x2
 
 /* CE Control Register */
 #define R_CE_CTRL            (0x04 / 4)
@@ -436,6 +439,25 @@ static void aspeed_smc_reset(DeviceState *d)
         s->regs[R_SEG_ADDR0 + i] =
             aspeed_smc_segment_to_reg(&s->ctrl->segments[i]);
     }
+
+    /* HW strapping for AST2500 FMC controllers  */
+    if (s->ctrl->segments == aspeed_segments_ast2500_fmc) {
+        /* flash type is fixed to SPI for CE0 and CE1 */
+        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
+        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
+
+        /* 4BYTE mode is autodetected for CE0. Let's force it to 1 for
+         * now */
+        s->regs[s->r_ce_ctrl] |= (1 << (CTRL_EXTENDED0));
+    }
+
+    /* HW strapping for AST2400 FMC controllers (SCU70). Let's use the
+     * configuration of the palmetto-bmc machine */
+    if (s->ctrl->segments == aspeed_segments_fmc) {
+        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
+
+        s->regs[s->r_ce_ctrl] |= (1 << (CTRL_EXTENDED0));
+    }
 }
 
 static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
-- 
2.7.4

[Qemu-devel] [PATCH v2 05/11] aspeed/smc: unfold the AspeedSMCController array

[Cédric Le Goater]

This is getting difficult to read. Also add a 'has_dma' field for each
controller type.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/ssi/aspeed_smc.c         | 91 ++++++++++++++++++++++++++++++++++++---------
 include/hw/ssi/aspeed_smc.h |  1 +
 2 files changed, 74 insertions(+), 18 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index d62221a6de58..1ab5575dc848 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -173,24 +173,79 @@ static const AspeedSegments aspeed_segments_ast2500_spi2[] = {
 };
 
 static const AspeedSMCController controllers[] = {
-    { "aspeed.smc.smc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
-      CONF_ENABLE_W0, 5, aspeed_segments_legacy,
-      ASPEED_SOC_SMC_FLASH_BASE, 0x6000000 },
-    { "aspeed.smc.fmc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
-      CONF_ENABLE_W0, 5, aspeed_segments_fmc,
-      ASPEED_SOC_FMC_FLASH_BASE, 0x10000000 },
-    { "aspeed.smc.spi", R_SPI_CONF, 0xff, R_SPI_CTRL0, R_SPI_TIMINGS,
-      SPI_CONF_ENABLE_W0, 1, aspeed_segments_spi,
-      ASPEED_SOC_SPI_FLASH_BASE, 0x10000000 },
-    { "aspeed.smc.ast2500-fmc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
-      CONF_ENABLE_W0, 3, aspeed_segments_ast2500_fmc,
-      ASPEED_SOC_FMC_FLASH_BASE, 0x10000000 },
-    { "aspeed.smc.ast2500-spi1", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
-      CONF_ENABLE_W0, 2, aspeed_segments_ast2500_spi1,
-      ASPEED_SOC_SPI_FLASH_BASE, 0x8000000 },
-    { "aspeed.smc.ast2500-spi2", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
-      CONF_ENABLE_W0, 2, aspeed_segments_ast2500_spi2,
-      ASPEED_SOC_SPI2_FLASH_BASE, 0x8000000 },
+    {
+        .name              = "aspeed.smc.smc",
+        .r_conf            = R_CONF,
+        .r_ce_ctrl         = R_CE_CTRL,
+        .r_ctrl0           = R_CTRL0,
+        .r_timings         = R_TIMINGS,
+        .conf_enable_w0    = CONF_ENABLE_W0,
+        .max_slaves        = 5,
+        .segments          = aspeed_segments_legacy,
+        .flash_window_base = ASPEED_SOC_SMC_FLASH_BASE,
+        .flash_window_size = 0x6000000,
+        .has_dma           = false,
+    }, {
+        .name              = "aspeed.smc.fmc",
+        .r_conf            = R_CONF,
+        .r_ce_ctrl         = R_CE_CTRL,
+        .r_ctrl0           = R_CTRL0,
+        .r_timings         = R_TIMINGS,
+        .conf_enable_w0    = CONF_ENABLE_W0,
+        .max_slaves        = 5,
+        .segments          = aspeed_segments_fmc,
+        .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
+        .flash_window_size = 0x10000000,
+        .has_dma           = true,
+    }, {
+        .name              = "aspeed.smc.spi",
+        .r_conf            = R_SPI_CONF,
+        .r_ce_ctrl         = 0xff,
+        .r_ctrl0           = R_SPI_CTRL0,
+        .r_timings         = R_SPI_TIMINGS,
+        .conf_enable_w0    = SPI_CONF_ENABLE_W0,
+        .max_slaves        = 1,
+        .segments          = aspeed_segments_spi,
+        .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
+        .flash_window_size = 0x10000000,
+        .has_dma           = false,
+    }, {
+        .name              = "aspeed.smc.ast2500-fmc",
+        .r_conf            = R_CONF,
+        .r_ce_ctrl         = R_CE_CTRL,
+        .r_ctrl0           = R_CTRL0,
+        .r_timings         = R_TIMINGS,
+        .conf_enable_w0    = CONF_ENABLE_W0,
+        .max_slaves        = 3,
+        .segments          = aspeed_segments_ast2500_fmc,
+        .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
+        .flash_window_size = 0x10000000,
+        .has_dma           = true,
+    }, {
+        .name              = "aspeed.smc.ast2500-spi1",
+        .r_conf            = R_CONF,
+        .r_ce_ctrl         = R_CE_CTRL,
+        .r_ctrl0           = R_CTRL0,
+        .r_timings         = R_TIMINGS,
+        .conf_enable_w0    = CONF_ENABLE_W0,
+        .max_slaves        = 2,
+        .segments          = aspeed_segments_ast2500_spi1,
+        .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
+        .flash_window_size = 0x8000000,
+        .has_dma           = false,
+    }, {
+        .name              = "aspeed.smc.ast2500-spi2",
+        .r_conf            = R_CONF,
+        .r_ce_ctrl         = R_CE_CTRL,
+        .r_ctrl0           = R_CTRL0,
+        .r_timings         = R_TIMINGS,
+        .conf_enable_w0    = CONF_ENABLE_W0,
+        .max_slaves        = 2,
+        .segments          = aspeed_segments_ast2500_spi2,
+        .flash_window_base = ASPEED_SOC_SPI2_FLASH_BASE,
+        .flash_window_size = 0x8000000,
+        .has_dma           = false,
+    },
 };
 
 /*
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index bdfbcc0ffa7d..861120b6e213 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -44,6 +44,7 @@ typedef struct AspeedSMCController {
     const AspeedSegments *segments;
     hwaddr flash_window_base;
     uint32_t flash_window_size;
+    bool has_dma;
 } AspeedSMCController;
 
 typedef struct AspeedSMCFlash {
-- 
2.7.4

[Qemu-devel] [PATCH v2 06/11] aspeed/smc: adjust the size of the register region

[Cédric Le Goater]

The SPI controller of the AST2400 SoC has less registers. So we can
adjust the size of the memory region holding the registers depending
on the controller type. We can also remove the guest_error logging
which is useless as the range of the region is strict enough.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 hw/ssi/aspeed_smc.c         | 25 ++++++++++---------------
 include/hw/ssi/aspeed_smc.h |  1 +
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 1ab5575dc848..7103d0c5b64a 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -130,6 +130,9 @@
 #define R_SPI_MISC_CTRL   (0x10 / 4)
 #define R_SPI_TIMINGS     (0x14 / 4)
 
+#define ASPEED_SMC_R_SPI_MAX (0x20 / 4)
+#define ASPEED_SMC_R_SMC_MAX (0x20 / 4)
+
 #define ASPEED_SOC_SMC_FLASH_BASE   0x10000000
 #define ASPEED_SOC_FMC_FLASH_BASE   0x20000000
 #define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
@@ -185,6 +188,7 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_SMC_FLASH_BASE,
         .flash_window_size = 0x6000000,
         .has_dma           = false,
+        .nregs             = ASPEED_SMC_R_SMC_MAX,
     }, {
         .name              = "aspeed.smc.fmc",
         .r_conf            = R_CONF,
@@ -197,6 +201,7 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
         .flash_window_size = 0x10000000,
         .has_dma           = true,
+        .nregs             = ASPEED_SMC_R_MAX,
     }, {
         .name              = "aspeed.smc.spi",
         .r_conf            = R_SPI_CONF,
@@ -209,6 +214,7 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
         .flash_window_size = 0x10000000,
         .has_dma           = false,
+        .nregs             = ASPEED_SMC_R_SPI_MAX,
     }, {
         .name              = "aspeed.smc.ast2500-fmc",
         .r_conf            = R_CONF,
@@ -221,6 +227,7 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
         .flash_window_size = 0x10000000,
         .has_dma           = true,
+        .nregs             = ASPEED_SMC_R_MAX,
     }, {
         .name              = "aspeed.smc.ast2500-spi1",
         .r_conf            = R_CONF,
@@ -233,6 +240,7 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
         .flash_window_size = 0x8000000,
         .has_dma           = false,
+        .nregs             = ASPEED_SMC_R_MAX,
     }, {
         .name              = "aspeed.smc.ast2500-spi2",
         .r_conf            = R_CONF,
@@ -245,6 +253,7 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_SPI2_FLASH_BASE,
         .flash_window_size = 0x8000000,
         .has_dma           = false,
+        .nregs             = ASPEED_SMC_R_MAX,
     },
 };
 
@@ -522,13 +531,6 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
 
     addr >>= 2;
 
-    if (addr >= ARRAY_SIZE(s->regs)) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: Out-of-bounds read at 0x%" HWADDR_PRIx "\n",
-                      __func__, addr);
-        return 0;
-    }
-
     if (addr == s->r_conf ||
         addr == s->r_timings ||
         addr == s->r_ce_ctrl ||
@@ -551,13 +553,6 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
 
     addr >>= 2;
 
-    if (addr >= ARRAY_SIZE(s->regs)) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: Out-of-bounds write at 0x%" HWADDR_PRIx "\n",
-                      __func__, addr);
-        return;
-    }
-
     if (addr == s->r_conf ||
         addr == s->r_timings ||
         addr == s->r_ce_ctrl) {
@@ -625,7 +620,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
     /* The memory region for the controller registers */
     memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
-                          s->ctrl->name, ASPEED_SMC_R_MAX * 4);
+                          s->ctrl->name, s->ctrl->nregs * 4);
     sysbus_init_mmio(sbd, &s->mmio);
 
     /*
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 861120b6e213..e811742728f8 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -45,6 +45,7 @@ typedef struct AspeedSMCController {
     hwaddr flash_window_base;
     uint32_t flash_window_size;
     bool has_dma;
+    uint32_t nregs;
 } AspeedSMCController;
 
 typedef struct AspeedSMCFlash {
-- 
2.7.4

[Qemu-devel] [PATCH v2 07/11] aspeed/smc: handle SPI flash Command mode

[Cédric Le Goater]

The Aspeed SMC controllers have a mode (Command mode) in which
accesses to the flash content are no different than doing MMIOs. The
controller generates all the necessary commands to load (or store)
data in memory.

However, accesses are restricted to the segment window assigned the
the flash module by the controller. This window is defined by the
Segment Address Register.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/ssi/aspeed_smc.c         | 152 ++++++++++++++++++++++++++++++++++++++------
 include/hw/ssi/aspeed_smc.h |   2 +-
 2 files changed, 132 insertions(+), 22 deletions(-)

 Changes since v1:

 - removed use of some SPI commands. Firmware should make sure the
   chip is properly configured before using the command mode.

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 7103d0c5b64a..28d563a5800f 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -69,6 +69,7 @@
 #define R_CTRL0           (0x10 / 4)
 #define   CTRL_CMD_SHIFT           16
 #define   CTRL_CMD_MASK            0xff
+#define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
 #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
 #define   CTRL_CMD_MODE_MASK       0x3
 #define     CTRL_READMODE          0x0
@@ -138,6 +139,9 @@
 #define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
 #define ASPEED_SOC_SPI2_FLASH_BASE  0x38000000
 
+/* Flash opcodes. */
+#define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
+
 /*
  * Default segments mapping addresses and size for each slave per
  * controller. These can be changed when board is initialized with the
@@ -414,21 +418,123 @@ static inline bool aspeed_smc_is_writable(const AspeedSMCFlash *fl)
     return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + fl->id));
 }
 
+static inline int aspeed_smc_flash_cmd(const AspeedSMCFlash *fl)
+{
+    const AspeedSMCState *s = fl->controller;
+    int cmd = (s->regs[s->r_ctrl0 + fl->id] >> CTRL_CMD_SHIFT) & CTRL_CMD_MASK;
+
+    /* In read mode, the default SPI command is READ (0x3). In other
+     * modes, the command should necessarily be defined */
+    if (aspeed_smc_flash_mode(fl) == CTRL_READMODE) {
+        cmd = SPI_OP_READ;
+    }
+
+    if (!cmd) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no command defined for mode %d\n",
+                      __func__, aspeed_smc_flash_mode(fl));
+    }
+
+    return cmd;
+}
+
+static inline int aspeed_smc_flash_is_4byte(const AspeedSMCFlash *fl)
+{
+    const AspeedSMCState *s = fl->controller;
+
+    if (s->ctrl->segments == aspeed_segments_spi) {
+        return s->regs[s->r_ctrl0] & CTRL_AST2400_SPI_4BYTE;
+    } else {
+        return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->id));
+    }
+}
+
+static inline bool aspeed_smc_is_ce_stop_active(const AspeedSMCFlash *fl)
+{
+    const AspeedSMCState *s = fl->controller;
+
+    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE;
+}
+
+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));
+}
+
+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));
+}
+
+static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
+                                              uint32_t addr)
+{
+    const AspeedSMCState *s = fl->controller;
+    AspeedSegments seg;
+
+    aspeed_smc_reg_to_segment(s->regs[R_SEG_ADDR0 + fl->id], &seg);
+    if ((addr & (seg.size - 1)) != addr) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid address 0x%08x for CS%d segment : "
+                      "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
+                      s->ctrl->name, addr, fl->id, seg.addr,
+                      seg.addr + seg.size);
+    }
+
+    addr &= seg.size - 1;
+    return addr;
+}
+
+static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
+{
+    const AspeedSMCState *s = fl->controller;
+    uint8_t cmd = aspeed_smc_flash_cmd(fl);
+
+    /* Flash access can not exceed CS segment */
+    addr = aspeed_smc_check_segment_addr(fl, addr);
+
+    ssi_transfer(s->spi, cmd);
+
+    if (aspeed_smc_flash_is_4byte(fl)) {
+        ssi_transfer(s->spi, (addr >> 24) & 0xff);
+    }
+    ssi_transfer(s->spi, (addr >> 16) & 0xff);
+    ssi_transfer(s->spi, (addr >> 8) & 0xff);
+    ssi_transfer(s->spi, (addr & 0xff));
+}
+
 static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
 {
     AspeedSMCFlash *fl = opaque;
-    const AspeedSMCState *s = fl->controller;
+    AspeedSMCState *s = fl->controller;
     uint64_t ret = 0;
     int i;
 
-    if (aspeed_smc_is_usermode(fl)) {
+    switch (aspeed_smc_flash_mode(fl)) {
+    case CTRL_USERMODE:
         for (i = 0; i < size; i++) {
             ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
         }
-    } else {
-        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
-                      __func__);
-        ret = -1;
+        break;
+    case CTRL_READMODE:
+    case CTRL_FREADMODE:
+        aspeed_smc_flash_select(fl);
+        aspeed_smc_flash_send_addr(fl, addr);
+
+        for (i = 0; i < size; i++) {
+            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
+        }
+
+        aspeed_smc_flash_unselect(fl);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid flash mode %d\n",
+                      __func__, aspeed_smc_flash_mode(fl));
     }
 
     return ret;
@@ -438,7 +544,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
                            unsigned size)
 {
     AspeedSMCFlash *fl = opaque;
-    const AspeedSMCState *s = fl->controller;
+    AspeedSMCState *s = fl->controller;
     int i;
 
     if (!aspeed_smc_is_writable(fl)) {
@@ -447,14 +553,25 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
         return;
     }
 
-    if (!aspeed_smc_is_usermode(fl)) {
-        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
-                      __func__);
-        return;
-    }
+    switch (aspeed_smc_flash_mode(fl)) {
+    case CTRL_USERMODE:
+        for (i = 0; i < size; i++) {
+            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
+        }
+        break;
+    case CTRL_WRITEMODE:
+        aspeed_smc_flash_select(fl);
+        aspeed_smc_flash_send_addr(fl, addr);
+
+        for (i = 0; i < size; i++) {
+            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
+        }
 
-    for (i = 0; i < size; i++) {
-        ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
+        aspeed_smc_flash_unselect(fl);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid flash mode %d\n",
+                      __func__, aspeed_smc_flash_mode(fl));
     }
 }
 
@@ -468,13 +585,6 @@ static const MemoryRegionOps aspeed_smc_flash_ops = {
     },
 };
 
-static inline bool aspeed_smc_is_ce_stop_active(const AspeedSMCFlash *fl)
-{
-    const AspeedSMCState *s = fl->controller;
-
-    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE;
-}
-
 static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index e811742728f8..1f557313fa93 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -49,7 +49,7 @@ typedef struct AspeedSMCController {
 } AspeedSMCController;
 
 typedef struct AspeedSMCFlash {
-    const struct AspeedSMCState *controller;
+    struct AspeedSMCState *controller;
 
     uint8_t id;
     uint32_t size;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: handle SPI flash Command mode

[Peter Maydell]

On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
> The Aspeed SMC controllers have a mode (Command mode) in which
> accesses to the flash content are no different than doing MMIOs. The
> controller generates all the necessary commands to load (or store)
> data in memory.
>
> However, accesses are restricted to the segment window assigned the
> the flash module by the controller. This window is defined by the
> Segment Address Register.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/ssi/aspeed_smc.c         | 152 ++++++++++++++++++++++++++++++++++++++------
>  include/hw/ssi/aspeed_smc.h |   2 +-
>  2 files changed, 132 insertions(+), 22 deletions(-)

This deleted the only call to aspeed_smc_is_usermode() but not
its definition, which makes clang complain:
/Users/pm215/src/qemu-for-merges/hw/ssi/aspeed_smc.c:409:20: error:
unused function 'aspeed_smc_is_usermode' [-Werror,-Wunused-function]

Presumably the function itself should be deleted?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: handle SPI flash Command mode

[Cédric Le Goater]

On 01/19/2017 08:26 PM, Peter Maydell wrote:
> On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
>> The Aspeed SMC controllers have a mode (Command mode) in which
>> accesses to the flash content are no different than doing MMIOs. The
>> controller generates all the necessary commands to load (or store)
>> data in memory.
>>
>> However, accesses are restricted to the segment window assigned the
>> the flash module by the controller. This window is defined by the
>> Segment Address Register.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>>  hw/ssi/aspeed_smc.c         | 152 ++++++++++++++++++++++++++++++++++++++------
>>  include/hw/ssi/aspeed_smc.h |   2 +-
>>  2 files changed, 132 insertions(+), 22 deletions(-)
> 
> This deleted the only call to aspeed_smc_is_usermode() but not
> its definition, which makes clang complain:
> /Users/pm215/src/qemu-for-merges/hw/ssi/aspeed_smc.c:409:20: error:
> unused function 'aspeed_smc_is_usermode' [-Werror,-Wunused-function]
> 
> Presumably the function itself should be deleted?

yes. This is correct. I will send a patch for it.

Thanks,

C.

Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: handle SPI flash Command mode

[Peter Maydell]

On 19 January 2017 at 20:35, Cédric Le Goater <clg@kaod.org> wrote:
> On 01/19/2017 08:26 PM, Peter Maydell wrote:
>> On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
>>> The Aspeed SMC controllers have a mode (Command mode) in which
>>> accesses to the flash content are no different than doing MMIOs. The
>>> controller generates all the necessary commands to load (or store)
>>> data in memory.
>>>
>>> However, accesses are restricted to the segment window assigned the
>>> the flash module by the controller. This window is defined by the
>>> Segment Address Register.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>>  hw/ssi/aspeed_smc.c         | 152 ++++++++++++++++++++++++++++++++++++++------
>>>  include/hw/ssi/aspeed_smc.h |   2 +-
>>>  2 files changed, 132 insertions(+), 22 deletions(-)
>>
>> This deleted the only call to aspeed_smc_is_usermode() but not
>> its definition, which makes clang complain:
>> /Users/pm215/src/qemu-for-merges/hw/ssi/aspeed_smc.c:409:20: error:
>> unused function 'aspeed_smc_is_usermode' [-Werror,-Wunused-function]
>>
>> Presumably the function itself should be deleted?
>
> yes. This is correct. I will send a patch for it.

I'll just edit the commit in my target-arm tree, that
will be simplest.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: handle SPI flash Command mode

[Cédric Le Goater]

On 01/20/2017 11:13 AM, Peter Maydell wrote:
> On 19 January 2017 at 20:35, Cédric Le Goater <clg@kaod.org> wrote:
>> On 01/19/2017 08:26 PM, Peter Maydell wrote:
>>> On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
>>>> The Aspeed SMC controllers have a mode (Command mode) in which
>>>> accesses to the flash content are no different than doing MMIOs. The
>>>> controller generates all the necessary commands to load (or store)
>>>> data in memory.
>>>>
>>>> However, accesses are restricted to the segment window assigned the
>>>> the flash module by the controller. This window is defined by the
>>>> Segment Address Register.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>>> ---
>>>>  hw/ssi/aspeed_smc.c         | 152 ++++++++++++++++++++++++++++++++++++++------
>>>>  include/hw/ssi/aspeed_smc.h |   2 +-
>>>>  2 files changed, 132 insertions(+), 22 deletions(-)
>>>
>>> This deleted the only call to aspeed_smc_is_usermode() but not
>>> its definition, which makes clang complain:
>>> /Users/pm215/src/qemu-for-merges/hw/ssi/aspeed_smc.c:409:20: error:
>>> unused function 'aspeed_smc_is_usermode' [-Werror,-Wunused-function]
>>>
>>> Presumably the function itself should be deleted?
>>
>> yes. This is correct. I will send a patch for it.
> 
> I'll just edit the commit in my target-arm tree, that
> will be simplest.

ok. sure. 

I do push a branch on github before sending for travis to test.
Should that error have been caught by travis ? Just asking, as
I might have missed it.

Thanks,

C.   

Re: [Qemu-devel] [PATCH v2 07/11] aspeed/smc: handle SPI flash Command mode

[Cédric Le Goater]

On 01/20/2017 11:17 AM, Cédric Le Goater wrote:
> On 01/20/2017 11:13 AM, Peter Maydell wrote:
>> On 19 January 2017 at 20:35, Cédric Le Goater <clg@kaod.org> wrote:
>>> On 01/19/2017 08:26 PM, Peter Maydell wrote:
>>>> On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
>>>>> The Aspeed SMC controllers have a mode (Command mode) in which
>>>>> accesses to the flash content are no different than doing MMIOs. The
>>>>> controller generates all the necessary commands to load (or store)
>>>>> data in memory.
>>>>>
>>>>> However, accesses are restricted to the segment window assigned the
>>>>> the flash module by the controller. This window is defined by the
>>>>> Segment Address Register.
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>>>> ---
>>>>>  hw/ssi/aspeed_smc.c         | 152 ++++++++++++++++++++++++++++++++++++++------
>>>>>  include/hw/ssi/aspeed_smc.h |   2 +-
>>>>>  2 files changed, 132 insertions(+), 22 deletions(-)
>>>>
>>>> This deleted the only call to aspeed_smc_is_usermode() but not
>>>> its definition, which makes clang complain:
>>>> /Users/pm215/src/qemu-for-merges/hw/ssi/aspeed_smc.c:409:20: error:
>>>> unused function 'aspeed_smc_is_usermode' [-Werror,-Wunused-function]
>>>>
>>>> Presumably the function itself should be deleted?
>>>
>>> yes. This is correct. I will send a patch for it.
>>
>> I'll just edit the commit in my target-arm tree, that
>> will be simplest.
> 
> ok. sure. 
> 
> I do push a branch on github before sending for travis to test.
> Should that error have been caught by travis ? Just asking, as
> I might have missed it.

So I did miss it. I will take a closer look at the logs next time.

Thanks,

C.

[Qemu-devel] [PATCH v2 08/11] aspeed/smc: reset flash after each test

[Cédric Le Goater]

Let's make sure when each test is run that the flash object is in an
initial state and did not keep configuration from the previous tests.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 tests/m25p80-test.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
index cb7ec81f1a6d..8dd550deb95e 100644
--- a/tests/m25p80-test.c
+++ b/tests/m25p80-test.c
@@ -50,6 +50,8 @@ enum {
     READ = 0x03,
     PP = 0x02,
     WREN = 0x6,
+    RESET_ENABLE = 0x66,
+    RESET_MEMORY = 0x99,
     EN_4BYTE_ADDR = 0xB7,
     ERASE_SECTOR = 0xd8,
 };
@@ -76,6 +78,14 @@ static void spi_conf(uint32_t value)
     writel(ASPEED_FMC_BASE + R_CONF, conf);
 }
 
+static void spi_conf_remove(uint32_t value)
+{
+    uint32_t conf = readl(ASPEED_FMC_BASE + R_CONF);
+
+    conf &= ~value;
+    writel(ASPEED_FMC_BASE + R_CONF, conf);
+}
+
 static void spi_ctrl_start_user(void)
 {
     uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
@@ -95,6 +105,18 @@ static void spi_ctrl_stop_user(void)
     writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
 }
 
+static void flash_reset(void)
+{
+    spi_conf(CONF_ENABLE_W0);
+
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, RESET_ENABLE);
+    writeb(ASPEED_FLASH_BASE, RESET_MEMORY);
+    spi_ctrl_stop_user();
+
+    spi_conf_remove(CONF_ENABLE_W0);
+}
+
 static void test_read_jedec(void)
 {
     uint32_t jedec = 0x0;
@@ -108,6 +130,8 @@ static void test_read_jedec(void)
     jedec |= readb(ASPEED_FLASH_BASE);
     spi_ctrl_stop_user();
 
+    flash_reset();
+
     g_assert_cmphex(jedec, ==, FLASH_JEDEC);
 }
 
@@ -155,6 +179,8 @@ static void test_erase_sector(void)
     for (i = 0; i < PAGE_SIZE / 4; i++) {
         g_assert_cmphex(page[i], ==, 0xffffffff);
     }
+
+    flash_reset();
 }
 
 static void test_erase_all(void)
@@ -182,6 +208,8 @@ static void test_erase_all(void)
     for (i = 0; i < PAGE_SIZE / 4; i++) {
         g_assert_cmphex(page[i], ==, 0xffffffff);
     }
+
+    flash_reset();
 }
 
 static void test_write_page(void)
@@ -195,6 +223,7 @@ static void test_write_page(void)
 
     spi_ctrl_start_user();
     writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
+    writeb(ASPEED_FLASH_BASE, WREN);
     writeb(ASPEED_FLASH_BASE, PP);
     writel(ASPEED_FLASH_BASE, make_be32(my_page_addr));
 
@@ -215,6 +244,8 @@ static void test_write_page(void)
     for (i = 0; i < PAGE_SIZE / 4; i++) {
         g_assert_cmphex(page[i], ==, 0xffffffff);
     }
+
+    flash_reset();
 }
 
 static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: reset flash after each test

[mar.krzeminski]

W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
> Let's make sure when each test is run that the flash object is in an
> initial state and did not keep configuration from the previous tests.
Is there any particukar reason why you do that (bug/side 
effect/whatever) or you
just want to be sure what the flash state is?

Thanks,
Marcin
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   tests/m25p80-test.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
> index cb7ec81f1a6d..8dd550deb95e 100644
> --- a/tests/m25p80-test.c
> +++ b/tests/m25p80-test.c
> @@ -50,6 +50,8 @@ enum {
>       READ = 0x03,
>       PP = 0x02,
>       WREN = 0x6,
> +    RESET_ENABLE = 0x66,
> +    RESET_MEMORY = 0x99,
>       EN_4BYTE_ADDR = 0xB7,
>       ERASE_SECTOR = 0xd8,
>   };
> @@ -76,6 +78,14 @@ static void spi_conf(uint32_t value)
>       writel(ASPEED_FMC_BASE + R_CONF, conf);
>   }
>   
> +static void spi_conf_remove(uint32_t value)
> +{
> +    uint32_t conf = readl(ASPEED_FMC_BASE + R_CONF);
> +
> +    conf &= ~value;
> +    writel(ASPEED_FMC_BASE + R_CONF, conf);
> +}
> +
>   static void spi_ctrl_start_user(void)
>   {
>       uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
> @@ -95,6 +105,18 @@ static void spi_ctrl_stop_user(void)
>       writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
>   }
>   
> +static void flash_reset(void)
> +{
> +    spi_conf(CONF_ENABLE_W0);
> +
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, RESET_ENABLE);
> +    writeb(ASPEED_FLASH_BASE, RESET_MEMORY);
> +    spi_ctrl_stop_user();
> +
> +    spi_conf_remove(CONF_ENABLE_W0);
> +}
> +
>   static void test_read_jedec(void)
>   {
>       uint32_t jedec = 0x0;
> @@ -108,6 +130,8 @@ static void test_read_jedec(void)
>       jedec |= readb(ASPEED_FLASH_BASE);
>       spi_ctrl_stop_user();
>   
> +    flash_reset();
> +
>       g_assert_cmphex(jedec, ==, FLASH_JEDEC);
>   }
>   
> @@ -155,6 +179,8 @@ static void test_erase_sector(void)
>       for (i = 0; i < PAGE_SIZE / 4; i++) {
>           g_assert_cmphex(page[i], ==, 0xffffffff);
>       }
> +
> +    flash_reset();
>   }
>   
>   static void test_erase_all(void)
> @@ -182,6 +208,8 @@ static void test_erase_all(void)
>       for (i = 0; i < PAGE_SIZE / 4; i++) {
>           g_assert_cmphex(page[i], ==, 0xffffffff);
>       }
> +
> +    flash_reset();
>   }
>   
>   static void test_write_page(void)
> @@ -195,6 +223,7 @@ static void test_write_page(void)
>   
>       spi_ctrl_start_user();
>       writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
> +    writeb(ASPEED_FLASH_BASE, WREN);
>       writeb(ASPEED_FLASH_BASE, PP);
>       writel(ASPEED_FLASH_BASE, make_be32(my_page_addr));
>   
> @@ -215,6 +244,8 @@ static void test_write_page(void)
>       for (i = 0; i < PAGE_SIZE / 4; i++) {
>           g_assert_cmphex(page[i], ==, 0xffffffff);
>       }
> +
> +    flash_reset();
>   }
>   
>   static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: reset flash after each test

[Cédric Le Goater]

On 01/16/2017 06:37 PM, mar.krzeminski wrote:
> 
> 
> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>> Let's make sure when each test is run that the flash object is in an
>> initial state and did not keep configuration from the previous tests.
> Is there any particukar reason why you do that (bug/side effect/whatever) or you
> just want to be sure what the flash state is?

I want to be sure it is not in 4BYTE mode.

C. 

> 
> Thanks,
> Marcin
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   tests/m25p80-test.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
>> index cb7ec81f1a6d..8dd550deb95e 100644
>> --- a/tests/m25p80-test.c
>> +++ b/tests/m25p80-test.c
>> @@ -50,6 +50,8 @@ enum {
>>       READ = 0x03,
>>       PP = 0x02,
>>       WREN = 0x6,
>> +    RESET_ENABLE = 0x66,
>> +    RESET_MEMORY = 0x99,
>>       EN_4BYTE_ADDR = 0xB7,
>>       ERASE_SECTOR = 0xd8,
>>   };
>> @@ -76,6 +78,14 @@ static void spi_conf(uint32_t value)
>>       writel(ASPEED_FMC_BASE + R_CONF, conf);
>>   }
>>   +static void spi_conf_remove(uint32_t value)
>> +{
>> +    uint32_t conf = readl(ASPEED_FMC_BASE + R_CONF);
>> +
>> +    conf &= ~value;
>> +    writel(ASPEED_FMC_BASE + R_CONF, conf);
>> +}
>> +
>>   static void spi_ctrl_start_user(void)
>>   {
>>       uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
>> @@ -95,6 +105,18 @@ static void spi_ctrl_stop_user(void)
>>       writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
>>   }
>>   +static void flash_reset(void)
>> +{
>> +    spi_conf(CONF_ENABLE_W0);
>> +
>> +    spi_ctrl_start_user();
>> +    writeb(ASPEED_FLASH_BASE, RESET_ENABLE);
>> +    writeb(ASPEED_FLASH_BASE, RESET_MEMORY);
>> +    spi_ctrl_stop_user();
>> +
>> +    spi_conf_remove(CONF_ENABLE_W0);
>> +}
>> +
>>   static void test_read_jedec(void)
>>   {
>>       uint32_t jedec = 0x0;
>> @@ -108,6 +130,8 @@ static void test_read_jedec(void)
>>       jedec |= readb(ASPEED_FLASH_BASE);
>>       spi_ctrl_stop_user();
>>   +    flash_reset();
>> +
>>       g_assert_cmphex(jedec, ==, FLASH_JEDEC);
>>   }
>>   @@ -155,6 +179,8 @@ static void test_erase_sector(void)
>>       for (i = 0; i < PAGE_SIZE / 4; i++) {
>>           g_assert_cmphex(page[i], ==, 0xffffffff);
>>       }
>> +
>> +    flash_reset();
>>   }
>>     static void test_erase_all(void)
>> @@ -182,6 +208,8 @@ static void test_erase_all(void)
>>       for (i = 0; i < PAGE_SIZE / 4; i++) {
>>           g_assert_cmphex(page[i], ==, 0xffffffff);
>>       }
>> +
>> +    flash_reset();
>>   }
>>     static void test_write_page(void)
>> @@ -195,6 +223,7 @@ static void test_write_page(void)
>>         spi_ctrl_start_user();
>>       writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>> +    writeb(ASPEED_FLASH_BASE, WREN);
>>       writeb(ASPEED_FLASH_BASE, PP);
>>       writel(ASPEED_FLASH_BASE, make_be32(my_page_addr));
>>   @@ -215,6 +244,8 @@ static void test_write_page(void)
>>       for (i = 0; i < PAGE_SIZE / 4; i++) {
>>           g_assert_cmphex(page[i], ==, 0xffffffff);
>>       }
>> +
>> +    flash_reset();
>>   }
>>     static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
> 

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: reset flash after each test

[mar.krzeminski]

W dniu 16.01.2017 o 19:10, Cédric Le Goater pisze:
> On 01/16/2017 06:37 PM, mar.krzeminski wrote:
>>
>> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>>> Let's make sure when each test is run that the flash object is in an
>>> initial state and did not keep configuration from the previous tests.
>> Is there any particukar reason why you do that (bug/side effect/whatever) or you
>> just want to be sure what the flash state is?
> I want to be sure it is not in 4BYTE mode.
There is a command for that (EX_4BYTE_ADDR = 0xE9), but reset also make 
sense here.

Thanks,
Marcin
>
> C.
>
>> Thanks,
>> Marcin
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>    tests/m25p80-test.c | 31 +++++++++++++++++++++++++++++++
>>>    1 file changed, 31 insertions(+)
>>>
>>> diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
>>> index cb7ec81f1a6d..8dd550deb95e 100644
>>> --- a/tests/m25p80-test.c
>>> +++ b/tests/m25p80-test.c
>>> @@ -50,6 +50,8 @@ enum {
>>>        READ = 0x03,
>>>        PP = 0x02,
>>>        WREN = 0x6,
>>> +    RESET_ENABLE = 0x66,
>>> +    RESET_MEMORY = 0x99,
>>>        EN_4BYTE_ADDR = 0xB7,
>>>        ERASE_SECTOR = 0xd8,
>>>    };
>>> @@ -76,6 +78,14 @@ static void spi_conf(uint32_t value)
>>>        writel(ASPEED_FMC_BASE + R_CONF, conf);
>>>    }
>>>    +static void spi_conf_remove(uint32_t value)
>>> +{
>>> +    uint32_t conf = readl(ASPEED_FMC_BASE + R_CONF);
>>> +
>>> +    conf &= ~value;
>>> +    writel(ASPEED_FMC_BASE + R_CONF, conf);
>>> +}
>>> +
>>>    static void spi_ctrl_start_user(void)
>>>    {
>>>        uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
>>> @@ -95,6 +105,18 @@ static void spi_ctrl_stop_user(void)
>>>        writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
>>>    }
>>>    +static void flash_reset(void)
>>> +{
>>> +    spi_conf(CONF_ENABLE_W0);
>>> +
>>> +    spi_ctrl_start_user();
>>> +    writeb(ASPEED_FLASH_BASE, RESET_ENABLE);
>>> +    writeb(ASPEED_FLASH_BASE, RESET_MEMORY);
>>> +    spi_ctrl_stop_user();
>>> +
>>> +    spi_conf_remove(CONF_ENABLE_W0);
>>> +}
>>> +
>>>    static void test_read_jedec(void)
>>>    {
>>>        uint32_t jedec = 0x0;
>>> @@ -108,6 +130,8 @@ static void test_read_jedec(void)
>>>        jedec |= readb(ASPEED_FLASH_BASE);
>>>        spi_ctrl_stop_user();
>>>    +    flash_reset();
>>> +
>>>        g_assert_cmphex(jedec, ==, FLASH_JEDEC);
>>>    }
>>>    @@ -155,6 +179,8 @@ static void test_erase_sector(void)
>>>        for (i = 0; i < PAGE_SIZE / 4; i++) {
>>>            g_assert_cmphex(page[i], ==, 0xffffffff);
>>>        }
>>> +
>>> +    flash_reset();
>>>    }
>>>      static void test_erase_all(void)
>>> @@ -182,6 +208,8 @@ static void test_erase_all(void)
>>>        for (i = 0; i < PAGE_SIZE / 4; i++) {
>>>            g_assert_cmphex(page[i], ==, 0xffffffff);
>>>        }
>>> +
>>> +    flash_reset();
>>>    }
>>>      static void test_write_page(void)
>>> @@ -195,6 +223,7 @@ static void test_write_page(void)
>>>          spi_ctrl_start_user();
>>>        writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>>> +    writeb(ASPEED_FLASH_BASE, WREN);
>>>        writeb(ASPEED_FLASH_BASE, PP);
>>>        writel(ASPEED_FLASH_BASE, make_be32(my_page_addr));
>>>    @@ -215,6 +244,8 @@ static void test_write_page(void)
>>>        for (i = 0; i < PAGE_SIZE / 4; i++) {
>>>            g_assert_cmphex(page[i], ==, 0xffffffff);
>>>        }
>>> +
>>> +    flash_reset();
>>>    }
>>>      static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>

[Qemu-devel] [PATCH v2 09/11] aspeed/smc: extend tests for Command mode

[Cédric Le Goater]

The Aspeed SMC controllers have a mode (Command mode) in which
accesses to the flash content are no different than doing MMIOs. The
controller generates all the necessary commands to load (or store)
data in memory.

So add a couple of tests doing direct reads and writes on the AHB bus.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 tests/m25p80-test.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

 Changes since v1:

 - added preliminary configuration of the controller and the flash
   before starting the test. 
 - added a flash reset 

diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
index 8dd550deb95e..244aa33dd941 100644
--- a/tests/m25p80-test.c
+++ b/tests/m25p80-test.c
@@ -36,6 +36,9 @@
 #define   CRTL_EXTENDED0       0  /* 32 bit addressing for SPI */
 #define R_CTRL0             0x10
 #define   CTRL_CE_STOP_ACTIVE  (1 << 2)
+#define   CTRL_READMODE        0x0
+#define   CTRL_FREADMODE       0x1
+#define   CTRL_WRITEMODE       0x2
 #define   CTRL_USERMODE        0x3
 
 #define ASPEED_FMC_BASE    0x1E620000
@@ -86,6 +89,22 @@ static void spi_conf_remove(uint32_t value)
     writel(ASPEED_FMC_BASE + R_CONF, conf);
 }
 
+static void spi_ce_ctrl(uint32_t value)
+{
+    uint32_t conf = readl(ASPEED_FMC_BASE + R_CE_CTRL);
+
+    conf |= value;
+    writel(ASPEED_FMC_BASE + R_CE_CTRL, conf);
+}
+
+static void spi_ctrl_setmode(uint8_t mode, uint8_t cmd)
+{
+    uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
+    ctrl &= ~(CTRL_USERMODE | 0xff << 16);
+    ctrl |= mode | (cmd << 16);
+    writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
+}
+
 static void spi_ctrl_start_user(void)
 {
     uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
@@ -152,6 +171,18 @@ static void read_page(uint32_t addr, uint32_t *page)
     spi_ctrl_stop_user();
 }
 
+static void read_page_mem(uint32_t addr, uint32_t *page)
+{
+    int i;
+
+    /* move out USER mode to use direct reads from the AHB bus */
+    spi_ctrl_setmode(CTRL_READMODE, READ);
+
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        page[i] = make_be32(readl(ASPEED_FLASH_BASE + addr + i * 4));
+    }
+}
+
 static void test_erase_sector(void)
 {
     uint32_t some_page_addr = 0x600 * PAGE_SIZE;
@@ -248,6 +279,75 @@ static void test_write_page(void)
     flash_reset();
 }
 
+static void test_read_page_mem(void)
+{
+    uint32_t my_page_addr = 0x14000 * PAGE_SIZE; /* beyond 16MB */
+    uint32_t some_page_addr = 0x15000 * PAGE_SIZE;
+    uint32_t page[PAGE_SIZE / 4];
+    int i;
+
+    /* Enable 4BYTE mode for controller. This is should be strapped by
+     * HW for CE0 anyhow.
+     */
+    spi_ce_ctrl(1 << CRTL_EXTENDED0);
+
+    /* Enable 4BYTE mode for flash. */
+    spi_conf(CONF_ENABLE_W0);
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
+    spi_ctrl_stop_user();
+    spi_conf_remove(CONF_ENABLE_W0);
+
+    /* Check what was written */
+    read_page_mem(my_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
+    }
+
+    /* Check some other page. It should be full of 0xff */
+    read_page_mem(some_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, 0xffffffff);
+    }
+
+    flash_reset();
+}
+
+static void test_write_page_mem(void)
+{
+    uint32_t my_page_addr = 0x15000 * PAGE_SIZE;
+    uint32_t page[PAGE_SIZE / 4];
+    int i;
+
+    /* Enable 4BYTE mode for controller. This is should be strapped by
+     * HW for CE0 anyhow.
+     */
+    spi_ce_ctrl(1 << CRTL_EXTENDED0);
+
+    /* Enable 4BYTE mode for flash. */
+    spi_conf(CONF_ENABLE_W0);
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
+    writeb(ASPEED_FLASH_BASE, WREN);
+    spi_ctrl_stop_user();
+
+    /* move out USER mode to use direct writes to the AHB bus */
+    spi_ctrl_setmode(CTRL_WRITEMODE, PP);
+
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        writel(ASPEED_FLASH_BASE + my_page_addr + i * 4,
+               make_be32(my_page_addr + i * 4));
+    }
+
+    /* Check what was written */
+    read_page_mem(my_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
+    }
+
+    flash_reset();
+}
+
 static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
 
 int main(int argc, char **argv)
@@ -273,6 +373,8 @@ int main(int argc, char **argv)
     qtest_add_func("/m25p80/erase_sector", test_erase_sector);
     qtest_add_func("/m25p80/erase_all",  test_erase_all);
     qtest_add_func("/m25p80/write_page", test_write_page);
+    qtest_add_func("/m25p80/read_page_mem", test_read_page_mem);
+    qtest_add_func("/m25p80/write_page_mem", test_write_page_mem);
 
     ret = g_test_run();
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 09/11] aspeed/smc: extend tests for Command mode

[mar.krzeminski]

W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
> The Aspeed SMC controllers have a mode (Command mode) in which
> accesses to the flash content are no different than doing MMIOs. The
> controller generates all the necessary commands to load (or store)
> data in memory.
>
> So add a couple of tests doing direct reads and writes on the AHB bus.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   tests/m25p80-test.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 102 insertions(+)
>
>   Changes since v1:
>
>   - added preliminary configuration of the controller and the flash
>     before starting the test.
>   - added a flash reset
>
> diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
> index 8dd550deb95e..244aa33dd941 100644
> --- a/tests/m25p80-test.c
> +++ b/tests/m25p80-test.c
> @@ -36,6 +36,9 @@
>   #define   CRTL_EXTENDED0       0  /* 32 bit addressing for SPI */
>   #define R_CTRL0             0x10
>   #define   CTRL_CE_STOP_ACTIVE  (1 << 2)
> +#define   CTRL_READMODE        0x0
> +#define   CTRL_FREADMODE       0x1
> +#define   CTRL_WRITEMODE       0x2
>   #define   CTRL_USERMODE        0x3
>   
>   #define ASPEED_FMC_BASE    0x1E620000
> @@ -86,6 +89,22 @@ static void spi_conf_remove(uint32_t value)
>       writel(ASPEED_FMC_BASE + R_CONF, conf);
>   }
>   
> +static void spi_ce_ctrl(uint32_t value)
> +{
> +    uint32_t conf = readl(ASPEED_FMC_BASE + R_CE_CTRL);
> +
> +    conf |= value;
> +    writel(ASPEED_FMC_BASE + R_CE_CTRL, conf);
> +}
> +
> +static void spi_ctrl_setmode(uint8_t mode, uint8_t cmd)
> +{
> +    uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
> +    ctrl &= ~(CTRL_USERMODE | 0xff << 16);
> +    ctrl |= mode | (cmd << 16);
> +    writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
> +}
> +
>   static void spi_ctrl_start_user(void)
>   {
>       uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
> @@ -152,6 +171,18 @@ static void read_page(uint32_t addr, uint32_t *page)
>       spi_ctrl_stop_user();
>   }
>   
> +static void read_page_mem(uint32_t addr, uint32_t *page)
> +{
> +    int i;
> +
> +    /* move out USER mode to use direct reads from the AHB bus */
> +    spi_ctrl_setmode(CTRL_READMODE, READ);
> +
> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
> +        page[i] = make_be32(readl(ASPEED_FLASH_BASE + addr + i * 4));
Wouldn't be better to make i < PAGE_SIZE and get rid of multiplications?
Please check other similar loops in the code.
> +    }
> +}
> +
>   static void test_erase_sector(void)
>   {
>       uint32_t some_page_addr = 0x600 * PAGE_SIZE;
> @@ -248,6 +279,75 @@ static void test_write_page(void)
>       flash_reset();
>   }
>   
> +static void test_read_page_mem(void)
> +{
This function repeats write_page tests. Maybe would be better to have 
one function,
do read - epected 0xFF, write then read - the same data and read other 
sector in one function?
> +    uint32_t my_page_addr = 0x14000 * PAGE_SIZE; /* beyond 16MB */
> +    uint32_t some_page_addr = 0x15000 * PAGE_SIZE;
> +    uint32_t page[PAGE_SIZE / 4];
> +    int i;
> +
> +    /* Enable 4BYTE mode for controller. This is should be strapped by
> +     * HW for CE0 anyhow.
> +     */
> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
> +
> +    /* Enable 4BYTE mode for flash. */
> +    spi_conf(CONF_ENABLE_W0);
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
> +    spi_ctrl_stop_user();
> +    spi_conf_remove(CONF_ENABLE_W0);
> +
> +    /* Check what was written */
> +    read_page_mem(my_page_addr, page);
> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
> +        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
> +    }
> +
> +    /* Check some other page. It should be full of 0xff */
> +    read_page_mem(some_page_addr, page);
> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
> +        g_assert_cmphex(page[i], ==, 0xffffffff);
> +    }
> +
> +    flash_reset();
> +}
> +
> +static void test_write_page_mem(void)
> +{
> +    uint32_t my_page_addr = 0x15000 * PAGE_SIZE;
> +    uint32_t page[PAGE_SIZE / 4];
> +    int i;
> +
> +    /* Enable 4BYTE mode for controller. This is should be strapped by
> +     * HW for CE0 anyhow.
> +     */
> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
> +
> +    /* Enable 4BYTE mode for flash. */
> +    spi_conf(CONF_ENABLE_W0);
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
> +    writeb(ASPEED_FLASH_BASE, WREN);
This is a bit tricky, in real HW you need to set WREN before issue 
PROGRAM command on most
of the devices. If there is no cache in emulated in SMC controller (if 
any in HW), WREN should be
issued before every write.

Thanks,
Marcin
> +    spi_ctrl_stop_user();
> +
> +    /* move out USER mode to use direct writes to the AHB bus */
> +    spi_ctrl_setmode(CTRL_WRITEMODE, PP);
> +
> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
> +        writel(ASPEED_FLASH_BASE + my_page_addr + i * 4,
> +               make_be32(my_page_addr + i * 4));
> +    }
> +
> +    /* Check what was written */
> +    read_page_mem(my_page_addr, page);
> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
> +        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
> +    }
> +
> +    flash_reset();
> +}
> +
>   static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>   
>   int main(int argc, char **argv)
> @@ -273,6 +373,8 @@ int main(int argc, char **argv)
>       qtest_add_func("/m25p80/erase_sector", test_erase_sector);
>       qtest_add_func("/m25p80/erase_all",  test_erase_all);
>       qtest_add_func("/m25p80/write_page", test_write_page);
> +    qtest_add_func("/m25p80/read_page_mem", test_read_page_mem);
> +    qtest_add_func("/m25p80/write_page_mem", test_write_page_mem);
>   
>       ret = g_test_run();
>   

Re: [Qemu-devel] [PATCH v2 09/11] aspeed/smc: extend tests for Command mode

[Cédric Le Goater]

On 01/16/2017 06:51 PM, mar.krzeminski wrote:
> 
> 
> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>> The Aspeed SMC controllers have a mode (Command mode) in which
>> accesses to the flash content are no different than doing MMIOs. The
>> controller generates all the necessary commands to load (or store)
>> data in memory.
>>
>> So add a couple of tests doing direct reads and writes on the AHB bus.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>>   tests/m25p80-test.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 102 insertions(+)
>>
>>   Changes since v1:
>>
>>   - added preliminary configuration of the controller and the flash
>>     before starting the test.
>>   - added a flash reset
>>
>> diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
>> index 8dd550deb95e..244aa33dd941 100644
>> --- a/tests/m25p80-test.c
>> +++ b/tests/m25p80-test.c
>> @@ -36,6 +36,9 @@
>>   #define   CRTL_EXTENDED0       0  /* 32 bit addressing for SPI */
>>   #define R_CTRL0             0x10
>>   #define   CTRL_CE_STOP_ACTIVE  (1 << 2)
>> +#define   CTRL_READMODE        0x0
>> +#define   CTRL_FREADMODE       0x1
>> +#define   CTRL_WRITEMODE       0x2
>>   #define   CTRL_USERMODE        0x3
>>     #define ASPEED_FMC_BASE    0x1E620000
>> @@ -86,6 +89,22 @@ static void spi_conf_remove(uint32_t value)
>>       writel(ASPEED_FMC_BASE + R_CONF, conf);
>>   }
>>   +static void spi_ce_ctrl(uint32_t value)
>> +{
>> +    uint32_t conf = readl(ASPEED_FMC_BASE + R_CE_CTRL);
>> +
>> +    conf |= value;
>> +    writel(ASPEED_FMC_BASE + R_CE_CTRL, conf);
>> +}
>> +
>> +static void spi_ctrl_setmode(uint8_t mode, uint8_t cmd)
>> +{
>> +    uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
>> +    ctrl &= ~(CTRL_USERMODE | 0xff << 16);
>> +    ctrl |= mode | (cmd << 16);
>> +    writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
>> +}
>> +
>>   static void spi_ctrl_start_user(void)
>>   {
>>       uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
>> @@ -152,6 +171,18 @@ static void read_page(uint32_t addr, uint32_t *page)
>>       spi_ctrl_stop_user();
>>   }
>>   +static void read_page_mem(uint32_t addr, uint32_t *page)
>> +{
>> +    int i;
>> +
>> +    /* move out USER mode to use direct reads from the AHB bus */
>> +    spi_ctrl_setmode(CTRL_READMODE, READ);
>> +
>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>> +        page[i] = make_be32(readl(ASPEED_FLASH_BASE + addr + i * 4));
> Wouldn't be better to make i < PAGE_SIZE and get rid of multiplications?

Hmm, we need the addresses to be 32bits aligned. Am I missing something ?

> Please check other similar loops in the code.
>> +    }
>> +}
>> +
>>   static void test_erase_sector(void)
>>   {
>>       uint32_t some_page_addr = 0x600 * PAGE_SIZE;
>> @@ -248,6 +279,75 @@ static void test_write_page(void)
>>       flash_reset();
>>   }
>>   +static void test_read_page_mem(void)
>> +{
> This function repeats write_page tests. 

It is true that this test depends on the previous test 
'test_write_page()'. The test checks the flash contents 
using the command mode. 

Resetting the flash contents to an initial state did not 
seem necessary but I should probably comment on the test 
case  dependencies though.

> Maybe would be better to have one function, do read - epected 0xFF, 
> write then read - the same data and read other sector in one function?

OK. I think I understand. So I should provide some toolbox
with highlevel functions for each test to use in order to 
clarify what is being done. 

I will try that in a followup patch.

>> +    uint32_t my_page_addr = 0x14000 * PAGE_SIZE; /* beyond 16MB */
>> +    uint32_t some_page_addr = 0x15000 * PAGE_SIZE;
>> +    uint32_t page[PAGE_SIZE / 4];
>> +    int i;
>> +
>> +    /* Enable 4BYTE mode for controller. This is should be strapped by
>> +     * HW for CE0 anyhow.
>> +     */
>> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
>> +
>> +    /* Enable 4BYTE mode for flash. */
>> +    spi_conf(CONF_ENABLE_W0);
>> +    spi_ctrl_start_user();
>> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>> +    spi_ctrl_stop_user();
>> +    spi_conf_remove(CONF_ENABLE_W0);
>> +
>> +    /* Check what was written */
>> +    read_page_mem(my_page_addr, page);
>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>> +        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
>> +    }
>> +
>> +    /* Check some other page. It should be full of 0xff */
>> +    read_page_mem(some_page_addr, page);
>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>> +        g_assert_cmphex(page[i], ==, 0xffffffff);
>> +    }
>> +
>> +    flash_reset();
>> +}
>> +
>> +static void test_write_page_mem(void)
>> +{
>> +    uint32_t my_page_addr = 0x15000 * PAGE_SIZE;
>> +    uint32_t page[PAGE_SIZE / 4];
>> +    int i;
>> +
>> +    /* Enable 4BYTE mode for controller. This is should be strapped by
>> +     * HW for CE0 anyhow.
>> +     */
>> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
>> +
>> +    /* Enable 4BYTE mode for flash. */
>> +    spi_conf(CONF_ENABLE_W0);
>> +    spi_ctrl_start_user();
>> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>> +    writeb(ASPEED_FLASH_BASE, WREN);
>
> This is a bit tricky, in real HW you need to set WREN 
> before issue PROGRAM command on most of the devices. 
> If there is no cache in emulated in SMC controller 
> (if any in HW), WREN should be issued before every write.

ah yes. So to be more precise, WREN should be moved in the
loop below. 

Thanks,

C.

> 
> Thanks,
> Marcin
>> +    spi_ctrl_stop_user();
>> +
>> +    /* move out USER mode to use direct writes to the AHB bus */
>> +    spi_ctrl_setmode(CTRL_WRITEMODE, PP);
>> +
>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>> +        writel(ASPEED_FLASH_BASE + my_page_addr + i * 4,
>> +               make_be32(my_page_addr + i * 4));
>> +    }
>> +
>> +    /* Check what was written */
>> +    read_page_mem(my_page_addr, page);
>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>> +        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
>> +    }
>> +
>> +    flash_reset();
>> +}
>> +
>>   static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>>     int main(int argc, char **argv)
>> @@ -273,6 +373,8 @@ int main(int argc, char **argv)
>>       qtest_add_func("/m25p80/erase_sector", test_erase_sector);
>>       qtest_add_func("/m25p80/erase_all",  test_erase_all);
>>       qtest_add_func("/m25p80/write_page", test_write_page);
>> +    qtest_add_func("/m25p80/read_page_mem", test_read_page_mem);
>> +    qtest_add_func("/m25p80/write_page_mem", test_write_page_mem);
>>         ret = g_test_run();
>>   
> 

Re: [Qemu-devel] [PATCH v2 09/11] aspeed/smc: extend tests for Command mode

[mar.krzeminski]

W dniu 17.01.2017 o 09:34, Cédric Le Goater pisze:
> On 01/16/2017 06:51 PM, mar.krzeminski wrote:
>>
>> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>>> The Aspeed SMC controllers have a mode (Command mode) in which
>>> accesses to the flash content are no different than doing MMIOs. The
>>> controller generates all the necessary commands to load (or store)
>>> data in memory.
>>>
>>> So add a couple of tests doing direct reads and writes on the AHB bus.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>>    tests/m25p80-test.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 102 insertions(+)
>>>
>>>    Changes since v1:
>>>
>>>    - added preliminary configuration of the controller and the flash
>>>      before starting the test.
>>>    - added a flash reset
>>>
>>> diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
>>> index 8dd550deb95e..244aa33dd941 100644
>>> --- a/tests/m25p80-test.c
>>> +++ b/tests/m25p80-test.c
>>> @@ -36,6 +36,9 @@
>>>    #define   CRTL_EXTENDED0       0  /* 32 bit addressing for SPI */
>>>    #define R_CTRL0             0x10
>>>    #define   CTRL_CE_STOP_ACTIVE  (1 << 2)
>>> +#define   CTRL_READMODE        0x0
>>> +#define   CTRL_FREADMODE       0x1
>>> +#define   CTRL_WRITEMODE       0x2
>>>    #define   CTRL_USERMODE        0x3
>>>      #define ASPEED_FMC_BASE    0x1E620000
>>> @@ -86,6 +89,22 @@ static void spi_conf_remove(uint32_t value)
>>>        writel(ASPEED_FMC_BASE + R_CONF, conf);
>>>    }
>>>    +static void spi_ce_ctrl(uint32_t value)
>>> +{
>>> +    uint32_t conf = readl(ASPEED_FMC_BASE + R_CE_CTRL);
>>> +
>>> +    conf |= value;
>>> +    writel(ASPEED_FMC_BASE + R_CE_CTRL, conf);
>>> +}
>>> +
>>> +static void spi_ctrl_setmode(uint8_t mode, uint8_t cmd)
>>> +{
>>> +    uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
>>> +    ctrl &= ~(CTRL_USERMODE | 0xff << 16);
>>> +    ctrl |= mode | (cmd << 16);
>>> +    writel(ASPEED_FMC_BASE + R_CTRL0, ctrl);
>>> +}
>>> +
>>>    static void spi_ctrl_start_user(void)
>>>    {
>>>        uint32_t ctrl = readl(ASPEED_FMC_BASE + R_CTRL0);
>>> @@ -152,6 +171,18 @@ static void read_page(uint32_t addr, uint32_t *page)
>>>        spi_ctrl_stop_user();
>>>    }
>>>    +static void read_page_mem(uint32_t addr, uint32_t *page)
>>> +{
>>> +    int i;
>>> +
>>> +    /* move out USER mode to use direct reads from the AHB bus */
>>> +    spi_ctrl_setmode(CTRL_READMODE, READ);
>>> +
>>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>>> +        page[i] = make_be32(readl(ASPEED_FLASH_BASE + addr + i * 4));
>> Wouldn't be better to make i < PAGE_SIZE and get rid of multiplications?
> Hmm, we need the addresses to be 32bits aligned. Am I missing something ?
Yes, sorry I missed page[i] in the loop.
>
>> Please check other similar loops in the code.
>>> +    }
>>> +}
>>> +
>>>    static void test_erase_sector(void)
>>>    {
>>>        uint32_t some_page_addr = 0x600 * PAGE_SIZE;
>>> @@ -248,6 +279,75 @@ static void test_write_page(void)
>>>        flash_reset();
>>>    }
>>>    +static void test_read_page_mem(void)
>>> +{
>> This function repeats write_page tests.
> It is true that this test depends on the previous test
> 'test_write_page()'. The test checks the flash contents
> using the command mode.
>
> Resetting the flash contents to an initial state did not
> seem necessary but I should probably comment on the test
> case  dependencies though.
>
>> Maybe would be better to have one function, do read - epected 0xFF,
>> write then read - the same data and read other sector in one function?
> OK. I think I understand. So I should provide some toolbox
> with highlevel functions for each test to use in order to
> clarify what is being done.
>
> I will try that in a followup patch.
This is just suggestion, one function that do read/write is clearer than 
two that do
some part twice.
>
>>> +    uint32_t my_page_addr = 0x14000 * PAGE_SIZE; /* beyond 16MB */
>>> +    uint32_t some_page_addr = 0x15000 * PAGE_SIZE;
>>> +    uint32_t page[PAGE_SIZE / 4];
>>> +    int i;
>>> +
>>> +    /* Enable 4BYTE mode for controller. This is should be strapped by
>>> +     * HW for CE0 anyhow.
>>> +     */
>>> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
>>> +
>>> +    /* Enable 4BYTE mode for flash. */
>>> +    spi_conf(CONF_ENABLE_W0);
>>> +    spi_ctrl_start_user();
>>> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>>> +    spi_ctrl_stop_user();
>>> +    spi_conf_remove(CONF_ENABLE_W0);
>>> +
>>> +    /* Check what was written */
>>> +    read_page_mem(my_page_addr, page);
>>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>>> +        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
>>> +    }
>>> +
>>> +    /* Check some other page. It should be full of 0xff */
>>> +    read_page_mem(some_page_addr, page);
>>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>>> +        g_assert_cmphex(page[i], ==, 0xffffffff);
>>> +    }
>>> +
>>> +    flash_reset();
>>> +}
>>> +
>>> +static void test_write_page_mem(void)
>>> +{
>>> +    uint32_t my_page_addr = 0x15000 * PAGE_SIZE;
>>> +    uint32_t page[PAGE_SIZE / 4];
>>> +    int i;
>>> +
>>> +    /* Enable 4BYTE mode for controller. This is should be strapped by
>>> +     * HW for CE0 anyhow.
>>> +     */
>>> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
>>> +
>>> +    /* Enable 4BYTE mode for flash. */
>>> +    spi_conf(CONF_ENABLE_W0);
>>> +    spi_ctrl_start_user();
>>> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>>> +    writeb(ASPEED_FLASH_BASE, WREN);
>> This is a bit tricky, in real HW you need to set WREN
>> before issue PROGRAM command on most of the devices.
>> If there is no cache in emulated in SMC controller
>> (if any in HW), WREN should be issued before every write.
> ah yes. So to be more precise, WREN should be moved in the
> loop below.
Yes.

Thanks,
Marcin
> Thanks,
>
> C.
>
>> Thanks,
>> Marcin
>>> +    spi_ctrl_stop_user();
>>> +
>>> +    /* move out USER mode to use direct writes to the AHB bus */
>>> +    spi_ctrl_setmode(CTRL_WRITEMODE, PP);
>>> +
>>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>>> +        writel(ASPEED_FLASH_BASE + my_page_addr + i * 4,
>>> +               make_be32(my_page_addr + i * 4));
>>> +    }
>>> +
>>> +    /* Check what was written */
>>> +    read_page_mem(my_page_addr, page);
>>> +    for (i = 0; i < PAGE_SIZE / 4; i++) {
>>> +        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
>>> +    }
>>> +
>>> +    flash_reset();
>>> +}
>>> +
>>>    static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>>>      int main(int argc, char **argv)
>>> @@ -273,6 +373,8 @@ int main(int argc, char **argv)
>>>        qtest_add_func("/m25p80/erase_sector", test_erase_sector);
>>>        qtest_add_func("/m25p80/erase_all",  test_erase_all);
>>>        qtest_add_func("/m25p80/write_page", test_write_page);
>>> +    qtest_add_func("/m25p80/read_page_mem", test_read_page_mem);
>>> +    qtest_add_func("/m25p80/write_page_mem", test_write_page_mem);
>>>          ret = g_test_run();
>>>    
>

Re: [Qemu-devel] [PATCH v2 09/11] aspeed/smc: extend tests for Command mode

[Cédric Le Goater]

On 01/17/2017 06:34 PM, mar.krzeminski wrote:
>>>> +static void test_write_page_mem(void)
>>>> +{
>>>> +    uint32_t my_page_addr = 0x15000 * PAGE_SIZE;
>>>> +    uint32_t page[PAGE_SIZE / 4];
>>>> +    int i;
>>>> +
>>>> +    /* Enable 4BYTE mode for controller. This is should be strapped by
>>>> +     * HW for CE0 anyhow.
>>>> +     */
>>>> +    spi_ce_ctrl(1 << CRTL_EXTENDED0);
>>>> +
>>>> +    /* Enable 4BYTE mode for flash. */
>>>> +    spi_conf(CONF_ENABLE_W0);
>>>> +    spi_ctrl_start_user();
>>>> +    writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
>>>> +    writeb(ASPEED_FLASH_BASE, WREN);
>>> This is a bit tricky, in real HW you need to set WREN
>>> before issue PROGRAM command on most of the devices.
>>> If there is no cache in emulated in SMC controller
>>> (if any in HW), WREN should be issued before every write.
>> ah yes. So to be more precise, WREN should be moved in the
>> loop below.
> Yes.

So, as the test is writing one page only, I think the code is 
currently correct. 

I will come up with your suggested cleanups and more tests 
later on, may be in the 2.9 time frame. 

Thanks for the review, 

C.

[Qemu-devel] [PATCH v2 10/11] aspeed: use first FMC flash as a boot ROM

[Cédric Le Goater]

Create a ROM region, using the default size of the mapping window for
the CE0 FMC flash module, and fill it with the flash content.

This is a little hacky but until we can boot from a MMIO region, it
seems difficult to do anything else.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 40c13838fb2d..a92c2f1c362b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -20,6 +20,8 @@
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
+#include "hw/loader.h"
+#include "qemu/error-report.h"
 
 static struct arm_boot_info aspeed_board_binfo = {
     .board_id = -1, /* device-tree-only board */
@@ -104,6 +106,28 @@ static const AspeedBoardConfig aspeed_boards[] = {
     },
 };
 
+#define FIRMWARE_ADDR 0x0
+
+static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
+                           Error **errp)
+{
+    BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
+    uint8_t *storage;
+
+    if (rom_size > blk_getlength(blk)) {
+        rom_size = blk_getlength(blk);
+    }
+
+    storage = g_new0(uint8_t, rom_size);
+    if (blk_pread(blk, 0, storage, rom_size) < 0) {
+        error_setg(errp, "failed to read the initial flash content");
+        return;
+    }
+
+    rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr);
+    g_free(storage);
+}
+
 static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
                                       Error **errp)
 {
@@ -135,6 +159,7 @@ static void aspeed_board_init(MachineState *machine,
 {
     AspeedBoardState *bmc;
     AspeedSoCClass *sc;
+    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
 
     bmc = g_new0(AspeedBoardState, 1);
     object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
@@ -168,6 +193,22 @@ static void aspeed_board_init(MachineState *machine,
     aspeed_board_init_flashes(&bmc->soc.fmc, cfg->fmc_model, &error_abort);
     aspeed_board_init_flashes(&bmc->soc.spi[0], cfg->spi_model, &error_abort);
 
+    /* Install first FMC flash content as a boot rom. */
+    if (drive0) {
+        AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
+        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
+
+        /*
+         * create a ROM region using the default mapping window size of
+         * the flash module.
+         */
+        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
+                               fl->size, &error_abort);
+        memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
+                                    boot_rom);
+        write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
+    }
+
     aspeed_board_binfo.kernel_filename = machine->kernel_filename;
     aspeed_board_binfo.initrd_filename = machine->initrd_filename;
     aspeed_board_binfo.kernel_cmdline = machine->kernel_cmdline;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 10/11] aspeed: use first FMC flash as a boot ROM

[Peter Maydell]

On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
> Create a ROM region, using the default size of the mapping window for
> the CE0 FMC flash module, and fill it with the flash content.
>
> This is a little hacky but until we can boot from a MMIO region, it
> seems difficult to do anything else.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/arm/aspeed.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 40c13838fb2d..a92c2f1c362b 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -20,6 +20,8 @@
>  #include "qemu/log.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
> +#include "hw/loader.h"
> +#include "qemu/error-report.h"
>
>  static struct arm_boot_info aspeed_board_binfo = {
>      .board_id = -1, /* device-tree-only board */
> @@ -104,6 +106,28 @@ static const AspeedBoardConfig aspeed_boards[] = {
>      },
>  };
>
> +#define FIRMWARE_ADDR 0x0
> +
> +static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
> +                           Error **errp)
> +{
> +    BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> +    uint8_t *storage;
> +
> +    if (rom_size > blk_getlength(blk)) {
> +        rom_size = blk_getlength(blk);
> +    }
> +
> +    storage = g_new0(uint8_t, rom_size);

Hi -- coverity points out that you're not checking for the
case where blk_getlength() returns negative. (This can happen
for the case where the BlockBackend represents an insertable
device like a floppy or cdrom with no medium present.)

Since for aspeed this dinfo always represents a flash device,
we know this shouldn't happen, so you can just make it a fatal
error if blk_getlength() doesn't return what you're expecting.

> +    if (blk_pread(blk, 0, storage, rom_size) < 0) {
> +        error_setg(errp, "failed to read the initial flash content");
> +        return;
> +    }
> +
> +    rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr);
> +    g_free(storage);
> +}

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 10/11] aspeed: use first FMC flash as a boot ROM

[Cédric Le Goater]

>> +#define FIRMWARE_ADDR 0x0
>> +
>> +static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
>> +                           Error **errp)
>> +{
>> +    BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> +    uint8_t *storage;
>> +
>> +    if (rom_size > blk_getlength(blk)) {
>> +        rom_size = blk_getlength(blk);
>> +    }
>> +
>> +    storage = g_new0(uint8_t, rom_size);
> 
> Hi -- coverity points out that you're not checking for the
> case where blk_getlength() returns negative. (This can happen
> for the case where the BlockBackend represents an insertable
> device like a floppy or cdrom with no medium present.)
> 
> Since for aspeed this dinfo always represents a flash device,
> we know this shouldn't happen, so you can just make it a fatal
> error if blk_getlength() doesn't return what you're expecting.

OK. I will send a fix to improve this routine. 

Thanks,

C.

[Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[Cédric Le Goater]

When doing fast read, a certain amount of dummy bytes should be sent
before the read. This number is configurable in the controler CE0
Control Register and needs to be modeled using fake transfers the
flash module.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

 Changes since v1:

 - splitted user mode from command mode support.

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 28d563a5800f..7a76d3344df2 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -69,7 +69,9 @@
 #define R_CTRL0           (0x10 / 4)
 #define   CTRL_CMD_SHIFT           16
 #define   CTRL_CMD_MASK            0xff
+#define   CTRL_DUMMY_HIGH_SHIFT    14
 #define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
+#define   CTRL_DUMMY_LOW_SHIFT     6 /* 2 bits [7:6] */
 #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
 #define   CTRL_CMD_MODE_MASK       0x3
 #define     CTRL_READMODE          0x0
@@ -141,6 +143,7 @@
 
 /* Flash opcodes. */
 #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
+#define SPI_OP_READ_FAST  0x0b    /* Read data bytes (high frequency) */
 
 /*
  * Default segments mapping addresses and size for each slave per
@@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
     return addr;
 }
 
+static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
+{
+    const AspeedSMCState *s = fl->controller;
+    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
+    uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
+    uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
+
+    return ((dummy_high << 2) | dummy_low) * 8;
+}
+
 static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
 {
     const AspeedSMCState *s = fl->controller;
     uint8_t cmd = aspeed_smc_flash_cmd(fl);
+    int i;
 
     /* Flash access can not exceed CS segment */
     addr = aspeed_smc_check_segment_addr(fl, addr);
@@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
     ssi_transfer(s->spi, (addr >> 16) & 0xff);
     ssi_transfer(s->spi, (addr >> 8) & 0xff);
     ssi_transfer(s->spi, (addr & 0xff));
+
+    /* Handle dummies in case of fast read */
+    if (cmd == SPI_OP_READ_FAST) {
+        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
+            ssi_transfer(fl->controller->spi, 0xFF);
+        }
+    }
 }
 
 static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[mar.krzeminski]

W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
> When doing fast read, a certain amount of dummy bytes should be sent
> before the read. This number is configurable in the controler CE0
> Control Register and needs to be modeled using fake transfers the
> flash module.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
>   Changes since v1:
>
>   - splitted user mode from command mode support.
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 28d563a5800f..7a76d3344df2 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -69,7 +69,9 @@
>   #define R_CTRL0           (0x10 / 4)
>   #define   CTRL_CMD_SHIFT           16
>   #define   CTRL_CMD_MASK            0xff
> +#define   CTRL_DUMMY_HIGH_SHIFT    14
>   #define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
> +#define   CTRL_DUMMY_LOW_SHIFT     6 /* 2 bits [7:6] */
>   #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
>   #define   CTRL_CMD_MODE_MASK       0x3
>   #define     CTRL_READMODE          0x0
> @@ -141,6 +143,7 @@
>   
>   /* Flash opcodes. */
>   #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
> +#define SPI_OP_READ_FAST  0x0b    /* Read data bytes (high frequency) */
>   
>   /*
>    * Default segments mapping addresses and size for each slave per
> @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
>       return addr;
>   }
>   
> +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
> +{
> +    const AspeedSMCState *s = fl->controller;
> +    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
> +    uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
> +    uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
> +
> +    return ((dummy_high << 2) | dummy_low) * 8;
> +}
> +
>   static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>   {
>       const AspeedSMCState *s = fl->controller;
>       uint8_t cmd = aspeed_smc_flash_cmd(fl);
> +    int i;
>   
>       /* Flash access can not exceed CS segment */
>       addr = aspeed_smc_check_segment_addr(fl, addr);
> @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>       ssi_transfer(s->spi, (addr >> 16) & 0xff);
>       ssi_transfer(s->spi, (addr >> 8) & 0xff);
>       ssi_transfer(s->spi, (addr & 0xff));
> +
> +    /* Handle dummies in case of fast read */
> +    if (cmd == SPI_OP_READ_FAST) {
I feel this is wrong. It indicates that the controller knows FAST_READ 
op code.
I believe controller always try to send dummy cycles, and do not do this 
when
their count is set to 0 (so you do not need above if).

Thanks,
Marcin
> +        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
> +            ssi_transfer(fl->controller->spi, 0xFF);
> +        }
> +    }
>   }
>   
>   static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[Cédric Le Goater]

On 01/11/2017 07:20 PM, mar.krzeminski wrote:
> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>> When doing fast read, a certain amount of dummy bytes should be sent
>> before the read. This number is configurable in the controler CE0
>> Control Register and needs to be modeled using fake transfers the
>> flash module.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>>  hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>>  Changes since v1:
>>
>>  - splitted user mode from command mode support.
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 28d563a5800f..7a76d3344df2 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -69,7 +69,9 @@
>>  #define R_CTRL0           (0x10 / 4)
>>  #define   CTRL_CMD_SHIFT           16
>>  #define   CTRL_CMD_MASK            0xff
>> +#define   CTRL_DUMMY_HIGH_SHIFT    14
>>  #define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
>> +#define   CTRL_DUMMY_LOW_SHIFT     6 /* 2 bits [7:6] */
>>  #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
>>  #define   CTRL_CMD_MODE_MASK       0x3
>>  #define     CTRL_READMODE          0x0
>> @@ -141,6 +143,7 @@
>>  
>>  /* Flash opcodes. */
>>  #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
>> +#define SPI_OP_READ_FAST  0x0b    /* Read data bytes (high frequency) */
>>  
>>  /*
>>   * Default segments mapping addresses and size for each slave per
>> @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
>>      return addr;
>>  }
>>  
>> +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
>> +{
>> +    const AspeedSMCState *s = fl->controller;
>> +    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
>> +    uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
>> +    uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
>> +
>> +    return ((dummy_high << 2) | dummy_low) * 8;
>> +}
>> +
>>  static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>>  {
>>      const AspeedSMCState *s = fl->controller;
>>      uint8_t cmd = aspeed_smc_flash_cmd(fl);
>> +    int i;
>>  
>>      /* Flash access can not exceed CS segment */
>>      addr = aspeed_smc_check_segment_addr(fl, addr);
>> @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>>      ssi_transfer(s->spi, (addr >> 16) & 0xff);
>>      ssi_transfer(s->spi, (addr >> 8) & 0xff);
>>      ssi_transfer(s->spi, (addr & 0xff));
>> +
>> +    /* Handle dummies in case of fast read */
>> +    if (cmd == SPI_OP_READ_FAST) {
> I feel this is wrong. It indicates that the controller knows FAST_READ op code.

you are right. I should not be testing the SPI OP command code 
but the controller mode :

	CTRL_FREADMODE

> I believe controller always try to send dummy cycles, and do not do this when
> their count is set to 0 (so you do not need above if).

Indeed. I will check If I can just drop the test then. 

Thanks,

C. 

> Thanks,
> Marcin
>> +        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
>> +            ssi_transfer(fl->controller->spi, 0xFF);
>> +        }
>> +    }
>>  }
>>  
>>  static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
> 

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[mar.krzeminski]

W dniu 11.01.2017 o 19:55, Cédric Le Goater pisze:
> On 01/11/2017 07:20 PM, mar.krzeminski wrote:
>> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze:
>>> When doing fast read, a certain amount of dummy bytes should be sent
>>> before the read. This number is configurable in the controler CE0
>>> Control Register and needs to be modeled using fake transfers the
>>> flash module.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>>   hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>>   Changes since v1:
>>>
>>>   - splitted user mode from command mode support.
>>>
>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>>> index 28d563a5800f..7a76d3344df2 100644
>>> --- a/hw/ssi/aspeed_smc.c
>>> +++ b/hw/ssi/aspeed_smc.c
>>> @@ -69,7 +69,9 @@
>>>   #define R_CTRL0           (0x10 / 4)
>>>   #define   CTRL_CMD_SHIFT           16
>>>   #define   CTRL_CMD_MASK            0xff
>>> +#define   CTRL_DUMMY_HIGH_SHIFT    14
>>>   #define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
>>> +#define   CTRL_DUMMY_LOW_SHIFT     6 /* 2 bits [7:6] */
>>>   #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
>>>   #define   CTRL_CMD_MODE_MASK       0x3
>>>   #define     CTRL_READMODE          0x0
>>> @@ -141,6 +143,7 @@
>>>   
>>>   /* Flash opcodes. */
>>>   #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
>>> +#define SPI_OP_READ_FAST  0x0b    /* Read data bytes (high frequency) */
>>>   
>>>   /*
>>>    * Default segments mapping addresses and size for each slave per
>>> @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
>>>       return addr;
>>>   }
>>>   
>>> +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
>>> +{
>>> +    const AspeedSMCState *s = fl->controller;
>>> +    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
>>> +    uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
>>> +    uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
>>> +
>>> +    return ((dummy_high << 2) | dummy_low) * 8;
>>> +}
>>> +
>>>   static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>>>   {
>>>       const AspeedSMCState *s = fl->controller;
>>>       uint8_t cmd = aspeed_smc_flash_cmd(fl);
>>> +    int i;
>>>   
>>>       /* Flash access can not exceed CS segment */
>>>       addr = aspeed_smc_check_segment_addr(fl, addr);
>>> @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>>>       ssi_transfer(s->spi, (addr >> 16) & 0xff);
>>>       ssi_transfer(s->spi, (addr >> 8) & 0xff);
>>>       ssi_transfer(s->spi, (addr & 0xff));
>>> +
>>> +    /* Handle dummies in case of fast read */
>>> +    if (cmd == SPI_OP_READ_FAST) {
>> I feel this is wrong. It indicates that the controller knows FAST_READ op code.
> you are right. I should not be testing the SPI OP command code
> but the controller mode :
>
> 	CTRL_FREADMODE
>
>> I believe controller always try to send dummy cycles, and do not do this when
>> their count is set to 0 (so you do not need above if).
> Indeed. I will check If I can just drop the test then.
I did not notice that this function is also called in writes, isn't it?
If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE
needs to be tested.

Thanks,
Marcin
>
> Thanks,
>
> C.
>
>> Thanks,
>> Marcin
>>> +        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
>>> +            ssi_transfer(fl->controller->spi, 0xFF);
>>> +        }
>>> +    }
>>>   }
>>>   
>>>   static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[Cédric Le Goater]

> I did not notice that this function is also called in writes, isn't it?
> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE
> needs to be tested.

yes. I can take care of that in a follow up patchset for 
dummy support.
 
Dummies in user mode is a bit painful to implement, as I had 
to snoop into the command flow to catch the fast read op.
Not sure this is the right approach so I kept it for later.


Did you have time to take look at the other patches adding 
Command mode and extending the tests ? I should have addressed
your comments there.

Thanks,

C.     

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[mar.krzeminski]

W dniu 16.01.2017 o 09:18, Cédric Le Goater pisze:
>> I did not notice that this function is also called in writes, isn't it?
>> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE
>> needs to be tested.
> yes. I can take care of that in a follow up patchset for
> dummy support.
>   
> Dummies in user mode is a bit painful to implement, as I had
> to snoop into the command flow to catch the fast read op.
> Not sure this is the right approach so I kept it for later.
Definitelly wrong, controller should not be aware of tha, fix is still 
on me :(

>
>
> Did you have time to take look at the other patches adding
> Command mode and extending the tests ? I should have addressed
> your comments there.
Yes, there is one more thing that could be important. It popped out in 
Sabrelite
SPI model. The question is does SCM support different CS active (so device
is active at CS high). Your code assume that SMC will always use CS LOW 
to activate
device. If this is not true you might be interested in update this too.

Thanks,
Marcin
>
> Thanks,
>
> C.
>

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[Cédric Le Goater]

On 01/16/2017 07:58 PM, mar.krzeminski wrote:
> 
> 
> W dniu 16.01.2017 o 09:18, Cédric Le Goater pisze:
>>> I did not notice that this function is also called in writes, isn't it?
>>> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE
>>> needs to be tested.
>> yes. I can take care of that in a follow up patchset for
>> dummy support.
>>   Dummies in user mode is a bit painful to implement, as I had
>> to snoop into the command flow to catch the fast read op.
>> Not sure this is the right approach so I kept it for later.
> Definitelly wrong, controller should not be aware of tha, fix is still on me :(

ok. np. I will send the support for command mode though, 
as this is a must have for booting.

>>
>>
>> Did you have time to take look at the other patches adding
>> Command mode and extending the tests ? I should have addressed
>> your comments there.
> Yes, there is one more thing that could be important. It popped out in Sabrelite
> SPI model. The question is does SCM support different CS active (so device
> is active at CS high). Your code assume that SMC will always use CS LOW to activate
> device. If this is not true you might be interested in update this too.

Aspeed SoC does not let you configure the chip select polarity 
(nor the clock phase ) So it is active low only.

Thanks,

C.

Re: [Qemu-devel] [PATCH v2 11/11] aspeed/smc: handle dummy bytes when doing fast reads in command mode

[mar.krzeminski]

W dniu 17.01.2017 o 09:37, Cédric Le Goater pisze:
> On 01/16/2017 07:58 PM, mar.krzeminski wrote:
>>
>> W dniu 16.01.2017 o 09:18, Cédric Le Goater pisze:
>>>> I did not notice that this function is also called in writes, isn't it?
>>>> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE
>>>> needs to be tested.
>>> yes. I can take care of that in a follow up patchset for
>>> dummy support.
>>>    Dummies in user mode is a bit painful to implement, as I had
>>> to snoop into the command flow to catch the fast read op.
>>> Not sure this is the right approach so I kept it for later.
>> Definitelly wrong, controller should not be aware of tha, fix is still on me :(
> ok. np. I will send the support for command mode though,
> as this is a must have for booting.
>
>>>
>>> Did you have time to take look at the other patches adding
>>> Command mode and extending the tests ? I should have addressed
>>> your comments there.
>> Yes, there is one more thing that could be important. It popped out in Sabrelite
>> SPI model. The question is does SCM support different CS active (so device
>> is active at CS high). Your code assume that SMC will always use CS LOW to activate
>> device. If this is not true you might be interested in update this too.
> Aspeed SoC does not let you configure the chip select polarity
> (nor the clock phase ) So it is active low only.
Thanks.
>
> Thanks,
>
> C.
>
>

Re: [Qemu-devel] [PATCH v2 00/11] Aspeed SMC controller fixes and improvements

[Peter Maydell]

On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> I have reduced the patchset size to focus on some improvements of the
> SMC (Flash) controller model only and will address the watchdog and
> network models in other patchset.
>
> The main benefit of this series is to enable booting directly from a
> flash image containing U-Boot. It adds :
>
>  - some minor fixes and code rearrangements
>  - Command mode support. Flash contents is accessed directly on the
>    AHB bus
>  - auto strapping of configuration for boot flash
>  - use CE0 as a boot ROM. Today, qemu can not boot from a MMIO region.
>    As this is complex to do (TCG layer modification), we use a ROM
>    region in which we copy the flash contents. Hopefully, I got it
>    right this time.
>  - dummy bytes support for Command mode only
>
> Still in the pipe for the SMC controller are :
>
>  - DMA support
>  - dummy bytes support in user mode (hacky)
>
> which I will send later.

Applied 1-10 to target-arm.next, but not 11 since there
seemed to be some discussion on whether it was correct.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 00/11] Aspeed SMC controller fixes and improvements

[Cédric Le Goater]

On 01/16/2017 06:00 PM, Peter Maydell wrote:
> On 9 January 2017 at 16:24, Cédric Le Goater <clg@kaod.org> wrote:
>> Hello,
>>
>> I have reduced the patchset size to focus on some improvements of the
>> SMC (Flash) controller model only and will address the watchdog and
>> network models in other patchset.
>>
>> The main benefit of this series is to enable booting directly from a
>> flash image containing U-Boot. It adds :
>>
>>  - some minor fixes and code rearrangements
>>  - Command mode support. Flash contents is accessed directly on the
>>    AHB bus
>>  - auto strapping of configuration for boot flash
>>  - use CE0 as a boot ROM. Today, qemu can not boot from a MMIO region.
>>    As this is complex to do (TCG layer modification), we use a ROM
>>    region in which we copy the flash contents. Hopefully, I got it
>>    right this time.
>>  - dummy bytes support for Command mode only
>>
>> Still in the pipe for the SMC controller are :
>>
>>  - DMA support
>>  - dummy bytes support in user mode (hacky)
>>
>> which I will send later.
> 
> Applied 1-10 to target-arm.next, but not 11 since there
> seemed to be some discussion on whether it was correct.

yes. 11 needs a change in the condition which is used 
to send dummies. After that, I think we should be fine 
for dummy support in Command mode, which we need to boot.

For User mode, this is another story.   

Thanks,

C.