[PATCH v8 0/6] stm32 clocksource driver rework

[PATCH v8 0/6] stm32 clocksource driver rework

[Benjamin Gaignard]

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: timer_of: rename timer_of_exit to timer_of_cleanup
  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
  arm: dts: stm32: remove useless clocksource nodes

 arch/arm/boot/dts/stm32f429.dtsi  |  32 -----
 arch/arm/boot/dts/stm32f746.dtsi  |  32 -----
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-of.c    |   9 +-
 drivers/clocksource/timer-of.h    |   2 +-
 drivers/clocksource/timer-stm32.c | 242 ++++++++++++++++++++------------------
 6 files changed, 138 insertions(+), 180 deletions(-)

-- 
2.7.4

[PATCH v8 1/6] clocksource: timer_of: rename timer_of_exit to timer_of_cleanup

[Benjamin Gaignard]

Change function name to something more explicit since it is only
used in init error cases.
Add __init annotation and description about the function usage.

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

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index 7c64a5c1..a319904 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -177,7 +177,14 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
 	return ret;
 }
 
-void timer_of_exit(struct timer_of *to)
+/**
+ * timer_of_cleanup - release timer_of ressources
+ * @to: timer_of structure
+ *
+ * Release the ressources that has been used in timer_of_init().
+ * This function should be called in init error cases
+ */
+void __init timer_of_cleanup(struct timer_of *to)
 {
 	if (to->flags & TIMER_OF_IRQ)
 		timer_irq_exit(&to->of_irq);
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index 44f57e0..f521477 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -67,6 +67,6 @@ static inline unsigned long timer_of_period(struct timer_of *to)
 extern int __init timer_of_init(struct device_node *np,
 				struct timer_of *to);
 
-extern void timer_of_exit(struct timer_of *to);
+extern void __init timer_of_cleanup(struct timer_of *to);
 
 #endif
-- 
2.7.4

[PATCH v8 2/6] clocksource: stm32: convert driver to timer_of

[Benjamin Gaignard]

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 c729a88..28bc5595 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 8f24237..fc61fd1 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.7.4

[PATCH v8 3/6] clocksource: stm32: increase min delta value

[Benjamin Gaignard]

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index fc61fd1..ae41a19 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -36,6 +36,8 @@
 
 #define TIM_EGR_UG	BIT(0)
 
+#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 +131,7 @@ 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.7.4

[PATCH v8 3/6] clocksource: stm32: increase min delta value

[Daniel Lezcano]

On 14/11/2017 09:52, Benjamin Gaignard wrote:
> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
> index fc61fd1..ae41a19 100644
> --- a/drivers/clocksource/timer-stm32.c
> +++ b/drivers/clocksource/timer-stm32.c
> @@ -36,6 +36,8 @@
>  
>  #define TIM_EGR_UG	BIT(0)
>  
> +#define MIN_DELTA	0x60

Explain why 0x60 is a good value.

>  static int stm32_clock_event_shutdown(struct clock_event_device *evt)
>  {
>  	struct timer_of *to = to_timer_of(evt);
> @@ -129,7 +131,7 @@ 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);
> 


-- 
 <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

The clock driving counters is at 90MHz so the maximum period
for 16 bis counters is around 750 ms which is a short period
for a clocksource. For 32 bits counters this period is close
47 secondes which is more acceptable.

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 | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

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

[PATCH v8 4/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 14/11/2017 09:52, Benjamin Gaignard wrote:
> The clock driving counters is at 90MHz so the maximum period
> for 16 bis counters is around 750 ms which is a short period
> for a clocksource.

Isn't it 728us ?

> For 32 bits counters this period is close
> 47 secondes which is more acceptable.
> 
> 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 | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
> index ae41a19..8173bcf 100644
> --- a/drivers/clocksource/timer-stm32.c
> +++ b/drivers/clocksource/timer-stm32.c
> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>  static int __init stm32_clockevent_init(struct device_node *node)
>  {
>  	struct reset_control *rstc;
> -	unsigned long max_delta;
> -	int ret, bits, prescaler = 1;
> +	unsigned long max_arr;
>  	struct timer_of *to;
> +	int ret;
>  
>  	to = kzalloc(sizeof(*to), GFP_KERNEL);
>  	if (!to)
> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>  
>  	/* Detect whether the timer is 16 or 32 bits */
>  	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
> -	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
> -	if (max_delta == ~0U) {
> -		prescaler = 1;
> -		bits = 32;
> -	} else {
> -		prescaler = 1024;
> -		bits = 16;
> +	max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
> +	if (max_arr != ~0U) {
> +		pr_err("32 bits timer is needed\n");
> +		ret = -EINVAL;
> +		goto deinit;
>  	}
> +
>  	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>  
> -	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
> +	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
>  	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
>  	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
>  	writel_relaxed(0, timer_of_base(to) + TIM_SR);
>  
>  	clockevents_config_and_register(&to->clkevt,
> -					timer_of_period(to), MIN_DELTA, max_delta);
> -
> -	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
> -			node, bits);
> +					timer_of_period(to), MIN_DELTA, ~0U);
>  
>  	return 0;
>  
> +deinit:
> +	timer_of_exit(to);
>  err:
>  	kfree(to);
>  	return ret;
> 


-- 
 <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

2017-12-07 16:27 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>> The clock driving counters is at 90MHz so the maximum period
>> for 16 bis counters is around 750 ms which is a short period
>> for a clocksource.
>
> Isn't it 728us ?

yes it is: 2^16 / 90.000.000 => 728us

>
>> For 32 bits counters this period is close
>> 47 secondes which is more acceptable.
>>
>> 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 | 26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>> index ae41a19..8173bcf 100644
>> --- a/drivers/clocksource/timer-stm32.c
>> +++ b/drivers/clocksource/timer-stm32.c
>> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>  static int __init stm32_clockevent_init(struct device_node *node)
>>  {
>>       struct reset_control *rstc;
>> -     unsigned long max_delta;
>> -     int ret, bits, prescaler = 1;
>> +     unsigned long max_arr;
>>       struct timer_of *to;
>> +     int ret;
>>
>>       to = kzalloc(sizeof(*to), GFP_KERNEL);
>>       if (!to)
>> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>>
>>       /* Detect whether the timer is 16 or 32 bits */
>>       writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>> -     max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>> -     if (max_delta == ~0U) {
>> -             prescaler = 1;
>> -             bits = 32;
>> -     } else {
>> -             prescaler = 1024;
>> -             bits = 16;
>> +     max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
>> +     if (max_arr != ~0U) {
>> +             pr_err("32 bits timer is needed\n");
>> +             ret = -EINVAL;
>> +             goto deinit;
>>       }
>> +
>>       writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>
>> -     writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>> +     writel_relaxed(0, timer_of_base(to) + TIM_PSC);
>>       writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
>>       writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
>>       writel_relaxed(0, timer_of_base(to) + TIM_SR);
>>
>>       clockevents_config_and_register(&to->clkevt,
>> -                                     timer_of_period(to), MIN_DELTA, max_delta);
>> -
>> -     pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
>> -                     node, bits);
>> +                                     timer_of_period(to), MIN_DELTA, ~0U);
>>
>>       return 0;
>>
>> +deinit:
>> +     timer_of_exit(to);
>>  err:
>>       kfree(to);
>>       return ret;
>>
>
>
> --
>  <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 07/12/2017 17:33, Benjamin Gaignard wrote:
> 2017-12-07 16:27 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>>> The clock driving counters is at 90MHz so the maximum period
>>> for 16 bis counters is around 750 ms which is a short period
>>> for a clocksource.
>>
>> Isn't it 728us ?
> 
> yes it is: 2^16 / 90.000.000 => 728us

Ok, now I can do the connection with the previous patch.

So, the real issue of all this is the 16bits clocksource is wrapping up
every 728us, hence the clockevent periodically expires every ~728us to
keep the timekeeping consistent. Unfortunately, the kernel has a too
high overhead for this as the system is consistently processing this
timer leading to a CPU time resource starvation.

Is that correct ?


>>> For 32 bits counters this period is close
>>> 47 secondes which is more acceptable.
>>>
>>> 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 | 26 ++++++++++++--------------
>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>>> index ae41a19..8173bcf 100644
>>> --- a/drivers/clocksource/timer-stm32.c
>>> +++ b/drivers/clocksource/timer-stm32.c
>>> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>>  static int __init stm32_clockevent_init(struct device_node *node)
>>>  {
>>>       struct reset_control *rstc;
>>> -     unsigned long max_delta;
>>> -     int ret, bits, prescaler = 1;
>>> +     unsigned long max_arr;
>>>       struct timer_of *to;
>>> +     int ret;
>>>
>>>       to = kzalloc(sizeof(*to), GFP_KERNEL);
>>>       if (!to)
>>> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>>>
>>>       /* Detect whether the timer is 16 or 32 bits */
>>>       writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>>> -     max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>> -     if (max_delta == ~0U) {
>>> -             prescaler = 1;
>>> -             bits = 32;
>>> -     } else {
>>> -             prescaler = 1024;
>>> -             bits = 16;
>>> +     max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>> +     if (max_arr != ~0U) {
>>> +             pr_err("32 bits timer is needed\n");
>>> +             ret = -EINVAL;
>>> +             goto deinit;
>>>       }
>>> +
>>>       writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>>
>>> -     writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>>> +     writel_relaxed(0, timer_of_base(to) + TIM_PSC);
>>>       writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
>>>       writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
>>>       writel_relaxed(0, timer_of_base(to) + TIM_SR);
>>>
>>>       clockevents_config_and_register(&to->clkevt,
>>> -                                     timer_of_period(to), MIN_DELTA, max_delta);
>>> -
>>> -     pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
>>> -                     node, bits);
>>> +                                     timer_of_period(to), MIN_DELTA, ~0U);
>>>
>>>       return 0;
>>>
>>> +deinit:
>>> +     timer_of_exit(to);
>>>  err:
>>>       kfree(to);
>>>       return ret;
>>>
>>
>>
>> --
>>  <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
>>
> 
> 
> 


-- 
 <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

2017-12-07 17:49 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 07/12/2017 17:33, Benjamin Gaignard wrote:
>> 2017-12-07 16:27 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>>>> The clock driving counters is at 90MHz so the maximum period
>>>> for 16 bis counters is around 750 ms which is a short period
>>>> for a clocksource.
>>>
>>> Isn't it 728us ?
>>
>> yes it is: 2^16 / 90.000.000 => 728us
>
> Ok, now I can do the connection with the previous patch.
>
> So, the real issue of all this is the 16bits clocksource is wrapping up
> every 728us, hence the clockevent periodically expires every ~728us to
> keep the timekeeping consistent. Unfortunately, the kernel has a too
> high overhead for this as the system is consistently processing this
> timer leading to a CPU time resource starvation.
>
> Is that correct ?

Yes that is correct

>
>
>>>> For 32 bits counters this period is close
>>>> 47 secondes which is more acceptable.
>>>>
>>>> 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 | 26 ++++++++++++--------------
>>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>>>> index ae41a19..8173bcf 100644
>>>> --- a/drivers/clocksource/timer-stm32.c
>>>> +++ b/drivers/clocksource/timer-stm32.c
>>>> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>>>  static int __init stm32_clockevent_init(struct device_node *node)
>>>>  {
>>>>       struct reset_control *rstc;
>>>> -     unsigned long max_delta;
>>>> -     int ret, bits, prescaler = 1;
>>>> +     unsigned long max_arr;
>>>>       struct timer_of *to;
>>>> +     int ret;
>>>>
>>>>       to = kzalloc(sizeof(*to), GFP_KERNEL);
>>>>       if (!to)
>>>> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>>>>
>>>>       /* Detect whether the timer is 16 or 32 bits */
>>>>       writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>>>> -     max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>>> -     if (max_delta == ~0U) {
>>>> -             prescaler = 1;
>>>> -             bits = 32;
>>>> -     } else {
>>>> -             prescaler = 1024;
>>>> -             bits = 16;
>>>> +     max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>>> +     if (max_arr != ~0U) {
>>>> +             pr_err("32 bits timer is needed\n");
>>>> +             ret = -EINVAL;
>>>> +             goto deinit;
>>>>       }
>>>> +
>>>>       writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>>>
>>>> -     writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>>>> +     writel_relaxed(0, timer_of_base(to) + TIM_PSC);
>>>>       writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
>>>>       writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
>>>>       writel_relaxed(0, timer_of_base(to) + TIM_SR);
>>>>
>>>>       clockevents_config_and_register(&to->clkevt,
>>>> -                                     timer_of_period(to), MIN_DELTA, max_delta);
>>>> -
>>>> -     pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
>>>> -                     node, bits);
>>>> +                                     timer_of_period(to), MIN_DELTA, ~0U);
>>>>
>>>>       return 0;
>>>>
>>>> +deinit:
>>>> +     timer_of_exit(to);
>>>>  err:
>>>>       kfree(to);
>>>>       return ret;
>>>>
>>>
>>>
>>> --
>>>  <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
>>>
>>
>>
>>
>
>
> --
>  <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 07/12/2017 21:36, Benjamin Gaignard wrote:
> 2017-12-07 17:49 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 07/12/2017 17:33, Benjamin Gaignard wrote:
>>> 2017-12-07 16:27 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>>>>> The clock driving counters is at 90MHz so the maximum period
>>>>> for 16 bis counters is around 750 ms which is a short period
>>>>> for a clocksource.
>>>>
>>>> Isn't it 728us ?
>>>
>>> yes it is: 2^16 / 90.000.000 => 728us
>>
>> Ok, now I can do the connection with the previous patch.
>>
>> So, the real issue of all this is the 16bits clocksource is wrapping up
>> every 728us, hence the clockevent periodically expires every ~728us to
>> keep the timekeeping consistent. Unfortunately, the kernel has a too
>> high overhead for this as the system is consistently processing this
>> timer leading to a CPU time resource starvation.
>>
>> Is that correct ?
> 
> Yes that is correct

Oh man. That was unclear since the beginning, we are not talking about
inaccurate clocksource or whatever but just these 16bits timers can't
work on Linux.


-- 
 <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 14/11/2017 09:52, Benjamin Gaignard wrote:
> The clock driving counters is at 90MHz so the maximum period
> for 16 bis counters is around 750 ms

728 us

> which is a short period for a clocksource.

Which clocksource are you talking about ?

> For 32 bits counters this period is close
> 47 secondes which is more acceptable.
> 
> This patch remove 16 bits counters support and makes sure that
> they won't be probed anymore.

Are we talking about clockevent or clocksource?

Is this issue present today ? Or is it if we add the clocksource support
? We are talking about clocksource but we change the clockevent code.

All this is very confusing.

I have a rough idea of what is happening, but it is not up to me to
decode and infer from the changes, you need to describe *clearly* the
situation.

 - What happens if we use a 16bits timer as a clockevent ?
 - What happens if we use a 16bits timer as a clocksource ?
 - Why is it preferable to remove the support of the 16bits timers
instead of downgrading them with the rating ?

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

> ---
>  drivers/clocksource/timer-stm32.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
> index ae41a19..8173bcf 100644
> --- a/drivers/clocksource/timer-stm32.c
> +++ b/drivers/clocksource/timer-stm32.c
> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>  static int __init stm32_clockevent_init(struct device_node *node)
>  {
>  	struct reset_control *rstc;
> -	unsigned long max_delta;
> -	int ret, bits, prescaler = 1;
> +	unsigned long max_arr;
>  	struct timer_of *to;
> +	int ret;
>  
>  	to = kzalloc(sizeof(*to), GFP_KERNEL);
>  	if (!to)
> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>  
>  	/* Detect whether the timer is 16 or 32 bits */
>  	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
> -	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
> -	if (max_delta == ~0U) {
> -		prescaler = 1;
> -		bits = 32;
> -	} else {
> -		prescaler = 1024;
> -		bits = 16;
> +	max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
> +	if (max_arr != ~0U) {
> +		pr_err("32 bits timer is needed\n");
> +		ret = -EINVAL;
> +		goto deinit;
>  	}

Wrap this in a function:

static bool stm32_timer_is_32bits(struct timer_of *to)
{
	return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
}

Then clearly inform the user.

if (!stm32_timer_is_32bits(to)) {
	pr_warn("Timer %pOF is a 16 bits timer\n", node);
	/* abort the registration or downgrade the timer's rating */
}

> +
>  	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);
> +					timer_of_period(to), MIN_DELTA, ~0U);
>  
>  	return 0;
>  
> +deinit:
> +	timer_of_exit(to);

Fix this please (timer_of_cleanup).

In the future, make sure the patches are git-bisect safe.



-- 
 <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

2017-12-08 9:34 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>> The clock driving counters is at 90MHz so the maximum period
>> for 16 bis counters is around 750 ms
>
> 728 us
>
>> which is a short period for a clocksource.
>
> Which clocksource are you talking about ?
>
>> For 32 bits counters this period is close
>> 47 secondes which is more acceptable.
>>
>> This patch remove 16 bits counters support and makes sure that
>> they won't be probed anymore.
>
> Are we talking about clockevent or clocksource?
>
> Is this issue present today ? Or is it if we add the clocksource support
> ? We are talking about clocksource but we change the clockevent code.
>
> All this is very confusing.
>
> I have a rough idea of what is happening, but it is not up to me to
> decode and infer from the changes, you need to describe *clearly* the
> situation.
>
>  - What happens if we use a 16bits timer as a clockevent ?
>  - What happens if we use a 16bits timer as a clocksource ?
>  - Why is it preferable to remove the support of the 16bits timers
> instead of downgrading them with the rating ?

Up to this patch it is only about clockevent, clocksource code is
introduced in patch 5.
For the both cases 16bits counter have a a too short period (728us)
and can't be used
so downgrading the rating is not a solution.

I will change the wording in v9

>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>
>> ---
>>  drivers/clocksource/timer-stm32.c | 26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>> index ae41a19..8173bcf 100644
>> --- a/drivers/clocksource/timer-stm32.c
>> +++ b/drivers/clocksource/timer-stm32.c
>> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>  static int __init stm32_clockevent_init(struct device_node *node)
>>  {
>>       struct reset_control *rstc;
>> -     unsigned long max_delta;
>> -     int ret, bits, prescaler = 1;
>> +     unsigned long max_arr;
>>       struct timer_of *to;
>> +     int ret;
>>
>>       to = kzalloc(sizeof(*to), GFP_KERNEL);
>>       if (!to)
>> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>>
>>       /* Detect whether the timer is 16 or 32 bits */
>>       writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>> -     max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>> -     if (max_delta == ~0U) {
>> -             prescaler = 1;
>> -             bits = 32;
>> -     } else {
>> -             prescaler = 1024;
>> -             bits = 16;
>> +     max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
>> +     if (max_arr != ~0U) {
>> +             pr_err("32 bits timer is needed\n");
>> +             ret = -EINVAL;
>> +             goto deinit;
>>       }
>
> Wrap this in a function:
>
> static bool stm32_timer_is_32bits(struct timer_of *to)
> {
>         return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
> }
>
> Then clearly inform the user.
>
> if (!stm32_timer_is_32bits(to)) {
>         pr_warn("Timer %pOF is a 16 bits timer\n", node);
>         /* abort the registration or downgrade the timer's rating */
> }

Ok I will change that in v9

>
>> +
>>       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);
>> +                                     timer_of_period(to), MIN_DELTA, ~0U);
>>
>>       return 0;
>>
>> +deinit:
>> +     timer_of_exit(to);
>
> Fix this please (timer_of_cleanup).
>
> In the future, make sure the patches are git-bisect safe.
>
>
>
> --
>  <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Daniel Lezcano]

On 08/12/2017 10:25, Benjamin Gaignard wrote:
> 2017-12-08 9:34 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>>> The clock driving counters is at 90MHz so the maximum period
>>> for 16 bis counters is around 750 ms
>>
>> 728 us
>>
>>> which is a short period for a clocksource.
>>
>> Which clocksource are you talking about ?
>>
>>> For 32 bits counters this period is close
>>> 47 secondes which is more acceptable.
>>>
>>> This patch remove 16 bits counters support and makes sure that
>>> they won't be probed anymore.
>>
>> Are we talking about clockevent or clocksource?
>>
>> Is this issue present today ? Or is it if we add the clocksource support
>> ? We are talking about clocksource but we change the clockevent code.
>>
>> All this is very confusing.
>>
>> I have a rough idea of what is happening, but it is not up to me to
>> decode and infer from the changes, you need to describe *clearly* the
>> situation.
>>
>>  - What happens if we use a 16bits timer as a clockevent ?
>>  - What happens if we use a 16bits timer as a clocksource ?
>>  - Why is it preferable to remove the support of the 16bits timers
>> instead of downgrading them with the rating ?
> 
> Up to this patch it is only about clockevent, clocksource code is
> introduced in patch 5.
> For the both cases 16bits counter have a a too short period (728us)
> and can't be used
> so downgrading the rating is not a solution.

You have to explain why it is a too short period. I will be happy to see
an example of the issues the user is facing.



> I will change the wording in v9
> 
>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>>> ---
>>>  drivers/clocksource/timer-stm32.c | 26 ++++++++++++--------------
>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>>> index ae41a19..8173bcf 100644
>>> --- a/drivers/clocksource/timer-stm32.c
>>> +++ b/drivers/clocksource/timer-stm32.c
>>> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>>  static int __init stm32_clockevent_init(struct device_node *node)
>>>  {
>>>       struct reset_control *rstc;
>>> -     unsigned long max_delta;
>>> -     int ret, bits, prescaler = 1;
>>> +     unsigned long max_arr;
>>>       struct timer_of *to;
>>> +     int ret;
>>>
>>>       to = kzalloc(sizeof(*to), GFP_KERNEL);
>>>       if (!to)
>>> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>>>
>>>       /* Detect whether the timer is 16 or 32 bits */
>>>       writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>>> -     max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>> -     if (max_delta == ~0U) {
>>> -             prescaler = 1;
>>> -             bits = 32;
>>> -     } else {
>>> -             prescaler = 1024;
>>> -             bits = 16;
>>> +     max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>> +     if (max_arr != ~0U) {
>>> +             pr_err("32 bits timer is needed\n");
>>> +             ret = -EINVAL;
>>> +             goto deinit;
>>>       }
>>
>> Wrap this in a function:
>>
>> static bool stm32_timer_is_32bits(struct timer_of *to)
>> {
>>         return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
>> }
>>
>> Then clearly inform the user.
>>
>> if (!stm32_timer_is_32bits(to)) {
>>         pr_warn("Timer %pOF is a 16 bits timer\n", node);
>>         /* abort the registration or downgrade the timer's rating */
>> }
> 
> Ok I will change that in v9
> 
>>
>>> +
>>>       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);
>>> +                                     timer_of_period(to), MIN_DELTA, ~0U);
>>>
>>>       return 0;
>>>
>>> +deinit:
>>> +     timer_of_exit(to);
>>
>> Fix this please (timer_of_cleanup).
>>
>> In the future, make sure the patches are git-bisect safe.
>>
>>
>>
>> --
>>  <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


-- 
 <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 v8 4/6] clocksource: stm32: only use 32 bits timers

[Benjamin Gaignard]

2017-12-08 10:29 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 08/12/2017 10:25, Benjamin Gaignard wrote:
>> 2017-12-08 9:34 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 14/11/2017 09:52, Benjamin Gaignard wrote:
>>>> The clock driving counters is at 90MHz so the maximum period
>>>> for 16 bis counters is around 750 ms
>>>
>>> 728 us
>>>
>>>> which is a short period for a clocksource.
>>>
>>> Which clocksource are you talking about ?
>>>
>>>> For 32 bits counters this period is close
>>>> 47 secondes which is more acceptable.
>>>>
>>>> This patch remove 16 bits counters support and makes sure that
>>>> they won't be probed anymore.
>>>
>>> Are we talking about clockevent or clocksource?
>>>
>>> Is this issue present today ? Or is it if we add the clocksource support
>>> ? We are talking about clocksource but we change the clockevent code.
>>>
>>> All this is very confusing.
>>>
>>> I have a rough idea of what is happening, but it is not up to me to
>>> decode and infer from the changes, you need to describe *clearly* the
>>> situation.
>>>
>>>  - What happens if we use a 16bits timer as a clockevent ?
>>>  - What happens if we use a 16bits timer as a clocksource ?
>>>  - Why is it preferable to remove the support of the 16bits timers
>>> instead of downgrading them with the rating ?
>>
>> Up to this patch it is only about clockevent, clocksource code is
>> introduced in patch 5.
>> For the both cases 16bits counter have a a too short period (728us)
>> and can't be used
>> so downgrading the rating is not a solution.
>
> You have to explain why it is a too short period. I will be happy to see
> an example of the issues the user is facing.

This a very basic issue, the kernel doesn't boot at all...

>
>
>
>> I will change the wording in v9
>>
>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>>
>>>> ---
>>>>  drivers/clocksource/timer-stm32.c | 26 ++++++++++++--------------
>>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>>>> index ae41a19..8173bcf 100644
>>>> --- a/drivers/clocksource/timer-stm32.c
>>>> +++ b/drivers/clocksource/timer-stm32.c
>>>> @@ -83,9 +83,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>>>>  static int __init stm32_clockevent_init(struct device_node *node)
>>>>  {
>>>>       struct reset_control *rstc;
>>>> -     unsigned long max_delta;
>>>> -     int ret, bits, prescaler = 1;
>>>> +     unsigned long max_arr;
>>>>       struct timer_of *to;
>>>> +     int ret;
>>>>
>>>>       to = kzalloc(sizeof(*to), GFP_KERNEL);
>>>>       if (!to)
>>>> @@ -115,29 +115,27 @@ static int __init stm32_clockevent_init(struct device_node *node)
>>>>
>>>>       /* Detect whether the timer is 16 or 32 bits */
>>>>       writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>>>> -     max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>>> -     if (max_delta == ~0U) {
>>>> -             prescaler = 1;
>>>> -             bits = 32;
>>>> -     } else {
>>>> -             prescaler = 1024;
>>>> -             bits = 16;
>>>> +     max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>>> +     if (max_arr != ~0U) {
>>>> +             pr_err("32 bits timer is needed\n");
>>>> +             ret = -EINVAL;
>>>> +             goto deinit;
>>>>       }
>>>
>>> Wrap this in a function:
>>>
>>> static bool stm32_timer_is_32bits(struct timer_of *to)
>>> {
>>>         return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
>>> }
>>>
>>> Then clearly inform the user.
>>>
>>> if (!stm32_timer_is_32bits(to)) {
>>>         pr_warn("Timer %pOF is a 16 bits timer\n", node);
>>>         /* abort the registration or downgrade the timer's rating */
>>> }
>>
>> Ok I will change that in v9
>>
>>>
>>>> +
>>>>       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);
>>>> +                                     timer_of_period(to), MIN_DELTA, ~0U);
>>>>
>>>>       return 0;
>>>>
>>>> +deinit:
>>>> +     timer_of_exit(to);
>>>
>>> Fix this please (timer_of_cleanup).
>>>
>>> In the future, make sure the patches are git-bisect safe.
>>>
>>>
>>>
>>> --
>>>  <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
>
>
> --
>  <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 v8 5/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 8173bcf..c0a62cd 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)
 
@@ -42,28 +44,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;
 }
@@ -75,12 +93,57 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
+	if (clockevent_state_periodic(evt))
+		stm32_clock_event_set_periodic(evt);
+	else
+		stm32_clock_event_shutdown(evt);
+
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static int __init stm32_clockevent_init(struct device_node *node)
+static void __init stm32_clockevent_init(struct timer_of *to)
+{
+	writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+	writel_relaxed(0, timer_of_base(to) + TIM_SR);
+
+	clockevents_config_and_register(&to->clkevt,
+					timer_of_rate(to), 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;
 	unsigned long max_arr;
@@ -92,12 +155,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;
 
@@ -122,23 +186,19 @@ 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;
 
 deinit:
-	timer_of_exit(to);
+	timer_of_cleanup(to);
 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.7.4

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

[Benjamin Gaignard]

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

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

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

[PATCH v8 0/6] stm32 clocksource driver rework

[Benjamin Gaignard]

2017-11-14 9:52 GMT+01:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
> version 8:
>  - rebased on timers/core
>  - change timer_of_exit() name to timer_of_cleanup()
>  - update stm32 clocksource driver to use this function
>

Gentle ping to get this v8 reviewed, the first patch has already been merged
int v4.15-rc1

Thanks,
Benjamin

> 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: timer_of: rename timer_of_exit to timer_of_cleanup
>   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
>   arm: dts: stm32: remove useless clocksource nodes
>
>  arch/arm/boot/dts/stm32f429.dtsi  |  32 -----
>  arch/arm/boot/dts/stm32f746.dtsi  |  32 -----
>  drivers/clocksource/Kconfig       |   1 +
>  drivers/clocksource/timer-of.c    |   9 +-
>  drivers/clocksource/timer-of.h    |   2 +-
>  drivers/clocksource/timer-stm32.c | 242 ++++++++++++++++++++------------------
>  6 files changed, 138 insertions(+), 180 deletions(-)
>
> --
> 2.7.4
>

[PATCH v8 0/6] stm32 clocksource driver rework

[Alexandre Torgue]

Hi

On 11/14/2017 09:52 AM, Benjamin Gaignard wrote:
> 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: timer_of: rename timer_of_exit to timer_of_cleanup
>    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
>    arm: dts: stm32: remove useless clocksource nodes
> 
>   arch/arm/boot/dts/stm32f429.dtsi  |  32 -----
>   arch/arm/boot/dts/stm32f746.dtsi  |  32 -----
>   drivers/clocksource/Kconfig       |   1 +
>   drivers/clocksource/timer-of.c    |   9 +-
>   drivers/clocksource/timer-of.h    |   2 +-
>   drivers/clocksource/timer-stm32.c | 242 ++++++++++++++++++++------------------
>   6 files changed, 138 insertions(+), 180 deletions(-)
> 

What is the status of this patch-set ? Is there a chance to have it for 
v4.16 ?

Thanks
Alex

[PATCH v8 0/6] stm32 clocksource driver rework

[Daniel Lezcano]

On 05/12/2017 11:12, Alexandre Torgue wrote:
[ ... ]

>> Benjamin Gaignard (6):
>> ?? clocksource: timer_of: rename timer_of_exit to timer_of_cleanup
>> ?? 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
>> ?? arm: dts: stm32: remove useless clocksource nodes
>>
>> ? arch/arm/boot/dts/stm32f429.dtsi? |? 32 -----
>> ? arch/arm/boot/dts/stm32f746.dtsi? |? 32 -----
>> ? drivers/clocksource/Kconfig?????? |?? 1 +
>> ? drivers/clocksource/timer-of.c??? |?? 9 +-
>> ? drivers/clocksource/timer-of.h??? |?? 2 +-
>> ? drivers/clocksource/timer-stm32.c | 242
>> ++++++++++++++++++++------------------
>> ? 6 files changed, 138 insertions(+), 180 deletions(-)
>>
> 
> What is the status of this patch-set ? Is there a chance to have it for
> v4.16 ?

I will take care of reviewing them this week. It is in my todo list.


-- 
 <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 v8 0/6] stm32 clocksource driver rework

[Alexandre Torgue]

On 12/05/2017 11:15 AM, Daniel Lezcano wrote:
> On 05/12/2017 11:12, Alexandre Torgue wrote:
> [ ... ]
> 
>>> Benjamin Gaignard (6):
>>>  ?? clocksource: timer_of: rename timer_of_exit to timer_of_cleanup
>>>  ?? 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
>>>  ?? arm: dts: stm32: remove useless clocksource nodes
>>>
>>>  ? arch/arm/boot/dts/stm32f429.dtsi? |? 32 -----
>>>  ? arch/arm/boot/dts/stm32f746.dtsi? |? 32 -----
>>>  ? drivers/clocksource/Kconfig?????? |?? 1 +
>>>  ? drivers/clocksource/timer-of.c??? |?? 9 +-
>>>  ? drivers/clocksource/timer-of.h??? |?? 2 +-
>>>  ? drivers/clocksource/timer-stm32.c | 242
>>> ++++++++++++++++++++------------------
>>>  ? 6 files changed, 138 insertions(+), 180 deletions(-)
>>>
>>
>> What is the status of this patch-set ? Is there a chance to have it for
>> v4.16 ?
> 
> I will take care of reviewing them this week. It is in my todo list.
> 
> 
Thanks Daniel