Re: [RFC PATCH] tests/qtest: don't step clock at start of npcm7xx periodic IRQ test

Re: [RFC PATCH] tests/qtest: don't step clock at start of npcm7xx periodic IRQ test

[Hao Wu]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

Have you tried that the test can pass with this? If I remember correctly,
interrupt won't trigger properly if not advancing the timer

If the test passes it's probably fine to remove that.

On Mon, Jan 20, 2025 at 11:00 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> Until there are timers enabled the semantics of clock_step_next() will
> fail. Since d524441a36 (system/qtest: properly feedback results of
> clock_[step|set]) we will signal a FAIL if time doesn't advance.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/qtest/npcm7xx_timer-test.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tests/qtest/npcm7xx_timer-test.c
> b/tests/qtest/npcm7xx_timer-test.c
> index 58f58c2f71..43711049ca 100644
> --- a/tests/qtest/npcm7xx_timer-test.c
> +++ b/tests/qtest/npcm7xx_timer-test.c
> @@ -465,7 +465,6 @@ static void test_periodic_interrupt(gconstpointer
> test_data)
>      int i;
>
>      tim_reset(td);
> -    clock_step_next();
>
>      tim_write_ticr(td, count);
>      tim_write_tcsr(td, CEN | IE | MODE_PERIODIC | PRESCALE(ps));
> --
> 2.39.5
>
>

[-- Attachment #2: Type: text/html, Size: 1590 bytes --]

Re: [RFC PATCH] tests/qtest: don't step clock at start of npcm7xx periodic IRQ test

[Peter Maydell]

On Mon, 20 Jan 2025 at 23:52, Hao Wu <wuhaotsh@google.com> wrote:
>
> Have you tried that the test can pass with this? If I remember correctly, interrupt won't trigger properly if not advancing the timer
>
> If the test passes it's probably fine to remove that.

This specific clock_step_next() call is done immediately after
resetting the timer device, so there *is* no pending interrupt.
(That's why it's a problem -- it says "step the clock forward
to the next pending timer expiry", and there is no next
pending timer expiry.)

There's another clock_step_next() just after the context
in this patch, and that one is fine, because it's after
the test enables the timer and so there will be a
next pending expiry to step forward to.

thanks
-- PMM

Re: [RFC PATCH] tests/qtest: don't step clock at start of npcm7xx periodic IRQ test

[Alex Bennée]

Hao Wu <wuhaotsh@google.com> writes:

> Have you tried that the test can pass with this? If I remember correctly, interrupt won't trigger properly if not advancing the
> timer

Yes but the IRQ has yet to be enabled at this point.

>
> If the test passes it's probably fine to remove that.

Of course.

>
> On Mon, Jan 20, 2025 at 11:00 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  Until there are timers enabled the semantics of clock_step_next() will
>  fail. Since d524441a36 (system/qtest: properly feedback results of
>  clock_[step|set]) we will signal a FAIL if time doesn't advance.
>
>  Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>  ---
>   tests/qtest/npcm7xx_timer-test.c | 1 -
>   1 file changed, 1 deletion(-)
>
>  diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c
>  index 58f58c2f71..43711049ca 100644
>  --- a/tests/qtest/npcm7xx_timer-test.c
>  +++ b/tests/qtest/npcm7xx_timer-test.c
>  @@ -465,7 +465,6 @@ static void test_periodic_interrupt(gconstpointer test_data)
>       int i;
>
>       tim_reset(td);
>  -    clock_step_next();
>
>       tim_write_ticr(td, count);
>       tim_write_tcsr(td, CEN | IE | MODE_PERIODIC | PRESCALE(ps));
>  -- 
>  2.39.5

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] tests/qtest: don't step clock at start of npcm7xx periodic IRQ test

[Hao Wu]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

On Tue, Jan 21, 2025 at 6:20 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> Hao Wu <wuhaotsh@google.com> writes:
>
> > Have you tried that the test can pass with this? If I remember
> correctly, interrupt won't trigger properly if not advancing the
> > timer
>
> Yes but the IRQ has yet to be enabled at this point.
>
I believe that's the case,  so we should be able to remove this line in the
test.

>
> >
> > If the test passes it's probably fine to remove that.
>
> Of course.
>
> >
> > On Mon, Jan 20, 2025 at 11:00 PM Alex Bennée <alex.bennee@linaro.org>
> wrote:
> >
> >  Until there are timers enabled the semantics of clock_step_next() will
> >  fail. Since d524441a36 (system/qtest: properly feedback results of
> >  clock_[step|set]) we will signal a FAIL if time doesn't advance.
> >
> >  Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
Reviewed-by: Hao Wu <wuhaotsh@google.com>

> >  ---
> >   tests/qtest/npcm7xx_timer-test.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> >  diff --git a/tests/qtest/npcm7xx_timer-test.c
> b/tests/qtest/npcm7xx_timer-test.c
> >  index 58f58c2f71..43711049ca 100644
> >  --- a/tests/qtest/npcm7xx_timer-test.c
> >  +++ b/tests/qtest/npcm7xx_timer-test.c
> >  @@ -465,7 +465,6 @@ static void test_periodic_interrupt(gconstpointer
> test_data)
> >       int i;
> >
> >       tim_reset(td);
> >  -    clock_step_next();
> >
> >       tim_write_ticr(td, count);
> >       tim_write_tcsr(td, CEN | IE | MODE_PERIODIC | PRESCALE(ps));
> >  --
> >  2.39.5
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

[-- Attachment #2: Type: text/html, Size: 2696 bytes --]