Re: [PATCH 1/2] clk: qcom: Add Global Clock Controller (GCC) driver for SM7250
From: sashiko-bot@kernel.org
Date: 2026-08-16 08:42:16
Also in:
linux-clk
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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. +
[ ... ]
quoted hunk ↗ jump to hunk
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 <sreeshankar0910@gmail.com> + */ + +#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/20260816082801.5773-1-sreeshankar0910@gmail.com?part=1