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

[Chen-Yu Tsai]

On Wed, Dec 27, 2023 at 6:05 PM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote:
>
> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate this
> clock needs a runtime PM get during the probing stage.

Actually it means (based on our discussions and your code here) that
runtime PM should be enabled for the clock controller. If runtime PM
is not enabled before the clocks are registered, the CCF subsequently
never toggles runtime PM.

The runtime PM get during the probe stage is to avoid triggering runtime
suspend/resume during each clock registration, and hopefully avoid a
deadlock. It should be mentioned separately. A comment should be added
so that folks going over the code in the future don't remove it.

> Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxxxx>
> ---
>
>  drivers/clk/mediatek/clk-mtk.c | 15 +++++++++++++++
>  drivers/clk/mediatek/clk-mtk.h |  2 ++
>  2 files changed, 17 insertions(+)
>
> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> index 2e55368dc4d8..c31e535909c8 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,14 @@ 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);
> +               r = pm_runtime_resume_and_get(&pdev->dev);

A comment for the resume and get should be added. Otherwise someone looking
at this and the CCF could think that this isn't needed, since the CCF already
has similar calls.

> +               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 +583,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 +616,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);

Please keep the error path calls strictly in reverse order of the setup
calls. So this should go before iounmap().

ChenYu

>         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
>