Re: [PATCH v3 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 2 potential issue(s) to consider:
- [High] Missing `.use_rpm = true` in `cam_cc_eliza_desc` prevents Runtime PM enablement for the Camera CC domain.
- [High] Missing `clk_alpha_pll_postdiv` for `cam_bist_mclk_cc_pll0_out_even` leads to a 50% clock rate reduction in hardware.
--

commit f093d67b6e760a407d12b8dbe60badad11296713
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, which includes both the primary camera clock controller and
the Camera BIST clock controller.

> 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 },
> +};

Is P_CAM_BIST_MCLK_CC_PLL0_OUT_EVEN pointing to the correct hardware
representation here?

Since it points directly to the main PLL (&cam_bist_mclk_cc_pll0.clkr.hw)
instead of a dedicated clk_alpha_pll_postdiv struct, the framework might
calculate the clock rate using the main PLL's 960 MHz instead of the
physical EVEN output's 480 MHz.

This means F(24000000, P_CAM_BIST_MCLK_CC_PLL0_OUT_EVEN, 10, 1, 4) would
evaluate to 24 MHz in software (960 MHz / 10 * 1/4), but might actually
produce 12 MHz in hardware. Does this need a clk_alpha_pll_postdiv defined
for the EVEN output?

[ ... ]

> 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,
> +};

Should .use_rpm = true be set here to enable runtime PM?

Without it, qcom_cc_probe() won't call devm_pm_runtime_enable(). This
could cause clk_pm_runtime_get() to trivially succeed without turning on
the required power domain, which might lead to unclocked hardware accesses.

Note that the cambistmclkcc-eliza_desc structure correctly sets this flag.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-eliza_mm_cc_v2-v3-0-b59c370dc281@oss.qualcomm.com?part=6