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

On 18/12/2017 10:44, Benjamin Gaignard wrote:

2017-12-18 10:26 GMT+01:00 Daniel Lezcano [off-list ref]:

quoted

On 15/12/2017 09:52, Benjamin Gaignard wrote:

quoted

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

quoted

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.

quoted

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.

quoted

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

quoted

quoted

      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