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

[Igor Grinberg]

On 11/07/12 23:36, Jon Hunter wrote:
> 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 at compulab.co.il>
>> Cc: Jon Hunter <jon-hunter at ti.com>
>> Cc: Santosh Shilimkar <santosh.shilimkar at ti.com>
>> Cc: Vaibhav Hiremath <hvaibhav at 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.

Well, I don't understand what do you mean by "any abstraction".
The purpose of defining those two was to eliminate multiple occurrences
of the string in the code, so for example if someone decides to change the
property string for some currently unknown reason - it will be easy and small.
Defines are a common practice for that, no?
If you still think it should be inlined, I will do.

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

I didn't want to add unused arguments to this function, so I've made a
wrapper below to have both the sync32k and the gp functions have the same
signature (argument list) and be called from a single macro.
Anyway, see below.

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

I've made this change, so we will not get something like:
omapx_gptimer_timer_init(), but this really does not meter to me,
so no problem will do for v2.

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

no, not really, just consider the explanation above.
I will remove the wrapper for v2.

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

Yes, you are right - this is odd.
What do you think of:

if (use_gptimer_clksrc) {
	omap2_gp_clocksource_init((clksrc_nr), clksrc_src);
	return;
}
omap2_##clksrc_name##_clocksource_init((clksrc_nr), clksrc_src);

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

There is no reliable way to determine which source should be used in runtime
for boards that do not have the 32k oscillator wired.

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

I agree for the 32k-sync as a default, but gptimer will not be selected
on SoC that have 32k while board does not have the 32k wired.

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

Well, this is more or less how it works today, but it does not consider
the board wiring information that after all defines which source should
be used. (Not all boards out there are clones of beagles and evms...)
And the generic code should be flexible enough
to enable any legal configuration.

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

Yes, but then we will not have the right description of the hardware
but IMHO workarounds on workarounds on...

There are several clock sources - all can be used,
why not have them described and ready for use?


-- 
Regards,
Igor.