* [Qemu-devel] [PATCH 0/2] Aspeed watchdog controller model
@ 2017-01-18 16:55 Cédric Le Goater
2017-01-18 16:55 ` [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model Cédric Le Goater
2017-01-18 16:55 ` [Qemu-devel] [PATCH 2/2] aspeed: add a watchdog controller Cédric Le Goater
0 siblings, 2 replies; 5+ messages in thread
From: Cédric Le Goater @ 2017-01-18 16:55 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-arm, Joel Stanley, Andrew Jeffery,
Cédric Le Goater
Hello,
The Aspeed SoC includes a set of watchdog timers using 32-bit
decrement counters. This patchset provides a model for this controller
and adds the first watchdog to the Aspeed SoC model. A second watchdog
exists and is used on real HW to boot from a second flash module
containing a golden image of the firmware. This is not supported yet
in qemu.
The main benefit today of this model is to enables reboot/reset of a
guest from U-Boot and Linux.
Thanks,
C.
Cédric Le Goater (1):
aspeed: add a watchdog controller
Joel Stanley (1):
wdt: Add Aspeed watchdog device model
hw/arm/aspeed_soc.c | 13 +++
hw/watchdog/Makefile.objs | 1 +
hw/watchdog/wdt_aspeed.c | 214 +++++++++++++++++++++++++++++++++++++++
include/hw/arm/aspeed_soc.h | 2 +
include/hw/watchdog/wdt_aspeed.h | 37 +++++++
5 files changed, 267 insertions(+)
create mode 100644 hw/watchdog/wdt_aspeed.c
create mode 100644 include/hw/watchdog/wdt_aspeed.h
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model
2017-01-18 16:55 [Qemu-devel] [PATCH 0/2] Aspeed watchdog controller model Cédric Le Goater
@ 2017-01-18 16:55 ` Cédric Le Goater
2017-01-24 11:50 ` Peter Maydell
2017-01-18 16:55 ` [Qemu-devel] [PATCH 2/2] aspeed: add a watchdog controller Cédric Le Goater
1 sibling, 1 reply; 5+ messages in thread
From: Cédric Le Goater @ 2017-01-18 16:55 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-arm, Joel Stanley, Andrew Jeffery,
Cédric Le Goater
From: Joel Stanley <joel@jms.id.au>
The Aspeed SoC includes a set of watchdog timers using 32-bit
decrement counters, which can be based either on the APB clock or
a 1 MHz clock.
The watchdog timer is designed to prevent system deadlock and, in
general, it should be restarted before timeout. When a timeout occurs,
different types of signals can be generated, ARM reset, SOC reset,
System reset, CPU Interrupt, external signal or boot from alternate
block. The current model only performs the system reset function as
this is used by U-Boot and Linux.
Signed-off-by: Joel Stanley <joel@jms.id.au>
[clg: - fixed compile breakage
- fixed io region size
- added watchdog_perform_action() on timer expiry
- wrote a commit log
- merged fixes from Andrew Jeffery to scale the reload value ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/watchdog/Makefile.objs | 1 +
hw/watchdog/wdt_aspeed.c | 214 +++++++++++++++++++++++++++++++++++++++
include/hw/watchdog/wdt_aspeed.h | 37 +++++++
3 files changed, 252 insertions(+)
create mode 100644 hw/watchdog/wdt_aspeed.c
create mode 100644 include/hw/watchdog/wdt_aspeed.h
diff --git a/hw/watchdog/Makefile.objs b/hw/watchdog/Makefile.objs
index 72e3ffd93c59..9589bed63a3d 100644
--- a/hw/watchdog/Makefile.objs
+++ b/hw/watchdog/Makefile.objs
@@ -2,3 +2,4 @@ common-obj-y += watchdog.o
common-obj-$(CONFIG_WDT_IB6300ESB) += wdt_i6300esb.o
common-obj-$(CONFIG_WDT_IB700) += wdt_ib700.o
common-obj-$(CONFIG_WDT_DIAG288) += wdt_diag288.o
+common-obj-$(CONFIG_ASPEED_SOC) += wdt_aspeed.o
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
new file mode 100644
index 000000000000..96e62c54dc04
--- /dev/null
+++ b/hw/watchdog/wdt_aspeed.c
@@ -0,0 +1,214 @@
+/*
+ * ASPEED Watchdog Controller
+ *
+ * Copyright (C) 2016-2017 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "sysemu/watchdog.h"
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#include "hw/watchdog/wdt_aspeed.h"
+
+#define WDT_IO_REGION_SIZE 0x20
+
+#define WDT_STATUS 0x00
+#define WDT_RELOAD_VALUE 0x04
+#define WDT_RESTART 0x08
+#define WDT_CTRL 0x0C
+#define WDT_TIMEOUT_STATUS 0x10
+#define WDT_TIMEOUT_CLEAR 0x14
+#define WDT_RESET_WDITH 0x18
+
+#define WDT_RESTART_MAGIC 0x4755
+
+static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
+{
+ AspeedWDTState *s = ASPEED_WDT(opaque);
+
+ switch (offset) {
+ case WDT_STATUS:
+ return s->reg_status;
+ case WDT_RELOAD_VALUE:
+ return s->reg_reload_value;
+ case WDT_RESTART:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: read from write-only reg at offset 0x%"
+ HWADDR_PRIx "\n", __func__, offset);
+ return 0;
+ case WDT_CTRL:
+ return s->reg_ctrl;
+ case WDT_TIMEOUT_STATUS:
+ case WDT_TIMEOUT_CLEAR:
+ case WDT_RESET_WDITH:
+ qemu_log_mask(LOG_UNIMP,
+ "%s: uninmplemented read at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ return 0;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ return 0;
+ }
+
+}
+
+#define PCLK_HZ 24000000
+
+static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
+ unsigned size)
+{
+ AspeedWDTState *s = ASPEED_WDT(opaque);
+ bool en = data & BIT(0);
+ bool pclk = !(data & BIT(4));
+
+ switch (offset) {
+ case WDT_STATUS:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: write to read-only reg at offset 0x%"
+ HWADDR_PRIx "\n", __func__, offset);
+ break;
+ case WDT_RELOAD_VALUE:
+ s->reg_reload_value = data;
+ break;
+ case WDT_RESTART:
+ if ((data & 0xFFFF) == 0x4755) {
+ uint32_t reload;
+
+ s->reg_status = s->reg_reload_value;
+
+ if (pclk) {
+ reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
+ PCLK_HZ) ;
+ } else {
+ reload = s->reg_reload_value * 1000;
+ }
+
+ if (s->enabled) {
+ timer_mod(s->timer,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+ }
+ }
+ break;
+ case WDT_CTRL:
+ if (en && !s->enabled) {
+ uint32_t reload;
+
+ if (pclk) {
+ reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
+ PCLK_HZ);
+ } else {
+ reload = s->reg_reload_value * 1000;
+ }
+
+ s->enabled = true;
+ timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+ } else if (!en && s->enabled) {
+ s->enabled = false;
+ timer_del(s->timer);
+ }
+ break;
+ case WDT_TIMEOUT_STATUS:
+ case WDT_TIMEOUT_CLEAR:
+ case WDT_RESET_WDITH:
+ qemu_log_mask(LOG_UNIMP,
+ "%s: uninmplemented write at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+ return;
+}
+
+static WatchdogTimerModel model = {
+ .wdt_name = TYPE_ASPEED_WDT,
+ .wdt_description = "aspeed watchdog device",
+};
+
+static const VMStateDescription vmstate_aspeed_wdt = {
+ .name = "vmstate_aspeed_wdt",
+ .version_id = 0,
+ .minimum_version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_TIMER_PTR(timer, AspeedWDTState),
+ VMSTATE_BOOL(enabled, AspeedWDTState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const MemoryRegionOps aspeed_wdt_ops = {
+ .read = aspeed_wdt_read,
+ .write = aspeed_wdt_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+ .valid.unaligned = false,
+};
+
+static void aspeed_wdt_reset(DeviceState *dev)
+{
+ AspeedWDTState *s = ASPEED_WDT(dev);
+
+ s->reg_status = 0x3EF1480;
+ s->reg_reload_value = 0x03EF1480;
+ s->reg_restart = 0;
+ s->reg_ctrl = 0;
+
+ s->enabled = false;
+ timer_del(s->timer);
+}
+
+static void aspeed_wdt_timer_expired(void *dev)
+{
+ AspeedWDTState *s = ASPEED_WDT(dev);
+
+ qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
+ watchdog_perform_action();
+ timer_del(s->timer);
+}
+
+static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
+{
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedWDTState *s = ASPEED_WDT(dev);
+
+ s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, aspeed_wdt_timer_expired,
+ dev);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
+ TYPE_ASPEED_WDT, WDT_IO_REGION_SIZE);
+ sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void aspeed_wdt_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = aspeed_wdt_realize;
+ dc->reset = aspeed_wdt_reset;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ dc->vmsd = &vmstate_aspeed_wdt;
+}
+
+static const TypeInfo aspeed_wdt_info = {
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .name = TYPE_ASPEED_WDT,
+ .instance_size = sizeof(AspeedWDTState),
+ .class_init = aspeed_wdt_class_init,
+};
+
+static void wdt_aspeed_register_types(void)
+{
+ watchdog_add_model(&model);
+ type_register_static(&aspeed_wdt_info);
+}
+
+type_init(wdt_aspeed_register_types)
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
new file mode 100644
index 000000000000..a11b687b6aff
--- /dev/null
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -0,0 +1,37 @@
+/*
+ * ASPEED Watchdog Controller
+ *
+ * Copyright (C) 2016-2017 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+#ifndef ASPEED_WDT_H
+#define ASPEED_WDT_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_WDT "aspeed.wdt"
+#define ASPEED_WDT(obj) \
+ OBJECT_CHECK(AspeedWDTState, (obj), TYPE_ASPEED_WDT)
+#define ASPEED_WDT_CLASS(klass) \
+ OBJECT_CLASS_CHECK(AspeedWDTClass, (klass), TYPE_ASPEED_WDT)
+#define ASPEED_WDT_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(AspeedWDTClass, (obj), TYPE_ASPEED_WDT)
+
+typedef struct AspeedWDTState {
+ /*< private >*/
+ SysBusDevice parent_obj;
+ QEMUTimer *timer;
+ bool enabled;
+
+ /*< public >*/
+ MemoryRegion iomem;
+
+ uint32_t reg_status;
+ uint32_t reg_reload_value;
+ uint32_t reg_restart;
+ uint32_t reg_ctrl;
+} AspeedWDTState;
+
+#endif /* ASPEED_WDT_H */
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 2/2] aspeed: add a watchdog controller
2017-01-18 16:55 [Qemu-devel] [PATCH 0/2] Aspeed watchdog controller model Cédric Le Goater
2017-01-18 16:55 ` [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model Cédric Le Goater
@ 2017-01-18 16:55 ` Cédric Le Goater
1 sibling, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2017-01-18 16:55 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-arm, Joel Stanley, Andrew Jeffery,
Cédric Le Goater
This enables reboot of a guest from U-Boot and Linux.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
hw/arm/aspeed_soc.c | 13 +++++++++++++
include/hw/arm/aspeed_soc.h | 2 ++
2 files changed, 15 insertions(+)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 4fb777e6df6e..6df76382f007 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -31,6 +31,7 @@
#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
static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
@@ -170,6 +171,10 @@ static void aspeed_soc_init(Object *obj)
sc->info->silicon_rev);
object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
"ram-size", &error_abort);
+
+ object_initialize(&s->wdt, sizeof(s->wdt), TYPE_ASPEED_WDT);
+ object_property_add_child(obj, "wdt", OBJECT(&s->wdt), NULL);
+ qdev_set_parent_bus(DEVICE(&s->wdt), sysbus_get_default());
}
static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -289,6 +294,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, ASPEED_SOC_SDMC_BASE);
+
+ /* Watch dog */
+ object_property_set_bool(OBJECT(&s->wdt), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt), 0, ASPEED_SOC_WDT_BASE);
}
static void aspeed_soc_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 1ab5deaa0813..dbec0c159885 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -19,6 +19,7 @@
#include "hw/timer/aspeed_timer.h"
#include "hw/i2c/aspeed_i2c.h"
#include "hw/ssi/aspeed_smc.h"
+#include "hw/watchdog/wdt_aspeed.h"
#define ASPEED_SPIS_NUM 2
@@ -37,6 +38,7 @@ typedef struct AspeedSoCState {
AspeedSMCState fmc;
AspeedSMCState spi[ASPEED_SPIS_NUM];
AspeedSDMCState sdmc;
+ AspeedWDTState wdt;
} AspeedSoCState;
#define TYPE_ASPEED_SOC "aspeed-soc"
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model
2017-01-18 16:55 ` [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model Cédric Le Goater
@ 2017-01-24 11:50 ` Peter Maydell
2017-01-25 16:29 ` Cédric Le Goater
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2017-01-24 11:50 UTC (permalink / raw)
To: Cédric Le Goater
Cc: QEMU Developers, qemu-arm, Joel Stanley, Andrew Jeffery
On 18 January 2017 at 16:55, Cédric Le Goater <clg@kaod.org> wrote:
> From: Joel Stanley <joel@jms.id.au>
>
> The Aspeed SoC includes a set of watchdog timers using 32-bit
> decrement counters, which can be based either on the APB clock or
> a 1 MHz clock.
>
> The watchdog timer is designed to prevent system deadlock and, in
> general, it should be restarted before timeout. When a timeout occurs,
> different types of signals can be generated, ARM reset, SOC reset,
> System reset, CPU Interrupt, external signal or boot from alternate
> block. The current model only performs the system reset function as
> this is used by U-Boot and Linux.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> [clg: - fixed compile breakage
> - fixed io region size
> - added watchdog_perform_action() on timer expiry
> - wrote a commit log
> - merged fixes from Andrew Jeffery to scale the reload value ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/watchdog/Makefile.objs | 1 +
> hw/watchdog/wdt_aspeed.c | 214 +++++++++++++++++++++++++++++++++++++++
> include/hw/watchdog/wdt_aspeed.h | 37 +++++++
> 3 files changed, 252 insertions(+)
> create mode 100644 hw/watchdog/wdt_aspeed.c
> create mode 100644 include/hw/watchdog/wdt_aspeed.h
>
> diff --git a/hw/watchdog/Makefile.objs b/hw/watchdog/Makefile.objs
> index 72e3ffd93c59..9589bed63a3d 100644
> --- a/hw/watchdog/Makefile.objs
> +++ b/hw/watchdog/Makefile.objs
> @@ -2,3 +2,4 @@ common-obj-y += watchdog.o
> common-obj-$(CONFIG_WDT_IB6300ESB) += wdt_i6300esb.o
> common-obj-$(CONFIG_WDT_IB700) += wdt_ib700.o
> common-obj-$(CONFIG_WDT_DIAG288) += wdt_diag288.o
> +common-obj-$(CONFIG_ASPEED_SOC) += wdt_aspeed.o
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> new file mode 100644
> index 000000000000..96e62c54dc04
> --- /dev/null
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -0,0 +1,214 @@
> +/*
> + * ASPEED Watchdog Controller
> + *
> + * Copyright (C) 2016-2017 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "sysemu/watchdog.h"
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "hw/watchdog/wdt_aspeed.h"
> +
> +#define WDT_IO_REGION_SIZE 0x20
> +
> +#define WDT_STATUS 0x00
> +#define WDT_RELOAD_VALUE 0x04
> +#define WDT_RESTART 0x08
> +#define WDT_CTRL 0x0C
> +#define WDT_TIMEOUT_STATUS 0x10
> +#define WDT_TIMEOUT_CLEAR 0x14
> +#define WDT_RESET_WDITH 0x18
> +
> +#define WDT_RESTART_MAGIC 0x4755
> +
> +static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + AspeedWDTState *s = ASPEED_WDT(opaque);
> +
> + switch (offset) {
> + case WDT_STATUS:
> + return s->reg_status;
> + case WDT_RELOAD_VALUE:
> + return s->reg_reload_value;
> + case WDT_RESTART:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: read from write-only reg at offset 0x%"
> + HWADDR_PRIx "\n", __func__, offset);
> + return 0;
> + case WDT_CTRL:
> + return s->reg_ctrl;
> + case WDT_TIMEOUT_STATUS:
> + case WDT_TIMEOUT_CLEAR:
> + case WDT_RESET_WDITH:
> + qemu_log_mask(LOG_UNIMP,
> + "%s: uninmplemented read at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + return 0;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + return 0;
> + }
> +
> +}
> +
> +#define PCLK_HZ 24000000
> +
> +static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
> + unsigned size)
> +{
> + AspeedWDTState *s = ASPEED_WDT(opaque);
> + bool en = data & BIT(0);
> + bool pclk = !(data & BIT(4));
These are only valid for WDT_CTRL, right? That being so,
initialising them up here is a bit odd.
> +
> + switch (offset) {
> + case WDT_STATUS:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: write to read-only reg at offset 0x%"
> + HWADDR_PRIx "\n", __func__, offset);
> + break;
> + case WDT_RELOAD_VALUE:
> + s->reg_reload_value = data;
> + break;
> + case WDT_RESTART:
> + if ((data & 0xFFFF) == 0x4755) {
> + uint32_t reload;
> +
> + s->reg_status = s->reg_reload_value;
> +
> + if (pclk) {
> + reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
> + PCLK_HZ) ;
> + } else {
> + reload = s->reg_reload_value * 1000;
> + }
> +
> + if (s->enabled) {
> + timer_mod(s->timer,
> + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
> + }
> + }
> + break;
> + case WDT_CTRL:
> + if (en && !s->enabled) {
> + uint32_t reload;
> +
> + if (pclk) {
> + reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
> + PCLK_HZ);
> + } else {
> + reload = s->reg_reload_value * 1000;
> + }
> +
> + s->enabled = true;
> + timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
> + } else if (!en && s->enabled) {
> + s->enabled = false;
> + timer_del(s->timer);
> + }
> + break;
Shouldn't this write to s->reg_ctrl ?
s->enabled seems to duplicate state also in s->reg_ctrl.
> + case WDT_TIMEOUT_STATUS:
> + case WDT_TIMEOUT_CLEAR:
> + case WDT_RESET_WDITH:
> + qemu_log_mask(LOG_UNIMP,
> + "%s: uninmplemented write at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + }
> + return;
> +}
> +
> +static WatchdogTimerModel model = {
> + .wdt_name = TYPE_ASPEED_WDT,
> + .wdt_description = "aspeed watchdog device",
> +};
> +
> +static const VMStateDescription vmstate_aspeed_wdt = {
> + .name = "vmstate_aspeed_wdt",
> + .version_id = 0,
> + .minimum_version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_TIMER_PTR(timer, AspeedWDTState),
> + VMSTATE_BOOL(enabled, AspeedWDTState),
This doesn't seem to have fields for most of the register state.
> + VMSTATE_END_OF_LIST()
> + }
> +};
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model
2017-01-24 11:50 ` Peter Maydell
@ 2017-01-25 16:29 ` Cédric Le Goater
0 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2017-01-25 16:29 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, qemu-arm, Joel Stanley, Andrew Jeffery
On 01/24/2017 12:50 PM, Peter Maydell wrote:
> On 18 January 2017 at 16:55, Cédric Le Goater <clg@kaod.org> wrote:
>> From: Joel Stanley <joel@jms.id.au>
>>
>> The Aspeed SoC includes a set of watchdog timers using 32-bit
>> decrement counters, which can be based either on the APB clock or
>> a 1 MHz clock.
>>
>> The watchdog timer is designed to prevent system deadlock and, in
>> general, it should be restarted before timeout. When a timeout occurs,
>> different types of signals can be generated, ARM reset, SOC reset,
>> System reset, CPU Interrupt, external signal or boot from alternate
>> block. The current model only performs the system reset function as
>> this is used by U-Boot and Linux.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> [clg: - fixed compile breakage
>> - fixed io region size
>> - added watchdog_perform_action() on timer expiry
>> - wrote a commit log
>> - merged fixes from Andrew Jeffery to scale the reload value ]
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/watchdog/Makefile.objs | 1 +
>> hw/watchdog/wdt_aspeed.c | 214 +++++++++++++++++++++++++++++++++++++++
>> include/hw/watchdog/wdt_aspeed.h | 37 +++++++
>> 3 files changed, 252 insertions(+)
>> create mode 100644 hw/watchdog/wdt_aspeed.c
>> create mode 100644 include/hw/watchdog/wdt_aspeed.h
>>
>> diff --git a/hw/watchdog/Makefile.objs b/hw/watchdog/Makefile.objs
>> index 72e3ffd93c59..9589bed63a3d 100644
>> --- a/hw/watchdog/Makefile.objs
>> +++ b/hw/watchdog/Makefile.objs
>> @@ -2,3 +2,4 @@ common-obj-y += watchdog.o
>> common-obj-$(CONFIG_WDT_IB6300ESB) += wdt_i6300esb.o
>> common-obj-$(CONFIG_WDT_IB700) += wdt_ib700.o
>> common-obj-$(CONFIG_WDT_DIAG288) += wdt_diag288.o
>> +common-obj-$(CONFIG_ASPEED_SOC) += wdt_aspeed.o
>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
>> new file mode 100644
>> index 000000000000..96e62c54dc04
>> --- /dev/null
>> +++ b/hw/watchdog/wdt_aspeed.c
>> @@ -0,0 +1,214 @@
>> +/*
>> + * ASPEED Watchdog Controller
>> + *
>> + * Copyright (C) 2016-2017 IBM Corp.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "sysemu/watchdog.h"
>> +#include "hw/sysbus.h"
>> +#include "qemu/timer.h"
>> +#include "hw/watchdog/wdt_aspeed.h"
>> +
>> +#define WDT_IO_REGION_SIZE 0x20
>> +
>> +#define WDT_STATUS 0x00
>> +#define WDT_RELOAD_VALUE 0x04
>> +#define WDT_RESTART 0x08
>> +#define WDT_CTRL 0x0C
>> +#define WDT_TIMEOUT_STATUS 0x10
>> +#define WDT_TIMEOUT_CLEAR 0x14
>> +#define WDT_RESET_WDITH 0x18
>> +
>> +#define WDT_RESTART_MAGIC 0x4755
>> +
>> +static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> + AspeedWDTState *s = ASPEED_WDT(opaque);
>> +
>> + switch (offset) {
>> + case WDT_STATUS:
>> + return s->reg_status;
>> + case WDT_RELOAD_VALUE:
>> + return s->reg_reload_value;
>> + case WDT_RESTART:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: read from write-only reg at offset 0x%"
>> + HWADDR_PRIx "\n", __func__, offset);
>> + return 0;
>> + case WDT_CTRL:
>> + return s->reg_ctrl;
>> + case WDT_TIMEOUT_STATUS:
>> + case WDT_TIMEOUT_CLEAR:
>> + case WDT_RESET_WDITH:
>> + qemu_log_mask(LOG_UNIMP,
>> + "%s: uninmplemented read at offset 0x%" HWADDR_PRIx "\n",
>> + __func__, offset);
>> + return 0;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
>> + __func__, offset);
>> + return 0;
>> + }
>> +
>> +}
>> +
>> +#define PCLK_HZ 24000000
>> +
>> +static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>> + unsigned size)
>> +{
>> + AspeedWDTState *s = ASPEED_WDT(opaque);
>> + bool en = data & BIT(0);
>> + bool pclk = !(data & BIT(4));
>
> These are only valid for WDT_CTRL, right? That being so,
> initialising them up here is a bit odd.
Well, pclk is for the WDT_RESTART reg also. But, yes, it
can be improved.
>> +
>> + switch (offset) {
>> + case WDT_STATUS:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: write to read-only reg at offset 0x%"
>> + HWADDR_PRIx "\n", __func__, offset);
>> + break;
>> + case WDT_RELOAD_VALUE:
>> + s->reg_reload_value = data;
>> + break;
>> + case WDT_RESTART:
>> + if ((data & 0xFFFF) == 0x4755) {
>> + uint32_t reload;
>> +
>> + s->reg_status = s->reg_reload_value;
>> +
>> + if (pclk) {
>> + reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
>> + PCLK_HZ) ;
>> + } else {
>> + reload = s->reg_reload_value * 1000;
>> + }
>> +
>> + if (s->enabled) {
>> + timer_mod(s->timer,
>> + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
>> + }
>> + }
>> + break;
>> + case WDT_CTRL:
>> + if (en && !s->enabled) {
>> + uint32_t reload;
>> +
>> + if (pclk) {
>> + reload = muldiv64(s->reg_reload_value, NANOSECONDS_PER_SECOND,
>> + PCLK_HZ);
>> + } else {
>> + reload = s->reg_reload_value * 1000;
>> + }
>> +
>> + s->enabled = true;
>> + timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
>> + } else if (!en && s->enabled) {
>> + s->enabled = false;
>> + timer_del(s->timer);
>> + }
>> + break;
>
> Shouldn't this write to s->reg_ctrl ?
>
> s->enabled seems to duplicate state also in s->reg_ctrl.
yes clearly. I will use a regs array in next version,
it should be cleaner and also migration friendly.
I completely left out that part.
Thanks,
C.
>> + case WDT_TIMEOUT_STATUS:
>> + case WDT_TIMEOUT_CLEAR:
>> + case WDT_RESET_WDITH:
>> + qemu_log_mask(LOG_UNIMP,
>> + "%s: uninmplemented write at offset 0x%" HWADDR_PRIx "\n",
>> + __func__, offset);
>> + break;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
>> + __func__, offset);
>> + }
>> + return;
>> +}
>> +
>> +static WatchdogTimerModel model = {
>> + .wdt_name = TYPE_ASPEED_WDT,
>> + .wdt_description = "aspeed watchdog device",
>> +};
>> +
>> +static const VMStateDescription vmstate_aspeed_wdt = {
>> + .name = "vmstate_aspeed_wdt",
>> + .version_id = 0,
>> + .minimum_version_id = 0,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_TIMER_PTR(timer, AspeedWDTState),
>> + VMSTATE_BOOL(enabled, AspeedWDTState),
>
> This doesn't seem to have fields for most of the register state.
>
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-25 16:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-18 16:55 [Qemu-devel] [PATCH 0/2] Aspeed watchdog controller model Cédric Le Goater
2017-01-18 16:55 ` [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model Cédric Le Goater
2017-01-24 11:50 ` Peter Maydell
2017-01-25 16:29 ` Cédric Le Goater
2017-01-18 16:55 ` [Qemu-devel] [PATCH 2/2] aspeed: add a watchdog controller Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.