[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] wdt: Add Aspeed watchdog device model |
Date: |
Wed, 25 Jan 2017 17:29:03 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 |
On 01/24/2017 12:50 PM, Peter Maydell wrote:
> 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.
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
>