ARM: OMAP2+: System timer updates

Currently on boot, when displaying the name of the gptimer used for clockevents and clocksource timers, the timer ID is shown. However, when booting with device-tree, the timer ID is not used to select a gptimer but a timer property. Hence, it is possible that the timer selected when booting with device-tree does not match the ID shown. Therefore, instead display the HWMOD name of the gptimer and use the HWMOD name as the name of clockevent and clocksource timer (if a gptimer is used).

Signed-off-by Jon Hunter <[email protected]>

---
 arch/arm/mach-omap2/timer.c |   44 +++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

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

index 72c2ca1..18cb856 100644

Hi Jon,

Quoted 22 lines · Jon

Currently on boot, when displaying the name of the gptimer used for clockevents and clocksource timers, the timer ID is shown. However, when booting with device-tree, the timer ID is not used to select a gptimer but a timer property. Hence, it is possible that the timer selected when booting with device-tree does not match the ID shown. Therefore, instead display the HWMOD name of the gptimer and use the HWMOD name as the name of clockevent and clocksource timer (if a gptimer is used).no

Signed-off-by Jon Hunter <[email protected]>

---
 arch/arm/mach-omap2/timer.c |   44 +++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

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

index 72c2ca1..18cb856 100644
@@ -71,6 +71,9 @@
 #define INCREMENTER_DENUMERATOR_RELOAD_OFFSET		0x14
 #define NUMERATOR_DENUMERATOR_MASK			0xfffff000
 

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

Quoted 3 lines

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

+/* Timer name needs to be big enough to store a string of "timerXX" */
+static char timer_name[10];
+

Why not move this inside omap_dm_timer_init_one()?

Quoted 6 lines

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

Quoted 4 lines

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

+/* Timer name needs to be big enough to store a string of "timerXX" */
+static char timer_name[10];
+

Why not move this inside omap_dm_timer_init_one()?

In the non-DT case, the name member of the clocksource/event struct will point to this array and so it needs to reside in memory permanently and not just temporary. Once we migrate completely to DT then we will be able to remove this completely. See following snippet ...

On Fri, Feb 01, 2013 at 14:23:43, Hunter, Jon wrote:
[...]

Quoted 12 lines

Quoted 6 lines

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

Quoted 4 lines

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

+/* Timer name needs to be big enough to store a string of "timerXX" */
+static char timer_name[10];
+

Why not move this inside omap_dm_timer_init_one()?

In the non-DT case, the name member of the clocksource/event struct will point to this array and so it needs to reside in memory permanently and not just temporary. Once we migrate completely to DT then we will be able to remove this completely. See following snippet ...

arch/arm/mach-omap2/timer.c static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, · line 251

Quoted 4 lines

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

-		sprintf(name, "timer%d", gptimer_id);
-		oh_name = name;
+		sprintf(timer_name, "timer%d", gptimer_id);
+		*name = timer_name;

Ok. But in case of non-DT boot if someone selects gptimers for both clkevt and clksrc, both the name members will end up pointing to the same memory location. To be specific, in the current code the clkevt timer name will point to the clksrc name. This won't be noticeable during boot since the clkevt name gets printed before it is over-written.

There is a lot of redundancy in the definitions for the various system timers for OMAP2+ devices. For example, the omap3_am33xx_gptimer_timer_init() function is the same as the omap3_gp_gptimer_timer_init() function and the function omap2_sync32k_timer_init() can be re-used for OMAP4/5 devices. Therefore, consolidate the definitions to simplify the code.

Signed-off-by Jon Hunter <[email protected]>

There is a lot of redundancy in the definitions for the various system timers for OMAP2+ devices. For example, the omap3_am33xx_gptimer_timer_init() function is the same as the omap3_gp_gptimer_timer_init() function and the function omap2_sync32k_timer_init() can be re-used for OMAP4/5 devices.

When booting with device-tree for OMAP3 and AM335x devices and a gptimer is used as the clocksource (which is always the case for AM335x), a gptimer located in a power domain that is not always-on is selected. Ideally we should use a gptimer located in a power domain that is always on (such as the wake-up domain) so that time can be maintained during a kernel suspend without keeping on additional power domains unnecessarily.

In order to fix this so that we can select a gptimer located in a power domain that is always-on, the following changes were made ...
1. Currently, only when selecting a gptimer to use for a clockevent timer, do we pass a timer property that can be used to select a specific gptimer. Change this so that we can pass a property when selecting a gptimer to use for a clocksource timer too.
2. Currently, when selecting either a gptimer to use for a clockevent timer or a clocksource timer and no timer property is passed, then the first available timer is selected regardless of the properties it has. Change this so that if no properties are passed, then a timer that does not have additional features (such as always-on, dsp-irq, pwm, and secure) is selected.

Please note that using a gptimer for both clocksource and clockevents can have a system power impact during idle. The reason being is that OMAP and AMxxx devices typically only have one gptimer in a power domain that is always-on. Therefore when the kernel is idle both the clocksource and clockevent timers will be active and this will keep additional power domains on. During kernel suspend, only the clocksource timer is active and therefore, it is better to use a gptimer in a power domain that is always-on for clocksource.

Signed-off-by Jon Hunter <[email protected]>

---
 arch/arm/mach-omap2/timer.c |   32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

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

index af20be7..acf9f82 100644

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

@@ -557,19 +567,19 @@ void __init omap##name##_sync32k_timer_init(void)		\
 #if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP4) ||		\
 	defined(CONFIG_SOC_OMAP5)
 OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon",
-			2, "timer_sys_ck");
+			2, "timer_sys_ck", NULL);
 #endif
 
 #ifdef CONFIG_ARCH_OMAP3
 OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon",
-			2, "timer_sys_ck");
+			2, "timer_sys_ck", NULL);
 OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure",
-			2, "timer_sys_ck");
+			2, "timer_sys_ck", NULL);
 #endif /* CONFIG_ARCH_OMAP3 */
 
 #if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM33XX)
-OMAP_SYS_GP_TIMER_INIT(3, 1, "timer_sys_ck", "ti,timer-alwon",
-		       2, "timer_sys_ck");
+OMAP_SYS_GP_TIMER_INIT(3, 2, "timer_sys_ck", NULL,
+		       1, "timer_sys_ck", "ti,timer-alwon");
 #endif
 
 #ifdef CONFIG_ARCH_OMAP4

Quoted 20 lines · Jon Hunter

When booting with device-tree for OMAP3 and AM335x devices and a gptimer is used as the clocksource (which is always the case for AM335x), a gptimer located in a power domain that is not always-on is selected. Ideally we should use a gptimer located in a power domain that is always on (such as the wake-up domain) so that time can be maintained during a kernel suspend without keeping on additional power domains unnecessarily.

In order to fix this so that we can select a gptimer located in a power domain that is always-on, the following changes were made ...
1. Currently, only when selecting a gptimer to use for a clockevent timer, do we pass a timer property that can be used to select a specific gptimer. Change this so that we can pass a property when selecting a gptimer to use for a clocksource timer too.
2. Currently, when selecting either a gptimer to use for a clockevent timer or a clocksource timer and no timer property is passed, then the first available timer is selected regardless of the properties it has. Change this so that if no properties are passed, then a timer that does not have additional features (such as always-on, dsp-irq, pwm, and secure) is selected.

Quoted 8 lines

Please note that using a gptimer for both clocksource and clockevents can have a system power impact during idle. The reason being is that OMAP and AMxxx devices typically only have one gptimer in a power domain that is always-on. Therefore when the kernel is idle both the clocksource and clockevent timers will be active and this will keep additional power domains on. During kernel suspend, only the clocksource timer is active and therefore, it is better to use a gptimer in a power domain that is always-on for clocksource.

This should explain the gptimer number switch in the
#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM33XX) section below, right?
I would really like to see that clearly stated in the commit message. For instance:
... it is better to use a gptimer in a power domain that is always-on for clocksource. Therefore we switch to use the first gptimer for clocksource and the second for clockevents.

Quoted 2 lines

Signed-off-by Jon Hunter <[email protected]>

Apart from above,

Acked-by Igor Grinberg <[email protected]>

When booting with device-tree for OMAP3 and AM335x devices and a gptimer is used as the clocksource (which is always the case for AM335x), a gptimer located in a power domain that is not always-on is selected. Ideally we should use a gptimer located in a power domain that is always on (such as the wake-up domain) so that time can be maintained during a kernel suspend without keeping on additional power domains unnecessarily.

In order to fix this so that we can select a gptimer located in a power domain that is always-on, the following changes were made ...
1. Currently, only when selecting a gptimer to use for a clockevent timer, do we pass a timer property that can be used to select a specific gptimer. Change this so that we can pass a property when selecting a gptimer to use for a clocksource timer too.
2. Currently, when selecting either a gptimer to use for a clockevent timer or a clocksource timer and no timer property is passed, then the first available timer is selected regardless of the properties it has. Change this so that if no properties are passed, then a timer that does not have additional features (such as always-on, dsp-irq, pwm, and secure) is selected.

Please note that using a gptimer for both clocksource and clockevents can have a system power impact during idle. The reason being is that OMAP and AMxxx devices typically only have one gptimer in a power domain that is always-on. Therefore when the kernel is idle both the clocksource and clockevent timers will be active and this will keep additional power domains on. During kernel suspend, only the clocksource timer is active and therefore, it is better to use a gptimer in a power domain that is always-on for clocksource.

Hi Jon,

Quoted 20 lines · Jon

When booting with device-tree for OMAP3 and AM335x devices and a gptimer is used as the clocksource (which is always the case for AM335x), a gptimer located in a power domain that is not always-on is selected. Ideally we should use a gptimer located in a power domain that is always on (such as the wake-up domain) so that time can be maintained during a kernel suspend without keeping on additional power domains unnecessarily.

In order to fix this so that we can select a gptimer located in a power domain that is always-on, the following changes were made ...
1. Currently, only when selecting a gptimer to use for a clockevent timer, do we pass a timer property that can be used to select a specific gptimer. Change this so that we can pass a property when selecting a gptimer to use for a clocksource timer too.
2. Currently, when selecting either a gptimer to use for a clockevent timer or a clocksource timer and no timer property is passed, then the first available timer is selected regardless of the properties it has. Change this so that if no properties are passed, then a timer that does not have additional features (such as always-on, dsp-irq, pwm, and secure) is selected.

Quoted 9 lines

Please note that using a gptimer for both clocksource and clockevents can have a system power impact during idle. The reason being is that OMAP and AMxxx devices typically only have one gptimer in a power domain that is always-on. Therefore when the kernel is idle both the clocksource and clockevent timers will be active and this will keep additional power domains on. During kernel suspend, only the clocksource timer is active and therefore, it is better to use a gptimer in a power domain that is always-on for clocksource.

It's always a pleasure reading the changelog in your patches :)

[...]

arch/arm/mach-omap2/timer.c void __init omap##name##_sync32k_timer_init(void) \ · line 583

Quoted 8 lines

#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM33XX)

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

-OMAP_SYS_GP_TIMER_INIT(3, 1, "timer_sys_ck", "ti,timer-alwon",
-		       2, "timer_sys_ck");
+OMAP_SYS_GP_TIMER_INIT(3, 2, "timer_sys_ck", NULL,
+		       1, "timer_sys_ck", "ti,timer-alwon");
 #endif

Minor point... was the intention of changing of clkev_nr and clksrc_nr to make it consistent with what happens on AM33xx which is DT-only?

Quoted 33 lines · Vaibhav

Hi Jon,

Quoted 20 lines · Jon

When booting with device-tree for OMAP3 and AM335x devices and a gptimer is used as the clocksource (which is always the case for AM335x), a gptimer located in a power domain that is not always-on is selected. Ideally we should use a gptimer located in a power domain that is always on (such as the wake-up domain) so that time can be maintained during a kernel suspend without keeping on additional power domains unnecessarily.

In order to fix this so that we can select a gptimer located in a power domain that is always-on, the following changes were made ...
1. Currently, only when selecting a gptimer to use for a clockevent timer, do we pass a timer property that can be used to select a specific gptimer. Change this so that we can pass a property when selecting a gptimer to use for a clocksource timer too.
2. Currently, when selecting either a gptimer to use for a clockevent timer or a clocksource timer and no timer property is passed, then the first available timer is selected regardless of the properties it has. Change this so that if no properties are passed, then a timer that does not have additional features (such as always-on, dsp-irq, pwm, and secure) is selected.

Quoted 9 lines

Please note that using a gptimer for both clocksource and clockevents can have a system power impact during idle. The reason being is that OMAP and AMxxx devices typically only have one gptimer in a power domain that is always-on. Therefore when the kernel is idle both the clocksource and clockevent timers will be active and this will keep additional power domains on. During kernel suspend, only the clocksource timer is active and therefore, it is better to use a gptimer in a power domain that is always-on for clocksource.

Quoted 1 line

It's always a pleasure reading the changelog in your patches :)

Thanks.

Quoted 12 lines

[...]

arch/arm/mach-omap2/timer.c void __init omap##name##_sync32k_timer_init(void) \ · line 583

Quoted 8 lines

#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM33XX)

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

-OMAP_SYS_GP_TIMER_INIT(3, 1, "timer_sys_ck", "ti,timer-alwon",
-		       2, "timer_sys_ck");
+OMAP_SYS_GP_TIMER_INIT(3, 2, "timer_sys_ck", NULL,
+		       1, "timer_sys_ck", "ti,timer-alwon");
 #endif

Minor point... was the intention of changing of clkev_nr and clksrc_nr to make it consistent with what happens on AM33xx which is DT-only?

I don't see this as being DT specific. It is more of a policy change to ensure a wake-up domain timer is used for clocksource when we are using gptimers for both clocksource and clockevents. It was your patch for AM335x that made me see the need for this, if that makes sense. May be I should have referenced that in the changelog.

Quoted 6 lines

I don't see this as being DT specific. It is more of a policy change to ensure a wake-up domain timer is used for clocksource when we are using gptimers for both clocksource and clockevents. It was your patch for AM335x that made me see the need for this, if that makes sense. May be I should have referenced that in the changelog.

Sorry to be clear, I don't see the policy change as DT specific.

In answer to your question, yes clkev_nr and clksrc_nr are changed so the policy it is consistent regardless of whether you use DT or not. In other words, an OMAP3 board using a gptimer for clocksource (such as cm-t3517) and does not use DT, would work the same as AM335x with DT.

This series consists mainly of clean-ups for clockevents and clocksource timers on OMAP2+ devices. The most significant change in functionality comes from the 5th patch which is changing the selection of the clocksource timer for OMAP3 and AM335x devices when gptimers are used for clocksource. This change came about from Vaibhav Bedia's series for AM335x [1]. See patch for more details on the exact nature of the change.

Boot tested with and without device-tree on OMAP2420 H4, OMAP3430 SDP, OMAP3430 Beagle Board, OMAP4430 SDP and AM335x EVM (AM335x only supports device-tree boot).

This series is based upon ARM-SoC next branch.

[1] https://patchwork.kernel.org/patch/1921421/

Jon Hunter (5):
ARM: OMAP2+: Display correct system timer name
ARM: OMAP2+: Remove hard-coded test on timer ID
ARM: OMAP2+: Simplify system timer clock definitions
ARM: OMAP2+: Simplify system timers definitions
ARM: OMAP3: Update clocksource timer selection

This series consists mainly of clean-ups for clockevents and clocksource timers on OMAP2+ devices. The most significant change in functionality comes from the 5th patch which is changing the selection of the clocksource timer for OMAP3 and AM335x devices when gptimers are used for clocksource. This change came about from Vaibhav Bedia's series for AM335x [1]. See patch for more details on the exact nature of the change.

Boot tested with and without device-tree on OMAP2420 H4, OMAP3430 SDP, OMAP3430 Beagle Board, OMAP4430 SDP and AM335x EVM (AM335x only supports device-tree boot).