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

[Stephen Boyd <sboyd@kernel.org>]

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 <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?

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 [1]

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

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.

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

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