| Message ID | 20201008232154.94221-1-hskinnemoen@google.com |
|---|---|
| Headers | show |
| Series | Additional NPCM7xx features, devices and tests | expand |
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
On Fri, 9 Oct 2020 at 00:22, Havard Skinnemoen <hskinnemoen@google.com> wrote: > > This is an update to the initial NPCM7xx patch series adding > > - A timer test that found several issues that were fixed in the final version > of the series (see > https://www.mail-archive.com/qemu-devel@nongnu.org/msg739516.html). > - Watchdog timer support. This makes the reboot command work. > - Random Number Generator device. > - USB Host Controllers. > - GPIO Controllers. > > The watchdog was implemented by my new teammate Hao Wu. Expect to see more > patches from him in the near future. > > This series has also been pushed to the npcm7xx-5.2-update branch of my github > repository at > > https://github.com/hskinnemoen/qemu > > Again, thanks a lot for reviewing! I'm going to take patch 1 (the timer test case) into my target-arm.next queue; I've sent some review comments on the other parts of the series. thanks -- PMM
This is an update to the initial NPCM7xx patch series adding - A timer test that found several issues that were fixed in the final version of the series (see https://www.mail-archive.com/qemu-devel@nongnu.org/msg739516.html). - Watchdog timer support. This makes the reboot command work. - Random Number Generator device. - USB Host Controllers. - GPIO Controllers. The watchdog was implemented by my new teammate Hao Wu. Expect to see more patches from him in the near future. This series has also been pushed to the npcm7xx-5.2-update branch of my github repository at https://github.com/hskinnemoen/qemu Again, thanks a lot for reviewing! Havard Hao Wu (1): hw/timer: Adding watchdog for NPCM7XX Timer. Havard Skinnemoen (5): tests/qtest: Add npcm7xx timer test Move npcm7xx_timer_reached_zero call out of npcm7xx_timer_pause hw/misc: Add npcm7xx random number generator hw/arm/npcm7xx: Add EHCI and OHCI controllers hw/gpio: Add GPIO model for Nuvoton NPCM7xx docs/system/arm/nuvoton.rst | 6 +- hw/usb/hcd-ehci.h | 1 + include/hw/arm/npcm7xx.h | 8 + include/hw/gpio/npcm7xx_gpio.h | 55 +++ include/hw/misc/npcm7xx_clk.h | 9 + include/hw/misc/npcm7xx_rng.h | 34 ++ include/hw/timer/npcm7xx_timer.h | 43 +- hw/arm/npcm7xx.c | 125 ++++- hw/gpio/npcm7xx_gpio.c | 409 ++++++++++++++++ hw/misc/npcm7xx_clk.c | 20 + hw/misc/npcm7xx_rng.c | 179 +++++++ hw/timer/npcm7xx_timer.c | 279 +++++++++-- hw/usb/hcd-ehci-sysbus.c | 19 + tests/qtest/npcm7xx_gpio-test.c | 385 +++++++++++++++ tests/qtest/npcm7xx_rng-test.c | 278 +++++++++++ tests/qtest/npcm7xx_timer-test.c | 562 ++++++++++++++++++++++ tests/qtest/npcm7xx_watchdog_timer-test.c | 313 ++++++++++++ MAINTAINERS | 1 + hw/gpio/meson.build | 1 + hw/gpio/trace-events | 7 + hw/misc/meson.build | 1 + hw/misc/trace-events | 4 + tests/qtest/meson.build | 4 + 23 files changed, 2682 insertions(+), 61 deletions(-) create mode 100644 include/hw/gpio/npcm7xx_gpio.h create mode 100644 include/hw/misc/npcm7xx_rng.h create mode 100644 hw/gpio/npcm7xx_gpio.c create mode 100644 hw/misc/npcm7xx_rng.c create mode 100644 tests/qtest/npcm7xx_gpio-test.c create mode 100644 tests/qtest/npcm7xx_rng-test.c create mode 100644 tests/qtest/npcm7xx_timer-test.c create mode 100644 tests/qtest/npcm7xx_watchdog_timer-test.c