Re: [PATCH v4 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 `clk_alpha_pll_postdiv` clock definition for `cam_bist_mclk_cc_pll0_out_even` causes MCLKs to run at half their expected frequency.
- [High] Missing `.use_rpm = true` flag in `cam_cc_eliza_desc` in `camcc-eliza.c`.
--

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

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>

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

Will mapping both P_CAM_BIST_MCLK_CC_PLL0_OUT_EVEN and
P_CAM_BIST_MCLK_CC_PLL0_OUT_MAIN to the exact same hardware object cause
frequency miscalculations?

Hardware PLLs physically divide their OUT_EVEN output by 2. Without an
explicit postdiv structure modeled in the clock tree, the Common Clock
Framework might treat both inputs as supplying the undivided 960 MHz VCO
rate.

Since the ftbl_cam_bist_mclk_cc_mclk0_clk_src table configures the RCG
assuming a 960 MHz parent to yield 24 MHz, but the hardware actually
provides 480 MHz on the even output, could this result in a 12 MHz
output and break downstream camera operations?

[ ... ]
> 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 this struct include the .use_rpm = true flag, similar to how it is
configured for cam_bist_mclk_cc_eliza_desc in this same patch?

If this flag is omitted, the driver might attempt to read and write hardware
registers during probe without voting for the required CX and MX power
domains to be active. Can this lead to synchronous external aborts during
probe?

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