timer: cortex-m systick: add idle timer #63187

niedzwiecki-dawid · 2023-09-28T06:38:38Z

Some chips, that use Cortex-M SysTick as the system timer, disable a clock in a low power mode, that is the input for the SysTick e.g. STM32Fx family.

It blocks enabling power management for these chips. The wake-up function doesn't work and the time measurement is lost.

Add an additional IDLE timer that handles these functionality when the system is about to enter IDLE. It has to wake up the chip and update the cycle counter by time not measured by the SysTick. The IDLE timer has to support counter API (setting alarm and reading current value).

Related:
#61631
#60558
#19755

erwango

Thanks for submitting this. I have a quite similar need for a different use case:

In other STM32 series which have a low power timer which helps counting tick in lp modes (stop modes in STM32 nomenclature), we would need the same functionality to enter deeper sleep states (standby mode) where even LP timer would lose its clock and only RTC could be used (common point with your use case).

My understanding is that to implement my use case, one option would be to reproduce the nearly* same piece of code in drivers/timer/stm32_lptim_timer.c. Then maybe others would need the same implementation in their own timer driver.

So here is my (not blocking) suggestion:
Since this piece of code is destined to be duplicated in multiple timer drivers, I wonder if it could be implemented as a "generic timer back up service", rather than a specific cortex_m_systick one.

*Note: in my case, I would not need an extra logic:
Using lptimer, I would be able to enter some of the sleep states and only the deepest one would need to trigger use of counter/RTC wakeup. So the idle parameter will not be sufficient and I would need way to distinguish next state: stop or standby.
I guess this extra functionality can be added later on in a "generic timer back up service".

niedzwiecki-dawid · 2023-10-02T06:15:09Z

Thanks for the comment.

Very interesting case. However, I've tried to extract common code of the "back up service" for all timers and my conclusion is that it is not a good idea. 2 main points: the code uses a local variable, and it is important to read values of the two timers as soon as possible (no calculation between).

The one thing would make sense, I think I can change the config and chosen string to CORTEX_IDLE_TIMER, to use it by all drivers.

Please let me know what you think.

niedzwiecki-dawid · 2023-10-04T06:55:43Z

Do you know who would be able to review this PR?

erwango · 2023-10-04T06:57:24Z

2 main points: the code uses a local variable, and it is important to read values of the two timers as soon as possible (no calculation between).

What about setting variable scope to global?

erwango · 2023-10-04T06:58:18Z

Do you know who would be able to review this PR?

@anangl and @andyross

erwango · 2023-10-04T15:15:27Z

and .. @teburd :) !

niedzwiecki-dawid · 2023-10-06T06:02:51Z

Using lptimer, I would be able to enter some of the sleep states and only the deepest one would need to trigger use of counter/RTC wakeup. So the idle parameter will not be sufficient and I would need way to distinguish next state: stop or standby.

I've been thinking about your case. IMO, there is a simply way to handle that. You can compare the measurement of IDLE and system timers. If a difference is bigger than ~10%/1-2 IDLE timer ticks, it means the system timer was stopped and needs correction. Does it make sense?

erwango · 2023-10-06T14:25:57Z

Using lptimer, I would be able to enter some of the sleep states and only the deepest one would need to trigger use of counter/RTC wakeup. So the idle parameter will not be sufficient and I would need way to distinguish next state: stop or standby.

I've been thinking about your case. IMO, there is a simply way to handle that. You can compare the measurement of IDLE and system timers. If a difference is bigger than ~10%/1-2 IDLE timer ticks, it means the system timer was stopped and needs correction. Does it make sense?

Actually my point is that the timer to use depends on the sleep duration (defined by low power state residency). Since standby mode (and use of RTC counter) would require longer time to exit (and potentially also enter), I would go in standby mode only if time allows and, for shorter timeout periods, I would use STOP Mode / LPTIM. This knowledge is available in sys_clock_set_timeout caller.

niedzwiecki-dawid · 2023-10-06T14:36:40Z

Make sense

drivers/timer/cortex_m_systick.c

niedzwiecki-dawid · 2023-10-11T04:11:12Z

Friendly ping :-) Please review if you can, this change is very important for my work.

teburd · 2023-10-12T15:07:03Z

This is a pretty significant change to the cortex-m timer, and that's a widely used critical piece of software in Zephyr.

I get the idea here, though I'm unsure about this idea of the timer depending on the counter API and driver.

@npitre is honestly probably the best person to review this

niedzwiecki-dawid · 2023-10-13T04:30:04Z

This is a pretty significant change to the cortex-m timer, and that's a widely used critical piece of software in Zephyr.

I get the idea here, though I'm unsure about this idea of the timer depending on the counter API and driver.

@npitre is honestly probably the best person to review this

Thanks for your response.
That's true :-) However, the change impacts only users that add the idle timer. Each part of code I've added is inside ifdef #ifdef CONFIG_CORTEX_M_SYSTICK_IDLE_TIMER. Of course, it doesn't mean it can be buggy.

npitre · 2023-10-14T18:26:00Z

What does the `idle_lock` protect against?

niedzwiecki-dawid · 2023-10-16T06:39:50Z

What does the idle_lock protect against?

When we enter the sys_clock_idle_exit function, the SysTick is already running. The LOAD register hasn’t been changed while setting the timeout, so we can not be sure about its value. That means the SysTick IRQ can occur any time. cycle_count is updated in the sys_clock_idle_exit so we can not be interrupted. The second important thing is that these two measurements

		counter_get_value(idle_timer, &idle_timer_post);
		systick_diff = cycle_count + elapsed() - cycle_pre_idle;

have to happen one after another, without any interruption.

EDIT: I can see now, that sys_clock_idle_exit is called with interrupts disabled, so that means idle_lock is not needed. Is that true?

npitre · 2023-10-16T14:50:23Z

What does the idle_lock protect against?

When we enter the sys_clock_idle_exit function, the SysTick is already
running. The LOAD register hasn’t been changed while setting the timeout,
so we can not be sure about its value. That means the SysTick IRQ can
occur any time. cycle_count is updated in the sys_clock_idle_exit so
we can not be interrupted.

Still, my question remains. Unless you fear concurrent reentrancy into
sys_clock_idle_exit(), the idle_lock gives you no protection other than
simply disabling interrupts on the local CPU. I doubt there are many SMP
systems using the systick timer out there but still. It looks to me that
you'd better be using the existing lock instead.

Some chips, that use Cortex-M SysTick as the system timer, disable a clock in a low power mode, that is the input for the SysTick e.g. STM32Fx family. It blocks enabling power management for these chips. The wake-up function doesn't work and the time measurement is lost. Add an additional IDLE timer that handles these functionality when the system is about to enter IDLE. It has to wake up the chip and update the cycle counter by time not measured by the SysTick. The IDLE timer has to support counter API (setting alarm and reading current value). Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

niedzwiecki-dawid · 2023-10-17T11:57:23Z

Thanks for the responses. Disabling interrupts was what I needed, but after making sure the sys_clock_idle_exit is called with the interrupts disabled, the lock is not needed anymore.

Anyway, the existing lock couldn't be used, because the sys_clock_announce function is called, which calls the sys_clock_set_timeout function that uses the lock.

npitre

I don't fully understand it all and I don't have access to affected platforms, but those changes are all confined inside CONFIG_CORTEX_M_SYSTICK_IDLE_TIMER so that is safe to those who don't need it.

niedzwiecki-dawid · 2023-10-20T04:22:04Z

I don't fully understand it all and I don't have access to affected platforms, but those changes are all confined inside CONFIG_CORTEX_M_SYSTICK_IDLE_TIMER so that is safe to those who don't need it.

Thanks :-) Is there anything I should explain more in details?

niedzwiecki-dawid · 2023-10-25T10:48:21Z

Hello :-) do you know anyone else who could review the PR and approve it? Or I should do something more.

teburd · 2023-10-26T03:49:37Z

Hello :-) do you know anyone else who could review the PR and approve it? Or I should do something more.

This needs maintainer approval (@andyross)

niedzwiecki-dawid · 2023-10-30T13:22:21Z

@andyross can I somehow speed up review? :-)

andyross

Came here on a ping. This all seems reasonable to me, a few notes, but the logic seems fine.

andyross · 2023-11-06T16:15:40Z

drivers/timer/cortex_m_systick.c

+static uint32_t idle_timer_pre_idle;
+
+/* Idle timer used for timer while entering the idle state */
+static const struct device *idle_timer = DEVICE_DT_GET(DT_CHOSEN(zephyr_cortex_m_idle_timer));


Given that you're predicating this on a DTS counter device already, you could do this entirely with devicetree and avoid the extra kconfig.

Good point, but I would like to keep the kconfig with the help for additional documentation. If you want, I can change the config to non user-configurable one as a follow-up PR. Does it make sense?

andyross · 2023-11-06T16:16:32Z

drivers/timer/Kconfig.cortex_m_systick

+	  There are chips e.g. STMFX family that use SysTick as a system timer,
+	  but SysTick is not clocked in low power mode. These chips usually have
+	  another timer that is not stopped, but it has lower frequency e.g.
+	  RTC, thus it can't be used as a main system timer.


Would be good to document the required interface for this somewhere. "The idle timer must be a counter device chosen in device tree via...", etc...

The requirement is mentioned later in the help message

The chosen IDLE timer node has to support setting alarm from the counter API.

niedzwiecki-dawid requested a review from anangl as a code owner September 28, 2023 06:38

zephyrbot added the area: Timer Timer label Sep 28, 2023

zephyrbot requested review from andyross and teburd September 28, 2023 06:39

zephyrbot assigned andyross Sep 28, 2023

niedzwiecki-dawid force-pushed the systick_idle branch from 914db9d to d787fe5 Compare September 28, 2023 06:48

niedzwiecki-dawid requested a review from erwango September 28, 2023 07:57

niedzwiecki-dawid added the area: Power Management label Sep 28, 2023

erwango reviewed Sep 29, 2023

View reviewed changes

erwango requested review from ceolin and JordanYates September 29, 2023 13:47

erwango reviewed Oct 6, 2023

View reviewed changes

drivers/timer/cortex_m_systick.c Outdated Show resolved Hide resolved

niedzwiecki-dawid force-pushed the systick_idle branch from d787fe5 to bb0c2db Compare October 10, 2023 09:39

teburd requested a review from npitre October 12, 2023 15:07

niedzwiecki-dawid force-pushed the systick_idle branch from bb0c2db to 7bae342 Compare October 17, 2023 10:52

npitre approved these changes Oct 19, 2023

View reviewed changes

andyross approved these changes Nov 6, 2023

View reviewed changes

carlescufi merged commit 457d437 into zephyrproject-rtos:main Nov 7, 2023
18 checks passed

This was referenced Nov 24, 2023

Low Power Mode backup timer #61631

Closed

Low power mode for STM32F4 chips #60558

Closed

gautierg-st mentioned this pull request Jan 12, 2024

STM32 Add PM Suspend to Ram support #67534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timer: cortex-m systick: add idle timer #63187

timer: cortex-m systick: add idle timer #63187

niedzwiecki-dawid commented Sep 28, 2023 •

edited

erwango left a comment

niedzwiecki-dawid commented Oct 2, 2023

niedzwiecki-dawid commented Oct 4, 2023

erwango commented Oct 4, 2023

erwango commented Oct 4, 2023

erwango commented Oct 4, 2023

niedzwiecki-dawid commented Oct 6, 2023

erwango commented Oct 6, 2023 •

edited

niedzwiecki-dawid commented Oct 6, 2023

niedzwiecki-dawid commented Oct 11, 2023

teburd commented Oct 12, 2023 •

edited

niedzwiecki-dawid commented Oct 13, 2023 •

edited

npitre commented Oct 14, 2023 via email

niedzwiecki-dawid commented Oct 16, 2023 •

edited

npitre commented Oct 16, 2023

niedzwiecki-dawid commented Oct 17, 2023

npitre left a comment

niedzwiecki-dawid commented Oct 20, 2023

niedzwiecki-dawid commented Oct 25, 2023

teburd commented Oct 26, 2023

niedzwiecki-dawid commented Oct 30, 2023

andyross left a comment

andyross Nov 6, 2023

niedzwiecki-dawid Nov 7, 2023

andyross Nov 6, 2023

niedzwiecki-dawid Nov 7, 2023

timer: cortex-m systick: add idle timer #63187

timer: cortex-m systick: add idle timer #63187

Conversation

niedzwiecki-dawid commented Sep 28, 2023 • edited

erwango left a comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented Oct 2, 2023

niedzwiecki-dawid commented Oct 4, 2023

erwango commented Oct 4, 2023

erwango commented Oct 4, 2023

erwango commented Oct 4, 2023

niedzwiecki-dawid commented Oct 6, 2023

erwango commented Oct 6, 2023 • edited

niedzwiecki-dawid commented Oct 6, 2023

niedzwiecki-dawid commented Oct 11, 2023

teburd commented Oct 12, 2023 • edited

niedzwiecki-dawid commented Oct 13, 2023 • edited

npitre commented Oct 14, 2023 via email

niedzwiecki-dawid commented Oct 16, 2023 • edited

npitre commented Oct 16, 2023

niedzwiecki-dawid commented Oct 17, 2023

npitre left a comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented Oct 20, 2023

niedzwiecki-dawid commented Oct 25, 2023

teburd commented Oct 26, 2023

niedzwiecki-dawid commented Oct 30, 2023

andyross left a comment

Choose a reason for hiding this comment

andyross Nov 6, 2023

Choose a reason for hiding this comment

niedzwiecki-dawid Nov 7, 2023

Choose a reason for hiding this comment

andyross Nov 6, 2023

Choose a reason for hiding this comment

niedzwiecki-dawid Nov 7, 2023

Choose a reason for hiding this comment

niedzwiecki-dawid commented Sep 28, 2023 •

edited

erwango commented Oct 6, 2023 •

edited

teburd commented Oct 12, 2023 •

edited

niedzwiecki-dawid commented Oct 13, 2023 •

edited

niedzwiecki-dawid commented Oct 16, 2023 •

edited