RE: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver

[Fabrizio Castro]

Hello Uwe,

Thanks for your feedback!

> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Friday, March 1, 2024 9:15 AM
> To: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
> 
> Hello Fabrizio,
> 
> your MUA introduces strange line breaks. You could do every reader of you mails a favour and fix that.

I think I understand what you mean, long lines would be wrapped onto new
lines, which are then indented incorrectly?
Hopefully it's been fixed now.
Thanks for highlighting this, as it has been going on for some time,
and was never noticed before.

> I fixed it up for my reply.

Thanks for that.

> 
> On Thu, Feb 29, 2024 at 10:45:01PM +0000, Fabrizio Castro wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > Sent: Thursday, February 29, 2024 4:42 PM
> > > To: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > > Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver
> > >
> > > On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> > > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a,
> > > > +u64 b,
> > > u64 c)
> > > > +{
> > > > +	u64 ab = a * b;
> > >
> > > a * b might overflow?!
> >
> > In the context of this driver, this cannot overflow.
> > The 2 formulas the above is needed for are:
> > 1) period = (cyc + 1)*(NSEC_PER_SEC * frequency_divisor)/rate
> > 2) duty_cycle = (cyc + 1 - low)*(NSEC_PER_SEC *
> > frequency_divisor)/rate
> >
> > With respect to 1), the dividend overflows when period * rate also
> > overflows (its product is calculated in rzv2m_pwm_config).
> > However, limiting the period to a maximum value of U64_MAX / rate
> > prevents the calculations from overflowing (in both directions, from period to cyc, and from cyc to
> period). v6 introduced max_period for this.
> > The situation for 2) is very similar to 1), with duty_cycle<=period,
> > therefore limiting period to a max value (and clamping the duty cycle
> > accordingly) will ensure that the calculation for duty_cycle won't
> > overflow, either.
> 
> OK, so it might be right from a technical POV. From a maintainer POV this is still bad. Authors for
> other drivers might copy it, or the driver might be changed and there is no indication that the the
> function relies on only be called with certain parameters.

I could add comments to clarify this, or checks to make sure the parameters
are passed as expected, or both?

Or if you have a better suggestion?

I would still like to be able to use the below formula if possible, as it
allows for the smallest restriction on the period:
(a * b) / c + ( (a * b) - (((a * b) / c) * c) ? 1 : 0 )

> 
> > > > +	u64 d = div64_u64(ab, c);
> > > > +	u64 e = d * c;
> > > > +
> > > > +	return d + ((ab - e) ? 1 : 0);
> > > > +}
> > > > +
> > > > +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a,
> > > > +u64 b,
> > > u64 c)
> > > > +{
> > > > +	return div64_u64(a * b, c);
> > >
> > > ditto. This is the same function as mul_u64_u64_div_u64() isn't it?
> >
> > Since a * b cannot overflow in the case of this driver, I believe the
> > above to be a better option than mul_u64_u64_div_u64.
> 
> Same technical POV vs maintainer POV as above. Plus: Even if
> mul_u64_u64_div_u64 is a tad slower, reusing it has some benefits nevertheless.

I'll use mul_u64_u64_div_u64 instead.

> 
> > > > [...]
> > > > +	cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > > > +	state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> > > > +				NSEC_PER_SEC * frequency_divisor,
> > > > +				rzv2m_pwm->rate);
> > > > +
> > > > +	low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > > > +	state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
> > > > +				NSEC_PER_SEC * frequency_divisor,
> > > > +				rzv2m_pwm->rate);
> > >
> > > The register semantic makes me wonder if each period starts with the
> > > low part. In that case the hardware called "normal" what is called
> > > inverted in the pwm framework?!
> >
> > My understanding is that the PWM framework defines "normal" polarity a
> > signal that starts high (and stays high) for the duration of the duty
> > cycle, and goes low for the remainder of the period. Conversely, a
> > signal with "inversed" polarity starts low (and stays low) for the
> > duration of the duty cycle and goes high for the remainder of the period.
> 
> Ack.
> 
> > This IP _does_ start low, but it _doesn't_ stay low for the duration
> > of the duty cycle, as it then goes high for the duration of the duty
> > cycle, therefore this IP doesn't perfectly fit either ("normal" or
> > "inverted") definitions.
> > I think you can say that the "normal" signal is _shifted_ in phase for
> > this IP, rather than being "inverted".
> 
> Alternatively (and a better match): What you describe is an inverted wave form with duty_cycle = period
> - duty_cycle.

That is also true. Also, it'll have the benefit of getting the first
period out of the door without a shift in phase.
I'll adjust accordingly.

> 
> > > > +	return pm_runtime_put(chip->dev);
> > >
> > > If you evaluate the return value of pm_runtime_put() maybe check
> > > pm_runtime_get_sync() for symmetry, too?
> >
> > Or I could just discard it and return 0?
> > I am fine with either, what's your preference?
> 
> My preference would be to always check the return value, but given that many drivers don't care for
> that, I agree to accept never checking it.
> So choose one option and do it consistently please.

Thanks.

> 
> > > > +	if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> > > > +		pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;
> > >
> > > I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
> > > pwm_cyc - 1).
> >
> > CYC is only made of 24 bits, therefore this is to make sure we don't
> > go beyond a 24-bit representation.
> 
> I would have understood:
> 
> 	if (FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc + 1))
> 		pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;

By the time the above test is run, pwm_cyc is incremented by 1, therefore
it needs to be decremented in order to be tested, and incremented when
re-assigned. pwm_cyc was left incremented to simplify the pwm_low calculation,
but I can see it's very confusing, therefore I'll improve this in v8.

Cheers,
Fab

> 
> Notice there are three changes compared to your variant:
>  - drop pwm_cyc != 0 check
>  - drop ! from FIELD_FIT
>  - pwm_cyc + 1 instead of pwm_cyc - 1
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |