[PATCH v9 0/6] stm32 clocksource driver rework

[PATCH v9 0/6] stm32 clocksource driver rework

[Benjamin Gaignard]

version 9:
 - rebased on timer/master where timer_of_cleanup() exist
 - fix the comments done by Daniel on v8
 - reword commits message to be more explicit about 16 bits counter issue
 - add one patch about copyrights and licences
   
version 8:
 - rebased on timers/core
 - change timer_of_exit() name to timer_of_cleanup()
 - update stm32 clocksource driver to use this function

version 7:
 - reword "clocksource: stm32: only use 32 bits timers" commit message
   to give more details about why 16 bits are problematics.

version 6:
 - add dedicated patch min delta change
 - rework commit messages, I hope it will be better now
 - change new function name from timer_of_deinit to timer_of_exit
 - make stm32_clock_event_set_next_event() safer like done in other
   drivers

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

Benjamin Gaignard (6):
  clocksource: stm32: convert driver to timer_of
  clocksource: stm32: increase min delta value
  clocksource: stm32: only use 32 bits timers
  clocksource: stm32: add clocksource support
  clocksource: stm32: Update license and copyright
  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-stm32.c | 253 +++++++++++++++++++++-----------------
 4 files changed, 139 insertions(+), 179 deletions(-)

-- 
2.15.0

[PATCH v9 1/6] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

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

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 c729a88007d0..28bc55951512 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -269,6 +269,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 8f2423789ba9..fc61fd18a182 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), 0x1, 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.15.0

[PATCH v9 2/6] clocksource: stm32: increase min delta value

[Benjamin Gaignard]

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

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 more time (around 1 ms)
to CPU to handle the interrupt.

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

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index fc61fd18a182..a45f1f1cd040 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -36,6 +36,12 @@
 
 #define TIM_EGR_UG	BIT(0)
 
+/*
+ * The clock driving the hardware is at 90MHZ with a MIN_DELTA
+ * at 0x60 it gives around 1 ms to the CPU to handle the interrupt
+ */
+#define MIN_DELTA	0x60
+
 static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
 	struct timer_of *to = to_timer_of(evt);
@@ -129,7 +135,8 @@ static int __init stm32_clockevent_init(struct device_node *node)
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
 	clockevents_config_and_register(&to->clkevt,
-					timer_of_period(to), 0x1, max_delta);
+					timer_of_period(to),
+					MIN_DELTA, max_delta);
 
 	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
 			node, bits);
-- 
2.15.0

[PATCH v9 3/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

The clock driving counters is at 90MHz so the maximum period
for 16 bis counters is around 728us (2^16 / 90.000.000).
For 32 bits counters this period is close 47 secondes which is
more acceptable.

When using 16 bits counters the kernel may not be able to boot
because it has a too high overhead compare to the clockevent period.
Downgrading the rating of 16bits counter won't change anything
to this problem so this patch remove 16 bits counters support
and makes sure that they won't be probed anymore.

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

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index a45f1f1cd040..707808d91bf0 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -84,12 +84,16 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static bool stm32_timer_is_32bits(struct timer_of *to)
+{
+	return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
+}
+
 static int __init stm32_clockevent_init(struct device_node *node)
 {
 	struct reset_control *rstc;
-	unsigned long max_delta;
-	int ret, bits, prescaler = 1;
 	struct timer_of *to;
+	int ret;
 
 	to = kzalloc(sizeof(*to), GFP_KERNEL);
 	if (!to)
@@ -118,31 +122,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;
+	if (!stm32_timer_is_32bits(to)) {
+		pr_warn("Timer %pOF is a 16 bits timer\n", node);
+		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),
-					MIN_DELTA, max_delta);
-
-	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			node, bits);
+					MIN_DELTA, ~0U);
 
 	return 0;
 
+deinit:
+	timer_of_cleanup(to);
 err:
 	kfree(to);
 	return ret;
-- 
2.15.0

