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

[Dong, Eddie]

Hi Tyler:
Some other comments embedded.


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

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

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

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

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

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

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

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

> +}
> +}
> +}
> +break;
> +case A_INTR_TEST:
> +qemu_log_mask(LOG_UNIMP, "%s: Intr test not implemented",
> __func__);
> +break;
> +case A_WKUP_CAUSE:
> +qemu_log_mask(LOG_UNIMP,
> +"%s: Wkup cause not implemented", __func__);
> +break;
> +default:
> +qemu_log_mask(LOG_GUEST_ERROR,
> +"%s: Invalid register read 0x%" HWADDR_PRIx "\n",
> +__func__, addr);
> +break;
> +}
> +}
> +
> +static void ibex_aon_enter_reset(Object *obj, ResetType type) {
> +IbexAONTimerState *s = IBEX_AON_TIMER(obj);
> +s->regs[R_ALERT_TEST]  = 0x0;
> +s->regs[R_WKUP_CTRL]   = 0x0;
> +s->regs[R_WKUP_THOLD]  = 0x0;
> +s->regs[R_WKUP_COUNT]  = 0x0;
> +s->regs[R_WDOG_REGWEN] = 0x1;
> +

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

[Tyler Ng]

Hi Eddie,


On Tue, Sep 27, 2022 at 3:04 PM Dong, Eddie  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

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

[Dong, Eddie]

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

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

[Thomas Huth]

 Hi Tyler!

On 27/09/2022 01.03, Tyler Ng wrote:

Hi Thomas,

On Thu, Sep 22, 2022 at 9:17 AM Thomas Huth > 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 00..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 (0x4047ul)
 > +#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
 > +#define AON_WKUP_IRQ 158
 > +#define AON_BARK_IRQ 159
 > +#define AON_FREQ 20 /* 200 KHz */
 > +#define AON_PERIOD_NS 5000
 > +#define NANOS_PER_SECOND 10LL
 > +/* 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, 

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

[Tyler Ng]

Hi Eddie,


On Mon, Sep 26, 2022 at 1:46 PM Dong, Eddie  wrote:

>
>
> > -Original Message-
> > From: Qemu-devel 
> > On Behalf Of Tyler Ng
> > Sent: Thursday, September 22, 2022 8:59 AM
> > To: open list:RISC-V ; qemu-devel@nongnu.org
> > Developers 
> > Cc: Alistair Francis ; Meng, Bin
> > ; Thomas Huth ; Paolo
> > Bonzini ; Palmer Dabbelt ;
> > Laurent Vivier 
> > 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 
> > ---
> >  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] = {  0x4041,  0x1000  },
> >  [IBEX_DEV_CLKMGR] = {  0x4042,  0x1000  },
> >  [IBEX_DEV_PINMUX] = {  0x4046,  0x1000  },
> > -[IBEX_DEV_PADCTRL] ={  0x4047,  0x1000  },
> > +[IBEX_DEV_AON_TIMER] =  {  0x4047,  0x1000  },
> >  [IBEX_DEV_FLASH_CTRL] = {  0x4100,  0x1000  },
> >  [IBEX_DEV_AES] ={  0x4110,  0x1000  },
> >  [IBEX_DEV_HMAC] =   {  0x4111,  0x1000  },
> > @@ -122,6 +122,8 @@ static void lowrisc_ibex_soc_init(Object *obj)
> >
> >  object_initialize_child(obj, "timer", >timer, TYPE_IBEX_TIMER);
> >
> > +object_initialize_child(obj, "aon_timer", >aon_timer,
> > TYPE_IBEX_AON_TIMER);
> > +
> >  for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; i++) {
> >  object_initialize_child(obj, "spi_host[*]", >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(>plic),
> > IBEX_UART0_RX_OVERFLOW_IRQ));
> >
> > +/* RV Timer */
> >  if (!sysbus_realize(SYS_BUS_DEVICE(>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(>aon_timer), errp)) {
> > +return;
> > +}
> > +sysbus_mmio_map(SYS_BUS_DEVICE(>aon_timer), 0,
> > memmap[IBEX_DEV_AON_TIMER].base);
> > +sysbus_connect_irq(SYS_BUS_DEVICE(>aon_timer),

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

[Tyler Ng]

Hi Thomas,

On Thu, Sep 22, 2022 at 9:17 AM Thomas Huth  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 00..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 (0x4047ul)
> > +#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
> > +#define AON_WKUP_IRQ 158
> > +#define AON_BARK_IRQ 159
> > +#define AON_FREQ 20 /* 200 KHz */
> > +#define AON_PERIOD_NS 5000
> > +#define NANOS_PER_SECOND 10LL
> > +/* 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

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

[Dong, Eddie]

> -Original Message-
> From: Qemu-devel 
> On Behalf Of Tyler Ng
> Sent: Thursday, September 22, 2022 8:59 AM
> To: open list:RISC-V ; qemu-devel@nongnu.org
> Developers 
> Cc: Alistair Francis ; Meng, Bin
> ; Thomas Huth ; Paolo
> Bonzini ; Palmer Dabbelt ;
> Laurent Vivier 
> 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 
> ---
>  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] = {  0x4041,  0x1000  },
>  [IBEX_DEV_CLKMGR] = {  0x4042,  0x1000  },
>  [IBEX_DEV_PINMUX] = {  0x4046,  0x1000  },
> -[IBEX_DEV_PADCTRL] ={  0x4047,  0x1000  },
> +[IBEX_DEV_AON_TIMER] =  {  0x4047,  0x1000  },
>  [IBEX_DEV_FLASH_CTRL] = {  0x4100,  0x1000  },
>  [IBEX_DEV_AES] ={  0x4110,  0x1000  },
>  [IBEX_DEV_HMAC] =   {  0x4111,  0x1000  },
> @@ -122,6 +122,8 @@ static void lowrisc_ibex_soc_init(Object *obj)
> 
>  object_initialize_child(obj, "timer", >timer, TYPE_IBEX_TIMER);
> 
> +object_initialize_child(obj, "aon_timer", >aon_timer,
> TYPE_IBEX_AON_TIMER);
> +
>  for (int i = 0; i < OPENTITAN_NUM_SPI_HOSTS; i++) {
>  object_initialize_child(obj, "spi_host[*]", >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(>plic),
> IBEX_UART0_RX_OVERFLOW_IRQ));
> 
> +/* RV Timer */
>  if (!sysbus_realize(SYS_BUS_DEVICE(>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(>aon_timer), errp)) {
> +return;
> +}
> +sysbus_mmio_map(SYS_BUS_DEVICE(>aon_timer), 0,
> memmap[IBEX_DEV_AON_TIMER].base);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>aon_timer),
> +   0, qdev_get_gpio_in(DEVICE(>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
> + * 

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

[Thomas Huth]

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 00..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 (0x4047ul)
+#define AON_ADDR(addr) (AON_BASE_ADDR + addr)
+#define AON_WKUP_IRQ 158
+#define AON_BARK_IRQ 159
+#define AON_FREQ 20 /* 200 KHz */
+#define AON_PERIOD_NS 5000
+#define NANOS_PER_SECOND 10LL
+/* 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));
+