RE: [PATCH v2 1/3] hw/watchdog: wdt_ibex_aon.c: Implement the watchdog for the OpenTitan

[Dong, Eddie]

Hi Tyler:
        Some other comments embedded.


> +
> +static void ibex_aon_write(void *opaque,
> +                           hwaddr addr, uint64_t value,
> +                           unsigned int size) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> +
> +    /* When writing, need to consider if the configuration is locked */
> +    switch (addr) {
> +    case A_ALERT_TEST:
> +        s->regs[R_ALERT_TEST] = value & 0x1;
> +        break;
> +    case A_WKUP_CTRL:
> +        qemu_log_mask(LOG_UNIMP, "%s: AON wkup not implemented",
> __func__);
> +        break;
> +    case A_WKUP_THOLD:
> +        qemu_log_mask(LOG_UNIMP, "%s: AON wkup not implemented",
> __func__);
> +        break;
> +    case A_WKUP_COUNT:
> +        qemu_log_mask(LOG_UNIMP, "%s: AON wkup not implemented",
> __func__);
> +        break;
> +    case A_WDOG_REGWEN:
> +        if (s->regs[R_WDOG_REGWEN] == IBEX_AONTIMER_WDOG_UNLOCKED)
> {
> +            s->regs[R_WDOG_REGWEN] = value &
> IBEX_AONTIMER_WDOG_REGWEN_MASK;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                            "%s: AON Timer configuration locked\n", 
> __func__);
> +        }
> +        break;
> +    case A_WDOG_CTRL:
> +        if (s->regs[R_WDOG_REGWEN] == IBEX_AONTIMER_WDOG_UNLOCKED)
> {
> +            if (!(s->regs[R_WDOG_CTRL] & IBEX_AONTIMER_WDOG_CTRL_MASK))
> {
> +                s->wdog_last = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +            }
> +            s->regs[R_WDOG_CTRL] = value &
> IBEX_AONTIMER_WDOG_CTRL_MASK;
> +            ibex_aon_update_bark_timer(s);
> +            ibex_aon_update_bite_timer(s);

In case, the guest clears the ENABLE bit of CTRL register, we want to remove 
the registered timer, right?
The current code path do nothing when ENABLE is cleared.  That is incorrect and 
may lead to bark and bite event happen again.

> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                            "%s: AON Timer configuration locked\n", 
> __func__);
> +        }
> +        break;
> +    case A_WDOG_BARK_THOLD:
> +        if (s->regs[R_WDOG_REGWEN] == IBEX_AONTIMER_WDOG_UNLOCKED)
> {
> +            s->regs[R_WDOG_BARK_THOLD] = value;
> +            ibex_aon_update_bark_timer(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                            "%s: AON Timer configuration locked\n", 
> __func__);
> +        }
> +        break;
> +    case A_WDOG_BITE_THOLD:
> +        if (s->regs[R_WDOG_REGWEN] == IBEX_AONTIMER_WDOG_UNLOCKED)
> {
> +            s->regs[R_WDOG_BITE_THOLD] = value;
> +            ibex_aon_update_bite_timer(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                            "%s: AON Timer configuration locked\n", 
> __func__);
> +        }
> +        break;
> +    case A_WDOG_COUNT:
> +        s->regs[R_WDOG_COUNT] = value;
> +        s->wdog_last = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        ibex_aon_update_bark_timer(s);
> +        ibex_aon_update_bite_timer(s);
> +        break;
> +    case A_INTR_STATE:
> +        /* Service the IRQs by writing 1 to the appropriate field */
> +        if ((value & R_INTR_STATE_WDOG_MASK)) {

Do we need to clear the bit in s->regs[R_INTR_STATE] ?
Otherwise, guest read of the register gets wrong value.

> +            qemu_irq_lower(s->bark_irq);
> +            ibex_aon_update_count(s);
> +            int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +            /*
> +             * We need to make sure that COUNT < *_THOLD. If it isn't, an
> +             * interrupt is generated the next clock cycle
> +             */
> +            if (s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
> +                if (now + IBEX_AONTIMER_PERIOD_NS < now) {
> +                    timer_mod_ns(s->barker, INT64_MAX);

Overflow of an 64 bits timer(in the unit of ns) is too far away, which can 
never happen. 

> +                } else {
> +                    timer_mod_ns(s->barker, now + IBEX_AONTIMER_PERIOD_NS);

We can call the fire API (ibex_aon_barker_expired) directly here.

> +                }
> +            }
> +        }
> +        break;
> +    case A_INTR_TEST:
> +        qemu_log_mask(LOG_UNIMP, "%s: Intr test not implemented",
> __func__);
> +        break;
> +    case A_WKUP_CAUSE:
> +        qemu_log_mask(LOG_UNIMP,
> +                        "%s: Wkup cause not implemented", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: Invalid register read 0x%" HWADDR_PRIx "\n",
> +                    __func__, addr);
> +        break;
> +    }
> +}
> +
> +static void ibex_aon_enter_reset(Object *obj, ResetType type) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(obj);
> +    s->regs[R_ALERT_TEST]      = 0x0;
> +    s->regs[R_WKUP_CTRL]       = 0x0;
> +    s->regs[R_WKUP_THOLD]      = 0x0;
> +    s->regs[R_WKUP_COUNT]      = 0x0;
> +    s->regs[R_WDOG_REGWEN]     = 0x1;
> +    s->regs[R_WDOG_CTRL]       = 0x0;
> +    s->regs[R_WDOG_BARK_THOLD] = 0x0;
> +    s->regs[R_WDOG_BITE_THOLD] = 0x0;
> +    s->regs[R_WDOG_COUNT]      = 0x0;
> +    s->regs[R_INTR_STATE]      = 0x0;
> +    s->regs[R_INTR_TEST]       = 0x0;
> +    s->regs[R_WKUP_CAUSE]      = 0x0;
> +
> +    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    s->wdog_last = now;
> +    timer_del(s->barker);
> +    timer_del(s->biter);
> +}
> +
> +static void ibex_aon_hold_reset(Object *obj) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(obj);
> +    qemu_irq_lower(s->bark_irq);
> +}
> +
> +static void ibex_aon_realize(DeviceState *dev, Error **errp) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(dev);
> +    s->barker = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> ibex_aon_barker_expired, dev);
> +    s->biter = timer_new_ns(QEMU_CLOCK_VIRTUAL, ibex_aon_biter_expired,
> +dev); }
> +
> +static void ibex_aon_unrealize(DeviceState *dev) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(dev);
> +
> +    timer_free(s->barker);
> +    timer_free(s->biter);
> +}
> +
> +static WatchdogTimerModel model = {
> +    .wdt_name = TYPE_IBEX_AON_TIMER,
> +    .wdt_description = "OpenTitan always-on timer"
> +};
> +
> +static const VMStateDescription vmstate_ibex_aon = {
> +    .name = "vmstate_ibex_aon",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, IbexAONTimerState,
> IBEX_AONTIMER_REGCOUNT),
> +        VMSTATE_TIMER_PTR(barker, IbexAONTimerState),
> +        VMSTATE_TIMER_PTR(biter, IbexAONTimerState),
> +        VMSTATE_INT64(wdog_last, IbexAONTimerState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const struct MemoryRegionOps ibex_aon_ops = {
> +    .read = ibex_aon_read,
> +    .write = ibex_aon_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN
> +};
> +
> +static void ibex_aon_init(Object *obj)
> +{
> +    IbexAONTimerState *s = IBEX_AON_TIMER(obj);
> +    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    sysbus_init_irq(dev, &s->bark_irq);
> +    memory_region_init_io(&s->iomem, obj, &ibex_aon_ops, s,
> +                          TYPE_IBEX_AON_TIMER, 4 *
> +IBEX_AONTIMER_REGCOUNT); }
> +
> +static void ibex_aon_class_init(ObjectClass *oc, void *data) {
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    ResettableClass *rc = RESETTABLE_CLASS(oc);
> +    dc->realize = ibex_aon_realize;
> +    dc->unrealize = ibex_aon_unrealize;
> +    dc->hotpluggable = false;
> +    set_bit(DEVICE_CATEGORY_WATCHDOG, dc->categories);
> +    dc->vmsd = &vmstate_ibex_aon;
> +    dc->desc = "opentitan always-on timer ip block";
> +    /* Resettable class inits */
> +    rc->phases.enter = ibex_aon_enter_reset;
> +    rc->phases.hold = ibex_aon_hold_reset; }
> +
> +static const TypeInfo ibex_aon_info = {
> +    .class_init = ibex_aon_class_init,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .name = TYPE_IBEX_AON_TIMER,
> +    .instance_size = sizeof(IbexAONTimerState),
> +    .instance_init = ibex_aon_init,
> +};
> +
> +static void ibex_aon_register_types(void) {
> +    watchdog_add_model(&model);
> +    type_register_static(&ibex_aon_info);
> +}
> +
> +type_init(ibex_aon_register_types)
> diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h index
> 26d960f288..3758f614bd 100644
> --- a/include/hw/riscv/opentitan.h
> +++ b/include/hw/riscv/opentitan.h
> @@ -24,6 +24,7 @@
>  #include "hw/char/ibex_uart.h"
>  #include "hw/timer/ibex_timer.h"
>  #include "hw/ssi/ibex_spi_host.h"
> +#include "hw/watchdog/wdt_ibex_aon.h"
>  #include "qom/object.h"
> 
>  #define TYPE_RISCV_IBEX_SOC "riscv.lowrisc.ibex.soc"
> @@ -46,6 +47,8 @@ struct LowRISCIbexSoCState {
>      IbexTimerState timer;
>      IbexSPIHostState spi_host[OPENTITAN_NUM_SPI_HOSTS];
> 
> +    IbexAONTimerState aon_timer;
> +
>      MemoryRegion flash_mem;
>      MemoryRegion rom;
>      MemoryRegion flash_alias;
> @@ -79,7 +82,7 @@ enum {
>      IBEX_DEV_RSTMGR,
>      IBEX_DEV_CLKMGR,
>      IBEX_DEV_PINMUX,
> -    IBEX_DEV_PADCTRL,
> +    IBEX_DEV_AON_TIMER,
>      IBEX_DEV_USBDEV,
>      IBEX_DEV_FLASH_CTRL,
>      IBEX_DEV_PLIC,
> @@ -111,6 +114,8 @@ enum {
>      IBEX_SPI_HOST0_SPI_EVENT_IRQ  = 152,
>      IBEX_SPI_HOST1_ERR_IRQ        = 153,
>      IBEX_SPI_HOST1_SPI_EVENT_IRQ  = 154,
> +    IBEX_AONTIMER_WKUP_EXPIRED    = 158,
> +    IBEX_AONTIMER_WDOG_BARK       = 159,
>  };
> 
>  #endif
> diff --git a/include/hw/watchdog/wdt_ibex_aon.h
> b/include/hw/watchdog/wdt_ibex_aon.h
> new file mode 100644
> index 0000000000..894968488f
> --- /dev/null
> +++ b/include/hw/watchdog/wdt_ibex_aon.h
> @@ -0,0 +1,67 @@
> +/*
> + * QEMU lowRISC OpenTitan Always-On Timer device
> + *
> + * Copyright (c) 2022 Rivos Inc.
> + *
> + * For details check the documentation here:
> + *   https://docs.opentitan.org/hw/ip/aon_timer/doc/
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a copy
> + * of this software and associated documentation files (the
> "Software"), to deal
> + * in the Software without restriction, including without limitation
> + the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + sell
> + * copies of the Software, and to permit persons to whom the Software
> + is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT
> + SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> + OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#ifndef WDT_IBEX_AON_H
> +#define WDT_IBEX_AON_H
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "qom/object.h"
> +
> +#define TYPE_IBEX_AON_TIMER "ibex-aon-timer"
> +OBJECT_DECLARE_SIMPLE_TYPE(IbexAONTimerState, IBEX_AON_TIMER)
> +
> +#define IBEX_AONTIMER_REGCOUNT 12
> +#define IBEX_AONTIMER_FREQ 200000 /* 200 KHz */ #define
> +IBEX_AONTIMER_PERIOD_NS 5000
> +
> +#define IBEX_AONTIMER_WDOG_LOCKED 0
> +#define IBEX_AONTIMER_WDOG_UNLOCKED 1
> +
> +#define IBEX_AONTIMER_WDOG_REGWEN_MASK 0x1 #define
> +IBEX_AONTIMER_WDOG_CTRL_MASK 0x3 #define
> IBEX_AONTIMER_INTR_STATE_MASK
> +0x3
> +
> +struct IbexAONTimerState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +
> +    QEMUTimer *barker;
> +    QEMUTimer *biter;
> +
> +    qemu_irq bark_irq;
> +
> +    /* Registers */
> +    uint32_t regs[IBEX_AONTIMER_REGCOUNT];
> +    /* Last-used Timestamps */
> +    int64_t wdog_last;
> +    /*< public >*/
> +};
> +
> +
> +#endif /* WDT_IBEX_AON_H */