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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed8f95d2-ef62-d91a-618c-402ba1c9d09f@sberdevices.ru>
Date:   Mon, 5 Jun 2023 10:11:09 +0300
From:   George Stark <gnstark@...rdevices.ru>
To:     Heiner Kallweit <hkallweit1@...il.com>
CC:     "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "neil.armstrong@...aro.org" <neil.armstrong@...aro.org>,
        "martin.blumenstingl@...glemail.com" 
        <martin.blumenstingl@...glemail.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-amlogic@...ts.infradead.org" 
        <linux-amlogic@...ts.infradead.org>,
        kernel <kernel@...rdevices.ru>,
        Dmitry Rokosov <DDRokosov@...rdevices.ru>,
        "jbrunet@...libre.com" <jbrunet@...libre.com>,
        "khilman@...libre.com" <khilman@...libre.com>
Subject: Re: [PATCH] pwm: meson: compute cnt register value in proper way

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