Hello, Daniel, Heiko.
Here is try 7 :) Could you please take a look into it?
rockchip_timer init code implemented using CLOCKEVENT_OF_DECLARE()
introduced in commit 0c8893c9095d ("clockevents: Add a clkevt-of
mechanism like clksrc-of")
There is change in the arch/arm/mach-rockchip/rockchip.c.
This series should be applied after the commit:
https://lkml.org/lkml/2017/3/22/426
As without the commit, you will get linker error ("clkevt-probe.c:63:
undefined reference to `__clkevt_of_table’")
Regards,
Alexander.
Alexander Kochetkov (6):
dt-bindings: clarify compatible property for rockchip timers
ARM: dts: rockchip: update compatible property for rk322x timer
ARM: dts: rockchip: add clockevent attribute to rockchip timers
clocksource/drivers/rockchip_timer: implement clocksource timer
ARM: dts: rockchip: add timer entries to rk3188 SoC
ARM: dts: rockchip: disable arm-global-timer for rk3188
Daniel Lezcano (1):
clocksource/drivers/clksrc-evt-probe: Describe with the DT both the
clocksource and the clockevent
.../bindings/timer/rockchip,rk-timer.txt | 12 +-
Documentation/devicetree/bindings/timer/timer.txt | 38 ++++
arch/arm/boot/dts/rk3036.dtsi | 1 +
arch/arm/boot/dts/rk3188.dtsi | 19 ++
arch/arm/boot/dts/rk322x.dtsi | 3 +-
arch/arm/boot/dts/rk3288.dtsi | 1 +
arch/arm/mach-rockchip/rockchip.c | 2 +
arch/arm64/boot/dts/rockchip/rk3368.dtsi | 1 +
drivers/clocksource/Kconfig | 2 +
drivers/clocksource/clkevt-probe.c | 7 +
drivers/clocksource/clksrc-probe.c | 7 +
drivers/clocksource/rockchip_timer.c | 215 ++++++++++++++------
12 files changed, 241 insertions(+), 67 deletions(-)
create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
--
1.7.9.5
From: Daniel Lezcano <[email protected]>
The CLOCKSOURCE_OF_DECLARE() was introduced before the
CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
clockevent and the clocksource are both initialized in the same init
routine.
With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
now split the clocksource and the clockevent init code. However, the
device tree may specify a single node, so the same node will be passed
to the clockevent/clocksource's init function, with the same base
address.
with this patch it is possible to specify an attribute to the timer's node to
specify if it is a clocksource or a clockevent and define two timers node.
For example:
timer: timer@98400000 {
compatible = "moxa,moxart-timer";
reg = <0x98400000 0x42>;
interrupts = <19 1>;
clocks = <&coreclk>;
clockevent;
};
timer: timer@98400010 {
compatible = "moxa,moxart-timer";
reg = <0x98400010 0x42>;
clocks = <&coreclk>;
clocksource;
};
With this approach, we allow a mechanism to clearly define a clocksource or a
clockevent without aerobatics we can find around in some drivers:
timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
Signed-off-by: Daniel Lezcano <[email protected]>
Signed-off-by: Alexander Kochetkov <[email protected]>
---
Documentation/devicetree/bindings/timer/timer.txt | 38 +++++++++++++++++++++
drivers/clocksource/clkevt-probe.c | 7 ++++
drivers/clocksource/clksrc-probe.c | 7 ++++
3 files changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
diff --git a/Documentation/devicetree/bindings/timer/timer.txt b/Documentation/devicetree/bindings/timer/timer.txt
new file mode 100644
index 0000000..f1ee0cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/timer.txt
@@ -0,0 +1,38 @@
+
+Specifying timer information for devices
+========================================
+
+The timer can be declared via the macro:
+
+CLOCKSOURCE_OF_DECLARE(name, init) if it is a clocksource
+CLOCKEVENT_OF_DECLARE(name, init) if it is a clockevent
+
+The CLOCKSOURCE_OF_DECLARE() was introduced before the
+CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
+clockevent and the clocksource are both initialized in the same init
+routine.
+
+With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
+now split the clocksource and the clockevent init code. However, the
+device tree may specify a single node, so the same node will be passed
+to the clockevent/clocksource's init function, with the same base
+address. It is possible to specify an attribute to the timer's node to
+specify if it is a clocksource or a clockevent and define two timers
+node.
+
+Example:
+
+ timer: timer@98400000 {
+ compatible = "moxa,moxart-timer";
+ reg = <0x98400000 0x42>;
+ interrupts = <19 1>;
+ clocks = <&coreclk>;
+ clockevent;
+ };
+
+ timer: timer@98400010 {
+ compatible = "moxa,moxart-timer";
+ reg = <0x98400010 0x42>;
+ clocks = <&coreclk>;
+ clocksource;
+ };
diff --git a/drivers/clocksource/clkevt-probe.c b/drivers/clocksource/clkevt-probe.c
index eb89b50..fa02ac1 100644
--- a/drivers/clocksource/clkevt-probe.c
+++ b/drivers/clocksource/clkevt-probe.c
@@ -37,6 +37,13 @@ int __init clockevent_probe(void)
init_func = match->data;
+ /*
+ * The device node describes a clocksource, ignore it
+ * as we are in the clockevent init routine.
+ */
+ if (of_property_read_bool(np, "clocksource"))
+ continue;
+
ret = init_func(np);
if (ret) {
pr_warn("Failed to initialize '%s' (%d)\n",
diff --git a/drivers/clocksource/clksrc-probe.c b/drivers/clocksource/clksrc-probe.c
index bc62be9..ce50f33 100644
--- a/drivers/clocksource/clksrc-probe.c
+++ b/drivers/clocksource/clksrc-probe.c
@@ -38,6 +38,13 @@ void __init clocksource_probe(void)
init_func_ret = match->data;
+ /*
+ * The device node describes a clockevent, ignore it
+ * as we are in the clocksource init routine.
+ */
+ if (of_property_read_bool(np, "clockevent"))
+ continue;
+
ret = init_func_ret(np);
if (ret) {
pr_err("Failed to initialize '%s': %d",
--
1.7.9.5
All rockchip timers present in the DT act as clockevent.
It is possible to specify timer role using DT attribute.
Mark them accordingly.
Also this patch specify that for timer should be called init
function specified with CLOCKEVENT_OF_DECLARE().
Without the commit boot warnings will appear because clock
framework will try initialize them as clocksource.
Signed-off-by: Alexander Kochetkov <[email protected]>
---
arch/arm/boot/dts/rk3036.dtsi | 1 +
arch/arm/boot/dts/rk322x.dtsi | 1 +
arch/arm/boot/dts/rk3288.dtsi | 1 +
arch/arm64/boot/dts/rockchip/rk3368.dtsi | 1 +
4 files changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/rk3036.dtsi b/arch/arm/boot/dts/rk3036.dtsi
index 843d2be..a04bb5a 100644
--- a/arch/arm/boot/dts/rk3036.dtsi
+++ b/arch/arm/boot/dts/rk3036.dtsi
@@ -353,6 +353,7 @@
interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&xin24m>, <&cru PCLK_TIMER>;
clock-names = "timer", "pclk";
+ clockevent;
};
pwm0: pwm@20050000 {
diff --git a/arch/arm/boot/dts/rk322x.dtsi b/arch/arm/boot/dts/rk322x.dtsi
index 2a4eee2..2250640 100644
--- a/arch/arm/boot/dts/rk322x.dtsi
+++ b/arch/arm/boot/dts/rk322x.dtsi
@@ -328,6 +328,7 @@
interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&xin24m>, <&cru PCLK_TIMER>;
clock-names = "timer", "pclk";
+ clockevent;
};
cru: clock-controller@110e0000 {
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 91c4b3c..f45b7ad 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -217,6 +217,7 @@
interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&xin24m>, <&cru PCLK_TIMER>;
clock-names = "timer", "pclk";
+ clockevent;
};
display-subsystem {
diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index 4f44d11..054dadd 100644
--- a/arch/arm64/boot/dts/rockchip/rk3368.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
@@ -673,6 +673,7 @@
compatible = "rockchip,rk3368-timer", "rockchip,rk3288-timer";
reg = <0x0 0xff810000 0x0 0x20>;
interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
+ clockevent;
};
gic: interrupt-controller@ffb71000 {
--
1.7.9.5
The patch add two timers to all rk3188 based boards.
The first timer is from alive subsystem and it act as a backup
for the local timers at sleep time. It act the same as other
SoC rockchip timers already present in kernel.
The second timer is from CPU subsystem and act as replacement
for the arm-global-timer clocksource and sched clock. It run
at stable frequency 24MHz.
Signed-off-by: Alexander Kochetkov <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
---
arch/arm/boot/dts/rk3188.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index b3e6a30..a20d501 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -106,6 +106,24 @@
};
};
+ timer3: timer@2000e000 {
+ compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+ reg = <0x2000e000 0x20>;
+ interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
+ clock-names = "timer", "pclk";
+ clockevent;
+ };
+
+ timer6: timer@200380a0 {
+ compatible = "rockchip,rk3188-timer", "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";
+ clocksource;
+ };
+
i2s0: i2s@1011a000 {
compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
reg = <0x1011a000 0x2000>;
--
1.7.9.5
Property set to '"rockchip,rk3228-timer", "rockchip,rk3288-timer"'
to match devicetree bindings.
Signed-off-by: Alexander Kochetkov <[email protected]>
Suggested-by: Heiko Stübner <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
---
arch/arm/boot/dts/rk322x.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/rk322x.dtsi b/arch/arm/boot/dts/rk322x.dtsi
index 9e6bf0e..2a4eee2 100644
--- a/arch/arm/boot/dts/rk322x.dtsi
+++ b/arch/arm/boot/dts/rk322x.dtsi
@@ -323,7 +323,7 @@
};
timer: timer@110c0000 {
- compatible = "rockchip,rk3288-timer";
+ compatible = "rockchip,rk3228-timer", "rockchip,rk3288-timer";
reg = <0x110c0000 0x20>;
interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&xin24m>, <&cru PCLK_TIMER>;
--
1.7.9.5
Make all properties description in form '"rockchip,<chip>-timer",
"rockchip,rk3288-timer"' for all chips found in linux kernel.
Suggested-by: Heiko Stübner <[email protected]>
Signed-off-by: Alexander Kochetkov <[email protected]>
Acked-by: Rob Herring <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
---
.../bindings/timer/rockchip,rk-timer.txt | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
index a41b184..16a5f45 100644
--- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
@@ -1,9 +1,15 @@
Rockchip rk timer
Required properties:
-- compatible: shall be one of:
- "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368
- "rockchip,rk3399-timer" - for rk3399
+- compatible: should be:
+ "rockchip,rk3036-timer", "rockchip,rk3288-timer": for Rockchip RK3036
+ "rockchip,rk3066-timer", "rockchip,rk3288-timer": for Rockchip RK3066
+ "rockchip,rk3188-timer", "rockchip,rk3288-timer": for Rockchip RK3188
+ "rockchip,rk3228-timer", "rockchip,rk3288-timer": for Rockchip RK3228
+ "rockchip,rk3229-timer", "rockchip,rk3288-timer": for Rockchip RK3229
+ "rockchip,rk3288-timer": for Rockchip RK3288
+ "rockchip,rk3368-timer", "rockchip,rk3288-timer": for Rockchip RK3368
+ "rockchip,rk3399-timer": for Rockchip RK3399
- reg: base address of the timer register starting with TIMERS CONTROL register
- interrupts: should contain the interrupts for Timer0
- clocks : must contain an entry for each entry in clock-names
--
1.7.9.5
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
and sched clock.
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 and sched clock on rk3188.
In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.
cpufreq-set -f 1.6GHZ
date; sleep 60; date
In order to use the patch you need to declare two timers in the dts
file. The first timer will be initialized as clockevent provider
and the second one as clocksource. The clockevent must be from
alive subsystem as it used as backup for the local timers at sleep
time.
The patch does not break compatibility with older device tree files.
The older device tree files contain only one timer. The timer
will be initialized as clockevent, as expected. But new warnings like
'Failed to initialize /timer@....' will appear with old device tree
files. To resolve them 'clockevent' attribute should be added to the
timer.
rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be useful
for Cortex-A9/A5 based parts.
Signed-off-by: Alexander Kochetkov <[email protected]>
---
arch/arm/mach-rockchip/rockchip.c | 2 +
drivers/clocksource/Kconfig | 2 +
drivers/clocksource/rockchip_timer.c | 215 ++++++++++++++++++++++++----------
3 files changed, 156 insertions(+), 63 deletions(-)
diff --git a/arch/arm/mach-rockchip/rockchip.c b/arch/arm/mach-rockchip/rockchip.c
index a7ab9ec..5184f7d 100644
--- a/arch/arm/mach-rockchip/rockchip.c
+++ b/arch/arm/mach-rockchip/rockchip.c
@@ -21,6 +21,7 @@
#include <linux/irqchip.h>
#include <linux/clk-provider.h>
#include <linux/clocksource.h>
+#include <linux/clockchips.h>
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
#include <asm/mach/arch.h>
@@ -67,6 +68,7 @@ static void __init rockchip_timer_init(void)
}
of_clk_init(NULL);
+ clockevent_probe();
clocksource_probe();
}
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 21f84ea..5e0df76 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -71,6 +71,8 @@ config ROCKCHIP_TIMER
bool "Rockchip timer driver" if COMPILE_TEST
depends on ARM || ARM64
select CLKSRC_OF
+ select CLKEVT_OF
+ select CLKSRC_MMIO
help
Enables the support for the rockchip timer driver.
diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..0ac71bb 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -11,6 +11,8 @@
#include <linux/clockchips.h>
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
@@ -19,6 +21,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
@@ -29,103 +33,118 @@
#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;
+ struct clk *clk;
+ struct clk *pclk;
u32 freq;
+ int irq;
};
-static struct bc_timer bc_timer;
+struct rk_clkevt {
+ struct clock_event_device ce;
+ struct rk_timer timer;
+};
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
-{
- return container_of(ce, struct bc_timer, ce);
-}
+/* global instance for rk_timer_sched_read() */
+static struct rk_timer *rk_clksrc;
-static inline void __iomem *rk_base(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
{
- return rk_timer(ce)->base;
+ return &container_of(ce, struct rk_clkevt, ce)->timer;
}
-static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+static inline void rk_timer_disable(struct rk_timer *timer)
{
- return rk_timer(ce)->ctrl;
+ writel_relaxed(TIMER_DISABLE, timer->ctrl);
}
-static inline void rk_timer_disable(struct clock_event_device *ce)
+static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
{
- writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
-}
-
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
-{
- writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
- rk_ctrl(ce));
+ writel_relaxed(TIMER_ENABLE | flags, 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 |
+ TIMER_INT_UNMASK);
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 | TIMER_INT_UNMASK);
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);
return IRQ_HANDLED;
}
-static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
+static u64 notrace rk_timer_sched_read(void)
+{
+ return ~readl_relaxed(rk_clksrc->base + TIMER_CURRENT_VALUE0);
+}
+
+static int __init
+rk_timer_probe(struct rk_timer *timer, struct device_node *np)
{
- struct clock_event_device *ce = &bc_timer.ce;
struct clk *timer_clk;
struct clk *pclk;
int ret = -EINVAL, irq;
+ u32 ctrl_reg = TIMER_CONTROL_REG3288;
- 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;
+
+ if (of_device_is_compatible(np, "rockchip,rk3399-timer"))
+ ctrl_reg = TIMER_CONTROL_REG3399;
+
+ timer->ctrl = timer->base + ctrl_reg;
pclk = of_clk_get_by_name(np, "pclk");
if (IS_ERR(pclk)) {
@@ -139,6 +158,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
pr_err("Failed to enable pclk for '%s'\n", TIMER_NAME);
goto out_unmap;
}
+ timer->pclk = pclk;
timer_clk = of_clk_get_by_name(np, "timer");
if (IS_ERR(timer_clk)) {
@@ -152,8 +172,9 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
pr_err("Failed to enable timer clock\n");
goto out_timer_clk;
}
+ timer->clk = 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) {
@@ -161,51 +182,119 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
pr_err("Failed to map interrupts for '%s'\n", TIMER_NAME);
goto out_irq;
}
+ timer->irq = irq;
+ rk_timer_interrupt_clear(timer);
+ rk_timer_disable(timer);
+ return 0;
+
+out_irq:
+ clk_disable_unprepare(timer_clk);
+out_timer_clk:
+ clk_disable_unprepare(pclk);
+out_unmap:
+ iounmap(timer->base);
+
+ return ret;
+}
+
+static void __init rk_timer_cleanup(struct rk_timer *timer)
+{
+ clk_disable_unprepare(timer->clk);
+ clk_disable_unprepare(timer->pclk);
+ iounmap(timer->base);
+}
+
+static int __init rk_clkevt_init(struct device_node *np)
+{
+ struct rk_clkevt *clkevt;
+ struct clock_event_device *ce;
+ int ret = -EINVAL;
+
+ clkevt = kzalloc(sizeof(struct rk_clkevt), GFP_KERNEL);
+ if (!clkevt)
+ return -ENOMEM;
+
+ ret = rk_timer_probe(&clkevt->timer, np);
+ if (ret)
+ goto out_probe;
+
+ ce = &clkevt->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->irq = clkevt->timer.irq;
ce->cpumask = cpu_possible_mask;
ce->rating = 250;
- rk_timer_interrupt_clear(ce);
- rk_timer_disable(ce);
-
- ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
+ ret = request_irq(clkevt->timer.irq, rk_timer_interrupt, IRQF_TIMER,
+ TIMER_NAME, ce);
if (ret) {
- pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
+ pr_err("Failed to initialize '%s': %d\n",
+ TIMER_NAME, ret);
goto out_irq;
}
- clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
-
+ clockevents_config_and_register(&clkevt->ce,
+ clkevt->timer.freq, 1, UINT_MAX);
return 0;
out_irq:
- clk_disable_unprepare(timer_clk);
-out_timer_clk:
- clk_disable_unprepare(pclk);
-out_unmap:
- iounmap(bc_timer.base);
-
+ rk_timer_cleanup(&clkevt->timer);
+out_probe:
+ kfree(clkevt);
return ret;
}
-static int __init rk3288_timer_init(struct device_node *np)
+static int __init rk_clksrc_init(struct device_node *np)
{
- return rk_timer_init(np, TIMER_CONTROL_REG3288);
-}
+ struct rk_timer *clksrc;
+ int ret = -EINVAL;
+
+ /*
+ * Old DT-files has timer entries without 'clocksource' property
+ * and that timer assumed to be a clockevent, so skip them.
+ */
+ if (!of_property_read_bool(np, "clocksource"))
+ return -EINVAL;
+
+ clksrc = kzalloc(sizeof(struct rk_timer), GFP_KERNEL);
+ if (!clksrc)
+ return -ENOMEM;
+
+ ret = rk_timer_probe(clksrc, np);
+ if (ret)
+ goto out_probe;
+
+ rk_timer_update_counter(UINT_MAX, clksrc);
+ rk_timer_enable(clksrc, 0);
+
+ ret = clocksource_mmio_init(clksrc->base + TIMER_CURRENT_VALUE0,
+ TIMER_NAME, clksrc->freq, 250, 32,
+ clocksource_mmio_readl_down);
+ if (ret) {
+ pr_err("Failed to register clocksource");
+ goto out_clocksource;
+ }
-static int __init rk3399_timer_init(struct device_node *np)
-{
- return rk_timer_init(np, TIMER_CONTROL_REG3399);
+ if (!rk_clksrc) {
+ rk_clksrc = clksrc;
+ sched_clock_register(rk_timer_sched_read, 32, rk_clksrc->freq);
+ }
+ return 0;
+
+out_clocksource:
+ rk_timer_cleanup(clksrc);
+out_probe:
+ kfree(clksrc);
+ return ret;
}
-CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
- rk3288_timer_init);
-CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
- rk3399_timer_init);
+CLOCKEVENT_OF_DECLARE(rk3288_clkevt, "rockchip,rk3288-timer", rk_clkevt_init);
+CLOCKEVENT_OF_DECLARE(rk3399_clkevt, "rockchip,rk3399-timer", rk_clkevt_init);
+
+CLOCKSOURCE_OF_DECLARE(rk3288_clksrc, "rockchip,rk3288-timer", rk_clksrc_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_clksrc, "rockchip,rk3399-timer", rk_clksrc_init);
--
1.7.9.5
The clocksource and the sched_clock provided by the arm_global_timer
are quite unstable because their rates depend on the cpu frequency.
On the other side, the arm_global_timer has a higher rating than the
rockchip_timer, it will be selected by default by the time framework
while we want to use the stable rockchip clocksource.
Let's disable the arm_global_timer in order to have the rockchip
clocksource selected by default.
Signed-off-by: Alexander Kochetkov <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
---
arch/arm/boot/dts/rk3188.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index a20d501..c316468 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -548,6 +548,7 @@
&global_timer {
interrupts = <GIC_PPI 11 0xf04>;
+ status = "disabled";
};
&local_timer {
--
1.7.9.5
Hi Alexander,
[auto build test ERROR on tip/timers/core]
[also build test ERROR on v4.11-rc3 next-20170323]
[cannot apply to rockchip/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alexander-Kochetkov/Implement-clocksource-for-rockchip-SoC-using-rockchip-timer/20170324-113008
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
In file included from include/linux/tick.h:7:0,
from drivers//acpi/processor_idle.c:32:
>> include/linux/clockchips.h:232:2: error: invalid preprocessing directive #els
#els
^~~
>> include/linux/clockchips.h:233:19: error: static declaration of 'clockevent_probe' follows non-static declaration
static inline int clockevent_probe(void) { return 0; }
^~~~~~~~~~~~~~~~
include/linux/clockchips.h:231:12: note: previous declaration of 'clockevent_probe' was here
extern int clockevent_probe(void);
^~~~~~~~~~~~~~~~
--
>> drivers//clocksource/clkevt-probe.c:20:29: fatal error: linux/clockchip.h: No such file or directory
#include <linux/clockchip.h>
^
compilation terminated.
vim +232 include/linux/clockchips.h
d316c57f Thomas Gleixner 2007-02-16 226
376bc271 Daniel Lezcano 2016-04-19 227 #define CLOCKEVENT_OF_DECLARE(name, compat, fn) \
376bc271 Daniel Lezcano 2016-04-19 228 OF_DECLARE_1_RET(clkevt, name, compat, fn)
376bc271 Daniel Lezcano 2016-04-19 229
376bc271 Daniel Lezcano 2016-04-19 230 #ifdef CONFIG_CLKEVT_PROBE
376bc271 Daniel Lezcano 2016-04-19 231 extern int clockevent_probe(void);
376bc271 Daniel Lezcano 2016-04-19 @232 #els
376bc271 Daniel Lezcano 2016-04-19 @233 static inline int clockevent_probe(void) { return 0; }
376bc271 Daniel Lezcano 2016-04-19 234 #endif
376bc271 Daniel Lezcano 2016-04-19 235
9eed56e8 Ingo Molnar 2015-04-02 236 #endif /* _LINUX_CLOCKCHIPS_H */
:::::: The code at line 232 was first introduced by commit
:::::: 376bc27150f180d9f5eddec6a14117780177589d clockevents: Add a clkevt-of mechanism like clksrc-of
:::::: TO: Daniel Lezcano <[email protected]>
:::::: CC: Daniel Lezcano <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
The patch series should be applied after the patches [1] and [2] haven’t merged yet into the kernel.
That mention in the cover letter.
[1] https://lkml.org/lkml/2017/3/22/420
[2] https://lkml.org/lkml/2017/3/22/426
> 24 марта 2017 г., в 11:29, kbuild test robot <[email protected]> написал(а):
>
> Hi Alexander,
>
> [auto build test ERROR on tip/timers/core]
> [also build test ERROR on v4.11-rc3 next-20170323]
> [cannot apply to rockchip/for-next]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Alexander-Kochetkov/Implement-clocksource-for-rockchip-SoC-using-rockchip-timer/20170324-113008
> config: arm64-defconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm64
>
> All errors (new ones prefixed by >>):
>
> In file included from include/linux/tick.h:7:0,
> from drivers//acpi/processor_idle.c:32:
>>> include/linux/clockchips.h:232:2: error: invalid preprocessing directive #els
> #els
> ^~~
>>> include/linux/clockchips.h:233:19: error: static declaration of 'clockevent_probe' follows non-static declaration
> static inline int clockevent_probe(void) { return 0; }
> ^~~~~~~~~~~~~~~~
> include/linux/clockchips.h:231:12: note: previous declaration of 'clockevent_probe' was here
> extern int clockevent_probe(void);
> ^~~~~~~~~~~~~~~~
> --
>>> drivers//clocksource/clkevt-probe.c:20:29: fatal error: linux/clockchip.h: No such file or directory
> #include <linux/clockchip.h>
> ^
> compilation terminated.
>
> vim +232 include/linux/clockchips.h
>
> d316c57f Thomas Gleixner 2007-02-16 226
> 376bc271 Daniel Lezcano 2016-04-19 227 #define CLOCKEVENT_OF_DECLARE(name, compat, fn) \
> 376bc271 Daniel Lezcano 2016-04-19 228 OF_DECLARE_1_RET(clkevt, name, compat, fn)
> 376bc271 Daniel Lezcano 2016-04-19 229
> 376bc271 Daniel Lezcano 2016-04-19 230 #ifdef CONFIG_CLKEVT_PROBE
> 376bc271 Daniel Lezcano 2016-04-19 231 extern int clockevent_probe(void);
> 376bc271 Daniel Lezcano 2016-04-19 @232 #els
> 376bc271 Daniel Lezcano 2016-04-19 @233 static inline int clockevent_probe(void) { return 0; }
> 376bc271 Daniel Lezcano 2016-04-19 234 #endif
> 376bc271 Daniel Lezcano 2016-04-19 235
> 9eed56e8 Ingo Molnar 2015-04-02 236 #endif /* _LINUX_CLOCKCHIPS_H */
>
> :::::: The code at line 232 was first introduced by commit
> :::::: 376bc27150f180d9f5eddec6a14117780177589d clockevents: Add a clkevt-of mechanism like clksrc-of
>
> :::::: TO: Daniel Lezcano <[email protected]>
> :::::: CC: Daniel Lezcano <[email protected]>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
> <.config.gz>
On 24/03/2017 09:41, Alexander Kochetkov wrote:
> The patch series should be applied after the patches [1] and [2] haven’t merged yet into the kernel.
> That mention in the cover letter.
>
> [1] https://lkml.org/lkml/2017/3/22/420
> [2] https://lkml.org/lkml/2017/3/22/426
Thanks for the fix Alexander.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> From: Daniel Lezcano <[email protected]>
>
> The CLOCKSOURCE_OF_DECLARE() was introduced before the
> CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> clockevent and the clocksource are both initialized in the same init
> routine.
>
> With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> now split the clocksource and the clockevent init code. However, the
> device tree may specify a single node, so the same node will be passed
> to the clockevent/clocksource's init function, with the same base
> address.
>
> with this patch it is possible to specify an attribute to the timer's node to
> specify if it is a clocksource or a clockevent and define two timers node.
Daniel and I discussed and agreed against this a while back. What
changed?
>
> For example:
>
> timer: timer@98400000 {
> compatible = "moxa,moxart-timer";
> reg = <0x98400000 0x42>;
This overlaps the next node. You can change this to 0x10, but are these
really 2 independent h/w blocks? Don't design the nodes around the
current needs of Linux.
> interrupts = <19 1>;
> clocks = <&coreclk>;
> clockevent;
This is not needed. The presence of "interrupts" is enough to say use
this timer for clockevent.
> };
>
> timer: timer@98400010 {
> compatible = "moxa,moxart-timer";
> reg = <0x98400010 0x42>;
> clocks = <&coreclk>;
> clocksource;
Likewise.
> };
>
> With this approach, we allow a mechanism to clearly define a clocksource or a
> clockevent without aerobatics we can find around in some drivers:
> timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
These all already have bindings and work. What problem are you trying to
solve other than restructuring Linux?
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> Signed-off-by: Alexander Kochetkov <[email protected]>
> ---
> Documentation/devicetree/bindings/timer/timer.txt | 38 +++++++++++++++++++++
> drivers/clocksource/clkevt-probe.c | 7 ++++
> drivers/clocksource/clksrc-probe.c | 7 ++++
> 3 files changed, 52 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
>
> diff --git a/Documentation/devicetree/bindings/timer/timer.txt b/Documentation/devicetree/bindings/timer/timer.txt
> new file mode 100644
> index 0000000..f1ee0cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/timer.txt
> @@ -0,0 +1,38 @@
> +
> +Specifying timer information for devices
> +========================================
> +
> +The timer can be declared via the macro:
> +
> +CLOCKSOURCE_OF_DECLARE(name, init) if it is a clocksource
> +CLOCKEVENT_OF_DECLARE(name, init) if it is a clockevent
> +
> +The CLOCKSOURCE_OF_DECLARE() was introduced before the
> +CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> +clockevent and the clocksource are both initialized in the same init
> +routine.
> +
> +With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> +now split the clocksource and the clockevent init code. However, the
> +device tree may specify a single node, so the same node will be passed
> +to the clockevent/clocksource's init function, with the same base
> +address. It is possible to specify an attribute to the timer's node to
> +specify if it is a clocksource or a clockevent and define two timers
> +node.
This is all Linux details and doesn't belong in binding docs.
> +
> +Example:
> +
> + timer: timer@98400000 {
> + compatible = "moxa,moxart-timer";
> + reg = <0x98400000 0x42>;
> + interrupts = <19 1>;
> + clocks = <&coreclk>;
> + clockevent;
> + };
> +
> + timer: timer@98400010 {
> + compatible = "moxa,moxart-timer";
> + reg = <0x98400010 0x42>;
> + clocks = <&coreclk>;
> + clocksource;
> + };
> diff --git a/drivers/clocksource/clkevt-probe.c b/drivers/clocksource/clkevt-probe.c
> index eb89b50..fa02ac1 100644
> --- a/drivers/clocksource/clkevt-probe.c
> +++ b/drivers/clocksource/clkevt-probe.c
> @@ -37,6 +37,13 @@ int __init clockevent_probe(void)
>
> init_func = match->data;
>
> + /*
> + * The device node describes a clocksource, ignore it
> + * as we are in the clockevent init routine.
> + */
> + if (of_property_read_bool(np, "clocksource"))
> + continue;
> +
> ret = init_func(np);
> if (ret) {
> pr_warn("Failed to initialize '%s' (%d)\n",
> diff --git a/drivers/clocksource/clksrc-probe.c b/drivers/clocksource/clksrc-probe.c
> index bc62be9..ce50f33 100644
> --- a/drivers/clocksource/clksrc-probe.c
> +++ b/drivers/clocksource/clksrc-probe.c
> @@ -38,6 +38,13 @@ void __init clocksource_probe(void)
>
> init_func_ret = match->data;
>
> + /*
> + * The device node describes a clockevent, ignore it
> + * as we are in the clocksource init routine.
> + */
> + if (of_property_read_bool(np, "clockevent"))
> + continue;
> +
> ret = init_func_ret(np);
> if (ret) {
> pr_err("Failed to initialize '%s': %d",
> --
> 1.7.9.5
>
On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote:
> On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> > From: Daniel Lezcano <[email protected]>
> >
> > The CLOCKSOURCE_OF_DECLARE() was introduced before the
> > CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> > clockevent and the clocksource are both initialized in the same init
> > routine.
> >
> > With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> > now split the clocksource and the clockevent init code. However, the
> > device tree may specify a single node, so the same node will be passed
> > to the clockevent/clocksource's init function, with the same base
> > address.
> >
> > with this patch it is possible to specify an attribute to the timer's node to
> > specify if it is a clocksource or a clockevent and define two timers node.
>
> Daniel and I discussed and agreed against this a while back. What
> changed?
Hi Rob,
> > For example:
> >
> > timer: timer@98400000 {
> > compatible = "moxa,moxart-timer";
> > reg = <0x98400000 0x42>;
>
> This overlaps the next node. You can change this to 0x10, but are these
> really 2 independent h/w blocks? Don't design the nodes around the
> current needs of Linux.
Mmh, thanks for raising this. Conceptually speaking there are two (or more)
different entities, the clocksource and the clockevents but they share the same
IP block.
In the driver timer-sp804.c there is a fragile mechanism with the timer
counting assuming the timers are declared in a specific order in the DT
(integratorcp.dts). There are multiple nodes, which are IIUC overlapping like
what you are describing.
I'm a bit puzzled with how this should be done.
Do you have a suggestion ?
> > interrupts = <19 1>;
> > clocks = <&coreclk>;
> > clockevent;
>
> This is not needed. The presence of "interrupts" is enough to say use
> this timer for clockevent.
Yes, that is true if no drivers was already taking CLOCKSOURCE_OF and
initializing the clockevent. The driver will pass through the clocksource_probe
function, check the interrupt and bail out because there is an interrupt
declared and assume it is a clockevent, so no initialization for the driver.
IOW, it is not backward compatible.
We need this attribute somehow in order to separate clearly a clocksource or a
clockevent with a new implementation.
> > };
> >
> > timer: timer@98400010 {
> > compatible = "moxa,moxart-timer";
> > reg = <0x98400010 0x42>;
> > clocks = <&coreclk>;
> > clocksource;
>
> Likewise.
>
> > };
> >
> > With this approach, we allow a mechanism to clearly define a clocksource or a
> > clockevent without aerobatics we can find around in some drivers:
> > timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> > renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
>
> These all already have bindings and work. What problem are you trying to
> solve other than restructuring Linux?
Yes, there is already the bindings, but that force to do some hackish
initialization.
I would like to give the opportunity to declare separately a clocksource and a
clockevent, in order to have full control of how this is initialized.
Hi,
On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote:
> On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote:
> > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> > > From: Daniel Lezcano <[email protected]>
> > >
> > > The CLOCKSOURCE_OF_DECLARE() was introduced before the
> > > CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> > > clockevent and the clocksource are both initialized in the same init
> > > routine.
> > >
> > > With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> > > now split the clocksource and the clockevent init code. However, the
> > > device tree may specify a single node, so the same node will be passed
> > > to the clockevent/clocksource's init function, with the same base
> > > address.
> > >
> > > with this patch it is possible to specify an attribute to the timer's node to
> > > specify if it is a clocksource or a clockevent and define two timers node.
> >
> > Daniel and I discussed and agreed against this a while back. What
> > changed?
>
> Hi Rob,
>
> > > For example:
> > >
> > > timer: timer@98400000 {
> > > compatible = "moxa,moxart-timer";
> > > reg = <0x98400000 0x42>;
> >
> > This overlaps the next node. You can change this to 0x10, but are these
> > really 2 independent h/w blocks? Don't design the nodes around the
> > current needs of Linux.
>
> Mmh, thanks for raising this. Conceptually speaking there are two (or more)
> different entities, the clocksource and the clockevents but they share the same
> IP block.
>From the DT's PoV, this is one entity, which is the IP block.
The clocksource/clockevent concept is a Linux implementation detail. The
DT cannot and should not be aware of that.
[...]
> > > interrupts = <19 1>;
> > > clocks = <&coreclk>;
> > > clockevent;
> >
> > This is not needed. The presence of "interrupts" is enough to say use
> > this timer for clockevent.
>
> Yes, that is true if no drivers was already taking CLOCKSOURCE_OF and
> initializing the clockevent. The driver will pass through the clocksource_probe
> function, check the interrupt and bail out because there is an interrupt
> declared and assume it is a clockevent, so no initialization for the driver.
> IOW, it is not backward compatible.
>
> We need this attribute somehow in order to separate clearly a clocksource or a
> clockevent with a new implementation.
Why? A single IP block can provide the functionality of both (though
there are reasons that functionality may be mutually exclusive). What is
the benefit of this split?
> > > With this approach, we allow a mechanism to clearly define a clocksource or a
> > > clockevent without aerobatics we can find around in some drivers:
> > > timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> > > renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
> >
> > These all already have bindings and work. What problem are you trying to
> > solve other than restructuring Linux?
>
> Yes, there is already the bindings, but that force to do some hackish
> initialization.
Here, you are forcing hackish DT changes that do not truly describe HW.
How is that better?
> I would like to give the opportunity to declare separately a clocksource and a
> clockevent, in order to have full control of how this is initialized.
To me it sounds like what we need is Linux infrastructure that allows
one to register a device as having both clockevent/clocksource
functionality.
That way, we can choose to do something sane at boot time, and if the
user really wants specific devices used in specific ways, they can
request that.
Thanks,
Mark.
On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote:
> Hi,
>
> On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote:
> > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote:
> > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> > > > From: Daniel Lezcano <[email protected]>
> > > >
> > > > The CLOCKSOURCE_OF_DECLARE() was introduced before the
> > > > CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> > > > clockevent and the clocksource are both initialized in the same init
> > > > routine.
> > > >
> > > > With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> > > > now split the clocksource and the clockevent init code. However, the
> > > > device tree may specify a single node, so the same node will be passed
> > > > to the clockevent/clocksource's init function, with the same base
> > > > address.
> > > >
> > > > with this patch it is possible to specify an attribute to the timer's node to
> > > > specify if it is a clocksource or a clockevent and define two timers node.
> > >
> > > Daniel and I discussed and agreed against this a while back. What
> > > changed?
> >
> > Hi Rob,
> >
> > > > For example:
> > > >
> > > > timer: timer@98400000 {
> > > > compatible = "moxa,moxart-timer";
> > > > reg = <0x98400000 0x42>;
> > >
> > > This overlaps the next node. You can change this to 0x10, but are these
> > > really 2 independent h/w blocks? Don't design the nodes around the
> > > current needs of Linux.
> >
> > Mmh, thanks for raising this. Conceptually speaking there are two (or more)
> > different entities, the clocksource and the clockevents but they share the same
> > IP block.
>
> From the DT's PoV, this is one entity, which is the IP block.
>
> The clocksource/clockevent concept is a Linux implementation detail. The
> DT cannot and should not be aware of that.
>
> [...]
>
> > > > interrupts = <19 1>;
> > > > clocks = <&coreclk>;
> > > > clockevent;
> > >
> > > This is not needed. The presence of "interrupts" is enough to say use
> > > this timer for clockevent.
> >
> > Yes, that is true if no drivers was already taking CLOCKSOURCE_OF and
> > initializing the clockevent. The driver will pass through the clocksource_probe
> > function, check the interrupt and bail out because there is an interrupt
> > declared and assume it is a clockevent, so no initialization for the driver.
> > IOW, it is not backward compatible.
> >
> > We need this attribute somehow in order to separate clearly a clocksource or a
> > clockevent with a new implementation.
>
> Why? A single IP block can provide the functionality of both (though
> there are reasons that functionality may be mutually exclusive). What is
> the benefit of this split?
Hi Mark,
You can have several timers on the system and may want to use the clockevents
from one IP block and the clocksource from another IP block. For example, in
the case of a bogus clocksource.
Moreover there are some drivers with a node for a clocksource and
another one for the clockevent, and the driver is assuming the clockevent is
defined first and then the clocksource.
eg.
arch/arc/boot/dts/abilis_tb10x.dtsi:
/* TIMER0 with interrupt for clockevent */
timer0 {
compatible = "snps,arc-timer";
interrupts = <3>;
interrupt-parent = <&intc>;
clocks = <&cpu_clk>;
};
/* TIMER1 for free running clocksource */
timer1 {
compatible = "snps,arc-timer";
clocks = <&cpu_clk>;
};
drivers/clocksource/arc_timer.c:
static int __init arc_of_timer_init(struct device_node *np)
{
static int init_count = 0;
int ret;
if (!init_count) {
init_count = 1;
ret = arc_clockevent_setup(np);
} else {
ret = arc_cs_setup_timer1(np);
}
return ret;
}
Even if that works, it is a fragile mechanism.
So the purpose of these changes is to provide a stronger timer declaration in
order to clearly split in the kernel a clocksource and a clockevent
initialization.
> > > > With this approach, we allow a mechanism to clearly define a clocksource or a
> > > > clockevent without aerobatics we can find around in some drivers:
> > > > timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> > > > renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
> > >
> > > These all already have bindings and work. What problem are you trying to
> > > solve other than restructuring Linux?
> >
> > Yes, there is already the bindings, but that force to do some hackish
> > initialization.
>
> Here, you are forcing hackish DT changes that do not truly describe HW.
> How is that better?
So if this is hackish DT changes, then the existing DTs should be fixed, no?
> > I would like to give the opportunity to declare separately a clocksource and a
> > clockevent, in order to have full control of how this is initialized.
>
> To me it sounds like what we need is Linux infrastructure that allows
> one to register a device as having both clockevent/clocksource
> functionality.
That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them
calling their respective init routines. And in addition a TIMER_OF doing both
CLOCKEVENT_OF and CLOCKSOURCE_OF.
It is the DT which does not allow to do this separation.
Would be the following approach more acceptable ?
1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming)
2. A node can have a clockevent and|or a clocksource attributes
3. The timer_probe pass a flag to the driver's init function, so this one knows
if it should invoke the clockevent/clocksource init functions.
No attribute defaults to clocksource|clockevent.
That would be backward compatible and will let to create drivers with clutch
activated device via DT. Also, it will give the opportunity to the existing
drivers to change consolidate their initialization routines.
> That way, we can choose to do something sane at boot time, and if the
> user really wants specific devices used in specific ways, they can
> request that.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote:
> On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote:
> > On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote:
> > > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote:
> > > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
>
> You can have several timers on the system and may want to use the clockevents
> from one IP block and the clocksource from another IP block. For example, in
> the case of a bogus clocksource.
I understand this. However, what I was trying to say is that *how* we
use a particular device should be a software decision. I have a more
concrete suggestion on that below.
> Moreover there are some drivers with a node for a clocksource and
> another one for the clockevent, and the driver is assuming the clockevent is
> defined first and then the clocksource.
>
> eg.
>
> arch/arc/boot/dts/abilis_tb10x.dtsi:
>
> /* TIMER0 with interrupt for clockevent */
> timer0 {
> compatible = "snps,arc-timer";
> interrupts = <3>;
> interrupt-parent = <&intc>;
> clocks = <&cpu_clk>;
> };
>
> /* TIMER1 for free running clocksource */
> timer1 {
> compatible = "snps,arc-timer";
> clocks = <&cpu_clk>;
> };
>
> drivers/clocksource/arc_timer.c:
>
> static int __init arc_of_timer_init(struct device_node *np)
> {
> static int init_count = 0;
> int ret;
>
> if (!init_count) {
> init_count = 1;
> ret = arc_clockevent_setup(np);
> } else {
> ret = arc_cs_setup_timer1(np);
> }
>
> return ret;
> }
>
> Even if that works, it is a fragile mechanism.
>
> So the purpose of these changes is to provide a stronger timer declaration in
> order to clearly split in the kernel a clocksource and a clockevent
> initialization.
I agree that this pattern is not nice. However, I think that splitting
devices as this level makes the problem *worse*.
Users care that they have a clocksource and a clockevent device. They
do not care *which* particular device is used as either. The comments in
the DT above are at best misleading.
What we need is for the kernel to understand that devices can be both
clockevent and clocksource (perhaps mutually exclusively), such that the
kernel can decide how to make use of devices.
That way, for the above the kernel can figure out that timer0 could be
used as clocksource or clockevent, while timer1 can only be used as a
clocksource due to the lack of an interrupt. Thus, it can choose to use
timer0 as a clockevent, and timer1 and a clocksource.
> > > > > With this approach, we allow a mechanism to clearly define a clocksource or a
> > > > > clockevent without aerobatics we can find around in some drivers:
> > > > > timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> > > > > renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
> > > >
> > > > These all already have bindings and work. What problem are you trying to
> > > > solve other than restructuring Linux?
> > >
> > > Yes, there is already the bindings, but that force to do some hackish
> > > initialization.
> >
> > Here, you are forcing hackish DT changes that do not truly describe HW.
> > How is that better?
>
> So if this is hackish DT changes, then the existing DTs should be fixed, no?
Yes.
For the above snippet, the only thing that needs to change is the
comment.
> > > I would like to give the opportunity to declare separately a clocksource and a
> > > clockevent, in order to have full control of how this is initialized.
> >
> > To me it sounds like what we need is Linux infrastructure that allows
> > one to register a device as having both clockevent/clocksource
> > functionality.
>
> That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them
> calling their respective init routines. And in addition a TIMER_OF doing both
> CLOCKEVENT_OF and CLOCKSOURCE_OF.
>
> It is the DT which does not allow to do this separation.
>
> Would be the following approach more acceptable ?
>
> 1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming)
I am fine with this renaming.
> 2. A node can have a clockevent and|or a clocksource attributes
As above, this should not be in the DT given it's describing a
(Linux-specific) SW policy and not a HW detail.
So I must disagree with this.
> 3. The timer_probe pass a flag to the driver's init function, so this one knows
> if it should invoke the clockevent/clocksource init functions.
> No attribute defaults to clocksource|clockevent.
>
> That would be backward compatible and will let to create drivers with clutch
> activated device via DT. Also, it will give the opportunity to the existing
> drivers to change consolidate their initialization routines.
I think that if anything, we need a combined clocksource+clockevent
device that we register to the core code. That means all
clocksource/clockevent drivers have a consolidated routine.
Subsequently, core code should determine how specifically to use the
device (e.g. based on what other devices are registered, and their
capabilities).
Thanks,
Mark.
Hello, Daniel.
Due to recent comments from Mark[1], may be is better to apply v6[2] series instead of v7[3]?
Because my main goal was to fix wall time on rk3188. And I did it the same way how that was
already done for other timer drivers (one of possible solution).
You can rename CLOCKSOURCE_OF to TIMER_OF later. I can help with that, but I don’t
think it is good idea to combine my changes and timer framework cleanups/improvements
into single series.
And I thinks, that probably is is better to drop [3] and [4] and revert 0c8893c9095d ("clockevents: Add a
clkevt-of mechanism like clksrc-of»).
What do you think?
[1] https://lkml.org/lkml/2017/3/29/286
[2] https://lkml.org/lkml/2017/1/31/401
[3] https://lkml.org/lkml/2017/3/22/508
[4] https://lkml.org/lkml/2017/3/22/420
[5] https://lkml.org/lkml/2017/3/22/426
> 22 марта 2017 г., в 18:48, Alexander Kochetkov <[email protected]> написал(а):
>
> Hello, Daniel, Heiko.
>
> Here is try 7 :) Could you please take a look into it?
>
> rockchip_timer init code implemented using CLOCKEVENT_OF_DECLARE()
> introduced in commit 0c8893c9095d ("clockevents: Add a clkevt-of
> mechanism like clksrc-of")
>
> There is change in the arch/arm/mach-rockchip/rockchip.c.
>
> This series should be applied after the commit:
> https://lkml.org/lkml/2017/3/22/426
>
> As without the commit, you will get linker error ("clkevt-probe.c:63:
> undefined reference to `__clkevt_of_table’")
>
> Regards,
> Alexander.
>
>
> Alexander Kochetkov (6):
> dt-bindings: clarify compatible property for rockchip timers
> ARM: dts: rockchip: update compatible property for rk322x timer
> ARM: dts: rockchip: add clockevent attribute to rockchip timers
> clocksource/drivers/rockchip_timer: implement clocksource timer
> ARM: dts: rockchip: add timer entries to rk3188 SoC
> ARM: dts: rockchip: disable arm-global-timer for rk3188
>
> Daniel Lezcano (1):
> clocksource/drivers/clksrc-evt-probe: Describe with the DT both the
> clocksource and the clockevent
>
> .../bindings/timer/rockchip,rk-timer.txt | 12 +-
> Documentation/devicetree/bindings/timer/timer.txt | 38 ++++
> arch/arm/boot/dts/rk3036.dtsi | 1 +
> arch/arm/boot/dts/rk3188.dtsi | 19 ++
> arch/arm/boot/dts/rk322x.dtsi | 3 +-
> arch/arm/boot/dts/rk3288.dtsi | 1 +
> arch/arm/mach-rockchip/rockchip.c | 2 +
> arch/arm64/boot/dts/rockchip/rk3368.dtsi | 1 +
> drivers/clocksource/Kconfig | 2 +
> drivers/clocksource/clkevt-probe.c | 7 +
> drivers/clocksource/clksrc-probe.c | 7 +
> drivers/clocksource/rockchip_timer.c | 215 ++++++++++++++------
> 12 files changed, 241 insertions(+), 67 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
>
> --
> 1.7.9.5
>
On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote:
> On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote:
> > On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote:
> > > On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote:
> > > > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote:
> > > > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> >
> > You can have several timers on the system and may want to use the clockevents
> > from one IP block and the clocksource from another IP block. For example, in
> > the case of a bogus clocksource.
>
> I understand this. However, what I was trying to say is that *how* we
> use a particular device should be a software decision. I have a more
> concrete suggestion on that below.
>
> > Moreover there are some drivers with a node for a clocksource and
> > another one for the clockevent, and the driver is assuming the clockevent is
> > defined first and then the clocksource.
> >
> > eg.
> >
> > arch/arc/boot/dts/abilis_tb10x.dtsi:
> >
> > /* TIMER0 with interrupt for clockevent */
> > timer0 {
> > compatible = "snps,arc-timer";
> > interrupts = <3>;
> > interrupt-parent = <&intc>;
> > clocks = <&cpu_clk>;
> > };
> >
> > /* TIMER1 for free running clocksource */
> > timer1 {
> > compatible = "snps,arc-timer";
> > clocks = <&cpu_clk>;
> > };
> >
> > drivers/clocksource/arc_timer.c:
> >
> > static int __init arc_of_timer_init(struct device_node *np)
> > {
> > static int init_count = 0;
> > int ret;
> >
> > if (!init_count) {
> > init_count = 1;
> > ret = arc_clockevent_setup(np);
> > } else {
> > ret = arc_cs_setup_timer1(np);
> > }
> >
> > return ret;
> > }
> >
> > Even if that works, it is a fragile mechanism.
> >
> > So the purpose of these changes is to provide a stronger timer declaration in
> > order to clearly split in the kernel a clocksource and a clockevent
> > initialization.
>
> I agree that this pattern is not nice. However, I think that splitting
> devices as this level makes the problem *worse*.
>
> Users care that they have a clocksource and a clockevent device. They
> do not care *which* particular device is used as either. The comments in
> the DT above are at best misleading.
Agree.
And the driver is assuming the first node is the clockevent and the second one
is the clocksource. If the DT invert these nodes, that breaks the driver.
> What we need is for the kernel to understand that devices can be both
> clockevent and clocksource (perhaps mutually exclusively), such that the
> kernel can decide how to make use of devices.
>
> That way, for the above the kernel can figure out that timer0 could be
> used as clocksource or clockevent, while timer1 can only be used as a
> clocksource due to the lack of an interrupt. Thus, it can choose to use
> timer0 as a clockevent, and timer1 and a clocksource.
Well, 'interrupt' gives an indication the timer can be used as a clockevent and
clocksource, not the clockevent only.
If we take the case of the rockchip, the arm_arch_timer clocksource is stopped
when the CPU is clock gated. So specifically, we don't want to use this
clocksource but we want to use the arch clockevents because they are better.
> > > > > > With this approach, we allow a mechanism to clearly define a clocksource or a
> > > > > > clockevent without aerobatics we can find around in some drivers:
> > > > > > timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> > > > > > renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
> > > > >
> > > > > These all already have bindings and work. What problem are you trying to
> > > > > solve other than restructuring Linux?
> > > >
> > > > Yes, there is already the bindings, but that force to do some hackish
> > > > initialization.
> > >
> > > Here, you are forcing hackish DT changes that do not truly describe HW.
> > > How is that better?
> >
> > So if this is hackish DT changes, then the existing DTs should be fixed, no?
>
> Yes.
>
> For the above snippet, the only thing that needs to change is the
> comment.
>
> > > > I would like to give the opportunity to declare separately a clocksource and a
> > > > clockevent, in order to have full control of how this is initialized.
> > >
> > > To me it sounds like what we need is Linux infrastructure that allows
> > > one to register a device as having both clockevent/clocksource
> > > functionality.
> >
> > That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them
> > calling their respective init routines. And in addition a TIMER_OF doing both
> > CLOCKEVENT_OF and CLOCKSOURCE_OF.
> >
> > It is the DT which does not allow to do this separation.
> >
> > Would be the following approach more acceptable ?
> >
> > 1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming)
>
> I am fine with this renaming.
>
> > 2. A node can have a clockevent and|or a clocksource attributes
>
> As above, this should not be in the DT given it's describing a
> (Linux-specific) SW policy and not a HW detail.
>
> So I must disagree with this.
IIUC my discussion with Rob, an attribute is acceptable (btw if
'clocksource'|'clockevent' names are too Linux specific (+1), what
about a more generic name like 'tick' and 'time' ?).
> > 3. The timer_probe pass a flag to the driver's init function, so this one knows
> > if it should invoke the clockevent/clocksource init functions.
> > No attribute defaults to clocksource|clockevent.
> >
> > That would be backward compatible and will let to create drivers with clutch
> > activated device via DT. Also, it will give the opportunity to the existing
> > drivers to change consolidate their initialization routines.
>
> I think that if anything, we need a combined clocksource+clockevent
> device that we register to the core code. That means all
> clocksource/clockevent drivers have a consolidated routine.
>
> Subsequently, core code should determine how specifically to use the
> device (e.g. based on what other devices are registered, and their
> capabilities).
IMO, the core code is complex enough and that may imply more heuristics.
Regarding the number of timers, I do believe it is much better to simply tell
which one we want to use via the DT (assuming the DT is a description of the
desired hardware, not all the available hardware).
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Wed, Mar 29, 2017 at 03:41:34PM +0200, Daniel Lezcano wrote:
> On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote:
> > On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote:
> > > arch/arc/boot/dts/abilis_tb10x.dtsi:
> > >
> > > /* TIMER0 with interrupt for clockevent */
> > > timer0 {
> > > compatible = "snps,arc-timer";
> > > interrupts = <3>;
> > > interrupt-parent = <&intc>;
> > > clocks = <&cpu_clk>;
> > > };
> > >
> > > /* TIMER1 for free running clocksource */
> > > timer1 {
> > > compatible = "snps,arc-timer";
> > > clocks = <&cpu_clk>;
> > > };
> > >
> > > drivers/clocksource/arc_timer.c:
> > >
> > > static int __init arc_of_timer_init(struct device_node *np)
> > > {
> > > static int init_count = 0;
> > > int ret;
> > >
> > > if (!init_count) {
> > > init_count = 1;
> > > ret = arc_clockevent_setup(np);
> > > } else {
> > > ret = arc_cs_setup_timer1(np);
> > > }
> > >
> > > return ret;
> > > }
> > >
> > > So the purpose of these changes is to provide a stronger timer declaration in
> > > order to clearly split in the kernel a clocksource and a clockevent
> > > initialization.
> >
> > I agree that this pattern is not nice. However, I think that splitting
> > devices as this level makes the problem *worse*.
> >
> > Users care that they have a clocksource and a clockevent device. They
> > do not care *which* particular device is used as either. The comments in
> > the DT above are at best misleading.
>
> Agree.
>
> And the driver is assuming the first node is the clockevent and the second one
> is the clocksource. If the DT invert these nodes, that breaks the driver.
Sure, but that is something we can and should fix within Linux.
> > What we need is for the kernel to understand that devices can be both
> > clockevent and clocksource (perhaps mutually exclusively), such that the
> > kernel can decide how to make use of devices.
> >
> > That way, for the above the kernel can figure out that timer0 could be
> > used as clocksource or clockevent, while timer1 can only be used as a
> > clocksource due to the lack of an interrupt. Thus, it can choose to use
> > timer0 as a clockevent, and timer1 and a clocksource.
>
> Well, 'interrupt' gives an indication the timer can be used as a clockevent and
> clocksource, not the clockevent only.
Which is exactly what I said above, when I said:
the kernel can figure out that timer0 could be used as
clocksource or clockevent
Considering both timer0 and timer1 is how we can figure out timer0 must
be the clockevent, since timer1 cannot be.
> If we take the case of the rockchip, the arm_arch_timer clocksource is stopped
> when the CPU is clock gated. So specifically, we don't want to use this
> clocksource but we want to use the arch clockevents because they are better.
Sure. As I pointed out, we want to consider the holistic details to make
the right decision. i.e. the infrastructure should make the choice, not
the individual drivers.
Consider that the kernel may need to make decisions that differ it a
kernel is built wihout certain drivers. That cannot work if the use is
allocated in the DT.
[...]
> > > 2. A node can have a clockevent and|or a clocksource attributes
> >
> > As above, this should not be in the DT given it's describing a
> > (Linux-specific) SW policy and not a HW detail.
> >
> > So I must disagree with this.
>
> IIUC my discussion with Rob, an attribute is acceptable (btw if
> 'clocksource'|'clockevent' names are too Linux specific (+1), what
> about a more generic name like 'tick' and 'time' ?).
The *meaning* of these is Linux specific. The naming is irrelevant.
> > > 3. The timer_probe pass a flag to the driver's init function, so this one knows
> > > if it should invoke the clockevent/clocksource init functions.
> > > No attribute defaults to clocksource|clockevent.
> > >
> > > That would be backward compatible and will let to create drivers with clutch
> > > activated device via DT. Also, it will give the opportunity to the existing
> > > drivers to change consolidate their initialization routines.
> >
> > I think that if anything, we need a combined clocksource+clockevent
> > device that we register to the core code. That means all
> > clocksource/clockevent drivers have a consolidated routine.
> >
> > Subsequently, core code should determine how specifically to use the
> > device (e.g. based on what other devices are registered, and their
> > capabilities).
>
> IMO, the core code is complex enough and that may imply more heuristics.
Given that the majority of cases are going to be multiple instances of
the same IP, I cannot imagine it is complex to get something that works.
A generally optimal configuration may require some heuristics, but
that's a different matter to a correct and functional configuration.
I must disagree with trying to push that complexity into the DT by means
of static SW policy, rather than solving the problem in SW where we have
all the information to do so.
Thanks,
Mark.
On 29/03/2017 15:22, Alexander Kochetkov wrote:
> Hello, Daniel.
Hi Alexander,
> Due to recent comments from Mark[1], may be is better to apply v6[2] series instead of v7[3]?
> Because my main goal was to fix wall time on rk3188. And I did it the same way how that was
> already done for other timer drivers (one of possible solution).
>
> You can rename CLOCKSOURCE_OF to TIMER_OF later. I can help with that, but I don’t
> think it is good idea to combine my changes and timer framework cleanups/improvements
> into single series.
>
> And I thinks, that probably is is better to drop [3] and [4] and revert 0c8893c9095d ("clockevents: Add a
> clkevt-of mechanism like clksrc-of»).
>
> What do you think?
I share your opinion.
-- Daniel
> [1] https://lkml.org/lkml/2017/3/29/286
> [2] https://lkml.org/lkml/2017/1/31/401
> [3] https://lkml.org/lkml/2017/3/22/508
> [4] https://lkml.org/lkml/2017/3/22/420
> [5] https://lkml.org/lkml/2017/3/22/426
>
>> 22 марта 2017 г., в 18:48, Alexander Kochetkov <[email protected]> написал(а):
>>
>> Hello, Daniel, Heiko.
>>
>> Here is try 7 :) Could you please take a look into it?
>>
>> rockchip_timer init code implemented using CLOCKEVENT_OF_DECLARE()
>> introduced in commit 0c8893c9095d ("clockevents: Add a clkevt-of
>> mechanism like clksrc-of")
>>
>> There is change in the arch/arm/mach-rockchip/rockchip.c.
>>
>> This series should be applied after the commit:
>> https://lkml.org/lkml/2017/3/22/426
>>
>> As without the commit, you will get linker error ("clkevt-probe.c:63:
>> undefined reference to `__clkevt_of_table’")
>>
>> Regards,
>> Alexander.
>>
>>
>> Alexander Kochetkov (6):
>> dt-bindings: clarify compatible property for rockchip timers
>> ARM: dts: rockchip: update compatible property for rk322x timer
>> ARM: dts: rockchip: add clockevent attribute to rockchip timers
>> clocksource/drivers/rockchip_timer: implement clocksource timer
>> ARM: dts: rockchip: add timer entries to rk3188 SoC
>> ARM: dts: rockchip: disable arm-global-timer for rk3188
>>
>> Daniel Lezcano (1):
>> clocksource/drivers/clksrc-evt-probe: Describe with the DT both the
>> clocksource and the clockevent
>>
>> .../bindings/timer/rockchip,rk-timer.txt | 12 +-
>> Documentation/devicetree/bindings/timer/timer.txt | 38 ++++
>> arch/arm/boot/dts/rk3036.dtsi | 1 +
>> arch/arm/boot/dts/rk3188.dtsi | 19 ++
>> arch/arm/boot/dts/rk322x.dtsi | 3 +-
>> arch/arm/boot/dts/rk3288.dtsi | 1 +
>> arch/arm/mach-rockchip/rockchip.c | 2 +
>> arch/arm64/boot/dts/rockchip/rk3368.dtsi | 1 +
>> drivers/clocksource/Kconfig | 2 +
>> drivers/clocksource/clkevt-probe.c | 7 +
>> drivers/clocksource/clksrc-probe.c | 7 +
>> drivers/clocksource/rockchip_timer.c | 215 ++++++++++++++------
>> 12 files changed, 241 insertions(+), 67 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
>>
>> --
>> 1.7.9.5
>>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog