hw/timer/armv7m_systick.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.