Re: [PATCH 6/6] clk: qcom: gpucc: Add Nord graphics clock controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect frequency table reused for gpu_2_cc_hub_clk_src.
- [High] Missing resets in gpu_cc_nord_resets leading to potential PLL corruption, and missing AHB divider in GPU2CC.
- [Medium] Mismatched clock halt_check flags between GPUCC and GPU2CC controllers.
--

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

clk: qcom: gpucc: Add Nord graphics clock controller support

Add support for the GPU clock controllers (GPUCC) on the Qualcomm
Nord platform. The platform includes two GPU clock controller instances,
GPUCC and GPU2CC, which provide clocks required for the graphics subsystem.

> diff --git a/drivers/clk/qcom/gpu2cc-nord.c b/drivers/clk/qcom/gpu2cc-nord.c
> new file mode 100644
> index 0000000000000..d1baf019704c4
> --- /dev/null
> +++ b/drivers/clk/qcom/gpu2cc-nord.c
[ ... ]
> +static struct clk_rcg2 gpu_2_cc_hub_clk_src = {
> +	.cmd_rcgr = 0x91a8,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = gpu_2_cc_parent_map_2,
> +	.freq_tbl = ftbl_gpu_2_cc_ff_clk_src,

[Severity: High]
Is it intentional to use ftbl_gpu_2_cc_ff_clk_src for the hub clock
frequency table?

Could this cause incorrect frequency configurations, such as using the
ff clock's frequency settings instead of a dedicated hub frequency table?

> +	.hw_clk_ctrl = true,
> +	.clkr.hw.init = &(const struct clk_init_data) {
[ ... ]
> +static struct clk_branch gpu_2_cc_ahb_clk = {
> +	.halt_reg = 0x90cc,
> +	.halt_check = BRANCH_HALT_VOTED,

[Severity: Medium]
Does this halt_check mismatch with the equivalent clock in gpucc?

This branch uses BRANCH_HALT_VOTED, but looking at gpu_cc_ahb_clk in
gpucc-nord.c, it uses BRANCH_HALT_DELAY.

Could an incorrect halt check cause the common clock framework to improperly
wait for clock status changes during enable/disable operations?

> +	.clkr = {
> +		.enable_reg = 0x90cc,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "gpu_2_cc_ahb_clk",
> +			.parent_hws = (const struct clk_hw*[]) {
> +				&gpu_2_cc_hub_clk_src.clkr.hw,

[Severity: High]
Are we missing the hub divider block in the parent chain here?

The parent is set directly to gpu_2_cc_hub_clk_src, whereas the equivalent
gpu_cc_ahb_clk in gpucc-nord.c is parented to a gpu_cc_hub_div_clk_src
divider block.

> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
[ ... ]
> diff --git a/drivers/clk/qcom/gpucc-nord.c b/drivers/clk/qcom/gpucc-nord.c
> new file mode 100644
> index 0000000000000..407cf7e5ad437
> --- /dev/null
> +++ b/drivers/clk/qcom/gpucc-nord.c
[ ... ]
> +static const struct qcom_reset_map gpu_cc_nord_resets[] = {
> +	[GPU_CC_ACD_BCR] = { 0x92f8 },
> +	[GPU_CC_CB_BCR] = { 0x9340 },
> +	[GPU_CC_CX_BCR] = { 0x90e4 },
> +	[GPU_CC_FAST_HUB_BCR] = { 0x9384 },
> +	[GPU_CC_GFX3D_AON_BCR] = { 0x91ac },
> +	[GPU_CC_GX_BCR] = { 0x9058 },
> +	[GPU_CC_XO_BCR] = { 0x9000 },
> +};

[Severity: High]
Are we missing entries for GPU_CC_FF_BCR and GPU_CC_GMU_BCR in this list?

Since qcom_reset_map uses sparse static array initialization, any
uninitialized IDs below the maximum enum value will default to 0x0.

If a client attempts to assert these missing resets, could it inadvertently
write to offset 0x0 and corrupt the gpu_cc_pll0 state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-nords_mm_v1-v1-0-860c84539804@oss.qualcomm.com?part=6