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

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

[Johan Jonker]

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 <jbx6244@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Previous version not needed for serie, so resend separate.
https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/

Changed:
rebase
remove text
---
 arch/arm/mach-rockchip/spl.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c
index e989c148079a..894c24608e64 100644
--- a/arch/arm/mach-rockchip/spl.c
+++ b/arch/arm/mach-rockchip/spl.c
@@ -122,10 +122,9 @@ void board_init_f(ulong dummy)

 	rockchip_stimer_init();

-#ifdef CONFIG_SYS_ARCH_TIMER
-	/* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
-	timer_init();
-#endif
+	if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
+		timer_init();
+
 #if !defined(CONFIG_TPL) || defined(CONFIG_SPL_RAM)
 	debug("\nspl:init dram\n");
 	ret = dram_init();
--
2.39.5

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

[Quentin Schulz]

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 <jbx6244@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Previous version not needed for serie, so resend separate.
> https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
> 

You didn't answer Kever's question in the linked patch and I have the 
same question.

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

Cheers,
Quentin

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

[Johan Jonker]

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 <jbx6244@gmail.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Previous version not needed for serie, so resend separate.
>> https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
>>
> 

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/20220403230659.12039-6-jbx6244@gmail.com/

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.
There is even a warning for it in ./scripts/checkpatch.pl

============
__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.


Johan


> 
> Cheers,
> Quentin

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

[Quentin Schulz]

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 <jbx6244@gmail.com>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Previous version not needed for serie, so resend separate.
>>> https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
>>>
>>
> 
> 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/20220403230659.12039-6-jbx6244@gmail.com/
> 

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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4960 bytes --]

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 <jbx6244@gmail.com>
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > > 
> > > > Previous version not needed for serie, so resend separate.
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
> > > > 
> > > 
> > 
> > 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/20220403230659.12039-6-jbx6244@gmail.com/
> > 
> 
> 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).

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Quentin Schulz]

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 <jbx6244@gmail.com>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Previous version not needed for serie, so resend separate.
>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
>>>>>
>>>>
>>>
>>> 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/20220403230659.12039-6-jbx6244@gmail.com/
>>>
>>
>> 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

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

[Jonas Karlman]

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 <jbx6244@gmail.com>
>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Previous version not needed for serie, so resend separate.
>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20220403230659.12039-5-jbx6244@gmail.com/
>>>>>>
>>>>>
>>>>
>>>> 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/20220403230659.12039-6-jbx6244@gmail.com/
>>>>
>>>
>>> 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