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 <grinberg@xxxxxxxxxxxxxx>
>>> Cc: Jon Hunter <jon-hunter@xxxxxx>
>>> Cc: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
>>> Cc: Vaibhav Hiremath <hvaibhav@xxxxxx>
>>
>> [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?
>> 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.
Ok, again let me know which device(s) this applies too.
>>
>> 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.
My whole thought here was that the 32kHz is always present. If that is
not the case then I would agree this would not work.
>>
>>> }
>>>
>>> -#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?
Well we really want to simplify this code and so I was thinking that if
a device has a 32k-sync timer AND there is a 32kHz source, then what's
the point in having an option to use a gptimer with a 32kHz source for
that device? I guess I don't see the benefit there, at least for OMAP2-5
devices specifically.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
