Re: [PATCH v2 2/3] clk: qcom: Add NSS clock controller driver for IPQ5210 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] clk: qcom: nsscc-ipq5210: CLK_SET_RATE_PARENT on XGMAC PTP
  dividers
--

commit ba0d35b1d755dedd51b7ae9f8752c3443ce8f41c
Author: Luo Jie <jie.luo@oss.qualcomm.com>

clk: qcom: Add NSS clock controller driver for IPQ5210 SoC

This commit adds the NSS clock controller driver for IPQ5210, providing
clocks, resets, and interconnect clock paths for the networking subsystem.

> diff --git a/drivers/clk/qcom/nsscc-ipq5210.c b/drivers/clk/qcom/nsscc-ipq5210.c
> new file mode 100644
> index 0000000000000..97673d01c9569
> --- /dev/null
> +++ b/drivers/clk/qcom/nsscc-ipq5210.c
> @@ -0,0 +1,2202 @@

[ ... ]

> +static const struct freq_tbl ftbl_nss_cc_ppe_clk_src[] = {
> +	F(24000000, P_XO, 1, 0, 0),
> +	F(429000000, P_CMN_PLL_NSS_CLK_429M, 1, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 nss_cc_ppe_clk_src = {
> +	.cmd_rcgr = 0x3ec,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = nss_cc_parent_map_ppe,
> +	.freq_tbl = ftbl_nss_cc_ppe_clk_src,
> +	.clkr.hw.init = &(const struct clk_init_data){
> +		.name = "nss_cc_ppe_clk_src",
> +		.parent_data = nss_cc_ppe_parent_data_ppe,
> +		.num_parents = ARRAY_SIZE(nss_cc_ppe_parent_data_ppe),
> +		.ops = &clk_rcg2_ops,
> +	},
> +};
> +
> +/*
> + * The XGMAC PTP reference clock rate needs to be configurable, so the
> + * PTP reference dividers use the writable divider ops.
> + */
> +static struct clk_regmap_div nss_cc_xgmac0_ptp_ref_div_clk_src = {
> +	.reg = 0x3f4,
> +	.shift = 0,
> +	.width = 4,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "nss_cc_xgmac0_ptp_ref_div_clk_src",
> +		.parent_hws = (const struct clk_hw*[]){
> +			&nss_cc_ppe_clk_src.clkr.hw,
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_regmap_div_ops,
> +	},
> +};
> +
> +static struct clk_regmap_div nss_cc_xgmac1_ptp_ref_div_clk_src = {
> +	.reg = 0x3f8,
> +	.shift = 0,
> +	.width = 4,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "nss_cc_xgmac1_ptp_ref_div_clk_src",
> +		.parent_hws = (const struct clk_hw*[]){
> +			&nss_cc_ppe_clk_src.clkr.hw,
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_regmap_div_ops,
> +	},
> +};
> +
> +static struct clk_regmap_div nss_cc_xgmac2_ptp_ref_div_clk_src = {
> +	.reg = 0x3fc,
> +	.shift = 0,
> +	.width = 4,
> +	.clkr.hw.init = &(const struct clk_init_data) {
> +		.name = "nss_cc_xgmac2_ptp_ref_div_clk_src",
> +		.parent_hws = (const struct clk_hw*[]){
> +			&nss_cc_ppe_clk_src.clkr.hw,
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_regmap_div_ops,
> +	},
> +};

[Severity: Medium]
Should nss_cc_xgmac0_ptp_ref_div_clk_src,
nss_cc_xgmac1_ptp_ref_div_clk_src, and nss_cc_xgmac2_ptp_ref_div_clk_src
omit CLK_SET_RATE_PARENT now that they use writable clk_regmap_div_ops
(like nss_cc_uniphy_port1_rx_div4_clk_src)?

Because these 4-bit dividers can only divide the 429 MHz parent down to
429000000 / 16 = 26812500 Hz, requesting a PTP reference rate of 25 MHz or
24 MHz causes clk_divider_bestdiv() (with CLK_DIVIDER_ROUND_CLOSEST) to
select F(24000000, P_XO, 1, 0, 0) on nss_cc_ppe_clk_src.  That reparents
the shared nss_cc_ppe_clk_src root clock from 429 MHz to the 24 MHz XO,
throttling the PPE switch, EDMA, MACs, PON, and NSSNOC PPE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-qcom_ipq5210_nsscc-v2-0-7e2c1dda3ed2@oss.qualcomm.com?part=2