[PATCH 0/4] stm32 clocksource driver rework

[PATCH 0/4] stm32 clocksource driver rework

[Benjamin Gaignard]

version 10:
 - do not remove 16 bits timer support
 - use prescaler to get a 10KHz clock

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 (4):
  clocksource: stm32: convert driver to timer_of
  clocksource: stm32: use prescaler to adjust the resolution
  clocksource: stm32: add clocksource support
  clocksource: stm32: Update license and copyright

 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-stm32.c | 258 +++++++++++++++++++++-----------------
 2 files changed, 146 insertions(+), 113 deletions(-)

-- 
2.15.0

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

[Benjamin Gaignard]

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

Given counter number of bits (16 or 32) set a different
prescaler value and adjust timer_of rate, period and rating.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-stm32.c | 179 +++++++++++++++-----------------------
 2 files changed, 72 insertions(+), 108 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..23a321cca45b 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -16,6 +16,9 @@
 #include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
+#include <linux/slab.h>
+
+#include "timer-of.h"
 
 #define TIM_CR1		0x00
 #define TIM_DIER	0x0c
@@ -34,157 +37,117 @@
 
 #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, timer_of_base(to) + TIM_CR1);
 
-	writel_relaxed(0, base + 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 void __init stm32_clockevent_init(struct timer_of *to)
 {
-	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;
-	}
+	unsigned long max_delta;
+	int prescaler;
 
-	rate = clk_get_rate(clk);
-
-	rstc = of_reset_control_get(np, 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;
-	}
+	to->clkevt.name = "stm32_clockevent";
+	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;
 
 	/* 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;
+		to->clkevt.rating = 250;
 	} else {
 		prescaler = 1024;
-		bits = 16;
+		to->clkevt.rating = 50;
 	}
-	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);
+	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);
+
+	/* adjust rate and period given the prescaler value */
+	to->of_clk.rate = DIV_ROUND_CLOSEST(to->of_clk.rate, prescaler);
+	to->of_clk.period = DIV_ROUND_UP(to->of_clk.rate, HZ);
+
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_rate(to), 0x1, max_delta);
+}
+
+static int __init stm32_timer_init(struct device_node *node)
+{
+	struct reset_control *rstc;
+	struct timer_of *to;
+	int ret;
+
+	to = kzalloc(sizeof(*to), GFP_KERNEL);
+	if (!to)
+		return -ENOMEM;
 
-	data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
+	to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
+	to->of_irq.handler = stm32_clock_event_handler;
 
-	clockevents_config_and_register(&data->evtdev,
-					DIV_ROUND_CLOSEST(rate, prescaler),
-					0x1, max_delta);
+	ret = timer_of_init(node, to);
+	if (ret)
+		goto err;
 
-	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;
+	rstc = of_reset_control_get(node, NULL);
+	if (!IS_ERR(rstc)) {
+		reset_control_assert(rstc);
+		reset_control_deassert(rstc);
 	}
 
-	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			np, bits);
+	stm32_clockevent_init(to);
 
-	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;
 }
 
-TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_clockevent_init);
+TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_timer_init);
-- 
2.15.0

[PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution

[Benjamin Gaignard]

Rather than use fixed prescaler values compute it to get a clock
as close as possible of 10KHz and a resolution of 0.1ms.

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

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 23a321cca45b..de721d318065 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -37,6 +37,11 @@
 
 #define TIM_EGR_UG	BIT(0)
 
+#define MAX_TIM_PSC	0xFFFF
+
+/* Target a 10KHz clock to get a resolution of 0.1 ms */
+#define TARGETED_CLK_RATE 10000
+
 static int stm32_clock_event_shutdown(struct clock_event_device *evt)
 {
 	struct timer_of *to = to_timer_of(evt);
@@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 static void __init stm32_clockevent_init(struct timer_of *to)
 {
 	unsigned long max_delta;
-	int prescaler;
+	unsigned long prescaler;
 
 	to->clkevt.name = "stm32_clockevent";
 	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
@@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct timer_of *to)
 	/* 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;
+	to->clkevt.rating = 50;
+	if (max_delta == ~0U)
 		to->clkevt.rating = 250;
-	} else {
-		prescaler = 1024;
-		to->clkevt.rating = 50;
-	}
+
+	/*
+	 * Get the highest possible prescaler value to be as close
+	 * as possible of TARGETED_CLK_RATE
+	 */
+	prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);
+	if (prescaler > MAX_TIM_PSC)
+		prescaler = MAX_TIM_PSC;
 
 	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
-- 
2.15.0

[PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution

[Daniel Lezcano]

On 15/12/2017 09:52, Benjamin Gaignard wrote:
> Rather than use fixed prescaler values compute it to get a clock
> as close as possible of 10KHz and a resolution of 0.1ms.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  drivers/clocksource/timer-stm32.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
> index 23a321cca45b..de721d318065 100644
> --- a/drivers/clocksource/timer-stm32.c
> +++ b/drivers/clocksource/timer-stm32.c
> @@ -37,6 +37,11 @@
>  
>  #define TIM_EGR_UG	BIT(0)
>  
> +#define MAX_TIM_PSC	0xFFFF
> +
> +/* Target a 10KHz clock to get a resolution of 0.1 ms */
> +#define TARGETED_CLK_RATE 10000
> +
>  static int stm32_clock_event_shutdown(struct clock_event_device *evt)
>  {
>  	struct timer_of *to = to_timer_of(evt);
> @@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>  static void __init stm32_clockevent_init(struct timer_of *to)
>  {
>  	unsigned long max_delta;
> -	int prescaler;
> +	unsigned long prescaler;
>  
>  	to->clkevt.name = "stm32_clockevent";
>  	to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
> @@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct timer_of *to)
>  	/* 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;
> +	to->clkevt.rating = 50;
> +	if (max_delta == ~0U)
>  		to->clkevt.rating = 250;
> -	} else {
> -		prescaler = 1024;
> -		to->clkevt.rating = 50;
> -	}
> +
> +	/*
> +	 * Get the highest possible prescaler value to be as close
> +	 * as possible of TARGETED_CLK_RATE
> +	 */
> +	prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);

With a 90MHz or 125MHz, the prescaler will be 9000 or 12500, so much
more than the 1024 we have today for 16b, and 1 for 32b.

Shouldn't the computation be weighted with the bits width ?

Otherwise the timer will wrap like:

32bits:

before:	(2^32 / 90e6) x 1 = 47.72 seconds
after:	(2^32 / 90e6) x 9000 = 119.3 *hours* ~= 5days

16bits:

before:	(2^16 / 90e6) x 1024 = 0.745 seconds
after:	(2^16 / 90e6) x 9000 = 6.55 seconds

The patch is ok to target the 10KHz timer rate for 16b with a 1ms
resolution wrapping up after 6.55 seconds. But not for the 32bits timer.
Furthermore, we can't tell anymore the 32bits timers have a rating of
250 after this patch.

Leave the 32bits part as it is and compute the prescaler only in case of
16bits with the target rate, which sounds a reasonable approach.

> +	if (prescaler > MAX_TIM_PSC)
> +		prescaler = MAX_TIM_PSC;

That can happen only if the clock rate is greater than ~655MHz, that
could not happen today as far as I can tell regarding the DT. So if we
hit this condition, we should speak up in the log (pr_warn).

>  	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>  	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);

Can you fix this prescaler - 1 in order to be consistent with the
computation with 16b ? (32b prescaler = 0, 16b prescaler = clk_rate /
target ).

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 2/4] clocksource: stm32: use prescaler to adjust the resolution

[Benjamin Gaignard]

2017-12-18 10:26 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 15/12/2017 09:52, Benjamin Gaignard wrote:
>> Rather than use fixed prescaler values compute it to get a clock
>> as close as possible of 10KHz and a resolution of 0.1ms.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  drivers/clocksource/timer-stm32.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>> index 23a321cca45b..de721d318065 100644
>> --- a/drivers/clocksource/timer-stm32.c
>> +++ b/drivers/clocksource/timer-stm32.c
>> @@ -37,6 +37,11 @@
>>
>>  #define TIM_EGR_UG   BIT(0)
>>
>> +#define MAX_TIM_PSC  0xFFFF
>> +
>> +/* Target a 10KHz clock to get a resolution of 0.1 ms */
>> +#define TARGETED_CLK_RATE 10000
>> +
>>  static int stm32_clock_event_shutdown(struct clock_event_device *evt)
>>  {
>>       struct timer_of *to = to_timer_of(evt);
>> @@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>  static void __init stm32_clockevent_init(struct timer_of *to)
>>  {
>>       unsigned long max_delta;
>> -     int prescaler;
>> +     unsigned long prescaler;
>>
>>       to->clkevt.name = "stm32_clockevent";
>>       to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
>> @@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct timer_of *to)
>>       /* 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;
>> +     to->clkevt.rating = 50;
>> +     if (max_delta == ~0U)
>>               to->clkevt.rating = 250;
>> -     } else {
>> -             prescaler = 1024;
>> -             to->clkevt.rating = 50;
>> -     }
>> +
>> +     /*
>> +      * Get the highest possible prescaler value to be as close
>> +      * as possible of TARGETED_CLK_RATE
>> +      */
>> +     prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);
>
> With a 90MHz or 125MHz, the prescaler will be 9000 or 12500, so much
> more than the 1024 we have today for 16b, and 1 for 32b.
>
> Shouldn't the computation be weighted with the bits width ?

My goal was to get the same resolution (0.1ms) for all the timers so
the wrap will depend of the number of bits like you describe below.

>
> Otherwise the timer will wrap like:
>
> 32bits:
>
> before: (2^32 / 90e6) x 1 = 47.72 seconds
> after:  (2^32 / 90e6) x 9000 = 119.3 *hours* ~= 5days
>
> 16bits:
>
> before: (2^16 / 90e6) x 1024 = 0.745 seconds
> after:  (2^16 / 90e6) x 9000 = 6.55 seconds
>
> The patch is ok to target the 10KHz timer rate for 16b with a 1ms
> resolution wrapping up after 6.55 seconds. But not for the 32bits timer.
> Furthermore, we can't tell anymore the 32bits timers have a rating of
> 250 after this patch.

What is the link between rating and resolution (or wrap) ?
Is it a problem to get a long wrap ?

>
> Leave the 32bits part as it is and compute the prescaler only in case of
> 16bits with the target rate, which sounds a reasonable approach.
>
>> +     if (prescaler > MAX_TIM_PSC)
>> +             prescaler = MAX_TIM_PSC;
>
> That can happen only if the clock rate is greater than ~655MHz, that
> could not happen today as far as I can tell regarding the DT. So if we
> hit this condition, we should speak up in the log (pr_warn).

It is to be futur proof for next possible SoC but even if prescaler
reach this limit
it is not a problem the only consequence would be that resolution and
wrap change.

>
>>       writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>       writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>
> Can you fix this prescaler - 1 in order to be consistent with the
> computation with 16b ? (32b prescaler = 0, 16b prescaler = clk_rate /
> target ).

In the hardware the clock is divise by " TIM_PSC value  1" so to be coherent
with that I need to do prescaler -1.

Benjamin

>
> 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 2/4] clocksource: stm32: use prescaler to adjust the resolution

[Daniel Lezcano]

