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?
> > 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
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. :-)
> 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/ |
Attachment:
signature.asc
Description: PGP signature
