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

[Thomas Huth <thuth@redhat.com>]

  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