[PATCH] clk: mediatek: mt8173-mfgtop: do not use the clk API from power_on

[PATCH] clk: mediatek: mt8173-mfgtop: do not use the clk API from power_on

[Ryan Brue]

clk_mt8173_mfgtop_power_on() calls clk_prepare_enable() on the mfg_26m gate
that this same driver provides. That is a layering inversion which cannot
work: clk_core_prepare() calls clk_pm_runtime_get() on the clock provider
-- this driver's own device -- and genpd power transitions run at _noirq
time, where runtime PM is disabled and pm_runtime_resume_and_get() returns
-EACCES.

Measured on mt8173 (amazon-suez) during resume from suspend-to-RAM,
immediately after the secondary CPUs come back up:

  clk-mt8173-mfgtop 13fff000.clock-controller: 26 MHz clock enable failed: -13

The failure is not survivable, because genpd_sync_power_on() ignores what
power_on() returns and marks the domain on regardless. The clock is then
left ungated-but-unprepared, and the next power_off() underflows its
refcount:

  mfg_26m already disabled
  mfg_26m already unprepared
  WARNING: drivers/clk/clk.c:1048 at clk_core_unprepare
  Workqueue: pm genpd_power_off_work_fn
    clk_unprepare / clk_mt8173_mfgtop_power_off / _genpd_power_off

Gate the bit with a direct regmap write instead. The vendor driver
(mtk_mfgsys.c) also drives the four MFG CG bits with raw register writes
from its power sequencing. The runtime-PM path was never affected, only the
_noirq transitions of system suspend and resume. mfg_26m stays registered
as a clock for any consumer that wants it; nothing currently does.

Fixes: ebd0b73d2137 ("clk: mediatek: Add mt8173-mfgtop driver")
Assisted-by: LLM
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
 drivers/clk/mediatek/clk-mt8173-mfgtop.c | 39 +++++++++++++++++---------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8173-mfgtop.c b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
index 5669ca9954dc..fed6fa6100dc 100644
--- a/drivers/clk/mediatek/clk-mt8173-mfgtop.c
+++ b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
@@ -30,6 +30,12 @@ static const struct mtk_gate_regs mfg_cg_regs = {
 	.set_ofs = 0x0004,
 };
 
+/*
+ * The 26 MHz gate, addressed directly rather than through the clk framework.
+ * See the comment in clk_mt8173_mfgtop_power_on().
+ */
+#define MFG_CG_26M	BIT(3)
+
 #define GATE_MFG(_id, _name, _parent, _shift, _flags)	\
 	GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, _shift, &mtk_clk_gate_ops_setclr, _flags)
 
@@ -46,7 +52,6 @@ struct mt8173_mfgtop_data {
 	struct regmap *regmap;
 	struct generic_pm_domain genpd;
 	struct of_phandle_args parent_pd, child_pd;
-	struct clk *clk_26m;
 };
 
 /* Delay count in clock cycles */
@@ -66,12 +71,16 @@ struct mt8173_mfgtop_data {
 static int clk_mt8173_mfgtop_power_on(struct generic_pm_domain *domain)
 {
 	struct mt8173_mfgtop_data *data = container_of(domain, struct mt8173_mfgtop_data, genpd);
-	int ret;
 
-	/* drives internal power management */
-	ret = clk_prepare_enable(data->clk_26m);
-	if (ret)
-		return ret;
+	/*
+	 * Ungate the 26 MHz clock, which drives the block's internal power
+	 * management. This is a raw write to the gate's CG register, not
+	 * clk_prepare_enable() on the mfg_26m gate this driver provides: that
+	 * would call clk_pm_runtime_get() on our own device, and genpd power
+	 * transitions run at _noirq time, where runtime PM is disabled and
+	 * that returns -EACCES. mfg_26m stays registered for any consumer.
+	 */
+	regmap_write(data->regmap, mfg_cg_regs.clr_ofs, MFG_CG_26M);
 
 	/* Power on/off delays for various signals */
 	regmap_write(data->regmap, MFG_ACTIVE_POWER_CON0,
@@ -102,8 +111,11 @@ static int clk_mt8173_mfgtop_power_off(struct generic_pm_domain *domain)
 	/* Magic numbers related to core switch sequence and delays */
 	regmap_write(data->regmap, 0xec, 0);
 
-	/* drives internal power management */
-	clk_disable_unprepare(data->clk_26m);
+	/*
+	 * Gate the 26 MHz clock again; see power_on() for why this is a raw
+	 * register write and not clk_disable_unprepare().
+	 */
+	regmap_write(data->regmap, mfg_cg_regs.set_ofs, MFG_CG_26M);
 
 	return 0;
 }
@@ -155,16 +167,10 @@ static int clk_mt8173_mfgtop_probe(struct platform_device *pdev)
 		goto put_pm_runtime;
 	}
 
-	data->clk_26m = clk_hw_get_clk(data->clk_data->hws[CLK_MFG_26M], "26m");
-	if (IS_ERR(data->clk_26m)) {
-		ret = dev_err_probe(dev, PTR_ERR(data->clk_26m), "Failed to get 26 MHz clock\n");
-		goto unregister_clks;
-	}
-
 	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, data->clk_data);
 	if (ret) {
 		dev_err_probe(dev, ret, "Failed to add clk OF provider\n");
-		goto put_26m_clk;
+		goto unregister_clks;
 	}
 
 	data->genpd.name = "mfg-top";
@@ -197,8 +203,6 @@ static int clk_mt8173_mfgtop_probe(struct platform_device *pdev)
 	pm_genpd_remove(&data->genpd);
 del_clk_provider:
 	of_clk_del_provider(node);
-put_26m_clk:
-	clk_put(data->clk_26m);
 unregister_clks:
 	mtk_clk_unregister_gates(mfg_clks, ARRAY_SIZE(mfg_clks), data->clk_data);
 put_pm_runtime:
@@ -217,7 +221,6 @@ static void clk_mt8173_mfgtop_remove(struct platform_device *pdev)
 	of_genpd_del_provider(node);
 	pm_genpd_remove(&data->genpd);
 	of_clk_del_provider(node);
-	clk_put(data->clk_26m);
 	mtk_clk_unregister_gates(mfg_clks, ARRAY_SIZE(mfg_clks), data->clk_data);
 	of_node_put(data->parent_pd.np);
 }

---
base-commit: df2908090cda368b01ff43709f51890076c56157
change-id: 20260912-mfgtop-no-clk-api-power-on-a5e0a3a43b33

Best regards,
--  
Ryan Brue <ryanbrue.dev@gmail.com>

Re: [PATCH] clk: mediatek: mt8173-mfgtop: do not use the clk API from power_on

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bypassing the clock framework for mfg_26m with raw regmap writes leaves its enable_count at 0, causing clk_disable_unused() to erroneously gate the active clock during late init.
--

commit 21d85aa137f918a11f3303672477f1a831e84a33
Author: Ryan Brue <ryanbrue.dev@gmail.com>

clk: mediatek: mt8173-mfgtop: do not use the clk API from power_on

This commit replaces clk_prepare_enable() and clk_disable_unprepare() calls
for mfg_26m with raw regmap writes to the clock gate registers in the
power_on and power_off callbacks. This is done to avoid a layering inversion
during _noirq system suspend/resume transitions where runtime PM is disabled
and calling the clk API fails.

> diff --git a/drivers/clk/mediatek/clk-mt8173-mfgtop.c b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> index 5669ca9954dc0..fed6fa6100dc5 100644
> --- a/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> +++ b/drivers/clk/mediatek/clk-mt8173-mfgtop.c

[ ... ]

> @@ -66,12 +71,16 @@ struct mt8173_mfgtop_data {
>  static int clk_mt8173_mfgtop_power_on(struct generic_pm_domain *domain)
>  {
>  	struct mt8173_mfgtop_data *data = container_of(domain, struct mt8173_mfgtop_data, genpd);
> -	int ret;
>  
> -	/* drives internal power management */
> -	ret = clk_prepare_enable(data->clk_26m);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * Ungate the 26 MHz clock, which drives the block's internal power
> +	 * management. This is a raw write to the gate's CG register, not
> +	 * clk_prepare_enable() on the mfg_26m gate this driver provides: that
> +	 * would call clk_pm_runtime_get() on our own device, and genpd power
> +	 * transitions run at _noirq time, where runtime PM is disabled and
> +	 * that returns -EACCES. mfg_26m stays registered for any consumer.
> +	 */
> +	regmap_write(data->regmap, mfg_cg_regs.clr_ofs, MFG_CG_26M);

[Severity: High]
Since this raw regmap write in clk_mt8173_mfgtop_power_on() in
drivers/clk/mediatek/clk-mt8173-mfgtop.c directly asserts the gate in
hardware, bypassing the Common Clock Framework (CCF) state tracking, the
clock's enable_count remains 0.

Because the mfg_26m clock is still fully registered via mfg_clks without the
CLK_IGNORE_UNUSED flag, won't clk_disable_unused() forcibly disable this
clock during late init?

If a consumer has probed and powered on the domain before or during
late_initcall_sync, clk_disable_unused() will check the hardware state, see
that the bit is active while enable_count is 0, and turn it off by writing
to set_ofs.

This abruptly shuts off the internal power management clock while the genpd
is still active, potentially leading to hardware hangs and failure of
subsequent power-off operations.

[ ... ]

> @@ -102,8 +111,11 @@ static int clk_mt8173_mfgtop_power_off(struct generic_pm_domain *domain)
>  	/* Magic numbers related to core switch sequence and delays */
>  	regmap_write(data->regmap, 0xec, 0);
>  
> -	/* drives internal power management */
> -	clk_disable_unprepare(data->clk_26m);
> +	/*
> +	 * Gate the 26 MHz clock again; see power_on() for why this is a raw
> +	 * register write and not clk_disable_unprepare().
> +	 */
> +	regmap_write(data->regmap, mfg_cg_regs.set_ofs, MFG_CG_26M);

[Severity: High]
This raw regmap write in clk_mt8173_mfgtop_power_off() deasserts the gate
directly, overriding any state assumed by CCF, which is part of the same
framework bypass issue described above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-mfgtop-no-clk-api-power-on-v1-1-def157ad1277@gmail.com?part=1

Re: [PATCH] clk: mediatek: mt8173-mfgtop: do not use the clk API from power_on

[Ryan Brue]

On 9/12/26 1:44 PM, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Since this raw regmap write in clk_mt8173_mfgtop_power_on() in
> drivers/clk/mediatek/clk-mt8173-mfgtop.c directly asserts the gate in
> hardware, bypassing the Common Clock Framework (CCF) state tracking, the
> clock's enable_count remains 0.
>
> Because the mfg_26m clock is still fully registered via mfg_clks without the
> CLK_IGNORE_UNUSED flag, won't clk_disable_unused() forcibly disable this
> clock during late init?
>
> If a consumer has probed and powered on the domain before or during
> late_initcall_sync, clk_disable_unused() will check the hardware state, see
> that the bit is active while enable_count is 0, and turn it off by writing
> to set_ofs.
>
> This abruptly shuts off the internal power management clock while the genpd
> is still active, potentially leading to hardware hangs and failure of
> subsequent power-off operations.
Yep, that's a real gap. I'll add CLK_IGNORE_UNUSED to the mfg_26m gate. 
Thanks for catching this!
> [Severity: High]
> This raw regmap write in clk_mt8173_mfgtop_power_off() deasserts the gate
> directly, overriding any state assumed by CCF, which is part of the same
> framework bypass issue described above.
This shares the same fix as the first one; once CCF stops touching the 
gate, the raw set/clr writes from the callbacks are the only writers. 
I'll send out a v2.