Messages in this thread | | | From | Chen-Yu Tsai <> | Date | Tue, 30 Aug 2022 21:40:53 +0800 | Subject | Re: [PATCH RESEND v2 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops |
| |
On Tue, Aug 30, 2022 at 8:37 PM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Hi, > > Am Montag, 29. August 2022, 11:22:16 CEST schrieb Chen-Yu Tsai: > > Hi, > > > > On Fri, Aug 26, 2022 at 8:28 PM Alexander Stein > > > > <alexander.stein@ew.tq-group.com> wrote: > > > Hi everybody, > > > > > > Am Montag, 22. August 2022, 10:14:23 CEST schrieb Chen-Yu Tsai: > > > > 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> > > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > --- > > > > > > > > drivers/clk/clk.c | 28 ++++++++++++++++++++++++++++ > > > > 1 file changed, 28 insertions(+) > > > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > > index 7fc191c15507..9b365cd6d14b 100644 > > > > --- a/drivers/clk/clk.c > > > > +++ b/drivers/clk/clk.c > > > > @@ -196,6 +196,9 @@ static bool clk_core_rate_is_protected(struct > > > > clk_core > > > > *core) return core->protect_count; > > > > > > > > } > > > > > > > > +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_prepared(struct clk_core *core) > > > > { > > > > > > > > bool ret = false; > > > > > > > > @@ -208,7 +211,11 @@ static bool clk_core_is_prepared(struct clk_core > > > > *core) return core->prepare_count; > > > > > > > > if (!clk_pm_runtime_get(core)) { > > > > > > > > + if (core->flags & CLK_OPS_PARENT_ENABLE) > > > > + clk_core_prepare_enable(core->parent); > > > > > > > > ret = core->ops->is_prepared(core->hw); > > > > > > > > + if (core->flags & CLK_OPS_PARENT_ENABLE) > > > > + clk_core_disable_unprepare(core->parent); > > > > > > > > clk_pm_runtime_put(core); > > > > > > > > } > > > > > > > > @@ -244,7 +251,13 @@ static bool clk_core_is_enabled(struct clk_core > > > > *core) > > > > > > > > } > > > > > > > > } > > > > > > > > + if (core->flags & CLK_OPS_PARENT_ENABLE) > > > > + clk_core_prepare_enable(core->parent); > > > > + > > > > > > > > ret = core->ops->is_enabled(core->hw); > > > > > > > > + > > > > + if (core->flags & CLK_OPS_PARENT_ENABLE) > > > > + clk_core_disable_unprepare(core->parent); > > > > > > > > done: > > > > if (core->rpm_enabled) > > > > > > > > pm_runtime_put(core->dev); > > > > > > > > @@ -812,6 +825,9 @@ int clk_rate_exclusive_get(struct clk *clk) > > > > > > > > } > > > > EXPORT_SYMBOL_GPL(clk_rate_exclusive_get); > > > > > > > > +static int clk_core_enable_lock(struct clk_core *core); > > > > +static void clk_core_disable_lock(struct clk_core *core); > > > > + > > > > > > > > static void clk_core_unprepare(struct clk_core *core) > > > > { > > > > > > > > lockdep_assert_held(&prepare_lock); > > > > > > > > @@ -835,6 +851,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_lock(core->parent); > > > > + > > > > > > > > trace_clk_unprepare(core); > > > > > > > > if (core->ops->unprepare) > > > > > > > > @@ -843,6 +862,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_lock(core->parent); > > > > > > > > clk_core_unprepare(core->parent); > > > > > > > > } > > > > > > > > @@ -891,6 +913,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_lock(core->parent); > > > > + > > > > > > > > trace_clk_prepare(core); > > > > > > > > if (core->ops->prepare) > > > > > > > > @@ -898,6 +923,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_lock(core->parent); > > > > + > > > > > > > > if (ret) > > > > > > > > goto unprepare; > > > > > > > > } > > > > > > Unfortunately this completely locks up my i.MX8M Plus based board during > > > early boot. > > > I'm currently running on next-20220826 using > > > arch/arm64/boot/dts/freescale/ > > > imx8mp-tqma8mpql-mba8mpxl.dts > > > Reverting this patch gets my board booting again. dmesg until hard lockup > > > below. > > > > The standard logs don't have anything to go on. Could you add some printk > > calls to the clk core around the areas this patch touchs? That would help. > > > > Could you also provide a dump of /sys/kernel/debug/clk/clk_summary? That > > would help to understand the clock tree. > > Sure, > > These are the last kernel log lines before hard lockup: > [ 0.686357] io scheduler mq-deadline registered > [ 0.690907] io scheduler kyber registered > [ 0.699275] clk_core_prepare: clk: main_axi CLK_OPS_PARENT_ENABLE > > main_axi is also the only debug output up to this point. This is the used > patch for debugging: > ---8<--- > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -211,8 +211,10 @@ static bool clk_core_is_prepared(struct clk_core *core) > return core->prepare_count; > > if (!clk_pm_runtime_get(core)) { > - if (core->flags & CLK_OPS_PARENT_ENABLE) > + if (core->flags & CLK_OPS_PARENT_ENABLE) { > + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n", > __func__, core->name); > clk_core_prepare_enable(core->parent); > + } > ret = core->ops->is_prepared(core->hw); > if (core->flags & CLK_OPS_PARENT_ENABLE) > clk_core_disable_unprepare(core->parent); > @@ -251,8 +253,10 @@ static bool clk_core_is_enabled(struct clk_core *core) > } > } > > - if (core->flags & CLK_OPS_PARENT_ENABLE) > + if (core->flags & CLK_OPS_PARENT_ENABLE) { > + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n", __func__, core- > >name); > clk_core_prepare_enable(core->parent); > + } > > ret = core->ops->is_enabled(core->hw); > > @@ -851,8 +855,10 @@ 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) > + if (core->flags & CLK_OPS_PARENT_ENABLE) { > + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n", __func__, core- > >name); > clk_core_enable_lock(core->parent); > + } > > trace_clk_unprepare(core); > > @@ -912,8 +918,10 @@ static int clk_core_prepare(struct clk_core *core) > if (ret) > goto runtime_put; > > - if (core->flags & CLK_OPS_PARENT_ENABLE) > + if (core->flags & CLK_OPS_PARENT_ENABLE) { > + pr_info("%s: clk: %s CLK_OPS_PARENT_ENABLE\n", > __func__, core->name); > clk_core_enable_lock(core->parent); > + } > > trace_clk_prepare(core); > > > ---8<---
Thanks. So the part of the clock tree that's problematic is this:
osc_24m (fixed) sys_pll1_ref_sel (mux) sys_pll1 (imx pll14xx) sys_pll1_bypass (mux) sys_pll1_out (gate) sys_pll1_800m (fixed factor) main_axi (composite CLK_IS_CRITICAL)
Would it be possible for you to produce a stack dump as well? And also enable lock debugging? This likely still won't catch anything if it's a spinlock deadlock though.
Thanks ChenYu
| |