On 18/12/2017 10:44, Benjamin Gaignard wrote:
> 2017-12-18 10:26 GMT+01:00 Daniel Lezcano <daniel.lezc...@linaro.org>:
>> On 15/12/2017 09:52, Benjamin Gaignard wrote:
>>> Rather than use fixed prescaler values compute it to get a clock
>>> as close as possible of 10KHz and a resolution of 0.1ms.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaign...@st.com>
>>> ---
>>> drivers/clocksource/timer-stm32.c | 23 ++++++++++++++++-------
>>> 1 file changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-stm32.c
>>> b/drivers/clocksource/timer-stm32.c
>>> index 23a321cca45b..de721d318065 100644
>>> --- a/drivers/clocksource/timer-stm32.c
>>> +++ b/drivers/clocksource/timer-stm32.c
>>> @@ -37,6 +37,11 @@
>>>
>>> #define TIM_EGR_UG BIT(0)
>>>
>>> +#define MAX_TIM_PSC 0xFFFF
>>> +
>>> +/* Target a 10KHz clock to get a resolution of 0.1 ms */
>>> +#define TARGETED_CLK_RATE 10000
>>> +
>>> static int stm32_clock_event_shutdown(struct clock_event_device *evt)
>>> {
>>> struct timer_of *to = to_timer_of(evt);
>>> @@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq,
>>> void *dev_id)
>>> static void __init stm32_clockevent_init(struct timer_of *to)
>>> {
>>> unsigned long max_delta;
>>> - int prescaler;
>>> + unsigned long prescaler;
>>>
>>> to->clkevt.name = "stm32_clockevent";
>>> to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
>>> @@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct
>>> timer_of *to)
>>> /* Detect whether the timer is 16 or 32 bits */
>>> writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
>>> max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
>>> - if (max_delta == ~0U) {
>>> - prescaler = 1;
>>> + to->clkevt.rating = 50;
>>> + if (max_delta == ~0U)
>>> to->clkevt.rating = 250;
>>> - } else {
>>> - prescaler = 1024;
>>> - to->clkevt.rating = 50;
>>> - }
>>> +
>>> + /*
>>> + * Get the highest possible prescaler value to be as close
>>> + * as possible of TARGETED_CLK_RATE
>>> + */
>>> + prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);
>>
>> With a 90MHz or 125MHz, the prescaler will be 9000 or 12500, so much
>> more than the 1024 we have today for 16b, and 1 for 32b.
>>
>> Shouldn't the computation be weighted with the bits width ?
>
> My goal was to get the same resolution (0.1ms) for all the timers so
> the wrap will depend of the number of bits like you describe below.
Do you really want 1ms resolution with a 32bits timer ?
>> Otherwise the timer will wrap like:
>>
>> 32bits:
>>
>> before: (2^32 / 90e6) x 1 = 47.72 seconds
>> after: (2^32 / 90e6) x 9000 = 119.3 *hours* ~= 5days
>>
>> 16bits:
>>
>> before: (2^16 / 90e6) x 1024 = 0.745 seconds
>> after: (2^16 / 90e6) x 9000 = 6.55 seconds
>>
>> The patch is ok to target the 10KHz timer rate for 16b with a 1ms
>> resolution wrapping up after 6.55 seconds. But not for the 32bits timer.
>> Furthermore, we can't tell anymore the 32bits timers have a rating of
>> 250 after this patch.
>
> What is the link between rating and resolution (or wrap) ?
Low resolution => hardly suitable for real use case => bad rating.
>From include/linux/clocksource.h
[ ... ]
* 100-199: Base level usability.
* Functional for real use, but not desired.
* 200-299: Good.
* A correct and usable clocksource.
[ ... ]
If you want to set a timer with a delta of 12.345ms and the resolution
is 1ms. Then you end up with a timer expiring after 13ms.
> Is it a problem to get a long wrap ?
It is not a problem to go for a long wrap, it is usually interesting
when the CPU has deep idle states. But it is not worth to sacrifice the
resolution with the 32bits timer in order to have 5 days before wrap.
Keeping 47secs is fine for the moment. If you want a coarser grain, that
could be acceptable because the resolution is very high but we can
postpone that for later after solving this 16b / 32b thing.
>> Leave the 32bits part as it is and compute the prescaler only in case of
>> 16bits with the target rate, which sounds a reasonable approach.
>>
>>> + if (prescaler > MAX_TIM_PSC)
>>> + prescaler = MAX_TIM_PSC;
>>
>> That can happen only if the clock rate is greater than ~655MHz, that
>> could not happen today as far as I can tell regarding the DT. So if we
>> hit this condition, we should speak up in the log (pr_warn).
>
> It is to be futur proof for next possible SoC but even if prescaler
> reach this limit
> it is not a problem the only consequence would be that resolution and
> wrap change.
Got that, but that needs to be logged with a pr_warn or pr_info.
>>> writel_relaxed(0, timer_of_base(to) + TIM_ARR);
>>> writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
>>
>> Can you fix this prescaler - 1 in order to be consistent with the
>> computation with 16b ? (32b prescaler = 0, 16b prescaler = clk_rate /
>> target ).
>
> In the hardware the clock is divise by " TIM_PSC value 1" so to be coherent
> with that I need to do prescaler -1.
Ah, ok.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
