| Message ID | 1657629905-24685-1-git-send-email-quic_vnivarth@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V5] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate. | expand |
Hi, On Tue, Jul 12, 2022 at 5:45 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> > --- > 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 <dianders@chromium.org>
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,
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,
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;
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> --- 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(-)