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

Message ID 20171208113250.359-4-benjamin.gaignard@st.com
State New
Headers show
Series
  • stm32 clocksource driver rework
Related show

Commit Message

Benjamin Gaignard Dec. 8, 2017, 11:32 a.m.
From: Benjamin Gaignard <benjamin.gaignard@linaro.org>


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

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

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

---
 drivers/clocksource/timer-stm32.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

-- 
2.15.0

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

Comments

Daniel Lezcano Dec. 8, 2017, 12:51 p.m. | #1
On 08/12/2017 12:32, Benjamin Gaignard wrote:
> From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

> 

> The clock driving counters is at 90MHz so the maximum period

> for 16 bis counters is around 728us (2^16 / 90.000.000).

> For 32 bits counters this period is close 47 secondes which is

> more acceptable.

> 

> When using 16 bits counters the kernel may not be able to boot

> because it has a too high overhead compare to the clockevent period.

> Downgrading the rating of 16bits counter won't change anything

> to this problem so this patch remove 16 bits counters support

> and makes sure that they won't be probed anymore.


Benjamin,

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

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

Also, why are you removing the DT nodes ?

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

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

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

Thanks.

  -- Daniel




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

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Lezcano Dec. 8, 2017, 1:47 p.m. | #2
On 08/12/2017 14:05, Benjamin Gaignard wrote:
> 2017-12-08 13:51 GMT+01:00 Daniel Lezcano <daniel.lezcano@linaro.org>:

>> On 08/12/2017 12:32, Benjamin Gaignard wrote:

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

>>>

>>> The clock driving counters is at 90MHz so the maximum period

>>> for 16 bis counters is around 728us (2^16 / 90.000.000).

>>> For 32 bits counters this period is close 47 secondes which is

>>> more acceptable.

>>>

>>> When using 16 bits counters the kernel may not be able to boot

>>> because it has a too high overhead compare to the clockevent period.

>>> Downgrading the rating of 16bits counter won't change anything

>>> to this problem so this patch remove 16 bits counters support

>>> and makes sure that they won't be probed anymore.

>>

>> Benjamin,

>>

>> there is an inconsistency in this description and the patchset. This is

>> why it is so confusing to review and understand the purpose.

>>

>> Why are you preventing the clockevents to work with 16bits while the

>> issue is related to the clocksource you introduce in the next patch ?

> 

> No the issue is existing also for clockevent because the max period is

> around 728us so the interrupt will fire each 728us which is really too much.


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

Are you the author of this series ?

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

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

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

Patch

diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index a45f1f1cd040..707808d91bf0 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -84,12 +84,16 @@  static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static bool stm32_timer_is_32bits(struct timer_of *to)
+{
+	return readl_relaxed(timer_of_base(to) + TIM_ARR) == ~0UL;
+}
+
 static int __init stm32_clockevent_init(struct device_node *node)
 {
 	struct reset_control *rstc;
-	unsigned long max_delta;
-	int ret, bits, prescaler = 1;
 	struct timer_of *to;
+	int ret;
 
 	to = kzalloc(sizeof(*to), GFP_KERNEL);
 	if (!to)
@@ -118,31 +122,27 @@  static int __init stm32_clockevent_init(struct device_node *node)
 	}
 
 	/* Detect whether the timer is 16 or 32 bits */
-	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
-	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
-	if (max_delta == ~0U) {
-		prescaler = 1;
-		bits = 32;
-	} else {
-		prescaler = 1024;
-		bits = 16;
+	if (!stm32_timer_is_32bits(to)) {
+		pr_warn("Timer %pOF is a 16 bits timer\n", node);
+		ret = -EINVAL;
+		goto deinit;
 	}
+
 	writel_relaxed(0, timer_of_base(to) + TIM_ARR);
 
-	writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
+	writel_relaxed(0, timer_of_base(to) + TIM_PSC);
 	writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
 	writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
 	writel_relaxed(0, timer_of_base(to) + TIM_SR);
 
 	clockevents_config_and_register(&to->clkevt,
 					timer_of_period(to),
-					MIN_DELTA, max_delta);
-
-	pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
-			node, bits);
+					MIN_DELTA, ~0U);
 
 	return 0;
 
+deinit:
+	timer_of_cleanup(to);
 err:
 	kfree(to);
 	return ret;