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] [day] [month] [year] [list]

Message-ID: <aJwnH2mlRdqp1K1u@stanley.mountain>
Date: Wed, 13 Aug 2025 08:48:15 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Peng Fan <peng.fan@....com>
Cc: 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

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.

	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;

> +		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.

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;
> +}