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

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

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

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

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

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
#

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

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.

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

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

Is there any use case that requires accessing the CMN PLL registers when no downstream consumer is active? If not, I don’t think a fix is needed here. As you may have observed, debugfs clk_summary can still display the clock rate correctly even when there is no downstream consumer and the AHB and SYS clocks are disabled.

The clk_summary observation does not show what it seems to show, and I have to correct my own previous mail on the same point: this driver does not set CLK_GET_RATE_NOCACHE, and clk_core_get_rate_recalc() only calls .recalc_rate for clocks that have that flag. So clk_summary (and clk_get_rate()) return the rate cached at registration time, when probe still held the bus clocks enabled - no register access happens at all. It displaying correct rates with the clocks gated is exactly the cached value; it says nothing about whether an actual access would survive.

As for the use case: on IPQ5018 it is booting the SoC. With the clocks gated after probe, boards hang within milliseconds - 100% reproducible on some builds, before userspace exists, with no consumer anywhere - and the only variable that changes the outcome is holding this reference. That is also why I do not think moving runtime PM references into the clk ops would help this platform: by your own argument nothing calls the ops at that point, yet the SoC still dies. Whatever the fatal access is
- a CCF path we have not pinned down, or something else in the same clock domain - the platform demonstrably does not survive the gate itself.

One more general point: the CCF gives no guarantee that clk API calls only happen while some consumer device is runtime-active. The DT device link keeps the supplier active while the consumer *device* is active, but a consumer is free to call clk_set_rate()/clk_round_rate() at any time, including while itself runtime-suspended - and on IPQ5018 there is no link at all. So "registers are only accessed while a consumer is active" is not an invariant anything enforces; it just happens to hold on the platforms where the DT wiring exists.

If there is an alternative fix you would prefer - a different clock handoff from the bootloader state, describing the in-silicon consumers in DT, anything else - I am happy to test it on this hardware. But as it stands, a mainline kernel cannot boot reliably on IPQ5018 without this change, which is why I believe a fix is needed and Cc'd stable.

Thanks,
Stanislaw

Once the CMN PLL module has been loaded, there should be no further need to access its registers during normal operation. The CMN PLL provides fixed-rate clocks, and its output clocks should be initialized and operating at the correct fixed rates after the module is loaded for the IPQ5018 platform.

Regarding the hang issue you mentioned, it should not be related to the inability to access the CMN PLL registers. The actual root cause of the hang should be investigated separately.

Following along here - just want to make sure this doesn't get lost:
Stanislaw pointed out earlier that clk_summary can't actually be used as evidence here, since this driver doesn't set CLK_GET_RATE_NOCACHE and so never reads the registers live; it just returns the cached registration-time rate. That seems like a fairly load-bearing point for the discussion and I haven't seen it addressed yet.

Given the hang is reportedly 100% reproducible pre-userspace, is there a concrete next step to root-cause it, or is v3 the right call for now with a follow-up tracked separately?

There is a concrete step, and I ran it today on a third board - this time with no code of mine involved at all.

Hardware: GL.iNet GL-B3000 (IPQ5018), a supported in-tree OpenWrt board, running the current bone-stock OpenWrt snapshot (kernel 6.18.41, which does not carry this patch). Out of the box it boot
loops: the last line on earlycon is at ~0.39s (the final initcall before the driver probes start), then silence and a watchdog reset, 100% reproducible.

Adding exactly one thing to the kernel command line - no rebuild, no patch:

initcall_blacklist=ipq_cmn_pll_clk_driver_init

makes the same image boot: serial, SPI-NAND, SMEM partitions, remoteproc all come up.

Combined with what we already know from the other two boards, this isolates the trigger fairly tightly:

- cmn-pll probe runs, last PM reference dropped (vanilla): dies
- cmn-pll probe never runs (blacklist, clocks stay as the bootloader left them, i.e. enabled): boots
- cmn-pll probe runs, reference held: boots - verified on this same board today with an image carrying v3 of this patch and nothing else changed; it comes up fully (shell over SSH, NAND, remoteproc), and clk_summary shows the CMN block bus clocks held enabled by the provider device.

The only variable separating the dying case from both surviving ones is the AHB/SYS gate after probe. What exactly performs the fatal access afterwards is the remaining open question - and this board is well suited to answer it, since it has full U-Boot control and can run experiment kernels from RAM. I intend to bisect that next (my current suspicion is that the CMN block AHB clock also feeds the register path of neighbouring blocks in the same region - MDIO at 0x88000/0x90000, uniphy at 0x98000, CMN at 0x9b000 - which would explain why the CCF's own runtime PM handling around the clk ops cannot help here). I am happy to run any experiment Jie would like to see on this hardware.

So to answer the question directly: I believe v3 is the right call now
- three IPQ5018 boards, including a stock-image one, cannot boot without it and the cost of keeping two bus clocks of a small block enabled is negligible - with the exact-access root-causing tracked as a follow-up. If the follow-up ends up pointing at a cleaner fix (e.g. describing the real consumers of these bus clocks in DT), I will gladly send it as a successor.

Thanks,
Stanislaw

I tested the latest linux-next code with the following changes:

Enabled the MDIO interface and added the Ethernet PHY as DTS below. Enabled CONFIG_IPQ_CMN_PLL and CONFIG_MDIO_IPQ4019 as kernel modules.

--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi

+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi

@@ -219,12 +219,11 @@ mdio0: mdio@88000 {

                              <0x019475c4 0x4>;



                        #address-cells = <1>;



                        #size-cells = <0>;

+ clock-frequency = <6250000>;

                        clocks = <&gcc GCC_MDIO0_AHB_CLK>;



                        clock-names = "gcc_mdio_ahb_clk";

- status = "disabled";

-

                        ge_phy: ethernet-phy@7 {



                                compatible = "ethernet-phy-id004d.d0c0";



                                reg = <7>;

@@ -241,11 +240,16 @@ mdio1: mdio@90000 {

                        reg = <0x00090000 0x64>;



                        #address-cells = <1>;



                        #size-cells = <0>;

+ pinctrl-0 = <&mdio_pins>;

+ pinctrl-names = "default";

+ clock-frequency = <6250000>;

                        clocks = <&gcc GCC_MDIO1_AHB_CLK>;



                        clock-names = "gcc_mdio_ahb_clk";

- status = "disabled";

+ ethernet-phy@1c {

+ reg = <0x1c>;

+ };

                };







                cmn_pll: clock-controller@9b000 {

@@ -427,6 +431,23 @@ uart1_pins: uart1-state {

                                drive-strength = <8>;



                                bias-pull-down;



                        };

+

+ mdio_pins: mdio-state {

+ mdc-pins {

+ pins = "gpio36";

+ function = "mdc";

+ drive-strength = <8>;

+ bias-pull-up;

+ };

+

+ mdio-pins {

+ pins = "gpio37";

+ function = "mdio";

+ drive-strength = <8>;

+ bias-pull-up;

+ };

+ };

+

                };

The board booted successfully on the IPQ5018 RDP platform.

# insmod ipq-cmn-pll.ko
#
# devmem 0x1856308
0x80000000
# devmem 0x185630c
0x80000000
#
# insmod mdio-ipq4019.ko
#
# ls -l /sys/bus/mdio_bus/devices/
88000.mdio-1:07/  90000.mdio-1:1c/
# ls -l /sys/bus/mdio_bus/devices/
total 0
lrwxrwxrwx    1 root     root             0 Jan  1 00:01 88000.mdio-1:07
->
../../../devices/platform/soc@0/88000.mdio/mdio_bus/88000.mdio-1/88000.mdio-1:07
lrwxrwxrwx    1 root     root             0 Jan  1 00:01 90000.mdio-1:1c
->
../../../devices/platform/soc@0/90000.mdio/mdio_bus/90000.mdio-1/90000.mdio-1:1c

Thank you for running this. Your result is correct, and I can now reproduce the equivalent on the failing board - together the experiments finally bound the problem tightly. I spent the evening on a GL-B3000 running three variants of the same tree, all with the fix reverted (i.e. vanilla put in probe). Full data below.

Variant 1: gate delayed to idle. Vanilla behaviour, but the last reference is dropped via pm_runtime_put_autosuspend() with a 60 s autosuspend delay, and uniphy (the only in-tree consumer of the PLL outputs in the OpenWrt tree; mainline has none) disabled in DT so the gate actually lands. Result: the gate lands at ~75 s on an idle system and the board does not care. runtime_status reads "suspended", both WiFi radios keep serving clients, the console works, nothing in dmesg. This is your RDP result reproduced on the board that dies: with the system quiet, gating these clocks is harmless, and nothing in steady state needs them - your efficiency argument is confirmed.

Variant 2: same DT (uniphy disabled), unmodified vanilla driver, so the same gate lands right after probe, during early boot. Result over seven boots of the identical image: one survived, six died with the familiar signature - silence before the serial console comes up, then a watchdog reset. So the crash does not need the ethernet path at all: it happens with every PLL consumer disabled in DT, and it is probabilistic.

Variant 3 is the stock configuration (uniphy enabled): 100% boot loop on this board, and the same on three boards from three vendors.

One methodological note for fairness: variants 1 and 2 are initramfs images booted from RAM over tftp, so their early-boot activity profile differs from a normal flash boot (no UBI attach in the fatal window, for one). The 6-of-7 ratio is specific to that path and I would not generalize the number. The stock 100% failure, however, *is* the normal NAND boot path, so both paths are represented in the data and both die - only the probability differs with the timing profile, which is itself consistent with the collision picture.

Putting it together:

gate at idle -> safe, deterministically (your RDP, my V1)
gate during boot -> dies, probabilistically (V2: 6 of 7)
never gate (this fix) -> boots, deterministically (3 boards)

The only variable separating V1 from V2 is *when* the gate lands. That also retires my earlier "shared CSR bridge" theory - with the whole ethernet path disabled the board still dies - and explains every odd observation from the last month: the victim being whichever device probes next, and the failure probability swinging wildly with binary layout (both are just micro-timing of an asynchronous pm_clk_suspend landing somewhere in early-boot bus activity; which transaction it collides with is still not pinned down).

It also shows why a consumer-based model cannot close this hole: the fatal window lies *between* the cmn-pll probe and the moment any consumer could possibly take its first reference. On boards where Linux is entered with these clocks running (every bootloader does), the put in probe is what creates that window.

I am open on the shape of the fix - if you would rather see the reference dropped once boot has settled, or a synchronous gate, I will gladly test that on this board. But the boards are unbootable today, and holding the reference is the smallest change that deterministically removes the window, so I would still ask for v3 (or an equivalent) now, with refinements as follow-ups.

Thanks,
Stanislaw

Linux sees there's no consumers for the clocks the CMN PLL provides and gates them, subsequently also gating the PLL itself as all of its children become offline.

I can't find the docs that would show me what IPs consume this block, could you please help out here Jie?

Konrad

On IPQ5018, the CMN PLL provides a 50 MHz output clock that is used as the reference clock for the connected Ethernet device.

The PLL outputs keep running, but the AHB register interface does not. Once autosuspend gates AHB/SYS, any later register access to the CMN block hangs the bus. That is the bug we are fixing.

Correct, and that consumer is hardwired in silicon — the internal GE PHY and uniphy blocks take proxied this clock without any Linux driver calling clk_get() on it. Therefore no device link exists to keep the provider active, and CCF rightfully gates the bus clocks after probe.

Adding DT consumers would require touching PHY and datapath drivers across multiple subsystems just to create artificial clk references. That is not a stable material. The v4 driver fix is minimal, correct, and self-contained.

As far as I understand, there is no expectation that the CMN PLL registers need to be accessed after the CMN PLL driver has finished loading on IPQ5018.

Could we identify which module is blocked after the CMN PLL driver probe completes?

The UNIPHY block is the consumer of this 50 MHz clock. It divides and gates the 50 MHz clock, then routes it to the connected PHY or switch on the IPQ5018 platform.

Would you be able to try this approach in your code workspace? Since the UNIPHY driver is not currently available in the upstream kernel, this may be a practical way to validate whether keeping the relevant clock path active through the UNIPHY side resolves the issue.

UNIPHY register offset: 0x74

Bit 1: Clock divider control
0: no divide-by-2
1: divide by 2 enabled

Bit 0: Clock gate control
0: clock disabled
1: clock enabled

I don't have a captured trace to point to — this was found through code inspection, not a reproduced hang on hardware, and I'd rather say that plainly than overstate it.

That said, "no expectation of post-probe access" isn't something CCF enforces. debugfs (clk_summary and similar) can read back rate/state for any registered clk at any time regardless of whether a formal consumer exists, and nothing stops a future driver — including a possible upstream UNIPHY driver — from calling clk_get_rate() or clk_round_rate() on this provider later. Leaving the AHB/SYS interface gated while the provider is still registered and advertised as active seems fragile on those grounds alone, even without a concrete stack trace in hand today.

If anyone on this thread has access to IPQ5018 hardware and can force autosuspend then read back clk_summary for this provider, that would settle it either way — I don't currently have access to test this myself.

Agreed, and measured: with the AHB/SYS pair gated on a fully booted GL-B3000 (runtime PM autosuspend, gate landing ~75 s after probe, runtime_status "suspended"), the board keeps running, both radios keep serving clients, nothing complains. Nothing in Linux needs those registers after probe, and the v4 commit message says so. The fix is not about post-probe register access.

To Mieczyslaw's point on clk_summary: that read does not reach the hardware either way. The provider ops are wrapped in clk_pm_runtime_get()/put() by the framework (clk.c: recalc_rate at ~1923, prepare at ~1110, set_rate at ~2415/2539 in 6.18), so a debugfs read would resume the block before touching it - and the outputs are registered without CLK_GET_RATE_NOCACHE, so clk_summary reports the cached rate without recalc at all. Accesses through CCF are safe with or without this patch; the argument for it never rested on them.

None gets the chance. This is not a driver hanging on a register - the SoC dies and the watchdog resets it, silently, with no console output after the CMN PLL probe. Georg's timing on the Cudy P5: probe completes in ~628 us, pm_runtime_put() returns, and the board is dead before the next initcall starts. So the question "which module" has no answer in the form of a stuck driver; what exists is a window right after probe in which the asynchronous gate is fatal, and outside of which (idle system, above) the very same gate is harmless.

Thank you - that confirms the picture from the DT side (the GE PHY clocks are fixed-clock stubs in ipq5018.dtsi and no node references &cmn_pll, so Linux sees a provider with zero consumers while the consumer is wired in silicon). The 0x74 divider/gate description is useful and I have noted it.

Two measurements already bracket this, so let me put them on the table before anyone spends time on it:

- GL-B3000, UNIPHY0 disabled in DT (no uniphy driver ever probes), vanilla put: 6 of 7 boots die. The failure does not need the uniphy driver, or its clock path, to exist.
- Cudy P5: the board dies before the next initcall after the CMN PLL probe - i.e. before any uniphy driver could run at all.

So the fatal window opens and closes before the uniphy side gets to execute anything, upstream driver or not. A fix on the uniphy side can only start acting after that window, which is why holding the reference in the provider - the one place that exists at that moment - is what boots deterministically on all three boards. That said, the gating behaviour of 0x74 is worth understanding for the uniphy driver that will eventually come upstream, and I will keep it in mind for that work.

The full set of numbers (per-block isolation, the 15 ms / 2 s delay experiments on two boards, the idle-gate result) is in v4:
https://lore.kernel.org/linux-clk/[email protected]/

Is it only the UNIPHY consuming the 50 MHz one?

static const struct cmn_pll_fixed_output_clk ipq5018_output_clks[] = {
	CLK_PLL_OUTPUT(IPQ5018_XO_24MHZ_CLK, "xo-24mhz", 24000000UL),
	CLK_PLL_OUTPUT(IPQ5018_SLEEP_32KHZ_CLK, "sleep-32khz", 32000UL),
	CLK_PLL_OUTPUT(IPQ5018_ETH_50MHZ_CLK, "eth-50mhz", 50000000UL),
	{ /* Sentinel */ }
};

What consumes the XO clock? What consumes the sleep clock?

Konrad

Nothing in Linux consumes any of the three today.

grep -rn '&cmn_pll' arch/arm64/boot/dts/qcom/ on v6.18 gives, in full:

  ipq5018.dtsi:258:  assigned-clocks = <&cmn_pll IPQ5018_CMN_PLL_CLK>;
  ipq5424.dtsi:288:  assigned-clocks = <&cmn_pll IPQ5424_CMN_PLL_CLK>;
  ipq9574.dtsi:359:  assigned-clocks = <&cmn_pll CMN_PLL_CLK>;

ipq9574.dtsi:1253: <&cmn_pll NSS_1200MHZ_CLK>,
ipq9574.dtsi:1254: <&cmn_pll PPE_353MHZ_CLK>,

The first three are the provider setting its own VCO rate. The only real consumer anywhere is the IPQ9574 NSS clock controller. No device tree in the tree references XO_24MHZ_CLK or SLEEP_32KHZ_CLK, on any SoC - and of the five compatibles the driver carries, only ipq5018, ipq5424 and ipq9574 have a cmn_pll node in mainline at all.

On IPQ5018 specifically:

- eth-50mhz: Jie is right that the UNIPHY takes it, and it will become a clk_get() consumer once IPQ5018 UNIPHY/PCS support lands. The node in the pending work has

	clocks = <&gcc GCC_UNIPHY_AHB_CLK>, <&gcc GCC_UNIPHY_SYS_CLK>,
		 <&gcc GCC_UNIPHY_RX_CLK>, <&gcc GCC_UNIPHY_TX_CLK>,
		 <&cmn_pll IPQ5018_ETH_50MHZ_CLK>;
	clock-names = "ahb", "sys", "port5_rx", "port5_tx", "ref";

In mainline there is no such node yet - the only "uniphy" nodes on IPQ5018 are the two PCIe PHYs - so that 50 MHz path exists in silicon with nothing holding a handle on it from Linux.

- xo-24mhz and sleep-32khz: I know of no consumer, in tree or out. Worth noting these are not what the rest of the SoC runs on: the platform's 24 MHz XO and 32 kHz sleep clock come from the board oscillators described separately in ipq5018.dtsi (xo_clk -> ref_96mhz_clk -> xo_board_clk, and sleep_clk), not from the CMN block. The CMN outputs at the same nominal rates look like re-exports for hard-wired internal use.

I would rather this did not decide the patch, though, because the failure it fixes is not a later register access to the CMN block. Mieczyslaw's summary in the v4 thread put it that way; the mechanism I measured is narrower, and it is what the v4 commit message describes:

- gating those same two clocks on an idle, fully booted GL-B3000 (runtime PM autosuspend, gate landing ~75 s after probe) is harmless - runtime_status "suspended", both radios still serving clients;
- with UNIPHY0 disabled in the device tree, so that no uniphy driver exists in that boot at all, the board still dies in 6 of 7 boots;
- stretching the end of probe by 15 ms, or by a full 2 s, still ends in a watchdog reset, 8 of 8 boots each.

So what hangs the SoC is the gate transition landing amid early-boot bus activity, not any consumer's access afterwards. That is also why no consumer-side scheme can help: the window is between the CMN PLL probe returning and the first clk_get() by anybody, whichever consumer eventually turns up.

Thanks for the Ack on v4.

Best regards,
Stanislaw

Yes, that's more than clear :)

I'm trying to ask Jie about the platform details, to which I unfortunately don't have access at the moment..

or maybe +Kathiravan?

[...]

I would be more inclined to believe it's racing against some other driver enabling/disabling something else, perhaps a register that depends on a clock derived from these two.

When that crash occurs does the device enter crashdump mode (USB PID 0x900e)?

Konrad

The XO and sleep clocks should be supplied to the GCC. We could try adding the GCC as a consumer to verify whether this resolves the issue.

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.

---
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/[email protected]/

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

@@ -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);

[...]

if (ret) {
	pm_runtime_put(dev);
	return ret;
}

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.

---
Changes in v3:
- Fix a reference leak on the devm_pm_runtime_get_noresume() failure
path: v2 placed the call after pm_runtime_resume_and_get(), so an error return skipped the pm_runtime_put() further down and left that reference unbalanced. Spotted by Mieczyslaw Nalewaj. Rather than unwinding explicitly, the devres get is now taken before pm_runtime_resume_and_get(). Both helpers undo their own get on failure (devm_add_action_or_reset() runs the action, pm_runtime_get_active() calls pm_runtime_put_noidle()), so no error path needs cleanup at all. Happy to switch to the explicit pm_runtime_put() form if that reads better.

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.
- The diff is 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/[email protected]/
v2: https://lore.kernel.org/linux-clk/[email protected]/

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

@@ -433,6 +433,17 @@ static int ipq_cmn_pll_clk_probe(struct
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add SYS clock\n");
 
+	/*
+	 * 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;
+
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;

Recording a tag that came in off-list, so it does not get lost: a second user hit the same hang independently, on a board neither I nor anyone in this thread has touched.

Georg Seema is bringing up a Cudy P5 (also IPQ5018). Without this patch the board hangs during boot; with it applied it boots reliably. He found the patch on his own while debugging that hang, and gave the tag on the OpenWrt pull request that carries it:

https://github.com/openwrt/openwrt/pull/24653

That makes three IPQ5018 boards from three vendors - TP-Link Archer AX55 v1, GL.iNet GL-B3000 and Cudy P5 - where gating the CMN block bus clocks after probe kills the boot, and holding the reference fixes it.

I am not repeating the earlier arguments here. The bisection of the fatal access is still on my list and I will come back with v4 and a corrected explanation once I have named the register and the code path, rather than before.

Thanks,
Stanislaw

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 asynchronously a few milliseconds after probe has returned.

On IPQ5018 that gate races with early-boot activity on the bus and can hang the SoC: boards die silently right after the CMN PLL probe, before the next initcall gets to run, and the watchdog resets them. Whether a given kernel binary survives depends on micro-timing, ranging from an occasional hang to a 100% reproducible boot loop. The failure has been reported independently on three boards from three vendors (TP-Link Archer AX55 v1, GL.iNet GL-B3000, Cudy P5).

Isolation on the Cudy P5 (by Georg Seema) shows the failure is a matter of timing against boot activity, not a steady-state clock dependency:

- the probe completes in ~628 us, pm_runtime_put() returns, and the board dies before the next initcall starts;
- stretching the end of probe by ~15 ms (first unintentionally with debug prints, then with usleep_range()) makes the same kernel boot reliably;
- on that board an enabled UNIPHY0 node is what arms the failure; MDIO0/1, GMAC0/1 and the attached QCA8337 switch do not trigger it.

The armed configuration differs per board: on the GL-B3000 the failure persists with UNIPHY0 disabled (6 of 7 boots die), so the gate collides with whatever bus activity is in flight at that moment rather than with one specific peripheral.

Nor is a delay a workaround: replayed on the GL-B3000, stretching the end of probe by the same 15 ms - or by a full 2 s - still ends in a watchdog reset (8 of 8 boots each). A delay only helps where the sensitive activity happens to be finished before the gate lands, and how far into boot that extends is board-specific.

Consistently with the race picture, gating the very same clocks on an idle, fully booted system is harmless: delaying the gate via runtime PM autosuspend to ~75 s after boot on the GL-B3000 leaves a fully working system (runtime_status "suspended", WiFi serving clients), matching the module-insertion test on the IPQ5018 RDP posted by Jie Luo in the review thread. The clocks are not needed in steady state; it is the gate landing amid boot activity that kills the SoC. The window between the CMN PLL probe and the first reference taken by any consumer is exactly where the gate lands, so no consumer-side scheme can cover it.

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.

---
Changes in v4:
- No functional change; the code differs from v3 only in the comment. Commit message and comment rewritten now that the failure mode has been isolated: v3 claimed the clock ops access the registers without a runtime PM reference of their own, which is not accurate (the common clock framework wraps provider ops in clk_pm_runtime_get() / put()); the actual failure is the race on the gate transition described above.
- Added Georg Seema's Tested-by from the OpenWrt pull request carrying this patch (https://github.com/openwrt/openwrt/pull/24653); the Cudy P5 isolation above is his work, quoted with his permission.
- The idle-gate / boot-gate measurements on the GL-B3000 referenced above were posted earlier in this thread:
https://lore.kernel.org/linux-clk/[email protected]/
- New measurement for this revision: the probe-stretch experiment replayed on the GL-B3000 (vanilla put plus usleep_range(15000, 16000), then plus msleep(2000), everything else stock) dies 8 of 8 boots with either delay, while the same 15 ms rescues the Cudy P5 - the basis for the "delay is not a workaround" paragraph above.

Changes in v3:
- Fix a reference leak on the devm_pm_runtime_get_noresume() failure
path: v2 placed the call after pm_runtime_resume_and_get(), so an error return skipped the pm_runtime_put() further down and left that reference unbalanced. Spotted by Mieczyslaw Nalewaj. Rather than unwinding explicitly, the devres get is now taken before pm_runtime_resume_and_get(). Both helpers undo their own get on failure, so no error path needs cleanup at all.

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.

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/[email protected]/
v2: https://lore.kernel.org/linux-clk/[email protected]/
v3: https://lore.kernel.org/linux-clk/[email protected]/

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

@@ -448,6 +448,18 @@ static int ipq_cmn_pll_clk_probe(struct
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add SYS clock\n");
 
+	/*
+	 * Gating the CMN block AHB & SYS clocks is only safe on an idle
+	 * system: without this reference the gate lands asynchronously a
+	 * few milliseconds after probe, in the middle of the early-boot
+	 * probe activity, and on IPQ5018 that races with other bus
+	 * traffic and hangs the SoC. Hold the 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;
+
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;

Hi Stephen, Bjorn,

Just a quick ping regarding this series. The discussion between Stanislaw and Qualcomm (Jie Luo) has been fully settled in late August. It was proven that handling this on the UNIPHY side is not possible due to the early stage of the watchdog reset.

Are there any remaining concerns, or can this v4 be picked up for the next cycle?

Mieczyslaw Nalewaj<[email protected]>

Since it doesn't seem like a proper fix is coming soon, this is much better than keeping the platform broken in the meantime

Konrad

Please drop this patch.

Gabor's fix for the same hang has been applied:

33f9cb56cc28 ("clk: qcom: gcc-ipq5018: mark 'gpll0_main' clock as critical")

It addresses the platform-level cause: 'gpll0_main' feeds the CPUs through GPLL0 during early boot, before the APCS clock is registered and CCF can see that consumer, so the first driver to drop a runtime PM reference on a GCC clock under it takes the CPUs down with it. The ipq-cmn-pll probe was only the first driver to do so on the AX55 v1; I had verified on that board that Gabor's change alone gives a reliable boot with this patch absent, and OpenWrt has since replaced this patch with his in its tree.

That also answers the two open questions here. Jie's suggestion of making the GCC a consumer of the CMN PLL outputs is no longer needed for this hang, and Konrad's question about crashdump mode I cannot
answer: I never captured that state, and with the fix in place I have nothing left that reproduces it.

Thank you all for the review, and Gabor for finding the actual cause.

Best regards,
Stanislaw