Hi Johan,
On 7/24/26 11:53 AM, Johan Jonker wrote:
On 7/24/26 10:45, Quentin Schulz wrote:
Hi Johan,
On 7/24/26 12:17 AM, Johan Jonker wrote:
Not all Rockchip SoC models use the ARM arch timer.
Call the function timer_init() only when
CONFIG_SYS_ARCH_TIMER is available.
Replace the ifdef call condition by IS_ENABLED
to increase build coverage and make the code easier to read.
Signed-off-by: Johan Jonker <[email protected]>
Reviewed-by: Simon Glass <[email protected]>
---
Previous version not needed for serie, so resend separate.
https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/
Hi Quentin,
You didn't answer Kever's question in the linked patch and I have the same
question.
Yes we end up the same code. But...
This is essentially the same code, so what's the benefit, are you trying to fix
a specific issue? How does this improve the situation?
> How does doing that increase code coverage... etc :)
This patch originates around the time this concept as introduced.
We are changing all code to the new norm and we leave this as it is...
Fix this as well as a favor to Simon as part of the review. As we are there
then fix them all as this is the new norm.
https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/
This doesn't point at what Simon could have said that prompted this
patch. The pointed patch did actually fix something, and instead of using
#ifdef CONFIG_SYS_ARCH_TIMER
you used
if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
which is absolutely the correct and "modern" way of doing it.
The concept:
Currently with #ifdef the compiler sees this code:
=============
rockchip_stimer_init();
ret = dram_init();
=============
Now the compiler sees this code:
int timer_init(void)
{
gd->arch.tbl = 0;
gd->arch.tbu = 0;
#ifdef CFG_SYS_HZ_CLOCK
gd->arch.timer_rate_hz = CFG_SYS_HZ_CLOCK;
#else
gd->arch.timer_rate_hz = read_cntfrq();
#endif
return 0;
}
rockchip_stimer_init();
if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
timer_init();
ret = dram_init();
============
By using IS_ENABLED and CONFIG_IS_ENABLED the compiler is able to look further
into code and catch possible errors or warnings.
I don't know anything about compilers but I'm surprised this would
actually do anything different than what we currently have.
If CONFIG_SYS_ARCH_TIMER is not set, then you get
if (0)
timer_init();
which the compiler will (hopefully) see as a non-reachable branch and
discard it.
Otherwise, it'll be:
if (1)
timer_init();
which hopefully the compiler will simply replace without the branch:
timer_init();
Maybe Simon or someone else can teach me something here because my naive
view on this is: does not make a difference. What kind of benefits do we
have, what do you run to see those benefits?
There is even a warning for it in ./scripts/checkpatch.pl
I'm aware, I quite often trigger it :)
============
__weak void rockchip_stimer_init(void)
{
#if defined(CONFIG_ROCKCHIP_STIMER_BASE)
#endif
}
============
There are exceptions like in rockchip_stimer_init where certain defines are
missing, so that's still allowed.
In all other settings we use IS_ENABLED and CONFIG_IS_ENABLED.
Hope that explains your questions.
Not really no, sorry.
The commit log is misleading and needs rewording. As far as my
understanding goes, it's clean-up. Maybe there's something helpful for
the compiler but you need to prove it because I don't see it (I'm
interested to know if it does, so please tell us!).
Cheers,
Quentin
