From: Biju Das <biju.das.jz@bp.renesas.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Philipp Zabel <p.zabel@pengutronix.de>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Fabrizio Castro <fabrizio.castro.jz@renesas.com>,
Magnus Damm <magnus.damm@gmail.com>,
"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
"linux-renesas-soc@vger.kernel.org"
<linux-renesas-soc@vger.kernel.org>,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: RE: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
Date: Tue, 20 Feb 2024 16:04:35 +0000 [thread overview]
Message-ID: <TYCPR01MB11269807ECEDDEC9F47EA153B86502@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <TYVPR01MB11279298D880FA94B31219865864B2@TYVPR01MB11279.jpnprd01.prod.outlook.com>
Hi Uwe,
> -----Original Message-----
> From: Biju Das
> Sent: Friday, February 9, 2024 1:39 PM
> To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Thierry Reding <thierry.reding@gmail.com>; Philipp Zabel
> <p.zabel@pengutronix.de>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Magnus Damm
> <magnus.damm@gmail.com>; linux-pwm@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> Subject: RE: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
>
> Hi Uwe,
>
> Thanks for the feedback
>
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: Wednesday, December 13, 2023 11:40 AM
> > Subject: Re: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
> >
> > On Wed, Dec 13, 2023 at 09:06:56AM +0000, Biju Das wrote:
> > > Hi Uwe,
> > >
> > > > -----Original Message-----
> > > > From: Biju Das
> > > > Sent: Friday, December 8, 2023 2:12 PM
> > > > Subject: RE: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
> > > >
> > > > Hi Uwe Kleine-König,
> > > >
> > > > > -----Original Message-----
> > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > Sent: Friday, December 8, 2023 2:07 PM
> > > > > Subject: Re: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
> > > > >
> > > > > Hello Biju,
> > > > >
> > > > > On Fri, Dec 08, 2023 at 10:34:55AM +0000, Biju Das wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > > Sent: Thursday, December 7, 2023 9:42 PM
> > > > > > > Subject: Re: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
> > > > > > >
> > > > > > > Hello Biju,
> > > > > > >
> > > > > > > On Thu, Dec 07, 2023 at 06:26:44PM +0000, Biju Das wrote:
> > > > > > > > ######[ 304.213944] pwm-rzg2l-gpt 10048000.pwm: .apply is
> > > > > > > > not idempotent (ena=1 pol=0 5500000000000/43980352512000)
> > > > > > > > ->
> > > > > > > > (ena=1
> > > > > > > > pol=0
> > > > > > > > 5500000000000/43980239923200)
> > > > > > > > High setting##
> > > > > > > > [ 304.230854] pwm-rzg2l-gpt 10048000.pwm: .apply is not
> > > > > > > > idempotent
> > > > > > > > (ena=1 pol=0 23980465100800/43980352512000) -> (ena=1
> > > > > > > > pol=0
> > > > > > > > 23980465100800/43980239923200)
> > > > > > >
> > > > > > > Have you tried to understand that? What is the clk rate when
> > > > > > > this
> > > > > happens?
> > > > > > > You're not suggesting that mul_u64_u64_div_u64 is wrong, are
> > you?
> > > > > >
> > > > > > mul_u64_u64_div_u64() works for certain values. But for very
> > > > > > high values we are losing precision and is giving unexpected
> > values.
> > > > >
> > > > > Can you reduce the problem to a bogus result of
> > mul_u64_u64_div_u64()?
> > > > > I'd be very surprised if the problem was mul_u64_u64_div_u64()
> > > > > and not how it's used in your pwm driver.
> > > >
> > > > When I looked last time, it drops precision here[1]. I will
> > > > recheck
> > again.
> > > > On RZ/G2L family devices, the PWM rate is 100MHz.
> > > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v6.7-rc4/source/lib/math/div64.c#L2
> > > 14
> > >
> > >
> > > Please find the bug details in mul_u64_u64_div_u64() compared to
> > > mul_u64_u32_div()
> > >
> > > Theoretical calculation:
> > >
> > > Period = 43980465100800 nsec
> > > Duty_cycle = 23980465100800 nsec
> > > PWM rate = 100MHz
> > >
> > > period_cycles(tmp) = 43980465100800 * (100 * 10 ^ 6) / (10 ^ 9) =
> > > 4398046510080 prescale = ((43980465100800 >> 32) >= 256) = 5
> > > period_cycles = min (round_up(4398046510080,( 1 << (2 * 5 )),
> > > U32_MAX) = min (4295162607, U32_MAX) = U32_MAX = 0xFFFFFFFF
> > > duty_cycles = min (2398046510080, ,( 1 << (2 * 5 )), U32_MAX) = min
> > > (2341842295,
> > > U32_MAX) = 0x8B95AD77
> > >
> > >
> > > with mul_u64_u64_div_u64 (ARM64):
> > > [ 54.551612] ##### period_cycles_norm=43980465100800
> > > [ 54.305923] ##### period_cycles_tmp=4398035251080 ---> This is the
> > bug.
> >
> > It took me a while to read from your mail that
> >
> > mul_u64_u64_div_u64(43980465100800, 100000000, 1000000000)
> >
> > yields 4398035251080 on your machine (which isn't the exact result).
> >
> > I came to the same conclusion, damn, I thought mul_u64_u64_div_u64()
> > was exact. I wonder if it's worth to improve that. One fun fact is
> > that while mul_u64_u64_div_u64(43980465100800, 100000000, 1000000000)
> > yields
> > 4398035251080 (which is off by 11259000), swapping the parameters (and
> > thus using mul_u64_u64_div_u64(100000000, 43980465100800, 1000000000))
> > yields 4398046510080 which is the exact result.
> >
> > So this exact issue can be improved by:
> >
> > diff --git a/lib/math/div64.c b/lib/math/div64.c index
> > 55a81782e271..9523c3cd37f7 100644
> > --- a/lib/math/div64.c
> > +++ b/lib/math/div64.c
> > @@ -188,6 +188,9 @@ u64 mul_u64_u64_div_u64(u64 a, u64 b, u64 c)
> > u64 res = 0, div, rem;
> > int shift;
> >
> > + if (a > b)
> > + return mul_u64_u64_div_u64(b, a, c);
> > +
> > /* can a * b overflow ? */
> > if (ilog2(a) + ilog2(b) > 62) {
> > /*
> >
> > but the issue stays in principle. I'll think about that for a while.
>
> OK, I found a way to fix this issue
>
> static inline u64 rzg2l_gpt_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64
> c) {
> u64 retval;
>
> if (a > b)
> retval = mul_u64_u64_div_u64(b, a, c / 2);
> else
> retval = mul_u64_u64_div_u64(a, b, c / 2);
>
> return DIV64_U64_ROUND_UP(retval, 2);
> }
>
> In my case divisor is multiple of 2 as it is clk frequency.
>
> a = 43980465100800, b= 100000000, c = 1000000000, expected result after
> rounding up = 4398046510080
>
> with using above api,
>
> 43980465100800 * 100000000 / 500000000 = 8796093020160. roundup
> (8796093020160, 2) = 4398046510080
>
> I am planning to send v18 with these changes.
>
> Please let me know if you have any comments.
I found another way to avoid overflow and also we are not losing any precision.
+ * Rate is in MHz and is always integer for peripheral clk
+ * 2^32(val) * 2^10 (prescalar) * 10^9 > 2^64
+ * 2^32(val) * 2^10 (prescalar) * 10^6 < 2^64
+ * Multiply val with prescalar first, if the result is less than
+ * 2^34, then multiply by 10^9. Otherwise divide nr and dr by 10^3
+ * so that it will never overflow.
*/
Here I can useDIV64_U64_ROUND_UP() instead. I will send v18 based on this.
Cheers,
Biju
next prev parent reply other threads:[~2024-02-20 16:04 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-20 11:33 [PATCH v17 0/4] Add support for RZ/G2L GPT Biju Das
2023-11-20 11:33 ` [PATCH v17 1/4] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2023-11-20 11:33 ` [PATCH v17 2/4] dt-bindings: pwm: rzg2l-gpt: Document renesas,poegs property Biju Das
2023-11-20 11:33 ` [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT Biju Das
2023-12-04 14:55 ` Biju Das
2023-12-06 18:38 ` Uwe Kleine-König
2023-12-07 18:26 ` Biju Das
2023-12-07 21:41 ` Uwe Kleine-König
2023-12-08 10:34 ` Biju Das
2023-12-08 14:07 ` Uwe Kleine-König
2023-12-08 14:11 ` Biju Das
2023-12-13 9:06 ` Biju Das
2023-12-13 11:40 ` Uwe Kleine-König
2024-02-09 13:39 ` Biju Das
2024-02-20 16:04 ` Biju Das [this message]
2023-11-20 11:33 ` [PATCH v17 4/4] pwm: rzg2l-gpt: Add support for gpt linking with poeg Biju Das
2023-12-25 18:29 ` Uwe Kleine-König
2024-02-09 13:00 ` Biju Das
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=TYCPR01MB11269807ECEDDEC9F47EA153B86502@TYCPR01MB11269.jpnprd01.prod.outlook.com \
--to=biju.das.jz@bp.renesas.com \
--cc=fabrizio.castro.jz@renesas.com \
--cc=geert+renesas@glider.be \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=p.zabel@pengutronix.de \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
--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 read-only IMAP folder(s) and NNTP newsgroup(s).