ARM: OMAP: Make OMAP clocksource source selection runtime

Current OMAP code supports couple of clocksource options based on compilation flag (CONFIG_OMAP_32K_TIMER). The 32KHz sync-timer and a gptimer which can run on 32KHz or system clock (e.g 38.4 MHz)

This patch series cleans up the existing 32k-sync timer implementation, movind SoC init code to respective files (mach-omap1/timer32k.c and mach-omap2/timer.c) and uses kernel parameter to override the default clocksource of "counter_32k", also in order to support some OMAP based derivative SoCs like AM33XX which doesn't have 32K sync-timer hardware IP, adds hwmod lookup for omap2+ devices, and if lookup fails then fall back to gp-timer.

if(use_gptimer_clksrc == true)

	gptimer clocksource init;
else if (counter_32 init == false)
	/* Fallback to gptimer */
	gptimer clocksource init(;

With this, we should be able to support multi-omap boot including devices with/without 32k-sync timer.

This patch-series has been boot tested on AM37xEVM platform, it would be helpful if somebody help me to validate it on OMAP1/2 platforms.

The patches are also available at (based on linux-omap/master) -
https://github.com/hvaibhav/am335x-linux 32ksync-timer-cleanup

History:
========
Changes from V6:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg67623.html
- Changed Patch2/3 descrption to add affected kernel parameter
- Warning Fix: Added missing inline prefix to omap_32k_timer_init()
- Added Acked-by and Tested-by from Kevin Hilman

Changes from V5:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg67163.html
- 2 of the original patch-series (1/3 and 2/3) has already been accepted and queued by Paul, so removing them.
Also, add 2 new patches,
- OMAP2 timer32k cleanup, check for return values.
- Changed name of system timers, "gp timer" => "gp_timer".
- Moved SoC specific init code to respective files (mach-omap1/timer32k.c and mach-omap2/timer.c)

Changes from V4:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg67019.html
- Fix the possible bug, leftover the cnt addr argument to clocksource_mmio_init().

Changes from V3:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg66462.html
- Fixed all review comments from Kevin H
* Moved counter_32k CR register offset handling to counter_32k.c file, so now, calling funtion don't have to maintain or add offset to base addr.
* Added comment for function omap_init_clocksource_32k()
* Used resource_size() for calculate size
* Convert WARN_ON to pr_warn

Changes from V2:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/092037.html
- Added early_param support to read clocksource selection from user through kernel parameter ("clocksource=")
- Converted to ocp_if changes from Paul

Changes from V1:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/081037.html
- Based on Tony's comment, added pbase & size argument to omap_init_clocksource_32k(), to avoid cpu_is_xxx() check.
- Added commit description based on discussion on list (Thanks to Santosh here)
- Reorder patch sequence

Vaibhav Hiremath (3):
ARM: OMAP1: FIX: check possible error condition in timer_init
ARM: OMAP2+: Replace space=>underscore in the name field of system
timers
ARM: OMAP: Make OMAP clocksource source selection using kernel param

 arch/arm/mach-omap1/common.h             |   10 ++-
 arch/arm/mach-omap1/time.c               |   16 +----
 arch/arm/mach-omap1/timer32k.c           |   28 ++++++-
 arch/arm/mach-omap2/timer.c              |  118 +++++++++++++++++++++++------
 arch/arm/plat-omap/counter_32k.c         |   91 ++++++++++-------------
 arch/arm/plat-omap/include/plat/common.h |    2 +-
 6 files changed, 167 insertions(+), 98 deletions(-)

On OMAP1, omap_32k_timer_init() function always returns "true", irrespective of whether error occurred while initializing 32k sync counter as a kernel clocksource or not and execution will never fallback to mpu_timer clocksource init code.

This patch adds check for return value from function omap_init_clocksource_32k(), and fallback to omap_mpu_timer_init() in case of failure/error from omap_init_clocksource_32k().

---
No Functional code change compared to V6 version, only 'inline' prefix added to the function omap_32k_timer_init(), required to fix warning (Thanks to Kevin).

 arch/arm/mach-omap1/common.h   |   10 +++++++++-
 arch/arm/mach-omap1/time.c     |   16 +---------------
 arch/arm/mach-omap1/timer32k.c |   13 +++++++++----
 3 files changed, 19 insertions(+), 20 deletions(-)

index 8cc616e..cc83251 100644
@@ -63,7 +63,15 @@ extern void omap1_nand_cmd_ctl(struct mtd_info *mtd, int cmd,
 			       unsigned int ctrl);
 
 extern struct sys_timer omap1_timer;
-extern bool omap_32k_timer_init(void);
+
+#ifdef CONFIG_OMAP_32K_TIMER
+extern int omap_32k_timer_init(void);
+#else
+static inline int __init omap_32k_timer_init(void)
+{
+	return -ENODEV;
+}
+#endif
 
 extern u32 omap_irq_flags;
 

index 4d8dd9a..4062480 100644
@@ -232,20 +232,6 @@ static inline void omap_mpu_timer_init(void)
 }
 #endif	/* CONFIG_OMAP_MPU_TIMER */
 
-static inline int omap_32k_timer_usable(void)
-{
-	int res = false;
-
-	if (cpu_is_omap730() || cpu_is_omap15xx())
-		return res;
-
-#ifdef CONFIG_OMAP_32K_TIMER
-	res = omap_32k_timer_init();
-#endif
-
-	return res;
-}
-
 /*
  * ---------------------------------------------------------------------------
  * Timer initialization

@@ -253,7 +239,7 @@ static inline int omap_32k_timer_usable(void)
  */
 static void __init omap1_timer_init(void)
 {
-	if (!omap_32k_timer_usable())
+	if (omap_32k_timer_init() != 0)
 		omap_mpu_timer_init();
 }
 

index 325b9a0..e3613a8 100644
@@ -182,10 +182,15 @@ static __init void omap_init_32k_timer(void)
  * Timer initialization
  * ---------------------------------------------------------------------------
  */
-bool __init omap_32k_timer_init(void)
+int __init omap_32k_timer_init(void)
 {
-	omap_init_clocksource_32k();
-	omap_init_32k_timer();
+	int ret = -ENODEV;
 
-	return true;
+	if (cpu_is_omap16xx())
+		ret = omap_init_clocksource_32k();
+
+	if (!ret)
+		omap_init_32k_timer();
+
+	return ret;
 }

--
1.7.0.4

Depending on the bootloader, passing command-line arguments with spaces may have issues. Some bootloaders doesn't seem to pass along the quotes, passing only 'gp' part of the string, which leads to wrong override configuration.

The only affected kernel parameter configuration for OMAP family is "clocksource=", used to override kernel clocksource.

So this patch changes "gp timer" => "gp_timer", for clockevent, clocksource and timer irq_handler.

---
No code change compared to V6 patch series, only commit description has been updated as per Kevin's comment.

 arch/arm/mach-omap2/timer.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index ecec873..777f84b 100644
@@ -90,7 +90,7 @@ static irqreturn_t omap2_gp_timer_interrupt(int irq, void *dev_id)
 }
 
 static struct irqaction omap2_gp_timer_irq = {
-	.name		= "gp timer",
+	.name		= "gp_timer",
 	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
 	.handler	= omap2_gp_timer_interrupt,
 };

