clk: qcom: gcc-msm8660: register CE2 H clock

PATCH In review Herman van Hazendonk 7 participants 11 messages Jun 2 – Jun 15 1 message not in the archive

Subsystems (4)

Device tree bindings Arm Qualcomm mailing list Common clk Qualcomm clock drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Herman van Hazendonk

author

2026-06-02 04:27 UTC #1 lore raw

PATCH 1/2 clk: qcom: gcc-msm8660: register CE2 H clock

On MSM8x60 the Crypto Engine 2 (CE2) block at 0x18500000 is gated by a single hardware enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The existing dt-binding header already reserves CE2_H_CLK (ID 77) for this clock but the driver never registered an entry for it, so probe of any consumer that resolves the binding fails: the CE2 MMIO window reads back 0x0 and qce's DMA hangs indefinitely waiting for handshake signals that never arrive.

Add a single clk_branch under CE2_H_CLK pointing at the GCC enable. The upstream qce driver requests both "core" and "iface" via devm_clk_get_optional_enabled(); on MSM8x60 the vendor MSM8660 clock-8x60.c maps both consumer-name lookups to the same hardware register, so the consumer device tree can reference the single CE2_H_CLK phandle twice under both clock-names. The framework returns the same struct clk for both clk_get() calls, per-consumer refcounting works correctly, and the underlying enable bit stays asserted while either consumer holds the clock prepared -- avoiding the refcount race two independent clk_branch structs would create against the same hardware bit.

Signed-off-by Herman van Hazendonk <[email protected]>

---
 drivers/clk/qcom/gcc-msm8660.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

drivers/clk/qcom/gcc-msm8660.c +33 -0

index a6a4477ccdef..e81b8851a786 100644
@@ -1518,6 +1518,38 @@ static struct clk_branch pmem_clk = {
 	},
 };
 
