Re: [PATCH] tty: serial: qcom-geni-serial: minor fixes to get_clk_div_rate()

[Vijaya Krishna Nivarthi]

Hi,


On 6/8/2022 12:55 AM, Doug Anderson wrote:
> 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:
> >
> > 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.
Thank you for clarification.
> > > 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?
Ok, 2% seems good.
> > > > 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;
> }

But then once again, we would likely need 2 loops because while we are 
ok with giving up on search for best_div on finding something within 2% 
tolerance, we may not want to give up on exact match (freq % desired_clk 
== 0 )

So how about something like this with 2 loops (more optimised than 
previous version with 2 loops)? (untested)


    maxdiv = CLK_DIV_MSK >> CLK_DIV_SHFT;
    prev = 0;

    /* run through quicker loop anticipating to find an exact match */
    for (div = 1; div <= maxdiv; div++) {
        mult = (unsigned long long)div * desired_clk;
        if (mult > ULONG_MAX)
            break;

        freq = clk_round_rate(clk, max((unsigned long)mult, prev+1));
        if (!(freq % desired_clk)) {
            *clk_div = freq / desired_clk;
            return freq;
        }

        if (prev && prev == freq)
            break;

        prev = freq;
    }

    pr_warn("Can't find exact match frequency and divider\n");

    freq = 0;
    best_diff = ULONG_MAX;
    prev_candidate_div = -1;
    while (true) {
        prev = freq;
        freq = clk_round_rate(clk, freq+1);

        if (freq == prev)
            break; /* end of table */

        candidate_div = DIV_ROUND_CLOSEST(freq, desired_clk);
        /*
         * Since the frequencies are increasing, previous is better
         * if we have same divider, proceed to next in table
         */
        if (prev_candidate_div == candidate_div)
            continue;
        prev_candidate_div = candidate_div;

        if (candidate_div)
            candidate_freq = freq / candidate_div;
        else
            candidate_freq = freq;

        diff = abs((long)desired_clk - candidate_freq);
        if (diff < best_diff) {
            best_diff = diff;
            ser_clk = freq;
            *clk_div = candidate_div;
            if (diff * 50 < ser_clk) {
                two_percent_tolerance = true;
                break;
            }
        }
    }

    if (!two_percent_tolerance) {
        pr_warn("Can't find frequency within 2 percent tolerance\n");
    }

    return ser_clk;
}

Thank you.