Re: [PATCH v3 3/3] tests/qtest: sifive-e-aon-watchdog-test.c : Add QTest of watchdog of sifive_e

[Tommy Wu <tommy.wu@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]

Hi Thomas,
Sorry for the very late reply.
Thanks for the code review! I'll send patch v4 to fix these issues.

On Wed, Nov 30, 2022 at 7:29 PM Thomas Huth <thuth@redhat.com> wrote:

> On 30/11/2022 02.54, Tommy Wu wrote:
> > Add some simple tests of the watchdog timer in the always-on domain
> device
> > of HiFive 1 rev b.
> >
> > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> > ---
> ...
> > diff --git a/tests/qtest/sifive-e-aon-watchdog-test.c
> b/tests/qtest/sifive-e-aon-watchdog-test.c
> > new file mode 100644
> > index 0000000000..1f454c266a
> > --- /dev/null
> > +++ b/tests/qtest/sifive-e-aon-watchdog-test.c
> > @@ -0,0 +1,650 @@
>
> I'd suggest to add at least add a SPDX identifier so that people know
> which
> license this file has. A short comment on what is being tested here would
> also be nice (though it is already quite obvious from the file name).
>
> > +#include "qemu/osdep.h"
> > +#include "qemu/timer.h"
> > +#include "qemu/bitops.h"
> > +#include "libqtest-single.h"
>
> Note that libqtest-single.h restricts your code to testing with one QEMU
> binary. If you want to write code that is a little bit more future-proof
> (e.g. if some of the functions should be used with migration testing later
> etc.), I'd suggest to get along without libqtest-single.h and pass the
> QTestState around as a parameter of the functions.
>
> [...]
> > +static void test_wdogcfg(void)
> > +{
> > +    uint32_t tmp_cfg;
> > +
> > +    test_init();
> > +
> > +    tmp_cfg = readl(WDOG_BASE + WDOGCFG);
> > +    writel(WDOG_BASE + WDOGCFG, 0xFFFFFFFF);
> > +    g_assert(readl(WDOG_BASE + WDOGCFG) == tmp_cfg);
> > +
> > +    writel(WDOG_BASE + WDOGKEY, SIFIVE_E_AON_WDOGKEY);
> > +    writel(WDOG_BASE + WDOGCFG, 0xFFFFFFFF);
> > +    g_assert(0xFFFFFFFF == readl(WDOG_BASE + WDOGCFG));
> > +
> > +    tmp_cfg = readl(WDOG_BASE + WDOGCFG);
> > +    g_assert(15 ==
> > +             FIELD_EX32(tmp_cfg,
> > +                        AON_WDT_WDOGCFG,
> > +                        SCALE));
> > +    g_assert(1 ==
> > +             FIELD_EX32(tmp_cfg,
> > +                        AON_WDT_WDOGCFG,
> > +                         RSTEN));
> > +    g_assert(1 ==
> > +             FIELD_EX32(tmp_cfg,
> > +                        AON_WDT_WDOGCFG,
> > +                        ZEROCMP));
> > +    g_assert(1 ==
> > +             FIELD_EX32(tmp_cfg,
> > +                        AON_WDT_WDOGCFG,
> > +                        EN_ALWAYS));
> > +    g_assert(1 ==
> > +             FIELD_EX32(tmp_cfg,
> > +                        AON_WDT_WDOGCFG,
> > +                        EN_CORE_AWAKE));
> > +    g_assert(1 ==
> > +             FIELD_EX32(tmp_cfg,
> > +                        AON_WDT_WDOGCFG,
> > +                        IP0));
>
> Just a matter of taste, but at least I would prefer the statements on one
> line as long as they still fit into 80 columns.
>
>   Thomas
>
>

[-- Attachment #2: Type: text/html, Size: 3907 bytes --]