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

From: [email-masked] (Jon Hunter)
Date: Thu, 8 Nov 2012 11:39:57 -0600

On 11/08/2012 11:08 AM, Hiremath, Vaibhav wrote:
> On Thu, Nov 08, 2012 at 21:46:53, Hunter, Jon wrote:
>>
>> On 11/08/2012 01:59 AM, Igor Grinberg wrote:
>>> 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 <[email-masked]>
>>>>> Cc: Jon Hunter <[email-masked]>
>>>>> Cc: Santosh Shilimkar <[email-masked]>
>>>>> Cc: Vaibhav Hiremath <[email-masked]>
>>>>
>>>> [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.
>>
>> I understand your point, but right now I don't anticipate that we will
>> have many options here and so I think that we should drop these.
>>
>>>>>  #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.
>>
>> Ok.
>>
>>>>
>>>>>  {
>>>>>  	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.
>>
>> Thanks.
>>
>>>>
>>>>>  {
>>>>>  	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.
>>
>> Ok, thanks.
>>
>>>>
>>>>>  #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);
>>
>> Yes, but it still seems a little odd that we could have ...
>>
>>  if (use_gptimer_clksrc) {
>>  	omap2_gp_clocksource_init((clksrc_nr), clksrc_src);
>>  	return;
>>  }
>>  omap2_gp_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.
>>
>> Hmmm ... well for OMAP devices the 32kHz clock is mandatory AFAIK. At
>> least for OMAP devices and I would need to check on the AM33xx but I
>> would imagine they are the same. Which devices are you referring to
>> where the 32kHz is optional?
>>
> No we do not have 32k_counter block in AM335x.

I am painfully aware of that :-)

> If you are referring to 32Khz clock availability alone, then yes, we need to 
> get persistent clock and we use RTC 32Khz clock source for it. 
> But please note that this is not a 32k_counter block which OMAP family of 
> devices has.
> 
> The way AM335x works is, we have timers (0-7), timer0 being secure timer.
> We use timer1 and timer2 for clockevent and clocksource and they are driven 
> by 26MHz OSC clock currently. So when you go into Low power state, OSC is turned off and you need persistent clock for time keeping, right?
> 
> And only persistent clock available is RTC 32khz crystal clock, being RTC is 
> in wakeup domain.

I think you are missing the point here. For OMAP devices we have two
main external clock sources which are the 32kHz clock and the sys_clk
(can be a range of frequencies from 12-38.4MHz for OMAP4). The point is
for OMAP these clock sources are always present and AFAIK there is no
h/w configuration that allows you not to have the 32kHz clock source.
PRCM needs it and I think for OMAP1 the reset logic needs it (if memory
serves).

Igor was mentioning a h/w scenario where the 32kHz source is not
present. However, I am not sure which devices support this and is
applicable too.

So we are not discussing the 32k-sync-timer here. We are discussing
whether there are any device configurations where a 32k clock source
would not be available for the gptimer.

Jon