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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect hardware clock mapping for P_CAM_CC_PLL2_OUT_EVEN in cam_cc_parent_data_1.
- [High] Mismatch between parent map and frequency table for cam_cc_xo_clk_src leading to clock configuration failure.
- [High] Inconsistent hardware register offsets for CSI4 clocks.
- [Medium] Cross-Config Consistency Kconfig violation for CLK_GLYMUR_CAMCC.
--

commit 5405dbfa49980ee6b2cd4bb1348b52c906b3f710
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/Kconfig b/drivers/clk/qcom/Kconfig
> index df21ef5ffd686..754c3b823f3f8 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -45,6 +45,17 @@ config CLK_ELIZA_TCSRCC
>  	  Support for the TCSR clock controller on Eliza devices.
>  	  Say Y if you want to use peripheral devices such as USB/PCIe/UFS.
>  
> +config CLK_GLYMUR_CAMCC
> +	tristate "Glymur Camera Clock Controller"
> +	depends on ARM64 || COMPILE_TEST
> +	select CLK_GLYMUR_GCC
> +	default m if ARCH_QCOM

This isn't a bug, but is it intentional to use default m if ARCH_QCOM here?
Existing sibling configurations for the same SoC family (such as
CLK_GLYMUR_DISPCC and CLK_GLYMUR_GPUCC) omit this default.

> +	help
> +	  Support for the camera clock controller on Qualcomm Technologies, Inc

[ ... ]

> diff --git a/drivers/clk/qcom/camcc-glymur.c b/drivers/clk/qcom/camcc-glymur.c
> new file mode 100644
> index 0000000000000..b21e6830a72b4
> --- /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 },

Does this correctly map the EVEN and MAIN outputs? Both are currently mapped
to the undivided &cam_cc_pll2.clkr.hw structure, and it appears the
cam_cc_pll2_out_even post-divider structure is missing from the file
entirely.

Other PLLs like PLL0 and PLL1 have proper post-divider structures defined
for their EVEN outputs. Could this cause the framework to miscalculate
the rate of the EVEN output by treating it as the full MAIN frequency?

> +};

[ ... ]

> +static struct clk_rcg2 cam_cc_xo_clk_src = {
> +	.cmd_rcgr = 0x13948,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = cam_cc_parent_map_6_ao,
> +	.freq_tbl = ftbl_cam_cc_xo_clk_src,

Will this configuration cause rate operations on cam_cc_xo_clk_src to fail?
The frequency table relies on P_BI_TCXO, but the mapped
cam_cc_parent_map_6_ao only contains P_BI_TCXO_AO:

static const struct parent_map cam_cc_parent_map_6_ao[] = {
	{ P_BI_TCXO_AO, 0 },
};

When the clock framework searches for the source, qcom_find_src_index()
might return -ENOENT.

> +	.hw_clk_ctrl = true,

[ ... ]

> +static struct clk_rcg2 cam_cc_csi1phytimer_clk_src = {
> +	.cmd_rcgr = 0x15104,
> +	.mnd_width = 0,
> +	.hid_width = 5,

[ ... ]

> +static struct clk_rcg2 cam_cc_csi4phytimer_clk_src = {
> +	.cmd_rcgr = 0x15124,
> +	.mnd_width = 0,
> +	.hid_width = 5,

Is the .cmd_rcgr offset correct for this clock block?

The address 0x15124 is immediately sequential to the CSI1 RCGR (0x15104),
which suggests this might actually be the CSI2 offset. Additionally, the
corresponding branch clocks for CSI4 use .halt_reg at 0x15250 and
0x15254, which are spaced unusually far from the 0x15124 RCGR.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-glymur_camcc-v3-0-a7196fee2779@oss.qualcomm.com?part=2