[PATCH v6 0/5] stm32 clocksource driver rework

[PATCH v6 0/5] stm32 clocksource driver rework

[Benjamin Gaignard]

version 6:
- add timer_of_deinit function in core
- rework failure cases in probe function

version 5:
- rebase on top of timer/core branch
- rework commit message of the first patch

version 4:
- split patch in 3 parts
  - convert code to timer_of
  - only use 32 bits timers
  - add clocksource support

version 3:
- fix comments done by Daniel
- use timer_of helper functions

version 2:
- fix uninitialized variable

These patches implements clocksource and clockevent by using only one hardware block.
Getting both clock source and events on the same hardware lead to change quite
a lot driver code.
It also limits usage of clocksource to 32 bits timers because 16 bits ones
aren't enough accurate.
Thanks to timer_fo helpers this series includes minor clean up in structures,
function prototypes and driver name.

Since 16 bits timers become useless it also removes them from stm32f4 and
stm32f7 devicetree.

Increase min delta value to be sure to not have too much interrupts.


Benjamin Gaignard (5):
  timer: add timer_of_deinit function
  clocksource: stm32: convert driver to timer_of
  clocksource: stm32: only use 32 bits timers
  clocksource: stm32: add clocksource support
  arm: dts: stm32: remove useless clocksource nodes

 arch/arm/boot/dts/stm32f429.dtsi  |  32 ------
 arch/arm/boot/dts/stm32f746.dtsi  |  32 ------
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-of.c    |  12 ++
 drivers/clocksource/timer-of.h    |   3 +
 drivers/clocksource/timer-stm32.c | 232 +++++++++++++++++++-------------------
 6 files changed, 132 insertions(+), 180 deletions(-)

-- 
2.7.4

[PATCH v6 0/5] stm32 clocksource driver rework

[Benjamin Gaignard]

version 6:
- add timer_of_deinit function in core
- rework failure cases in probe function

version 5:
- rebase on top of timer/core branch
- rework commit message of the first patch

version 4:
- split patch in 3 parts
  - convert code to timer_of
  - only use 32 bits timers
  - add clocksource support

version 3:
- fix comments done by Daniel
- use timer_of helper functions

version 2:
- fix uninitialized variable

These patches implements clocksource and clockevent by using only one hardware block.
Getting both clock source and events on the same hardware lead to change quite
a lot driver code.
It also limits usage of clocksource to 32 bits timers because 16 bits ones
aren't enough accurate.
Thanks to timer_fo helpers this series includes minor clean up in structures,
function prototypes and driver name.

Since 16 bits timers become useless it also removes them from stm32f4 and
stm32f7 devicetree.

Increase min delta value to be sure to not have too much interrupts.


Benjamin Gaignard (5):
  timer: add timer_of_deinit function
  clocksource: stm32: convert driver to timer_of
  clocksource: stm32: only use 32 bits timers
  clocksource: stm32: add clocksource support
  arm: dts: stm32: remove useless clocksource nodes

 arch/arm/boot/dts/stm32f429.dtsi  |  32 ------
 arch/arm/boot/dts/stm32f746.dtsi  |  32 ------
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-of.c    |  12 ++
 drivers/clocksource/timer-of.h    |   3 +
 drivers/clocksource/timer-stm32.c | 232 +++++++++++++++++++-------------------
 6 files changed, 132 insertions(+), 180 deletions(-)

-- 
2.7.4

[PATCH v6 1/5] timer: add timer_of_deinit function

[Benjamin Gaignard]

Add this deinit function to be able to undo what have been done
in timer_of_init().

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/timer-of.c | 12 ++++++++++++
 drivers/clocksource/timer-of.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index c79122d..14404a0 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -176,3 +176,15 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
 		timer_base_exit(&to->of_base);
 	return ret;
 }
+
+void timer_of_deinit(struct timer_of *to)
+{
+	if (to->flags & TIMER_OF_IRQ)
+		timer_irq_exit(&to->of_irq);
+
+	if (to->flags & TIMER_OF_CLOCK)
+		timer_clk_exit(&to->of_clk);
+
+	if (to->flags & TIMER_OF_BASE)
+		timer_base_exit(&to->of_base);
+}
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index e0d7272..3833ab1 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -66,4 +66,7 @@ static inline unsigned long timer_of_period(struct timer_of *to)
 
 extern int __init timer_of_init(struct device_node *np,
 				struct timer_of *to);
+
+extern void timer_of_deinit(struct timer_of *to);
+
 #endif
-- 
2.7.4

[PATCH v6 1/5] timer: add timer_of_deinit function

[Benjamin Gaignard]

Add this deinit function to be able to undo what have been done
in timer_of_init().

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/timer-of.c | 12 ++++++++++++
 drivers/clocksource/timer-of.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index c79122d..14404a0 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -176,3 +176,15 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
 		timer_base_exit(&to->of_base);
 	return ret;
 }
+
+void timer_of_deinit(struct timer_of *to)
+{
+	if (to->flags & TIMER_OF_IRQ)
+		timer_irq_exit(&to->of_irq);
+
+	if (to->flags & TIMER_OF_CLOCK)
+		timer_clk_exit(&to->of_clk);
+
+	if (to->flags & TIMER_OF_BASE)
+		timer_base_exit(&to->of_base);
+}
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index e0d7272..3833ab1 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -66,4 +66,7 @@ static inline unsigned long timer_of_period(struct timer_of *to)
 
 extern int __init timer_of_init(struct device_node *np,
 				struct timer_of *to);
+
+extern void timer_of_deinit(struct timer_of *to);
+
 #endif
-- 
2.7.4

[PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

Convert driver to use timer_of helpers. This allow to remove
custom proprietary structure.

Increase min delta value because if it is too small it could
generate too much interrupts and the system will not be able
to catch them all.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-stm32.c | 160 ++++++++++++++------------------------
 2 files changed, 58 insertions(+), 103 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cc60620..755c0cc 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -289,6 +289,7 @@ config CLKSRC_STM32
 	bool "Clocksource for STM32 SoCs" if !ARCH_STM32
 	depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
 	select CLKSRC_MMIO
+	select TIMER_OF
 
 config CLKSRC_MPS2
 	bool "Clocksource for MPS2 SoCs" if COMPILE_TEST
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 8f24237..67dcf48 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -17,6 +17,8 @@
 #include <linux/clk.h>
 #include <linux/reset.h>
 
+#include "timer-of.h"
+
 #define TIM_CR1		0x00
 #define TIM_DIER	0x0c
 #define TIM_SR		0x10
@@ -34,117 +36,84 @@
 
 #define TIM_EGR_UG	BIT(0)
 
-struct stm32_clock_event_ddata {
-	struct clock_event_device evtdev;
-	unsigned periodic_top;
-	void __iomem *base;
-};
-
-static int stm32_clock_event_shutdown(struct clock_event_device *evtdev)
+static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
-	void *base = data->base;
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, base + TIM_CR1);
+	writel_relaxed(0, timer_of_base(to) + TIM_CR1);
 	return 0;
 }
 
-static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
-	void *base = data->base;
+	struct timer_of *to = to_timer_of(evt);
+
+	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
 
-	writel_relaxed(data->periodic_top, base + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, base + TIM_CR1);
 	return 0;
 }
 
 static int stm32_clock_event_set_next_event(unsigned long evt,
-					    struct clock_event_device *evtdev)
+					    struct clock_event_device *clkevt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
+	struct timer_of *to = to_timer_of(clkevt);
 
-	writel_relaxed(evt, data->base + TIM_ARR);
+	writel_relaxed(evt, timer_of_base(to) + TIM_ARR);
 	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
-		       data->base + TIM_CR1);
+		       timer_of_base(to) + TIM_CR1);
 
 	return 0;
 }
 
 static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 {
-	struct stm32_clock_event_ddata *data = dev_id;
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, data->base + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
-	data->evtdev.event_handler(&data->evtdev);
+	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static struct stm32_clock_event_ddata clock_event_ddata = {
-	.evtdev = {
-		.name = "stm32 clockevent",
-		.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
-		.set_state_shutdown = stm32_clock_event_shutdown,
-		.set_state_periodic = stm32_clock_event_set_periodic,
-		.set_state_oneshot = stm32_clock_event_shutdown,
-		.tick_resume = stm32_clock_event_shutdown,
-		.set_next_event = stm32_clock_event_set_next_event,
-		.rating = 200,
-	},
-};
-
-static int __init stm32_clockevent_init(struct device_node *np)
+static int __init stm32_clockevent_init(struct device_node *node)
 {
-	struct stm32_clock_event_ddata *data = &clock_event_ddata;
-	struct clk *clk;
 	struct reset_control *rstc;
-	unsigned long rate, max_delta;
-	int irq, ret, bits, prescaler = 1;
-
-	clk = of_clk_get(np, 0);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		pr_err("failed to get clock for clockevent (%d)\n", ret);
-		goto err_clk_get;
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		pr_err("failed to enable timer clock for clockevent (%d)\n",
-		       ret);
-		goto err_clk_enable;
-	}
-
-	rate = clk_get_rate(clk);
-
-	rstc = of_reset_control_get(np, NULL);
+	unsigned long max_delta;
+	int ret, bits, prescaler = 1;
+	struct timer_of *to;
+
+	to = kzalloc(sizeof(*to), GFP_KERNEL);
+	if (!to)
+		return -ENOMEM;
+
+	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
+	to->clkevt.name = "stm32_clockevent";
+	to->clkevt.rating = 200;
+	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
+	to->clkevt.set_state_shutdown = stm32_clock_event_shutdown;
+	to->clkevt.set_state_periodic = stm32_clock_event_set_periodic;
+	to->clkevt.set_state_oneshot = stm32_clock_event_shutdown;
+	to->clkevt.tick_resume = stm32_clock_event_shutdown;
+	to->clkevt.set_next_event = stm32_clock_event_set_next_event;
+
+	to->of_irq.handler = stm32_clock_event_handler;
+
+	ret = timer_of_init(node, to);
+	if (ret)
+		goto err;
+
+	rstc = of_reset_control_get(node, NULL);
 	if (!IS_ERR(rstc)) {
 		reset_control_assert(rstc);
 		reset_control_deassert(rstc);
 	}
 
-	data->base = of_iomap(np, 0);
-	if (!data->base) {
-		ret = -ENXIO;
-		pr_err("failed to map registers for clockevent\n");
-		goto err_iomap;
-	}
-
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		ret = -EINVAL;
-		pr_err("%pOF: failed to get irq.\n", np);
-		goto err_get_irq;
-	}
-
 	/* Detect whether the timer is 16 or 32 bits */
-	writel_relaxed(~0U, data->base + TIM_ARR);
-	max_delta = readl_relaxed(data->base + TIM_ARR);
+	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
 	if (max_delta == ~0U) {
 		prescaler = 1;
 		bits = 32;
@@ -152,38 +121,23 @@ static int __init stm32_clockevent_init(struct device_node *np)
 		prescaler = 1024;
 		bits = 16;
 	}
-	writel_relaxed(0, data->base + TIM_ARR);
-
-	writel_relaxed(prescaler - 1, data->base + TIM_PSC);
-	writel_relaxed(TIM_EGR_UG, data->base + TIM_EGR);
-	writel_relaxed(TIM_DIER_UIE, data->base + TIM_DIER);
-	writel_relaxed(0, data->base + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 
-	data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
+	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
+	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
-	clockevents_config_and_register(&data->evtdev,
-					DIV_ROUND_CLOSEST(rate, prescaler),
-					0x1, max_delta);
-
-	ret = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
-			"stm32 clockevent", data);
-	if (ret) {
-		pr_err("%pOF: failed to request irq.\n", np);
-		goto err_get_irq;
-	}
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_period(to), 0x60, max_delta);
 
 	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			np, bits);
+			node, bits);
 
-	return ret;
+	return 0;
 
-err_get_irq:
-	iounmap(data->base);
-err_iomap:
-	clk_disable_unprepare(clk);
-err_clk_enable:
-	clk_put(clk);
-err_clk_get:
+err:
+	kfree(to);
 	return ret;
 }
 
-- 
2.7.4

[PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

Convert driver to use timer_of helpers. This allow to remove
custom proprietary structure.

Increase min delta value because if it is too small it could
generate too much interrupts and the system will not be able
to catch them all.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-stm32.c | 160 ++++++++++++++------------------------
 2 files changed, 58 insertions(+), 103 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cc60620..755c0cc 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -289,6 +289,7 @@ config CLKSRC_STM32
 	bool "Clocksource for STM32 SoCs" if !ARCH_STM32
 	depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
 	select CLKSRC_MMIO
+	select TIMER_OF
 
 config CLKSRC_MPS2
 	bool "Clocksource for MPS2 SoCs" if COMPILE_TEST
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 8f24237..67dcf48 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -17,6 +17,8 @@
 #include <linux/clk.h>
 #include <linux/reset.h>
 
+#include "timer-of.h"
+
 #define TIM_CR1		0x00
 #define TIM_DIER	0x0c
 #define TIM_SR		0x10
@@ -34,117 +36,84 @@
 
 #define TIM_EGR_UG	BIT(0)
 
-struct stm32_clock_event_ddata {
-	struct clock_event_device evtdev;
-	unsigned periodic_top;
-	void __iomem *base;
-};
-
-static int stm32_clock_event_shutdown(struct clock_event_device *evtdev)
+static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
-	void *base = data->base;
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, base + TIM_CR1);
+	writel_relaxed(0, timer_of_base(to) + TIM_CR1);
 	return 0;
 }
 
-static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
-	void *base = data->base;
+	struct timer_of *to = to_timer_of(evt);
+
+	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
 
-	writel_relaxed(data->periodic_top, base + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, base + TIM_CR1);
 	return 0;
 }
 
 static int stm32_clock_event_set_next_event(unsigned long evt,
-					    struct clock_event_device *evtdev)
+					    struct clock_event_device *clkevt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
+	struct timer_of *to = to_timer_of(clkevt);
 
-	writel_relaxed(evt, data->base + TIM_ARR);
+	writel_relaxed(evt, timer_of_base(to) + TIM_ARR);
 	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
-		       data->base + TIM_CR1);
+		       timer_of_base(to) + TIM_CR1);
 
 	return 0;
 }
 
 static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 {
-	struct stm32_clock_event_ddata *data = dev_id;
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, data->base + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
-	data->evtdev.event_handler(&data->evtdev);
+	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static struct stm32_clock_event_ddata clock_event_ddata = {
-	.evtdev = {
-		.name = "stm32 clockevent",
-		.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
-		.set_state_shutdown = stm32_clock_event_shutdown,
-		.set_state_periodic = stm32_clock_event_set_periodic,
-		.set_state_oneshot = stm32_clock_event_shutdown,
-		.tick_resume = stm32_clock_event_shutdown,
-		.set_next_event = stm32_clock_event_set_next_event,
-		.rating = 200,
-	},
-};
-
-static int __init stm32_clockevent_init(struct device_node *np)
+static int __init stm32_clockevent_init(struct device_node *node)
 {
-	struct stm32_clock_event_ddata *data = &clock_event_ddata;
-	struct clk *clk;
 	struct reset_control *rstc;
-	unsigned long rate, max_delta;
-	int irq, ret, bits, prescaler = 1;
-
-	clk = of_clk_get(np, 0);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		pr_err("failed to get clock for clockevent (%d)\n", ret);
-		goto err_clk_get;
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		pr_err("failed to enable timer clock for clockevent (%d)\n",
-		       ret);
-		goto err_clk_enable;
-	}
-
-	rate = clk_get_rate(clk);
-
-	rstc = of_reset_control_get(np, NULL);
+	unsigned long max_delta;
+	int ret, bits, prescaler = 1;
+	struct timer_of *to;
+
+	to = kzalloc(sizeof(*to), GFP_KERNEL);
+	if (!to)
+		return -ENOMEM;
+
+	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
+	to->clkevt.name = "stm32_clockevent";
+	to->clkevt.rating = 200;
+	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
+	to->clkevt.set_state_shutdown = stm32_clock_event_shutdown;
+	to->clkevt.set_state_periodic = stm32_clock_event_set_periodic;
+	to->clkevt.set_state_oneshot = stm32_clock_event_shutdown;
+	to->clkevt.tick_resume = stm32_clock_event_shutdown;
+	to->clkevt.set_next_event = stm32_clock_event_set_next_event;
+
+	to->of_irq.handler = stm32_clock_event_handler;
+
+	ret = timer_of_init(node, to);
+	if (ret)
+		goto err;
+
+	rstc = of_reset_control_get(node, NULL);
 	if (!IS_ERR(rstc)) {
 		reset_control_assert(rstc);
 		reset_control_deassert(rstc);
 	}
 
-	data->base = of_iomap(np, 0);
-	if (!data->base) {
-		ret = -ENXIO;
-		pr_err("failed to map registers for clockevent\n");
-		goto err_iomap;
-	}
-
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		ret = -EINVAL;
-		pr_err("%pOF: failed to get irq.\n", np);
-		goto err_get_irq;
-	}
-
 	/* Detect whether the timer is 16 or 32 bits */
-	writel_relaxed(~0U, data->base + TIM_ARR);
-	max_delta = readl_relaxed(data->base + TIM_ARR);
+	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
 	if (max_delta == ~0U) {
 		prescaler = 1;
 		bits = 32;
@@ -152,38 +121,23 @@ static int __init stm32_clockevent_init(struct device_node *np)
 		prescaler = 1024;
 		bits = 16;
 	}
-	writel_relaxed(0, data->base + TIM_ARR);
-
-	writel_relaxed(prescaler - 1, data->base + TIM_PSC);
-	writel_relaxed(TIM_EGR_UG, data->base + TIM_EGR);
-	writel_relaxed(TIM_DIER_UIE, data->base + TIM_DIER);
-	writel_relaxed(0, data->base + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 
-	data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
+	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
+	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
-	clockevents_config_and_register(&data->evtdev,
-					DIV_ROUND_CLOSEST(rate, prescaler),
-					0x1, max_delta);
-
-	ret = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
-			"stm32 clockevent", data);
-	if (ret) {
-		pr_err("%pOF: failed to request irq.\n", np);
-		goto err_get_irq;
-	}
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_period(to), 0x60, max_delta);
 
 	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			np, bits);
+			node, bits);
 
-	return ret;
+	return 0;
 
-err_get_irq:
-	iounmap(data->base);
-err_iomap:
-	clk_disable_unprepare(clk);
-err_clk_enable:
-	clk_put(clk);
-err_clk_get:
+err:
+	kfree(to);
 	return ret;
 }
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

Convert driver to use timer_of helpers. This allow to remove
custom proprietary structure.

Increase min delta value because if it is too small it could
generate too much interrupts and the system will not be able
to catch them all.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-stm32.c | 160 ++++++++++++++------------------------
 2 files changed, 58 insertions(+), 103 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cc60620..755c0cc 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -289,6 +289,7 @@ config CLKSRC_STM32
 	bool "Clocksource for STM32 SoCs" if !ARCH_STM32
 	depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
 	select CLKSRC_MMIO
+	select TIMER_OF
 
 config CLKSRC_MPS2
 	bool "Clocksource for MPS2 SoCs" if COMPILE_TEST
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 8f24237..67dcf48 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -17,6 +17,8 @@
 #include <linux/clk.h>
 #include <linux/reset.h>
 
+#include "timer-of.h"
+
 #define TIM_CR1		0x00
 #define TIM_DIER	0x0c
 #define TIM_SR		0x10
@@ -34,117 +36,84 @@
 
 #define TIM_EGR_UG	BIT(0)
 
-struct stm32_clock_event_ddata {
-	struct clock_event_device evtdev;
-	unsigned periodic_top;
-	void __iomem *base;
-};
-
-static int stm32_clock_event_shutdown(struct clock_event_device *evtdev)
+static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
-	void *base = data->base;
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, base + TIM_CR1);
+	writel_relaxed(0, timer_of_base(to) + TIM_CR1);
 	return 0;
 }
 
-static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
-	void *base = data->base;
+	struct timer_of *to = to_timer_of(evt);
+
+	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
 
-	writel_relaxed(data->periodic_top, base + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, base + TIM_CR1);
 	return 0;
 }
 
 static int stm32_clock_event_set_next_event(unsigned long evt,
-					    struct clock_event_device *evtdev)
+					    struct clock_event_device *clkevt)
 {
-	struct stm32_clock_event_ddata *data =
-		container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
+	struct timer_of *to = to_timer_of(clkevt);
 
-	writel_relaxed(evt, data->base + TIM_ARR);
+	writel_relaxed(evt, timer_of_base(to) + TIM_ARR);
 	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
-		       data->base + TIM_CR1);
+		       timer_of_base(to) + TIM_CR1);
 
 	return 0;
 }
 
 static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 {
-	struct stm32_clock_event_ddata *data = dev_id;
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, data->base + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
-	data->evtdev.event_handler(&data->evtdev);
+	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static struct stm32_clock_event_ddata clock_event_ddata = {
-	.evtdev = {
-		.name = "stm32 clockevent",
-		.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
-		.set_state_shutdown = stm32_clock_event_shutdown,
-		.set_state_periodic = stm32_clock_event_set_periodic,
-		.set_state_oneshot = stm32_clock_event_shutdown,
-		.tick_resume = stm32_clock_event_shutdown,
-		.set_next_event = stm32_clock_event_set_next_event,
-		.rating = 200,
-	},
-};
-
-static int __init stm32_clockevent_init(struct device_node *np)
+static int __init stm32_clockevent_init(struct device_node *node)
 {
-	struct stm32_clock_event_ddata *data = &clock_event_ddata;
-	struct clk *clk;
 	struct reset_control *rstc;
-	unsigned long rate, max_delta;
-	int irq, ret, bits, prescaler = 1;
-
-	clk = of_clk_get(np, 0);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		pr_err("failed to get clock for clockevent (%d)\n", ret);
-		goto err_clk_get;
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		pr_err("failed to enable timer clock for clockevent (%d)\n",
-		       ret);
-		goto err_clk_enable;
-	}
-
-	rate = clk_get_rate(clk);
-
-	rstc = of_reset_control_get(np, NULL);
+	unsigned long max_delta;
+	int ret, bits, prescaler = 1;
+	struct timer_of *to;
+
+	to = kzalloc(sizeof(*to), GFP_KERNEL);
+	if (!to)
+		return -ENOMEM;
+
+	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
+	to->clkevt.name = "stm32_clockevent";
+	to->clkevt.rating = 200;
+	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
+	to->clkevt.set_state_shutdown = stm32_clock_event_shutdown;
+	to->clkevt.set_state_periodic = stm32_clock_event_set_periodic;
+	to->clkevt.set_state_oneshot = stm32_clock_event_shutdown;
+	to->clkevt.tick_resume = stm32_clock_event_shutdown;
+	to->clkevt.set_next_event = stm32_clock_event_set_next_event;
+
+	to->of_irq.handler = stm32_clock_event_handler;
+
+	ret = timer_of_init(node, to);
+	if (ret)
+		goto err;
+
+	rstc = of_reset_control_get(node, NULL);
 	if (!IS_ERR(rstc)) {
 		reset_control_assert(rstc);
 		reset_control_deassert(rstc);
 	}
 
-	data->base = of_iomap(np, 0);
-	if (!data->base) {
-		ret = -ENXIO;
-		pr_err("failed to map registers for clockevent\n");
-		goto err_iomap;
-	}
-
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		ret = -EINVAL;
-		pr_err("%pOF: failed to get irq.\n", np);
-		goto err_get_irq;
-	}
-
 	/* Detect whether the timer is 16 or 32 bits */
-	writel_relaxed(~0U, data->base + TIM_ARR);
-	max_delta = readl_relaxed(data->base + TIM_ARR);
+	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
 	if (max_delta == ~0U) {
 		prescaler = 1;
 		bits = 32;
@@ -152,38 +121,23 @@ static int __init stm32_clockevent_init(struct device_node *np)
 		prescaler = 1024;
 		bits = 16;
 	}
-	writel_relaxed(0, data->base + TIM_ARR);
-
-	writel_relaxed(prescaler - 1, data->base + TIM_PSC);
-	writel_relaxed(TIM_EGR_UG, data->base + TIM_EGR);
-	writel_relaxed(TIM_DIER_UIE, data->base + TIM_DIER);
-	writel_relaxed(0, data->base + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 
-	data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
+	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
+	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
-	clockevents_config_and_register(&data->evtdev,
-					DIV_ROUND_CLOSEST(rate, prescaler),
-					0x1, max_delta);
-
-	ret = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
-			"stm32 clockevent", data);
-	if (ret) {
-		pr_err("%pOF: failed to request irq.\n", np);
-		goto err_get_irq;
-	}
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_period(to), 0x60, max_delta);
 
 	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			np, bits);
+			node, bits);
 
-	return ret;
+	return 0;
 
-err_get_irq:
-	iounmap(data->base);
-err_iomap:
-	clk_disable_unprepare(clk);
-err_clk_enable:
-	clk_put(clk);
-err_clk_get:
+err:
+	kfree(to);
 	return ret;
 }
 
-- 
2.7.4

[PATCH v6 3/5] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

16 bits hardware are not enough accure to be used.
Do no allow them to be probed by tested max counter value.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/timer-stm32.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 67dcf48..c834648 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -81,9 +81,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 static int __init stm32_clockevent_init(struct device_node *node)
 {
 	struct reset_control *rstc;
-	unsigned long max_delta;
-	int ret, bits, prescaler = 1;
+	unsigned long max_arr;
 	struct timer_of *to;
+	int ret;
 
 	to = kzalloc(sizeof(*to), GFP_KERNEL);
 	if (!to)
@@ -113,29 +113,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
 
 	/* Detect whether the timer is 16 or 32 bits */
 	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
-	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
-	if (max_delta == ~0U) {
-		prescaler = 1;
-		bits = 32;
-	} else {
-		prescaler = 1024;
-		bits = 16;
+	max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
+	if (max_arr != ~0U) {
+		pr_err("32 bits timer is needed\n");
+		ret = -EINVAL;
+		goto deinit;
 	}
+
 	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 
-	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
 	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
 	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
 	clockevents_config_and_register(&to->clkevt,
-					timer_of_period(to), 0x60, max_delta);
-
-	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			node, bits);
+					timer_of_period(to), 0x60, ~0U);
 
 	return 0;
 
