[PATCH 0/5] Make WDOGCONTROL.INTEN the counter enable of the CMSDK APB Watchdog

[PATCH 0/5] Make WDOGCONTROL.INTEN the counter enable of the CMSDK APB Watchdog

[Roque Arcudia Hernandez]

The following patchset tries to address an issue where the watchdog
counter was running as soon as the device is out of reset. This
created a few problems with the firmware under test. It was pointed
out that the firmware under test was already working on an emulator
using the real RTL. Further review showed that the WDOGCONTROL.INTEN
is also expected to be the counter enable as it is defined in this
page from ARM:

https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model?lang=en

     |       | Enable the interrupt event, WDOGINT. Set to 1 to
     |       | enable the counter and the interrupt, or to 0 to
 [0] | INTEN | disable the counter and interrupt. Reloads the
     |       | counter from the value in WDOGLOAD when the interrupt
     |       | is enabled, after previously being disabled.

The second half of the patch is focused on adding tests for this
behavior. It was required to modify the test to be able to run some
test cases in the MPS2 since the the stellaris machine had 2 issues:

  - The stellaris machine did not run the counter out of reset
    because of the way it is reset. The function
    ssys_calculate_system_clock is not called until the reset phase
    stellaris_sys_reset_hold which is after the watchdog executes its
    reset, so the watchdog would not start the counter due to the
    clock not being set up at the time of reset.

  - The Stellaris machine uses a derived watchdog and not the
    original CMSDK one which has a special behavior where it cannot
    be stopped.

The MPS2 machine on the other hand uses the standard CMSDK watchdog
and could expose the problems we were dealing with.

Roque Arcudia Hernandez (5):
  hw/watchdog/cmsdk_apb_watchdog: Fix broken link
  hw/watchdog/cmsdk_apb_watchdog: Fix INTEN issues
  tests/qtest/cmsdk-apb-watchdog-test: Parameterize tests
  tests/qtest/cmsdk-apb-watchdog-test: Don't abort on assertion failure
  tests/qtest/cmsdk-apb-watchdog-test: Test INTEN as counter enable

 hw/watchdog/cmsdk-apb-watchdog.c      |  38 ++-
 tests/qtest/cmsdk-apb-watchdog-test.c | 328 +++++++++++++++++++++++---
 tests/qtest/meson.build               |   3 +-
 3 files changed, 329 insertions(+), 40 deletions(-)

-- 
2.47.0.338.g60cca15819-goog

[PATCH 1/5] hw/watchdog/cmsdk_apb_watchdog: Fix broken link

[Roque Arcudia Hernandez]

The patch changes the comments to point to the latest Design Kit
Technical Reference Manual.

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
---
 hw/watchdog/cmsdk-apb-watchdog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/watchdog/cmsdk-apb-watchdog.c b/hw/watchdog/cmsdk-apb-watchdog.c
index 7ad46f9410..e4d25a25f7 100644
--- a/hw/watchdog/cmsdk-apb-watchdog.c
+++ b/hw/watchdog/cmsdk-apb-watchdog.c
@@ -12,8 +12,8 @@
 /*
  * This is a model of the "APB watchdog" which is part of the Cortex-M
  * System Design Kit (CMSDK) and documented in the Cortex-M System
- * Design Kit Technical Reference Manual (ARM DDI0479C):
- * https://developer.arm.com/products/system-design/system-design-kits/cortex-m-system-design-kit
+ * Design Kit Technical Reference Manual (ARM DDI0479):
+ * https://developer.arm.com/documentation/ddi0479/
  *
  * We also support the variant of this device found in the TI
  * Stellaris/Luminary boards and documented in:
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 1/5] hw/watchdog/cmsdk_apb_watchdog: Fix broken link

[Philippe Mathieu-Daudé]

On 15/11/24 16:03, Roque Arcudia Hernandez wrote:
> The patch changes the comments to point to the latest Design Kit
> Technical Reference Manual.
> 
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> ---
>   hw/watchdog/cmsdk-apb-watchdog.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/5] hw/watchdog/cmsdk_apb_watchdog: Fix INTEN issues

[Roque Arcudia Hernandez]

Current watchdog is free running out of reset, this combined with the
fact that current implementation also ensures the counter is running
when programing WDOGLOAD creates issues when the firmware defer the
programing of WDOGCONTROL.INTEN much later after WDOGLOAD. Arm
Programmer's Model documentation states that INTEN is also the
counter enable:

> INTEN
>
> Enable the interrupt event, WDOGINT. Set HIGH to enable the counter
> and the interrupt, or LOW to disable the counter and interrupt.
> Reloads the counter from the value in WDOGLOAD when the interrupt
> is enabled, after previously being disabled.

Source of the time of writing:

https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Reviewed-by: Stephen Longfield <slongfield@google.com>
Reviewed-by: Joe Komlodi <komlodi@google.com>
---
 hw/watchdog/cmsdk-apb-watchdog.c | 34 +++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/hw/watchdog/cmsdk-apb-watchdog.c b/hw/watchdog/cmsdk-apb-watchdog.c
index e4d25a25f7..ed5ff4257c 100644
--- a/hw/watchdog/cmsdk-apb-watchdog.c
+++ b/hw/watchdog/cmsdk-apb-watchdog.c
@@ -196,16 +196,13 @@ static void cmsdk_apb_watchdog_write(void *opaque, hwaddr offset,
 
     switch (offset) {
     case A_WDOGLOAD:
-        /*
-         * Reset the load value and the current count, and make sure
-         * we're counting.
-         */
+        /* Reset the load value and the current count. */
         ptimer_transaction_begin(s->timer);
         ptimer_set_limit(s->timer, value, 1);
-        ptimer_run(s->timer, 0);
         ptimer_transaction_commit(s->timer);
         break;
-    case A_WDOGCONTROL:
+    case A_WDOGCONTROL: {
+        uint32_t prev_control = s->control;
         if (s->is_luminary && 0 != (R_WDOGCONTROL_INTEN_MASK & s->control)) {
             /*
              * The Luminary version of this device ignores writes to
@@ -215,8 +212,25 @@ static void cmsdk_apb_watchdog_write(void *opaque, hwaddr offset,
             break;
         }
         s->control = value & R_WDOGCONTROL_VALID_MASK;
+        if (R_WDOGCONTROL_INTEN_MASK & (s->control ^ prev_control)) {
+            ptimer_transaction_begin(s->timer);
+            if (R_WDOGCONTROL_INTEN_MASK & s->control) {
+                /*
+                 * Set HIGH to enable the counter and the interrupt. Reloads
+                 * the counter from the value in WDOGLOAD when the interrupt
+                 * is enabled, after previously being disabled.
+                 */
+                ptimer_set_count(s->timer, ptimer_get_limit(s->timer));
+                ptimer_run(s->timer, 0);
+            } else {
+                /* Or LOW to disable the counter and interrupt. */
+                ptimer_stop(s->timer);
+            }
+            ptimer_transaction_commit(s->timer);
+        }
         cmsdk_apb_watchdog_update(s);
         break;
+    }
     case A_WDOGINTCLR:
         s->intstatus = 0;
         ptimer_transaction_begin(s->timer);
@@ -305,8 +319,14 @@ static void cmsdk_apb_watchdog_reset(DeviceState *dev)
     s->resetstatus = 0;
     /* Set the limit and the count */
     ptimer_transaction_begin(s->timer);
+    /*
+     * We need to stop the ptimer before setting its limit reset value. If the
+     * order is the opposite when the code executes the stop after setting a new
+     * limit it may want to recalculate the count based on the current time (if
+     * the timer was currently running) and it won't get the proper reset value.
+     */
+    ptimer_stop(s->timer);
     ptimer_set_limit(s->timer, 0xffffffff, 1);
-    ptimer_run(s->timer, 0);
     ptimer_transaction_commit(s->timer);
 }
 
