* [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS [not found] <20260602042747.277270-1-github.com@herrie.org> @ 2026-06-02 4:27 ` Herman van Hazendonk 2026-06-02 4:48 ` sashiko-bot 2026-06-02 5:46 ` Herman van Hazendonk 0 siblings, 2 replies; 4+ messages in thread From: Herman van Hazendonk @ 2026-06-02 4:27 UTC (permalink / raw) To: sboyd Cc: Herman van Hazendonk, Bjorn Andersson, Michael Turquette, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-clk, linux-kernel, devicetree 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS 2026-06-02 4:27 ` [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS Herman van Hazendonk @ 2026-06-02 4:48 ` sashiko-bot 2026-06-02 5:46 ` Herman van Hazendonk 1 sibling, 0 replies; 4+ messages in thread From: sashiko-bot @ 2026-06-02 4:48 UTC (permalink / raw) To: Herman van Hazendonk; +Cc: devicetree, robh, conor+dt 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS 2026-06-02 4:27 ` [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS Herman van Hazendonk 2026-06-02 4:48 ` sashiko-bot @ 2026-06-02 5:46 ` Herman van Hazendonk 2026-06-02 6:49 ` Herman van Hazendonk 1 sibling, 1 reply; 4+ messages in thread From: Herman van Hazendonk @ 2026-06-02 5:46 UTC (permalink / raw) To: sboyd Cc: Bjorn Andersson, Michael Turquette, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-clk, linux-kernel, devicetree, 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS 2026-06-02 5:46 ` Herman van Hazendonk @ 2026-06-02 6:49 ` Herman van Hazendonk 0 siblings, 0 replies; 4+ messages in thread From: Herman van Hazendonk @ 2026-06-02 6:49 UTC (permalink / raw) To: sboyd Cc: Bjorn Andersson, Michael Turquette, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-clk, linux-kernel, devicetree, 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-06-02 6:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260602042747.277270-1-github.com@herrie.org>
2026-06-02 4:27 ` [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS Herman van Hazendonk
2026-06-02 4:48 ` sashiko-bot
2026-06-02 5:46 ` Herman van Hazendonk
2026-06-02 6:49 ` Herman van Hazendonk
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox