[PATCH] hw/timer/armv7m_systick: Update clock source before enabling timer

[PATCH] hw/timer/armv7m_systick: Update clock source before enabling timer

[Richard Petri]

Starting the SysTick timer and changing the clock source a the same time
will result in an error, if the previous clock period was zero. For exmaple,
on the mps2-tz platforms, no refclk is present. Right after reset, the
configured ptimer period is zero, and trying to enabling it will turn it off
right away. E.g., code running on the platform setting

    SysTick->CTRL  = SysTick_CTRL_CLKSOURCE_Msk | SysTick_CTRL_ENABLE_Msk;

should change the clock source and enable the timer on real hardware, but
resulted in an error in qemu.

Signed-off-by: Richard Petri <git@rpls.de>
---
 hw/timer/armv7m_systick.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
index 3bd951dd04..5dfe39afe3 100644
--- a/hw/timer/armv7m_systick.c
+++ b/hw/timer/armv7m_systick.c
@@ -149,6 +149,10 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
         s->control &= 0xfffffff8;
         s->control |= value & 7;
 
+        if ((oldval ^ value) & SYSTICK_CLKSOURCE) {
+            systick_set_period_from_clock(s);
+        }
+
         if ((oldval ^ value) & SYSTICK_ENABLE) {
             if (value & SYSTICK_ENABLE) {
                 ptimer_run(s->ptimer, 0);
@@ -156,10 +160,6 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
                 ptimer_stop(s->ptimer);
             }
         }
-
-        if ((oldval ^ value) & SYSTICK_CLKSOURCE) {
-            systick_set_period_from_clock(s);
-        }
         ptimer_transaction_commit(s->ptimer);
         break;
     }
-- 
2.35.1

Re: [PATCH] hw/timer/armv7m_systick: Update clock source before enabling timer

[Peter Maydell]

On Tue, 1 Feb 2022 at 19:28, Richard Petri <git@rpls.de> wrote:
>
> Starting the SysTick timer and changing the clock source a the same time
> will result in an error, if the previous clock period was zero. For exmaple,
> on the mps2-tz platforms, no refclk is present. Right after reset, the
> configured ptimer period is zero, and trying to enabling it will turn it off
> right away. E.g., code running on the platform setting
>
>     SysTick->CTRL  = SysTick_CTRL_CLKSOURCE_Msk | SysTick_CTRL_ENABLE_Msk;
>
> should change the clock source and enable the timer on real hardware, but
> resulted in an error in qemu.
>
> Signed-off-by: Richard Petri <git@rpls.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks, you've saved me a debugging session! I had a bug report about
a problem with the systick timer a couple of days back, but I hadn't yet
got round to investigating it, and now I don't have to, because this
patch fixes the reported failure :-)

-- PMM

Re: [PATCH] hw/timer/armv7m_systick: Update clock source before enabling timer

[Richard Petri]

On Tue, Feb 01 2022, Peter Maydell <peter.maydell@linaro.org> wrote:
> Thanks, you've saved me a debugging session! I had a bug report about
> a problem with the systick timer a couple of days back, but I hadn't yet
> got round to investigating it, and now I don't have to, because this
> patch fixes the reported failure :-)

Oh, I didn't see the issue in the tracker. I guess someone else played
around with the new machines at the same time :-) A quick workaround on
the guest code side btw: set the clock source and then add the enable
flag with a separate write.

And if you still want to do a debug session: I think there is another
related problem. The `systick_reset` function sets the right clock
source, but I guess at the time of calling reset the `cpuclk` doesn't
have the right value (probably zero)? I was confused at first, because
the reset code suggests that everything is OK from the start, but that
doesn't seem to be the case. I don't have a good enough overview of the
qemu sources to know what is called when. But even if the reset would be
right, first setting the clock source and then en/disabling the timer is
the better order IMO.

/Richard

Re: [PATCH] hw/timer/armv7m_systick: Update clock source before enabling timer

[Peter Maydell]

On Tue, 1 Feb 2022 at 23:15, Richard Petri <git@rpls.de> wrote:
> And if you still want to do a debug session: I think there is another
> related problem. The `systick_reset` function sets the right clock
> source, but I guess at the time of calling reset the `cpuclk` doesn't
> have the right value (probably zero)? I was confused at first, because
> the reset code suggests that everything is OK from the start, but that
> doesn't seem to be the case. I don't have a good enough overview of the
> qemu sources to know what is called when. But even if the reset would be
> right, first setting the clock source and then en/disabling the timer is
> the better order IMO.

Yeah, there is a bug here. On these boards there is no refclk, so
the systick timer is supposed to say "refclk not connected, on
reset we set SYST_CSR.CLOCKSOURCE to 1 (meaning use cpu clock) and
make that bit read-only". But we weren't correctly detecting the
lack of refclk so we were resetting to "use the refclk", which
is bad because it has period 0, not being connected to anything.
(If the guest explicitly programs the CLOCKSOURCE bit then it works
around this bug, but in theory it shouldn't have to.)
I'll send a patch in a moment.

thanks
-- PMM

Re: [PATCH] hw/timer/armv7m_systick: Update clock source before enabling timer

[Peter Maydell]

On Tue, 1 Feb 2022 at 19:28, Richard Petri <git@rpls.de> wrote:
>
> Starting the SysTick timer and changing the clock source a the same time
> will result in an error, if the previous clock period was zero. For exmaple,
> on the mps2-tz platforms, no refclk is present. Right after reset, the
> configured ptimer period is zero, and trying to enabling it will turn it off
> right away. E.g., code running on the platform setting
>
>     SysTick->CTRL  = SysTick_CTRL_CLKSOURCE_Msk | SysTick_CTRL_ENABLE_Msk;
>
> should change the clock source and enable the timer on real hardware, but
> resulted in an error in qemu.
>
> Signed-off-by: Richard Petri <git@rpls.de>



Applied to target-arm.next, thanks.

-- PMM