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

On 8/6/2026 6:46 PM, Quentin Schulz wrote:
> 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.

Correct, an empty weak function will still cost 8 bytes on AArch64, most
likely branch and return instructions. And it seem to be similar for
a call to timer_init() in Rockchip TPL.

I have since then learned that if we use a '__weak void func(void);'
declaration without a default empty definition the entire operation is
optimized out at link time.

I even have one pending patch related to that TPL work that will drop
the default definition of the Rockchip TPL specific tpl_board_init() to
save those 8 bytes, and another one to make the timer_init() call
conditional on !IS_ENABLED(CONFIG_ARM64) to save 8 more bytes :-)

Regards,
Jonas

> 
>>>> 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