Re: [PATCH v2 3/3] clk: qcom: add MSM8x60 LCC (LPASS) driver

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing Clock Parent `pll4_vote` leading to orphaned clocks.

Pre-existing issues:
- [High] Race condition between RCG clock `set_rate` and Branch clock `enable`/`disable` sharing the same register.
--

commit 0c6b3f0cf3eb1cc15e3add96b0433ff823210d70
Author: Herman van Hazendonk <github.com@herrie.org>

clk: qcom: add MSM8x60 LCC (LPASS) driver

This commit adds a clock driver for the LPASS Clock Controller on the
MSM8x60 family. It handles the specific register layout, muxing, and
divider topology for this generation, ensuring LPASS audio DSP clocks
work correctly before audio playback or capture.

> diff --git a/drivers/clk/qcom/lcc-msm8660.c b/drivers/clk/qcom/lcc-msm8660.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/qcom/lcc-msm8660.c
[ ... ]
> +static const struct clk_parent_data lcc_pxo_pll4[] = {
> +	{ .fw_name = "pxo", .name = "pxo_board" },
> +	{ .fw_name = "pll4_vote", .name = "pll4_vote" },
> +};

[Severity: High]
Does this correctly reference the local PLL4?

The driver registers the local PLL simply as "pll4" (via clk_pll pll4),
and unlike MSM8960, the MSM8660 platform doesn't appear to export a
"pll4_vote" clock from the global clock controller.

Because the clock framework cannot find "pll4_vote", will parent
resolution fail and leave the essential LPASS root clocks orphaned?

Should this refer to the local PLL using .hw = &pll4.clkr.hw or
.name = "pll4" instead?

[ ... ]
> +#define CLK_AIF_OSR_SRC(prefix, _ns, _md)			\
> +static struct clk_rcg prefix##_osr_src = {			\
> +	.ns_reg = _ns,						\
[ ... ]
> +#define CLK_AIF_OSR_CLK(prefix, _ns, hr, en_bit)		\
> +static struct clk_branch prefix##_osr_clk = {			\
> +	.halt_reg = hr,						\
> +	.halt_bit = 1,						\
> +	.halt_check = BRANCH_HALT_ENABLE,			\
> +	.clkr = {						\
> +		.enable_reg = _ns,				\
[ ... ]
> +CLK_AIF_OSR_SRC(mi2s, 0x48, 0x4c)
> +CLK_AIF_OSR_CLK(mi2s, 0x48, 0x50, 17)

[Severity: High]
This is a pre-existing issue, but does sharing the ns_reg (e.g., 0x48)
between the RCG clock and Branch clock expose a race condition?

The RCG clock's set_rate callback (__clk_rcg_set_rate) performs a
non-atomic read-modify-write on this register protected only by the
global CCF prepare_lock mutex.

Concurrently, a branch clock's enable callback (clk_enable_regmap)
performs an atomic regmap_update_bits on the same register protected
by the global CCF enable_lock spinlock.

Since these two locks don't serialize against each other, can an
interrupt-driven clk_enable preempt the clk_set_rate thread between
its read and write steps? 

If clk_set_rate writes its stale value back, it could revert the
branch clock's hardware enable state, potentially triggering a fatal
external abort on unclocked register access.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780197511.git.github.com@herrie.org?part=3