[Qemu-devel] [PATCH 00/11] aspeed: misc fixes and enhancements (SMC)

[Qemu-devel] [PATCH 00/11] aspeed: misc fixes and enhancements (SMC)

[Cédric Le Goater]

Hello,

This series adds a couple of cleanups and two main features to the
Aspeed machines :

 - a 'mmio-exec' property to boot directly from a memory region alias
   of the FMC flash module using MMIO execution. This is not activated
   by default because boot time needs to be improved on recent
   firmwares.

 - support for DMA to access the flash modules. Our primary need is
   the checksum calculation which is used to evaluate the best clock
   settings for reads.

Thanks,

C.

Cédric Le Goater (11):
  aspeed/timer: fix compile breakage with clang 3.4.2
  hw/arm/aspeed: change the FMC flash model of the AST2500 evb
  hw/arm/aspeed: Add an Aspeed machine class
  hw/arm/aspeed: add a 'mmio-exec' property to boot from the FMC flash
    module
  aspeed/smc: fix some alignment issues
  aspeed/smc: fix default read value
  aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties
  aspeed/smc: add support for DMAs
  aspeed/smc: add DMA calibration settings
  aspeed/smc: inject errors in DMA checksum
  aspeed/smc: Add dummy data register

 include/hw/arm/aspeed.h         |  48 ++++++
 include/hw/ssi/aspeed_smc.h     |   4 +
 include/hw/timer/aspeed_timer.h |   3 +-
 hw/arm/aspeed.c                 | 255 +++++++++++++------------------
 hw/arm/aspeed_soc.c             |  28 ++--
 hw/ssi/aspeed_smc.c             | 263 ++++++++++++++++++++++++++++++--
 hw/timer/aspeed_timer.c         |   1 -
 7 files changed, 428 insertions(+), 174 deletions(-)
 create mode 100644 include/hw/arm/aspeed.h

-- 
2.17.1

[Qemu-devel] [PATCH 01/11] aspeed/timer: fix compile breakage with clang 3.4.2

[Cédric Le Goater]

In file included from /home/thuth/devel/qemu/hw/timer/aspeed_timer.c:16:
/home/thuth/devel/qemu/include/hw/misc/aspeed_scu.h:37:3: error:
redefinition of typedef 'AspeedSCUState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
} AspeedSCUState;
  ^
/home/thuth/devel/qemu/include/hw/timer/aspeed_timer.h:27:31: note:
previous definition is here
typedef struct AspeedSCUState AspeedSCUState;

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/timer/aspeed_timer.h | 3 +--
 hw/timer/aspeed_timer.c         | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
index 040a08873432..1fb949e16710 100644
--- a/include/hw/timer/aspeed_timer.h
+++ b/include/hw/timer/aspeed_timer.h
@@ -23,8 +23,7 @@
 #define ASPEED_TIMER_H
 
 #include "qemu/timer.h"
-
-typedef struct AspeedSCUState AspeedSCUState;
+#include "hw/misc/aspeed_scu.h"
 
 #define ASPEED_TIMER(obj) \
     OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 5e3f51b66b43..54b400b94aa9 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -13,7 +13,6 @@
 #include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/timer/aspeed_timer.h"
-#include "hw/misc/aspeed_scu.h"
 #include "qemu-common.h"
 #include "qemu/bitops.h"
 #include "qemu/timer.h"
-- 
2.17.1

[Qemu-devel] [PATCH 02/11] hw/arm/aspeed: change the FMC flash model of the AST2500 evb

[Cédric Le Goater]

The AST2500 evb is shipped with a W25Q256 which has a non volatile bit
to make the chip operate in 4 Byte address mode at power up. This
should be an interesting feature to model as it will exercise a bit
more the SMC controllers and MMIO execution at boot time.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index bb9590f1aed1..f2d64e45511a 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -105,7 +105,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
     [AST2500_EVB]  = {
         .soc_name  = "ast2500-a1",
         .hw_strap1 = AST2500_EVB_HW_STRAP1,
-        .fmc_model = "n25q256a",
+        .fmc_model = "w25q256",
         .spi_model = "mx25l25635e",
         .num_cs    = 1,
         .i2c_init  = ast2500_evb_i2c_init,
-- 
2.17.1

[Qemu-devel] [PATCH 03/11] hw/arm/aspeed: Add an Aspeed machine class

[Cédric Le Goater]

The code looks better, it removes duplicated lines and it will ease
the introduction of common properties for the Aspeed machines.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed.h |  46 +++++++++
 hw/arm/aspeed.c         | 212 +++++++++++++---------------------------
 2 files changed, 116 insertions(+), 142 deletions(-)
 create mode 100644 include/hw/arm/aspeed.h

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
new file mode 100644
index 000000000000..2b77f8d2b3c8
--- /dev/null
+++ b/include/hw/arm/aspeed.h
@@ -0,0 +1,46 @@
+/*
+ * Aspeed Machines
+ *
+ * Copyright 2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef _ARM_ASPEED_H
+#define _ARM_ASPEED_H
+
+#include "hw/boards.h"
+
+typedef struct AspeedBoardState AspeedBoardState;
+
+typedef struct AspeedBoardConfig {
+    const char *name;
+    const char *desc;
+    const char *soc_name;
+    uint32_t hw_strap1;
+    const char *fmc_model;
+    const char *spi_model;
+    uint32_t num_cs;
+    void (*i2c_init)(AspeedBoardState *bmc);
+} AspeedBoardConfig;
+
+#define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
+#define ASPEED_MACHINE(obj) \
+    OBJECT_CHECK(AspeedMachine, (obj), TYPE_ASPEED_MACHINE)
+
+typedef struct AspeedMachine {
+    MachineState parent_obj;
+} AspeedMachine;
+
+#define ASPEED_MACHINE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(AspeedMachineClass, (klass), TYPE_ASPEED_MACHINE)
+#define ASPEED_MACHINE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(AspeedMachineClass, (obj), TYPE_ASPEED_MACHINE)
+
+typedef struct AspeedMachineClass {
+    MachineClass parent_obj;
+    const AspeedBoardConfig *board;
+} AspeedMachineClass;
+
+
+#endif
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index f2d64e45511a..6b33ecd5aa43 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -15,6 +15,7 @@
 #include "cpu.h"
 #include "exec/address-spaces.h"
 #include "hw/arm/arm.h"
+#include "hw/arm/aspeed.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/boards.h"
 #include "hw/i2c/smbus.h"
@@ -34,22 +35,6 @@ typedef struct AspeedBoardState {
     MemoryRegion max_ram;
 } AspeedBoardState;
 
-typedef struct AspeedBoardConfig {
-    const char *soc_name;
-    uint32_t hw_strap1;
-    const char *fmc_model;
-    const char *spi_model;
-    uint32_t num_cs;
-    void (*i2c_init)(AspeedBoardState *bmc);
-} AspeedBoardConfig;
-
-enum {
-    PALMETTO_BMC,
-    AST2500_EVB,
-    ROMULUS_BMC,
-    WITHERSPOON_BMC,
-};
-
 /* Palmetto hardware value: 0x120CE416 */
 #define PALMETTO_BMC_HW_STRAP1 (                                        \
         SCU_AST2400_HW_STRAP_DRAM_SIZE(DRAM_SIZE_256MB) |               \
@@ -88,46 +73,6 @@ enum {
 /* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
 #define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
 
-static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
-static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
-static void romulus_bmc_i2c_init(AspeedBoardState *bmc);
-static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
-
-static const AspeedBoardConfig aspeed_boards[] = {
-    [PALMETTO_BMC] = {
-        .soc_name  = "ast2400-a1",
-        .hw_strap1 = PALMETTO_BMC_HW_STRAP1,
-        .fmc_model = "n25q256a",
-        .spi_model = "mx25l25635e",
-        .num_cs    = 1,
-        .i2c_init  = palmetto_bmc_i2c_init,
-    },
-    [AST2500_EVB]  = {
-        .soc_name  = "ast2500-a1",
-        .hw_strap1 = AST2500_EVB_HW_STRAP1,
-        .fmc_model = "w25q256",
-        .spi_model = "mx25l25635e",
-        .num_cs    = 1,
-        .i2c_init  = ast2500_evb_i2c_init,
-    },
-    [ROMULUS_BMC]  = {
-        .soc_name  = "ast2500-a1",
-        .hw_strap1 = ROMULUS_BMC_HW_STRAP1,
-        .fmc_model = "n25q256a",
-        .spi_model = "mx66l1g45g",
-        .num_cs    = 2,
-        .i2c_init  = romulus_bmc_i2c_init,
-    },
-    [WITHERSPOON_BMC]  = {
-        .soc_name  = "ast2500-a1",
-        .hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
-        .fmc_model = "mx25l25635e",
-        .spi_model = "mx66l1g45g",
-        .num_cs    = 2,
-        .i2c_init  = witherspoon_bmc_i2c_init,
-    },
-};
-
 /*
  * The max ram region is for firmwares that scan the address space
  * with load/store to guess how much RAM the SoC has.
@@ -313,30 +258,6 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
     object_property_set_int(OBJECT(dev), 110000, "temperature3", &error_abort);
 }
 
-static void palmetto_bmc_init(MachineState *machine)
-{
-    aspeed_board_init(machine, &aspeed_boards[PALMETTO_BMC]);
-}
-
-static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->desc = "OpenPOWER Palmetto BMC (ARM926EJ-S)";
-    mc->init = palmetto_bmc_init;
-    mc->max_cpus = 1;
-    mc->no_sdcard = 1;
-    mc->no_floppy = 1;
-    mc->no_cdrom = 1;
-    mc->no_parallel = 1;
-}
-
-static const TypeInfo palmetto_bmc_type = {
-    .name = MACHINE_TYPE_NAME("palmetto-bmc"),
-    .parent = TYPE_MACHINE,
-    .class_init = palmetto_bmc_class_init,
-};
-
 static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
@@ -353,30 +274,6 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
-static void ast2500_evb_init(MachineState *machine)
-{
-    aspeed_board_init(machine, &aspeed_boards[AST2500_EVB]);
-}
-
-static void ast2500_evb_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->desc = "Aspeed AST2500 EVB (ARM1176)";
-    mc->init = ast2500_evb_init;
-    mc->max_cpus = 1;
-    mc->no_sdcard = 1;
-    mc->no_floppy = 1;
-    mc->no_cdrom = 1;
-    mc->no_parallel = 1;
-}
-
-static const TypeInfo ast2500_evb_type = {
-    .name = MACHINE_TYPE_NAME("ast2500-evb"),
-    .parent = TYPE_MACHINE,
-    .class_init = ast2500_evb_class_init,
-};
-
 static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
@@ -386,30 +283,6 @@ static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
-static void romulus_bmc_init(MachineState *machine)
-{
-    aspeed_board_init(machine, &aspeed_boards[ROMULUS_BMC]);
-}
-
-static void romulus_bmc_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->desc = "OpenPOWER Romulus BMC (ARM1176)";
-    mc->init = romulus_bmc_init;
-    mc->max_cpus = 1;
-    mc->no_sdcard = 1;
-    mc->no_floppy = 1;
-    mc->no_cdrom = 1;
-    mc->no_parallel = 1;
-}
-
-static const TypeInfo romulus_bmc_type = {
-    .name = MACHINE_TYPE_NAME("romulus-bmc"),
-    .parent = TYPE_MACHINE,
-    .class_init = romulus_bmc_class_init,
-};
-
 static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
@@ -433,36 +306,91 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
                      0x60);
 }
 
-static void witherspoon_bmc_init(MachineState *machine)
+static void aspeed_machine_init(MachineState *machine)
 {
-    aspeed_board_init(machine, &aspeed_boards[WITHERSPOON_BMC]);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
+
+    aspeed_board_init(machine, amc->board);
 }
 
-static void witherspoon_bmc_class_init(ObjectClass *oc, void *data)
+static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+    const AspeedBoardConfig *board = data;
 
-    mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)";
-    mc->init = witherspoon_bmc_init;
+    mc->desc = board->desc;
+    mc->init = aspeed_machine_init;
     mc->max_cpus = 1;
     mc->no_sdcard = 1;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
+    amc->board = board;
 }
 
-static const TypeInfo witherspoon_bmc_type = {
-    .name = MACHINE_TYPE_NAME("witherspoon-bmc"),
+static const TypeInfo aspeed_machine_type = {
+    .name = TYPE_ASPEED_MACHINE,
     .parent = TYPE_MACHINE,
-    .class_init = witherspoon_bmc_class_init,
+    .instance_size = sizeof(AspeedMachine),
+    .class_size = sizeof(AspeedMachineClass),
+    .abstract = true,
+};
+
+static const AspeedBoardConfig aspeed_boards[] = {
+    {
+        .name      = MACHINE_TYPE_NAME("palmetto-bmc"),
+        .desc      = "OpenPOWER Palmetto BMC (ARM926EJ-S)",
+        .soc_name  = "ast2400-a1",
+        .hw_strap1 = PALMETTO_BMC_HW_STRAP1,
+        .fmc_model = "n25q256a",
+        .spi_model = "mx25l25635e",
+        .num_cs    = 1,
+        .i2c_init  = palmetto_bmc_i2c_init,
+    }, {
+        .name      = MACHINE_TYPE_NAME("ast2500-evb"),
+        .desc      = "Aspeed AST2500 EVB (ARM1176)",
+        .soc_name  = "ast2500-a1",
+        .hw_strap1 = AST2500_EVB_HW_STRAP1,
+        .fmc_model = "w25q256",
+        .spi_model = "mx25l25635e",
+        .num_cs    = 1,
+        .i2c_init  = ast2500_evb_i2c_init,
+    }, {
+        .name      = MACHINE_TYPE_NAME("romulus-bmc"),
+        .desc      = "OpenPOWER Romulus BMC (ARM1176)",
+        .soc_name  = "ast2500-a1",
+        .hw_strap1 = ROMULUS_BMC_HW_STRAP1,
+        .fmc_model = "n25q256a",
+        .spi_model = "mx66l1g45g",
+        .num_cs    = 2,
+        .i2c_init  = romulus_bmc_i2c_init,
+    }, {
+        .name      = MACHINE_TYPE_NAME("witherspoon-bmc"),
+        .desc      = "OpenPOWER Witherspoon BMC (ARM1176)",
+        .soc_name  = "ast2500-a1",
+        .hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
+        .fmc_model = "mx25l25635e",
+        .spi_model = "mx66l1g45g",
+        .num_cs    = 2,
+        .i2c_init  = witherspoon_bmc_i2c_init,
+    },
 };
 
-static void aspeed_machine_init(void)
+static void aspeed_machine_types(void)
 {
-    type_register_static(&palmetto_bmc_type);
-    type_register_static(&ast2500_evb_type);
-    type_register_static(&romulus_bmc_type);
-    type_register_static(&witherspoon_bmc_type);
+    int i;
+
+    type_register_static(&aspeed_machine_type);
+    for (i = 0; i < ARRAY_SIZE(aspeed_boards); ++i) {
+        TypeInfo ti = {
+            .name       = aspeed_boards[i].name,
+            .parent     = TYPE_ASPEED_MACHINE,
+            .class_init = aspeed_machine_class_init,
+            .class_data = (void *)&aspeed_boards[i],
+        };
+        type_register(&ti);
+    }
 }
 
-type_init(aspeed_machine_init)
+type_init(aspeed_machine_types)
-- 
2.17.1

[Qemu-devel] [PATCH 04/11] hw/arm/aspeed: add a 'mmio-exec' property to boot from the FMC flash module

[Cédric Le Goater]

Now that MMIO execution is supported, introduce a 'mmio-exec' property
to boot directly from CE0 of the FMC controller using a memory region
alias.

The overhead for the current firmware images using a custom U-Boot is
around 2 seconds, which is fine, but with a U-Boot from mainline, it
takes an extra 50 seconds or so to reach Linux. This might be related
to the fact that a device tree is used.

MMIO execution is not activated by default because until boot time is
improved.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed.h |  2 ++
 hw/arm/aspeed.c         | 43 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 2b77f8d2b3c8..d079f4d6e5db 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -30,6 +30,8 @@ typedef struct AspeedBoardConfig {
 
 typedef struct AspeedMachine {
     MachineState parent_obj;
+
+    bool mmio_exec;
 } AspeedMachine;
 
 #define ASPEED_MACHINE_CLASS(klass) \
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6b33ecd5aa43..3a66c2dedc3e 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -216,11 +216,18 @@ static void aspeed_board_init(MachineState *machine,
          * SoC and 128MB for the AST2500 SoC, which is twice as big as
          * needed by the flash modules of the Aspeed machines.
          */
-        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);
+        if (ASPEED_MACHINE(machine)->mmio_exec) {
+            memory_region_init_alias(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
+                                     &fl->mmio, 0, fl->size);
+            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
+                                        boot_rom);
+        } else {
+            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;
@@ -313,6 +320,29 @@ static void aspeed_machine_init(MachineState *machine)
     aspeed_board_init(machine, amc->board);
 }
 
+static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
+{
+    return ASPEED_MACHINE(obj)->mmio_exec;
+}
+
+static void aspeed_set_mmio_exec(Object *obj, bool value, Error **errp)
+{
+    ASPEED_MACHINE(obj)->mmio_exec = value;
+}
+
+static void aspeed_machine_instance_init(Object *obj)
+{
+    ASPEED_MACHINE(obj)->mmio_exec = false;
+}
+
+static void aspeed_machine_class_props_init(ObjectClass *oc)
+{
+    object_class_property_add_bool(oc, "mmio-exec", aspeed_get_mmio_exec,
+                                   aspeed_set_mmio_exec, &error_abort);
+    object_class_property_set_description(oc, "mmio-exec",
+                              "boot using MMIO execution", &error_abort);
+}
+
 static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -327,6 +357,8 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
     amc->board = board;
+
+    aspeed_machine_class_props_init(oc);
 }
 
 static const TypeInfo aspeed_machine_type = {
@@ -334,6 +366,7 @@ static const TypeInfo aspeed_machine_type = {
     .parent = TYPE_MACHINE,
     .instance_size = sizeof(AspeedMachine),
     .class_size = sizeof(AspeedMachineClass),
+    .instance_init = aspeed_machine_instance_init,
     .abstract = true,
 };
 
-- 
2.17.1

[Qemu-devel] [PATCH 05/11] aspeed/smc: fix some alignment issues

[Cédric Le Goater]

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index b29bfd3124a9..1270842dcf0c 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -388,8 +388,8 @@ static uint64_t aspeed_smc_flash_default_read(void *opaque, hwaddr addr,
 static void aspeed_smc_flash_default_write(void *opaque, hwaddr addr,
                                            uint64_t data, unsigned size)
 {
-   qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u: 0x%"
-                 PRIx64 "\n", __func__, addr, size, data);
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u: 0x%"
+                  PRIx64 "\n", __func__, addr, size, data);
 }
 
 static const MemoryRegionOps aspeed_smc_flash_default_ops = {
@@ -529,7 +529,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
      */
     if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
         for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
-                ssi_transfer(fl->controller->spi, 0xFF);
+            ssi_transfer(fl->controller->spi, 0xFF);
         }
     }
 }
@@ -567,7 +567,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
 }
 
 static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
-                           unsigned size)
+                                   unsigned size)
 {
     AspeedSMCFlash *fl = opaque;
     AspeedSMCState *s = fl->controller;
-- 
2.17.1

[Qemu-devel] [PATCH 06/11] aspeed/smc: fix default read value

[Cédric Le Goater]

0xFFFFFFFF should be returned for non implemented registers.

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

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

[Qemu-devel] [PATCH 07/11] aspeed/smc: add a 'sdram_base' and 'max-ram-size' properties

[Cédric Le Goater]

The setting of the DRAM address of the DMA transaction depends on the
DRAM base address and the maximun DRAM size of the SoC. Let's add a
couple of properties to give this information to the SMC controller
model.

Also, move the SDRAM Memory controller realization before the other
controllers which need it.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  4 ++++
 hw/arm/aspeed_soc.c         | 28 +++++++++++++++++++---------
 hw/ssi/aspeed_smc.c         |  2 ++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 1f557313fa93..d7090bb5e9b7 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -97,6 +97,10 @@ typedef struct AspeedSMCState {
     uint8_t r_timings;
     uint8_t conf_enable_w0;
 
+    /* for DMA support */
+    uint64_t sdram_base;
+    uint64_t max_ram_size;
+
     AspeedSMCFlash *flashes;
 } AspeedSMCState;
 
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 2cbacb4430bb..bbc05d172fe1 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -209,6 +209,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, ASPEED_SOC_SCU_BASE);
 
+    /* SDMC - SDRAM Memory Controller */
+    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, ASPEED_SOC_SDMC_BASE);
+
     /* VIC */
     object_property_set_bool(OBJECT(&s->vic), true, "realized", &err);
     if (err) {
@@ -252,7 +260,17 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                        qdev_get_gpio_in(DEVICE(&s->vic), 12));
 
     /* FMC, The number of CS is set at the board level */
-    object_property_set_bool(OBJECT(&s->fmc), true, "realized", &err);
+    object_property_set_int(OBJECT(&s->fmc), sc->info->sdram_base, "sdram-base",
+                            &err);
+    object_property_set_int(OBJECT(&s->fmc), s->sdmc.max_ram_size,
+                            "max-ram-size", &local_err);
+    error_propagate(&err, local_err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    object_property_set_bool(OBJECT(&s->fmc), true, "realized", &local_err);
+    error_propagate(&err, local_err);
     if (err) {
         error_propagate(errp, err);
         return;
@@ -278,14 +296,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                         s->spi[i].ctrl->flash_window_base);
     }
 
-    /* SDMC - SDRAM Memory Controller */
-    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, ASPEED_SOC_SDMC_BASE);
-
     /* Watch dog */
     for (i = 0; i < sc->info->wdts_num; i++) {
         object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized", &err);
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 6045ca11b969..500de6d16d09 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -800,6 +800,8 @@ static const VMStateDescription vmstate_aspeed_smc = {
 
 static Property aspeed_smc_properties[] = {
     DEFINE_PROP_UINT32("num-cs", AspeedSMCState, num_cs, 1),
+    DEFINE_PROP_UINT64("sdram-base", AspeedSMCState, sdram_base, 0),
+    DEFINE_PROP_UINT64("max-ram-size", AspeedSMCState, max_ram_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.17.1

[Qemu-devel] [PATCH 08/11] aspeed/smc: add support for DMAs

[Cédric Le Goater]

The FMC controller on the Aspeed SoCs support DMA to access the flash
modules. It can operate in a normal mode, to copy to or from the flash
module mapping window, or in a checksum calculation mode, to evaluate
the best clock settings for reads.

Our primary need is to support the checksum calculation mode and the
model only implements synchronous DMA accesses. Something to improve
in the future.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 500de6d16d09..534faec4c111 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -72,6 +72,10 @@
 #define   CTRL_CMD_MASK            0xff
 #define   CTRL_DUMMY_HIGH_SHIFT    14
 #define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
+#define CE_CTRL_CLOCK_FREQ_SHIFT   8
+#define CE_CTRL_CLOCK_FREQ_MASK    0xf
+#define CE_CTRL_CLOCK_FREQ(div)                                         \
+    (((div) & CE_CTRL_CLOCK_FREQ_MASK) << CE_CTRL_CLOCK_FREQ_SHIFT)
 #define   CTRL_DUMMY_LOW_SHIFT     6 /* 2 bits [7:6] */
 #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
 #define   CTRL_CMD_MODE_MASK       0x3
@@ -107,10 +111,10 @@
 #define   DMA_CTRL_DELAY_SHIFT  8
 #define   DMA_CTRL_FREQ_MASK    0xf
 #define   DMA_CTRL_FREQ_SHIFT   4
-#define   DMA_CTRL_MODE         (1 << 3)
+#define   DMA_CTRL_CALIB        (1 << 3)
 #define   DMA_CTRL_CKSUM        (1 << 2)
-#define   DMA_CTRL_DIR          (1 << 1)
-#define   DMA_CTRL_EN           (1 << 0)
+#define   DMA_CTRL_WRITE        (1 << 1)
+#define   DMA_CTRL_ENABLE       (1 << 0)
 
 /* DMA Flash Side Address */
 #define R_DMA_FLASH_ADDR  (0x84 / 4)
@@ -142,6 +146,21 @@
 #define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
 #define ASPEED_SOC_SPI2_FLASH_BASE  0x38000000
 
+/*
+ * DMA DRAM addresses should be 4 bytes aligned and the valid address
+ * range is the full address space of the SoC
+ *
+ * DMA flash addresses should be 4 bytes aligned and the valid address
+ * range is 0x20000000 - 0x2FFFFFFF.
+ *
+ * DMA length is from 4 bytes to 32MB
+ *   0: 4 bytes
+ *   0x7FFFFF: 32M bytes
+ */
+#define DMA_DRAM_MASK(s)        ((s)->max_ram_size - 4)
+#define DMA_FLASH_MASK          0x0FFFFFFC
+#define DMA_LENGTH_MASK         0x01FFFFFC
+
 /* Flash opcodes. */
 #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
 
@@ -625,9 +644,6 @@ static void aspeed_smc_reset(DeviceState *d)
 
     memset(s->regs, 0, sizeof s->regs);
 
-    /* Pretend DMA is done (u-boot initialization) */
-    s->regs[R_INTR_CTRL] = INTR_CTRL_DMA_STATUS;
-
     /* Unselect all slaves */
     for (i = 0; i < s->num_cs; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
@@ -664,6 +680,11 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == s->r_timings ||
         addr == s->r_ce_ctrl ||
         addr == R_INTR_CTRL ||
+        (s->ctrl->has_dma && addr == R_DMA_CTRL) ||
+        (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) ||
+        (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) ||
+        (s->ctrl->has_dma && addr == R_DMA_LEN) ||
+        (s->ctrl->has_dma && addr == R_DMA_CHECKSUM) ||
         (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
         (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
         return s->regs[addr];
@@ -674,6 +695,115 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
     }
 }
 
+/*
+ * Accumulate the result of the reads to provide a checksum that will
+ * be used to validate the read timing settings.
+ */
+static void aspeed_smc_dma_checksum(AspeedSMCState *s)
+{
+    uint32_t data;
+
+    if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid direction for DMA checksum\n",  __func__);
+        return;
+    }
+
+    while (s->regs[R_DMA_LEN]) {
+        cpu_physical_memory_read(s->regs[R_DMA_FLASH_ADDR], &data, 4);
+
+        /*
+         * When the DMA is on-going, the DMA registers are updated
+         * with the current working addresses and length.
+         */
+        s->regs[R_DMA_CHECKSUM] += data;
+        s->regs[R_DMA_FLASH_ADDR] += 4;
+        s->regs[R_DMA_LEN] -= 4;
+    }
+}
+
+static void aspeed_smc_dma_rw(AspeedSMCState *s)
+{
+    uint32_t data;
+
+    while (s->regs[R_DMA_LEN]) {
+        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
+            cpu_physical_memory_read(s->regs[R_DMA_DRAM_ADDR], &data, 4);
+            cpu_physical_memory_write(s->regs[R_DMA_FLASH_ADDR], &data, 4);
+        } else {
+            cpu_physical_memory_read(s->regs[R_DMA_FLASH_ADDR], &data, 4);
+            cpu_physical_memory_write(s->regs[R_DMA_DRAM_ADDR], &data, 4);
+        }
+
+        /*
+         * When the DMA is on-going, the DMA registers are updated
+         * with the current working addresses and length.
+         */
+        s->regs[R_DMA_FLASH_ADDR] += 4;
+        s->regs[R_DMA_DRAM_ADDR] += 4;
+        s->regs[R_DMA_LEN] -= 4;
+    }
+}
+
+static void aspeed_smc_dma_stop(AspeedSMCState *s)
+{
+    /*
+     * When the DMA is disabled, INTR_CTRL_DMA_STATUS=0 means the
+     * engine is idle
+     */
+    s->regs[R_INTR_CTRL] &= ~INTR_CTRL_DMA_STATUS;
+    s->regs[R_DMA_CHECKSUM] = 0;
+
+    /*
+     * Lower the DMA irq in any case. The IRQ control register could
+     * have been cleared before disabling the DMA.
+     */
+    qemu_irq_lower(s->irq);
+}
+
+/*
+ * When INTR_CTRL_DMA_STATUS=1, the DMA has completed and a new DMA
+ * can start even if the result of the previous was not collected.
+ */
+static bool aspeed_smc_dma_in_progress(AspeedSMCState *s)
+{
+    return s->regs[R_DMA_CTRL] & DMA_CTRL_ENABLE &&
+        !(s->regs[R_INTR_CTRL] & INTR_CTRL_DMA_STATUS);
+}
+
+static void aspeed_smc_dma_done(AspeedSMCState *s)
+{
+    s->regs[R_INTR_CTRL] |= INTR_CTRL_DMA_STATUS;
+    if (s->regs[R_INTR_CTRL] & INTR_CTRL_DMA_EN) {
+        qemu_irq_raise(s->irq);
+    }
+}
+
+static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint64_t dma_ctrl)
+{
+    if (!(dma_ctrl & DMA_CTRL_ENABLE)) {
+        s->regs[R_DMA_CTRL] = dma_ctrl;
+
+        aspeed_smc_dma_stop(s);
+        return;
+    }
+
+    if (aspeed_smc_dma_in_progress(s)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA in progress\n",  __func__);
+        return;
+    }
+
+    s->regs[R_DMA_CTRL] = dma_ctrl;
+
+    if (s->regs[R_DMA_CTRL] & DMA_CTRL_CKSUM) {
+        aspeed_smc_dma_checksum(s);
+    } else {
+        aspeed_smc_dma_rw(s);
+    }
+
+    aspeed_smc_dma_done(s);
+}
+
 static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
                              unsigned int size)
 {
@@ -697,6 +827,19 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         if (value != s->regs[R_SEG_ADDR0 + cs]) {
             aspeed_smc_flash_set_segment(s, cs, value);
         }
+    } else if (addr == R_INTR_CTRL) {
+        s->regs[addr] = value;
+    } else if (s->ctrl->has_dma && addr == R_DMA_CTRL) {
+        aspeed_smc_dma_ctrl(s, value);
+    } else if (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) {
+        value &= DMA_DRAM_MASK(s);
+        s->regs[addr] = s->sdram_base | value;
+    } else if (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) {
+        value &= DMA_FLASH_MASK;
+        s->regs[addr] = ASPEED_SOC_FMC_FLASH_BASE | value;
+    } else if (s->ctrl->has_dma && addr == R_DMA_LEN) {
+        value &= DMA_LENGTH_MASK;
+        s->regs[addr] = value;
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
@@ -729,6 +872,9 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     s->r_timings = s->ctrl->r_timings;
     s->conf_enable_w0 = s->ctrl->conf_enable_w0;
 
+    /* DMA irq */
+    sysbus_init_irq(sbd, &s->irq);
+
     /* Enforce some real HW limits */
     if (s->num_cs > s->ctrl->max_slaves) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: num_cs cannot exceed: %d\n",
@@ -739,7 +885,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     s->spi = ssi_create_bus(dev, "spi");
 
     /* Setup cs_lines for slaves */
-    sysbus_init_irq(sbd, &s->irq);
     s->cs_lines = g_new0(qemu_irq, s->num_cs);
     ssi_auto_connect_slaves(dev, s->cs_lines, s->spi);
 
-- 
2.17.1

[Qemu-devel] [PATCH 09/11] aspeed/smc: add DMA calibration settings

[Cédric Le Goater]

When doing calibration, the SPI clock rate in the CE0 Control Register
and the read delay cycles in the Read Timing Compensation Register are
replaced by bit[11:4] of the DMA Control Register.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 534faec4c111..983066f5ad1d 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -695,6 +695,56 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
     }
 }
 
+static uint8_t aspeed_smc_hclk_divisor(uint8_t hclk_mask)
+{
+    /* HCLK/1 .. HCLK/16 */
+    const uint8_t hclk_divisors[] = {
+        15, 7, 14, 6, 13, 5, 12, 4, 11, 3, 10, 2, 9, 1, 8, 0
+    };
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
+        if (hclk_mask == hclk_divisors[i]) {
+            return i + 1;
+        }
+    }
+
+    qemu_log_mask(LOG_GUEST_ERROR, "invalid HCLK mask %x", hclk_mask);
+    return 0;
+}
+
+/*
+ * When doing calibration, the SPI clock rate in the CE0 Control
+ * Register and the read delay cycles in the Read Timing
+ * Compensation Register are replaced by bit[11:4] of the DMA
+ * Control Register.
+ */
+static void aspeed_smc_dma_calibration(AspeedSMCState *s)
+{
+    uint8_t delay =
+        (s->regs[R_DMA_CTRL] >> DMA_CTRL_DELAY_SHIFT) & DMA_CTRL_DELAY_MASK;
+    uint8_t hclk_mask =
+        (s->regs[R_DMA_CTRL] >> DMA_CTRL_FREQ_SHIFT) & DMA_CTRL_FREQ_MASK;
+    uint8_t hclk_div = aspeed_smc_hclk_divisor(hclk_mask);
+    uint32_t hclk_shift = (hclk_div - 1) << 2;
+    uint8_t cs;
+
+    /* Only HCLK/1 - HCLK/5 have tunable delays */
+    if (hclk_div && hclk_div < 6) {
+        s->regs[s->r_timings] &= ~(0xf << hclk_shift);
+        s->regs[s->r_timings] |= delay << hclk_shift;
+    }
+
+    /*
+     * TODO: choose CS depending on the DMA address. This is not used
+     * on the field.
+     */
+    cs = 0;
+    s->regs[s->r_ctrl0 + cs] &=
+        ~(CE_CTRL_CLOCK_FREQ_MASK << CE_CTRL_CLOCK_FREQ_SHIFT);
+    s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
+}
+
 /*
  * Accumulate the result of the reads to provide a checksum that will
  * be used to validate the read timing settings.
@@ -709,6 +759,10 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
         return;
     }
 
+    if (s->regs[R_DMA_CTRL] & DMA_CTRL_CALIB) {
+        aspeed_smc_dma_calibration(s);
+    }
+
     while (s->regs[R_DMA_LEN]) {
         cpu_physical_memory_read(s->regs[R_DMA_FLASH_ADDR], &data, 4);
 
-- 
2.17.1

[Qemu-devel] [PATCH 10/11] aspeed/smc: inject errors in DMA checksum

[Cédric Le Goater]

Emulate read errors in the DMA Checksum Register for high frequencies
and optimistic settings of the Read Timing Compensation Register. This
will help in tuning the SPI timing calibration algorithm.

The values below are those to expect from the first flash device of
the FMC controller of a palmetto-bmc machine.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 983066f5ad1d..da2fedfcd3cd 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -745,6 +745,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
     s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
 }
 
+static bool aspeed_smc_inject_read_failure(AspeedSMCState *s)
+{
+    uint8_t delay =
+        (s->regs[R_DMA_CTRL] >> DMA_CTRL_DELAY_SHIFT) & DMA_CTRL_DELAY_MASK;
+    uint8_t hclk_mask =
+        (s->regs[R_DMA_CTRL] >> DMA_CTRL_FREQ_SHIFT) & DMA_CTRL_FREQ_MASK;
+
+    /*
+     * Typical values of a palmetto-bmc machine.
+     */
+    switch (aspeed_smc_hclk_divisor(hclk_mask)) {
+    case 4 ... 16:
+        return false;
+    case 3: /* at least one HCLK cycle delay */
+        return (delay & 0x7) < 1;
+    case 2: /* at least two HCLK cycle delay */
+        return (delay & 0x7) < 2;
+    case 1: /* (> 100MHz) is above the max freq of the controller */
+        return true;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /*
  * Accumulate the result of the reads to provide a checksum that will
  * be used to validate the read timing settings.
@@ -774,6 +798,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
         s->regs[R_DMA_FLASH_ADDR] += 4;
         s->regs[R_DMA_LEN] -= 4;
     }
+
+    if (aspeed_smc_inject_read_failure(s)) {
+        s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
+    }
+
 }
 
 static void aspeed_smc_dma_rw(AspeedSMCState *s)
-- 
2.17.1

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

[Cédric Le Goater]

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

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index da2fedfcd3cd..f31bbc895caa 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -102,8 +102,8 @@
 /* Misc Control Register #1 */
 #define R_MISC_CTRL1      (0x50 / 4)
 
-/* Misc Control Register #2 */
-#define R_MISC_CTRL2      (0x54 / 4)
+/* SPI dummy cycle data */
+#define R_DUMMY_DATA      (0x54 / 4)
 
 /* DMA Control/Status Register */
 #define R_DMA_CTRL        (0x80 / 4)
@@ -548,7 +548,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
      */
     if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
         for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
-            ssi_transfer(fl->controller->spi, 0xFF);
+            ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff);
         }
     }
 }
@@ -680,6 +680,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == s->r_timings ||
         addr == s->r_ce_ctrl ||
         addr == R_INTR_CTRL ||
+        addr == R_DUMMY_DATA ||
         (s->ctrl->has_dma && addr == R_DMA_CTRL) ||
         (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) ||
         (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) ||
@@ -912,6 +913,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         }
     } else if (addr == R_INTR_CTRL) {
         s->regs[addr] = value;
+    } else if (addr == R_DUMMY_DATA) {
+        s->regs[addr] = value & 0xff ;
     } else if (s->ctrl->has_dma && addr == R_DMA_CTRL) {
         aspeed_smc_dma_ctrl(s, value);
     } else if (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 00/11] aspeed: misc fixes and enhancements (SMC)

[Joel Stanley]

On Fri, 31 Aug 2018 at 03:38, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello,
>
> This series adds a couple of cleanups and two main features to the
> Aspeed machines :
>
>  - a 'mmio-exec' property to boot directly from a memory region alias
>    of the FMC flash module using MMIO execution. This is not activated
>    by default because boot time needs to be improved on recent
>    firmwares.
>
>  - support for DMA to access the flash modules. Our primary need is
>    the checksum calculation which is used to evaluate the best clock
>    settings for reads.

I gave this series a number of tests and it looked good.

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

Cheers,

Joel

Re: [Qemu-devel] [Qemu-arm] [PATCH 05/11] aspeed/smc: fix some alignment issues

[Philippe Mathieu-Daudé]

On 8/31/18 7:38 AM, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

> ---
>  hw/ssi/aspeed_smc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index b29bfd3124a9..1270842dcf0c 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -388,8 +388,8 @@ static uint64_t aspeed_smc_flash_default_read(void *opaque, hwaddr addr,
>  static void aspeed_smc_flash_default_write(void *opaque, hwaddr addr,
>                                             uint64_t data, unsigned size)
>  {
> -   qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u: 0x%"
> -                 PRIx64 "\n", __func__, addr, size, data);
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u: 0x%"
> +                  PRIx64 "\n", __func__, addr, size, data);
>  }
>  
>  static const MemoryRegionOps aspeed_smc_flash_default_ops = {
> @@ -529,7 +529,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>       */
>      if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
>          for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
> -                ssi_transfer(fl->controller->spi, 0xFF);
> +            ssi_transfer(fl->controller->spi, 0xFF);
>          }
>      }
>  }
> @@ -567,7 +567,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>  }
>  
>  static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
> -                           unsigned size)
> +                                   unsigned size)
>  {
>      AspeedSMCFlash *fl = opaque;
>      AspeedSMCState *s = fl->controller;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 11/11] aspeed/smc: Add dummy data register

[Philippe Mathieu-Daudé]

On 8/31/18 8:15 AM, Cédric Le Goater wrote:
> The SMC controllers have a register containing the byte that will be
> used as dummy output. It can be modified by software.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index da2fedfcd3cd..f31bbc895caa 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -102,8 +102,8 @@
>  /* Misc Control Register #1 */
>  #define R_MISC_CTRL1      (0x50 / 4)
>  
> -/* Misc Control Register #2 */
> -#define R_MISC_CTRL2      (0x54 / 4)
> +/* SPI dummy cycle data */
> +#define R_DUMMY_DATA      (0x54 / 4)
>  
>  /* DMA Control/Status Register */
>  #define R_DMA_CTRL        (0x80 / 4)
> @@ -548,7 +548,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>       */
>      if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
>          for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
> -            ssi_transfer(fl->controller->spi, 0xFF);
> +            ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff);

The DUMMY_DATA register always contains a 8-bit value, so this AND
shouldn't be necessary. Regardless:

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


>          }
>      }
>  }
> @@ -680,6 +680,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>          addr == s->r_timings ||
>          addr == s->r_ce_ctrl ||
>          addr == R_INTR_CTRL ||
> +        addr == R_DUMMY_DATA ||
>          (s->ctrl->has_dma && addr == R_DMA_CTRL) ||
>          (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) ||
>          (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) ||
> @@ -912,6 +913,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>          }
>      } else if (addr == R_INTR_CTRL) {
>          s->regs[addr] = value;
> +    } else if (addr == R_DUMMY_DATA) {
> +        s->regs[addr] = value & 0xff ;
>      } else if (s->ctrl->has_dma && addr == R_DMA_CTRL) {
>          aspeed_smc_dma_ctrl(s, value);
>      } else if (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) {
> 

Re: [Qemu-devel] [PATCH 06/11] aspeed/smc: fix default read value

[Philippe Mathieu-Daudé]

Hi Cédric,

On 8/31/18 7:38 AM, Cédric Le Goater wrote:
> 0xFFFFFFFF should be returned for non implemented registers.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 1270842dcf0c..6045ca11b969 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -665,12 +665,12 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>          addr == s->r_ce_ctrl ||
>          addr == R_INTR_CTRL ||
>          (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
> -        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
> +        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {

This change seems unrelated to this commit.

>          return s->regs[addr];
>      } else {
>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                        __func__, addr);
> -        return 0;
> +        return -1;
>      }
>  }
>  
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 03/11] hw/arm/aspeed: Add an Aspeed machine class

