Re: [PATCH v4 4/4] clk: qcom: Add Global clock controller support on Qualcomm Shikra SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Clock parent data structures incorrectly map `P_GPLL9_OUT_MAIN` to the raw PLL `&gpll9.clkr.hw` instead of its post-divider `&gpll9_out_main.clkr.hw`.
- [Medium] The clock `gcc_camss_tfe_1_clk_src` is missing the `CLK_SET_RATE_PARENT` flag, whereas its sibling clock `gcc_camss_tfe_0_clk_src` has it.
--

commit 4a831d5485683b422aed6cd3f5dbf1ba131e85cc
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>
Subject: clk: qcom: Add Global clock controller support on Qualcomm Shikra SoC

This commit adds support for the Global Clock Controller (GCC) on the
Qualcomm Shikra SoC. It registers the necessary clocks, PLLs, dividers,
and resets for the device.

> diff --git a/drivers/clk/qcom/gcc-shikra.c b/drivers/clk/qcom/gcc-shikra.c
> new file mode 100644
> index 0000000000000..d5222756f214c
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-shikra.c

[ ... ]

> +static const struct parent_map gcc_parent_map_7[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_GPLL0_OUT_EARLY, 1 },
> +	{ P_GPLL8_OUT_EARLY, 2 },
> +	{ P_GPLL10_OUT_MAIN, 3 },
> +	{ P_GPLL8_OUT_MAIN, 4 },
> +	{ P_GPLL9_OUT_MAIN, 5 },
> +	{ P_GPLL3_OUT_EARLY, 6 },
> +};
> +
> +static const struct clk_parent_data gcc_parent_data_7[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &gpll0.clkr.hw },
> +	{ .hw = &gpll8.clkr.hw },
> +	{ .hw = &gpll10.clkr.hw },
> +	{ .hw = &gpll8_out_main.clkr.hw },
> +	{ .hw = &gpll9.clkr.hw },

[Severity: High]
Should this map to the post-divider output gpll9_out_main.clkr.hw instead of
the raw PLL gpll9.clkr.hw?

The corresponding parent map gcc_parent_map_7 declares index 5 as
P_GPLL9_OUT_MAIN, but the data array maps it to the raw VCO output. This same
mismatch appears in gcc_parent_data_8, gcc_parent_data_9, and
gcc_parent_data_14. Using the wrong parent hardware clock might cause the
common clock framework to compute incorrect frequencies for downstream
consumers.

[ ... ]

> +static struct clk_rcg2 gcc_camss_tfe_1_clk_src = {
> +	.cmd_rcgr = 0x52024,
> +	.mnd_width = 8,
> +	.hid_width = 5,
> +	.parent_map = gcc_parent_map_8,
> +	.freq_tbl = ftbl_gcc_camss_tfe_0_clk_src,
> +	.hw_clk_ctrl = true,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "gcc_camss_tfe_1_clk_src",
> +		.parent_data = gcc_parent_data_8,
> +		.num_parents = ARRAY_SIZE(gcc_parent_data_8),
> +		.ops = &clk_rcg2_shared_ops,

[Severity: Medium]
Is the CLK_SET_RATE_PARENT flag intentionally omitted here?

The sibling clock gcc_camss_tfe_0_clk_src explicitly sets the
CLK_SET_RATE_PARENT flag in its init data. Without this flag,
gcc_camss_tfe_1_clk_src might be unable to propagate rate changes to its
parent clock, potentially leading to frequency mismatches for the TFE_1 block.

> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-shikra-gcc-rpmcc-clks-v4-0-a0a7bfb44455@oss.qualcomm.com?part=4