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

[Uwe Kleine-König]

On Mon, Jun 05, 2023 at 11:01:40PM +0200, Heiner Kallweit wrote:
> On 05.06.2023 09:11, George Stark wrote:
> > 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
> 
> Supposedly, depending on the prior rounding errors, something incrementing,
> and sometimes not incrementing may provide the more precise result.
> Another source of error is shown your example, the duty cycle isn't 50%
> due to the rounding.
> Not sure however where there's any use case where such small deviations
> would cause problems. Therefore I don't have a strong opinion.

My strong opinion here is: .apply should implement the the best
round-down setting. That is it should pick the maximal period that isn't
bigger than the requested one, and for that period pick the maximal
duty_cycle that isn't bigger than the requested one.

I have some implementation ideas to allow consumers to want different
rounding, but for that to work the .apply functions should all round the
same way.

> > | 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
> 
> I think I got this wrong. When checking where I got this information from
> I found the following in pwm_apply_state_debug():
> 
> if (state->enabled && state->period < s2.period)
>   dev_warn(chip->dev,
>   ".apply is supposed to round down period (requested: %llu, applied: %llu)\n",
>   state->period, s2.period);

pwm_apply_state_debug() also checks the above sketched rule.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature