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

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

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

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/gcc-msm8660.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/clk/qcom/gcc-msm8660.c b/drivers/clk/qcom/gcc-msm8660.c
index a6a4477ccdef..e81b8851a786 100644
--- a/drivers/clk/qcom/gcc-msm8660.c
+++ b/drivers/clk/qcom/gcc-msm8660.c
@@ -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,
-- 
2.43.0

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

[Dmitry Baryshkov]

On Tue, Jun 02, 2026 at 06:27:44AM +0200, Herman van Hazendonk wrote:
> 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 <github.com@herrie.org>
> ---
>  drivers/clk/qcom/gcc-msm8660.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

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

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

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/gcc-msm8660.c               | 15 +++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8660.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/clk/qcom/gcc-msm8660.c b/drivers/clk/qcom/gcc-msm8660.c
index e81b8851a786..cd392e140e95 100644
--- a/drivers/clk/qcom/gcc-msm8660.c
+++ b/drivers/clk/qcom/gcc-msm8660.c
@@ -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,
diff --git a/include/dt-bindings/clock/qcom,gcc-msm8660.h b/include/dt-bindings/clock/qcom,gcc-msm8660.h
index 4777c002711a..51d2e97441c8 100644
--- a/include/dt-bindings/clock/qcom,gcc-msm8660.h
+++ b/include/dt-bindings/clock/qcom,gcc-msm8660.h
@@ -264,5 +264,6 @@
 #define PLL10					255
 #define PLL11					256
 #define PLL12					257
+#define PLL4_VOTE				258
 
 #endif
-- 
2.43.0

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

[sashiko-bot]

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 <github.com@herrie.org>

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

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

> diff --git a/drivers/clk/qcom/gcc-msm8660.c b/drivers/clk/qcom/gcc-msm8660.c
> index e81b8851a7861..cd392e140e95c 100644
> --- a/drivers/clk/qcom/gcc-msm8660.c
> +++ b/drivers/clk/qcom/gcc-msm8660.c
[ ... ]
> @@ -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?

> +		},
> +		.num_parents = 1,
> +		.ops = &clk_pll_vote_ops,
> +	},
> +};
> +
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602042747.277270-1-github.com@herrie.org?part=2

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

[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

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

[Herman van Hazendonk]

Hi,

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

  https://lore.kernel.org/linux-clk/20260602062927.467249-1-github.com@herrie.org/

  [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

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

[Krzysztof Kozlowski]

On Tue, Jun 02, 2026 at 06:27:45AM +0200, Herman van Hazendonk wrote:
> 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 <github.com@herrie.org>
> ---
>  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