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

[sashiko-bot@kernel.org]

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