[Qemu-devel] [PATCH v2 00/21] aspeed: machine extensions and fixes

[Qemu-devel] [PATCH v2 00/21] aspeed: machine extensions and fixes

[Cédric Le Goater]

Hello,

This series improves the current models of the Aspeed machines in QEMU
and adds new ones. It also prepares ground for the future Aspeed SoC.
You will find patches for :

 - per SoC mappings of the memory space and the interrupt number space
 - support for multiple CPUs (improved)
 - support for multiple NICs
 - a RTC model from Joel
 - a basic XDMA model from Eddie
 - support for multiple CPUs and NICs
 - fixes for the irq and timer models from Joel, Andrew and Christian
 - DMA support for the SMC controller, which was reworked to use a RAM
   container region as suggested by Peter in September 2018
 - new swift-bmc machine from Adriana (P9' processor)


Thanks,

C.

Changes since v1:

 - reworked the CPU logic to put the number of CPUs under the SoC
 - introduced a "inject-failure" property as suggested by Philippe


Adriana Kobylak (1):
  aspeed: Add support for the swift-bmc board

Andrew Jeffery (4):
  aspeed/timer: Status register contains reload for stopped timer
  aspeed/timer: Fix match calculations
  aspeed/timer: Provide back-pressure information for short periods
  aspeed: vic: Add support for legacy register interface

Christian Svensson (2):
  aspeed/timer: Ensure positive muldiv delta
  aspeed/smc: Calculate checksum on normal DMA

Cédric Le Goater (10):
  aspeed: add a per SoC mapping for the interrupt space
  aspeed: add a per SoC mapping for the memory space
  aspeed: introduce a configurable number of CPU per machine
  aspeed: add support for multiple NICs
  aspeed: remove the "ram" link
  aspeed: add a RAM memory region container
  aspeed/smc: add a 'sdram_base' property
  aspeed/smc: add support for DMAs
  aspeed/smc: add DMA calibration settings
  aspeed/smc: inject errors in DMA checksum

Eddie James (1):
  hw/misc/aspeed_xdma: New device

Joel Stanley (3):
  hw: timer: Add ASPEED RTC device
  hw/arm/aspeed: Add RTC to SoC
  aspeed/timer: Fix behaviour running Linux

 include/hw/arm/aspeed_soc.h     |  53 ++++-
 include/hw/misc/aspeed_xdma.h   |  30 +++
 include/hw/ssi/aspeed_smc.h     |  10 +
 include/hw/timer/aspeed_rtc.h   |  31 +++
 include/hw/timer/aspeed_timer.h |   1 +
 hw/arm/aspeed.c                 |  78 +++++++-
 hw/arm/aspeed_soc.c             | 268 +++++++++++++++++++-------
 hw/intc/aspeed_vic.c            | 105 ++++++----
 hw/misc/aspeed_scu.c            |   6 +
 hw/misc/aspeed_xdma.c           | 165 ++++++++++++++++
 hw/ssi/aspeed_smc.c             | 330 +++++++++++++++++++++++++++++++-
 hw/timer/aspeed_rtc.c           | 180 +++++++++++++++++
 hw/timer/aspeed_timer.c         |  84 +++++---
 hw/misc/Makefile.objs           |   1 +
 hw/misc/trace-events            |   3 +
 hw/timer/Makefile.objs          |   2 +-
 hw/timer/trace-events           |   4 +
 17 files changed, 1188 insertions(+), 163 deletions(-)
 create mode 100644 include/hw/misc/aspeed_xdma.h
 create mode 100644 include/hw/timer/aspeed_rtc.h
 create mode 100644 hw/misc/aspeed_xdma.c
 create mode 100644 hw/timer/aspeed_rtc.c

-- 
2.21.0

[Qemu-devel] [PATCH v2 01/21] aspeed: add a per SoC mapping for the interrupt space

[Cédric Le Goater]

This will simplify the definition of new SoCs, like the AST2600 which
should use a different CPU and a different IRQ number layout.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 include/hw/arm/aspeed_soc.h | 36 +++++++++++++++++++++++
 hw/arm/aspeed_soc.c         | 57 +++++++++++++++++++++++++++++++------
 2 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 836b2ba8bf15..963abecb7244 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -56,6 +56,7 @@ typedef struct AspeedSoCInfo {
     const char *fmc_typename;
     const char **spi_typename;
     int wdts_num;
+    const int *irqmap;
 } AspeedSoCInfo;
 
 typedef struct AspeedSoCClass {
@@ -68,4 +69,39 @@ typedef struct AspeedSoCClass {
 #define ASPEED_SOC_GET_CLASS(obj)                               \
     OBJECT_GET_CLASS(AspeedSoCClass, (obj), TYPE_ASPEED_SOC)
 
+enum {
+    ASPEED_IOMEM,
+    ASPEED_UART1,
+    ASPEED_UART2,
+    ASPEED_UART3,
+    ASPEED_UART4,
+    ASPEED_UART5,
+    ASPEED_VUART,
+    ASPEED_FMC,
+    ASPEED_SPI1,
+    ASPEED_SPI2,
+    ASPEED_VIC,
+    ASPEED_SDMC,
+    ASPEED_SCU,
+    ASPEED_ADC,
+    ASPEED_SRAM,
+    ASPEED_GPIO,
+    ASPEED_RTC,
+    ASPEED_TIMER1,
+    ASPEED_TIMER2,
+    ASPEED_TIMER3,
+    ASPEED_TIMER4,
+    ASPEED_TIMER5,
+    ASPEED_TIMER6,
+    ASPEED_TIMER7,
+    ASPEED_TIMER8,
+    ASPEED_WDT,
+    ASPEED_PWM,
+    ASPEED_LPC,
+    ASPEED_IBT,
+    ASPEED_I2C,
+    ASPEED_ETH1,
+    ASPEED_ETH2,
+};
+
 #endif /* ASPEED_SOC_H */
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index a2ea8c748449..de75bf04027d 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -38,12 +38,42 @@
 #define ASPEED_SOC_ETH1_BASE        0x1E660000
 #define ASPEED_SOC_ETH2_BASE        0x1E680000
 
-static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
-static const int timer_irqs[] = { 16, 17, 18, 35, 36, 37, 38, 39, };
+static const int aspeed_soc_ast2400_irqmap[] = {
+    [ASPEED_UART1]  = 9,
+    [ASPEED_UART2]  = 32,
+    [ASPEED_UART3]  = 33,
+    [ASPEED_UART4]  = 34,
+    [ASPEED_UART5]  = 10,
+    [ASPEED_VUART]  = 8,
+    [ASPEED_FMC]    = 19,
+    [ASPEED_SDMC]   = 0,
+    [ASPEED_SCU]    = 21,
+    [ASPEED_ADC]    = 31,
+    [ASPEED_GPIO]   = 20,
+    [ASPEED_RTC]    = 22,
+    [ASPEED_TIMER1] = 16,
+    [ASPEED_TIMER2] = 17,
+    [ASPEED_TIMER3] = 18,
+    [ASPEED_TIMER4] = 35,
+    [ASPEED_TIMER5] = 36,
+    [ASPEED_TIMER6] = 37,
+    [ASPEED_TIMER7] = 38,
+    [ASPEED_TIMER8] = 39,
+    [ASPEED_WDT]    = 27,
+    [ASPEED_PWM]    = 28,
+    [ASPEED_LPC]    = 8,
+    [ASPEED_IBT]    = 8, /* LPC */
+    [ASPEED_I2C]    = 12,
+    [ASPEED_ETH1]   = 2,
+    [ASPEED_ETH2]   = 3,
+};
 
 #define AST2400_SDRAM_BASE       0x40000000
 #define AST2500_SDRAM_BASE       0x80000000
 
+/* AST2500 uses the same IRQs as the AST2400 */
+#define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
+
 static const hwaddr aspeed_soc_ast2400_spi_bases[] = { ASPEED_SOC_SPI_BASE };
 static const char *aspeed_soc_ast2400_typenames[] = { "aspeed.smc.spi" };
 
@@ -64,6 +94,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
         .wdts_num     = 2,
+        .irqmap       = aspeed_soc_ast2400_irqmap,
     }, {
         .name         = "ast2400-a1",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
@@ -75,6 +106,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
         .wdts_num     = 2,
+        .irqmap       = aspeed_soc_ast2400_irqmap,
     }, {
         .name         = "ast2400",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
@@ -86,6 +118,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
         .wdts_num     = 2,
+        .irqmap       = aspeed_soc_ast2400_irqmap,
     }, {
         .name         = "ast2500-a1",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm1176"),
@@ -97,9 +130,17 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .fmc_typename = "aspeed.smc.ast2500-fmc",
         .spi_typename = aspeed_soc_ast2500_typenames,
         .wdts_num     = 3,
+        .irqmap       = aspeed_soc_ast2500_irqmap,
     },
 };
 
+static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
+{
+    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
+
+    return qdev_get_gpio_in(DEVICE(&s->vic), sc->info->irqmap[ctrl]);
+}
+
 static void aspeed_soc_init(Object *obj)
 {
     AspeedSoCState *s = ASPEED_SOC(obj);
@@ -216,14 +257,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         return;
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->timerctrl), 0, ASPEED_SOC_TIMER_BASE);
-    for (i = 0; i < ARRAY_SIZE(timer_irqs); i++) {
-        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->vic), timer_irqs[i]);
+    for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+        qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_TIMER1 + i);
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
     /* UART - attach an 8250 to the IO space as our UART5 */
     if (serial_hd(0)) {
-        qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
+        qemu_irq uart5 = aspeed_soc_get_irq(s, ASPEED_UART5);
         serial_mm_init(get_system_memory(),
                        ASPEED_SOC_IOMEM_BASE + ASPEED_SOC_UART_5_BASE, 2,
                        uart5, 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
@@ -237,7 +278,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, ASPEED_SOC_I2C_BASE);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c), 0,
-                       qdev_get_gpio_in(DEVICE(&s->vic), 12));
+                       aspeed_soc_get_irq(s, ASPEED_I2C));
 
     /* FMC, The number of CS is set at the board level */
     object_property_set_bool(OBJECT(&s->fmc), true, "realized", &err);
@@ -249,7 +290,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 1,
                     s->fmc.ctrl->flash_window_base);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->fmc), 0,
-                       qdev_get_gpio_in(DEVICE(&s->vic), 19));
+                       aspeed_soc_get_irq(s, ASPEED_FMC));
 
     /* SPI */
     for (i = 0; i < sc->info->spis_num; i++) {
@@ -297,7 +338,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100), 0, ASPEED_SOC_ETH1_BASE);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
-                       qdev_get_gpio_in(DEVICE(&s->vic), 2));
+                       aspeed_soc_get_irq(s, ASPEED_ETH1));
 }
 
 static void aspeed_soc_class_init(ObjectClass *oc, void *data)
-- 
2.21.0

[Qemu-devel] [PATCH v2 02/21] aspeed: add a per SoC mapping for the memory space

[Cédric Le Goater]

This will simplify the definition of new SoCs, like the AST2600 which
should use a slightly different address space and have a different set
of controllers.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 include/hw/arm/aspeed_soc.h |   4 +-
 hw/arm/aspeed.c             |   8 +--
 hw/arm/aspeed_soc.c         | 117 ++++++++++++++++++++++--------------
 3 files changed, 78 insertions(+), 51 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 963abecb7244..88b901d5dfa9 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -49,14 +49,13 @@ typedef struct AspeedSoCInfo {
     const char *name;
     const char *cpu_type;
     uint32_t silicon_rev;
-    hwaddr sdram_base;
     uint64_t sram_size;
     int spis_num;
-    const hwaddr *spi_bases;
     const char *fmc_typename;
     const char **spi_typename;
     int wdts_num;
     const int *irqmap;
+    const hwaddr *memmap;
 } AspeedSoCInfo;
 
 typedef struct AspeedSoCClass {
@@ -102,6 +101,7 @@ enum {
     ASPEED_I2C,
     ASPEED_ETH1,
     ASPEED_ETH2,
+    ASPEED_SDRAM,
 };
 
 #endif /* ASPEED_SOC_H */
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index d2ad2da24b50..c692ca1dba90 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -191,8 +191,8 @@ static void aspeed_board_init(MachineState *machine,
                                         &error_abort);
 
     memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
-    memory_region_add_subregion(get_system_memory(), sc->info->sdram_base,
-                                &bmc->ram);
+    memory_region_add_subregion(get_system_memory(),
+                                sc->info->memmap[ASPEED_SDRAM], &bmc->ram);
     object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
                                    &error_abort);
 
@@ -201,7 +201,7 @@ static void aspeed_board_init(MachineState *machine,
     memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
                           "max_ram", max_ram_size  - ram_size);
     memory_region_add_subregion(get_system_memory(),
-                                sc->info->sdram_base + ram_size,
+                                sc->info->memmap[ASPEED_SDRAM] + ram_size,
                                 &bmc->max_ram);
 
     aspeed_board_init_flashes(&bmc->soc.fmc, cfg->fmc_model, &error_abort);
@@ -229,7 +229,7 @@ static void aspeed_board_init(MachineState *machine,
     aspeed_board_binfo.initrd_filename = machine->initrd_filename;
     aspeed_board_binfo.kernel_cmdline = machine->kernel_cmdline;
     aspeed_board_binfo.ram_size = ram_size;
-    aspeed_board_binfo.loader_start = sc->info->sdram_base;
+    aspeed_board_binfo.loader_start = sc->info->memmap[ASPEED_SDRAM];
 
     if (cfg->i2c_init) {
         cfg->i2c_init(bmc);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index de75bf04027d..1cc98b9f4045 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -22,21 +22,58 @@
 #include "hw/i2c/aspeed_i2c.h"
 #include "net/net.h"
 
-#define ASPEED_SOC_UART_5_BASE      0x00184000
 #define ASPEED_SOC_IOMEM_SIZE       0x00200000
-#define ASPEED_SOC_IOMEM_BASE       0x1E600000
-#define ASPEED_SOC_FMC_BASE         0x1E620000
-#define ASPEED_SOC_SPI_BASE         0x1E630000
-#define ASPEED_SOC_SPI2_BASE        0x1E631000
-#define ASPEED_SOC_VIC_BASE         0x1E6C0000
-#define ASPEED_SOC_SDMC_BASE        0x1E6E0000
-#define ASPEED_SOC_SCU_BASE         0x1E6E2000
-#define ASPEED_SOC_SRAM_BASE        0x1E720000
-#define ASPEED_SOC_TIMER_BASE       0x1E782000
-#define ASPEED_SOC_WDT_BASE         0x1E785000
-#define ASPEED_SOC_I2C_BASE         0x1E78A000
-#define ASPEED_SOC_ETH1_BASE        0x1E660000
-#define ASPEED_SOC_ETH2_BASE        0x1E680000
+
+static const hwaddr aspeed_soc_ast2400_memmap[] = {
+    [ASPEED_IOMEM]  = 0x1E600000,
+    [ASPEED_FMC]    = 0x1E620000,
+    [ASPEED_SPI1]   = 0x1E630000,
+    [ASPEED_VIC]    = 0x1E6C0000,
+    [ASPEED_SDMC]   = 0x1E6E0000,
+    [ASPEED_SCU]    = 0x1E6E2000,
+    [ASPEED_ADC]    = 0x1E6E9000,
+    [ASPEED_SRAM]   = 0x1E720000,
+    [ASPEED_GPIO]   = 0x1E780000,
+    [ASPEED_RTC]    = 0x1E781000,
+    [ASPEED_TIMER1] = 0x1E782000,
+    [ASPEED_WDT]    = 0x1E785000,
+    [ASPEED_PWM]    = 0x1E786000,
+    [ASPEED_LPC]    = 0x1E789000,
+    [ASPEED_IBT]    = 0x1E789140,
+    [ASPEED_I2C]    = 0x1E78A000,
+    [ASPEED_ETH1]   = 0x1E660000,
+    [ASPEED_ETH2]   = 0x1E680000,
+    [ASPEED_UART1]  = 0x1E783000,
+    [ASPEED_UART5]  = 0x1E784000,
+    [ASPEED_VUART]  = 0x1E787000,
+    [ASPEED_SDRAM]  = 0x40000000,
+};
+
+static const hwaddr aspeed_soc_ast2500_memmap[] = {
+    [ASPEED_IOMEM]  = 0x1E600000,
+    [ASPEED_FMC]    = 0x1E620000,
+    [ASPEED_SPI1]   = 0x1E630000,
+    [ASPEED_SPI2]   = 0x1E631000,
+    [ASPEED_VIC]    = 0x1E6C0000,
+    [ASPEED_SDMC]   = 0x1E6E0000,
+    [ASPEED_SCU]    = 0x1E6E2000,
+    [ASPEED_ADC]    = 0x1E6E9000,
+    [ASPEED_SRAM]   = 0x1E720000,
+    [ASPEED_GPIO]   = 0x1E780000,
+    [ASPEED_RTC]    = 0x1E781000,
+    [ASPEED_TIMER1] = 0x1E782000,
+    [ASPEED_WDT]    = 0x1E785000,
+    [ASPEED_PWM]    = 0x1E786000,
+    [ASPEED_LPC]    = 0x1E789000,
+    [ASPEED_IBT]    = 0x1E789140,
+    [ASPEED_I2C]    = 0x1E78A000,
+    [ASPEED_ETH1]   = 0x1E660000,
+    [ASPEED_ETH2]   = 0x1E680000,
+    [ASPEED_UART1]  = 0x1E783000,
+    [ASPEED_UART5]  = 0x1E784000,
+    [ASPEED_VUART]  = 0x1E787000,
+    [ASPEED_SDRAM]  = 0x80000000,
+};
 
 static const int aspeed_soc_ast2400_irqmap[] = {
     [ASPEED_UART1]  = 9,
@@ -68,17 +105,9 @@ static const int aspeed_soc_ast2400_irqmap[] = {
     [ASPEED_ETH2]   = 3,
 };
 
-#define AST2400_SDRAM_BASE       0x40000000
-#define AST2500_SDRAM_BASE       0x80000000
-
-/* AST2500 uses the same IRQs as the AST2400 */
 #define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
 
-static const hwaddr aspeed_soc_ast2400_spi_bases[] = { ASPEED_SOC_SPI_BASE };
 static const char *aspeed_soc_ast2400_typenames[] = { "aspeed.smc.spi" };
-
-static const hwaddr aspeed_soc_ast2500_spi_bases[] = { ASPEED_SOC_SPI_BASE,
-                                                       ASPEED_SOC_SPI2_BASE};
 static const char *aspeed_soc_ast2500_typenames[] = {
     "aspeed.smc.ast2500-spi1", "aspeed.smc.ast2500-spi2" };
 
@@ -87,50 +116,46 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .name         = "ast2400-a0",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
         .silicon_rev  = AST2400_A0_SILICON_REV,
-        .sdram_base   = AST2400_SDRAM_BASE,
         .sram_size    = 0x8000,
         .spis_num     = 1,
-        .spi_bases    = aspeed_soc_ast2400_spi_bases,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
+        .memmap       = aspeed_soc_ast2400_memmap,
     }, {
         .name         = "ast2400-a1",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
         .silicon_rev  = AST2400_A1_SILICON_REV,
-        .sdram_base   = AST2400_SDRAM_BASE,
         .sram_size    = 0x8000,
         .spis_num     = 1,
-        .spi_bases    = aspeed_soc_ast2400_spi_bases,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
+        .memmap       = aspeed_soc_ast2400_memmap,
     }, {
         .name         = "ast2400",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
         .silicon_rev  = AST2400_A0_SILICON_REV,
-        .sdram_base   = AST2400_SDRAM_BASE,
         .sram_size    = 0x8000,
         .spis_num     = 1,
-        .spi_bases    = aspeed_soc_ast2400_spi_bases,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
+        .memmap       = aspeed_soc_ast2400_memmap,
     }, {
         .name         = "ast2500-a1",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm1176"),
         .silicon_rev  = AST2500_A1_SILICON_REV,
-        .sdram_base   = AST2500_SDRAM_BASE,
         .sram_size    = 0x9000,
         .spis_num     = 2,
-        .spi_bases    = aspeed_soc_ast2500_spi_bases,
         .fmc_typename = "aspeed.smc.ast2500-fmc",
         .spi_typename = aspeed_soc_ast2500_typenames,
         .wdts_num     = 3,
         .irqmap       = aspeed_soc_ast2500_irqmap,
+        .memmap       = aspeed_soc_ast2500_memmap,
     },
 };
 
@@ -210,8 +235,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     Error *err = NULL, *local_err = NULL;
 
     /* IO space */
-    create_unimplemented_device("aspeed_soc.io",
-                                ASPEED_SOC_IOMEM_BASE, ASPEED_SOC_IOMEM_SIZE);
+    create_unimplemented_device("aspeed_soc.io", sc->info->memmap[ASPEED_IOMEM],
+                                ASPEED_SOC_IOMEM_SIZE);
 
     /* CPU */
     object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
@@ -227,8 +252,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    memory_region_add_subregion(get_system_memory(), ASPEED_SOC_SRAM_BASE,
-                                &s->sram);
+    memory_region_add_subregion(get_system_memory(),
+                                sc->info->memmap[ASPEED_SRAM], &s->sram);
 
     /* SCU */
     object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
@@ -236,7 +261,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, ASPEED_SOC_SCU_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, sc->info->memmap[ASPEED_SCU]);
 
     /* VIC */
     object_property_set_bool(OBJECT(&s->vic), true, "realized", &err);
@@ -244,7 +269,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->vic), 0, ASPEED_SOC_VIC_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->vic), 0, sc->info->memmap[ASPEED_VIC]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 0,
                        qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_IRQ));
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 1,
@@ -256,7 +281,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->timerctrl), 0, ASPEED_SOC_TIMER_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->timerctrl), 0,
+                    sc->info->memmap[ASPEED_TIMER1]);
     for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
         qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_TIMER1 + i);
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
@@ -265,8 +291,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     /* UART - attach an 8250 to the IO space as our UART5 */
     if (serial_hd(0)) {
         qemu_irq uart5 = aspeed_soc_get_irq(s, ASPEED_UART5);
-        serial_mm_init(get_system_memory(),
-                       ASPEED_SOC_IOMEM_BASE + ASPEED_SOC_UART_5_BASE, 2,
+        serial_mm_init(get_system_memory(), sc->info->memmap[ASPEED_UART5], 2,
                        uart5, 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
     }
 
@@ -276,7 +301,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, ASPEED_SOC_I2C_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, sc->info->memmap[ASPEED_I2C]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c), 0,
                        aspeed_soc_get_irq(s, ASPEED_I2C));
 
@@ -286,7 +311,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 0, ASPEED_SOC_FMC_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 0, sc->info->memmap[ASPEED_FMC]);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 1,
                     s->fmc.ctrl->flash_window_base);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->fmc), 0,
@@ -302,7 +327,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
             error_propagate(errp, err);
             return;
         }
-        sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0, sc->info->spi_bases[i]);
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0,
+                        sc->info->memmap[ASPEED_SPI1 + i]);
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 1,
                         s->spi[i].ctrl->flash_window_base);
     }
@@ -313,7 +339,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, ASPEED_SOC_SDMC_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, sc->info->memmap[ASPEED_SDMC]);
 
     /* Watch dog */
     for (i = 0; i < sc->info->wdts_num; i++) {
@@ -323,7 +349,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
             return;
         }
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0,
-                        ASPEED_SOC_WDT_BASE + i * 0x20);
+                        sc->info->memmap[ASPEED_WDT] + i * 0x20);
     }
 
     /* Net */
@@ -336,7 +362,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100), 0, ASPEED_SOC_ETH1_BASE);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100), 0,
+                    sc->info->memmap[ASPEED_ETH1]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
                        aspeed_soc_get_irq(s, ASPEED_ETH1));
 }
-- 
2.21.0

[Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device

[Cédric Le Goater]

From: Joel Stanley <joel@jms.id.au>

The RTC is modeled to provide time and date functionality. It is
initialised at zero to match the hardware.

There is no modelling of the alarm functionality, which includes the IRQ
line. As there is no guest code to exercise this function that is
acceptable for now.

Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/timer/aspeed_rtc.h |  31 ++++++
 hw/timer/aspeed_rtc.c         | 180 ++++++++++++++++++++++++++++++++++
 hw/timer/Makefile.objs        |   2 +-
 hw/timer/trace-events         |   4 +
 4 files changed, 216 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/timer/aspeed_rtc.h
 create mode 100644 hw/timer/aspeed_rtc.c

diff --git a/include/hw/timer/aspeed_rtc.h b/include/hw/timer/aspeed_rtc.h
new file mode 100644
index 000000000000..1f1155a676c1
--- /dev/null
+++ b/include/hw/timer/aspeed_rtc.h
@@ -0,0 +1,31 @@
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef ASPEED_RTC_H
+#define ASPEED_RTC_H
+
+#include <stdint.h>
+
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+
+typedef struct AspeedRtcState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t reg[0x18];
+    int offset;
+
+} AspeedRtcState;
+
+#define TYPE_ASPEED_RTC "aspeed.rtc"
+#define ASPEED_RTC(obj) OBJECT_CHECK(AspeedRtcState, (obj), TYPE_ASPEED_RTC)
+
+#endif /* ASPEED_RTC_H */
diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c
new file mode 100644
index 000000000000..19f061c846e8
--- /dev/null
+++ b/hw/timer/aspeed_rtc.c
@@ -0,0 +1,180 @@
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/timer/aspeed_rtc.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+
+#include "trace.h"
+
+#define COUNTER1        (0x00 / 4)
+#define COUNTER2        (0x04 / 4)
+#define ALARM           (0x08 / 4)
+#define CONTROL         (0x10 / 4)
+#define ALARM_STATUS    (0x14 / 4)
+
+#define RTC_UNLOCKED    BIT(1)
+#define RTC_ENABLED     BIT(0)
+
+static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
+{
+    struct tm tm;
+    uint32_t year, cent;
+    uint32_t reg1 = rtc->reg[COUNTER1];
+    uint32_t reg2 = rtc->reg[COUNTER2];
+
+    tm.tm_mday = (reg1 >> 24) & 0x1f;
+    tm.tm_hour = (reg1 >> 16) & 0x1f;
+    tm.tm_min = (reg1 >> 8) & 0x3f;
+    tm.tm_sec = (reg1 >> 0) & 0x3f;
+
+    cent = (reg2 >> 16) & 0x1f;
+    year = (reg2 >> 8) & 0x7f;
+    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
+    tm.tm_year = year + (cent * 100) - 1900;
+
+    rtc->offset = qemu_timedate_diff(&tm);
+}
+
+static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)
+{
+    uint32_t year, cent;
+    struct tm now;
+
+    qemu_get_timedate(&now, rtc->offset);
+
+    switch (r) {
+    case COUNTER1:
+        return (now.tm_mday << 24) | (now.tm_hour << 16) |
+            (now.tm_min << 8) | now.tm_sec;
+    case COUNTER2:
+        cent = (now.tm_year + 1900) / 100;
+        year = now.tm_year % 100;
+        return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |
+            ((now.tm_mon + 1) & 0xf);
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,
+                                unsigned size)
+{
+    AspeedRtcState *rtc = opaque;
+    uint64_t val;
+    uint32_t r = addr >> 2;
+
+    switch (r) {
+    case COUNTER1:
+    case COUNTER2:
+        if (rtc->reg[CONTROL] & RTC_ENABLED) {
+            rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);
+        }
+        /* fall through */
+    case CONTROL:
+        val = rtc->reg[r];
+        break;
+    case ALARM:
+    case ALARM_STATUS:
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+        return 0;
+    }
+
+    trace_aspeed_rtc_read(addr, val);
+
+    return val;
+}
+
+static void aspeed_rtc_write(void *opaque, hwaddr addr,
+                             uint64_t val, unsigned size)
+{
+    AspeedRtcState *rtc = opaque;
+    uint32_t r = addr >> 2;
+
+    switch (r) {
+    case COUNTER1:
+    case COUNTER2:
+        if (!(rtc->reg[CONTROL] & RTC_UNLOCKED)) {
+            break;
+        }
+        /* fall through */
+    case CONTROL:
+        rtc->reg[r] = val;
+        aspeed_rtc_calc_offset(rtc);
+        break;
+    case ALARM:
+    case ALARM_STATUS:
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+        break;
+    }
+    trace_aspeed_rtc_write(addr, val);
+}
+
+static void aspeed_rtc_reset(DeviceState *d)
+{
+    AspeedRtcState *rtc = ASPEED_RTC(d);
+
+    rtc->offset = 0;
+    memset(rtc->reg, 0, sizeof(rtc->reg));
+}
+
+static const MemoryRegionOps aspeed_rtc_ops = {
+    .read = aspeed_rtc_read,
+    .write = aspeed_rtc_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription vmstate_aspeed_rtc = {
+    .name = TYPE_ASPEED_RTC,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(reg, AspeedRtcState, 0x18),
+        VMSTATE_INT32(offset, AspeedRtcState),
+        VMSTATE_INT32(offset, AspeedRtcState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void aspeed_rtc_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedRtcState *s = ASPEED_RTC(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_rtc_ops, s,
+                          "aspeed-rtc", 0x18ULL);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void aspeed_rtc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aspeed_rtc_realize;
+    dc->vmsd = &vmstate_aspeed_rtc;
+    dc->reset = aspeed_rtc_reset;
+}
+
+static const TypeInfo aspeed_rtc_info = {
+    .name          = TYPE_ASPEED_RTC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedRtcState),
+    .class_init    = aspeed_rtc_class_init,
+};
+
+static void aspeed_rtc_register_types(void)
+{
+    type_register_static(&aspeed_rtc_info);
+}
+
+type_init(aspeed_rtc_register_types)
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 0e9a4530f848..123d92c9692c 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -41,7 +41,7 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
 obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
 
 common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
-common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
+common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o
 
 common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o
 common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index dcaf3d6da6c8..db02a9142cda 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -66,6 +66,10 @@ cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK A
 cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
 
+# hw/timer/aspeed-rtc.c
+aspeed_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
+aspeed_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
+
 # sun4v-rtc.c
 sun4v_rtc_read(uint64_t addr, uint64_t value) "read: addr 0x%" PRIx64 " value 0x%" PRIx64
 sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 0x%" PRIx64 " value 0x%" PRIx64
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device

[Peter Maydell]

On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Joel Stanley <joel@jms.id.au>
>
> The RTC is modeled to provide time and date functionality. It is
> initialised at zero to match the hardware.
>
> There is no modelling of the alarm functionality, which includes the IRQ
> line. As there is no guest code to exercise this function that is
> acceptable for now.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Hi; Coverity complains about this function (CID 1402782):

> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> +{
> +    struct tm tm;
> +    uint32_t year, cent;
> +    uint32_t reg1 = rtc->reg[COUNTER1];
> +    uint32_t reg2 = rtc->reg[COUNTER2];
> +
> +    tm.tm_mday = (reg1 >> 24) & 0x1f;
> +    tm.tm_hour = (reg1 >> 16) & 0x1f;
> +    tm.tm_min = (reg1 >> 8) & 0x3f;
> +    tm.tm_sec = (reg1 >> 0) & 0x3f;
> +
> +    cent = (reg2 >> 16) & 0x1f;
> +    year = (reg2 >> 8) & 0x7f;
> +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
> +    tm.tm_year = year + (cent * 100) - 1900;
> +
> +    rtc->offset = qemu_timedate_diff(&tm);
> +}

because the tm_wday field of 'struct tm tm' is not initialized
before we call qemu_timedate_diff(). This is a false
positive because the "read" of this field is just the place
in qemu_timedate_diff() that does "struct tm tmp = *tm;"
before calling mktime(), and mktime() ignores tm_wday.
We could make Coverity happy by using a struct initializer
on 'tm' here; on the other hand we don't do that anywhere else
which calls qemu_timedate_diff(), so maybe I should just mark
this a false positive?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device

[Joel Stanley]

On Tue, 2 Jul 2019 at 19:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:
> >
> > From: Joel Stanley <joel@jms.id.au>
> >
> > The RTC is modeled to provide time and date functionality. It is
> > initialised at zero to match the hardware.
> >
> > There is no modelling of the alarm functionality, which includes the IRQ
> > line. As there is no guest code to exercise this function that is
> > acceptable for now.
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Hi; Coverity complains about this function (CID 1402782):
>
> > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> > +{
> > +    struct tm tm;
> > +    uint32_t year, cent;
> > +    uint32_t reg1 = rtc->reg[COUNTER1];
> > +    uint32_t reg2 = rtc->reg[COUNTER2];
> > +
> > +    tm.tm_mday = (reg1 >> 24) & 0x1f;
> > +    tm.tm_hour = (reg1 >> 16) & 0x1f;
> > +    tm.tm_min = (reg1 >> 8) & 0x3f;
> > +    tm.tm_sec = (reg1 >> 0) & 0x3f;
> > +
> > +    cent = (reg2 >> 16) & 0x1f;
> > +    year = (reg2 >> 8) & 0x7f;
> > +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
> > +    tm.tm_year = year + (cent * 100) - 1900;
> > +
> > +    rtc->offset = qemu_timedate_diff(&tm);
> > +}
>
> because the tm_wday field of 'struct tm tm' is not initialized
> before we call qemu_timedate_diff(). This is a false
> positive because the "read" of this field is just the place
> in qemu_timedate_diff() that does "struct tm tmp = *tm;"
> before calling mktime(), and mktime() ignores tm_wday.
> We could make Coverity happy by using a struct initializer
> on 'tm' here; on the other hand we don't do that anywhere else
> which calls qemu_timedate_diff(), so maybe I should just mark
> this a false positive?

I don't have an opinion on which option to take. Perhaps mark it as a
false positive?

Cheers,

Joel

Re: [Qemu-devel] [PATCH v2 03/21] hw: timer: Add ASPEED RTC device

[Peter Maydell]

On Thu, 4 Jul 2019 at 08:49, Joel Stanley <joel@jms.id.au> wrote:
>
> On Tue, 2 Jul 2019 at 19:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:
> > >
> > > From: Joel Stanley <joel@jms.id.au>
> > >
> > > The RTC is modeled to provide time and date functionality. It is
> > > initialised at zero to match the hardware.
> > >
> > > There is no modelling of the alarm functionality, which includes the IRQ
> > > line. As there is no guest code to exercise this function that is
> > > acceptable for now.
> > >
> > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > Hi; Coverity complains about this function (CID 1402782):
> >
> > > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
> > > +{
> > > +    struct tm tm;
> > > +    uint32_t year, cent;
> > > +    uint32_t reg1 = rtc->reg[COUNTER1];
> > > +    uint32_t reg2 = rtc->reg[COUNTER2];
> > > +
> > > +    tm.tm_mday = (reg1 >> 24) & 0x1f;
> > > +    tm.tm_hour = (reg1 >> 16) & 0x1f;
> > > +    tm.tm_min = (reg1 >> 8) & 0x3f;
> > > +    tm.tm_sec = (reg1 >> 0) & 0x3f;
> > > +
> > > +    cent = (reg2 >> 16) & 0x1f;
> > > +    year = (reg2 >> 8) & 0x7f;
> > > +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
> > > +    tm.tm_year = year + (cent * 100) - 1900;
> > > +
> > > +    rtc->offset = qemu_timedate_diff(&tm);
> > > +}
> >
> > because the tm_wday field of 'struct tm tm' is not initialized
> > before we call qemu_timedate_diff(). This is a false
> > positive because the "read" of this field is just the place
> > in qemu_timedate_diff() that does "struct tm tmp = *tm;"
> > before calling mktime(), and mktime() ignores tm_wday.
> > We could make Coverity happy by using a struct initializer
> > on 'tm' here; on the other hand we don't do that anywhere else
> > which calls qemu_timedate_diff(), so maybe I should just mark
> > this a false positive?
>
> I don't have an opinion on which option to take. Perhaps mark it as a
> false positive?

Yeah, seems reasonable.

-- PMM

[Qemu-devel] [PATCH v2 04/21] hw/arm/aspeed: Add RTC to SoC

[Cédric Le Goater]

From: Joel Stanley <joel@jms.id.au>

All systems have an RTC.

The IRQ is hooked up but the model does not use it at this stage. There
is no guest code that uses it, so this limitation is acceptable.

Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/aspeed_soc.h |  2 ++
 hw/arm/aspeed_soc.c         | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 88b901d5dfa9..fa0ba957a611 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -16,6 +16,7 @@
 #include "hw/misc/aspeed_scu.h"
 #include "hw/misc/aspeed_sdmc.h"
 #include "hw/timer/aspeed_timer.h"
+#include "hw/timer/aspeed_rtc.h"
 #include "hw/i2c/aspeed_i2c.h"
 #include "hw/ssi/aspeed_smc.h"
 #include "hw/watchdog/wdt_aspeed.h"
@@ -32,6 +33,7 @@ typedef struct AspeedSoCState {
     ARMCPU cpu;
     MemoryRegion sram;
     AspeedVICState vic;
+    AspeedRtcState rtc;
     AspeedTimerCtrlState timerctrl;
     AspeedI2CState i2c;
     AspeedSCUState scu;
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 1cc98b9f4045..5faa78d81fd6 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -189,6 +189,9 @@ static void aspeed_soc_init(Object *obj)
     sysbus_init_child_obj(obj, "vic", OBJECT(&s->vic), sizeof(s->vic),
                           TYPE_ASPEED_VIC);
 
+    sysbus_init_child_obj(obj, "rtc", OBJECT(&s->rtc), sizeof(s->rtc),
+                          TYPE_ASPEED_RTC);
+
     sysbus_init_child_obj(obj, "timerctrl", OBJECT(&s->timerctrl),
                           sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
     object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
@@ -275,6 +278,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 1,
                        qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_FIQ));
 
+    /* RTC */
+    object_property_set_bool(OBJECT(&s->rtc), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->rtc), 0, sc->info->memmap[ASPEED_RTC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_RTC));
+
     /* Timer */
     object_property_set_bool(OBJECT(&s->timerctrl), true, "realized", &err);
     if (err) {
-- 
2.21.0

[Qemu-devel] [PATCH v2 05/21] aspeed: introduce a configurable number of CPU per machine

[Cédric Le Goater]

The current models of the Aspeed SoCs only have one CPU but future
ones will support SMP. Introduce a new num_cpus field at the SoC class
level to define the number of available CPUs per SoC and also
introduce a 'num-cpus' property to activate the CPUs configured for
the machine.

The max_cpus limit of the machine should depend on the SoC definition
but, unfortunately, these values are not available when the machine
class is initialized. This is the reason why we add a check on
num_cpus in the AspeedSoC realize handler.

SMP support will be activated when models for such SoCs are implemented.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 Changes sinve v1:

  - reworked the logic to put the number of CPUs under the SoC

 include/hw/arm/aspeed_soc.h |  5 ++++-
 hw/arm/aspeed.c             |  7 +++++--
 hw/arm/aspeed_soc.c         | 33 +++++++++++++++++++++++++++------
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index fa0ba957a611..b613b00600fc 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -24,13 +24,15 @@
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_WDTS_NUM  3
+#define ASPEED_CPUS_NUM  2
 
 typedef struct AspeedSoCState {
     /*< private >*/
     DeviceState parent;
 
     /*< public >*/
-    ARMCPU cpu;
+    ARMCPU cpu[ASPEED_CPUS_NUM];
+    uint32_t num_cpus;
     MemoryRegion sram;
     AspeedVICState vic;
     AspeedRtcState rtc;
@@ -58,6 +60,7 @@ typedef struct AspeedSoCInfo {
     int wdts_num;
     const int *irqmap;
     const hwaddr *memmap;
+    uint32_t num_cpus;
 } AspeedSoCInfo;
 
 typedef struct AspeedSoCClass {
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index c692ca1dba90..96de4f5c2a87 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -22,13 +22,13 @@
 #include "hw/misc/tmp105.h"
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
 #include "hw/loader.h"
 #include "qemu/error-report.h"
 #include "qemu/units.h"
 
 static struct arm_boot_info aspeed_board_binfo = {
     .board_id = -1, /* device-tree-only board */
-    .nb_cpus = 1,
 };
 
 struct AspeedBoardState {
@@ -171,6 +171,8 @@ static void aspeed_board_init(MachineState *machine,
                             &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), cfg->num_cs, "num-cs",
                             &error_abort);
+    object_property_set_int(OBJECT(&bmc->soc), smp_cpus, "num-cpus",
+                            &error_abort);
     if (machine->kernel_filename) {
         /*
          * When booting with a -kernel command line there is no u-boot
@@ -230,6 +232,7 @@ static void aspeed_board_init(MachineState *machine,
     aspeed_board_binfo.kernel_cmdline = machine->kernel_cmdline;
     aspeed_board_binfo.ram_size = ram_size;
     aspeed_board_binfo.loader_start = sc->info->memmap[ASPEED_SDRAM];
+    aspeed_board_binfo.nb_cpus = bmc->soc.num_cpus;
 
     if (cfg->i2c_init) {
         cfg->i2c_init(bmc);
@@ -326,7 +329,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 
     mc->desc = board->desc;
     mc->init = aspeed_machine_init;
-    mc->max_cpus = 1;
+    mc->max_cpus = ASPEED_CPUS_NUM;
     mc->no_sdcard = 1;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 5faa78d81fd6..d38fb0aaa0f5 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -19,6 +19,7 @@
 #include "hw/char/serial.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "hw/i2c/aspeed_i2c.h"
 #include "net/net.h"
 
@@ -123,6 +124,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
+        .num_cpus     = 1,
     }, {
         .name         = "ast2400-a1",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
@@ -134,6 +136,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
+        .num_cpus     = 1,
     }, {
         .name         = "ast2400",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm926"),
@@ -145,6 +148,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
+        .num_cpus     = 1,
     }, {
         .name         = "ast2500-a1",
         .cpu_type     = ARM_CPU_TYPE_NAME("arm1176"),
@@ -156,6 +160,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .wdts_num     = 3,
         .irqmap       = aspeed_soc_ast2500_irqmap,
         .memmap       = aspeed_soc_ast2500_memmap,
+        .num_cpus     = 1,
     },
 };
 
@@ -172,8 +177,11 @@ static void aspeed_soc_init(Object *obj)
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     int i;
 
-    object_initialize_child(obj, "cpu", OBJECT(&s->cpu), sizeof(s->cpu),
-                            sc->info->cpu_type, &error_abort, NULL);
+    for (i = 0; i < sc->info->num_cpus; i++) {
+        object_initialize_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
+                                sizeof(s->cpu[i]), sc->info->cpu_type,
+                                &error_abort, NULL);
+    }
 
     sysbus_init_child_obj(obj, "scu", OBJECT(&s->scu), sizeof(s->scu),
                           TYPE_ASPEED_SCU);
@@ -241,11 +249,19 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     create_unimplemented_device("aspeed_soc.io", sc->info->memmap[ASPEED_IOMEM],
                                 ASPEED_SOC_IOMEM_SIZE);
 
+    if (s->num_cpus > sc->info->num_cpus) {
+        warn_report("%s: invalid number of CPUs %d, using default %d",
+                    sc->info->name, s->num_cpus, sc->info->num_cpus);
+        s->num_cpus = sc->info->num_cpus;
+    }
+
     /* CPU */
-    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
+    for (i = 0; i < s->num_cpus; i++) {
+        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
     }
 
     /* SRAM */
@@ -380,6 +396,10 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
                        aspeed_soc_get_irq(s, ASPEED_ETH1));
 }
+static Property aspeed_soc_properties[] = {
+    DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
 static void aspeed_soc_class_init(ObjectClass *oc, void *data)
 {
@@ -390,6 +410,7 @@ static void aspeed_soc_class_init(ObjectClass *oc, void *data)
     dc->realize = aspeed_soc_realize;
     /* Reason: Uses serial_hds and nd_table in realize() directly */
     dc->user_creatable = false;
+    dc->props = aspeed_soc_properties;
 }
 
 static const TypeInfo aspeed_soc_type_info = {
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 05/21] aspeed: introduce a configurable number of CPU per machine

[Joel Stanley]

On Tue, 18 Jun 2019 at 16:54, Cédric Le Goater <clg@kaod.org> wrote:
>
> The current models of the Aspeed SoCs only have one CPU but future
> ones will support SMP. Introduce a new num_cpus field at the SoC class
> level to define the number of available CPUs per SoC and also
> introduce a 'num-cpus' property to activate the CPUs configured for
> the machine.
>
> The max_cpus limit of the machine should depend on the SoC definition
> but, unfortunately, these values are not available when the machine
> class is initialized. This is the reason why we add a check on
> num_cpus in the AspeedSoC realize handler.
>
> SMP support will be activated when models for such SoCs are implemented.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

[Qemu-devel] [PATCH v2 06/21] aspeed: add support for multiple NICs

[Cédric Le Goater]

The Aspeed SoCs have two MACs. Extend the Aspeed model to support a
second NIC.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed_soc.h |  3 ++-
 hw/arm/aspeed_soc.c         | 33 +++++++++++++++++++--------------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index b613b00600fc..75b557060b9b 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -25,6 +25,7 @@
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_WDTS_NUM  3
 #define ASPEED_CPUS_NUM  2
+#define ASPEED_MACS_NUM  2
 
 typedef struct AspeedSoCState {
     /*< private >*/
@@ -43,7 +44,7 @@ typedef struct AspeedSoCState {
     AspeedSMCState spi[ASPEED_SPIS_NUM];
     AspeedSDMCState sdmc;
     AspeedWDTState wdt[ASPEED_WDTS_NUM];
-    FTGMAC100State ftgmac100;
+    FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
 } AspeedSoCState;
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index d38fb0aaa0f5..736e52366a66 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -234,8 +234,10 @@ static void aspeed_soc_init(Object *obj)
                                     sc->info->silicon_rev);
     }
 
-    sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100),
-                          sizeof(s->ftgmac100), TYPE_FTGMAC100);
+    for (i = 0; i < ASPEED_MACS_NUM; i++) {
+        sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
+                              sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
+    }
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -382,19 +384,22 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
 
     /* Net */
-    qdev_set_nic_properties(DEVICE(&s->ftgmac100), &nd_table[0]);
-    object_property_set_bool(OBJECT(&s->ftgmac100), true, "aspeed", &err);
-    object_property_set_bool(OBJECT(&s->ftgmac100), true, "realized",
-                             &local_err);
-    error_propagate(&err, local_err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
+    for (i = 0; i < nb_nics; i++) {
+        qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
+        object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
+                                 &err);
+        object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
+                                 &local_err);
+        error_propagate(&err, local_err);
+        if (err) {
+            error_propagate(errp, err);
+           return;
+        }
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
+                        sc->info->memmap[ASPEED_ETH1 + i]);
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
+                           aspeed_soc_get_irq(s, ASPEED_ETH1 + i));
     }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100), 0,
-                    sc->info->memmap[ASPEED_ETH1]);
-    sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
-                       aspeed_soc_get_irq(s, ASPEED_ETH1));
 }
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 06/21] aspeed: add support for multiple NICs

[Joel Stanley]

On Tue, 18 Jun 2019 at 16:54, Cédric Le Goater <clg@kaod.org> wrote:
>
> The Aspeed SoCs have two MACs. Extend the Aspeed model to support a
> second NIC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/arm/aspeed_soc.h |  3 ++-
>  hw/arm/aspeed_soc.c         | 33 +++++++++++++++++++--------------
>  2 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index b613b00600fc..75b557060b9b 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -25,6 +25,7 @@
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_WDTS_NUM  3
>  #define ASPEED_CPUS_NUM  2
> +#define ASPEED_MACS_NUM  2

This will be 4 in the future. No need to change it now though.

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

[Qemu-devel] [PATCH v2 07/21] aspeed/timer: Fix behaviour running Linux

[Cédric Le Goater]

From: Joel Stanley <joel@jms.id.au>

The Linux kernel driver was updated in commit 4451d3f59f2a
("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an
issue observed on hardware:

 > RELOAD register is loaded into COUNT register when the aspeed timer
 > is enabled, which means the next event may be delayed because timer
 > interrupt won't be generated until <0xFFFFFFFF - current_count +
 > cycles>.

When running under Qemu, the system appeared "laggy". The guest is now
scheduling timer events too regularly, starving the host of CPU time.

This patch modifies the timer model to attempt to schedule the timer
expiry as the guest requests, but if we have missed the deadline we
re interrupt and try again, which allows the guest to catch up.

Provides expected behaviour with old and new guest code.

Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model")
Signed-off-by: Joel Stanley <joel@jms.id.au>
[clg: - merged a fix from Andrew Jeffery <andrew@aj.id.au>
        "Fire interrupt on failure to meet deadline"
        https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html
      - adapted commit log
      - checkpatch fixes ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/timer/aspeed_timer.c | 59 ++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 2c3a4d0fe770..537f072cf87f 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks)
 
 static uint64_t calculate_next(struct AspeedTimer *t)
 {
-    uint64_t next = 0;
-    uint32_t rate = calculate_rate(t);
+    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint64_t next;
 
-    while (!next) {
-        /* We don't know the relationship between the values in the match
-         * registers, so sort using MAX/MIN/zero. We sort in that order as the
-         * timer counts down to zero. */
-        uint64_t seq[] = {
-            calculate_time(t, MAX(t->match[0], t->match[1])),
-            calculate_time(t, MIN(t->match[0], t->match[1])),
-            calculate_time(t, 0),
-        };
-        uint64_t reload_ns;
-        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-        if (now < seq[0]) {
-            next = seq[0];
-        } else if (now < seq[1]) {
-            next = seq[1];
-        } else if (now < seq[2]) {
-            next = seq[2];
-        } else if (t->reload) {
-            reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
-            t->start = now - ((now - t->start) % reload_ns);
-        } else {
-            /* no reload value, return 0 */
-            break;
-        }
+    /*
+     * We don't know the relationship between the values in the match
+     * registers, so sort using MAX/MIN/zero. We sort in that order as
+     * the timer counts down to zero.
+     */
+
+    next = calculate_time(t, MAX(t->match[0], t->match[1]));
+    if (now < next) {
+        return next;
+    }
+
+    next = calculate_time(t, MIN(t->match[0], t->match[1]));
+    if (now < next) {
+        return next;
+    }
+
+    next = calculate_time(t, 0);
+    if (now < next) {
+        return next;
+    }
+
+    /* We've missed all deadlines, fire interrupt and try again */
+    timer_del(&t->timer);
+
+    if (timer_overflow_interrupt(t)) {
+        t->level = !t->level;
+        qemu_set_irq(t->irq, t->level);
     }
 
-    return next;
+    t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
 }
 
 static void aspeed_timer_mod(AspeedTimer *t)
-- 
2.21.0

[Qemu-devel] [PATCH v2 08/21] aspeed/timer: Status register contains reload for stopped timer

[Cédric Le Goater]

From: Andrew Jeffery <andrew@aj.id.au>

From the datasheet:

  This register stores the current status of counter #N. When timer
  enable bit TMC30[N * b] is disabled, the reload register will be
  loaded into this counter. When timer bit TMC30[N * b] is set, the
  counter will start to decrement. CPU can update this register value
  when enable bit is set.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/timer/aspeed_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 537f072cf87f..8d6266b0fd81 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -187,7 +187,11 @@ static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
 
     switch (reg) {
     case TIMER_REG_STATUS:
-        value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        if (timer_enabled(t)) {
+            value = calculate_ticks(t, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        } else {
+            value = t->reload;
+        }
         break;
     case TIMER_REG_RELOAD:
         value = t->reload;
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 08/21] aspeed/timer: Status register contains reload for stopped timer

[Joel Stanley]

On Tue, 18 Jun 2019 at 16:54, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Andrew Jeffery <andrew@aj.id.au>
>
> From the datasheet:
>
>   This register stores the current status of counter #N. When timer
>   enable bit TMC30[N * b] is disabled, the reload register will be
>   loaded into this counter. When timer bit TMC30[N * b] is set, the
>   counter will start to decrement. CPU can update this register value
>   when enable bit is set.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

[Qemu-devel] [PATCH v2 09/21] aspeed/timer: Fix match calculations

[Cédric Le Goater]

From: Andrew Jeffery <andrew@aj.id.au>

If the match value exceeds reload then we don't want to include it in
calculations for the next event.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/timer/aspeed_timer.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 8d6266b0fd81..745eb8608b56 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -107,6 +107,11 @@ static inline uint64_t calculate_time(struct AspeedTimer *t, uint32_t ticks)
     return t->start + delta_ns;
 }
 
+static inline uint32_t calculate_match(struct AspeedTimer *t, int i)
+{
+    return t->match[i] < t->reload ? t->match[i] : 0;
+}
+
 static uint64_t calculate_next(struct AspeedTimer *t)
 {
     uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -118,12 +123,12 @@ static uint64_t calculate_next(struct AspeedTimer *t)
      * the timer counts down to zero.
      */
 
-    next = calculate_time(t, MAX(t->match[0], t->match[1]));
+    next = calculate_time(t, MAX(calculate_match(t, 0), calculate_match(t, 1)));
     if (now < next) {
         return next;
     }
 
-    next = calculate_time(t, MIN(t->match[0], t->match[1]));
+    next = calculate_time(t, MIN(calculate_match(t, 0), calculate_match(t, 1)));
     if (now < next) {
         return next;
     }
@@ -141,8 +146,10 @@ static uint64_t calculate_next(struct AspeedTimer *t)
         qemu_set_irq(t->irq, t->level);
     }
 
+    next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
     t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
+
+    return calculate_time(t, next);
 }
 
 static void aspeed_timer_mod(AspeedTimer *t)
-- 
2.21.0

[Qemu-devel] [PATCH v2 10/21] aspeed/timer: Provide back-pressure information for short periods

[Cédric Le Goater]

From: Andrew Jeffery <andrew@aj.id.au>

First up: This is not the way the hardware behaves.

However, it helps resolve real-world problems with short periods being
used under Linux. Commit 4451d3f59f2a ("clocksource/drivers/fttmr010:
Fix set_next_event handler") in Linux fixed the timer driver to
correctly schedule the next event for the Aspeed controller, and in
combination with 5daa8212c08e ("ARM: dts: aspeed: Describe random number
device") Linux will now set a timer with a period as low as 1us.

Configuring a qemu timer with such a short period results in spending
time handling the interrupt in the model rather than executing guest
code, leading to noticeable "sticky" behaviour in the guest.

The behaviour of Linux is correct with respect to the hardware, so we
need to improve our handling under emulation. The approach chosen is to
provide back-pressure information by calculating an acceptable minimum
number of ticks to be set on the model. Under Linux an additional read
is added in the timer configuration path to detect back-pressure, which
will never occur on hardware. However if back-pressure is observed, the
driver alerts the clock event subsystem, which then performs its own
next event dilation via a config option - d1748302f70b ("clockevents:
Make minimum delay adjustments configurable")

A minimum period of 5us was experimentally determined on a Lenovo
T480s, which I've increased to 20us for "safety".

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/timer/aspeed_timer.h | 1 +
 hw/misc/aspeed_scu.c            | 6 ++++++
 hw/timer/aspeed_timer.c         | 6 +++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
index 1fb949e16710..10c851ebb6d7 100644
--- a/include/hw/timer/aspeed_timer.h
+++ b/include/hw/timer/aspeed_timer.h
@@ -41,6 +41,7 @@ typedef struct AspeedTimer {
      * interrupts, signalling with both the rising and falling edge.
      */
     int32_t level;
+    uint32_t min_ticks;
     uint32_t reload;
     uint32_t match[2];
     uint64_t start;
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 71a0d4b7be7a..32e58da90657 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -423,6 +423,12 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)
                           TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
 
     sysbus_init_mmio(sbd, &s->iomem);
+
+    /*
+     * Reset on realize to ensure the APB clock value is calculated in time for
+     * use by the timer model, which is reset before the SCU.
+     */
+    aspeed_scu_reset(dev);
 }
 
 static const VMStateDescription vmstate_aspeed_scu = {
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 745eb8608b56..6501fa0768e7 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -259,7 +259,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
     switch (reg) {
     case TIMER_REG_RELOAD:
         old_reload = t->reload;
-        t->reload = value;
+        t->reload = value < t->min_ticks ? t->min_ticks : value;
 
         /* If the reload value was not previously set, or zero, and
          * the current value is valid, try to start the timer if it is
@@ -311,7 +311,11 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
 
 static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
 {
+    AspeedTimerCtrlState *s = timer_to_ctrl(t);
+    uint32_t rate = enable ? TIMER_CLOCK_EXT_HZ : s->scu->apb_freq;
+
     trace_aspeed_timer_ctrl_external_clock(t->id, enable);
+    t->min_ticks = muldiv64(20 * SCALE_US, rate, NANOSECONDS_PER_SECOND);
 }
 
 static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 10/21] aspeed/timer: Provide back-pressure information for short periods

[Peter Maydell]

On Tue, 18 Jun 2019 at 17:54, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Andrew Jeffery <andrew@aj.id.au>
>
> First up: This is not the way the hardware behaves.
>
> However, it helps resolve real-world problems with short periods being
> used under Linux. Commit 4451d3f59f2a ("clocksource/drivers/fttmr010:
> Fix set_next_event handler") in Linux fixed the timer driver to
> correctly schedule the next event for the Aspeed controller, and in
> combination with 5daa8212c08e ("ARM: dts: aspeed: Describe random number
> device") Linux will now set a timer with a period as low as 1us.
>
> Configuring a qemu timer with such a short period results in spending
> time handling the interrupt in the model rather than executing guest
> code, leading to noticeable "sticky" behaviour in the guest.
>
> The behaviour of Linux is correct with respect to the hardware, so we
> need to improve our handling under emulation. The approach chosen is to
> provide back-pressure information by calculating an acceptable minimum
> number of ticks to be set on the model. Under Linux an additional read
> is added in the timer configuration path to detect back-pressure, which
> will never occur on hardware. However if back-pressure is observed, the
> driver alerts the clock event subsystem, which then performs its own
> next event dilation via a config option - d1748302f70b ("clockevents:
> Make minimum delay adjustments configurable")
>
> A minimum period of 5us was experimentally determined on a Lenovo
> T480s, which I've increased to 20us for "safety".
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -423,6 +423,12 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)
>                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
>
>      sysbus_init_mmio(sbd, &s->iomem);
> +
> +    /*
> +     * Reset on realize to ensure the APB clock value is calculated in time for
> +     * use by the timer model, which is reset before the SCU.
> +     */
> +    aspeed_scu_reset(dev);

This looks wrong. QEMU should always be resetting devices
after realize and before actually running anything.

>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 10/21] aspeed/timer: Provide back-pressure information for short periods

[Cédric Le Goater]

On 01/07/2019 14:59, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 17:54, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> From: Andrew Jeffery <andrew@aj.id.au>
>>
>> First up: This is not the way the hardware behaves.
>>
>> However, it helps resolve real-world problems with short periods being
>> used under Linux. Commit 4451d3f59f2a ("clocksource/drivers/fttmr010:
>> Fix set_next_event handler") in Linux fixed the timer driver to
>> correctly schedule the next event for the Aspeed controller, and in
>> combination with 5daa8212c08e ("ARM: dts: aspeed: Describe random number
>> device") Linux will now set a timer with a period as low as 1us.
>>
>> Configuring a qemu timer with such a short period results in spending
>> time handling the interrupt in the model rather than executing guest
>> code, leading to noticeable "sticky" behaviour in the guest.
>>
>> The behaviour of Linux is correct with respect to the hardware, so we
>> need to improve our handling under emulation. The approach chosen is to
>> provide back-pressure information by calculating an acceptable minimum
>> number of ticks to be set on the model. Under Linux an additional read
>> is added in the timer configuration path to detect back-pressure, which
>> will never occur on hardware. However if back-pressure is observed, the
>> driver alerts the clock event subsystem, which then performs its own
>> next event dilation via a config option - d1748302f70b ("clockevents:
>> Make minimum delay adjustments configurable")
>>
>> A minimum period of 5us was experimentally determined on a Lenovo
>> T480s, which I've increased to 20us for "safety".
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
>> --- a/hw/misc/aspeed_scu.c
>> +++ b/hw/misc/aspeed_scu.c
>> @@ -423,6 +423,12 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)
>>                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
>>
>>      sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    /*
>> +     * Reset on realize to ensure the APB clock value is calculated in time for
>> +     * use by the timer model, which is reset before the SCU.
>> +     */
>> +    aspeed_scu_reset(dev);
> 
> This looks wrong. QEMU should always be resetting devices
> after realize and before actually running anything.

Ah yes ... We need to rework SCU with class data to provide a cleaner 
model for all SoCs. AST2600 will require it anyhow. 

Thanks,

C. 
> 
>>  }
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v2 10/21] aspeed/timer: Provide back-pressure information for short periods

[Cédric Le Goater]

On 01/07/2019 14:59, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 17:54, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> From: Andrew Jeffery <andrew@aj.id.au>
>>
>> First up: This is not the way the hardware behaves.
>>
>> However, it helps resolve real-world problems with short periods being
>> used under Linux. Commit 4451d3f59f2a ("clocksource/drivers/fttmr010:
>> Fix set_next_event handler") in Linux fixed the timer driver to
>> correctly schedule the next event for the Aspeed controller, and in
>> combination with 5daa8212c08e ("ARM: dts: aspeed: Describe random number
>> device") Linux will now set a timer with a period as low as 1us.
>>
>> Configuring a qemu timer with such a short period results in spending
>> time handling the interrupt in the model rather than executing guest
>> code, leading to noticeable "sticky" behaviour in the guest.
>>
>> The behaviour of Linux is correct with respect to the hardware, so we
>> need to improve our handling under emulation. The approach chosen is to
>> provide back-pressure information by calculating an acceptable minimum
>> number of ticks to be set on the model. Under Linux an additional read
>> is added in the timer configuration path to detect back-pressure, which
>> will never occur on hardware. However if back-pressure is observed, the
>> driver alerts the clock event subsystem, which then performs its own
>> next event dilation via a config option - d1748302f70b ("clockevents:
>> Make minimum delay adjustments configurable")
>>
>> A minimum period of 5us was experimentally determined on a Lenovo
>> T480s, which I've increased to 20us for "safety".
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
>> --- a/hw/misc/aspeed_scu.c
>> +++ b/hw/misc/aspeed_scu.c
>> @@ -423,6 +423,12 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)
>>                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
>>
>>      sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    /*
>> +     * Reset on realize to ensure the APB clock value is calculated in time for
>> +     * use by the timer model, which is reset before the SCU.
>> +     */
>> +    aspeed_scu_reset(dev);
> 
> This looks wrong. QEMU should always be resetting devices
> after realize and before actually running anything.

We have something better for this patch (getting rid of the reset 
ordering issue). Can we still send you the fix this week ?

Thanks,

C.

[Qemu-devel] [PATCH v2 11/21] aspeed/timer: Ensure positive muldiv delta

[Cédric Le Goater]

From: Christian Svensson <bluecmd@google.com>

If the host decrements the counter register that results in a negative
delta. This is then passed to muldiv64 which only handles unsigned
numbers resulting in bogus results.

This fix ensures the delta being operated on is positive.

Test case: kexec a kernel using aspeed_timer and it will freeze on the
second bootup when the kernel initializes the timer. With this patch
that no longer happens and the timer appears to run OK.

Signed-off-by: Christian Svensson <bluecmd@google.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/timer/aspeed_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 6501fa0768e7..1f0f1886fb2a 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -275,7 +275,11 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
             int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now);
             uint32_t rate = calculate_rate(t);
 
-            t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
+            if (delta >= 0) {
+                t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
+            } else {
+                t->start -= muldiv64(-delta, NANOSECONDS_PER_SECOND, rate);
+            }
             aspeed_timer_mod(t);
         }
         break;
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 11/21] aspeed/timer: Ensure positive muldiv delta

[Joel Stanley]

On Tue, 18 Jun 2019 at 16:54, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Christian Svensson <bluecmd@google.com>
>
> If the host decrements the counter register that results in a negative
> delta. This is then passed to muldiv64 which only handles unsigned
> numbers resulting in bogus results.
>
> This fix ensures the delta being operated on is positive.
>
> Test case: kexec a kernel using aspeed_timer and it will freeze on the
> second bootup when the kernel initializes the timer. With this patch
> that no longer happens and the timer appears to run OK.
>
> Signed-off-by: Christian Svensson <bluecmd@google.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

Re: [Qemu-devel] [PATCH v2 11/21] aspeed/timer: Ensure positive muldiv delta

[Andrew Jeffery]

On Wed, 19 Jun 2019, at 02:24, Cédric Le Goater wrote:
> From: Christian Svensson <bluecmd@google.com>
> 
> If the host decrements the counter register that results in a negative
> delta. This is then passed to muldiv64 which only handles unsigned
> numbers resulting in bogus results.
> 
> This fix ensures the delta being operated on is positive.
> 
> Test case: kexec a kernel using aspeed_timer and it will freeze on the
> second bootup when the kernel initializes the timer. With this patch
> that no longer happens and the timer appears to run OK.
> 
> Signed-off-by: Christian Svensson <bluecmd@google.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

> ---
>  hw/timer/aspeed_timer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 6501fa0768e7..1f0f1886fb2a 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -275,7 +275,11 @@ static void 
> aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>              int64_t delta = (int64_t) value - (int64_t) 
> calculate_ticks(t, now);
>              uint32_t rate = calculate_rate(t);
>  
> -            t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
> +            if (delta >= 0) {
> +                t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
> +            } else {
> +                t->start -= muldiv64(-delta, NANOSECONDS_PER_SECOND, rate);
> +            }
>              aspeed_timer_mod(t);
>          }
>          break;
> -- 
> 2.21.0
> 
>

[Qemu-devel] [PATCH v2 12/21] aspeed: remove the "ram" link

[Cédric Le Goater]

It has never been used as far as I can tell from the git history.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 96de4f5c2a87..5d73267da16f 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -195,8 +195,6 @@ static void aspeed_board_init(MachineState *machine,
     memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
     memory_region_add_subregion(get_system_memory(),
                                 sc->info->memmap[ASPEED_SDRAM], &bmc->ram);
-    object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
-                                   &error_abort);
 
     max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
                                             &error_abort);
-- 
2.21.0

[Qemu-devel] [PATCH v2 13/21] aspeed: add a RAM memory region container

[Cédric Le Goater]

The RAM memory region is defined after the SoC is realized when the
SDMC controller has checked that the defined RAM size for the machine
is correct. This is problematic for controller models requiring a link
on the RAM region, for DMA support in the SMC controller for instance.

Introduce a container memory region for the RAM that we can link into
the controllers early, before the SoC is realized. It will be
populated with the RAM region after the checks have be done.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 5d73267da16f..7f01df1b61d8 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -33,6 +33,7 @@ static struct arm_boot_info aspeed_board_binfo = {
 
 struct AspeedBoardState {
     AspeedSoCState soc;
+    MemoryRegion ram_container;
     MemoryRegion ram;
     MemoryRegion max_ram;
 };
@@ -159,6 +160,10 @@ static void aspeed_board_init(MachineState *machine,
     ram_addr_t max_ram_size;
 
     bmc = g_new0(AspeedBoardState, 1);
+
+    memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
+                       UINT32_MAX);
+
     object_initialize_child(OBJECT(machine), "soc", &bmc->soc,
                             (sizeof(bmc->soc)), cfg->soc_name, &error_abort,
                             NULL);
@@ -193,16 +198,16 @@ static void aspeed_board_init(MachineState *machine,
                                         &error_abort);
 
     memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
+    memory_region_add_subregion(&bmc->ram_container, 0, &bmc->ram);
     memory_region_add_subregion(get_system_memory(),
-                                sc->info->memmap[ASPEED_SDRAM], &bmc->ram);
+                                sc->info->memmap[ASPEED_SDRAM],
+                                &bmc->ram_container);
 
     max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
                                             &error_abort);
     memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
                           "max_ram", max_ram_size  - ram_size);
-    memory_region_add_subregion(get_system_memory(),
-                                sc->info->memmap[ASPEED_SDRAM] + ram_size,
-                                &bmc->max_ram);
+    memory_region_add_subregion(&bmc->ram_container, ram_size, &bmc->max_ram);
 
     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);
-- 
2.21.0

[Qemu-devel] [PATCH v2 14/21] aspeed/smc: add a 'sdram_base' property

[Cédric Le Goater]

The DRAM address of a DMA transaction depends on the DRAM base address
of the SoC. Inform the SMC controller model with this value.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/ssi/aspeed_smc.h | 3 +++
 hw/arm/aspeed_soc.c         | 6 ++++++
 hw/ssi/aspeed_smc.c         | 1 +
 3 files changed, 10 insertions(+)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 3b1e7fce6c86..591279ba1f43 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -97,6 +97,9 @@ typedef struct AspeedSMCState {
     uint8_t r_timings;
     uint8_t conf_enable_w0;
 
+    /* for DMA support */
+    uint64_t sdram_base;
+
     AspeedSMCFlash *flashes;
 
     uint8_t snoop_index;
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 736e52366a66..02feb4361ba4 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -337,6 +337,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                        aspeed_soc_get_irq(s, ASPEED_I2C));
 
     /* FMC, The number of CS is set at the board level */
+    object_property_set_int(OBJECT(&s->fmc), sc->info->memmap[ASPEED_SDRAM],
+                            "sdram-base", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     object_property_set_bool(OBJECT(&s->fmc), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 9eda0d720be6..81f2fb7f707a 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -913,6 +913,7 @@ 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_END_OF_LIST(),
 };
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 15/21] 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.

The model introduces two custom address spaces for DMAs: one for the
AHB window of the FMC flash devices and one for the DRAM. The latter
is populated using a "dram" link set from the machine with the RAM
container region.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Acked-by: Joel Stanley <joel@jms.id.au>
---
 include/hw/ssi/aspeed_smc.h |   6 +
 hw/arm/aspeed.c             |   2 +
 hw/arm/aspeed_soc.c         |   2 +
 hw/ssi/aspeed_smc.c         | 228 +++++++++++++++++++++++++++++++++++-
 4 files changed, 232 insertions(+), 6 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 591279ba1f43..453357cc09bf 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -45,6 +45,8 @@ typedef struct AspeedSMCController {
     hwaddr flash_window_base;
     uint32_t flash_window_size;
     bool has_dma;
+    hwaddr dma_flash_mask;
+    hwaddr dma_dram_mask;
     uint32_t nregs;
 } AspeedSMCController;
 
@@ -100,6 +102,10 @@ typedef struct AspeedSMCState {
     /* for DMA support */
     uint64_t sdram_base;
 
+    AddressSpace flash_as;
+    MemoryRegion *dram_mr;
+    AddressSpace dram_as;
+
     AspeedSMCFlash *flashes;
 
     uint8_t snoop_index;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 7f01df1b61d8..e404f8b244a9 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -178,6 +178,8 @@ static void aspeed_board_init(MachineState *machine,
                             &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), smp_cpus, "num-cpus",
                             &error_abort);
+    object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container),
+                             "dram", &error_abort);
     if (machine->kernel_filename) {
         /*
          * When booting with a -kernel command line there is no u-boot
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 02feb4361ba4..8a1545545faf 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -212,6 +212,8 @@ static void aspeed_soc_init(Object *obj)
                           sc->info->fmc_typename);
     object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
                               &error_abort);
+    object_property_add_alias(obj, "dram", OBJECT(&s->fmc), "dram",
+                              &error_abort);
 
     for (i = 0; i < sc->info->spis_num; i++) {
         sysbus_init_child_obj(obj, "spi[*]", OBJECT(&s->spi[i]),
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 81f2fb7f707a..e00cdebae232 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -23,11 +23,13 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
+#include "exec/address-spaces.h"
 
 #include "hw/ssi/aspeed_smc.h"
 
@@ -110,8 +112,8 @@
 #define   DMA_CTRL_FREQ_SHIFT   4
 #define   DMA_CTRL_MODE         (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)
@@ -143,6 +145,24 @@
 #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 0x40000000 - 0x5FFFFFFF (AST2400)
+ *          0x80000000 - 0xBFFFFFFF (AST2500)
+ *
+ * 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_ADDR(s, val)   ((s)->sdram_base | \
+                                 ((val) & (s)->ctrl->dma_dram_mask))
+#define DMA_FLASH_ADDR(s, val)  ((s)->ctrl->flash_window_base | \
+                                ((val) & (s)->ctrl->dma_flash_mask))
+#define DMA_LENGTH(val)         ((val) & 0x01FFFFFC)
+
 /* Flash opcodes. */
 #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
 
@@ -212,6 +232,8 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
         .flash_window_size = 0x10000000,
         .has_dma           = true,
+        .dma_flash_mask    = 0x0FFFFFFC,
+        .dma_dram_mask     = 0x1FFFFFFC,
         .nregs             = ASPEED_SMC_R_MAX,
     }, {
         .name              = "aspeed.smc.spi",
@@ -238,6 +260,8 @@ static const AspeedSMCController controllers[] = {
         .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
         .flash_window_size = 0x10000000,
         .has_dma           = true,
+        .dma_flash_mask    = 0x0FFFFFFC,
+        .dma_dram_mask     = 0x3FFFFFFC,
         .nregs             = ASPEED_SMC_R_MAX,
     }, {
         .name              = "aspeed.smc.ast2500-spi1",
@@ -730,9 +754,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;
@@ -773,6 +794,11 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         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) ||
+        (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];
@@ -783,6 +809,155 @@ 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)
+{
+    MemTxResult result;
+    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]) {
+        result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
+                                    MEMTXATTRS_UNSPECIFIED,
+                                    (uint8_t *)&data, 4);
+        if (result != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
+                          __func__, s->regs[R_DMA_FLASH_ADDR]);
+            return;
+        }
+
+        /*
+         * 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)
+{
+    MemTxResult result;
+    uint32_t data;
+
+    while (s->regs[R_DMA_LEN]) {
+        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
+            result = address_space_read(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
+                                        MEMTXATTRS_UNSPECIFIED,
+                                        (uint8_t *)&data, 4);
+            if (result != MEMTX_OK) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
+                              __func__, s->regs[R_DMA_DRAM_ADDR]);
+                return;
+            }
+
+            result = address_space_write(&s->flash_as,
+                                         s->regs[R_DMA_FLASH_ADDR],
+                                         MEMTXATTRS_UNSPECIFIED,
+                                         (uint8_t *)&data, 4);
+            if (result != MEMTX_OK) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash write failed @%08x\n",
+                              __func__, s->regs[R_DMA_FLASH_ADDR]);
+                return;
+            }
+        } else {
+            result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
+                                        MEMTXATTRS_UNSPECIFIED,
+                                        (uint8_t *)&data, 4);
+            if (result != MEMTX_OK) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
+                              __func__, s->regs[R_DMA_FLASH_ADDR]);
+                return;
+            }
+
+            result = address_space_write(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
+                                         MEMTXATTRS_UNSPECIFIED,
+                                         (uint8_t *)&data, 4);
+            if (result != MEMTX_OK) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
+                              __func__, s->regs[R_DMA_DRAM_ADDR]);
+                return;
+            }
+        }
+
+        /*
+         * 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)
 {
@@ -808,6 +983,16 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         }
     } else if (addr == R_DUMMY_DATA) {
         s->regs[addr] = value & 0xff;
+    } 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) {
+        s->regs[addr] = DMA_DRAM_ADDR(s, value);
+    } else if (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) {
+        s->regs[addr] = DMA_FLASH_ADDR(s, value);
+    } else if (s->ctrl->has_dma && addr == R_DMA_LEN) {
+        s->regs[addr] = DMA_LENGTH(value);
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
@@ -822,6 +1007,28 @@ static const MemoryRegionOps aspeed_smc_ops = {
     .valid.unaligned = true,
 };
 
+
+/*
+ * Initialize the custom address spaces for DMAs
+ */
+static void aspeed_smc_dma_setup(AspeedSMCState *s, Error **errp)
+{
+    char *name;
+
+    if (!s->dram_mr) {
+        error_setg(errp, TYPE_ASPEED_SMC ": 'dram' link not set");
+        return;
+    }
+
+    name = g_strdup_printf("%s-dma-flash", s->ctrl->name);
+    address_space_init(&s->flash_as, &s->mmio_flash, name);
+    g_free(name);
+
+    name = g_strdup_printf("%s-dma-dram", s->ctrl->name);
+    address_space_init(&s->dram_as, s->dram_mr, name);
+    g_free(name);
+}
+
 static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -847,10 +1054,12 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
         s->num_cs = s->ctrl->max_slaves;
     }
 
+    /* DMA irq. Keep it first for the initialization in the SoC */
+    sysbus_init_irq(sbd, &s->irq);
+
     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);
 
@@ -897,6 +1106,11 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
         memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
         offset += fl->size;
     }
+
+    /* DMA support */
+    if (s->ctrl->has_dma) {
+        aspeed_smc_dma_setup(s, errp);
+    }
 }
 
 static const VMStateDescription vmstate_aspeed_smc = {
@@ -914,6 +1128,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_LINK("dram", AspeedSMCState, dram_mr,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 15/21] aspeed/smc: add support for DMAs

[Peter Maydell]

On Tue, 18 Jun 2019 at 17:55, Cédric Le Goater <clg@kaod.org> wrote:
>
> 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.
>
> The model introduces two custom address spaces for DMAs: one for the
> AHB window of the FMC flash devices and one for the DRAM. The latter
> is populated using a "dram" link set from the machine with the RAM
> container region.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Acked-by: Joel Stanley <joel@jms.id.au>
> +/*
> + * 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)
> +{
> +    MemTxResult result;
> +    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]) {
> +        result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
> +                                    MEMTXATTRS_UNSPECIFIED,
> +                                    (uint8_t *)&data, 4);

This does a byte-by-byte read into a local uint32_t...

> +        if (result != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
> +                          __func__, s->regs[R_DMA_FLASH_ADDR]);
> +            return;
> +        }
> +
> +        /*
> +         * When the DMA is on-going, the DMA registers are updated
> +         * with the current working addresses and length.
> +         */
> +        s->regs[R_DMA_CHECKSUM] += data;

...which we then use as a (host-endian) 32-bit value.

This will behave differently on big endian and little endian hosts.
If the h/w behaviour is to to load a 32-bit data type you probably
want address_space_ldl_le() (or _be() if it's doing a big-endian load).

> +        s->regs[R_DMA_FLASH_ADDR] += 4;
> +        s->regs[R_DMA_LEN] -= 4;
> +    }
> +}
> +
> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
> +{
> +    MemTxResult result;
> +    uint32_t data;
> +
> +    while (s->regs[R_DMA_LEN]) {
> +        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> +            result = address_space_read(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +                                        MEMTXATTRS_UNSPECIFIED,
> +                                        (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
> +                return;
> +            }
> +
> +            result = address_space_write(&s->flash_as,
> +                                         s->regs[R_DMA_FLASH_ADDR],
> +                                         MEMTXATTRS_UNSPECIFIED,
> +                                         (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash write failed @%08x\n",
> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
> +                return;
> +            }
> +        } else {
> +            result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
> +                                        MEMTXATTRS_UNSPECIFIED,
> +                                        (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
> +                return;
> +            }
> +
> +            result = address_space_write(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +                                         MEMTXATTRS_UNSPECIFIED,
> +                                         (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
> +                return;
> +            }
> +        }

Since the code here doesn't do anything with the data the
address_space_read/write here aren't wrong, but you might
prefer to use the ldl and stl functions to avoid the casts
to uint8_t* and need to specify the length.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 15/21] aspeed/smc: add support for DMAs

[Cédric Le Goater]

On 01/07/2019 15:06, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 17:55, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> 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.
>>
>> The model introduces two custom address spaces for DMAs: one for the
>> AHB window of the FMC flash devices and one for the DRAM. The latter
>> is populated using a "dram" link set from the machine with the RAM
>> container region.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Acked-by: Joel Stanley <joel@jms.id.au>
>> +/*
>> + * 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)
>> +{
>> +    MemTxResult result;
>> +    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]) {
>> +        result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
>> +                                    MEMTXATTRS_UNSPECIFIED,
>> +                                    (uint8_t *)&data, 4);
> 
> This does a byte-by-byte read into a local uint32_t...
> 
>> +        if (result != MEMTX_OK) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
>> +                          __func__, s->regs[R_DMA_FLASH_ADDR]);
>> +            return;
>> +        }
>> +
>> +        /*
>> +         * When the DMA is on-going, the DMA registers are updated
>> +         * with the current working addresses and length.
>> +         */
>> +        s->regs[R_DMA_CHECKSUM] += data;
> 
> ...which we then use as a (host-endian) 32-bit value.
> 
> This will behave differently on big endian and little endian hosts.
> If the h/w behaviour is to to load a 32-bit data type you probably
> want address_space_ldl_le() (or _be() if it's doing a big-endian load).
> 
>> +        s->regs[R_DMA_FLASH_ADDR] += 4;
>> +        s->regs[R_DMA_LEN] -= 4;
>> +    }
>> +}
>> +
>> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
>> +{
>> +    MemTxResult result;
>> +    uint32_t data;
>> +
>> +    while (s->regs[R_DMA_LEN]) {
>> +        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
>> +            result = address_space_read(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
>> +                                        MEMTXATTRS_UNSPECIFIED,
>> +                                        (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
>> +                return;
>> +            }
>> +
>> +            result = address_space_write(&s->flash_as,
>> +                                         s->regs[R_DMA_FLASH_ADDR],
>> +                                         MEMTXATTRS_UNSPECIFIED,
>> +                                         (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash write failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
>> +                return;
>> +            }
>> +        } else {
>> +            result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
>> +                                        MEMTXATTRS_UNSPECIFIED,
>> +                                        (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
>> +                return;
>> +            }
>> +
>> +            result = address_space_write(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
>> +                                         MEMTXATTRS_UNSPECIFIED,
>> +                                         (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
>> +                return;
>> +            }
>> +        }
> 
> Since the code here doesn't do anything with the data the
> address_space_read/write here aren't wrong, but you might
> prefer to use the ldl and stl functions to avoid the casts
> to uint8_t* and need to specify the length.

yes. I will check with the HW guys to know precisely how the values are
read and accumulated.

Thanks,

C. 

[Qemu-devel] [PATCH v2 16/21] 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
set using bit[11:4] of the DMA Control Register.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Acked-by: Joel Stanley <joel@jms.id.au>
---
 hw/ssi/aspeed_smc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index e00cdebae232..4a2e3a9135b6 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -75,6 +75,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
@@ -110,7 +114,7 @@
 #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_WRITE        (1 << 1)
 #define   DMA_CTRL_ENABLE       (1 << 0)
@@ -809,6 +813,60 @@ 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 set using 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;
+
+    /*
+     * The Read Timing Compensation Register values apply to all CS on
+     * the SPI bus and only HCLK/1 - HCLK/5 can 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: compute the CS from the DMA address and the segment
+     * registers. This is not really a problem for now because the
+     * Timing Register values apply to all CS and software uses CS0 to
+     * do calibration.
+     */
+    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.
@@ -824,6 +882,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]) {
         result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
                                     MEMTXATTRS_UNSPECIFIED,
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 16/21] aspeed/smc: add DMA calibration settings

[Peter Maydell]

On Tue, 18 Jun 2019 at 17:55, Cédric Le Goater <clg@kaod.org> wrote:
>
> When doing calibration, the SPI clock rate in the CE0 Control Register
> and the read delay cycles in the Read Timing Compensation Register are
> set using bit[11:4] of the DMA Control Register.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Acked-by: Joel Stanley <joel@jms.id.au>
> ---
>  hw/ssi/aspeed_smc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v2 17/21] 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>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---

 Changes since v1:

 - introduced a "inject-failure" property as suggested by Philippe

 include/hw/ssi/aspeed_smc.h |  1 +
 hw/ssi/aspeed_smc.c         | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 453357cc09bf..ba496956cd5e 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -87,6 +87,7 @@ typedef struct AspeedSMCState {
 
     uint32_t num_cs;
     qemu_irq *cs_lines;
+    bool inject_failure;
 
     SSIBus *spi;
 
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 4a2e3a9135b6..5c017f631ffd 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -867,6 +867,36 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
     s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
 }
 
+/*
+ * 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.
+ */
+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.
@@ -904,6 +934,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
         s->regs[R_DMA_FLASH_ADDR] += 4;
         s->regs[R_DMA_LEN] -= 4;
     }
+
+    if (s->inject_failure && aspeed_smc_inject_read_failure(s)) {
+        s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
+    }
+
 }
 
 static void aspeed_smc_dma_rw(AspeedSMCState *s)
@@ -1189,6 +1224,7 @@ static const VMStateDescription vmstate_aspeed_smc = {
 
 static Property aspeed_smc_properties[] = {
     DEFINE_PROP_UINT32("num-cs", AspeedSMCState, num_cs, 1),
+    DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure, false),
     DEFINE_PROP_UINT64("sdram-base", AspeedSMCState, sdram_base, 0),
     DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
                      TYPE_MEMORY_REGION, MemoryRegion *),
-- 
2.21.0

[Qemu-devel] [PATCH v2 18/21] aspeed/smc: Calculate checksum on normal DMA

[Cédric Le Goater]

From: Christian Svensson <bluecmd@google.com>

This patch adds the missing checksum calculation on normal DMA transfer.
According to the datasheet this is how the SMC should behave.

Verified on AST1250 that the hardware matches the behaviour.

Signed-off-by: Christian Svensson <bluecmd@google.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 5c017f631ffd..9d0f17ce84ae 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -993,6 +993,7 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
         s->regs[R_DMA_FLASH_ADDR] += 4;
         s->regs[R_DMA_DRAM_ADDR] += 4;
         s->regs[R_DMA_LEN] -= 4;
+        s->regs[R_DMA_CHECKSUM] += data;
     }
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 19/21] aspeed: Add support for the swift-bmc board

[Cédric Le Goater]

From: Adriana Kobylak <anoo@us.ibm.com>

The Swift board is an OpenPOWER system hosting POWER processors.
Add support for their BMC including the I2C devices as found on HW.

Signed-off-by: Adriana Kobylak <anoo@us.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index e404f8b244a9..51d6b5dd9627 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -73,6 +73,17 @@ struct AspeedBoardState {
         SCU_AST2500_HW_STRAP_ACPI_ENABLE |                              \
         SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
 
+/* Swift hardware value: 0xF11AD206 */
+#define SWIFT_BMC_HW_STRAP1 (                                           \
+        AST2500_HW_STRAP1_DEFAULTS |                                    \
+        SCU_AST2500_HW_STRAP_SPI_AUTOFETCH_ENABLE |                     \
+        SCU_AST2500_HW_STRAP_GPIO_STRAP_ENABLE |                        \
+        SCU_AST2500_HW_STRAP_UART_DEBUG |                               \
+        SCU_AST2500_HW_STRAP_DDR4_ENABLE |                              \
+        SCU_H_PLL_BYPASS_EN |                                           \
+        SCU_AST2500_HW_STRAP_ACPI_ENABLE |                              \
+        SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
+
 /* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
 #define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
 
@@ -294,6 +305,35 @@ static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
+static void swift_bmc_i2c_init(AspeedBoardState *bmc)
+{
+    AspeedSoCState *soc = &bmc->soc;
+
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), "pca9552", 0x60);
+
+    /* The swift board expects a TMP275 but a TMP105 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "tmp105", 0x48);
+    /* The swift board expects a pca9551 but a pca9552 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "pca9552", 0x60);
+
+    /* The swift board expects an Epson RX8900 RTC but a ds1338 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "ds1338", 0x32);
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 8), "pca9552", 0x60);
+
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "tmp423", 0x4c);
+    /* The swift board expects a pca9539 but a pca9552 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "pca9552", 0x74);
+
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 10), "tmp423", 0x4c);
+    /* The swift board expects a pca9539 but a pca9552 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 10), "pca9552",
+                     0x74);
+
+    /* The swift board expects a TMP275 but a TMP105 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x48);
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 12), "tmp105", 0x4a);
+}
+
 static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
@@ -384,6 +424,16 @@ static const AspeedBoardConfig aspeed_boards[] = {
         .num_cs    = 2,
         .i2c_init  = romulus_bmc_i2c_init,
         .ram       = 512 * MiB,
+    }, {
+        .name      = MACHINE_TYPE_NAME("swift-bmc"),
+        .desc      = "OpenPOWER Swift BMC (ARM1176)",
+        .soc_name  = "ast2500-a1",
+        .hw_strap1 = SWIFT_BMC_HW_STRAP1,
+        .fmc_model = "mx66l1g45g",
+        .spi_model = "mx66l1g45g",
+        .num_cs    = 2,
+        .i2c_init  = swift_bmc_i2c_init,
+        .ram       = 512 * MiB,
     }, {
         .name      = MACHINE_TYPE_NAME("witherspoon-bmc"),
         .desc      = "OpenPOWER Witherspoon BMC (ARM1176)",
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 19/21] aspeed: Add support for the swift-bmc board

[Joel Stanley]

On Tue, 18 Jun 2019 at 16:55, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Adriana Kobylak <anoo@us.ibm.com>
>
> The Swift board is an OpenPOWER system hosting POWER processors.
> Add support for their BMC including the I2C devices as found on HW.
>
> Signed-off-by: Adriana Kobylak <anoo@us.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

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

[Qemu-devel] [PATCH v2 20/21] hw/misc/aspeed_xdma: New device

[Cédric Le Goater]

From: Eddie James <eajames@linux.ibm.com>

The XDMA engine embedded in the Aspeed SOCs performs PCI DMA operations
between the SOC (acting as a BMC) and a host processor in a server.

The XDMA engine exists on the AST2400, AST2500, and AST2600 SOCs, so
enable it for all of those. Add trace events on the important register
writes in the XDMA engine.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[clg: - changed title ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed_soc.h   |   3 +
 include/hw/misc/aspeed_xdma.h |  30 +++++++
 hw/arm/aspeed_soc.c           |  17 ++++
 hw/misc/aspeed_xdma.c         | 165 ++++++++++++++++++++++++++++++++++
 hw/misc/Makefile.objs         |   1 +
 hw/misc/trace-events          |   3 +
 6 files changed, 219 insertions(+)
 create mode 100644 include/hw/misc/aspeed_xdma.h
 create mode 100644 hw/misc/aspeed_xdma.c

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 75b557060b9b..cef605ad6bde 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -15,6 +15,7 @@
 #include "hw/intc/aspeed_vic.h"
 #include "hw/misc/aspeed_scu.h"
 #include "hw/misc/aspeed_sdmc.h"
+#include "hw/misc/aspeed_xdma.h"
 #include "hw/timer/aspeed_timer.h"
 #include "hw/timer/aspeed_rtc.h"
 #include "hw/i2c/aspeed_i2c.h"
@@ -40,6 +41,7 @@ typedef struct AspeedSoCState {
     AspeedTimerCtrlState timerctrl;
     AspeedI2CState i2c;
     AspeedSCUState scu;
+    AspeedXDMAState xdma;
     AspeedSMCState fmc;
     AspeedSMCState spi[ASPEED_SPIS_NUM];
     AspeedSDMCState sdmc;
@@ -108,6 +110,7 @@ enum {
     ASPEED_ETH1,
     ASPEED_ETH2,
     ASPEED_SDRAM,
+    ASPEED_XDMA,
 };
 
 #endif /* ASPEED_SOC_H */
diff --git a/include/hw/misc/aspeed_xdma.h b/include/hw/misc/aspeed_xdma.h
new file mode 100644
index 000000000000..00b45d931f87
--- /dev/null
+++ b/include/hw/misc/aspeed_xdma.h
@@ -0,0 +1,30 @@
+/*
+ * ASPEED XDMA Controller
+ * Eddie James <eajames@linux.ibm.com>
+ *
+ * Copyright (C) 2019 IBM Corp.
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ */
+
+#ifndef ASPEED_XDMA_H
+#define ASPEED_XDMA_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_XDMA "aspeed.xdma"
+#define ASPEED_XDMA(obj) OBJECT_CHECK(AspeedXDMAState, (obj), TYPE_ASPEED_XDMA)
+
+#define ASPEED_XDMA_NUM_REGS (ASPEED_XDMA_REG_SIZE / sizeof(uint32_t))
+#define ASPEED_XDMA_REG_SIZE 0x7C
+
+typedef struct AspeedXDMAState {
+    SysBusDevice parent;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    char bmc_cmdq_readp_set;
+    uint32_t regs[ASPEED_XDMA_NUM_REGS];
+} AspeedXDMAState;
+
+#endif /* ASPEED_XDMA_H */
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 8a1545545faf..146eeee8d320 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -32,6 +32,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
     [ASPEED_VIC]    = 0x1E6C0000,
     [ASPEED_SDMC]   = 0x1E6E0000,
     [ASPEED_SCU]    = 0x1E6E2000,
+    [ASPEED_XDMA]   = 0x1E6E7000,
     [ASPEED_ADC]    = 0x1E6E9000,
     [ASPEED_SRAM]   = 0x1E720000,
     [ASPEED_GPIO]   = 0x1E780000,
@@ -58,6 +59,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
     [ASPEED_VIC]    = 0x1E6C0000,
     [ASPEED_SDMC]   = 0x1E6E0000,
     [ASPEED_SCU]    = 0x1E6E2000,
+    [ASPEED_XDMA]   = 0x1E6E7000,
     [ASPEED_ADC]    = 0x1E6E9000,
     [ASPEED_SRAM]   = 0x1E720000,
     [ASPEED_GPIO]   = 0x1E780000,
@@ -104,6 +106,7 @@ static const int aspeed_soc_ast2400_irqmap[] = {
     [ASPEED_I2C]    = 12,
     [ASPEED_ETH1]   = 2,
     [ASPEED_ETH2]   = 3,
+    [ASPEED_XDMA]   = 6,
 };
 
 #define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
@@ -240,6 +243,9 @@ static void aspeed_soc_init(Object *obj)
         sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
                               sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
     }
+
+    sysbus_init_child_obj(obj, "xdma", OBJECT(&s->xdma), sizeof(s->xdma),
+                          TYPE_ASPEED_XDMA);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -408,6 +414,17 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
                            aspeed_soc_get_irq(s, ASPEED_ETH1 + i));
     }