On 18/12/2017 10:44, Benjamin Gaignard wrote:
> 2017-12-18 10:26 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 15/12/2017 09:52, Benjamin Gaignard wrote:
>>> Rather than use fixed prescaler values compute it to get a clock
>>> as close as possible of 10KHz and a resolution of 0.1ms.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> ---
>>>  drivers/clocksource/timer-stm32.c | 23 ++++++++++++++++-------
>>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>>> index 23a321cca45b..de721d318065 100644
>>> --- a/drivers/clocksource/timer-stm32.c
>>> +++ b/drivers/clocksource/timer-stm32.c
>>> @@ -37,6 +37,11 @@
>>>
>>>  #define TIM_EGR_UG   BIT(0)
>>>
>>> +#define MAX_TIM_PSC  0xFFFF
>>> +
>>> +/* Target a 10KHz clock to get a resolution of 0.1 ms */
>>> +#define TARGETED_CLK_RATE 10000
>>> +
>>>  static int stm32_clock_event_shutdown(struct clock_event_device *evt)
>>>  {
>>>       struct timer_of *to = to_timer_of(evt);
>>> @@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>>  static void __init stm32_clockevent_init(struct timer_of *to)
>>>  {
>>>       unsigned long max_delta;
>>> -     int prescaler;
>>> +     unsigned long prescaler;
>>>
>>>       to->clkevt.name = "stm32_clockevent";
>>>       to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
>>> @@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct timer_of *to)
>>>       /* 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;
>>> +     to->clkevt.rating = 50;
>>> +     if (max_delta == ~0U)
>>>               to->clkevt.rating = 250;
>>> -     } else {
>>> -             prescaler = 1024;
>>> -             to->clkevt.rating = 50;
>>> -     }
>>> +
>>> +     /*
>>> +      * Get the highest possible prescaler value to be as close
>>> +      * as possible of TARGETED_CLK_RATE
>>> +      */
>>> +     prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);
>>
>> With a 90MHz or 125MHz, the prescaler will be 9000 or 12500, so much
>> more than the 1024 we have today for 16b, and 1 for 32b.
>>
>> Shouldn't the computation be weighted with the bits width ?
> 
> My goal was to get the same resolution (0.1ms) for all the timers so
> the wrap will depend of the number of bits like you describe below.

Do you really want 1ms resolution with a 32bits timer ?

>> Otherwise the timer will wrap like:
>>
>> 32bits:
>>
>> before: (2^32 / 90e6) x 1 = 47.72 seconds
>> after:  (2^32 / 90e6) x 9000 = 119.3 *hours* ~= 5days
>>
>> 16bits:
>>
>> before: (2^16 / 90e6) x 1024 = 0.745 seconds
>> after:  (2^16 / 90e6) x 9000 = 6.55 seconds
>>
>> The patch is ok to target the 10KHz timer rate for 16b with a 1ms
>> resolution wrapping up after 6.55 seconds. But not for the 32bits timer.
>> Furthermore, we can't tell anymore the 32bits timers have a rating of
>> 250 after this patch.
> 
> What is the link between rating and resolution (or wrap) ?

Low resolution => hardly suitable for real use case => bad rating.

>From include/linux/clocksource.h

[ ... ]

 *                      100-199: Base level usability.
 *                              Functional for real use, but not desired.
 *                      200-299: Good.
 *                              A correct and usable clocksource.

[ ... ]

If you want to set a timer with a delta of 12.345ms and the resolution
is 1ms. Then you end up with a timer expiring after 13ms.

> Is it a problem to get a long wrap ?

It is not a problem to go for a long wrap, it is usually interesting
when the CPU has deep idle states. But it is not worth to sacrifice the
resolution with the 32bits timer in order to have 5 days before wrap.

Keeping 47secs is fine for the moment. If you want a coarser grain, that
could be acceptable because the resolution is very high but we can
postpone that for later after solving this 16b / 32b thing.

>> Leave the 32bits part as it is and compute the prescaler only in case of
>> 16bits with the target rate, which sounds a reasonable approach.
>>
>>> +     if (prescaler > MAX_TIM_PSC)
>>> +             prescaler = MAX_TIM_PSC;
>>
>> That can happen only if the clock rate is greater than ~655MHz, that
>> could not happen today as far as I can tell regarding the DT. So if we
>> hit this condition, we should speak up in the log (pr_warn).
> 
> It is to be futur proof for next possible SoC but even if prescaler
> reach this limit
> it is not a problem the only consequence would be that resolution and
> wrap change.

Got that, but that needs to be logged with a pr_warn or pr_info.

>>>       writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>>       writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>>
>> Can you fix this prescaler - 1 in order to be consistent with the
>> computation with 16b ? (32b prescaler = 0, 16b prescaler = clk_rate /
>> target ).
> 
> In the hardware the clock is divise by " TIM_PSC value  1" so to be coherent
> with that I need to do prescaler -1.

Ah, ok.


-- 
 <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 2/4] clocksource: stm32: use prescaler to adjust the resolution

[Benjamin Gaignard]

2017-12-18 11:54 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 18/12/2017 10:44, Benjamin Gaignard wrote:
>> 2017-12-18 10:26 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 15/12/2017 09:52, Benjamin Gaignard wrote:
>>>> Rather than use fixed prescaler values compute it to get a clock
>>>> as close as possible of 10KHz and a resolution of 0.1ms.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>>> ---
>>>>  drivers/clocksource/timer-stm32.c | 23 ++++++++++++++++-------
>>>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>>>> index 23a321cca45b..de721d318065 100644
>>>> --- a/drivers/clocksource/timer-stm32.c
>>>> +++ b/drivers/clocksource/timer-stm32.c
>>>> @@ -37,6 +37,11 @@
>>>>
>>>>  #define TIM_EGR_UG   BIT(0)
>>>>
>>>> +#define MAX_TIM_PSC  0xFFFF
>>>> +
>>>> +/* Target a 10KHz clock to get a resolution of 0.1 ms */
>>>> +#define TARGETED_CLK_RATE 10000
>>>> +
>>>>  static int stm32_clock_event_shutdown(struct clock_event_device *evt)
>>>>  {
>>>>       struct timer_of *to = to_timer_of(evt);
>>>> @@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>>>  static void __init stm32_clockevent_init(struct timer_of *to)
>>>>  {
>>>>       unsigned long max_delta;
>>>> -     int prescaler;
>>>> +     unsigned long prescaler;
>>>>
>>>>       to->clkevt.name = "stm32_clockevent";
>>>>       to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
>>>> @@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct timer_of *to)
>>>>       /* 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;
>>>> +     to->clkevt.rating = 50;
>>>> +     if (max_delta == ~0U)
>>>>               to->clkevt.rating = 250;
>>>> -     } else {
>>>> -             prescaler = 1024;
>>>> -             to->clkevt.rating = 50;
>>>> -     }
>>>> +
>>>> +     /*
>>>> +      * Get the highest possible prescaler value to be as close
>>>> +      * as possible of TARGETED_CLK_RATE
>>>> +      */
>>>> +     prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);
>>>
>>> With a 90MHz or 125MHz, the prescaler will be 9000 or 12500, so much
>>> more than the 1024 we have today for 16b, and 1 for 32b.
>>>
>>> Shouldn't the computation be weighted with the bits width ?
>>
>> My goal was to get the same resolution (0.1ms) for all the timers so
>> the wrap will depend of the number of bits like you describe below.
>
> Do you really want 1ms resolution with a 32bits timer ?

I want a resolution of 0.1 ms (TARGETED_CLK_RATE = 10.000)
for all the timers or 0.01ms if you think is better.

>
>>> Otherwise the timer will wrap like:
>>>
>>> 32bits:
>>>
>>> before: (2^32 / 90e6) x 1 = 47.72 seconds
>>> after:  (2^32 / 90e6) x 9000 = 119.3 *hours* ~= 5days
>>>
>>> 16bits:
>>>
>>> before: (2^16 / 90e6) x 1024 = 0.745 seconds
>>> after:  (2^16 / 90e6) x 9000 = 6.55 seconds
>>>
>>> The patch is ok to target the 10KHz timer rate for 16b with a 1ms
>>> resolution wrapping up after 6.55 seconds. But not for the 32bits timer.
>>> Furthermore, we can't tell anymore the 32bits timers have a rating of
>>> 250 after this patch.
>>
>> What is the link between rating and resolution (or wrap) ?
>
> Low resolution => hardly suitable for real use case => bad rating.
>
> From include/linux/clocksource.h
>
> [ ... ]
>
>  *                      100-199: Base level usability.
>  *                              Functional for real use, but not desired.
>  *                      200-299: Good.
>  *                              A correct and usable clocksource.
>
> [ ... ]
>
> If you want to set a timer with a delta of 12.345ms and the resolution
> is 1ms. Then you end up with a timer expiring after 13ms.
>
>> Is it a problem to get a long wrap ?
>
> It is not a problem to go for a long wrap, it is usually interesting
> when the CPU has deep idle states. But it is not worth to sacrifice the
> resolution with the 32bits timer in order to have 5 days before wrap.
>
> Keeping 47secs is fine for the moment. If you want a coarser grain, that
> could be acceptable because the resolution is very high but we can
> postpone that for later after solving this 16b / 32b thing.

When the resolution is too high I have issues with min delta value because
CPU can handle interrupt each 11ns.

>
>>> Leave the 32bits part as it is and compute the prescaler only in case of
>>> 16bits with the target rate, which sounds a reasonable approach.
>>>
>>>> +     if (prescaler > MAX_TIM_PSC)
>>>> +             prescaler = MAX_TIM_PSC;
>>>
>>> That can happen only if the clock rate is greater than ~655MHz, that
>>> could not happen today as far as I can tell regarding the DT. So if we
>>> hit this condition, we should speak up in the log (pr_warn).
>>
>> It is to be futur proof for next possible SoC but even if prescaler
>> reach this limit
>> it is not a problem the only consequence would be that resolution and
>> wrap change.
>
> Got that, but that needs to be logged with a pr_warn or pr_info.

OK

>
>>>>       writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>>>       writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>>>
>>> Can you fix this prescaler - 1 in order to be consistent with the
>>> computation with 16b ? (32b prescaler = 0, 16b prescaler = clk_rate /
>>> target ).
>>
>> In the hardware the clock is divise by " TIM_PSC value  1" so to be coherent
>> with that I need to do prescaler -1.
>
> Ah, ok.
>
>
> --
>  <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 3/4] clocksource: stm32: add clocksource support

[Benjamin Gaignard]

The stm32 timer hardware is currently only used as a clock event device,
but it can be used 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@st.com>
---
 drivers/clocksource/timer-stm32.c | 114 ++++++++++++++++++++++++++++----------
 1 file changed, 86 insertions(+), 28 deletions(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index de721d318065..38eb59bb7f8a 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -16,6 +16,7 @@
 #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"
@@ -24,17 +25,15 @@
 #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)
 
 #define MAX_TIM_PSC	0xFFFF
@@ -46,29 +45,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;
+
+	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(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(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;
 }
@@ -80,6 +94,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;
@@ -88,22 +107,46 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 static void __init stm32_clockevent_init(struct timer_of *to)
 {
 	unsigned long max_delta;
-	unsigned long prescaler;
 
 	to->clkevt.name = "stm32_clockevent";
-	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;
 
 	/* Detect whether the timer is 16 or 32 bits */
+	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
+
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_rate(to), 0x1, max_delta);
+}
+
+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)
+{
+	unsigned long max_delta, prescaler;
+	int bits = 16;
+
 	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+
+	/* Detect whether the timer is 16 or 32 bits */
 	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
+
 	to->clkevt.rating = 50;
-	if (max_delta == ~0U)
+	if (max_delta == ~0U) {
+		bits = 32;
 		to->clkevt.rating = 250;
+	}
 
 	/*
 	 * Get the highest possible prescaler value to be as close
@@ -113,18 +156,27 @@ static void __init stm32_clockevent_init(struct timer_of *to)
 	if (prescaler > MAX_TIM_PSC)
 		prescaler = MAX_TIM_PSC;
 
-	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 	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);
 
-	/* adjust rate and period given the prescaler value */
+	/* Adjust rate and period given the prescaler value */
 	to->of_clk.rate = DIV_ROUND_CLOSEST(to->of_clk.rate, prescaler);
 	to->of_clk.period = DIV_ROUND_UP(to->of_clk.rate, HZ);
 
-	clockevents_config_and_register(&to->clkevt,
-					timer_of_rate(to), 0x1, max_delta);
+	/* 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, bits, timer_of_rate(to));
+
+	return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
+				     timer_of_rate(to),
+				     to->clkevt.rating,
+				     bits,
+				     clocksource_mmio_readl_up);
 }
 
 static int __init stm32_timer_init(struct device_node *node)
@@ -150,10 +202,16 @@ static int __init stm32_timer_init(struct device_node *node)
 		reset_control_deassert(rstc);
 	}
 
+	ret = stm32_clocksource_init(to);
+	if (ret)
+		goto deinit;
+
 	stm32_clockevent_init(to);
 
 	return 0;
 
+deinit:
+	timer_of_cleanup(to);
 err:
 	kfree(to);
 	return ret;
-- 
2.15.0

[PATCH 4/4] 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 38eb59bb7f8a..83603d4b5706 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