Hi,
This is a follow-up of the discussion here: https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/
and here: https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/
While the initial proposal implemented a new API to temporarily raise and lower clock rates based on consumer workloads, Stephen suggested an alternative approach implemented here.
The main issue that needed to be addressed in our case was that in a situation where we would have multiple calls to clk_set_rate_range, we would end up with a clock at the maximum of the minimums being set. This would be expected, but the issue was that if one of the users was to relax or drop its requirements, the rate would be left unchanged, even though the ideal rate would have changed.
So something like
clk_set_rate(user1_clk, 1000); clk_set_min_rate(user1_clk, 2000); clk_set_min_rate(user2_clk, 3000); clk_set_min_rate(user2_clk, 1000);
Would leave the clock running at 3000Hz, while the minimum would now be 2000Hz.
This was mostly due to the fact that the core only triggers a rate change in clk_set_rate_range() if the current rate is outside of the boundaries, but not if it's within the new boundaries.
That series changes that and will trigger a rate change on every call, with the former rate being tried again. This way, providers have a chance to follow whatever policy they see fit for a given clock each time the boundaries change.
This series also implements some kunit tests, first to test a few rate related functions in the CCF, and then extends it to make sure that behaviour has some test coverage.
Let me know what you think Maxime
Changes from v2: - Rebased on current next - Rewrote the whole thing according to Stephen reviews - Implemented some kunit tests
Changes from v1: - Return NULL in clk_request_start if clk pointer is NULL - Test for clk_req pointer in clk_request_done - Add another user in vc4 - Rebased on top of v5.15-rc1
Maxime Ripard (10): clk: Add Kunit tests for rate clk: Always clamp the rounded rate clk: Use clamp instead of open-coding our own clk: Always set the rate on clk_set_range_rate clk: Add clk_drop_range clk: bcm: rpi: Add variant structure clk: bcm: rpi: Set a default minimum rate clk: bcm: rpi: Run some clocks at the minimum rate allowed drm/vc4: Add logging and comments drm/vc4: hdmi: Remove clock rate initialization
drivers/clk/Kconfig | 7 + drivers/clk/Makefile | 1 + drivers/clk/bcm/clk-raspberrypi.c | 125 ++++++- drivers/clk/clk-rate-test.c | 603 ++++++++++++++++++++++++++++++ drivers/clk/clk.c | 51 ++- drivers/gpu/drm/vc4/vc4_hdmi.c | 13 - drivers/gpu/drm/vc4/vc4_kms.c | 11 + include/linux/clk.h | 11 + 8 files changed, 767 insertions(+), 55 deletions(-) create mode 100644 drivers/clk/clk-rate-test.c
Let's test various parts of the rate-related clock API with the kunit testing framework.
Cc: kunit-dev@googlegroups.com Suggested-by: Stephen Boyd sboyd@kernel.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/Kconfig | 7 + drivers/clk/Makefile | 1 + drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++ 3 files changed, 286 insertions(+) create mode 100644 drivers/clk/clk-rate-test.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index f5807d190ba2..7ae48a91f738 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -436,4 +436,11 @@ config CLK_GATE_TEST help Kunit test for the basic clk gate type.
+config CLK_RATE_TEST + tristate "Basic Core Rate Kunit Tests" + depends on KUNIT + default KUNIT + help + Kunit test for the basic clock rate management. + endif diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index b940c6d35922..0238a595167a 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,6 +2,7 @@ # common clock types obj-$(CONFIG_HAVE_CLK) += clk-devres.o clk-bulk.o clkdev.o obj-$(CONFIG_COMMON_CLK) += clk.o +obj-$(CONFIG_CLK_RATE_TEST) += clk-rate-test.o obj-$(CONFIG_COMMON_CLK) += clk-divider.o obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c new file mode 100644 index 000000000000..f2d3df791b5a --- /dev/null +++ b/drivers/clk/clk-rate-test.c @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Kunit test for clk rate management + */ +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/slab.h> + +#include <kunit/test.h> + +#define DUMMY_CLOCK_INIT_RATE (42 * 1000 * 1000) +#define DUMMY_CLOCK_RATE_1 (142 * 1000 * 1000) +#define DUMMY_CLOCK_RATE_2 (242 * 1000 * 1000) + +struct clk_dummy_rate_context { + struct clk_hw hw; + unsigned long rate; +}; + +static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct clk_dummy_rate_context *ctx = + container_of(hw, struct clk_dummy_rate_context, hw); + + return ctx->rate; +} + +static int clk_dummy_rate_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + /* Just return the same rate without modifying it */ + return 0; +} + +static int clk_dummy_rate_set_rate(struct clk_hw *hw, + unsigned long rate, + unsigned long parent_rate) +{ + struct clk_dummy_rate_context *ctx = + container_of(hw, struct clk_dummy_rate_context, hw); + + ctx->rate = rate; + return 0; +} + +const static struct clk_ops clk_dummy_rate_ops = { + .recalc_rate = clk_dummy_rate_recalc_rate, + .determine_rate = clk_dummy_rate_determine_rate, + .set_rate = clk_dummy_rate_set_rate, +}; + +static int clk_rate_test_init_with_ops(struct kunit *test, + const struct clk_ops *ops) +{ + struct clk_dummy_rate_context *ctx; + struct clk_init_data init = { }; + int ret; + + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + ctx->rate = DUMMY_CLOCK_INIT_RATE; + test->priv = ctx; + + init.name = "test_dummy_rate"; + init.ops = ops; + ctx->hw.init = &init; + + ret = clk_hw_register(NULL, &ctx->hw); + if (ret) + return ret; + + return 0; +} + +static int clk_rate_test_init(struct kunit *test) +{ + return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops); +} + +static void clk_rate_test_exit(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + + clk_hw_unregister(&ctx->hw); + kfree(ctx); +} + +/* + * Test that the actual rate matches what is returned by clk_get_rate() + */ +static void clk_rate_test_get_rate(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, ctx->rate); +} + +/* + * Test that, after a call to clk_set_rate(), the rate returned by + * clk_get_rate() matches. + * + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't + * modify the requested rate, which is our case in clk_dummy_rate_ops. + */ +static void clk_rate_test_set_get_rate(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + int ret; + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); +} + +/* + * Test that, after several calls to clk_set_rate(), the rate returned + * by clk_get_rate() matches the last one. + * + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't + * modify the requested rate, which is our case in clk_dummy_rate_ops. + */ +static void clk_rate_test_set_set_get_rate(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + int ret; + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); +} + +static struct kunit_case clk_rate_test_cases[] = { + KUNIT_CASE(clk_rate_test_get_rate), + KUNIT_CASE(clk_rate_test_set_get_rate), + KUNIT_CASE(clk_rate_test_set_set_get_rate), + {} +}; + +static struct kunit_suite clk_rate_test_suite = { + .name = "clk-rate-test", + .init = clk_rate_test_init, + .exit = clk_rate_test_exit, + .test_cases = clk_rate_test_cases, +}; + +/* + * Test that clk_set_rate_range won't return an error for a valid range. + */ +static void clk_rate_range_test_set_range(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + int ret; + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); +} + +/* + * Test that calling clk_set_rate_range with a minimum rate higher than + * the maximum rate returns an error. + */ +static void clk_rate_range_test_set_range_invalid(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + int ret; + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1 + 1000, + DUMMY_CLOCK_RATE_1); + KUNIT_ASSERT_EQ(test, ret, -EINVAL); +} + +/* + * Test that if our clock has a rate lower than the minimum set by a + * call to clk_set_rate_range(), the rate will be raised to match the + * new minimum. + * + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't + * modify the requested rate, which is our case in clk_dummy_rate_ops. + */ +static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + int ret; + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); +} + +/* + * Test that if our clock has a rate higher than the maximum set by a + * call to clk_set_rate_range(), the rate will be lowered to match the + * new maximum. + * + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't + * modify the requested rate, which is our case in clk_dummy_rate_ops. + */ +static void clk_rate_range_test_set_range_get_rate_lowered(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + int ret; + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); +} + +static struct kunit_case clk_rate_range_test_cases[] = { + KUNIT_CASE(clk_rate_range_test_set_range), + KUNIT_CASE(clk_rate_range_test_set_range_invalid), + KUNIT_CASE(clk_rate_range_test_set_range_get_rate_raised), + KUNIT_CASE(clk_rate_range_test_set_range_get_rate_lowered), + {} +}; + +static struct kunit_suite clk_rate_range_test_suite = { + .name = "clk-rate-range-test", + .init = clk_rate_test_init, + .exit = clk_rate_test_exit, + .test_cases = clk_rate_range_test_cases, +}; + +kunit_test_suites( + &clk_rate_test_suite, + &clk_rate_range_test_suite +); +MODULE_LICENSE("GPL v2");
Quoting Maxime Ripard (2022-01-20 06:34:08)
Let's test various parts of the rate-related clock API with the kunit testing framework.
Cc: kunit-dev@googlegroups.com Suggested-by: Stephen Boyd sboyd@kernel.org Signed-off-by: Maxime Ripard maxime@cerno.tech
This is great! Thanks for doing this.
drivers/clk/Kconfig | 7 + drivers/clk/Makefile | 1 + drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++ 3 files changed, 286 insertions(+) create mode 100644 drivers/clk/clk-rate-test.c
I was thinking this would be more generic so that one file tests clk.c and all the code in there, but I guess there may be config dependencies like CONFIG_OF that we may want to extract out and depend on differently. I'm not sure how kunit will handle testing different paths depending on build configuration so this approach may head off future problems. If it doesn't then we can always slam the test together.
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index f5807d190ba2..7ae48a91f738 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -436,4 +436,11 @@ config CLK_GATE_TEST help Kunit test for the basic clk gate type.
+config CLK_RATE_TEST
See v3 here[1] and have it follow that.
tristate "Basic Core Rate Kunit Tests"depends on KUNITdefault KUNIThelpKunit test for the basic clock rate management.endif diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index b940c6d35922..0238a595167a 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,6 +2,7 @@ # common clock types obj-$(CONFIG_HAVE_CLK) += clk-devres.o clk-bulk.o clkdev.o obj-$(CONFIG_COMMON_CLK) += clk.o +obj-$(CONFIG_CLK_RATE_TEST) += clk-rate-test.o obj-$(CONFIG_COMMON_CLK) += clk-divider.o obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c new file mode 100644 index 000000000000..f2d3df791b5a --- /dev/null +++ b/drivers/clk/clk-rate-test.c @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Kunit test for clk rate management
- */
+#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/slab.h>
+#include <kunit/test.h>
+#define DUMMY_CLOCK_INIT_RATE (42 * 1000 * 1000) +#define DUMMY_CLOCK_RATE_1 (142 * 1000 * 1000) +#define DUMMY_CLOCK_RATE_2 (242 * 1000 * 1000)
+struct clk_dummy_rate_context {
struct clk_hw hw;unsigned long rate;+};
+static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)+{
struct clk_dummy_rate_context *ctx =container_of(hw, struct clk_dummy_rate_context, hw);return ctx->rate;+}
+static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
struct clk_rate_request *req)+{
/* Just return the same rate without modifying it */return 0;+}
+static int clk_dummy_rate_set_rate(struct clk_hw *hw,
unsigned long rate,unsigned long parent_rate)+{
struct clk_dummy_rate_context *ctx =container_of(hw, struct clk_dummy_rate_context, hw);ctx->rate = rate;return 0;+}
+const static struct clk_ops clk_dummy_rate_ops = {
static const?
.recalc_rate = clk_dummy_rate_recalc_rate,.determine_rate = clk_dummy_rate_determine_rate,.set_rate = clk_dummy_rate_set_rate,+};
+static int clk_rate_test_init_with_ops(struct kunit *test,
const struct clk_ops *ops)+{
struct clk_dummy_rate_context *ctx;struct clk_init_data init = { };int ret;ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
Use kunit_kzalloc() here
if (!ctx)return -ENOMEM;ctx->rate = DUMMY_CLOCK_INIT_RATE;test->priv = ctx;init.name = "test_dummy_rate";init.ops = ops;ctx->hw.init = &init;ret = clk_hw_register(NULL, &ctx->hw);if (ret)return ret;return 0;+}
+static int clk_rate_test_init(struct kunit *test) +{
return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);+}
+static void clk_rate_test_exit(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;clk_hw_unregister(&ctx->hw);kfree(ctx);
And drop the kfree as it is now test managed.
+}
+/*
- Test that the actual rate matches what is returned by clk_get_rate()
- */
+static void clk_rate_test_get_rate(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;unsigned long rate;rate = clk_get_rate(clk);KUNIT_ASSERT_TRUE(test, rate > 0);KUNIT_ASSERT_EQ(test, rate, ctx->rate);
These should be KUNIT_EXPECT_*() as we don't want to stop the test if the rate is wrong, we want to check that the rate is what we expected it to be. Assertions are about making sure things are sane and if not we should stop testing, whereas expectations are about testing the code. A test must have an EXPECT while it can have an ASSERT.
Maybe kunit should check that there was an EXPECT on return from the test. Daniel?
+}
+/*
- Test that, after a call to clk_set_rate(), the rate returned by
- clk_get_rate() matches.
- This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
- modify the requested rate, which is our case in clk_dummy_rate_ops.
- */
+static void clk_rate_test_set_get_rate(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;unsigned long rate;int ret;ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);KUNIT_ASSERT_EQ(test, ret, 0);
I'd like to throw the ret check directly into KUNIT_ASSERT_EQ() here.
KUNIT_ASSERT_EQ(test, clk_set_rate(clk, DUMMY_CLOCK_RATE_1), 0);
so we can easily see if something goes wrong that the set rate failed. Good use of assert here, we don't want to continue with the test unless the set rate actually worked.
rate = clk_get_rate(clk);KUNIT_ASSERT_TRUE(test, rate > 0);KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);+}
+/*
- Test that, after several calls to clk_set_rate(), the rate returned
- by clk_get_rate() matches the last one.
- This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
- modify the requested rate, which is our case in clk_dummy_rate_ops.
- */
+static void clk_rate_test_set_set_get_rate(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;unsigned long rate;int ret;ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);KUNIT_ASSERT_EQ(test, ret, 0);ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2);KUNIT_ASSERT_EQ(test, ret, 0);rate = clk_get_rate(clk);KUNIT_ASSERT_TRUE(test, rate > 0);KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);+}
+static struct kunit_case clk_rate_test_cases[] = {
KUNIT_CASE(clk_rate_test_get_rate),KUNIT_CASE(clk_rate_test_set_get_rate),KUNIT_CASE(clk_rate_test_set_set_get_rate),{}+};
+static struct kunit_suite clk_rate_test_suite = {
.name = "clk-rate-test",.init = clk_rate_test_init,.exit = clk_rate_test_exit,.test_cases = clk_rate_test_cases,+};
+/*
- Test that clk_set_rate_range won't return an error for a valid range.
- */
+static void clk_rate_range_test_set_range(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;int ret;ret = clk_set_rate_range(clk,DUMMY_CLOCK_RATE_1,DUMMY_CLOCK_RATE_2);KUNIT_ASSERT_EQ(test, ret, 0);
Also make sure that the rate is within the bounds of rate_1 and rate_2?
+}
+/*
- Test that calling clk_set_rate_range with a minimum rate higher than
- the maximum rate returns an error.
- */
+static void clk_rate_range_test_set_range_invalid(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;int ret;ret = clk_set_rate_range(clk,DUMMY_CLOCK_RATE_1 + 1000,DUMMY_CLOCK_RATE_1);KUNIT_ASSERT_EQ(test, ret, -EINVAL);
Let's not check for a specific error, but a negative value instead. I'd rather not encode particular error values unless we need them to be particular.
+}
+/*
- Test that if our clock has a rate lower than the minimum set by a
- call to clk_set_rate_range(), the rate will be raised to match the
- new minimum.
- This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
- modify the requested rate, which is our case in clk_dummy_rate_ops.
- */
+static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;unsigned long rate;int ret;ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);KUNIT_ASSERT_EQ(test, ret, 0);ret = clk_set_rate_range(clk,DUMMY_CLOCK_RATE_1,DUMMY_CLOCK_RATE_2);KUNIT_ASSERT_EQ(test, ret, 0);rate = clk_get_rate(clk);KUNIT_ASSERT_TRUE(test, rate > 0);
KUNIT_EXPECT_LE(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_2);
Or just drop it entirely as DUMMY_CLOCK_RATE_1 is greater than 0 and it's tested next.
KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
KUNIT_EXPECT_EQ(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);
+}
On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd sboyd@kernel.org wrote:
I was thinking this would be more generic so that one file tests clk.c and all the code in there, but I guess there may be config dependencies like CONFIG_OF that we may want to extract out and depend on differently. I'm not sure how kunit will handle testing different paths depending on build configuration so this approach may head off future problems. If it doesn't then we can always slam the test together.
KUnit doesn't have hard technical limitations in this regard.
You could have something like this
static void my_optional_kunit_test(struct kunit *test) { #ifdef CONFIG_OPTIONAL_FEATURE
# else kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled"); #endif }
I think it's just a matter of what's least confusing to users.
+}
+/*
- Test that the actual rate matches what is returned by clk_get_rate()
- */
+static void clk_rate_test_get_rate(struct kunit *test) +{
struct clk_dummy_rate_context *ctx = test->priv;struct clk_hw *hw = &ctx->hw;struct clk *clk = hw->clk;unsigned long rate;rate = clk_get_rate(clk);KUNIT_ASSERT_TRUE(test, rate > 0);
KUNIT_EXPECT_GT(test, rate, 0);
KUNIT_ASSERT_EQ(test, rate, ctx->rate);These should be KUNIT_EXPECT_*() as we don't want to stop the test if the rate is wrong, we want to check that the rate is what we expected it to be. Assertions are about making sure things are sane and if not we should stop testing, whereas expectations are about testing the code. A test must have an EXPECT while it can have an ASSERT.
Maybe kunit should check that there was an EXPECT on return from the test. Daniel?
Sorry, I'm not sure I understand the question.
Are you saying you want kunit to flag cases like static void empty_test(struct kunit *) {} ?
Quoting Daniel Latypov (2022-01-20 13:56:39)
On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd sboyd@kernel.org wrote:
I was thinking this would be more generic so that one file tests clk.c and all the code in there, but I guess there may be config dependencies like CONFIG_OF that we may want to extract out and depend on differently. I'm not sure how kunit will handle testing different paths depending on build configuration so this approach may head off future problems. If it doesn't then we can always slam the test together.
KUnit doesn't have hard technical limitations in this regard.
You could have something like this
static void my_optional_kunit_test(struct kunit *test) { #ifdef CONFIG_OPTIONAL_FEATURE
# else kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled"); #endif }
I think it's just a matter of what's least confusing to users.
Ok, I see. Is there some way to have multiple configs checked into the tree so we can test different kernel configuration code paths? This discussion isn't really relevant to this patch so we can take this up in another thread if you like.
Maybe kunit should check that there was an EXPECT on return from the test. Daniel?
Sorry, I'm not sure I understand the question.
Are you saying you want kunit to flag cases like static void empty_test(struct kunit *) {} ?
Yes. I'd like kunit to enforce that all tests have at least one EXPECT_*() in them.
On Thu, Jan 20, 2022 at 8:34 PM Stephen Boyd sboyd@kernel.org wrote:
Quoting Daniel Latypov (2022-01-20 13:56:39)
On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd sboyd@kernel.org wrote: KUnit doesn't have hard technical limitations in this regard.
You could have something like this
static void my_optional_kunit_test(struct kunit *test) { #ifdef CONFIG_OPTIONAL_FEATURE
# else kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled"); #endif }
I think it's just a matter of what's least confusing to users.
Ok, I see. Is there some way to have multiple configs checked into the tree so we can test different kernel configuration code paths? This
Multiple kunitconfigs? There's no restrictions on those
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.foo $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.bar
The first one will assume drivers/clk/.kunitconfig. But there's no reason you need to have a file called that. One could just have multiple standalone kunitconfigs, named however they like.
--kunitconfig is new enough (5.12+) that there's no real conventions yet.
Another option is $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk \ --kconfig_add=CONFIG_RARELY_USED=y
This is another case where we can do whatever is least confusing.
discussion isn't really relevant to this patch so we can take this up in another thread if you like.
Maybe kunit should check that there was an EXPECT on return from the test. Daniel?
Sorry, I'm not sure I understand the question.
Are you saying you want kunit to flag cases like static void empty_test(struct kunit *) {} ?
Yes. I'd like kunit to enforce that all tests have at least one EXPECT_*() in them.
I totally understand the rationale. It's a bit misleading to say PASSED if no expectation/assertion passed. One might want a NO_STATUS (or maybe SKIPPED) result instead.
But other unit test frameworks act the way KUnit does here, so there's an argument for consistency with others so users don't have to have a whole new mental model.
Some examples below for reference. All of these output something like test result: ok. 1 passed; ...
E.g. in Python import unittest
class ExampleTest(unittest.TestCase): def test_foo(self): pass
if __name__ == '__main__': unittest.main()
In Golang: package example
import "testing"
func TestFoo(t *testing.T) {}
In C++ using Googletest: #include "gtest/gtest.h"
TEST(Suite, Case) { }
In Rust: #[cfg(test)] mod tests { #[test] fn test_empty() {} }
Quoting Daniel Latypov (2022-01-20 21:25:03)
On Thu, Jan 20, 2022 at 8:34 PM Stephen Boyd sboyd@kernel.org wrote:
Quoting Daniel Latypov (2022-01-20 13:56:39)
On Thu, Jan 20, 2022 at 1:31 PM Stephen Boyd sboyd@kernel.org wrote: KUnit doesn't have hard technical limitations in this regard.
You could have something like this
static void my_optional_kunit_test(struct kunit *test) { #ifdef CONFIG_OPTIONAL_FEATURE
# else kunit_skip(test, "CONFIG_OPTIONAL_FEATURE is not enabled"); #endif }
I think it's just a matter of what's least confusing to users.
Ok, I see. Is there some way to have multiple configs checked into the tree so we can test different kernel configuration code paths? This
Multiple kunitconfigs? There's no restrictions on those
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.foo $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk/kunitconfig.bar
The first one will assume drivers/clk/.kunitconfig. But there's no reason you need to have a file called that. One could just have multiple standalone kunitconfigs, named however they like.
--kunitconfig is new enough (5.12+) that there's no real conventions yet.
Another option is $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk \ --kconfig_add=CONFIG_RARELY_USED=y
This is another case where we can do whatever is least confusing.
Got it, thanks.
discussion isn't really relevant to this patch so we can take this up in another thread if you like.
Maybe kunit should check that there was an EXPECT on return from the test. Daniel?
Sorry, I'm not sure I understand the question.
Are you saying you want kunit to flag cases like static void empty_test(struct kunit *) {} ?
Yes. I'd like kunit to enforce that all tests have at least one EXPECT_*() in them.
I totally understand the rationale. It's a bit misleading to say PASSED if no expectation/assertion passed. One might want a NO_STATUS (or maybe SKIPPED) result instead.
But other unit test frameworks act the way KUnit does here, so there's an argument for consistency with others so users don't have to have a whole new mental model.
Ok if other test frameworks don't care then there's nothing to do.
The current core while setting the min and max rate properly in the clk_request structure will not make sure that the requested rate is within these boundaries, leaving it to each and every driver to make sure it is.
Add a clamp call to make sure it's always done, and add a few unit tests to make sure we don't have any regression there.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-rate-test.c | 46 +++++++++++++++++++++++++++++++++++++ drivers/clk/clk.c | 2 ++ 2 files changed, 48 insertions(+)
diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c index f2d3df791b5a..a13b02702d20 100644 --- a/drivers/clk/clk-rate-test.c +++ b/drivers/clk/clk-rate-test.c @@ -198,6 +198,50 @@ static void clk_rate_range_test_set_range_invalid(struct kunit *test) KUNIT_ASSERT_EQ(test, ret, -EINVAL); }
+/* + * Test that if our clock has some boundaries and we try to round a rate + * lower than the minimum, the returned rate will be within range. + */ +static void clk_rate_range_test_set_range_round_rate_lower(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + long rate; + int ret; + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000); + KUNIT_ASSERT_FALSE(test, rate < 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); +} + +/* + * Test that if our clock has some boundaries and we try to round a rate + * higher than the maximum, the returned rate will be within range. + */ +static void clk_rate_range_test_set_range_round_rate_higher(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + long rate; + int ret; + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_2 + 1000); + KUNIT_ASSERT_FALSE(test, rate < 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); +} + /* * Test that if our clock has a rate lower than the minimum set by a * call to clk_set_rate_range(), the rate will be raised to match the @@ -259,6 +303,8 @@ static void clk_rate_range_test_set_range_get_rate_lowered(struct kunit *test) static struct kunit_case clk_rate_range_test_cases[] = { KUNIT_CASE(clk_rate_range_test_set_range), KUNIT_CASE(clk_rate_range_test_set_range_invalid), + KUNIT_CASE(clk_rate_range_test_set_range_round_rate_lower), + KUNIT_CASE(clk_rate_range_test_set_range_round_rate_higher), KUNIT_CASE(clk_rate_range_test_set_range_get_rate_raised), KUNIT_CASE(clk_rate_range_test_set_range_get_rate_lowered), {} diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8de6a22498e7..7bb5ae0fb688 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1330,6 +1330,8 @@ static int clk_core_determine_round_nolock(struct clk_core *core, if (!core) return 0;
+ req->rate = clamp(req->rate, req->min_rate, req->max_rate); + /* * At this point, core protection will be disabled * - if the provider is not protected at all
The code in clk_set_rate_range() will, if the current rate is outside of the new range, will force it to the minimum or maximum. This is equivalent to using clamp, while being less readable. Let's switch to using clamp instead.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 7bb5ae0fb688..150d1bc0985b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2365,11 +2365,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) * this corner case when determining the rate */
- if (rate < min) - rate = min; - else - rate = max; - + rate = clamp(clk->core->req_rate, min, max); ret = clk_core_set_rate_nolock(clk->core, rate); if (ret) { /* rollback the changes */
When we change a clock minimum or maximum using clk_set_rate_range(), clk_set_min_rate() or clk_set_max_rate(), the current code will only trigger a new rate change if the rate is outside of the new boundaries.
However, a clock driver might want to always keep the clock rate to one of its boundary, for example the minimum to keep the power consumption as low as possible.
Since they don't always get called though, clock providers don't have the opportunity to implement this behaviour.
Let's trigger a clk_set_rate() on the previous requested rate every time clk_set_rate_range() is called. That way, providers that care about the new boundaries have a chance to adjust the rate, while providers that don't care about those new boundaries will return the same rate than before, which will be ignored by clk_set_rate() and won't result in a new rate change.
Also add a few new test cases to make sure that behaviour is not broken in the future.
Suggested-by: Stephen Boyd sboyd@kernel.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-rate-test.c | 281 +++++++++++++++++++++++++++++++++++- drivers/clk/clk.c | 45 +++--- 2 files changed, 303 insertions(+), 23 deletions(-)
diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c index a13b02702d20..baf0ea315322 100644 --- a/drivers/clk/clk-rate-test.c +++ b/drivers/clk/clk-rate-test.c @@ -6,6 +6,9 @@ #include <linux/clk-provider.h> #include <linux/slab.h>
+/* Needed for clk_hw_create_clk() */ +#include "clk.h" + #include <kunit/test.h>
#define DUMMY_CLOCK_INIT_RATE (42 * 1000 * 1000) @@ -33,6 +36,32 @@ static int clk_dummy_rate_determine_rate(struct clk_hw *hw, return 0; }
+static int clk_dummy_rate_maximize_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + /* + * If there's a maximum set, always run the clock at the maximum + * allowed. + */ + if (req->max_rate < ULONG_MAX) + req->rate = req->max_rate; + + return 0; +} + +static int clk_dummy_rate_minimize_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + /* + * If there's a minimum set, always run the clock at the minimum + * allowed. + */ + if (req->min_rate > 0) + req->rate = req->min_rate; + + return 0; +} + static int clk_dummy_rate_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) @@ -50,6 +79,18 @@ const static struct clk_ops clk_dummy_rate_ops = { .set_rate = clk_dummy_rate_set_rate, };
+const static struct clk_ops clk_dummy_maximize_rate_ops = { + .recalc_rate = clk_dummy_rate_recalc_rate, + .determine_rate = clk_dummy_rate_maximize_rate, + .set_rate = clk_dummy_rate_set_rate, +}; + +const static struct clk_ops clk_dummy_minimize_rate_ops = { + .recalc_rate = clk_dummy_rate_recalc_rate, + .determine_rate = clk_dummy_rate_minimize_rate, + .set_rate = clk_dummy_rate_set_rate, +}; + static int clk_rate_test_init_with_ops(struct kunit *test, const struct clk_ops *ops) { @@ -79,6 +120,16 @@ static int clk_rate_test_init(struct kunit *test) return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops); }
+static int clk_rate_maximize_test_init(struct kunit *test) +{ + return clk_rate_test_init_with_ops(test, &clk_dummy_maximize_rate_ops); +} + +static int clk_rate_minimize_test_init(struct kunit *test) +{ + return clk_rate_test_init_with_ops(test, &clk_dummy_minimize_rate_ops); +} + static void clk_rate_test_exit(struct kunit *test) { struct clk_dummy_rate_context *ctx = test->priv; @@ -317,8 +368,236 @@ static struct kunit_suite clk_rate_range_test_suite = { .test_cases = clk_rate_range_test_cases, };
+/* + * Test that if we have several subsequent calls to + * clk_set_rate_range(), the core will reevaluate whether a new rate is + * needed each and every time. + * + * With clk_dummy_maximize_rate_ops, this means that the the rate will + * trail along the maximum as it evolves. + */ +static void clk_rate_range_test_set_range_rate_maximized(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + int ret; + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2 - 1000); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); +} +/* + * Test that if we have several subsequent calls to + * clk_set_rate_range(), across multiple users, the core will reevaluate + * whether a new rate is needed each and every time. + * + * With clk_dummy_maximize_rate_ops, this means that the the rate will + * trail along the maximum as it evolves. + */ +static void clk_rate_range_test_multiple_set_range_rate_maximized(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + struct clk *user1, *user2; + unsigned long rate; + int ret; + + user1 = clk_hw_create_clk(NULL, hw, NULL, NULL); + KUNIT_ASSERT_FALSE(test, IS_ERR(user1)); + + user2 = clk_hw_create_clk(NULL, hw, NULL, NULL); + KUNIT_ASSERT_FALSE(test, IS_ERR(user2)); + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = clk_set_rate_range(user1, + 0, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); + + ret = clk_set_rate_range(user2, + 0, + DUMMY_CLOCK_RATE_1); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); + + ret = clk_set_rate_range(user2, 0, ULONG_MAX); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); + + clk_put(user2); + clk_put(user1); +} + +static struct kunit_case clk_rate_range_maximize_test_cases[] = { + KUNIT_CASE(clk_rate_range_test_set_range_rate_maximized), + KUNIT_CASE(clk_rate_range_test_multiple_set_range_rate_maximized), + {} +}; + +static struct kunit_suite clk_rate_range_maximize_test_suite = { + .name = "clk-rate-range-maximize-test", + .init = clk_rate_maximize_test_init, + .exit = clk_rate_test_exit, + .test_cases = clk_rate_range_maximize_test_cases, +}; + +/* + * Test that if we have several subsequent calls to + * clk_set_rate_range(), the core will reevaluate whether a new rate is + * needed each and every time. + * + * With clk_dummy_minimize_rate_ops, this means that the the rate will + * trail along the minimum as it evolves. + */ +static void clk_rate_range_test_set_range_rate_minimized(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + unsigned long rate; + int ret; + + ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1 + 1000, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1 + 1000); + + ret = clk_set_rate_range(clk, + DUMMY_CLOCK_RATE_1, + DUMMY_CLOCK_RATE_2); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); +} + +/* + * Test that if we have several subsequent calls to + * clk_set_rate_range(), across multiple users, the core will reevaluate + * whether a new rate is needed each and every time. + * + * With clk_dummy_minimize_rate_ops, this means that the the rate will + * trail along the minimum as it evolves. + */ +static void clk_rate_range_test_multiple_set_range_rate_minimized(struct kunit *test) +{ + struct clk_dummy_rate_context *ctx = test->priv; + struct clk_hw *hw = &ctx->hw; + struct clk *clk = hw->clk; + struct clk *user1, *user2; + unsigned long rate; + int ret; + + user1 = clk_hw_create_clk(NULL, hw, NULL, NULL); + KUNIT_ASSERT_FALSE(test, IS_ERR(user1)); + + user2 = clk_hw_create_clk(NULL, hw, NULL, NULL); + KUNIT_ASSERT_FALSE(test, IS_ERR(user2)); + + ret = clk_set_rate_range(user1, + DUMMY_CLOCK_RATE_1, + ULONG_MAX); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); + + ret = clk_set_rate_range(user2, + DUMMY_CLOCK_RATE_2, + ULONG_MAX); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2); + + ret = clk_set_rate_range(user2, 0, ULONG_MAX); + KUNIT_ASSERT_EQ(test, ret, 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_TRUE(test, rate > 0); + KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); + + clk_put(user2); + clk_put(user1); +} + +static struct kunit_case clk_rate_range_minimize_test_cases[] = { + KUNIT_CASE(clk_rate_range_test_set_range_rate_minimized), + KUNIT_CASE(clk_rate_range_test_multiple_set_range_rate_minimized), + {} +}; + +static struct kunit_suite clk_rate_range_minimize_test_suite = { + .name = "clk-rate-range-minimize-test", + .init = clk_rate_minimize_test_init, + .exit = clk_rate_test_exit, + .test_cases = clk_rate_range_minimize_test_cases, +}; + kunit_test_suites( &clk_rate_test_suite, - &clk_rate_range_test_suite + &clk_rate_range_test_suite, + &clk_rate_range_maximize_test_suite, + &clk_rate_range_minimize_test_suite ); MODULE_LICENSE("GPL v2"); diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 150d1bc0985b..718cab23f706 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2350,28 +2350,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) clk->min_rate = min; clk->max_rate = max;
- rate = clk_core_get_rate_nolock(clk->core); - if (rate < min || rate > max) { - /* - * FIXME: - * We are in bit of trouble here, current rate is outside the - * the requested range. We are going try to request appropriate - * range boundary but there is a catch. It may fail for the - * usual reason (clock broken, clock protected, etc) but also - * because: - * - round_rate() was not favorable and fell on the wrong - * side of the boundary - * - the determine_rate() callback does not really check for - * this corner case when determining the rate - */ - - rate = clamp(clk->core->req_rate, min, max); - ret = clk_core_set_rate_nolock(clk->core, rate); - if (ret) { - /* rollback the changes */ - clk->min_rate = old_min; - clk->max_rate = old_max; - } + /* + * Since the boundaries have been changed, let's give the + * opportunity to the provider to adjust the clock rate based on + * the new boundaries. + * + * We also need to handle the case where the clock is currently + * outside of the boundaries. Clamping the last requested rate + * to the current minimum and maximum will also handle this. + * + * FIXME: + * There is a catch. It may fail for the usual reason (clock + * broken, clock protected, etc) but also because: + * - round_rate() was not favorable and fell on the wrong + * side of the boundary + * - the determine_rate() callback does not really check for + * this corner case when determining the rate + */ + rate = clamp(clk->core->req_rate, min, max); + ret = clk_core_set_rate_nolock(clk->core, rate); + if (ret) { + /* rollback the changes */ + clk->min_rate = old_min; + clk->max_rate = old_max; }
if (clk->exclusive_count)
In order to reset the range on a clock, we need to call clk_set_rate_range with a minimum of 0 and a maximum of ULONG_MAX. Since it's fairly inconvenient, let's introduce a clk_drop_range() function that will do just this.
Suggested-by: Stephen Boyd sboyd@kernel.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-rate-test.c | 4 ++-- include/linux/clk.h | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c index baf0ea315322..9beed87f663c 100644 --- a/drivers/clk/clk-rate-test.c +++ b/drivers/clk/clk-rate-test.c @@ -458,7 +458,7 @@ static void clk_rate_range_test_multiple_set_range_rate_maximized(struct kunit * KUNIT_ASSERT_TRUE(test, rate > 0); KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
- ret = clk_set_rate_range(user2, 0, ULONG_MAX); + ret = clk_drop_range(user2); KUNIT_ASSERT_EQ(test, ret, 0);
rate = clk_get_rate(clk); @@ -570,7 +570,7 @@ static void clk_rate_range_test_multiple_set_range_rate_minimized(struct kunit * KUNIT_ASSERT_TRUE(test, rate > 0); KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
- ret = clk_set_rate_range(user2, 0, ULONG_MAX); + ret = clk_drop_range(user2); KUNIT_ASSERT_EQ(test, ret, 0);
rate = clk_get_rate(clk); diff --git a/include/linux/clk.h b/include/linux/clk.h index 266e8de3cb51..f365dac7be17 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -1005,6 +1005,17 @@ static inline struct clk *clk_get_optional(struct device *dev, const char *id) return clk; }
+/** + * clk_drop_range - Reset any range set on that clock + * @clk: clock source + * + * Returns success (0) or negative errno. + */ +static inline int clk_drop_range(struct clk *clk) +{ + return clk_set_rate_range(clk, 0, ULONG_MAX); +} + #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *of_clk_get(struct device_node *np, int index); struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
We only export a bunch of firmware clocks, and some of them require special treatment.
This has been do so far using some tests on the clock id in various places, but this is fairly hard to extend and doesn't scale very well.
Since we'll need some more cases in the next patches, let's switch to a variant structure that defines the behaviour we need to have for a given clock.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/bcm/clk-raspberrypi.c | 62 +++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 16 deletions(-)
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index dd3b71eafabf..f7185d421085 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -56,6 +56,8 @@ static char *rpi_firmware_clk_names[] = { #define RPI_FIRMWARE_STATE_ENABLE_BIT BIT(0) #define RPI_FIRMWARE_STATE_WAIT_BIT BIT(1)
+struct raspberrypi_clk_variant; + struct raspberrypi_clk { struct device *dev; struct rpi_firmware *firmware; @@ -66,10 +68,36 @@ struct raspberrypi_clk_data { struct clk_hw hw;
unsigned int id; + struct raspberrypi_clk_variant *variant;
struct raspberrypi_clk *rpi; };
+struct raspberrypi_clk_variant { + bool export; + char *clkdev; +}; + +static struct raspberrypi_clk_variant +raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { + [RPI_FIRMWARE_ARM_CLK_ID] = { + .export = true, + .clkdev = "cpu0", + }, + [RPI_FIRMWARE_CORE_CLK_ID] = { + .export = true, + }, + [RPI_FIRMWARE_M2MC_CLK_ID] = { + .export = true, + }, + [RPI_FIRMWARE_V3D_CLK_ID] = { + .export = true, + }, + [RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = { + .export = true, + }, +}; + /* * Structure of the message passed to Raspberry Pi's firmware in order to * change clock rates. The 'disable_turbo' option is only available to the ARM @@ -183,7 +211,8 @@ static const struct clk_ops raspberrypi_firmware_clk_ops = {
static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, unsigned int parent, - unsigned int id) + unsigned int id, + struct raspberrypi_clk_variant *variant) { struct raspberrypi_clk_data *data; struct clk_init_data init = {}; @@ -195,6 +224,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, return ERR_PTR(-ENOMEM); data->rpi = rpi; data->id = id; + data->variant = variant;
init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%s", @@ -228,9 +258,9 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
clk_hw_set_rate_range(&data->hw, min_rate, max_rate);
- if (id == RPI_FIRMWARE_ARM_CLK_ID) { + if (variant->clkdev) { ret = devm_clk_hw_register_clkdev(rpi->dev, &data->hw, - NULL, "cpu0"); + NULL, variant->clkdev); if (ret) { dev_err(rpi->dev, "Failed to initialize clkdev\n"); return ERR_PTR(ret); @@ -264,27 +294,27 @@ static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi, return ret;
while (clks->id) { - struct clk_hw *hw; + struct raspberrypi_clk_variant *variant; + + if (clks->id > RPI_FIRMWARE_NUM_CLK_ID) { + dev_err(rpi->dev, "Unknown clock id: %u", clks->id); + return -EINVAL; + } + + variant = &raspberrypi_clk_variants[clks->id]; + if (variant->export) { + struct clk_hw *hw;
- switch (clks->id) { - case RPI_FIRMWARE_ARM_CLK_ID: - case RPI_FIRMWARE_CORE_CLK_ID: - case RPI_FIRMWARE_M2MC_CLK_ID: - case RPI_FIRMWARE_V3D_CLK_ID: - case RPI_FIRMWARE_PIXEL_BVB_CLK_ID: hw = raspberrypi_clk_register(rpi, clks->parent, - clks->id); + clks->id, variant); if (IS_ERR(hw)) return PTR_ERR(hw);
data->hws[clks->id] = hw; data->num = clks->id + 1; - fallthrough; - - default: - clks++; - break; } + + clks++; }
return 0;
The M2MC clock provides the state machine clock for both HDMI controllers.
However, if no HDMI monitor is plugged in at boot, its clock rate will be left at 0 by the firmware and will make any register access end up in a CPU stall, even though the clock was enabled.
We had some code in the HDMI controller to deal with this before, but it makes more sense to have it in the clock driver. Move it there.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/bcm/clk-raspberrypi.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index f7185d421085..c879f2e9a4a7 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -76,6 +76,7 @@ struct raspberrypi_clk_data { struct raspberrypi_clk_variant { bool export; char *clkdev; + unsigned long min_rate; };
static struct raspberrypi_clk_variant @@ -89,6 +90,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { }, [RPI_FIRMWARE_M2MC_CLK_ID] = { .export = true, + + /* + * If we boot without any cable connected to any of the + * HDMI connector, the firmware will skip the HSM + * initialization and leave it with a rate of 0, + * resulting in a bus lockup when we're accessing the + * registers even if it's enabled. + * + * Let's put a sensible default so that we don't end up + * in this situation. + */ + .min_rate = 120000000, }, [RPI_FIRMWARE_V3D_CLK_ID] = { .export = true, @@ -267,6 +280,19 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, } }
+ if (variant->min_rate) { + unsigned long rate; + + clk_hw_set_rate_range(&data->hw, variant->min_rate, max_rate); + + rate = raspberrypi_fw_get_rate(&data->hw, 0); + if (rate < variant->min_rate) { + ret = raspberrypi_fw_set_rate(&data->hw, variant->min_rate, 0); + if (ret) + return ERR_PTR(ret); + } + } + return &data->hw; }
The core clock and M2MC clocks are shared between some devices (Unicam controllers and the HVS, and the HDMI controllers, respectively) that will have various, varying, requirements depending on their current work load.
Since those loads can require a fairly high clock rate in extreme conditions (up to ~600MHz), we can end up running those clocks at their maximum frequency even though we no longer require such a high rate.
Fortunately, those devices don't require an exact rate but a minimum rate, and all the drivers are using clk_set_min_rate. Thus, we can just rely on the fact that the clk_request minimum (which is the aggregated minimum of all the clock users) is what we want at all times.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/bcm/clk-raspberrypi.c | 37 +++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index c879f2e9a4a7..9d09621549b9 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -77,6 +77,7 @@ struct raspberrypi_clk_variant { bool export; char *clkdev; unsigned long min_rate; + bool minimize; };
static struct raspberrypi_clk_variant @@ -87,6 +88,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { }, [RPI_FIRMWARE_CORE_CLK_ID] = { .export = true, + + /* + * The clock is shared between the HVS and the CSI + * controllers, on the BCM2711 and will change depending + * on the pixels composited on the HVS and the capture + * resolution on Unicam. + * + * Since the rate can get quite large, and we need to + * coordinate between both driver instances, let's + * always use the minimum the drivers will let us. + */ + .minimize = true, }, [RPI_FIRMWARE_M2MC_CLK_ID] = { .export = true, @@ -102,6 +115,16 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { * in this situation. */ .min_rate = 120000000, + + /* + * The clock is shared between the two HDMI controllers + * on the BCM2711 and will change depending on the + * resolution output on each. Since the rate can get + * quite large, and we need to coordinate between both + * driver instances, let's always use the minimum the + * drivers will let us. + */ + .minimize = true, }, [RPI_FIRMWARE_V3D_CLK_ID] = { .export = true, @@ -206,12 +229,26 @@ static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate, static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { + struct raspberrypi_clk_data *data = + container_of(hw, struct raspberrypi_clk_data, hw); + struct raspberrypi_clk_variant *variant = data->variant; + /* * The firmware will do the rounding but that isn't part of * the interface with the firmware, so we just do our best * here. */ + req->rate = clamp(req->rate, req->min_rate, req->max_rate); + + /* + * We want to aggressively reduce the clock rate here, so let's + * just ignore the requested rate and return the bare minimum + * rate we can get away with. + */ + if (variant->minimize && req->min_rate > 0) + req->rate = req->min_rate; + return 0; }
The HVS core clock isn't really obvious, so let's add a bunch more comments and some logging for easier debugging.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 24de29bc1cda..6fe03fc17d73 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -389,8 +389,15 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) 500000000, new_hvs_state->core_clock_rate);
+ drm_dbg(dev, "Raising the core clock at %lu Hz\n", core_rate); + + /* + * Do a temporary request on the core clock during the + * modeset. + */ clk_set_min_rate(hvs->core_clk, core_rate); } + drm_atomic_helper_commit_modeset_disables(dev, state);
vc4_ctm_commit(vc4, state); @@ -416,6 +423,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) drm_dbg(dev, "Running the core clock at %lu Hz\n", new_hvs_state->core_clock_rate);
+ /* + * Request a clock rate based on the current HVS + * requirements. + */ clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate); } }
Now that the clock driver makes sure we never end up with a rate of 0, the HDMI driver doesn't need to care anymore.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 13 ------------- 1 file changed, 13 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 053fbaf765ca..43aced269082 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2543,19 +2543,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi->disable_4kp60 = true; }
- /* - * If we boot without any cable connected to the HDMI connector, - * the firmware will skip the HSM initialization and leave it - * with a rate of 0, resulting in a bus lockup when we're - * accessing the registers even if it's enabled. - * - * Let's put a sensible default at runtime_resume so that we - * don't end up in this situation. - */ - ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ); - if (ret) - goto err_put_ddc; - /* * We need to have the device powered up at this point to call * our reset hook and for the CEC init.
dri-devel@lists.freedesktop.org
-
Daniel Latypov -
Maxime Ripard -
Stephen Boyd