Skip to contentSkip to conversation

clk: fractional-divider: Improve approximation when zero based

PATCH Superseded Frank Oltmanns 3 participants 13 messages Jun 13, 2023 – Jun 17, 2023 Common clk

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Frank Oltmanns

author
#0 lore raw

v2 cover clk: fractional-divider: Improve approximation when zero based

I stumpled upon this, when familiarizing myself with clk drivers. Unfortunately, I have no boards to test this patch. It seems the only user of this flag in mainline is drivers/clk/imx/clk-composite-7ulp.c, therefore I'm cc-ing get_maintainers.pl --git-blame -f drivers/clk/imx/clk-composite-7ulp.c in the hopes of a wider audience.

V1: https://lore.kernel.org/lkml/[email protected]/

Changes since V1:
- Added test case as requested by Stephen Boyd
- Fixed commit message as the Cc: was missing a closing bracket, so that the original mail unfortunately did not go out to A. s. Dong.

Thank you for considering this contribution,
Frank

Frank Oltmanns (2):
clk: fractional-divider: Improve approximation when zero based
clk: tests: Add tests for fractional divisor approximation

 drivers/clk/clk-fractional-divider.c | 26 ++++++++---
 drivers/clk/clk_test.c               | 69 +++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 8 deletions(-)
Signature · 1 line
2.41.0
author
#1 lore raw

PATCH v2 1/2 clk: fractional-divider: Improve approximation when zero based

Consider the CLK_FRAC_DIVIDER_ZERO_BASED flag when finding the best approximation for m and n. By doing so, increase the range of valid values for the numerator and denominator by 1.

Cc A.s. Dong <[email protected]>
Signed-off-by Frank Oltmanns <[email protected]>
---
 drivers/clk/clk-fractional-divider.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)
drivers/clk/clk-fractional-divider.c +19 -7
index 479297763e70..7da21cd2bdb1 100644
@@ -123,6 +123,7 @@ void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 						  unsigned long *m, unsigned long *n)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
+	unsigned long max_m, max_n;
 
 	/*
 	 * Get rate closer to *parent_rate to guarantee there is no overflow

@@ -138,9 +139,15 @@ void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 			rate <<= scale - fd->nwidth;
 	}
 
-	rational_best_approximation(rate, *parent_rate,
-			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
-			m, n);
+	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
+		max_m = 1 << fd->mwidth;
+		max_n = 1 << fd->nwidth;
+	} else {
+		max_m = GENMASK(fd->mwidth - 1, 0);
+		max_n = GENMASK(fd->nwidth - 1, 0);
+	}
+
+	rational_best_approximation(rate, *parent_rate, max_m, max_n, m, n);
 }
 
 static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,

@@ -169,13 +176,18 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long flags = 0;
-	unsigned long m, n;
+	unsigned long m, n, max_m, max_n;
 	u32 mmask, nmask;
 	u32 val;
 
-	rational_best_approximation(rate, parent_rate,
-			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
-			&m, &n);
+	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
+		max_m = 1 << fd->mwidth;
+		max_n = 1 << fd->nwidth;
+	} else {
+		max_m = GENMASK(fd->mwidth - 1, 0);
+		max_n = GENMASK(fd->nwidth - 1, 0);
+	}
+	rational_best_approximation(rate, parent_rate, max_m, max_n, &m, &n);
 
 	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
 		m--;
Signature · 1 line
2.41.0
author
#2 lore raw

PATCH v2 2/2 clk: tests: Add tests for fractional divisor approximation

In reply to Frank Oltmanns

In light of the recent discovery that the fractional divisor approximation does not utilize the full available range for clocks that are flagged CLK_FRAC_DIVIDER_ZERO_BASED, implement tests for the edge cases of this clock type.

Signed-off-by Frank Oltmanns <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here
---
 drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)
drivers/clk/clk_test.c +68 -1
index f9a5c2964c65..b247ba841cbd 100644
@@ -8,6 +8,9 @@
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
 
+/* Needed for clk_fractional_divider_general_approximation */
+#include "clk-fractional-divider.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)

@@ -2394,6 +2397,69 @@ static struct kunit_suite clk_mux_notifier_test_suite = {
 	.test_cases = clk_mux_notifier_test_cases,
 };
 
+
+/*
+ * Test that clk_fractional_divider_general_approximation will work with the
+ * highest available numerator and denominator.
+ */
+static void clk_fd_test_round_rate_max_mn(struct kunit *test)
+{
+	struct clk_fractional_divider *fd;
+	struct clk_hw *hw;
+	unsigned long rate, parent_rate, m, n;
+
+	fd = kunit_kzalloc(test, sizeof(*fd), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, fd);
+
+	fd->mwidth = 3;
+	fd->nwidth = 3;
+
+	hw = &fd->hw;
+
+	rate = DUMMY_CLOCK_RATE_1;
+
+	// Highest denominator, no flags
+	parent_rate = 10 * DUMMY_CLOCK_RATE_1;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 1);
+	KUNIT_EXPECT_EQ(test, n, 7);
+
+	// Highest numerator, no flags
+	parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 7);
+	KUNIT_EXPECT_EQ(test, n, 1);
+
+	// Highest denominator, zero based
+	parent_rate = 10 * DUMMY_CLOCK_RATE_1;
+	fd->flags = CLK_FRAC_DIVIDER_ZERO_BASED;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 1);
+	KUNIT_EXPECT_EQ(test, n, 8);
+
+	// Highest numerator, zero based
+	parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 8);
+	KUNIT_EXPECT_EQ(test, n, 1);
+}
+
+static struct kunit_case clk_fd_test_cases[] = {
+	KUNIT_CASE(clk_fd_test_round_rate_max_mn),
+	{}
+};
+
+/*
+ * Test suite for a fractional divider clock.
+ *
+ * These tests exercise the fractional divider API: clk_recalc_rate,
+ * clk_set_rate(), clk_round_rate().
+ */
+static struct kunit_suite clk_fd_test_suite = {
+	.name = "clk-fd-test",
+	.test_cases = clk_fd_test_cases,
+};
+
 kunit_test_suites(
 	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,

@@ -2406,6 +2472,7 @@ kunit_test_suites(
 	&clk_range_maximize_test_suite,
 	&clk_range_minimize_test_suite,
 	&clk_single_parent_mux_test_suite,
-	&clk_uncached_test_suite
+	&clk_uncached_test_suite,
+	&clk_fd_test_suite
 );
 MODULE_LICENSE("GPL v2");
Signature · 1 line
2.41.0
author
#3 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Hi Stephen,

Quoted 51 lines · Frank Oltmanns

In light of the recent discovery that the fractional divisor approximation does not utilize the full available range for clocks that are flagged CLK_FRAC_DIVIDER_ZERO_BASED, implement tests for the edge cases of this clock type.

Signed-off-by Frank Oltmanns <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here
---
 drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)
drivers/clk/clk_test.c +25 -0
index f9a5c2964c65..b247ba841cbd 100644
@@ -8,6 +8,9 @@
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
 
+/* Needed for clk_fractional_divider_general_approximation */
+#include "clk-fractional-divider.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE	(42 * 1000 * 1000)

@@ -2394,6 +2397,69 @@ static struct kunit_suite clk_mux_notifier_test_suite = {
 	.test_cases = clk_mux_notifier_test_cases,
 };
 
+
+/*
+ * Test that clk_fractional_divider_general_approximation will work with the
+ * highest available numerator and denominator.
+ */
+static void clk_fd_test_round_rate_max_mn(struct kunit *test)
+{
+	struct clk_fractional_divider *fd;
+	struct clk_hw *hw;
+	unsigned long rate, parent_rate, m, n;
+
+	fd = kunit_kzalloc(test, sizeof(*fd), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, fd);
+
+	fd->mwidth = 3;
+	fd->nwidth = 3;
+
+	hw = &fd->hw;
+
+	rate = DUMMY_CLOCK_RATE_1;
+
+	// Highest denominator, no flags

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2431

Quoted 10 lines
drivers/clk/clk_test.c +10 -0
+	parent_rate = 10 * DUMMY_CLOCK_RATE_1;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 1);
+	KUNIT_EXPECT_EQ(test, n, 7);
+
+	// Highest numerator, no flags
+	parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 7);
+	KUNIT_EXPECT_EQ(test, n, 1);

The two calls above aim at proving that the change does not break existing functionality.

Quoted 20 lines
drivers/clk/clk_test.c +20 -0
+
+	// Highest denominator, zero based
+	parent_rate = 10 * DUMMY_CLOCK_RATE_1;
+	fd->flags = CLK_FRAC_DIVIDER_ZERO_BASED;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 1);
+	KUNIT_EXPECT_EQ(test, n, 8);
+
+	// Highest numerator, zero based
+	parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+	clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+	KUNIT_EXPECT_EQ(test, m, 8);
+	KUNIT_EXPECT_EQ(test, n, 1);
+}
+
+static struct kunit_case clk_fd_test_cases[] = {
+	KUNIT_CASE(clk_fd_test_round_rate_max_mn),
+	{}
+};
+

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2461

Quoted 10 lines
drivers/clk/clk_test.c +10 -0
+/*
+ * Test suite for a fractional divider clock.
+ *
+ * These tests exercise the fractional divider API: clk_recalc_rate,
+ * clk_set_rate(), clk_round_rate().
+ */
+static struct kunit_suite clk_fd_test_suite = {
+	.name = "clk-fd-test",
+	.test_cases = clk_fd_test_cases,
+};

Unfortunately, the style of the tests does not really match with the style of the existing tests, because those where all aimed at the framework itself and not at specific functions.

Please let me know, if you require any changes.

Thanks,
Frank

Quoted 13 lines
drivers/clk/clk_test.c +3 -1
+
 kunit_test_suites(
 	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,

@@ -2406,6 +2472,7 @@ kunit_test_suites(
 	&clk_range_maximize_test_suite,
 	&clk_range_minimize_test_suite,
 	&clk_single_parent_mux_test_suite,
-	&clk_uncached_test_suite
+	&clk_uncached_test_suite,
+	&clk_fd_test_suite
 );
 MODULE_LICENSE("GPL v2");
build bot
#4 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

In reply to Frank Oltmanns
kernel test robot noticed the following build errors:

Hi Frank,

kernel test robot noticed the following build errors:

[auto build test ERROR on v6.4-rc6]
[also build test ERROR on linus/master]
[cannot apply to clk/clk-next next-20230613]
[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/Frank-Oltmanns/clk-fractional-divider-Improve-approximation-when-zero-based/20230613-163903
base:   v6.4-rc6
patch link:    https://lore.kernel.org/r/20230613083626.227476-3-frank%40oltmanns.dev
patch subject: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation
config: csky-randconfig-r011-20230612 (https://download.01.org/0day-ci/archive/20230613/[email protected]/config)
compiler: csky-linux-gcc (GCC) 12.3.0

reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
git checkout v6.4-rc6
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "clk_fractional_divider_general_approximation" [drivers/clk/clk_test.ko] undefined!
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
author
#5 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Hi,

Quoted 28 lines · kernel test robot

Hi Frank,

kernel test robot noticed the following build errors:

[auto build test ERROR on v6.4-rc6]
[also build test ERROR on linus/master]
[cannot apply to clk/clk-next next-20230613]
[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/Frank-Oltmanns/clk-fractional-divider-Improve-approximation-when-zero-based/20230613-163903
base:   v6.4-rc6
patch link:    https://lore.kernel.org/r/20230613083626.227476-3-frank%40oltmanns.dev
patch subject: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation
config: csky-randconfig-r011-20230612 (https://download.01.org/0day-ci/archive/20230613/[email protected]/config)
compiler: csky-linux-gcc (GCC) 12.3.0

reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
git checkout v6.4-rc6
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash

Quoted 8 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

Quoted 1 line
Quoted 1 line

ERROR: modpost: "clk_fractional_divider_general_approximation" [drivers/clk/clk_test.ko] undefined!

The issue seems to be that clk_fractional_divider_general_approximation is not exported as a symbol, while the config builds the clk_test as a module:
CONFIG_CLK_KUNIT_TEST=m

If I'm not mistaken, this means that I can't test clk_fractional_divider_general_approximation directly. Instead I'd have to test it using clk_fractional_divider_ops.round_rate.

Can someone more knowlegdable than me please confirm if my understanding is correct?

Thanks,
Frank

maintainer
#6 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Quoting Frank Oltmanns (2023-06-14 01:19:37)

Quoted 45 lines

Hi,

Quoted 28 lines · kernel test robot

Hi Frank,

kernel test robot noticed the following build errors:

[auto build test ERROR on v6.4-rc6]
[also build test ERROR on linus/master]
[cannot apply to clk/clk-next next-20230613]
[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/Frank-Oltmanns/clk-fractional-divider-Improve-approximation-when-zero-based/20230613-163903
base:   v6.4-rc6
patch link:    https://lore.kernel.org/r/20230613083626.227476-3-frank%40oltmanns.dev
patch subject: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation
config: csky-randconfig-r011-20230612 (https://download.01.org/0day-ci/archive/20230613/[email protected]/config)
compiler: csky-linux-gcc (GCC) 12.3.0

reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
git checkout v6.4-rc6
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash

Quoted 8 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

Quoted 1 line
Quoted 1 line

ERROR: modpost: "clk_fractional_divider_general_approximation" [drivers/clk/clk_test.ko] undefined!

The issue seems to be that clk_fractional_divider_general_approximation is not exported as a symbol, while the config builds the clk_test as a module:
CONFIG_CLK_KUNIT_TEST=m

Quoted 6 lines

If I'm not mistaken, this means that I can't test clk_fractional_divider_general_approximation directly. Instead I'd have to test it using clk_fractional_divider_ops.round_rate.

Can someone more knowlegdable than me please confirm if my understanding is correct?

Export the symbol.

author
#7 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Hi Stephen,

Quoted 46 lines · Stephen Boyd

Quoting Frank Oltmanns (2023-06-14 01:19:37)

Quoted 45 lines

Hi,

Quoted 28 lines · kernel test robot

Hi Frank,

kernel test robot noticed the following build errors:

[auto build test ERROR on v6.4-rc6]
[also build test ERROR on linus/master]
[cannot apply to clk/clk-next next-20230613]
[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/Frank-Oltmanns/clk-fractional-divider-Improve-approximation-when-zero-based/20230613-163903
base:   v6.4-rc6
patch link:    https://lore.kernel.org/r/20230613083626.227476-3-frank%40oltmanns.dev
patch subject: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation
config: csky-randconfig-r011-20230612 (https://download.01.org/0day-ci/archive/20230613/[email protected]/config)
compiler: csky-linux-gcc (GCC) 12.3.0

reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
git checkout v6.4-rc6
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash

Quoted 8 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

Quoted 1 line
Quoted 1 line

ERROR: modpost: "clk_fractional_divider_general_approximation" [drivers/clk/clk_test.ko] undefined!

The issue seems to be that clk_fractional_divider_general_approximation is not exported as a symbol, while the config builds the clk_test as a module:
CONFIG_CLK_KUNIT_TEST=m

Quoted 8 lines
Quoted 6 lines

If I'm not mistaken, this means that I can't test clk_fractional_divider_general_approximation directly. Instead I'd have to test it using clk_fractional_divider_ops.round_rate.

Can someone more knowlegdable than me please confirm if my understanding is correct?

Export the symbol.

Ok. I can do that. Please note that I had already submitted a V3 [1], that went the way of using clk_fractional_divider_ops.round_rate. I apologize for not waiting for your feedback prior to submission. It won't happen again.

I liked the approach of calling clk_fd_round_rate directly via the ops, because it might allow me to test the other ops as well using the same blueprint. Of course, I will not add test cases, if you don't want it. (Calling clk_fd_round_rate also had the side effect of teaching me, that fd clocks expect the fraction to be less than or equal to 1.)

I don't want to waste your time, but if you could maybe have a chance to look at the approach I took in V3 and tell me if you still want me to export the symbol instead, that would be really helpful. I'll follow your preference.

If I don't hear back until the weekend, I will treat your three words above as your preference and prepare a V4 that goes back to calling clk_fractional_divider_general_approximation directly.

Thank you,
Frank

[1]: https://lore.kernel.org/all/[email protected]/

maintainer
#8 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Quoting Frank Oltmanns (2023-06-14 22:16:17)

Quoted 23 lines

Hi Stephen,

Quoted 8 lines · Stephen Boyd

Quoting Frank Oltmanns (2023-06-14 01:19:37)

Quoted 5 lines

Hi,

On 2023-06-13 at 20:48:21 +0800, kernel test robot <[email protected]> wrote:
Can someone more knowlegdable than me please confirm if my understanding is correct?

Export the symbol.

Ok. I can do that. Please note that I had already submitted a V3 [1], that went the way of using clk_fractional_divider_ops.round_rate. I apologize for not waiting for your feedback prior to submission. It won't happen again.

I liked the approach of calling clk_fd_round_rate directly via the ops, because it might allow me to test the other ops as well using the same blueprint. Of course, I will not add test cases, if you don't want it. (Calling clk_fd_round_rate also had the side effect of teaching me, that fd clocks expect the fraction to be less than or equal to 1.)

Quoted 9 lines

I don't want to waste your time, but if you could maybe have a chance to look at the approach I took in V3 and tell me if you still want me to export the symbol instead, that would be really helpful. I'll follow your preference.

If I don't hear back until the weekend, I will treat your three words above as your preference and prepare a V4 that goes back to calling clk_fractional_divider_general_approximation directly.

Just call the API directly. That narrows the test to exactly what we want to test. If you export the API it will make the rockchip folks happy too[1]. We of course need to make sure that the registration API works as well and actually uses the widths that are passed in, but it doesn't need to fully exercise the approximation algorithm.

[1] https://lore.kernel.org/r/[email protected]

author
#9 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Hi Stephen,

Quoted 34 lines · Stephen Boyd

Quoting Frank Oltmanns (2023-06-14 22:16:17)

Quoted 23 lines

Hi Stephen,

Quoted 8 lines · Stephen Boyd

Quoting Frank Oltmanns (2023-06-14 01:19:37)

Quoted 5 lines

Hi,

On 2023-06-13 at 20:48:21 +0800, kernel test robot <[email protected]> wrote:
Can someone more knowlegdable than me please confirm if my understanding is correct?

Export the symbol.

Ok. I can do that. Please note that I had already submitted a V3 [1], that went the way of using clk_fractional_divider_ops.round_rate. I apologize for not waiting for your feedback prior to submission. It won't happen again.

I liked the approach of calling clk_fd_round_rate directly via the ops, because it might allow me to test the other ops as well using the same blueprint. Of course, I will not add test cases, if you don't want it. (Calling clk_fd_round_rate also had the side effect of teaching me, that fd clocks expect the fraction to be less than or equal to 1.)

Quoted 9 lines

I don't want to waste your time, but if you could maybe have a chance to look at the approach I took in V3 and tell me if you still want me to export the symbol instead, that would be really helpful. I'll follow your preference.

If I don't hear back until the weekend, I will treat your three words above as your preference and prepare a V4 that goes back to calling clk_fractional_divider_general_approximation directly.

Quoted 7 lines

Just call the API directly. That narrows the test to exactly what we want to test. If you export the API it will make the rockchip folks happy too[1]. We of course need to make sure that the registration API works as well and actually uses the widths that are passed in, but it doesn't need to fully exercise the approximation algorithm.

[1] https://lore.kernel.org/r/[email protected]

I've now submitted V5 [1] of this patchset. Unfortunately, V4 [2] had a compiler warning on clang that slipped through the cracks. I'm sorry for the noise.

In my opinion V5 is ready for review and hopefully addresses all your previous concerns.

Thank you for your patience.

Best regards,
Frank

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

maintainer
#10 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

In reply to Frank Oltmanns

Quoting Frank Oltmanns (2023-06-13 01:36:26)

Quoted 7 lines

In light of the recent discovery that the fractional divisor approximation does not utilize the full available range for clocks that are flagged CLK_FRAC_DIVIDER_ZERO_BASED, implement tests for the edge cases of this clock type.

Signed-off-by Frank Oltmanns <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here

What is the link for?

Quoted 2 lines
---
 drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-

Please make a new file, drivers/clk/clk-fractional-divider_test.c instead.

Quoted 32 lines
 1 file changed, 68 insertions(+), 1 deletion(-)
drivers/clk/clk_test.c +15 -0
index f9a5c2964c65..b247ba841cbd 100644
@@ -8,6 +8,9 @@
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
 
+/* Needed for clk_fractional_divider_general_approximation */
+#include "clk-fractional-divider.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE  (42 * 1000 * 1000)

@@ -2394,6 +2397,69 @@ static struct kunit_suite clk_mux_notifier_test_suite = {
        .test_cases = clk_mux_notifier_test_cases,
 };
 
+
+/*
+ * Test that clk_fractional_divider_general_approximation will work with the
+ * highest available numerator and denominator.
+ */
+static void clk_fd_test_round_rate_max_mn(struct kunit *test)
+{
+       struct clk_fractional_divider *fd;
+       struct clk_hw *hw;
+       unsigned long rate, parent_rate, m, n;
+
+       fd = kunit_kzalloc(test, sizeof(*fd), GFP_KERNEL);

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2421

Quoted 10 lines
drivers/clk/clk_test.c +10 -0
+       KUNIT_ASSERT_NOT_NULL(test, fd);
+
+       fd->mwidth = 3;
+       fd->nwidth = 3;
+
+       hw = &fd->hw;
+
+       rate = DUMMY_CLOCK_RATE_1;
+
+       // Highest denominator, no flags

Use /* this for comments */

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2422

Quoted 1 line
drivers/clk/clk_test.c +1 -0
+       parent_rate = 10 * DUMMY_CLOCK_RATE_1;

Just write out the actual frequency. Don't use DUMMY_CLOCK_RATE_1 at all in the test.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2425

Quoted 3 lines
drivers/clk/clk_test.c +3 -0
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 1);
+       KUNIT_EXPECT_EQ(test, n, 7);

This is a different test case.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2430

Quoted 5 lines
drivers/clk/clk_test.c +5 -0
+
+       // Highest numerator, no flags
+       parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 7);

Is 7 related to mwidth? Maybe this should be clk_div_mask(fd->mwidth) instead of 7.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2431

Quoted 1 line
drivers/clk/clk_test.c +1 -0
+       KUNIT_EXPECT_EQ(test, n, 1);

This is a different test case.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2438

Quoted 7 lines
drivers/clk/clk_test.c +7 -0
+
+       // Highest denominator, zero based
+       parent_rate = 10 * DUMMY_CLOCK_RATE_1;
+       fd->flags = CLK_FRAC_DIVIDER_ZERO_BASED;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 1);
+       KUNIT_EXPECT_EQ(test, n, 8);

This is a different test case.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2444

Quoted 6 lines
drivers/clk/clk_test.c +6 -0
+
+       // Highest numerator, zero based
+       parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 8);
+       KUNIT_EXPECT_EQ(test, n, 1);

This is a different test case. If it has a meaningful name it will be easier to understand as well.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

author
#11 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Hi Stephen,

Thank you for your feedback.

Quoted 10 lines · Stephen Boyd

Quoting Frank Oltmanns (2023-06-13 01:36:26)

Quoted 7 lines

In light of the recent discovery that the fractional divisor approximation does not utilize the full available range for clocks that are flagged CLK_FRAC_DIVIDER_ZERO_BASED, implement tests for the edge cases of this clock type.

Signed-off-by Frank Oltmanns <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here

What is the link for?

The intention was to show why the tests were added ("In light of the recent discovery..."). I announced this discovery in the mail I referred to. Since that intention didn't come across: Should I drop the link?

Quoted 52 lines
Quoted 3 lines
---
 drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-

Please make a new file, drivers/clk/clk-fractional-divider_test.c instead.

Quoted 32 lines
 1 file changed, 68 insertions(+), 1 deletion(-)
drivers/clk/clk_test.c +15 -0
index f9a5c2964c65..b247ba841cbd 100644
@@ -8,6 +8,9 @@
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
 
+/* Needed for clk_fractional_divider_general_approximation */
+#include "clk-fractional-divider.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE  (42 * 1000 * 1000)

@@ -2394,6 +2397,69 @@ static struct kunit_suite clk_mux_notifier_test_suite = {
        .test_cases = clk_mux_notifier_test_cases,
 };
 
+
+/*
+ * Test that clk_fractional_divider_general_approximation will work with the
+ * highest available numerator and denominator.
+ */
+static void clk_fd_test_round_rate_max_mn(struct kunit *test)
+{
+       struct clk_fractional_divider *fd;
+       struct clk_hw *hw;
+       unsigned long rate, parent_rate, m, n;
+
+       fd = kunit_kzalloc(test, sizeof(*fd), GFP_KERNEL);

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2421

Quoted 10 lines
drivers/clk/clk_test.c +10 -0
+       KUNIT_ASSERT_NOT_NULL(test, fd);
+
+       fd->mwidth = 3;
+       fd->nwidth = 3;
+
+       hw = &fd->hw;
+
+       rate = DUMMY_CLOCK_RATE_1;
+
+       // Highest denominator, no flags

Use /* this for comments */

Quoted 4 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2422

Quoted 1 line
drivers/clk/clk_test.c +1 -0
+       parent_rate = 10 * DUMMY_CLOCK_RATE_1;

Just write out the actual frequency. Don't use DUMMY_CLOCK_RATE_1 at all in the test.

Sure, will do. The idea was to highlight that we want to have the parent running at 10 times the speed, while the divider has a maximum value of 7 (or 8 if zero based).

Quoted 7 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2425

Quoted 4 lines
drivers/clk/clk_test.c +3 -0
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 1);
+       KUNIT_EXPECT_EQ(test, n, 7);

This is a different test case.

Quoted 8 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2430

Quoted 5 lines
drivers/clk/clk_test.c +5 -0
+
+       // Highest numerator, no flags
+       parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 7);

Is 7 related to mwidth? Maybe this should be clk_div_mask(fd->mwidth) instead of 7.

Yes, 7 is related to mwidth. I thought about this too, but I'm not sure that's the best move here. The function under test uses:

    max_m = GENMASK(fd->mwidth - 1, 0);
    max_n = GENMASK(fd->nwidth - 1, 0);

I come from a safety-concerned industry and as a general rule we avoid using functions from the code under test in our tests. I'm doing this here as a hobby, but still I find it to be a good rule that I'd like to stick to unless asked otherwise.

If I use the same code to generate the expected values, I'm not really testing my change, but only the underlying best_rational_approximation.

Instead, how about I add a comment to the test function that more thoroughly explains its intention?

Something along those lines:

/*
* Introduce a parent that runs at 10 times the frequency of the
* requested rate.
* m and n are 3 bits wide each.
* The clock has no flags set, hence the maximum value that fits in
* m and n is 7.
* Therefore, expect the highest possible divisor.
*/
static void clk_fd_test_round_rate_max_m(struct kunit *test)

/*
* Introduce a parent that runs at 1/10th the frequency of the
* requested rate.
* m and n are 3 bits wide each.
* The clock has no flags set, hence the maximum value that fits in
* m and n is 7.
* Therefore, expect the highest possible multiplier.
*/
static void clk_fd_test_round_rate_max_n(struct kunit *test)

/*
* Introduce a parent that runs at 10 times the frequency of the
* requested rate.
* m and n are 3 bits wide each.
* The clock is zero based, hence the maximum value that fits in
* m and n is 8.
* Therefore, expect the highest possible divisor.
*/
static void clk_fd_test_round_rate_max_m_zero_based(struct kunit *test)

/*
* Introduce a parent that runs at 1/10th the frequency of the
* requested rate.
* m and n are 3 bits wide each.
* The clock is zero based, hence the maximum value that fits in
* m and n is 8.
* Therefore, expect the highest possible multiplier.
*/
static void clk_fd_test_round_rate_max_n_zero_based(struct kunit *test)

Please note that from your original comment, I wasn't sure, if you wanted a one time test or someting that could become part of the clk-frameworks test suite. Therefore, I sent this test case to test the waters and ask for your comments. It's clear to me now that you want it to be permanent, so I'll spend some time on it to make it ready for inclusion. :)

Is there anything else you'd like me to cover in the tests for the fix?

Thanks,
Frank

Quoted 24 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2431

Quoted 2 lines
drivers/clk/clk_test.c +1 -0
+       KUNIT_EXPECT_EQ(test, n, 1);

This is a different test case.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2438

Quoted 7 lines
drivers/clk/clk_test.c +7 -0
+
+       // Highest denominator, zero based
+       parent_rate = 10 * DUMMY_CLOCK_RATE_1;
+       fd->flags = CLK_FRAC_DIVIDER_ZERO_BASED;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 1);
+       KUNIT_EXPECT_EQ(test, n, 8);

This is a different test case.

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2444

Quoted 6 lines
drivers/clk/clk_test.c +6 -0
+
+       // Highest numerator, zero based
+       parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 8);
+       KUNIT_EXPECT_EQ(test, n, 1);

This is a different test case. If it has a meaningful name it will be easier to understand as well.

maintainer
#12 lore raw

Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation

Quoting Frank Oltmanns (2023-06-13 23:51:32)

Quoted 11 lines

Hi Stephen,

Thank you for your feedback.

Quoted 6 lines · Stephen Boyd

Quoting Frank Oltmanns (2023-06-13 01:36:26)

Quoted 5 lines

In light of the recent discovery that the fractional divisor approximation does not utilize the full available range for clocks that are flagged CLK_FRAC_DIVIDER_ZERO_BASED, implement tests for the edge cases of this clock type.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Signed-off-by Frank Oltmanns <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here

What is the link for?

The intention was to show why the tests were added ("In light of the recent discovery..."). I announced this discovery in the mail I referred to. Since that intention didn't come across: Should I drop the link?

Ok. You can say that and reference the link with [1] like this.

Link https://lore.kernel.org/lkml/[email protected] [1] open here
Quoted 53 lines
Quoted 53 lines
Quoted 4 lines

---
 drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-

Please make a new file, drivers/clk/clk-fractional-divider_test.c instead.

Quoted 32 lines
 1 file changed, 68 insertions(+), 1 deletion(-)
drivers/clk/clk_test.c +15 -0
index f9a5c2964c65..b247ba841cbd 100644
@@ -8,6 +8,9 @@
 /* Needed for clk_hw_get_clk() */
 #include "clk.h"
 
+/* Needed for clk_fractional_divider_general_approximation */
+#include "clk-fractional-divider.h"
+
 #include <kunit/test.h>
 
 #define DUMMY_CLOCK_INIT_RATE  (42 * 1000 * 1000)

@@ -2394,6 +2397,69 @@ static struct kunit_suite clk_mux_notifier_test_suite = {
        .test_cases = clk_mux_notifier_test_cases,
 };
 
+
+/*
+ * Test that clk_fractional_divider_general_approximation will work with the
+ * highest available numerator and denominator.
+ */
+static void clk_fd_test_round_rate_max_mn(struct kunit *test)
+{
+       struct clk_fractional_divider *fd;
+       struct clk_hw *hw;
+       unsigned long rate, parent_rate, m, n;
+
+       fd = kunit_kzalloc(test, sizeof(*fd), GFP_KERNEL);

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2421

Quoted 10 lines
drivers/clk/clk_test.c +10 -0
+       KUNIT_ASSERT_NOT_NULL(test, fd);
+
+       fd->mwidth = 3;
+       fd->nwidth = 3;
+
+       hw = &fd->hw;
+
+       rate = DUMMY_CLOCK_RATE_1;
+
+       // Highest denominator, no flags

Use /* this for comments */

Quoted 8 lines
Quoted 4 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2422

Quoted 1 line
drivers/clk/clk_test.c +1 -0
+       parent_rate = 10 * DUMMY_CLOCK_RATE_1;

Just write out the actual frequency. Don't use DUMMY_CLOCK_RATE_1 at all in the test.

Sure, will do. The idea was to highlight that we want to have the parent running at 10 times the speed, while the divider has a maximum value of 7 (or 8 if zero based).

Ok. In that case, have a local variable for 'rate' that's const and then have

	parent_rate = 10 * rate;

to make it clear. We don't need a comment for that.

Quoted 17 lines
Quoted 8 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2425

Quoted 5 lines
drivers/clk/clk_test.c +3 -0
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 1);
+       KUNIT_EXPECT_EQ(test, n, 7);

This is a different test case.

Quoted 8 lines

drivers/clk/clk_test.c static struct kunit_suite clk_mux_notifier_test_suite = { · line 2430

Quoted 5 lines
drivers/clk/clk_test.c +5 -0
+
+       // Highest numerator, no flags
+       parent_rate = DUMMY_CLOCK_RATE_1 / 10;
+       clk_fractional_divider_general_approximation(hw, rate, &parent_rate, &m, &n);
+       KUNIT_EXPECT_EQ(test, m, 7);

Is 7 related to mwidth? Maybe this should be clk_div_mask(fd->mwidth) instead of 7.

Quoted 7 lines

Yes, 7 is related to mwidth. I thought about this too, but I'm not sure that's the best move here. The function under test uses:

    max_m = GENMASK(fd->mwidth - 1, 0);
    max_n = GENMASK(fd->nwidth - 1, 0);

I come from a safety-concerned industry and as a general rule we avoid using functions from the code under test in our tests.

GENMASK is a macro, not a function. Does that help? In all seriousness though, I understand not wanting to test code with the same code. This is basic math though. Maybe it should be something like this?

	const unsigned long max_m = 8;
	const unsigned long max_n = 8;

	fd->mwidth = bits_per(max_m);
	fd->nwidth = bits_per(max_n);

This expresses that there's a maximum m and n value and we calculate the width from that information. Then the test can expect to see that max value come out.

Quoted 3 lines

I'm doing this
here as a hobby, but still I find it to be a good rule that I'd like to stick to unless asked otherwise.

Cool.

Quoted 6 lines

If I use the same code to generate the expected values, I'm not really testing my change, but only the underlying best_rational_approximation.

Instead, how about I add a comment to the test function that more thoroughly explains its intention?

No thanks. We shouldn't need more than a sentence indicating what the test is testing

Quoted 12 lines

Something along those lines:

/*
* Introduce a parent that runs at 10 times the frequency of the
* requested rate.
* m and n are 3 bits wide each.
* The clock has no flags set, hence the maximum value that fits in
* m and n is 7.
* Therefore, expect the highest possible divisor.
*/
static void clk_fd_test_round_rate_max_m(struct kunit *test)

I'd just indicate the expected outcome in the function name.

clk_fd_test_general_approximation_m_saturates()

and then it could be parameterized with KUNIT_ARRAY_PARAM() so that we make sure any width saturates.

Quoted 3 lines

/*
* Introduce a parent that runs at 1/10th the frequency of the

I'm not sure where 10 times and 1/10th came from. Is it just a magnitude larger for simplicity?

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel