Re: [V4] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 7/11/2022 11:35 PM, Doug Anderson wrote:
Hi,

On Mon, Jul 11, 2022 at 10:33 AM Vijaya Krishna Nivarthi
<quic_vnivarth@xxxxxxxxxxx> wrote:
In the logic around call to clk_round_rate(), for some corner conditions,
get_clk_div_rate() could return an sub-optimal clock rate. Also, if an
exact clock rate was not found lowest clock was being returned.

Search for suitable clock rate in 2 steps
a) exact match or within 2% tolerance
b) within 5% tolerance
This also takes care of corner conditions.

Fixes: c2194bc999d4 ("tty: serial: qcom-geni-serial: Remove uart frequency table. Instead, find suitable frequency with call to clk_round_rate")
Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@xxxxxxxxxxx>
---
v4: replaced pr_dbg calls with dev_dbg
v3: simplified algorithm further, fixed robot compile warnings
v2: removed minor optimisations to make more readable
v1: intial patch contained slightly complicated logic
---
  drivers/tty/serial/qcom_geni_serial.c | 89 +++++++++++++++++++++--------------
  1 file changed, 54 insertions(+), 35 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 2e23b65..f88b042 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -943,52 +943,71 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
         return 0;
  }

-static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
-                       unsigned int sampling_rate, unsigned int *clk_div)
+static unsigned long find_clk_rate_in_tol(struct clk *clk, unsigned int desired_clk,
+                       unsigned int *clk_div, unsigned int percent_tol)
  {
-       unsigned long ser_clk;
-       unsigned long desired_clk;
-       unsigned long freq, prev;
+       unsigned long freq;
         unsigned long div, maxdiv;
-       int64_t mult;
-
-       desired_clk = baud * sampling_rate;
-       if (!desired_clk) {
-               pr_err("%s: Invalid frequency\n", __func__);
-               return 0;
-       }
+       u64 mult;
+       unsigned long offset, abs_tol, achieved;

+       abs_tol = div_u64((u64)desired_clk * percent_tol, 100);
         maxdiv = CLK_DIV_MSK >> CLK_DIV_SHFT;
-       prev = 0;
-
-       for (div = 1; div <= maxdiv; div++) {
-               mult = div * desired_clk;
-               if (mult > ULONG_MAX)
+       div = 1;
+       while (div <= maxdiv) {
+               mult = (u64)div * desired_clk;
+               if (mult != (unsigned long)mult)
                         break;

-               freq = clk_round_rate(clk, (unsigned long)mult);
-               if (!(freq % desired_clk)) {
-                       ser_clk = freq;
-                       break;
-               }
+               offset = div * abs_tol;
+               freq = clk_round_rate(clk, mult - offset);

-               if (!prev)
-                       ser_clk = freq;
-               else if (prev == freq)
+               /* Can only get lower if we're done */
+               if (freq < mult - offset)
                         break;

-               prev = freq;
+               /*
+                * Re-calculate div in case rounding skipped rates but we
+                * ended up at a good one, then check for a match.
+                */
+               div = DIV_ROUND_CLOSEST(freq, desired_clk);
+               achieved = DIV_ROUND_CLOSEST(freq, div);
+               if (achieved <= desired_clk + abs_tol &&
+                   achieved >= desired_clk - abs_tol) {
+                       *clk_div = div;
+                       return freq;
+               }
+
+               div = DIV_ROUND_UP(freq, desired_clk);
         }

-       if (!ser_clk) {
-               pr_err("%s: Can't find matching DFS entry for baud %d\n",
-                                                               __func__, baud);
-               return ser_clk;
+       return 0;
+}
+
+static unsigned long get_clk_div_rate(struct clk *clk, struct device *dev,
+               unsigned int baud, unsigned int sampling_rate, unsigned int *clk_div)
+{
+       unsigned long ser_clk;
+       unsigned long desired_clk;
+
+       desired_clk = baud * sampling_rate;
+       if (!desired_clk) {
+               dev_dbg(dev, "Invalid frequency\n");
+               return 0;
         }

-       *clk_div = ser_clk / desired_clk;
-       if (!(*clk_div))
-               *clk_div = 1;
+       /*
+        * try to find a clock rate within 2% tolerance, then within
+        */
+       ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 2);
+       if (!ser_clk)
+               ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 5);
+
+       if (!ser_clk)
+               dev_err(dev, "Couldn't find suitable clock rate for %d\n", desired_clk);
+       else
+               dev_dbg(dev, "desired_clk-%d, ser_clk-%d, clk_div-%d\n",
+                               desired_clk, ser_clk, *clk_div);
Pretty sure the robot is going to yell at you again here. Here is my
analysis in detail:

desired_clk
* type: unsigned long
* proper format code: %lu
* marginally acceptable code: %ld
* format code you used in v3: %lu
* did robot yell at you about this in v3: no
* format code you used in v4: %d
* will robot yell at you in v4: YES

ser_clk:
* type: unsigned long
* proper format code: %lu
* marginally acceptable code: %ld
* format code you used in v3: %lu
* did robot yell at you about this in v3: no
* format code you used in v4: %d
* will robot yell at you in v4: YES

*clk_div:
* type: unsigned int
* proper format code: %u
* marginally acceptable code: %d
* format code you used in v3: %lu
* did robot yell at you about this in v3: YES
* format code you used in v4: %d
* will robot yell at you in v4: no
* should you change it to %u: yes


-Doug
Thank you very much.
For some reason robot seems to be more forgiving towards dev_funcs than pr_funcs, at least it did not yell at v4.

Also I found usage of %d for uls at many locations.

Made the changes and uploaded V5.

-Vijay/





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux