From: Daniel Lezcano <daniel.lezcano@linaro.org> To: Benjamin Gaignard <benjamin.gaignard@linaro.org>, 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 <benjamin.gaignard@st.com> Subject: Re: [PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution Date: Mon, 18 Dec 2017 10:26:10 +0100 [thread overview] Message-ID: <4e57bec8-a52b-9574-5f6e-985457d44147@linaro.org> (raw) In-Reply-To: <20171215085247.14946-3-benjamin.gaignard@st.com> 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@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 ? 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 -- <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
WARNING: multiple messages have this Message-ID (diff)
From: daniel.lezcano@linaro.org (Daniel Lezcano) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution Date: Mon, 18 Dec 2017 10:26:10 +0100 [thread overview] Message-ID: <4e57bec8-a52b-9574-5f6e-985457d44147@linaro.org> (raw) In-Reply-To: <20171215085247.14946-3-benjamin.gaignard@st.com> 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@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 ? 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 -- <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
next prev parent reply other threads:[~2017-12-18 9:26 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-12-15 8:52 [PATCH 0/4] stm32 clocksource driver rework Benjamin Gaignard 2017-12-15 8:52 ` Benjamin Gaignard 2017-12-15 8:52 ` [PATCH 1/4] clocksource: stm32: convert driver to timer_of Benjamin Gaignard 2017-12-15 8:52 ` Benjamin Gaignard 2017-12-15 8:52 ` [PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution Benjamin Gaignard 2017-12-15 8:52 ` Benjamin Gaignard 2017-12-18 9:26 ` Daniel Lezcano [this message] 2017-12-18 9:26 ` Daniel Lezcano 2017-12-18 9:44 ` Benjamin Gaignard 2017-12-18 9:44 ` Benjamin Gaignard 2017-12-18 10:54 ` Daniel Lezcano 2017-12-18 10:54 ` Daniel Lezcano 2017-12-18 11:10 ` Benjamin Gaignard 2017-12-18 11:10 ` Benjamin Gaignard 2017-12-15 8:52 ` [PATCH 3/4] clocksource: stm32: add clocksource support Benjamin Gaignard 2017-12-15 8:52 ` Benjamin Gaignard 2017-12-15 8:52 ` [PATCH 4/4] clocksource: stm32: Update license and copyright Benjamin Gaignard 2017-12-15 8:52 ` Benjamin Gaignard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4e57bec8-a52b-9574-5f6e-985457d44147@linaro.org \ --to=daniel.lezcano@linaro.org \ --cc=alexandre.torgue@st.com \ --cc=benjamin.gaignard@linaro.org \ --cc=benjamin.gaignard@st.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=mark.rutland@arm.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.