[PATCH 1/9] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[PATCH 1/9] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

Move ce field out of struct bc_timer into struct rk_clock_event_device,
rename struct bc_timer to struct rk_timer.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..6d68d4c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -29,18 +29,28 @@
 #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
 #define TIMER_INT_UNMASK			(1 << 2)
 
-struct bc_timer {
-	struct clock_event_device ce;
+struct rk_timer {
 	void __iomem *base;
 	void __iomem *ctrl;
 	u32 freq;
 };
 
-static struct bc_timer bc_timer;
+struct rk_clock_event_device {
+	struct clock_event_device ce;
+	struct rk_timer timer;
+};
+
+static struct rk_clock_event_device bc_timer;
+
+static inline struct rk_clock_event_device*
+rk_clock_event_device(struct clock_event_device *ce)
+{
+	return container_of(ce, struct rk_clock_event_device, ce);
+}
 
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 {
-	return container_of(ce, struct bc_timer, ce);
+	return &rk_clock_event_device(ce)->timer;
 }
 
 static inline void __iomem *rk_base(struct clock_event_device *ce)
@@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
+	struct rk_timer *timer = &bc_timer.timer;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
-	bc_timer.base = of_iomap(np, 0);
-	if (!bc_timer.base) {
+	timer->base = of_iomap(np, 0);
+	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
 		return -ENXIO;
 	}
-	bc_timer.ctrl = bc_timer.base + ctrl_reg;
+	timer->ctrl = timer->base + ctrl_reg;
 
 	pclk = of_clk_get_by_name(np, "pclk");
 	if (IS_ERR(pclk)) {
@@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_timer_clk;
 	}
 
-	bc_timer.freq = clk_get_rate(timer_clk);
+	timer->freq = clk_get_rate(timer_clk);
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
@@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 
 	return 0;
 
@@ -190,7 +201,7 @@ out_irq:
 out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
-	iounmap(bc_timer.base);
+	iounmap(timer->base);
 
 	return ret;
 }
-- 
1.7.9.5

[PATCH 2/9] clocksource/drivers/rockchip_timer: low level routines take rk_timer as parameter

[Alexander Kochetkov]

Pass rk_timer instead of clock_event_device to low lever timer routines.
So that code could be reused by clocksource implementation.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   44 ++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 6d68d4c..f84f67c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -63,60 +63,64 @@ static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
 	return rk_timer(ce)->ctrl;
 }
 
-static inline void rk_timer_disable(struct clock_event_device *ce)
+static inline void rk_timer_disable(struct rk_timer *timer)
 {
-	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
+	writel_relaxed(TIMER_DISABLE, timer->ctrl);
 }
 
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_ctrl(ce));
+		       timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
-				    struct clock_event_device *ce)
+				    struct rk_timer *timer)
 {
-	writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1);
+	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
 }
 
-static void rk_timer_interrupt_clear(struct clock_event_device *ce)
+static void rk_timer_interrupt_clear(struct rk_timer *timer)
 {
-	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
+	writel_relaxed(1, timer->base + TIMER_INT_STATUS);
 }
 
 static inline int rk_timer_set_next_event(unsigned long cycles,
 					  struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(cycles, ce);
-	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	struct rk_timer *timer = rk_timer(ce);
+	rk_timer_disable(timer);
+	rk_timer_update_counter(cycles, timer);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
 	return 0;
 }
 
 static int rk_timer_shutdown(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	struct rk_timer *timer = rk_timer(ce);
+	rk_timer_disable(timer);
 	return 0;
 }
 
 static int rk_timer_set_periodic(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
-	rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	struct rk_timer *timer = rk_timer(ce);
+	rk_timer_disable(timer);
+	rk_timer_update_counter(timer->freq / HZ - 1, timer);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
 	return 0;
 }
 
 static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *ce = dev_id;
+	struct rk_timer *timer = rk_timer(ce);
 
-	rk_timer_interrupt_clear(ce);
+	rk_timer_interrupt_clear(timer);
 
 	if (clockevent_state_oneshot(ce))
-		rk_timer_disable(ce);
+		rk_timer_disable(timer);
 
 	ce->event_handler(ce);
 
@@ -183,8 +187,8 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 	ce->cpumask = cpu_possible_mask;
 	ce->rating = 250;
 
