[PATCH v2 0/4] clk: ccf: fix enable_count mismatch
As described in [1], enable_count is incremented by 2 when ccf_clk_enable() is called. This series of patch fixed this issue and added a testcase for that.
[1]: https://lore.kernel.org/all/SEZPR06MB695927A6DEEEF8489A06897396A7A@SEZPR06MB...
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- Changes in v2: - add missing SoB in patch 1/4, no functional change - Link to v1: https://lore.kernel.org/r/20231111-enable_count-v1-0-509f400a99cb@outlook.co...
--- Yang Xiwen (4): clk: clk_sandbox_ccf: assign ccf_clk_ops to .ops of the driver clk: get correct ops for clk_enable() and clk_disable() clk: clk_sandbox: get devm clock i2c_root test: dm: clk_ccf: get "i2c_root" clock from &clk_ccf
arch/sandbox/dts/test.dts | 6 ++++-- arch/sandbox/include/asm/clk.h | 1 + drivers/clk/clk-uclass.c | 2 ++ drivers/clk/clk_sandbox_ccf.c | 1 + drivers/clk/clk_sandbox_test.c | 5 +++++ test/dm/clk_ccf.c | 14 +++++++++++--- 6 files changed, 24 insertions(+), 5 deletions(-) --- base-commit: 3b913c148249a2b9d12ff25517ec311646e83bee change-id: 20231111-enable_count-ad5001326815
Best regards,
From: Yang Xiwen forbidden405@outlook.com
It can now act as an clk provider on which ccf_clk_ops can be tested. Also add "#clock-cells=<1>" to test.dts.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- arch/sandbox/dts/test.dts | 1 + drivers/clk/clk_sandbox_ccf.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index c7197795ef..b1773f1bc2 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -654,6 +654,7 @@
ccf: clk-ccf { compatible = "sandbox,clk-ccf"; + #clock-cells = <1>; };
efi-media { diff --git a/drivers/clk/clk_sandbox_ccf.c b/drivers/clk/clk_sandbox_ccf.c index fedcdd4044..38184e27aa 100644 --- a/drivers/clk/clk_sandbox_ccf.c +++ b/drivers/clk/clk_sandbox_ccf.c @@ -284,6 +284,7 @@ static int sandbox_clk_ccf_probe(struct udevice *dev) U_BOOT_DRIVER(sandbox_clk_ccf) = { .name = "sandbox_clk_ccf", .id = UCLASS_CLK, + .ops = &ccf_clk_ops, .probe = sandbox_clk_ccf_probe, .of_match = sandbox_clk_ccf_test_ids, };
From: Yang Xiwen forbidden405@outlook.com
assign clk_dev_ops(clkp->dev) to ops to ensure correct clk operations are called on clocks.
This fixes the incorrect enable_count issue as described in [1].
[1]: https://lore.kernel.org/all/SEZPR06MB695927A6DEEEF8489A06897396A7A@SEZPR06MB...
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- drivers/clk/clk-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 3b5e3f9c86..3e9d68feb3 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -640,6 +640,7 @@ int clk_enable(struct clk *clk) if (CONFIG_IS_ENABLED(CLK_CCF)) { /* Take id 0 as a non-valid clk, such as dummy */ if (clk->id && !clk_get_by_id(clk->id, &clkp)) { + ops = clk_dev_ops(clkp->dev); if (clkp->enable_count) { clkp->enable_count++; return 0; @@ -699,6 +700,7 @@ int clk_disable(struct clk *clk)
if (CONFIG_IS_ENABLED(CLK_CCF)) { if (clk->id && !clk_get_by_id(clk->id, &clkp)) { + ops = clk_dev_ops(clkp->dev); if (clkp->flags & CLK_IS_CRITICAL) return 0;
On 11/18/23 17:10, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
assign clk_dev_ops(clkp->dev) to ops to ensure correct clk operations are called on clocks.
This fixes the incorrect enable_count issue as described in [1].
Signed-off-by: Yang Xiwen forbidden405@outlook.com
drivers/clk/clk-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 3b5e3f9c86..3e9d68feb3 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -640,6 +640,7 @@ int clk_enable(struct clk *clk) if (CONFIG_IS_ENABLED(CLK_CCF)) { /* Take id 0 as a non-valid clk, such as dummy */ if (clk->id && !clk_get_by_id(clk->id, &clkp)) {
ops = clk_dev_ops(clkp->dev); if (clkp->enable_count) { clkp->enable_count++; return 0;@@ -699,6 +700,7 @@ int clk_disable(struct clk *clk)
if (CONFIG_IS_ENABLED(CLK_CCF)) { if (clk->id && !clk_get_by_id(clk->id, &clkp)) {
ops = clk_dev_ops(clkp->dev); if (clkp->flags & CLK_IS_CRITICAL) return 0;
Reviewed-by: Sean Anderson seanga2@gmail.com
On Sun, 19 Nov 2023 06:10:06 +0800, Yang Xiwen wrote:
assign clk_dev_ops(clkp->dev) to ops to ensure correct clk operations are called on clocks.
This fixes the incorrect enable_count issue as described in [1].
[...]
Applied, thanks!
[2/4] clk: get correct ops for clk_enable() and clk_disable() https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/3fb2d3d6acba
Best regards,
From: Yang Xiwen forbidden405@outlook.com
This clock is added to dts. Get it in the devm group in the driver or the testcases will fail.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- arch/sandbox/dts/test.dts | 5 +++-- arch/sandbox/include/asm/clk.h | 1 + drivers/clk/clk_sandbox_test.c | 5 +++++ 3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index b1773f1bc2..f99397c528 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -632,8 +632,9 @@ <&clk_sandbox 1>, <&clk_sandbox 0>, <&clk_sandbox 3>, - <&clk_sandbox 2>; - clock-names = "fixed", "i2c", "spi", "uart2", "uart1"; + <&clk_sandbox 2>, + <&ccf 11>; + clock-names = "fixed", "i2c", "spi", "uart2", "uart1", "i2c_root"; };
clk-test2 { diff --git a/arch/sandbox/include/asm/clk.h b/arch/sandbox/include/asm/clk.h index df7156fe31..597bc528dc 100644 --- a/arch/sandbox/include/asm/clk.h +++ b/arch/sandbox/include/asm/clk.h @@ -41,6 +41,7 @@ enum sandbox_clk_test_id { SANDBOX_CLK_TEST_ID_I2C, SANDBOX_CLK_TEST_ID_DEVM1, SANDBOX_CLK_TEST_ID_DEVM2, + SANDBOX_CLK_TEST_ID_I2C_ROOT, SANDBOX_CLK_TEST_ID_DEVM_NULL,
SANDBOX_CLK_TEST_ID_COUNT, diff --git a/drivers/clk/clk_sandbox_test.c b/drivers/clk/clk_sandbox_test.c index 5807a454f3..c0623dee10 100644 --- a/drivers/clk/clk_sandbox_test.c +++ b/drivers/clk/clk_sandbox_test.c @@ -53,6 +53,11 @@ int sandbox_clk_test_devm_get(struct udevice *dev) return PTR_ERR(clk); sbct->clkps[SANDBOX_CLK_TEST_ID_DEVM2] = clk;
+ clk = devm_clk_get_optional(dev, "i2c_root"); + if (IS_ERR(clk)) + return PTR_ERR(clk); + sbct->clkps[SANDBOX_CLK_TEST_ID_I2C_ROOT] = clk; + sbct->clkps[SANDBOX_CLK_TEST_ID_DEVM_NULL] = NULL; clk = devm_clk_get_optional(dev, "not_an_existing_clock"); if (IS_ERR(clk))
From: Yang Xiwen forbidden405@outlook.com
get i2c_root clock from device tree. In this way we get an CCF clock and also test ccf_clk_ops.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- test/dm/clk_ccf.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/test/dm/clk_ccf.c b/test/dm/clk_ccf.c index e4ebb93cda..b8be6d6572 100644 --- a/test/dm/clk_ccf.c +++ b/test/dm/clk_ccf.c @@ -18,8 +18,8 @@ /* Tests for Common Clock Framework driver */ static int dm_test_clk_ccf(struct unit_test_state *uts) { - struct clk *clk, *pclk; - struct udevice *dev; + struct clk *clk, *pclk, clk_ccf; + struct udevice *dev, *test_dev; long long rate; int ret; #if CONFIG_IS_ENABLED(CLK_CCF) @@ -29,6 +29,7 @@ static int dm_test_clk_ccf(struct unit_test_state *uts)
/* Get the device using the clk device */ ut_assertok(uclass_get_device_by_name(UCLASS_CLK, "clk-ccf", &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "clk-test", &test_dev));
/* Test for clk_get_by_id() */ ret = clk_get_by_id(SANDBOX_CLK_ECSPI_ROOT, &clk); @@ -110,11 +111,18 @@ static int dm_test_clk_ccf(struct unit_test_state *uts)
#if CONFIG_IS_ENABLED(CLK_CCF) /* Test clk tree enable/disable */ + + ret = clk_get_by_index(test_dev, SANDBOX_CLK_TEST_ID_I2C_ROOT, &clk_ccf); + ut_assertok(ret); + ut_asserteq_str("clk-ccf", clk_ccf.dev->name); + ut_asserteq(clk_ccf.id, SANDBOX_CLK_I2C_ROOT); + ret = clk_get_by_id(SANDBOX_CLK_I2C_ROOT, &clk); ut_assertok(ret); ut_asserteq_str("i2c_root", clk->dev->name); + ut_asserteq(clk->id, SANDBOX_CLK_I2C_ROOT);
- ret = clk_enable(clk); + ret = clk_enable(&clk_ccf); ut_assertok(ret);
ret = sandbox_clk_enable_count(clk);
On 11/18/23 17:10, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
get i2c_root clock from device tree. In this way we get an CCF clock and also test ccf_clk_ops.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
test/dm/clk_ccf.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/test/dm/clk_ccf.c b/test/dm/clk_ccf.c index e4ebb93cda..b8be6d6572 100644 --- a/test/dm/clk_ccf.c +++ b/test/dm/clk_ccf.c @@ -18,8 +18,8 @@ /* Tests for Common Clock Framework driver */ static int dm_test_clk_ccf(struct unit_test_state *uts) {
- struct clk *clk, *pclk;
- struct udevice *dev;
- struct clk *clk, *pclk, clk_ccf;
- struct udevice *dev, *test_dev; long long rate; int ret; #if CONFIG_IS_ENABLED(CLK_CCF)
@@ -29,6 +29,7 @@ static int dm_test_clk_ccf(struct unit_test_state *uts)
/* Get the device using the clk device */ ut_assertok(uclass_get_device_by_name(UCLASS_CLK, "clk-ccf", &dev));
ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "clk-test", &test_dev));
/* Test for clk_get_by_id() */ ret = clk_get_by_id(SANDBOX_CLK_ECSPI_ROOT, &clk);
@@ -110,11 +111,18 @@ static int dm_test_clk_ccf(struct unit_test_state *uts)
#if CONFIG_IS_ENABLED(CLK_CCF) /* Test clk tree enable/disable */
- ret = clk_get_by_index(test_dev, SANDBOX_CLK_TEST_ID_I2C_ROOT, &clk_ccf);
- ut_assertok(ret);
- ut_asserteq_str("clk-ccf", clk_ccf.dev->name);
- ut_asserteq(clk_ccf.id, SANDBOX_CLK_I2C_ROOT);
- ret = clk_get_by_id(SANDBOX_CLK_I2C_ROOT, &clk); ut_assertok(ret); ut_asserteq_str("i2c_root", clk->dev->name);
- ut_asserteq(clk->id, SANDBOX_CLK_I2C_ROOT);
- ret = clk_enable(clk);
ret = clk_enable(&clk_ccf); ut_assertok(ret);
ret = sandbox_clk_enable_count(clk);
This patch should be combined with patches 1 and 3. With the changes spread out over 3 patches it is hard to see what is going on.
--Sean
participants (2)
-
Sean Anderson -
Yang Xiwen via B4 Relay