Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Emanuele Ghidoli]

Hi,

I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
is applied. Nothing is printed on the serial.

I bisected the issue: booting the parent commit works fine, and
reverting this patch on top of current main also restores
boot. It's not yet obvious to me what in this change causes the
regression.

Would it make sense to revert it until the root cause is found?

Thanks,
Emanuele

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Marek Vasut via U-Boot]

On 7/23/26 4:53 PM, Emanuele Ghidoli wrote:

Hello everyone,

> I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
> ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
> is applied. Nothing is printed on the serial.
> 
> I bisected the issue: booting the parent commit works fine, and
> reverting this patch on top of current main also restores
> boot. It's not yet obvious to me what in this change causes the
> regression.
> 
> Would it make sense to revert it until the root cause is found?

Give Patrice a bit of time to analyze the problem.

-- 
Best regards,
Marek Vasut

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Emanuele Ghidoli]

On 7/23/26 20:35, Marek Vasut wrote:
> On 7/23/26 4:53 PM, Emanuele Ghidoli wrote:
> 
> Hello everyone,
> 
>> I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
>> ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
>> is applied. Nothing is printed on the serial.
>>
>> I bisected the issue: booting the parent commit works fine, and
>> reverting this patch on top of current main also restores
>> boot. It's not yet obvious to me what in this change causes the
>> regression.
>>
>> Would it make sense to revert it until the root cause is found?
> 
> Give Patrice a bit of time to analyze the problem.
> 

Hello,

after the regression, get_timer_us() is called on every cyclic_run() call,
even when the cyclic list is empty. On imx7 this happens earlier than before,
when the timer has not yet been initialized: get_timer_us() calls get_tbclk(),
whose value is zero, and uses it as a divisor in tick_to_time_us(), leading to
a division by zero.

I would fix this by ensuring get_tbclk() returns at least 1 on imx7, but we
could hide bugs in this way. However, this bug could also be present on other
architectures, so I would just guard against entering the cyclic loop by
checking whether the list is empty before doing anything else, so
get_timer_us() is called "at the same point in time" as before.
Something like:
if (hlist_empty(cyclic_get_list())) {
	gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
	return;
}

Kind regards,
Emanuele

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Marek Vasut via U-Boot]

On 7/23/26 10:41 PM, Emanuele Ghidoli wrote:
> 
> 
> On 7/23/26 20:35, Marek Vasut wrote:
>> On 7/23/26 4:53 PM, Emanuele Ghidoli wrote:
>>
>> Hello everyone,
>>
>>> I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
>>> ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
>>> is applied. Nothing is printed on the serial.
>>>
>>> I bisected the issue: booting the parent commit works fine, and
>>> reverting this patch on top of current main also restores
>>> boot. It's not yet obvious to me what in this change causes the
>>> regression.
>>>
>>> Would it make sense to revert it until the root cause is found?
>>
>> Give Patrice a bit of time to analyze the problem.
>>
> 
> Hello,
> 
> after the regression, get_timer_us() is called on every cyclic_run() call,
> even when the cyclic list is empty. On imx7 this happens earlier than before,
> when the timer has not yet been initialized: get_timer_us() calls get_tbclk(),
> whose value is zero, and uses it as a divisor in tick_to_time_us(), leading to
> a division by zero.
> 
> I would fix this by ensuring get_tbclk() returns at least 1 on imx7, but we
> could hide bugs in this way. However, this bug could also be present on other
> architectures, so I would just guard against entering the cyclic loop by
> checking whether the list is empty before doing anything else, so
> get_timer_us() is called "at the same point in time" as before.
> Something like:
> if (hlist_empty(cyclic_get_list())) {
> 	gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
> 	return;
> }
It looks like there are two topics -- optimize the cyclic run and fix 
the MX7 timer.

I agree the cyclic run should exit if the list is empty, but please wait 
for input from Rasmus on that.

Regarding the timer, how come the timer is initialized so late on MX7 ?

