Re: [PATCH v4 1/3] hw/misc: sifive_e_aon: Support the watchdog timer of HiFive 1 rev b.

[Tommy Wu]

Hi Phil,

Thanks for all the suggestions, I'll send patch v5 to fix all the issues.

Best Regards,

Tommy

On Thu, May 25, 2023 at 8:05 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

Hi Tommy,

On 23/5/23 10:49, Tommy Wu wrote:
> The watchdog timer is in the always-on domain device of HiFive 1 rev b,
> so this patch added the AON device to the sifive_e machine. This patch
> only implemented the functionality of the watchdog timer.
>
> Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>   hw/misc/Kconfig                |   3 +
>   hw/misc/meson.build            |   1 +
>   hw/misc/sifive_e_aon.c         | 326 +++++++++++++++++++++++++++++++++
>   include/hw/misc/sifive_e_aon.h |  60 ++++++
>   4 files changed, 390 insertions(+)
>   create mode 100644 hw/misc/sifive_e_aon.c
>   create mode 100644 include/hw/misc/sifive_e_aon.h


> +REG32(AON_WDT_WDOGCFG, 0x0)
> +    FIELD(AON_WDT_WDOGCFG, SCALE, 0, 4)
> +    FIELD(AON_WDT_WDOGCFG, RSVD0, 4, 4)
> +    FIELD(AON_WDT_WDOGCFG, RSTEN, 8, 1)
> +    FIELD(AON_WDT_WDOGCFG, ZEROCMP, 9, 1)
> +    FIELD(AON_WDT_WDOGCFG, RSVD1, 10, 2)
> +    FIELD(AON_WDT_WDOGCFG, EN_ALWAYS, 12, 1)
> +    FIELD(AON_WDT_WDOGCFG, EN_CORE_AWAKE, 13, 1)
> +    FIELD(AON_WDT_WDOGCFG, RSVD2, 14, 14)
> +    FIELD(AON_WDT_WDOGCFG, IP0, 28, 1)
> +    FIELD(AON_WDT_WDOGCFG, RSVD3, 29, 3)
> +REG32(AON_WDT_WDOGCOUNT, 0x8)

Adding:

        FIELD(AON_WDT_WDOGCOUNT, VALUE, 0, 31)

...

> +REG32(AON_WDT_WDOGS, 0x10)
> +REG32(AON_WDT_WDOGFEED, 0x18)
> +REG32(AON_WDT_WDOGKEY, 0x1c)
> +REG32(AON_WDT_WDOGCMP0, 0x20)
> +
> +static void sifive_e_aon_wdt_update_wdogcount(SiFiveEAONState *r)
> +{
> +    int64_t now;
> +    if (0 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_ALWAYS) &&
> +        0 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_CORE_AWAKE)) {
> +        return;
> +    }
> +
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    r->wdogcount += muldiv64(now - r->wdog_restart_time,
> +                             r->wdogclk_freq, NANOSECONDS_PER_SECOND);
> +
> +    /* Clean the most significant bit. */
> +    r->wdogcount = ((r->wdogcount << 1) >> 1);

... you could use:

        r->wdogcount &= R_AON_WDT_WDOGCOUNT_VALUE_MASK

> +    r->wdog_restart_time = now;
> +}
> +
> +static void sifive_e_aon_wdt_update_state(SiFiveEAONState *r)
> +{
> +    uint16_t wdogs;
> +    bool cmp_signal = false;
> +    sifive_e_aon_wdt_update_wdogcount(r);
> +    wdogs = (uint16_t)(r->wdogcount >>
> +                           FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, SCALE));
> +
> +    if (wdogs >= r->wdogcmp0) {
> +        cmp_signal = true;
> +        if (1 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, ZEROCMP)) {
> +            r->wdogcount = 0;
> +            wdogs = 0;
> +        }
> +    }
> +
> +    if (cmp_signal) {
> +        if (1 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, RSTEN)) {

You sometimes check bit equality, ...

> +            watchdog_perform_action();
> +        }
> +        r->wdogcfg = FIELD_DP32(r->wdogcfg, AON_WDT_WDOGCFG, IP0, 1);
> +    }
> +
> +    qemu_set_irq(r->wdog_irq, FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, IP0));
> +
> +    if (wdogs < r->wdogcmp0 &&
> +        (FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_ALWAYS) ||

... sometimes you don't. Code style consistency would be better.

(Also, most of QEMU code base check equality using constant value
on the right side of the comparaison).

> +         FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_CORE_AWAKE))) {
> +        int64_t next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        next += muldiv64((r->wdogcmp0 - wdogs) <<
> +                         FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, SCALE),
> +                         NANOSECONDS_PER_SECOND, r->wdogclk_freq);
> +        timer_mod(r->wdog_timer, next);
> +    } else {
> +        timer_mod(r->wdog_timer, INT64_MAX);
> +    }
> +}

> +static void sifive_e_aon_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    SiFiveEAONState *r = SIFIVE_E_AON(obj);
> +
> +    memory_region_init_io(&r->mmio, OBJECT(r), &sifive_e_aon_ops, r,
> +                          TYPE_SIFIVE_E_AON, SIFIVE_E_AON_MAX);
> +    sysbus_init_mmio(sbd, &r->mmio);
> +
> +    /* watchdog timer */
> +    r->wdogclk_freq = SIFIVE_E_LFCLK_DEFAULT_FREQ;
> +    sysbus_init_irq(sbd, &r->wdog_irq);
> +}
> +
> +static void sifive_e_aon_realize(DeviceState *dev, Error **errp)
> +{
> +    SiFiveEAONState *r = SIFIVE_E_AON(dev);
> +
> +    /* watchdog timer */
> +    r->wdog_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                 sifive_e_aon_wdt_expired_cb, r);

You should be able to create the timer in sifive_e_aon_init().

> +}

Regards,

Phil.