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

[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