| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+M3ks6=tQbozhTEoSmnFsK4VfHCeymGu7zBdC6tGQ1is609dw@mail.gmail.com>
Date: Mon, 18 Dec 2017 12:10:02 +0100
From: Benjamin Gaignard <benjamin.gaignard@...aro.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Mark Rutland <mark.rutland@....com>,
Russell King - ARM Linux <linux@...linux.org.uk>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...com>,
Thomas Gleixner <tglx@...utronix.de>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Benjamin Gaignard <benjamin.gaignard@...com>
Subject: Re: [PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution
2017-12-18 11:54 GMT+01:00 Daniel Lezcano <daniel.lezcano@...aro.org>:
> On 18/12/2017 10:44, Benjamin Gaignard wrote:
>> 2017-12-18 10:26 GMT+01:00 Daniel Lezcano <daniel.lezcano@...aro.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.gaignard@...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 ?
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
Powered by blists - more mailing lists