Hi Uwe,
Thanks for your reply!
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Wednesday, October 4, 2023 3:15 PM
> Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
>
> Hello Fabrizio,
>
> On Tue, Oct 03, 2023 at 09:19:41PM +0000, Fabrizio Castro wrote:
> > > > + if (period)
> > > > + period += 1;
> > >
> > > This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
> >
> > Agreed. We should always add 1.
> >
> > > Also it suggests that the hardware cannot do a 100% relative duty
> > > cycle?
> >
> > It does support 100% duty cycle.
> > PWMCYC = 0 actually means 1 clock cycle, that's why the faff with
> > increment and decrement operations, and that's why the confusion.
>
> So it doesn't support a 0% relative duty cycle?
It does support 0% duty cycle.
>
> > > If I didn't miss anything here, please add that to the list of
> > > Limitations above.
> >
> > Thankfully not a limitation.
> >
> > >
> > > > + state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period
> << (4
> > > * prescale),
> > > > + rzv2m_pwm->rate);
> > >
> > > The multiplication can overflow. I see no easy way to prevent this
> > > without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
> > > something?
> >
> > It does overflow, good catch!
> > I think we could split this in three operations for now, and maybe
> > improve it later on:
> > period = NSEC_PER_SEC * (cyc + 1);
> > period = DIV64_U64_ROUND_UP(period, rzv2m_pwm->rate);
> > period <<= rzv2m_pwm_prescale_to_shift(prescale);
>
> You're loosing precision here though. With /^ = div_round_up:
>
> 1000000000 * 5 /^ 3 << 2 == 6666666668
> 1000000000 * 5 << 2 /^ 3 == 6666666667
Yep.
>
> So I correct my statement to: I see no easy and exact way to prevent
> an
> overflow without introducing a mul_u64_u64_div_roundup helper. :-)
Indeed. In my mind, the best way to tackle this problem is for such an
API to use an internal representation of 128-bit, so that it can calculate
(a*b + c - 1)/c accurately, and return both the high and low part of the
128-bit result (and maybe keeping the high part result optional?).
Something like
u64 mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c, u64 *res_high);
or
void mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c, u64 *res_high, u64 *res_low);
Needless to say, the API may take quite some time to perform the calculations,
but if precision/accuracy is what we need, there is probably no better
way of addressing this, unless you have a better idea?
Looking around, my understanding is that other drivers may benefit from
this sort of accurate rounded up result when a, b, and c are 64-bit, is that
why you think it has come the time to address this, or do you think this
is the first instance of such need?
Thanks,
Fab
>
> > with rzv2m_pwm_prescale_to_shift as below:
> > static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
> > {
> > return prescale == 3 ? 11 : prescale * 4;
> > }
> >
> > As it turns out "<< (4 * prescale)" and ">> (4 * prescale)" are not
> > correct for prescale == 3.
> > As per manual: PWMPS = 3 means a frequency division by 2048, and not
> > 4096.
>
> funny hardware.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> https://www.pengutronix.de/ |