+
+    /* XDMA */
+    object_property_set_bool(OBJECT(&s->xdma), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->xdma), 0,
+                    sc->info->memmap[ASPEED_XDMA]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->xdma), 0,
+                       aspeed_soc_get_irq(s, ASPEED_XDMA));
 }
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
diff --git a/hw/misc/aspeed_xdma.c b/hw/misc/aspeed_xdma.c
new file mode 100644
index 000000000000..eebd4ad540aa
--- /dev/null
+++ b/hw/misc/aspeed_xdma.c
@@ -0,0 +1,165 @@
+/*
+ * ASPEED XDMA Controller
+ * Eddie James <eajames@linux.ibm.com>
+ *
+ * Copyright (C) 2019 IBM Corp
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_xdma.h"
+#include "qapi/error.h"
+
+#include "trace.h"
+
+#define XDMA_BMC_CMDQ_ADDR         0x10
+#define XDMA_BMC_CMDQ_ENDP         0x14
+#define XDMA_BMC_CMDQ_WRP          0x18
+#define  XDMA_BMC_CMDQ_W_MASK      0x0003FFFF
+#define XDMA_BMC_CMDQ_RDP          0x1C
+#define  XDMA_BMC_CMDQ_RDP_MAGIC   0xEE882266
+#define XDMA_IRQ_ENG_CTRL          0x20
+#define  XDMA_IRQ_ENG_CTRL_US_COMP BIT(4)
+#define  XDMA_IRQ_ENG_CTRL_DS_COMP BIT(5)
+#define  XDMA_IRQ_ENG_CTRL_W_MASK  0xBFEFF07F
+#define XDMA_IRQ_ENG_STAT          0x24
+#define  XDMA_IRQ_ENG_STAT_US_COMP BIT(4)
+#define  XDMA_IRQ_ENG_STAT_DS_COMP BIT(5)
+#define  XDMA_IRQ_ENG_STAT_RESET   0xF8000000
+#define XDMA_MEM_SIZE              0x1000
+
+#define TO_REG(addr) ((addr) / sizeof(uint32_t))
+
+static uint64_t aspeed_xdma_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    uint32_t val = 0;
+    AspeedXDMAState *xdma = opaque;
+
+    if (addr < ASPEED_XDMA_REG_SIZE) {
+        val = xdma->regs[TO_REG(addr)];
+    }
+
+    return (uint64_t)val;
+}
+
+static void aspeed_xdma_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned int size)
+{
+    unsigned int idx;
+    uint32_t val32 = (uint32_t)val;
+    AspeedXDMAState *xdma = opaque;
+
+    if (addr >= ASPEED_XDMA_REG_SIZE) {
+        return;
+    }
+
+    switch (addr) {
+    case XDMA_BMC_CMDQ_ENDP:
+        xdma->regs[TO_REG(addr)] = val32 & XDMA_BMC_CMDQ_W_MASK;
+        break;
+    case XDMA_BMC_CMDQ_WRP:
+        idx = TO_REG(addr);
+        xdma->regs[idx] = val32 & XDMA_BMC_CMDQ_W_MASK;
+        xdma->regs[TO_REG(XDMA_BMC_CMDQ_RDP)] = xdma->regs[idx];
+
+        trace_aspeed_xdma_write(addr, val);
+
+        if (xdma->bmc_cmdq_readp_set) {
+            xdma->bmc_cmdq_readp_set = 0;
+        } else {
+            xdma->regs[TO_REG(XDMA_IRQ_ENG_STAT)] |=
+                XDMA_IRQ_ENG_STAT_US_COMP | XDMA_IRQ_ENG_STAT_DS_COMP;
+
+            if (xdma->regs[TO_REG(XDMA_IRQ_ENG_CTRL)] &
+                (XDMA_IRQ_ENG_CTRL_US_COMP | XDMA_IRQ_ENG_CTRL_DS_COMP))
+                qemu_irq_raise(xdma->irq);
+        }
+        break;
+    case XDMA_BMC_CMDQ_RDP:
+        trace_aspeed_xdma_write(addr, val);
+
+        if (val32 == XDMA_BMC_CMDQ_RDP_MAGIC) {
+            xdma->bmc_cmdq_readp_set = 1;
+        }
+        break;
+    case XDMA_IRQ_ENG_CTRL:
+        xdma->regs[TO_REG(addr)] = val32 & XDMA_IRQ_ENG_CTRL_W_MASK;
+        break;
+    case XDMA_IRQ_ENG_STAT:
+        trace_aspeed_xdma_write(addr, val);
+
+        idx = TO_REG(addr);
+        if (val32 & (XDMA_IRQ_ENG_STAT_US_COMP | XDMA_IRQ_ENG_STAT_DS_COMP)) {
+            xdma->regs[idx] &=
+                ~(XDMA_IRQ_ENG_STAT_US_COMP | XDMA_IRQ_ENG_STAT_DS_COMP);
+            qemu_irq_lower(xdma->irq);
+        }
+        break;
+    default:
+        xdma->regs[TO_REG(addr)] = val32;
+        break;
+    }
+}
+
+static const MemoryRegionOps aspeed_xdma_ops = {
+    .read = aspeed_xdma_read,
+    .write = aspeed_xdma_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+};
+
+static void aspeed_xdma_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedXDMAState *xdma = ASPEED_XDMA(dev);
+
+    sysbus_init_irq(sbd, &xdma->irq);
+    memory_region_init_io(&xdma->iomem, OBJECT(xdma), &aspeed_xdma_ops, xdma,
+                          TYPE_ASPEED_XDMA, XDMA_MEM_SIZE);
+    sysbus_init_mmio(sbd, &xdma->iomem);
+}
+
+static void aspeed_xdma_reset(DeviceState *dev)
+{
+    AspeedXDMAState *xdma = ASPEED_XDMA(dev);
+
+    xdma->bmc_cmdq_readp_set = 0;
+    memset(xdma->regs, 0, ASPEED_XDMA_REG_SIZE);
+    xdma->regs[TO_REG(XDMA_IRQ_ENG_STAT)] = XDMA_IRQ_ENG_STAT_RESET;
+
+    qemu_irq_lower(xdma->irq);
+}
+
+static const VMStateDescription aspeed_xdma_vmstate = {
+    .name = TYPE_ASPEED_XDMA,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedXDMAState, ASPEED_XDMA_NUM_REGS),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static void aspeed_xdma_class_init(ObjectClass *classp, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(classp);
+
+    dc->realize = aspeed_xdma_realize;
+    dc->reset = aspeed_xdma_reset;
+    dc->vmsd = &aspeed_xdma_vmstate;
+}
+
+static const TypeInfo aspeed_xdma_info = {
+    .name          = TYPE_ASPEED_XDMA,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedXDMAState),
+    .class_init    = aspeed_xdma_class_init,
+};
+
+static void aspeed_xdma_register_type(void)
+{
+    type_register_static(&aspeed_xdma_info);
+}
+type_init(aspeed_xdma_register_type);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 77b9df9796e3..e9aab519a1a4 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -74,6 +74,7 @@ obj-$(CONFIG_ARMSSE_MHU) += armsse-mhu.o
 
 obj-$(CONFIG_PVPANIC) += pvpanic.o
 obj-$(CONFIG_AUX) += auxbus.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_xdma.o
 obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
 obj-$(CONFIG_MSF2) += msf2-sysreg.o
 obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 47e1bccf71d5..c1ea1aa4376d 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -140,3 +140,6 @@ armsse_cpuid_write(uint64_t offset, uint64_t data, unsigned size) "SSE-200 CPU_I
 # armsse-mhu.c
 armsse_mhu_read(uint64_t offset, uint64_t data, unsigned size) "SSE-200 MHU read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 armsse_mhu_write(uint64_t offset, uint64_t data, unsigned size) "SSE-200 MHU write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
+
+# aspeed_xdma.c
+aspeed_xdma_write(uint64_t offset, uint64_t data) "XDMA write: offset 0x%" PRIx64 " data 0x%" PRIx64
-- 
2.21.0

[Qemu-devel] [PATCH v2 21/21] aspeed: vic: Add support for legacy register interface

[Cédric Le Goater]

From: Andrew Jeffery <andrew@aj.id.au>

The legacy interface only supported up to 32 IRQs, which became
restrictive around the AST2400 generation. QEMU support for the SoCs
started with the AST2400 along with an effort to reimplement and
upstream drivers for Linux, so up until this point the consumers of the
QEMU ASPEED support only required the 64 IRQ register interface.

In an effort to support older BMC firmware, add support for the 32 IRQ
interface.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/aspeed_vic.c | 105 ++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 42 deletions(-)

diff --git a/hw/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
index 927638d5322a..266a309f3b47 100644
--- a/hw/intc/aspeed_vic.c
+++ b/hw/intc/aspeed_vic.c
@@ -104,54 +104,63 @@ static void aspeed_vic_set_irq(void *opaque, int irq, int level)
 
 static uint64_t aspeed_vic_read(void *opaque, hwaddr offset, unsigned size)
 {
-    uint64_t val;
-    const bool high = !!(offset & 0x4);
-    hwaddr n_offset = (offset & ~0x4);
     AspeedVICState *s = (AspeedVICState *)opaque;
+    hwaddr n_offset;
+    uint64_t val;
+    bool high;
 
     if (offset < AVIC_NEW_BASE_OFFSET) {
-        qemu_log_mask(LOG_UNIMP, "%s: Ignoring read from legacy registers "
-                      "at 0x%" HWADDR_PRIx "[%u]\n", __func__, offset, size);
-        return 0;
+        high = false;
+        n_offset = offset;
+    } else {
+        high = !!(offset & 0x4);
+        n_offset = (offset & ~0x4);
     }
 
-    n_offset -= AVIC_NEW_BASE_OFFSET;
-
     switch (n_offset) {
-    case 0x0: /* IRQ Status */
+    case 0x80: /* IRQ Status */
+    case 0x00:
         val = s->raw & ~s->select & s->enable;
         break;
-    case 0x08: /* FIQ Status */
+    case 0x88: /* FIQ Status */
+    case 0x04:
         val = s->raw & s->select & s->enable;
         break;
-    case 0x10: /* Raw Interrupt Status */
+    case 0x90: /* Raw Interrupt Status */
+    case 0x08:
         val = s->raw;
         break;
-    case 0x18: /* Interrupt Selection */
+    case 0x98: /* Interrupt Selection */
+    case 0x0c:
         val = s->select;
         break;
-    case 0x20: /* Interrupt Enable */
+    case 0xa0: /* Interrupt Enable */
+    case 0x10:
         val = s->enable;
         break;
-    case 0x30: /* Software Interrupt */
+    case 0xb0: /* Software Interrupt */
+    case 0x18:
         val = s->trigger;
         break;
-    case 0x40: /* Interrupt Sensitivity */
+    case 0xc0: /* Interrupt Sensitivity */
+    case 0x24:
         val = s->sense;
         break;
-    case 0x48: /* Interrupt Both Edge Trigger Control */
+    case 0xc8: /* Interrupt Both Edge Trigger Control */
+    case 0x28:
         val = s->dual_edge;
         break;
-    case 0x50: /* Interrupt Event */
+    case 0xd0: /* Interrupt Event */
+    case 0x2c:
         val = s->event;
         break;
-    case 0x60: /* Edge Triggered Interrupt Status */
+    case 0xe0: /* Edge Triggered Interrupt Status */
         val = s->raw & ~s->sense;
         break;
         /* Illegal */
-    case 0x28: /* Interrupt Enable Clear */
-    case 0x38: /* Software Interrupt Clear */
-    case 0x58: /* Edge Triggered Interrupt Clear */
+    case 0xa8: /* Interrupt Enable Clear */
+    case 0xb8: /* Software Interrupt Clear */
+    case 0xd8: /* Edge Triggered Interrupt Clear */
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Read of write-only register with offset 0x%"
                       HWADDR_PRIx "\n", __func__, offset);
@@ -166,6 +175,8 @@ static uint64_t aspeed_vic_read(void *opaque, hwaddr offset, unsigned size)
     }
     if (high) {
         val = extract64(val, 32, 19);
+    } else {
+        val = extract64(val, 0, 32);
     }
     trace_aspeed_vic_read(offset, size, val);
     return val;
@@ -174,19 +185,18 @@ static uint64_t aspeed_vic_read(void *opaque, hwaddr offset, unsigned size)
 static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
                              unsigned size)
 {
-    const bool high = !!(offset & 0x4);
-    hwaddr n_offset = (offset & ~0x4);
     AspeedVICState *s = (AspeedVICState *)opaque;
+    hwaddr n_offset;
+    bool high;
 
     if (offset < AVIC_NEW_BASE_OFFSET) {
-        qemu_log_mask(LOG_UNIMP,
-                      "%s: Ignoring write to legacy registers at 0x%"
-                      HWADDR_PRIx "[%u] <- 0x%" PRIx64 "\n", __func__, offset,
-                      size, data);
-        return;
+        high = false;
+        n_offset = offset;
+    } else {
+        high = !!(offset & 0x4);
+        n_offset = (offset & ~0x4);
     }
 
-    n_offset -= AVIC_NEW_BASE_OFFSET;
     trace_aspeed_vic_write(offset, size, data);
 
     /* Given we have members using separate enable/clear registers, deposit64()
@@ -201,7 +211,8 @@ static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
     }
 
     switch (n_offset) {
-    case 0x18: /* Interrupt Selection */
+    case 0x98: /* Interrupt Selection */
+    case 0x0c:
         /* Register has deposit64() semantics - overwrite requested 32 bits */
         if (high) {
             s->select &= AVIC_L_MASK;
@@ -210,21 +221,25 @@ static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
         }
         s->select |= data;
         break;
-    case 0x20: /* Interrupt Enable */
+    case 0xa0: /* Interrupt Enable */
+    case 0x10:
         s->enable |= data;
         break;
-    case 0x28: /* Interrupt Enable Clear */
+    case 0xa8: /* Interrupt Enable Clear */
+    case 0x14:
         s->enable &= ~data;
         break;
-    case 0x30: /* Software Interrupt */
+    case 0xb0: /* Software Interrupt */
+    case 0x18:
         qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
                       "IRQs requested: 0x%016" PRIx64 "\n", __func__, data);
         break;
-    case 0x38: /* Software Interrupt Clear */
+    case 0xb8: /* Software Interrupt Clear */
+    case 0x1c:
         qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
                       "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
         break;
-    case 0x50: /* Interrupt Event */
+    case 0xd0: /* Interrupt Event */
         /* Register has deposit64() semantics - overwrite the top four valid
          * IRQ bits, as only the top four IRQs (GPIOs) can change their event
          * type */
@@ -236,15 +251,21 @@ static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
                           "Ignoring invalid write to interrupt event register");
         }
         break;
-    case 0x58: /* Edge Triggered Interrupt Clear */
+    case 0xd8: /* Edge Triggered Interrupt Clear */
+    case 0x38:
         s->raw &= ~(data & ~s->sense);
         break;
-    case 0x00: /* IRQ Status */
-    case 0x08: /* FIQ Status */
-    case 0x10: /* Raw Interrupt Status */
-    case 0x40: /* Interrupt Sensitivity */
-    case 0x48: /* Interrupt Both Edge Trigger Control */
-    case 0x60: /* Edge Triggered Interrupt Status */
+    case 0x80: /* IRQ Status */
+    case 0x00:
+    case 0x88: /* FIQ Status */
+    case 0x04:
+    case 0x90: /* Raw Interrupt Status */
+    case 0x08:
+    case 0xc0: /* Interrupt Sensitivity */
+    case 0x24:
+    case 0xc8: /* Interrupt Both Edge Trigger Control */
+    case 0x28:
+    case 0xe0: /* Edge Triggered Interrupt Status */
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Write of read-only register with offset 0x%"
                       HWADDR_PRIx "\n", __func__, offset);
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 21/21] aspeed: vic: Add support for legacy register interface

[Joel Stanley]

On Tue, 18 Jun 2019 at 16:55, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Andrew Jeffery <andrew@aj.id.au>
>
> The legacy interface only supported up to 32 IRQs, which became
> restrictive around the AST2400 generation. QEMU support for the SoCs
> started with the AST2400 along with an effort to reimplement and
> upstream drivers for Linux, so up until this point the consumers of the
> QEMU ASPEED support only required the 64 IRQ register interface.
>
> In an effort to support older BMC firmware, add support for the 32 IRQ
> interface.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

Re: [Qemu-devel] [PATCH v2 00/21] aspeed: machine extensions and fixes

[Peter Maydell]

On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello,
>
> This series improves the current models of the Aspeed machines in QEMU
> and adds new ones. It also prepares ground for the future Aspeed SoC.
> You will find patches for :
>
>  - per SoC mappings of the memory space and the interrupt number space
>  - support for multiple CPUs (improved)
>  - support for multiple NICs
>  - a RTC model from Joel
>  - a basic XDMA model from Eddie
>  - support for multiple CPUs and NICs
>  - fixes for the irq and timer models from Joel, Andrew and Christian
>  - DMA support for the SMC controller, which was reworked to use a RAM
>    container region as suggested by Peter in September 2018
>  - new swift-bmc machine from Adriana (P9' processor)

Hi; I've left review comments on some of the patches.
I'm going to take patches 1-9, 11, 12, 13, 14, 19, 20, 21
into target-arm.next (since we're nearly at softfreeze);
the others I either had comments on or they were dependent
on earlier patches in the series.

thanks
-- PMM