Re: [PATCH v2 02/19] clk: qcom: Add global clock controller driver for MSM8952

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: qcom: gp1/2/3_clk_src CCF parent incorrectly mapped to gpll0_vote instead of xo
- [High] clk: qcom: reset map targets CBCR enable bits instead of BCR reset registers
--

commit f6425874e2d9adc87ee0fd906bb71cd217405222
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

clk: qcom: Add global clock controller driver for MSM8952

This commit adds the global clock controller (GCC) driver for the Qualcomm
MSM8952 SoC. It provides clocks, resets, and power domains needed for various
hardware components on the device.

Link: https://android.googlesource.com/kernel/msm.git/+/refs/tags/android-wear-7.0.0_r0.3/drivers/clk/qcom/clock-gcc-8952.c

diff --git a/drivers/clk/qcom/gcc-msm8952.c b/drivers/clk/qcom/gcc-msm8952.c
--- a/drivers/clk/qcom/gcc-msm8952.c
+++ b/drivers/clk/qcom/gcc-msm8952.c
[ ... ]
> static struct clk_rcg2 gp1_clk_src = {
> 	.cmd_rcgr = 0x08004,
> 	.mnd_width = 8,
> 	.hid_width = 5,
> 	.parent_map = gcc_xo_map,
> 	.freq_tbl = ftbl_gcc_gp1_3_clk,
> 	.clkr.hw.init = &(struct clk_init_data){
> 		.name = "gp1_clk_src",
> 		.parent_hws = (const struct clk_hw *[]) {
> 			&gpll0_vote.hw,
> 		},

[Severity: High]
Does this code map to the wrong parent hardware clock? The parent_map is set
to gcc_xo_map (which implies hardware source 0, typically XO), but the CCF
parent_hws array points to &gpll0_vote.hw (800MHz) instead of the XO
reference.

This causes the Common Clock Framework to report the parent rate as 800MHz
while the hardware operates at 19.2MHz. Consequently, downstream drivers
configuring these clocks may compute incorrect hardware dividers, leading to
severe underclocking, and it will keep gpll0_vote enabled unnecessarily.

> 		.num_parents = 1,
> 		.ops = &clk_rcg2_ops,
> 	},
> };
> 
> static struct clk_rcg2 gp2_clk_src = {
> 	.cmd_rcgr = 0x09004,
> 	.mnd_width = 8,
> 	.hid_width = 5,
> 	.parent_map = gcc_xo_map,
> 	.freq_tbl = ftbl_gcc_gp1_3_clk,
> 	.clkr.hw.init = &(struct clk_init_data){
> 		.name = "gp2_clk_src",
> 		.parent_hws = (const struct clk_hw *[]) {
> 			&gpll0_vote.hw,
> 		},

[Severity: High]
Does the same parent mismatch apply here to gp2_clk_src as well?

> 		.num_parents = 1,
> 		.ops = &clk_rcg2_ops,
> 	},
> };
> 
> static struct clk_rcg2 gp3_clk_src = {
> 	.cmd_rcgr = 0x0a004,
> 	.mnd_width = 8,
> 	.hid_width = 5,
> 	.parent_map = gcc_xo_map,
> 	.freq_tbl = ftbl_gcc_gp1_3_clk,
> 	.clkr.hw.init = &(struct clk_init_data){
> 		.name = "gp3_clk_src",
> 		.parent_hws = (const struct clk_hw *[]) {
> 			&gpll0_vote.hw,
> 		},

[Severity: High]
And similarly, does gp3_clk_src also point to gpll0_vote.hw instead of xo?

[ ... ]
> static const struct qcom_reset_map gcc_msm8952_resets[] = {
> 	[RST_CAMSS_MICRO_BCR]		= { 0x56008 },
> 	[RST_USB_HS_BCR]		= { 0x41000 },
> 	[RST_QUSB2_PHY_BCR]		= { 0x4103c },
> 	[RST_USB_HS_PHY_CFG_AHB_CBCR]	= { 0x41030 },

[Severity: High]
Are we targeting the Clock Branch Control Register (CBCR) here instead of the
Block Control Register (BCR)?

By omitting .bit, the reset framework defaults to bit 0, which is the clock
enable bit for CBCRs. When a subsystem requests a block reset, the driver
will toggle the clock enable bit instead of actually resetting the hardware
block. This corrupts the clock's enabled/disabled state behind the
framework's back and fails to reset the hardware.

> 	[RST_USB2_HS_PHY_ONLY_BCR]	= { 0x41034 },
> 	[RST_USB_FS_BCR]		= { 0x3f000 },
> 	[RST_CAMSS_CSI1PIX_CBCR]	= { 0x4f058 },
> 	[RST_CAMSS_CSI_VFE1_CBCR]	= { 0x58074 },
> 	[RST_CAMSS_VFE1_CBCR]		= { 0x5805c },
> 	[RST_CAMSS_CPP_CBCR]		= { 0x5803c },

[Severity: High]
Do these CAMSS entries also incorrectly target CBCR registers instead of their
respective BCR hardware reset registers? Similar to the USB reset above, this
could break hardware state recovery for fundamental blocks by toggling clock
enablements instead.

> 	[RST_MSS_BCR]			= { 0x71000 },
> 	[RST_MDSS_BCR]			= { 0x4d074 },
> };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-msm8952-initial-support-v2-0-798c2602dde1@mainlining.org?part=2