diff mbox series

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

Message ID CAB88-qOh7dBoFP4SWcM4G5EVp6cumVnZ5j4tNRqSvMT08=dAcA@mail.gmail.com
State New
Headers show
Series [v2,1/3] hw/watchdog: wdt_ibex_aon.c: Implement the watchdog for the OpenTitan | expand

Commit Message

Tyler Ng Sept. 22, 2022, 3:58 p.m. UTC
This commit adds most of an implementation of the OpenTitan Always-On
Timer. The documentation for this timer is found here:

https://docs.opentitan.org/hw/ip/aon_timer/doc/

Using commit 217a0168ba118503c166a9587819e3811eeb0c0c

The implementation includes most of the watchdog features; it does not
implement the wakeup timer.

An important note: the OpenTitan board uses the sifive_plic. The plic
will not be able to claim the bark interrupt (159) because the sifive
plic sets priority[159], but checks priority[158] for the priority, so
it thinks that the interrupt's priority is 0 (effectively disabled).

Changed Files:
hw/riscv/Kconfig: Add configuration for the watchdog.
hw/riscv/opentitan.c: Connect AON Timer to the OpenTitan board.

hw/watchdog/Kconfig: Configuration for the watchdog.
hw/watchdog/meson.build: Compile the watchdog.
hw/watchdog/wdt_ibex_aon.c: The watchdog itself.

include/hw/riscv/opentitan.h: Add watchdog bark/wakeup IRQs and timer.
include/hw/watchdog/wdt_ibex_aon.h: Add watchdog.

tests/qtest/ibex-aon-timer-test.c: Ibex AON Timer test.
tests/qtest/meson.build: Build the timer test.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
---
 hw/riscv/Kconfig                   |   4 +
 hw/riscv/opentitan.c               |  21 +-
 hw/watchdog/Kconfig                |   3 +
 hw/watchdog/meson.build            |   1 +
 hw/watchdog/wdt_ibex_aon.c         | 405 +++++++++++++++++++++++++++++
 include/hw/riscv/opentitan.h       |   7 +-
 include/hw/watchdog/wdt_ibex_aon.h |  67 +++++
 tests/qtest/ibex-aon-timer-test.c  | 189 ++++++++++++++
 tests/qtest/meson.build            |   3 +
 9 files changed, 696 insertions(+), 4 deletions(-)
 create mode 100644 hw/watchdog/wdt_ibex_aon.c
 create mode 100644 include/hw/watchdog/wdt_ibex_aon.h
 create mode 100644 tests/qtest/ibex-aon-timer-test.c

--
2.34.1

Comments

Thomas Huth Sept. 22, 2022, 4:17 p.m. UTC | #1
On 22/09/2022 17.58, Tyler Ng wrote:
> This commit adds most of an implementation of the OpenTitan Always-On
> Timer. The documentation for this timer is found here:
> 
> https://docs.opentitan.org/hw/ip/aon_timer/doc/
> 
> Using commit 217a0168ba118503c166a9587819e3811eeb0c0c
> 
> The implementation includes most of the watchdog features; it does not
> implement the wakeup timer.
> 
> An important note: the OpenTitan board uses the sifive_plic. The plic
> will not be able to claim the bark interrupt (159) because the sifive
> plic sets priority[159], but checks priority[158] for the priority, so
> it thinks that the interrupt's priority is 0 (effectively disabled).
...
> diff --git a/tests/qtest/ibex-aon-timer-test.c
> b/tests/qtest/ibex-aon-timer-test.c
> new file mode 100644
> index 0000000000..af33feac39
> --- /dev/null
> +++ b/tests/qtest/ibex-aon-timer-test.c
> @@ -0,0 +1,189 @@
> +/*
> + * Testing the OpenTitan AON Timer
> + *
> + * Copyright (c) 2022 Rivos Inc.
> + *
> + * 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.

Could you maybe add a SPDX license identifier at the beginning of the 
comment, so that it's easier to identify the license at a first glance? 
(also in the other new files)

> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +
> +#define AON_BASE_ADDR (0x40470000ul)
> +#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
> +#define AON_WKUP_IRQ 158
> +#define AON_BARK_IRQ 159
> +#define AON_FREQ 200000 /* 200 KHz */
> +#define AON_PERIOD_NS 5000
> +#define NANOS_PER_SECOND 1000000000LL
> +/* Test that reads work, and that the regs get reset to the correct value */
> +static void test_reads(void)
> +{
> +    QTestState *test = qtest_init("-M opentitan");
> +    g_assert(qtest_readl(test, AON_ADDR(0x00)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x04)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x08)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x0c)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x10)) == 1); > +    g_assert(qtest_readl(test, AON_ADDR(0x14)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x18)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x1c)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x20)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x24)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x28)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x2c)) == 0);

The read tests that check for 0 could maybe be simplified with a for-loop 
(or two).

> +    qtest_quit(test);
> +}
> +
> +static void test_writes(void)
> +{
> +    /* Test that writes worked, while the config is unlocked */
> +    QTestState *test = qtest_init("-M opentitan");
> +
> +
> +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> +                     ==, (1 << 19));
> +
> +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> +                     ==, (1 << 20));
> +
> +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* WDOG_CTRL enable */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> +                     ==, 0x1);
> +
> +    qtest_writel(test, AON_ADDR(0x10), 0x0); /* WDOG_REGWEN enable */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)), ==, 0x0);

I think the above code would be better readable if you'd provide a helper 
function like this:

static void writel_and_assert(QTestState qts, int addr, int val)
{
     qtest_writel(qts, AON_ADDR(addr), val);
     g_assert_cmpuint(qtest_readl(qts, AON_ADDR(addr)), val);
}

> +    /*
> +     * Test configuration lock
> +     * These should not successfully write.
> +     */
> +    qtest_writel(test, AON_ADDR(0x14), 0);
> +    qtest_writel(test, AON_ADDR(0x18), 0);
> +    qtest_writel(test, AON_ADDR(0x1c), 0);
> +
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> +                     ==, 0x1);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> +                     ==, (1 << 19));
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> +                     ==, (1 << 20));
> +
> +    /* This should not work, as it's a rw0c reg. */
> +    qtest_writel(test, AON_ADDR(0x10), 1);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)),
> +                     ==, 0x0);
> +
> +    qtest_quit(test);
> +}
> +
> +
> +/* Test whether the watchdog timer works during normal operation */
> +static void test_operation(void)
> +{
> +    QTestState *test = qtest_init("-M opentitan");
> +
> +    /* Set up interrupts */
> +    qtest_irq_intercept_in(test, "/machine/soc/plic");
> +
> +    /* Setup timer */
> +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
> +
> +    /* Run simulation, without enabling timer: */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 30);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, 0); /* checks if WDOG_COUNT gets updated */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, 0); /* checks if INTR_STATE is clear */
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Enable the timer, and test if the count is updated correctly */
> +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, 0);
> +    qtest_clock_step(test, NANOS_PER_SECOND);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, AON_FREQ);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, 0);
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Disable the timer, and test if the count freezes */
> +    qtest_writel(test, AON_ADDR(0x14), 0x0); /* set WDOG_CTRL = 0 */
> +    qtest_clock_step(test, NANOS_PER_SECOND);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, AON_FREQ);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, 0);
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Enable the timer, and run to bark */
> +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 1.62145);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     >=, (1 << 19));
> +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, (1 << 1));
> +
> +    /* Disable IRQ by writing to INTR_STATE. Should bark next cycle */
> +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, (1 << 1));
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +    qtest_clock_step(test, AON_PERIOD_NS);
> +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> +    /*
> +     * Disable IRQ again, this time by setting WDOG_COUNT = 0 (pet) and
> +     * writing to INTR_STATE.
> +     */
> +    qtest_writel(test, AON_ADDR(0x20), 0);
> +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Ensure no bite occurs, after resetting the timer. */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 2.621436);
> +    QDict *resp = qtest_qmp(test, "{'execute':'query-status'}");
> +    g_assert(qdict_haskey(resp, "return"));
> +    qobject_unref(resp);
> +
> +    /* Allow test to run to bite. */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 5.24289);
> +    QDict *data = qdict_get_qdict(qtest_qmp_eventwait_ref(test, "WATCHDOG"),
> +                                  "data");
> +    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "reset");
> +    qobject_unref(data);
> +    qtest_quit(test);
> +}
> +
> +
> +

Please avoid multiple blank lines (it's not very common in QEMU, I think).

> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +    qtest_add_func("/ibex-aon-timer/reads", test_reads);
> +    qtest_add_func("/ibex-aon-timer/writes", test_writes);
> +    qtest_add_func("/ibex-aon-timer/op", test_operation);
> +    return g_test_run();
> +}

  Thomas
Dong, Eddie Sept. 26, 2022, 8:46 p.m. UTC | #2
> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+eddie.dong=intel.com@nongnu.org>
> On Behalf Of Tyler Ng
> Sent: Thursday, September 22, 2022 8:59 AM
> To: open list:RISC-V <qemu-riscv@nongnu.org>; qemu-devel@nongnu.org
> Developers <qemu-devel@nongnu.org>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Meng, Bin
> <bin.meng@windriver.com>; Thomas Huth <thuth@redhat.com>; Paolo
> Bonzini <pbonzini@redhat.com>; Palmer Dabbelt <palmer@dabbelt.com>;
> Laurent Vivier <lvivier@redhat.com>
> Subject: [PATCH v2 1/3] hw/watchdog: wdt_ibex_aon.c: Implement the
> watchdog for the OpenTitan
> 
> This commit adds most of an implementation of the OpenTitan Always-On
> Timer. The documentation for this timer is found here:
> 
> https://docs.opentitan.org/hw/ip/aon_timer/doc/
> 
> Using commit 217a0168ba118503c166a9587819e3811eeb0c0c
> 
> The implementation includes most of the watchdog features; it does not
> implement the wakeup timer.
> 
> An important note: the OpenTitan board uses the sifive_plic. The plic will not
> be able to claim the bark interrupt (159) because the sifive plic sets
> priority[159], but checks priority[158] for the priority, so it thinks that the
> interrupt's priority is 0 (effectively disabled).
> 
> Changed Files:
> hw/riscv/Kconfig: Add configuration for the watchdog.
> hw/riscv/opentitan.c: Connect AON Timer to the OpenTitan board.
> 
> hw/watchdog/Kconfig: Configuration for the watchdog.
> hw/watchdog/meson.build: Compile the watchdog.
> hw/watchdog/wdt_ibex_aon.c: The watchdog itself.
> 
> include/hw/riscv/opentitan.h: Add watchdog bark/wakeup IRQs and timer.
> include/hw/watchdog/wdt_ibex_aon.h: Add watchdog.
> 
> tests/qtest/ibex-aon-timer-test.c: Ibex AON Timer test.
> tests/qtest/meson.build: Build the timer test.
> 
> Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> ---
>  hw/riscv/Kconfig                   |   4 +
>  hw/riscv/opentitan.c               |  21 +-
>  hw/watchdog/Kconfig                |   3 +
>  hw/watchdog/meson.build            |   1 +
>  hw/watchdog/wdt_ibex_aon.c         | 405 +++++++++++++++++++++++++++++
>  include/hw/riscv/opentitan.h       |   7 +-
>  include/hw/watchdog/wdt_ibex_aon.h |  67 +++++  tests/qtest/ibex-aon-
> timer-test.c  | 189 ++++++++++++++
>  tests/qtest/meson.build            |   3 +
>  9 files changed, 696 insertions(+), 4 deletions(-)  create mode 100644
> hw/watchdog/wdt_ibex_aon.c  create mode 100644
> include/hw/watchdog/wdt_ibex_aon.h
>  create mode 100644 tests/qtest/ibex-aon-timer-test.c
> 
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig index
> 79ff61c464..72094010be 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -4,6 +4,9 @@ config RISCV_NUMA
>  config IBEX
>      bool
> 
> +config IBEX_AON
> +    bool
> +
>  config MICROCHIP_PFSOC
>      bool
>      select CADENCE_SDHCI
> @@ -20,6 +23,7 @@ config MICROCHIP_PFSOC  config OPENTITAN
>      bool
>      select IBEX
> +    select IBEX_AON
>      select UNIMP
> 
>  config SHAKTI_C
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index
> af13dbe3b1..eae9b2504f 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -48,7 +48,7 @@ static const MemMapEntry ibex_memmap[] = {
>      [IBEX_DEV_RSTMGR] =         {  0x40410000,  0x1000  },
>      [IBEX_DEV_CLKMGR] =         {  0x40420000,  0x1000  },
>      [IBEX_DEV_PINMUX] =         {  0x40460000,  0x1000  },
> -    [IBEX_DEV_PADCTRL] =        {  0x40470000,  0x1000  },
> +    [IBEX_DEV_AON_TIMER] =      {  0x40470000,  0x1000  },
>      [IBEX_DEV_FLASH_CTRL] =     {  0x41000000,  0x1000  },
>      [IBEX_DEV_AES] =            {  0x41100000,  0x1000  },
>      [IBEX_DEV_HMAC] =           {  0x41110000,  0x1000  },
> @@ -122,6 +122,8 @@ static void lowrisc_ibex_soc_init(Object *obj)
> 
>      object_initialize_child(obj, "timer", &s->timer, TYPE_IBEX_TIMER);
> 
> +    object_initialize_child(obj, "aon_timer", &s->aon_timer,
> TYPE_IBEX_AON_TIMER);
> +
>      for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; i++) {
>          object_initialize_child(obj, "spi_host[*]", &s->spi_host[i],
>                                  TYPE_IBEX_SPI_HOST); @@ -207,6 +209,7 @@ static void
> lowrisc_ibex_soc_realize(DeviceState
> *dev_soc, Error **errp)
>                         3, qdev_get_gpio_in(DEVICE(&s->plic),
>                         IBEX_UART0_RX_OVERFLOW_IRQ));
> 
> +    /* RV Timer */
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer), errp)) {
>          return;
>      }
> @@ -218,6 +221,20 @@ static void lowrisc_ibex_soc_realize(DeviceState
> *dev_soc, Error **errp)
>                            qdev_get_gpio_in(DEVICE(qemu_get_cpu(0)),
>                                             IRQ_M_TIMER));
> 
> +    /* AON Timer */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->aon_timer), errp)) {
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->aon_timer), 0,
> memmap[IBEX_DEV_AON_TIMER].base);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->aon_timer),
> +                       0, qdev_get_gpio_in(DEVICE(&s->plic),
> +                       IBEX_AONTIMER_WDOG_BARK));
> +    /*
> +     * Note: There should be a line to pwrmgr but it's not implemented.
> +     * TODO: Needs a line connected in, "counter-run" (stop the watchdog if
> +     * debugging)
> +     */
> +
>      /* SPI-Hosts */
>      for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; ++i) {
>          dev = DEVICE(&(s->spi_host[i])); @@ -265,8 +282,6 @@ static void
> lowrisc_ibex_soc_realize(DeviceState
> *dev_soc, Error **errp)
>          memmap[IBEX_DEV_CLKMGR].base, memmap[IBEX_DEV_CLKMGR].size);
>      create_unimplemented_device("riscv.lowrisc.ibex.pinmux",
>          memmap[IBEX_DEV_PINMUX].base, memmap[IBEX_DEV_PINMUX].size);
> -    create_unimplemented_device("riscv.lowrisc.ibex.padctrl",
> -        memmap[IBEX_DEV_PADCTRL].base,
> memmap[IBEX_DEV_PADCTRL].size);
>      create_unimplemented_device("riscv.lowrisc.ibex.usbdev",
>          memmap[IBEX_DEV_USBDEV].base, memmap[IBEX_DEV_USBDEV].size);
>      create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl",
> diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig index
> 66e1d029e3..dde6c01a8c 100644
> --- a/hw/watchdog/Kconfig
> +++ b/hw/watchdog/Kconfig
> @@ -20,3 +20,6 @@ config WDT_IMX2
> 
>  config WDT_SBSA
>      bool
> +
> +config WDT_IBEX_AON
> +    bool
> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build index
> 8974b5cf4c..21e2ede28f 100644
> --- a/hw/watchdog/meson.build
> +++ b/hw/watchdog/meson.build
> @@ -7,3 +7,4 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
> files('wdt_aspeed.c'))
>  softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>  softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
>  specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
> +softmmu_ss.add(when: 'CONFIG_IBEX_AON', if_true:
> +files('wdt_ibex_aon.c'))
> diff --git a/hw/watchdog/wdt_ibex_aon.c b/hw/watchdog/wdt_ibex_aon.c
> new file mode 100644 index 0000000000..d3cd56c634
> --- /dev/null
> +++ b/hw/watchdog/wdt_ibex_aon.c
> @@ -0,0 +1,405 @@
> +/*
> + * 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.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu/timer.h"
> +#include "qemu/log.h"
> +#include "sysemu/reset.h"
> +#include "sysemu/watchdog.h"
> +#include "hw/register.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> +
> +#include "hw/watchdog/wdt_ibex_aon.h"
> +
> +REG32(ALERT_TEST,      0x00)
> +REG32(WKUP_CTRL,       0x04)
> +REG32(WKUP_THOLD,      0x08)
> +REG32(WKUP_COUNT,      0x0c)
> +REG32(WDOG_REGWEN,     0x10)
> +REG32(WDOG_CTRL,       0x14)
> +    FIELD(WDOG_CTRL, EN, 0, 1)
> +    FIELD(WDOG_CTRL, PIS, 1, 1) /* Pause in sleep */
> +REG32(WDOG_BARK_THOLD, 0x18) REG32(WDOG_BITE_THOLD, 0x1c)
> +REG32(WDOG_COUNT,      0x20)
> +REG32(INTR_STATE,      0x24)
> +    FIELD(INTR_STATE, WKUP, 0, 1)
> +    FIELD(INTR_STATE, WDOG, 1, 1)
> +REG32(INTR_TEST,       0x28)
> +REG32(WKUP_CAUSE,      0x2c)
> +
> +/*
> + * This function updates the count in the register. It depends on the
> +last time
> + * a read had occurred and extrapolates the count via the clock freq
> +and the
> + * time elapsed.
> + */
> +static bool ibex_aon_update_count(IbexAONTimerState *s) {
> +    /* If the timer is disabled, do not update count */
> +    if (!(s->regs[R_WDOG_CTRL] & R_WDOG_CTRL_EN_MASK)) {
> +        return false;
> +    }
> +    /* Lazily update wdog count. The count is truncated to fit. */
> +    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    /* If for some reason we went back in time, elapsed cycles is negative. */
Why it becomes negative? Due to bug?

> +    int64_t elapsed = now - s->wdog_last;
> +    /* Get the count. */
> +    const int64_t count = (elapsed / IBEX_AONTIMER_PERIOD_NS) +
> +                          (int64_t) s->regs[R_WDOG_COUNT];
> +    /* Saturate the counter. */
> +    if (count < 0) {
> +        s->regs[R_WDOG_COUNT] = 0;
> +    } else if (count <= UINT32_MAX) {
> +        s->regs[R_WDOG_COUNT] = (uint32_t) count;
> +    } else {
This implicitly says the COUNT remains to be 0XFFFFFFFF, if it overflows.
Where does the spec say so ? I don't find it, and I think hardware implementation will be relatively complicated if we expect this behavior.

> +        s->regs[R_WDOG_COUNT] = UINT32_MAX;
> +    }
> +    /* Update the last-used timestamps */
> +    s->wdog_last = now;
> +    return true;
> +}
> +
> +/* Called when the bark timer expires */ static void
> +ibex_aon_barker_expired(void *opaque) {
This may happen during ibex_aon_update_count(), right? 

> +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> +    if (ibex_aon_update_count(s) &&
This may happen during ibex_aon_update_count().
Nested call may lead to incorrect s->regs[R_WDOG_COUNT] & s->wdog_last. 

> +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
> +        s->regs[R_INTR_STATE] |= (1 << 1);
> +        qemu_irq_raise(s->bark_irq);
> +    }
> +}
> +
> +/* Called when the bite timer expires */ static void
> +ibex_aon_biter_expired(void *opaque) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> +    if (ibex_aon_update_count(s) &&
> +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BITE_THOLD]) {
> +        resettable_reset(opaque, RESET_TYPE_COLD);

Ditto.

> +        watchdog_perform_action();
> +    }
> +}
> +
> +/*
> + * Update the bark timer to reflect a new value of WDOG_COUNT or
> + * WDOG_BARK_THOLD.
> + */
> +static void ibex_aon_update_bark_timer(IbexAONTimerState *s) {
> +    if (!ibex_aon_update_count(s)) {
> +        return;
> +    }
> +    /* Calculate the register count remaining */
> +    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    const int64_t cur_count = (int64_t) s->regs[R_WDOG_COUNT];
> +    const int64_t rem_bark = s->regs[R_WDOG_BARK_THOLD] - cur_count;
> +    /* Extrapolate realtime from count based on clock period */
> +    const int64_t delta_ns_bark = rem_bark * IBEX_AONTIMER_PERIOD_NS;
> +    /* Timer updates */
> +    timer_mod_ns(s->barker, now + delta_ns_bark); }
> +
> +/*
> + * Update the bite timer to reflect a new value of WDOG_COUNT or
> + * WDOG_BITE_THOLD.
> + */
> +static void ibex_aon_update_bite_timer(IbexAONTimerState *s) {
> +    if (!ibex_aon_update_count(s)) {
> +        return;
> +    }
> +    /* Calculate the register count remaining */
> +    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    const int64_t cur_count = (int64_t) s->regs[R_WDOG_COUNT];
> +    const int64_t rem_bite = s->regs[R_WDOG_BITE_THOLD] - cur_count;
> +    /* Extrapolate realtime from count based on clock period */
> +    const int64_t delta_ns_bite = rem_bite * IBEX_AONTIMER_PERIOD_NS;
> +    /* Timer updates */
> +    timer_mod_ns(s->biter, now + delta_ns_bite); }
> +
> +static uint64_t ibex_aon_read(void *opaque, hwaddr addr, unsigned int
> +size) {
> +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> +
> +    uint64_t retval = 0;
> +    switch (addr) {
> +    case A_ALERT_TEST:
> +        retval = s->regs[R_ALERT_TEST];
> +        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:
> +        retval = s->regs[R_WDOG_REGWEN];
> +        break;
> +    case A_WDOG_CTRL:
> +        retval = s->regs[R_WDOG_CTRL];
> +        break;
> +    case A_WDOG_BARK_THOLD:
> +        retval = s->regs[R_WDOG_BARK_THOLD];
> +        break;
> +    case A_WDOG_BITE_THOLD:
> +        retval = s->regs[R_WDOG_BITE_THOLD];
> +        break;
> +    case A_WDOG_COUNT:
> +        /* Lazily update the wdog count. */
> +        ibex_aon_update_count(s);
> +        retval = s->regs[R_WDOG_COUNT];
> +        break;
> +    case A_INTR_STATE:
> +        retval = s->regs[R_INTR_STATE];
> +        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;
> +    }
> +    return retval;
> +}
> +
> +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);
Unconditionally call "update_bark_timer", and rely on ibex_aon_update_bark_timer to skip if the timer is not enabled.
This is not very straight forward IMO.

> +            ibex_aon_update_bite_timer(s);
> +        } 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);

If the bark timer happens here and update s->regs[R_WDOG_COUNT] and s->wdog_last, we have trouble.

> +        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)) {
> +            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);
> +                } else {
> +                    timer_mod_ns(s->barker, now + IBEX_AONTIMER_PERIOD_NS);
> +                }
> +            }
> +        }
> +        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 */
> diff --git a/tests/qtest/ibex-aon-timer-test.c
> b/tests/qtest/ibex-aon-timer-test.c
> new file mode 100644
> index 0000000000..af33feac39
> --- /dev/null
> +++ b/tests/qtest/ibex-aon-timer-test.c
> @@ -0,0 +1,189 @@
> +/*
> + * Testing the OpenTitan AON Timer
> + *
> + * Copyright (c) 2022 Rivos Inc.
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +
> +#define AON_BASE_ADDR (0x40470000ul)
> +#define AON_ADDR(addr) (AON_BASE_ADDR + addr) #define
> AON_WKUP_IRQ 158
> +#define AON_BARK_IRQ 159 #define AON_FREQ 200000 /* 200 KHz */
> #define
> +AON_PERIOD_NS 5000 #define NANOS_PER_SECOND 1000000000LL
> +/* Test that reads work, and that the regs get reset to the correct
> +value */ static void test_reads(void) {
> +    QTestState *test = qtest_init("-M opentitan");
> +    g_assert(qtest_readl(test, AON_ADDR(0x00)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x04)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x08)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x0c)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x10)) == 1);
> +    g_assert(qtest_readl(test, AON_ADDR(0x14)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x18)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x1c)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x20)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x24)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x28)) == 0);
> +    g_assert(qtest_readl(test, AON_ADDR(0x2c)) == 0);
> +
> +    qtest_quit(test);
> +}
> +
> +static void test_writes(void)
> +{
> +    /* Test that writes worked, while the config is unlocked */
> +    QTestState *test = qtest_init("-M opentitan");
> +
> +
> +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> +                     ==, (1 << 19));
> +
> +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> +                     ==, (1 << 20));
> +
> +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* WDOG_CTRL enable */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> +                     ==, 0x1);
> +
> +    qtest_writel(test, AON_ADDR(0x10), 0x0); /* WDOG_REGWEN enable */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)), ==, 0x0);
> +
> +    /*
> +     * Test configuration lock
> +     * These should not successfully write.
> +     */
> +    qtest_writel(test, AON_ADDR(0x14), 0);
> +    qtest_writel(test, AON_ADDR(0x18), 0);
> +    qtest_writel(test, AON_ADDR(0x1c), 0);
> +
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> +                     ==, 0x1);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> +                     ==, (1 << 19));
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> +                     ==, (1 << 20));
> +
> +    /* This should not work, as it's a rw0c reg. */
> +    qtest_writel(test, AON_ADDR(0x10), 1);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)),
> +                     ==, 0x0);
> +
> +    qtest_quit(test);
> +}
> +
> +
> +/* Test whether the watchdog timer works during normal operation */
> +static void test_operation(void) {
> +    QTestState *test = qtest_init("-M opentitan");
> +
> +    /* Set up interrupts */
> +    qtest_irq_intercept_in(test, "/machine/soc/plic");
> +
> +    /* Setup timer */
> +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD
> + */
> +
> +    /* Run simulation, without enabling timer: */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 30);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, 0); /* checks if WDOG_COUNT gets updated */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, 0); /* checks if INTR_STATE is clear */
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Enable the timer, and test if the count is updated correctly */
> +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, 0);
> +    qtest_clock_step(test, NANOS_PER_SECOND);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, AON_FREQ);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, 0);
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Disable the timer, and test if the count freezes */
> +    qtest_writel(test, AON_ADDR(0x14), 0x0); /* set WDOG_CTRL = 0 */
> +    qtest_clock_step(test, NANOS_PER_SECOND);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     ==, AON_FREQ);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, 0);
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Enable the timer, and run to bark */
> +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 1.62145);
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> +                     >=, (1 << 19));
> +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, (1 << 1));
> +
> +    /* Disable IRQ by writing to INTR_STATE. Should bark next cycle */
> +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> +                     ==, (1 << 1));
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +    qtest_clock_step(test, AON_PERIOD_NS);
> +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> +    /*
> +     * Disable IRQ again, this time by setting WDOG_COUNT = 0 (pet) and
> +     * writing to INTR_STATE.
> +     */
> +    qtest_writel(test, AON_ADDR(0x20), 0);
> +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> +
> +    /* Ensure no bite occurs, after resetting the timer. */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 2.621436);
> +    QDict *resp = qtest_qmp(test, "{'execute':'query-status'}");
> +    g_assert(qdict_haskey(resp, "return"));
> +    qobject_unref(resp);
> +
> +    /* Allow test to run to bite. */
> +    qtest_clock_step(test, NANOS_PER_SECOND * 5.24289);
> +    QDict *data = qdict_get_qdict(qtest_qmp_eventwait_ref(test,
> "WATCHDOG"),
> +                                  "data");
> +    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "reset");
> +    qobject_unref(data);
> +    qtest_quit(test);
> +}
> +
> +
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +    qtest_add_func("/ibex-aon-timer/reads", test_reads);
> +    qtest_add_func("/ibex-aon-timer/writes", test_writes);
> +    qtest_add_func("/ibex-aon-timer/op", test_operation);
> +    return g_test_run();
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index
> e910cb32ca..fb63b8d3fa 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -234,6 +234,9 @@ qtests_s390x = \
>     'cpu-plug-test',
>     'migration-test']
> 
> +qtests_riscv32 = \
> +  ['ibex-aon-timer-test']
> +
>  qos_test_ss = ss.source_set()
>  qos_test_ss.add(
>    'ac97-test.c',
> --
> 2.34.1
Tyler Ng Sept. 26, 2022, 11:03 p.m. UTC | #3
Hi Thomas,

On Thu, Sep 22, 2022 at 9:17 AM Thomas Huth <thuth@redhat.com> wrote:

> On 22/09/2022 17.58, Tyler Ng wrote:
> > This commit adds most of an implementation of the OpenTitan Always-On
> > Timer. The documentation for this timer is found here:
> >
> > https://docs.opentitan.org/hw/ip/aon_timer/doc/
> >
> > Using commit 217a0168ba118503c166a9587819e3811eeb0c0c
> >
> > The implementation includes most of the watchdog features; it does not
> > implement the wakeup timer.
> >
> > An important note: the OpenTitan board uses the sifive_plic. The plic
> > will not be able to claim the bark interrupt (159) because the sifive
> > plic sets priority[159], but checks priority[158] for the priority, so
> > it thinks that the interrupt's priority is 0 (effectively disabled).
> ...
> > diff --git a/tests/qtest/ibex-aon-timer-test.c
> > b/tests/qtest/ibex-aon-timer-test.c
> > new file mode 100644
> > index 0000000000..af33feac39
> > --- /dev/null
> > +++ b/tests/qtest/ibex-aon-timer-test.c
> > @@ -0,0 +1,189 @@
> > +/*
> > + * Testing the OpenTitan AON Timer
> > + *
> > + * Copyright (c) 2022 Rivos Inc.
> > + *
> > + * 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.
>
> Could you maybe add a SPDX license identifier at the beginning of the
> comment, so that it's easier to identify the license at a first glance?
> (also in the other new files)
>
> Will do, was actually thinking of switching over to GPL-2.0-or-later as
opposed to MIT.


> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "libqtest.h"
> > +#include "qapi/qmp/qdict.h"
> > +
> > +#define AON_BASE_ADDR (0x40470000ul)
> > +#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
> > +#define AON_WKUP_IRQ 158
> > +#define AON_BARK_IRQ 159
> > +#define AON_FREQ 200000 /* 200 KHz */
> > +#define AON_PERIOD_NS 5000
> > +#define NANOS_PER_SECOND 1000000000LL
> > +/* Test that reads work, and that the regs get reset to the correct
> value */
> > +static void test_reads(void)
> > +{
> > +    QTestState *test = qtest_init("-M opentitan");
> > +    g_assert(qtest_readl(test, AON_ADDR(0x00)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x04)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x08)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x0c)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x10)) == 1); > +
> g_assert(qtest_readl(test, AON_ADDR(0x14)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x18)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x1c)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x20)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x24)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x28)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x2c)) == 0);
>
> The read tests that check for 0 could maybe be simplified with a for-loop
> (or two).
>
I'm not entirely sure about what benefit this would bring after writing it
out.

>
> > +    qtest_quit(test);
> > +}
> > +
> > +static void test_writes(void)
> > +{
> > +    /* Test that writes worked, while the config is unlocked */
> > +    QTestState *test = qtest_init("-M opentitan");
> > +
> > +
> > +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> > +                     ==, (1 << 19));
> > +
> > +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> > +                     ==, (1 << 20));
> > +
> > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* WDOG_CTRL enable */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> > +                     ==, 0x1);
> > +
> > +    qtest_writel(test, AON_ADDR(0x10), 0x0); /* WDOG_REGWEN enable */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)), ==, 0x0);
>
> I think the above code would be better readable if you'd provide a helper
> function like this:
>
> static void writel_and_assert(QTestState qts, int addr, int val)
> {
>      qtest_writel(qts, AON_ADDR(addr), val);
>      g_assert_cmpuint(qtest_readl(qts, AON_ADDR(addr)), val);
> }
>
> Thanks for the suggestion. I decided to go with a macro instead though,
because it makes it easier to distinguish where an assertion failed without
a debugger.


> > +    /*
> > +     * Test configuration lock
> > +     * These should not successfully write.
> > +     */
> > +    qtest_writel(test, AON_ADDR(0x14), 0);
> > +    qtest_writel(test, AON_ADDR(0x18), 0);
> > +    qtest_writel(test, AON_ADDR(0x1c), 0);
> > +
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> > +                     ==, 0x1);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> > +                     ==, (1 << 19));
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> > +                     ==, (1 << 20));
> > +
> > +    /* This should not work, as it's a rw0c reg. */
> > +    qtest_writel(test, AON_ADDR(0x10), 1);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)),
> > +                     ==, 0x0);
> > +
> > +    qtest_quit(test);
> > +}
> > +
> > +
> > +/* Test whether the watchdog timer works during normal operation */
> > +static void test_operation(void)
> > +{
> > +    QTestState *test = qtest_init("-M opentitan");
> > +
> > +    /* Set up interrupts */
> > +    qtest_irq_intercept_in(test, "/machine/soc/plic");
> > +
> > +    /* Setup timer */
> > +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> > +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
> > +
> > +    /* Run simulation, without enabling timer: */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 30);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, 0); /* checks if WDOG_COUNT gets updated */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, 0); /* checks if INTR_STATE is clear */
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Enable the timer, and test if the count is updated correctly */
> > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, 0);
> > +    qtest_clock_step(test, NANOS_PER_SECOND);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, AON_FREQ);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, 0);
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Disable the timer, and test if the count freezes */
> > +    qtest_writel(test, AON_ADDR(0x14), 0x0); /* set WDOG_CTRL = 0 */
> > +    qtest_clock_step(test, NANOS_PER_SECOND);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, AON_FREQ);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, 0);
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Enable the timer, and run to bark */
> > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 1.62145);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     >=, (1 << 19));
> > +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, (1 << 1));
> > +
> > +    /* Disable IRQ by writing to INTR_STATE. Should bark next cycle */
> > +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, (1 << 1));
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +    qtest_clock_step(test, AON_PERIOD_NS);
> > +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> > +    /*
> > +     * Disable IRQ again, this time by setting WDOG_COUNT = 0 (pet) and
> > +     * writing to INTR_STATE.
> > +     */
> > +    qtest_writel(test, AON_ADDR(0x20), 0);
> > +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Ensure no bite occurs, after resetting the timer. */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 2.621436);
> > +    QDict *resp = qtest_qmp(test, "{'execute':'query-status'}");
> > +    g_assert(qdict_haskey(resp, "return"));
> > +    qobject_unref(resp);
> > +
> > +    /* Allow test to run to bite. */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 5.24289);
> > +    QDict *data = qdict_get_qdict(qtest_qmp_eventwait_ref(test,
> "WATCHDOG"),
> > +                                  "data");
> > +    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "reset");
> > +    qobject_unref(data);
> > +    qtest_quit(test);
> > +}
> > +
> > +
> > +
>
> Please avoid multiple blank lines (it's not very common in QEMU, I think).
>
I always miss a few strays here and there. Thanks.

> +int main(int argc, char **argv)
> > +{
> > +    g_test_init(&argc, &argv, NULL);
> > +    qtest_add_func("/ibex-aon-timer/reads", test_reads);
> > +    qtest_add_func("/ibex-aon-timer/writes", test_writes);
> > +    qtest_add_func("/ibex-aon-timer/op", test_operation);
> > +    return g_test_run();
> > +}
>
>   Thomas
>
> -Tyler
Tyler Ng Sept. 26, 2022, 11:24 p.m. UTC | #4
Hi Eddie,


On Mon, Sep 26, 2022 at 1:46 PM Dong, Eddie <eddie.dong@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: Qemu-devel <qemu-devel-bounces+eddie.dong=intel.com@nongnu.org>
> > On Behalf Of Tyler Ng
> > Sent: Thursday, September 22, 2022 8:59 AM
> > To: open list:RISC-V <qemu-riscv@nongnu.org>; qemu-devel@nongnu.org
> > Developers <qemu-devel@nongnu.org>
> > Cc: Alistair Francis <Alistair.Francis@wdc.com>; Meng, Bin
> > <bin.meng@windriver.com>; Thomas Huth <thuth@redhat.com>; Paolo
> > Bonzini <pbonzini@redhat.com>; Palmer Dabbelt <palmer@dabbelt.com>;
> > Laurent Vivier <lvivier@redhat.com>
> > Subject: [PATCH v2 1/3] hw/watchdog: wdt_ibex_aon.c: Implement the
> > watchdog for the OpenTitan
> >
> > This commit adds most of an implementation of the OpenTitan Always-On
> > Timer. The documentation for this timer is found here:
> >
> > https://docs.opentitan.org/hw/ip/aon_timer/doc/
> >
> > Using commit 217a0168ba118503c166a9587819e3811eeb0c0c
> >
> > The implementation includes most of the watchdog features; it does not
> > implement the wakeup timer.
> >
> > An important note: the OpenTitan board uses the sifive_plic. The plic
> will not
> > be able to claim the bark interrupt (159) because the sifive plic sets
> > priority[159], but checks priority[158] for the priority, so it thinks
> that the
> > interrupt's priority is 0 (effectively disabled).
> >
> > Changed Files:
> > hw/riscv/Kconfig: Add configuration for the watchdog.
> > hw/riscv/opentitan.c: Connect AON Timer to the OpenTitan board.
> >
> > hw/watchdog/Kconfig: Configuration for the watchdog.
> > hw/watchdog/meson.build: Compile the watchdog.
> > hw/watchdog/wdt_ibex_aon.c: The watchdog itself.
> >
> > include/hw/riscv/opentitan.h: Add watchdog bark/wakeup IRQs and timer.
> > include/hw/watchdog/wdt_ibex_aon.h: Add watchdog.
> >
> > tests/qtest/ibex-aon-timer-test.c: Ibex AON Timer test.
> > tests/qtest/meson.build: Build the timer test.
> >
> > Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> > ---
> >  hw/riscv/Kconfig                   |   4 +
> >  hw/riscv/opentitan.c               |  21 +-
> >  hw/watchdog/Kconfig                |   3 +
> >  hw/watchdog/meson.build            |   1 +
> >  hw/watchdog/wdt_ibex_aon.c         | 405 +++++++++++++++++++++++++++++
> >  include/hw/riscv/opentitan.h       |   7 +-
> >  include/hw/watchdog/wdt_ibex_aon.h |  67 +++++  tests/qtest/ibex-aon-
> > timer-test.c  | 189 ++++++++++++++
> >  tests/qtest/meson.build            |   3 +
> >  9 files changed, 696 insertions(+), 4 deletions(-)  create mode 100644
> > hw/watchdog/wdt_ibex_aon.c  create mode 100644
> > include/hw/watchdog/wdt_ibex_aon.h
> >  create mode 100644 tests/qtest/ibex-aon-timer-test.c
> >
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig index
> > 79ff61c464..72094010be 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -4,6 +4,9 @@ config RISCV_NUMA
> >  config IBEX
> >      bool
> >
> > +config IBEX_AON
> > +    bool
> > +
> >  config MICROCHIP_PFSOC
> >      bool
> >      select CADENCE_SDHCI
> > @@ -20,6 +23,7 @@ config MICROCHIP_PFSOC  config OPENTITAN
> >      bool
> >      select IBEX
> > +    select IBEX_AON
> >      select UNIMP
> >
> >  config SHAKTI_C
> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index
> > af13dbe3b1..eae9b2504f 100644
> > --- a/hw/riscv/opentitan.c
> > +++ b/hw/riscv/opentitan.c
> > @@ -48,7 +48,7 @@ static const MemMapEntry ibex_memmap[] = {
> >      [IBEX_DEV_RSTMGR] =         {  0x40410000,  0x1000  },
> >      [IBEX_DEV_CLKMGR] =         {  0x40420000,  0x1000  },
> >      [IBEX_DEV_PINMUX] =         {  0x40460000,  0x1000  },
> > -    [IBEX_DEV_PADCTRL] =        {  0x40470000,  0x1000  },
> > +    [IBEX_DEV_AON_TIMER] =      {  0x40470000,  0x1000  },
> >      [IBEX_DEV_FLASH_CTRL] =     {  0x41000000,  0x1000  },
> >      [IBEX_DEV_AES] =            {  0x41100000,  0x1000  },
> >      [IBEX_DEV_HMAC] =           {  0x41110000,  0x1000  },
> > @@ -122,6 +122,8 @@ static void lowrisc_ibex_soc_init(Object *obj)
> >
> >      object_initialize_child(obj, "timer", &s->timer, TYPE_IBEX_TIMER);
> >
> > +    object_initialize_child(obj, "aon_timer", &s->aon_timer,
> > TYPE_IBEX_AON_TIMER);
> > +
> >      for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; i++) {
> >          object_initialize_child(obj, "spi_host[*]", &s->spi_host[i],
> >                                  TYPE_IBEX_SPI_HOST); @@ -207,6 +209,7
> @@ static void
> > lowrisc_ibex_soc_realize(DeviceState
> > *dev_soc, Error **errp)
> >                         3, qdev_get_gpio_in(DEVICE(&s->plic),
> >                         IBEX_UART0_RX_OVERFLOW_IRQ));
> >
> > +    /* RV Timer */
> >      if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer), errp)) {
> >          return;
> >      }
> > @@ -218,6 +221,20 @@ static void lowrisc_ibex_soc_realize(DeviceState
> > *dev_soc, Error **errp)
> >                            qdev_get_gpio_in(DEVICE(qemu_get_cpu(0)),
> >                                             IRQ_M_TIMER));
> >
> > +    /* AON Timer */
> > +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->aon_timer), errp)) {
> > +        return;
> > +    }
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->aon_timer), 0,
> > memmap[IBEX_DEV_AON_TIMER].base);
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->aon_timer),
> > +                       0, qdev_get_gpio_in(DEVICE(&s->plic),
> > +                       IBEX_AONTIMER_WDOG_BARK));
> > +    /*
> > +     * Note: There should be a line to pwrmgr but it's not implemented.
> > +     * TODO: Needs a line connected in, "counter-run" (stop the
> watchdog if
> > +     * debugging)
> > +     */
> > +
> >      /* SPI-Hosts */
> >      for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; ++i) {
> >          dev = DEVICE(&(s->spi_host[i])); @@ -265,8 +282,6 @@ static void
> > lowrisc_ibex_soc_realize(DeviceState
> > *dev_soc, Error **errp)
> >          memmap[IBEX_DEV_CLKMGR].base, memmap[IBEX_DEV_CLKMGR].size);
> >      create_unimplemented_device("riscv.lowrisc.ibex.pinmux",
> >          memmap[IBEX_DEV_PINMUX].base, memmap[IBEX_DEV_PINMUX].size);
> > -    create_unimplemented_device("riscv.lowrisc.ibex.padctrl",
> > -        memmap[IBEX_DEV_PADCTRL].base,
> > memmap[IBEX_DEV_PADCTRL].size);
> >      create_unimplemented_device("riscv.lowrisc.ibex.usbdev",
> >          memmap[IBEX_DEV_USBDEV].base, memmap[IBEX_DEV_USBDEV].size);
> >      create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl",
> > diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig index
> > 66e1d029e3..dde6c01a8c 100644
> > --- a/hw/watchdog/Kconfig
> > +++ b/hw/watchdog/Kconfig
> > @@ -20,3 +20,6 @@ config WDT_IMX2
> >
> >  config WDT_SBSA
> >      bool
> > +
> > +config WDT_IBEX_AON
> > +    bool
> > diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build index
> > 8974b5cf4c..21e2ede28f 100644
> > --- a/hw/watchdog/meson.build
> > +++ b/hw/watchdog/meson.build
> > @@ -7,3 +7,4 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
> > files('wdt_aspeed.c'))
> >  softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
> >  softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
> >  specific_ss.add(when: 'CONFIG_PSERIES', if_true:
> files('spapr_watchdog.c'))
> > +softmmu_ss.add(when: 'CONFIG_IBEX_AON', if_true:
> > +files('wdt_ibex_aon.c'))
> > diff --git a/hw/watchdog/wdt_ibex_aon.c b/hw/watchdog/wdt_ibex_aon.c
> > new file mode 100644 index 0000000000..d3cd56c634
> > --- /dev/null
> > +++ b/hw/watchdog/wdt_ibex_aon.c
> > @@ -0,0 +1,405 @@
> > +/*
> > + * 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.
> > + */
> > +#include "qemu/osdep.h"
> > +#include "qemu/timer.h"
> > +#include "qemu/log.h"
> > +#include "sysemu/reset.h"
> > +#include "sysemu/watchdog.h"
> > +#include "hw/register.h"
> > +#include "hw/irq.h"
> > +#include "migration/vmstate.h"
> > +
> > +#include "hw/watchdog/wdt_ibex_aon.h"
> > +
> > +REG32(ALERT_TEST,      0x00)
> > +REG32(WKUP_CTRL,       0x04)
> > +REG32(WKUP_THOLD,      0x08)
> > +REG32(WKUP_COUNT,      0x0c)
> > +REG32(WDOG_REGWEN,     0x10)
> > +REG32(WDOG_CTRL,       0x14)
> > +    FIELD(WDOG_CTRL, EN, 0, 1)
> > +    FIELD(WDOG_CTRL, PIS, 1, 1) /* Pause in sleep */
> > +REG32(WDOG_BARK_THOLD, 0x18) REG32(WDOG_BITE_THOLD, 0x1c)
> > +REG32(WDOG_COUNT,      0x20)
> > +REG32(INTR_STATE,      0x24)
> > +    FIELD(INTR_STATE, WKUP, 0, 1)
> > +    FIELD(INTR_STATE, WDOG, 1, 1)
> > +REG32(INTR_TEST,       0x28)
> > +REG32(WKUP_CAUSE,      0x2c)
> > +
> > +/*
> > + * This function updates the count in the register. It depends on the
> > +last time
> > + * a read had occurred and extrapolates the count via the clock freq
> > +and the
> > + * time elapsed.
> > + */
> > +static bool ibex_aon_update_count(IbexAONTimerState *s) {
> > +    /* If the timer is disabled, do not update count */
> > +    if (!(s->regs[R_WDOG_CTRL] & R_WDOG_CTRL_EN_MASK)) {
> > +        return false;
> > +    }
> > +    /* Lazily update wdog count. The count is truncated to fit. */
> > +    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    /* If for some reason we went back in time, elapsed cycles is
> negative. */
> Why it becomes negative? Due to bug?
>
Since QEMU is a simulation, time could (in theory) go backwards. Plus I
wanted to handle it just in case.

>
> > +    int64_t elapsed = now - s->wdog_last;
> > +    /* Get the count. */
> > +    const int64_t count = (elapsed / IBEX_AONTIMER_PERIOD_NS) +
> > +                          (int64_t) s->regs[R_WDOG_COUNT];
> > +    /* Saturate the counter. */
> > +    if (count < 0) {
> > +        s->regs[R_WDOG_COUNT] = 0;
> > +    } else if (count <= UINT32_MAX) {
> > +        s->regs[R_WDOG_COUNT] = (uint32_t) count;
> > +    } else {
> This implicitly says the COUNT remains to be 0XFFFFFFFF, if it overflows.
> Where does the spec say so ? I don't find it, and I think hardware
> implementation will be relatively complicated if we expect this behavior.
>
I don't know what behavior is supposed to happen (theoretically), but  I do
know that by the definition of the timer, it will have already triggered a
bite by then, causing a system reset. Thus, this if-branch should never be
reached. I could make that explicitly clear in the next version by calling
a reset then and there, but I'd prefer to handle bites within the
biter_expired function solely. I could also remove this else-branch and
replace it with a comment to make it clearer, which is probably what I'll
do.

> > +        s->regs[R_WDOG_COUNT] = UINT32_MAX;
> > +    }
> > +    /* Update the last-used timestamps */
> > +    s->wdog_last = now;
> > +    return true;
> > +}
> > +
> > +/* Called when the bark timer expires */ static void
> > +ibex_aon_barker_expired(void *opaque) {
> This may happen during ibex_aon_update_count(), right?
>
> > +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> > +    if (ibex_aon_update_count(s) &&
> This may happen during ibex_aon_update_count().
> Nested call may lead to incorrect s->regs[R_WDOG_COUNT] & s->wdog_last.
>

Can you elaborate? The timers for bark and bite are not updated in
"update_count".


> > +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
> > +        s->regs[R_INTR_STATE] |= (1 << 1);
> > +        qemu_irq_raise(s->bark_irq);
> > +    }
> > +}
> > +
> > +/* Called when the bite timer expires */ static void
> > +ibex_aon_biter_expired(void *opaque) {
> > +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> > +    if (ibex_aon_update_count(s) &&
> > +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BITE_THOLD]) {
> > +        resettable_reset(opaque, RESET_TYPE_COLD);
>
> Ditto.


> > +        watchdog_perform_action();
> > +    }
> > +}
> > +
> > +/*
> > + * Update the bark timer to reflect a new value of WDOG_COUNT or
> > + * WDOG_BARK_THOLD.
> > + */
> > +static void ibex_aon_update_bark_timer(IbexAONTimerState *s) {
> > +    if (!ibex_aon_update_count(s)) {
> > +        return;
> > +    }
> > +    /* Calculate the register count remaining */
> > +    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    const int64_t cur_count = (int64_t) s->regs[R_WDOG_COUNT];
> > +    const int64_t rem_bark = s->regs[R_WDOG_BARK_THOLD] - cur_count;
> > +    /* Extrapolate realtime from count based on clock period */
> > +    const int64_t delta_ns_bark = rem_bark * IBEX_AONTIMER_PERIOD_NS;
> > +    /* Timer updates */
> > +    timer_mod_ns(s->barker, now + delta_ns_bark); }
> > +
> > +/*
> > + * Update the bite timer to reflect a new value of WDOG_COUNT or
> > + * WDOG_BITE_THOLD.
> > + */
> > +static void ibex_aon_update_bite_timer(IbexAONTimerState *s) {
> > +    if (!ibex_aon_update_count(s)) {
> > +        return;
> > +    }
> > +    /* Calculate the register count remaining */
> > +    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +    const int64_t cur_count = (int64_t) s->regs[R_WDOG_COUNT];
> > +    const int64_t rem_bite = s->regs[R_WDOG_BITE_THOLD] - cur_count;
> > +    /* Extrapolate realtime from count based on clock period */
> > +    const int64_t delta_ns_bite = rem_bite * IBEX_AONTIMER_PERIOD_NS;
> > +    /* Timer updates */
> > +    timer_mod_ns(s->biter, now + delta_ns_bite); }
> > +
> > +static uint64_t ibex_aon_read(void *opaque, hwaddr addr, unsigned int
> > +size) {
> > +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> > +
> > +    uint64_t retval = 0;
> > +    switch (addr) {
> > +    case A_ALERT_TEST:
> > +        retval = s->regs[R_ALERT_TEST];
> > +        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:
> > +        retval = s->regs[R_WDOG_REGWEN];
> > +        break;
> > +    case A_WDOG_CTRL:
> > +        retval = s->regs[R_WDOG_CTRL];
> > +        break;
> > +    case A_WDOG_BARK_THOLD:
> > +        retval = s->regs[R_WDOG_BARK_THOLD];
> > +        break;
> > +    case A_WDOG_BITE_THOLD:
> > +        retval = s->regs[R_WDOG_BITE_THOLD];
> > +        break;
> > +    case A_WDOG_COUNT:
> > +        /* Lazily update the wdog count. */
> > +        ibex_aon_update_count(s);
> > +        retval = s->regs[R_WDOG_COUNT];
> > +        break;
> > +    case A_INTR_STATE:
> > +        retval = s->regs[R_INTR_STATE];
> > +        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;
> > +    }
> > +    return retval;
> > +}
> > +
> > +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);
> Unconditionally call "update_bark_timer", and rely on
> ibex_aon_update_bark_timer to skip if the timer is not enabled.
> This is not very straight forward IMO.
>

