Re: [PATCH v2 1/5] clk: qcom: gcc-qcm2290: Drop modelling of critical clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Device Tree ABI breakage causing consumer drivers to fail probing
- [Medium] Inaccurate commit message regarding the previous state of the removed clocks
- [Low] Misuse of `qcom_branch_set_clk_en` macro for an APCS vote register
--

commit fe7153ece7b1b29e7cf0efe410ca1855c6dc922e
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

clk: qcom: gcc-qcm2290: Drop modelling of critical clocks

This patch drops the modeling of certain clocks on the QCM2290 SoC, instead
enabling them unconditionally from probe to simplify PM clock handling in
consumer drivers.

> Drop the modelling of critical GCC clocks on QCM2290 SoC, and keep them
> enabled from probe as per the latest convention. This helps to drop the

[Severity: Medium]
Is this description completely accurate? Several of the removed clocks, such
as gcc_gpu_iref_clk, gcc_video_ahb_clk, and gcc_video_xo_clk, did not have
the CLK_IS_CRITICAL flag set in their init data. 

Could the commit message be clarified to reflect that both critical and
non-critical clocks are being dropped?

> diff --git a/drivers/clk/qcom/gcc-qcm2290.c b/drivers/clk/qcom/gcc-qcm2290.c
> index 6684cab63ae11..937db68a45b17 100644
> --- a/drivers/clk/qcom/gcc-qcm2290.c
> +++ b/drivers/clk/qcom/gcc-qcm2290.c

[ ... ]

> @@ -2775,8 +2642,6 @@ static struct clk_regmap *gcc_qcm2290_clocks[] = {
>  	[GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
>  	[GCC_CAM_THROTTLE_NRT_CLK] = &gcc_cam_throttle_nrt_clk.clkr,
>  	[GCC_CAM_THROTTLE_RT_CLK] = &gcc_cam_throttle_rt_clk.clkr,
> -	[GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr,
> -	[GCC_CAMERA_XO_CLK] = &gcc_camera_xo_clk.clkr,

[Severity: High]
Does removing these clocks completely from the provider array break Device
Tree ABI compatibility? Older device trees specify these clocks in the
'clocks' property of their consumer nodes (e.g., qcom,qcm2290-mdss requests
GCC_DISP_AHB_CLK).

When these consumer drivers boot with a newer kernel using the old DT,
functions like devm_clk_bulk_get_all() might fail with -ENOENT when they
encounter a missing clock index. 

Should these indices remain populated with a dummy clock or a basic AON clock
to prevent consumer driver probe failures?

[ ... ]

> @@ -2990,6 +2848,17 @@ static int gcc_qcm2290_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	/* Keep some clocks always-on */
> +	qcom_branch_set_clk_en(regmap, 0x17008); /* GCC_CAMERA_AHB_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x17028); /* GCC_CAMERA_XO_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x1700c); /* GCC_DISP_AHB_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x1702c); /* GCC_DISP_XO_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x36004); /* GCC_GPU_CFG_AHB_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x36100); /* GCC_GPU_IREF_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x79004); /* GCC_SYS_NOC_CPUSS_AHB_CLK */

[Severity: Low]
Is qcom_branch_set_clk_en() the correct macro for GCC_SYS_NOC_CPUSS_AHB_CLK?
This macro sets the CBCR_CLOCK_ENABLE bit (BIT(0)) in a standard Clock Branch
Control Register. Register 0x79004 is an APCS vote register, not a CBCR.

While it functionally works because this specific clock's vote mask is also
BIT(0), using a CBCR-specific macro for an APCS vote register might be
brittle. 

Should direct regmap_update_bits() be used instead?

> +	qcom_branch_set_clk_en(regmap, 0x17004); /* GCC_VIDEO_AHB_CLK */
> +	qcom_branch_set_clk_en(regmap, 0x17024); /* GCC_VIDEO_XO_CLK */
> +
>  	clk_alpha_pll_configure(&gpll10, regmap, &gpll10_config);
>  	clk_alpha_pll_configure(&gpll11, regmap, &gpll11_config);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-shikra-dispcc-gpucc-v2-0-953f246a0fbb@oss.qualcomm.com?part=1