+/*
+ * Crypto Engine 2 (CE2) clock.
+ *
+ * On MSM8x60 the CE2 block at 0x18500000 is gated by a single hardware
+ * enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The vendor MSM8660
+ * clock-8x60.c routes both the "core" and "iface" consumer-name lookups
+ * to this one register, and the upstream QCE crypto driver requests
+ * both clock names via devm_clk_get_optional_enabled(). Without the
+ * clock present at probe the QCE MMIO window reads back 0x0 and DMA
+ * hangs indefinitely waiting for handshake signals that never arrive.
+ *
+ * Register a single clk_branch: the consumer DT can reference the same
+ * clock phandle twice under different clock-names ("core" and "iface"),
+ * which yields the same struct clk for both clk_get() calls. Per-
+ * consumer refcounting then works correctly and the single underlying
+ * enable bit is asserted while either consumer holds the clock
+ * prepared, instead of having two independent clk_branch structs
+ * racing the same hardware bit.
+ */
+static struct clk_branch ce2_h_clk = {
+	.halt_reg = 0x2fd4,
+	.halt_bit = 0,
+	.clkr = {
+		.enable_reg = 0x2740,
+		.enable_mask = BIT(4),
+		.hw.init = &(struct clk_init_data){
+			.name = "ce2_h_clk",
+			.ops = &clk_branch_ops,
+		},
+	},
+};
+
 static struct clk_rcg prng_src = {
 	.ns_reg = 0x2e80,
 	.p = {

@@ -2566,6 +2598,7 @@ static struct clk_regmap *gcc_msm8660_clks[] = {
 	[GP2_SRC] = &gp2_src.clkr,
 	[GP2_CLK] = &gp2_clk.clkr,
 	[PMEM_CLK] = &pmem_clk.clkr,
+	[CE2_H_CLK] = &ce2_h_clk.clkr,
 	[PRNG_SRC] = &prng_src.clkr,
 	[PRNG_CLK] = &prng_clk.clkr,
 	[SDC1_SRC] = &sdc1_src.clkr,

Signature · 1 line

2.43.0

Dmitry Baryshkov

Reviewed-by on v1 1/2

2026-06-08 07:27 UTC #2 lore raw

Quoted 20 lines · Herman van Hazendonk

On MSM8x60 the Crypto Engine 2 (CE2) block at 0x18500000 is gated by a single hardware enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The existing dt-binding header already reserves CE2_H_CLK (ID 77) for this clock but the driver never registered an entry for it, so probe of any consumer that resolves the binding fails: the CE2 MMIO window reads back 0x0 and qce's DMA hangs indefinitely waiting for handshake signals that never arrive.
Add a single clk_branch under CE2_H_CLK pointing at the GCC enable. The upstream qce driver requests both "core" and "iface" via devm_clk_get_optional_enabled(); on MSM8x60 the vendor MSM8660 clock-8x60.c maps both consumer-name lookups to the same hardware register, so the consumer device tree can reference the single CE2_H_CLK phandle twice under both clock-names. The framework returns the same struct clk for both clk_get() calls, per-consumer refcounting works correctly, and the underlying enable bit stays asserted while either consumer holds the clock prepared -- avoiding the refcount race two independent clk_branch structs would create against the same hardware bit.

Quoted 5 lines

Signed-off-by Herman van Hazendonk <[email protected]>
---
 drivers/clk/qcom/gcc-msm8660.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Reviewed-by Dmitry Baryshkov <[email protected]>

Signature · 2 lines

With best wishes
Dmitry

Linus Walleij

Reviewed-by on v1 1/2

2026-06-10 20:27 UTC #3 lore raw

In reply to Herman van Hazendonk

Quoted 20 lines · Herman van Hazendonk

On MSM8x60 the Crypto Engine 2 (CE2) block at 0x18500000 is gated by a single hardware enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The existing dt-binding header already reserves CE2_H_CLK (ID 77) for this clock but the driver never registered an entry for it, so probe of any consumer that resolves the binding fails: the CE2 MMIO window reads back 0x0 and qce's DMA hangs indefinitely waiting for handshake signals that never arrive.
Add a single clk_branch under CE2_H_CLK pointing at the GCC enable. The upstream qce driver requests both "core" and "iface" via devm_clk_get_optional_enabled(); on MSM8x60 the vendor MSM8660 clock-8x60.c maps both consumer-name lookups to the same hardware register, so the consumer device tree can reference the single CE2_H_CLK phandle twice under both clock-names. The framework returns the same struct clk for both clk_get() calls, per-consumer refcounting works correctly, and the underlying enable bit stays asserted while either consumer holds the clock prepared -- avoiding the refcount race two independent clk_branch structs would create against the same hardware bit.

Quoted 1 line

Signed-off-by Herman van Hazendonk <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Konrad Dybcio

2026-06-15 16:37 UTC #4 lore raw

In reply to Herman van Hazendonk

Quoted 7 lines · Herman van Hazendonk

On MSM8x60 the Crypto Engine 2 (CE2) block at 0x18500000 is gated by a single hardware enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The existing dt-binding header already reserves CE2_H_CLK (ID 77) for this clock but the driver never registered an entry for it, so probe of any consumer that resolves the binding fails: the CE2 MMIO window reads back 0x0 and qce's DMA hangs indefinitely waiting for handshake signals that never arrive.

[...]

Quoted 11 lines

+/*
+ * Crypto Engine 2 (CE2) clock.
+ *
+ * On MSM8x60 the CE2 block at 0x18500000 is gated by a single hardware
+ * enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The vendor MSM8660
+ * clock-8x60.c routes both the "core" and "iface" consumer-name lookups
+ * to this one register, and the upstream QCE crypto driver requests
+ * both clock names via devm_clk_get_optional_enabled(). Without the
+ * clock present at probe the QCE MMIO window reads back 0x0 and DMA
+ * hangs indefinitely waiting for handshake signals that never arrive.
+ *

Quoted 8 lines

+ * Register a single clk_branch: the consumer DT can reference the same
+ * clock phandle twice under different clock-names ("core" and "iface"),
+ * which yields the same struct clk for both clk_get() calls. Per-
+ * consumer refcounting then works correctly and the single underlying
+ * enable bit is asserted while either consumer holds the clock
+ * prepared, instead of having two independent clk_branch structs
+ * racing the same hardware bit.
+ */

I don't find this comment particularly valuable, given it ends up describing the fact that the common clock framework has refcounted enables (pretty widely known) and details how the DT is going to use this (which we can read in the DT itself)

I think the commit message is really exhaustive and it's a better place for this info anyway

Konrad

[email protected]

2026-06-15 17:21 UTC #5 lore raw

Quoted 42 lines · Konrad Dybcio

Quoted 7 lines · Herman van Hazendonk
On MSM8x60 the Crypto Engine 2 (CE2) block at 0x18500000 is gated by a single hardware enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The existing dt-binding header already reserves CE2_H_CLK (ID 77) for this clock but the driver never registered an entry for it, so probe of any consumer that resolves the binding fails: the CE2 MMIO window reads back 0x0 and qce's DMA hangs indefinitely waiting for handshake signals that never arrive.
[...]
Quoted 17 lines
+/*
+ * Crypto Engine 2 (CE2) clock.
+ *
+ * On MSM8x60 the CE2 block at 0x18500000 is gated by a single hardware
+ * enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The vendor MSM8660
+ * clock-8x60.c routes both the "core" and "iface" consumer-name lookups
+ * to this one register, and the upstream QCE crypto driver requests
+ * both clock names via devm_clk_get_optional_enabled(). Without the
+ * clock present at probe the QCE MMIO window reads back 0x0 and DMA
+ * hangs indefinitely waiting for handshake signals that never arrive.
+ *
+ * Register a single clk_branch: the consumer DT can reference the same
+ * clock phandle twice under different clock-names ("core" and
Quoted 8 lines
"iface"),
+ * which yields the same struct clk for both clk_get() calls. Per-
+ * consumer refcounting then works correctly and the single underlying
+ * enable bit is asserted while either consumer holds the clock
+ * prepared, instead of having two independent clk_branch structs
+ * racing the same hardware bit.
+ */
I don't find this comment particularly valuable, given it ends up describing the fact that the common clock framework has refcounted enables (pretty widely known) and details how the DT is going to use this (which we can read in the DT itself)

Quoted 4 lines

I think the commit message is really exhaustive and it's a better place for this info anyway
Konrad

Hi Konrad,

Happy to clean it up. MSM8x60 is poorly documented in public and whatever is available in downstream kernels is often incomplete, so I tried to document most in the various commits. Happy to put it in another place if that's more appropriate. A lot of the info was found by register poking and trial and error because the lack of documentation.

Thanks,
Herman

Dmitry Baryshkov

2026-06-15 23:56 UTC #6 lore raw

Quoted 46 lines · On Mon, Jun 15, 2026 at 07:21:24PM +0200, [email protected] wrote:

Quoted 45 lines · Konrad Dybcio
Quoted 7 lines · Herman van Hazendonk
On MSM8x60 the Crypto Engine 2 (CE2) block at 0x18500000 is gated by a single hardware enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The existing dt-binding header already reserves CE2_H_CLK (ID 77) for this clock but the driver never registered an entry for it, so probe of any consumer that resolves the binding fails: the CE2 MMIO window reads back 0x0 and qce's DMA hangs indefinitely waiting for handshake signals that never arrive.
[...]
Quoted 17 lines
+/*
+ * Crypto Engine 2 (CE2) clock.
+ *
+ * On MSM8x60 the CE2 block at 0x18500000 is gated by a single hardware
+ * enable in GCC_CE2_HCLK_CTL (0x2740, BIT(4)). The vendor MSM8660
+ * clock-8x60.c routes both the "core" and "iface" consumer-name lookups
+ * to this one register, and the upstream QCE crypto driver requests
+ * both clock names via devm_clk_get_optional_enabled(). Without the
+ * clock present at probe the QCE MMIO window reads back 0x0 and DMA
+ * hangs indefinitely waiting for handshake signals that never arrive.
+ *
+ * Register a single clk_branch: the consumer DT can reference the same
+ * clock phandle twice under different clock-names ("core" and
Quoted 8 lines
"iface"),
+ * which yields the same struct clk for both clk_get() calls. Per-
+ * consumer refcounting then works correctly and the single underlying
+ * enable bit is asserted while either consumer holds the clock
+ * prepared, instead of having two independent clk_branch structs
+ * racing the same hardware bit.
+ */
I don't find this comment particularly valuable, given it ends up describing the fact that the common clock framework has refcounted enables (pretty widely known) and details how the DT is going to use this (which we can read in the DT itself)
I think the commit message is really exhaustive and it's a better place for this info anyway

Quoted 8 lines

Quoted 1 line
Konrad
Hi Konrad,
Happy to clean it up. MSM8x60 is poorly documented in public and whatever is available in downstream kernels is often incomplete, so I tried to document most in the various commits. Happy to put it in another place if that's more appropriate. A lot of the info was found by register poking and trial and error because the lack of documentation.

You can move it to the commit message. Another point would be to make QCE driver request only one clock on this platform. Is there a point in having two names for a single instance? (well, unless there is a good reason in the driver).

Quoted 3 lines

Thanks,
Herman

Signature · 2 lines

With best wishes
Dmitry

Herman van Hazendonk

author

2026-06-02 04:27 UTC #7 lore raw

PATCH 2/2 clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

In reply to a message not in the archive

Add the CPU-side software vote register for LPASS PLL4. PLL4 itself lives in the LCC (Low Power Audio Subsystem clock controller); GCC holds the apps-processor vote in PLL_ENA_SC0 (0x34c0) BIT(4). The LCC driver references "pll4" as the parent of its slimbus / SAIF / audio mclk roots, so without this vote PLL4 is gated off when the apps processor is the only consumer and LCC clocks silently fail to enable.

Expose it as a single clk_regmap with clk_pll_vote_ops and append the dt-binding ID at the next free slot (258) after the existing PLL12 (257), so DT ABI for boards already using the prior header is preserved.

Signed-off-by Herman van Hazendonk <[email protected]>

---
 drivers/clk/qcom/gcc-msm8660.c               | 15 +++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8660.h |  1 +
 2 files changed, 16 insertions(+)

drivers/clk/qcom/gcc-msm8660.c +15 -0

index e81b8851a786..cd392e140e95 100644
@@ -54,6 +54,20 @@ static struct clk_regmap pll8_vote = {
 	},
 };
 
+/* PLL4 is the LPASS PLL, defined in LCC. This is the voting clock. */
+static struct clk_regmap pll4_vote = {
+	.enable_reg = 0x34c0,
+	.enable_mask = BIT(4),
+	.hw.init = &(struct clk_init_data){
+		.name = "pll4_vote",
+		.parent_data = &(const struct clk_parent_data){
+			.fw_name = "pll4", .name = "pll4",
+		},
+		.num_parents = 1,
+		.ops = &clk_pll_vote_ops,
+	},
+};
+
 enum {
 	P_PXO,
 	P_PLL8,

@@ -2543,6 +2557,7 @@ static struct clk_branch rpm_msg_ram_h_clk = {
 static struct clk_regmap *gcc_msm8660_clks[] = {
 	[PLL8] = &pll8.clkr,
 	[PLL8_VOTE] = &pll8_vote,
+	[PLL4_VOTE] = &pll4_vote,
 	[GSBI1_UART_SRC] = &gsbi1_uart_src.clkr,
 	[GSBI1_UART_CLK] = &gsbi1_uart_clk.clkr,
 	[GSBI2_UART_SRC] = &gsbi2_uart_src.clkr,

include/dt-bindings/clock/qcom,gcc-msm8660.h +1 -0

index 4777c002711a..51d2e97441c8 100644
@@ -264,5 +264,6 @@
 #define PLL10					255
 #define PLL11					256
 #define PLL12					257
+#define PLL4_VOTE				258
 
 #endif

Signature · 1 line

2.43.0

[email protected]

AI review

2026-06-02 04:48 UTC #8 lore raw

Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] DT Binding Schema mismatch due to missing input clock documentation.

Pre-existing issues:
- [High] NULL pointer dereference when enabling an orphan pll4_vote clock. --

commit 7c30a11d991e96ae79ebcc182c62453a708e5ec5
Author: Herman van Hazendonk <[email protected]>

clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

Quoted 4 lines

drivers/clk/qcom/gcc-msm8660.c +0 -0
index e81b8851a7861..cd392e140e95c 100644

[ ... ]

drivers/clk/qcom/gcc-msm8660.c static struct clk_regmap pll8_vote = { · line 64

Quoted 12 lines

drivers/clk/qcom/gcc-msm8660.c +8 -0

@@ -54,6 +54,20 @@ static struct clk_regmap pll8_vote = {
 	},
 };
 
+/* PLL4 is the LPASS PLL, defined in LCC. This is the voting clock. */
+static struct clk_regmap pll4_vote = {
+	.enable_reg = 0x34c0,
+	.enable_mask = BIT(4),
+	.hw.init = &(struct clk_init_data){
+		.name = "pll4_vote",
+		.parent_data = &(const struct clk_parent_data){
+			.fw_name = "pll4", .name = "pll4",

[Severity: High]
This is a pre-existing issue, but will this cause a NULL pointer dereference if the LCC driver is not loaded or the DT node is disabled, leaving pll4_vote as an orphan?

The Common Clock Framework permits clk_enable() to be called on orphan clocks. If this happens, clk_pll_vote_enable() fetches the parent via clk_hw_get_parent(), which returns NULL.

It then unconditionally casts this NULL to struct clk_pll using to_clk_pll(). Because clkr is the last member of struct clk_pll, this yields a non-NULL invalid pointer offset:

drivers/clk/qcom/clk-pll.c:clk_pll_vote_enable() {
    struct clk_pll *p = to_clk_pll(clk_hw_get_parent(hw));
    ...
    return wait_for_pll(p);
}

This pointer is passed to wait_for_pll(), which calls clk_hw_get_name(&pll->clkr.hw). The &pll->clkr.hw calculation perfectly reverses the offset, yielding exactly NULL:

drivers/clk/qcom/clk-pll.c:wait_for_pll() {
    const char *name = clk_hw_get_name(&pll->clkr.hw);
    ...
}

clk_hw_get_name() then dereferences NULL->core->name, causing a panic. This patch exposes this pre-existing flaw by introducing the first clk_pll_vote_ops clock with an external parent.

[Severity: Medium]
Will this fw_name direct the clock framework to look for a clock named pll4 in the clock-names property of the qcom,gcc-msm8660 device tree node?

The YAML schema for this binding (Documentation/devicetree/bindings/clock/qcom,gcc-msm8660.yaml) strictly enforces maxItems: 2 for the clocks property and only permits pxo and cxo in clock-names.

Any device tree that correctly supplies pll4 to satisfy this lookup will fail dt_binding_check validation. Should the DT binding schema be updated alongside this patch to accommodate the new input clock?

Quoted 6 lines

drivers/clk/qcom/gcc-msm8660.c +6 -0

+		},
+		.num_parents = 1,
+		.ops = &clk_pll_vote_ops,
+	},
+};
+

[ ... ]

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Herman van Hazendonk

author

2026-06-02 05:46 UTC #9 lore raw

Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

In reply to Herman van Hazendonk

Hi,

Thanks, two real items here. Triage:

[High] clk_pll_vote_enable() NULL deref on orphan parent.

Confirmed: clk_hw_get_parent() can return NULL while the LCC parent is not yet bound, to_clk_pll(NULL) is then handed to wait_for_pll(), and clk_hw_get_name(&pll->clkr.hw) reverses the offset back to NULL and panics in core/core->name.

Not introduced by this patch though: drivers/clk/qcom/gcc-msm8960.c and gcc-apq8064.c already register an identical pll4_vote with the same parent_data fw_name = "pll4" and clk_pll_vote_ops, and have for years. The hazard already lives in mainline; my patch is a clone of the same pattern for the older Scorpion-class MSM8x60 family.

I will send a separate one-liner fix to drivers/clk/qcom/clk-pll.c adding the NULL check in clk_pll_vote_enable() so the cross-driver voter pattern stops being a latent panic everywhere it is used. That patch is a precondition for v2 of this series. I would rather not invent a parallel non-vote ops for MSM8660 specifically when the right answer is to make the existing one safe.

[Medium] qcom,gcc-msm8660.yaml does not allow "pll4" in clock-names.

Real, and an oversight on my part. The qcom,gcc-apq8064.yaml schema already documents the same shape -- clocks maxItems = 3, third entry "pll4" -- because apq8064's gcc-apq8064.c has the same pll4_vote pattern. I will mirror that here in v2:

- clocks:
- maxItems: 2
- clock-names:
- items:
- - const: pxo
- - const: cxo
+ clocks:
+ minItems: 2
+ maxItems: 3
+ clock-names:
+ minItems: 2
+ items:

  +      - const: pxo
  +      - const: cxo
  +      - const: pll4

The yaml fix becomes a new PATCH 1/3 in v2 (ahead of the existing CE2 + PLL4_VOTE driver patches) so the schema lands before the consumer.

I will hold v2 of this series until both the clk-pll.c NULL-check fix has had review traction and any further feedback on the v1 patches has come in.

Thanks,
Herman

Herman van Hazendonk

author

2026-06-02 06:49 UTC #10 lore raw

Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

Hi,

The promised clk-pll.c NULL-check fix is on linux-clk now:

https://lore.kernel.org/linux-clk/[email protected]/

[PATCH] clk: qcom: clk-pll: reject vote enable on orphan parent

Single hunk in drivers/clk/qcom/clk-pll.c::clk_pll_vote_enable():
resolve clk_hw_get_parent() first, return -ENODEV when it is NULL, only then call wait_for_pll() with the resolved parent. Same change benefits gcc-msm8960/apq8064/msm8660 and any future cross-controller pll4_vote pattern.

Once that lands I will roll v2 of this gcc-msm8660 series with the yaml maxItems=3 + pll4 clock-name addition (replied earlier in this thread) as PATCH 1/3, the existing CE2 + PLL4_VOTE driver patches as PATCH 2/3 and 3/3, and a cover-letter pointer to the clk-pll.c fix as the parent-orphan precondition.

Thanks,
Herman

Krzysztof Kozlowski

2026-06-08 10:09 UTC #11 lore raw

Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

In reply to Herman van Hazendonk

Quoted 13 lines · Herman van Hazendonk

Add the CPU-side software vote register for LPASS PLL4. PLL4 itself lives in the LCC (Low Power Audio Subsystem clock controller); GCC holds the apps-processor vote in PLL_ENA_SC0 (0x34c0) BIT(4). The LCC driver references "pll4" as the parent of its slimbus / SAIF / audio mclk roots, so without this vote PLL4 is gated off when the apps processor is the only consumer and LCC clocks silently fail to enable.
Expose it as a single clk_regmap with clk_pll_vote_ops and append the dt-binding ID at the next free slot (258) after the existing PLL12 (257), so DT ABI for boards already using the prior header is preserved.

Quoted 4 lines

Signed-off-by Herman van Hazendonk <[email protected]>
---
 drivers/clk/qcom/gcc-msm8660.c               | 15 +++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8660.h |  1 +

You need to slow down with your patches. I see so many similar issues and I don't know if I already commented on this or not.

And run the checkpatch finally.

Best regards,
Krzysztof