drivers: systick: Improve the logic for detecting counter wrap-around events when exiting low power mode.

[ZhaoxiangJin]

When exiting from low power mode, there are three scenarios for idle_timer_pre_idle and idle_timer_post:

• idle_timer_pre_idle > idle_timer_post In this case, a wraparound has definitely occurred, so: diff = top - pre + post + 1

    /* exmaples: Top=100, Scheduled=90, Pre=60, Post=50 */
    if (idle_timer_pre_idle > idle_timer_post) {
           idle_timer_diff = idle_timer_top - idle_timer_pre_idle + idle_timer_post + 1;
    }

• idle_timer_pre_idle = idle_timer_post In this case, there are two possibilities:

  • No wraparound occurred: diff = post - pre
  • Wraparound occurred (determined by alarm_pending being true): diff = top - pre + post + 1

    /* example: Top=100, Scheduled=100, Pre=0, Post=0 */
    if (idle_timer_pre_idle == idle_timer_post) {
        if (alarm_pending) {
            idle_timer_diff = idle_timer_top - idle_timer_pre_idle + idle_timer_post + 1;
        } else {
             idle_timer_diff = idle_timer_post - idle_timer_pre_idle; /* did not enter low power mode. */
        }   
    }

• idle_timer_pre_idle < idle_timer_post In this case, there are two possibilities:

  • No wraparound occurred: diff = post - pre (note that alarm_pending maybe true caused by alarm, if alarm_pending is not true, then it means it was woken up early by another event. )
  • Wraparound occurred: diff = top - pre + post + 1 (note that alarm_pending is true caused by wraparound), in this situation, there are two possibilities: one is pre + scheduled >= top, and the other is pre + scheduled < top. Currently there is no suitable method to distinguish the latter case.

    if (idle_timer_pre_idle < idle_timer_post) {
        if (alarm_pending) {
             if () { /* Currently there is no way to find the corresponding condition for this case, so this situation will not be covered for now.  */
                 /* case: wraparound (CPU wakeup time is relatively long). example: Top=100, Scheduled=85, Pre=10, Post=15 (delay 20) */
                 idle_timer_diff = idle_timer_top - idle_timer_pre_idle + idle_timer_post + 1;
             } else if (idle_timer_scheduled_sleep_ticks != 0 && (uint64_t)idle_timer_pre_idle + idle_timer_scheduled_sleep_ticks >= idle_timer_top) {
                 /* case: wraparound (CPU wakeup time is relatively long). example: Top=100, Scheduled=95, Pre=10, Post=15 (delay 10) */
                 idle_timer_diff = idle_timer_top - idle_timer_pre_idle + idle_timer_post + 1;
             } else {
                 /* case: no wraparound. example: Top=100, Scheduled=60, Pre=10, Post=70 */
                 idle_timer_diff = idle_timer_post - idle_timer_pre_idle;
             }
        } else {
             /* case: wakeup early by another event. example: Top=100, Scheduled=60, Pre=10, Post=30 */
             idle_timer_diff = idle_timer_post - idle_timer_pre_idle;
        }   
    }

[mathieuchopstm]

Makes sense to me

[ZhaoxiangJin]

During development, I found that the code logic in the current patch is not sufficient to cover all scenarios. I will update the PR later.

[mathieuchopstm]

I'm struggling to understand the new logic.

Quoting you:

3: idle_timer_pre_idle < idle_timer_post In this case, there are two possibilities:

• No wraparound occurred: diff = post - pre (note that alarm_pending is also true in this case, but the condition (uint64_t)idle_timer_pre_idle + idle_timer_alarm_ticks >= idle_timer_top is not satisfied)
• Wraparound occurred: diff = top + post + 1 - pre (note that alarm_pending is true and the condition (uint64_t)idle_timer_pre_idle + idle_timer_alarm_ticks >= idle_timer_top is satisfied)

How can alarm_pending be true if the timer has not overflowed? Why would idle_timer_pre_idle + idle_timer_alarm_ticks >= idle_timer_top - "is the tick on which alarm was expected higher than the counter's max tick value" - affect whether or not the timer has overflowed?

---

After review and from my understanding of the counter API, I think we should just be checking counter_get_pending_int(): since interrupts are suspended during a PM sequence, the timer cannot service its IRQ and thus the interrupt pending status is a reliable[1] indicator of whether or not an overflow occurred.

[1] It's only reliable up to one overflow: two or more will go undetected, but this should never occur...

[ZhaoxiangJin]

I think this is the current shortcoming of the counter API. We obtain alarm_pending through counter_get_pending_int, but both alarm and top can cause pending_int. Let me give an example to illustrate what happens if we only use alarm_pending as the judgment condition:

Top=100, scheduled_ticks=90, Pre=60, actual measured Post=70.

1. Possible without wraparound (alarm_pending=false): diff = post - pre;
2. Possible with wraparound (alarm_pending=true): diff = top + post + 1 - pre;

Top=100, scheduled_ticks=60, Pre=10, actual measured Post=70. We can't use the above formula anymore, because an alarm was generated. At this time alarm_pending is also true, so using diff = top + post + 1 - pre would be wrong. Therefore, we need to determine the relationship between pre + scheduled_ticks and top.

[mathieuchopstm]

I think this is the current shortcoming of the counter API. We obtain alarm_pending through counter_get_pending_int, but both alarm and top can cause pending_int.

OK, that's the part I didn't understand. (In this case, maybe the SysTick driver should change the counter's top value to guarantee that pending == 1 = timer rollover?)

I'll go through the logic again with this new insight.

[mathieuchopstm]

Top=100, scheduled_ticks=60, Pre=10, actual measured Post=70. We can't use the above formula anymore, because an alarm was generated. At this time alarm_pending is also true, so using diff = top + post + 1 - pre would be wrong. Therefore, we need to determine the relationship between pre + scheduled_ticks and top.

I don't understand why this should be special cased: if Top=100 and Pre=10 and Post=70 (> Pre) and the counter is pending, it must be because of an alarm: if a rollover did occur, it would mean that Top + Post - Pre = 160 ticks have elapsed which is almost 3 times the Scheduled=60 - this should never be occurring?! (SoC is allowed to wake-up earlier than scheduled, but not (too much) later, as far as I understand)

Even though it is not enforced yet, I think Scheduled < Top should always be true, and if it is then Post > Pre implies no overflow occurred (because if it did, the + Top would mean that at least 2x more time than scheduled has elapsed, which to me seems impossible)

[mathieuchopstm]

A more brutal but simple solution is to say that the low-power timer is for exclusive usage by the SysTick driver, because it would allow us to reset the counter to 0 before sleeping, ensuring that pending corresponds to "rollover occurred" and nothing else(-ish?)

[ZhaoxiangJin]

Top=100, scheduled_ticks=60, Pre=10, actual measured Post=70. We can't use the above formula anymore, because an alarm was generated. At this time alarm_pending is also true, so using diff = top + post + 1 - pre would be wrong. Therefore, we need to determine the relationship between pre + scheduled_ticks and top.

I don't understand why this should be special cased: if Top=100 and Pre=10 and Post=70 (> Pre) and the counter is pending, it must be because of an alarm: if a rollover did occur, it would mean that Top + Post - Pre = 160 ticks have elapsed which is almost 3 times the Scheduled=60 - this should never be occurring?! (SoC is allowed to wake-up earlier than scheduled, but not (too much) later, as far as I understand)

Post > Pre overflow can indeed occur. This is actually the situation I discovered during debugging. Assume Top=100, Pre=10, Scheduled=85. Theoretically, an alarm signal (interrupt) should be generated at 95 to wake up the MCU. However, on the MCU I was working with, there's a 20 idle timer ticks delay from when the idle timer alarm signal is issued to when the CPU actually resumes and starts executing code. During this time, the idle timer continues counting. By the time the CPU code executes the z_cms_lptim_hook_on_lpm_exit function, the idle timer has already advanced an additional 20 idle timer ticks, meaning the actual Post value obtained is 15. In this case, the calculation formula needed is: diff = top + post + 1 - pre. This is one type of "post > pre" case.

Another Post > Pre case is the scenario where wrap is unlikely to occur. For example, as I mentioned earlier: Top=100, Scheduled=60, Pre=10. In this situation, wrap is unlikely to happen. Even if the CPU has a 20 idle timer ticks delay, Post should be around 90, and in this case alarm_pending is caused by the alarm. The calculation formula needed is: diff = post - pre.

So we need to cover both of these cases. Using alarm_pending alone is insufficient. However, I now find that the condition: (uint64_t)idle_timer_pre_idle + idle_timer_scheduled_sleep_ticks >= idle_timer_top cannot be used to distinguish between these two cases either. Do you have any suggestions?

[mathieuchopstm]

Post > Pre overflow can indeed occur. This is actually the situation I discovered during debugging. Assume Top=100, Pre=10, Scheduled=85. Theoretically, an alarm signal (interrupt) should be generated at 95 to wake up the MCU. However, on the MCU I was working with, there's a 20 idle timer ticks delay from when the idle timer alarm signal is issued to when the CPU actually resumes and starts executing code. During this time, the idle timer continues counting. By the time the CPU code executes the z_cms_lptim_hook_on_lpm_exit function, the idle timer has already advanced an additional 20 idle timer ticks, meaning the actual Post value obtained is 15. In this case, the calculation formula needed is: diff = top + post + 1 - pre. This is one type of "post > pre" case.

Thanks for the example. That's one edge case I had in the back of my head, but I had trouble finding an example that did break...

Do you have any suggestions?

I'll take the example quoted above. Post=15 could be obtained if:

• SoC woke up early, and only 5 ticks elapsed
• SoC woke up as expected but delay was observed, and 105 ticks have elapsed

Assuming no other alarm on the counter, counter_pending should be False in the former case but True in the latter... but I don't know if that applies in the general case.

---

Looking at tree right now, the only users of zephyr,cortex-m-idle-timer are ST and Analog Devices, and both are using a single-channel counter. Since the SysTick driver assumes ownership of channel 0, this means that de facto the SysTick drivers "owns" the idle timer.

What do you think of updating the documented semantics to say that the zephyr,cortex-m-idle-timer is owned by the SysTick driver, which allows us to reset the counter to 0 before entering idle? This should ensure that pending() can be reliably used to distinguish early wake-up from overflows (assuming the sleep period is capped to top_value ticks, it allows up to an "overhead delay" of 100%/top_value, which seems reasonable as you report your SoC is around 20%).

[ZhaoxiangJin]

I'll take the example quoted above. Post=15 could be obtained if:

• SoC woke up early, and only 5 ticks elapsed
• SoC woke up as expected but delay was observed, and 105 ticks have elapsed

Assuming no other alarm on the counter, counter_pending should be False in the former case but True in the latter... but I don't know if that applies in the general case.

1. Early wakeup should be a general case, and it's easy to detect (counter_pending is false, and post > pre).
2. Not sure whether delayed wakeup is a general case, but it does occur on some NXP MCUs when waking from deep power-down mode. My idea is to not handle delayed wakeup case in the Systick driver for now, because when post > pre and counter_pending is true, the three cases I enumerated in the PR description currently cannot be distinguished (It's also possible that I missed some of the available conditions).

Looking at tree right now, the only users of zephyr,cortex-m-idle-timer are ST and Analog Devices, and both are using a single-channel counter. Since the SysTick driver assumes ownership of channel 0, this means that de facto the SysTick drivers "owns" the idle timer.

What do you think of updating the documented semantics to say that the zephyr,cortex-m-idle-timer is owned by the SysTick driver, which allows us to reset the counter to 0 before entering idle? This should ensure that pending() can be reliably used to distinguish early wake-up from overflows (assuming the sleep period is capped to top_value ticks, it allows up to an "overhead delay" of 100%/top_value, which seems reasonable as you report your SoC is around 20%).

The idle-timer currently used by NXP is also one channel. However, I think enhancing the counter API might be better, because I'm not sure whether there will be cases where the alarm is one of multiple channels in the idle-timer as more companies use this feature.

[mathieuchopstm]

Top=100, scheduled_ticks=60, Pre=10, actual measured Post=70. We can't use the above formula anymore, because an alarm was generated. At this time alarm_pending is also true, so using diff = top + post + 1 - pre would be wrong. Therefore, we need to determine the relationship between pre + scheduled_ticks and top.

I don't understand why this should be special cased: if Top=100 and Pre=10 and Post=70 (> Pre) and the counter is pending, it must be because of an alarm: if a rollover did occur, it would mean that Top + Post - Pre = 160 ticks have elapsed which is almost 3 times the Scheduled=60 - this should never be occurring?! (SoC is allowed to wake-up earlier than scheduled, but not (too much) later, as far as I understand)

Even though it is not enforced yet, I think Scheduled < Top should always be true, and if it is then Post > Pre implies no overflow occurred (because if it did, the + Top would mean that at least 2x more time than scheduled has elapsed, which to me seems impossible)

[ZhaoxiangJin]

Hello @mathieuchopstm, I have updated the code and the PR description, please help to review again, thanks.

[mathieuchopstm]

I'd just add comments in the code, otherwise seems reasonable...
(these are just suggestions: feel free to reword)

[ZhaoxiangJin]

I'd just add comments in the code, otherwise seems reasonable... (these are just suggestions: feel free to reword)

Thank you for your patient review. I have updated the comments. Please help review again.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud