Re: [PATCH] pwm: meson: compute cnt register value in proper way

[George Stark]

On 6/2/23 23:52, Heiner Kallweit wrote:
> On 02.06.2023 12:32, George Stark wrote:
> > According to the datasheet, the PWM high and low clock count values
> > should be set to at least one. Therefore, setting the clock count
> > register to 0 actually means 1 clock count.
> >
> > Signed-off-by: George Stark <GNStark@xxxxxxxxxxxxxx>
> > Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx>
> > ---
> > This patch is based on currently unmerged patch by Heiner Kallweit
> > https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@xxxxxxxxx
> > ---
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index 834acd7..57e7d9c 100644
> > --- a/drivers/pwm/pwm-meson.c
> > +++ b/drivers/pwm/pwm-meson.c
> > @@ -206,6 +206,11 @@
> >   		channel->pre_div = pre_div;
> >   		channel->hi = duty_cnt;
> >   		channel->lo = cnt - duty_cnt;
> > +
> > +		if (channel->hi)
> > +			channel->hi--;
> > +		if (channel->lo)
> > +			channel->lo--;
Hello Heiner

Thanks for review
> I'm not sure whether we should do this. duty_cnt and cnt are results
> of an integer division and therefore potentially rounded down.
> The chip-internal increment may help to compensate such rounding
> errors, so to say. With the proposed change we may end up with the
> effective period being shorter than the requested one.
Although chip-internal increment sometimes may help accidentally
there are cases when the increment ruins precise calculation in 
unexpected way.

Here's our experience on meson a113l (meson-a1) with pwm driver based on 
ccf:
we need to get pwm period as close as possible to 32768hz.
config pwm to period 1/32768 = 30517ns, duty 15258n
How driver calculates hi\lo regs:
rate = NSEC_PER_SEC * 0xffff / 30517 = ~2147Mhz
rate = clk_round_rate(rate) clk_round_rate selects fastest parent clock 
which is 64Mhz in our case then calculating hi\lo at last: period= 
mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC); // 1953
duty= mul_u64_u64_div_u64(rate, state->duty_cycle, NSEC_PER_SEC); // 976
channel->hi= duty;
channel->lo= period- duty;
with the internal increment we'll have real output (1953-976 + 1 + 976 + 
1) * 1 / 64Mhz = 32736.57Hz but we should have (1953-976 + 976) * 1 / 
64Mhz = 32770.09Hz
| And IIRC this should not happen.
Could you please explain why or point out doc/description where it's stated?
If so we can add explicit check to prevent such a case
> >   	}
> >   
> >   	return 0;
> > @@ -340,7 +345,8 @@
> >   	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
> >   	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
> >   
> > -	state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi);
> > +	state->period = meson_pwm_cnt_to_ns(chip, pwm,
> > +					    channel->lo + 1 + channel->hi + 1);
> >   	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> >   
> Doesn't channel->hi have to be incremented here too?
Yes, lost the line. I'll fix it

Best regards
George
> >   	return 0;