From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933527AbdLRJ0T (ORCPT ); Mon, 18 Dec 2017 04:26:19 -0500 Received: from mail-lf0-f68.google.com ([209.85.215.68]:34750 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933252AbdLRJ0P (ORCPT ); Mon, 18 Dec 2017 04:26:15 -0500 X-Google-Smtp-Source: ACJfBotxPZkukJADNBCjSrcghB69uBRYjWTgD51qnw8vUt4uc3iIr0Efk+eVvU/yxI92jl8PvwucoA== Subject: Re: [PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution To: Benjamin Gaignard , mark.rutland@arm.com, linux@armlinux.org.uk, mcoquelin.stm32@gmail.com, alexandre.torgue@st.com, tglx@linutronix.de Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Benjamin Gaignard References: <20171215085247.14946-1-benjamin.gaignard@st.com> <20171215085247.14946-3-benjamin.gaignard@st.com> From: Daniel Lezcano Message-ID: <4e57bec8-a52b-9574-5f6e-985457d44147@linaro.org> Date: Mon, 18 Dec 2017 10:26:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171215085247.14946-3-benjamin.gaignard@st.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 > --- > 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 ? 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. 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). > 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 ). Thanks. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog