Re: [PATCH v5 2/3] clk: qcom: camcc-glymur: Add camera clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect register offsets for CSI4 clocks due to a copy-paste error.
- [High] Missing `cam_cc_pll2_out_even` struct definition resulting in incorrect CCF frequency calculation for EVEN PLL output.
--

commit 4ff3f366f401331dcb96c0eb65a3236a34345740
Author: Jagadeesh Kona <jagadeesh.kona@oss.qualcomm.com>

clk: qcom: camcc-glymur: Add camera clock controller driver

Add support for the camera clock controller for camera clients
to be able to request for camcc clocks on Glymur platform.

> diff --git a/drivers/clk/qcom/camcc-glymur.c b/drivers/clk/qcom/camcc-glymur.c
> new file mode 100644
> index 0000000000000..a80f05b7bc484
> --- /dev/null
> +++ b/drivers/clk/qcom/camcc-glymur.c
[ ... ]
> @@ -0,0 +1,2280 @@
[ ... ]
> +static const struct parent_map cam_cc_parent_map_1[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_CAM_CC_PLL2_OUT_EVEN, 3 },
> +	{ P_CAM_CC_PLL2_OUT_MAIN, 5 },
> +};
> +
> +static const struct clk_parent_data cam_cc_parent_data_1[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &cam_cc_pll2.clkr.hw },
> +	{ .hw = &cam_cc_pll2.clkr.hw },

[Severity: High]
Does this code miss the cam_cc_pll2_out_even struct definition?

Index 1 should point to &cam_cc_pll2_out_even.clkr.hw to represent the divided
EVEN output of the PLL. The duplicated pointer results in the Common Clock
Framework seeing the raw PLL rate without division. When a clock relying on this
parent map switches to the EVEN hardware mux, the framework might calculate the
downstream clock is running at twice its actual hardware frequency.

> +};
[ ... ]
> +static struct clk_rcg2 cam_cc_csi4phytimer_clk_src = {
> +	.cmd_rcgr = 0x15124,

[Severity: High]
Are these register offsets correct for the CSI4 PHY timer clock source?

On identical or highly similar reference hardware designs, offsets 0x15124,
0x15250, and 0x15254 map to the CSI2 PHY branch and timer clocks, while
CSI4 is typically mapped to 0x1538c, 0x154b8, and 0x154bc.

Writing configuration to these offsets for CSI4 might incorrectly modify CSI2
clock registers, preventing the CSI4 camera interface from being clocked
properly and potentially inducing unintended behavior on the CSI2 IP block.

> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = cam_cc_parent_map_0,
> +	.freq_tbl = ftbl_cam_cc_csi0phytimer_clk_src,
[ ... ]
> +static struct clk_branch cam_cc_csi4phytimer_clk = {
> +	.halt_reg = 0x15250,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x15250,

[Severity: High]
Could this offset be incorrect as well? As mentioned above, 0x15250 usually
corresponds to the CSI2 PHY timer branch clock.

> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "cam_cc_csi4phytimer_clk",
[ ... ]
> +static struct clk_branch cam_cc_csiphy4_clk = {
> +	.halt_reg = 0x15254,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x15254,

[Severity: High]
Is this the correct offset for the CSI4 PHY branch clock? It seems this might
also be pointing to the CSI2 PHY branch clock.

> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "cam_cc_csiphy4_clk",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-glymur_camcc-v5-0-a321df74b1a1@oss.qualcomm.com?part=2