Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220822081424.1310926-1-wenst@chromium.org> <12115951.O9o76ZdvQC@steina-w>
 <CAGXv+5E788T01XJF-dYRW6ZB5-TTU_L5=0hT3AQ0g+zA=LzG2w@mail.gmail.com> <2254919.ElGaqSPkdT@steina-w>
In-Reply-To: <2254919.ElGaqSPkdT@steina-w>
From:   Chen-Yu Tsai <wenst@chromium.org>
Date:   Tue, 30 Aug 2022 21:40:53 +0800
Message-ID: <CAGXv+5Hf0n5jkqwuQmh0PG8ejxDND6BRVG47J9HTQrqz9OhLdQ@mail.gmail.com>
Subject: Re: [PATCH RESEND v2 1/2] clk: core: Honor CLK_OPS_PARENT_ENABLE for
 clk gate ops
To:     Alexander Stein <alexander.stein@ew.tq-group.com>
Cc:     Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        =?UTF-8?B?TsOtY29sYXMgRiAuIFIgLiBBIC4gUHJhZG8=?= 
        <nfraprado@collabora.com>, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 f=
or
> > > > this flag was only added to rate change operations (rate setting an=
d
> > > > reparent) and disabling unused subtree. It was not added to the
> > > > clock gate related operations. Any hardware driver that needs it fo=
r
> > > > 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 pare=
nts
> > > > enable (part 2)") Fixes: a4b3518d146f ("clk: core: support clocks w=
hich
> > > > requires parents enable (part 1)") Signed-off-by: Chen-Yu Tsai
> > > > <wenst@chromium.org>
> > > > Reviewed-by: N=C3=ADcolas F. R. A. Prado <nfraprado@collabora.com>
> > > > Tested-by: N=C3=ADcolas 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 =3D false;
> > > >
> > > > @@ -208,7 +211,11 @@ static bool clk_core_is_prepared(struct clk_co=
re
> > > > *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 =3D 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_cor=
e
> > > > *core)
> > > >
> > > >               }
> > > >
> > > >       }
> > > >
> > > > +     if (core->flags & CLK_OPS_PARENT_ENABLE)
> > > > +             clk_core_prepare_enable(core->parent);
> > > > +
> > > >
> > > >       ret =3D 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 *co=
re)
> > > >
> > > >               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 *co=
re)
> > > >
> > > >               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 dur=
ing
> > > 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 lo=
ckup
> > > below.
> >
> > The standard logs don't have anything to go on. Could you add some prin=
tk
> > calls to the clk core around the areas this patch touchs? That would he=
lp.
> >
> > Could you also provide a dump of /sys/kernel/debug/clk/clk_summary? Tha=
t
> > 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 *co=
re)
>                 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 =3D 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 *cor=
e)
>                 }
>         }
>
> -       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 =3D 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->na=
me);
>
> -       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