Messages in this thread | | | Date | Tue, 7 Jun 2022 23:10:25 +0530 | Subject | Re: [PATCH] tty: serial: qcom-geni-serial: minor fixes to get_clk_div_rate() | From | Vijaya Krishna Nivarthi <> |
| |
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@quicinc.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@quicinc.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
| |