Re: [PATCH v5 6/7] clk: qcom: camcc: Add support for camera clock controller for Eliza

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `cam_cc_eliza_desc` structure in `camcc-eliza.c` is missing the `.use_rpm = true` flag, preventing runtime PM enablement.
- [High] Missing `clk_alpha_pll_postdiv` definition for `cam_bist_mclk_cc_pll0_out_even` causes incorrect rate calculation and hardware configuration.
- [Low] Inconsistent 'default m if ARCH_QCOM' across related Eliza clock configs.
--


commit 5422290959f8d7c156105b5bd2ca77def397be5f
Author: Taniya Das <taniya.das@oss.qualcomm.com>

clk: qcom: camcc: Add support for camera clock controller for Eliza

Add support for the Camera Clock Controller (CAMCC) on the Eliza
platform. The CAMCC block on Eliza includes both the primary camera
clock controller and the Camera BIST clock controller.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 5641929a3da49..cb44a9972a6a6 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -19,6 +19,17 @@ menuconfig COMMON_CLK_QCOM
>  
>  if COMMON_CLK_QCOM
>  
> +config CLK_ELIZA_CAMCC
> +	tristate "Eliza Camera Clock Controller"
> +	depends on ARM64 || COMPILE_TEST
> +	select CLK_ELIZA_GCC
> +	default m if ARCH_QCOM

[Severity: Low]
This isn't a bug, but should this default match the adjacent
CLK_ELIZA_DISPCC config, which does not specify a default?

> +	help
> +	  Support for the camera clock controller on Qualcomm Technologies, Inc
> +	  Eliza devices.
> +	  Say Y if you want to support camera devices and functionality such as
> +	  capturing pictures.
> +
>  config CLK_ELIZA_DISPCC
>  	tristate "Eliza Display Clock Controller"
>  	depends on ARM64 || COMPILE_TEST

[ ... ]

> diff --git a/drivers/clk/qcom/cambistmclkcc-eliza.c b/drivers/clk/qcom/cambistmclkcc-eliza.c
> new file mode 100644
> index 0000000000000..b65e224a1be90
> --- /dev/null
> +++ b/drivers/clk/qcom/cambistmclkcc-eliza.c

[ ... ]

> +static const struct parent_map cam_bist_mclk_cc_parent_map_0[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_CAM_BIST_MCLK_CC_PLL0_OUT_EVEN, 3 },
> +	{ P_CAM_BIST_MCLK_CC_PLL0_OUT_MAIN, 5 },
> +};
> +
> +static const struct clk_parent_data cam_bist_mclk_cc_parent_data_0[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &cam_bist_mclk_cc_pll0.clkr.hw },
> +	{ .hw = &cam_bist_mclk_cc_pll0.clkr.hw },

[Severity: High]
Does this code map P_CAM_BIST_MCLK_CC_PLL0_OUT_EVEN directly to the main
PLL hardware instead of a dedicated clk_alpha_pll_postdiv structure? Without
a postdiv structure, the Common Clock Framework will evaluate the EVEN output
rate identically to the MAIN output (960 MHz) instead of its physically
divided rate (480 MHz).

> +};

[ ... ]

> +static const struct freq_tbl ftbl_cam_bist_mclk_cc_mclk0_clk_src[] = {
> +	F(19200000, P_BI_TCXO, 1, 0, 0),
> +	F(24000000, P_CAM_BIST_MCLK_CC_PLL0_OUT_EVEN, 10, 1, 4),

[Severity: High]
With the missing postdiv structure above, this frequency table uses a divisor
of 10 to target 24 MHz based on a 960 MHz parent. Since the hardware
actually supplies 480 MHz on the physical EVEN output, applying the same
divisor will result in a physical output of 12 MHz, halving the MCLK
frequency. Should a clk_alpha_pll_postdiv be added and this frequency table
updated to use the correct divisor?

> +	F(68571429, P_CAM_BIST_MCLK_CC_PLL0_OUT_MAIN, 14, 0, 0),
> +	{ }
> +};

[ ... ]

> diff --git a/drivers/clk/qcom/camcc-eliza.c b/drivers/clk/qcom/camcc-eliza.c
> new file mode 100644
> index 0000000000000..b96a2a58b32a7
> --- /dev/null
> +++ b/drivers/clk/qcom/camcc-eliza.c

[ ... ]

> +static const struct qcom_cc_desc cam_cc_eliza_desc = {
> +	.config = &cam_cc_eliza_regmap_config,
> +	.clks = cam_cc_eliza_clocks,
> +	.num_clks = ARRAY_SIZE(cam_cc_eliza_clocks),
> +	.resets = cam_cc_eliza_resets,
> +	.num_resets = ARRAY_SIZE(cam_cc_eliza_resets),
> +	.gdscs = cam_cc_eliza_gdscs,
> +	.num_gdscs = ARRAY_SIZE(cam_cc_eliza_gdscs),
> +	.driver_data = &cam_cc_eliza_driver_data,

[Severity: High]
Is the .use_rpm = true flag missing from this structure? In the qcom_cc
framework, this flag instructs qcom_cc_probe() to enable runtime PM.
Omitting it means the power domain remains off, which leads to unclocked
register accesses and causes a Data Abort or bus stall when the driver
attempts to access the registers or GDSCs during probe.

> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-eliza_mm_cc_v2-v5-0-a1d125619a5a@oss.qualcomm.com?part=6