Re: [PATCH v10 2/6] clk: qcom: Add Global Clock Controller (GCC) driver for SM7250

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: qcom: rate configuration failure for gcc_cpuss_ahb_clk_src due to src mismatch
- [Low] kconfig: missing default ARCH_QCOM in SM_GCC_7250
--

commit d75ec50c6226017a3fb6c285ecf2e67ec2c78bb0
Author: Sreeshankar K <sreeshankar0910@gmail.com>
Subject: clk: qcom: Add Global Clock Controller (GCC) driver for SM7250

Add support for the global clock controller found on SM7250 based devices.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -1747,6 +1747,15 @@ config SM_GCC_7150
>  	  Say Y if you want to use peripheral devices such as UART,
>  	  SPI, I2C, USB, SD/UFS, PCIe etc.
>  
> +config SM_GCC_7250
> +	tristate "SM7250 Global Clock Controller"
> +	depends on ARM64 || COMPILE_TEST
> +	select QCOM_GDSC

[Severity: Low]
Is this Kconfig entry missing a default ARCH_QCOM clause? Peer configs
typically include it so the driver is built by default for Qualcomm targets.
Without it, this driver will be silently excluded from default builds.

> +	help
> +	  Support for the global clock controller on SM7250 devices.
> +	  Say Y if you want to use peripheral devices such as UART,
> +	  SPI, I2C, USB, SD/UFS etc.
> +
>  config SM_GCC_MILOS

[ ... ]

> diff --git a/drivers/clk/qcom/gcc-sm7250.c b/drivers/clk/qcom/gcc-sm7250.c
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm7250.c
> @@ -0,0 +1,2358 @@

[ ... ]

> +static const struct freq_tbl ftbl_gcc_cpuss_ahb_clk_src[] = {
> +	F(19200000, DT_BI_TCXO_AO, 1, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 gcc_cpuss_ahb_clk_src = {
> +	.cmd_rcgr = 0x48010,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = gcc_parent_map_0,
> +	.freq_tbl = ftbl_gcc_cpuss_ahb_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data){
> +		.name = "gcc_cpuss_ahb_clk_src",
> +		.parent_data = gcc_parent_data_0_ao,
> +		.num_parents = ARRAY_SIZE(gcc_parent_data_0_ao),

[Severity: Medium]
Does ftbl_gcc_cpuss_ahb_clk_src use the wrong enum for its src field? It
currently uses the device tree index DT_BI_TCXO_AO (1) instead of the
hardware parent P_BI_TCXO (0).

Since the clock binds this frequency table with gcc_parent_map_0, which maps
the first entry to P_BI_TCXO (0), and bounds the iteration with a num_parents
of 1, a src of 1 will not match map[0].src (which is 0).

Could this cause the parent lookup to fail with -ENOENT and break rate
configuration for the CPUSS AHB clock?

> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_rcg2_ops,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015051926.13752-2-sreeshankar0910@gmail.com?part=2