Re: [PATCH v2 2/5] hw/timer: Adding watchdog for NPCM7XX Timer.

[Thomas Huth]

On 21/10/2020 23.35, Havard Skinnemoen via wrote:
> From: Hao Wu <wuhaotsh@google.com>
> 
> The watchdog is part of NPCM7XX's timer module. Its behavior is
> controlled by the WTCR register in the timer.
> 
> When enabled, the watchdog issues an interrupt signal after a pre-set
> amount of cycles, and issues a reset signal shortly after that.
> 
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
[...]
> diff --git a/tests/qtest/npcm7xx_watchdog_timer-test.c 
> b/tests/qtest/npcm7xx_watchdog_timer-test.c
> new file mode 100644
> index 0000000000..bfe0020ffc
> --- /dev/null
> +++ b/tests/qtest/npcm7xx_watchdog_timer-test.c
> @@ -0,0 +1,313 @@
> +/*
> + * QTests for Nuvoton NPCM7xx Timer Watchdog Modules.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/timer.h"
> +
> +#include "libqtest-single.h"
[...]
> +/*
> + * Check a watchdog doesn't fire if corresponding flags (WTIE and WTRE) are 
> not
> + * set.
> + */
> +static void test_enabling_flags(gconstpointer watchdog)
> +{
> +    const Watchdog *wd = watchdog;
> +    QTestState *qts;
> +
> +    /* Neither WTIE or WTRE is set, no interrupt or reset should happen */
> +    qts = qtest_start("-machine quanta-gsj");
> +    qtest_irq_intercept_in(qts, "/machine/soc/a9mpcore/gic");
> +    watchdog_write_wtcr(wd, WTCLK(0) | WTE | WTIF | WTRF | WTR);
> +    clock_step(watchdog_interrupt_steps(wd));
> +    g_assert_true(watchdog_read_wtcr(wd) & WTIF);
> +    g_assert_false(qtest_get_irq(qts, wd->irq));
> +    clock_step(watchdog_calculate_steps(RESET_CYCLES, 
> watchdog_prescaler(wd)));
> +    g_assert_true(watchdog_read_wtcr(wd) & WTIF);
> +    g_assert_false(watchdog_read_wtcr(wd) & WTRF);
> +    qtest_end();

You are mixing functions from libqtest.h and libqtest-single.h ... e.g. you
use qtest_get_irq() from libqtest.h instead of get_irq() from
libqtest-single.h, on the other hand, you use clock_step() from
libqtest-single.h instead of qtest_clock_step() from libqtest.h ... there is
no real problem with this, but the style looks a little bit strange.

For new tests, we prefer nowadays to avoid libqtest-single.h since it causes
trouble if you have to track multiple test states (e.g. when writing tests
that include migration tests). Could you maybe try to rewrite your code
without libqtest-single.h? I.e. use qtest_clock_step() instead of
clock_step() etc. ?

> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 28d4068718..f7b0e3ca9d 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -139,6 +139,7 @@ qtests_arm = \
>     'microbit-test',
>     'm25p80-test',
>     'npcm7xx_timer-test',
> +   'npcm7xx_watchdog_timer-test',

Could you please fence this test with CONFIG_NPCM7XX ? ... since it won't
work if the quanta-gsj machine has not been compiled into the binary.

 Thanks,
  Thomas