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

[Jagadeesh Kona <jagadeesh.kona@oss.qualcomm.com>]

On 5/14/2026 3:24 AM, sashiko-bot@kernel.org wrote:
> 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.
> 

Yes, it is intentional. Existing sibling defconfigs were part of separate arm64
defconfig file, but the recent convention is to enable the defconfig from Kconfig
itself.

>> +	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.
> 

Yes, no need to define separate even and odd outputs here since they are
unused and only pll2 main output is used to derive the clk frequencies.

> 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.
> 

Will fix the frequency table to use P_BI_TCXO_AO src.

>> +	.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.
> 

There are no CSI2,3 clocks supported on Glymur HW and above address is correct
for CSI4 as per HW register layout.

Thanks,
Jagadeesh