[PATCH v2 1/4] rockchip: spl: change call condition rockchip_stimer_init()

[PATCH v2 1/4] rockchip: spl: change call condition rockchip_stimer_init()

[Johan Jonker]

The Rockchip SoCs rk3066/rk3188 have no CONFIG_ROCKCHIP_STIMER_BASE
defined. Currently only rk3188 has an exception. Make this more
generic and call the function rockchip_stimer_init() only when
CONFIG_ROCKCHIP_STIMER_BASE is available.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---

Changed V2:
  use IS_ENABLED
  add include kconfig.h
  move define location so that rockchip_stimer_init() is always
  visible to the compiler
---
 arch/arm/mach-rockchip/spl.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c
index 02c40fb3..31558415 100644
--- a/arch/arm/mach-rockchip/spl.c
+++ b/arch/arm/mach-rockchip/spl.c
@@ -16,6 +16,7 @@
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <linux/bitops.h>
+#include <linux/kconfig.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -70,7 +71,6 @@ u32 spl_mmc_boot_mode(const u32 boot_device)
 	return MMCSD_MODE_RAW;
 }
 
-#if !defined(CONFIG_ROCKCHIP_RK3188)
 #define TIMER_LOAD_COUNT_L	0x00
 #define TIMER_LOAD_COUNT_H	0x04
 #define TIMER_CONTROL_REG	0x10
@@ -80,6 +80,7 @@ u32 spl_mmc_boot_mode(const u32 boot_device)
 
 __weak void rockchip_stimer_init(void)
 {
+#if defined(CONFIG_ROCKCHIP_STIMER_BASE)
 	/* If Timer already enabled, don't re-init it */
 	u32 reg = readl(CONFIG_ROCKCHIP_STIMER_BASE + TIMER_CONTROL_REG);
 
@@ -94,8 +95,8 @@ __weak void rockchip_stimer_init(void)
 	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
 	writel(TIMER_EN | TIMER_FMODE, CONFIG_ROCKCHIP_STIMER_BASE +
 	       TIMER_CONTROL_REG);
-}
 #endif
+}
 
 __weak int board_early_init_f(void)
 {
@@ -132,9 +133,11 @@ void board_init_f(ulong dummy)
 		hang();
 	}
 	arch_cpu_init();
-#if !defined(CONFIG_ROCKCHIP_RK3188)
-	rockchip_stimer_init();
-#endif
+
+	/* Init secure timer */
+	if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
+		rockchip_stimer_init();
+
 #ifdef CONFIG_SYS_ARCH_TIMER
 	/* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
 	timer_init();
-- 
2.20.1

[PATCH v2 2/4] rockchip: tpl: change call condition rockchip_stimer_init()

[Johan Jonker]

The Rockchip SoCs rk3066/rk3188 have no CONFIG_ROCKCHIP_STIMER_BASE
defined. Currently only rk3188 has an exception. Make this more
generic and call the function rockchip_stimer_init() only when
CONFIG_ROCKCHIP_STIMER_BASE is available.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---

Changed V2:
  use IS_ENABLED
  add include kconfig.h
  move define location so that rockchip_stimer_init() is always
  visible to the compiler
---
 arch/arm/mach-rockchip/tpl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
index 3c007bb4..5ce6e57c 100644
--- a/arch/arm/mach-rockchip/tpl.c
+++ b/arch/arm/mach-rockchip/tpl.c
@@ -15,6 +15,7 @@
 #include <asm/io.h>
 #include <asm/arch-rockchip/bootrom.h>
 #include <linux/bitops.h>
+#include <linux/kconfig.h>
 
 #if CONFIG_IS_ENABLED(BANNER_PRINT)
 #include <timestamp.h>
@@ -29,6 +30,7 @@
 
 __weak void rockchip_stimer_init(void)
 {
+#if defined(CONFIG_ROCKCHIP_STIMER_BASE)
 	/* If Timer already enabled, don't re-init it */
 	u32 reg = readl(CONFIG_ROCKCHIP_STIMER_BASE + TIMER_CONTROL_REG);
 
@@ -45,6 +47,7 @@ __weak void rockchip_stimer_init(void)
 	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
 	writel(TIMER_EN | TIMER_FMODE, CONFIG_ROCKCHIP_STIMER_BASE +
 	       TIMER_CONTROL_REG);
+#endif
 }
 
 void board_init_f(ulong dummy)
@@ -74,7 +77,9 @@ void board_init_f(ulong dummy)
 	}
 
 	/* Init secure timer */
-	rockchip_stimer_init();
+	if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
+		rockchip_stimer_init();
+
 	/* Init ARM arch timer in arch/arm/cpu/ */
 	timer_init();
 
-- 
2.20.1

[PATCH v2 3/4] 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>
---
 arch/arm/mach-rockchip/spl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c
index 31558415..c1df6ce2 100644
--- a/arch/arm/mach-rockchip/spl.c
+++ b/arch/arm/mach-rockchip/spl.c
@@ -138,10 +138,10 @@ void board_init_f(ulong dummy)
 	if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
 		rockchip_stimer_init();
 
-#ifdef CONFIG_SYS_ARCH_TIMER
-	/* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
-	timer_init();
-#endif
+	/* Init ARM arch timer */
+	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.20.1

[PATCH v2 4/4] rockchip: tpl: use 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.
Use the call condition IS_ENABLED to increase
build coverage and make the code easier to read.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 arch/arm/mach-rockchip/tpl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c
index 5ce6e57c..2729264b 100644
--- a/arch/arm/mach-rockchip/tpl.c
+++ b/arch/arm/mach-rockchip/tpl.c
@@ -80,8 +80,9 @@ void board_init_f(ulong dummy)
 	if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
 		rockchip_stimer_init();
 
-	/* Init ARM arch timer in arch/arm/cpu/ */
-	timer_init();
+	/* Init ARM arch timer */
+	if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
+		timer_init();
 
 	ret = uclass_get_device(UCLASS_RAM, 0, &dev);
 	if (ret) {
-- 
2.20.1

Re: [PATCH v2 1/4] rockchip: spl: change call condition rockchip_stimer_init()

[Johan Jonker]

Hi Simon Glass,

Sorry for the inconvenience, when I decompile "u-boot-spl"
rk3188 vs. rk3288 -> rock vs rock2

ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make rock_defconfig
[..]
ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make rock2_defconfig
ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make all


	/* Init secure timer */
	if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
		rockchip_stimer_init();

	/* Init ARM arch timer */
	if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
		timer_init();

For rk3288 both rockchip_stimer_init() and timer_init() DON'T show up in
decompiled source.

====

#if defined(CONFIG_ROCKCHIP_STIMER_BASE)
	/* Init secure timer */
	rockchip_stimer_init();
#endif

#ifdef CONFIG_SYS_ARCH_TIMER
	/* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
	timer_init();
#endif

The conversion of CONFIG_ROCKCHIP_STIMER_BASE to KCONFIG seems to work
with #ifdef.

====

Could someone confirm this behavior of IS_ENABLED() in combination with
TPL/SPL and please advise which other options/solutions we have?

(Goal is fit a rk3066 timer using both common TPL/SPL in a similar way
to rk3188)

Kind regards,

Johan Jonker
On 12/29/21 5:58 PM, Johan Jonker wrote:
> The Rockchip SoCs rk3066/rk3188 have no CONFIG_ROCKCHIP_STIMER_BASE
> defined. Currently only rk3188 has an exception. Make this more
> generic and call the function rockchip_stimer_init() only when
> CONFIG_ROCKCHIP_STIMER_BASE is available.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
> 
> Changed V2:
>   use IS_ENABLED
>   add include kconfig.h
>   move define location so that rockchip_stimer_init() is always
>   visible to the compiler
> ---
>  arch/arm/mach-rockchip/spl.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c
> index 02c40fb3..31558415 100644
> --- a/arch/arm/mach-rockchip/spl.c
> +++ b/arch/arm/mach-rockchip/spl.c
> @@ -16,6 +16,7 @@
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <linux/bitops.h>
> +#include <linux/kconfig.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -70,7 +71,6 @@ u32 spl_mmc_boot_mode(const u32 boot_device)
>  	return MMCSD_MODE_RAW;
>  }
>  
> -#if !defined(CONFIG_ROCKCHIP_RK3188)
>  #define TIMER_LOAD_COUNT_L	0x00
>  #define TIMER_LOAD_COUNT_H	0x04
>  #define TIMER_CONTROL_REG	0x10
> @@ -80,6 +80,7 @@ u32 spl_mmc_boot_mode(const u32 boot_device)
>  
>  __weak void rockchip_stimer_init(void)
>  {
> +#if defined(CONFIG_ROCKCHIP_STIMER_BASE)
>  	/* If Timer already enabled, don't re-init it */
>  	u32 reg = readl(CONFIG_ROCKCHIP_STIMER_BASE + TIMER_CONTROL_REG);
>  
> @@ -94,8 +95,8 @@ __weak void rockchip_stimer_init(void)
>  	writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
>  	writel(TIMER_EN | TIMER_FMODE, CONFIG_ROCKCHIP_STIMER_BASE +
>  	       TIMER_CONTROL_REG);
> -}
>  #endif
> +}
>  
>  __weak int board_early_init_f(void)
>  {
> @@ -132,9 +133,11 @@ void board_init_f(ulong dummy)
>  		hang();
>  	}
>  	arch_cpu_init();
> -#if !defined(CONFIG_ROCKCHIP_RK3188)
> -	rockchip_stimer_init();
> -#endif
> +
> +	/* Init secure timer */
> +	if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
> +		rockchip_stimer_init();
> +
>  #ifdef CONFIG_SYS_ARCH_TIMER
>  	/* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
>  	timer_init();
> 

Re: [PATCH v2 1/4] rockchip: spl: change call condition rockchip_stimer_init()

[Simon Glass]

Hi Johan,

On Wed, 29 Dec 2021 at 13:58, Johan Jonker <jbx6244@gmail.com> wrote:
>
> Hi Simon Glass,
>
> Sorry for the inconvenience, when I decompile "u-boot-spl"
> rk3188 vs. rk3288 -> rock vs rock2
>
> ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make rock_defconfig
> [..]
> ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make rock2_defconfig
> ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make all
>
>
>         /* Init secure timer */
>         if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
>                 rockchip_stimer_init();
>
>         /* Init ARM arch timer */
>         if (IS_ENABLED(CONFIG_SYS_ARCH_TIMER))
>                 timer_init();
>
> For rk3288 both rockchip_stimer_init() and timer_init() DON'T show up in
> decompiled source.
>
> ====
>
> #if defined(CONFIG_ROCKCHIP_STIMER_BASE)
>         /* Init secure timer */
>         rockchip_stimer_init();
> #endif
>
> #ifdef CONFIG_SYS_ARCH_TIMER
>         /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
>         timer_init();
> #endif
>
> The conversion of CONFIG_ROCKCHIP_STIMER_BASE to KCONFIG seems to work
> with #ifdef.
>
> ====
>
> Could someone confirm this behavior of IS_ENABLED() in combination with
> TPL/SPL and please advise which other options/solutions we have?
>
> (Goal is fit a rk3066 timer using both common TPL/SPL in a similar way
> to rk3188)

Oh, you need a bool option, like CONFIG_ROCKCHIP_HAS_STIMER and make
your _BASE option depend on that.

Regards,
Simon


>
> Kind regards,
>
> Johan Jonker
> On 12/29/21 5:58 PM, Johan Jonker wrote:
> > The Rockchip SoCs rk3066/rk3188 have no CONFIG_ROCKCHIP_STIMER_BASE
> > defined. Currently only rk3188 has an exception. Make this more
> > generic and call the function rockchip_stimer_init() only when
> > CONFIG_ROCKCHIP_STIMER_BASE is available.
> >
> > Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> > ---
> >
> > Changed V2:
> >   use IS_ENABLED
> >   add include kconfig.h
> >   move define location so that rockchip_stimer_init() is always
> >   visible to the compiler
> > ---
> >  arch/arm/mach-rockchip/spl.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c
> > index 02c40fb3..31558415 100644
> > --- a/arch/arm/mach-rockchip/spl.c
> > +++ b/arch/arm/mach-rockchip/spl.c
> > @@ -16,6 +16,7 @@
> >  #include <asm/global_data.h>
> >  #include <asm/io.h>
> >  #include <linux/bitops.h>
> > +#include <linux/kconfig.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -70,7 +71,6 @@ u32 spl_mmc_boot_mode(const u32 boot_device)
> >       return MMCSD_MODE_RAW;
> >  }
> >
> > -#if !defined(CONFIG_ROCKCHIP_RK3188)
> >  #define TIMER_LOAD_COUNT_L   0x00
> >  #define TIMER_LOAD_COUNT_H   0x04
> >  #define TIMER_CONTROL_REG    0x10
> > @@ -80,6 +80,7 @@ u32 spl_mmc_boot_mode(const u32 boot_device)
> >
> >  __weak void rockchip_stimer_init(void)
> >  {
> > +#if defined(CONFIG_ROCKCHIP_STIMER_BASE)
> >       /* If Timer already enabled, don't re-init it */
> >       u32 reg = readl(CONFIG_ROCKCHIP_STIMER_BASE + TIMER_CONTROL_REG);
> >
> > @@ -94,8 +95,8 @@ __weak void rockchip_stimer_init(void)
> >       writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
> >       writel(TIMER_EN | TIMER_FMODE, CONFIG_ROCKCHIP_STIMER_BASE +
> >              TIMER_CONTROL_REG);
> > -}
> >  #endif
> > +}
> >
> >  __weak int board_early_init_f(void)
> >  {
> > @@ -132,9 +133,11 @@ void board_init_f(ulong dummy)
> >               hang();
> >       }
> >       arch_cpu_init();
> > -#if !defined(CONFIG_ROCKCHIP_RK3188)
> > -     rockchip_stimer_init();
> > -#endif
> > +
> > +     /* Init secure timer */
> > +     if (IS_ENABLED(CONFIG_ROCKCHIP_STIMER_BASE))
> > +             rockchip_stimer_init();
> > +
> >  #ifdef CONFIG_SYS_ARCH_TIMER
> >       /* Init ARM arch timer in arch/arm/cpu/armv7/arch_timer.c */
> >       timer_init();
> >

Re: [PATCH v2 2/4] rockchip: tpl: change call condition rockchip_stimer_init()

[Simon Glass]

On Wed, 29 Dec 2021 at 09:59, Johan Jonker <jbx6244@gmail.com> wrote:
>
> The Rockchip SoCs rk3066/rk3188 have no CONFIG_ROCKCHIP_STIMER_BASE
> defined. Currently only rk3188 has an exception. Make this more
> generic and call the function rockchip_stimer_init() only when
> CONFIG_ROCKCHIP_STIMER_BASE is available.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>
> Changed V2:
>   use IS_ENABLED
>   add include kconfig.h
>   move define location so that rockchip_stimer_init() is always
>   visible to the compiler
> ---
>  arch/arm/mach-rockchip/tpl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>