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

On 7/31/26 12:06 AM, Tom Rini wrote:
> On Thu, Jul 30, 2026 at 05:37:52PM +0200, Quentin Schulz wrote:
>> 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.
> 
> Right, and when it makes sense to and improves the readability of the
> overall code. If it not a must-do every time. There is a judgement call
> to 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?
> 
> The compiler benefit that using IS_ENABLED provides is that we will make
> sure that timer_init is declared in a header that is included. That's
> it. It can be useful for more generic code, but it's of course imperfect
> if the include chain brings it on some platforms, but not others (as a
> warning pointed out on IRC today reminded me).
> 

Yeah I'm not too sure of the benefit. We need to have timer_init() 
defined and thus may require "fallbacks" that are just empty stubs. 
Jonas had a look at doing size optimization for TPL for Rockchip a month 
ago and if i remember correctly, empty __weak stubs were actually 
costing a few bytes compared to simply not having one declared. See 
https://libera.catirclogs.org/linux-rockchip/2026-07-01 for some context.

>>> 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!).
> 
> I agree, at minimum, the commit message isn't clear that we're just
> replacing #ifdef with if (IS_ENABLED()) as a clean-up. I'll defer to
> Quentin on if that's worthwhile doing here, or not.
> 

It's fine for me, I just don't want to only receive that kind of patches 
as I don't find them particularly useful :)

Cheers,
Quentin