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
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 <[email protected]>
---
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
Adopt SPDX License Identifier and add STMicroelectronics
copyright
Signed-off-by: Benjamin Gaignard <[email protected]>
---
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 <[email protected]>
- * License terms: GNU General Public License (GPL), version 2
+ * Author: Benjamin Gaignard <[email protected]> for STMicroelectronics.
*
* Inspired by time-efm32.c from Uwe Kleine-Koenig
*/
--
2.15.0
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
2017-12-18 10:26 GMT+01:00 Daniel Lezcano <[email protected]>:
> 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 <[email protected]>
>> ---
>> 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
>
On 18/12/2017 10:44, Benjamin Gaignard wrote:
> 2017-12-18 10:26 GMT+01:00 Daniel Lezcano <[email protected]>:
>> 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 <[email protected]>
>>> ---
>>> 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
2017-12-18 11:54 GMT+01:00 Daniel Lezcano <[email protected]>:
> On 18/12/2017 10:44, Benjamin Gaignard wrote:
>> 2017-12-18 10:26 GMT+01:00 Daniel Lezcano <[email protected]>:
>>> 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 <[email protected]>
>>>> ---
>>>> 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