[PATCH v2 0/2] clk: fractional-divider: Improve approximation when zero based

[PATCH v2 0/2] clk: fractional-divider: Improve approximation when zero based

[Frank Oltmanns]

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/20230529133433.56215-1-frank@oltmanns.dev/

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(-)

-- 
2.41.0

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

[Frank Oltmanns]

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 <aisheng.dong@nxp.com>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/clk/clk-fractional-divider.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 479297763e70..7da21cd2bdb1 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -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--;
-- 
2.41.0

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

[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 <frank@oltmanns.dev>
Link: https://lore.kernel.org/lkml/20230529133433.56215-1-frank@oltmanns.dev
---
 drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index f9a5c2964c65..b247ba841cbd 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -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");
-- 
2.41.0

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

[Frank Oltmanns]

Hi Stephen,

On 2023-06-13 at 10:36:26 +0200, Frank Oltmanns <frank@oltmanns.dev> wrote:
> 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 <frank@oltmanns.dev>
> Link: https://lore.kernel.org/lkml/20230529133433.56215-1-frank@oltmanns.dev
> ---
>  drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index f9a5c2964c65..b247ba841cbd 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -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);

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

> +
> +	// 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,
> +};

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

> +
>  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");

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

[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/202306132038.nUB6hmCv-lkp@intel.com/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/20230613083626.227476-3-frank@oltmanns.dev
        # 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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306132038.nUB6hmCv-lkp@intel.com/

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

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

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Frank Oltmanns]

Hi,

On 2023-06-13 at 20:48:21 +0800, kernel test robot <lkp@intel.com> wrote:
> 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/202306132038.nUB6hmCv-lkp@intel.com/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/20230613083626.227476-3-frank@oltmanns.dev
>         # 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 <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202306132038.nUB6hmCv-lkp@intel.com/
>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
>>> 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

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

[Frank Oltmanns]

Hi Stephen,

On 2023-06-14 at 13:02:24 -0700, Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Frank Oltmanns (2023-06-14 01:19:37)
>> Hi,
>>
>> On 2023-06-13 at 20:48:21 +0800, kernel test robot <lkp@intel.com> wrote:
>> > 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/202306132038.nUB6hmCv-lkp@intel.com/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/20230613083626.227476-3-frank@oltmanns.dev
>> >         # 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 <lkp@intel.com>
>> > | Closes: https://lore.kernel.org/oe-kbuild-all/202306132038.nUB6hmCv-lkp@intel.com/
>> >
>> > All errors (new ones prefixed by >>, old ones prefixed by <<):
>> >
>> >>> 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?
>
> 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/20230614185521.477924-3-frank@oltmanns.dev/

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

[Frank Oltmanns]

Hi Stephen,

On 2023-06-16 at 12:33:51 -0700, Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Frank Oltmanns (2023-06-14 22:16:17)
>> Hi Stephen,
>>
>> On 2023-06-14 at 13:02:24 -0700, Stephen Boyd <sboyd@kernel.org> wrote:
>> > Quoting Frank Oltmanns (2023-06-14 01:19:37)
>> >> Hi,
>> >>
>> >> On 2023-06-13 at 20:48:21 +0800, kernel test robot <lkp@intel.com> 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.)
>>
>> 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/20230601095512.18029-1-zhangqing@rock-chips.com

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/20230617131041.18313-1-frank@oltmanns.dev/
[2] https://lore.kernel.org/all/20230617102919.27564-1-frank@oltmanns.dev/

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

[Frank Oltmanns]

Hi Stephen,

Thank you for your feedback.

On 2023-06-13 at 12:42:05 -0700, Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Frank Oltmanns (2023-06-13 01:36:26)
>> 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 <frank@oltmanns.dev>
>> Link: https://lore.kernel.org/lkml/20230529133433.56215-1-frank@oltmanns.dev
>
> 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?

>
>> ---
>>  drivers/clk/clk_test.c | 69 +++++++++++++++++++++++++++++++++++++++++-
>
> Please make a new file, drivers/clk/clk-fractional-divider_test.c
> instead.
>
>>  1 file changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index f9a5c2964c65..b247ba841cbd 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -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
>
> Use /* this for comments */
>
>> +       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).

>
>> +       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.
>
>> +
>> +       // 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

>
>> +       KUNIT_EXPECT_EQ(test, n, 1);
>
> This is a different test case.
>
>> +
>> +       // 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.
>
>> +
>> +       // 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.