+deinit:
+	timer_of_deinit(to);
 err:
 	kfree(to);
 	return ret;
-- 
2.7.4

[PATCH v6 3/5] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

16 bits hardware are not enough accure to be used.
Do no allow them to be probed by tested max counter value.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/timer-stm32.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 67dcf48..c834648 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -81,9 +81,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 static int __init stm32_clockevent_init(struct device_node *node)
 {
 	struct reset_control *rstc;
-	unsigned long max_delta;
-	int ret, bits, prescaler = 1;
+	unsigned long max_arr;
 	struct timer_of *to;
+	int ret;
 
 	to = kzalloc(sizeof(*to), GFP_KERNEL);
 	if (!to)
@@ -113,29 +113,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
 
 	/* Detect whether the timer is 16 or 32 bits */
 	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
-	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
-	if (max_delta == ~0U) {
-		prescaler = 1;
-		bits = 32;
-	} else {
-		prescaler = 1024;
-		bits = 16;
+	max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
+	if (max_arr != ~0U) {
+		pr_err("32 bits timer is needed\n");
+		ret = -EINVAL;
+		goto deinit;
 	}
+
 	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 
-	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
 	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
 	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
 	clockevents_config_and_register(&to->clkevt,
-					timer_of_period(to), 0x60, max_delta);
-
-	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			node, bits);
+					timer_of_period(to), 0x60, ~0U);
 
 	return 0;
 
+deinit:
+	timer_of_deinit(to);
 err:
 	kfree(to);
 	return ret;
-- 
2.7.4

[PATCH v6 4/5] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

Rework driver code to be able to implement clocksource and clockevent
on the same hardware block.
Before this patch only the counter of the hardware block was used to
generate clock events. Now counter will be used to provide a 32 bits
clock source and a comparator will provide clock events.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/timer-stm32.c | 104 ++++++++++++++++++++++++++++----------
 1 file changed, 76 insertions(+), 28 deletions(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index c834648..461b3ba 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -16,6 +16,8 @@
 #include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
 
 #include "timer-of.h"
 
@@ -23,16 +25,16 @@
 #define TIM_DIER	0x0c
 #define TIM_SR		0x10
 #define TIM_EGR		0x14
+#define TIM_CNT		0x24
 #define TIM_PSC		0x28
 #define TIM_ARR		0x2c
+#define TIM_CCR1	0x34
 
 #define TIM_CR1_CEN	BIT(0)
-#define TIM_CR1_OPM	BIT(3)
+#define TIM_CR1_UDIS	BIT(1)
 #define TIM_CR1_ARPE	BIT(7)
 
-#define TIM_DIER_UIE	BIT(0)
-
-#define TIM_SR_UIF	BIT(0)
+#define TIM_DIER_CC1IE	BIT(1)
 
 #define TIM_EGR_UG	BIT(0)
 
@@ -40,30 +42,34 @@ static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
 	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, timer_of_base(to) + TIM_CR1);
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+
 	return 0;
 }
 
-static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
+static int stm32_clock_event_set_next_event(unsigned long evt,
+					    struct clock_event_device *clkevt)
 {
-	struct timer_of *to = to_timer_of(evt);
+	struct timer_of *to = to_timer_of(clkevt);
+	unsigned long cnt;
 
-	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
+	cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
+	writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
+	writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
 
 	return 0;
 }
 
-static int stm32_clock_event_set_next_event(unsigned long evt,
-					    struct clock_event_device *clkevt)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
 {
-	struct timer_of *to = to_timer_of(clkevt);
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(evt, timer_of_base(to) + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
-		       timer_of_base(to) + TIM_CR1);
+	return stm32_clock_event_set_next_event(timer_of_period(to), evt);
+}
 
-	return 0;
+static int stm32_clock_event_set_oneshot(struct clock_event_device *evt)
+{
+	return stm32_clock_event_set_next_event(0, evt);
 }
 
 static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
@@ -73,12 +79,57 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
+	if (clockevent_state_periodic(evt))
+		stm32_clock_event_set_periodic(evt);
+	else
+		stm32_clock_event_shutdown(evt);
+
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static int __init stm32_clockevent_init(struct device_node *node)
+static void __init stm32_clockevent_init(struct timer_of *to)
+{
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_rate(to), 0x60, ~0U);
+}
+
+static void __iomem *stm32_timer_cnt __read_mostly;
+static u64 notrace stm32_read_sched_clock(void)
+{
+	return readl_relaxed(stm32_timer_cnt);
+}
+
+static int __init stm32_clocksource_init(struct timer_of *to)
+{
+	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS,
+		       timer_of_base(to) + TIM_CR1);
+
+	/* Make sure that registers are updated */
+	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
+
+	/* Enable controller */
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS | TIM_CR1_CEN,
+		       timer_of_base(to) + TIM_CR1);
+
+	stm32_timer_cnt = timer_of_base(to) + TIM_CNT;
+	sched_clock_register(stm32_read_sched_clock, 32, timer_of_rate(to));
+
+	return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
+				     timer_of_rate(to), 250, 32,
+				     clocksource_mmio_readl_up);
+}
+
+static int __init stm32_timer_init(struct device_node *node)
 {
 	struct reset_control *rstc;
 	unsigned long max_arr;
@@ -90,12 +141,13 @@ static int __init stm32_clockevent_init(struct device_node *node)
 		return -ENOMEM;
 
 	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
+
 	to->clkevt.name = "stm32_clockevent";
 	to->clkevt.rating = 200;
-	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
+	to->clkevt.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
 	to->clkevt.set_state_shutdown = stm32_clock_event_shutdown;
 	to->clkevt.set_state_periodic = stm32_clock_event_set_periodic;
-	to->clkevt.set_state_oneshot = stm32_clock_event_shutdown;
+	to->clkevt.set_state_oneshot = stm32_clock_event_set_oneshot;
 	to->clkevt.tick_resume = stm32_clock_event_shutdown;
 	to->clkevt.set_next_event = stm32_clock_event_set_next_event;
 
@@ -120,15 +172,11 @@ static int __init stm32_clockevent_init(struct device_node *node)
 		goto deinit;
 	}
 
-	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
-
-	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
-	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
-	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
-	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	ret = stm32_clocksource_init(to);
+	if (ret)
+		goto deinit;
 
-	clockevents_config_and_register(&to->clkevt,
-					timer_of_period(to), 0x60, ~0U);
+	stm32_clockevent_init(to);
 
 	return 0;
 
@@ -139,4 +187,4 @@ static int __init stm32_clockevent_init(struct device_node *node)
 	return ret;
 }
 
-TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_clockevent_init);
+TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_timer_init);
-- 
2.7.4

[PATCH v6 4/5] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

Rework driver code to be able to implement clocksource and clockevent
on the same hardware block.
Before this patch only the counter of the hardware block was used to
generate clock events. Now counter will be used to provide a 32 bits
clock source and a comparator will provide clock events.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/clocksource/timer-stm32.c | 104 ++++++++++++++++++++++++++++----------
 1 file changed, 76 insertions(+), 28 deletions(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index c834648..461b3ba 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -16,6 +16,8 @@
 #include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
 
 #include "timer-of.h"
 
@@ -23,16 +25,16 @@
 #define TIM_DIER	0x0c
 #define TIM_SR		0x10
 #define TIM_EGR		0x14
+#define TIM_CNT		0x24
 #define TIM_PSC		0x28
 #define TIM_ARR		0x2c
+#define TIM_CCR1	0x34
 
 #define TIM_CR1_CEN	BIT(0)
-#define TIM_CR1_OPM	BIT(3)
+#define TIM_CR1_UDIS	BIT(1)
 #define TIM_CR1_ARPE	BIT(7)
 
-#define TIM_DIER_UIE	BIT(0)
-
-#define TIM_SR_UIF	BIT(0)
+#define TIM_DIER_CC1IE	BIT(1)
 
 #define TIM_EGR_UG	BIT(0)
 
@@ -40,30 +42,34 @@ static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
 	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(0, timer_of_base(to) + TIM_CR1);
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+
 	return 0;
 }
 
-static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
+static int stm32_clock_event_set_next_event(unsigned long evt,
+					    struct clock_event_device *clkevt)
 {
-	struct timer_of *to = to_timer_of(evt);
+	struct timer_of *to = to_timer_of(clkevt);
+	unsigned long cnt;
 
-	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
+	cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
+	writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
+	writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
 
 	return 0;
 }
 
-static int stm32_clock_event_set_next_event(unsigned long evt,
-					    struct clock_event_device *clkevt)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
 {
-	struct timer_of *to = to_timer_of(clkevt);
+	struct timer_of *to = to_timer_of(evt);
 
-	writel_relaxed(evt, timer_of_base(to) + TIM_ARR);
-	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
-		       timer_of_base(to) + TIM_CR1);
+	return stm32_clock_event_set_next_event(timer_of_period(to), evt);
+}
 
-	return 0;
+static int stm32_clock_event_set_oneshot(struct clock_event_device *evt)
+{
+	return stm32_clock_event_set_next_event(0, evt);
 }
 
 static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
@@ -73,12 +79,57 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
+	if (clockevent_state_periodic(evt))
+		stm32_clock_event_set_periodic(evt);
+	else
+		stm32_clock_event_shutdown(evt);
+
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static int __init stm32_clockevent_init(struct device_node *node)
+static void __init stm32_clockevent_init(struct timer_of *to)
+{
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_rate(to), 0x60, ~0U);
+}
+
+static void __iomem *stm32_timer_cnt __read_mostly;
+static u64 notrace stm32_read_sched_clock(void)
+{
+	return readl_relaxed(stm32_timer_cnt);
+}
+
+static int __init stm32_clocksource_init(struct timer_of *to)
+{
+	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS,
+		       timer_of_base(to) + TIM_CR1);
+
+	/* Make sure that registers are updated */
+	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
+
+	/* Enable controller */
+	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS | TIM_CR1_CEN,
+		       timer_of_base(to) + TIM_CR1);
+
+	stm32_timer_cnt = timer_of_base(to) + TIM_CNT;
+	sched_clock_register(stm32_read_sched_clock, 32, timer_of_rate(to));
+
+	return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
+				     timer_of_rate(to), 250, 32,
+				     clocksource_mmio_readl_up);
+}
+
+static int __init stm32_timer_init(struct device_node *node)
 {
 	struct reset_control *rstc;
 	unsigned long max_arr;
@@ -90,12 +141,13 @@ static int __init stm32_clockevent_init(struct device_node *node)
 		return -ENOMEM;
 
 	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
+
 	to->clkevt.name = "stm32_clockevent";
 	to->clkevt.rating = 200;
-	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
+	to->clkevt.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
 	to->clkevt.set_state_shutdown = stm32_clock_event_shutdown;
 	to->clkevt.set_state_periodic = stm32_clock_event_set_periodic;
-	to->clkevt.set_state_oneshot = stm32_clock_event_shutdown;
+	to->clkevt.set_state_oneshot = stm32_clock_event_set_oneshot;
 	to->clkevt.tick_resume = stm32_clock_event_shutdown;
 	to->clkevt.set_next_event = stm32_clock_event_set_next_event;
 
@@ -120,15 +172,11 @@ static int __init stm32_clockevent_init(struct device_node *node)
 		goto deinit;
 	}
 
-	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
-
-	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
-	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
-	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
-	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	ret = stm32_clocksource_init(to);
+	if (ret)
+		goto deinit;
 
-	clockevents_config_and_register(&to->clkevt,
-					timer_of_period(to), 0x60, ~0U);
+	stm32_clockevent_init(to);
 
 	return 0;
 
@@ -139,4 +187,4 @@ static int __init stm32_clockevent_init(struct device_node *node)
 	return ret;
 }
 
-TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_clockevent_init);
+TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_timer_init);
-- 
2.7.4

[PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

16 bits timers aren't accurate enough to be used as
clocksource, remove them from stm32f4 and stm32f7 devicetree.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 arch/arm/boot/dts/stm32f429.dtsi | 32 --------------------------------
 arch/arm/boot/dts/stm32f746.dtsi | 32 --------------------------------
 2 files changed, 64 deletions(-)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index dd7e99b..ac9a3e6 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -108,14 +108,6 @@
 			};
 		};
 
-		timer3: timer@40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
 		timers3: timers@40000400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -137,14 +129,6 @@
 			};
 		};
 
-		timer4: timer@40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timers4: timers@40000800 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -194,14 +178,6 @@
 			};
 		};
 
-		timer6: timer@40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
 		timers6: timers@40001000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -218,14 +194,6 @@
 			};
 		};
 
-		timer7: timer@40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		timers7: timers@40001400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi
index 5633860..a9077e6 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -82,22 +82,6 @@
 			status = "disabled";
 		};
 
-		timer3: timer@40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
-		timer4: timer@40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timer5: timer@40000c00 {
 			compatible = "st,stm32-timer";
 			reg = <0x40000c00 0x400>;
@@ -105,22 +89,6 @@
 			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM5)>;
 		};
 
-		timer6: timer@40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
-		timer7: timer@40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		rtc: rtc@40002800 {
 			compatible = "st,stm32-rtc";
 			reg = <0x40002800 0x400>;
-- 
2.7.4

[PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

16 bits timers aren't accurate enough to be used as
clocksource, remove them from stm32f4 and stm32f7 devicetree.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/stm32f429.dtsi | 32 --------------------------------
 arch/arm/boot/dts/stm32f746.dtsi | 32 --------------------------------
 2 files changed, 64 deletions(-)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index dd7e99b..ac9a3e6 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -108,14 +108,6 @@
 			};
 		};
 
-		timer3: timer@40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
 		timers3: timers@40000400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -137,14 +129,6 @@
 			};
 		};
 
-		timer4: timer@40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timers4: timers@40000800 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -194,14 +178,6 @@
 			};
 		};
 
-		timer6: timer@40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
 		timers6: timers@40001000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -218,14 +194,6 @@
 			};
 		};
 
-		timer7: timer@40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		timers7: timers@40001400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi
index 5633860..a9077e6 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -82,22 +82,6 @@
 			status = "disabled";
 		};
 
-		timer3: timer@40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
-		timer4: timer@40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timer5: timer@40000c00 {
 			compatible = "st,stm32-timer";
 			reg = <0x40000c00 0x400>;
@@ -105,22 +89,6 @@
 			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM5)>;
 		};
 
-		timer6: timer@40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
-		timer7: timer@40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		rtc: rtc@40002800 {
 			compatible = "st,stm32-rtc";
 			reg = <0x40002800 0x400>;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

16 bits timers aren't accurate enough to be used as
clocksource, remove them from stm32f4 and stm32f7 devicetree.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 arch/arm/boot/dts/stm32f429.dtsi | 32 --------------------------------
 arch/arm/boot/dts/stm32f746.dtsi | 32 --------------------------------
 2 files changed, 64 deletions(-)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index dd7e99b..ac9a3e6 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -108,14 +108,6 @@
 			};
 		};
 
-		timer3: timer at 40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
 		timers3: timers at 40000400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -137,14 +129,6 @@
 			};
 		};
 
-		timer4: timer at 40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timers4: timers at 40000800 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -194,14 +178,6 @@
 			};
 		};
 
-		timer6: timer at 40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
 		timers6: timers at 40001000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -218,14 +194,6 @@
 			};
 		};
 
-		timer7: timer at 40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		timers7: timers at 40001400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi
index 5633860..a9077e6 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -82,22 +82,6 @@
 			status = "disabled";
 		};
 
-		timer3: timer at 40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
-		timer4: timer at 40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timer5: timer at 40000c00 {
 			compatible = "st,stm32-timer";
 			reg = <0x40000c00 0x400>;
@@ -105,22 +89,6 @@
 			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM5)>;
 		};
 
-		timer6: timer at 40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
-		timer7: timer at 40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		rtc: rtc at 40002800 {
 			compatible = "st,stm32-rtc";
 			reg = <0x40002800 0x400>;
-- 
2.7.4

Re: [PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Rob Herring]

On Wed, Oct 18, 2017 at 7:58 AM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> 16 bits timers aren't accurate enough to be used as
> clocksource, remove them from stm32f4 and stm32f7 devicetree.

They aren't useful for anything? Zephyr? u-boot?

Rob

[PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Rob Herring]

On Wed, Oct 18, 2017 at 7:58 AM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> 16 bits timers aren't accurate enough to be used as
> clocksource, remove them from stm32f4 and stm32f7 devicetree.

They aren't useful for anything? Zephyr? u-boot?

Rob

Re: [PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

2017-10-18 15:21 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Wed, Oct 18, 2017 at 7:58 AM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> 16 bits timers aren't accurate enough to be used as
>> clocksource, remove them from stm32f4 and stm32f7 devicetree.
>
> They aren't useful for anything? Zephyr? u-boot?

I have check with the teams and timers (either 16 or 32 bits) are not
used by zephyr or u-boot

>
> Rob

Re: [PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

2017-10-18 15:21 GMT+02:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Wed, Oct 18, 2017 at 7:58 AM, Benjamin Gaignard
> <benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> 16 bits timers aren't accurate enough to be used as
>> clocksource, remove them from stm32f4 and stm32f7 devicetree.
>
> They aren't useful for anything? Zephyr? u-boot?

I have check with the teams and timers (either 16 or 32 bits) are not
used by zephyr or u-boot

>
> Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 5/5] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

2017-10-18 15:21 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Wed, Oct 18, 2017 at 7:58 AM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> 16 bits timers aren't accurate enough to be used as
>> clocksource, remove them from stm32f4 and stm32f7 devicetree.
>
> They aren't useful for anything? Zephyr? u-boot?

I have check with the teams and timers (either 16 or 32 bits) are not
used by zephyr or u-boot

>
> Rob

Re: [PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> Convert driver to use timer_of helpers. This allow to remove
> custom proprietary structure.
> 
> Increase min delta value because if it is too small it could
> generate too much interrupts and the system will not be able
> to catch them all.

This does two completely independent changes at once. What the heck has
increasing min delta to do with converting it to timer_of() helpers?

Nothing at all. So please split this into two distinct patches. Each doing
ONE thing.

See Documentation/process/submitting-patches.rst:

  3) Separate your changes
  ------------------------

  Separate each **logical change** into a separate patch.

Reading, understanding and complying with that document is not optional.

Thanks,

	tglx

[PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> Convert driver to use timer_of helpers. This allow to remove
> custom proprietary structure.
> 
> Increase min delta value because if it is too small it could
> generate too much interrupts and the system will not be able
> to catch them all.

This does two completely independent changes at once. What the heck has
increasing min delta to do with converting it to timer_of() helpers?

Nothing at all. So please split this into two distinct patches. Each doing
ONE thing.

See Documentation/process/submitting-patches.rst:

  3) Separate your changes
  ------------------------

  Separate each **logical change** into a separate patch.

Reading, understanding and complying with that document is not optional.

Thanks,

	tglx

Re: [PATCH v6 3/5] clocksource: stm32: only use 32 bits timers

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> 16 bits hardware are not enough accure to be used.

That should probably read:

  16 bit counters are not accurate enough to be used.

Right?

> Do no allow them to be probed by tested max counter value.

I really do not understand why you want to do this. Accuracy is not really
a reason to dismiss working hardware right away. What you can do is to
lower the rating so other eventually available hardware will be preferred.

I might be missing something, but if so this is obviously missing in the
changelog.

Thanks,

	tglx

[PATCH v6 3/5] clocksource: stm32: only use 32 bits timers

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> 16 bits hardware are not enough accure to be used.

That should probably read:

  16 bit counters are not accurate enough to be used.

Right?

> Do no allow them to be probed by tested max counter value.

I really do not understand why you want to do this. Accuracy is not really
a reason to dismiss working hardware right away. What you can do is to
lower the rating so other eventually available hardware will be preferred.

I might be missing something, but if so this is obviously missing in the
changelog.

Thanks,

	tglx

Re: [PATCH v6 4/5] clocksource: stm32: add clocksource support

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> Rework driver code to be able to implement clocksource and clockevent
> on the same hardware block.
> Before this patch only the counter of the hardware block was used to
> generate clock events. Now counter will be used to provide a 32 bits
> clock source and a comparator will provide clock events.

Again. Read, understand and comply with the patch submission
documentation. Proper changelogs are not optional.

"Before this patch ...." is bogus because it suggests that the patch is
already applied which is obviously not the case.

Let me give you an example.

  The stm32 timer hardware is currently only used as a clock event device,
  but it can be utilized as a clocksource as well.

  Implement this by enabling the free running counter in the hardware block
  and converting the clock event part from a count down event timer to a
  comparator based timer.

Can you see the difference?

> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
> +static int stm32_clock_event_set_next_event(unsigned long evt,
> +					    struct clock_event_device *clkevt)
>  {
> -	struct timer_of *to = to_timer_of(evt);
> +	struct timer_of *to = to_timer_of(clkevt);
> +	unsigned long cnt;
>  
> -	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
> -	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
> +	cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
> +	writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
> +	writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);

This implementation is doomed. You cannot rely on the assumption that the
read/modify/write sequence is 'atomic'.

Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
enough so that the write comes too late which means that you have to wait
for a full wraparound of the counter for the next interrupt.

See the big fat comment in hpet_next_event() for gory details of issues
caused by comparator based timers.

Your change of min delay in one of the previous patches is papering over
this problem and I really wonder if your argumentation of 'required because
the CPU can't keep up otherwise' is just wrong and you failed to decode the
RMW issue proper.

Though fact is, that your implementation CANNOT cover all potential RMW
disturbances safely. You need a proper safety net for these cases.

To be honest. I prefer having a slow, inaccurate down counting timer over a
fast comparator based one any time as long as the comparator is not
cleverly implemented and can do less than equal comparisons which take the
wraparound of the counter into account. It's not rocket science to do that,
it just takes a few more gates, but hardware people can't be bothered to
think about the consequences of their cheap implementations ever.

Thanks,

	tglx

Re: [PATCH v6 4/5] clocksource: stm32: add clocksource support

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> Rework driver code to be able to implement clocksource and clockevent
> on the same hardware block.
> Before this patch only the counter of the hardware block was used to
> generate clock events. Now counter will be used to provide a 32 bits
> clock source and a comparator will provide clock events.

Again. Read, understand and comply with the patch submission
documentation. Proper changelogs are not optional.

"Before this patch ...." is bogus because it suggests that the patch is
already applied which is obviously not the case.

Let me give you an example.

  The stm32 timer hardware is currently only used as a clock event device,
  but it can be utilized as a clocksource as well.

  Implement this by enabling the free running counter in the hardware block
  and converting the clock event part from a count down event timer to a
  comparator based timer.

Can you see the difference?

> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
> +static int stm32_clock_event_set_next_event(unsigned long evt,
> +					    struct clock_event_device *clkevt)
>  {
> -	struct timer_of *to = to_timer_of(evt);
> +	struct timer_of *to = to_timer_of(clkevt);
> +	unsigned long cnt;
>  
> -	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
> -	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
> +	cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
> +	writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
> +	writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);

This implementation is doomed. You cannot rely on the assumption that the
read/modify/write sequence is 'atomic'.

Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
enough so that the write comes too late which means that you have to wait
for a full wraparound of the counter for the next interrupt.

See the big fat comment in hpet_next_event() for gory details of issues
caused by comparator based timers.

Your change of min delay in one of the previous patches is papering over
this problem and I really wonder if your argumentation of 'required because
the CPU can't keep up otherwise' is just wrong and you failed to decode the
RMW issue proper.

Though fact is, that your implementation CANNOT cover all potential RMW
disturbances safely. You need a proper safety net for these cases.

To be honest. I prefer having a slow, inaccurate down counting timer over a
fast comparator based one any time as long as the comparator is not
cleverly implemented and can do less than equal comparisons which take the
wraparound of the counter into account. It's not rocket science to do that,
it just takes a few more gates, but hardware people can't be bothered to
think about the consequences of their cheap implementations ever.

Thanks,

	tglx


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 4/5] clocksource: stm32: add clocksource support

[Thomas Gleixner]

On Wed, 18 Oct 2017, Benjamin Gaignard wrote:

> Rework driver code to be able to implement clocksource and clockevent
> on the same hardware block.
> Before this patch only the counter of the hardware block was used to
> generate clock events. Now counter will be used to provide a 32 bits
> clock source and a comparator will provide clock events.

Again. Read, understand and comply with the patch submission
documentation. Proper changelogs are not optional.

"Before this patch ...." is bogus because it suggests that the patch is
already applied which is obviously not the case.

Let me give you an example.

  The stm32 timer hardware is currently only used as a clock event device,
  but it can be utilized as a clocksource as well.

  Implement this by enabling the free running counter in the hardware block
  and converting the clock event part from a count down event timer to a
  comparator based timer.

Can you see the difference?

> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
> +static int stm32_clock_event_set_next_event(unsigned long evt,
> +					    struct clock_event_device *clkevt)
>  {
> -	struct timer_of *to = to_timer_of(evt);
> +	struct timer_of *to = to_timer_of(clkevt);
> +	unsigned long cnt;
>  
> -	writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
> -	writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
> +	cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
> +	writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
> +	writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);

This implementation is doomed. You cannot rely on the assumption that the
read/modify/write sequence is 'atomic'.

Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
enough so that the write comes too late which means that you have to wait
for a full wraparound of the counter for the next interrupt.

See the big fat comment in hpet_next_event() for gory details of issues
caused by comparator based timers.

Your change of min delay in one of the previous patches is papering over
this problem and I really wonder if your argumentation of 'required because
the CPU can't keep up otherwise' is just wrong and you failed to decode the
RMW issue proper.

Though fact is, that your implementation CANNOT cover all potential RMW
disturbances safely. You need a proper safety net for these cases.

To be honest. I prefer having a slow, inaccurate down counting timer over a
fast comparator based one any time as long as the comparator is not
cleverly implemented and can do less than equal comparisons which take the
wraparound of the counter into account. It's not rocket science to do that,
it just takes a few more gates, but hardware people can't be bothered to
think about the consequences of their cheap implementations ever.

Thanks,

	tglx

Re: [PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

2017-10-18 20:31 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>
>> Convert driver to use timer_of helpers. This allow to remove
>> custom proprietary structure.
>>
>> Increase min delta value because if it is too small it could
>> generate too much interrupts and the system will not be able
>> to catch them all.
>
> This does two completely independent changes at once. What the heck has
> increasing min delta to do with converting it to timer_of() helpers?
>
> Nothing at all. So please split this into two distinct patches. Each doing
> ONE thing.

I will do that.

>
> See Documentation/process/submitting-patches.rst:
>
>   3) Separate your changes
>   ------------------------
>
>   Separate each **logical change** into a separate patch.
>
> Reading, understanding and complying with that document is not optional.
>
> Thanks,
>
>         tglx



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

[PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

2017-10-18 20:31 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>
>> Convert driver to use timer_of helpers. This allow to remove
>> custom proprietary structure.
>>
>> Increase min delta value because if it is too small it could
>> generate too much interrupts and the system will not be able
>> to catch them all.
>
> This does two completely independent changes at once. What the heck has
> increasing min delta to do with converting it to timer_of() helpers?
>
> Nothing at all. So please split this into two distinct patches. Each doing
> ONE thing.

I will do that.

>
> See Documentation/process/submitting-patches.rst:
>
>   3) Separate your changes
>   ------------------------
>
>   Separate each **logical change** into a separate patch.
>
> Reading, understanding and complying with that document is not optional.
>
> Thanks,
>
>         tglx



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org ? Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 4/5] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

2017-10-18 20:59 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>
>> Rework driver code to be able to implement clocksource and clockevent
>> on the same hardware block.
>> Before this patch only the counter of the hardware block was used to
>> generate clock events. Now counter will be used to provide a 32 bits
>> clock source and a comparator will provide clock events.
>
> Again. Read, understand and comply with the patch submission
> documentation. Proper changelogs are not optional.
>
> "Before this patch ...." is bogus because it suggests that the patch is
> already applied which is obviously not the case.
>
> Let me give you an example.
>
>   The stm32 timer hardware is currently only used as a clock event device,
>   but it can be utilized as a clocksource as well.
>
>   Implement this by enabling the free running counter in the hardware block
>   and converting the clock event part from a count down event timer to a
>   comparator based timer.
>
> Can you see the difference?

I will rework the commit message

>
>> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
>> +static int stm32_clock_event_set_next_event(unsigned long evt,
>> +                                         struct clock_event_device *clkevt)
>>  {
>> -     struct timer_of *to = to_timer_of(evt);
>> +     struct timer_of *to = to_timer_of(clkevt);
>> +     unsigned long cnt;
>>
>> -     writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
>> -     writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
>> +     cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
>> +     writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
>> +     writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
>
> This implementation is doomed. You cannot rely on the assumption that the
> read/modify/write sequence is 'atomic'.
>
> Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
> enough so that the write comes too late which means that you have to wait
> for a full wraparound of the counter for the next interrupt.
>
> See the big fat comment in hpet_next_event() for gory details of issues
> caused by comparator based timers.

Other drivers like prima2 have the same problem.
They have solve it by checking if the current time is still below next event
time after wirting it, so the function will be like that:

unsigned long now, next;

next = readl_relaxed(timer_of_base(to) + TIM_CNT) + evt;
writel_relaxed(next, timer_of_base(to) + TIM_CCR1);
writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
now = readl_relaxed(timer_of_base(to) + TIM_CNT);

return next - now > delta ? -ETIME : 0;

>
> Your change of min delay in one of the previous patches is papering over
> this problem and I really wonder if your argumentation of 'required because
> the CPU can't keep up otherwise' is just wrong and you failed to decode the
> RMW issue proper.

The  CPU is a CortexM4 @ 200MHZ and the clocks driving the timers are at 90MHZ
with a min delta at 1 you could have an interrupt each 0.01 ms which
is really to much.
By increase it to 0x60 it give time to CPU to handle the interrupt.

Also want to remove 16 bits counters because the maximum period is around 750 ms
which is a short period for a clocksource.
With 32 bits this period is close 47 secondes.

>
> Though fact is, that your implementation CANNOT cover all potential RMW
> disturbances safely. You need a proper safety net for these cases.
>
> To be honest. I prefer having a slow, inaccurate down counting timer over a
> fast comparator based one any time as long as the comparator is not
> cleverly implemented and can do less than equal comparisons which take the
> wraparound of the counter into account. It's not rocket science to do that,
> it just takes a few more gates, but hardware people can't be bothered to
> think about the consequences of their cheap implementations ever.

I will forward you point of to the hardware designer but I will have to deal the
hardware I have anyway.

Benjamin
>
> Thanks,
>
>         tglx
>
>

[PATCH v6 4/5] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

2017-10-18 20:59 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>
>> Rework driver code to be able to implement clocksource and clockevent
>> on the same hardware block.
>> Before this patch only the counter of the hardware block was used to
>> generate clock events. Now counter will be used to provide a 32 bits
>> clock source and a comparator will provide clock events.
>
> Again. Read, understand and comply with the patch submission
> documentation. Proper changelogs are not optional.
>
> "Before this patch ...." is bogus because it suggests that the patch is
> already applied which is obviously not the case.
>
> Let me give you an example.
>
>   The stm32 timer hardware is currently only used as a clock event device,
>   but it can be utilized as a clocksource as well.
>
>   Implement this by enabling the free running counter in the hardware block
>   and converting the clock event part from a count down event timer to a
>   comparator based timer.
>
> Can you see the difference?

I will rework the commit message

>
>> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
>> +static int stm32_clock_event_set_next_event(unsigned long evt,
>> +                                         struct clock_event_device *clkevt)
>>  {
>> -     struct timer_of *to = to_timer_of(evt);
>> +     struct timer_of *to = to_timer_of(clkevt);
>> +     unsigned long cnt;
>>
>> -     writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
>> -     writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
>> +     cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
>> +     writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
>> +     writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
>
> This implementation is doomed. You cannot rely on the assumption that the
> read/modify/write sequence is 'atomic'.
>
> Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
> enough so that the write comes too late which means that you have to wait
> for a full wraparound of the counter for the next interrupt.
>
> See the big fat comment in hpet_next_event() for gory details of issues
> caused by comparator based timers.

Other drivers like prima2 have the same problem.
They have solve it by checking if the current time is still below next event
time after wirting it, so the function will be like that:

unsigned long now, next;

next = readl_relaxed(timer_of_base(to) + TIM_CNT) + evt;
writel_relaxed(next, timer_of_base(to) + TIM_CCR1);
writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
now = readl_relaxed(timer_of_base(to) + TIM_CNT);

return next - now > delta ? -ETIME : 0;

>
> Your change of min delay in one of the previous patches is papering over
> this problem and I really wonder if your argumentation of 'required because
> the CPU can't keep up otherwise' is just wrong and you failed to decode the
> RMW issue proper.

The  CPU is a CortexM4 @ 200MHZ and the clocks driving the timers are at 90MHZ
with a min delta at 1 you could have an interrupt each 0.01 ms which
is really to much.
By increase it to 0x60 it give time to CPU to handle the interrupt.

Also want to remove 16 bits counters because the maximum period is around 750 ms
which is a short period for a clocksource.
With 32 bits this period is close 47 secondes.

>
> Though fact is, that your implementation CANNOT cover all potential RMW
> disturbances safely. You need a proper safety net for these cases.
>
> To be honest. I prefer having a slow, inaccurate down counting timer over a
> fast comparator based one any time as long as the comparator is not
> cleverly implemented and can do less than equal comparisons which take the
> wraparound of the counter into account. It's not rocket science to do that,
> it just takes a few more gates, but hardware people can't be bothered to
> think about the consequences of their cheap implementations ever.

I will forward you point of to the hardware designer but I will have to deal the
hardware I have anyway.

Benjamin
>
> Thanks,
>
>         tglx
>
>

Re: [PATCH v6 4/5] clocksource: stm32: add clocksource support

[Thomas Gleixner]

On Thu, 19 Oct 2017, Benjamin Gaignard wrote:
> 2017-10-18 20:59 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
> >> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
> >> +static int stm32_clock_event_set_next_event(unsigned long evt,
> >> +                                         struct clock_event_device *clkevt)
> >>  {
> >> -     struct timer_of *to = to_timer_of(evt);
> >> +     struct timer_of *to = to_timer_of(clkevt);
> >> +     unsigned long cnt;
> >>
> >> -     writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
> >> -     writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
> >> +     cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
> >> +     writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
> >> +     writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
> >
> > This implementation is doomed. You cannot rely on the assumption that the
> > read/modify/write sequence is 'atomic'.
> >
> > Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
> > enough so that the write comes too late which means that you have to wait
> > for a full wraparound of the counter for the next interrupt.
> >
> > See the big fat comment in hpet_next_event() for gory details of issues
> > caused by comparator based timers.
> 
> Other drivers like prima2 have the same problem.

That does not make it any better.

> > Your change of min delay in one of the previous patches is papering over
> > this problem and I really wonder if your argumentation of 'required because
> > the CPU can't keep up otherwise' is just wrong and you failed to decode the
> > RMW issue proper.
> 
> The  CPU is a CortexM4 @ 200MHZ and the clocks driving the timers are at 90MHZ
> with a min delta at 1 you could have an interrupt each 0.01 ms which
> is really to much.
> By increase it to 0x60 it give time to CPU to handle the interrupt.

Fair enough, but exactly this information wants to be in the changelog. And
still, if the hardware only supports 16 bit you still can use the clock
events part and not initialize the clocksource.

> Also want to remove 16 bits counters because the maximum period is around
> 750 ms which is a short period for a clocksource.  With 32 bits this
> period is close 47 secondes.

Again. The changelog is missing this information. You cannot expect
reviewers to crystal ball your reasonings.

> > To be honest. I prefer having a slow, inaccurate down counting timer over a
> > fast comparator based one any time as long as the comparator is not
> > cleverly implemented and can do less than equal comparisons which take the
> > wraparound of the counter into account. It's not rocket science to do that,
> > it just takes a few more gates, but hardware people can't be bothered to
> > think about the consequences of their cheap implementations ever.
> 
> I will forward you point of to the hardware designer but I will have to deal the
> hardware I have anyway.

I know that it's to late. Just wanted to mention it as a general note.

Thanks,

	tglx

Re: [PATCH v6 4/5] clocksource: stm32: add clocksource support

[Thomas Gleixner]

On Thu, 19 Oct 2017, Benjamin Gaignard wrote:
> 2017-10-18 20:59 GMT+02:00 Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>:
> >> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
> >> +static int stm32_clock_event_set_next_event(unsigned long evt,
> >> +                                         struct clock_event_device *clkevt)
> >>  {
> >> -     struct timer_of *to = to_timer_of(evt);
> >> +     struct timer_of *to = to_timer_of(clkevt);
> >> +     unsigned long cnt;
> >>
> >> -     writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
> >> -     writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
> >> +     cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
> >> +     writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
> >> +     writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
> >
> > This implementation is doomed. You cannot rely on the assumption that the
> > read/modify/write sequence is 'atomic'.
> >
> > Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
> > enough so that the write comes too late which means that you have to wait
> > for a full wraparound of the counter for the next interrupt.
> >
> > See the big fat comment in hpet_next_event() for gory details of issues
> > caused by comparator based timers.
> 
> Other drivers like prima2 have the same problem.

That does not make it any better.

> > Your change of min delay in one of the previous patches is papering over
> > this problem and I really wonder if your argumentation of 'required because
> > the CPU can't keep up otherwise' is just wrong and you failed to decode the
> > RMW issue proper.
> 
> The  CPU is a CortexM4 @ 200MHZ and the clocks driving the timers are at 90MHZ
> with a min delta at 1 you could have an interrupt each 0.01 ms which
> is really to much.
> By increase it to 0x60 it give time to CPU to handle the interrupt.

Fair enough, but exactly this information wants to be in the changelog. And
still, if the hardware only supports 16 bit you still can use the clock
events part and not initialize the clocksource.

> Also want to remove 16 bits counters because the maximum period is around
> 750 ms which is a short period for a clocksource.  With 32 bits this
> period is close 47 secondes.

Again. The changelog is missing this information. You cannot expect
reviewers to crystal ball your reasonings.

> > To be honest. I prefer having a slow, inaccurate down counting timer over a
> > fast comparator based one any time as long as the comparator is not
> > cleverly implemented and can do less than equal comparisons which take the
> > wraparound of the counter into account. It's not rocket science to do that,
> > it just takes a few more gates, but hardware people can't be bothered to
> > think about the consequences of their cheap implementations ever.
> 
> I will forward you point of to the hardware designer but I will have to deal the
> hardware I have anyway.

I know that it's to late. Just wanted to mention it as a general note.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 4/5] clocksource: stm32: add clocksource support

[Thomas Gleixner]

On Thu, 19 Oct 2017, Benjamin Gaignard wrote:
> 2017-10-18 20:59 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
> >> -static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
> >> +static int stm32_clock_event_set_next_event(unsigned long evt,
> >> +                                         struct clock_event_device *clkevt)
> >>  {
> >> -     struct timer_of *to = to_timer_of(evt);
> >> +     struct timer_of *to = to_timer_of(clkevt);
> >> +     unsigned long cnt;
> >>
> >> -     writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
> >> -     writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
> >> +     cnt = readl_relaxed(timer_of_base(to) + TIM_CNT);
> >> +     writel_relaxed(cnt + evt, timer_of_base(to) + TIM_CCR1);
> >> +     writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
> >
> > This implementation is doomed. You cannot rely on the assumption that the
> > read/modify/write sequence is 'atomic'.
> >
> > Bus/pipeline delays, FIQs, hypervisor exits and whatever can delay it
> > enough so that the write comes too late which means that you have to wait
> > for a full wraparound of the counter for the next interrupt.
> >
> > See the big fat comment in hpet_next_event() for gory details of issues
> > caused by comparator based timers.
> 
> Other drivers like prima2 have the same problem.

That does not make it any better.

> > Your change of min delay in one of the previous patches is papering over
> > this problem and I really wonder if your argumentation of 'required because
> > the CPU can't keep up otherwise' is just wrong and you failed to decode the
> > RMW issue proper.
> 
> The  CPU is a CortexM4 @ 200MHZ and the clocks driving the timers are at 90MHZ
> with a min delta at 1 you could have an interrupt each 0.01 ms which
> is really to much.
> By increase it to 0x60 it give time to CPU to handle the interrupt.

Fair enough, but exactly this information wants to be in the changelog. And
still, if the hardware only supports 16 bit you still can use the clock
events part and not initialize the clocksource.

> Also want to remove 16 bits counters because the maximum period is around
> 750 ms which is a short period for a clocksource.  With 32 bits this
> period is close 47 secondes.

Again. The changelog is missing this information. You cannot expect
reviewers to crystal ball your reasonings.

> > To be honest. I prefer having a slow, inaccurate down counting timer over a
> > fast comparator based one any time as long as the comparator is not
> > cleverly implemented and can do less than equal comparisons which take the
> > wraparound of the counter into account. It's not rocket science to do that,
> > it just takes a few more gates, but hardware people can't be bothered to
> > think about the consequences of their cheap implementations ever.
> 
> I will forward you point of to the hardware designer but I will have to deal the
> hardware I have anyway.

I know that it's to late. Just wanted to mention it as a general note.

Thanks,

	tglx

Re: [PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Daniel Lezcano]

On 18/10/2017 21:32, Benjamin Gaignard wrote:
> 2017-10-18 20:31 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
>> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>>
>>> Convert driver to use timer_of helpers. This allow to remove
>>> custom proprietary structure.
>>>
>>> Increase min delta value because if it is too small it could
>>> generate too much interrupts and the system will not be able
>>> to catch them all.
>>
>> This does two completely independent changes at once. What the heck has
>> increasing min delta to do with converting it to timer_of() helpers?
>>
>> Nothing at all. So please split this into two distinct patches. Each doing
>> ONE thing.
> 
> I will do that.

With push ups.


-- 
 <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

Re: [PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Daniel Lezcano]

On 18/10/2017 21:32, Benjamin Gaignard wrote:
> 2017-10-18 20:31 GMT+02:00 Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>:
>> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>>
>>> Convert driver to use timer_of helpers. This allow to remove
>>> custom proprietary structure.
>>>
>>> Increase min delta value because if it is too small it could
>>> generate too much interrupts and the system will not be able
>>> to catch them all.
>>
>> This does two completely independent changes at once. What the heck has
>> increasing min delta to do with converting it to timer_of() helpers?
>>
>> Nothing at all. So please split this into two distinct patches. Each doing
>> ONE thing.
> 
> I will do that.

With push ups.


-- 
 <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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 2/5] clocksource: stm32: convert driver to timer_of

[Daniel Lezcano]

On 18/10/2017 21:32, Benjamin Gaignard wrote:
> 2017-10-18 20:31 GMT+02:00 Thomas Gleixner <tglx@linutronix.de>:
>> On Wed, 18 Oct 2017, Benjamin Gaignard wrote:
>>
>>> Convert driver to use timer_of helpers. This allow to remove
>>> custom proprietary structure.
>>>
>>> Increase min delta value because if it is too small it could
>>> generate too much interrupts and the system will not be able
>>> to catch them all.
>>
>> This does two completely independent changes at once. What the heck has
>> increasing min delta to do with converting it to timer_of() helpers?
>>
>> Nothing at all. So please split this into two distinct patches. Each doing
>> ONE thing.
> 
> I will do that.

With push ups.


-- 
 <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