Re: [PATCH 3/6] hw/timer: Adding watchdog for NPCM7XX Timer.

[Peter Maydell]

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