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

[Vijaya Krishna Nivarthi]

Hi,


On 6/9/2022 4:07 AM, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 8, 2022 at 11:34 AM Vijaya Krishna Nivarthi
> <quic_vnivarth@xxxxxxxxxxx> wrote:
> > 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:
> > > 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 )
> Ah, it took me a while to understand why two loops. It's because in
> one case you're trying multiplies and in the other you're bumping up
> to the next closest clock rate. I don't think you really need to do
> that. Just test the (rate - 2%) and the rate. How about this (only
> lightly tested):
>
>      ser_clk = 0;
>      maxdiv = CLK_DIV_MSK >> CLK_DIV_SHFT;
>      div = 1;
>      while (div < maxdiv) {

div <= maxdiv ?

>          mult = (unsigned long long)div * desired_clk;
>          if (mult != (unsigned long)mult)
>              break;
>
>          two_percent = mult / 50;
>
>          /*
>           * Loop requesting (freq - 2%) and possibly (freq).
>           *
>           * We'll keep track of the lowest freq inexact match we found
>           * but always try to find a perfect match. NOTE: this algorithm
>           * could miss a slightly better freq if there's more than one
>           * freq between (freq - 2%) and (freq) but (freq) can't be made
>           * exactly, but that's OK.
>           *
>           * This absolutely relies on the fact that the Qualcomm clock
>           * driver always rounds up.
>           */
>          test_freq = mult - two_percent;
>          while (test_freq <= mult) {
>              freq = clk_round_rate(clk, test_freq);
>
>              /*
>               * A dead-on freq is an insta-win. This implicitly
>               * handles when "freq == mult"
>               */
>              if (!(freq % desired_clk)) {
>                  *clk_div = freq / desired_clk;
>                  return freq;
>              }
>
>              /*
>               * Only time clock framework doesn't round up is if
>               * we're past the max clock rate. We're done searching
>               * if that's the case.
>               */
>              if (freq < test_freq)
>                  return ser_clk;
>
>              /* Save the first (lowest freq) within 2% */
>              if (!ser_clk && freq <= mult + two_percent) {
>                  ser_clk = freq;
>                  *clk_div = div;
>              }

My last concern is with search happening only within 2% tolerance.

Do we fail otherwise?

This real case has best tolerance of 1.9%.

[   17.963672] 20220530 desired_clk-51200000
[   21.193550] 20220530 returning ser_clk-52174000, div-1, diff-974000

Seems close.

Thank you.

>              /*
>               * If we already rounded up past mult then this will
>               * cause the loop to exit. If not then this will run
>               * the loop a second time with exactly mult.
>               */
>              test_freq = max(freq + 1, mult);
>          }
>
>          /*
>           * test_freq will always be bigger than mult by at least 1.
>           * That means we can get the next divider with a DIV_ROUND_UP.
>           * This has the advantage of skipping by a whole bunch of divs
>           * If the clock framework already bypassed them.
>           */
>          div = DIV_ROUND_UP(test_freq, desired_clk);
>          }
>
>      return ser_clk;