On 01/24/2017 12:50 PM, Peter Maydell wrote: > On 18 January 2017 at 16:55, Cédric Le Goater <c...@kaod.org> wrote: >> From: Joel Stanley <j...@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 <j...@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 <c...@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 >