[PATCH 0/8] OMAP: timers: Preparation for migration to clocksource

[PATCH 0/8] OMAP: timers: Preparation for migration to clocksource

[Joel Fernandes]

This series is in preparation to make the OMAP dmtimer a clocksource driver
(moving it drivers/clocksource/). As a first step, we introduce a  a generic
.init_time function OMAP DT platforms that can be used across OMAP devices.
Also introduced are a better separation between omap-specific code such as
hwmod and the generic timer code. This will allow us to move the generic
portion to drivers/clocksource/ in the future.

The generic functions with the new DT bindings were tested on OMAP4 and AM33xx.
Although, OMAP4 uses the local timer for clocksource by default.

Lastly, this series also adds a new omap-timer.h header that allows drivers
to use the public dmtimer API in MULTIPLATFORM configurations, and along
with this, ir-rx51 is enabled as well, with some fixes to it.

Joel Fernandes (8):
  ARM: OMAP: Move public portion of dmtimer.h to
    include/linux/omap-timer.h
  rc: ir-rx51: Use clk API to get clock rate
  rc: ir-rx51: Turn ON ir-rx51 as it should work for MULTIPLATFORM
  ARM: OMAP4: timer: Remove non-DT code for TWD timer
  ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent
    system timers
  devicetree: doc: Document ti,timer-parent property
  ARM: DTS: am33xx: Provide the ti,timer-parent property
  ARM: AM33xx: Move to using omap_generic_timer_init for init_time

 .../devicetree/bindings/arm/omap/timer.txt         |  17 ++
 arch/arm/boot/dts/am33xx.dtsi                      |   2 +
 arch/arm/mach-omap2/board-generic.c                |   2 +-
 arch/arm/mach-omap2/common.h                       |   1 +
 arch/arm/mach-omap2/timer.c                        | 257 +++++++++++++++++++--
 arch/arm/plat-omap/include/plat/dmtimer.h          | 121 +---------
 drivers/media/rc/Kconfig                           |   2 +-
 drivers/media/rc/ir-rx51.c                         |   6 +-
 include/linux/omap-timer.h                         | 167 +++++++++++++
 9 files changed, 432 insertions(+), 143 deletions(-)
 create mode 100644 include/linux/omap-timer.h

-- 
1.8.1.2

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Joel Fernandes]

Multiplatform support has made arch/arm/plat-omap/include/plat/ inaccessible to
drivers outside the plat-omap directory [1]. Due to this the following drivers
are disabled with !CONFIG_ARCH_MULTIPLATFORM:
CONFIG_IR_RX51 (drivers/media/rc/ir-rx51.c)
CONFIG_TIDSPBRIDGE (drivers/staging/tidspbridge/core/dsp-clock.c)

We move the portion of the dmtimer "API" that should be accessible to the
drivers, into include/linux/omap-timer.h

Build tested changes with IR_RX51.

[1] http://marc.info/?l=linux-omap&m=138421692332108&w=2

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 arch/arm/plat-omap/include/plat/dmtimer.h | 121 +---------------------
 include/linux/omap-timer.h                | 167 ++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 120 deletions(-)
 create mode 100644 include/linux/omap-timer.h

diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index fb92abb..7d6cff8 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -35,130 +35,11 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
+#include <linux/omap-timer.h>
 
 #ifndef __ASM_ARCH_DMTIMER_H
 #define __ASM_ARCH_DMTIMER_H
 
-/* clock sources */
-#define OMAP_TIMER_SRC_SYS_CLK			0x00
-#define OMAP_TIMER_SRC_32_KHZ			0x01
-#define OMAP_TIMER_SRC_EXT_CLK			0x02
-
-/* timer interrupt enable bits */
-#define OMAP_TIMER_INT_CAPTURE			(1 << 2)
-#define OMAP_TIMER_INT_OVERFLOW			(1 << 1)
-#define OMAP_TIMER_INT_MATCH			(1 << 0)
-
-/* trigger types */
-#define OMAP_TIMER_TRIGGER_NONE			0x00
-#define OMAP_TIMER_TRIGGER_OVERFLOW		0x01
-#define OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE	0x02
-
-/* posted mode types */
-#define OMAP_TIMER_NONPOSTED			0x00
-#define OMAP_TIMER_POSTED			0x01
-
-/* timer capabilities used in hwmod database */
-#define OMAP_TIMER_SECURE				0x80000000
-#define OMAP_TIMER_ALWON				0x40000000
-#define OMAP_TIMER_HAS_PWM				0x20000000
-#define OMAP_TIMER_NEEDS_RESET				0x10000000
-#define OMAP_TIMER_HAS_DSP_IRQ				0x08000000
-
-/*
- * timer errata flags
- *
- * Errata i103/i767 impacts all OMAP3/4/5 devices including AM33xx. This
- * errata prevents us from using posted mode on these devices, unless the
- * timer counter register is never read. For more details please refer to
- * the OMAP3/4/5 errata documents.
- */
-#define OMAP_TIMER_ERRATA_I103_I767			0x80000000
-
-struct omap_timer_capability_dev_attr {
-	u32 timer_capability;
-};
-
-struct timer_regs {
-	u32 tidr;
-	u32 tier;
-	u32 twer;
-	u32 tclr;
-	u32 tcrr;
-	u32 tldr;
-	u32 ttrg;
-	u32 twps;
-	u32 tmar;
-	u32 tcar1;
-	u32 tsicr;
-	u32 tcar2;
-	u32 tpir;
-	u32 tnir;
-	u32 tcvr;
-	u32 tocr;
-	u32 towr;
-};
-
-struct omap_dm_timer {
-	int id;
-	int irq;
-	struct clk *fclk;
-
-	void __iomem	*io_base;
-	void __iomem	*irq_stat;	/* TISR/IRQSTATUS interrupt status */
-	void __iomem	*irq_ena;	/* irq enable */
-	void __iomem	*irq_dis;	/* irq disable, only on v2 ip */
-	void __iomem	*pend;		/* write pending */
-	void __iomem	*func_base;	/* function register base */
-
-	unsigned long rate;
-	unsigned reserved:1;
-	unsigned posted:1;
-	struct timer_regs context;
-	int (*get_context_loss_count)(struct device *);
-	int ctx_loss_count;
-	int revision;
-	u32 capability;
-	u32 errata;
-	struct platform_device *pdev;
-	struct list_head node;
-};
-
-int omap_dm_timer_reserve_systimer(int id);
-struct omap_dm_timer *omap_dm_timer_request(void);
-struct omap_dm_timer *omap_dm_timer_request_specific(int timer_id);
-struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap);
-struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np);
-int omap_dm_timer_free(struct omap_dm_timer *timer);
-void omap_dm_timer_enable(struct omap_dm_timer *timer);
-void omap_dm_timer_disable(struct omap_dm_timer *timer);
-
-int omap_dm_timer_get_irq(struct omap_dm_timer *timer);
-
-u32 omap_dm_timer_modify_idlect_mask(u32 inputmask);
-struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer);
-
-int omap_dm_timer_trigger(struct omap_dm_timer *timer);
-int omap_dm_timer_start(struct omap_dm_timer *timer);
-int omap_dm_timer_stop(struct omap_dm_timer *timer);
-
-int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source);
-int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, unsigned int value);
-int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, unsigned int value);
-int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, unsigned int match);
-int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle, int trigger);
-int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler);
-
-int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, unsigned int value);
-int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask);
-
-unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer);
-int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value);
-unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer);
-int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value);
-
-int omap_dm_timers_active(void);
-
 /*
  * Do not use the defines below, they are not needed. They should be only
  * used by dmtimer.c and sys_timer related code.
diff --git a/include/linux/omap-timer.h b/include/linux/omap-timer.h
new file mode 100644
index 0000000..e21ee18
--- /dev/null
+++ b/include/linux/omap-timer.h
@@ -0,0 +1,167 @@
+/*
+ * include/misc/dmtimer.h
+ *
+ * OMAP Dual-Mode Timers public header
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Tarun Kanti DebBarma <tarun.kanti@ti.com>
+ * Thara Gopinath <thara@ti.com>
+ *
+ * Platform device conversion and hwmod support.
+ *
+ * Copyright (C) 2005 Nokia Corporation
+ * Author: Lauri Leukkunen <lauri.leukkunen@nokia.com>
+ * PWM and clock framwork support by Timo Teras.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN
+ * NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * You should have received a copy of the  GNU General Public License along
+ * with this program; if not, write  to the Free Software Foundation, Inc.,
+ * 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+#ifndef __MISC_DMTIMER_H
+#define __MISC_DMTIMER_H
+
+/* clock sources */
+#define OMAP_TIMER_SRC_SYS_CLK			0x00
+#define OMAP_TIMER_SRC_32_KHZ			0x01
+#define OMAP_TIMER_SRC_EXT_CLK			0x02
+
+/* timer interrupt enable bits */
+#define OMAP_TIMER_INT_CAPTURE			(1 << 2)
+#define OMAP_TIMER_INT_OVERFLOW			(1 << 1)
+#define OMAP_TIMER_INT_MATCH			(1 << 0)
+
+/* trigger types */
+#define OMAP_TIMER_TRIGGER_NONE			0x00
+#define OMAP_TIMER_TRIGGER_OVERFLOW		0x01
+#define OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE	0x02
+
+/* posted mode types */
+#define OMAP_TIMER_NONPOSTED			0x00
+#define OMAP_TIMER_POSTED			0x01
+
+/* timer capabilities used in hwmod database */
+#define OMAP_TIMER_SECURE				0x80000000
+#define OMAP_TIMER_ALWON				0x40000000
+#define OMAP_TIMER_HAS_PWM				0x20000000
+#define OMAP_TIMER_NEEDS_RESET				0x10000000
+#define OMAP_TIMER_HAS_DSP_IRQ				0x08000000
+
+/*
+ * timer errata flags
+ *
+ * Errata i103/i767 impacts all OMAP3/4/5 devices including AM33xx. This
+ * errata prevents us from using posted mode on these devices, unless the
+ * timer counter register is never read. For more details please refer to
+ * the OMAP3/4/5 errata documents.
+ */
+#define OMAP_TIMER_ERRATA_I103_I767			0x80000000
+
+struct omap_timer_capability_dev_attr {
+	u32 timer_capability;
+};
+
+struct timer_regs {
+	u32 tidr;
+	u32 tier;
+	u32 twer;
+	u32 tclr;
+	u32 tcrr;
+	u32 tldr;
+	u32 ttrg;
+	u32 twps;
+	u32 tmar;
+	u32 tcar1;
+	u32 tsicr;
+	u32 tcar2;
+	u32 tpir;
+	u32 tnir;
+	u32 tcvr;
+	u32 tocr;
+	u32 towr;
+};
+
+struct omap_dm_timer {
+	int id;
+	int irq;
+	struct clk *fclk;
+
+	void __iomem	*io_base;
+	void __iomem	*irq_stat;	/* TISR/IRQSTATUS interrupt status */
+	void __iomem	*irq_ena;	/* irq enable */
+	void __iomem	*irq_dis;	/* irq disable, only on v2 ip */
+	void __iomem	*pend;		/* write pending */
+	void __iomem	*func_base;	/* function register base */
+
+	unsigned long rate;
+	unsigned reserved:1;
+	unsigned posted:1;
+	struct timer_regs context;
+	int (*get_context_loss_count)(struct device *);
+	int ctx_loss_count;
+	int revision;
+	u32 capability;
+	u32 errata;
+	struct platform_device *pdev;
+	struct list_head node;
+};
+
+int omap_dm_timer_reserve_systimer(int id);
+struct omap_dm_timer *omap_dm_timer_request(void);
+struct omap_dm_timer *omap_dm_timer_request_specific(int timer_id);
+struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap);
+struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np);
+int omap_dm_timer_free(struct omap_dm_timer *timer);
+void omap_dm_timer_enable(struct omap_dm_timer *timer);
+void omap_dm_timer_disable(struct omap_dm_timer *timer);
+
+int omap_dm_timer_get_irq(struct omap_dm_timer *timer);
+
+u32 omap_dm_timer_modify_idlect_mask(u32 inputmask);
+struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer);
+
+int omap_dm_timer_trigger(struct omap_dm_timer *timer);
+int omap_dm_timer_start(struct omap_dm_timer *timer);
+int omap_dm_timer_stop(struct omap_dm_timer *timer);
+
+int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source);
+int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
+			   unsigned int value);
+int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
+				 unsigned int value);
+int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
+			    unsigned int match);
+int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle,
+			  int trigger);
+int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler);
+
+int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
+				 unsigned int value);
+int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask);
+
+unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer);
+int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value);
+unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer);
+int omap_dm_timer_write_counter(struct omap_dm_timer *timer,
+				unsigned int value);
+
+int omap_dm_timers_active(void);
+
+#endif
-- 
1.8.1.2

