diff mbox series

[1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops

Message ID 20220712113402.871838-2-wenst@chromium.org (mailing list archive)
State Superseded, archived
Headers show
Series clk: Fix CLK_OPS_PARENT_ENABLE and runtime PM | expand

Commit Message

Chen-Yu Tsai July 12, 2022, 11:34 a.m. UTC 
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(+)

Comments

Nícolas F. R. A. Prado July 12, 2022, 6:43 p.m. UTC | #1 
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
>
diff mbox series

Patch

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;
 	}