Re: [PATCH v4 11/15] clk: qcom: Add Global Clock Controller driver for Kuno

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The CLK_KUNO_GCC Kconfig entry is missing `select QCOM_GDSC`, which leads to a silent runtime probe failure if QCOM_GDSC is not enabled.
- [High] The SDCC (Secure Digital) apps clock uses `clk_rcg2_ops` instead of `clk_rcg2_floor_ops`, which can cause SD card overclocking and data corruption.
--

commit e6e05944bec5f5d0474370120050ea04e9b3eb87
Author: Hardeep Sharma <hardeep.sharma@oss.qualcomm.com>

clk: qcom: Add Global Clock Controller driver for Kuno
    
Add the global clock controller (GCC) driver for the Qualcomm Kuno SoC,
providing the PLLs, root clock generators, gate/branch clocks and resets
used by the peripheral devices such as UART, SPI, I2C, USB, SD, PCIe and
Ethernet.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index e609a7819e072..1f31bf09de0cd 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig

[ ... ]

> @@ -212,6 +212,16 @@ config CLK_KAANAPALI_VIDEOCC
>  	  Say Y if you want to support video devices and functionality such as
>  	  video encode/decode.
>  
> +config CLK_KUNO_GCC
> +	tristate "Kuno Global Clock Controller"
> +	depends on ARM || COMPILE_TEST
> +	default ARCH_QCOM

[Severity: High]
Should this config also select QCOM_GDSC?

If CONFIG_QCOM_GDSC is not enabled, the static inline stub for gdsc_register()
in gdsc.h unconditionally returns -ENOSYS. When qcom_cc_really_probe() calls
gdsc_register() for the GDSCs declared in gcc_kuno_desc, it receives -ENOSYS
and aborts the probe entirely. This can leave dependent peripherals without
clocks or resets at runtime.

> +	help
> +	  Support for the global clock controller (GCC) on Kuno devices.
> +	  Say Y if you want to use peripheral devices such as UART, SPI,
> +	  I2C, USB, SD, PCIe and Ethernet on the Kuno SoC. This clock
> +	  controller supplies the clocks and resets to those peripherals.
> +
>  config CLK_NORD_DISPCC
>  	tristate "Nord Display Clock Controller"
>  	depends on ARM64 || COMPILE_TEST

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

[ ... ]

> +static struct clk_rcg2 gcc_sdcc4_apps_clk_src = {
> +	.cmd_rcgr = 0x6a01c,
> +	.mnd_width = 8,
> +	.hid_width = 5,
> +	.parent_map = gcc_parent_map_0,
> +	.freq_tbl = ftbl_gcc_sdcc4_apps_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "gcc_sdcc4_apps_clk_src",
> +		.parent_data = gcc_parent_data_0,
> +		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
> +		.ops = &clk_rcg2_ops,

[Severity: High]
Should gcc_sdcc4_apps_clk_src use clk_rcg2_floor_ops instead of clk_rcg2_ops?

When the MMC core requests a clock rate for the SD card, the standard
clk_rcg2_ops uses qcom_find_freq(), which rounds up to the nearest available
frequency greater than or equal to the requested rate. If the requested rate
isn't an exact match in the frequency table, it will exceed the maximum safe
frequency mandated by the SD specification, which can cause SD card
overclocking and potential data corruption.

> +	},
> +};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-kuno-soc-support-v4-0-dd34f235fc1a@oss.qualcomm.com?part=11