[PATCH 2/8] rc: ir-rx51: Use clk API to get clock rate

[Joel Fernandes]

ir-rx51 tries to use struct clk directly, which is private. Go through to clk
API to get the rate.

Compile tested only.

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 drivers/media/rc/ir-rx51.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 31b955b..7367122 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -26,8 +26,8 @@
 #include <linux/sched.h>
 #include <linux/wait.h>
 
-#include <plat/dmtimer.h>
-#include <plat/clock.h>
+#include <linux/omap-timer.h>
+#include <linux/clk.h>
 
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
@@ -209,7 +209,7 @@ static int lirc_rx51_init_port(struct lirc_rx51 *lirc_rx51)
 	}
 
 	clk_fclk = omap_dm_timer_get_fclk(lirc_rx51->pwm_timer);
-	lirc_rx51->fclk_khz = clk_fclk->rate / 1000;
+	lirc_rx51->fclk_khz = clk_get_rate(clk_fclk) / 1000;
 
 	return 0;
 
-- 
1.8.1.2

[PATCH 3/8] rc: ir-rx51: Turn ON ir-rx51 as it should work for MULTIPLATFORM

[Joel Fernandes]

Previous patches moved public portion of dmtimer declaration out into
include/linux/. With this, ir-rx51 compiles fine with CONFIG_ARCH_MULTIPLATFORM

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 drivers/media/rc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 11e84bc..36997b6 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -292,7 +292,7 @@ config IR_TTUSBIR
 
 config IR_RX51
 	tristate "Nokia N900 IR transmitter diode"
-	depends on OMAP_DM_TIMER && ARCH_OMAP2PLUS && LIRC && !ARCH_MULTIPLATFORM
+	depends on OMAP_DM_TIMER && ARCH_OMAP2PLUS && LIRC
 	---help---
 	   Say Y or M here if you want to enable support for the IR
 	   transmitter diode built in the Nokia N900 (RX51) device.
-- 
1.8.1.2

[PATCH 4/8] ARM: OMAP4: timer: Remove non-DT code for TWD timer

[Joel Fernandes]

OMAP4 is DT only boot and TWD local timer will be created from clocksource-of
layer. We remove OMAP4 code that creates it for non-DT.

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 arch/arm/mach-omap2/timer.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index d42da7e..dd41f57 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -605,32 +605,17 @@ static OMAP_SYS_32K_TIMER_INIT(4, 1, "timer_32k_ck", "ti,timer-alwon",
 #endif
 
 #ifdef CONFIG_ARCH_OMAP4
-#ifdef CONFIG_HAVE_ARM_TWD
-static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29);
 void __init omap4_local_timer_init(void)
 {
 	omap4_sync32k_timer_init();
-	/* Local timers are not supprted on OMAP4430 ES1.0 */
-	if (omap_rev() != OMAP4430_REV_ES1_0) {
-		int err;
-
-		if (of_have_populated_dt()) {
-			clocksource_of_init();
-			return;
-		}
 
-		err = twd_local_timer_register(&twd_local_timer);
-		if (err)
-			pr_err("twd_local_timer_register failed %d\n", err);
-	}
-}
-#else
-void __init omap4_local_timer_init(void)
-{
-	omap4_sync32k_timer_init();
-}
-#endif /* CONFIG_HAVE_ARM_TWD */
+#ifdef CONFIG_HAVE_ARM_TWD
+	/* TWD Local timers are not supprted on OMAP4430 ES1.0 */
+	if (omap_rev() != OMAP4430_REV_ES1_0)
+		clocksource_of_init();
 #endif /* CONFIG_ARCH_OMAP4 */
+}
+#endif
 
 #if defined(CONFIG_SOC_OMAP5) || defined(CONFIG_SOC_DRA7XX)
 void __init omap5_realtime_timer_init(void)
-- 
1.8.1.2

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Joel Fernandes]

This work is a migration effort of OMAP system timers to the
clocksource/clockevent framework. Consider this as a first-pass in this effort.
There are few cleanups that need to be done first. The HWMOD code is
intertwined with the timer code. HWMOD code cleanups in the future will
hopefully make most of this code go away, so till then we separate out the
power/clocks portion of the code from the actual timer bits.  This will
facilitate near-future work of adapting the system timer as a clocksource.

New functions for OF-only boot are introduced, and we can soon delete the old
versions once we migrate all platforms. Currently only AM335x is migrated and
testedA new omap_generic_timer_init function is introduced for DT platforms.
Code required earlier for non-DT platforms such as setup of timer IDs and timer
parent clock is not required.  parent clocks are automatically setup by the mux
clock driver through DT so they no longer need to be hardcoded.

The init code will try to pick the best timer for clocksource and clockevent
however bindings are added to force a particular timer as clocksource or
clockevent through DT.

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
 arch/arm/mach-omap2/common.h                       |   1 +
 arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
 3 files changed, 248 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
index d02e27c..6cf7a75 100644
--- a/Documentation/devicetree/bindings/arm/omap/timer.txt
+++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
@@ -32,6 +32,18 @@ Optional properties:
 - ti,timer-secure: 	Indicates the timer is reserved on a secure OMAP device
 			and therefore cannot be used by the kernel.
 
+- ti,timer-clockevent,
+  ti,timer-clocksource	These properties force the system timer code to choose
+			the particular timer as a clockevent or clocksource.
+			If these properties are not specified, the timer code
+			picks up a "ti,timer-alwon" as the clocksource and a
+			timer containing one of the following properties as
+			the clockevent in the following order:
+				ti,timer-alwon
+				ti,timer-dsp
+				ti,timer-pwm
+				ti,timer-secure
+
 Example:
 
 timer12: timer at 48304000 {
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 4a5684b..2a6b588 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -86,6 +86,7 @@ extern void omap3_secure_sync32k_timer_init(void);
 extern void omap3_gptimer_timer_init(void);
 extern void omap4_local_timer_init(void);
 extern void omap5_realtime_timer_init(void);
+void omap_generic_timer_init(void);
 
 void omap2420_init_early(void);
 void omap2430_init_early(void);
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index dd41f57..8ee2de0 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -318,6 +318,89 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	return r;
 }
 
+static int __init omap_dmtimer_power_init(struct omap_dm_timer *timer,
+					struct device_node *np,
+					bool is_counter) {
+	struct omap_hwmod *oh;
+	const char *oh_name = NULL;
+	struct clk *src;
+	const char *parent;
+	int r;
+
+	of_property_read_string_index(np, "ti,hwmods", 0, &oh_name);
+	if (!oh_name)
+		return -ENODEV;
+
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh)
+		return -ENODEV;
+
+	omap_hwmod_setup_one(oh_name);
+
+	if (!is_counter) {
+		if (oh->_clk)
+			timer->fclk = oh->_clk;
+		else
+			timer->fclk = clk_get(NULL,
+					      omap_hwmod_get_main_clk(oh));
+		if (IS_ERR(timer->fclk))
+			return PTR_ERR(timer->fclk);
+
+		parent = of_get_property(np, "ti,timer-parent", NULL);
+		if (!parent) {
+			pr_err("ti,timer-parent required for system timer\n");
+			return -1;
+		}
+
+		src = clk_get(NULL, parent);
+		if (IS_ERR(src)) {
+			pr_err("Couldn't clk_get timer parent\n");
+			return PTR_ERR(src);
+		}
+
+		if (clk_get_parent(timer->fclk) != src) {
+			r = clk_set_parent(timer->fclk, src);
+			if (r < 0) {
+				pr_err("%s: %s cannot set source\n", __func__,
+				       oh->name);
+				clk_put(src);
+				return r;
+			}
+		}
+		clk_put(src);
+	}
+
+	omap_hwmod_enable(oh);
+	return 0;
+}
+
+static int __init omap_dmtimer_init_one(struct omap_dm_timer *timer,
+					struct device_node *np,
+					int posted)
+{
+	timer->irq = irq_of_parse_and_map(np, 0);
+	if (!timer->irq)
+		return -ENXIO;
+
+	timer->io_base = of_iomap(np, 0);
+	if (!timer->io_base)
+		return -ENXIO;
+
+	__omap_dm_timer_init_regs(timer);
+
+	if (posted)
+		__omap_dm_timer_enable_posted(timer);
+
+	/* Check that the intended posted configuration matches the actual */
+	if (posted != timer->posted)
+		return -EINVAL;
+
+	timer->rate = clk_get_rate(timer->fclk);
+	timer->reserved = 1;
+
+	return 0;
+}
+
 static void __init omap2_gp_clockevent_init(int gptimer_id,
 						const char *fck_source,
 						const char *property)
@@ -353,6 +436,41 @@ static void __init omap2_gp_clockevent_init(int gptimer_id,
 		clkev.rate);
 }
 
+static int __init omap_clockevent_init(struct device_node *np)
+{
+	int res;
+
+	clkev.errata = omap_dm_timer_get_errata();
+
+	/*
+	 * For clock-event timers we never read the timer counter and
+	 * so we are not impacted by errata i103 and i767. Therefore,
+	 * we can safely ignore this errata for clock-event timers.
+	 */
+	__omap_dm_timer_override_errata(&clkev, OMAP_TIMER_ERRATA_I103_I767);
+
+	clockevent_gpt.name = "timer_clkev";
+
+	res = omap_dmtimer_init_one(&clkev, np, OMAP_TIMER_POSTED);
+	if (res)
+		return res;
+
+	omap2_gp_timer_irq.dev_id = &clkev;
+	setup_irq(clkev.irq, &omap2_gp_timer_irq);
+
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+
+	clockevent_gpt.cpumask = cpu_possible_mask;
+	clockevent_gpt.irq = omap_dm_timer_get_irq(&clkev);
+	clockevents_config_and_register(&clockevent_gpt, clkev.rate,
+					3, /* Timer internal resynch latency */
+					0xffffffff);
+
+	pr_info("OMAP clockevent source: %s at %lu Hz\n", clockevent_gpt.name,
+		clkev.rate);
+	return 0;
+}
+
 /* Clocksource code */
 static struct omap_dm_timer clksrc;
 static bool use_gptimer_clksrc;
@@ -448,6 +566,31 @@ static int __init __maybe_unused omap2_sync32k_clocksource_init(void)
 	return ret;
 }
 
+/* Setup free-running counter for clocksource */
+static int __init omap_sync32k_clocksource_init(struct device_node *np)
+{
+	int ret = 0;
+	void __iomem *vbase;
+
+	vbase = of_iomap(np, 0);
+	if (!vbase) {
+		pr_err("%s: failed to get counter_32k resource\n", __func__);
+		return -ENXIO;
+	}
+
+	ret = omap_init_clocksource_32k(vbase);
+	if (ret) {
+		pr_err("%s: failed to initialize counter_32k clocksource(%d)\n",
+		       __func__, ret);
+		return ret;
+	}
+
+	pr_err("Sync32k clocksource initialized\n");
+
+	return ret;
+}
+
+
 static void __init omap2_gptimer_clocksource_init(int gptimer_id,
 						  const char *fck_source,
 						  const char *property)
@@ -475,6 +618,39 @@ static void __init omap2_gptimer_clocksource_init(int gptimer_id,
 			clocksource_gpt.name, clksrc.rate);
 }
 
+static int __init omap_clocksource_init(struct device_node *np)
+{
+	int res = 0;
+
+	clksrc.errata = omap_dm_timer_get_errata();
+
+	if (strlen(np->name) > 7) {
+		pr_err("%s: OF node name too big\n", __func__);
+		return -ENODEV;
+	}
+	clocksource_gpt.name = "timer_clksrc";
+
+	res = omap_dmtimer_init_one(&clksrc, np, OMAP_TIMER_NONPOSTED);
+	if (res)
+		return res;
+
+	__omap_dm_timer_load_start(&clksrc,
+				   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0,
+				   OMAP_TIMER_NONPOSTED);
+	setup_sched_clock(dmtimer_read_sched_clock, 32, clksrc.rate);
+
+	res = clocksource_register_hz(&clocksource_gpt, clksrc.rate);
+	if (res) {
+		pr_err("Could not register clocksource %s\n",
+		       clocksource_gpt.name);
+		return res;
+	} else {
+		pr_info("OMAP clocksource: %s at %lu Hz\n",
+			clocksource_gpt.name, clksrc.rate);
+	}
+	return 0;
+}
+
 #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
 /*
  * The realtime counter also called master counter, is a free-running
@@ -604,6 +780,65 @@ static OMAP_SYS_32K_TIMER_INIT(4, 1, "timer_32k_ck", "ti,timer-alwon",
 			       2, "sys_clkin_ck", NULL);
 #endif
 
+void omap_generic_timer_init(void)
+{
+	struct device_node *np_clksrc = NULL, *np_clkev = NULL;
+	int res, using_counter = false;
+
+	if (!of_have_populated_dt())
+		BUG_ON("Generic timer init should only be used for DT boot\n");
+
+	if (omap_clk_init)
+		omap_clk_init();
+
+	omap_dmtimer_init();
+
+	/*
+	 * Search for a clocksource:
+	 * Check if dmtimer with property timer-clocksource is in DT,
+	 * If not, by default see if a counter is available unless the
+	 * clocksource=use_gptimer is present on boot command line.
+	 * If no counter on platform, or use_gptimer=1, search for
+	 * a dmtimer with ti,timer-alwon property (timers that don't
+	 * turn off during suspend state). If not found, then fail.
+	 */
+	np_clksrc = omap_get_timer_dt(omap_timer_match, "ti,timer-clocksource");
+	if (!np_clksrc && !use_gptimer_clksrc) {
+		np_clksrc = omap_get_timer_dt(omap_counter_match, NULL);
+		if (np_clksrc)
+			using_counter = true;
+	}
+
+	if (!np_clksrc)
+		np_clksrc = omap_get_timer_dt(omap_timer_match,
+					      "ti,timer-alwon");
+
+	BUG_ON(!np_clksrc);
+
+	np_clkev = omap_get_timer_dt(omap_timer_match, "ti,timer-clockevent");
+	if (!np_clkev) {
+		np_clkev = omap_get_timer_dt(omap_timer_match, NULL);
+		BUG_ON(!np_clkev);
+	}
+
+	res = omap_dmtimer_power_init(&clkev, np_clkev, false);
+	BUG_ON(res);
+
+	res = omap_clockevent_init(np_clkev);
+	BUG_ON(res);
+
+	res = omap_dmtimer_power_init(&clksrc, np_clksrc, using_counter);
+	BUG_ON(res);
+
+	if (using_counter)
+		res = omap_sync32k_clocksource_init(np_clksrc);
+	else
+		res = omap_clocksource_init(np_clksrc);
+	BUG_ON(res);
+
+	of_node_put(np_clksrc);
+}
+
 #ifdef CONFIG_ARCH_OMAP4
 void __init omap4_local_timer_init(void)
 {
-- 
1.8.1.2

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Joel Fernandes]

Timer's parent mux clocks require a parent clock alias, provide the same from
device tree.

Ultimately this will be provided from DT as clock node phandles but clk-mux
driver DT bindings series is still under going review. So to keep things
working during the timer migration, we add this property.  Originally this was
hardcoded in timer.c, now we do the same in DT and would be removing the
hardcoded names from timer.c

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 Documentation/devicetree/bindings/arm/omap/timer.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
index 6cf7a75..644bc61 100644
--- a/Documentation/devicetree/bindings/arm/omap/timer.txt
+++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
@@ -24,6 +24,11 @@ Required properties:
 			where <X> is the instance number of the timer from the
 			HW spec.
 
+Required properties for system timers (clockevents/clocksource):
+- ti,timer-parent:	System timer's parent mux clock needs to be setup.
+			This is currently hardcoded in code, for DT boot we
+			move this to DT.
+
 Optional properties:
 - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
 - ti,timer-dsp:		Indicates the timer can interrupt the on-chip DSP in
-- 
1.8.1.2

[PATCH 7/8] ARM: DTS: am33xx: Provide the ti,timer-parent property

[Joel Fernandes]

Provide the clock alias of the parent clock for the system timer, using the
just added ti,timer-parent property.

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index f9c5da9..3934630 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -259,6 +259,7 @@
 			interrupts = <67>;
 			ti,hwmods = "timer1";
 			ti,timer-alwon;
+			ti,timer-parent = "timer_sys_ck";
 		};
 
 		timer2: timer at 48040000 {
@@ -266,6 +267,7 @@
 			reg = <0x48040000 0x400>;
 			interrupts = <68>;
 			ti,hwmods = "timer2";
+			ti,timer-parent = "timer_sys_ck";
 		};
 
 		timer3: timer at 48042000 {
-- 
1.8.1.2

[PATCH 8/8] ARM: AM33xx: Move to using omap_generic_timer_init for init_time

[Joel Fernandes]

Earlier patch in this series introduced a function omap_generic_timer_init
for all DT platforms. Use it for AM33xx SoC.

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 arch/arm/mach-omap2/board-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index 87162e1..cfda6bc 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -180,7 +180,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)")
 	.init_irq	= omap_intc_of_init,
 	.handle_irq	= omap3_intc_handle_irq,
 	.init_machine	= omap_generic_init,
-	.init_time	= omap3_gptimer_timer_init,
+	.init_time	= omap_generic_timer_init,
 	.dt_compat	= am33xx_boards_compat,
 	.restart	= am33xx_restart,
 MACHINE_END
-- 
1.8.1.2

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Felipe Balbi]

Hi,

On Thu, Nov 21, 2013 at 07:56:51PM -0600, Joel Fernandes wrote:

[...]

> New functions for OF-only boot are introduced, and we can soon delete the old
> versions once we migrate all platforms. Currently only AM335x is migrated and

actually, you don't need to initialize .init_timer at all in DT boot.
Just use CLKSOURCE_OF_DECLARE() and pass your omap_generic_timer_init()
as argument (although, I'd call it omap_of_timer_init()).

That will put of_device_id structure on a special section
(__clksource_of_table) and pass your function as a data argument. That
function will be called automatically during init.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131121/cacbd4ae/attachment.sig>

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Joel Fernandes]

On 11/21/2013 09:51 PM, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Nov 21, 2013 at 07:56:51PM -0600, Joel Fernandes wrote:
> 
> [...]
> 
>> New functions for OF-only boot are introduced, and we can soon delete the old
>> versions once we migrate all platforms. Currently only AM335x is migrated and
> 
> actually, you don't need to initialize .init_timer at all in DT boot.

Actually we still do, because the plan is to keep the hwmod stuff that's
required in timer.c in a custom .init_time, and then of_clocksource_init maybe
called to do what you're suggesting but (not yet) more on that below.

> Just use CLKSOURCE_OF_DECLARE() and pass your omap_generic_timer_init()
> as argument (although, I'd call it omap_of_timer_init()).
> That will put of_device_id structure on a special section
> (__clksource_of_table) and pass your function as a data argument. That
> function will be called automatically during init.
>

I thought of doing that, but currently the timer selection for clocksource
is not a simple matching of compatible string, rather it is selecting the timer
based on properties such as ti,timer-alwon and such.

In omap3 for example, there are needs for specific timers and such have been
provided with the macros passing in timer id etc. Right now, this can be forced
through DT with the "ti,timer-clocksource" property I introduced. All this
selection algorithm is too complex to be handle directly by the
CLOCKSOURCE_OF_DECLARE / clocksource_of_init matching mechanism.

thanks,

-Joel

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Tony Lindgren]

Hi,

* Joel Fernandes <joelf@ti.com> [131121 18:00]:
> Multiplatform support has made arch/arm/plat-omap/include/plat/ inaccessible to
> drivers outside the plat-omap directory [1]. Due to this the following drivers
> are disabled with !CONFIG_ARCH_MULTIPLATFORM:
> CONFIG_IR_RX51 (drivers/media/rc/ir-rx51.c)
> CONFIG_TIDSPBRIDGE (drivers/staging/tidspbridge/core/dsp-clock.c)
> 
> We move the portion of the dmtimer "API" that should be accessible to the
> drivers, into include/linux/omap-timer.h

As we chatted earlier, we don't have to expose all these hardware specific
functions and use existing Linux generic frameworks instead.

We can implement an irqchip and a clocksource in the dmtimer code for the
client drivers to use, and after that we only have a couple of dmtimer
specific functions left to export.

I'm thinkging some thing like this for the public API:

omap_dm_timer_request			request_irq
omap_dm_timer_request_specific		request_irq
omap_dm_timer_get_irq			request_irq
omap_dm_timer_set_source		clk_set_rate
omap_dm_timer_stop			disable_irq
omap_dm_timer_start			enable_irq
omap_dm_timer_disable			disable_irq
omap_dm_timer_free			free_irq
omap_dm_timer_set_int_enable		enable_irq

After that, what's left to export are some functions to configure
the hardware timer:

omap_dm_timer_write_counter
omap_dm_timer_set_match
omap_dm_timer_read_counter
omap_dm_timer_read_status
omap_dm_timer_write_status

And for those we can then eventually probably have some Linux generic
hardware timer API :)

Regards,

Tony

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Rob Herring]

On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote:
> This work is a migration effort of OMAP system timers to the
> clocksource/clockevent framework. Consider this as a first-pass in this effort.
> There are few cleanups that need to be done first. The HWMOD code is
> intertwined with the timer code. HWMOD code cleanups in the future will
> hopefully make most of this code go away, so till then we separate out the
> power/clocks portion of the code from the actual timer bits.  This will
> facilitate near-future work of adapting the system timer as a clocksource.
>
> New functions for OF-only boot are introduced, and we can soon delete the old
> versions once we migrate all platforms. Currently only AM335x is migrated and
> testedA new omap_generic_timer_init function is introduced for DT platforms.
> Code required earlier for non-DT platforms such as setup of timer IDs and timer
> parent clock is not required.  parent clocks are automatically setup by the mux
> clock driver through DT so they no longer need to be hardcoded.
>
> The init code will try to pick the best timer for clocksource and clockevent
> however bindings are added to force a particular timer as clocksource or
> clockevent through DT.
>
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
>  .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
>  arch/arm/mach-omap2/common.h                       |   1 +
>  arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
>  3 files changed, 248 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
> index d02e27c..6cf7a75 100644
> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
> @@ -32,6 +32,18 @@ Optional properties:
>  - ti,timer-secure:     Indicates the timer is reserved on a secure OMAP device
>                         and therefore cannot be used by the kernel.
>
> +- ti,timer-clockevent,
> +  ti,timer-clocksource These properties force the system timer code to choose
> +                       the particular timer as a clockevent or clocksource.
> +                       If these properties are not specified, the timer code
> +                       picks up a "ti,timer-alwon" as the clocksource and a
> +                       timer containing one of the following properties as
> +                       the clockevent in the following order:
> +                               ti,timer-alwon
> +                               ti,timer-dsp
> +                               ti,timer-pwm
> +                               ti,timer-secure

These properties were added specifically for the reason of avoiding
linux specific properties like these. When is this not sufficient?

And I agree with the comment to use OF_CLKSRC.

Rob

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Tony Lindgren]

* Joel Fernandes <joelf@ti.com> [131121 18:00]:
> +Required properties for system timers (clockevents/clocksource):
> +- ti,timer-parent:	System timer's parent mux clock needs to be setup.
> +			This is currently hardcoded in code, for DT boot we
> +			move this to DT.
> +

This can be replaced with just clk_set_rate, or clk_set_parent if needed.
Or by having a clocks = <&32k_clk> property in the dmtimer node in the
.dts file.

>  Optional properties:
>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.

Hmm this we may not need, this can probably be deciphered from the compatible
flag already?

Then for the users of a specific dmtimer, they can select the right one using
the interrupt-parent property:

timer1: timer at 0x4800abcd {
	compatible = "ti,omap5430-timer";
	#interrupt-cells = <1>;		/* needs irqchip implemented for dmtimer */
	interrupt-controller;
	#clock-cells = <1>;		/* needs clocksource implemented for dmtimer */
	clock-output-names = "32k", "sys_ck";
	...
};

counter32k: counter at 4ae04000 {
	compatible = "ti,omap-counter32k";
	#clock-cells = <1>;		/* needs clocksource implemented for 32k counter */
	clock-output-names = "32k";
	...
};

timer {
	compatible = "ti,omap5-timer";
	interrupt-parent = <&timer1>;
	interrupts = <1>;
	clocks = <&timer1 0>,		/* use timer1 as clockevent, clock index 0 = 32k, 1 = sys_ck ... */
		 <&counter32k 0>;	/* use 32k counter as clocksource */
};

>  - ti,timer-dsp:		Indicates the timer can interrupt the on-chip DSP in

This can be probably also be mapped based on the compatible property?

Regards,

Tony

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Joel Fernandes]

On 11/22/2013 09:33 AM, Tony Lindgren wrote:
> Hi,
> 
> * Joel Fernandes <joelf@ti.com> [131121 18:00]:
>> Multiplatform support has made arch/arm/plat-omap/include/plat/ inaccessible to
>> drivers outside the plat-omap directory [1]. Due to this the following drivers
>> are disabled with !CONFIG_ARCH_MULTIPLATFORM:
>> CONFIG_IR_RX51 (drivers/media/rc/ir-rx51.c)
>> CONFIG_TIDSPBRIDGE (drivers/staging/tidspbridge/core/dsp-clock.c)
>>
>> We move the portion of the dmtimer "API" that should be accessible to the
>> drivers, into include/linux/omap-timer.h
> 
> As we chatted earlier, we don't have to expose all these hardware specific
> functions and use existing Linux generic frameworks instead.
> 
> We can implement an irqchip and a clocksource in the dmtimer code for the
> client drivers to use, and after that we only have a couple of dmtimer
> specific functions left to export.
> 
> I'm thinkging some thing like this for the public API:
> 
> omap_dm_timer_request			request_irq
> omap_dm_timer_request_specific		request_irq
> omap_dm_timer_get_irq			request_irq
> omap_dm_timer_set_source		clk_set_rate
> omap_dm_timer_stop			disable_irq
> omap_dm_timer_start			enable_irq
> omap_dm_timer_disable			disable_irq
> omap_dm_timer_free			free_irq
> omap_dm_timer_set_int_enable		enable_irq

Hi Tony,

The thing I feel we're trying to fit a square peg into round hole type of thing
here.

Some reasons I feel this is overkill (point 2 makes it even not possible):

1. Ideally IR_RX51 should be using clockevents/hrtimer framework instead of
irqchip. Other than that, dsp bridge is the only other user.

2. These functions will be also be required to be public once mach-omap2/timer.c
moves to drivers/clocksource/ . At such an early point of code, we can't use
irqchip anyway so we'd need these functions public.

3. This is a minor point, but its also not immediately intuitive or clear to
someone who needs a dedicated hardware timer that they need to use an IRQ chip
driver for the same. But we can avoid any discussion about this till we can
discuss/agree on the above 2.

thanks,

-Joel

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Joel Fernandes]

Hi Tony,

Thanks for your comments, few replies inline below..

On 11/22/2013 09:58 AM, Tony Lindgren wrote:
> * Joel Fernandes <joelf@ti.com> [131121 18:00]:
>> +Required properties for system timers (clockevents/clocksource):
>> +- ti,timer-parent:	System timer's parent mux clock needs to be setup.
>> +			This is currently hardcoded in code, for DT boot we
>> +			move this to DT.
>> +
> 
> This can be replaced with just clk_set_rate, or clk_set_parent if needed.
> Or by having a clocks = <&32k_clk> property in the dmtimer node in the
> .dts file.
> 

Sure, but clock-data is still not available to make this possible in mainline.
We also discussed earlier right that we don't want dependencies as much as
possible to get one chunk in and working at a time. I was thinking like for a
first-pass since there's a lot of unrelated code that doesn't have dependencies,
but needs this to work, we can introduce this property for now and drop it later
as a "cost of migration"?

>>  Optional properties:
>>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
> 
> Hmm this we may not need, this can probably be deciphered from the compatible
> flag already?

How? Compatible contains the same string, for example for OMAP4:

timer8 has:
                        compatible = "ti,omap4430-timer";
                        ti,timer-pwm;
                        ti,timer-dsp;

and timer9 has:
                        compatible = "ti,omap4430-timer";
                        ti,hwmods = "timer9";
                        ti,timer-pwm;


> 
> Then for the users of a specific dmtimer, they can select the right one using
> the interrupt-parent property:
> 
> timer1: timer at 0x4800abcd {
> 	compatible = "ti,omap5430-timer";
> 	#interrupt-cells = <1>;		/* needs irqchip implemented for dmtimer */
> 	interrupt-controller;
> 	#clock-cells = <1>;		/* needs clocksource implemented for dmtimer */
> 	clock-output-names = "32k", "sys_ck";
> 	...
> };

In reference to my last thread reply, irqchip may not be available early in the
boot process (.init_time) for system timer usage?

thanks,

-Joel

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Joel Fernandes]

On 11/22/2013 09:58 AM, Rob Herring wrote:
> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote:
>> This work is a migration effort of OMAP system timers to the
>> clocksource/clockevent framework. Consider this as a first-pass in this effort.
>> There are few cleanups that need to be done first. The HWMOD code is
>> intertwined with the timer code. HWMOD code cleanups in the future will
>> hopefully make most of this code go away, so till then we separate out the
>> power/clocks portion of the code from the actual timer bits.  This will
>> facilitate near-future work of adapting the system timer as a clocksource.
>>
>> New functions for OF-only boot are introduced, and we can soon delete the old
>> versions once we migrate all platforms. Currently only AM335x is migrated and
>> testedA new omap_generic_timer_init function is introduced for DT platforms.
>> Code required earlier for non-DT platforms such as setup of timer IDs and timer
>> parent clock is not required.  parent clocks are automatically setup by the mux
>> clock driver through DT so they no longer need to be hardcoded.
>>
>> The init code will try to pick the best timer for clocksource and clockevent
>> however bindings are added to force a particular timer as clocksource or
>> clockevent through DT.
>>
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> ---
>>  .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
>>  arch/arm/mach-omap2/common.h                       |   1 +
>>  arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
>>  3 files changed, 248 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
>> index d02e27c..6cf7a75 100644
>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
>> @@ -32,6 +32,18 @@ Optional properties:
>>  - ti,timer-secure:     Indicates the timer is reserved on a secure OMAP device
>>                         and therefore cannot be used by the kernel.
>>
>> +- ti,timer-clockevent,
>> +  ti,timer-clocksource These properties force the system timer code to choose
>> +                       the particular timer as a clockevent or clocksource.
>> +                       If these properties are not specified, the timer code
>> +                       picks up a "ti,timer-alwon" as the clocksource and a
>> +                       timer containing one of the following properties as
>> +                       the clockevent in the following order:
>> +                               ti,timer-alwon
>> +                               ti,timer-dsp
>> +                               ti,timer-pwm
>> +                               ti,timer-secure
> 
> These properties were added specifically for the reason of avoiding
> linux specific properties like these. When is this not sufficient?

Some platforms cannot use certain timers as clockevents and clocksource, to keep
this code functionally equivalent and working, I added these properties so that
its possible to select specific timers as clockevents/sources as was being done
in the non-DT case. I'm open to suggestions for doing this in a better way for DT.

> 
> And I agree with the comment to use OF_CLKSRC.
> 

There are difficulties I mentioned in a previous post [1] stating why it may not
be possible to use OF_CLKSRC macros, and still keep the code functionally
equivalent to when it was non-DT.

thanks,

-Joel

[1] http://marc.info/?l=linux-arm-kernel&m=138513299917850&w=2

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Joel Fernandes]

On 11/22/2013 09:58 AM, Tony Lindgren wrote:
> * Joel Fernandes <joelf@ti.com> [131121 18:00]:
>> +Required properties for system timers (clockevents/clocksource):
>> +- ti,timer-parent:	System timer's parent mux clock needs to be setup.
>> +			This is currently hardcoded in code, for DT boot we
>> +			move this to DT.
>> +
> 
> This can be replaced with just clk_set_rate, or clk_set_parent if needed.
> Or by having a clocks = <&32k_clk> property in the dmtimer node in the
> .dts file.
> 
>>  Optional properties:
>>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
> 
> Hmm this we may not need, this can probably be deciphered from the compatible
> flag already?

I was thinking maybe we can improve the of_clocksource_init matching
capabilities to match better, not just on compatible but other properties for
better selection?

For example, a generic clocksource DT property to say that the timer is in the
wakeup domain and so should be preferred as the clocksource? Also additional
properties to force its selection?

Then we can switch to clocksource_of_init and have it do the selection. Not sure
if there is such a need or value for other ARM platforms so we may be over
complicating the framework to handle these quirks. Comments welcomed about it.

thanks,

-Joel

> 
> Then for the users of a specific dmtimer, they can select the right one using
> the interrupt-parent property:
> 
> timer1: timer at 0x4800abcd {
> 	compatible = "ti,omap5430-timer";
> 	#interrupt-cells = <1>;		/* needs irqchip implemented for dmtimer */
> 	interrupt-controller;
> 	#clock-cells = <1>;		/* needs clocksource implemented for dmtimer */
> 	clock-output-names = "32k", "sys_ck";
> 	...
> };
> 
> counter32k: counter at 4ae04000 {
> 	compatible = "ti,omap-counter32k";
> 	#clock-cells = <1>;		/* needs clocksource implemented for 32k counter */
> 	clock-output-names = "32k";
> 	...
> };
> 
> timer {
> 	compatible = "ti,omap5-timer";
> 	interrupt-parent = <&timer1>;
> 	interrupts = <1>;
> 	clocks = <&timer1 0>,		/* use timer1 as clockevent, clock index 0 = 32k, 1 = sys_ck ... */
> 		 <&counter32k 0>;	/* use 32k counter as clocksource */
> };
> 
>>  - ti,timer-dsp:		Indicates the timer can interrupt the on-chip DSP in
> 
> This can be probably also be mapped based on the compatible property?
> 
> Regards,
> 
> Tony
> 

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Tony Lindgren]

* Joel Fernandes <joelf@ti.com> [131122 08:37]:
> Hi Tony,
> 
> Thanks for your comments, few replies inline below..
> 
> On 11/22/2013 09:58 AM, Tony Lindgren wrote:
> > * Joel Fernandes <joelf@ti.com> [131121 18:00]:
> >> +Required properties for system timers (clockevents/clocksource):
> >> +- ti,timer-parent:	System timer's parent mux clock needs to be setup.
> >> +			This is currently hardcoded in code, for DT boot we
> >> +			move this to DT.
> >> +
> > 
> > This can be replaced with just clk_set_rate, or clk_set_parent if needed.
> > Or by having a clocks = <&32k_clk> property in the dmtimer node in the
> > .dts file.
> > 
> 
> Sure, but clock-data is still not available to make this possible in mainline.
> We also discussed earlier right that we don't want dependencies as much as
> possible to get one chunk in and working at a time. I was thinking like for a
> first-pass since there's a lot of unrelated code that doesn't have dependencies,
> but needs this to work, we can introduce this property for now and drop it later
> as a "cost of migration"?

I don't think there's a dependency here to the omap clocks as the dmtimer
can implement the clocksource separately and internally still use clk_get
using the clock alias table.
 
> >>  Optional properties:
> >>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
> > 
> > Hmm this we may not need, this can probably be deciphered from the compatible
> > flag already?
> 
> How? Compatible contains the same string, for example for OMAP4:
> 
> timer8 has:
>                         compatible = "ti,omap4430-timer";
>                         ti,timer-pwm;
>                         ti,timer-dsp;
> 
> and timer9 has:
>                         compatible = "ti,omap4430-timer";
>                         ti,hwmods = "timer9";
>                         ti,timer-pwm;
> 

Some of these features are always hardwired certain way and could be mapped to
the right timer based on the timer offset and the compatible flag if we want
to avoid adding the ti,timer-alwon property. It seems that most omaps have
just one always on timer that's the first timer, and only on am33xx it does
not exist?

I'm fine adding ti,timer-alwon if it help to leave out static data in the
driver and avoid patching the driver for every new SoC. But sounds like in
this case we really have just the am33xx exception to the rule?

> > Then for the users of a specific dmtimer, they can select the right one using
> > the interrupt-parent property:
> > 
> > timer1: timer at 0x4800abcd {
> > 	compatible = "ti,omap5430-timer";
> > 	#interrupt-cells = <1>;		/* needs irqchip implemented for dmtimer */
> > 	interrupt-controller;
> > 	#clock-cells = <1>;		/* needs clocksource implemented for dmtimer */
> > 	clock-output-names = "32k", "sys_ck";
> > 	...
> > };
> 
> In reference to my last thread reply, irqchip may not be available early in the
> boot process (.init_time) for system timer usage?

Hmm it should be, looks like we have in arch/arm/kernel/irq.c:

void __init init_IRQ(void)
{
	if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
		irqchip_init();
	else    
		machine_desc->init_irq();
}               

Then in init/main.c has:
	...
	early_irq_init();
	init_IRQ();
	tick_init();
	init_timers();
	hrtimers_init();
	softirq_init();
	timekeeping_init();
	time_init();
	...

So looks like we should have irqchip available?

Regards,

Tony

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Tony Lindgren]

* Joel Fernandes <joelf@ti.com> [131122 09:06]:
> On 11/22/2013 09:58 AM, Tony Lindgren wrote:
> > * Joel Fernandes <joelf@ti.com> [131121 18:00]:
> >> +Required properties for system timers (clockevents/clocksource):
> >> +- ti,timer-parent:	System timer's parent mux clock needs to be setup.
> >> +			This is currently hardcoded in code, for DT boot we
> >> +			move this to DT.
> >> +
> > 
> > This can be replaced with just clk_set_rate, or clk_set_parent if needed.
> > Or by having a clocks = <&32k_clk> property in the dmtimer node in the
> > .dts file.
> > 
> >>  Optional properties:
> >>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
> > 
> > Hmm this we may not need, this can probably be deciphered from the compatible
> > flag already?
> 
> I was thinking maybe we can improve the of_clocksource_init matching
> capabilities to match better, not just on compatible but other properties for
> better selection?
> 
> For example, a generic clocksource DT property to say that the timer is in the
> wakeup domain and so should be preferred as the clocksource? Also additional
> properties to force its selection?
> 
> Then we can switch to clocksource_of_init and have it do the selection. Not sure
> if there is such a need or value for other ARM platforms so we may be over
> complicating the framework to handle these quirks. Comments welcomed about it.

Maybe, but so far it seems that it can all be specified in the board specific
.dts file using standard properties, so I'd first play with that.

Regards,

Tony

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Rob Herring]

On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote:
> On 11/22/2013 09:58 AM, Rob Herring wrote:
>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote:
>>> This work is a migration effort of OMAP system timers to the
>>> clocksource/clockevent framework. Consider this as a first-pass in this effort.
>>> There are few cleanups that need to be done first. The HWMOD code is
>>> intertwined with the timer code. HWMOD code cleanups in the future will
>>> hopefully make most of this code go away, so till then we separate out the
>>> power/clocks portion of the code from the actual timer bits.  This will
>>> facilitate near-future work of adapting the system timer as a clocksource.
>>>
>>> New functions for OF-only boot are introduced, and we can soon delete the old
>>> versions once we migrate all platforms. Currently only AM335x is migrated and
>>> testedA new omap_generic_timer_init function is introduced for DT platforms.
>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer
>>> parent clock is not required.  parent clocks are automatically setup by the mux
>>> clock driver through DT so they no longer need to be hardcoded.
>>>
>>> The init code will try to pick the best timer for clocksource and clockevent
>>> however bindings are added to force a particular timer as clocksource or
>>> clockevent through DT.
>>>
>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>> ---
>>>  .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
>>>  arch/arm/mach-omap2/common.h                       |   1 +
>>>  arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
>>>  3 files changed, 248 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>> index d02e27c..6cf7a75 100644
>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>> @@ -32,6 +32,18 @@ Optional properties:
>>>  - ti,timer-secure:     Indicates the timer is reserved on a secure OMAP device
>>>                         and therefore cannot be used by the kernel.
>>>
>>> +- ti,timer-clockevent,
>>> +  ti,timer-clocksource These properties force the system timer code to choose
>>> +                       the particular timer as a clockevent or clocksource.
>>> +                       If these properties are not specified, the timer code
>>> +                       picks up a "ti,timer-alwon" as the clocksource and a
>>> +                       timer containing one of the following properties as
>>> +                       the clockevent in the following order:
>>> +                               ti,timer-alwon
>>> +                               ti,timer-dsp
>>> +                               ti,timer-pwm
>>> +                               ti,timer-secure
>>
>> These properties were added specifically for the reason of avoiding
>> linux specific properties like these. When is this not sufficient?
>
> Some platforms cannot use certain timers as clockevents and clocksource, to keep
> this code functionally equivalent and working, I added these properties so that
> its possible to select specific timers as clockevents/sources as was being done
> in the non-DT case. I'm open to suggestions for doing this in a better way for DT.

There has to be a defined reason why a given timer cannot be used. You
are not explaining what that reason is. Define a property or set of
properties that describe the h/w feature or quirk.

>>
>> And I agree with the comment to use OF_CLKSRC.
>>
>
> There are difficulties I mentioned in a previous post [1] stating why it may not
> be possible to use OF_CLKSRC macros, and still keep the code functionally
> equivalent to when it was non-DT.

Sorry, I don't buy it.

Rob

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Joel Fernandes]

On 11/22/2013 11:08 AM, Tony Lindgren wrote:
> * Joel Fernandes <joelf@ti.com> [131122 08:37]:
>> Hi Tony,
>>
>> Thanks for your comments, few replies inline below..
>>
>> On 11/22/2013 09:58 AM, Tony Lindgren wrote:
>>> * Joel Fernandes <joelf@ti.com> [131121 18:00]:
>>>> +Required properties for system timers (clockevents/clocksource):
>>>> +- ti,timer-parent:	System timer's parent mux clock needs to be setup.
>>>> +			This is currently hardcoded in code, for DT boot we
>>>> +			move this to DT.
>>>> +
>>>
>>> This can be replaced with just clk_set_rate, or clk_set_parent if needed.
>>> Or by having a clocks = <&32k_clk> property in the dmtimer node in the
>>> .dts file.
>>>
>>
>> Sure, but clock-data is still not available to make this possible in mainline.
>> We also discussed earlier right that we don't want dependencies as much as
>> possible to get one chunk in and working at a time. I was thinking like for a
>> first-pass since there's a lot of unrelated code that doesn't have dependencies,
>> but needs this to work, we can introduce this property for now and drop it later
>> as a "cost of migration"?
> 
> I don't think there's a dependency here to the omap clocks as the dmtimer
> can implement the clocksource separately and internally still use clk_get
> using the clock alias table.

You mean implement clock-tree separately? Sorry I'm confused can you clarify
what you mean?

In clock tree data (not upstream), here is the system clock for am335x for example:
sys_clkin_ck: sys_clkin_ck at 44e10040 {
        #clock-cells = <0>;
        compatible = "mux-clock";

It uses the mux-clock driver. Are you saying we duplicate clock-tree stuff? I
don't think that's a good idea specially since once clock dt data is available,
we will switch to using it.

>>>>  Optional properties:
>>>>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
>>>
>>> Hmm this we may not need, this can probably be deciphered from the compatible
>>> flag already?
>>
>> How? Compatible contains the same string, for example for OMAP4:
>>
>> timer8 has:
>>                         compatible = "ti,omap4430-timer";
>>                         ti,timer-pwm;
>>                         ti,timer-dsp;
>>
>> and timer9 has:
>>                         compatible = "ti,omap4430-timer";
>>                         ti,hwmods = "timer9";
>>                         ti,timer-pwm;
>>
> 
> Some of these features are always hardwired certain way and could be mapped to
> the right timer based on the timer offset and the compatible flag if we want
> to avoid adding the ti,timer-alwon property. It seems that most omaps have
> just one always on timer that's the first timer, and only on am33xx it does
> not exist?
> 
> I'm fine adding ti,timer-alwon if it help to leave out static data in the
> driver and avoid patching the driver for every new SoC. But sounds like in
> this case we really have just the am33xx exception to the rule?

ti,timer-alwon may not be needed yes, since on all platforms I've observed first
timer has this property. However from OMAP3 dt, timer12 has it too. Not sure
what that implies.

I think what Jon was trying to do is to find a DT node by property, he had no
other way of getting a device_node * otherwise.

But notice this macro used for HS (secure devices):
OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure",
                        2, "timer_sys_ck", NULL);

How would you specify in DT that you want a node with the timer-secure property
as the clocksource if we drop these kind of properties?

>>> Then for the users of a specific dmtimer, they can select the right one using
>>> the interrupt-parent property:
>>>
>>> timer1: timer at 0x4800abcd {
>>> 	compatible = "ti,omap5430-timer";
>>> 	#interrupt-cells = <1>;		/* needs irqchip implemented for dmtimer */
>>> 	interrupt-controller;
>>> 	#clock-cells = <1>;		/* needs clocksource implemented for dmtimer */
>>> 	clock-output-names = "32k", "sys_ck";
>>> 	...
>>> };
>>
>> In reference to my last thread reply, irqchip may not be available early in the
>> boot process (.init_time) for system timer usage?
> 
> Hmm it should be, looks like we have in arch/arm/kernel/irq.c:
> 
> void __init init_IRQ(void)
> {
> 	if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
> 		irqchip_init();
> 	else    
> 		machine_desc->init_irq();
> }               
> 
> Then in init/main.c has:
> 	...
> 	early_irq_init();
> 	init_IRQ();
> 	tick_init();
> 	init_timers();
> 	hrtimers_init();
> 	softirq_init();
> 	timekeeping_init();
> 	time_init();
> 	...
> 
> So looks like we should have irqchip available?

Right. I think your idea of using irqchip is certainly a clean way. Let me go
back to the drawing board and try to see if we have all the pieces we need and
there are no surprises in doing it this way.

Then we can have a general purpose clocksource driver that uses the irqchip
driver. I still worry about things like hwmod that may be need to be called on
specific timer, and runtime PM is not available that early in the boot process.