The code already does that, but needs a check before to make sure an update
to the count doesn't occur the moment the enable bit is set. The
alternative was to pull "update_count" out of "update_xx_timer" and require
an additional call to "update_count", which might actually be the better
call.


>
> > +            ibex_aon_update_bite_timer(s);
> > +        } 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);
>
> If the bark timer happens here and update s->regs[R_WDOG_COUNT] and
> s->wdog_last, we have trouble.
>

Could you please elaborate? I don't see why the bark timer would happen
here.


>
> > +        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)) {
> > +            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);
> > +                } else {
> > +                    timer_mod_ns(s->barker, now +
> IBEX_AONTIMER_PERIOD_NS);
> > +                }
> > +            }
> > +        }
> > +        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 */
> > diff --git a/tests/qtest/ibex-aon-timer-test.c
> > b/tests/qtest/ibex-aon-timer-test.c
> > new file mode 100644
> > index 0000000000..af33feac39
> > --- /dev/null
> > +++ b/tests/qtest/ibex-aon-timer-test.c
> > @@ -0,0 +1,189 @@
> > +/*
> > + * Testing the OpenTitan AON Timer
> > + *
> > + * Copyright (c) 2022 Rivos Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "libqtest.h"
> > +#include "qapi/qmp/qdict.h"
> > +
> > +#define AON_BASE_ADDR (0x40470000ul)
> > +#define AON_ADDR(addr) (AON_BASE_ADDR + addr) #define
> > AON_WKUP_IRQ 158
> > +#define AON_BARK_IRQ 159 #define AON_FREQ 200000 /* 200 KHz */
> > #define
> > +AON_PERIOD_NS 5000 #define NANOS_PER_SECOND 1000000000LL
> > +/* Test that reads work, and that the regs get reset to the correct
> > +value */ static void test_reads(void) {
> > +    QTestState *test = qtest_init("-M opentitan");
> > +    g_assert(qtest_readl(test, AON_ADDR(0x00)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x04)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x08)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x0c)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x10)) == 1);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x14)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x18)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x1c)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x20)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x24)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x28)) == 0);
> > +    g_assert(qtest_readl(test, AON_ADDR(0x2c)) == 0);
> > +
> > +    qtest_quit(test);
> > +}
> > +
> > +static void test_writes(void)
> > +{
> > +    /* Test that writes worked, while the config is unlocked */
> > +    QTestState *test = qtest_init("-M opentitan");
> > +
> > +
> > +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> > +                     ==, (1 << 19));
> > +
> > +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> > +                     ==, (1 << 20));
> > +
> > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* WDOG_CTRL enable */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> > +                     ==, 0x1);
> > +
> > +    qtest_writel(test, AON_ADDR(0x10), 0x0); /* WDOG_REGWEN enable */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)), ==, 0x0);
> > +
> > +    /*
> > +     * Test configuration lock
> > +     * These should not successfully write.
> > +     */
> > +    qtest_writel(test, AON_ADDR(0x14), 0);
> > +    qtest_writel(test, AON_ADDR(0x18), 0);
> > +    qtest_writel(test, AON_ADDR(0x1c), 0);
> > +
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
> > +                     ==, 0x1);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
> > +                     ==, (1 << 19));
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
> > +                     ==, (1 << 20));
> > +
> > +    /* This should not work, as it's a rw0c reg. */
> > +    qtest_writel(test, AON_ADDR(0x10), 1);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)),
> > +                     ==, 0x0);
> > +
> > +    qtest_quit(test);
> > +}
> > +
> > +
> > +/* Test whether the watchdog timer works during normal operation */
> > +static void test_operation(void) {
> > +    QTestState *test = qtest_init("-M opentitan");
> > +
> > +    /* Set up interrupts */
> > +    qtest_irq_intercept_in(test, "/machine/soc/plic");
> > +
> > +    /* Setup timer */
> > +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
> > +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD
> > + */
> > +
> > +    /* Run simulation, without enabling timer: */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 30);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, 0); /* checks if WDOG_COUNT gets updated */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, 0); /* checks if INTR_STATE is clear */
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Enable the timer, and test if the count is updated correctly */
> > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, 0);
> > +    qtest_clock_step(test, NANOS_PER_SECOND);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, AON_FREQ);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, 0);
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Disable the timer, and test if the count freezes */
> > +    qtest_writel(test, AON_ADDR(0x14), 0x0); /* set WDOG_CTRL = 0 */
> > +    qtest_clock_step(test, NANOS_PER_SECOND);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     ==, AON_FREQ);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, 0);
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Enable the timer, and run to bark */
> > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 1.62145);
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
> > +                     >=, (1 << 19));
> > +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, (1 << 1));
> > +
> > +    /* Disable IRQ by writing to INTR_STATE. Should bark next cycle */
> > +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
> > +                     ==, (1 << 1));
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +    qtest_clock_step(test, AON_PERIOD_NS);
> > +    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
> > +    /*
> > +     * Disable IRQ again, this time by setting WDOG_COUNT = 0 (pet) and
> > +     * writing to INTR_STATE.
> > +     */
> > +    qtest_writel(test, AON_ADDR(0x20), 0);
> > +    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
> > +    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
> > +
> > +    /* Ensure no bite occurs, after resetting the timer. */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 2.621436);
> > +    QDict *resp = qtest_qmp(test, "{'execute':'query-status'}");
> > +    g_assert(qdict_haskey(resp, "return"));
> > +    qobject_unref(resp);
> > +
> > +    /* Allow test to run to bite. */
> > +    qtest_clock_step(test, NANOS_PER_SECOND * 5.24289);
> > +    QDict *data = qdict_get_qdict(qtest_qmp_eventwait_ref(test,
> > "WATCHDOG"),
> > +                                  "data");
> > +    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "reset");
> > +    qobject_unref(data);
> > +    qtest_quit(test);
> > +}
> > +
> > +
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    g_test_init(&argc, &argv, NULL);
> > +    qtest_add_func("/ibex-aon-timer/reads", test_reads);
> > +    qtest_add_func("/ibex-aon-timer/writes", test_writes);
> > +    qtest_add_func("/ibex-aon-timer/op", test_operation);
> > +    return g_test_run();
> > +}
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index
> > e910cb32ca..fb63b8d3fa 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -234,6 +234,9 @@ qtests_s390x = \
> >     'cpu-plug-test',
> >     'migration-test']
> >
> > +qtests_riscv32 = \
> > +  ['ibex-aon-timer-test']
> > +
> >  qos_test_ss = ss.source_set()
> >  qos_test_ss.add(
> >    'ac97-test.c',
> > --
> > 2.34.1
>
> Thanks for the review.

-Tyler
Thomas Huth Sept. 27, 2022, 6:31 a.m. UTC | #5
Hi Tyler!

On 27/09/2022 01.03, Tyler Ng wrote:
> Hi Thomas,
> 
> On Thu, Sep 22, 2022 at 9:17 AM Thomas Huth <thuth@redhat.com 
> <mailto:thuth@redhat.com>> wrote:
> 
>     On 22/09/2022 17.58, Tyler Ng wrote:
>      > This commit adds most of an implementation of the OpenTitan Always-On
>      > Timer. The documentation for this timer is found here:
>      >
>      > https://docs.opentitan.org/hw/ip/aon_timer/doc/
>     <https://docs.opentitan.org/hw/ip/aon_timer/doc/>
>      >
>      > Using commit 217a0168ba118503c166a9587819e3811eeb0c0c
>      >
>      > The implementation includes most of the watchdog features; it does not
>      > implement the wakeup timer.
>      >
>      > An important note: the OpenTitan board uses the sifive_plic. The plic
>      > will not be able to claim the bark interrupt (159) because the sifive
>      > plic sets priority[159], but checks priority[158] for the priority, so
>      > it thinks that the interrupt's priority is 0 (effectively disabled).
>     ...
>      > diff --git a/tests/qtest/ibex-aon-timer-test.c
>      > b/tests/qtest/ibex-aon-timer-test.c
>      > new file mode 100644
>      > index 0000000000..af33feac39
>      > --- /dev/null
>      > +++ b/tests/qtest/ibex-aon-timer-test.c
>      > @@ -0,0 +1,189 @@
>      > +/*
>      > + * Testing the OpenTitan AON Timer
>      > + *
>      > + * Copyright (c) 2022 Rivos Inc.
>      > + *
>      > + * 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.
> 
>     Could you maybe add a SPDX license identifier at the beginning of the
>     comment, so that it's easier to identify the license at a first glance?
>     (also in the other new files)
> 
> Will do, was actually thinking of switching over to GPL-2.0-or-later as 
> opposed to MIT.

Yes, that would be the best fit for QEMU, I think.

>      > + */
>      > +
>      > +#include "qemu/osdep.h"
>      > +#include "libqtest.h"
>      > +#include "qapi/qmp/qdict.h"
>      > +
>      > +#define AON_BASE_ADDR (0x40470000ul)
>      > +#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
>      > +#define AON_WKUP_IRQ 158
>      > +#define AON_BARK_IRQ 159
>      > +#define AON_FREQ 200000 /* 200 KHz */
>      > +#define AON_PERIOD_NS 5000
>      > +#define NANOS_PER_SECOND 1000000000LL
>      > +/* Test that reads work, and that the regs get reset to the correct
>     value */
>      > +static void test_reads(void)
>      > +{
>      > +    QTestState *test = qtest_init("-M opentitan");
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x00)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x04)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x08)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x0c)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x10)) == 1); > +   
>     g_assert(qtest_readl(test, AON_ADDR(0x14)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x18)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x1c)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x20)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x24)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x28)) == 0);
>      > +    g_assert(qtest_readl(test, AON_ADDR(0x2c)) == 0);
> 
>     The read tests that check for 0 could maybe be simplified with a for-loop
>     (or two).
> 
> I'm not entirely sure about what benefit this would bring after writing it out.

