Re: [PATCH v3 1/2] clk: mediatek: Introduce need_pm_runtime to mtk_clk_desc

[Chen-Yu Tsai <wenst@chromium.org>]

On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
> > On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote:
> >>
> >> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
> >> this clock controller needs runtime PM for its operations.
> >> Also do a runtime PM get on the clock controller during the
> >> probing stage to workaround a possible deadlock.
> >>
> >> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> >
> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> >
> > The patch itself looks fine.
> >
> > Besides the MT8183 MFG clock issues, we do actually need this for the
> > MT8192 ADSP clock. Its power domain is not enabled by default.
> >
>
> ...but on MT8195 the ADSP clock works - because the ADSP node exists.

That's an indirect dependency that should not be relied on. Say the clock
driver probed but the ADSP hasn't, and you try to read out the current
status. What would happen?

- Read out works fine, because the power domain is default on, and hasn't
  been turned off by late cleanup
- Read out is bogus (but you can't tell)
- Read out hangs.

The third is what happens on MT8192. There's still some issues on that
front, as even after I applied the ADSP power domain patches from MediaTek,
the readout was still hanging.

> This poses a question: should we make clock controllers depend on power domains,
> or should we keep everything powered off (hence clocks down - no power consumption)
> *unless* the user exists?

That's a policy discussion separate from actual hardware dependencies.
*If* the clock controller needs the power domain to be active for the
registers to be accessed, the clock controller *must* have a direct
dependency on the power domain.

> For the second one, this means that the *device* gets the power domain (adsp), and
> not the clock controller (which clocks are effectively useless if there's no user).

No. See my previous paragraph.

ChenYu

> Angelo
>
> >> ---
> >>
> >> Changes in v3:
> >> - Update the commit message and the comments before runtime PM call
> >>
> >> Changes in v2:
> >> - Fix the order of error handling
> >> - Update the commit message and add a comment before the runtime PM call
> >>
> >>   drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
> >>   drivers/clk/mediatek/clk-mtk.h |  2 ++
> >>   2 files changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> >> index 2e55368dc4d8..ba1d1c495bc2 100644
> >> --- a/drivers/clk/mediatek/clk-mtk.c
> >> +++ b/drivers/clk/mediatek/clk-mtk.c
> >> @@ -13,6 +13,7 @@
> >>   #include <linux/of.h>
> >>   #include <linux/of_address.h>
> >>   #include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >>   #include <linux/slab.h>
> >>
> >>   #include "clk-mtk.h"
> >> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>                          return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
> >>          }
> >>
> >> +
> >> +       if (mcd->need_runtime_pm) {
> >> +               devm_pm_runtime_enable(&pdev->dev);
> >> +               /*
> >> +                * Do a pm_runtime_resume_and_get() to workaround a possible
> >> +                * deadlock between clk_register() and the genpd framework.
> >> +                */
> >> +               r = pm_runtime_resume_and_get(&pdev->dev);
> >> +               if (r)
> >> +                       return r;
> >> +       }
> >> +
> >>          /* Calculate how many clk_hw_onecell_data entries to allocate */
> >>          num_clks = mcd->num_clks + mcd->num_composite_clks;
> >>          num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
> >> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>                          goto unregister_clks;
> >>          }
> >>
> >> +       if (mcd->need_runtime_pm)
> >> +               pm_runtime_put(&pdev->dev);
> >> +
> >>          return r;
> >>
> >>   unregister_clks:
> >> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>   free_base:
> >>          if (mcd->shared_io && base)
> >>                  iounmap(base);
> >> +
> >> +       if (mcd->need_runtime_pm)
> >> +               pm_runtime_put(&pdev->dev);
> >>          return r;
> >>   }
> >>
> >> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> >> index 22096501a60a..c17fe1c2d732 100644
> >> --- a/drivers/clk/mediatek/clk-mtk.h
> >> +++ b/drivers/clk/mediatek/clk-mtk.h
> >> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
> >>
> >>          int (*clk_notifier_func)(struct device *dev, struct clk *clk);
> >>          unsigned int mfg_clk_idx;
> >> +
> >> +       bool need_runtime_pm;
> >>   };
> >>
> >>   int mtk_clk_pdev_probe(struct platform_device *pdev);
> >> --
> >> 2.43.0.472.g3155946c3a-goog
> >>
>
>
>