Hi Uwe, Thanks for the feedback > -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > 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@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. 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. Cheers, Biju
