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

[Heiner Kallweit]

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.

> | 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);

>>>       }
>>>         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;
>>
>