From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 407AFEB64D8 for ; Wed, 14 Jun 2023 20:42:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Date:To:Cc:From:Subject:References: In-Reply-To:MIME-Version:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LXwL0yqZ/YaXhnRhMw8k0/j7iSQe1amtuSgGCamPBx8=; b=dc3tAWKNQQ1CdM eXUCm+CjDXPSktRNZzBJFThDYHcD0ujwrBMDLq8wID2R/2vKRd26lTb4RlhQV5gKrxP0oOx6EfcDc blEz7McTlLHaw5XkliJ4DL9vVW6H6EHFGW1LXjNLIj3hlJ9m8D3NJ3ndvEILqiQ5qcWnbRFLShqHm 9raG/V7Xu6VizqnJqa2V2Nvq2ML/ff1O293rQECZmtRJGGWOhgW/C4IHxdVwUw0QtOoc+xZ2JgjxE duzbUY4UNNzXtunrmU/O4i4r1NJVuCeG3kAwUb0wDcSUcDfy3xrP9B9m7XRpF04N5yU73orfhSL6T CHkC8WRb0k4vgyGyXCpA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q9XJe-00CfOw-37; Wed, 14 Jun 2023 20:42:06 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q9XJb-00CfNd-1R for linux-arm-kernel@lists.infradead.org; Wed, 14 Jun 2023 20:42:05 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id EA2AE630C1; Wed, 14 Jun 2023 20:42:02 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 476D8C433C8; Wed, 14 Jun 2023 20:42:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1686775322; bh=MFijL4id8opceRgFSXrXuWpdtq3cYx/ut2/HazIMu24=; h=In-Reply-To:References:Subject:From:Cc:To:Date:From; b=cWheUOLTh/XMmQ4tbQLp+DhLGNbUCBgFbp6Yk8FPdeIM8BHlt4MZHcj7/VOch+bmP fKT9KpTdIG3z2e25lxEkFwJ4s4ECPOzV4NO9Vj7ik/oC8T0Dlultr/toKprR6vpFoW U17xExS/C6/BSXo9cv8FBOvZMLbAqVRIoi+sVSkrEx5q3ktggzeDia6OuncZhB2QLH qbp4rvN1D1oRaYO1ncDMMEJZmqS3HN7pLmBuUbKsUvepLA+41y+RqX0kMQ4yzOeWzz RpzefE09q4JAaugSEG0p0gVzU57Xrdxz4kIOY/YFiKTRbtj3qMecCNcUR8F+jXZEHO xuQaSEIfgVBNg== Message-ID: MIME-Version: 1.0 In-Reply-To: <87pm5yzgcr.fsf@oltmanns.dev> References: <20230613083626.227476-1-frank@oltmanns.dev> <20230613083626.227476-3-frank@oltmanns.dev> <83f9ee3ce26e4d4ba7c395aab316cae6.sboyd@kernel.org> <87pm5yzgcr.fsf@oltmanns.dev> Subject: Re: [PATCH v2 2/2] clk: tests: Add tests for fractional divisor approximation From: Stephen Boyd Cc: Michael Turquette , A.s. Dong , Abel Vesa , Fabio Estevam , linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, NXP Linux Team , Peng Fan , Pengutronix Kernel Team , Sascha Hauer , Shawn Guo To: Frank Oltmanns Date: Wed, 14 Jun 2023 13:42:00 -0700 User-Agent: alot/0.10 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230614_134203_627500_498CEA61 X-CRM114-Status: GOOD ( 45.61 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.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 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 > >> 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 > >> > >> #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