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@xxxxxxxxxxxxxx> > > > 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@xxxxxxxxxxxxxx> > > > > > 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#L214 > > > 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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
