* [Qemu-devel] [PATCH 01/19] hw/arm/aspeed: Use object_initialize_child for correct ref. counting
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 02/19] aspeed: add a per SoC mapping for the interrupt space Cédric Le Goater
` (18 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Thomas Huth, Eduardo Habkost, Andrew Jeffery, qemu-devel,
qemu-arm, Joel Stanley, Paolo Bonzini,
Philippe Mathieu-Daudé,
Cédric Le Goater
From: Philippe Mathieu-Daudé <philmd@redhat.com>
As explained in commit aff39be0ed97:
Both functions, object_initialize() and object_property_add_child()
increase the reference counter of the new object, so one of the
references has to be dropped afterwards to get the reference
counting right. Otherwise the child object will not be properly
cleaned up when the parent gets destroyed.
Thus let's use now object_initialize_child() instead to get the
reference counting here right.
This patch was generated using the following Coccinelle script
(with a bit of manual fix-up for overly long lines):
@use_object_initialize_child@
expression parent_obj;
expression child_ptr;
expression child_name;
expression child_type;
expression child_size;
expression errp;
@@
(
- object_initialize(child_ptr, child_size, child_type);
+ object_initialize_child(parent_obj, child_name, child_ptr, child_size,
+ child_type, &error_abort, NULL);
... when != parent_obj
- object_property_add_child(parent_obj, child_name, OBJECT(child_ptr), NULL);
...
?- object_unref(OBJECT(child_ptr));
|
- object_initialize(child_ptr, child_size, child_type);
+ object_initialize_child(parent_obj, child_name, child_ptr, child_size,
+ child_type, errp, NULL);
... when != parent_obj
- object_property_add_child(parent_obj, child_name, OBJECT(child_ptr), errp);
...
?- object_unref(OBJECT(child_ptr));
)
@use_sysbus_init_child_obj@
expression parent_obj;
expression dev;
expression child_ptr;
expression child_name;
expression child_type;
expression child_size;
expression errp;
@@
(
- object_initialize_child(parent_obj, child_name, child_ptr, child_size,
- child_type, errp, NULL);
+ sysbus_init_child_obj(parent_obj, child_name, child_ptr, child_size,
+ child_type);
...
- qdev_set_parent_bus(DEVICE(child_ptr), sysbus_get_default());
|
- object_initialize_child(parent_obj, child_name, child_ptr, child_size,
- child_type, errp, NULL);
+ sysbus_init_child_obj(parent_obj, child_name, child_ptr, child_size,
+ child_type);
- dev = DEVICE(child_ptr);
- qdev_set_parent_bus(dev, sysbus_get_default());
)
While the object_initialize() function doesn't take an
'Error *errp' argument, the object_initialize_child() does.
Since this code is used when a machine is created (and is not
yet running), we deliberately choose to use the &error_abort
argument instead of ignoring errors if an object creation failed.
This choice also matches when using sysbus_init_child_obj(),
since its code is:
void sysbus_init_child_obj(Object *parent,
const char *childname, void *child,
size_t childsize, const char *childtype)
{
object_initialize_child(parent, childname, child, childsize,
childtype, &error_abort, NULL);
qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
}
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Inspired-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Message-Id: <20190507163416.24647-8-philmd@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/arm/aspeed.c | 6 +++---
hw/arm/aspeed_soc.c | 50 ++++++++++++++++++---------------------------
2 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 415cff7a015d..33070a6df843 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -160,9 +160,9 @@ static void aspeed_board_init(MachineState *machine,
ram_addr_t max_ram_size;
bmc = g_new0(AspeedBoardState, 1);
- object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
- object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
- &error_abort);
+ object_initialize_child(OBJECT(machine), "soc", &bmc->soc,
+ (sizeof(bmc->soc)), cfg->soc_name, &error_abort,
+ NULL);
sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index a27233d4876b..faff42b84ad1 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -106,12 +106,11 @@ static void aspeed_soc_init(Object *obj)
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
int i;
- object_initialize(&s->cpu, sizeof(s->cpu), sc->info->cpu_type);
- object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
+ object_initialize_child(obj, "cpu", OBJECT(&s->cpu), sizeof(s->cpu),
+ sc->info->cpu_type, &error_abort, NULL);
- object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
- object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
- qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
+ sysbus_init_child_obj(obj, "scu", OBJECT(&s->scu), sizeof(s->scu),
+ TYPE_ASPEED_SCU);
qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
sc->info->silicon_rev);
object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
@@ -121,36 +120,29 @@ static void aspeed_soc_init(Object *obj)
object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),
"hw-prot-key", &error_abort);
- object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
- object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
- qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
+ sysbus_init_child_obj(obj, "vic", OBJECT(&s->vic), sizeof(s->vic),
+ TYPE_ASPEED_VIC);
- object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
- object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
+ sysbus_init_child_obj(obj, "timerctrl", OBJECT(&s->timerctrl),
+ sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
OBJECT(&s->scu), &error_abort);
- qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
- object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
- object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL);
- qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default());
+ sysbus_init_child_obj(obj, "i2c", OBJECT(&s->i2c), sizeof(s->i2c),
+ TYPE_ASPEED_I2C);
- object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename);
- object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL);
- qdev_set_parent_bus(DEVICE(&s->fmc), sysbus_get_default());
+ sysbus_init_child_obj(obj, "fmc", OBJECT(&s->fmc), sizeof(s->fmc),
+ sc->info->fmc_typename);
object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
&error_abort);
for (i = 0; i < sc->info->spis_num; i++) {
- object_initialize(&s->spi[i], sizeof(s->spi[i]),
- sc->info->spi_typename[i]);
- object_property_add_child(obj, "spi[*]", OBJECT(&s->spi[i]), NULL);
- qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
+ sysbus_init_child_obj(obj, "spi[*]", OBJECT(&s->spi[i]),
+ sizeof(s->spi[i]), sc->info->spi_typename[i]);
}
- object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
- object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
- qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
+ sysbus_init_child_obj(obj, "sdmc", OBJECT(&s->sdmc), sizeof(s->sdmc),
+ TYPE_ASPEED_SDMC);
qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
sc->info->silicon_rev);
object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
@@ -159,16 +151,14 @@ static void aspeed_soc_init(Object *obj)
"max-ram-size", &error_abort);
for (i = 0; i < sc->info->wdts_num; i++) {
- object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
- object_property_add_child(obj, "wdt[*]", OBJECT(&s->wdt[i]), NULL);
- qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus_get_default());
+ sysbus_init_child_obj(obj, "wdt[*]", OBJECT(&s->wdt[i]),
+ sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev",
sc->info->silicon_rev);
}
- object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
- object_property_add_child(obj, "ftgmac100", OBJECT(&s->ftgmac100), NULL);
- qdev_set_parent_bus(DEVICE(&s->ftgmac100), sysbus_get_default());
+ sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100),
+ sizeof(s->ftgmac100), TYPE_FTGMAC100);
}
static void aspeed_soc_realize(DeviceState *dev, Error **errp)
--
2.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 02/19] aspeed: add a per SoC mapping for the interrupt space
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 01/19] hw/arm/aspeed: Use object_initialize_child for correct ref. counting Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 03/19] aspeed: add a per SoC mapping for the memory space Cédric Le Goater
` (17 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, qemu-devel, qemu-arm, Cédric Le Goater,
Philippe Mathieu-Daudé,
Joel Stanley
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 faff42b84ad1..0f661ba3879d 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 03/19] aspeed: add a per SoC mapping for the memory space
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 01/19] hw/arm/aspeed: Use object_initialize_child for correct ref. counting Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 02/19] aspeed: add a per SoC mapping for the interrupt space Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 04/19] hw: timer: Add ASPEED RTC device Cédric Le Goater
` (16 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, qemu-devel, qemu-arm, Cédric Le Goater,
Philippe Mathieu-Daudé,
Joel Stanley
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 33070a6df843..10ba3f50481a 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -192,8 +192,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);
@@ -202,7 +202,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);
@@ -230,7 +230,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 0f661ba3879d..4b705afd096a 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 04/19] hw: timer: Add ASPEED RTC device
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (2 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 03/19] aspeed: add a per SoC mapping for the memory space Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 05/19] hw/arm/aspeed: Add RTC to SoC Cédric Le Goater
` (15 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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>
Signed-off-by: Cédric Le Goater <clg@kaod.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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 05/19] hw/arm/aspeed: Add RTC to SoC
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (3 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 04/19] hw: timer: Add ASPEED RTC device Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine Cédric Le Goater
` (14 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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>
Signed-off-by: Cédric Le Goater <clg@kaod.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 4b705afd096a..d1dc8f03f35c 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (4 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 05/19] hw/arm/aspeed: Add RTC to SoC Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:32 ` Joel Stanley
2019-05-25 15:12 ` [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs Cédric Le Goater
` (13 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
The current models of the Aspeed SoCs only have one CPU but future
ones will support SMP. Introduce a way to configure the maximum number
of CPU per machine. SMP support will be activated when models for such
SoCs are implemented.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/arm/aspeed.h | 1 +
include/hw/arm/aspeed_soc.h | 3 ++-
hw/arm/aspeed.c | 8 ++++++--
hw/arm/aspeed_soc.c | 17 +++++++++++------
4 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 02073a6b4d61..f2f238ea83cc 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -23,6 +23,7 @@ typedef struct AspeedBoardConfig {
uint32_t num_cs;
void (*i2c_init)(AspeedBoardState *bmc);
uint32_t ram;
+ uint32_t num_cpus;
} AspeedBoardConfig;
#define TYPE_ASPEED_MACHINE MACHINE_TYPE_NAME("aspeed")
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index fa0ba957a611..7247f6da2505 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -24,13 +24,14 @@
#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];
MemoryRegion sram;
AspeedVICState vic;
AspeedRtcState rtc;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 10ba3f50481a..004b0c318951 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -29,7 +29,6 @@
static struct arm_boot_info aspeed_board_binfo = {
.board_id = -1, /* device-tree-only board */
- .nb_cpus = 1,
};
struct AspeedBoardState {
@@ -231,6 +230,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 = cfg->num_cpus;
if (cfg->i2c_init) {
cfg->i2c_init(bmc);
@@ -327,7 +327,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;
@@ -357,6 +357,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
.num_cs = 1,
.i2c_init = palmetto_bmc_i2c_init,
.ram = 256 * MiB,
+ .num_cpus = 1,
}, {
.name = MACHINE_TYPE_NAME("ast2500-evb"),
.desc = "Aspeed AST2500 EVB (ARM1176)",
@@ -367,6 +368,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
.num_cs = 1,
.i2c_init = ast2500_evb_i2c_init,
.ram = 512 * MiB,
+ .num_cpus = 1,
}, {
.name = MACHINE_TYPE_NAME("romulus-bmc"),
.desc = "OpenPOWER Romulus BMC (ARM1176)",
@@ -377,6 +379,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
.num_cs = 2,
.i2c_init = romulus_bmc_i2c_init,
.ram = 512 * MiB,
+ .num_cpus = 1,
}, {
.name = MACHINE_TYPE_NAME("witherspoon-bmc"),
.desc = "OpenPOWER Witherspoon BMC (ARM1176)",
@@ -387,6 +390,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
.num_cs = 2,
.i2c_init = witherspoon_bmc_i2c_init,
.ram = 512 * MiB,
+ .num_cpus = 1,
},
};
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index d1dc8f03f35c..b983d5efc5d1 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -172,8 +172,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 < MIN(smp_cpus, ASPEED_CPUS_NUM); 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);
@@ -242,10 +245,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
ASPEED_SOC_IOMEM_SIZE);
/* CPU */
- object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
- if (err) {
- error_propagate(errp, err);
- return;
+ for (i = 0; i < smp_cpus; i++) {
+ object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
}
/* SRAM */
--
2.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine
2019-05-25 15:12 ` [Qemu-devel] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine Cédric Le Goater
@ 2019-06-12 1:32 ` Joel Stanley
2019-06-12 8:03 ` Cédric Le Goater
0 siblings, 1 reply; 44+ messages in thread
From: Joel Stanley @ 2019-06-12 1:32 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers
On Sat, 25 May 2019 at 15:13, 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 way to configure the maximum number
> of CPU per machine. SMP support will be activated when models for such
> SoCs are implemented.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/arm/aspeed.h | 1 +
> include/hw/arm/aspeed_soc.h | 3 ++-
> hw/arm/aspeed.c | 8 ++++++--
> hw/arm/aspeed_soc.c | 17 +++++++++++------
> 4 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index 02073a6b4d61..f2f238ea83cc 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -23,6 +23,7 @@ typedef struct AspeedBoardConfig {
> uint32_t num_cs;
> void (*i2c_init)(AspeedBoardState *bmc);
> uint32_t ram;
> + uint32_t num_cpus;
> } AspeedBoardConfig;
>
> #define TYPE_ASPEED_MACHINE MACHINE_TYPE_NAME("aspeed")
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index fa0ba957a611..7247f6da2505 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -24,13 +24,14 @@
>
> #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];
> MemoryRegion sram;
> AspeedVICState vic;
> AspeedRtcState rtc;
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 10ba3f50481a..004b0c318951 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -29,7 +29,6 @@
>
> static struct arm_boot_info aspeed_board_binfo = {
> .board_id = -1, /* device-tree-only board */
> - .nb_cpus = 1,
> };
>
> struct AspeedBoardState {
> @@ -231,6 +230,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 = cfg->num_cpus;
>
> if (cfg->i2c_init) {
> cfg->i2c_init(bmc);
> @@ -327,7 +327,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;
> @@ -357,6 +357,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
> .num_cs = 1,
> .i2c_init = palmetto_bmc_i2c_init,
> .ram = 256 * MiB,
> + .num_cpus = 1,
> }, {
> .name = MACHINE_TYPE_NAME("ast2500-evb"),
> .desc = "Aspeed AST2500 EVB (ARM1176)",
> @@ -367,6 +368,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
> .num_cs = 1,
> .i2c_init = ast2500_evb_i2c_init,
> .ram = 512 * MiB,
> + .num_cpus = 1,
> }, {
> .name = MACHINE_TYPE_NAME("romulus-bmc"),
> .desc = "OpenPOWER Romulus BMC (ARM1176)",
> @@ -377,6 +379,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
> .num_cs = 2,
> .i2c_init = romulus_bmc_i2c_init,
> .ram = 512 * MiB,
> + .num_cpus = 1,
> }, {
> .name = MACHINE_TYPE_NAME("witherspoon-bmc"),
> .desc = "OpenPOWER Witherspoon BMC (ARM1176)",
> @@ -387,6 +390,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
> .num_cs = 2,
> .i2c_init = witherspoon_bmc_i2c_init,
> .ram = 512 * MiB,
> + .num_cpus = 1,
> },
> };
>
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index d1dc8f03f35c..b983d5efc5d1 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -172,8 +172,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 < MIN(smp_cpus, ASPEED_CPUS_NUM); i++) {
What's the intent of this test?
If we're checking that the user hasn't requested more CPUs that the
SoC supports, shoudln't it be testing against ->num_cpus instead of
ASPEED_CPUS_NUM?
> + 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);
> @@ -242,10 +245,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> ASPEED_SOC_IOMEM_SIZE);
>
> /* CPU */
> - object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> + for (i = 0; i < smp_cpus; i++) {
Can you explain why we use smp_cpus instead of ->num_cpus?
> + object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> }
>
> /* SRAM */
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine
2019-06-12 1:32 ` Joel Stanley
@ 2019-06-12 8:03 ` Cédric Le Goater
0 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-06-12 8:03 UTC (permalink / raw)
To: Joel Stanley; +Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers
On 12/06/2019 03:32, Joel Stanley wrote:
> On Sat, 25 May 2019 at 15:13, 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 way to configure the maximum number
>> of CPU per machine. SMP support will be activated when models for such
>> SoCs are implemented.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> include/hw/arm/aspeed.h | 1 +
>> include/hw/arm/aspeed_soc.h | 3 ++-
>> hw/arm/aspeed.c | 8 ++++++--
>> hw/arm/aspeed_soc.c | 17 +++++++++++------
>> 4 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index 02073a6b4d61..f2f238ea83cc 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -23,6 +23,7 @@ typedef struct AspeedBoardConfig {
>> uint32_t num_cs;
>> void (*i2c_init)(AspeedBoardState *bmc);
>> uint32_t ram;
>> + uint32_t num_cpus;
>> } AspeedBoardConfig;
>>
>> #define TYPE_ASPEED_MACHINE MACHINE_TYPE_NAME("aspeed")
>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
>> index fa0ba957a611..7247f6da2505 100644
>> --- a/include/hw/arm/aspeed_soc.h
>> +++ b/include/hw/arm/aspeed_soc.h
>> @@ -24,13 +24,14 @@
>>
>> #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];
>> MemoryRegion sram;
>> AspeedVICState vic;
>> AspeedRtcState rtc;
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 10ba3f50481a..004b0c318951 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -29,7 +29,6 @@
>>
>> static struct arm_boot_info aspeed_board_binfo = {
>> .board_id = -1, /* device-tree-only board */
>> - .nb_cpus = 1,
>> };
>>
>> struct AspeedBoardState {
>> @@ -231,6 +230,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 = cfg->num_cpus;
>>
>> if (cfg->i2c_init) {
>> cfg->i2c_init(bmc);
>> @@ -327,7 +327,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;
>> @@ -357,6 +357,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>> .num_cs = 1,
>> .i2c_init = palmetto_bmc_i2c_init,
>> .ram = 256 * MiB,
>> + .num_cpus = 1,
>> }, {
>> .name = MACHINE_TYPE_NAME("ast2500-evb"),
>> .desc = "Aspeed AST2500 EVB (ARM1176)",
>> @@ -367,6 +368,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>> .num_cs = 1,
>> .i2c_init = ast2500_evb_i2c_init,
>> .ram = 512 * MiB,
>> + .num_cpus = 1,
>> }, {
>> .name = MACHINE_TYPE_NAME("romulus-bmc"),
>> .desc = "OpenPOWER Romulus BMC (ARM1176)",
>> @@ -377,6 +379,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>> .num_cs = 2,
>> .i2c_init = romulus_bmc_i2c_init,
>> .ram = 512 * MiB,
>> + .num_cpus = 1,
>> }, {
>> .name = MACHINE_TYPE_NAME("witherspoon-bmc"),
>> .desc = "OpenPOWER Witherspoon BMC (ARM1176)",
>> @@ -387,6 +390,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>> .num_cs = 2,
>> .i2c_init = witherspoon_bmc_i2c_init,
>> .ram = 512 * MiB,
>> + .num_cpus = 1,
>> },
>> };
>>
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index d1dc8f03f35c..b983d5efc5d1 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -172,8 +172,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 < MIN(smp_cpus, ASPEED_CPUS_NUM); i++) {
>
> What's the intent of this test?
>
> If we're checking that the user hasn't requested more CPUs that the
> SoC supports, shoudln't it be testing against ->num_cpus instead of
> ASPEED_CPUS_NUM?
yes but we are in the SoC and ->num_cpus is currently a board level
information. As we are in the _init routine I can't use a SoC property.
This can be improved. I will send a v2.
Thanks,
C.
>
>> + 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);
>> @@ -242,10 +245,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>> ASPEED_SOC_IOMEM_SIZE);
>>
>> /* CPU */
>> - object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
>> - if (err) {
>> - error_propagate(errp, err);
>> - return;
>> + for (i = 0; i < smp_cpus; i++) {
>
> Can you explain why we use smp_cpus instead of ->num_cpus?
>
>> + object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>> + if (err) {
>> + error_propagate(errp, err);
>> + return;
>> + }
>> }
>>
>> /* SRAM */
>> --
>> 2.20.1
>>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (5 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 06/19] aspeed: introduce a configurable number of CPU per machine Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-26 1:01 ` Keno Fischer
2019-05-25 15:12 ` [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux Cédric Le Goater
` (12 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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 7247f6da2505..e8556abf4737 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 >*/
@@ -42,7 +43,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 b983d5efc5d1..8cfe9e9515ed 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -229,8 +229,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)
@@ -371,19 +373,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 void aspeed_soc_class_init(ObjectClass *oc, void *data)
--
2.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs
2019-05-25 15:12 ` [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs Cédric Le Goater
@ 2019-05-26 1:01 ` Keno Fischer
2019-05-26 17:10 ` Cédric Le Goater
0 siblings, 1 reply; 44+ messages in thread
From: Keno Fischer @ 2019-05-26 1:01 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers, Joel Stanley
Drive by comment, since I spotted this in my inbox.
When I tried to make this change (two years ago though),
I additionally needed the following. Unfortunately, I don't quite remember
exactly what the issue was, but I think qemu would crash trying to create more
than one nic.
---
hw/net/ftgmac100.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 8127d0532dc..1318de85a4e 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -977,7 +977,8 @@ static void ftgmac100_realize(DeviceState *dev,
Error **errp)
sysbus_init_irq(sbd, &s->irq);
qemu_macaddr_default_if_unset(&s->conf.macaddr);
- s->conf.peers.ncs[0] = nd_table[0].netdev;
+ char *netdev_id = object_property_get_str(OBJECT(dev), "netdev", NULL);
+ s->conf.peers.ncs[0] = qemu_find_netdev(netdev_id);
s->nic = qemu_new_nic(&net_ftgmac100_info, &s->conf,
object_get_typename(OBJECT(dev)), DEVICE(dev)->id,
On Sat, May 25, 2019 at 11:22 AM 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 7247f6da2505..e8556abf4737 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 >*/
> @@ -42,7 +43,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 b983d5efc5d1..8cfe9e9515ed 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -229,8 +229,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)
> @@ -371,19 +373,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 void aspeed_soc_class_init(ObjectClass *oc, void *data)
> --
> 2.20.1
>
>
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs
2019-05-26 1:01 ` Keno Fischer
@ 2019-05-26 17:10 ` Cédric Le Goater
0 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-26 17:10 UTC (permalink / raw)
To: Keno Fischer
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers, Joel Stanley
On 26/05/2019 03:01, Keno Fischer wrote:
> Drive by comment, since I spotted this in my inbox.
> When I tried to make this change (two years ago though),
> I additionally needed the following. Unfortunately, I don't quite remember
> exactly what the issue was, but I think qemu would crash trying to create more
> than one nic.
Yes.
The fix was :
http://patchwork.ozlabs.org/patch/1102295/
Thanks,
C.
> ---
> hw/net/ftgmac100.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 8127d0532dc..1318de85a4e 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -977,7 +977,8 @@ static void ftgmac100_realize(DeviceState *dev,
> Error **errp)
> sysbus_init_irq(sbd, &s->irq);
> qemu_macaddr_default_if_unset(&s->conf.macaddr);
>
> - s->conf.peers.ncs[0] = nd_table[0].netdev;
> + char *netdev_id = object_property_get_str(OBJECT(dev), "netdev", NULL);
> + s->conf.peers.ncs[0] = qemu_find_netdev(netdev_id);
>
> s->nic = qemu_new_nic(&net_ftgmac100_info, &s->conf,
> object_get_typename(OBJECT(dev)), DEVICE(dev)->id,
>
>
>
> On Sat, May 25, 2019 at 11:22 AM 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 7247f6da2505..e8556abf4737 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 >*/
>> @@ -42,7 +43,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 b983d5efc5d1..8cfe9e9515ed 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -229,8 +229,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)
>> @@ -371,19 +373,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 void aspeed_soc_class_init(ObjectClass *oc, void *data)
>> --
>> 2.20.1
>>
>>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (6 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 07/19] aspeed: add support for multiple NICs Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2023-09-22 13:21 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 09/19] aspeed/timer: Status register contains reload for stopped timer Cédric Le Goater
` (11 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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 5c786e512815..9ffd8e09f670 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2019-05-25 15:12 ` [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux Cédric Le Goater
@ 2023-09-22 13:21 ` Cédric Le Goater
2023-09-25 7:54 ` Andrew Jeffery
2023-09-27 2:12 ` Joel Stanley
0 siblings, 2 replies; 44+ messages in thread
From: Cédric Le Goater @ 2023-09-22 13:21 UTC (permalink / raw)
To: Peter Maydell; +Cc: Andrew Jeffery, qemu-arm, qemu-devel, Joel Stanley
Joel, Andrew,
On 5/25/19 17:12, Cédric Le Goater wrote:
> 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 5c786e512815..9ffd8e09f670 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));
This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
from ? Thanks,
C.
> }
>
> static void aspeed_timer_mod(AspeedTimer *t)
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2023-09-22 13:21 ` Cédric Le Goater
@ 2023-09-25 7:54 ` Andrew Jeffery
2023-09-25 9:20 ` Cédric Le Goater
2023-09-27 2:12 ` Joel Stanley
1 sibling, 1 reply; 44+ messages in thread
From: Andrew Jeffery @ 2023-09-25 7:54 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Philippe Mathieu-Daudé via, Cameron Esfahani via, Joel Stanley
On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote:
> Joel, Andrew,
>
> On 5/25/19 17:12, Cédric Le Goater wrote:
>> 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 5c786e512815..9ffd8e09f670 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));
>
> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
> from ? Thanks,
The inner MAX() deals with the lack of ordering constraints between the match values. I think the outer MAX() is redundant. We should probably remove it. The match member is type uint32_t so it can't be negative. You did steal that from an RFC patch :D
Andrew
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2023-09-25 7:54 ` Andrew Jeffery
@ 2023-09-25 9:20 ` Cédric Le Goater
2023-09-26 1:37 ` Andrew Jeffery
0 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2023-09-25 9:20 UTC (permalink / raw)
To: Andrew Jeffery, Peter Maydell
Cc: Philippe Mathieu-Daudé via, Cameron Esfahani via, Joel Stanley
On 9/25/23 09:54, Andrew Jeffery wrote:
>
>
> On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote:
>> Joel, Andrew,
>>
>> On 5/25/19 17:12, Cédric Le Goater wrote:
>>> 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 5c786e512815..9ffd8e09f670 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));
>>
>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
>> from ? Thanks,
>
> The inner MAX() deals with the lack of ordering constraints between the match values. I think the outer MAX() is redundant. We should probably remove it. The match member is type uint32_t so it can't be negative. You did steal that from an RFC patch :D
I did ! Fixed there :
https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-clg@kaod.org/
Cheers,
C.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2023-09-25 9:20 ` Cédric Le Goater
@ 2023-09-26 1:37 ` Andrew Jeffery
0 siblings, 0 replies; 44+ messages in thread
From: Andrew Jeffery @ 2023-09-26 1:37 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Philippe Mathieu-Daudé via, Cameron Esfahani via, Joel Stanley
On Mon, 25 Sep 2023, at 18:50, Cédric Le Goater wrote:
> On 9/25/23 09:54, Andrew Jeffery wrote:
>>
>>
>> On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote:
>>> Joel, Andrew,
>>>
>>> On 5/25/19 17:12, Cédric Le Goater wrote:
>>>> 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 5c786e512815..9ffd8e09f670 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));
>>>
>>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
>>> from ? Thanks,
>>
>> The inner MAX() deals with the lack of ordering constraints between the match values. I think the outer MAX() is redundant. We should probably remove it. The match member is type uint32_t so it can't be negative. You did steal that from an RFC patch :D
>
> I did ! Fixed there :
>
>
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-clg@kaod.org/
>
Thanks. That one might be further down in my review queue 😅
Andrew
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2023-09-22 13:21 ` Cédric Le Goater
2023-09-25 7:54 ` Andrew Jeffery
@ 2023-09-27 2:12 ` Joel Stanley
2023-09-27 5:44 ` Cédric Le Goater
1 sibling, 1 reply; 44+ messages in thread
From: Joel Stanley @ 2023-09-27 2:12 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell; +Cc: Andrew Jeffery, qemu-arm, qemu-devel
On Fri, 22 Sept 2023 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
> > + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
>
> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
> from ? Thanks,
That looks very strange. I think you've sorted it, so I wanted to
bring up the MAX macros themselves. It's unfortunate that they create
a non-unique local variable. Are we allowed to borrow the kernels
macros? They have some infrastructure for creating a unique local
variable name, as well as handling the const and non-const variants
with the one macro.
https://elixir.bootlin.com/linux/v6.5/source/include/linux/minmax.h
Cheers,
Joel
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2023-09-27 2:12 ` Joel Stanley
@ 2023-09-27 5:44 ` Cédric Le Goater
2023-09-27 6:46 ` Markus Armbruster
0 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2023-09-27 5:44 UTC (permalink / raw)
To: Joel Stanley, Peter Maydell; +Cc: Andrew Jeffery, qemu-arm, qemu-devel
On 9/27/23 04:12, Joel Stanley wrote:
> On Fri, 22 Sept 2023 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
>>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
>>
>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
>> from ? Thanks,
>
> That looks very strange. I think you've sorted it, so I wanted to
> bring up the MAX macros themselves. It's unfortunate that they create
> a non-unique local variable. Are we allowed to borrow the kernels
> macros? They have some infrastructure for creating a unique local
> variable name, as well as handling the const and non-const variants
> with the one macro.
>
> https://elixir.bootlin.com/linux/v6.5/source/include/linux/minmax.h
I believe that's what Markus does in its own way :
https://lore.kernel.org/qemu-devel/20230921121312.1301864-8-armbru@redhat.com/
Thanks,
C.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
2023-09-27 5:44 ` Cédric Le Goater
@ 2023-09-27 6:46 ` Markus Armbruster
0 siblings, 0 replies; 44+ messages in thread
From: Markus Armbruster @ 2023-09-27 6:46 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Joel Stanley, Peter Maydell, Andrew Jeffery, qemu-arm, qemu-devel
Cédric Le Goater <clg@kaod.org> writes:
> On 9/27/23 04:12, Joel Stanley wrote:
>> On Fri, 22 Sept 2023 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>>>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
>>>
>>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
>>> from ? Thanks,
>> That looks very strange. I think you've sorted it, so I wanted to
>> bring up the MAX macros themselves. It's unfortunate that they create
>> a non-unique local variable. Are we allowed to borrow the kernels
>> macros? They have some infrastructure for creating a unique local
>> variable name, as well as handling the const and non-const variants
>> with the one macro.
>> https://elixir.bootlin.com/linux/v6.5/source/include/linux/minmax.h
>
> I believe that's what Markus does in its own way :
>
> https://lore.kernel.org/qemu-devel/20230921121312.1301864-8-armbru@redhat.com/
I wasn't aware of the kernel's infrastructure. We can steal it if
people think it provides additional value. Make your case :)
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 09/19] aspeed/timer: Status register contains reload for stopped timer
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (7 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 10/19] aspeed/timer: Fix match calculations Cédric Le Goater
` (10 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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 9ffd8e09f670..2f8522a597d3 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 10/19] aspeed/timer: Fix match calculations
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (8 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 09/19] aspeed/timer: Status register contains reload for stopped timer Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 11/19] aspeed/timer: Provide back-pressure information for short periods Cédric Le Goater
` (9 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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 2f8522a597d3..6c184572ea04 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 11/19] aspeed/timer: Provide back-pressure information for short periods
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (9 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 10/19] aspeed/timer: Fix match calculations Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 12/19] aspeed/timer: Ensure positive muldiv delta Cédric Le Goater
` (8 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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 ab1e18ed4b0f..a7b37e5ece01 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -422,6 +422,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 6c184572ea04..9988b8fbbf17 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 12/19] aspeed/timer: Ensure positive muldiv delta
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (10 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 11/19] aspeed/timer: Provide back-pressure information for short periods Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-05-25 15:12 ` [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie Cédric Le Goater
` (7 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Christian Svensson, Andrew Jeffery, qemu-devel, qemu-arm,
Cédric Le Goater, Joel Stanley
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 9988b8fbbf17..53b70f859c86 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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (11 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 12/19] aspeed/timer: Ensure positive muldiv delta Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:34 ` Joel Stanley
2019-06-13 14:32 ` Philippe Mathieu-Daudé
2019-05-25 15:12 ` [Qemu-devel] [PATCH 14/19] aspeed: remove the "ram" link Cédric Le Goater
` (6 subsequent siblings)
19 siblings, 2 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
The DRAM address of a DMA transaction depends on the DRAM base address
of the SoC. Inform the SMC controller model of this value.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
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 8cfe9e9515ed..65fbac896c85 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -326,6 +326,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 f1e66870d71f..4ff12f7b27fc 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -912,6 +912,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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie
2019-05-25 15:12 ` [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie Cédric Le Goater
@ 2019-06-12 1:34 ` Joel Stanley
2019-06-13 14:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 44+ messages in thread
From: Joel Stanley @ 2019-06-12 1:34 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers
On Sat, 25 May 2019 at 15:14, Cédric Le Goater <clg@kaod.org> wrote:
>
> The DRAM address of a DMA transaction depends on the DRAM base address
> of the SoC. Inform the SMC controller model of this value.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Nit: s/propertie/property/ in the patch subject.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie
2019-05-25 15:12 ` [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie Cédric Le Goater
2019-06-12 1:34 ` Joel Stanley
@ 2019-06-13 14:32 ` Philippe Mathieu-Daudé
2019-06-14 11:49 ` Cédric Le Goater
1 sibling, 1 reply; 44+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-13 14:32 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, qemu-arm, qemu-devel, Joel Stanley
On 5/25/19 5:12 PM, Cédric Le Goater wrote:
> The DRAM address of a DMA transaction depends on the DRAM base address
> of the SoC. Inform the SMC controller model of this value.
I'd reorder this one previous patch #16 "aspeed/smc: add support for
DMAs" where you start to use sdram_base.
Regardless,
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 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 8cfe9e9515ed..65fbac896c85 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -326,6 +326,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 f1e66870d71f..4ff12f7b27fc 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -912,6 +912,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(),
> };
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie
2019-06-13 14:32 ` Philippe Mathieu-Daudé
@ 2019-06-14 11:49 ` Cédric Le Goater
0 siblings, 0 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-06-14 11:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell
Cc: Andrew Jeffery, qemu-arm, qemu-devel, Joel Stanley
On 13/06/2019 16:32, Philippe Mathieu-Daudé wrote:
> On 5/25/19 5:12 PM, Cédric Le Goater wrote:
>> The DRAM address of a DMA transaction depends on the DRAM base address
>> of the SoC. Inform the SMC controller model of this value.
>
> I'd reorder this one previous patch #16 "aspeed/smc: add support for
> DMAs" where you start to use sdram_base.
>
> Regardless,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
done.
Thanks,
C.
>
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> 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 8cfe9e9515ed..65fbac896c85 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -326,6 +326,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 f1e66870d71f..4ff12f7b27fc 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -912,6 +912,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(),
>> };
>>
>>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 14/19] aspeed: remove the "ram" link
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (12 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 13/19] aspeed/smc: add a 'sdram_base' propertie Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:35 ` Joel Stanley
2019-05-25 15:12 ` [Qemu-devel] [PATCH 15/19] aspeed: add a RAM memory region container Cédric Le Goater
` (5 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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>
---
hw/arm/aspeed.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 004b0c318951..228fdbcf65e2 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -193,8 +193,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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 15/19] aspeed: add a RAM memory region container
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (13 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 14/19] aspeed: remove the "ram" link Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:36 ` Joel Stanley
2019-05-25 15:12 ` [Qemu-devel] [PATCH 16/19] aspeed/smc: add support for DMAs Cédric Le Goater
` (4 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
The RAM memory region is defined after the SoC is realized when the
SDMC controller has cheched 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>
---
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 228fdbcf65e2..ed5c8dd5133a 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);
@@ -191,16 +196,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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 15/19] aspeed: add a RAM memory region container
2019-05-25 15:12 ` [Qemu-devel] [PATCH 15/19] aspeed: add a RAM memory region container Cédric Le Goater
@ 2019-06-12 1:36 ` Joel Stanley
0 siblings, 0 replies; 44+ messages in thread
From: Joel Stanley @ 2019-06-12 1:36 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers
On Sat, 25 May 2019 at 15:14, Cédric Le Goater <clg@kaod.org> wrote:
>
> The RAM memory region is defined after the SoC is realized when the
> SDMC controller has cheched that the defined RAM size for the machine
checked
> 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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 16/19] aspeed/smc: add support for DMAs
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (14 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 15/19] aspeed: add a RAM memory region container Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:38 ` Joel Stanley
2019-05-25 15:12 ` [Qemu-devel] [PATCH 17/19] aspeed/smc: add DMA calibration settings Cédric Le Goater
` (3 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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>
---
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 ed5c8dd5133a..f1722ee04ac9 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -176,6 +176,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_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 65fbac896c85..16508f1e1844 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -207,6 +207,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 4ff12f7b27fc..b3c8e0a84f8b 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -23,10 +23,12 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "hw/sysbus.h"
#include "sysemu/sysemu.h"
#include "qemu/log.h"
#include "qemu/error-report.h"
+#include "exec/address-spaces.h"
#include "hw/ssi/aspeed_smc.h"
@@ -109,8 +111,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)
@@ -142,6 +144,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) */
@@ -211,6 +231,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",
@@ -237,6 +259,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",
@@ -729,9 +753,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;
@@ -772,6 +793,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];
@@ -782,6 +808,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)
{
@@ -807,6 +982,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);
@@ -821,6 +1006,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);
@@ -846,10 +1053,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);
@@ -896,6 +1105,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 = {
@@ -913,6 +1127,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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 16/19] aspeed/smc: add support for DMAs
2019-05-25 15:12 ` [Qemu-devel] [PATCH 16/19] aspeed/smc: add support for DMAs Cédric Le Goater
@ 2019-06-12 1:38 ` Joel Stanley
0 siblings, 0 replies; 44+ messages in thread
From: Joel Stanley @ 2019-06-12 1:38 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers
On Sat, 25 May 2019 at 15:14, 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>
Cédric is our flash controller expert. This looks good as far as I can tell.
Acked-by: Joel Stanley <joel@jms.id.au>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 17/19] aspeed/smc: add DMA calibration settings
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (15 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 16/19] aspeed/smc: add support for DMAs Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:40 ` Joel Stanley
2019-05-25 15:12 ` [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum Cédric Le Goater
` (2 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
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>
---
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 b3c8e0a84f8b..406c30c60b3f 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -74,6 +74,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
@@ -109,7 +113,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)
@@ -808,6 +812,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.
@@ -823,6 +881,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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (16 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 17/19] aspeed/smc: add DMA calibration settings Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:41 ` Joel Stanley
2019-06-13 14:31 ` Philippe Mathieu-Daudé
2019-05-25 15:12 ` [Qemu-devel] [PATCH 19/19] aspeed/smc: Calculate checksum on normal DMA Cédric Le Goater
2019-06-07 10:10 ` [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Peter Maydell
19 siblings, 2 replies; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Andrew Jeffery, Cédric Le Goater, qemu-arm, qemu-devel,
Joel Stanley
Emulate read errors in the DMA Checksum Register for high frequencies
and optimistic settings of the Read Timing Compensation Register. This
will help in tuning the SPI timing calibration algorithm.
The values below are those to expect from the first flash device of
the FMC controller of a palmetto-bmc machine.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ssi/aspeed_smc.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 406c30c60b3f..4c162912cf62 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -866,6 +866,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
}
+static bool aspeed_smc_inject_read_failure(AspeedSMCState *s)
+{
+ uint8_t delay =
+ (s->regs[R_DMA_CTRL] >> DMA_CTRL_DELAY_SHIFT) & DMA_CTRL_DELAY_MASK;
+ uint8_t hclk_mask =
+ (s->regs[R_DMA_CTRL] >> DMA_CTRL_FREQ_SHIFT) & DMA_CTRL_FREQ_MASK;
+
+ /*
+ * Typical values of a palmetto-bmc machine.
+ */
+ switch (aspeed_smc_hclk_divisor(hclk_mask)) {
+ case 4 ... 16:
+ return false;
+ case 3: /* at least one HCLK cycle delay */
+ return (delay & 0x7) < 1;
+ case 2: /* at least two HCLK cycle delay */
+ return (delay & 0x7) < 2;
+ case 1: /* (> 100MHz) is above the max freq of the controller */
+ return true;
+ default:
+ g_assert_not_reached();
+ }
+}
+
/*
* Accumulate the result of the reads to provide a checksum that will
* be used to validate the read timing settings.
@@ -903,6 +927,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
s->regs[R_DMA_FLASH_ADDR] += 4;
s->regs[R_DMA_LEN] -= 4;
}
+
+ if (aspeed_smc_inject_read_failure(s)) {
+ s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
+ }
+
}
static void aspeed_smc_dma_rw(AspeedSMCState *s)
--
2.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum
2019-05-25 15:12 ` [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum Cédric Le Goater
@ 2019-06-12 1:41 ` Joel Stanley
2019-06-13 14:31 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 44+ messages in thread
From: Joel Stanley @ 2019-06-12 1:41 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers
On Sat, 25 May 2019 at 15:14, Cédric Le Goater <clg@kaod.org> wrote:
>
> 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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum
2019-05-25 15:12 ` [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum Cédric Le Goater
2019-06-12 1:41 ` Joel Stanley
@ 2019-06-13 14:31 ` Philippe Mathieu-Daudé
2019-06-14 12:02 ` Cédric Le Goater
1 sibling, 1 reply; 44+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-13 14:31 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, qemu-arm, qemu-devel, Joel Stanley
Hi Cédric,
On 5/25/19 5:12 PM, Cédric Le Goater wrote:
> Emulate read errors in the DMA Checksum Register for high frequencies
> and optimistic settings of the Read Timing Compensation Register. This
> will help in tuning the SPI timing calibration algorithm.
>
> The values below are those to expect from the first flash device of
> the FMC controller of a palmetto-bmc machine.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ssi/aspeed_smc.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 406c30c60b3f..4c162912cf62 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -866,6 +866,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
> s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
> }
>
Can you add a comment (like the patch description) here?
> +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.
> @@ -903,6 +927,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
> s->regs[R_DMA_FLASH_ADDR] += 4;
> s->regs[R_DMA_LEN] -= 4;
> }
> +
> + if (aspeed_smc_inject_read_failure(s)) {
So this model real world where noise eventually triggers errors. Don't
we want this to be enable by the user (or a QMP command)?
> + s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
> + }
> +
> }
>
> static void aspeed_smc_dma_rw(AspeedSMCState *s)
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum
2019-06-13 14:31 ` Philippe Mathieu-Daudé
@ 2019-06-14 12:02 ` Cédric Le Goater
2019-06-14 12:35 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-06-14 12:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell
Cc: Andrew Jeffery, qemu-arm, qemu-devel, Joel Stanley
On 13/06/2019 16:31, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
>
> On 5/25/19 5:12 PM, Cédric Le Goater wrote:
>> Emulate read errors in the DMA Checksum Register for high frequencies
>> and optimistic settings of the Read Timing Compensation Register. This
>> will help in tuning the SPI timing calibration algorithm.
>>
>> The values below are those to expect from the first flash device of
>> the FMC controller of a palmetto-bmc machine.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/ssi/aspeed_smc.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 406c30c60b3f..4c162912cf62 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -866,6 +866,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
>> s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
>> }
>>
>
> Can you add a comment (like the patch description) here?
yes. done.
>> +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.
>> @@ -903,6 +927,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>> s->regs[R_DMA_FLASH_ADDR] += 4;
>> s->regs[R_DMA_LEN] -= 4;
>> }
>> +
>> + if (aspeed_smc_inject_read_failure(s)) {
>
> So this model real world where noise eventually triggers errors. Don't
> we want this to be enable by the user (or a QMP command)?
I can add a property at the device model level to trigger this behavior.
Such as :
-global driver=aspeed.smc,property=timing,value=true
timing if defined would provide the maximum clock and delay settings.
Are there any other examples in QEMU ?
Thanks,
C.
>
>> + s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
>> + }
>> +
>> }
>>
>> static void aspeed_smc_dma_rw(AspeedSMCState *s)
>>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum
2019-06-14 12:02 ` Cédric Le Goater
@ 2019-06-14 12:35 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 44+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-14 12:35 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, Marc-André Lureau, qemu-arm, qemu-devel,
Joel Stanley
Cc'ing Markus & Marc-André
On 6/14/19 2:02 PM, Cédric Le Goater wrote:
> On 13/06/2019 16:31, Philippe Mathieu-Daudé wrote:
>> Hi Cédric,
>>
>> On 5/25/19 5:12 PM, Cédric Le Goater wrote:
>>> Emulate read errors in the DMA Checksum Register for high frequencies
>>> and optimistic settings of the Read Timing Compensation Register. This
>>> will help in tuning the SPI timing calibration algorithm.
>>>
>>> The values below are those to expect from the first flash device of
>>> the FMC controller of a palmetto-bmc machine.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> hw/ssi/aspeed_smc.c | 29 +++++++++++++++++++++++++++++
>>> 1 file changed, 29 insertions(+)
>>>
>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>>> index 406c30c60b3f..4c162912cf62 100644
>>> --- a/hw/ssi/aspeed_smc.c
>>> +++ b/hw/ssi/aspeed_smc.c
>>> @@ -866,6 +866,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
>>> s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
>>> }
>>>
>>
>> Can you add a comment (like the patch description) here?
>
> yes. done.
>
>>> +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.
>>> @@ -903,6 +927,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>>> s->regs[R_DMA_FLASH_ADDR] += 4;
>>> s->regs[R_DMA_LEN] -= 4;
>>> }
>>> +
>>> + if (aspeed_smc_inject_read_failure(s)) {
>>
>> So this model real world where noise eventually triggers errors. Don't
>> we want this to be enable by the user (or a QMP command)?
>
> I can add a property at the device model level to trigger this behavior.
> Such as :
>
> -global driver=aspeed.smc,property=timing,value=true
>
> timing if defined would provide the maximum clock and delay settings.
I was thinking of a simpler:
-global driver=aspeed.smc,property=inject_failure,value=true
Then
if (s->inject_failure && aspeed_smc_inject_read_failure(s)) {
s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
}
>
> Are there any other examples in QEMU ?
I think most of them are hidden in the codebase...
>
> Thanks,
>
> C.
>
>>
>>> + s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
>>> + }
>>> +
>>> }
>>>
>>> static void aspeed_smc_dma_rw(AspeedSMCState *s)
>>>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Qemu-devel] [PATCH 19/19] aspeed/smc: Calculate checksum on normal DMA
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (17 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum Cédric Le Goater
@ 2019-05-25 15:12 ` Cédric Le Goater
2019-06-12 1:42 ` Joel Stanley
2019-06-07 10:10 ` [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Peter Maydell
19 siblings, 1 reply; 44+ messages in thread
From: Cédric Le Goater @ 2019-05-25 15:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Christian Svensson, Andrew Jeffery, qemu-devel, qemu-arm,
Cédric Le Goater, Joel Stanley
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>
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 4c162912cf62..9afd0e920dfb 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -986,6 +986,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.20.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [Qemu-devel] [PATCH 19/19] aspeed/smc: Calculate checksum on normal DMA
2019-05-25 15:12 ` [Qemu-devel] [PATCH 19/19] aspeed/smc: Calculate checksum on normal DMA Cédric Le Goater
@ 2019-06-12 1:42 ` Joel Stanley
0 siblings, 0 replies; 44+ messages in thread
From: Joel Stanley @ 2019-06-12 1:42 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers,
Christian Svensson
On Sat, 25 May 2019 at 15:15, Cédric Le Goater <clg@kaod.org> wrote:
>
> 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>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes
2019-05-25 15:12 [Qemu-devel] [PATCH 00/19] aspeed: machine extensions and fixes Cédric Le Goater
` (18 preceding siblings ...)
2019-05-25 15:12 ` [Qemu-devel] [PATCH 19/19] aspeed/smc: Calculate checksum on normal DMA Cédric Le Goater
@ 2019-06-07 10:10 ` Peter Maydell
19 siblings, 0 replies; 44+ messages in thread
From: Peter Maydell @ 2019-06-07 10:10 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Andrew Jeffery, qemu-arm, QEMU Developers, Joel Stanley
On Sat, 25 May 2019 at 16:12, 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
> - a RTC model from Joel
> - support for multiple CPUs and NICs
> - fixes for the timer model 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
>
> It is based on Eduardo's series" Machine Core queue, 2019-05-24"
>
> http://patchwork.ozlabs.org/patch/1105091/
>
> I have included Philippe's patch (comes first) in this patchset for
> reference only.
...I'm hoping some of the other folks interested in Aspeed
are going to review this series.
thanks
-- PMM
^ permalink raw reply [flat|nested] 44+ messages in thread