* [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock`
@ 2025-12-31 3:53 Tzung-Bi Shih
2026-01-22 8:37 ` Chen-Yu Tsai
0 siblings, 1 reply; 5+ messages in thread
From: Tzung-Bi Shih @ 2025-12-31 3:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Matthias Brugger, AngeloGioacchino Del Regno, linux-pm,
linux-mediatek, tzungbi
Break a circular locking dependency between the Generic Power Domain
lock (`genpd->mlock`) and the clock framework's `prepare_lock`. Move
the prepare() to the domain initialization phase and the unprepare()
to the cleanup phase.
The possible deadlock occurs in the following scenario:
1. `genpd_power_on` acquires `genpd->mlock` and then calls the driver's
`scpsys_power_on`. The driver calls `clk_bulk_prepare_enable`,
which attempts to acquire `prepare_lock`.
> -> #0 (prepare_lock){+.+.}-{3:3}:
> __lock_acquire
> lock_acquire
> __mutex_lock_common
> mutex_lock_nested
> clk_prepare
> clk_bulk_prepare
> scpsys_power_on
> genpd_power_on
2. A clock provider (managed by a power domain) is resumed.
`clk_prepare` acquires `prepare_lock` and triggers a runtime resume of
its power domain, which attempts to acquire `genpd->mlock`.
> -> #1 (&genpd->mlock){+.+.}-{3:3}:
> __mutex_lock_common
> mutex_lock_nested
> genpd_lock_mtx
> genpd_runtime_resume
> __rpm_callback
> rpm_callback
> rpm_resume
> __pm_runtime_resume
> clk_core_prepare
> clk_prepare
> clk_bulk_prepare
This creates a cycle: `mlock` -> `prepare_lock` -> `mlock`.
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&genpd->mlock);
> lock(prepare_lock);
> lock(&genpd->mlock);
> lock(prepare_lock);
This breaks the dependency chain in #0.
This is a revert of f0fce06e345d ("soc: mtk-pm-domains: Fix the clock
prepared issue"). However, addressing the issue by moving the
unprepare()/prepare() to PM suspend()/resume() callbacks.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- Fix build error reported by "kernel test robot <lkp@intel.com>".
v1: https://lore.kernel.org/all/20251229043244.4103262-1-tzungbi@kernel.org/
drivers/pmdomain/mediatek/mtk-pm-domains.c | 101 +++++++++++++++++----
1 file changed, 81 insertions(+), 20 deletions(-)
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index 80561d27f2b2..c371b08c9170 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -318,12 +318,12 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
if (ret)
goto err_infra;
- ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
+ ret = clk_bulk_enable(pd->num_clks, pd->clks);
if (ret)
goto err_reg;
/* For HWV the subsys clocks refer to the HWV low power subsystem */
- ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
+ ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
if (ret)
goto err_disable_clks;
@@ -365,7 +365,7 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
}
/* It's done! Disable the HWV low power subsystem clocks */
- clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
+ clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
if (MTK_SCPD_CAPS(pd, MTK_SCPD_INFRA_PWR_CTL))
scpsys_sec_infra_power_on(false);
@@ -373,9 +373,9 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
return 0;
err_disable_subsys_clks:
- clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
+ clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
err_disable_clks:
- clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
+ clk_bulk_disable(pd->num_clks, pd->clks);
err_reg:
scpsys_regulator_disable(pd->supply);
err_infra:
@@ -398,7 +398,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
return ret;
}
- ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
+ ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
if (ret)
goto err_infra;
@@ -437,8 +437,8 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
if (ret)
goto err_disable_subsys_clks;
- clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
- clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
+ clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
+ clk_bulk_disable(pd->num_clks, pd->clks);
scpsys_regulator_disable(pd->supply);
@@ -448,7 +448,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
return 0;
err_disable_subsys_clks:
- clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
+ clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
err_infra:
if (MTK_SCPD_CAPS(pd, MTK_SCPD_INFRA_PWR_CTL))
scpsys_sec_infra_power_on(false);
@@ -616,7 +616,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
if (ret)
return ret;
- ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
+ ret = clk_bulk_enable(pd->num_clks, pd->clks);
if (ret)
goto err_reg;
@@ -638,8 +638,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
* access.
*/
if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
- ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
- pd->subsys_clks);
+ ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
if (ret)
goto err_pwr_ack;
}
@@ -653,8 +652,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
goto err_disable_sram;
if (MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
- ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
- pd->subsys_clks);
+ ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
if (ret)
goto err_enable_bus_protect;
}
@@ -667,10 +665,9 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
scpsys_sram_disable(pd);
err_disable_subsys_clks:
if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
- clk_bulk_disable_unprepare(pd->num_subsys_clks,
- pd->subsys_clks);
+ clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
err_pwr_ack:
- clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
+ clk_bulk_disable(pd->num_clks, pd->clks);
err_reg:
scpsys_regulator_disable(pd->supply);
return ret;
@@ -695,7 +692,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
pd->data->ext_buck_iso_mask);
- clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
+ clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
scpsys_modem_pwrseq_off(pd);
@@ -708,7 +705,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
if (ret < 0)
return ret;
- clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
+ clk_bulk_disable(pd->num_clks, pd->clks);
scpsys_regulator_disable(pd->supply);
@@ -855,6 +852,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
}
+ ret = clk_bulk_prepare(pd->num_clks, pd->clks);
+ if (ret)
+ goto err_put_subsys_clocks;
+
+ ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
+ if (ret)
+ goto err_unprepare_clocks;
+
/*
* Initially turn on all domains to make the domains usable
* with !CONFIG_PM and to get the hardware in sync with the
@@ -869,7 +874,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
ret = pd->genpd.power_on(&pd->genpd);
if (ret < 0) {
dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
- goto err_put_subsys_clocks;
+ goto err_unprepare_subsys_clocks;
}
if (MTK_SCPD_CAPS(pd, MTK_SCPD_ALWAYS_ON))
@@ -888,6 +893,10 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
return scpsys->pd_data.domains[id];
+err_unprepare_subsys_clocks:
+ clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
+err_unprepare_clocks:
+ clk_bulk_unprepare(pd->num_clks, pd->clks);
err_put_subsys_clocks:
clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
err_put_clocks:
@@ -965,6 +974,8 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
if (scpsys_domain_is_on(pd))
scpsys_power_off(&pd->genpd);
+ clk_bulk_unprepare(pd->num_clks, pd->clks);
+ clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
clk_bulk_put(pd->num_clks, pd->clks);
clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
}
@@ -1208,6 +1219,7 @@ static int scpsys_probe(struct platform_device *pdev)
if (!scpsys)
return -ENOMEM;
+ platform_set_drvdata(pdev, scpsys);
scpsys->dev = dev;
scpsys->soc_data = soc;
@@ -1270,12 +1282,61 @@ static int scpsys_probe(struct platform_device *pdev)
return ret;
}
+#ifdef CONFIG_PM_SLEEP
+static int scpsys_suspend(struct device *dev)
+{
+ struct scpsys *scpsys = dev_get_drvdata(dev);
+ struct generic_pm_domain *genpd;
+ struct scpsys_domain *pd;
+ int i;
+
+ for (i = 0; i < scpsys->pd_data.num_domains; i++) {
+ genpd = scpsys->pd_data.domains[i];
+ if (!genpd)
+ continue;
+
+ pd = to_scpsys_domain(genpd);
+ clk_bulk_unprepare(pd->num_clks, pd->clks);
+ clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
+ }
+ return 0;
+}
+
+static int scpsys_resume(struct device *dev)
+{
+ struct scpsys *scpsys = dev_get_drvdata(dev);
+ struct generic_pm_domain *genpd;
+ struct scpsys_domain *pd;
+ int i, ret;
+
+ for (i = 0; i < scpsys->pd_data.num_domains; i++) {
+ genpd = scpsys->pd_data.domains[i];
+ if (!genpd)
+ continue;
+
+ pd = to_scpsys_domain(genpd);
+ ret = clk_bulk_prepare(pd->num_clks, pd->clks);
+ if (ret)
+ return ret;
+ ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops scpsys_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(scpsys_suspend, scpsys_resume)
+};
+
static struct platform_driver scpsys_pm_domain_driver = {
.probe = scpsys_probe,
.driver = {
.name = "mtk-power-controller",
.suppress_bind_attrs = true,
.of_match_table = scpsys_of_match,
+ .pm = &scpsys_pm_ops,
},
};
builtin_platform_driver(scpsys_pm_domain_driver);
--
2.52.0.351.gbe84eed79e-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock`
2025-12-31 3:53 [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock` Tzung-Bi Shih
@ 2026-01-22 8:37 ` Chen-Yu Tsai
2026-01-27 13:57 ` Ulf Hansson
0 siblings, 1 reply; 5+ messages in thread
From: Chen-Yu Tsai @ 2026-01-22 8:37 UTC (permalink / raw)
To: Tzung-Bi Shih, Ulf Hansson
Cc: Matthias Brugger, AngeloGioacchino Del Regno, linux-pm,
linux-mediatek
On Wed, Dec 31, 2025 at 11:54 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Break a circular locking dependency between the Generic Power Domain
> lock (`genpd->mlock`) and the clock framework's `prepare_lock`. Move
> the prepare() to the domain initialization phase and the unprepare()
> to the cleanup phase.
>
> The possible deadlock occurs in the following scenario:
>
> 1. `genpd_power_on` acquires `genpd->mlock` and then calls the driver's
> `scpsys_power_on`. The driver calls `clk_bulk_prepare_enable`,
> which attempts to acquire `prepare_lock`.
>
> > -> #0 (prepare_lock){+.+.}-{3:3}:
> > __lock_acquire
> > lock_acquire
> > __mutex_lock_common
> > mutex_lock_nested
> > clk_prepare
> > clk_bulk_prepare
> > scpsys_power_on
> > genpd_power_on
>
> 2. A clock provider (managed by a power domain) is resumed.
> `clk_prepare` acquires `prepare_lock` and triggers a runtime resume of
> its power domain, which attempts to acquire `genpd->mlock`.
>
> > -> #1 (&genpd->mlock){+.+.}-{3:3}:
> > __mutex_lock_common
> > mutex_lock_nested
> > genpd_lock_mtx
> > genpd_runtime_resume
> > __rpm_callback
> > rpm_callback
> > rpm_resume
> > __pm_runtime_resume
> > clk_core_prepare
> > clk_prepare
> > clk_bulk_prepare
>
> This creates a cycle: `mlock` -> `prepare_lock` -> `mlock`.
>
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(&genpd->mlock);
> > lock(prepare_lock);
> > lock(&genpd->mlock);
> > lock(prepare_lock);
>
> This breaks the dependency chain in #0.
>
> This is a revert of f0fce06e345d ("soc: mtk-pm-domains: Fix the clock
> prepared issue"). However, addressing the issue by moving the
> unprepare()/prepare() to PM suspend()/resume() callbacks.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
Tested-by: Chen-Yu Tsai <wenst@chromium.org> # MT8183 & MT8188 no regressions
This is unfortunately a known problem of the interaction between pmdomains
and the clk subsystem. This *breaks* the circular lock dependency. AFAIU
this works because the clk prepare op is a no-op, because all the clocks
are MMIO based.
There is another question of whether this actually deadlocks or not, i.e.
if the involved &genpd->mlock is the same object or not. Perhaps the
accuracy of lockdep could be improved by setting a different lock class
key and name? See what regmap does.
> ---
> v2:
> - Fix build error reported by "kernel test robot <lkp@intel.com>".
>
> v1: https://lore.kernel.org/all/20251229043244.4103262-1-tzungbi@kernel.org/
>
> drivers/pmdomain/mediatek/mtk-pm-domains.c | 101 +++++++++++++++++----
> 1 file changed, 81 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index 80561d27f2b2..c371b08c9170 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -318,12 +318,12 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
> if (ret)
> goto err_infra;
>
> - ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> + ret = clk_bulk_enable(pd->num_clks, pd->clks);
> if (ret)
> goto err_reg;
>
> /* For HWV the subsys clocks refer to the HWV low power subsystem */
> - ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> if (ret)
> goto err_disable_clks;
>
> @@ -365,7 +365,7 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
> }
>
> /* It's done! Disable the HWV low power subsystem clocks */
> - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
>
> if (MTK_SCPD_CAPS(pd, MTK_SCPD_INFRA_PWR_CTL))
> scpsys_sec_infra_power_on(false);
> @@ -373,9 +373,9 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
> return 0;
>
> err_disable_subsys_clks:
> - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> err_disable_clks:
> - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> + clk_bulk_disable(pd->num_clks, pd->clks);
> err_reg:
> scpsys_regulator_disable(pd->supply);
> err_infra:
> @@ -398,7 +398,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> return ret;
> }
>
> - ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> if (ret)
> goto err_infra;
>
> @@ -437,8 +437,8 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> if (ret)
> goto err_disable_subsys_clks;
>
> - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> + clk_bulk_disable(pd->num_clks, pd->clks);
>
> scpsys_regulator_disable(pd->supply);
>
> @@ -448,7 +448,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> return 0;
>
> err_disable_subsys_clks:
> - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> err_infra:
> if (MTK_SCPD_CAPS(pd, MTK_SCPD_INFRA_PWR_CTL))
> scpsys_sec_infra_power_on(false);
> @@ -616,7 +616,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> if (ret)
> return ret;
>
> - ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> + ret = clk_bulk_enable(pd->num_clks, pd->clks);
> if (ret)
> goto err_reg;
>
> @@ -638,8 +638,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> * access.
> */
> if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
> - ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
> - pd->subsys_clks);
> + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> if (ret)
> goto err_pwr_ack;
> }
> @@ -653,8 +652,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> goto err_disable_sram;
>
> if (MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
> - ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
> - pd->subsys_clks);
> + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> if (ret)
> goto err_enable_bus_protect;
> }
> @@ -667,10 +665,9 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> scpsys_sram_disable(pd);
> err_disable_subsys_clks:
> if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
> - clk_bulk_disable_unprepare(pd->num_subsys_clks,
> - pd->subsys_clks);
> + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> err_pwr_ack:
> - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> + clk_bulk_disable(pd->num_clks, pd->clks);
> err_reg:
> scpsys_regulator_disable(pd->supply);
> return ret;
> @@ -695,7 +692,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
> pd->data->ext_buck_iso_mask);
>
> - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
>
> if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
> scpsys_modem_pwrseq_off(pd);
> @@ -708,7 +705,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> if (ret < 0)
> return ret;
>
> - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> + clk_bulk_disable(pd->num_clks, pd->clks);
>
> scpsys_regulator_disable(pd->supply);
>
> @@ -855,6 +852,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
> }
>
> + ret = clk_bulk_prepare(pd->num_clks, pd->clks);
> + if (ret)
> + goto err_put_subsys_clocks;
> +
> + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
> + if (ret)
> + goto err_unprepare_clocks;
> +
> /*
> * Initially turn on all domains to make the domains usable
> * with !CONFIG_PM and to get the hardware in sync with the
> @@ -869,7 +874,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> ret = pd->genpd.power_on(&pd->genpd);
> if (ret < 0) {
> dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
> - goto err_put_subsys_clocks;
> + goto err_unprepare_subsys_clocks;
> }
>
> if (MTK_SCPD_CAPS(pd, MTK_SCPD_ALWAYS_ON))
> @@ -888,6 +893,10 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>
> return scpsys->pd_data.domains[id];
>
> +err_unprepare_subsys_clocks:
> + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> +err_unprepare_clocks:
> + clk_bulk_unprepare(pd->num_clks, pd->clks);
> err_put_subsys_clocks:
> clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
> err_put_clocks:
> @@ -965,6 +974,8 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
> if (scpsys_domain_is_on(pd))
> scpsys_power_off(&pd->genpd);
>
> + clk_bulk_unprepare(pd->num_clks, pd->clks);
> + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> clk_bulk_put(pd->num_clks, pd->clks);
> clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
> }
> @@ -1208,6 +1219,7 @@ static int scpsys_probe(struct platform_device *pdev)
> if (!scpsys)
> return -ENOMEM;
>
> + platform_set_drvdata(pdev, scpsys);
> scpsys->dev = dev;
> scpsys->soc_data = soc;
>
> @@ -1270,12 +1282,61 @@ static int scpsys_probe(struct platform_device *pdev)
> return ret;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int scpsys_suspend(struct device *dev)
> +{
> + struct scpsys *scpsys = dev_get_drvdata(dev);
> + struct generic_pm_domain *genpd;
> + struct scpsys_domain *pd;
> + int i;
> +
> + for (i = 0; i < scpsys->pd_data.num_domains; i++) {
> + genpd = scpsys->pd_data.domains[i];
> + if (!genpd)
> + continue;
> +
> + pd = to_scpsys_domain(genpd);
> + clk_bulk_unprepare(pd->num_clks, pd->clks);
> + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> + }
> + return 0;
> +}
> +
> +static int scpsys_resume(struct device *dev)
> +{
> + struct scpsys *scpsys = dev_get_drvdata(dev);
> + struct generic_pm_domain *genpd;
> + struct scpsys_domain *pd;
> + int i, ret;
> +
> + for (i = 0; i < scpsys->pd_data.num_domains; i++) {
> + genpd = scpsys->pd_data.domains[i];
> + if (!genpd)
> + continue;
> +
> + pd = to_scpsys_domain(genpd);
> + ret = clk_bulk_prepare(pd->num_clks, pd->clks);
> + if (ret)
> + return ret;
> + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops scpsys_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(scpsys_suspend, scpsys_resume)
> +};
> +
> static struct platform_driver scpsys_pm_domain_driver = {
> .probe = scpsys_probe,
> .driver = {
> .name = "mtk-power-controller",
> .suppress_bind_attrs = true,
> .of_match_table = scpsys_of_match,
> + .pm = &scpsys_pm_ops,
> },
> };
> builtin_platform_driver(scpsys_pm_domain_driver);
> --
> 2.52.0.351.gbe84eed79e-goog
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock`
2026-01-22 8:37 ` Chen-Yu Tsai
@ 2026-01-27 13:57 ` Ulf Hansson
2026-01-30 8:17 ` Tzung-Bi Shih
0 siblings, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2026-01-27 13:57 UTC (permalink / raw)
To: Tzung-Bi Shih, Chen-Yu Tsai
Cc: Matthias Brugger, AngeloGioacchino Del Regno, linux-pm,
linux-mediatek
On Thu, 22 Jan 2026 at 09:38, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Wed, Dec 31, 2025 at 11:54 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > Break a circular locking dependency between the Generic Power Domain
> > lock (`genpd->mlock`) and the clock framework's `prepare_lock`. Move
> > the prepare() to the domain initialization phase and the unprepare()
> > to the cleanup phase.
> >
> > The possible deadlock occurs in the following scenario:
> >
> > 1. `genpd_power_on` acquires `genpd->mlock` and then calls the driver's
> > `scpsys_power_on`. The driver calls `clk_bulk_prepare_enable`,
> > which attempts to acquire `prepare_lock`.
> >
> > > -> #0 (prepare_lock){+.+.}-{3:3}:
> > > __lock_acquire
> > > lock_acquire
> > > __mutex_lock_common
> > > mutex_lock_nested
> > > clk_prepare
> > > clk_bulk_prepare
> > > scpsys_power_on
> > > genpd_power_on
> >
> > 2. A clock provider (managed by a power domain) is resumed.
> > `clk_prepare` acquires `prepare_lock` and triggers a runtime resume of
> > its power domain, which attempts to acquire `genpd->mlock`.
> >
> > > -> #1 (&genpd->mlock){+.+.}-{3:3}:
> > > __mutex_lock_common
> > > mutex_lock_nested
> > > genpd_lock_mtx
> > > genpd_runtime_resume
> > > __rpm_callback
> > > rpm_callback
> > > rpm_resume
> > > __pm_runtime_resume
> > > clk_core_prepare
> > > clk_prepare
> > > clk_bulk_prepare
> >
> > This creates a cycle: `mlock` -> `prepare_lock` -> `mlock`.
> >
> > > Possible unsafe locking scenario:
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > lock(&genpd->mlock);
> > > lock(prepare_lock);
> > > lock(&genpd->mlock);
> > > lock(prepare_lock);
> >
> > This breaks the dependency chain in #0.
> >
> > This is a revert of f0fce06e345d ("soc: mtk-pm-domains: Fix the clock
> > prepared issue"). However, addressing the issue by moving the
> > unprepare()/prepare() to PM suspend()/resume() callbacks.
> >
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
>
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org> # MT8183 & MT8188 no regressions
>
> This is unfortunately a known problem of the interaction between pmdomains
> and the clk subsystem. This *breaks* the circular lock dependency. AFAIU
> this works because the clk prepare op is a no-op, because all the clocks
> are MMIO based.
Yes, this is a known problem that we need to fix properly for the
clock/genpd subsystems.
As an intermediate step, we could consider platform specific patches
to fix the problem too, along with $subject patch. However, $subject
patch has issues too, see more comments below.
>
> There is another question of whether this actually deadlocks or not, i.e.
> if the involved &genpd->mlock is the same object or not. Perhaps the
> accuracy of lockdep could be improved by setting a different lock class
> key and name? See what regmap does.
>
>
> > ---
> > v2:
> > - Fix build error reported by "kernel test robot <lkp@intel.com>".
> >
> > v1: https://lore.kernel.org/all/20251229043244.4103262-1-tzungbi@kernel.org/
> >
> > drivers/pmdomain/mediatek/mtk-pm-domains.c | 101 +++++++++++++++++----
> > 1 file changed, 81 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > index 80561d27f2b2..c371b08c9170 100644
> > --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> > @@ -318,12 +318,12 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
> > if (ret)
> > goto err_infra;
> >
> > - ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> > + ret = clk_bulk_enable(pd->num_clks, pd->clks);
> > if (ret)
> > goto err_reg;
> >
> > /* For HWV the subsys clocks refer to the HWV low power subsystem */
> > - ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> > if (ret)
> > goto err_disable_clks;
> >
> > @@ -365,7 +365,7 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
> > }
> >
> > /* It's done! Disable the HWV low power subsystem clocks */
> > - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> >
> > if (MTK_SCPD_CAPS(pd, MTK_SCPD_INFRA_PWR_CTL))
> > scpsys_sec_infra_power_on(false);
> > @@ -373,9 +373,9 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
> > return 0;
> >
> > err_disable_subsys_clks:
> > - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> > err_disable_clks:
> > - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> > + clk_bulk_disable(pd->num_clks, pd->clks);
> > err_reg:
> > scpsys_regulator_disable(pd->supply);
> > err_infra:
> > @@ -398,7 +398,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> > return ret;
> > }
> >
> > - ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
scpsys_hwv_power_off() is typically called by genpd in the suspend
noirq() phase, when all devices attached to the genpd in question have
been suspended too. See genpd_suspend_noirq().
This means that scpsys_suspend() (below) may be called to unprepare
the clock, before scpsys_hwv_power_off() may call clk_disable(). This
is a bug according to the clock framework.
Moving scpsys_suspend() to the noirq phase too could maybe work.
Although, perhaps an even simpler solution would be to do the
clk_prepare() during ->probe() and clk_unprepare() during ->remove()
(and error path in probe). Of course, this assumes that
clk_prepare/unprepare doesn't really do anything hardware wise, so we
don't start wasting power by keeping the clocks prepared.
> > if (ret)
> > goto err_infra;
> >
> > @@ -437,8 +437,8 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> > if (ret)
> > goto err_disable_subsys_clks;
> >
> > - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> > + clk_bulk_disable(pd->num_clks, pd->clks);
> >
> > scpsys_regulator_disable(pd->supply);
> >
> > @@ -448,7 +448,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> > return 0;
> >
> > err_disable_subsys_clks:
> > - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> > err_infra:
> > if (MTK_SCPD_CAPS(pd, MTK_SCPD_INFRA_PWR_CTL))
> > scpsys_sec_infra_power_on(false);
> > @@ -616,7 +616,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > if (ret)
> > return ret;
> >
> > - ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> > + ret = clk_bulk_enable(pd->num_clks, pd->clks);
> > if (ret)
> > goto err_reg;
> >
> > @@ -638,8 +638,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > * access.
> > */
> > if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
> > - ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
> > - pd->subsys_clks);
> > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> > if (ret)
> > goto err_pwr_ack;
> > }
> > @@ -653,8 +652,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > goto err_disable_sram;
> >
> > if (MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
> > - ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
> > - pd->subsys_clks);
> > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> > if (ret)
> > goto err_enable_bus_protect;
> > }
> > @@ -667,10 +665,9 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > scpsys_sram_disable(pd);
> > err_disable_subsys_clks:
> > if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
> > - clk_bulk_disable_unprepare(pd->num_subsys_clks,
> > - pd->subsys_clks);
> > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> > err_pwr_ack:
> > - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> > + clk_bulk_disable(pd->num_clks, pd->clks);
> > err_reg:
> > scpsys_regulator_disable(pd->supply);
> > return ret;
> > @@ -695,7 +692,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> > regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
> > pd->data->ext_buck_iso_mask);
> >
> > - clk_bulk_disable_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks);
> >
> > if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
> > scpsys_modem_pwrseq_off(pd);
> > @@ -708,7 +705,7 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> > if (ret < 0)
> > return ret;
> >
> > - clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> > + clk_bulk_disable(pd->num_clks, pd->clks);
> >
> > scpsys_regulator_disable(pd->supply);
> >
> > @@ -855,6 +852,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> > pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
> > }
> >
> > + ret = clk_bulk_prepare(pd->num_clks, pd->clks);
> > + if (ret)
> > + goto err_put_subsys_clocks;
> > +
> > + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
> > + if (ret)
> > + goto err_unprepare_clocks;
> > +
> > /*
> > * Initially turn on all domains to make the domains usable
> > * with !CONFIG_PM and to get the hardware in sync with the
> > @@ -869,7 +874,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> > ret = pd->genpd.power_on(&pd->genpd);
> > if (ret < 0) {
> > dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
> > - goto err_put_subsys_clocks;
> > + goto err_unprepare_subsys_clocks;
> > }
> >
> > if (MTK_SCPD_CAPS(pd, MTK_SCPD_ALWAYS_ON))
> > @@ -888,6 +893,10 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> >
> > return scpsys->pd_data.domains[id];
> >
> > +err_unprepare_subsys_clocks:
> > + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > +err_unprepare_clocks:
> > + clk_bulk_unprepare(pd->num_clks, pd->clks);
> > err_put_subsys_clocks:
> > clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
> > err_put_clocks:
> > @@ -965,6 +974,8 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
> > if (scpsys_domain_is_on(pd))
> > scpsys_power_off(&pd->genpd);
> >
> > + clk_bulk_unprepare(pd->num_clks, pd->clks);
> > + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > clk_bulk_put(pd->num_clks, pd->clks);
> > clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
> > }
> > @@ -1208,6 +1219,7 @@ static int scpsys_probe(struct platform_device *pdev)
> > if (!scpsys)
> > return -ENOMEM;
> >
> > + platform_set_drvdata(pdev, scpsys);
> > scpsys->dev = dev;
> > scpsys->soc_data = soc;
> >
> > @@ -1270,12 +1282,61 @@ static int scpsys_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int scpsys_suspend(struct device *dev)
> > +{
> > + struct scpsys *scpsys = dev_get_drvdata(dev);
> > + struct generic_pm_domain *genpd;
> > + struct scpsys_domain *pd;
> > + int i;
> > +
> > + for (i = 0; i < scpsys->pd_data.num_domains; i++) {
> > + genpd = scpsys->pd_data.domains[i];
> > + if (!genpd)
> > + continue;
> > +
> > + pd = to_scpsys_domain(genpd);
> > + clk_bulk_unprepare(pd->num_clks, pd->clks);
> > + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
> > + }
> > + return 0;
> > +}
> > +
> > +static int scpsys_resume(struct device *dev)
> > +{
> > + struct scpsys *scpsys = dev_get_drvdata(dev);
> > + struct generic_pm_domain *genpd;
> > + struct scpsys_domain *pd;
> > + int i, ret;
> > +
> > + for (i = 0; i < scpsys->pd_data.num_domains; i++) {
> > + genpd = scpsys->pd_data.domains[i];
> > + if (!genpd)
> > + continue;
> > +
> > + pd = to_scpsys_domain(genpd);
> > + ret = clk_bulk_prepare(pd->num_clks, pd->clks);
> > + if (ret)
> > + return ret;
> > + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops scpsys_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(scpsys_suspend, scpsys_resume)
> > +};
> > +
> > static struct platform_driver scpsys_pm_domain_driver = {
> > .probe = scpsys_probe,
> > .driver = {
> > .name = "mtk-power-controller",
> > .suppress_bind_attrs = true,
> > .of_match_table = scpsys_of_match,
> > + .pm = &scpsys_pm_ops,
> > },
> > };
> > builtin_platform_driver(scpsys_pm_domain_driver);
> > --
> > 2.52.0.351.gbe84eed79e-goog
> >
> >
Kind regards
Uffe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock`
2026-01-27 13:57 ` Ulf Hansson
@ 2026-01-30 8:17 ` Tzung-Bi Shih
2026-02-05 5:36 ` Tzung-Bi Shih
0 siblings, 1 reply; 5+ messages in thread
From: Tzung-Bi Shih @ 2026-01-30 8:17 UTC (permalink / raw)
To: Ulf Hansson
Cc: Chen-Yu Tsai, Matthias Brugger, AngeloGioacchino Del Regno,
linux-pm, linux-mediatek
On Tue, Jan 27, 2026 at 02:57:46PM +0100, Ulf Hansson wrote:
> On Thu, 22 Jan 2026 at 09:38, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > On Wed, Dec 31, 2025 at 11:54 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > @@ -398,7 +398,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> > > return ret;
> > > }
> > >
> > > - ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> > > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
>
> scpsys_hwv_power_off() is typically called by genpd in the suspend
> noirq() phase, when all devices attached to the genpd in question have
> been suspended too. See genpd_suspend_noirq().
>
> This means that scpsys_suspend() (below) may be called to unprepare
> the clock, before scpsys_hwv_power_off() may call clk_disable(). This
> is a bug according to the clock framework.
Ack, I observed a similar warning in a system suspend/resume cycle.
>
> Moving scpsys_suspend() to the noirq phase too could maybe work.
No, I guess it doesn't work as .prepare() can sleep however noirq phase
shouldn't/can't.
> Although, perhaps an even simpler solution would be to do the
> clk_prepare() during ->probe() and clk_unprepare() during ->remove()
> (and error path in probe). Of course, this assumes that
> clk_prepare/unprepare doesn't really do anything hardware wise, so we
> don't start wasting power by keeping the clocks prepared.
It turns out a pure revert of f0fce06e345d ("soc: mtk-pm-domains: Fix
the clock prepared issue"). Per the commit message of f0fce06e345d,
the concern is some PLLs keep prepared even if the system is suspended.
Not sure should we take the compromise?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock`
2026-01-30 8:17 ` Tzung-Bi Shih
@ 2026-02-05 5:36 ` Tzung-Bi Shih
0 siblings, 0 replies; 5+ messages in thread
From: Tzung-Bi Shih @ 2026-02-05 5:36 UTC (permalink / raw)
To: Ulf Hansson
Cc: Chen-Yu Tsai, Matthias Brugger, AngeloGioacchino Del Regno,
linux-pm, linux-mediatek
On Fri, Jan 30, 2026 at 08:17:34AM +0000, Tzung-Bi Shih wrote:
> On Tue, Jan 27, 2026 at 02:57:46PM +0100, Ulf Hansson wrote:
> > On Thu, 22 Jan 2026 at 09:38, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > > On Wed, Dec 31, 2025 at 11:54 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > > @@ -398,7 +398,7 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> > > > return ret;
> > > > }
> > > >
> > > > - ret = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> > > > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks);
> >
> > scpsys_hwv_power_off() is typically called by genpd in the suspend
> > noirq() phase, when all devices attached to the genpd in question have
> > been suspended too. See genpd_suspend_noirq().
> >
> > This means that scpsys_suspend() (below) may be called to unprepare
> > the clock, before scpsys_hwv_power_off() may call clk_disable(). This
> > is a bug according to the clock framework.
>
> Ack, I observed a similar warning in a system suspend/resume cycle.
>
> >
> > Moving scpsys_suspend() to the noirq phase too could maybe work.
>
> No, I guess it doesn't work as .prepare() can sleep however noirq phase
> shouldn't/can't.
>
> > Although, perhaps an even simpler solution would be to do the
> > clk_prepare() during ->probe() and clk_unprepare() during ->remove()
> > (and error path in probe). Of course, this assumes that
> > clk_prepare/unprepare doesn't really do anything hardware wise, so we
> > don't start wasting power by keeping the clocks prepared.
>
> It turns out a pure revert of f0fce06e345d ("soc: mtk-pm-domains: Fix
> the clock prepared issue"). Per the commit message of f0fce06e345d,
> the concern is some PLLs keep prepared even if the system is suspended.
>
> Not sure should we take the compromise?
Please ignore the patch. The observed paths are two independent power
domains. Distinguishing them by setting different lock class key fixes
the lockdep warning as Chen-Yu suggested in [1].
[1] https://lore.kernel.org/all/CAGXv+5Fm7DFkZ_JONhHN4467=oVhuw-e1XtXuD53qBQDWd7cNw@mail.gmail.com/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-05 5:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-31 3:53 [PATCH v2] pmdomain: mediatek: Break lock dependency to `prepare_lock` Tzung-Bi Shih
2026-01-22 8:37 ` Chen-Yu Tsai
2026-01-27 13:57 ` Ulf Hansson
2026-01-30 8:17 ` Tzung-Bi Shih
2026-02-05 5:36 ` Tzung-Bi Shih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox