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

[Tommy Wu]

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