[Philippe Mathieu-Daudé]

On 8/31/18 7:38 AM, Cédric Le Goater wrote:
> The code looks better, it removes duplicated lines and it will ease
> the introduction of common properties for the Aspeed machines.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Nice cleanup :)

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

> ---
>  include/hw/arm/aspeed.h |  46 +++++++++
>  hw/arm/aspeed.c         | 212 +++++++++++++---------------------------
>  2 files changed, 116 insertions(+), 142 deletions(-)
>  create mode 100644 include/hw/arm/aspeed.h
> 
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> new file mode 100644
> index 000000000000..2b77f8d2b3c8
> --- /dev/null
> +++ b/include/hw/arm/aspeed.h
> @@ -0,0 +1,46 @@
> +/*
> + * Aspeed Machines
> + *
> + * Copyright 2018 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef _ARM_ASPEED_H
> +#define _ARM_ASPEED_H
> +
> +#include "hw/boards.h"
> +
> +typedef struct AspeedBoardState AspeedBoardState;
> +
> +typedef struct AspeedBoardConfig {
> +    const char *name;
> +    const char *desc;
> +    const char *soc_name;
> +    uint32_t hw_strap1;
> +    const char *fmc_model;
> +    const char *spi_model;
> +    uint32_t num_cs;
> +    void (*i2c_init)(AspeedBoardState *bmc);
> +} AspeedBoardConfig;
> +
> +#define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
> +#define ASPEED_MACHINE(obj) \
> +    OBJECT_CHECK(AspeedMachine, (obj), TYPE_ASPEED_MACHINE)
> +
> +typedef struct AspeedMachine {
> +    MachineState parent_obj;
> +} AspeedMachine;
> +
> +#define ASPEED_MACHINE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(AspeedMachineClass, (klass), TYPE_ASPEED_MACHINE)
> +#define ASPEED_MACHINE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(AspeedMachineClass, (obj), TYPE_ASPEED_MACHINE)
> +
> +typedef struct AspeedMachineClass {
> +    MachineClass parent_obj;
> +    const AspeedBoardConfig *board;
> +} AspeedMachineClass;
> +
> +
> +#endif
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index f2d64e45511a..6b33ecd5aa43 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -15,6 +15,7 @@
>  #include "cpu.h"
>  #include "exec/address-spaces.h"
>  #include "hw/arm/arm.h"
> +#include "hw/arm/aspeed.h"
>  #include "hw/arm/aspeed_soc.h"
>  #include "hw/boards.h"
>  #include "hw/i2c/smbus.h"
> @@ -34,22 +35,6 @@ typedef struct AspeedBoardState {
>      MemoryRegion max_ram;
>  } AspeedBoardState;
>  
> -typedef struct AspeedBoardConfig {
> -    const char *soc_name;
> -    uint32_t hw_strap1;
> -    const char *fmc_model;
> -    const char *spi_model;
> -    uint32_t num_cs;
> -    void (*i2c_init)(AspeedBoardState *bmc);
> -} AspeedBoardConfig;
> -
> -enum {
> -    PALMETTO_BMC,
> -    AST2500_EVB,
> -    ROMULUS_BMC,
> -    WITHERSPOON_BMC,
> -};
> -
>  /* Palmetto hardware value: 0x120CE416 */
>  #define PALMETTO_BMC_HW_STRAP1 (                                        \
>          SCU_AST2400_HW_STRAP_DRAM_SIZE(DRAM_SIZE_256MB) |               \
> @@ -88,46 +73,6 @@ enum {
>  /* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
>  #define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
>  
> -static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
> -static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
> -static void romulus_bmc_i2c_init(AspeedBoardState *bmc);
> -static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
> -
> -static const AspeedBoardConfig aspeed_boards[] = {
> -    [PALMETTO_BMC] = {
> -        .soc_name  = "ast2400-a1",
> -        .hw_strap1 = PALMETTO_BMC_HW_STRAP1,
> -        .fmc_model = "n25q256a",
> -        .spi_model = "mx25l25635e",
> -        .num_cs    = 1,
> -        .i2c_init  = palmetto_bmc_i2c_init,
> -    },
> -    [AST2500_EVB]  = {
> -        .soc_name  = "ast2500-a1",
> -        .hw_strap1 = AST2500_EVB_HW_STRAP1,
> -        .fmc_model = "w25q256",
> -        .spi_model = "mx25l25635e",
> -        .num_cs    = 1,
> -        .i2c_init  = ast2500_evb_i2c_init,
> -    },
> -    [ROMULUS_BMC]  = {
> -        .soc_name  = "ast2500-a1",
> -        .hw_strap1 = ROMULUS_BMC_HW_STRAP1,
> -        .fmc_model = "n25q256a",
> -        .spi_model = "mx66l1g45g",
> -        .num_cs    = 2,
> -        .i2c_init  = romulus_bmc_i2c_init,
> -    },
> -    [WITHERSPOON_BMC]  = {
> -        .soc_name  = "ast2500-a1",
> -        .hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
> -        .fmc_model = "mx25l25635e",
> -        .spi_model = "mx66l1g45g",
> -        .num_cs    = 2,
> -        .i2c_init  = witherspoon_bmc_i2c_init,
> -    },
> -};
> -
>  /*
>   * The max ram region is for firmwares that scan the address space
>   * with load/store to guess how much RAM the SoC has.
> @@ -313,30 +258,6 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
>      object_property_set_int(OBJECT(dev), 110000, "temperature3", &error_abort);
>  }
>  
> -static void palmetto_bmc_init(MachineState *machine)
> -{
> -    aspeed_board_init(machine, &aspeed_boards[PALMETTO_BMC]);
> -}
> -
> -static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
> -{
> -    MachineClass *mc = MACHINE_CLASS(oc);
> -
> -    mc->desc = "OpenPOWER Palmetto BMC (ARM926EJ-S)";
> -    mc->init = palmetto_bmc_init;
> -    mc->max_cpus = 1;
> -    mc->no_sdcard = 1;
> -    mc->no_floppy = 1;
> -    mc->no_cdrom = 1;
> -    mc->no_parallel = 1;
> -}
> -
> -static const TypeInfo palmetto_bmc_type = {
> -    .name = MACHINE_TYPE_NAME("palmetto-bmc"),
> -    .parent = TYPE_MACHINE,
> -    .class_init = palmetto_bmc_class_init,
> -};
> -
>  static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
> @@ -353,30 +274,6 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
>  }
>  
> -static void ast2500_evb_init(MachineState *machine)
> -{
> -    aspeed_board_init(machine, &aspeed_boards[AST2500_EVB]);
> -}
> -
> -static void ast2500_evb_class_init(ObjectClass *oc, void *data)
> -{
> -    MachineClass *mc = MACHINE_CLASS(oc);
> -
> -    mc->desc = "Aspeed AST2500 EVB (ARM1176)";
> -    mc->init = ast2500_evb_init;
> -    mc->max_cpus = 1;
> -    mc->no_sdcard = 1;
> -    mc->no_floppy = 1;
> -    mc->no_cdrom = 1;
> -    mc->no_parallel = 1;
> -}
> -
> -static const TypeInfo ast2500_evb_type = {
> -    .name = MACHINE_TYPE_NAME("ast2500-evb"),
> -    .parent = TYPE_MACHINE,
> -    .class_init = ast2500_evb_class_init,
> -};
> -
>  static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
> @@ -386,30 +283,6 @@ static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
>  }
>  
> -static void romulus_bmc_init(MachineState *machine)
> -{
> -    aspeed_board_init(machine, &aspeed_boards[ROMULUS_BMC]);
> -}
> -
> -static void romulus_bmc_class_init(ObjectClass *oc, void *data)
> -{
> -    MachineClass *mc = MACHINE_CLASS(oc);
> -
> -    mc->desc = "OpenPOWER Romulus BMC (ARM1176)";
> -    mc->init = romulus_bmc_init;
> -    mc->max_cpus = 1;
> -    mc->no_sdcard = 1;
> -    mc->no_floppy = 1;
> -    mc->no_cdrom = 1;
> -    mc->no_parallel = 1;
> -}
> -
> -static const TypeInfo romulus_bmc_type = {
> -    .name = MACHINE_TYPE_NAME("romulus-bmc"),
> -    .parent = TYPE_MACHINE,
> -    .class_init = romulus_bmc_class_init,
> -};
> -
>  static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
> @@ -433,36 +306,91 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>                       0x60);
>  }
>  
> -static void witherspoon_bmc_init(MachineState *machine)
> +static void aspeed_machine_init(MachineState *machine)
>  {
> -    aspeed_board_init(machine, &aspeed_boards[WITHERSPOON_BMC]);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
> +
> +    aspeed_board_init(machine, amc->board);
>  }
>  
> -static void witherspoon_bmc_class_init(ObjectClass *oc, void *data)
> +static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> +    const AspeedBoardConfig *board = data;
>  
> -    mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)";
> -    mc->init = witherspoon_bmc_init;
> +    mc->desc = board->desc;
> +    mc->init = aspeed_machine_init;
>      mc->max_cpus = 1;
>      mc->no_sdcard = 1;
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
>      mc->no_parallel = 1;
> +    amc->board = board;
>  }
>  
> -static const TypeInfo witherspoon_bmc_type = {
> -    .name = MACHINE_TYPE_NAME("witherspoon-bmc"),
> +static const TypeInfo aspeed_machine_type = {
> +    .name = TYPE_ASPEED_MACHINE,
>      .parent = TYPE_MACHINE,
> -    .class_init = witherspoon_bmc_class_init,
> +    .instance_size = sizeof(AspeedMachine),
> +    .class_size = sizeof(AspeedMachineClass),
> +    .abstract = true,
> +};
> +
> +static const AspeedBoardConfig aspeed_boards[] = {
> +    {
> +        .name      = MACHINE_TYPE_NAME("palmetto-bmc"),
> +        .desc      = "OpenPOWER Palmetto BMC (ARM926EJ-S)",
> +        .soc_name  = "ast2400-a1",
> +        .hw_strap1 = PALMETTO_BMC_HW_STRAP1,
> +        .fmc_model = "n25q256a",
> +        .spi_model = "mx25l25635e",
> +        .num_cs    = 1,
> +        .i2c_init  = palmetto_bmc_i2c_init,
> +    }, {
> +        .name      = MACHINE_TYPE_NAME("ast2500-evb"),
> +        .desc      = "Aspeed AST2500 EVB (ARM1176)",
> +        .soc_name  = "ast2500-a1",
> +        .hw_strap1 = AST2500_EVB_HW_STRAP1,
> +        .fmc_model = "w25q256",
> +        .spi_model = "mx25l25635e",
> +        .num_cs    = 1,
> +        .i2c_init  = ast2500_evb_i2c_init,
> +    }, {
> +        .name      = MACHINE_TYPE_NAME("romulus-bmc"),
> +        .desc      = "OpenPOWER Romulus BMC (ARM1176)",
> +        .soc_name  = "ast2500-a1",
> +        .hw_strap1 = ROMULUS_BMC_HW_STRAP1,
> +        .fmc_model = "n25q256a",
> +        .spi_model = "mx66l1g45g",
> +        .num_cs    = 2,
> +        .i2c_init  = romulus_bmc_i2c_init,
> +    }, {
> +        .name      = MACHINE_TYPE_NAME("witherspoon-bmc"),
> +        .desc      = "OpenPOWER Witherspoon BMC (ARM1176)",
> +        .soc_name  = "ast2500-a1",
> +        .hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
> +        .fmc_model = "mx25l25635e",
> +        .spi_model = "mx66l1g45g",
> +        .num_cs    = 2,
> +        .i2c_init  = witherspoon_bmc_i2c_init,
> +    },
>  };
>  
> -static void aspeed_machine_init(void)
> +static void aspeed_machine_types(void)
>  {
> -    type_register_static(&palmetto_bmc_type);
> -    type_register_static(&ast2500_evb_type);
> -    type_register_static(&romulus_bmc_type);
> -    type_register_static(&witherspoon_bmc_type);
> +    int i;
> +
> +    type_register_static(&aspeed_machine_type);
> +    for (i = 0; i < ARRAY_SIZE(aspeed_boards); ++i) {
> +        TypeInfo ti = {
> +            .name       = aspeed_boards[i].name,
> +            .parent     = TYPE_ASPEED_MACHINE,
> +            .class_init = aspeed_machine_class_init,
> +            .class_data = (void *)&aspeed_boards[i],
> +        };
> +        type_register(&ti);
> +    }
>  }
>  
> -type_init(aspeed_machine_init)
> +type_init(aspeed_machine_types)
> 

Re: [Qemu-devel] [PATCH 06/11] aspeed/smc: fix default read value

[Cédric Le Goater]

On 09/08/2018 01:06 AM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 8/31/18 7:38 AM, Cédric Le Goater wrote:
>> 0xFFFFFFFF should be returned for non implemented registers.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ssi/aspeed_smc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 1270842dcf0c..6045ca11b969 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -665,12 +665,12 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>>          addr == s->r_ce_ctrl ||
>>          addr == R_INTR_CTRL ||
>>          (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
>> -        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
>> +        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
>
> This change seems unrelated to this commit.

The controller should have one control register for each flash device
if there is no CS available.

The problem is that s->num_cs is a QEMU software concept representing 
the number of flash devices handled by a board where as 'ctrl->max_slaves' 
is a HW limit representing the maximum number of flash devices a 
controller can handle.

Thanks,

C.
 
>>          return s->regs[addr];
>>      } else {
>>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>>                        __func__, addr);
>> -        return 0;
>> +        return -1;
>>      }
>>  }
>>  
>>