* [PATCH 0/2] clk: Fix CLK_OPS_PARENT_ENABLE and runtime PM @ 2022-07-12 11:34 Chen-Yu Tsai 2022-07-12 11:34 ` [PATCH 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops Chen-Yu Tsai 2022-07-12 11:34 ` [PATCH 2/2] clk: core: Fix runtime PM sequence in clk_core_unprepare() Chen-Yu Tsai 0 siblings, 2 replies; 4+ messages in thread From: Chen-Yu Tsai @ 2022-07-12 11:34 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd Cc: Chen-Yu Tsai, linux-clk, linux-kernel, linux-arm-kernel, AngeloGioacchino Del Regno, Nícolas F. R. A. Prado 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops 2022-07-12 11:34 [PATCH 0/2] clk: Fix CLK_OPS_PARENT_ENABLE and runtime PM Chen-Yu Tsai @ 2022-07-12 11:34 ` Chen-Yu Tsai 2022-07-12 18:43 ` Nícolas F. R. A. Prado 2022-07-12 11:34 ` [PATCH 2/2] clk: core: Fix runtime PM sequence in clk_core_unprepare() Chen-Yu Tsai 1 sibling, 1 reply; 4+ messages in thread From: Chen-Yu Tsai @ 2022-07-12 11:34 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd Cc: Chen-Yu Tsai, linux-clk, linux-kernel, linux-arm-kernel, AngeloGioacchino Del Regno, Nícolas F. R. A. Prado 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 <wenst@chromium.org> --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops 2022-07-12 11:34 ` [PATCH 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops Chen-Yu Tsai @ 2022-07-12 18:43 ` Nícolas F. R. A. Prado 0 siblings, 0 replies; 4+ messages in thread From: Nícolas F. R. A. Prado @ 2022-07-12 18:43 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Michael Turquette, Stephen Boyd, linux-clk, linux-kernel, linux-arm-kernel, AngeloGioacchino Del Regno 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 <wenst@chromium.org> > --- > > 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] clk: core: Fix runtime PM sequence in clk_core_unprepare() 2022-07-12 11:34 [PATCH 0/2] clk: Fix CLK_OPS_PARENT_ENABLE and runtime PM Chen-Yu Tsai 2022-07-12 11:34 ` [PATCH 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops Chen-Yu Tsai @ 2022-07-12 11:34 ` Chen-Yu Tsai 1 sibling, 0 replies; 4+ messages in thread From: Chen-Yu Tsai @ 2022-07-12 11:34 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd Cc: Chen-Yu Tsai, linux-clk, linux-kernel, linux-arm-kernel, AngeloGioacchino Del Regno, Nícolas F. R. A. Prado 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 <wenst@chromium.org> --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-12 18:49 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-12 11:34 [PATCH 0/2] clk: Fix CLK_OPS_PARENT_ENABLE and runtime PM Chen-Yu Tsai 2022-07-12 11:34 ` [PATCH 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops Chen-Yu Tsai 2022-07-12 18:43 ` Nícolas F. R. A. Prado 2022-07-12 11:34 ` [PATCH 2/2] clk: core: Fix runtime PM sequence in clk_core_unprepare() Chen-Yu Tsai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox