Re: [PATCH 2/2] clk: qcom: segcc-nord: Add I2C and TSC clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] clk: qcom: segcc-nord: erroneous DFS registration of
  se_gcc_cnoc_i2c_slave_clk_src
- [Low] clk: qcom: segcc-nord: duplicate 19.2 MHz freq_tbl and misplaced
  RCG definition
--

commit 5e0b8aed90909a3578c24b44cabdb5fafa31fe1c
Author: Taniya Das <taniya.das@oss.qualcomm.com>

clk: qcom: segcc-nord: Add I2C and TSC clock support

This commit adds the I2C slave and TSC subsystem clocks, reset lines, and
GPLL1 parent PLL definitions to the Nord SE GCC driver.

> diff --git a/drivers/clk/qcom/segcc-nord.c b/drivers/clk/qcom/segcc-nord.c
> index ef97da3852e35..7fdfa44fb1036 100644
> --- a/drivers/clk/qcom/segcc-nord.c
> +++ b/drivers/clk/qcom/segcc-nord.c
[ ... ]
> @@ -189,6 +207,43 @@ static const struct clk_parent_data se_gcc_parent_data_4[] = {
>  	{ .index = DT_SLEEP_CLK },
>  };
>  
> +static const struct freq_tbl ftbl_se_gcc_cnoc_i2c_slave_clk_src[] = {
> +	F(19200000, P_BI_TCXO, 1, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_init_data se_gcc_cnoc_i2c_slave_clk_src_init = {
> +	.name = "se_gcc_cnoc_i2c_slave_clk_src",
> +	.parent_data = se_gcc_parent_data_2,
> +	.num_parents = ARRAY_SIZE(se_gcc_parent_data_2),
> +	.flags = CLK_SET_RATE_PARENT,
> +	.ops = &clk_rcg2_shared_no_init_park_ops,
> +};
> +
> +static struct clk_rcg2 se_gcc_cnoc_i2c_slave_clk_src = {
> +	.cmd_rcgr = 0x13034,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = se_gcc_parent_map_2,
> +	.freq_tbl = ftbl_se_gcc_cnoc_i2c_slave_clk_src,
> +	.hw_clk_ctrl = true,
> +	.clkr.hw.init = &se_gcc_cnoc_i2c_slave_clk_src_init,
> +};
> +
> +static const struct parent_map se_gcc_parent_map_5[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_SE_GCC_GPLL0_OUT_MAIN, 1 },
> +	{ P_SE_GCC_GPLL5_OUT_MAIN, 3 },
> +	{ P_SE_GCC_GPLL1_OUT_MAIN, 4 },
> +};
> +
> +static const struct clk_parent_data se_gcc_parent_data_5[] = {
> +	{ .index = DT_BI_TCXO },
> +	{ .hw = &se_gcc_gpll0.clkr.hw },
> +	{ .hw = &se_gcc_gpll5.clkr.hw },
> +	{ .hw = &se_gcc_gpll1.clkr.hw },
> +};

[Severity: Low]
This isn't a bug, but ftbl_se_gcc_cnoc_i2c_slave_clk_src is identical to
ftbl_se_gcc_emac0_phy_aux_clk_src, which se_gcc_emac1_phy_aux_clk_src and
se_gcc_i2c_slave_xo_clk_src already reuse.  Could
se_gcc_cnoc_i2c_slave_clk_src reuse ftbl_se_gcc_emac0_phy_aux_clk_src and
be placed with the other RCG definitions after the parent map tables?

[ ... ]

> @@ -1570,11 +1754,14 @@ static const struct qcom_reset_map se_gcc_nord_resets[] = {
>  	[SE_GCC_EMAC0_BCR] = { 0x24000 },
>  	[SE_GCC_EMAC1_BCR] = { 0x25000 },
> +	[SE_GCC_I2C_BCR] = { 0x2c000 },
>  	[SE_GCC_QUPV3_WRAPPER_0_BCR] = { 0x26000 },
>  	[SE_GCC_QUPV3_WRAPPER_1_BCR] = { 0x27000 },
> +	[SE_GCC_TSCSS_BCR] = { 0x2a000 },
>  };
>  
>  static const struct clk_rcg_dfs_data se_gcc_nord_dfs_clocks[] = {
> +	DEFINE_RCG_DFS(se_gcc_cnoc_i2c_slave_clk_src),
>  	DEFINE_RCG_DFS(se_gcc_qupv3_wrap0_s0_clk_src),
>  	DEFINE_RCG_DFS(se_gcc_qupv3_wrap0_s1_clk_src),
>  	DEFINE_RCG_DFS(se_gcc_qupv3_wrap0_s2_clk_src),

[Severity: Medium]
Should se_gcc_cnoc_i2c_slave_clk_src be registered in
se_gcc_nord_dfs_clocks?

se_gcc_cnoc_i2c_slave_clk_src is a fixed-19.2 MHz CNOC RCG with
mnd_width = 0, not a QUPv3 GENI Serial Engine RCG.  Passing it to
qcom_cc_register_rcg_dfs() -> clk_rcg2_enable_dfs() reads cmd_rcgr +
SE_CMD_DFSR_OFFSET (0x13048), which is outside its 8-byte RCG register
block.

If bit 0 at 0x13048 happens to be set, clk_rcg2_enable_dfs() replaces the
clock's ops with clk_rcg2_dfs_ops (dropping the enable, disable, and
set_rate callbacks needed by se_gcc_i2c_slave_ahb_clk) and clears
freq_tbl.  Any subsequent rate calculation when CFG_MODE_MASK is non-zero
in the registers read as DFS performance levels will compute mask =
BIT(rcg->mnd_width) - 1 = 0, leaving m = 0 and n = 0 and triggering a
divide-by-zero in calc_rate().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-gcc_i2c_tsc_support-v1-0-433d02972d7e@oss.qualcomm.com?part=2