qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cédric Le Goater
Subject: Re: [Qemu-arm] [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
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]