[PATCH v9 3/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 08/12/2017 12:32, Benjamin Gaignard wrote:
> From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> 
> The clock driving counters is at 90MHz so the maximum period
> for 16 bis counters is around 728us (2^16 / 90.000.000).
> For 32 bits counters this period is close 47 secondes which is
> more acceptable.
> 
> When using 16 bits counters the kernel may not be able to boot
> because it has a too high overhead compare to the clockevent period.
> Downgrading the rating of 16bits counter won't change anything
> to this problem so this patch remove 16 bits counters support
> and makes sure that they won't be probed anymore.

Benjamin,

there is an inconsistency in this description and the patchset. This is
why it is so confusing to review and understand the purpose.

Why are you preventing the clockevents to work with 16bits while the
issue is related to the clocksource you introduce in the next patch ?

Also, why are you removing the DT nodes ?

Accept to register the clocksource only if it is a 32bits timer. Let the
clockevents to register themselves and have the rating to sort out the
this. I do believe that is what Thomas asked you the first time.

You can keep the hardware description in the DT and boot gracefully with
the first 32bits timer succeeding the init.

Take the time to think about it, comment and let's reach an agreement
before you send another version, I'm tired to review again and again
these stm32 timers.

Thanks.

  -- Daniel




-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v9 3/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

2017-12-08 13:51 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 08/12/2017 12:32, Benjamin Gaignard wrote:
>> From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>> The clock driving counters is at 90MHz so the maximum period
>> for 16 bis counters is around 728us (2^16 / 90.000.000).
>> For 32 bits counters this period is close 47 secondes which is
>> more acceptable.
>>
>> When using 16 bits counters the kernel may not be able to boot
>> because it has a too high overhead compare to the clockevent period.
>> Downgrading the rating of 16bits counter won't change anything
>> to this problem so this patch remove 16 bits counters support
>> and makes sure that they won't be probed anymore.
>
> Benjamin,
>
> there is an inconsistency in this description and the patchset. This is
> why it is so confusing to review and understand the purpose.
>
> Why are you preventing the clockevents to work with 16bits while the
> issue is related to the clocksource you introduce in the next patch ?

No the issue is existing also for clockevent because the max period is
around 728us so the interrupt will fire each 728us which is really too much.

>
> Also, why are you removing the DT nodes ?
>
> Accept to register the clocksource only if it is a 32bits timer. Let the
> clockevents to register themselves and have the rating to sort out the
> this. I do believe that is what Thomas asked you the first time.
>
> You can keep the hardware description in the DT and boot gracefully with
> the first 32bits timer succeeding the init.
>
> Take the time to think about it, comment and let's reach an agreement
> before you send another version, I'm tired to review again and again
> these stm32 timers.
>
> Thanks.
>
>   -- Daniel
>
>
>
>
> --
>  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org ? Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

[PATCH v9 3/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 08/12/2017 14:05, Benjamin Gaignard wrote:
> 2017-12-08 13:51 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 08/12/2017 12:32, Benjamin Gaignard wrote:
>>> From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>>
>>> The clock driving counters is at 90MHz so the maximum period
>>> for 16 bis counters is around 728us (2^16 / 90.000.000).
>>> For 32 bits counters this period is close 47 secondes which is
>>> more acceptable.
>>>
>>> When using 16 bits counters the kernel may not be able to boot
>>> because it has a too high overhead compare to the clockevent period.
>>> Downgrading the rating of 16bits counter won't change anything
>>> to this problem so this patch remove 16 bits counters support
>>> and makes sure that they won't be probed anymore.
>>
>> Benjamin,
>>
>> there is an inconsistency in this description and the patchset. This is
>> why it is so confusing to review and understand the purpose.
>>
>> Why are you preventing the clockevents to work with 16bits while the
>> issue is related to the clocksource you introduce in the next patch ?
> 
> No the issue is existing also for clockevent because the max period is
> around 728us so the interrupt will fire each 728us which is really too much.

No, that is because you ripped out in this patch the prescaler which was
1024.

Are you the author of this series ?

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

[PATCH v9 4/6] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

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.

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

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 707808d91bf0..c9aed2314194 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)
 
@@ -46,28 +48,44 @@ 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 now, next;
 
-	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);
+	next = readl_relaxed(timer_of_base(to) + TIM_CNT) + evt;
+	writel_relaxed(next, timer_of_base(to) + TIM_CCR1);
+	now = readl_relaxed(timer_of_base(to) + TIM_CNT);
+
+	if ((next - now) > evt)
+		return -ETIME;
+
+	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);
+}
+
+static int stm32_clock_event_set_oneshot(struct clock_event_device *evt)
+{
+	struct timer_of *to = to_timer_of(evt);
+	unsigned long val;
+
+	val = readl_relaxed(timer_of_base(to) + TIM_CNT);
+	writel_relaxed(val, timer_of_base(to) + TIM_CCR1);
+	writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
 
 	return 0;
 }
@@ -79,6 +97,11 @@ 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;
@@ -89,7 +112,48 @@ static bool stm32_timer_is_32bits(struct timer_of *to)
 	return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
 }
 
-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), MIN_DELTA, ~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;
 	struct timer_of *to;
@@ -100,12 +164,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;
 
@@ -128,16 +193,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),
-					MIN_DELTA, ~0U);
+	stm32_clockevent_init(to);
 
 	return 0;
 
@@ -148,4 +208,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.15.0

[PATCH v9 4/6] clocksource: stm32: add clocksource support

[Daniel Lezcano]

On 08/12/2017 12:32, Benjamin Gaignard wrote:
> 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.


Split this patch in two:
 - periodic support
 - clocksource support




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

[PATCH v9 4/6] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

2017-12-08 14:46 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 08/12/2017 12:32, Benjamin Gaignard wrote:
>> 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.
>
>
> Split this patch in two:
>  - periodic support
>  - clocksource support

No because the way of implementing periodic support depend of how clocksource
part is implemented on the hardware.
I don't want to code functions that I will completely remove just after

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

[PATCH v9 4/6] clocksource: stm32: add clocksource support

[Daniel Lezcano]

On 08/12/2017 15:04, Benjamin Gaignard wrote:
> 2017-12-08 14:46 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 08/12/2017 12:32, Benjamin Gaignard wrote:
>>> 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.
>>
>>
>> Split this patch in two:
>>  - periodic support
>>  - clocksource support
> 
> No because the way of implementing periodic support depend of how clocksource
> part is implemented on the hardware.
> I don't want to code functions that I will completely remove just after

Sorry, I don't get it. Can you elaborate please ?


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

[PATCH v9 4/6] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

2017-12-08 15:08 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 08/12/2017 15:04, Benjamin Gaignard wrote:
>> 2017-12-08 14:46 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 08/12/2017 12:32, Benjamin Gaignard wrote:
>>>> 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.
>>>
>>>
>>> Split this patch in two:
>>>  - periodic support
>>>  - clocksource support
>>
>> No because the way of implementing periodic support depend of how clocksource
>> part is implemented on the hardware.
>> I don't want to code functions that I will completely remove just after
>
> Sorry, I don't get it. Can you elaborate please ?

To add clocksource support I use the counter part of the hardware
which was before
used for clockevent and make clockevent use the comparator instead.
So implementing periodic on counter side of the hardware is a waste of
time because
I will to remove right after

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

[PATCH v9 5/6] clocksource: stm32: Update license and copyright

[Benjamin Gaignard]

Adopt SPDX License Identifier and add STMicroelectronics
copyright

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/clocksource/timer-stm32.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index c9aed2314194..21479c3cfcb9 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -1,7 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) Maxime Coquelin 2015
+ * Copyright (C) STMicroelectronics 2017 - All Rights Reserved
  * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
- * License terms:  GNU General Public License (GPL), version 2
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
  *
  * Inspired by time-efm32.c from Uwe Kleine-Koenig
  */
-- 
2.15.0

[PATCH v9 5/6] clocksource: stm32: Update license and copyright

[Philippe Ombredanne]

On Fri, Dec 8, 2017 at 12:32 PM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> Adopt SPDX License Identifier and add STMicroelectronics
> copyright
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  drivers/clocksource/timer-stm32.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
> index c9aed2314194..21479c3cfcb9 100644
> --- a/drivers/clocksource/timer-stm32.c
> +++ b/drivers/clocksource/timer-stm32.c
> @@ -1,7 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) Maxime Coquelin 2015
> + * Copyright (C) STMicroelectronics 2017 - All Rights Reserved
>   * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
> - * License terms:  GNU General Public License (GPL), version 2
> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>   *
>   * Inspired by time-efm32.c from Uwe Kleine-Koenig
>   */
> --
> 2.15.0
>

Benjamin,

Thank you for this: every little bit of help counts towards making the
whole kernel licensing easily greppable!

FWIW:

Reviewed-by: Philippe Ombredanne <pombredanne@nexB.com>

--
Philippe

[PATCH v9 6/6] arm: dts: stm32: remove useless clocksource nodes

[Benjamin Gaignard]

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

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 10099df8b73e..b507e04a52c6 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -107,14 +107,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>;
@@ -136,14 +128,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>;
@@ -193,14 +177,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>;
@@ -217,14 +193,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 5f66d151eedb..bb3e262bd456 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -103,14 +103,6 @@
 			};
 		};
 
-		timer3: timer at 40000400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000400 0x400>;
-			interrupts = <29>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM3)>;
-			status = "disabled";
-		};
-
 		timers3: timers at 40000400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -132,14 +124,6 @@
 			};
 		};
 
-		timer4: timer at 40000800 {
-			compatible = "st,stm32-timer";
-			reg = <0x40000800 0x400>;
-			interrupts = <30>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM4)>;
-			status = "disabled";
-		};
-
 		timers4: timers at 40000800 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -189,14 +173,6 @@
 			};
 		};
 
-		timer6: timer at 40001000 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001000 0x400>;
-			interrupts = <54>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM6)>;
-			status = "disabled";
-		};
-
 		timers6: timers at 40001000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -213,14 +189,6 @@
 			};
 		};
 
-		timer7: timer at 40001400 {
-			compatible = "st,stm32-timer";
-			reg = <0x40001400 0x400>;
-			interrupts = <55>;
-			clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM7)>;
-			status = "disabled";
-		};
-
 		timers7: timers at 40001400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.15.0