| 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
| ||
|
Date: Tue, 7 Jun 2022 23:10:25 +0530
From: Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To: Doug Anderson <dianders@...omium.org>
CC: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
<linux-serial@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, <quic_msavaliy@...cinc.com>,
Matthias Kaehlcke <mka@...omium.org>,
"Stephen Boyd" <swboyd@...omium.org>
Subject: Re: [PATCH] tty: serial: qcom-geni-serial: minor fixes to
get_clk_div_rate()
Hi,
On 6/7/2022 1:29 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 6, 2022 at 11:19 AM Vijaya Krishna Nivarthi
> <quic_vnivarth@...cinc.com> wrote:
>> Hi,
>>
>>
>> On 6/4/2022 12:10 AM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Jun 3, 2022 at 10:43 AM Vijaya Krishna Nivarthi
>>> <quic_vnivarth@...cinc.com> wrote:
>>>> Ah, or I guess what you're saying is that the table historically
>>>> contained "rounded" rates but that clk_round_rate() isn't returning
>>>> nice round rates. OK, but if we truly want to support an inexact
>>>> match, you'd want to pick the rate that reduces the error, not just
>>>> pick the first one. In other words, something like this (untested):
>>>>
>>>> freq = clk_round_rate(clk, mult);
>>>> diff = abs(((long)mult - freq) / div);
>>>> if (diff < best_diff) {
>>>> best_diff = diff;
>>>> ser_clk = freq;
>>>> best_div = div;
>>>> }
>>>> I am not sure if its required that freq is a multiple of best_div now
>>>> that we don't have a multiple of desired_clk anyway.
>>> How about just this (untested):
>>>
>>> freq = clk_round_rate(clk, mult);
>>> candidate_div = max(1, DIV_ROUND_CLOSEST(freq, desired_clk));
>>> candidate_freq = freq / candidate_div;
>>> diff = abs((long)desired_clk - candidate_freq);
>>> if (diff < best_diff) {
>>> best_diff = diff;
>>> ser_clk = freq;
>>> best_div = candidate_div;
>>> }
>> I am afraid this still doesn't guarantee that ser_clk is a multiple of
>> best_div
> OK. ...I guess my question would be: does it matter for some reason?
> "ser_clk" is just a local variable in this function. Who cares if it's
> not a multiple of best_div? This is why we're keeping track of
> "best_div" in the first place, so that later in the function instead
> of:
>
> *clk_div = ser_clk / desired_clk;
> if (!(*clk_div))
> *clk_div = 1;
>
> You just do:
>
> *clk_div = best_div;
My only concern continues to be...
Given ser_clk is the final frequency that this function is going to
return and best_div is going to be the clk_divider, is it ok if the
divider cant divide the frequency exactly?
In other words, Can this function output combinations like (402,4)
(501,5) ?
If ok, then we can go ahead with this patch or even previous perhaps.
>
>> I tested it with a function simulates clk_round_rate.
>>
>> static unsigned long clk_round_rate_test(struct clk *clk, unsigned long
>> in_freq)
>> {
>> unsigned long root_freq[6] = {105, 204, 303, 402, 501, 602};
>> int i;
>>
>> for (i = 0; i < 6; i++) {
>> if (root_freq[i] >= in_freq)
>> return root_freq[i];
>> }
>> return root_freq[6];
>> }
>>
>> {
>> unsigned long ser_clk;
>> unsigned long desired_clk;
>> unsigned long freq;
>> int div_round_closest;
>> unsigned long div;
>> unsigned long mult;
>> unsigned long candidate_div, candidate_freq;
>>
>> unsigned long diff, best_diff, best_div;
>> unsigned long one;
>>
>> desired_clk = 100;
>> one = 1;
>> best_diff = ULONG_MAX;
>> pr_err("\ndesired_clk-%d\n", desired_clk);
>> for (div = 1; div <= 10; div++) {
>> mult = div * desired_clk;
>>
>> freq = clk_round_rate_test(clk, mult);
>> div_round_closest = DIV_ROUND_CLOSEST(freq, desired_clk);
>> candidate_div = max(one, (unsigned long)div_round_closest);
>> candidate_freq = freq / candidate_div;
>> diff = abs((long)desired_clk - candidate_freq);
>> pr_err("div-%d, mult-%d, freq-%d, div_round_closest-%d,
>> candidate_div-%d, candidate_freq-%d, diff-%d\n",
>> div, mult, freq, div_round_closest, candidate_div,
>> candidate_freq, diff);
>> if (diff < best_diff) {
>> pr_err("This is best so far\n");
>> best_diff = diff;
>> ser_clk = freq;
>> best_div = candidate_div;
>> }
>> }
>> pr_err("\nbest_diff-%d, ser_clk-%d, best_div-%d\n",
>> best_diff, ser_clk, best_div);
>> }
>>
>> And here is the output
>>
>> [ 17.835167] desired_clk-100
>> [ 17.839567] div-1, mult-100, freq-105, div_round_closest-1,
>> candidate_div-1, candidate_freq-105, diff-5
>> [ 17.849220] This is best so far
>> [ 17.852458] div-2, mult-200, freq-204, div_round_closest-2,
>> candidate_div-2, candidate_freq-102, diff-2
>> [ 17.862104] This is best so far
>> [ 17.865345] div-3, mult-300, freq-303, div_round_closest-3,
>> candidate_div-3, candidate_freq-101, diff-1
>> [ 17.874995] This is best so far
>> [ 17.878237] div-4, mult-400, freq-402, div_round_closest-4,
>> candidate_div-4, candidate_freq-100, diff-0
>> [ 17.887882] This is best so far
>> [ 17.891118] div-5, mult-500, freq-501, div_round_closest-5,
>> candidate_div-5, candidate_freq-100, diff-0
>> [ 17.900770] div-6, mult-600, freq-602, div_round_closest-6,
>> candidate_div-6, candidate_freq-100, diff-0
>> [ 17.910415] div-7, mult-700, freq-602, div_round_closest-6,
>> candidate_div-6, candidate_freq-100, diff-0
>> [ 17.920057] div-8, mult-800, freq-602, div_round_closest-6,
>> candidate_div-6, candidate_freq-100, diff-0
>> [ 17.929703] div-9, mult-900, freq-602, div_round_closest-6,
>> candidate_div-6, candidate_freq-100, diff-0
>> [ 17.939353] div-10, mult-1000, freq-602, div_round_closest-6,
>> candidate_div-6, candidate_freq-100, diff-0
>> [ 17.949181]
>> [ 17.949181] best_diff-0, ser_clk-402, best_div-4
> That doesn't look like a terrible result. I guess nominally 602 is a
> better approximation, but if we're accepting that we're not going to
> have an exact rate anyway then maybe being off by that tiny amount
> doesn't matter and we'd do better with the slow clock (maybe saves
> power?)
Actually power saving was the anticipation behind returning first
frequency in original patch, when we cant find exact frequency.
>
>> Please note that we go past cases when we have an divider that can
>> exactly divide the frequency(105/1, 204/2, 303/3) and end up with one
>> that doesn't.
> Ah, good point. Luckily that's a 1-line fix, right?
Apologies, I could not figure out how.
Thank you.
>
>
> -Doug
Powered by blists - more mailing lists