Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock

From: Havard Skinnemoen <hskinnemoen@google.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Hao Wu" <wuhaotsh@google.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Tyrone Ting" <kfting@nuvoton.com>
Subject: Re: [PULL 16/21] hw/timer: Refactor NPCM7XX Timer to use CLK clock
Date: Tue, 27 Jul 2021 11:07:09 -0700	[thread overview]
Message-ID: <CAFQmdRYqb9GPi=Ng8LtJAHR8rbJxaCP9yCxyugkwU6MEebh0jg@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA_wrBcdeNVJVOfCYR2E-zoUsKFnGocF18xvN=S5d=iY0g@mail.gmail.com>

On Tue, Jul 27, 2021 at 7:19 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 4 Feb 2021 at 22:38, Hao Wu <wuhaotsh@google.com> wrote:
> >
> > I don't see this error. It could be some error in the clock that the timer module does not get a correct clock input.
> > How do you reproduce this?
> >
> > On Thu, Feb 4, 2021 at 1:39 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Jan 12, 2021 at 6:20 PM Peter Maydell <peter.maydell@linaro.org>
> >> wrote:
> >> >
> >> > From: Hao Wu <wuhaotsh@google.com>
> >> >
> >> > This patch makes NPCM7XX Timer to use a the timer clock generated by the
> >> > CLK module instead of the magic number TIMER_REF_HZ.
> >> >
> >> > Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> >> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> >> > Signed-off-by: Hao Wu <wuhaotsh@google.com>
> >> > Message-id: 20210108190945.949196-3-wuhaotsh@google.com
> >> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> > ---
> >> >  include/hw/misc/npcm7xx_clk.h    |  6 -----
> >> >  include/hw/timer/npcm7xx_timer.h |  1 +
> >> >  hw/arm/npcm7xx.c                 |  5 ++++
> >> >  hw/timer/npcm7xx_timer.c         | 39 +++++++++++++++-----------------
> >> >  4 files changed, 24 insertions(+), 27 deletions(-)
> >>
> >> Is that a spurious error (building with Clang)?
> >>
> >> Running test qtest-arm/npcm7xx_timer-test
> >> ERROR:../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
> >> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)): (0x00000000
> >> == 0x00000004)
> >> ERROR:../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
> >> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
> >> FAIL 155 qtest-arm/npcm7xx_timer-test
> >> /arm/npcm7xx_timer/tim[2]/timer[2]/periodic_interrupt
> >> make: *** [Makefile.mtest:1033: run-test-127] Error 1
>
> This intermittent is still with us:
>
> /arm/npcm7xx_timer/tim[2]/timer[0]/periodic_interrupt: **
> ERROR:../../tests/qtest/npcm7xx_timer-test.c:475:test_periodic_interrupt:
> assertion failed (tim_read(td, TISR) == tim_timer_bit(td)):
> (0x00000000 == 0x00000001)
> **
> ERROR:../../tests/qtest/npcm7xx_timer-test.c:476:test_periodic_interrupt:
> 'qtest_get_irq(global_qtest, tim_timer_irq(td))' should be TRUE
> FAIL

It looks like the interrupt fires consistently with the status bit. It
also only happens once during that loop of four timer periods (the
test calls g_test_set_nonfatal_assertions() so we would have seen more
failures reported if this wasn't the case).

I'm wondering if the clock_step_next() call is occasionally hitting a
different timer, causing it to return before it has advanced to the
next period of the timer module? If so, it might help to use
clock_step(tim_calculate_step(...)) like some of the other tests do.
This would also verify that the timer is actually firing at the right
interval.

Havard