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

[George Stark <gnstark@sberdevices.ru>]

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@sberdevices.ru>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@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;
>


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic