by Vijaya Krishna Nivarthi (Temp)

[permalink] [raw]

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

Hi,

For desired_clk = 100 and clock rates like 1st from below, DIV_ROUND_UP seems to cause missing candidate solutions.

static unsigned long clk_round_rate_test(struct clk *clk, unsigned long in_freq)
{
//unsigned long root_freq[] = {301, 702, 1004};
//unsigned long root_freq[] = {301, 702, 1004, 2000, 3000};
//unsigned long root_freq[] = {50, 97, 99};
//unsigned long root_freq[] = {50, 97, 99, 200};
//unsigned long root_freq[] = {92, 110, 193, 230};
//unsigned long root_freq[] = {92, 110, 193, 230, 300, 401};
//unsigned long root_freq[] = {92, 110, 193, 230, 295, 296, 297, 401};
//unsigned long root_freq[] = {92, 110, 193, 230, 295, 296, 297, 300, 401};
//unsigned long root_freq[] = {197, 198, 199};
unsigned long root_freq[] = {197, 198, 199, 200};
int i;
size_t n = sizeof root_freq / sizeof *root_freq;

for (i = 0; i < n; i++) {
if (root_freq[i] >= in_freq)
return root_freq[i];
}
return root_freq[n-1];
}

I modified to handle such cases, optimised little and uploaded a patch.
It seems to work for all the cases like above.

Thank you,
Vijay/

>
> 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) {
> 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;
> }
>
> /*
> * 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;

2022-06-24 00:06:32

by Doug Anderson

[permalink] [raw]

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

Hi,

On Tue, Jun 21, 2022 at 10:58 AM Vijaya Krishna Nivarthi (Temp)
<[email protected]> wrote:
>
> Hi,
>
> For desired_clk = 100 and clock rates like 1st from below, DIV_ROUND_UP seems to cause missing candidate solutions.
>
> static unsigned long clk_round_rate_test(struct clk *clk, unsigned long in_freq)
> {
> //unsigned long root_freq[] = {301, 702, 1004};
> //unsigned long root_freq[] = {301, 702, 1004, 2000, 3000};
> //unsigned long root_freq[] = {50, 97, 99};
> //unsigned long root_freq[] = {50, 97, 99, 200};
> //unsigned long root_freq[] = {92, 110, 193, 230};
> //unsigned long root_freq[] = {92, 110, 193, 230, 300, 401};
> //unsigned long root_freq[] = {92, 110, 193, 230, 295, 296, 297, 401};
> //unsigned long root_freq[] = {92, 110, 193, 230, 295, 296, 297, 300, 401};
> //unsigned long root_freq[] = {197, 198, 199};
> unsigned long root_freq[] = {197, 198, 199, 200};
> int i;
> size_t n = sizeof root_freq / sizeof *root_freq;
>
> for (i = 0; i < n; i++) {
> if (root_freq[i] >= in_freq)
> return root_freq[i];
> }
> return root_freq[n-1];
> }
>
> I modified to handle such cases, optimised little and uploaded a patch.
> It seems to work for all the cases like above.

I think it would have been simpler to just change this little section:

/* Save the first (lowest freq) within 2% */
actual_mult = DIV_ROUND_CLOSEST(freq, desired_clk) * desired_clk;
if (!ser_clk && freq <= actual_mult + two_percent) {
ser_clk = freq;
*clk_div = div;
}

That was the only bug, right? Then you could keep the DIV_ROUND_UP() solution?

-Doug