| Message ID | 1352299344-8011-1-git-send-email-grinberg@compulab.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Igor Grinberg <grinberg@compulab.co.il> [121107 06:44]: > 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. I think this should be the default for the timers as that counter does not stop during deeper idle states. > 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> > --- > Finally I'm sending this out... > I've lost following Tony's branches and deciding which one to base on, > so I used linux-omap/master as a base for the patch. > Tony, tell me if you want it based on some other branch. > This has been compile tested on omap1|2plus_defconfig only. Yes sorry it's been a bit crazy with branches to get this header clean up done.. If it applies to master it should be easy to apply on others. > --- a/arch/arm/mach-omap2/board-2430sdp.c > +++ b/arch/arm/mach-omap2/board-2430sdp.c > @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") > .handle_irq = omap2_intc_handle_irq, > .init_machine = omap_2430sdp_init, > .init_late = omap2430_init_late, > - .timer = &omap2_timer, > + .timer = &omap2_sync32k_timer, > .restart = omap_prcm_restart, > MACHINE_END > --- a/arch/arm/mach-omap2/board-3430sdp.c > +++ b/arch/arm/mach-omap2/board-3430sdp.c > @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") > .handle_irq = omap3_intc_handle_irq, > .init_machine = omap_3430sdp_init, > .init_late = omap3430_init_late, > - .timer = &omap3_timer, > + .timer = &omap3_sync32k_timer, > .restart = omap_prcm_restart, > MACHINE_END ... Can't we assume that the default timer is omap[234]_sync32k_timer to avoid renaming the timer entries in all the board files? Then we just need a new timer entries for the hardware that does not have the sycn32k_timer available? Regards, Tony
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
On 11/07/12 19:33, Tony Lindgren wrote: > * Igor Grinberg <grinberg@compulab.co.il> [121107 06:44]: >> 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. > > I think this should be the default for the timers as that counter > does not stop during deeper idle states. Well, it is the default as you can see from the patch. The problem is that for boards that for some reason do not have the 32k wired and rely on MPU/GP timer source, the default will not work and currently there is no way for board to specify which timer source it can use. We have discussed this in San Diego (remember?) and you actually proposed this way as a solution. Well, may be I took it a bit further than you thought, but this is because the board code cannot know which timer source should be used at runtime and the fall back described below, does not work. > >> 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> >> --- >> Finally I'm sending this out... >> I've lost following Tony's branches and deciding which one to base on, >> so I used linux-omap/master as a base for the patch. >> Tony, tell me if you want it based on some other branch. >> This has been compile tested on omap1|2plus_defconfig only. > > Yes sorry it's been a bit crazy with branches to get this > header clean up done.. If it applies to master it should > be easy to apply on others. No problem ;-) > >> --- a/arch/arm/mach-omap2/board-2430sdp.c >> +++ b/arch/arm/mach-omap2/board-2430sdp.c >> @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") >> .handle_irq = omap2_intc_handle_irq, >> .init_machine = omap_2430sdp_init, >> .init_late = omap2430_init_late, >> - .timer = &omap2_timer, >> + .timer = &omap2_sync32k_timer, >> .restart = omap_prcm_restart, >> MACHINE_END >> --- a/arch/arm/mach-omap2/board-3430sdp.c >> +++ b/arch/arm/mach-omap2/board-3430sdp.c >> @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") >> .handle_irq = omap3_intc_handle_irq, >> .init_machine = omap_3430sdp_init, >> .init_late = omap3430_init_late, >> - .timer = &omap3_timer, >> + .timer = &omap3_sync32k_timer, >> .restart = omap_prcm_restart, >> MACHINE_END > ... > > Can't we assume that the default timer is omap[234]_sync32k_timer to > avoid renaming the timer entries in all the board files? Hmmm... How will this work with the macros defining the sys_timer structure? I would also not want to hide the exact timer used under the default name. > > Then we just need a new timer entries for the hardware that does > not have the sycn32k_timer available? Well, I tried to make it small patch just for the hardware that needs it, but I always found some corner case where, IMHO, this does not work/look good.
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@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. 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?
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@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. > > 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
* Igor Grinberg <grinberg@compulab.co.il> [121107 23:15]: > On 11/07/12 19:33, Tony Lindgren wrote: > > > > I think this should be the default for the timers as that counter > > does not stop during deeper idle states. > > Well, it is the default as you can see from the patch. > The problem is that for boards that for some reason do not have > the 32k wired and rely on MPU/GP timer source, the default will not work > and currently there is no way for board to specify which timer source > it can use. Yes. I was just wondering if we can avoid patching all the board files by doing it the other way around by introducing a new omap_gp_timer rather than renaming all the existing ones? > We have discussed this in San Diego (remember?) and you actually proposed > this way as a solution. Well, may be I took it a bit further than you > thought, but this is because the board code cannot know which timer source > should be used at runtime and the fall back described below, does not work. Yes thanks I agree we should get rid of that Kconfig option for sure. > >> --- a/arch/arm/mach-omap2/board-2430sdp.c > >> +++ b/arch/arm/mach-omap2/board-2430sdp.c > >> @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") > >> .handle_irq = omap2_intc_handle_irq, > >> .init_machine = omap_2430sdp_init, > >> .init_late = omap2430_init_late, > >> - .timer = &omap2_timer, > >> + .timer = &omap2_sync32k_timer, > >> .restart = omap_prcm_restart, > >> MACHINE_END > >> --- a/arch/arm/mach-omap2/board-3430sdp.c > >> +++ b/arch/arm/mach-omap2/board-3430sdp.c > >> @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") > >> .handle_irq = omap3_intc_handle_irq, > >> .init_machine = omap_3430sdp_init, > >> .init_late = omap3430_init_late, > >> - .timer = &omap3_timer, > >> + .timer = &omap3_sync32k_timer, > >> .restart = omap_prcm_restart, > >> MACHINE_END > > ... > > > > Can't we assume that the default timer is omap[234]_sync32k_timer to > > avoid renaming the timer entries in all the board files? > > Hmmm... > How will this work with the macros defining the sys_timer structure? > I would also not want to hide the exact timer used under the default name. Can't you just add a new sys_timer (or a new macro) for GP only setups? > > Then we just need a new timer entries for the hardware that does > > not have the sycn32k_timer available? > > Well, I tried to make it small patch just for the hardware that needs it, > but I always found some corner case where, IMHO, this does not work/look good. Can you explain a bit further? I guess what I'm after is just to avoid renaming the existing timers in the board-*.c files and only rename the ones that need gp timer only. Regards, Tony
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 <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. > > > > 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. 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. Thanks, Vaibhav > >> 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 > >
On Thu, Nov 08, 2012 at 21:50:05, Tony Lindgren wrote: > * Igor Grinberg <grinberg@compulab.co.il> [121107 23:15]: > > On 11/07/12 19:33, Tony Lindgren wrote: > > > > > > I think this should be the default for the timers as that counter > > > does not stop during deeper idle states. > > > > Well, it is the default as you can see from the patch. > > The problem is that for boards that for some reason do not have > > the 32k wired and rely on MPU/GP timer source, the default will not work > > and currently there is no way for board to specify which timer source > > it can use. > > Yes. I was just wondering if we can avoid patching all the board > files by doing it the other way around by introducing a new > omap_gp_timer rather than renaming all the existing ones? > > > We have discussed this in San Diego (remember?) and you actually proposed > > this way as a solution. Well, may be I took it a bit further than you > > thought, but this is because the board code cannot know which timer source > > should be used at runtime and the fall back described below, does not work. > > Yes thanks I agree we should get rid of that Kconfig option for sure. > > > >> --- a/arch/arm/mach-omap2/board-2430sdp.c > > >> +++ b/arch/arm/mach-omap2/board-2430sdp.c > > >> @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") > > >> .handle_irq = omap2_intc_handle_irq, > > >> .init_machine = omap_2430sdp_init, > > >> .init_late = omap2430_init_late, > > >> - .timer = &omap2_timer, > > >> + .timer = &omap2_sync32k_timer, > > >> .restart = omap_prcm_restart, > > >> MACHINE_END > > >> --- a/arch/arm/mach-omap2/board-3430sdp.c > > >> +++ b/arch/arm/mach-omap2/board-3430sdp.c > > >> @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") > > >> .handle_irq = omap3_intc_handle_irq, > > >> .init_machine = omap_3430sdp_init, > > >> .init_late = omap3430_init_late, > > >> - .timer = &omap3_timer, > > >> + .timer = &omap3_sync32k_timer, > > >> .restart = omap_prcm_restart, > > >> MACHINE_END > > > ... > > > > > > Can't we assume that the default timer is omap[234]_sync32k_timer to > > > avoid renaming the timer entries in all the board files? > > > > Hmmm... > > How will this work with the macros defining the sys_timer structure? > > I would also not want to hide the exact timer used under the default name. > > Can't you just add a new sys_timer (or a new macro) for GP only setups? > > > > Then we just need a new timer entries for the hardware that does > > > not have the sycn32k_timer available? > > > > Well, I tried to make it small patch just for the hardware that needs it, > > but I always found some corner case where, IMHO, this does not work/look good. > > Can you explain a bit further? > > I guess what I'm after is just to avoid renaming the existing > timers in the board-*.c files and only rename the ones that > need gp timer only. > That would be AM33xx family :) Thanks, Vaibhav > Regards, > > Tony >
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 <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. >>> >>> 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
On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: > > 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 <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. > >>> > >>> 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. > Exactly that is the point I am trying to make here, In case of AM33xx, it is certainly possible to build the system without this 32Khz clock. Interesting point here is, this 32KHz clock is external clock coming from crystal connected to in-built RTC module. Thanks, Vaibhav
On Thu, 8 Nov 2012, Jon Hunter wrote: > 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. Pretty sure Igor is referring to the AM3517/3505. This is very poorly documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 "Clock Specifications". - Paul
On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: > On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: [snip] >> 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. >> > > Exactly that is the point I am trying to make here, > > In case of AM33xx, it is certainly possible to build the system without > this 32Khz clock. > > Interesting point here is, this 32KHz clock is external clock coming from > crystal connected to in-built RTC module. Thanks. Looking at the AM3358 data manual it states ... "The OSC1 oscillator provides a 32.768-kHz reference clock to the real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT terminals. OSC1 is disabled by default after power is applied. This clock input is optional and may not be required if the RTC is configured to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or peripheral PLL (CLK_32KHZ) which receives a reference clock from the OSC0 input." So what is clear to me that an external 32k clock source is optional. However, it still appears that you will always have an internal 32k source and so regardless of the h/w configuration, a gptimer will always have an 32k clock available on the AM335x devices. Is that correct? Jon
On Thu, Nov 08, 2012 at 23:28:53, Hunter, Jon wrote: > > On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: > > On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: > > [snip] > > >> 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. > >> > > > > Exactly that is the point I am trying to make here, > > > > In case of AM33xx, it is certainly possible to build the system without > > this 32Khz clock. > > > > Interesting point here is, this 32KHz clock is external clock coming from > > crystal connected to in-built RTC module. > > Thanks. Looking at the AM3358 data manual it states ... > > "The OSC1 oscillator provides a 32.768-kHz reference clock to the > real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT > terminals. OSC1 is disabled by default after power is applied. This > clock input is optional and may not be required if the RTC is configured > to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or > peripheral PLL (CLK_32KHZ) which receives a reference clock from the > OSC0 input." > > So what is clear to me that an external 32k clock source is optional. > However, it still appears that you will always have an internal 32k > source and so regardless of the h/w configuration, a gptimer will always > have an 32k clock available on the AM335x devices. Is that correct? > Internal RC oscillator is not accurate at all, so not guaranteed to give accurate 32.768Hz clock. The oscillation band is from 16Khz to 64Khz. So it may not be useful as a system clocks, right? Thanks, Vaibhav
On 11/08/2012 11:58 AM, Paul Walmsley wrote: > On Thu, 8 Nov 2012, Jon Hunter wrote: > >> 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. > > Pretty sure Igor is referring to the AM3517/3505. This is very poorly > documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure > 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 > "Clock Specifications". Thanks Paul. But AFAICT, even in that h/w configuration the internal 32k oscillator will be used and so the gptimer will still have a 32k clock source. So I still don't see a use-case where the gptimer would not have a 32k source available. Admittedly, I could be missing one somewhere or I am just plain old wrong ... Cheers Jon
On 11/08/2012 12:06 PM, Hiremath, Vaibhav wrote: > On Thu, Nov 08, 2012 at 23:28:53, Hunter, Jon wrote: >> >> On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: >>> On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: >> >> [snip] >> >>>> 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. >>>> >>> >>> Exactly that is the point I am trying to make here, >>> >>> In case of AM33xx, it is certainly possible to build the system without >>> this 32Khz clock. >>> >>> Interesting point here is, this 32KHz clock is external clock coming from >>> crystal connected to in-built RTC module. >> >> Thanks. Looking at the AM3358 data manual it states ... >> >> "The OSC1 oscillator provides a 32.768-kHz reference clock to the >> real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT >> terminals. OSC1 is disabled by default after power is applied. This >> clock input is optional and may not be required if the RTC is configured >> to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or >> peripheral PLL (CLK_32KHZ) which receives a reference clock from the >> OSC0 input." >> >> So what is clear to me that an external 32k clock source is optional. >> However, it still appears that you will always have an internal 32k >> source and so regardless of the h/w configuration, a gptimer will always >> have an 32k clock available on the AM335x devices. Is that correct? >> > > Internal RC oscillator is not accurate at all, so not guaranteed to give > accurate 32.768Hz clock. The oscillation band is from 16Khz to 64Khz. > > So it may not be useful as a system clocks, right? So I admit I am not familiar with the details on the AMxxxx stuff side so much. So maybe I should ask this question ... Do you support a configuration where there is no external 32k clock AND the internal oscillator and hence, RTC are not used? I would have expected that you would always want the RTC running, but if you are saying that you don't require an external 32k and the internal 32k is not accurate, then I assume that having the RTC available is not mandatory either. Jon
On Thu, 8 Nov 2012, Jon Hunter wrote: > On 11/08/2012 11:58 AM, Paul Walmsley wrote: > > On Thu, 8 Nov 2012, Jon Hunter wrote: > > > >> 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. > > > > Pretty sure Igor is referring to the AM3517/3505. This is very poorly > > documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure > > 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 > > "Clock Specifications". > > But AFAICT, even in that h/w configuration the internal 32k > oscillator will be used Just to clarify, there's no internal 32k oscillator used on the 3517/3505; just a divider from the HF clock. > and so the gptimer will still have a 32k clock source. That's a good question and you might want to check with Igor on that one, the AM3517 TRM conflicts with the DM as to whether it's available to the GPTIMER or not :-( - Paul
On Thu, Nov 08, 2012 at 23:43:31, Hunter, Jon wrote: > > On 11/08/2012 12:06 PM, Hiremath, Vaibhav wrote: > > On Thu, Nov 08, 2012 at 23:28:53, Hunter, Jon wrote: > >> > >> On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: > >>> On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: > >> > >> [snip] > >> > >>>> 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. > >>>> > >>> > >>> Exactly that is the point I am trying to make here, > >>> > >>> In case of AM33xx, it is certainly possible to build the system without > >>> this 32Khz clock. > >>> > >>> Interesting point here is, this 32KHz clock is external clock coming from > >>> crystal connected to in-built RTC module. > >> > >> Thanks. Looking at the AM3358 data manual it states ... > >> > >> "The OSC1 oscillator provides a 32.768-kHz reference clock to the > >> real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT > >> terminals. OSC1 is disabled by default after power is applied. This > >> clock input is optional and may not be required if the RTC is configured > >> to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or > >> peripheral PLL (CLK_32KHZ) which receives a reference clock from the > >> OSC0 input." > >> > >> So what is clear to me that an external 32k clock source is optional. > >> However, it still appears that you will always have an internal 32k > >> source and so regardless of the h/w configuration, a gptimer will always > >> have an 32k clock available on the AM335x devices. Is that correct? > >> > > > > Internal RC oscillator is not accurate at all, so not guaranteed to give > > accurate 32.768Hz clock. The oscillation band is from 16Khz to 64Khz. > > > > So it may not be useful as a system clocks, right? > > So I admit I am not familiar with the details on the AMxxxx stuff side > so much. > > So maybe I should ask this question ... > > Do you support a configuration where there is no external 32k clock AND > the internal oscillator and hence, RTC are not used? > Why not, you could give-up on persistent time across suspend/resume and use OSC clock as a input clock to timer. And the timer code, which we have we have added fallback mechanism for this, First make sure that timer is properly set for RTC32K, if yes, then use it, and if no, then fall back to OSC clock. > I would have expected that you would always want the RTC running, but if > you are saying that you don't require an external 32k and the internal > 32k is not accurate, then I assume that having the RTC available is not > mandatory either. > Yes, it is not mandatory, considering fact that, you will not have persistent time and other low-power modes. Actually it depends on board/EVM use-case, right? Thanks, Vaibhav > Jon >
On 11/08/2012 12:17 PM, Paul Walmsley wrote: > On Thu, 8 Nov 2012, Jon Hunter wrote: > >> On 11/08/2012 11:58 AM, Paul Walmsley wrote: >>> On Thu, 8 Nov 2012, Jon Hunter wrote: >>> >>>> 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. >>> >>> Pretty sure Igor is referring to the AM3517/3505. This is very poorly >>> documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure >>> 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 >>> "Clock Specifications". >> >> But AFAICT, even in that h/w configuration the internal 32k >> oscillator will be used > > Just to clarify, there's no internal 32k oscillator used on the 3517/3505; > just a divider from the HF clock. Ah yes I see that now! >> and so the gptimer will still have a 32k clock source. > > That's a good question and you might want to check with Igor on that one, > the AM3517 TRM conflicts with the DM as to whether it's available to the > GPTIMER or not :-( Well the external 32k and internal divided down version go through the same mux and so that seems to imply either they are both available to the gptimer or neither is. Cheers Jon
On 11/08/2012 12:28 PM, Hiremath, Vaibhav wrote: > On Thu, Nov 08, 2012 at 23:43:31, Hunter, Jon wrote: >> >> On 11/08/2012 12:06 PM, Hiremath, Vaibhav wrote: >>> On Thu, Nov 08, 2012 at 23:28:53, Hunter, Jon wrote: >>>> >>>> On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: >>>>> On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: >>>> >>>> [snip] >>>> >>>>>> 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. >>>>>> >>>>> >>>>> Exactly that is the point I am trying to make here, >>>>> >>>>> In case of AM33xx, it is certainly possible to build the system without >>>>> this 32Khz clock. >>>>> >>>>> Interesting point here is, this 32KHz clock is external clock coming from >>>>> crystal connected to in-built RTC module. >>>> >>>> Thanks. Looking at the AM3358 data manual it states ... >>>> >>>> "The OSC1 oscillator provides a 32.768-kHz reference clock to the >>>> real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT >>>> terminals. OSC1 is disabled by default after power is applied. This >>>> clock input is optional and may not be required if the RTC is configured >>>> to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or >>>> peripheral PLL (CLK_32KHZ) which receives a reference clock from the >>>> OSC0 input." >>>> >>>> So what is clear to me that an external 32k clock source is optional. >>>> However, it still appears that you will always have an internal 32k >>>> source and so regardless of the h/w configuration, a gptimer will always >>>> have an 32k clock available on the AM335x devices. Is that correct? >>>> >>> >>> Internal RC oscillator is not accurate at all, so not guaranteed to give >>> accurate 32.768Hz clock. The oscillation band is from 16Khz to 64Khz. >>> >>> So it may not be useful as a system clocks, right? >> >> So I admit I am not familiar with the details on the AMxxxx stuff side >> so much. >> >> So maybe I should ask this question ... >> >> Do you support a configuration where there is no external 32k clock AND >> the internal oscillator and hence, RTC are not used? >> > > Why not, you could give-up on persistent time across suspend/resume and use > OSC clock as a input clock to timer. > > And the timer code, which we have we have added fallback mechanism for this, > First make sure that timer is properly set for RTC32K, if yes, then use it, > and if no, then fall back to OSC clock. You mean sync-32k and not rtc32k right? We are just detecting the presence of the sync-32k counter and if so use it, otherwise use a gptimer. >> I would have expected that you would always want the RTC running, but if >> you are saying that you don't require an external 32k and the internal >> 32k is not accurate, then I assume that having the RTC available is not >> mandatory either. >> > > Yes, it is not mandatory, considering fact that, you will not have > persistent time and other low-power modes. > Actually it depends on board/EVM use-case, right? Yes absolutely, just trying to understand if that is a valid use-case or not. So for AM33xx the external 32k is not mandatory. Thanks. Jon
On 11/08/2012 01:59 AM, Igor Grinberg wrote: [snip] > There is no reliable way to determine which source should be used in runtime > for boards that do not have the 32k oscillator wired. So thinking about this some more and given that we are moving away from board files, if a board does not provide a 32kHz clock source, then this should be reflected in the device-tree source file for that board. Hence, at boot time we should be able to determine if a 32kHz clock source can be used. Cheers Jon
On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: > > On 11/08/2012 01:59 AM, Igor Grinberg wrote: > > [snip] > > > There is no reliable way to determine which source should be used in runtime > > for boards that do not have the 32k oscillator wired. > > So thinking about this some more and given that we are moving away from > board files, if a board does not provide a 32kHz clock source, then this > should be reflected in the device-tree source file for that board. > Hence, at boot time we should be able to determine if a 32kHz clock > source can be used. > Let me feed some more thoughts here :) The way it is being detected currently is based on timer idle status bit. I am worried that, this is the only option we have. You can also refer to the implementation, so that it will help us to conclude on this - http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;h=58abec6ac010f4f8818caa4a52d16c4802e14acc Note that this is based on v3.2 kernel (+ additional patches), you should look the implementation of function omap_dm_timer_switch_src(). Thanks, Vaibhav
On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: > On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >> >> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >> >> [snip] >> >>> There is no reliable way to determine which source should be used in runtime >>> for boards that do not have the 32k oscillator wired. >> >> So thinking about this some more and given that we are moving away from >> board files, if a board does not provide a 32kHz clock source, then this >> should be reflected in the device-tree source file for that board. >> Hence, at boot time we should be able to determine if a 32kHz clock >> source can be used. >> > > Let me feed some more thoughts here :) > > The way it is being detected currently is based on timer idle status bit. > I am worried that, this is the only option we have. Why not use device-tree to indicate the presence of a 32k clock source? This seems like a board level configuration and so device-tree seems to be the perfect place for this IMO. Jon
On 11/08/2012 12:06 PM, Hiremath, Vaibhav wrote: > On Thu, Nov 08, 2012 at 23:28:53, Hunter, Jon wrote: >> >> On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: >>> On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: >> >> [snip] >> >>>> 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. >>>> >>> >>> Exactly that is the point I am trying to make here, >>> >>> In case of AM33xx, it is certainly possible to build the system without >>> this 32Khz clock. >>> >>> Interesting point here is, this 32KHz clock is external clock coming from >>> crystal connected to in-built RTC module. >> >> Thanks. Looking at the AM3358 data manual it states ... >> >> "The OSC1 oscillator provides a 32.768-kHz reference clock to the >> real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT >> terminals. OSC1 is disabled by default after power is applied. This >> clock input is optional and may not be required if the RTC is configured >> to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or >> peripheral PLL (CLK_32KHZ) which receives a reference clock from the >> OSC0 input." >> >> So what is clear to me that an external 32k clock source is optional. >> However, it still appears that you will always have an internal 32k >> source and so regardless of the h/w configuration, a gptimer will always >> have an 32k clock available on the AM335x devices. Is that correct? >> > > Internal RC oscillator is not accurate at all, so not guaranteed to give > accurate 32.768Hz clock. The oscillation band is from 16Khz to 64Khz. > > So it may not be useful as a system clocks, right? By the way, according to the AM3358 data manual (paragraph above), even if there is no external 32k clock source and if you don't use the internal 32k oscillator, you can still generate a 32k clock from the PER PLL (CLK_32KHZ). So therefore, there should always be a 32k clock source available for the gptimer. Is that correct? At the end of the day, I am just trying to understand if any OMAP/AM device supports a h/w configuration where there is no 32k clock source available for the gptimer. Jon
On 11/08/12 18:20, Tony Lindgren wrote: > * Igor Grinberg <grinberg@compulab.co.il> [121107 23:15]: >> On 11/07/12 19:33, Tony Lindgren wrote: >>> >>> I think this should be the default for the timers as that counter >>> does not stop during deeper idle states. >> >> Well, it is the default as you can see from the patch. >> The problem is that for boards that for some reason do not have >> the 32k wired and rely on MPU/GP timer source, the default will not work >> and currently there is no way for board to specify which timer source >> it can use. > > Yes. I was just wondering if we can avoid patching all the board > files by doing it the other way around by introducing a new > omap_gp_timer rather than renaming all the existing ones? Is the renaming that bad? One line per machine_desc structure? Of course we can skip the renaming, but then it will be less consistent and will not reflect the actual timer source used. I tried to make it flexible as much as possible and self explanatory. So above are my considerations, but at this point in time I don't really care if we rename them or just add a new one, but we have to get rid of the ugly fall back. > >> We have discussed this in San Diego (remember?) and you actually proposed >> this way as a solution. Well, may be I took it a bit further than you >> thought, but this is because the board code cannot know which timer source >> should be used at runtime and the fall back described below, does not work. > > Yes thanks I agree we should get rid of that Kconfig option for sure. > >>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>> @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") >>>> .handle_irq = omap2_intc_handle_irq, >>>> .init_machine = omap_2430sdp_init, >>>> .init_late = omap2430_init_late, >>>> - .timer = &omap2_timer, >>>> + .timer = &omap2_sync32k_timer, >>>> .restart = omap_prcm_restart, >>>> MACHINE_END >>>> --- a/arch/arm/mach-omap2/board-3430sdp.c >>>> +++ b/arch/arm/mach-omap2/board-3430sdp.c >>>> @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") >>>> .handle_irq = omap3_intc_handle_irq, >>>> .init_machine = omap_3430sdp_init, >>>> .init_late = omap3430_init_late, >>>> - .timer = &omap3_timer, >>>> + .timer = &omap3_sync32k_timer, >>>> .restart = omap_prcm_restart, >>>> MACHINE_END >>> ... >>> >>> Can't we assume that the default timer is omap[234]_sync32k_timer to >>> avoid renaming the timer entries in all the board files? >> >> Hmmm... >> How will this work with the macros defining the sys_timer structure? >> I would also not want to hide the exact timer used under the default name. > > Can't you just add a new sys_timer (or a new macro) for GP only setups? Of course I can... but I tried to create a flexible generic code, so no meter how a board will be wired, you just need to specify which timer source it uses and be done with it. > >>> Then we just need a new timer entries for the hardware that does >>> not have the sycn32k_timer available? >> >> Well, I tried to make it small patch just for the hardware that needs it, >> but I always found some corner case where, IMHO, this does not work/look good. > > Can you explain a bit further? Yes, OMAP4 has its own "local" timer function, OMAP5 - the real time timer function, we have that fall back from 32k to gptimer for AM33xx and we also have the use_gptimer_clksrc parameter. All the above call the same timer source init functions at the end. > > I guess what I'm after is just to avoid renaming the existing > timers in the board-*.c files and only rename the ones that > need gp timer only. This means: get rid of the 32k to gptimer fall back. I've got your point, will cook something shortly.
On 11/08/12 18:16, Jon Hunter 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 <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. >> >> 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); Yes, of course I understand your point, but that's how macro expansion work. The only idea left in my mind is to move the check for use_gptimer_clksrc to the omap2_sync32k_clocksource_init() function, but I don't like it, as omap2_sync32k_clocksource_init() function should deal with the sync32k init and if use_gptimer_clksrc is set, the function should not be called at all. I really don't think this is a problem. Do you have another idea how we can reuse the macro and not have the above oddness? > >>> >>> 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? As Paul already mentioned, AM35xx can work without the external 32kHz oscillator. > >>> 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. So we already have two devices: AM35xx and AM33xx, right? > >>> >>> 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. We have also, another case where, you don't want to use the 32k as a source and use sys_clk to have higher precision. > >>> >>>> } >>>> >>>> -#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? Hmmm, that how it is done currently (before my patch), or do I miss something? > I guess I don't see the benefit there, at least for OMAP2-5 > devices specifically. Well, this allows certain hardware variants to work properly. Isn't this a benefit?
On 11/08/12 21:16, Jon Hunter wrote: > > On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: >> On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >>> >>> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >>> >>> [snip] >>> >>>> There is no reliable way to determine which source should be used in runtime >>>> for boards that do not have the 32k oscillator wired. >>> >>> So thinking about this some more and given that we are moving away from >>> board files, if a board does not provide a 32kHz clock source, then this >>> should be reflected in the device-tree source file for that board. >>> Hence, at boot time we should be able to determine if a 32kHz clock >>> source can be used. >>> >> >> Let me feed some more thoughts here :) >> >> The way it is being detected currently is based on timer idle status bit. >> I am worried that, this is the only option we have. > > Why not use device-tree to indicate the presence of a 32k clock source? > This seems like a board level configuration and so device-tree seems to > be the perfect place for this IMO. Well, that is what my commit message says...
On 11/08/12 20:34, Jon Hunter wrote: > > On 11/08/2012 12:17 PM, Paul Walmsley wrote: >> On Thu, 8 Nov 2012, Jon Hunter wrote: >> >>> On 11/08/2012 11:58 AM, Paul Walmsley wrote: >>>> On Thu, 8 Nov 2012, Jon Hunter wrote: >>>> >>>>> 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. >>>> >>>> Pretty sure Igor is referring to the AM3517/3505. This is very poorly >>>> documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure >>>> 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 >>>> "Clock Specifications". >>> >>> But AFAICT, even in that h/w configuration the internal 32k >>> oscillator will be used >> >> Just to clarify, there's no internal 32k oscillator used on the 3517/3505; >> just a divider from the HF clock. > > Ah yes I see that now! > >>> and so the gptimer will still have a 32k clock source. >> >> That's a good question and you might want to check with Igor on that one, >> the AM3517 TRM conflicts with the DM as to whether it's available to the >> GPTIMER or not :-( > > Well the external 32k and internal divided down version go through the > same mux and so that seems to imply either they are both available to > the gptimer or neither is. Yep, but the /800 do not get you the 32768... and that makes the timer suck. Of course this can be dealt with in the clock subsystem (I remember Paul said that he will look into that), but it will take time. Also, what about having the sys_clk instead of 32k for higher precision? Is that possible already (without my patch)?
On Sun, Nov 11, 2012 at 17:05:07, Igor Grinberg wrote: > > > On 11/08/12 20:34, Jon Hunter wrote: > > > > On 11/08/2012 12:17 PM, Paul Walmsley wrote: > >> On Thu, 8 Nov 2012, Jon Hunter wrote: > >> > >>> On 11/08/2012 11:58 AM, Paul Walmsley wrote: > >>>> On Thu, 8 Nov 2012, Jon Hunter wrote: > >>>> > >>>>> 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. > >>>> > >>>> Pretty sure Igor is referring to the AM3517/3505. This is very poorly > >>>> documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure > >>>> 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 > >>>> "Clock Specifications". > >>> > >>> But AFAICT, even in that h/w configuration the internal 32k > >>> oscillator will be used > >> > >> Just to clarify, there's no internal 32k oscillator used on the 3517/3505; > >> just a divider from the HF clock. > > > > Ah yes I see that now! > > > >>> and so the gptimer will still have a 32k clock source. > >> > >> That's a good question and you might want to check with Igor on that one, > >> the AM3517 TRM conflicts with the DM as to whether it's available to the > >> GPTIMER or not :-( > > > > Well the external 32k and internal divided down version go through the > > same mux and so that seems to imply either they are both available to > > the gptimer or neither is. > > Yep, but the /800 do not get you the 32768... > and that makes the timer suck. > Of course this can be dealt with in the clock subsystem > (I remember Paul said that he will look into that), but it will take time. > > Also, what about having the sys_clk instead of 32k for higher precision? > Is that possible already (without my patch)? > Yes, it is possible. You can choose it through bootargs. Thanks, Vaibhav > > -- > Regards, > Igor. >
On Fri, Nov 09, 2012 at 06:25:49, Hunter, Jon wrote: > > On 11/08/2012 12:06 PM, Hiremath, Vaibhav wrote: > > On Thu, Nov 08, 2012 at 23:28:53, Hunter, Jon wrote: > >> > >> On 11/08/2012 11:47 AM, Hiremath, Vaibhav wrote: > >>> On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote: > >> > >> [snip] > >> > >>>> 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. > >>>> > >>> > >>> Exactly that is the point I am trying to make here, > >>> > >>> In case of AM33xx, it is certainly possible to build the system without > >>> this 32Khz clock. > >>> > >>> Interesting point here is, this 32KHz clock is external clock coming from > >>> crystal connected to in-built RTC module. > >> > >> Thanks. Looking at the AM3358 data manual it states ... > >> > >> "The OSC1 oscillator provides a 32.768-kHz reference clock to the > >> real-time clock (RTC) and is connected to the RTC_XTALIN and RTC_XTALOUT > >> terminals. OSC1 is disabled by default after power is applied. This > >> clock input is optional and may not be required if the RTC is configured > >> to receive a clock from the internal 32k RC oscillator (CLK_RC32K) or > >> peripheral PLL (CLK_32KHZ) which receives a reference clock from the > >> OSC0 input." > >> > >> So what is clear to me that an external 32k clock source is optional. > >> However, it still appears that you will always have an internal 32k > >> source and so regardless of the h/w configuration, a gptimer will always > >> have an 32k clock available on the AM335x devices. Is that correct? > >> > > > > Internal RC oscillator is not accurate at all, so not guaranteed to give > > accurate 32.768Hz clock. The oscillation band is from 16Khz to 64Khz. > > > > So it may not be useful as a system clocks, right? > > By the way, according to the AM3358 data manual (paragraph above), even > if there is no external 32k clock source and if you don't use the > internal 32k oscillator, you can still generate a 32k clock from the PER > PLL (CLK_32KHZ). So therefore, there should always be a 32k clock source > available for the gptimer. Is that correct? Yes that's correct, but it is from PER domain, so duging suspend time, it will be disabled. This can not be treated as a persistent clock. > > At the end of the day, I am just trying to understand if any OMAP/AM > device supports a h/w configuration where there is no 32k clock source > available for the gptimer. > I know and completely understand. We need to come up with solution which works on all platforms. Thanks, Vaibhav
On 11/12/12 08:38, Hiremath, Vaibhav wrote: > On Sun, Nov 11, 2012 at 17:05:07, Igor Grinberg wrote: >> >> >> On 11/08/12 20:34, Jon Hunter wrote: >>> >>> On 11/08/2012 12:17 PM, Paul Walmsley wrote: >>>> On Thu, 8 Nov 2012, Jon Hunter wrote: >>>> >>>>> On 11/08/2012 11:58 AM, Paul Walmsley wrote: >>>>>> On Thu, 8 Nov 2012, Jon Hunter wrote: >>>>>> >>>>>>> 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. >>>>>> >>>>>> Pretty sure Igor is referring to the AM3517/3505. This is very poorly >>>>>> documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure >>>>>> 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 >>>>>> "Clock Specifications". >>>>> >>>>> But AFAICT, even in that h/w configuration the internal 32k >>>>> oscillator will be used >>>> >>>> Just to clarify, there's no internal 32k oscillator used on the 3517/3505; >>>> just a divider from the HF clock. >>> >>> Ah yes I see that now! >>> >>>>> and so the gptimer will still have a 32k clock source. >>>> >>>> That's a good question and you might want to check with Igor on that one, >>>> the AM3517 TRM conflicts with the DM as to whether it's available to the >>>> GPTIMER or not :-( >>> >>> Well the external 32k and internal divided down version go through the >>> same mux and so that seems to imply either they are both available to >>> the gptimer or neither is. >> >> Yep, but the /800 do not get you the 32768... >> and that makes the timer suck. >> Of course this can be dealt with in the clock subsystem >> (I remember Paul said that he will look into that), but it will take time. >> >> Also, what about having the sys_clk instead of 32k for higher precision? >> Is that possible already (without my patch)? >> > > Yes, it is possible. You can choose it through bootargs. Is the kernel command line the only way for doing this? I personally dislike it, because it brings multiple maintenance problems. This must be possible at least through DT.
On Fri, Nov 09, 2012 at 00:46:28, Hunter, Jon wrote: > > On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: > > On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: > >> > >> On 11/08/2012 01:59 AM, Igor Grinberg wrote: > >> > >> [snip] > >> > >>> There is no reliable way to determine which source should be used in runtime > >>> for boards that do not have the 32k oscillator wired. > >> > >> So thinking about this some more and given that we are moving away from > >> board files, if a board does not provide a 32kHz clock source, then this > >> should be reflected in the device-tree source file for that board. > >> Hence, at boot time we should be able to determine if a 32kHz clock > >> source can be used. > >> > > > > Let me feed some more thoughts here :) > > > > The way it is being detected currently is based on timer idle status bit. > > I am worried that, this is the only option we have. > > Why not use device-tree to indicate the presence of a 32k clock source? > This seems like a board level configuration and so device-tree seems to > be the perfect place for this IMO. > I think I agree with you, but for this to happen in clean way, its time to start populating clock-nodes in DT, don't you think? Something like, clocks { rtc_clk: clk@X { compatible = "crystal-32k, per-32k, xyz"; clock-frequency = <32768>; }; ... }; Timer { ref-clock = <&rtc_clk>; }; What do you think? Thanks, Vaibhav
On Mon, Nov 12, 2012 at 12:54:59, Igor Grinberg wrote: > On 11/12/12 08:38, Hiremath, Vaibhav wrote: > > On Sun, Nov 11, 2012 at 17:05:07, Igor Grinberg wrote: > >> > >> > >> On 11/08/12 20:34, Jon Hunter wrote: > >>> > >>> On 11/08/2012 12:17 PM, Paul Walmsley wrote: > >>>> On Thu, 8 Nov 2012, Jon Hunter wrote: > >>>> > >>>>> On 11/08/2012 11:58 AM, Paul Walmsley wrote: > >>>>>> On Thu, 8 Nov 2012, Jon Hunter wrote: > >>>>>> > >>>>>>> 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. > >>>>>> > >>>>>> Pretty sure Igor is referring to the AM3517/3505. This is very poorly > >>>>>> documented, but can be observed in the AM3517 TRM Rev B (SPRUGR0B) Figure > >>>>>> 4-23 "PRM Clock Generator" and the AM3517 DM Rev C (SPRS550C) Section 4 > >>>>>> "Clock Specifications". > >>>>> > >>>>> But AFAICT, even in that h/w configuration the internal 32k > >>>>> oscillator will be used > >>>> > >>>> Just to clarify, there's no internal 32k oscillator used on the 3517/3505; > >>>> just a divider from the HF clock. > >>> > >>> Ah yes I see that now! > >>> > >>>>> and so the gptimer will still have a 32k clock source. > >>>> > >>>> That's a good question and you might want to check with Igor on that one, > >>>> the AM3517 TRM conflicts with the DM as to whether it's available to the > >>>> GPTIMER or not :-( > >>> > >>> Well the external 32k and internal divided down version go through the > >>> same mux and so that seems to imply either they are both available to > >>> the gptimer or neither is. > >> > >> Yep, but the /800 do not get you the 32768... > >> and that makes the timer suck. > >> Of course this can be dealt with in the clock subsystem > >> (I remember Paul said that he will look into that), but it will take time. > >> > >> Also, what about having the sys_clk instead of 32k for higher precision? > >> Is that possible already (without my patch)? > >> > > > > Yes, it is possible. You can choose it through bootargs. > > Is the kernel command line the only way for doing this? > I personally dislike it, because it brings multiple maintenance problems. > This must be possible at least through DT. > Its standard interface, so carries all the applicable pros and cons. I am not quite sure about runtime change of clocksoyrce, its pros and cons. Thanks, Vaibhav > > -- > Regards, > Igor. >
Hi Vaibhav, On 11/12/2012 11:38 AM, Hiremath, Vaibhav wrote: > On Fri, Nov 09, 2012 at 00:46:28, Hunter, Jon wrote: >> >> On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: >>> On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >>>> >>>> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >>>> >>>> [snip] >>>> >>>>> There is no reliable way to determine which source should be used in runtime >>>>> for boards that do not have the 32k oscillator wired. >>>> >>>> So thinking about this some more and given that we are moving away from >>>> board files, if a board does not provide a 32kHz clock source, then this >>>> should be reflected in the device-tree source file for that board. >>>> Hence, at boot time we should be able to determine if a 32kHz clock >>>> source can be used. >>>> >>> >>> Let me feed some more thoughts here :) >>> >>> The way it is being detected currently is based on timer idle status bit. >>> I am worried that, this is the only option we have. >> >> Why not use device-tree to indicate the presence of a 32k clock source? >> This seems like a board level configuration and so device-tree seems to >> be the perfect place for this IMO. >> > > I think I agree with you, but for this to happen in clean way, its time to > start populating clock-nodes in DT, don't you think? Something like, > > > clocks { > rtc_clk: clk@X { > compatible = "crystal-32k, per-32k, xyz"; > clock-frequency = <32768>; > }; > ... > }; > > Timer { > > ref-clock = <&rtc_clk>; > }; > > What do you think? That's indeed the proper way to do it, since this is a pure board level parameter and we do have the binding to express that. We just have to add that in the DTS:-) Regards, Benoit
On 11/11/2012 03:16 AM, Igor Grinberg wrote: > On 11/08/12 18:20, Tony Lindgren wrote: >> * Igor Grinberg <grinberg@compulab.co.il> [121107 23:15]: >>> On 11/07/12 19:33, Tony Lindgren wrote: >>>> >>>> I think this should be the default for the timers as that counter >>>> does not stop during deeper idle states. >>> >>> Well, it is the default as you can see from the patch. >>> The problem is that for boards that for some reason do not have >>> the 32k wired and rely on MPU/GP timer source, the default will not work >>> and currently there is no way for board to specify which timer source >>> it can use. >> >> Yes. I was just wondering if we can avoid patching all the board >> files by doing it the other way around by introducing a new >> omap_gp_timer rather than renaming all the existing ones? > > Is the renaming that bad? One line per machine_desc structure? > Of course we can skip the renaming, but then it will be less consistent > and will not reflect the actual timer source used. > I tried to make it flexible as much as possible and self explanatory. > So above are my considerations, but at this point in time I don't really > care if we rename them or just add a new one, but we have to get rid of > the ugly fall back. I am not sure if you guys disagree, but does it make sense to start thinking about this with regard to device-tree? With device-tree all the boards files will become obsolete and so we need to be able to handle this during boot time and not compile time. >> >>> We have discussed this in San Diego (remember?) and you actually proposed >>> this way as a solution. Well, may be I took it a bit further than you >>> thought, but this is because the board code cannot know which timer source >>> should be used at runtime and the fall back described below, does not work. >> >> Yes thanks I agree we should get rid of that Kconfig option for sure. >> >>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>>> @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") >>>>> .handle_irq = omap2_intc_handle_irq, >>>>> .init_machine = omap_2430sdp_init, >>>>> .init_late = omap2430_init_late, >>>>> - .timer = &omap2_timer, >>>>> + .timer = &omap2_sync32k_timer, >>>>> .restart = omap_prcm_restart, >>>>> MACHINE_END >>>>> --- a/arch/arm/mach-omap2/board-3430sdp.c >>>>> +++ b/arch/arm/mach-omap2/board-3430sdp.c >>>>> @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") >>>>> .handle_irq = omap3_intc_handle_irq, >>>>> .init_machine = omap_3430sdp_init, >>>>> .init_late = omap3430_init_late, >>>>> - .timer = &omap3_timer, >>>>> + .timer = &omap3_sync32k_timer, >>>>> .restart = omap_prcm_restart, >>>>> MACHINE_END >>>> ... >>>> >>>> Can't we assume that the default timer is omap[234]_sync32k_timer to >>>> avoid renaming the timer entries in all the board files? >>> >>> Hmmm... >>> How will this work with the macros defining the sys_timer structure? >>> I would also not want to hide the exact timer used under the default name. >> >> Can't you just add a new sys_timer (or a new macro) for GP only setups? > > Of course I can... but I tried to create a flexible generic code, so > no meter how a board will be wired, you just need to specify which timer source > it uses and be done with it. If you are concerned about how a board is wired up (if the 32k is present), then I think that that is best handled via device-tree and we should query device-tree on boot to see what our options are. What do you guys think? Cheers Jon
On 11/11/2012 05:28 AM, Igor Grinberg wrote: > > > On 11/08/12 21:16, Jon Hunter wrote: >> >> On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: >>> On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >>>> >>>> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >>>> >>>> [snip] >>>> >>>>> There is no reliable way to determine which source should be used in runtime >>>>> for boards that do not have the 32k oscillator wired. >>>> >>>> So thinking about this some more and given that we are moving away from >>>> board files, if a board does not provide a 32kHz clock source, then this >>>> should be reflected in the device-tree source file for that board. >>>> Hence, at boot time we should be able to determine if a 32kHz clock >>>> source can be used. >>>> >>> >>> Let me feed some more thoughts here :) >>> >>> The way it is being detected currently is based on timer idle status bit. >>> I am worried that, this is the only option we have. >> >> Why not use device-tree to indicate the presence of a 32k clock source? >> This seems like a board level configuration and so device-tree seems to >> be the perfect place for this IMO. > > Well, that is what my commit message says... Sorry, but that was not clear to me from whats in the commit message. Should we be doing this now instead of adding all these static timer init functions? Are there any boards today (supported in the kernel that is), that don't support a 32k? If not, then this becomes simpler for the non-DT case and given that boards such as the AM335x will only support DT boot then we can figure out how to detect the presence of the 32k for DT only. Jon
* Igor Grinberg <grinberg@compulab.co.il> [121111 01:18]: > On 11/08/12 18:20, Tony Lindgren wrote: > > > > I guess what I'm after is just to avoid renaming the existing > > timers in the board-*.c files and only rename the ones that > > need gp timer only. > > This means: get rid of the 32k to gptimer fall back. > I've got your point, will cook something shortly. Thanks that sounds reasonable to me. Regards, Tony
On 11/12/12 21:05, Jon Hunter wrote: > > On 11/11/2012 03:16 AM, Igor Grinberg wrote: >> On 11/08/12 18:20, Tony Lindgren wrote: >>> * Igor Grinberg <grinberg@compulab.co.il> [121107 23:15]: >>>> On 11/07/12 19:33, Tony Lindgren wrote: >>>>> >>>>> I think this should be the default for the timers as that counter >>>>> does not stop during deeper idle states. >>>> >>>> Well, it is the default as you can see from the patch. >>>> The problem is that for boards that for some reason do not have >>>> the 32k wired and rely on MPU/GP timer source, the default will not work >>>> and currently there is no way for board to specify which timer source >>>> it can use. >>> >>> Yes. I was just wondering if we can avoid patching all the board >>> files by doing it the other way around by introducing a new >>> omap_gp_timer rather than renaming all the existing ones? >> >> Is the renaming that bad? One line per machine_desc structure? >> Of course we can skip the renaming, but then it will be less consistent >> and will not reflect the actual timer source used. >> I tried to make it flexible as much as possible and self explanatory. >> So above are my considerations, but at this point in time I don't really >> care if we rename them or just add a new one, but we have to get rid of >> the ugly fall back. > > I am not sure if you guys disagree, but does it make sense to start > thinking about this with regard to device-tree? With device-tree all the > boards files will become obsolete and so we need to be able to handle > this during boot time and not compile time. This is understood completely, but I personally think that we should not neglect the "still not converted to DT boards", because the conversion takes time and involves much more effort. Also taking into account that there are subsystems that are still not converted to DT. Also, removing the CONFIG_OMAP_32K_TIMER _does_ get us closer to handle the timers init during boot time. > >>> >>>> We have discussed this in San Diego (remember?) and you actually proposed >>>> this way as a solution. Well, may be I took it a bit further than you >>>> thought, but this is because the board code cannot know which timer source >>>> should be used at runtime and the fall back described below, does not work. >>> >>> Yes thanks I agree we should get rid of that Kconfig option for sure. >>> >>>>>> --- a/arch/arm/mach-omap2/board-2430sdp.c >>>>>> +++ b/arch/arm/mach-omap2/board-2430sdp.c >>>>>> @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") >>>>>> .handle_irq = omap2_intc_handle_irq, >>>>>> .init_machine = omap_2430sdp_init, >>>>>> .init_late = omap2430_init_late, >>>>>> - .timer = &omap2_timer, >>>>>> + .timer = &omap2_sync32k_timer, >>>>>> .restart = omap_prcm_restart, >>>>>> MACHINE_END >>>>>> --- a/arch/arm/mach-omap2/board-3430sdp.c >>>>>> +++ b/arch/arm/mach-omap2/board-3430sdp.c >>>>>> @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") >>>>>> .handle_irq = omap3_intc_handle_irq, >>>>>> .init_machine = omap_3430sdp_init, >>>>>> .init_late = omap3430_init_late, >>>>>> - .timer = &omap3_timer, >>>>>> + .timer = &omap3_sync32k_timer, >>>>>> .restart = omap_prcm_restart, >>>>>> MACHINE_END >>>>> ... >>>>> >>>>> Can't we assume that the default timer is omap[234]_sync32k_timer to >>>>> avoid renaming the timer entries in all the board files? >>>> >>>> Hmmm... >>>> How will this work with the macros defining the sys_timer structure? >>>> I would also not want to hide the exact timer used under the default name. >>> >>> Can't you just add a new sys_timer (or a new macro) for GP only setups? >> >> Of course I can... but I tried to create a flexible generic code, so >> no meter how a board will be wired, you just need to specify which timer source >> it uses and be done with it. > > If you are concerned about how a board is wired up (if the 32k is > present), then I think that that is best handled via device-tree and we > should query device-tree on boot to see what our options are. > > What do you guys think? Yes, but again, you can't neglect the non-DT case... at least until you have everything converted to DT.
On 11/12/12 21:15, Jon Hunter wrote: > > On 11/11/2012 05:28 AM, Igor Grinberg wrote: >> >> >> On 11/08/12 21:16, Jon Hunter wrote: >>> >>> On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: >>>> On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >>>>> >>>>> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >>>>> >>>>> [snip] >>>>> >>>>>> There is no reliable way to determine which source should be used in runtime >>>>>> for boards that do not have the 32k oscillator wired. >>>>> >>>>> So thinking about this some more and given that we are moving away from >>>>> board files, if a board does not provide a 32kHz clock source, then this >>>>> should be reflected in the device-tree source file for that board. >>>>> Hence, at boot time we should be able to determine if a 32kHz clock >>>>> source can be used. >>>>> >>>> >>>> Let me feed some more thoughts here :) >>>> >>>> The way it is being detected currently is based on timer idle status bit. >>>> I am worried that, this is the only option we have. >>> >>> Why not use device-tree to indicate the presence of a 32k clock source? >>> This seems like a board level configuration and so device-tree seems to >>> be the perfect place for this IMO. >> >> Well, that is what my commit message says... > > Sorry, but that was not clear to me from whats in the commit message. From the commit message: "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." The last sentence has it. The transition to DT is not immediate and we can't (still) neglect the non-DT setups. > > Should we be doing this now instead of adding all these static timer > init functions? I don't see this as "adding ...", I see this as expanding the setup which was previously hidden by the CONFIG_OMAP_32K_TIMER option. > > Are there any boards today (supported in the kernel that is), that don't > support a 32k? Yes, starting from revision 1.2, CM-T3517 does not have the 32k. [...]
On 11/13/2012 03:14 AM, Igor Grinberg wrote: > On 11/12/12 21:15, Jon Hunter wrote: >> >> On 11/11/2012 05:28 AM, Igor Grinberg wrote: >>> >>> >>> On 11/08/12 21:16, Jon Hunter wrote: >>>> >>>> On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: >>>>> On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >>>>>> >>>>>> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >>>>>> >>>>>> [snip] >>>>>> >>>>>>> There is no reliable way to determine which source should be used in runtime >>>>>>> for boards that do not have the 32k oscillator wired. >>>>>> >>>>>> So thinking about this some more and given that we are moving away from >>>>>> board files, if a board does not provide a 32kHz clock source, then this >>>>>> should be reflected in the device-tree source file for that board. >>>>>> Hence, at boot time we should be able to determine if a 32kHz clock >>>>>> source can be used. >>>>>> >>>>> >>>>> Let me feed some more thoughts here :) >>>>> >>>>> The way it is being detected currently is based on timer idle status bit. >>>>> I am worried that, this is the only option we have. >>>> >>>> Why not use device-tree to indicate the presence of a 32k clock source? >>>> This seems like a board level configuration and so device-tree seems to >>>> be the perfect place for this IMO. >>> >>> Well, that is what my commit message says... >> >> Sorry, but that was not clear to me from whats in the commit message. > > From the commit message: > "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." > > The last sentence has it. Right, but it does not go into the details. It would be good to have added a comment to the effect of "some boards do not have a 32k clock source and in the future this should be handled by device-tree". > The transition to DT is not immediate and we can't (still) neglect > the non-DT setups. Absolutely, but I am trying to understand if there are boards being "neglected". I see now that your CM-T3517 would be. This was not clear from your patch as even the CM-T3517 board was being configured to the use the sync32k timer. So from looking at your patch I did not see any neglected boards, however, I understand your motivation to add all these init functions so that boards could be customised easily. >> Should we be doing this now instead of adding all these static timer >> init functions? > > I don't see this as "adding ...", I see this as expanding the setup > which was previously hidden by the CONFIG_OMAP_32K_TIMER option. > >> >> Are there any boards today (supported in the kernel that is), that don't >> support a 32k? > > Yes, starting from revision 1.2, CM-T3517 does not have the 32k. Thanks, this is the exact information I was looking for. You should put this in your commit message to highlight the fact that there are boards that don't have a 32k clock source. I am familiar with the OMAP devices, but less familiar with these AMxxxx derivatives (as I don't work with these) and so it is good to put these specifics in the commit message. Cheers Jon
On 11/13/12 18:13, Jon Hunter wrote: > > On 11/13/2012 03:14 AM, Igor Grinberg wrote: >> On 11/12/12 21:15, Jon Hunter wrote: >>> >>> On 11/11/2012 05:28 AM, Igor Grinberg wrote: >>>> >>>> >>>> On 11/08/12 21:16, Jon Hunter wrote: >>>>> >>>>> On 11/08/2012 12:59 PM, Hiremath, Vaibhav wrote: >>>>>> On Fri, Nov 09, 2012 at 00:24:23, Hunter, Jon wrote: >>>>>>> >>>>>>> On 11/08/2012 01:59 AM, Igor Grinberg wrote: >>>>>>> >>>>>>> [snip] >>>>>>> >>>>>>>> There is no reliable way to determine which source should be used in runtime >>>>>>>> for boards that do not have the 32k oscillator wired. >>>>>>> >>>>>>> So thinking about this some more and given that we are moving away from >>>>>>> board files, if a board does not provide a 32kHz clock source, then this >>>>>>> should be reflected in the device-tree source file for that board. >>>>>>> Hence, at boot time we should be able to determine if a 32kHz clock >>>>>>> source can be used. >>>>>>> >>>>>> >>>>>> Let me feed some more thoughts here :) >>>>>> >>>>>> The way it is being detected currently is based on timer idle status bit. >>>>>> I am worried that, this is the only option we have. >>>>> >>>>> Why not use device-tree to indicate the presence of a 32k clock source? >>>>> This seems like a board level configuration and so device-tree seems to >>>>> be the perfect place for this IMO. >>>> >>>> Well, that is what my commit message says... >>> >>> Sorry, but that was not clear to me from whats in the commit message. >> >> From the commit message: >> "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." >> >> The last sentence has it. > > Right, but it does not go into the details. It would be good to have > added a comment to the effect of "some boards do not have a 32k clock > source and in the future this should be handled by device-tree". Ok. > >> The transition to DT is not immediate and we can't (still) neglect >> the non-DT setups. > > Absolutely, but I am trying to understand if there are boards being > "neglected". I see now that your CM-T3517 would be. This was not clear > from your patch as even the CM-T3517 board was being configured to the > use the sync32k timer. So from looking at your patch I did not see any > neglected boards, however, I understand your motivation to add all these > init functions so that boards could be customised easily. I did not changed the CM-T3517, because I believe it should be done in a separate patch. > >>> Should we be doing this now instead of adding all these static timer >>> init functions? >> >> I don't see this as "adding ...", I see this as expanding the setup >> which was previously hidden by the CONFIG_OMAP_32K_TIMER option. >> >>> >>> Are there any boards today (supported in the kernel that is), that don't >>> support a 32k? >> >> Yes, starting from revision 1.2, CM-T3517 does not have the 32k. > > Thanks, this is the exact information I was looking for. You should put > this in your commit message to highlight the fact that there are boards > that don't have a 32k clock source. > > I am familiar with the OMAP devices, but less familiar with these AMxxxx > derivatives (as I don't work with these) and so it is good to put these > specifics in the commit message. Ok.
On Sun, 11 Nov 2012, Igor Grinberg wrote: > Yep, but the /800 do not get you the 32768... > and that makes the timer suck. > Of course this can be dealt with in the clock subsystem > (I remember Paul said that he will look into that), but it will take time. It should be possible to implement this now that the CCF conversion is done. Not sure if I'll be able to get to it soon, though. Is there anyone out there at TI who is focused on AM3517/3505 upstream work? - Paul
On Thu, Nov 8, 2012 at 9:08 AM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote: > No we do not have 32k_counter block in AM335x. > > 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. RTC domain.
diff --git a/arch/arm/mach-omap2/board-2430sdp.c b/arch/arm/mach-omap2/board-2430sdp.c index d1c0162..90c1584 100644 --- a/arch/arm/mach-omap2/board-2430sdp.c +++ b/arch/arm/mach-omap2/board-2430sdp.c @@ -284,6 +284,6 @@ MACHINE_START(OMAP_2430SDP, "OMAP2430 sdp2430 board") .handle_irq = omap2_intc_handle_irq, .init_machine = omap_2430sdp_init, .init_late = omap2430_init_late, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-3430sdp.c b/arch/arm/mach-omap2/board-3430sdp.c index 79fd904..e14b355 100644 --- a/arch/arm/mach-omap2/board-3430sdp.c +++ b/arch/arm/mach-omap2/board-3430sdp.c @@ -596,6 +596,6 @@ MACHINE_START(OMAP_3430SDP, "OMAP3430 3430SDP board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap_3430sdp_init, .init_late = omap3430_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-3630sdp.c b/arch/arm/mach-omap2/board-3630sdp.c index 81871b1..030d292 100644 --- a/arch/arm/mach-omap2/board-3630sdp.c +++ b/arch/arm/mach-omap2/board-3630sdp.c @@ -211,6 +211,6 @@ MACHINE_START(OMAP_3630SDP, "OMAP 3630SDP board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap_sdp_init, .init_late = omap3630_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c index fd80d97..c29e446 100644 --- a/arch/arm/mach-omap2/board-4430sdp.c +++ b/arch/arm/mach-omap2/board-4430sdp.c @@ -880,6 +880,6 @@ MACHINE_START(OMAP_4430SDP, "OMAP4430 4430SDP board") .handle_irq = gic_handle_irq, .init_machine = omap_4430sdp_init, .init_late = omap4430_init_late, - .timer = &omap4_timer, + .timer = &omap4_local_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-am3517crane.c b/arch/arm/mach-omap2/board-am3517crane.c index 603503c..db2c007 100644 --- a/arch/arm/mach-omap2/board-am3517crane.c +++ b/arch/arm/mach-omap2/board-am3517crane.c @@ -92,6 +92,6 @@ MACHINE_START(CRANEBOARD, "AM3517/05 CRANEBOARD") .handle_irq = omap3_intc_handle_irq, .init_machine = am3517_crane_init, .init_late = am35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-am3517evm.c b/arch/arm/mach-omap2/board-am3517evm.c index 96d6c5a..8802928 100644 --- a/arch/arm/mach-omap2/board-am3517evm.c +++ b/arch/arm/mach-omap2/board-am3517evm.c @@ -392,6 +392,6 @@ MACHINE_START(OMAP3517EVM, "OMAP3517/AM3517 EVM") .handle_irq = omap3_intc_handle_irq, .init_machine = am3517_evm_init, .init_late = am35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-apollon.c b/arch/arm/mach-omap2/board-apollon.c index 64cf1bd..d7fa83c 100644 --- a/arch/arm/mach-omap2/board-apollon.c +++ b/arch/arm/mach-omap2/board-apollon.c @@ -337,6 +337,6 @@ MACHINE_START(OMAP_APOLLON, "OMAP24xx Apollon") .handle_irq = omap2_intc_handle_irq, .init_machine = omap_apollon_init, .init_late = omap2420_init_late, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-cm-t35.c b/arch/arm/mach-omap2/board-cm-t35.c index a8cad22..6cfed1a 100644 --- a/arch/arm/mach-omap2/board-cm-t35.c +++ b/arch/arm/mach-omap2/board-cm-t35.c @@ -750,19 +750,19 @@ MACHINE_START(CM_T35, "Compulab CM-T35") .handle_irq = omap3_intc_handle_irq, .init_machine = cm_t35_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END MACHINE_START(CM_T3730, "Compulab CM-T3730") - .atag_offset = 0x100, - .reserve = omap_reserve, - .map_io = omap3_map_io, - .init_early = omap3630_init_early, - .init_irq = omap3_init_irq, + .atag_offset = 0x100, + .reserve = omap_reserve, + .map_io = omap3_map_io, + .init_early = omap3630_init_early, + .init_irq = omap3_init_irq, .handle_irq = omap3_intc_handle_irq, - .init_machine = cm_t3730_init, - .init_late = omap3630_init_late, - .timer = &omap3_timer, + .init_machine = cm_t3730_init, + .init_late = omap3630_init_late, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-cm-t3517.c b/arch/arm/mach-omap2/board-cm-t3517.c index 2786647..4af5b75 100644 --- a/arch/arm/mach-omap2/board-cm-t3517.c +++ b/arch/arm/mach-omap2/board-cm-t3517.c @@ -297,6 +297,6 @@ MACHINE_START(CM_T3517, "Compulab CM-T3517") .handle_irq = omap3_intc_handle_irq, .init_machine = cm_t3517_init, .init_late = am35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-devkit8000.c b/arch/arm/mach-omap2/board-devkit8000.c index 933479e..3b2f5e58 100644 --- a/arch/arm/mach-omap2/board-devkit8000.c +++ b/arch/arm/mach-omap2/board-devkit8000.c @@ -642,6 +642,6 @@ MACHINE_START(DEVKIT8000, "OMAP3 Devkit8000") .handle_irq = omap3_intc_handle_irq, .init_machine = devkit8000_init, .init_late = omap35xx_init_late, - .timer = &omap3_secure_timer, + .timer = &omap3_secure_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c index d690180..b57f4ac 100644 --- a/arch/arm/mach-omap2/board-generic.c +++ b/arch/arm/mach-omap2/board-generic.c @@ -55,7 +55,7 @@ DT_MACHINE_START(OMAP242X_DT, "Generic OMAP2420 (Flattened Device Tree)") .init_irq = omap_intc_of_init, .handle_irq = omap2_intc_handle_irq, .init_machine = omap_generic_init, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .dt_compat = omap242x_boards_compat, .restart = omap_prcm_restart, MACHINE_END @@ -74,7 +74,7 @@ DT_MACHINE_START(OMAP243X_DT, "Generic OMAP2430 (Flattened Device Tree)") .init_irq = omap_intc_of_init, .handle_irq = omap2_intc_handle_irq, .init_machine = omap_generic_init, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .dt_compat = omap243x_boards_compat, .restart = omap_prcm_restart, MACHINE_END @@ -93,7 +93,7 @@ DT_MACHINE_START(OMAP3_DT, "Generic OMAP3 (Flattened Device Tree)") .init_irq = omap_intc_of_init, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_generic_init, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .dt_compat = omap3_boards_compat, .restart = omap_prcm_restart, MACHINE_END @@ -110,7 +110,7 @@ DT_MACHINE_START(OMAP3_GP_DT, "Generic OMAP3-GP (Flattened Device Tree)") .init_irq = omap_intc_of_init, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_generic_init, - .timer = &omap3_secure_timer, + .timer = &omap3_secure_sync32k_timer, .dt_compat = omap3_gp_boards_compat, .restart = omap_prcm_restart, MACHINE_END @@ -129,7 +129,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)") .init_irq = omap_intc_of_init, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_generic_init, - .timer = &omap3_am33xx_timer, + .timer = &omap3_am33xx_gp_timer, .dt_compat = am33xx_boards_compat, MACHINE_END #endif @@ -149,7 +149,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)") .handle_irq = gic_handle_irq, .init_machine = omap_generic_init, .init_late = omap4430_init_late, - .timer = &omap4_timer, + .timer = &omap4_local_sync32k_timer, .dt_compat = omap4_boards_compat, .restart = omap_prcm_restart, MACHINE_END @@ -169,7 +169,7 @@ DT_MACHINE_START(OMAP5_DT, "Generic OMAP5 (Flattened Device Tree)") .init_irq = omap_gic_of_init, .handle_irq = gic_handle_irq, .init_machine = omap_generic_init, - .timer = &omap5_timer, + .timer = &omap5_realtime_sync32k_timer, .dt_compat = omap5_boards_compat, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c index 8668c72..cf404f3 100644 --- a/arch/arm/mach-omap2/board-h4.c +++ b/arch/arm/mach-omap2/board-h4.c @@ -385,6 +385,6 @@ MACHINE_START(OMAP_H4, "OMAP2420 H4 board") .handle_irq = omap2_intc_handle_irq, .init_machine = omap_h4_init, .init_late = omap2420_init_late, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-igep0020.c b/arch/arm/mach-omap2/board-igep0020.c index dbc705a..279af3f 100644 --- a/arch/arm/mach-omap2/board-igep0020.c +++ b/arch/arm/mach-omap2/board-igep0020.c @@ -650,7 +650,7 @@ MACHINE_START(IGEP0020, "IGEP v2 board") .handle_irq = omap3_intc_handle_irq, .init_machine = igep_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END @@ -663,6 +663,6 @@ MACHINE_START(IGEP0030, "IGEP OMAP3 module") .handle_irq = omap3_intc_handle_irq, .init_machine = igep_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-ldp.c b/arch/arm/mach-omap2/board-ldp.c index 1164b10..7508f4c 100644 --- a/arch/arm/mach-omap2/board-ldp.c +++ b/arch/arm/mach-omap2/board-ldp.c @@ -435,6 +435,6 @@ MACHINE_START(OMAP_LDP, "OMAP LDP board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap_ldp_init, .init_late = omap3430_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c index 123fda3..49dc3eb 100644 --- a/arch/arm/mach-omap2/board-n8x0.c +++ b/arch/arm/mach-omap2/board-n8x0.c @@ -731,7 +731,7 @@ MACHINE_START(NOKIA_N800, "Nokia N800") .handle_irq = omap2_intc_handle_irq, .init_machine = n8x0_init_machine, .init_late = omap2420_init_late, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END @@ -744,7 +744,7 @@ MACHINE_START(NOKIA_N810, "Nokia N810") .handle_irq = omap2_intc_handle_irq, .init_machine = n8x0_init_machine, .init_late = omap2420_init_late, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END @@ -757,6 +757,6 @@ MACHINE_START(NOKIA_N810_WIMAX, "Nokia N810 WiMAX") .handle_irq = omap2_intc_handle_irq, .init_machine = n8x0_init_machine, .init_late = omap2420_init_late, - .timer = &omap2_timer, + .timer = &omap2_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c index 5a3800d..cd6e636 100644 --- a/arch/arm/mach-omap2/board-omap3beagle.c +++ b/arch/arm/mach-omap2/board-omap3beagle.c @@ -544,6 +544,6 @@ MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3_beagle_init, .init_late = omap3_init_late, - .timer = &omap3_secure_timer, + .timer = &omap3_secure_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3encore.c b/arch/arm/mach-omap2/board-omap3encore.c index 675c47a..6e92d6a 100644 --- a/arch/arm/mach-omap2/board-omap3encore.c +++ b/arch/arm/mach-omap2/board-omap3encore.c @@ -326,5 +326,5 @@ MACHINE_START(ENCORE, "encore") .init_irq = omap3_init_irq, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_encore_init, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-omap2/board-omap3evm.c index 3c0b9a9..d72bc7e 100644 --- a/arch/arm/mach-omap2/board-omap3evm.c +++ b/arch/arm/mach-omap2/board-omap3evm.c @@ -756,6 +756,6 @@ MACHINE_START(OMAP3EVM, "OMAP3 EVM") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3_evm_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3logic.c b/arch/arm/mach-omap2/board-omap3logic.c index e84e2a8..28b5d65 100644 --- a/arch/arm/mach-omap2/board-omap3logic.c +++ b/arch/arm/mach-omap2/board-omap3logic.c @@ -231,7 +231,7 @@ MACHINE_START(OMAP3_TORPEDO, "Logic OMAP3 Torpedo board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3logic_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END @@ -244,6 +244,6 @@ MACHINE_START(OMAP3530_LV_SOM, "OMAP Logic 3530 LV SOM board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3logic_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3pandora.c b/arch/arm/mach-omap2/board-omap3pandora.c index ce31bd3..074d84d 100644 --- a/arch/arm/mach-omap2/board-omap3pandora.c +++ b/arch/arm/mach-omap2/board-omap3pandora.c @@ -618,6 +618,6 @@ MACHINE_START(OMAP3_PANDORA, "Pandora Handheld Console") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3pandora_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3stalker.c b/arch/arm/mach-omap2/board-omap3stalker.c index ba11245..9e6c92a 100644 --- a/arch/arm/mach-omap2/board-omap3stalker.c +++ b/arch/arm/mach-omap2/board-omap3stalker.c @@ -426,6 +426,6 @@ MACHINE_START(SBC3530, "OMAP3 STALKER") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3_stalker_init, .init_late = omap35xx_init_late, - .timer = &omap3_secure_timer, + .timer = &omap3_secure_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap3touchbook.c b/arch/arm/mach-omap2/board-omap3touchbook.c index a225d81..7ae22d4 100644 --- a/arch/arm/mach-omap2/board-omap3touchbook.c +++ b/arch/arm/mach-omap2/board-omap3touchbook.c @@ -386,6 +386,6 @@ MACHINE_START(TOUCHBOOK, "OMAP3 touchbook Board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap3_touchbook_init, .init_late = omap3430_init_late, - .timer = &omap3_secure_timer, + .timer = &omap3_secure_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c index 8c00b99..02eb646 100644 --- a/arch/arm/mach-omap2/board-omap4panda.c +++ b/arch/arm/mach-omap2/board-omap4panda.c @@ -523,6 +523,6 @@ MACHINE_START(OMAP4_PANDA, "OMAP4 Panda board") .handle_irq = gic_handle_irq, .init_machine = omap4_panda_init, .init_late = omap4430_init_late, - .timer = &omap4_timer, + .timer = &omap4_local_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-omap4pcm049.c b/arch/arm/mach-omap2/board-omap4pcm049.c index 47a710e..7f3ffdd 100644 --- a/arch/arm/mach-omap2/board-omap4pcm049.c +++ b/arch/arm/mach-omap2/board-omap4pcm049.c @@ -538,5 +538,5 @@ MACHINE_START(PCM049, "phyCORE OMAP4") .init_irq = gic_init_irq, .handle_irq = gic_handle_irq, .init_machine = pcm049_init, - .timer = &omap4_timer, + .timer = &omap4_local_sync32k_timer, MACHINE_END diff --git a/arch/arm/mach-omap2/board-overo.c b/arch/arm/mach-omap2/board-overo.c index 1cfb037..864be4c 100644 --- a/arch/arm/mach-omap2/board-overo.c +++ b/arch/arm/mach-omap2/board-overo.c @@ -552,6 +552,6 @@ MACHINE_START(OVERO, "Gumstix Overo") .handle_irq = omap3_intc_handle_irq, .init_machine = overo_init, .init_late = omap35xx_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-rm680.c b/arch/arm/mach-omap2/board-rm680.c index 1997e0e..5514d2e 100644 --- a/arch/arm/mach-omap2/board-rm680.c +++ b/arch/arm/mach-omap2/board-rm680.c @@ -147,7 +147,7 @@ MACHINE_START(NOKIA_RM680, "Nokia RM-680 board") .handle_irq = omap3_intc_handle_irq, .init_machine = rm680_init, .init_late = omap3630_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END @@ -160,6 +160,6 @@ MACHINE_START(NOKIA_RM696, "Nokia RM-696 board") .handle_irq = omap3_intc_handle_irq, .init_machine = rm680_init, .init_late = omap3630_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-rx51.c b/arch/arm/mach-omap2/board-rx51.c index c388aec..03ae3f3 100644 --- a/arch/arm/mach-omap2/board-rx51.c +++ b/arch/arm/mach-omap2/board-rx51.c @@ -126,6 +126,6 @@ MACHINE_START(NOKIA_RX51, "Nokia RX-51 board") .handle_irq = omap3_intc_handle_irq, .init_machine = rx51_init, .init_late = omap3430_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/board-ti8168evm.c b/arch/arm/mach-omap2/board-ti8168evm.c index 5e672c2..9f03177 100644 --- a/arch/arm/mach-omap2/board-ti8168evm.c +++ b/arch/arm/mach-omap2/board-ti8168evm.c @@ -43,7 +43,7 @@ MACHINE_START(TI8168EVM, "ti8168evm") .map_io = ti81xx_map_io, .init_early = ti81xx_init_early, .init_irq = ti81xx_init_irq, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .init_machine = ti81xx_evm_init, .init_late = ti81xx_init_late, .restart = omap_prcm_restart, @@ -55,7 +55,7 @@ MACHINE_START(TI8148EVM, "ti8148evm") .map_io = ti81xx_map_io, .init_early = ti81xx_init_early, .init_irq = ti81xx_init_irq, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .init_machine = ti81xx_evm_init, .init_late = ti81xx_init_late, .restart = omap_prcm_restart, diff --git a/arch/arm/mach-omap2/board-zoom.c b/arch/arm/mach-omap2/board-zoom.c index 8feb4d9..b906430 100644 --- a/arch/arm/mach-omap2/board-zoom.c +++ b/arch/arm/mach-omap2/board-zoom.c @@ -137,7 +137,7 @@ MACHINE_START(OMAP_ZOOM2, "OMAP Zoom2 board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap_zoom_init, .init_late = omap3430_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END @@ -150,6 +150,6 @@ MACHINE_START(OMAP_ZOOM3, "OMAP Zoom3 board") .handle_irq = omap3_intc_handle_irq, .init_machine = omap_zoom_init, .init_late = omap3630_init_late, - .timer = &omap3_timer, + .timer = &omap3_sync32k_timer, .restart = omap_prcm_restart, MACHINE_END diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h index fd97f31..153aa39 100644 --- a/arch/arm/mach-omap2/common.h +++ b/arch/arm/mach-omap2/common.h @@ -135,12 +135,17 @@ static inline void omap5_map_common_io(void) extern void omap2_init_common_infrastructure(void); -extern struct sys_timer omap2_timer; -extern struct sys_timer omap3_timer; -extern struct sys_timer omap3_secure_timer; -extern struct sys_timer omap3_am33xx_timer; -extern struct sys_timer omap4_timer; -extern struct sys_timer omap5_timer; +extern struct sys_timer omap2_sync32k_timer; +extern struct sys_timer omap2_gp_timer; +extern struct sys_timer omap3_sync32k_timer; +extern struct sys_timer omap3_gp_timer; +extern struct sys_timer omap3_secure_sync32k_timer; +extern struct sys_timer omap3_secure_gp_timer; +extern struct sys_timer omap3_am33xx_gp_timer; +extern struct sys_timer omap4_local_sync32k_timer; +extern struct sys_timer omap4_local_gp_timer; +extern struct sys_timer omap5_realtime_sync32k_timer; +extern struct sys_timer omap5_realtime_gp_timer; void omap2420_init_early(void); void omap2430_init_early(void); 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" #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) { 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) { 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(); } #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); \ } -#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); #endif #ifdef CONFIG_ARCH_OMAP3 -OMAP_SYS_TIMER_INIT(3, 1, OMAP3_CLKEV_SOURCE, "ti,timer-alwon", - 2, OMAP3_MPU_SOURCE) -OMAP_SYS_TIMER(3) -OMAP_SYS_TIMER_INIT(3_secure, OMAP3_SECURE_TIMER, OMAP3_CLKEV_SOURCE, - TIMER_PROP_SECURE, 2, OMAP3_MPU_SOURCE) -OMAP_SYS_TIMER(3_secure) +OMAP_SYS_TIMER_INIT(3, sync32k, 1, OMAP3_32K_SOURCE, TIMER_PROP_ALWON, + 2, OMAP3_MPU_SOURCE); +OMAP_SYS_TIMER(3, sync32k); +OMAP_SYS_TIMER_INIT(3, gp, 1, OMAP3_MPU_SOURCE, TIMER_PROP_ALWON, + 2, OMAP3_MPU_SOURCE); +OMAP_SYS_TIMER(3, gp); +OMAP_SYS_TIMER_INIT(3_secure, sync32k, 12, OMAP3_32K_SOURCE, TIMER_PROP_SECURE, + 2, OMAP3_MPU_SOURCE); +OMAP_SYS_TIMER(3_secure, sync32k); +OMAP_SYS_TIMER_INIT(3_secure, gp, 1, OMAP3_MPU_SOURCE, TIMER_PROP_ALWON, + 2, OMAP3_MPU_SOURCE); +OMAP_SYS_TIMER(3_secure, gp); #endif #ifdef CONFIG_SOC_AM33XX -OMAP_SYS_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, "ti,timer-alwon", - 2, OMAP4_MPU_SOURCE) -OMAP_SYS_TIMER(3_am33xx) +OMAP_SYS_TIMER_INIT(3_am33xx, gp, 1, OMAP4_MPU_SOURCE, TIMER_PROP_ALWON, + 2, OMAP4_MPU_SOURCE); +OMAP_SYS_TIMER(3_am33xx, gp); #endif #ifdef CONFIG_ARCH_OMAP4 +OMAP_SYS_TIMER_INIT(4, sync32k, 1, OMAP4_32K_SOURCE, TIMER_PROP_ALWON, + 2, OMAP4_MPU_SOURCE); +OMAP_SYS_TIMER_INIT(4, gp, 1, OMAP4_MPU_SOURCE, TIMER_PROP_ALWON, + 2, OMAP4_MPU_SOURCE); #ifdef CONFIG_LOCAL_TIMERS -static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, - OMAP44XX_LOCAL_TWD_BASE, 29); -#endif - -static void __init omap4_timer_init(void) +static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29); +static void __init omap4_local_timer_init(void) { - omap2_gp_clockevent_init(1, OMAP4_CLKEV_SOURCE, "ti,timer-alwon"); - omap2_clocksource_init(2, OMAP4_MPU_SOURCE); -#ifdef CONFIG_LOCAL_TIMERS /* Local timers are not supprted on OMAP4430 ES1.0 */ if (omap_rev() != OMAP4430_REV_ES1_0) { int err; @@ -622,25 +602,45 @@ static void __init omap4_timer_init(void) if (err) pr_err("twd_local_timer_register failed %d\n", err); } +} +#else +static inline void omap4_local_timer_init(void) {} #endif + +#define OMAP4_LOCAL_TIMER_INIT(clksrc_name) \ +static void __init omap4_local_##clksrc_name##_timer_init(void) \ +{ \ + omap4_##clksrc_name##_timer_init(); \ + omap4_local_timer_init(); \ } -OMAP_SYS_TIMER(4) +OMAP4_LOCAL_TIMER_INIT(sync32k); +OMAP_SYS_TIMER(4_local, sync32k); +OMAP4_LOCAL_TIMER_INIT(gp); +OMAP_SYS_TIMER(4_local, gp); #endif #ifdef CONFIG_SOC_OMAP5 -static void __init omap5_timer_init(void) -{ - int err; - - omap2_gp_clockevent_init(1, OMAP4_CLKEV_SOURCE, "ti,timer-alwon"); - omap2_clocksource_init(2, OMAP4_MPU_SOURCE); - realtime_counter_init(); - - err = arch_timer_of_register(); - if (err) - pr_err("%s: arch_timer_register failed %d\n", __func__, err); +OMAP_SYS_TIMER_INIT(5, sync32k, 1, OMAP4_32K_SOURCE, TIMER_PROP_ALWON, + 2, OMAP4_MPU_SOURCE); +OMAP_SYS_TIMER_INIT(5, gp, 1, OMAP4_MPU_SOURCE, TIMER_PROP_ALWON, + 2, OMAP4_MPU_SOURCE); +#define OMAP5_REALTIME_TIMER_INIT(clksrc_name) \ +static void __init omap5_realtime_##clksrc_name##_timer_init(void) \ +{ \ + int err; \ + \ + omap5_##clksrc_name##_timer_init(); \ + realtime_counter_init(); \ + \ + err = arch_timer_of_register(); \ + if (err) \ + pr_err("%s: arch_timer_register failed %d\n", \ + __func__, err); \ } -OMAP_SYS_TIMER(5) +OMAP5_REALTIME_TIMER_INIT(sync32k); +OMAP_SYS_TIMER(5_realtime, sync32k); +OMAP5_REALTIME_TIMER_INIT(gp); +OMAP_SYS_TIMER(5_realtime, gp); #endif /** diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig index 82fcb20..9b36a2ad 100644 --- a/arch/arm/plat-omap/Kconfig +++ b/arch/arm/plat-omap/Kconfig @@ -154,6 +154,11 @@ config OMAP_32K_TIMER intra-tick resolution than OMAP_MPU_TIMER. The 32KHz timer is currently only available for OMAP16XX, 24XX, 34XX and OMAP4/5. + On OMAP2PLUS this value is only used for CONFIG_HZ and + CLOCK_TICK_RATE compile time calculation. + The actual timer selection is done in the in the board file + through (DT_)MACHINE_START structure. + config OMAP3_L2_AUX_SECURE_SAVE_RESTORE bool "OMAP3 HS/EMU save and restore for L2 AUX control register" depends on ARCH_OMAP3 && PM
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> --- Finally I'm sending this out... I've lost following Tony's branches and deciding which one to base on, so I used linux-omap/master as a base for the patch. Tony, tell me if you want it based on some other branch. This has been compile tested on omap1|2plus_defconfig only. arch/arm/mach-omap2/board-2430sdp.c | 2 +- arch/arm/mach-omap2/board-3430sdp.c | 2 +- arch/arm/mach-omap2/board-3630sdp.c | 2 +- arch/arm/mach-omap2/board-4430sdp.c | 2 +- arch/arm/mach-omap2/board-am3517crane.c | 2 +- arch/arm/mach-omap2/board-am3517evm.c | 2 +- arch/arm/mach-omap2/board-apollon.c | 2 +- arch/arm/mach-omap2/board-cm-t35.c | 18 ++-- arch/arm/mach-omap2/board-cm-t3517.c | 2 +- arch/arm/mach-omap2/board-devkit8000.c | 2 +- arch/arm/mach-omap2/board-generic.c | 14 ++-- arch/arm/mach-omap2/board-h4.c | 2 +- arch/arm/mach-omap2/board-igep0020.c | 4 +- arch/arm/mach-omap2/board-ldp.c | 2 +- arch/arm/mach-omap2/board-n8x0.c | 6 +- arch/arm/mach-omap2/board-omap3beagle.c | 2 +- arch/arm/mach-omap2/board-omap3encore.c | 2 +- arch/arm/mach-omap2/board-omap3evm.c | 2 +- arch/arm/mach-omap2/board-omap3logic.c | 4 +- arch/arm/mach-omap2/board-omap3pandora.c | 2 +- arch/arm/mach-omap2/board-omap3stalker.c | 2 +- arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- arch/arm/mach-omap2/board-omap4panda.c | 2 +- arch/arm/mach-omap2/board-omap4pcm049.c | 2 +- arch/arm/mach-omap2/board-overo.c | 2 +- arch/arm/mach-omap2/board-rm680.c | 4 +- arch/arm/mach-omap2/board-rx51.c | 2 +- arch/arm/mach-omap2/board-ti8168evm.c | 4 +- arch/arm/mach-omap2/board-zoom.c | 4 +- arch/arm/mach-omap2/common.h | 17 ++- arch/arm/mach-omap2/timer.c | 162 ++++++++++++++-------------- arch/arm/plat-omap/Kconfig | 5 + 32 files changed, 147 insertions(+), 137 deletions(-)