Message-ID: <CAGXv+5E788T01XJF-dYRW6ZB5-TTU_L5=0hT3AQ0g+zA=LzG2w@mail.gmail.com>
Date:   Mon, 29 Aug 2022 17:22:16 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Alexander Stein <alexander.stein@...tq-group.com>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Nícolas F . R . A . Prado 
        <nfraprado@...labora.com>, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v2 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for
 clk gate ops

Hi,

On Fri, Aug 26, 2022 at 8:28 PM Alexander Stein
<alexander.stein@...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@...omium.org>
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > Tested-by: Nícolas F. R. A. Prado <nfraprado@...labora.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.


Thanks
ChenYu

Powered by blists - more mailing lists