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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 9/13/26 23:47, Ryan Brue wrote:
> 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.
> 
> Because the gate is now driven from the domain callbacks, its enable count
> in the clock framework stays 0 while the domain is on. Mark it
> CLK_IGNORE_UNUSED so that clk_disable_unused() does not gate the 26 MHz
> clock under an already-powered domain when a consumer probes before late
> init; the domain callbacks are the only writers.
> 
> Fixes: ebd0b73d2137 ("clk: mediatek: Add mt8173-mfgtop driver")
> Assisted-by: LLM
> Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>

I don't exactly like this, but it's the least complicated way of doing this, and
it's also the one that is less likely to break over time, so...

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
> Changes in v2:
> - Mark mfg_26m CLK_IGNORE_UNUSED: with the gate driven from the domain
>    callbacks its CCF enable count stays 0, so clk_disable_unused() would gate
>    the 26 MHz clock under an already-powered domain if a consumer probed
>    before late init (reported by the Sashiko review of v1). Verified on the
>    Fire HD 10: the gate follows the domain (hardware Y with the GPU active, N
>    after unbind) while the enable count stays 0, and the GPU keeps working.
> - Link to v1: https://patch.msgid.link/20260912-mfgtop-no-clk-api-power-on-v1-1-def157ad1277@gmail.com
> ---
>   drivers/clk/mediatek/clk-mt8173-mfgtop.c | 29 +++++++++++------------------
>   1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8173-mfgtop.c b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> index 5669ca9954dc..53918ad7f402 100644
> --- a/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> +++ b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> @@ -30,6 +30,8 @@ static const struct mtk_gate_regs mfg_cg_regs = {
>   	.set_ofs = 0x0004,
>   };
>   
> +#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)
>   
> @@ -38,7 +40,8 @@ static const struct mtk_gate mfg_clks[] = {
>   	GATE_MFG(CLK_MFG_AXI, "mfg_axi", "axi_mfg_in_sel", 0, CLK_SET_RATE_PARENT),
>   	GATE_MFG(CLK_MFG_MEM, "mfg_mem", "mem_mfg_in_sel", 1, CLK_SET_RATE_PARENT),
>   	GATE_MFG(CLK_MFG_G3D, "mfg_g3d", "mfg_sel", 2, CLK_SET_RATE_PARENT),
> -	GATE_MFG(CLK_MFG_26M, "mfg_26m", "clk26m", 3, 0),
> +	/* Driven by the power domain callbacks, not by a clk consumer */
> +	GATE_MFG(CLK_MFG_26M, "mfg_26m", "clk26m", 3, CLK_IGNORE_UNUSED),
>   };
>   
>   struct mt8173_mfgtop_data {
> @@ -46,7 +49,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 +68,12 @@ 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;
> +	/*
> +	 * Drives internal power management. Written directly: the clk API
> +	 * needs runtime PM, which is disabled during _noirq transitions.
> +	 */
> +	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,
> @@ -103,7 +105,7 @@ static int clk_mt8173_mfgtop_power_off(struct generic_pm_domain *domain)
>   	regmap_write(data->regmap, 0xec, 0);
>   
>   	/* drives internal power management */
> -	clk_disable_unprepare(data->clk_26m);
> +	regmap_write(data->regmap, mfg_cg_regs.set_ofs, MFG_CG_26M);
>   
>   	return 0;
>   }
> @@ -155,16 +157,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 +193,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 +211,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>