[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

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

[Linus Walleij]

On Tue, Jun 2, 2026 at 6:27 AM Herman van Hazendonk
<github.com@herrie.org> 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>

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

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

[Konrad Dybcio]

On 6/2/26 6:27 AM, 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.

[...]

> +/*
> + * 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.
> + */

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

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

[me]

On 2026-06-15 18:36, Konrad Dybcio wrote:
> On 6/2/26 6:27 AM, 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.
> 
> [...]
> 
>> +/*
>> + * 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.
>> + */
> 
> 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
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

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

[Dmitry Baryshkov]

On Mon, Jun 15, 2026 at 07:21:24PM +0200, me@herrie.org wrote:
> On 2026-06-15 18:36, Konrad Dybcio wrote:
> > On 6/2/26 6:27 AM, 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.
> > 
> > [...]
> > 
> > > +/*
> > > + * 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.
> > > + */
> > 
> > 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
> 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).

> 
> Thanks,
> Herman

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

[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