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

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

[Stanislaw Pal]

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.

  - 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).

Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
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.

 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;
 }
 

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

[Mieczyslaw Nalewaj]

On 7/30/2026 9:13 PM, 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.
> 
>   - 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).
> 
> Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
> ---
Reviewed-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>

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

[Jie Luo]

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: stable@vger.kernel.org
> Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
> ---
> 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;
>  }
>  
> 

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

[Stanislaw Pal]

On 8/3/2026 Jie Luo wrote:
> 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.

That is exactly the mechanism - and it is exactly what IPQ5018 does not
have. On IPQ9574 the nsscc node consumes the CMN PLL outputs in DT:

  ipq9574.dtsi:
    nsscc: clock-controller@39b00000 {
        clocks = <&xo_board_clk>,
                 <&cmn_pll NSS_1200MHZ_CLK>,
                 <&cmn_pll PPE_353MHZ_CLK>, ...

so the device link holds the supplier active, which is what your
insmod/devmem experiment shows.

On IPQ5018 there is no such consumer: nothing in ipq5018.dtsi references
any cmn_pll output clock - the only occurrences of the phandle are the
provider node itself and its own assigned-clocks. The actual users of
the CMN outputs on this SoC (the internal GE PHY and the uniphy blocks)
take them directly in hardware, with no DT linkage, so no device link
ever holds the provider active. A few ms after probe the autosuspend
gates the AHB/SYS clocks, and the box dies on the next bus access -
which is the measured behaviour the patch description quotes.

So I would frame the patch as: keep the usage count elevated on SoCs
where the hardware consumes the CMN outputs behind Linux's back. If the
preferred long-term shape is instead to describe those consumers in DT
(or to make the clock ops take runtime PM references of their own), I am
happy to help test either on IPQ5018 hardware - but until one of those
exists, this one-liner is what makes the SoC boot reliably, which is why
I kept it minimal and Cc'd stable.

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

Understood - but then there may be something else wrong on this SoC,
because the measurement is unambiguous: on IPQ5018 the bit at offset
0x64 bit 8 never asserts. Every clk_cmn_pll_set_rate() call runs
regmap_read_poll_timeout() to the full 100 ms timeout, including for the
very configuration the bootloader programmed and the board demonstrably
runs on (ethernet and wifi clocks all functional). The -ETIMEDOUT is
then swallowed because clk_change_rate() ignores the .set_rate return
value, so nothing is ever logged. If the layout is the same, is there
anything that gates lock detection on this SoC (analog block state,
reference selection, a status-enable bit) that the bootloader may leave
in a different state than the driver expects? I can run any register
dump or experiment on the board that would help pin it down.

Thanks for looking at this, and thanks Mieczyslaw for the review.

Stanislaw

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

[Jie Luo]

On 8/3/2026 5:10 PM, Stanislaw Pal wrote:
>> 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.
> That is exactly the mechanism - and it is exactly what IPQ5018 does not
> have. On IPQ9574 the nsscc node consumes the CMN PLL outputs in DT:
> 
>   ipq9574.dtsi:
>     nsscc: clock-controller@39b00000 {
>         clocks = <&xo_board_clk>,
>                  <&cmn_pll NSS_1200MHZ_CLK>,
>                  <&cmn_pll PPE_353MHZ_CLK>, ...
> 
> so the device link holds the supplier active, which is what your
> insmod/devmem experiment shows.
> 
> On IPQ5018 there is no such consumer: nothing in ipq5018.dtsi references
> any cmn_pll output clock - the only occurrences of the phandle are the
> provider node itself and its own assigned-clocks. The actual users of
> the CMN outputs on this SoC (the internal GE PHY and the uniphy blocks)
> take them directly in hardware, with no DT linkage, so no device link
> ever holds the provider active. A few ms after probe the autosuspend
> gates the AHB/SYS clocks, and the box dies on the next bus access -
> which is the measured behaviour the patch description quotes.

The CMN PLL output clocks do not depend on the AHB or SYS clocks. They
continue to operate correctly at the fixed rates even when the AHB
and SYS clocks are disabled. Therefore, once the CMN PLL module is
loaded, its output clocks are expected to operate at the correct
frequencies.

The downstream consumer is used to keep the AHB and SYS clocks enabled,
allowing the CMN PLL registers to be accessed.

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

[Stanislaw Pal]

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, letting pm_clk gate both clocks a few milliseconds after
probe has returned. The clock ops access the CMN PLL registers without
a runtime PM reference of their own, and on IPQ5018 gating the CMN
block bus clocks makes the SoC hang on a subsequent bus access: boards
died silently within milliseconds of the CMN PLL probe, up to a 100%
reproducible boot loop, depending on binary layout (micro-timing).

Take a devres-managed runtime PM reference in probe, so the bus clocks
stay enabled for as long as the driver is bound and the reference is
released again on unbind.

Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ SoC")
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
Changes in v2:
- Use devm_pm_runtime_get_noresume() instead of simply skipping the
  pm_runtime_put() on the probe success path. The v1 arrangement left
  the usage count elevated with nothing to balance it on unbind; the
  devres action releases it. Spotted by the Sashiko automated review
  (thanks to Mieczyslaw Nalewaj for pointing it out). Note that
  pm_runtime_reinit() on unbind does set the status back to suspended,
  so the leak did not have the re-bind consequences the report
  suggested - but it was a leak nonetheless, and the devres form is
  the idiomatic way to express "keep this device resumed while bound".
- The diff is now purely additive; the existing error handling in
  probe is left untouched.

Note for stable: devm_pm_runtime_get_noresume() was added in v6.16 by
commit 73db799bf5ef ("PM: runtime: Add new devm functions"), while this
driver dates back to v6.14. On 6.14.y/6.15.y (both EOL) the equivalent
is to move the pm_runtime_put() out of probe and add one to
ipq_cmn_pll_clk_remove() instead.

v1: https://lore.kernel.org/linux-clk/20260730191353.557494-1-kuncy7@gmail.com/

 drivers/clk/qcom/ipq-cmn-pll.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

--- a/drivers/clk/qcom/ipq-cmn-pll.c
+++ b/drivers/clk/qcom/ipq-cmn-pll.c
@@ -437,6 +437,17 @@ static int ipq_cmn_pll_clk_probe(struct
 	if (ret)
 		return ret;

+	/*
+	 * The clock ops access the CMN PLL registers without taking a
+	 * runtime PM reference of their own, and on IPQ5018 gating the CMN
+	 * block AHB & SYS clocks after probe hangs the SoC on a subsequent
+	 * bus access. Hold a reference for as long as the driver is bound
+	 * so that the bus clocks stay enabled.
+	 */
+	ret = devm_pm_runtime_get_noresume(dev);
+	if (ret)
+		return ret;
+
 	/* Register CMN PLL clock and fixed rate output clocks. */
 	ret = ipq_cmn_pll_register_clks(pdev);
 	pm_runtime_put(dev);

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

[Stanislaw Pal]

On 8/4/2026 Jie Luo wrote:
> The CMN PLL output clocks do not depend on the AHB or SYS clocks. They
> continue to operate correctly at the fixed rates even when the AHB
> and SYS clocks are disabled. Therefore, once the CMN PLL module is
> loaded, its output clocks are expected to operate at the correct
> frequencies.
>
> The downstream consumer is used to keep the AHB and SYS clocks enabled,
> allowing the CMN PLL registers to be accessed.

Agreed on both points, and they match what I measured: with the bus
clocks gated the PLL outputs keep running (ethernet and wifi stay
clocked), only register access dies.

But I think these two points together are exactly the argument for the
patch. The register accesses do not stop when there is no consumer: the
CCF invokes the driver's ops regardless. clk_cmn_pll_recalc_rate() does
two regmap_read()s and runs on any clk_get_rate() of the PLL and on
every debugfs clk_summary read - the latter user-triggerable at an
arbitrary time. clk_cmn_pll_set_rate() likewise accesses registers
whenever a rate is set. On IPQ5018, where no DT consumer exists at all,
every one of those calls after probe touches the block with AHB/SYS
gated, and that is the measured hang - the boards died during boot with
no userspace involved, so an in-kernel path hits it too.

And note the consumer mechanism only guarantees access "while the
consumer is active": on the SoCs that do have a DT consumer, a
runtime-suspended consumer plus a clk_summary read is the same
access-with-gated-clocks situation, just harder to hit. So having the
provider hold the reference for as long as it can be asked to service
clk ops - i.e. while bound - seems like the robust shape regardless of
platform.

I have just posted v2 which does exactly that, in a cleaner form:
devm_pm_runtime_get_noresume() in probe, so the reference is dropped
automatically on unbind and the existing put in the error path stays
untouched.

Thanks,
Stanislaw