Skip to content

[arch][arm-m][systick] fix elapsed cycle calculation in current_time - #545

Open
jasonbridges wants to merge 1 commit into
littlekernel:masterfrom
jasonbridges:fix/arm-m-systick-delta
Open

jasonbridges wants to merge 1 commit into
littlekernel:masterfrom
jasonbridges:fix/arm-m-systick-delta

Conversation

@jasonbridges

Copy link
Copy Markdown

Per the Armv7-M and Armv8-M specifications, SysTick->CTRL.COUNTFLAG is asserted when SysTick->VAL transitions from 1 to 0, and SysTick->VAL reloads to LOAD (reload) on the next clock edge (0 -> reload). Within a single current_ticks period of reload + 1 clock cycles, SysTick->VAL therefore progresses through the sequence:

0, reload, reload - 1, ..., 1

corresponding to 0, 1, 2, ..., reload elapsed cycles since the 1 -> 0 transition that incremented current_ticks.

Computing (reload - val) in current_time() and current_time_hires() was off by 1 cycle for val in [1, reload] and evaluated to reload (an entire tick period) when val == 0, causing time to jump forward by a full tick interval whenever SysTick->VAL was read during the VAL == 0 cycle.

Compute delta_ticks as val ? (reload + 1 - val) : 0 so all reload + 1 states map 1-to-1 to 0..reload elapsed cycles without polling.

Per the Armv7-M and Armv8-M specifications, SysTick->CTRL.COUNTFLAG is
asserted when SysTick->VAL transitions from 1 to 0, and SysTick->VAL
reloads to LOAD (reload) on the next clock edge (0 -> reload). Within a
single current_ticks period of reload + 1 clock cycles, SysTick->VAL
therefore progresses through the sequence:

  0, reload, reload - 1, ..., 1

corresponding to 0, 1, 2, ..., reload elapsed cycles since the 1 -> 0
transition that incremented current_ticks.

Computing (reload - val) in current_time() and current_time_hires() was
off by 1 cycle for val in [1, reload] and evaluated to reload (an entire
tick period) when val == 0, causing time to jump forward by a full tick
interval whenever SysTick->VAL was read during the VAL == 0 cycle.

Compute delta_ticks as val ? (reload + 1 - val) : 0 so all reload + 1
states map 1-to-1 to 0..reload elapsed cycles without polling.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Both localized calculations correctly map the documented counter sequence to elapsed cycles, with no unresolved issues.

No open issues 🎉

What changed in this PR

Corrects SysTick elapsed-cycle calculations in both time APIs so they match the counter’s reload sequence.

Changes:

  • Treats VAL == 0 as zero elapsed cycles, preventing a spurious forward time jump.
  • Uses reload + 1 - val for nonzero values, correcting the one-cycle offset.
File Description
arch/​arm/​arm-m/​systick/​systick.c Corrects elapsed-cycle arithmetic in current_time() and current_time_hires().

Review effort: Balanced

@travisg travisg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the counting model against the v6-M (DDI0419 B3.3.1), v7-M (DDI0403 B3.3.1) and v8-M (DDI0553 IVHDT / SYST_CSR) manuals: all three say the counter reloads from SYST_RVR on the clock edge after reaching zero and that COUNTFLAG is set by the 1->0 transition, so VAL == 0 is a real one-cycle state on every profile and the reload + 1 period / val ? reload + 1 - val : 0 mapping is exact. At val == 1 it gives exactly tick_interval_us, so the sum stays continuous across the current_ticks increment. QEMU's model (PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) agrees.

Booted it on the mps2 targets (arm-m3/m4/m7/m33/m55 under run-qemu-boot-tests.py), 5/5 pass including kernel/test/clock_tests.

Looks good. A few non-blocking nits inline.


/* convert ticks to msec */
uint32_t delta = (reload - val) / (tick_rate_mhz * 1000);
uint32_t delta_ticks = val ? (reload + 1 - val) : 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: delta_ticks reads oddly next to current_ticks / *ticks, which mean timer periods in this file. elapsed_cycles or similar would avoid the overload.

Also worth a one-line comment here that VAL == 0 is the cycle between the 1->0 transition and the reload, so the next reader doesn't have to go to the ARM ARM to see why 0 maps to 0 and reload maps to 1.


/* convert ticks to usec */
uint32_t delta = (reload - val) / tick_rate_mhz;
uint32_t delta_ticks = val ? (reload + 1 - val) : 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same expression as in current_time(). It could live once, e.g. have systick_get_ticks_val() return elapsed cycles instead of the raw VAL, so the two callers only differ in the divisor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though these are AI generated this one actually makes the most sense to me. the others are pretty optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants