ARM: OMAP2+: timer: remove CONFIG_OMAP_32K_TIMER

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.

3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

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

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

Quoted 11 lines · Igor Grinberg

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.

Quoted 12 lines

3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Quoted 6 lines

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

Quoted 21 lines

arch/arm/mach-omap2/board-2430sdp.c +1 -1

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

arch/arm/mach-omap2/board-3430sdp.c +1 -1

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

Quoted 3 lines · Tony Lindgren

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.

Quoted 22 lines

Quoted 20 lines

arch/arm/mach-omap2/board-2430sdp.c +1 -1

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

arch/arm/mach-omap2/board-3430sdp.c +1 -1

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

...

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 10 lines · Igor Grinberg

Quoted 3 lines · Tony Lindgren

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?

Quoted 4 lines

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.

Quoted 22 lines

Quoted 22 lines

Quoted 20 lines

arch/arm/mach-omap2/board-2430sdp.c +1 -1

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

arch/arm/mach-omap2/board-3430sdp.c +1 -1

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

...

Quoted 6 lines

Quoted 2 lines

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?

Quoted 3 lines

Quoted 2 lines

Then we just need a new timer entries for the hardware that does not have the sycn32k_timer available?

Quoted 2 lines

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

Quoted 10 lines · Igor Grinberg

Quoted 3 lines · Tony Lindgren

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?

Quoted 5 lines

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.

Quoted 22 lines

Quoted 22 lines

Quoted 20 lines

arch/arm/mach-omap2/board-2430sdp.c +1 -1

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

arch/arm/mach-omap2/board-3430sdp.c +1 -1

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

...

Quoted 6 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 12 lines · Tony Lindgren

Quoted 10 lines · Igor Grinberg

Quoted 3 lines · Tony Lindgren

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.

Quoted 3 lines

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.

Quoted 37 lines

Quoted 5 lines

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.

Quoted 28 lines

Quoted 22 lines

Quoted 20 lines

arch/arm/mach-omap2/board-2430sdp.c +1 -1

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

arch/arm/mach-omap2/board-3430sdp.c +1 -1

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

...

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 17 lines · Igor Grinberg

Quoted 12 lines · Tony Lindgren

Quoted 10 lines · Igor Grinberg

Quoted 3 lines · Tony Lindgren

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.

Quoted 3 lines

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?

Quoted 7 lines

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.

Quoted 38 lines

Quoted 38 lines

Quoted 6 lines

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.

Quoted 28 lines

Quoted 22 lines

Quoted 20 lines

arch/arm/mach-omap2/board-2430sdp.c +1 -1

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

arch/arm/mach-omap2/board-3430sdp.c +1 -1

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

...

Quoted 2 lines

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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 4 lines · Tony Lindgren

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.

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c +1 -1

 #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);
 	}

arch/arm/mach-omap2/timer.c +0 -1

 {
 	int ret;
 	struct device_node *np = NULL;

@@ -439,15 +426,9 @@ static int __init omap2_sync32k_clocksource_init(void)
 
 	return ret;
 }
-#else

arch/arm/mach-omap2/timer.c +2 -8

 {
 	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

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\
 
 

arch/arm/mach-omap2/timer.c +4 -4

 }
 
-#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,			\
+}
 

Hi Igor,

Quoted 22 lines · Igor Grinberg

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

[snip]

Quoted 13 lines

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c line 67

Quoted 10 lines

arch/arm/mach-omap2/timer.c +1 -8

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

Quoted 16 lines

arch/arm/mach-omap2/timer.c +1 -1

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

arch/arm/mach-omap2/timer.c static u32 notrace dmtimer_read_sched_clock(void) · line 370

Quoted 4 lines

arch/arm/mach-omap2/timer.c +1 -2

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

Quoted 15 lines

arch/arm/mach-omap2/timer.c +0 -6

 {
 	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
 

arch/arm/mach-omap2/timer.c static int __init omap2_sync32k_clocksource_init(void) · line 431

Quoted 4 lines

arch/arm/mach-omap2/timer.c +2 -2

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

Quoted 24 lines

arch/arm/mach-omap2/timer.c +2 -13

 {
 	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);

arch/arm/mach-omap2/timer.c static void __init omap2_gptimer_clocksource_init(int gptimer_id, · line 454

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -3

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

Quoted 12 lines

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 545

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -1

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

Quoted 12 lines

arch/arm/mach-omap2/timer.c +4 -4

 }
 
-#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,			\
+}
 

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 559

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -3

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

Quoted 55 lines · Jon Hunter

Hi Igor,

Quoted 22 lines · Igor Grinberg

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

[snip]

Quoted 13 lines

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c line 67

Quoted 10 lines

arch/arm/mach-omap2/timer.c +1 -8

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

