qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Thomas Huth
Subject: Re: [PATCH v3 3/3] tests/qtest: sifive-e-aon-watchdog-test.c : Add QTest of watchdog of sifive_e
Date: Wed, 30 Nov 2022 12:29:31 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]