On Tue, Oct 10, 2023 at 10:48:19AM +0000, Fabrizio Castro wrote: > 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? The assumption of mul_u64_u64_div_u64 is that the result won't overflow a u64. Iff your hardware doesn't support a period > U64_MAX I think there is no need for an overflow-checking variant of mul_u64_u64_div_u64(). I didn't recheck, but this would be the first driver I'm aware of that can support a period > U64_MAX ns. But maybe it depends on the clkrate and if that's small enough it can happen?! Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
