Hi,
On Tue, Jun 7, 2022 at 10:40 AM Vijaya Krishna Nivarthi
<quic_vnivarth@xxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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 don't see why not. You're basically just getting a resulting clock
that's not an integral "Hz", right?
So if "baud" is 9600 and sampling_rate is 16 then desired_clk is (9600
* 16) = 153600
Let's imagine that we do all the math and we finally decide that our
best bet is with the rate 922000 and a divider of 6. That means that
the actual clock we'll make is 153666.67 when we _wanted_ 153600.
There's no reason it needs to be integral, though, and 153666.67 would
still be better than making 160000.
> >> 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.
Right, except that if you just pick the first clock you find it would
be _wildly_ off. I guess if you really want to do this the right way,
you need to set a maximum tolerance and pick the first rate you find
that meets that tolerance. Random web search for "uart baud rate
tolerance" makes me believe that +/- 5% deviation is OK, but to be
safe you probably want something lower. Maybe 2%? So if the desired
clock is within 2% of a clock you can make, can you just pick that
one?
> >> 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.
Ah, sorry. Not quite 1 line, but this (untested)
freq = clk_round_rate(clk, mult);
if (freq % desired_clk == 0) {
ser_clk = freq;
best_div = freq / desired_clk;
break;
}
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;
}
