[Qemu-devel] [PATCH 1/1] armv7m_nvic: systick: Reload the RELOAD value and count down only if ENABLE bit is set

[Qemu-devel] [PATCH 1/1] armv7m_nvic: systick: Reload the RELOAD value and count down only if ENABLE bit is set

[Adrian Huang]

Consider the following pseudo code to configure SYSTICK (The
recommended programming sequence from "the definitive guide to the
arm cortex-m3"):
    SYSTICK Reload Value Register = 0xffff
    SYSTICK Current Value Register = 0
    SYSTICK Control and Status Register = 0x7

The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
systick_reload(). As a consequence, the systick.tick member is updated
and the systick timer starts to count down when the ENABLE bit of
SYSTICK Control and Status Register is cleared.

The worst case is that: during the system initialization, the reset
value of the SYSTICK Control and Status Register is 0x00000000. 
When the code "SYSTICK Current Value Register = 0" is executed, the
systick.tick member is accumulated with "(s->systick.reload + 1) *
systick_scale(s)". The systick_scale() gets the external_ref_clock
scale because the CLKSOURCE bit of the SYSTICK Control and Status
Register is cleared. This is the incorrect behavior because of the
code "SYSTICK Control and Status Register = 0x7". Actually, we want
the processor clock instead of the external reference clock.

This incorrect behavior defers the generation of the first interrupt. 

The patch fixes the above-mentioned issue by setting the systick.tick
member and modifying the systick timer only if the ENABLE bit of
the SYSTICK Control and Status Register is set.

In addition, the Cortex-M3 Devices Generic User Guide mentioned that
"When ENABLE is set to 1, the counter loads the RELOAD value from the
SYST RVR register and then counts down". This patch adheres to the
statement of the user guide.

Signed-off-by: Adrian Huang <adrianhuang0701@gmail.com>
Reviewed-by: Jim Huang <jserv.tw@gmail.com>
---
 hw/intc/armv7m_nvic.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 6ff6c7f..a610d2b 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -77,6 +77,16 @@ static inline int64_t systick_scale(nvic_state *s)
 
 static void systick_reload(nvic_state *s, int reset)
 {
+    /* The Cortex-M3 Devices Generic User Guide mentioned that "When the
+     * ENABLE bit is set to 1, the counter loads the RELOAD value from the
+     * SYST RVR register and then counts down". So, we need to check the
+     * ENABLE bit before reloading the value. This can also prevent the
+     * unexpected SysTick delay.
+     */
+    if ((s->systick.control & SYSTICK_ENABLE) == 0) {
+        return;
+    }
+
     if (reset)
         s->systick.tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     s->systick.tick += (s->systick.reload + 1) * systick_scale(s);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/1] armv7m_nvic: systick: Reload the RELOAD value and count down only if ENABLE bit is set

[Peter Maydell]

On 7 May 2015 at 12:44, Adrian Huang <adrianhuang0701@gmail.com> wrote:
> Consider the following pseudo code to configure SYSTICK (The
> recommended programming sequence from "the definitive guide to the
> arm cortex-m3"):
>     SYSTICK Reload Value Register = 0xffff
>     SYSTICK Current Value Register = 0
>     SYSTICK Control and Status Register = 0x7
>
> The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
> systick_reload(). As a consequence, the systick.tick member is updated
> and the systick timer starts to count down when the ENABLE bit of
> SYSTICK Control and Status Register is cleared.
>
> The worst case is that: during the system initialization, the reset
> value of the SYSTICK Control and Status Register is 0x00000000.
> When the code "SYSTICK Current Value Register = 0" is executed, the
> systick.tick member is accumulated with "(s->systick.reload + 1) *
> systick_scale(s)". The systick_scale() gets the external_ref_clock
> scale because the CLKSOURCE bit of the SYSTICK Control and Status
> Register is cleared. This is the incorrect behavior because of the
> code "SYSTICK Control and Status Register = 0x7". Actually, we want
> the processor clock instead of the external reference clock.
>
> This incorrect behavior defers the generation of the first interrupt.
>
> The patch fixes the above-mentioned issue by setting the systick.tick
> member and modifying the systick timer only if the ENABLE bit of
> the SYSTICK Control and Status Register is set.
>
> In addition, the Cortex-M3 Devices Generic User Guide mentioned that
> "When ENABLE is set to 1, the counter loads the RELOAD value from the
> SYST RVR register and then counts down". This patch adheres to the
> statement of the user guide.
>
> Signed-off-by: Adrian Huang <adrianhuang0701@gmail.com>
> Reviewed-by: Jim Huang <jserv.tw@gmail.com>

Applied to target-arm.next, thanks, with a minor tweak to the
comment text phrasing:
    /* The Cortex-M3 Devices Generic User Guide says that "When the
     * ENABLE bit is set to 1, the counter loads the RELOAD value from the
     * SYST RVR register and then counts down". So, we need to check the
     * ENABLE bit before reloading the value.
     */

-- PMM