Mostly a matter of taste. Keep it in the current shape if you prefer that.

>      > +    qtest_quit(test);
>      > +}
>      > +
>      > +static void test_writes(void)
>      > +{
>      > +    /* Test that writes worked, while the config is unlocked */
>      > +    QTestState *test = qtest_init("-M opentitan");
>      > +
>      > +
>      > +    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
>      > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
>      > +                     ==, (1 << 19));
>      > +
>      > +    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
>      > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
>      > +                     ==, (1 << 20));
>      > +
>      > +    qtest_writel(test, AON_ADDR(0x14), 0x1); /* WDOG_CTRL enable */
>      > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
>      > +                     ==, 0x1);
>      > +
>      > +    qtest_writel(test, AON_ADDR(0x10), 0x0); /* WDOG_REGWEN enable */
>      > +    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)), ==, 0x0);
> 
>     I think the above code would be better readable if you'd provide a helper
>     function like this:
> 
>     static void writel_and_assert(QTestState qts, int addr, int val)
>     {
>           qtest_writel(qts, AON_ADDR(addr), val);
>           g_assert_cmpuint(qtest_readl(qts, AON_ADDR(addr)), val);
>     }
> 
> Thanks for the suggestion. I decided to go with a macro instead though, 
> because it makes it easier to distinguish where an assertion failed without 
> a debugger.

That's a good idea, indeed!

  Thomas
Dong, Eddie Sept. 27, 2022, 10:04 p.m. UTC | #6
Hi Tyler:

> +}
> +
> +/* Called when the bark timer expires */ static void
> +ibex_aon_barker_expired(void *opaque) {
This may happen during ibex_aon_update_count(), right? 

> +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> +    if (ibex_aon_update_count(s) &&
        This may happen during ibex_aon_update_count().
        Nested call may lead to incorrect s->regs[R_WDOG_COUNT] & s->wdog_last. 

    Can you elaborate? The timers for bark and bite are not updated in "update_count".

When 1st ibex_aon_update_count() is executing, and is in the place PPP (after updating s->regs[R_WDOG_COUNT] but before updating s->wdog_last), a timer (barker) may happen.
Inside ibex_aon_barker_expired(), it calls ibex_aon_update_count() again (nest call), and update s->regs[R_WDOG_COUNT] & s->wdog_last, with the new value.
After the nest execution ends, and returns to the initial point (PPP) , the s->wdog_last is updated (with the value of 1st execution time), this leads to mismatch of s->regs[R_WDOG_COUNT] & s->wdog_last.

This case may not be triggered at normal case, but if the guest read A_WDOG_COUNT, the 1st ibex_aon_update_count() does execute, and bring the potential issue.

I think we can solve the problem, by not updating s->regs[R_WDOG_COUNT] & s->wdog_last in the timer call back API.  The update is not necessary given that the stored value is anyway not the current COUNT. We only need to update when the guest write the COUNT.


> +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
> +        s->regs[R_INTR_STATE] |= (1 << 1);
> +        qemu_irq_raise(s->bark_irq);
> +    }
> +}
> +


THX  Eddie
Tyler Ng Sept. 28, 2022, 12:13 a.m. UTC | #7
Hi Eddie,


On Tue, Sep 27, 2022 at 3:04 PM Dong, Eddie <eddie.dong@intel.com> wrote:

> Hi Tyler:
>
> > +}
> > +
> > +/* Called when the bark timer expires */ static void
> > +ibex_aon_barker_expired(void *opaque) {
> This may happen during ibex_aon_update_count(), right?
>
> > +    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
> > +    if (ibex_aon_update_count(s) &&
>         This may happen during ibex_aon_update_count().
>         Nested call may lead to incorrect s->regs[R_WDOG_COUNT] &
> s->wdog_last.
>
>     Can you elaborate? The timers for bark and bite are not updated in
> "update_count".
>
> When 1st ibex_aon_update_count() is executing, and is in the place PPP
> (after updating s->regs[R_WDOG_COUNT] but before updating s->wdog_last), a
> timer (barker) may happen.
> Inside ibex_aon_barker_expired(), it calls ibex_aon_update_count() again
> (nest call), and update s->regs[R_WDOG_COUNT] & s->wdog_last, with the new
> value.
> After the nest execution ends, and returns to the initial point (PPP) ,
> the s->wdog_last is updated (with the value of 1st execution time), this
> leads to mismatch of s->regs[R_WDOG_COUNT] & s->wdog_last.
>
> This case may not be triggered at normal case, but if the guest read
> A_WDOG_COUNT, the 1st ibex_aon_update_count() does execute, and bring the
> potential issue.
>

I see, I wasn't aware that the virtual clock continued running while the
device address was being read.


> I think we can solve the problem, by not updating s->regs[R_WDOG_COUNT] &
> s->wdog_last in the timer call back API.  The update is not necessary given
> that the stored value is anyway not the current COUNT. We only need to
> update when the guest write the COUNT.
>
>
My initial concern about this is that the HW does the comparison check to
determine a bark/bite occurred, which is why I think the count should be
updated on a timer expiration,


>
> > +        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
> > +        s->regs[R_INTR_STATE] |= (1 << 1);
> > +        qemu_irq_raise(s->bark_irq);
> > +    }
> > +}
> > +
>
>
> THX  Eddie
>
Thanks,
-Tyler
Dong, Eddie Sept. 28, 2022, 6:25 p.m. UTC | #8
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 */
diff mbox series

Patch

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index 79ff61c464..72094010be 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -4,6 +4,9 @@  config RISCV_NUMA
 config IBEX
     bool

+config IBEX_AON
+    bool
+
 config MICROCHIP_PFSOC
     bool
     select CADENCE_SDHCI
@@ -20,6 +23,7 @@  config MICROCHIP_PFSOC
 config OPENTITAN
     bool
     select IBEX
+    select IBEX_AON
     select UNIMP

 config SHAKTI_C
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index af13dbe3b1..eae9b2504f 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -48,7 +48,7 @@  static const MemMapEntry ibex_memmap[] = {
     [IBEX_DEV_RSTMGR] =         {  0x40410000,  0x1000  },
     [IBEX_DEV_CLKMGR] =         {  0x40420000,  0x1000  },
     [IBEX_DEV_PINMUX] =         {  0x40460000,  0x1000  },
-    [IBEX_DEV_PADCTRL] =        {  0x40470000,  0x1000  },
+    [IBEX_DEV_AON_TIMER] =      {  0x40470000,  0x1000  },
     [IBEX_DEV_FLASH_CTRL] =     {  0x41000000,  0x1000  },
     [IBEX_DEV_AES] =            {  0x41100000,  0x1000  },
     [IBEX_DEV_HMAC] =           {  0x41110000,  0x1000  },
@@ -122,6 +122,8 @@  static void lowrisc_ibex_soc_init(Object *obj)

     object_initialize_child(obj, "timer", &s->timer, TYPE_IBEX_TIMER);

+    object_initialize_child(obj, "aon_timer", &s->aon_timer,
TYPE_IBEX_AON_TIMER);
+
     for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; i++) {
         object_initialize_child(obj, "spi_host[*]", &s->spi_host[i],
                                 TYPE_IBEX_SPI_HOST);
@@ -207,6 +209,7 @@  static void lowrisc_ibex_soc_realize(DeviceState
*dev_soc, Error **errp)
                        3, qdev_get_gpio_in(DEVICE(&s->plic),
                        IBEX_UART0_RX_OVERFLOW_IRQ));

+    /* RV Timer */
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer), errp)) {
         return;
     }
@@ -218,6 +221,20 @@  static void lowrisc_ibex_soc_realize(DeviceState
*dev_soc, Error **errp)
                           qdev_get_gpio_in(DEVICE(qemu_get_cpu(0)),
                                            IRQ_M_TIMER));

+    /* AON Timer */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->aon_timer), errp)) {
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->aon_timer), 0,
memmap[IBEX_DEV_AON_TIMER].base);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->aon_timer),
+                       0, qdev_get_gpio_in(DEVICE(&s->plic),
+                       IBEX_AONTIMER_WDOG_BARK));
+    /*
+     * Note: There should be a line to pwrmgr but it's not implemented.
+     * TODO: Needs a line connected in, "counter-run" (stop the watchdog if
+     * debugging)
+     */
+
     /* SPI-Hosts */
     for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; ++i) {
         dev = DEVICE(&(s->spi_host[i]));
@@ -265,8 +282,6 @@  static void lowrisc_ibex_soc_realize(DeviceState
*dev_soc, Error **errp)
         memmap[IBEX_DEV_CLKMGR].base, memmap[IBEX_DEV_CLKMGR].size);
     create_unimplemented_device("riscv.lowrisc.ibex.pinmux",
         memmap[IBEX_DEV_PINMUX].base, memmap[IBEX_DEV_PINMUX].size);
-    create_unimplemented_device("riscv.lowrisc.ibex.padctrl",
-        memmap[IBEX_DEV_PADCTRL].base, memmap[IBEX_DEV_PADCTRL].size);
     create_unimplemented_device("riscv.lowrisc.ibex.usbdev",
         memmap[IBEX_DEV_USBDEV].base, memmap[IBEX_DEV_USBDEV].size);
     create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl",
diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig
index 66e1d029e3..dde6c01a8c 100644
--- a/hw/watchdog/Kconfig
+++ b/hw/watchdog/Kconfig
@@ -20,3 +20,6 @@  config WDT_IMX2

 config WDT_SBSA
     bool
+
+config WDT_IBEX_AON
+    bool
diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
index 8974b5cf4c..21e2ede28f 100644
--- a/hw/watchdog/meson.build
+++ b/hw/watchdog/meson.build
@@ -7,3 +7,4 @@  softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
files('wdt_aspeed.c'))
 softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
 softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
 specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
+softmmu_ss.add(when: 'CONFIG_IBEX_AON', if_true: files('wdt_ibex_aon.c'))
diff --git a/hw/watchdog/wdt_ibex_aon.c b/hw/watchdog/wdt_ibex_aon.c
new file mode 100644
index 0000000000..d3cd56c634
--- /dev/null
+++ b/hw/watchdog/wdt_ibex_aon.c
@@ -0,0 +1,405 @@ 
+/*
+ * 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.
+ */
+#include "qemu/osdep.h"
+#include "qemu/timer.h"
+#include "qemu/log.h"
+#include "sysemu/reset.h"
+#include "sysemu/watchdog.h"
+#include "hw/register.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+
+#include "hw/watchdog/wdt_ibex_aon.h"
+
+REG32(ALERT_TEST,      0x00)
+REG32(WKUP_CTRL,       0x04)
+REG32(WKUP_THOLD,      0x08)
+REG32(WKUP_COUNT,      0x0c)
+REG32(WDOG_REGWEN,     0x10)
+REG32(WDOG_CTRL,       0x14)
+    FIELD(WDOG_CTRL, EN, 0, 1)
+    FIELD(WDOG_CTRL, PIS, 1, 1) /* Pause in sleep */
+REG32(WDOG_BARK_THOLD, 0x18)
+REG32(WDOG_BITE_THOLD, 0x1c)
+REG32(WDOG_COUNT,      0x20)
+REG32(INTR_STATE,      0x24)
+    FIELD(INTR_STATE, WKUP, 0, 1)
+    FIELD(INTR_STATE, WDOG, 1, 1)
+REG32(INTR_TEST,       0x28)
+REG32(WKUP_CAUSE,      0x2c)
+
+/*
+ * This function updates the count in the register. It depends on the last time
+ * a read had occurred and extrapolates the count via the clock freq and the
+ * time elapsed.
+ */
+static bool ibex_aon_update_count(IbexAONTimerState *s)
+{
+    /* If the timer is disabled, do not update count */
+    if (!(s->regs[R_WDOG_CTRL] & R_WDOG_CTRL_EN_MASK)) {
+        return false;
+    }
+    /* Lazily update wdog count. The count is truncated to fit. */
+    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    /* If for some reason we went back in time, elapsed cycles is negative. */
+    int64_t elapsed = now - s->wdog_last;
+    /* Get the count. */
+    const int64_t count = (elapsed / IBEX_AONTIMER_PERIOD_NS) +
+                          (int64_t) s->regs[R_WDOG_COUNT];
+    /* Saturate the counter. */
+    if (count < 0) {
+        s->regs[R_WDOG_COUNT] = 0;
+    } else if (count <= UINT32_MAX) {
+        s->regs[R_WDOG_COUNT] = (uint32_t) count;
+    } else {
+        s->regs[R_WDOG_COUNT] = UINT32_MAX;
+    }
+    /* Update the last-used timestamps */
+    s->wdog_last = now;
+    return true;
+}
+
+/* Called when the bark timer expires */
+static void ibex_aon_barker_expired(void *opaque)
+{
+    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
+    if (ibex_aon_update_count(s) &&
+        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BARK_THOLD]) {
+        s->regs[R_INTR_STATE] |= (1 << 1);
+        qemu_irq_raise(s->bark_irq);
+    }
+}
+
+/* Called when the bite timer expires */
+static void ibex_aon_biter_expired(void *opaque)
+{
+    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
+    if (ibex_aon_update_count(s) &&
+        s->regs[R_WDOG_COUNT] >= s->regs[R_WDOG_BITE_THOLD]) {
+        resettable_reset(opaque, RESET_TYPE_COLD);
+        watchdog_perform_action();
+    }
+}
+
+/*
+ * Update the bark timer to reflect a new value of WDOG_COUNT or
+ * WDOG_BARK_THOLD.
+ */
+static void ibex_aon_update_bark_timer(IbexAONTimerState *s)
+{
+    if (!ibex_aon_update_count(s)) {
+        return;
+    }
+    /* Calculate the register count remaining */
+    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    const int64_t cur_count = (int64_t) s->regs[R_WDOG_COUNT];
+    const int64_t rem_bark = s->regs[R_WDOG_BARK_THOLD] - cur_count;
+    /* Extrapolate realtime from count based on clock period */
+    const int64_t delta_ns_bark = rem_bark * IBEX_AONTIMER_PERIOD_NS;
+    /* Timer updates */
+    timer_mod_ns(s->barker, now + delta_ns_bark);
+}
+
+/*
+ * Update the bite timer to reflect a new value of WDOG_COUNT or
+ * WDOG_BITE_THOLD.
+ */
+static void ibex_aon_update_bite_timer(IbexAONTimerState *s)
+{
+    if (!ibex_aon_update_count(s)) {
+        return;
+    }
+    /* Calculate the register count remaining */
+    const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    const int64_t cur_count = (int64_t) s->regs[R_WDOG_COUNT];
+    const int64_t rem_bite = s->regs[R_WDOG_BITE_THOLD] - cur_count;
+    /* Extrapolate realtime from count based on clock period */
+    const int64_t delta_ns_bite = rem_bite * IBEX_AONTIMER_PERIOD_NS;
+    /* Timer updates */
+    timer_mod_ns(s->biter, now + delta_ns_bite);
+}
+
+static uint64_t ibex_aon_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    IbexAONTimerState *s = IBEX_AON_TIMER(opaque);
+
+    uint64_t retval = 0;
+    switch (addr) {
+    case A_ALERT_TEST:
+        retval = s->regs[R_ALERT_TEST];
+        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:
+        retval = s->regs[R_WDOG_REGWEN];
+        break;
+    case A_WDOG_CTRL:
+        retval = s->regs[R_WDOG_CTRL];
+        break;
+    case A_WDOG_BARK_THOLD:
+        retval = s->regs[R_WDOG_BARK_THOLD];
+        break;
+    case A_WDOG_BITE_THOLD:
+        retval = s->regs[R_WDOG_BITE_THOLD];
+        break;
+    case A_WDOG_COUNT:
+        /* Lazily update the wdog count. */
+        ibex_aon_update_count(s);
+        retval = s->regs[R_WDOG_COUNT];
+        break;
+    case A_INTR_STATE:
+        retval = s->regs[R_INTR_STATE];
+        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;
+    }
+    return retval;
+}
+
+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);
+        } 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)) {
+            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);
+                } else {
+                    timer_mod_ns(s->barker, now + IBEX_AONTIMER_PERIOD_NS);
+                }
+            }
+        }
+        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 */
diff --git a/tests/qtest/ibex-aon-timer-test.c
b/tests/qtest/ibex-aon-timer-test.c
new file mode 100644
index 0000000000..af33feac39
--- /dev/null
+++ b/tests/qtest/ibex-aon-timer-test.c
@@ -0,0 +1,189 @@ 
+/*
+ * Testing the OpenTitan AON Timer
+ *
+ * Copyright (c) 2022 Rivos Inc.
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qapi/qmp/qdict.h"
+
+#define AON_BASE_ADDR (0x40470000ul)
+#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
+#define AON_WKUP_IRQ 158
+#define AON_BARK_IRQ 159
+#define AON_FREQ 200000 /* 200 KHz */
+#define AON_PERIOD_NS 5000
+#define NANOS_PER_SECOND 1000000000LL
+/* Test that reads work, and that the regs get reset to the correct value */
+static void test_reads(void)
+{
+    QTestState *test = qtest_init("-M opentitan");
+    g_assert(qtest_readl(test, AON_ADDR(0x00)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x04)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x08)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x0c)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x10)) == 1);
+    g_assert(qtest_readl(test, AON_ADDR(0x14)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x18)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x1c)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x20)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x24)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x28)) == 0);
+    g_assert(qtest_readl(test, AON_ADDR(0x2c)) == 0);
+
+    qtest_quit(test);
+}
+
+static void test_writes(void)
+{
+    /* Test that writes worked, while the config is unlocked */
+    QTestState *test = qtest_init("-M opentitan");
+
+
+    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
+                     ==, (1 << 19));
+
+    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
+                     ==, (1 << 20));
+
+    qtest_writel(test, AON_ADDR(0x14), 0x1); /* WDOG_CTRL enable */
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
+                     ==, 0x1);
+
+    qtest_writel(test, AON_ADDR(0x10), 0x0); /* WDOG_REGWEN enable */
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)), ==, 0x0);
+
+    /*
+     * Test configuration lock
+     * These should not successfully write.
+     */
+    qtest_writel(test, AON_ADDR(0x14), 0);
+    qtest_writel(test, AON_ADDR(0x18), 0);
+    qtest_writel(test, AON_ADDR(0x1c), 0);
+
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x14)),
+                     ==, 0x1);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x18)),
+                     ==, (1 << 19));
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x1c)),
+                     ==, (1 << 20));
+
+    /* This should not work, as it's a rw0c reg. */
+    qtest_writel(test, AON_ADDR(0x10), 1);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x10)),
+                     ==, 0x0);
+
+    qtest_quit(test);
+}
+
+
+/* Test whether the watchdog timer works during normal operation */
+static void test_operation(void)
+{
+    QTestState *test = qtest_init("-M opentitan");
+
+    /* Set up interrupts */
+    qtest_irq_intercept_in(test, "/machine/soc/plic");
+
+    /* Setup timer */
+    qtest_writel(test, AON_ADDR(0x18), (1 << 19)); /* WDOG_BARK_THOLD */
+    qtest_writel(test, AON_ADDR(0x1c), (1 << 20)); /* WDOG_BITE_THOLD */
+
+    /* Run simulation, without enabling timer: */
+    qtest_clock_step(test, NANOS_PER_SECOND * 30);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
+                     ==, 0); /* checks if WDOG_COUNT gets updated */
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
+                     ==, 0); /* checks if INTR_STATE is clear */
+    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
+
+    /* Enable the timer, and test if the count is updated correctly */
+    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
+                     ==, 0);
+    qtest_clock_step(test, NANOS_PER_SECOND);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
+                     ==, AON_FREQ);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
+                     ==, 0);
+    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
+
+    /* Disable the timer, and test if the count freezes */
+    qtest_writel(test, AON_ADDR(0x14), 0x0); /* set WDOG_CTRL = 0 */
+    qtest_clock_step(test, NANOS_PER_SECOND);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
+                     ==, AON_FREQ);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
+                     ==, 0);
+    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
+
+    /* Enable the timer, and run to bark */
+    qtest_writel(test, AON_ADDR(0x14), 0x1); /* set WDOG_CTRL = 1 */
+    qtest_clock_step(test, NANOS_PER_SECOND * 1.62145);
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x20)),
+                     >=, (1 << 19));
+    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
+                     ==, (1 << 1));
+
+    /* Disable IRQ by writing to INTR_STATE. Should bark next cycle */
+    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
+    g_assert_cmpuint(qtest_readl(test, AON_ADDR(0x24)),
+                     ==, (1 << 1));
+    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
+    qtest_clock_step(test, AON_PERIOD_NS);
+    g_assert(qtest_get_irq(test, AON_BARK_IRQ));
+    /*
+     * Disable IRQ again, this time by setting WDOG_COUNT = 0 (pet) and
+     * writing to INTR_STATE.
+     */
+    qtest_writel(test, AON_ADDR(0x20), 0);
+    qtest_writel(test, AON_ADDR(0x24), (1 << 1));
+    g_assert(!qtest_get_irq(test, AON_BARK_IRQ));
+
+    /* Ensure no bite occurs, after resetting the timer. */
+    qtest_clock_step(test, NANOS_PER_SECOND * 2.621436);
+    QDict *resp = qtest_qmp(test, "{'execute':'query-status'}");
+    g_assert(qdict_haskey(resp, "return"));
+    qobject_unref(resp);
+
+    /* Allow test to run to bite. */
+    qtest_clock_step(test, NANOS_PER_SECOND * 5.24289);
+    QDict *data = qdict_get_qdict(qtest_qmp_eventwait_ref(test, "WATCHDOG"),
+                                  "data");
+    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "reset");
+    qobject_unref(data);
+    qtest_quit(test);
+}
+
+
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/ibex-aon-timer/reads", test_reads);
+    qtest_add_func("/ibex-aon-timer/writes", test_writes);
+    qtest_add_func("/ibex-aon-timer/op", test_operation);
+    return g_test_run();
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e910cb32ca..fb63b8d3fa 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -234,6 +234,9 @@  qtests_s390x = \
    'cpu-plug-test',
    'migration-test']

+qtests_riscv32 = \
+  ['ibex-aon-timer-test']
+
 qos_test_ss = ss.source_set()
 qos_test_ss.add(
   'ac97-test.c',