Re: [Qemu-arm] [PATCH 1/2] wdt: Add Aspeed watchdog device model

[Peter Maydell]

On 18 January 2017 at 16:55, Cédric Le Goater <address@hidden> wrote:
> From: Joel Stanley <address@hidden>
>
> 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 <address@hidden>
> [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 <address@hidden>
> ---
>  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