Message-ID: <87pm5yzgcr.fsf@oltmanns.dev>
Date:   Wed, 14 Jun 2023 08:51:32 +0200
From:   Frank Oltmanns <frank@...manns.dev>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     Michael Turquette <mturquette@...libre.com>,
        "A.s. Dong" <aisheng.dong@....com>,
        Abel Vesa <abelvesa@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, NXP Linux Team <linux-imx@....com>,
        Peng Fan <peng.fan@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>
Subject: Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor
 approximation

Hi Stephen,

Thank you for your feedback.

On 2023-06-13 at 12:42:05 -0700, Stephen Boyd <sboyd@...nel.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@...manns.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.

Powered by blists - more mailing lists