* [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.
@ 2022-07-07 19:17 Vijaya Krishna Nivarthi
2022-07-08 4:48 ` kernel test robot
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Vijaya Krishna Nivarthi @ 2022-07-07 19:17 UTC (permalink / raw)
To: agross, bjorn.andersson, konrad.dybcio, gregkh, jirislaby,
linux-arm-msm, linux-serial, linux-kernel
Cc: quic_msavaliy, dianders, mka, swboyd, Vijaya Krishna Nivarthi
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>
---
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__);
+ 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)
+ pr_err("Couldn't find suitable clock rate for %lu\n", desired_clk);
+ else
+ pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
+ desired_clk, ser_clk, *clk_div);
return ser_clk;
}
@@ -1021,8 +1040,7 @@ 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, 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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.
2022-07-07 19:17 [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate Vijaya Krishna Nivarthi
@ 2022-07-08 4:48 ` kernel test robot
2022-07-08 7:02 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-07-08 4:48 UTC (permalink / raw)
To: Vijaya Krishna Nivarthi, agross, bjorn.andersson, konrad.dybcio,
gregkh, jirislaby, linux-arm-msm, linux-serial, linux-kernel
Cc: kbuild-all, quic_msavaliy, dianders, mka, swboyd,
Vijaya Krishna Nivarthi
Hi Vijaya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vijaya-Krishna-Nivarthi/tty-serial-qcom-geni-serial-Fix-get_clk_div_rate-which-otherwise-could-return-a-sub-optimal-clock-rate/20220708-031921
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: nios2-randconfig-r024-20220707 (https://download.01.org/0day-ci/archive/20220708/202207081234.3SPgl4KO-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fbd8a1a4b7d91ea5caa048e4557ab18b0d08ea86
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vijaya-Krishna-Nivarthi/tty-serial-qcom-geni-serial-Fix-get_clk_div_rate-which-otherwise-could-return-a-sub-optimal-clock-rate/20220708-031921
git checkout fbd8a1a4b7d91ea5caa048e4557ab18b0d08ea86
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:29,
from include/linux/clk.h:13,
from drivers/tty/serial/qcom_geni_serial.c:4:
drivers/tty/serial/qcom_geni_serial.c: In function 'get_clk_div_rate':
>> drivers/tty/serial/qcom_geni_serial.c:1006:26: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'unsigned int' [-Wformat=]
1006 | pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:370:21: note: in definition of macro 'pr_fmt'
370 | #define pr_fmt(fmt) fmt
| ^~~
include/linux/dynamic_debug.h:152:9: note: in expansion of macro '__dynamic_func_call'
152 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:162:9: note: in expansion of macro '_dynamic_func_call'
162 | _dynamic_func_call(fmt, __dynamic_pr_debug, \
| ^~~~~~~~~~~~~~~~~~
include/linux/printk.h:604:9: note: in expansion of macro 'dynamic_pr_debug'
604 | dynamic_pr_debug(fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~
drivers/tty/serial/qcom_geni_serial.c:1006:17: note: in expansion of macro 'pr_debug'
1006 | pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
| ^~~~~~~~
drivers/tty/serial/qcom_geni_serial.c:1006:67: note: format string is defined here
1006 | pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
| ~~^
| |
| long unsigned int
| %u
vim +1006 drivers/tty/serial/qcom_geni_serial.c
983
984 static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
985 unsigned int sampling_rate, unsigned int *clk_div)
986 {
987 unsigned long ser_clk;
988 unsigned long desired_clk;
989
990 desired_clk = baud * sampling_rate;
991 if (!desired_clk) {
992 pr_err("%s: Invalid frequency\n", __func__);
993 return 0;
994 }
995
996 /*
997 * try to find a clock rate within 2% tolerance, then within
998 */
999 ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 2);
1000 if (!ser_clk)
1001 ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 5);
1002
1003 if (!ser_clk)
1004 pr_err("Couldn't find suitable clock rate for %lu\n", desired_clk);
1005 else
> 1006 pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
1007 desired_clk, ser_clk, *clk_div);
1008
1009 return ser_clk;
1010 }
1011
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.
2022-07-07 19:17 [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate Vijaya Krishna Nivarthi
2022-07-08 4:48 ` kernel test robot
@ 2022-07-08 7:02 ` kernel test robot
2022-07-08 13:11 ` Greg KH
2022-07-09 18:46 ` kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-07-08 7:02 UTC (permalink / raw)
To: Vijaya Krishna Nivarthi, agross, bjorn.andersson, konrad.dybcio,
gregkh, jirislaby, linux-arm-msm, linux-serial, linux-kernel
Cc: llvm, kbuild-all, quic_msavaliy, dianders, mka, swboyd,
Vijaya Krishna Nivarthi
Hi Vijaya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vijaya-Krishna-Nivarthi/tty-serial-qcom-geni-serial-Fix-get_clk_div_rate-which-otherwise-could-return-a-sub-optimal-clock-rate/20220708-031921
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: hexagon-randconfig-r023-20220707 (https://download.01.org/0day-ci/archive/20220708/202207081429.4VXpQCls-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fbd8a1a4b7d91ea5caa048e4557ab18b0d08ea86
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vijaya-Krishna-Nivarthi/tty-serial-qcom-geni-serial-Fix-get_clk_div_rate-which-otherwise-could-return-a-sub-optimal-clock-rate/20220708-031921
git checkout fbd8a1a4b7d91ea5caa048e4557ab18b0d08ea86
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/tty/serial/qcom_geni_serial.c:1007:26: warning: format specifies type 'unsigned long' but the argument has type 'unsigned int' [-Wformat]
desired_clk, ser_clk, *clk_div);
^~~~~~~~
include/linux/printk.h:610:38: note: expanded from macro 'pr_debug'
no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/printk.h:131:17: note: expanded from macro 'no_printk'
printk(fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
include/linux/printk.h:480:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/printk.h:452:19: note: expanded from macro 'printk_index_wrap'
_p_func(_fmt, ##__VA_ARGS__); \
~~~~ ^~~~~~~~~~~
1 warning generated.
vim +1007 drivers/tty/serial/qcom_geni_serial.c
983
984 static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
985 unsigned int sampling_rate, unsigned int *clk_div)
986 {
987 unsigned long ser_clk;
988 unsigned long desired_clk;
989
990 desired_clk = baud * sampling_rate;
991 if (!desired_clk) {
992 pr_err("%s: Invalid frequency\n", __func__);
993 return 0;
994 }
995
996 /*
997 * try to find a clock rate within 2% tolerance, then within
998 */
999 ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 2);
1000 if (!ser_clk)
1001 ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 5);
1002
1003 if (!ser_clk)
1004 pr_err("Couldn't find suitable clock rate for %lu\n", desired_clk);
1005 else
1006 pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
> 1007 desired_clk, ser_clk, *clk_div);
1008
1009 return ser_clk;
1010 }
1011
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.
2022-07-07 19:17 [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate Vijaya Krishna Nivarthi
2022-07-08 4:48 ` kernel test robot
2022-07-08 7:02 ` kernel test robot
@ 2022-07-08 13:11 ` Greg KH
2022-07-11 14:22 ` Vijaya Krishna Nivarthi
2022-07-09 18:46 ` kernel test robot
3 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2022-07-08 13:11 UTC (permalink / raw)
To: Vijaya Krishna Nivarthi
Cc: agross, bjorn.andersson, konrad.dybcio, jirislaby, linux-arm-msm,
linux-serial, linux-kernel, quic_msavaliy, dianders, mka, swboyd
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@quicinc.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.
2022-07-07 19:17 [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate Vijaya Krishna Nivarthi
` (2 preceding siblings ...)
2022-07-08 13:11 ` Greg KH
@ 2022-07-09 18:46 ` kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-07-09 18:46 UTC (permalink / raw)
To: Vijaya Krishna Nivarthi, agross, bjorn.andersson, konrad.dybcio,
gregkh, jirislaby, linux-arm-msm, linux-serial, linux-kernel
Cc: kbuild-all, quic_msavaliy, dianders, mka, swboyd,
Vijaya Krishna Nivarthi
Hi Vijaya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vijaya-Krishna-Nivarthi/tty-serial-qcom-geni-serial-Fix-get_clk_div_rate-which-otherwise-could-return-a-sub-optimal-clock-rate/20220708-031921
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arc-randconfig-r004-20220707 (https://download.01.org/0day-ci/archive/20220710/202207100234.kIBTfg5O-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fbd8a1a4b7d91ea5caa048e4557ab18b0d08ea86
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vijaya-Krishna-Nivarthi/tty-serial-qcom-geni-serial-Fix-get_clk_div_rate-which-otherwise-could-return-a-sub-optimal-clock-rate/20220708-031921
git checkout fbd8a1a4b7d91ea5caa048e4557ab18b0d08ea86
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:29,
from include/linux/clk.h:13,
from drivers/tty/serial/qcom_geni_serial.c:4:
drivers/tty/serial/qcom_geni_serial.c: In function 'get_clk_div_rate':
>> include/linux/kern_levels.h:5:25: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:452:25: note: in definition of macro 'printk_index_wrap'
452 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:131:17: note: in expansion of macro 'printk'
131 | printk(fmt, ##__VA_ARGS__); \
| ^~~~~~
include/linux/printk.h:610:9: note: in expansion of macro 'no_printk'
610 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
include/linux/kern_levels.h:15:25: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:610:19: note: in expansion of macro 'KERN_DEBUG'
610 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
drivers/tty/serial/qcom_geni_serial.c:1006:17: note: in expansion of macro 'pr_debug'
1006 | pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
| ^~~~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.
2022-07-08 13:11 ` Greg KH
@ 2022-07-11 14:22 ` Vijaya Krishna Nivarthi
0 siblings, 0 replies; 6+ messages in thread
From: Vijaya Krishna Nivarthi @ 2022-07-11 14:22 UTC (permalink / raw)
To: Greg KH
Cc: agross, bjorn.andersson, konrad.dybcio, jirislaby, linux-arm-msm,
linux-serial, linux-kernel, quic_msavaliy, dianders, mka, swboyd
On 7/8/2022 6:41 PM, Greg KH wrote:
> 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@quicinc.com>
>> ---
>> 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().
Ok.
>
>> + return 0;
> Why if you have a error, are you returning 0?
Yes, and it has been so earlier too.
0 is an invalid clock rate and will be handled accordingly by caller.
>> }
>>
>> - *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().
As mentioned, we didn't (and don't) return error from here but 0.
>
>> + else
>> + pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
>> + desired_clk, ser_clk, *clk_div);
> dev_dbg()?
Ok.
>
> Also, as the kernel test robot says, this does not build cleanly :(
change to dev_dbg should take care of these.
Will do.
Thank you.
Vijay/
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-11 14:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 19:17 [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate Vijaya Krishna Nivarthi
2022-07-08 4:48 ` kernel test robot
2022-07-08 7:02 ` kernel test robot
2022-07-08 13:11 ` Greg KH
2022-07-11 14:22 ` Vijaya Krishna Nivarthi
2022-07-09 18:46 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.