Hi Thomas, On Thu, Sep 22, 2022 at 9:17 AM Thomas Huth <[email protected]> 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
