Re: [PATCH v2 5/5] clk: qcom: Add support for Qualcomm GPU Clock Controller on Shikra

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Device Tree ABI Regression / Broken Probe
- [High] Missing PM Runtime Support / Hardware Access Fault
- [Medium] Global State Mutation / State Pollution
--

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

clk: qcom: Add support for Qualcomm GPU Clock Controller on Shikra

The Qualcomm Shikra GPU clock controller is similar to QCM2290 GPUCC
hardware block, with minor differences. Drop modelling of critical
clocks and keep them enabled at probe time, update the QCM2290 GPUCC
driver to align with the latest common qcom_cc_probe() model. Update the
GDSC *_wait_val and flags which are applicable for both QCM2290 and Shikra.

> diff --git a/drivers/clk/qcom/gpucc-qcm2290.c b/drivers/clk/qcom/gpucc-qcm2290.c
> index dc369dff882e6..296afcbfe0c9e 100644
> --- a/drivers/clk/qcom/gpucc-qcm2290.c
> +++ b/drivers/clk/qcom/gpucc-qcm2290.c
[ ... ]
>  static struct clk_regmap *gpu_cc_qcm2290_clocks[] = {
> -	[GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr,
>  	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
>  	[GPU_CC_CX_GFX3D_CLK] = &gpu_cc_cx_gfx3d_clk.clkr,
>  	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
>  	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,
> -	[GPU_CC_CXO_AON_CLK] = &gpu_cc_cxo_aon_clk.clkr,
>  	[GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr,
>  	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
>  	[GPU_CC_GX_GFX3D_CLK] = &gpu_cc_gx_gfx3d_clk.clkr,

[Severity: Critical]
Does removing these clocks from the array break the established Device Tree ABI
for qcom,qcm2290-gpucc?

Because their indices in the array are now uninitialized, the clock provider's
qcom_cc_clk_hw_get() will return NULL, causing dependent drivers referencing
these clocks to fail to probe.

[ ... ]
> @@ -359,55 +366,22 @@ static const struct qcom_cc_desc gpu_cc_qcm2290_desc = {
>  	.config = &gpu_cc_qcm2290_regmap_config,
>  	.clks = gpu_cc_qcm2290_clocks,
>  	.num_clks = ARRAY_SIZE(gpu_cc_qcm2290_clocks),
>  	.resets = gpu_cc_qcm2290_resets,
>  	.num_resets = ARRAY_SIZE(gpu_cc_qcm2290_resets),
>  	.gdscs = gpu_cc_qcm2290_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gpu_cc_qcm2290_gdscs),
> +	.driver_data = &gpu_cc_qcm2290_driver_data,
>  };

[Severity: High]
Does this description need to set .use_rpm = true?

Prior to this patch, the driver explicitly enabled PM runtime and called
pm_runtime_resume_and_get(). By omitting .use_rpm = true here,
qcom_cc_really_probe() will skip the PM runtime resume sequence.

Accessing unclocked or unpowered registers to configure PLLs and critical
CBCRs could trigger a synchronous external abort.

[ ... ]
>  static int gpu_cc_qcm2290_probe(struct platform_device *pdev)
>  {
> -	struct regmap *regmap;
> -	int ret;
[ ... ]
> -out_pm_runtime_put:
> -	pm_runtime_put_sync(&pdev->dev);
> +	if (device_is_compatible(&pdev->dev, "qcom,shikra-gpucc"))
> +		gpu_cc_gx_gfx3d_clk_src.freq_tbl = ftbl_gpu_cc_gx_gfx3d_clk_src_shikra;
>  
> -	return 0;
> +	return qcom_cc_probe(pdev, &gpu_cc_qcm2290_desc);
>  }

[Severity: Medium]
Does this code mutate static module data?

Because gpu_cc_gx_gfx3d_clk_src is a static global structure, dynamically
assigning a new frequency table here will permanently pollute the shared table
for the entire driver module.

Could this cause issues during driver unbind/rebind cycles?

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