thanks,

-Joel

[PATCH 6/8] devicetree: doc: Document ti,timer-parent property

[Tony Lindgren]

* Joel Fernandes <joelf@ti.com> [131122 16:32]:
> On 11/22/2013 11:08 AM, Tony Lindgren wrote:
> > 
> > I don't think there's a dependency here to the omap clocks as the dmtimer
> > can implement the clocksource separately and internally still use clk_get
> > using the clock alias table.
> 
> You mean implement clock-tree separately? Sorry I'm confused can you clarify
> what you mean?

You could implement the needed clocks for client drivers to use in dmtimer.c
directly if dmtimer.c is the gating those clocks.
 
> In clock tree data (not upstream), here is the system clock for am335x for example:
> sys_clkin_ck: sys_clkin_ck at 44e10040 {
>         #clock-cells = <0>;
>         compatible = "mux-clock";
> 
> It uses the mux-clock driver. Are you saying we duplicate clock-tree stuff? I
> don't think that's a good idea specially since once clock dt data is available,
> we will switch to using it.

Oh OK, then that naturally could be used directly too.
 
> >>>>  Optional properties:
> >>>>  - ti,timer-alwon:	Indicates the timer is in an alway-on power domain.
> >>>
> >>> Hmm this we may not need, this can probably be deciphered from the compatible
> >>> flag already?
> >>
> >> How? Compatible contains the same string, for example for OMAP4:
> >>
> >> timer8 has:
> >>                         compatible = "ti,omap4430-timer";
> >>                         ti,timer-pwm;
> >>                         ti,timer-dsp;
> >>
> >> and timer9 has:
> >>                         compatible = "ti,omap4430-timer";
> >>                         ti,hwmods = "timer9";
> >>                         ti,timer-pwm;
> >>
> > 
> > Some of these features are always hardwired certain way and could be mapped to
> > the right timer based on the timer offset and the compatible flag if we want
> > to avoid adding the ti,timer-alwon property. It seems that most omaps have
> > just one always on timer that's the first timer, and only on am33xx it does
> > not exist?
> > 
> > I'm fine adding ti,timer-alwon if it help to leave out static data in the
> > driver and avoid patching the driver for every new SoC. But sounds like in
> > this case we really have just the am33xx exception to the rule?
> 
> ti,timer-alwon may not be needed yes, since on all platforms I've observed first
> timer has this property. However from OMAP3 dt, timer12 has it too. Not sure
> what that implies.

I guess you could mark timer1 and 12 as always on if the compatible flag 
matches ti,omap3430-timer.
 
> I think what Jon was trying to do is to find a DT node by property, he had no
> other way of getting a device_node * otherwise.
> 
> But notice this macro used for HS (secure devices):
> OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure",
>                         2, "timer_sys_ck", NULL);
> 
> How would you specify in DT that you want a node with the timer-secure property
> as the clocksource if we drop these kind of properties?

timer {
	interrupt-parent = <&timer12>;
	...
}

Should do the trick I think :)
 
> >>> Then for the users of a specific dmtimer, they can select the right one using
> >>> the interrupt-parent property:
> >>>
> >>> timer1: timer at 0x4800abcd {
> >>> 	compatible = "ti,omap5430-timer";
> >>> 	#interrupt-cells = <1>;		/* needs irqchip implemented for dmtimer */
> >>> 	interrupt-controller;
> >>> 	#clock-cells = <1>;		/* needs clocksource implemented for dmtimer */
> >>> 	clock-output-names = "32k", "sys_ck";
> >>> 	...
> >>> };
> >>
> >> In reference to my last thread reply, irqchip may not be available early in the
> >> boot process (.init_time) for system timer usage?
> > 
> > Hmm it should be, looks like we have in arch/arm/kernel/irq.c:
> > 
> > void __init init_IRQ(void)
> > {
> > 	if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
> > 		irqchip_init();
> > 	else    
> > 		machine_desc->init_irq();
> > }               
> > 
> > Then in init/main.c has:
> > 	...
> > 	early_irq_init();
> > 	init_IRQ();
> > 	tick_init();
> > 	init_timers();
> > 	hrtimers_init();
> > 	softirq_init();
> > 	timekeeping_init();
> > 	time_init();
> > 	...
> > 
> > So looks like we should have irqchip available?
> 
> Right. I think your idea of using irqchip is certainly a clean way. Let me go
> back to the drawing board and try to see if we have all the pieces we need and
> there are no surprises in doing it this way.

OK cool.
 
> Then we can have a general purpose clocksource driver that uses the irqchip
> driver. I still worry about things like hwmod that may be need to be called on
> specific timer, and runtime PM is not available that early in the boot process.

Yeah we may still need a piece of code in mach-omap2 for now if runtime PM is
not available at that point yet.

Regards,

Tony

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Joel Fernandes]

Adding Thomas to the thread since discussion is about clocksource, and Mark
Rutland as discussion is related to timers and DT, thanks.

On 11/22/2013 02:01 PM, Rob Herring wrote:
> On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote:
>> On 11/22/2013 09:58 AM, Rob Herring wrote:
>>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote:
>>>> This work is a migration effort of OMAP system timers to the
>>>> clocksource/clockevent framework. Consider this as a first-pass in this effort.
>>>> There are few cleanups that need to be done first. The HWMOD code is
>>>> intertwined with the timer code. HWMOD code cleanups in the future will
>>>> hopefully make most of this code go away, so till then we separate out the
>>>> power/clocks portion of the code from the actual timer bits.  This will
>>>> facilitate near-future work of adapting the system timer as a clocksource.
>>>>
>>>> New functions for OF-only boot are introduced, and we can soon delete the old
>>>> versions once we migrate all platforms. Currently only AM335x is migrated and
>>>> testedA new omap_generic_timer_init function is introduced for DT platforms.
>>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer
>>>> parent clock is not required.  parent clocks are automatically setup by the mux
>>>> clock driver through DT so they no longer need to be hardcoded.
>>>>
>>>> The init code will try to pick the best timer for clocksource and clockevent
>>>> however bindings are added to force a particular timer as clocksource or
>>>> clockevent through DT.
>>>>
>>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>>> ---
>>>>  .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
>>>>  arch/arm/mach-omap2/common.h                       |   1 +
>>>>  arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
>>>>  3 files changed, 248 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>> index d02e27c..6cf7a75 100644
>>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>> @@ -32,6 +32,18 @@ Optional properties:
>>>>  - ti,timer-secure:     Indicates the timer is reserved on a secure OMAP device
>>>>                         and therefore cannot be used by the kernel.
>>>>
>>>> +- ti,timer-clockevent,
>>>> +  ti,timer-clocksource These properties force the system timer code to choose
>>>> +                       the particular timer as a clockevent or clocksource.
>>>> +                       If these properties are not specified, the timer code
>>>> +                       picks up a "ti,timer-alwon" as the clocksource and a
>>>> +                       timer containing one of the following properties as
>>>> +                       the clockevent in the following order:
>>>> +                               ti,timer-alwon
>>>> +                               ti,timer-dsp
>>>> +                               ti,timer-pwm
>>>> +                               ti,timer-secure
>>>
>>> These properties were added specifically for the reason of avoiding
>>> linux specific properties like these. When is this not sufficient?
>>
>> Some platforms cannot use certain timers as clockevents and clocksource, to keep
>> this code functionally equivalent and working, I added these properties so that
>> its possible to select specific timers as clockevents/sources as was being done
>> in the non-DT case. I'm open to suggestions for doing this in a better way for DT.
> 
> There has to be a defined reason why a given timer cannot be used. You
> are not explaining what that reason is. Define a property or set of
> properties that describe the h/w feature or quirk.

Reason? There are HW bugs that prevent a timer from being used. See [1]. And
there may be other reasons, I haven't written this code but I know that there
are other HW bugs that made authors pick a specific timer such as board issues
or accuracy.

This is nothing new, just that I'm trying to find a way to do it from DT.

You can see the ifdef'ry below in mach-omap2/timer.c. All I'm trying to do is
make it simpler and cleaner by adding these properties to DT so that we can
delete this code entirely.


#ifdef CONFIG_ARCH_OMAP2
OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon",
                        2, "timer_sys_ck", NULL);
#endif /* CONFIG_ARCH_OMAP2 */

#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM43XX)
OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon",
                        2, "timer_sys_ck", NULL);
OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure",
                        2, "timer_sys_ck", NULL);
#endif /* CONFIG_ARCH_OMAP3 */

etc...

>>>
>>> And I agree with the comment to use OF_CLKSRC.
>>>
>>
>> There are difficulties I mentioned in a previous post [1] stating why it may not
>> be possible to use OF_CLKSRC macros, and still keep the code functionally
>> equivalent to when it was non-DT.
> 
> Sorry, I don't buy it.
> 

Ofcourse even I want to use OF_CLOCKSOURCE macros for registering DT
clocksources, but re-iterating, the selection of timer is not a simple match of
compatible property as you may see in my patch. Some platforms need specific
timer as clocksource, all timers have same "compatible" flag. How would you
differentiate? Refer to ifdef's above to see how timer IDs change across
platforms in the non-DT code that's being converted.

I think clocksource_of_init has been written such that any timer having a
matching compatible can be enumerated.

I can change the compatible to something unique in the DT like:

timer1:timer at ... {
  compatible = "ti,dmtimer-clocksource"
  ....
}

 and that may make clocksource work. But then such a similar approach to
clockevent is not possible. What I observed in other platforms is a *single* DT
node represents both clocksource and clockevent.

For example, in zevio-timer:
drivers/clocksource/zevio-timer.c

does this:
CLOCKSOURCE_OF_DECLARE(zevio_timer, "lsi,zevio-timer", zevio_timer_add);

In the body of zevio_timer_add(struct device_node *node), they do this:
        timer->base = of_iomap(node, 0);
        timer->timer1 = timer->base + IO_TIMER1; // USED AS CLOCKSOURCE
        timer->timer2 = timer->base + IO_TIMER2; // USED AS CLOCKEVENT

What's happening here is a single device node (DT node) represents both
clockevent and source. But for OMAP, we use different timers for clockevent and
clocksource. They are physically 2 different timer DT nodes. How do we register
something like this with the CLOCKSOURCE_OF_DECLARE macro?

Looking forward to your suggestions.

thanks,

-Joel

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Joel Fernandes]

Sorry, resending due to an MUA issue.

Adding Thomas to the thread since discussion is about clocksource, and Mark
Rutland as discussion is related to timers and DT, thanks.