-- 
2.47.0.338.g60cca15819-goog

[PATCH 3/5] tests/qtest/cmsdk-apb-watchdog-test: Parameterize tests

[Roque Arcudia Hernandez]

Currently the CMSDK APB watchdog tests target an specialized version
of the device (luminaris using the lm3s811evb machine) that prevents
the development of tests for the more generic device documented in:

https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model

This patch allows the execution of the watchdog tests in an MPS2
machine (when applicable) which uses the generic version of the CMSDK
APB watchdog.

Finally the rules for compiling the test have to change because it is
possible not to have CONFIG_STELLARIS (required for the lm3s811evb
machine) while still having CONFIG_CMSDK_APB_WATCHDOG and the test
will fail. Due to the addition of the MPS2 machine CONFIG_MPS2
becomes also a dependency for the test compilation.

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Reviewed-by: Stephen Longfield <slongfield@google.com>
---
 tests/qtest/cmsdk-apb-watchdog-test.c | 112 +++++++++++++++++++-------
 tests/qtest/meson.build               |   3 +-
 2 files changed, 84 insertions(+), 31 deletions(-)

diff --git a/tests/qtest/cmsdk-apb-watchdog-test.c b/tests/qtest/cmsdk-apb-watchdog-test.c
index 00b5dbbc81..bdf6cfa256 100644
--- a/tests/qtest/cmsdk-apb-watchdog-test.c
+++ b/tests/qtest/cmsdk-apb-watchdog-test.c
@@ -15,14 +15,12 @@
  */
 
 #include "qemu/osdep.h"
+#include "exec/hwaddr.h"
 #include "qemu/bitops.h"
 #include "libqtest-single.h"
 
-/*
- * lm3s811evb watchdog; at board startup this runs at 200MHz / 16 == 12.5MHz,
- * which is 80ns per tick.
- */
 #define WDOG_BASE 0x40000000
+#define WDOG_BASE_MPS2 0x40008000
 
 #define WDOGLOAD 0
 #define WDOGVALUE 4
@@ -37,39 +35,87 @@
 #define SYSDIV_SHIFT 23
 #define SYSDIV_LENGTH 4
 
-static void test_watchdog(void)
+#define WDOGLOAD_DEFAULT 0xFFFFFFFF
+#define WDOGVALUE_DEFAULT 0xFFFFFFFF
+
+typedef struct CMSDKAPBWatchdogTestArgs {
+    int64_t tick;
+    hwaddr wdog_base;
+    const char *machine;
+} CMSDKAPBWatchdogTestArgs;
+
+enum {
+    MACHINE_LM3S811EVB,
+    MACHINE_MPS2_AN385,
+};
+
+/*
+ * lm3s811evb watchdog; at board startup this runs at 200MHz / 16 == 12.5MHz,
+ * which is 80ns per tick.
+ *
+ * IoTKit/ARMSSE dualtimer; driven at 25MHz in mps2-an385, so 40ns per tick
+ */
+static const CMSDKAPBWatchdogTestArgs machine_info[] = {
+    [MACHINE_LM3S811EVB] = {
+        .tick = 80,
+        .wdog_base = WDOG_BASE,
+        .machine = "lm3s811evb",
+    },
+    [MACHINE_MPS2_AN385] = {
+        .tick = 40,
+        .wdog_base = WDOG_BASE_MPS2,
+        .machine = "mps2-an385",
+    },
+};
+
+static void test_watchdog(const void *ptr)
 {
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGRIS), ==, 0);
+    const CMSDKAPBWatchdogTestArgs *args = ptr;
+    hwaddr wdog_base = args->wdog_base;
+    int64_t tick = args->tick;
+    g_autofree gchar *cmdline = g_strdup_printf("-machine %s", args->machine);
+    qtest_start(cmdline);
 
