diff mbox series

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

Message ID 1657560808-9795-1-git-send-email-quic_vnivarth@quicinc.com
State Superseded
Headers show
Series [V4] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate. | expand

Commit Message

Vijaya Krishna Nivarthi July 11, 2022, 5:33 p.m. UTC
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@quicinc.com>
---
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(-)

Comments

Doug Anderson July 11, 2022, 6:05 p.m. UTC | #1
Hi,

On Mon, Jul 11, 2022 at 10:33 AM Vijaya Krishna Nivarthi
<quic_vnivarth@quicinc.com> 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@quicinc.com>
> ---
> 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
Vijaya Krishna Nivarthi July 12, 2022, 12:51 p.m. UTC | #2
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@quicinc.com> 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@quicinc.com>
>> ---
>> 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/
diff mbox series

Patch

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);
 
 	return ser_clk;
 }
@@ -1021,8 +1040,8 @@  static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	if (ver >= QUP_SE_VERSION_2_5)
 		sampling_rate /= 2;
 
-	clk_rate = get_clk_div_rate(port->se.clk, baud,
-		sampling_rate, &clk_div);
+	clk_rate = get_clk_div_rate(port->se.clk, port->se.dev, baud,
+					sampling_rate, &clk_div);
 	if (!clk_rate)
 		goto out_restart_rx;