Re: [PATCH] ARM: OMAP2+: timer: remove CONFIG_OMAP_32K_TIMER

[Jon Hunter <jon-hunter@ti.com>]

Hi Igor,

On 11/07/2012 08:42 AM, Igor Grinberg wrote:
> CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way.
> Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER
> setting.
> To remove the dependancy, several conversions/additions had to be done:
> 1) Timer structures and initialization functions are named by the platform
>    name and the clock source in use. The decision which timer is
>    used is done statically from the machine_desc structure. In the
>    future it should come from DT.
> 2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into
>    separate timer structures along with the timer init functions.
>    This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
> 3) Since we have all the timers defined inside machine_desc structure
>    and we no longer need the fallback to gp_timer clock source in case
>    32k_timer clock source is unavailable (namely on AM33xx), we no
>    longer need the #ifdef around __omap2_sync32k_clocksource_init()
>    function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the
>    __omap2_sync32k_clocksource_init() function.
> 
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Vaibhav Hiremath <hvaibhav@ti.com>

[snip]

> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 684d2fc..a4ad7a0 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -63,20 +63,8 @@
>  #define OMAP2_32K_SOURCE	"func_32k_ck"
>  #define OMAP3_32K_SOURCE	"omap_32k_fck"
>  #define OMAP4_32K_SOURCE	"sys_32k_ck"
> -
> -#ifdef CONFIG_OMAP_32K_TIMER
> -#define OMAP2_CLKEV_SOURCE	OMAP2_32K_SOURCE
> -#define OMAP3_CLKEV_SOURCE	OMAP3_32K_SOURCE
> -#define OMAP4_CLKEV_SOURCE	OMAP4_32K_SOURCE
> -#define OMAP3_SECURE_TIMER	12
>  #define TIMER_PROP_SECURE	"ti,timer-secure"
> -#else
> -#define OMAP2_CLKEV_SOURCE	OMAP2_MPU_SOURCE
> -#define OMAP3_CLKEV_SOURCE	OMAP3_MPU_SOURCE
> -#define OMAP4_CLKEV_SOURCE	OMAP4_MPU_SOURCE
> -#define OMAP3_SECURE_TIMER	1
> -#define TIMER_PROP_SECURE	"ti,timer-alwon"
> -#endif
> +#define TIMER_PROP_ALWON	"ti,timer-alwon"

Nit-pick, can we drop the TIMER_PROP_SECURE/ALWON and use the
"ti,timer-secure" and "ti,timer-alwon" directly?

Initially, I also defined TIMER_PROP_ALWON and Rob Herring's feedback
was to drop this and use the property string directly to remove any
abstraction.

>  #define REALTIME_COUNTER_BASE				0x48243200
>  #define INCREMENTER_NUMERATOR_OFFSET			0x10
> @@ -216,7 +204,7 @@ void __init omap_dmtimer_init(void)
>  
>  	/* If we are a secure device, remove any secure timer nodes */
>  	if ((omap_type() != OMAP2_DEVICE_TYPE_GP)) {
> -		np = omap_get_timer_dt(omap_timer_match, "ti,timer-secure");
> +		np = omap_get_timer_dt(omap_timer_match, TIMER_PROP_SECURE);
>  		if (np)
>  			of_node_put(np);
>  	}
> @@ -378,9 +366,8 @@ static u32 notrace dmtimer_read_sched_clock(void)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_OMAP_32K_TIMER
>  /* Setup free-running counter for clocksource */
> -static int __init omap2_sync32k_clocksource_init(void)
> +static int __init __omap2_sync32k_clocksource_init(void)

Not sure I follow why you renamed this function here ...

>  {
>  	int ret;
>  	struct device_node *np = NULL;
> @@ -439,15 +426,9 @@ static int __init omap2_sync32k_clocksource_init(void)
>  
>  	return ret;
>  }
> -#else
> -static inline int omap2_sync32k_clocksource_init(void)
> -{
> -	return -ENODEV;
> -}
> -#endif
>  
> -static void __init omap2_gptimer_clocksource_init(int gptimer_id,
> -						const char *fck_source)
> +static void __init omap2_gp_clocksource_init(int gptimer_id,
> +					     const char *fck_source)

Nit, I personally prefer keeping gptimer, because gp just means
"general-purpose" and does not imply a timer per-se.

>  {
>  	int res;
>  
> @@ -466,23 +447,10 @@ static void __init omap2_gptimer_clocksource_init(int gptimer_id,
>  			gptimer_id, clksrc.rate);
>  }
>  
> -static void __init omap2_clocksource_init(int gptimer_id,
> -						const char *fck_source)
> +static void __init omap2_sync32k_clocksource_init(int gptimer_id,
> +						  const char *fck_source)
>  {
> -	/*
> -	 * First give preference to kernel parameter configuration
> -	 * by user (clocksource="gp_timer").
> -	 *
> -	 * In case of missing kernel parameter for clocksource,
> -	 * first check for availability for 32k-sync timer, in case
> -	 * of failure in finding 32k_counter module or registering
> -	 * it as clocksource, execution will fallback to gp-timer.
> -	 */
> -	if (use_gptimer_clksrc == true)
> -		omap2_gptimer_clocksource_init(gptimer_id, fck_source);
> -	else if (omap2_sync32k_clocksource_init())
> -		/* Fall back to gp-timer code */
> -		omap2_gptimer_clocksource_init(gptimer_id, fck_source);
> +	__omap2_sync32k_clocksource_init();
>  }

... this just appears to be a wrapper function, but I don't see why this
is needed? Do we need this wrapper?

>  #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
> @@ -563,52 +531,64 @@ static inline void __init realtime_counter_init(void)
>  {}
>  #endif
>  
> -#define OMAP_SYS_TIMER_INIT(name, clkev_nr, clkev_src, clkev_prop,	\
> -				clksrc_nr, clksrc_src)			\
> -static void __init omap##name##_timer_init(void)			\
> +#define OMAP_SYS_TIMER_INIT(n, clksrc_name, clkev_nr, clkev_src,	\
> +				clkev_prop, clksrc_nr, clksrc_src)	\
> +static void __init omap##n##_##clksrc_name##_timer_init(void)		\


>  {									\
>  	omap_dmtimer_init();						\
>  	omap2_gp_clockevent_init((clkev_nr), clkev_src, clkev_prop);	\
> -	omap2_clocksource_init((clksrc_nr), clksrc_src);		\
> +									\
> +	if (use_gptimer_clksrc)						\
> +		omap2_gp_clocksource_init((clksrc_nr), clksrc_src);	\
> +	else								\
> +		omap2_##clksrc_name##_clocksource_init((clksrc_nr),	\
> +						       clksrc_src);	\

Something here seems a little odd. If "clksrc_name" is "gp", then the
if-else parts will call the same function. Or am I missing something here?

I think that I prefer how it works today where we call just
omap2_clocksource_init(), and it determines whether to use the gptimer
or the 32k-sync.

For OMAP I think that it is fine to default to the 32k-sync and then if
the gptimer is selected, it uses the higher frequency sys_clk as the
timer source.

For AMxxx, devices, sync-32k does not exist, and so I understand it does
not work the same.

I am wondering if the use_gptimer_clksrc, should become
use_sysclk_clksrc, and then ...

For OMAP ...
use_sysclk_clksrc = 0 --> use sync-32k (default)
use_sysclk_clksrc = 1 --> use gptimer with sys_clk

For AM33xx ...
use_sysclk_clksrc = 0 --> use gptimer with 32khz clock (default)
use_sysclk_clksrc = 1 --> use gptimer with sys_clk

>  }
>  
> -#define OMAP_SYS_TIMER(name)						\
> -struct sys_timer omap##name##_timer = {					\
> -	.init	= omap##name##_timer_init,				\
> -};
> +#define OMAP_SYS_TIMER(n, clksrc)					\
> +struct sys_timer omap##n##_##clksrc##_timer = {				\
> +	.init	= omap##n##_##clksrc##_timer_init,			\
> +}
>  
>  #ifdef CONFIG_ARCH_OMAP2
> -OMAP_SYS_TIMER_INIT(2, 1, OMAP2_CLKEV_SOURCE, "ti,timer-alwon",
> -		    2, OMAP2_MPU_SOURCE)
> -OMAP_SYS_TIMER(2)
> +OMAP_SYS_TIMER_INIT(2, sync32k, 1, OMAP2_32K_SOURCE, TIMER_PROP_ALWON,
> +		    2, OMAP2_MPU_SOURCE);
> +OMAP_SYS_TIMER(2, sync32k);
> +OMAP_SYS_TIMER_INIT(2, gp, 1, OMAP2_MPU_SOURCE, TIMER_PROP_ALWON,
> +		    2, OMAP2_MPU_SOURCE);
> +OMAP_SYS_TIMER(2, gp);

It would be good if we can avoid having two timer_init functions for
each OMAP generation.

Cheers
Jon