-    writel(WDOG_BASE + WDOGCONTROL, 1);
-    writel(WDOG_BASE + WDOGLOAD, 1000);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    writel(wdog_base + WDOGCONTROL, 1);
+    writel(wdog_base + WDOGLOAD, 1000);
 
     /* Step to just past the 500th tick */
-    clock_step(500 * 80 + 1);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGRIS), ==, 0);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGVALUE), ==, 500);
+    clock_step(500 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 500);
 
     /* Just past the 1000th tick: timer should have fired */
-    clock_step(500 * 80);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGRIS), ==, 1);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGVALUE), ==, 0);
+    clock_step(500 * tick);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 0);
 
     /* VALUE reloads at following tick */
-    clock_step(80);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGVALUE), ==, 1000);
+    clock_step(tick);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 1000);
 
     /* Writing any value to WDOGINTCLR clears the interrupt and reloads */
-    clock_step(500 * 80);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGVALUE), ==, 500);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGRIS), ==, 1);
-    writel(WDOG_BASE + WDOGINTCLR, 0);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGVALUE), ==, 1000);
-    g_assert_cmpuint(readl(WDOG_BASE + WDOGRIS), ==, 0);
+    clock_step(500 * tick);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 500);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 1);
+    writel(wdog_base + WDOGINTCLR, 0);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 1000);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    qtest_end();
 }
 
-static void test_clock_change(void)
+/*
+ * This test can only be executed in the stellaris board since it relies on a
+ * component of the board to change the clocking parameters of the watchdog.
+ */
+static void test_clock_change(const void *ptr)
 {
     uint32_t rcc;
+    const CMSDKAPBWatchdogTestArgs *args = ptr;
+    g_autofree gchar *cmdline = g_strdup_printf("-machine %s", args->machine);
+    qtest_start(cmdline);
 
     /*
      * Test that writing to the stellaris board's RCC register to
@@ -109,6 +155,8 @@ static void test_clock_change(void)
     writel(WDOG_BASE + WDOGINTCLR, 0);
     g_assert_cmpuint(readl(WDOG_BASE + WDOGVALUE), ==, 1000);
     g_assert_cmpuint(readl(WDOG_BASE + WDOGRIS), ==, 0);
+
+    qtest_end();
 }
 
 int main(int argc, char **argv)
@@ -117,15 +165,19 @@ int main(int argc, char **argv)
 
     g_test_init(&argc, &argv, NULL);
 
-    qtest_start("-machine lm3s811evb");
-
-    qtest_add_func("/cmsdk-apb-watchdog/watchdog", test_watchdog);
-    qtest_add_func("/cmsdk-apb-watchdog/watchdog_clock_change",
-                   test_clock_change);
+    if (qtest_has_machine(machine_info[MACHINE_LM3S811EVB].machine)) {
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog",
+                            &machine_info[MACHINE_LM3S811EVB], test_watchdog);
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_clock_change",
+                            &machine_info[MACHINE_LM3S811EVB],
+                            test_clock_change);
+    }
+    if (qtest_has_machine(machine_info[MACHINE_MPS2_AN385].machine)) {
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_mps2",
+                            &machine_info[MACHINE_MPS2_AN385], test_watchdog);
+    }
 
     r = g_test_run();
 
-    qtest_end();
-
     return r;
 }
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index aa93e98418..f2f35367ae 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -227,7 +227,8 @@ qtests_arm = \
   (config_all_devices.has_key('CONFIG_MPS2') ? ['sse-timer-test'] : []) + \
   (config_all_devices.has_key('CONFIG_CMSDK_APB_DUALTIMER') ? ['cmsdk-apb-dualtimer-test'] : []) + \
   (config_all_devices.has_key('CONFIG_CMSDK_APB_TIMER') ? ['cmsdk-apb-timer-test'] : []) + \
-  (config_all_devices.has_key('CONFIG_CMSDK_APB_WATCHDOG') ? ['cmsdk-apb-watchdog-test'] : []) + \
+  (config_all_devices.has_key('CONFIG_STELLARIS') or
+   config_all_devices.has_key('CONFIG_MPS2') ? ['cmsdk-apb-watchdog-test'] : []) + \
   (config_all_devices.has_key('CONFIG_PFLASH_CFI02') and
    config_all_devices.has_key('CONFIG_MUSICPAL') ? ['pflash-cfi02-test'] : []) + \
   (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed : []) + \
-- 
2.47.0.338.g60cca15819-goog

[PATCH 4/5] tests/qtest/cmsdk-apb-watchdog-test: Don't abort on assertion failure

[Roque Arcudia Hernandez]

Currently the watchdog test has a behavior in which the first test
assertion that fails will make the test abort making it impossible to
see the result of other tests:

 # ERROR:../tests/qtest/cmsdk-apb-watchdog-test.c:87:test_watchdog:
 assertion failed ...
 Bail out!
 Aborted

Changing the behavior in order to let the test finish other tests and
report the ones that pass and fail:

 # ERROR:../tests/qtest/cmsdk-apb-watchdog-test.c:101:test_watchdog:
 assertion failed ...
 not ok 1 /arm/cmsdk-apb-watchdog/watchdog

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
---
 tests/qtest/cmsdk-apb-watchdog-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/cmsdk-apb-watchdog-test.c b/tests/qtest/cmsdk-apb-watchdog-test.c
index bdf6cfa256..fe535a553c 100644
--- a/tests/qtest/cmsdk-apb-watchdog-test.c
+++ b/tests/qtest/cmsdk-apb-watchdog-test.c
@@ -164,6 +164,7 @@ int main(int argc, char **argv)
     int r;
 
     g_test_init(&argc, &argv, NULL);
+    g_test_set_nonfatal_assertions();
 
     if (qtest_has_machine(machine_info[MACHINE_LM3S811EVB].machine)) {
         qtest_add_data_func("/cmsdk-apb-watchdog/watchdog",
-- 
2.47.0.338.g60cca15819-goog

[PATCH 5/5] tests/qtest/cmsdk-apb-watchdog-test: Test INTEN as counter enable

[Roque Arcudia Hernandez]

The following tests focus on making sure the counter is not running
out of reset and the proper use of INTEN as the counter enable. As
described in:

https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model

The new tests have to target an MPS2 machine because the original
machine used by the test (stellaris) has a variation of the
cmsdk_apb_watchdog that locks INTEN when it is programmed to 1. The
stellaris machine also does not reproduce the problem of the counter
running out of cold reset due to the way the clocks are initialized.

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Reviewed-by: Stephen Longfield <slongfield@google.com>
---
 tests/qtest/cmsdk-apb-watchdog-test.c | 215 ++++++++++++++++++++++++++
 1 file changed, 215 insertions(+)

diff --git a/tests/qtest/cmsdk-apb-watchdog-test.c b/tests/qtest/cmsdk-apb-watchdog-test.c
index fe535a553c..53538f98c9 100644
--- a/tests/qtest/cmsdk-apb-watchdog-test.c
+++ b/tests/qtest/cmsdk-apb-watchdog-test.c
@@ -68,6 +68,16 @@ static const CMSDKAPBWatchdogTestArgs machine_info[] = {
     },
 };
 
+static void system_reset(QTestState *qtest)
+{
+    QDict *resp;
+
+    resp = qtest_qmp(qtest, "{'execute': 'system_reset'}");
+    g_assert(qdict_haskey(resp, "return"));
+    qobject_unref(resp);
+    qtest_qmp_eventwait(qtest, "RESET");
+}
+
 static void test_watchdog(const void *ptr)
 {
     const CMSDKAPBWatchdogTestArgs *args = ptr;
@@ -159,6 +169,199 @@ static void test_clock_change(const void *ptr)
     qtest_end();
 }
 
+/* Tests the counter is not running after reset. */
+static void test_watchdog_reset(const void *ptr)
+{
+    const CMSDKAPBWatchdogTestArgs *args = ptr;
+    hwaddr wdog_base = args->wdog_base;
+    int64_t tick = args->tick;
+    g_autofree gchar *cmdline = g_strdup_printf("-machine %s", args->machine);
+    qtest_start(cmdline);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    g_assert_cmphex(readl(wdog_base + WDOGCONTROL), ==, 0);
+
+    /*
+     * The counter should not be running if WDOGCONTROL.INTEN has not been set,
+     * as it is the case after a cold reset.
+     */
+    clock_step(15 * tick + 1);
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    /* Let the counter run before reset */
+    writel(wdog_base + WDOGLOAD, 3000);
+    writel(wdog_base + WDOGCONTROL, 1);
+
+    /* Verify it is running */
+    clock_step(1000 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 3000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 2000);
+
+    system_reset(global_qtest);
+
+    /* Check defaults after reset */
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    /* The counter should not be running after reset. */
+    clock_step(1000 * tick + 1);
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    qtest_end();
+}
+
+/*
+ * Tests inten works as the counter enable based on this description:
+ *
+ * Enable the interrupt event, WDOGINT. Set HIGH to enable the counter and the
+ * interrupt, or LOW to disable the counter and interrupt. Reloads the counter
+ * from the value in WDOGLOAD when the interrupt is enabled, after previously
+ * being disabled.
+ */
+static void test_watchdog_inten(const void *ptr)
+{
+    const CMSDKAPBWatchdogTestArgs *args = ptr;
+    hwaddr wdog_base = args->wdog_base;
+    int64_t tick = args->tick;
+    g_autofree gchar *cmdline = g_strdup_printf("-machine %s", args->machine);
+    qtest_start(cmdline);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    /*
+     * When WDOGLOAD is written to, the count is immediately restarted from the
+     * new value.
+     *
+     * Note: the counter should not be running as long as WDOGCONTROL.INTEN is
+     * not set
+     */
+    writel(wdog_base + WDOGLOAD, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 4000);
+    clock_step(500 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 4000);
+
+    /* Set HIGH WDOGCONTROL.INTEN to enable the counter and the interrupt */
+    writel(wdog_base + WDOGCONTROL, 1);
+    clock_step(500 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 3500);
+
+    /* or LOW to disable the counter and interrupt. */
+    writel(wdog_base + WDOGCONTROL, 0);
+    clock_step(100 * tick);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 3500);
+
+    /*
+     * Reloads the counter from the value in WDOGLOAD when the interrupt is
+     * enabled, after previously being disabled.
+     */
+    writel(wdog_base + WDOGCONTROL, 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 4000);
+
+    /* Test counter is still on */
+    clock_step(50 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 3950);
+
+    /*
+     * When WDOGLOAD is written to, the count is immediately restarted from the
+     * new value.
+     *
+     * Note: the counter should be running since WDOGCONTROL.INTEN is set
+     */
+    writel(wdog_base + WDOGLOAD, 5000);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 5000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 5000);
+    clock_step(4999 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 5000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    /* Finally disable and check the conditions don't change */
+    writel(wdog_base + WDOGCONTROL, 0);
+    clock_step(10 * tick);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 5000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    qtest_end();
+}
+
+/*
+ * Tests the following custom behavior:
+ *
+ * The Luminary version of this device ignores writes to this register after the
+ * guest has enabled interrupts (so they can only be disabled again via reset).
+ */
+static void test_watchdog_inten_luminary(const void *ptr)
+{
+    const CMSDKAPBWatchdogTestArgs *args = ptr;
+    hwaddr wdog_base = args->wdog_base;
+    int64_t tick = args->tick;
+    g_autofree gchar *cmdline = g_strdup_printf("-machine %s", args->machine);
+    qtest_start(cmdline);
+    g_assert_cmpuint(readl(wdog_base + WDOGRIS), ==, 0);
+
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    /*
+     * When WDOGLOAD is written to, the count is immediately restarted from the
+     * new value.
+     *
+     * Note: the counter should not be running as long as WDOGCONTROL.INTEN is
+     * not set
+     */
+    writel(wdog_base + WDOGLOAD, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 4000);
+    clock_step(500 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 4000);
+
+    /* Set HIGH WDOGCONTROL.INTEN to enable the counter and the interrupt */
+    writel(wdog_base + WDOGCONTROL, 1);
+    clock_step(500 * tick + 1);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 3500);
+
+    /*
+     * The Luminary version of this device ignores writes to this register after
+     * the guest has enabled interrupts
+     */
+    writel(wdog_base + WDOGCONTROL, 0);
+    clock_step(100 * tick);
+    g_assert_cmpuint(readl(wdog_base + WDOGLOAD), ==, 4000);
+    g_assert_cmpuint(readl(wdog_base + WDOGVALUE), ==, 3400);
+    g_assert_cmphex(readl(wdog_base + WDOGCONTROL), ==, 0x1);
+
+    /* They can only be disabled again via reset */
+    system_reset(global_qtest);
+
+    /* Check defaults after reset */
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGCONTROL), ==, 0);
+
+    /* The counter should not be running after reset. */
+    clock_step(1000 * tick + 1);
+    g_assert_cmphex(readl(wdog_base + WDOGLOAD), ==, WDOGLOAD_DEFAULT);
+    g_assert_cmphex(readl(wdog_base + WDOGVALUE), ==, WDOGVALUE_DEFAULT);
+
+    qtest_end();
+}
+
 int main(int argc, char **argv)
 {
     int r;
@@ -172,10 +375,22 @@ int main(int argc, char **argv)
         qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_clock_change",
                             &machine_info[MACHINE_LM3S811EVB],
                             test_clock_change);
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_reset",
+                            &machine_info[MACHINE_LM3S811EVB],
+                            test_watchdog_reset);
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_inten_luminary",
+                            &machine_info[MACHINE_LM3S811EVB],
+                            test_watchdog_inten_luminary);
     }
     if (qtest_has_machine(machine_info[MACHINE_MPS2_AN385].machine)) {
         qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_mps2",
                             &machine_info[MACHINE_MPS2_AN385], test_watchdog);
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_reset_mps2",
+                            &machine_info[MACHINE_MPS2_AN385],
+                            test_watchdog_reset);
+        qtest_add_data_func("/cmsdk-apb-watchdog/watchdog_inten",
+                            &machine_info[MACHINE_MPS2_AN385],
+                            test_watchdog_inten);
     }
 
     r = g_test_run();
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 0/5] Make WDOGCONTROL.INTEN the counter enable of the CMSDK APB Watchdog

[Roque Arcudia Hernandez]

Hello,

I forgot to add the -v 2 to this patch series. This is the second
version after Peter's feedback. Shall I resend it again or is it fine
like this?

Thanks

Roque

On Fri, Nov 15, 2024 at 8:03 AM Roque Arcudia Hernandez
<roqueh@google.com> wrote:
>
> The following patchset tries to address an issue where the watchdog
> counter was running as soon as the device is out of reset. This
> created a few problems with the firmware under test. It was pointed
> out that the firmware under test was already working on an emulator
> using the real RTL. Further review showed that the WDOGCONTROL.INTEN
> is also expected to be the counter enable as it is defined in this
> page from ARM:
>
> https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model?lang=en
>
>      |       | Enable the interrupt event, WDOGINT. Set to 1 to
>      |       | enable the counter and the interrupt, or to 0 to
>  [0] | INTEN | disable the counter and interrupt. Reloads the
>      |       | counter from the value in WDOGLOAD when the interrupt
>      |       | is enabled, after previously being disabled.
>
> The second half of the patch is focused on adding tests for this
> behavior. It was required to modify the test to be able to run some
> test cases in the MPS2 since the the stellaris machine had 2 issues:
>
>   - The stellaris machine did not run the counter out of reset
>     because of the way it is reset. The function
>     ssys_calculate_system_clock is not called until the reset phase
>     stellaris_sys_reset_hold which is after the watchdog executes its
>     reset, so the watchdog would not start the counter due to the
>     clock not being set up at the time of reset.
>
>   - The Stellaris machine uses a derived watchdog and not the
>     original CMSDK one which has a special behavior where it cannot
>     be stopped.
>
> The MPS2 machine on the other hand uses the standard CMSDK watchdog
> and could expose the problems we were dealing with.
>
> Roque Arcudia Hernandez (5):
>   hw/watchdog/cmsdk_apb_watchdog: Fix broken link
>   hw/watchdog/cmsdk_apb_watchdog: Fix INTEN issues
>   tests/qtest/cmsdk-apb-watchdog-test: Parameterize tests
>   tests/qtest/cmsdk-apb-watchdog-test: Don't abort on assertion failure
>   tests/qtest/cmsdk-apb-watchdog-test: Test INTEN as counter enable
>
>  hw/watchdog/cmsdk-apb-watchdog.c      |  38 ++-
>  tests/qtest/cmsdk-apb-watchdog-test.c | 328 +++++++++++++++++++++++---
>  tests/qtest/meson.build               |   3 +-
>  3 files changed, 329 insertions(+), 40 deletions(-)
>
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH 0/5] Make WDOGCONTROL.INTEN the counter enable of the CMSDK APB Watchdog

[Peter Maydell]

On Fri, 15 Nov 2024 at 17:21, Roque Arcudia Hernandez <roqueh@google.com> wrote:
>
> Hello,
>
> I forgot to add the -v 2 to this patch series. This is the second
> version after Peter's feedback. Shall I resend it again or is it fine
> like this?

It's fine like this, no need to resend just to add the v2 tag.

-- PMM

Re: [PATCH 0/5] Make WDOGCONTROL.INTEN the counter enable of the CMSDK APB Watchdog

[Peter Maydell]

On Fri, 15 Nov 2024 at 16:03, Roque Arcudia Hernandez <roqueh@google.com> wrote:
>
> The following patchset tries to address an issue where the watchdog
> counter was running as soon as the device is out of reset. This
> created a few problems with the firmware under test. It was pointed
> out that the firmware under test was already working on an emulator
> using the real RTL. Further review showed that the WDOGCONTROL.INTEN
> is also expected to be the counter enable as it is defined in this
> page from ARM:
>
> https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model?lang=en
>
>      |       | Enable the interrupt event, WDOGINT. Set to 1 to
>      |       | enable the counter and the interrupt, or to 0 to
>  [0] | INTEN | disable the counter and interrupt. Reloads the
>      |       | counter from the value in WDOGLOAD when the interrupt
>      |       | is enabled, after previously being disabled.
>
> The second half of the patch is focused on adding tests for this
> behavior. It was required to modify the test to be able to run some
> test cases in the MPS2 since the the stellaris machine had 2 issues:
>
>   - The stellaris machine did not run the counter out of reset
>     because of the way it is reset. The function
>     ssys_calculate_system_clock is not called until the reset phase
>     stellaris_sys_reset_hold which is after the watchdog executes its
>     reset, so the watchdog would not start the counter due to the
>     clock not being set up at the time of reset.
>
>   - The Stellaris machine uses a derived watchdog and not the
>     original CMSDK one which has a special behavior where it cannot
>     be stopped.
>
> The MPS2 machine on the other hand uses the standard CMSDK watchdog
> and could expose the problems we were dealing with.
>
> Roque Arcudia Hernandez (5):
>   hw/watchdog/cmsdk_apb_watchdog: Fix broken link
>   hw/watchdog/cmsdk_apb_watchdog: Fix INTEN issues
>   tests/qtest/cmsdk-apb-watchdog-test: Parameterize tests
>   tests/qtest/cmsdk-apb-watchdog-test: Don't abort on assertion failure
>   tests/qtest/cmsdk-apb-watchdog-test: Test INTEN as counter enable

Applied to target-arm.next for 9.2, thanks.

-- PMM