[PATCH v2 2/2] cyclic: parse cyclic list only if next cyclic timestamp is reached

[Rasmus Villemoes]

On Fri, Jul 03 2026, "Patrice Chotard" <patrice.chotard at foss.st.com> wrote:

> On STM32MP157C-DK2, when using the "ums" command, in sleep_thread(),
> ctrlc() is called every ~640ms which doesn't allows high reactivity when
> user press CTRL+C in U-Boot console.
>
> In sleep_thread() loop, ctrlc() is called every 200000 iterations.
> But schedule is called on each loop iteration.
>
> Optimize cyclic_run() in order to parse the cyclic list only is the
> next cyclic to run is due.
> During cyclic list parsing, save the nearest cyclic timestamp, which
> allows to exit from next cyclic_run() call if no cyclic will be executed.
>
> This allow to save computation time :
>   _ before : ctrlc() is called every ~640ms
>   _ after  : ctrlc() is called every ~230ms
>
> Signed-off-by: Patrice Chotard <patrice.chotard at foss.st.com>
> Cc: Marek Vasut <marek.vasut at mailbox.org>
> ---
>  common/cyclic.c                   | 16 ++++++++++++++++
>  include/asm-generic/global_data.h |  4 ++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/common/cyclic.c b/common/cyclic.c
> index b37cd6d8ff0..10ea0711ba1 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -68,11 +68,17 @@ static void cyclic_run(void)
>  	struct cyclic_info *cyclic;
>  	struct hlist_node *tmp;
>  	u64 now, cpu_time;
> +	u64 next_call = UINT64_MAX;
>
>  	/* Prevent recursion */
>  	if (gd->flags & GD_FLG_CYCLIC_RUNNING)
>  		return;
>
> +	/* check if the next cyclic function's timestamp is reached */
> +	now = get_timer_us(0);
> +	if (time_after_eq64(gd->next_cyclic_call, now))
> +		return;
> +
>  	gd->flags |= GD_FLG_CYCLIC_RUNNING;
>  	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
>  		/*
> @@ -102,7 +108,17 @@ static void cyclic_run(void)
>  				cyclic->already_warned = true;
>  			}
>  		}
> +		if (next_call > cyclic->next_call) {
> +			next_call = cyclic->next_call;
> +			/* move this cyclic to list's head */
> +			hlist_del(&cyclic->list);
> +			hlist_add_head(&cyclic->list, cyclic_get_list());
> +		}
>  	}
> +
> +	if (next_call != UINT64_MAX)
> +		gd->next_cyclic_call = next_call;

Are you sure this is correct? There are at least two things I can think
of that might be wrong:

(1) Doing > or < comparison (as done in the next_call > cyclic_next_call
line), that should never be done, because the logic around timestamps is
that we do allow wrapping, assuming the two timestamps being compared
never represent absolute times that differ by more than what can be
represented in the signed version of the timestamp type - that's why the
time_after*/time_before* family do a subtraction and a signed compare
to 0.

(2) What happens if we have a single cyclic consumer registered, with a
rather long period (say 1s), hence a long time till its next_call, then another
cyclic consumer is registered which should be called every 50ms. Since I
don't see anything added to cyclic_register(), it seems that the new
consumer would be ignored for almost a second?

I'm also not really sure what the "move the first expiring timer to the
head" logic is supposed to achieve. Either way, if any timer has
expired, we'll run through the whole list, so while the first expiring
timer would then be handled first on the next cyclic_run(), that doesn't
mean we'll save any runtime. And this exercise seems to be about doing as
little as possible in cyclic_run(), so can you explain what the pointer
juggling does?

I assume that what makes cyclic_run() somewhat expensive isn't actually
running through the cyclic_list(), but the get_timer_us() calls done for
each entry. Could you try to see what something like this (entirely
untested, but you get the idea) would do:

diff --git a/common/cyclic.c b/common/cyclic.c
index ec952a01ee1..2616c4d5bc6 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -67,26 +67,28 @@ static void cyclic_run(void)
 {
        struct cyclic_info *cyclic;
        struct hlist_node *tmp;
-       uint64_t now, cpu_time;
+       uint64_t now, after, cpu_time;
 
        /* Prevent recursion */
        if (gd->flags & GD_FLG_CYCLIC_RUNNING)
                return;
 
        gd->flags |= GD_FLG_CYCLIC_RUNNING;
+       now = get_timer_us(0);
        hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
                /*
                 * Check if this cyclic function needs to get called, e.g.
                 * do not call the cyclic func too often
                 */
-               now = get_timer_us(0);
                if (time_after_eq64(now, cyclic->next_call)) {
                        /* Call cyclic function and account it's cpu-time */
                        cyclic->next_call = now + cyclic->delay_us;
                        cyclic->func(cyclic);
+                       after = get_timer_us(0);
                        cyclic->run_cnt++;
-                       cpu_time = get_timer_us(0) - now;
+                       cpu_time = after - now;
                        cyclic->cpu_time_us += cpu_time;
+                       now = after;
 
                        /* Check if cpu-time exceeds max allowed time */
                        if ((cpu_time > CONFIG_CYCLIC_MAX_CPU_TIME_US) &&

Because the time_after_eq64() test really doesn't take anywhere near a
micro-second, nor does fetching the next element from the linked list,
so using the same value for "now" until we actually do hit an expired
timer should be ok.

[I also want to move the computation of the new cyclic->next_call to
after the callback, so that the callback can update its ->delay_us value
if it needs to be called at another frequency, but that's a separate
topic.]

Rasmus