The Cortex-A7 should have its own ARM timer, that should be available 
right from the beginning. Is that ARM timer in use on your system, or 
does your system use GPT timer ?

Which board is this ?

+CC Peng

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Rasmus Villemoes]

On Fri, Jul 24 2026, "Marek Vasut" <marek.vasut@mailbox.org> wrote:

> On 7/23/26 10:41 PM, Emanuele Ghidoli wrote:
>>
>>
>> On 7/23/26 20:35, Marek Vasut wrote:
>>> On 7/23/26 4:53 PM, Emanuele Ghidoli wrote:
>>>
>>> Hello everyone,
>>>
>>>> I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
>>>> ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
>>>> is applied. Nothing is printed on the serial.
>>>>
>>>> I bisected the issue: booting the parent commit works fine, and
>>>> reverting this patch on top of current main also restores
>>>> boot. It's not yet obvious to me what in this change causes the
>>>> regression.
>>>>
>>>> Would it make sense to revert it until the root cause is found?
>>>
>>> Give Patrice a bit of time to analyze the problem.
>>>
>>
>> Hello,
>>
>> after the regression, get_timer_us() is called on every cyclic_run() call,
>> even when the cyclic list is empty. On imx7 this happens earlier than before,
>> when the timer has not yet been initialized: get_timer_us() calls get_tbclk(),
>> whose value is zero, and uses it as a divisor in tick_to_time_us(), leading to
>> a division by zero.
>>
>> I would fix this by ensuring get_tbclk() returns at least 1 on imx7, but we
>> could hide bugs in this way. However, this bug could also be present on other
>> architectures, so I would just guard against entering the cyclic loop by
>> checking whether the list is empty before doing anything else, so
>> get_timer_us() is called "at the same point in time" as before.
>> Something like:
>> if (hlist_empty(cyclic_get_list())) {
>> 	gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
>> 	return;
>> }
> It looks like there are two topics -- optimize the cyclic run and fix
> the MX7 timer.
>
> I agree the cyclic run should exit if the list is empty, but please wait
> for input from Rasmus on that.
>

Yes, we could/should definitely exit early if the list is empty, but
don't mix it up with the GD_FLG_CYCLIC_RUNNING bit. Just add it as a
separate early exit condition. But a future reader might read that as
"just" an optimization, seeing that the loop would just do nothing, I
think it also requires an explanation. Something like

diff --git a/common/cyclic.c b/common/cyclic.c
index 573e715587d..e46346536ab 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -69,6 +69,19 @@ static void cyclic_run(void)
        struct hlist_node *tmp;
        u64 now, after, cpu_time;
 
+       /*
+        * Nothing to do if the list is empty. Also, schedule() can be
+        * called before timer infrastructure is ready, in which case
+        * calling get_timer_us() before the (empty) loop could cause
+        * a divide-by-0 or otherwise crash the system. No clients
+        * should be registered before the timer infrastructure is up,
+        * so the check for the list being empty should be
+        * ok. Otherwise, we would need a new GD_FLG_TIMERS_READY
+        * flag.
+        */
+       if (hlist_empty(cyclic_get_list()))
+                return;
+
        /* Prevent recursion */
        if (gd->flags & GD_FLG_CYCLIC_RUNNING)
                return;

Completely unrelated, but now that I see a new use of that
cyclic_get_list() helper, I think it's time to get rid of that and just
spell it &gd->cyclic_list. It was only needed back when gd was
volatile-qualified.

Rasmus

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Emanuele Ghidoli]

On 7/24/26 02:00, Marek Vasut wrote:
> On 7/23/26 10:41 PM, Emanuele Ghidoli wrote:
>>
>>
>> On 7/23/26 20:35, Marek Vasut wrote:
>>> On 7/23/26 4:53 PM, Emanuele Ghidoli wrote:
>>>
>>> Hello everyone,
>>>
>>>> I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
>>>> ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
>>>> is applied. Nothing is printed on the serial.
>>>>
>>>> I bisected the issue: booting the parent commit works fine, and
>>>> reverting this patch on top of current main also restores
>>>> boot. It's not yet obvious to me what in this change causes the
>>>> regression.
>>>>
>>>> Would it make sense to revert it until the root cause is found?
>>>
>>> Give Patrice a bit of time to analyze the problem.
>>>
>>
>> Hello,
>>
>> after the regression, get_timer_us() is called on every cyclic_run() call,
>> even when the cyclic list is empty. On imx7 this happens earlier than before,
>> when the timer has not yet been initialized: get_timer_us() calls get_tbclk(),
>> whose value is zero, and uses it as a divisor in tick_to_time_us(), leading to
>> a division by zero.
>>
>> I would fix this by ensuring get_tbclk() returns at least 1 on imx7, but we
>> could hide bugs in this way. However, this bug could also be present on other
>> architectures, so I would just guard against entering the cyclic loop by
>> checking whether the list is empty before doing anything else, so
>> get_timer_us() is called "at the same point in time" as before.
>> Something like:
>> if (hlist_empty(cyclic_get_list())) {
>>     gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
>>     return;
>> }
> It looks like there are two topics -- optimize the cyclic run and fix the MX7
> timer.
> 
> I agree the cyclic run should exit if the list is empty, but please wait for
> input from Rasmus on that.
> 
> Regarding the timer, how come the timer is initialized so late on MX7 ?
timer_init() called in initcall_run_f().>
> The Cortex-A7 should have its own ARM timer, that should be available right
> from the beginning. Is that ARM timer in use on your system, or does your
> system use GPT timer ?
iMX7 uses arch/arm/mach-imx/syscounter.c.

udelay() is called in initcall_run_f(), before timer_init().
arch_cpu_init() (arch/arm/mach-imx/mx7/soc.c) -> imx_gpcv2_init -> udelay
(which runs schedule/cyclic).

And this is interesting, udelay() before timer_init() leads to a 0 us delay,
so there is another bug.

I have verified that removing this udelay(65) the board boots.
> 
> Which board is this ?
board/toradex/colibri_imx7/
> 
> +CC Peng

Kind regards,
Emanuele

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Marek Vasut via U-Boot]

On 7/24/26 9:36 AM, Emanuele Ghidoli wrote:

Hello everyone,

>> Regarding the timer, how come the timer is initialized so late on MX7 ?
> timer_init() called in initcall_run_f().>
>> The Cortex-A7 should have its own ARM timer, that should be available right
>> from the beginning. Is that ARM timer in use on your system, or does your
>> system use GPT timer ?
> iMX7 uses arch/arm/mach-imx/syscounter.c.
> 
> udelay() is called in initcall_run_f(), before timer_init().
> arch_cpu_init() (arch/arm/mach-imx/mx7/soc.c) -> imx_gpcv2_init -> udelay
> (which runs schedule/cyclic).

Yikes.

> And this is interesting, udelay() before timer_init() leads to a 0 us delay,
> so there is another bug.
> 
> I have verified that removing this udelay(65) the board boots.
Can we start the syscounter sooner ? Something like this:

diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h 
b/arch/arm/include/asm/arch-mx6/sys_proto.h
index 7845fa8e569..5f6759a3306 100644
--- a/arch/arm/include/asm/arch-mx6/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
@@ -36,4 +36,6 @@ static inline void iomuxc_set_rgmii_io_voltage(int io_vol)
  	__raw_writel(io_vol, IOMUXC_SW_PAD_CTL_GRP_DDR_TYPE_RGMII);
  }

+void syscounter_init(void);
+
  #endif /* __SYS_PROTO_IMX6_ */
diff --git a/arch/arm/include/asm/arch-mx7/sys_proto.h 
b/arch/arm/include/asm/arch-mx7/sys_proto.h
index 5da0037b2c6..765764cf1bc 100644
--- a/arch/arm/include/asm/arch-mx7/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx7/sys_proto.h
@@ -11,4 +11,6 @@ struct wdog_regs;

  void set_wdog_reset(struct wdog_regs *wdog);

+void syscounter_init(void);
+
  #endif /* __SYS_PROTO_IMX7_ */
diff --git a/arch/arm/mach-imx/mx6/soc.c b/arch/arm/mach-imx/mx6/soc.c
index 02df86156d4..641b5d813ea 100644
--- a/arch/arm/mach-imx/mx6/soc.c
+++ b/arch/arm/mach-imx/mx6/soc.c
@@ -437,6 +437,8 @@ int arch_cpu_init(void)
  		 */
  		if (mxc_get_clock(MXC_ARM_CLK) == 396000000)
  			set_ahb_rate(132000000);
+	} else {
+		syscounter_init();
  	}

  	if (is_mx6ul()) {
diff --git a/arch/arm/mach-imx/mx7/soc.c b/arch/arm/mach-imx/mx7/soc.c
index e504c1fd52a..82d851b7817 100644
--- a/arch/arm/mach-imx/mx7/soc.c
+++ b/arch/arm/mach-imx/mx7/soc.c
@@ -331,6 +331,8 @@ int arch_cpu_init(void)

  	init_snvs();

+	syscounter_init();
+
  	imx_gpcv2_init();

  	enable_ca7_smp();
diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c
index 96fe2c7c17b..7b902540737 100644
--- a/arch/arm/mach-imx/syscounter.c
+++ b/arch/arm/mach-imx/syscounter.c
@@ -60,7 +60,7 @@ static inline unsigned long long us_to_tick(unsigned 
long long usec)
  }

  #if !CONFIG_IS_ENABLED(SKIP_LOWLEVEL_INIT) || IS_ENABLED(CONFIG_XPL_BUILD)
-int timer_init(void)
+void syscounter_init(void)
  {
  	struct sctr_regs *sctr = (struct sctr_regs *)SCTR_BASE_ADDR;
  	unsigned long val, freq;
@@ -80,6 +80,12 @@ int timer_init(void)
  	gd->arch.tbu = 0;

  	gd->arch.timer_rate_hz = freq;
+
+	return 0;
+}
+
+int timer_init(void)
+{
  	return 0;
  }
  #endif

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Emanuele Ghidoli]

On 7/26/26 08:41, Marek Vasut wrote:
> On 7/24/26 9:36 AM, Emanuele Ghidoli wrote:
> 
> Hello everyone,
> 
>>> Regarding the timer, how come the timer is initialized so late on MX7 ?
>> timer_init() called in initcall_run_f().>
>>> The Cortex-A7 should have its own ARM timer, that should be available right
>>> from the beginning. Is that ARM timer in use on your system, or does your
>>> system use GPT timer ?
>> iMX7 uses arch/arm/mach-imx/syscounter.c.
>>
>> udelay() is called in initcall_run_f(), before timer_init().
>> arch_cpu_init() (arch/arm/mach-imx/mx7/soc.c) -> imx_gpcv2_init -> udelay
>> (which runs schedule/cyclic).
> 
> Yikes.
> 
>> And this is interesting, udelay() before timer_init() leads to a 0 us delay,
>> so there is another bug.
>>
>> I have verified that removing this udelay(65) the board boots.
> Can we start the syscounter sooner ? Something like this:
> 
> diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/
> arch-mx6/sys_proto.h
> index 7845fa8e569..5f6759a3306 100644
> --- a/arch/arm/include/asm/arch-mx6/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
> @@ -36,4 +36,6 @@ static inline void iomuxc_set_rgmii_io_voltage(int io_vol)
>      __raw_writel(io_vol, IOMUXC_SW_PAD_CTL_GRP_DDR_TYPE_RGMII);
>  }
> 
> +void syscounter_init(void);
> +
>  #endif /* __SYS_PROTO_IMX6_ */
> diff --git a/arch/arm/include/asm/arch-mx7/sys_proto.h b/arch/arm/include/asm/
> arch-mx7/sys_proto.h
> index 5da0037b2c6..765764cf1bc 100644
> --- a/arch/arm/include/asm/arch-mx7/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx7/sys_proto.h
> @@ -11,4 +11,6 @@ struct wdog_regs;
> 
>  void set_wdog_reset(struct wdog_regs *wdog);
> 
> +void syscounter_init(void);
> +
>  #endif /* __SYS_PROTO_IMX7_ */
> diff --git a/arch/arm/mach-imx/mx6/soc.c b/arch/arm/mach-imx/mx6/soc.c
> index 02df86156d4..641b5d813ea 100644
> --- a/arch/arm/mach-imx/mx6/soc.c
> +++ b/arch/arm/mach-imx/mx6/soc.c
> @@ -437,6 +437,8 @@ int arch_cpu_init(void)
>           */
>          if (mxc_get_clock(MXC_ARM_CLK) == 396000000)
>              set_ahb_rate(132000000);
> +    } else {
> +        syscounter_init();
>      }
> 
>      if (is_mx6ul()) {
> diff --git a/arch/arm/mach-imx/mx7/soc.c b/arch/arm/mach-imx/mx7/soc.c
> index e504c1fd52a..82d851b7817 100644
> --- a/arch/arm/mach-imx/mx7/soc.c
> +++ b/arch/arm/mach-imx/mx7/soc.c
> @@ -331,6 +331,8 @@ int arch_cpu_init(void)
> 
>      init_snvs();
> 
> +    syscounter_init();
> +
>      imx_gpcv2_init();
> 
>      enable_ca7_smp();
> diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c
> index 96fe2c7c17b..7b902540737 100644
> --- a/arch/arm/mach-imx/syscounter.c
> +++ b/arch/arm/mach-imx/syscounter.c
> @@ -60,7 +60,7 @@ static inline unsigned long long us_to_tick(unsigned long
> long usec)
>  }
> 
>  #if !CONFIG_IS_ENABLED(SKIP_LOWLEVEL_INIT) || IS_ENABLED(CONFIG_XPL_BUILD)
> -int timer_init(void)
> +void syscounter_init(void)
>  {
>      struct sctr_regs *sctr = (struct sctr_regs *)SCTR_BASE_ADDR;
>      unsigned long val, freq;
> @@ -80,6 +80,12 @@ int timer_init(void)
>      gd->arch.tbu = 0;
> 
>      gd->arch.timer_rate_hz = freq;
> +
> +    return 0;
> +}
> +
> +int timer_init(void)
> +{
>      return 0;
>  }
>  #endif

Hello Marek,
thanks for the suggestion.

It works, build and tested on Colibri iMX7.

But imx6 is obviously broken:
/home/emanuele/tdx/u-boot-build/u-boot/arch/arm/mach-imx/mx6/soc.c:441:
undefined reference to `syscounter_init'

Emanuele

Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Marek Vasut via U-Boot]

On 7/28/26 11:46 AM, Emanuele Ghidoli wrote:

Hello Emanuele,

>> +void syscounter_init(void)
>>   {
>>       struct sctr_regs *sctr = (struct sctr_regs *)SCTR_BASE_ADDR;
>>       unsigned long val, freq;
>> @@ -80,6 +80,12 @@ int timer_init(void)
>>       gd->arch.tbu = 0;
>>
>>       gd->arch.timer_rate_hz = freq;
>> +
>> +    return 0;
>> +}
>> +
>> +int timer_init(void)
>> +{
>>       return 0;
>>   }
>>   #endif
> 
> Hello Marek,
> thanks for the suggestion.
> 
> It works, build and tested on Colibri iMX7.
> 
> But imx6 is obviously broken:
> /home/emanuele/tdx/u-boot-build/u-boot/arch/arm/mach-imx/mx6/soc.c:441:
> undefined reference to `syscounter_init'
Excellent, could you please submit this timer patch as a proper fix / 
patch ?

Thank you !