@@ -132,7 +132,7 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 }
 
 static struct clock_event_device clockevent_gpt = {
-	.name		= "gp timer",
+	.name		= "gp_timer",
 	.features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
 	.shift		= 32,
 	.set_next_event	= omap2_gp_timer_set_next_event,

@@ -269,7 +269,7 @@ static cycle_t clocksource_read_cycles(struct clocksource *cs)
 }
 
 static struct clocksource clocksource_gpt = {
-	.name		= "gp timer",
+	.name		= "gp_timer",
 	.rating		= 300,
 	.read		= clocksource_read_cycles,
 	.mask		= CLOCKSOURCE_MASK(32),

--
1.7.0.4

Current OMAP code supports couple of clocksource options based on compilation flag (CONFIG_OMAP_32K_TIMER). The 32KHz sync-timer and a gptimer which can run on 32KHz or system clock (e.g 38.4 MHz). So there can be 3 options -

1. 32KHz sync-timer
2. Sys_clock based (e.g 13/19.2/26/38.4 MHz) gptimer
3. 32KHz based gptimer.

The optional gptimer based clocksource was added so that it can give the high precision than sync-timer, so expected usage was 2 and not 3.
Unfortunately option 2, clocksource doesn't meet the requirement of free-running clock as per clocksource need. It stops in low power states when sys_clock is cut. That makes gptimer based clocksource option useless for OMAP2/3/4 devices with sys_clock as a clock input. So, in order to use option 2, deeper idle state MUST be disabled.

Option 3 will still work but it is no better than 32K sync-timer based clocksource.

We must support both sync timer and gptimer based clocksource as some OMAP based derivative SoCs like AM33XX does not have the sync timer.

Considering above, make sync-timer and gptimer clocksource runtime selectable so that both OMAP and AMXXXX continue to use the same code.

And, in order to precisely configure/setup sched_clock for given clocksource, decision has to be made early enough in boot sequence.

So, the solution is,

Use standard kernel parameter ("clocksource=") to override default 32k_sync-timer, in addition to this, we also use hwmod database lookup mechanism, through which at run-time we can identify availability of 32k-sync timer on the device, else fall back to gptimer.

Also, moved low-level SoC specific init code to respective files, (mach-omap1/timer32k.c and mach-omap2/timer.c)

---
No code change compared to V6 patch series.

 arch/arm/mach-omap1/timer32k.c           |   19 +++++-
 arch/arm/mach-omap2/timer.c              |  112 ++++++++++++++++++++++++------
 arch/arm/plat-omap/counter_32k.c         |   91 ++++++++++--------------
 arch/arm/plat-omap/include/plat/common.h |    2 +-
 4 files changed, 147 insertions(+), 77 deletions(-)

index e3613a8..eae49c3 100644
@@ -71,6 +71,7 @@
 
 /* 16xx specific defines */
 #define OMAP1_32K_TIMER_BASE		0xfffb9000
+#define OMAP1_32KSYNC_TIMER_BASE	0xfffbc400
 #define OMAP1_32K_TIMER_CR		0x08
 #define OMAP1_32K_TIMER_TVR		0x00
 #define OMAP1_32K_TIMER_TCR		0x04

@@ -186,8 +187,22 @@ int __init omap_32k_timer_init(void)
 {
 	int ret = -ENODEV;
 
-	if (cpu_is_omap16xx())
-		ret = omap_init_clocksource_32k();
+	if (cpu_is_omap16xx()) {
+		void __iomem *base;
+		struct clk *sync32k_ick;
+
+		base = ioremap(OMAP1_32KSYNC_TIMER_BASE, SZ_1K);
+		if (!base) {
+			pr_err("32k_counter: failed to map base addr\n");
+			return -ENODEV;
+		}
+
+		sync32k_ick = clk_get(NULL, "omap_32ksync_ick");
+		if (!IS_ERR(sync32k_ick))
+			clk_enable(sync32k_ick);
+
+		ret = omap_init_clocksource_32k(base);
+	}
 
 	if (!ret)
 		omap_init_32k_timer();

index a557b84..d1cb6f5 100644
@@ -30,7 +30,7 @@
 #include <plat/i2c.h>
 #include <plat/omap_hwmod.h>
 
-extern int __init omap_init_clocksource_32k(void);
+extern int __init omap_init_clocksource_32k(void __iomem *vbase);
 
 extern void __init omap_check_revision(void);
 

--
1.7.0.4

I'll apply these into cleanup-timer branch now that we have the related hwmod patches merged into devel-hwmod-data.

Regards,

Tony

Dnia czwartek, 3 maja 2012 13:28:58 Vaibhav Hiremath pisze:

implementation,

IP,

Tried to test this branch, merged into 3.4-rc6, and with my Amstrad Delta fixes applied, using my custom Amstrad Delta config, but initially failed because an unrelated issue rised to the surface:

|   LD      .tmp_vmlinux1
| arch/arm/mach-omap1/built-in.o: In function `omap1_usb_init':
| lcd_dma.c:(.init.text+0x1420): undefined reference to `ocpi_enable'
| make: *** [.tmp_vmlinux1] Error 1

Apparently introduced with commit d3645d39ad0ed9f09535065676ea0ba114f93cdf, "ARM: OMAP1: OHCI: use platform_data fn ptr to enable OCPI bus", looks like one or more symbols exported by the not linked in arch/arm/mach-omap1/ocpi.o may be missing for non-OMAP16xx configs.

Otherwise, if configured with OMAP16xx selected, builds and seems to work correctly for me, to the extent possible to verify without physical access to the hardware. However, please note that the Amstrad Delta doesn't make use of the 32k timer, only mpu_timer.

Thanks,
Janusz

Hi Janusz,

Thanks for the report, I'll send a fix.

- Paul

OK good to hear, thanks!

Tony

Hello Janusz,

A fix for this is now posted here:

http://marc.info/?l=linux-omap&m=133668898529344&w=2

If you have the chance to test it, that would be great. Hopefully it should be pretty uncontroversial.

- Paul

Thanks Janusz for validating this, really appreciate your help here.

Thanks,
Vaibhav