Quoted 3 lines

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.

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +1 -1

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

Quoted 6 lines

arch/arm/mach-omap2/timer.c static u32 notrace dmtimer_read_sched_clock(void) · line 370

Quoted 4 lines

arch/arm/mach-omap2/timer.c +1 -2

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

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +0 -6

 {
 	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
 

Quoted 7 lines

arch/arm/mach-omap2/timer.c static int __init omap2_sync32k_clocksource_init(void) · line 431

Quoted 4 lines

arch/arm/mach-omap2/timer.c +2 -2

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

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-omap2/timer.c +2 -13

 {
 	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);

Quoted 8 lines

arch/arm/mach-omap2/timer.c static void __init omap2_gptimer_clocksource_init(int gptimer_id, · line 454

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -3

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

Quoted 14 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 545

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -1

 {									\
 	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);

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 7 lines

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.

Quoted 12 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +4 -4

 }
 
-#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,			\
+}
 

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 559

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -3

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

Quoted 60 lines · Igor Grinberg

Quoted 55 lines · Jon Hunter

Hi Igor,

Quoted 22 lines · Igor Grinberg

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

[snip]

Quoted 13 lines

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c line 67

Quoted 10 lines

arch/arm/mach-omap2/timer.c +1 -8

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

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +1 -1

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

Quoted 6 lines

arch/arm/mach-omap2/timer.c static u32 notrace dmtimer_read_sched_clock(void) · line 370

Quoted 4 lines

arch/arm/mach-omap2/timer.c +1 -2

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

Quoted 4 lines

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.

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +0 -6

 {
 	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
 

Quoted 7 lines

arch/arm/mach-omap2/timer.c static int __init omap2_sync32k_clocksource_init(void) · line 431

Quoted 4 lines

arch/arm/mach-omap2/timer.c +2 -2

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

Quoted 3 lines

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.

Quoted 33 lines

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-omap2/timer.c +2 -13

 {
 	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);

Quoted 8 lines

arch/arm/mach-omap2/timer.c static void __init omap2_gptimer_clocksource_init(int gptimer_id, · line 454

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -3

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

Quoted 2 lines

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

Ok, thanks.

Quoted 28 lines

Quoted 14 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 545

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -1

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

Quoted 8 lines

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

(quoted diff) +0 -0

 	omap2_gp_clocksource_init((clksrc_nr), clksrc_src);
 	return;
 }
 omap2_gp_clocksource_init((clksrc_nr), clksrc_src);

Quoted 6 lines

Quoted 5 lines

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.

Quoted 2 lines

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?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 15 lines

Quoted 7 lines

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

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 26 lines

Quoted 12 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +4 -4

 }
 
-#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,			\
+}
 

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 559

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -3

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

Quoted 5 lines

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

Quoted 221 lines · Jon

Quoted 60 lines · Igor Grinberg

Quoted 55 lines · Jon Hunter

Hi Igor,

Quoted 22 lines · Igor Grinberg

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

[snip]

Quoted 13 lines

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c line 67

Quoted 10 lines

arch/arm/mach-omap2/timer.c +1 -8

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

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +1 -1

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

Quoted 6 lines

arch/arm/mach-omap2/timer.c static u32 notrace dmtimer_read_sched_clock(void) · line 370

Quoted 4 lines

arch/arm/mach-omap2/timer.c +1 -2

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

Quoted 4 lines

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.

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +0 -6

 {
 	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
 

Quoted 7 lines

arch/arm/mach-omap2/timer.c static int __init omap2_sync32k_clocksource_init(void) · line 431

Quoted 4 lines

arch/arm/mach-omap2/timer.c +2 -2

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

Quoted 3 lines

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.

Quoted 33 lines

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-omap2/timer.c +2 -13

 {
 	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);

Quoted 8 lines

arch/arm/mach-omap2/timer.c static void __init omap2_gptimer_clocksource_init(int gptimer_id, · line 454

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -3

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

Quoted 2 lines

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

Ok, thanks.

Quoted 28 lines

Quoted 14 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 545

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -1

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

Quoted 8 lines

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

(quoted diff) +0 -0

 	omap2_gp_clocksource_init((clksrc_nr), clksrc_src);
 	return;
 }
 omap2_gp_clocksource_init((clksrc_nr), clksrc_src);

Quoted 8 lines

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 222 lines · Vaibhav

Quoted 221 lines · Jon

Quoted 60 lines · Igor Grinberg

Quoted 55 lines · Jon Hunter

Hi Igor,

Quoted 22 lines · Igor Grinberg

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

[snip]

Quoted 13 lines

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c line 67

Quoted 10 lines

arch/arm/mach-omap2/timer.c +1 -8

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

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +1 -1

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

Quoted 6 lines

arch/arm/mach-omap2/timer.c static u32 notrace dmtimer_read_sched_clock(void) · line 370

Quoted 4 lines

arch/arm/mach-omap2/timer.c +1 -2

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

Quoted 4 lines

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.

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +0 -6

 {
 	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
 

Quoted 7 lines

arch/arm/mach-omap2/timer.c static int __init omap2_sync32k_clocksource_init(void) · line 431

Quoted 4 lines

arch/arm/mach-omap2/timer.c +2 -2

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

Quoted 3 lines

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.

Quoted 33 lines

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-omap2/timer.c +2 -13

 {
 	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);

Quoted 8 lines

arch/arm/mach-omap2/timer.c static void __init omap2_gptimer_clocksource_init(int gptimer_id, · line 454

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -3

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

Quoted 2 lines

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

Ok, thanks.

Quoted 28 lines

Quoted 14 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 545

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -1

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

Quoted 8 lines

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

(quoted diff) +0 -0

 	omap2_gp_clocksource_init((clksrc_nr), clksrc_src);
 	return;
 }
 omap2_gp_clocksource_init((clksrc_nr), clksrc_src);

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 5 lines

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

Quoted 11 lines

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.

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 1 line · Vaibhav

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

[snip]

Quoted 17 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 7 lines

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?

Quoted 41 lines · Jon

Quoted 1 line · Vaibhav

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

[snip]

Quoted 17 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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?

Quoted 48 lines · Vaibhav

Quoted 41 lines · Jon

Quoted 1 line · Vaibhav

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

[snip]

Quoted 17 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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?

Quoted 4 lines

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

Quoted 55 lines · Jon

Quoted 48 lines · Vaibhav

Quoted 41 lines · Jon

Quoted 1 line · Vaibhav

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

[snip]

Quoted 17 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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?

Quoted 4 lines

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?

Quoted 8 lines

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.

Quoted 5 lines

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?

Quoted 41 lines · Jon

Quoted 1 line · Vaibhav

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

[snip]

Quoted 17 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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?

Quoted 51 lines · Vaibhav

Quoted 41 lines · Jon

Quoted 1 line · Vaibhav

On Thu, Nov 08, 2012 at 23:09:57, Hunter, Jon wrote:

[snip]

Quoted 17 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 1 line

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.

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.

Quoted 3 lines · Jon Hunter

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

Quoted 6 lines · Paul Walmsley

Quoted 3 lines · Jon Hunter

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.

Quoted 4 lines

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

and so the gptimer will still have a 32k clock source.

Quoted 12 lines · Jon Hunter

Quoted 10 lines · Paul Walmsley

Quoted 3 lines · Jon Hunter

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

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 14 lines · Paul Walmsley

Quoted 12 lines · Jon Hunter

Quoted 10 lines · Paul Walmsley

Quoted 3 lines · Jon Hunter

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

Quoted 5 lines

Quoted 2 lines

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!

Quoted 2 lines

Quoted 1 line

and so the gptimer will still have a 32k clock source.

Quoted 3 lines

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.

Quoted 26 lines · Jon Hunter

Quoted 14 lines · Paul Walmsley

Quoted 12 lines · Jon Hunter

Quoted 10 lines · Paul Walmsley

Quoted 3 lines · Jon Hunter

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

Quoted 5 lines

Quoted 2 lines

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!

Quoted 2 lines

Quoted 1 line

and so the gptimer will still have a 32k clock source.

Quoted 7 lines

Quoted 3 lines

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

Signature · 3 lines

-- 
Regards,
Igor.

Quoted 42 lines · Igor Grinberg

Quoted 26 lines · Jon Hunter

Quoted 14 lines · Paul Walmsley

Quoted 12 lines · Jon Hunter

Quoted 10 lines · Paul Walmsley

Quoted 3 lines · Jon Hunter

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

Quoted 5 lines

Quoted 2 lines

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!

Quoted 2 lines

Quoted 1 line

and so the gptimer will still have a 32k clock source.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 47 lines · Vaibhav

Quoted 45 lines · Igor Grinberg

Quoted 26 lines · Jon Hunter

Quoted 14 lines · Paul Walmsley

Quoted 12 lines · Jon Hunter

Quoted 10 lines · Paul Walmsley

Quoted 3 lines · Jon Hunter

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

Quoted 5 lines

Quoted 2 lines

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!

Quoted 2 lines

Quoted 1 line

and so the gptimer will still have a 32k clock source.

Quoted 7 lines

Quoted 3 lines

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

Quoted 1 line

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.

Signature · 3 lines

-- 
Regards,
Igor.

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.

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.

Quoted 60 lines · Igor Grinberg

Quoted 55 lines · Jon Hunter

Hi Igor,

Quoted 22 lines · Igor Grinberg

CONFIG_OMAP_32K_TIMER is kind of standing on the single zImage way. Make OMAP2+ timer code independant from the CONFIG_OMAP_32K_TIMER setting.
To remove the dependancy, several conversions/additions had to be done:
1) Timer structures and initialization functions are named by the platform name and the clock source in use. The decision which timer is used is done statically from the machine_desc structure. In the future it should come from DT.
2) Settings under the CONFIG_OMAP_32K_TIMER option are expanded into separate timer structures along with the timer init functions. This removes the CONFIG_OMAP_32K_TIMER on OMAP2+ timer code.
3) Since we have all the timers defined inside machine_desc structure and we no longer need the fallback to gp_timer clock source in case 32k_timer clock source is unavailable (namely on AM33xx), we no longer need the #ifdef around __omap2_sync32k_clocksource_init() function. Remove the #ifdef CONFIG_OMAP_32K_TIMER around the __omap2_sync32k_clocksource_init() function.

Signed-off-by Igor Grinberg <[email protected]>

Cc Jon Hunter <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

[snip]

Quoted 13 lines

arch/arm/mach-omap2/timer.c +0 -5

index 684d2fc..a4ad7a0 100644
@@ -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

arch/arm/mach-omap2/timer.c line 67

Quoted 10 lines

arch/arm/mach-omap2/timer.c +1 -8

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

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +1 -1

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

Quoted 6 lines

arch/arm/mach-omap2/timer.c static u32 notrace dmtimer_read_sched_clock(void) · line 370

Quoted 4 lines

arch/arm/mach-omap2/timer.c +1 -2

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

Quoted 4 lines

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.

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-omap2/timer.c +0 -6

 {
 	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
 

Quoted 7 lines

arch/arm/mach-omap2/timer.c static int __init omap2_sync32k_clocksource_init(void) · line 431

Quoted 4 lines

arch/arm/mach-omap2/timer.c +2 -2

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

Quoted 3 lines

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.

Quoted 33 lines

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-omap2/timer.c +2 -13

 {
 	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);

Quoted 8 lines

arch/arm/mach-omap2/timer.c static void __init omap2_gptimer_clocksource_init(int gptimer_id, · line 454

Quoted 5 lines

arch/arm/mach-omap2/timer.c +1 -3

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

Quoted 2 lines

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

Ok, thanks.

Quoted 28 lines

Quoted 14 lines

Quoted 12 lines

arch/arm/mach-omap2/timer.c +3 -3

 #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)		\

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 545

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -1

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

Quoted 8 lines

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 2 lines

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?

Quoted 7 lines

Quoted 4 lines

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.

Quoted 16 lines

Quoted 15 lines

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

Quoted 5 lines

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.

Quoted 30 lines

Quoted 13 lines

Quoted 13 lines

arch/arm/mach-omap2/timer.c +4 -4

 }
 
-#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,			\
+}
 

Quoted 13 lines

arch/arm/mach-omap2/timer.c static inline void __init realtime_counter_init(void) · line 559

Quoted 10 lines

arch/arm/mach-omap2/timer.c +6 -3

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

Quoted 2 lines

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.

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

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 2 lines

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.

Quoted 5 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 9 lines

Quoted 2 lines

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.

Quoted 16 lines · Vaibhav

Quoted 14 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 20 lines · Jon Hunter

Quoted 18 lines · Vaibhav

Quoted 5 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 9 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 2 lines

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

Quoted 30 lines · Igor Grinberg

Quoted 26 lines · Jon Hunter

Quoted 16 lines · Vaibhav

Quoted 5 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 9 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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:

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?

Quoted 28 lines · Jon Hunter

Quoted 26 lines · Igor Grinberg

Quoted 23 lines · Jon Hunter

Quoted 20 lines · Vaibhav

Quoted 5 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 9 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 43 lines · Igor Grinberg

Quoted 28 lines · Jon Hunter

Quoted 26 lines · Igor Grinberg

Quoted 23 lines · Jon Hunter

Quoted 20 lines · Vaibhav

Quoted 5 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 9 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

Should we be doing this now instead of adding all these static timer init functions?

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 16 lines · Vaibhav

Quoted 14 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 18 lines · Jon

Quoted 16 lines · Vaibhav

Quoted 14 lines · Jon

On 11/08/2012 01:59 AM, Igor Grinberg wrote:

[snip]

Quoted 2 lines

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.

Quoted 9 lines

Quoted 4 lines

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 at X {
		compatible = "crystal-32k, per-32k, xyz";
		clock-frequency = <32768>;
	};
	...
};

Timer {

	ref-clock = <&rtc_clk>;
};

What do you think?