On 11/22/2013 02:01 PM, Rob Herring wrote:
> On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote:
>> On 11/22/2013 09:58 AM, Rob Herring wrote:
>>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote:
>>>> This work is a migration effort of OMAP system timers to the
>>>> clocksource/clockevent framework. Consider this as a first-pass in this effort.
>>>> There are few cleanups that need to be done first. The HWMOD code is
>>>> intertwined with the timer code. HWMOD code cleanups in the future will
>>>> hopefully make most of this code go away, so till then we separate out the
>>>> power/clocks portion of the code from the actual timer bits.  This will
>>>> facilitate near-future work of adapting the system timer as a clocksource.
>>>>
>>>> New functions for OF-only boot are introduced, and we can soon delete the old
>>>> versions once we migrate all platforms. Currently only AM335x is migrated and
>>>> testedA new omap_generic_timer_init function is introduced for DT platforms.
>>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer
>>>> parent clock is not required.  parent clocks are automatically setup by the mux
>>>> clock driver through DT so they no longer need to be hardcoded.
>>>>
>>>> The init code will try to pick the best timer for clocksource and clockevent
>>>> however bindings are added to force a particular timer as clocksource or
>>>> clockevent through DT.
>>>>
>>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>>> ---
>>>>  .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
>>>>  arch/arm/mach-omap2/common.h                       |   1 +
>>>>  arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
>>>>  3 files changed, 248 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>> index d02e27c..6cf7a75 100644
>>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>> @@ -32,6 +32,18 @@ Optional properties:
>>>>  - ti,timer-secure:     Indicates the timer is reserved on a secure OMAP device
>>>>                         and therefore cannot be used by the kernel.
>>>>
>>>> +- ti,timer-clockevent,
>>>> +  ti,timer-clocksource These properties force the system timer code to choose
>>>> +                       the particular timer as a clockevent or clocksource.
>>>> +                       If these properties are not specified, the timer code
>>>> +                       picks up a "ti,timer-alwon" as the clocksource and a
>>>> +                       timer containing one of the following properties as
>>>> +                       the clockevent in the following order:
>>>> +                               ti,timer-alwon
>>>> +                               ti,timer-dsp
>>>> +                               ti,timer-pwm
>>>> +                               ti,timer-secure
>>>
>>> These properties were added specifically for the reason of avoiding
>>> linux specific properties like these. When is this not sufficient?
>>
>> Some platforms cannot use certain timers as clockevents and clocksource, to keep
>> this code functionally equivalent and working, I added these properties so that
>> its possible to select specific timers as clockevents/sources as was being done
>> in the non-DT case. I'm open to suggestions for doing this in a better way for DT.
> 
> There has to be a defined reason why a given timer cannot be used. You
> are not explaining what that reason is. Define a property or set of
> properties that describe the h/w feature or quirk.

Reason? There are HW bugs that prevent a timer from being used. See [1]. And
there may be other reasons, I haven't written this code but I know that there
are other HW bugs that made authors pick a specific timer such as board issues
or accuracy.

This is nothing new, just that I'm trying to find a way to do it from DT.

You can see the ifdef'ry below in mach-omap2/timer.c. All I'm trying to do is
make it simpler and cleaner by adding these properties to DT so that we can
delete this code entirely.


#ifdef CONFIG_ARCH_OMAP2
OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon",
                        2, "timer_sys_ck", NULL);
#endif /* CONFIG_ARCH_OMAP2 */

#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM43XX)
OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon",
                        2, "timer_sys_ck", NULL);
OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure",
                        2, "timer_sys_ck", NULL);
#endif /* CONFIG_ARCH_OMAP3 */

etc...

>>>
>>> And I agree with the comment to use OF_CLKSRC.
>>>
>>
>> There are difficulties I mentioned in a previous post [1] stating why it may not
>> be possible to use OF_CLKSRC macros, and still keep the code functionally
>> equivalent to when it was non-DT.
> 
> Sorry, I don't buy it.
> 

Ofcourse even I want to use OF_CLOCKSOURCE macros for registering DT
clocksources, but re-iterating, the selection of timer is not a simple match of
compatible property as you may see in my patch. Some platforms need specific
timer as clocksource, all timers have same "compatible" flag. How would you
differentiate? Refer to ifdef's above to see how timer IDs change across
platforms in the non-DT code that's being converted.

I think clocksource_of_init has been written such that any timer having a
matching compatible can be enumerated.

I can change the compatible to something unique in the DT like:

timer1:timer at ... {
  compatible = "ti,dmtimer-clocksource"
  ....
}

 and that may make clocksource work. But then such a similar approach to
clockevent is not possible. What I observed in other platforms is a *single* DT
node represents both clocksource and clockevent.

For example, in zevio-timer:
drivers/clocksource/zevio-timer.c

does this:
CLOCKSOURCE_OF_DECLARE(zevio_timer, "lsi,zevio-timer", zevio_timer_add);

In the body of zevio_timer_add(struct device_node *node), they do this:
        timer->base = of_iomap(node, 0);
        timer->timer1 = timer->base + IO_TIMER1; // USED AS CLOCKSOURCE
        timer->timer2 = timer->base + IO_TIMER2; // USED AS CLOCKEVENT

What's happening here is a single device node (DT node) represents both
clockevent and source. But for OMAP, we use different timers for clockevent and
clocksource. They are physically 2 different timer DT nodes. How do we register
something like this with the CLOCKSOURCE_OF_DECLARE macro?

Looking forward to your suggestions.

thanks,

-Joel

[PATCH 5/8] ARM: OMAP2+: timer: Introduce OF-friendly clocksource/clockevent system timers

[Rob Herring]

On Fri, Nov 22, 2013 at 7:12 PM, Joel Fernandes <joelf@ti.com> wrote:
> Adding Thomas to the thread since discussion is about clocksource, and Mark
> Rutland as discussion is related to timers and DT, thanks.
>
> On 11/22/2013 02:01 PM, Rob Herring wrote:
>> On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote:
>>> On 11/22/2013 09:58 AM, Rob Herring wrote:
>>>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote:
>>>>> This work is a migration effort of OMAP system timers to the
>>>>> clocksource/clockevent framework. Consider this as a first-pass in this effort.
>>>>> There are few cleanups that need to be done first. The HWMOD code is
>>>>> intertwined with the timer code. HWMOD code cleanups in the future will
>>>>> hopefully make most of this code go away, so till then we separate out the
>>>>> power/clocks portion of the code from the actual timer bits.  This will
>>>>> facilitate near-future work of adapting the system timer as a clocksource.
>>>>>
>>>>> New functions for OF-only boot are introduced, and we can soon delete the old
>>>>> versions once we migrate all platforms. Currently only AM335x is migrated and
>>>>> testedA new omap_generic_timer_init function is introduced for DT platforms.
>>>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer
>>>>> parent clock is not required.  parent clocks are automatically setup by the mux
>>>>> clock driver through DT so they no longer need to be hardcoded.
>>>>>
>>>>> The init code will try to pick the best timer for clocksource and clockevent
>>>>> however bindings are added to force a particular timer as clocksource or
>>>>> clockevent through DT.
>>>>>
>>>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>>>> ---
>>>>>  .../devicetree/bindings/arm/omap/timer.txt         |  12 ++
>>>>>  arch/arm/mach-omap2/common.h                       |   1 +
>>>>>  arch/arm/mach-omap2/timer.c                        | 235 +++++++++++++++++++++
>>>>>  3 files changed, 248 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>>> index d02e27c..6cf7a75 100644
>>>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
>>>>> @@ -32,6 +32,18 @@ Optional properties:
>>>>>  - ti,timer-secure:     Indicates the timer is reserved on a secure OMAP device
>>>>>                         and therefore cannot be used by the kernel.
>>>>>
>>>>> +- ti,timer-clockevent,
>>>>> +  ti,timer-clocksource These properties force the system timer code to choose
>>>>> +                       the particular timer as a clockevent or clocksource.
>>>>> +                       If these properties are not specified, the timer code
>>>>> +                       picks up a "ti,timer-alwon" as the clocksource and a
>>>>> +                       timer containing one of the following properties as
>>>>> +                       the clockevent in the following order:
>>>>> +                               ti,timer-alwon
>>>>> +                               ti,timer-dsp
>>>>> +                               ti,timer-pwm
>>>>> +                               ti,timer-secure
>>>>
>>>> These properties were added specifically for the reason of avoiding
>>>> linux specific properties like these. When is this not sufficient?
>>>
>>> Some platforms cannot use certain timers as clockevents and clocksource, to keep
>>> this code functionally equivalent and working, I added these properties so that
>>> its possible to select specific timers as clockevents/sources as was being done
>>> in the non-DT case. I'm open to suggestions for doing this in a better way for DT.
>>
>> There has to be a defined reason why a given timer cannot be used. You
>> are not explaining what that reason is. Define a property or set of
>> properties that describe the h/w feature or quirk.
>
> Reason? There are HW bugs that prevent a timer from being used. See [1]. And
> there may be other reasons, I haven't written this code but I know that there
> are other HW bugs that made authors pick a specific timer such as board issues
> or accuracy.

Bugs are a feature of the h/w. Add a "ti,timer-broken" property if you
don't know the specific bug. Accuracy is a function of counter bit
size and frequency. The only board issues for a timer I can imagine is
you want to reserve timers with output compare or input capture pins
for other uses. These are all properties that you can describe in the
binding. If you just don't want to use a timer for some arbitrary
reason, then add a 'status = "disabled";' property.

>
> This is nothing new, just that I'm trying to find a way to do it from DT.
>
> You can see the ifdef'ry below in mach-omap2/timer.c. All I'm trying to do is
> make it simpler and cleaner by adding these properties to DT so that we can
> delete this code entirely.
>
>
> #ifdef CONFIG_ARCH_OMAP2
> OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon",
>                         2, "timer_sys_ck", NULL);
> #endif /* CONFIG_ARCH_OMAP2 */
>
> #if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM43XX)
> OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon",
>                         2, "timer_sys_ck", NULL);
> OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure",
>                         2, "timer_sys_ck", NULL);
> #endif /* CONFIG_ARCH_OMAP3 */

Hence why compatible properties are supposed to be specific. You
should have ti,omap2-timer, ti,omap3-timer, and ti,am43xx-timer as
compatible properties.

> etc...
>
>>>>
>>>> And I agree with the comment to use OF_CLKSRC.
>>>>
>>>
>>> There are difficulties I mentioned in a previous post [1] stating why it may not
>>> be possible to use OF_CLKSRC macros, and still keep the code functionally
>>> equivalent to when it was non-DT.
>>
>> Sorry, I don't buy it.
>>
>
> Ofcourse even I want to use OF_CLOCKSOURCE macros for registering DT
> clocksources, but re-iterating, the selection of timer is not a simple match of
> compatible property as you may see in my patch. Some platforms need specific
> timer as clocksource, all timers have same "compatible" flag. How would you
> differentiate? Refer to ifdef's above to see how timer IDs change across
> platforms in the non-DT code that's being converted.
>
> I think clocksource_of_init has been written such that any timer having a
> matching compatible can be enumerated.
>
> I can change the compatible to something unique in the DT like:
>
> timer1:timer at ... {
>   compatible = "ti,dmtimer-clocksource"
>   ....
> }
>
>  and that may make clocksource work. But then such a similar approach to
> clockevent is not possible. What I observed in other platforms is a *single* DT
> node represents both clocksource and clockevent.

Because the timer h/w represents a single IP block in these cases.
This is the case for sp804 which I wrote. And I also care which timer
is which function because only 1 timer has an interrupt connected. All
this was handled by describing the h/w and leaving it to the kernel to
figure out which timer is which.

>
> For example, in zevio-timer:
> drivers/clocksource/zevio-timer.c
>
> does this:
> CLOCKSOURCE_OF_DECLARE(zevio_timer, "lsi,zevio-timer", zevio_timer_add);
>
> In the body of zevio_timer_add(struct device_node *node), they do this:
>         timer->base = of_iomap(node, 0);
>         timer->timer1 = timer->base + IO_TIMER1; // USED AS CLOCKSOURCE
>         timer->timer2 = timer->base + IO_TIMER2; // USED AS CLOCKEVENT
>
> What's happening here is a single device node (DT node) represents both
> clockevent and source. But for OMAP, we use different timers for clockevent and
> clocksource. They are physically 2 different timer DT nodes. How do we register
> something like this with the CLOCKSOURCE_OF_DECLARE macro?

To start with, declare a macro and init function for each SOC and add
an of_machine_is_compatible check if you need to different selection
logic for each SOC.

You might look at integrator_cp_of_init which deals with having 3
different timers.

Rob

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Joel Fernandes]

Hi Tony,

Few replies inline below. Adding Suman as well who is planning to use dmtimer
directly for his work with DSP.

On 11/22/2013 09:33 AM, Tony Lindgren wrote:
> Hi,
> 
> * Joel Fernandes <joelf@ti.com> [131121 18:00]:
>> Multiplatform support has made arch/arm/plat-omap/include/plat/ inaccessible to
>> drivers outside the plat-omap directory [1]. Due to this the following drivers
>> are disabled with !CONFIG_ARCH_MULTIPLATFORM:
>> CONFIG_IR_RX51 (drivers/media/rc/ir-rx51.c)
>> CONFIG_TIDSPBRIDGE (drivers/staging/tidspbridge/core/dsp-clock.c)
>>
>> We move the portion of the dmtimer "API" that should be accessible to the
>> drivers, into include/linux/omap-timer.h
> 
> As we chatted earlier, we don't have to expose all these hardware specific
> functions and use existing Linux generic frameworks instead.

I thought over this, and strongly feel that in this case moving to a generic
framework is not the right approach. I mentioned reasons in last post, but
summarizing all of them below:

1. It is not immediately intuitive or obvious unlike GPIO that a timer can be an
irqchip.

2. There are only 1-2 users of the dmtimer directly so moving to linux generic
framework is bit overkill I feel. Further there is already a framework-
clockevents/hrtimer which everyone should be using. For the 1-2 direct dmtimer
users, they should just use the dmtimer public API.

More reasons below...

> 
> We can implement an irqchip and a clocksource in the dmtimer code for the
> client drivers to use, and after that we only have a couple of dmtimer
> specific functions left to export.
> 
> I'm thinkging some thing like this for the public API:
> 
> omap_dm_timer_request			request_irq
> omap_dm_timer_request_specific		request_irq
> omap_dm_timer_get_irq			request_irq
> omap_dm_timer_set_source		clk_set_rate

For clk_set_rate, how would one directly access the timer node if we've hidden
it behind an irq chip abstraction?

per your suggestion, one would have something like:

dsp {
    interrupt-parent = <&timer1>;
}

so how do you clk_set_rate rate something like this given the dsp node?

If the suggestion is to get the timer1 node from the interrupt-parent property,
if I may say- that's a bit ugly because now you're breaking the irq chip
abstraction just to access the timer node..

> omap_dm_timer_stop			disable_irq
> omap_dm_timer_start			enable_irq
> omap_dm_timer_disable			disable_irq
> omap_dm_timer_free			free_irq
> omap_dm_timer_set_int_enable		enable_irq
> 
> After that, what's left to export are some functions to configure
> the hardware timer:
> 
> omap_dm_timer_write_counter
> omap_dm_timer_set_match
> omap_dm_timer_read_counter
> omap_dm_timer_read_status
> omap_dm_timer_write_status

Same comment for clk_set_rate applies here, we've to access interrupt-parent
property to get timer node and call dmtimer API which breaks the abstraction.

Usually I've seen frameworks come up because there is code duplication etc. In
this case there is no duplication as such, and the number of users of the API is
also few. What is the problem with exposing a small set of API to timer users in
drivers/ specially when they are prefixed by a unique name (omap_dm_timer)?

thanks,

-Joel

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Tony Lindgren]

* Joel Fernandes <joelf@ti.com> [131125 19:03]:
> Hi Tony,
> 
> Few replies inline below. Adding Suman as well who is planning to use dmtimer
> directly for his work with DSP.
> 
> On 11/22/2013 09:33 AM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Joel Fernandes <joelf@ti.com> [131121 18:00]:
> >> Multiplatform support has made arch/arm/plat-omap/include/plat/ inaccessible to
> >> drivers outside the plat-omap directory [1]. Due to this the following drivers
> >> are disabled with !CONFIG_ARCH_MULTIPLATFORM:
> >> CONFIG_IR_RX51 (drivers/media/rc/ir-rx51.c)
> >> CONFIG_TIDSPBRIDGE (drivers/staging/tidspbridge/core/dsp-clock.c)
> >>
> >> We move the portion of the dmtimer "API" that should be accessible to the
> >> drivers, into include/linux/omap-timer.h
> > 
> > As we chatted earlier, we don't have to expose all these hardware specific
> > functions and use existing Linux generic frameworks instead.
> 
> I thought over this, and strongly feel that in this case moving to a generic
> framework is not the right approach. I mentioned reasons in last post, but
> summarizing all of them below:
> 
> 1. It is not immediately intuitive or obvious unlike GPIO that a timer can be an
> irqchip.

But that's what it really is for hadware timers :)
 
> 2. There are only 1-2 users of the dmtimer directly so moving to linux generic
> framework is bit overkill I feel. Further there is already a framework-
> clockevents/hrtimer which everyone should be using. For the 1-2 direct dmtimer
> users, they should just use the dmtimer public API.

But the thing is, we don't want to expose the dmtimer public API. That will
lead to drivers tinkering directly with the hardware timers and then other
SoCs will follow.
 
> More reasons below...
> 
> > 
> > We can implement an irqchip and a clocksource in the dmtimer code for the
> > client drivers to use, and after that we only have a couple of dmtimer
> > specific functions left to export.
> > 
> > I'm thinkging some thing like this for the public API:
> > 
> > omap_dm_timer_request			request_irq
> > omap_dm_timer_request_specific		request_irq
> > omap_dm_timer_get_irq			request_irq
> > omap_dm_timer_set_source		clk_set_rate
> 
> For clk_set_rate, how would one directly access the timer node if we've hidden
> it behind an irq chip abstraction?
> 
> per your suggestion, one would have something like:
> 
> dsp {
>     interrupt-parent = <&timer1>;
> }
> 
> so how do you clk_set_rate rate something like this given the dsp node?

All you have to do is implement a clocksource driver in dmtimer.c code.
 
> If the suggestion is to get the timer1 node from the interrupt-parent property,
> if I may say- that's a bit ugly because now you're breaking the irq chip
> abstraction just to access the timer node..

Hmm sorry I don't follow you here.
 
> > omap_dm_timer_stop			disable_irq
> > omap_dm_timer_start			enable_irq
> > omap_dm_timer_disable			disable_irq
> > omap_dm_timer_free			free_irq
> > omap_dm_timer_set_int_enable		enable_irq
> > 
> > After that, what's left to export are some functions to configure
> > the hardware timer:
> > 
> > omap_dm_timer_write_counter
> > omap_dm_timer_set_match
> > omap_dm_timer_read_counter
> > omap_dm_timer_read_status
> > omap_dm_timer_write_status
> 
> Same comment for clk_set_rate applies here, we've to access interrupt-parent
> property to get timer node and call dmtimer API which breaks the abstraction.

Sorry, I don't follow you here either. I'm sure we'll have some Linux generic
standard for programming hardware timers, but still using request_irq + 
clk_set_rate is very standard behaviour no matter what happens later on.
 
> Usually I've seen frameworks come up because there is code duplication etc. In
> this case there is no duplication as such, and the number of users of the API is
> also few. What is the problem with exposing a small set of API to timer users in
> drivers/ specially when they are prefixed by a unique name (omap_dm_timer)?

Because we don't want to export 27 omap specific functions to drivers:

$ grep EXPORT_SYMBOL arch/arm/plat-omap/dmtimer.c | wc -l
27

Instead, it looks like we can get away with exporting just:

omap_dm_timer_write_counter
omap_dm_timer_set_match
omap_dm_timer_read_counter
omap_dm_timer_read_status
omap_dm_timer_write_status

And I'm sure we'll have some solution for those also later on.

Regards,

Tony

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Joel Fernandes]

On 11/26/2013 12:29 PM, Tony Lindgren wrote:
[..]
>>> We can implement an irqchip and a clocksource in the dmtimer code for the
>>> client drivers to use, and after that we only have a couple of dmtimer
>>> specific functions left to export.
>>>
>>> I'm thinkging some thing like this for the public API:
>>>
>>> omap_dm_timer_request			request_irq
>>> omap_dm_timer_request_specific		request_irq
>>> omap_dm_timer_get_irq			request_irq
>>> omap_dm_timer_set_source		clk_set_rate
>>
>> For clk_set_rate, how would one directly access the timer node if we've hidden
>> it behind an irq chip abstraction?
>>
>> per your suggestion, one would have something like:
>>
>> dsp {
>>     interrupt-parent = <&timer1>;
>> }
>>
>> so how do you clk_set_rate rate something like this given the dsp node?
> 
> All you have to do is implement a clocksource driver in dmtimer.c code.
>  
>> If the suggestion is to get the timer1 node from the interrupt-parent property,
>> if I may say- that's a bit ugly because now you're breaking the irq chip
>> abstraction just to access the timer node..
> 
> Hmm sorry I don't follow you here.

I assumed above that you were suggesting implementing interrupt chaining like
gpio-omap driver.

Can you give an example workflow to explain your suggestion?

I'll tell you how I understand what you were suggesting and then you can correct
me, and maybe we can meet somewhere:

Typically *without* irqchip or chaining what you suggested, we would  have
something like (purely an example):
dsp {
    timer = <&timer1>;
}

There is an API:
omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np); that one
would use.

Now moving to your suggestion, the dts would look like:
dsp {
    interrupt-parrent = <&timer1>;
    interrupts = <1>;
}

Naturally some APIs will not fit into the IRQ framework, so these subset of
dmtimer API may need to be exposed as you pointed. To use the API, the timer has
to first be retrieved from interrupt-parent property of "dsp" here to get a
device_node, and then the timer has to be requested and subset API used on it.
This is "hack" that's not acceptable according to me...

thanks,

-Joel

[PATCH 1/8] ARM: OMAP: Move public portion of dmtimer.h to include/linux/omap-timer.h

[Tony Lindgren]

* Joel Fernandes <joelf@ti.com> [131126 11:53]:
> On 11/26/2013 12:29 PM, Tony Lindgren wrote:
> [..]
> >>> We can implement an irqchip and a clocksource in the dmtimer code for the
> >>> client drivers to use, and after that we only have a couple of dmtimer
> >>> specific functions left to export.
> >>>
> >>> I'm thinkging some thing like this for the public API:
> >>>
> >>> omap_dm_timer_request			request_irq
> >>> omap_dm_timer_request_specific		request_irq
> >>> omap_dm_timer_get_irq			request_irq
> >>> omap_dm_timer_set_source		clk_set_rate
> >>
> >> For clk_set_rate, how would one directly access the timer node if we've hidden
> >> it behind an irq chip abstraction?
> >>
> >> per your suggestion, one would have something like:
> >>
> >> dsp {
> >>     interrupt-parent = <&timer1>;
> >> }
> >>
> >> so how do you clk_set_rate rate something like this given the dsp node?
> > 
> > All you have to do is implement a clocksource driver in dmtimer.c code.
> >  
> >> If the suggestion is to get the timer1 node from the interrupt-parent property,
> >> if I may say- that's a bit ugly because now you're breaking the irq chip
> >> abstraction just to access the timer node..
> > 
> > Hmm sorry I don't follow you here.
> 
> I assumed above that you were suggesting implementing interrupt chaining like
> gpio-omap driver.
> 
> Can you give an example workflow to explain your suggestion?
> 
> I'll tell you how I understand what you were suggesting and then you can correct
> me, and maybe we can meet somewhere:
> 
> Typically *without* irqchip or chaining what you suggested, we would  have
> something like (purely an example):
> dsp {
>     timer = <&timer1>;
> }
> 
> There is an API:
> omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np); that one
> would use.
> 
> Now moving to your suggestion, the dts would look like:
> dsp {
>     interrupt-parrent = <&timer1>;
>     interrupts = <1>;
> }

Yes something like that.
 
> Naturally some APIs will not fit into the IRQ framework, so these subset of
> dmtimer API may need to be exposed as you pointed. To use the API, the timer has
> to first be retrieved from interrupt-parent property of "dsp" here to get a
> device_node, and then the timer has to be requested and subset API used on it.
> This is "hack" that's not acceptable according to me...

Well it's really the same story with the GPIO framework if you follow the
request_irq analogy for a GPIO pin. You may still need to configure some things
manually that don't fit into the IRQ framework using let's say pinctrl framework.
Sure some of that can be automated eventually for GPIO and also for hardware
timers when we eventually have some generic hardware timer framework,
but the request_irq and clock_set_rate parts will stay valid.

Regards,

Tony