-	rk_timer_interrupt_clear(ce);
-	rk_timer_disable(ce);
+	rk_timer_interrupt_clear(timer);
+	rk_timer_disable(timer);
 
 	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
 	if (ret) {
-- 
1.7.9.5

[PATCH 3/9] clocksource/drivers/rockchip_timer: drop unused rk_base() and rk_ctrl()

[Alexander Kochetkov]

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index f84f67c..2f18166 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -53,16 +53,6 @@ static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 	return &rk_clock_event_device(ce)->timer;
 }
 
-static inline void __iomem *rk_base(struct clock_event_device *ce)
-{
-	return rk_timer(ce)->base;
-}
-
-static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
-{
-	return rk_timer(ce)->ctrl;
-}
-
 static inline void rk_timer_disable(struct rk_timer *timer)
 {
 	writel_relaxed(TIMER_DISABLE, timer->ctrl);
-- 
1.7.9.5

[PATCH 4/9] clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of rk_timer_enable()

[Alexander Kochetkov]

This allow to enable timer without enabling interrupts from it.
As that mode will be used in clocksource implementation.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 2f18166..3b530f9 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -60,8 +60,7 @@ static inline void rk_timer_disable(struct rk_timer *timer)
 
 static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
-	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       timer->ctrl);
+	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
@@ -82,7 +81,7 @@ static inline int rk_timer_set_next_event(unsigned long cycles,
 	struct rk_timer *timer = rk_timer(ce);
 	rk_timer_disable(timer);
 	rk_timer_update_counter(cycles, timer);
-	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT | TIMER_INT_UNMASK);
 	return 0;
 }
 
@@ -98,7 +97,7 @@ static int rk_timer_set_periodic(struct clock_event_device *ce)
 	struct rk_timer *timer = rk_timer(ce);
 	rk_timer_disable(timer);
 	rk_timer_update_counter(timer->freq / HZ - 1, timer);
-	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING | TIMER_INT_UNMASK);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 5/9] clocksource/drivers/rockchip_timer: implement loading 64bit value into timer

[Alexander Kochetkov]

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 3b530f9..f4c26be 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -63,11 +63,13 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
-static void rk_timer_update_counter(unsigned long cycles,
-				    struct rk_timer *timer)
+static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
 {
-	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
+	u32 lower = cycles & 0xFFFFFFFF;
+	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
+
+	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
 }
 
 static void rk_timer_interrupt_clear(struct rk_timer *timer)
-- 
1.7.9.5

[PATCH 6/9] clocksource/drivers/rockchip_timer: implement reading 64bit value from timer

[Alexander Kochetkov]

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index f4c26be..61787c5 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -19,6 +19,8 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
+#define TIMER_CURRENT_VALUE0	0x08
+#define TIMER_CURRENT_VALUE1	0x0C
 #define TIMER_CONTROL_REG3288	0x10
 #define TIMER_CONTROL_REG3399	0x1c
 #define TIMER_INT_STATUS	0x18
@@ -72,6 +74,25 @@ static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
 	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
 }
 
+static u64 rk_timer_counter_read(struct rk_timer *timer)
+{
+	u64 counter;
+	u32 lower;
+	u32 upper, old_upper;
+
+	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	do {
+		old_upper = upper;
+		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
+		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	} while (upper != old_upper);
+
+	counter = upper;
+	counter <<= 32;
+	counter |= lower;
+	return counter;
+}
+
 static void rk_timer_interrupt_clear(struct rk_timer *timer)
 {
 	writel_relaxed(1, timer->base + TIMER_INT_STATUS);
-- 
1.7.9.5

[PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

The patch implement stable clocksource for rk3188. It register
one of the timers as clocksource and sched_clock. Also it override
arm_global_timer clocksource using rating property (350).

arm_global_timer enabled on rk3188 provide unstable clocksource.
It's rate depend on ARM CPU rate. As result the command 'sleep 60'
could run either ~60s (with CPU freq 312 MHZ) or ~45s (with CPU
freq 1.6GHz).

In order to use the patch you have to setup the timer using
'rockchip,clocksource' device tree property. timer6 was used as
clocksource in kernel 3.0 from rockchip SDK.

    cpufreq-set -f 1.6GHZ
    date; sleep 60; date

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   79 +++++++++++++++++++++++++++-------
 1 file changed, 63 insertions(+), 16 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 61787c5..77bea97 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -11,6 +11,7 @@
 #include <linux/clockchips.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/sched_clock.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -42,7 +43,13 @@ struct rk_clock_event_device {
 	struct rk_timer timer;
 };
 
+struct rk_clocksource {
+	struct clocksource cs;
+	struct rk_timer timer;
+};
+
 static struct rk_clock_event_device bc_timer;
+static struct rk_clocksource cs_timer;
 
 static inline struct rk_clock_event_device*
 rk_clock_event_device(struct clock_event_device *ce)
@@ -139,14 +146,35 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static cycle_t rk_timer_clocksource_read(struct clocksource *cs)
+{
+	struct rk_clocksource *_cs = container_of(cs, struct rk_clocksource, cs);
+	return ~rk_timer_counter_read(&_cs->timer);
+}
+
+static u64 notrace rk_timer_sched_clock_read(void)
+{
+	struct rk_clocksource *_cs = &cs_timer;
+	return ~rk_timer_counter_read(&_cs->timer);
+}
+
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
-	struct clock_event_device *ce = &bc_timer.ce;
-	struct rk_timer *timer = &bc_timer.timer;
+	struct clock_event_device *ce = NULL;
+	struct clocksource *cs = NULL;
+	struct rk_timer *timer = NULL;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
+	if (of_property_read_bool(np, "rockchip,clocksource")) {
+		cs = &cs_timer.cs;
+		timer = &cs_timer.timer;
+	} else {
+		ce = &bc_timer.ce;
+		timer = &bc_timer.timer;
+	}
+
 	timer->base = of_iomap(np, 0);
 	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
@@ -189,26 +217,45 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	ce->name = TIMER_NAME;
-	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
-		       CLOCK_EVT_FEAT_DYNIRQ;
-	ce->set_next_event = rk_timer_set_next_event;
-	ce->set_state_shutdown = rk_timer_shutdown;
-	ce->set_state_periodic = rk_timer_set_periodic;
-	ce->irq = irq;
-	ce->cpumask = cpu_possible_mask;
-	ce->rating = 250;
+	if (ce) {
+		ce->name = TIMER_NAME;
+		ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
+			       CLOCK_EVT_FEAT_DYNIRQ;
+		ce->set_next_event = rk_timer_set_next_event;
+		ce->set_state_shutdown = rk_timer_shutdown;
+		ce->set_state_periodic = rk_timer_set_periodic;
+		ce->irq = irq;
+		ce->cpumask = cpu_possible_mask;
+		ce->rating = 250;
+	}
+
+	if (cs) {
+		cs->name = TIMER_NAME;
+		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = rk_timer_clocksource_read;
+		cs->rating = 350;
+	}
 
 	rk_timer_interrupt_clear(timer);
 	rk_timer_disable(timer);
 
-	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
-	if (ret) {
-		pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
-		goto out_irq;
+	if (ce) {
+		ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
+		if (ret) {
+			pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
+			goto out_irq;
+		}
+
+		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 	}
 
-	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
+	if (cs) {
+		rk_timer_update_counter(U64_MAX, timer);
+		rk_timer_enable(timer, 0);
+		clocksource_register_hz(cs, timer->freq);
+		sched_clock_register(rk_timer_sched_clock_read, 64, timer->freq);
+	}
 
 	return 0;
 
-- 
1.7.9.5

[PATCH 8/9] dt-bindings: add rockchip,clocksource property to rk-timer

[Alexander Kochetkov]

---
 .../bindings/timer/rockchip,rk-timer.txt           |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
index a41b184..ed5392a 100644
--- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
@@ -9,6 +9,7 @@ Required properties:
 - clocks : must contain an entry for each entry in clock-names
 - clock-names : must include the following entries:
   "timer", "pclk"
+- rockchip,clocksource: setup the timer as clocksource
 
 Example:
 	timer: timer@ff810000 {
-- 
1.7.9.5

[PATCH 9/9] ARM: dts: rockchip: add timer entries to rk3188.dtsi

[Alexander Kochetkov]

This is correct configuration borrowed from 3.0 kernel[1].
timer 6 used as clocksource, timers 0, 1, 4 and 5 used
as clockevents.

Timers can do interrupts and work as expected with correct
driver support.

[1] https://github.com/radxa/linux-rockchip/blob/radxa-stable-3.0/arch/arm/mach-rk3188/rk_timer.c

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 arch/arm/boot/dts/rk3188.dtsi |   45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index 31f81b2..e2f88c8 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -106,6 +106,51 @@
 		};
 	};
 
+	timer0: timer@20038000 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0x20038000 0x20>;
+		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER0>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+		status = "disabled";
+	};
+
+	timer1: timer@20038020 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0x20038020 0x20>;
+		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER1>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+		status = "disabled";
+	};
+
+	timer4: timer@20038060 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0x20038060 0x20>;
+		interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER4>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+		status = "disabled";
+	};
+
+	timer5: timer@20038080 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0x20038080 0x20>;
+		interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER5>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+		status = "disabled";
+	};
+
+	timer6: timer@200380A0 {
+		compatible = "rockchip,rk3288-timer";
+		reg = <0x200380A0 0x20>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+		status = "disabled";
+	};
+
 	i2s0: i2s@1011a000 {
 		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
 		reg = <0x1011a000 0x2000>;
-- 
1.7.9.5

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

> In order to use the patch you have to setup the timer using
> 'rockchip,clocksource' device tree property

Just came in mind, that it is better to replace 'rockchip,clocksource' device tree property
with KConfig option in order to enable clocksource on dedicated timer?

Someting like:
[ ] enable clocksource
   clocksource timer name:

Any feedback would be appreciated.

Alexander.

Re: [PATCH 1/9] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stübner]

Hi Alexander,

I haven't looked to deep into your patches yet, but what is missing is the 
general goal of your whole series.

git format-patch has this nice "--cover-letter" option that creates obviously 
a cover-letter where you can describe what your series wants to achieve.

For those reading along, I guess what you want to achieve should be what I 
describe below, so of course no need to resend just for this :-)

-----
The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts the 
cpu frequency making this timer unsuitable as a stable clocksource.

The rk3188, rk3288 and following socs share a separate timer block already 
handled by the rockchip-timer driver. Therefore adapt this driver to also be 
able to act as clocksource on rk3188.
-----

Right?


Heiko


Am Mittwoch, 23. November 2016, 20:29:29 schrieb Alexander Kochetkov:
> Move ce field out of struct bc_timer into struct rk_clock_event_device,
> rename struct bc_timer to struct rk_timer.
>
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>

Re: [PATCH 1/9] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

Hello Heiko,

> I haven't looked to deep into your patches yet, but what is missing is the 
> general goal of your whole series.


I will be very grateful to receive feedback

> 24 нояб. 2016 г., в 15:01, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> git format-patch has this nice "--cover-letter" option that creates obviously 
> a cover-letter where you can describe what your series wants to achieve.
> 
I’ve used --cover-letter option but forget to add message text. Just subject.
So all patches was sent without cover letter. I’ll be careful next time.

> For those reading along, I guess what you want to achieve should be what I 
> describe below, so of course no need to resend just for this :-)
> 
> -----
> The clock supplying the arm-global-timer on the rk3188 is coming from the
> the cpu clock itself and thus changes its rate everytime cpufreq adjusts the 
> cpu frequency making this timer unsuitable as a stable clocksource.
> 
> The rk3188, rk3288 and following socs share a separate timer block already 
> handled by the rockchip-timer driver. Therefore adapt this driver to also be 
> able to act as clocksource on rk3188.
> -----
> 
> Right?

Yes, exactly as you wrote.

Regards,
Alexander.

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Heiko Stübner]

Am Donnerstag, 24. November 2016, 12:36:20 schrieb Alexander Kochetkov:
> > In order to use the patch you have to setup the timer using
> > 'rockchip,clocksource' device tree property
> 
> Just came in mind, that it is better to replace 'rockchip,clocksource'
> device tree property with KConfig option in order to enable clocksource on
> dedicated timer?
> 
> Someting like:
> [ ] enable clocksource
>    clocksource timer name:

That would mean recompiling the kernel for a maybe board-specific setting and 
is definitly not how things are handled these days :-) .
I.e. the overall goal is to have one kernel image that can actually run on 
multiple arm architectures (rockchip, imx, etc) and only gets configured by the 
devicetree.

In your dts-patch you reuse the rk3288-timer compatible value, which is also 
non-ideal.

What you may want to do is introduce a rockchip,rk3188-timer compatible and 
then make the timer-driver act accordingly, as you then know you are on a 
rk3188-board ... see drivers attaching specific structs to the of_device_id 
entries. From the documentation it also shouldn't really matter which timer 
you use as clocksource, as on the rk3188 it seems all of them act the same way 
(except timer3 being always on).

When touching devicetree-properties, please also adapt the binding document
	Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt
in this case and also include the devicetree maintainers.


Heiko

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

Hello Heiko!

> 24 нояб. 2016 г., в 15:17, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> In your dts-patch you reuse the rk3288-timer compatible value, which is also 
> non-ideal.

rockchip,rk-timer.txt states what I should use rockchip,rk3288-timer for rk3188 timers:

Required properties:
- compatible: shall be one of:
  "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368

Should I add "rockchip,rk3188-timer» (or better "rockchip,rk3036-timer») ? Or may be second
approach should be used?

> What you may want to do is introduce a rockchip,rk3188-timer compatible and 
> then make the timer-driver act accordingly, as you then know you are on a 
> rk3188-board ... see drivers attaching specific structs to the of_device_id 
> entries. From the documentation

May be it is better to prepend compatible string with "rockchip,rk3188-timer» in the dts
file only? elinux[1] recommend build compatible string from "exact device» string and
«other devices» that the device is compatible with.

As «other devices» string defined already, I use it.

[1] http://elinux.org/Device_Tree_Usage#Understanding_the_compatible_Property

	timer0: timer@20038000 {
		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer»;
                …
	};

Just found what rk3036 already declared such way:

rk3036.dtsi:	timer: timer@20044000 {
rk3036.dtsi:		compatible = "rockchip,rk3036-timer", "rockchip,rk3288-timer»;

> 
> it also shouldn't really matter which timer 
> you use as clocksource, as on the rk3188 it seems all of them act the same way 
> (except timer3 being always on).

You are right.

I sent dts file with general timer settings, without clockevent enabled, so one could pick
up timer and setup it in the board dts (radxarock, for example) like:

&timer0 {
    rockchip,clocksource;
    status = «okay»;
};

> When touching devicetree-properties, please also adapt the binding document
> 	Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt
> in this case and also include the devicetree maintainers.

If the patch[2] ok, I'll resend it and include devicetree maintainers.

[2] http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013148.html

Regards,
Alexander.

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Heiko Stübner]

Hi Alexander,

Am Donnerstag, 24. November 2016, 16:05:55 schrieb Alexander Kochetkov:
> Hello Heiko!
> 
> > 24 нояб. 2016 г., в 15:17, Heiko Stübner <heiko@sntech.de> написал(а):
> > 
> > In your dts-patch you reuse the rk3288-timer compatible value, which is
> > also non-ideal.
> 
> rockchip,rk-timer.txt states what I should use rockchip,rk3288-timer for
> rk3188 timers:
> 
> Required properties:
> - compatible: shall be one of:
>   "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288,
> rk3368
> 
> Should I add "rockchip,rk3188-timer» (or better "rockchip,rk3036-timer») ?
> Or may be second approach should be used?
>
> > What you may want to do is introduce a rockchip,rk3188-timer compatible
> > and
> > then make the timer-driver act accordingly, as you then know you are on a
> > rk3188-board ... see drivers attaching specific structs to the
> > of_device_id
> > entries. From the documentation
> 
> May be it is better to prepend compatible string with
> "rockchip,rk3188-timer» in the dts file only? elinux[1] recommend build
> compatible string from "exact device» string and «other devices» that the
> device is compatible with.
> 
> As «other devices» string defined already, I use it.
> 
> [1]
> http://elinux.org/Device_Tree_Usage#Understanding_the_compatible_Property
> 
> 	timer0: timer@20038000 {
> 		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer»;
>                 …
> 	};
> 
> Just found what rk3036 already declared such way:
> 
> rk3036.dtsi:	timer: timer@20044000 {
> rk3036.dtsi:		compatible = "rockchip,rk3036-timer", "rockchip,rk3288-
timer»;

correct, use both but also update the binding, like 
mmc/rockchip-dw-mshc.txt does.


> > it also shouldn't really matter which timer
> > you use as clocksource, as on the rk3188 it seems all of them act the same
> > way (except timer3 being always on).
> 
> You are right.
> 
> I sent dts file with general timer settings, without clockevent enabled, so
> one could pick up timer and setup it in the board dts (radxarock, for
> example) like:
> 
> &timer0 {
>     rockchip,clocksource;
>     status = «okay»;
> };

what I actually meant was that the driver could also recognize the rk3188-
timer compatible as "we need a clocksource" and it shouldn't matter which 
timer actually gets used for this.


> > When touching devicetree-properties, please also adapt the binding
> > document
> > 
> > 	Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt
> > 
> > in this case and also include the devicetree maintainers.
> 
> If the patch[2] ok, I'll resend it and include devicetree maintainers.
> 
> [2]
> http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013148.ht
> ml

Only devicetree people can really tell you if that is ok :-) .

The devicetree is supposed to be a hardware-description and implementation-
independent, so rockchip,clocksource sounds pretty much like linux-specific 
configuration things leaking into the devicetree.


Heiko

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

> 24 нояб. 2016 г., в 16:21, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> what I actually meant was that the driver could also recognize the rk3188-
> timer compatible as "we need a clocksource" and it shouldn't matter which 
> timer actually gets used for this.

One rockchip timer cannot be used as clockevent and clocksource at the same time.

In case of clockevent we want interrupts from it at specified times. So we load one
value into timer counter and it generates an interrupt.

In case of clocksource we load max value into timer counter, run timer and read current
value on demand.

rockchip_timer driver currently implement clockevent. So, if I create only one timer
in the device tree, it should be clockevent timer. As that behavior already expected
from driver by people used it.

I may suggest such solution here: if I want clocksource, I have to declare two timer
in device tree. First probed timer would be clockevent and second one would be
clocksource. All other timers will be ignored. Is that solution good?

If I want one timer and want it be clocksource not clockevent how that situation should
be configured? Device tree not good for this. Kconfig not good. Pass that configuration
on kernel command line?

> Only devicetree people can really tell you if that is ok :-) .
> 
> The devicetree is supposed to be a hardware-description and implementation-
> independent, so rockchip,clocksource sounds pretty much like linux-specific 
> configuration things leaking into the devicetree.


You are right. They don’t allow pass linux configuration using device tree. 

Regards,
Alexander.

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Heiko Stübner]

Am Donnerstag, 24. November 2016, 17:14:56 schrieb Alexander Kochetkov:
> > 24 нояб. 2016 г., в 16:21, Heiko Stübner <heiko@sntech.de> написал(а):
> > 
> > what I actually meant was that the driver could also recognize the rk3188-
> > timer compatible as "we need a clocksource" and it shouldn't matter which
> > timer actually gets used for this.
> 
> One rockchip timer cannot be used as clockevent and clocksource at the same
> time.
> 
> In case of clockevent we want interrupts from it at specified times. So we
> load one value into timer counter and it generates an interrupt.
> 
> In case of clocksource we load max value into timer counter, run timer and
> read current value on demand.
> 
> rockchip_timer driver currently implement clockevent. So, if I create only
> one timer in the device tree, it should be clockevent timer. As that
> behavior already expected from driver by people used it.
> 
> I may suggest such solution here: if I want clocksource, I have to declare
> two timer in device tree. First probed timer would be clockevent and second
> one would be clocksource. All other timers will be ignored. Is that
> solution good?

yep, sounds good, especially as with your patch 9/9 you already declare these 
necessary timers.

> If I want one timer and want it be clocksource not clockevent how that
> situation should be configured? Device tree not good for this. Kconfig not
> good. Pass that configuration on kernel command line?

simply ignore that case :-)

I.e. newer kernels are supposed to be able to run old devicetrees and in that 
case they will have the global-timer as (slightly unstable) clocksource.

Also on the cortex-a9 we also still have the smp-twd as clockevent device.

Heiko

Re: [PATCH 7/9] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

Hello Heiko!

> 24 нояб. 2016 г., в 15:17, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> When touching devicetree-properties, please also adapt the binding document
> 	Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt
> in this case and also include the devicetree maintainers.

Could you please clarify, i should send whole patch series and include devicetree maintainers
for whole patch series or i should send devicetree patches separately?


> 24 нояб. 2016 г., в 16:21, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> correct, use both but also update the binding, like 
> mmc/rockchip-dw-mshc.txt does.

Here devicetree patch for this:
http://www.spinics.net/lists/devicetree/msg152246.html


> 24 нояб. 2016 г., в 17:32, Heiko Stübner <heiko@sntech.de> написал(а):
> 
>> I may suggest such solution here: if I want clocksource, I have to declare
>> two timer in device tree. First probed timer would be clockevent and second
>> one would be clocksource. All other timers will be ignored. Is that
>> solution good?
> 
> yep, sounds good, especially as with your patch 9/9 you already declare these 
> necessary timers.
> 
>> If I want one timer and want it be clocksource not clockevent how that
>> situation should be configured? Device tree not good for this. Kconfig not
>> good. Pass that configuration on kernel command line?
> 
> simply ignore that case :-)


Here devicetree patch for this:
http://www.spinics.net/lists/devicetree/msg152247.html

And I’ll going to resend new patch series with discussed modifications.
It will contain only rk_timer and dts modifications.
I have to do more tests.

Regards,
Alexander.