linux-kernel - Re: [PATCH 2/3] clk: conf: Support assigned-clock-sscs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250815071249.GA20726@nxa18884-linux.ap.freescale.net>
Date: Fri, 15 Aug 2025 16:50:42 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Peng Fan <peng.fan@....com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>,
	Sudeep Holla <sudeep.holla@....com>,
	Cristian Marussi <cristian.marussi@....com>,
	Marco Felsch <m.felsch@...gutronix.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
	arm-scmi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/3] clk: conf: Support assigned-clock-sscs

Hi Dan,

On Wed, Aug 13, 2025 at 08:48:15AM +0300, Dan Carpenter wrote:
>On Tue, Aug 12, 2025 at 08:17:06PM +0800, Peng Fan wrote:
>> Parse the Spread Spectrum Configuration(SSC) from device tree and configure
>> them before using the clock.
>> 
>> Each SSC is three u32 elements which means '<modfreq spreaddepth
>> modmethod>', so assigned-clock-sscs is an array of multiple three u32
>> elements.
>> 
>> Signed-off-by: Peng Fan <peng.fan@....com>
>> ---
>>  drivers/clk/clk-conf.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>> 
>> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
>> index 303a0bb26e54a95655ce094a35b989c97ebc6fd8..81a2c1f8ca4c44df2c54c1e51f800f533c9453b3 100644
>> --- a/drivers/clk/clk-conf.c
>> +++ b/drivers/clk/clk-conf.c
>> @@ -155,6 +155,72 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
>>  	return 0;
>>  }
>>  
>> +static int __set_clk_spread_spectrum(struct device_node *node, bool clk_supplier)
>> +{
>> +	u32 *sscs __free(kfree) = NULL;
>> +	u32 elem_size = sizeof(u32) * 3;
>> +	struct of_phandle_args clkspec;
>> +	int rc, count, index;
>> +	struct clk *clk;
>> +
>> +	/* modfreq, spreadPercent, modmethod */
>> +	count = of_property_count_elems_of_size(node, "assigned-clock-sscs", elem_size);
>> +	if (count > 0) {
>> +		sscs = kcalloc(count, elem_size, GFP_KERNEL);
>> +		if (!sscs)
>> +			return -ENOMEM;
>> +		rc = of_property_read_u32_array(node,
>> +						"assigned-clock-sscs",
>> +						sscs, count * 3);
>> +	} else {
>> +		return 0;
>> +	}
>> +
>> +	if (rc)
>> +		return rc;
>
>Nit pick: Please, flip these conditions around.

ok. Fix in next version.

>
>	count = of_property_count_elems_of_size(node, "assigned-clock-sscs", elem_size);
>	if (count <= 0)
>		return 0;
>
>	sscs = kcalloc(count, elem_size, GFP_KERNEL);
>	if (!sscs)
>		return -ENOMEM;
>
>	rc = of_property_read_u32_array(node, "assigned-clock-sscs", sscs,
>					count * 3);
>	if (rc)
>		return rc;
>
>> +
>> +	for (index = 0; index < count; index++) {
>> +		u32 modfreq_hz = sscs[index * 3], spread_bp = sscs[index * 3 + 1];
>> +		u32 method = sscs[index * 3 + 2];
>
>This math would be nicer if you created a struct:
>
>struct spread_config {
>	u32 modfreq_hz;
>	u32 spread_depth;
>	u32 method;
>};
>
>Then you could use that instead of sscs.
>
>	for (i = 0; i < count; i++) {
>		struct spread_config *conf = &configs[i];
>		struct clk_hw *hw;
>
>		if (!conf->modfreq_hz && !conf->spread_depth && !conf->method)
>			continue;

Thanks for the tips. Update in next version.

>
>> +		struct clk_hw *hw;
>> +
>> +		if (modfreq_hz || spread_bp || method) {
>> +			rc = of_parse_phandle_with_args(node, "assigned-clocks",
>> +					"#clock-cells",	index, &clkspec);
>> +			if (rc < 0) {
>> +				/* skip empty (null) phandles */
>> +				if (rc == -ENOENT)
>> +					continue;
>> +				else
>> +					return rc;
>> +			}
>> +
>> +			if (clkspec.np == node && !clk_supplier) {
>
>Could you add a comment for this condition?  It's strange to me that we
>don't iterate through the whole array.

I just follow the logic in __set_clk_parents and __set_clk_rates, nothing
special here.

It is just like to phase out cases as below:
  node-x {
	/* node-x is not a clk provider, but assigned-clocks uses node-x phandle */
	assigned-clocks = <&node-x   XYZ>;
  }

Thanks for reviewing the patch. I will wait to collect more comments before
posting next version.

Regards,
Peng

>
>regards,
>dan carpenter
>
>> +				of_node_put(clkspec.np);
>> +				return 0;
>> +			}
>> +
>> +			clk = of_clk_get_from_provider(&clkspec);
>> +			of_node_put(clkspec.np);
>> +			if (IS_ERR(clk)) {
>> +				if (PTR_ERR(clk) != -EPROBE_DEFER)
>> +					pr_warn("clk: couldn't get clock %d for %pOF\n",
>> +						index, node);
>> +				return PTR_ERR(clk);
>> +			}
>> +
>> +			hw = __clk_get_hw(clk);
>> +			rc = clk_hw_set_spread_spectrum(hw, modfreq_hz, spread_bp, method);
>> +			if (rc < 0)
>> +				pr_err("clk: couldn't set %s clk spread spectrum %u %u %u: %d\n",
>> +				       __clk_get_name(clk), modfreq_hz, spread_bp, method, rc);
>> +			clk_put(clk);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>