Re: [PATCH v4 2/4] clk: qcom: add Global Clock controller (GCC) driver for IPQ9650 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `gcc_qpic_sleep_clk` branch clock is missing its parent clock initialization in `clk_init_data`.
- [Medium] The `gpll4` and `gcc_pcnoc_bfdcd_clk_src` clocks use the `CLK_IS_CRITICAL` flag, which contradicts the explicit comments stating to use `CLK_IGNORE_UNUSED`.
--

commit 066e3207c3655c5fa209013fda4d22eaa8774c6c
Author: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
clk: qcom: add Global Clock controller (GCC) driver for IPQ9650 SoC

This commit adds support for the global clock controller found on the 
IPQ9650 SoC devices.

> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-ipq9650.c

[ ... ]

> +static struct clk_alpha_pll gpll4 = {
> +	.offset = 0x22000,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID],
> +	.clkr = {
> +		.enable_reg = 0xb000,
> +		.enable_mask = BIT(2),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "gpll4",
> +			.parent_data = &gcc_parent_data_xo,
> +			.num_parents = 1,
> +			/*
> +			 * There are no consumers for this GPLL in kernel yet,
> +			 * (will be added soon), so the clock framework
> +			 * disables this source. But some of the clocks
> +			 * initialized by boot loaders uses this source. So we
> +			 * need to keep this clock ON. Add the
> +			 * CLK_IGNORE_UNUSED flag so the clock will not be
> +			 * disabled. Once the consumer in kernel is added, we
> +			 * can get rid of this flag.
> +			 */
> +			.flags = CLK_IS_CRITICAL,

The comment explicitly mentions adding the CLK_IGNORE_UNUSED flag, but the
code sets CLK_IS_CRITICAL instead. Should this be CLK_IGNORE_UNUSED to match
the description and avoid unconditionally forcing the clock on?

[ ... ]

> +static struct clk_rcg2 gcc_pcnoc_bfdcd_clk_src = {
> +	.cmd_rcgr = 0x31004,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = gcc_parent_map_0,
> +	.freq_tbl = ftbl_gcc_pcnoc_bfdcd_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "gcc_pcnoc_bfdcd_clk_src",
> +		.parent_data = gcc_parent_data_0,
> +		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
> +		/*
> +		 * There are no consumers for this source in kernel yet,
> +		 * (will be added soon), so the clock framework
> +		 * disables this source. But some of the clocks
> +		 * initialized by boot loaders uses this source. So we
> +		 * need to keep this clock ON. Add the
> +		 * CLK_IGNORE_UNUSED flag so the clock will not be
> +		 * disabled. Once the consumer in kernel is added, we
> +		 * can get rid of this flag.
> +		 */
> +		.flags = CLK_IS_CRITICAL,

There is a similar mismatch here where the comment indicates CLK_IGNORE_UNUSED
but the implementation sets CLK_IS_CRITICAL.

[ ... ]

> +static struct clk_branch gcc_qpic_sleep_clk = {
> +	.halt_reg = 0x32018,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x32018,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "gcc_qpic_sleep_clk",
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};

Does this clock need parent initialization? Other sleep branch clocks in this
driver specify their parent, but this one appears to omit parent_hws and
num_parents. Could this leave the clock unparented in the clock framework?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-ipq9650_boot_to_shell-v4-0-1e87c8326e59@oss.qualcomm.com?part=2