Hi Mike, Stephen,
Here are a couple fixes for the clk core. They are unrelated but overlap
in diff context, so I'm sending them together.
Patch 1 makes the clk core honor CLK_OPS_PARENT_ENABLE for clk gate ops.
Without this, dumping clk_summary on the MT8192 would cause the system
to hang.
Patch 2 reorders the runtime PM call in clk_core_unprepare() to match
the order described in its original commit, and the opposite of that
in clk_core_prepare().
Regards
ChenYu
Chen-Yu Tsai (2):
clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops
clk: core: Fix runtime PM sequence in clk_core_unprepare()
drivers/clk/clk.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
--
2.37.0.144.g8ac04bfd2-goog
In the original commit 9a34b45397e5 ("clk: Add support for runtime PM"),
the commit message mentioned that pm_runtime_put_sync() would be done
at the end of clk_core_unprepare(). This mirrors the operations in
clk_core_prepare() in the opposite order.
However, the actual code that was added wasn't in the order the commit
message described. Move clk_pm_runtime_put() to the end of
clk_core_unprepare() so that it is in the correct order.
Fixes: 9a34b45397e5 ("clk: Add support for runtime PM")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/clk/clk.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b3de636eec84..87684daadedd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -855,13 +855,12 @@ static void clk_core_unprepare(struct clk_core *core)
if (core->ops->unprepare)
core->ops->unprepare(core->hw);
- clk_pm_runtime_put(core);
-
trace_clk_unprepare_complete(core);
if (core->flags & CLK_OPS_PARENT_ENABLE)
clk_core_disable(core->parent);
clk_core_unprepare(core->parent);
+ clk_pm_runtime_put(core);
}
static void clk_core_unprepare_lock(struct clk_core *core)
--
2.37.0.144.g8ac04bfd2-goog
In the previous commits that added CLK_OPS_PARENT_ENABLE, support for
this flag was only added to rate change operations (rate setting and
reparent) and disabling unused subtree. It was not added to the
clock gate related operations. Any hardware driver that needs it for
these operations will either see bogus results, or worse, hang.
This has been seen on MT8192 and MT8195, where the imp_ii2_* clk
drivers set this, but dumping debugfs clk_summary would cause it
to hang.
Fixes: fc8726a2c021 ("clk: core: support clocks which requires parents enable (part 2)")
Fixes: a4b3518d146f ("clk: core: support clocks which requires parents enable (part 1)")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
I'm guessing Stephen might have some things to say about adding forward
declarations. Moving code around would make the patch larger though.
drivers/clk/clk.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7fc191c15507..b3de636eec84 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -215,6 +215,9 @@ static bool clk_core_is_prepared(struct clk_core *core)
return ret;
}
+static int clk_core_prepare_enable(struct clk_core *core);
+static void clk_core_disable_unprepare(struct clk_core *core);
+
static bool clk_core_is_enabled(struct clk_core *core)
{
bool ret = false;
@@ -226,6 +229,9 @@ static bool clk_core_is_enabled(struct clk_core *core)
if (!core->ops->is_enabled)
return core->enable_count;
+ if (core->flags & CLK_OPS_PARENT_ENABLE)
+ clk_core_prepare_enable(core->parent);
+
/*
* Check if clock controller's device is runtime active before
* calling .is_enabled callback. If not, assume that clock is
@@ -249,6 +255,9 @@ static bool clk_core_is_enabled(struct clk_core *core)
if (core->rpm_enabled)
pm_runtime_put(core->dev);
+ if (core->flags & CLK_OPS_PARENT_ENABLE)
+ clk_core_disable_unprepare(core->parent);
+
return ret;
}
@@ -812,6 +821,9 @@ int clk_rate_exclusive_get(struct clk *clk)
}
EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
+static int clk_core_enable(struct clk_core *core);
+static void clk_core_disable(struct clk_core *core);
+
static void clk_core_unprepare(struct clk_core *core)
{
lockdep_assert_held(&prepare_lock);
@@ -835,6 +847,9 @@ static void clk_core_unprepare(struct clk_core *core)
WARN(core->enable_count > 0, "Unpreparing enabled %s\n", core->name);
+ if (core->flags & CLK_OPS_PARENT_ENABLE)
+ clk_core_enable(core->parent);
+
trace_clk_unprepare(core);
if (core->ops->unprepare)
@@ -843,6 +858,9 @@ static void clk_core_unprepare(struct clk_core *core)
clk_pm_runtime_put(core);
trace_clk_unprepare_complete(core);
+
+ if (core->flags & CLK_OPS_PARENT_ENABLE)
+ clk_core_disable(core->parent);
clk_core_unprepare(core->parent);
}
@@ -891,6 +909,9 @@ static int clk_core_prepare(struct clk_core *core)
if (ret)
goto runtime_put;
+ if (core->flags & CLK_OPS_PARENT_ENABLE)
+ clk_core_enable(core->parent);
+
trace_clk_prepare(core);
if (core->ops->prepare)
@@ -898,6 +919,9 @@ static int clk_core_prepare(struct clk_core *core)
trace_clk_prepare_complete(core);
+ if (core->flags & CLK_OPS_PARENT_ENABLE)
+ clk_core_disable(core->parent);
+
if (ret)
goto unprepare;
}
--
2.37.0.144.g8ac04bfd2-goog
Hi Chen-Yu,
On Tue, Jul 12, 2022 at 07:34:01PM +0800, Chen-Yu Tsai wrote:
> In the previous commits that added CLK_OPS_PARENT_ENABLE, support for
> this flag was only added to rate change operations (rate setting and
> reparent) and disabling unused subtree. It was not added to the
> clock gate related operations. Any hardware driver that needs it for
> these operations will either see bogus results, or worse, hang.
>
> This has been seen on MT8192 and MT8195, where the imp_ii2_* clk
> drivers set this, but dumping debugfs clk_summary would cause it
> to hang.
>
> Fixes: fc8726a2c021 ("clk: core: support clocks which requires parents enable (part 2)")
> Fixes: a4b3518d146f ("clk: core: support clocks which requires parents enable (part 1)")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
>
> I'm guessing Stephen might have some things to say about adding forward
> declarations. Moving code around would make the patch larger though.
>
> drivers/clk/clk.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7fc191c15507..b3de636eec84 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
[..]
> +static int clk_core_enable(struct clk_core *core);
> +static void clk_core_disable(struct clk_core *core);
> +
> static void clk_core_unprepare(struct clk_core *core)
> {
> lockdep_assert_held(&prepare_lock);
> @@ -835,6 +847,9 @@ static void clk_core_unprepare(struct clk_core *core)
>
> WARN(core->enable_count > 0, "Unpreparing enabled %s\n", core->name);
>
> + if (core->flags & CLK_OPS_PARENT_ENABLE)
> + clk_core_enable(core->parent);
Shouldn't it be clk_core_enable_lock ? Otherwise you're calling into
clk_core_enable without the enable_lock, which causes a lot of lockdep warnings
during boot [1]. Same thing for other enable/disable calls below.
Thanks,
N?colas
[1] Warnings:
<4>[ 5.781843] ------------[ cut here ]------------
<4>[ 5.786823] WARNING: CPU: 5 PID: 1 at drivers/clk/clk.c:982 clk_core_disable+0x6c/0x2a0
<4>[ 5.795258] Modules linked in:
<4>[ 5.798652] CPU: 5 PID: 1 Comm: swapper/0 Tainted: G W 5.19.0-rc5-next-20220707+ #214
<4>[ 5.808318] Hardware name: Google Spherion (rev0 - 3) (DT)
<4>[ 5.814179] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
<4>[ 5.821548] pc : clk_core_disable+0x6c/0x2a0
<4>[ 5.826176] lr : clk_core_disable+0x68/0x2a0
<4>[ 5.830805] sp : ffff80000808ba50
<4>[ 5.834458] x29: ffff80000808ba50 x28: 0000000000061a80 x27: 00000000000f4240
<4>[ 5.842022] x26: 0000000000000020 x25: ffff23db40f86890 x24: 0000000000000001
<4>[ 5.849587] x23: ffffb70fcbcba008 x22: ffffb70fcc3e1eb8 x21: ffff23db40f86890
<4>[ 5.857152] x20: 0000000000000000 x19: ffff23db40d59d00 x18: 0000000000000028
<4>[ 5.864717] x17: 00000000ffffffff x16: ffffb70fcb632518 x15: 000000000000100a
<4>[ 5.872282] x14: 0000000000000040 x13: 0000000000000040 x12: 0000000000000400
<4>[ 5.879847] x11: 0000000000000080 x10: 000000006a1a6693 x9 : ffffb70fcac15014
<4>[ 5.887412] x8 : 00000000b7258824 x7 : ffffb70fcc6fc7b0 x6 : 0000000000000000
<4>[ 5.894976] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000000000c0
<4>[ 5.902540] x2 : ffff23db40258000 x1 : 0000000000000080 x0 : 0000000000000000
<4>[ 5.910105] Call trace:
<4>[ 5.912872] clk_core_disable+0x6c/0x2a0
<4>[ 5.917148] clk_core_prepare+0x1f4/0x430
<4>[ 5.921513] clk_prepare+0x30/0x50
<4>[ 5.925259] clk_bulk_prepare+0x40/0xc0
<4>[ 5.929447] mtk_i2c_probe+0x3b8/0x610
<4>[ 5.933546] platform_probe+0x70/0xe0
<4>[ 5.937558] really_probe+0xc8/0x3b0
<4>[ 5.941479] __driver_probe_device+0x84/0x190
<4>[ 5.946197] driver_probe_device+0x44/0xf8
<4>[ 5.950648] __driver_attach+0xc8/0x1c8
<4>[ 5.954834] bus_for_each_dev+0x78/0xc8
<4>[ 5.959020] driver_attach+0x2c/0x38
<4>[ 5.962941] bus_add_driver+0x184/0x240
<4>[ 5.967127] driver_register+0x6c/0x128
<4>[ 5.971314] __platform_driver_register+0x30/0x40
<4>[ 5.976386] mtk_i2c_driver_init+0x24/0x30
<4>[ 5.980840] do_one_initcall+0x80/0x408
<4>[ 5.985025] kernel_init_freeable+0x2d0/0x358
<4>[ 5.989739] kernel_init+0x2c/0x138
<4>[ 5.993572] ret_from_fork+0x10/0x20
<4>[ 5.997493] irq event stamp: 280038
<4>[ 6.001322] hardirqs last enabled at (280037): [<ffffb70fcb429490>] _raw_spin_unlock_irqrestore+0x80/0xa0
<4>[ 6.011435] hardirqs last disabled at (280038): [<ffffb70fcb415838>] el1_dbg+0x28/0xa0
<4>[ 6.019780] softirqs last enabled at (280028): [<ffffb70fca410698>] __do_softirq+0x488/0x64c
<4>[ 6.028739] softirqs last disabled at (280021): [<ffffb70fca4ab114>] irq_exit_rcu+0x1bc/0x1e8
<4>[ 6.037704] ---[ end trace 0000000000000000 ]---
<4>[ 4.962132] ------------[ cut here ]------------
<4>[ 4.967111] WARNING: CPU: 5 PID: 1 at drivers/clk/clk.c:1041 clk_core_enable+0x74/0x2b8
<4>[ 4.975546] Modules linked in:
<4>[ 4.978940] CPU: 5 PID: 1 Comm: swapper/0 Tainted: G W 5.19.0-rc5-next-20220707+ #214
<4>[ 4.988606] Hardware name: Google Spherion (rev0 - 3) (DT)
<4>[ 4.994468] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
<4>[ 5.001838] pc : clk_core_enable+0x74/0x2b8
<4>[ 5.006378] lr : clk_core_enable+0x70/0x2b8
<4>[ 5.010918] sp : ffff80000808b9c0
<4>[ 5.014571] x29: ffff80000808b9c0 x28: 0000000000061a80 x27: 00000000000f4240
<4>[ 5.022136] x26: 0000000000000020 x25: ffff23db40f86890 x24: 0000000000000001
<4>[ 5.029701] x23: ffff23db40f86890 x22: 0000000000000004 x21: ffff23db40f86890
<4>[ 5.037266] x20: 0000000000000000 x19: ffff23db40d4fc00 x18: 0000000000000028
<4>[ 5.044830] x17: 00000000ffffffff x16: ffffb70fcb632518 x15: 000000000000100a
<4>[ 5.052395] x14: 0000000000000040 x13: 0000000000000040 x12: 0000000000000400
<4>[ 5.059960] x11: 0000000000000080 x10: 000000006a1a6693 x9 : ffffb70fcac146e4
<4>[ 5.067525] x8 : 00000000b7258824 x7 : ffffb70fcc6fc7b0 x6 : 0000000000000000
<4>[ 5.075089] x5 : 0000000000000002 x4 : 0000000000000000 x3 : 00000000000000c0
<4>[ 5.082655] x2 : ffff23db40258000 x1 : 0000000000000080 x0 : 0000000000000000
<4>[ 5.090220] Call trace:
<4>[ 5.092988] clk_core_enable+0x74/0x2b8
<4>[ 5.097175] clk_core_enable+0x9c/0x2b8
<4>[ 5.101363] clk_core_enable+0x9c/0x2b8
<4>[ 5.105550] clk_core_prepare+0x1e4/0x430
<4>[ 5.109914] clk_prepare+0x30/0x50
<4>[ 5.113660] clk_bulk_prepare+0x40/0xc0
<4>[ 5.117849] mtk_i2c_probe+0x3b8/0x610
<4>[ 5.121948] platform_probe+0x70/0xe0
<4>[ 5.125959] really_probe+0xc8/0x3b0
<4>[ 5.129881] __driver_probe_device+0x84/0x190
<4>[ 5.134597] driver_probe_device+0x44/0xf8
<4>[ 5.139048] __driver_attach+0xc8/0x1c8
<4>[ 5.143235] bus_for_each_dev+0x78/0xc8
<4>[ 5.147420] driver_attach+0x2c/0x38
<4>[ 5.151341] bus_add_driver+0x184/0x240
<4>[ 5.155527] driver_register+0x6c/0x128
<4>[ 5.159714] __platform_driver_register+0x30/0x40
<4>[ 5.164787] mtk_i2c_driver_init+0x24/0x30
<4>[ 5.169241] do_one_initcall+0x80/0x408
<4>[ 5.173426] kernel_init_freeable+0x2d0/0x358
<4>[ 5.178142] kernel_init+0x2c/0x138
<4>[ 5.181974] ret_from_fork+0x10/0x20
<4>[ 5.185895] irq event stamp: 279998
<4>[ 5.189724] hardirqs last enabled at (279997): [<ffffb70fcb416a9c>] exit_to_kernel_mode.isra.0+0x34/0x128
<4>[ 5.199833] hardirqs last disabled at (279998): [<ffffb70fcb415838>] el1_dbg+0x28/0xa0
<4>[ 5.208180] softirqs last enabled at (279996): [<ffffb70fca410698>] __do_softirq+0x488/0x64c
<4>[ 5.217139] softirqs last disabled at (279991): [<ffffb70fca4ab114>] irq_exit_rcu+0x1bc/0x1e8
<4>[ 5.226103] ---[ end trace 0000000000000000 ]---
> +
> trace_clk_unprepare(core);
>
> if (core->ops->unprepare)
> @@ -843,6 +858,9 @@ static void clk_core_unprepare(struct clk_core *core)
> clk_pm_runtime_put(core);
>
> trace_clk_unprepare_complete(core);
> +
> + if (core->flags & CLK_OPS_PARENT_ENABLE)
> + clk_core_disable(core->parent);
> clk_core_unprepare(core->parent);
> }
>
> @@ -891,6 +909,9 @@ static int clk_core_prepare(struct clk_core *core)
> if (ret)
> goto runtime_put;
>
> + if (core->flags & CLK_OPS_PARENT_ENABLE)
> + clk_core_enable(core->parent);
> +
> trace_clk_prepare(core);
>
> if (core->ops->prepare)
> @@ -898,6 +919,9 @@ static int clk_core_prepare(struct clk_core *core)
>
> trace_clk_prepare_complete(core);
>
> + if (core->flags & CLK_OPS_PARENT_ENABLE)
> + clk_core_disable(core->parent);
> +
> if (ret)
> goto unprepare;
> }
> --
> 2.37.0.144.g8ac04bfd2-goog
>