From: Arnd Bergmann <arnd@arndb.de>
To: Guru Das Srinagesh <gurus@codeaurora.org>
Cc: "Thierry Reding" <thierry.reding@gmail.com>,
"Linux PWM List" <linux-pwm@vger.kernel.org>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Subbaraman Narayanamurthy" <subbaram@codeaurora.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Shawn Guo" <shawnguo@kernel.org>,
"Sascha Hauer" <s.hauer@pengutronix.de>,
"Pengutronix Kernel Team" <kernel@pengutronix.de>,
"Fabio Estevam" <festevam@gmail.com>,
"NXP Linux Team" <linux-imx@nxp.com>,
"David Collins" <collinsd@codeaurora.org>
Subject: Re: [PATCH v11 06/12] pwm: imx27: Use 64-bit division macro and function
Date: Thu, 2 Apr 2020 23:16:22 +0200 [thread overview]
Message-ID: <CAK8P3a3mc_sYczyKiaDoQLTTwDj2odwHJ5LFNAOb4RaRyh0YLQ@mail.gmail.com> (raw)
In-Reply-To: <20200402201654.GA9191@codeaurora.org>
On Thu, Apr 2, 2020 at 10:16 PM Guru Das Srinagesh <gurus@codeaurora.org> wrote:
> On Tue, Mar 31, 2020 at 10:49:29PM +0200, Thierry Reding wrote:
> > Doesn't that mean that anything below a 1 second period will be clamped
> > to just 0?
>
> True. How about this then?
>
> int pwm_imx27_calc_period_cycles(struct pwm_state state,
> unsigned long clk_rate,
> unsigned long *period_cycles)
> {
> u64 c1, c2;
>
> c1 = clk_rate;
> c2 = state->period;
> if (c2 > c1) {
> c2 = c1;
> c1 = state->period;
> }
>
> if (!c1 || !c2) {
> pr_err("clk rate and period should be nonzero\n");
> return -EINVAL;
> }
>
> if (c2 <= div64_u64(U64_MAX, c1)) {
> c = c1 * c2;
> do_div(c, 1000000000);
> } else if (c2 <= div64_u64(U64_MAX, div64_u64(c1, 1000))) {
> do_div(c1, 1000);
> c = c1 * c2;
> do_div(c, 1000000);
> } else if (c2 <= div64_u64(U64_MAX, div64_u64(c1, 1000000))) {
> do_div(c1, 1000000);
> c = c1 * c2;
> do_div(c, 1000);
> } else if (c2 <= div64_u64(U64_MAX, div64_u64(c1, 1000000000))) {
> do_div(c1, 1000000000);
> c = c1 * c2;
> }
>
> *period_cycles = c;
>
> return 0;
> }
>
> ...
>
> ret = pwm_imx27_calc_period_cycles(state, clk_get_rate(imx->clk_per),
> &period_cycles);
> if (ret)
> return ret;
>
> I unit tested this logic out by calculating period_cycles using both the
> existing logic and the proposed one, and the results are as below.
>
> --------------------------------------------------------------------------------
> clk_rate period existing proposed
> --------------------------------------------------------------------------------
> 1000000000 18446744073709551615 18446744072 18446744073000000000
> (U64_MAX)
> --------------------------------------------------------------------------------
> 1000000000 4294967291 4294967291 4294967291
> --------------------------------------------------------------------------------
>
> Overflow occurs in the first case with the existing logic, whereas the
> proposed logic handles it correctly. As for the second case where there are
> more typical values of period, the proposed logic handles that correctly
> too.
This looks correct, but very expensive, and you don't really have to
go this far, given that c1 is guaranteed to be a 32-bit number, and
you divide by a constant in the end.
Why not do something like
#define SHIFT 41 /* arbitrarily picked, not too big, not too small */
#define MUL 2199 /* 2^SHIFT / NSEC_PER_SEC */
period_cycles = clk_get_rate(imx->clk_per) * ((state->period * MUL) >> SHIFT);
Arnd
next prev parent reply other threads:[~2020-04-02 21:16 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1584667964.git.gurus@codeaurora.org>
2020-03-20 1:41 ` [PATCH v11 01/12] drm/i915: Use 64-bit division macro Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 02/12] hwmon: pwm-fan: " Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 03/12] ir-rx51: " Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 04/12] pwm: clps711x: Cast period to u32 before use as divisor Guru Das Srinagesh
2020-03-20 17:11 ` Arnd Bergmann
2020-04-07 0:26 ` Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 05/12] pwm: pwm-imx-tpm: Use 64-bit division macro Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 06/12] pwm: imx27: Use 64-bit division macro and function Guru Das Srinagesh
2020-03-20 17:09 ` Arnd Bergmann
2020-03-30 20:43 ` Guru Das Srinagesh
2020-03-31 15:24 ` Arnd Bergmann
2020-03-31 20:20 ` Guru Das Srinagesh
2020-03-31 20:49 ` Thierry Reding
2020-04-02 20:16 ` Guru Das Srinagesh
2020-04-02 20:55 ` Guru Das Srinagesh
2020-04-02 21:16 ` Arnd Bergmann [this message]
2020-04-03 17:37 ` Guru Das Srinagesh
2020-04-03 19:13 ` Arnd Bergmann
2020-03-20 1:41 ` [PATCH v11 07/12] pwm: sifive: Use 64-bit division macro Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 08/12] pwm: stm32-lp: Use %llu format specifier for period Guru Das Srinagesh
2020-03-20 10:45 ` Joe Perches
2020-03-30 19:30 ` Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 09/12] pwm: sun4i: Use 64-bit division function Guru Das Srinagesh
2020-03-20 17:02 ` Arnd Bergmann
2020-03-20 1:41 ` [PATCH v11 10/12] backlight: pwm_bl: " Guru Das Srinagesh
2020-03-20 13:31 ` Lee Jones
2020-03-24 11:07 ` Lee Jones
2020-03-24 12:57 ` Uwe Kleine-König
2020-03-24 13:04 ` Daniel Thompson
2020-03-24 14:24 ` Lee Jones
2020-03-24 14:43 ` Uwe Kleine-König
2020-04-15 9:26 ` Lee Jones
2020-03-20 1:41 ` [PATCH v11 11/12] clk: pwm: Assign u64 divisor to unsigned int before use Guru Das Srinagesh
2020-03-20 17:00 ` Arnd Bergmann
2020-03-20 18:42 ` David Laight
2020-04-07 2:40 ` Guru Das Srinagesh
2020-03-20 1:41 ` [PATCH v11 12/12] pwm: core: Convert period and duty cycle to u64 Guru Das Srinagesh
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=CAK8P3a3mc_sYczyKiaDoQLTTwDj2odwHJ5LFNAOb4RaRyh0YLQ@mail.gmail.com \
--to=arnd@arndb.de \
--cc=collinsd@codeaurora.org \
--cc=festevam@gmail.com \
--cc=gurus@codeaurora.org \
--cc=kernel@pengutronix.de \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=subbaram@codeaurora.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).