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 <[email protected]>
---
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
Signed-off-by: Alexander Kochetkov <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This allow to enable timer without enabling interrupts from it.
As that mode will be used in clocksource implementation.
Signed-off-by: Alexander Kochetkov <[email protected]>
---
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
---
.../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
Signed-off-by: Alexander Kochetkov <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Alexander Kochetkov <[email protected]>
---
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
> 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.
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 <[email protected]>
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 <[email protected]> написал(а):
>
> 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.
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
Hello Heiko!
> 24 нояб. 2016 г., в 15:17, Heiko Stübner <[email protected]> написал(а):
>
> 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.
Hi Alexander,
Am Donnerstag, 24. November 2016, 16:05:55 schrieb Alexander Kochetkov:
> Hello Heiko!
>
> > 24 нояб. 2016 г., в 15:17, Heiko Stübner <[email protected]> написал(а):
> >
> > 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
> 24 нояб. 2016 г., в 16:21, Heiko Stübner <[email protected]> написал(а):
>
> 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.
Am Donnerstag, 24. November 2016, 17:14:56 schrieb Alexander Kochetkov:
> > 24 нояб. 2016 г., в 16:21, Heiko Stübner <[email protected]> написал(а):
> >
> > 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
Hello Heiko!
> 24 нояб. 2016 г., в 15:17, Heiko Stübner <[email protected]> написал(а):
>
> 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 <[email protected]> написал(а):
>
> 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 <[email protected]> написал(а):
>
>> 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.