diff mbox series

[v3,2/2] clk: mediatek: mt8183: Enable need_runtime_pm on mt8183-mfgcfg

Message ID 20240108081834.408403-2-treapking@chromium.org (mailing list archive)
State Superseded, archived
Headers show
Series [v3,1/2] clk: mediatek: Introduce need_pm_runtime to mtk_clk_desc | expand

Commit Message

Pin-yen Lin Jan. 8, 2024, 8:18 a.m. UTC
mt8183-mfgcfg has a mutual dependency with genpd during the probing
stage, so enable need_runtim_pm to prevent a deadlock in the following
call stack:

CPU0:  genpd_lock --> clk_prepare_lock
genpd_power_off_work_fn()
 genpd_lock()
 generic_pm_domain::power_off()
    clk_unprepare()
      clk_prepare_lock()

CPU1: clk_prepare_lock --> genpd_lock
clk_register()
  __clk_core_init()
    clk_prepare_lock()
    clk_pm_runtime_get()
      genpd_lock()

Do a runtime PM get at the probe function to make sure clk_register()
won't acquire the genpd lock.

Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

(no changes since v1)

 drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Chen-Yu Tsai Feb. 23, 2024, 4:43 a.m. UTC | #1
On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> mt8183-mfgcfg has a mutual dependency with genpd during the probing
> stage, so enable need_runtim_pm to prevent a deadlock in the following
> call stack:
>
> CPU0:  genpd_lock --> clk_prepare_lock
> genpd_power_off_work_fn()
>  genpd_lock()
>  generic_pm_domain::power_off()
>     clk_unprepare()
>       clk_prepare_lock()
>
> CPU1: clk_prepare_lock --> genpd_lock
> clk_register()
>   __clk_core_init()
>     clk_prepare_lock()
>     clk_pm_runtime_get()
>       genpd_lock()
>
> Do a runtime PM get at the probe function to make sure clk_register()
> won't acquire the genpd lock.
>
> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Note that this compliments a patch [1] adding the power domain for the mfgcfg
clock controller node, which has been floating around for almost 3 years.

[1] https://lore.kernel.org/linux-mediatek/20210414073108.3899082-1-ikjn@chromium.org/

> ---
>
> (no changes since v1)
>
>  drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
> index ba504e19d420..62d876e150e1 100644
> --- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
> +++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
> @@ -29,6 +29,7 @@ static const struct mtk_gate mfg_clks[] = {
>  static const struct mtk_clk_desc mfg_desc = {
>         .clks = mfg_clks,
>         .num_clks = ARRAY_SIZE(mfg_clks),
> +       .need_runtime_pm = true,
>  };
>
>  static const struct of_device_id of_match_clk_mt8183_mfg[] = {
> --
> 2.43.0.472.g3155946c3a-goog
>
AngeloGioacchino Del Regno Feb. 26, 2024, 11:35 a.m. UTC | #2
Il 23/02/24 05:43, Chen-Yu Tsai ha scritto:
> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote:
>>
>> mt8183-mfgcfg has a mutual dependency with genpd during the probing
>> stage, so enable need_runtim_pm to prevent a deadlock in the following
>> call stack:
>>
>> CPU0:  genpd_lock --> clk_prepare_lock
>> genpd_power_off_work_fn()
>>   genpd_lock()
>>   generic_pm_domain::power_off()
>>      clk_unprepare()
>>        clk_prepare_lock()
>>
>> CPU1: clk_prepare_lock --> genpd_lock
>> clk_register()
>>    __clk_core_init()
>>      clk_prepare_lock()
>>      clk_pm_runtime_get()
>>        genpd_lock()
>>
>> Do a runtime PM get at the probe function to make sure clk_register()
>> won't acquire the genpd lock.
>>
>> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> 
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> 
> Note that this compliments a patch [1] adding the power domain for the mfgcfg
> clock controller node, which has been floating around for almost 3 years.
> 

...but why does this happen *only* on MT8183 and not on any other MediaTek SoC?

I understand what you're trying to solve here, but if we explore a bit more, we
can maybe come to the conclusion that we don't need to add this flag, and perhaps
just enable PM runtime as regular flow for all clock controllers.

That would also be cleaner, to some extent.

Cheers,
Angelo

> [1] https://lore.kernel.org/linux-mediatek/20210414073108.3899082-1-ikjn@chromium.org/
> 
>> ---
>>
>> (no changes since v1)
>>
>>   drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
>> index ba504e19d420..62d876e150e1 100644
>> --- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
>> +++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
>> @@ -29,6 +29,7 @@ static const struct mtk_gate mfg_clks[] = {
>>   static const struct mtk_clk_desc mfg_desc = {
>>          .clks = mfg_clks,
>>          .num_clks = ARRAY_SIZE(mfg_clks),
>> +       .need_runtime_pm = true,
>>   };
>>
>>   static const struct of_device_id of_match_clk_mt8183_mfg[] = {
>> --
>> 2.43.0.472.g3155946c3a-goog
>>
diff mbox series

Patch

diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
index ba504e19d420..62d876e150e1 100644
--- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
+++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
@@ -29,6 +29,7 @@  static const struct mtk_gate mfg_clks[] = {
 static const struct mtk_clk_desc mfg_desc = {
 	.clks = mfg_clks,
 	.num_clks = ARRAY_SIZE(mfg_clks),
+	.need_runtime_pm = true,
 };
 
 static const struct of_device_id of_match_clk_mt8183_mfg[] = {