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

Hi Stephen,

Thank you for your feedback.

On 2023-06-13 at 12:42:05 -0700, Stephen Boyd [off-list ref] wrote:

quoted

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

quoted

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 <redacted>
Link: https://lore.kernel.org/lkml/20230529133433.56215-1-frank@oltmanns.dev (local)

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/20230529133433.56215-1-frank@oltmanns.dev (local) [1]

quoted

quoted

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

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

quoted

 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 */

quoted

+       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

quoted

+       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

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

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.

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.

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

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.

    /*
     * 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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel