On Fri, Jul 08, 2022 at 12:47:37AM +0530, Vijaya Krishna Nivarthi 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>
> ---
> 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 | 88 +++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2e23b65..ac2df1c 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, 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) {
> + pr_err("%s: Invalid frequency\n", __func__);
Note, this is a driver, ALWAYS use dev_err() and friends instead.
Also do not allow userspace to flood the kernel logs like this looks is
possible, this should just be dev_dbg().
And of course, never use __func__, it's not needed anymore for
dev_dbg().
> + return 0;
Why if you have a error, are you returning 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)
> + pr_err("Couldn't find suitable clock rate for %lu\n", desired_clk);
return an error?
dev_err().
> + else
> + pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
> + desired_clk, ser_clk, *clk_div);
dev_dbg()?
Also, as the kernel test robot says, this does not build cleanly :(
thanks,
greg k-h
