Re: [PATCH v1] rockchip: spl: replace ifdef by IS_ENABLED for timer_init() call condition

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