Re: [PATCH] clk: qcom: ipq-cmn-pll: keep the CMN block bus clocks enabled

On 7/31/2026 3:13 AM, Stanislaw Pal wrote:
> The probe function takes a runtime PM reference to enable the GCC AHB &
> SYS clocks of the CMN PLL block, registers the clocks, and then drops
> the reference. Once the autosuspend kicks in, pm_clk gates both clocks
> a few milliseconds after probe has returned.
> 
> That is wrong on two counts:
> 
>   - The clock ops access the CMN PLL registers without taking a runtime
>     PM reference of their own, so any later clk_set_rate() or rate
>     recalculation touches the register file with its bus clock gated.

clk_cmn_pll_set_rate() is invoked as part of the AHB and SYS clock
enable sequence. The CMN PLL clock rate is configured according to the
DTS assigned-clock-rates-u64 property, which is applied when
devm_of_clk_add_hw_provider() (called inside
ipq_cmn_pll_register_clks()) triggers the call chain:

of_clk_add_hw_provider() → of_clk_set_defaults()

This corresponds to the following DTS entries:
assigned-clocks = <&cmn_pll IPQ5018_CMN_PLL_CLK>;
assigned-clock-rates-u64 = /bits/ 64 <4800000000>;

> 
>   - On IPQ5018 the consequences are not contained to this device:
>     gating the CMN block bus clocks makes the SoC hang on a subsequent
>     bus access. Measured on a TP-Link Archer AX55 v1 (IPQ5018), the
>     machine dies silently within a few milliseconds of the CMN PLL
>     probe returning - mid-character on the UART - and the victim is
>     whichever device happens to probe next. Whether a given boot
>     survives the window is a micro-timing lottery: different binary
>     layouts of the same kernel ranged from occasional failures to a
>     100% reproducible boot loop.
> 
> Keep the runtime PM usage count elevated on the probe success path, so
> the bus clocks stay enabled. With this one change the same board went
> from boot-looping to surviving every boot attempt (verified across
> three previously-failing kernel binaries, plus repeated soft and hard
> resets on the final one).

The CMN PLL AHB/SYS clocks will remain enabled as long as a downstream
consumer of the CMN PLL clocks is active — this behaviour was confirmed
experimentally on the IPQ9574 platform as example.

# insmod ipq-cmn-pll.ko
#
# devmem 0x9b780
0x00000000
#
# insmod nsscc-ipq9574.ko
# devmem 0x9b780
0x000000FF
#

> 
> Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
> Cc: [email protected]
> Signed-off-by: Stanislaw Pal <[email protected]>
> ---
> One more observation from the same debugging session, for the people
> who know this silicon: on IPQ5018 the CMN_PLL_LOCKED bit (offset 0x64,
> bit 8) never asserts. Every clk_cmn_pll_set_rate() call runs its
> regmap_read_poll_timeout() to the full 100 ms timeout - including under
> the bootloader-programmed configuration the hardware demonstrably runs
> fine with - and nobody notices, because clk_change_rate() ignores the
> .set_rate return value, so the -ETIMEDOUT is swallowed silently. Is the
> lock status readable at a different offset on this SoC, or is the bit
> simply not functional there? Happy to send a follow-up (surfacing the
> error, or skipping the poll where it cannot work) once someone can say
> what the intended behavior is.

The CMN_PLL_LOCKED bit offset and behavior are consistent across all IPQ
platforms — the same register layout applies to IPQ5018.

> 
>  drivers/clk/qcom/ipq-cmn-pll.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> --- a/drivers/clk/qcom/ipq-cmn-pll.c
> +++ b/drivers/clk/qcom/ipq-cmn-pll.c
> @@ -454,11 +454,19 @@
>  
>  	/* Register CMN PLL clock and fixed rate output clocks. */
>  	ret = ipq_cmn_pll_register_clks(pdev);
> -	pm_runtime_put(dev);
> -	if (ret)
> +	if (ret) {
> +		pm_runtime_put(dev);
>  		return dev_err_probe(dev, ret,
>  				     "Failed to register CMN PLL clocks\n");
> +	}
>  
> +	/*
> +	 * Keep the runtime PM usage count elevated on the success path, so
> +	 * the CMN block AHB & SYS clocks stay enabled: the clock ops access
> +	 * the CMN PLL registers without taking a runtime PM reference, and
> +	 * on IPQ5018 gating these clocks after probe hangs the SoC on a
> +	 * subsequent bus access.
> +	 */
>  	return 0;
>  }
>  
>