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 <[email protected]>
---
v5: corrected format specifiers for logs
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..f5b9b8e 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 %lu\n", desired_clk);
+ else
+ dev_dbg(dev, "desired_clk-%lu, ser_clk-%lu, clk_div-%u\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;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
Hi,
On Tue, Jul 12, 2022 at 5:45 AM Vijaya Krishna Nivarthi
<[email protected]> 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 <[email protected]>
> ---
> v5: corrected format specifiers for logs
> 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(-)
Reviewed-by: Douglas Anderson <[email protected]>
On 12. 07. 22, 14:45, Vijaya Krishna Nivarthi wrote:
...
> +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
"then within" ... "5" is missing, perhaps?
> + */
> + 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 %lu\n", desired_clk);
> + else
> + dev_dbg(dev, "desired_clk-%lu, ser_clk-%lu, clk_div-%u\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,
Maybe worth passing whole geni_se (port->se) then?
> + sampling_rate, &clk_div);
> if (!clk_rate)
> goto out_restart_rx;
>
thanks,
--
js
suse labs
On 7/14/2022 4:16 PM, Jiri Slaby wrote:
> On 12. 07. 22, 14:45, Vijaya Krishna Nivarthi wrote:
> ...
>> +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
>
> "then within" ... "5" is missing, perhaps?
Yes :(
Will change.
>
>> + */
>> + 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 %lu\n",
>> desired_clk);
>> + else
>> + dev_dbg(dev, "desired_clk-%lu, ser_clk-%lu, clk_div-%u\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,
>
> Maybe worth passing whole geni_se (port->se) then?
but then geni_se is a struct; shall we pass port instead?
Alternately move logging to caller?
Thank you.
>
>> + sampling_rate, &clk_div);
>> if (!clk_rate)
>> goto out_restart_rx;
>
> thanks,