Re: [PATCH RESEND v2 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for clk gate ops

Chen-Yu Tsai
2022-08-30T13:45:26Z

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