Hello Biju,
On Thu, Dec 07, 2023 at 06:26:44PM +0000, Biju Das wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > Sent: Wednesday, December 6, 2023 6:38 PM
> > Subject: Re: [PATCH v17 3/4] pwm: Add support for RZ/G2L GPT
> > On Mon, Nov 20, 2023 at 11:33:06AM +0000, Biju Das wrote:
> > > +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
> > > +u32 val, u8 prescale) {
> > > + u64 retval;
> > > + u64 tmp;
> > > +
> > > + tmp = NSEC_PER_SEC * (u64)val;
> > > + /*
> > > + * To avoid losing precision for smaller period/duty cycle values
> > > + * ((2^32 * 10^9 << 2) < 2^64), do not process the rounded values.
> > > + */
> > > + if (prescale < 2)
> > > + retval = DIV64_U64_ROUND_UP(tmp << (2 * prescale), rzg2l_gpt-
> > >rate);
> > > + else
> > > + retval = DIV64_U64_ROUND_UP(tmp, rzg2l_gpt->rate) << (2 *
> > > +prescale);
> >
> > Maybe introduce a mul_u64_u64_div64_roundup (similar to
> > mul_u64_u64_div64) to also be exact for prescale >= 2?
>
> mul_u64_u64_div_u64()has a bug already and we are losing precision with very high values.
> See the output with CONFIG_PWM_DEBUG enabled[1]. So we cannot reuse mul_u64_u64_div64()
> for roundup operation.
>
> Maybe we need to come with 128bit operations for mul_u64_u64_div64_roundup().
> Do you have any specific algorithm in mind for doing mul_u64_u64_div64_roundup()?
>
> Fabrizio already developed an algorithm for 128 bit operation, I could reuse once it
> hits the mainline.
>
> [1]
> ######[ 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?
> > With prescale <= 5 and rzg2l_gpt->rate >= 1024 this shouldn't work just
> > fine.
I meant here "this should work just fine", I guess you understood that
right.
> Practically this is not possible. Yes, from theoretical point, rate=1kHz
> compared to the practical case, 100MHz won't work.
>
> For the practical case, the current logic is fine. If we need to achieve
> theoretical example like you mentioned then we need to have
> mul_u64_u64_div64_roundup().
That shouldn't be so hard to implement.
> > > + rzg2l_gpt->max_val = mul_u64_u64_div_u64(U32_MAX, NSEC_PER_SEC,
> > > + rzg2l_gpt->rate) * RZG2L_MAX_SCALE_FACTOR;
> >
> > Is it clear that this won't overflow?
U32_MAX * NSEC_PER_SEC doesn't even overflow an u64, so using a plain
u64 division would be more efficient.
You'd get a smaller rounding error with:
rzg2l_gpt->max_val = mul_u64_u64_div_u64((u64)U32_MAX * NSEC_PER_SEC, RZG2L_MAX_SCALE_FACTOR, rzg2l_gpt->rate);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
