[PATCH] pwm: meson: compute cnt register value in proper way
George Stark
gnstark at sberdevices.ru
Mon Jun 5 00:11:09 PDT 2023
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 at sberdevices.ru>
>> Signed-off-by: Dmitry Rokosov <ddrokosov at sberdevices.ru>
>> ---
>> This patch is based on currently unmerged patch by Heiner Kallweit
>> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com
>> ---
>> 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;
>
More information about the linux-amlogic
mailing list