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

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

[George Stark]

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--;
 	}
 
 	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);
 
 	return 0;

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

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

[Heiner Kallweit]

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

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.
And IIRC this should not happen.

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

>  	return 0;


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

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@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

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@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

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


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

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4143 bytes --]

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@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
> 
> 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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 516 bytes --]

Hello,

just a small note: This patch wasn't sent to the pwm mailing list and so
is missing in our patchwork instance. I didn't follow the discussion,
but please (and in your own interest) make sure to Cc:
linux-pwm@vger.kernel.org for the next revision (or resend if the
conclusion of the discussion is that the patch is fine).

Best regards
Uwe

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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