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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] gcc_cpuss_gnoc_clk is incorrectly configured with its enable_reg pointing to its CBCR instead of a central vote register, and uses an invalid enable_mask.
- [Low] Unused static constant arrays `gcc_parent_map_5` and `gcc_parent_data_5`.
- [Low] Incorrect space-based indentation in Kconfig block for SM_GCC_7250.
- [Low] The dt-bindings header file 'qcom,gcc-sm7250.h' is included but omitted from the commit tree.
--

--- Patch [1]: [PATCH 1/2] clk: qcom: Add Global Clock Controller (GCC) driver for SM7250 ---
commit 3285603c7b98a0179a26091f983c989773cbb48c
Author: Sreeshankar K <[email protected]>

clk: qcom: Add Global Clock Controller (GCC) driver for SM7250

Add support for the global clock controller found on SM7250
based devices. This should allow most non-multimedia device
drivers to probe and control their clocks.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -1416,6 +1416,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

[Severity: Low]
This isn't a bug, but there appears to be space-based indentation instead
of tabs in this Kconfig block.

> +        select QCOM_GDSC
> +        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, PCIe etc.
> +
[ ... ]
> diff --git a/drivers/clk/qcom/gcc-sm7250.c b/drivers/clk/qcom/gcc-sm7250.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm7250.c
> @@ -0,0 +1,2347 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2026, Sreeshankar K <[email protected]>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-sm7250.h>

[Severity: Low]
Is this header file missing from the commit? The dt-bindings header
qcom,gcc-sm7250.h does not appear to be included in the tree, which
might cause a build failure.

> +
> +#include "clk-alpha-pll.h"
[ ... ]
> +static const struct parent_map gcc_parent_map_5[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_SLEEP_CLK, 5 },
> +};
> +
> +static const struct clk_parent_data gcc_parent_data_5[] = {
> +	{ .fw_name = "bi_tcxo" },
> +	{ .fw_name = "sleep_clk" },
> +};

[Severity: Low]
Are the gcc_parent_map_5 and gcc_parent_data_5 arrays used anywhere in
the driver? They seem to be defined but unreferenced, which could trigger
unused-const-variable compiler warnings.

> +
> +static const struct freq_tbl ftbl_gcc_cpuss_ahb_clk_src[] = {
[ ... ]
> +static struct clk_branch gcc_cpuss_gnoc_clk = {
> +	.halt_reg = 0x48064,
> +	.halt_check = BRANCH_HALT_VOTED,
> +	.clkr = {
> +		.enable_reg = 0x48064,
> +		.enable_mask = BIT(29),

[Severity: High]
Does gcc_cpuss_gnoc_clk use the correct enable_reg and enable_mask? 

Since this clock uses BRANCH_HALT_VOTED, writing BIT(29) directly to 
its CBCR at 0x48064 instead of a central vote register (like 0x52000) 
might fail to properly enable the clock, potentially causing a timeout
when waiting for the halt bit to clear.

> +		.hw.init = &(struct clk_init_data){
> +			.name = "gcc_cpuss_gnoc_clk",
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1