[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/6] hw/timer: Adding watchdog for NPCM7XX Timer.
From: |
Peter Maydell |
Subject: |
Re: [PATCH 3/6] hw/timer: Adding watchdog for NPCM7XX Timer. |
Date: |
Tue, 20 Oct 2020 13:55:17 +0100 |
On Fri, 9 Oct 2020 at 00:22, Havard Skinnemoen <hskinnemoen@google.com> 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>
> ---
> include/hw/misc/npcm7xx_clk.h | 9 +
> include/hw/timer/npcm7xx_timer.h | 43 ++-
> hw/arm/npcm7xx.c | 11 +
> hw/misc/npcm7xx_clk.c | 20 ++
> hw/timer/npcm7xx_timer.c | 275 +++++++++++++++----
> tests/qtest/npcm7xx_watchdog_timer-test.c | 313 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> tests/qtest/meson.build | 1 +
> 8 files changed, 620 insertions(+), 53 deletions(-)
> create mode 100644 tests/qtest/npcm7xx_watchdog_timer-test.c
>
> diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h
> index cdcc9e8534..483028cf63 100644
> --- a/include/hw/misc/npcm7xx_clk.h
> +++ b/include/hw/misc/npcm7xx_clk.h
> @@ -42,6 +42,15 @@ typedef struct NPCM7xxCLKState {
> int64_t ref_ns;
> } NPCM7xxCLKState;
>
> +/**
> + * npcm7xx_clk_perform_watchdog_reset - Perform watchdog reset action
> generated
> + * by a watchdog event.
> + * @clk: The clock module that connects to the watchdog.
> + * @watchdog_index: The index of the watchdog that triggered the reset
> action.
> + */
> +void npcm7xx_clk_perform_watchdog_reset(NPCM7xxCLKState *clk,
> + int watchdog_index);
It looks like you're using this as a mechanism for having the
watchdog module signal to the clock module that it needs to
do a reset. The usual way I'd implement a cross-device
link like that would be to use a qemu_irq for it -- the
receiving end creates a named GPIO input, the sending end
has a named GPIO output, and the SoC that creates both devices
also wires the GPIO link from one to the other. (You can
send an arbitrary integer down a qemu_irq, not just an
on/off, so you could do this either with an array of N
GPIOs, one per watchdog, or with just one that you send
an index value down.)
I have a handful of nits below but other than the above issue
this patch looks good.
> +static void npcm7xx_watchdog_timer_reset_cycles(NPCM7xxWatchdogTimer *t,
> + int64_t cycles)
> +{
> + uint32_t prescaler = npcm7xx_watchdog_timer_prescaler(t);
> + int64_t ns = (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ) * cycles;
> +
> + /*
> + * The reset function always clear the current timer. The caller to the
> + * this needs to decide whether to start the watchdog timer based on
"always clears". "The caller of this".
> + * specific flag in WTCR.
> + */
> + npcm7xx_timer_clear(&t->base_timer);
> +
> + ns *= prescaler;
> + t->base_timer.remaining_ns = ns;
> +}
> @@ -259,9 +333,47 @@ static void
> npcm7xx_timer_write_tisr(NPCM7xxTimerCtrlState *s, uint32_t value)
> if (value & (1U << i)) {
> npcm7xx_timer_check_interrupt(&s->timer[i]);
> }
> +
Stray extra blank line?
> }
> }
>
> + /*
> + * Set WTCLK to 1(default) and resets all flags except WTRF.
> + * WTRF is not reset during a core domain reset.
> + */
"and reset all flags" (or "Sets", if you prefer)
+ s->watchdog_timer.wtcr = 0x00000400 | (s->watchdog_timer.wtcr &
> + NPCM7XX_WTCR_WTRF);
> +}
> +
> +static void npcm7xx_watchdog_timer_expired(void *opaque)
> +{
> + NPCM7xxWatchdogTimer *t = opaque;
> +
> + if (t->wtcr & NPCM7XX_WTCR_WTE) {
> + if (t->wtcr & NPCM7XX_WTCR_WTIF) {
> + if (t->wtcr & NPCM7XX_WTCR_WTRE) {
> + t->wtcr |= NPCM7XX_WTCR_WTRF;
> + /* send reset signal to CLK module*/
> + npcm7xx_clk_perform_watchdog_reset(t->ctrl->clk,
> + t->ctrl->index);
> + }
> + } else {
> + t->wtcr |= NPCM7XX_WTCR_WTIF;
> + if (t->wtcr & NPCM7XX_WTCR_WTIE)
> + /* send interrupt */
> + qemu_irq_raise(t->irq);
This if() is missing its mandatory braces. (Not sure why
checkpatch doesn't catch this.)
> + npcm7xx_watchdog_timer_reset_cycles(t,
> + NPCM7XX_WATCHDOG_INTERRUPT_TO_RESET_CYCLES);
> + npcm7xx_timer_start(&t->base_timer);
> + }
> + }
> }
thanks
-- PMM
- [PATCH 0/6] Additional NPCM7xx features, devices and tests, Havard Skinnemoen, 2020/10/08
- [PATCH 1/6] tests/qtest: Add npcm7xx timer test, Havard Skinnemoen, 2020/10/08
- [PATCH 2/6] Move npcm7xx_timer_reached_zero call out of npcm7xx_timer_pause, Havard Skinnemoen, 2020/10/08
- [PATCH 4/6] hw/misc: Add npcm7xx random number generator, Havard Skinnemoen, 2020/10/08
- [PATCH 5/6] hw/arm/npcm7xx: Add EHCI and OHCI controllers, Havard Skinnemoen, 2020/10/08
- [PATCH 3/6] hw/timer: Adding watchdog for NPCM7XX Timer., Havard Skinnemoen, 2020/10/08
- Re: [PATCH 3/6] hw/timer: Adding watchdog for NPCM7XX Timer.,
Peter Maydell <=
- [PATCH 6/6] hw/gpio: Add GPIO model for Nuvoton NPCM7xx, Havard Skinnemoen, 2020/10/08
- Re: [PATCH 0/6] Additional NPCM7xx features, devices and tests, Peter Maydell, 2020/10/20