linux-kernel - Re: [PATCH v4 2/5] clk: Introduce clk_hw_set_spread

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: <20250922020511.GA14753@nxa18884-linux.ap.freescale.net>
Date: Mon, 22 Sep 2025 10:05:11 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Brian Masney <bmasney@...hat.com>, Conor Dooley <conor+dt@...nel.org>,
	Cristian Marussi <cristian.marussi@....com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Marco Felsch <m.felsch@...gutronix.de>,
	Michael Turquette <mturquette@...libre.com>,
	Peng Fan <peng.fan@....com>, Rob Herring <robh@...nel.org>,
	Sudeep Holla <sudeep.holla@....com>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	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,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/5] clk: Introduce clk_hw_set_spread_spectrum

Hi Stephen,

On Sun, Sep 21, 2025 at 01:53:34PM -0700, Stephen Boyd wrote:
>Quoting Peng Fan (2025-09-15 01:29:36)
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b821b2cdb155331c85fafbd2fac8ab3703a08e4d..06db8918a1b35e3280e565272bc4603a88295a92 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2802,6 +2802,32 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>>  
>> +int clk_hw_set_spread_spectrum(struct clk_hw *hw, struct clk_spread_spectrum *conf)
>> +{
...
>> +       return ret;
>> +}
>
>Does it need to be exported? 

Yeah. it should be exported, I will add it in v5.

>
>> +
>>  /**
>>   * clk_get_parent - return the parent of a clk
>>   * @clk: the clk whose parent gets returned
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 630705a47129453c241f1b1755f2c2f2a7ed8f77..4f48a4df95a1c54638a0e91e0a449fcc8aa40b80 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -84,6 +84,19 @@ struct clk_duty {
>>         unsigned int den;
>>  };
>>  
>> +/**
>> + * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock
>> + *
>> + * @modfreq_hz:                Modulation frequency
>> + * @spread_bp:         Modulation percent in permyriad
>> + * @method:            Modulation method
>> + */
>> +struct clk_spread_spectrum {
>> +       u32 modfreq_hz;
>> +       u32 spread_bp;
>> +       u32 method;
>
>What are the possible values of 'method'? I'm guessing it's the defines
>in the dt-bindings header?

Yes. The values are in dt-bindings header and aligned with dt-schema.

>Please connect these two somehow, maybe
>through an enum.

ok. I will add back enum in V5.

>Also, why are these u32? Shouldn't these be more common
>types like unsigned long or unsigned long long instead being exactly 32
>bits?

u32 is large enough for modfreq_hz and spread_bp.

modfreq_hz will not exceed a few MHz, normally [1-9] * 10KHz per my
understanding. 

>
>> +};
>> +
>>  /**
>>   * struct clk_ops -  Callback operations for hardware clocks; these are to
>>   * be provided by the clock implementation, and will be called by drivers
>> @@ -178,6 +191,12 @@ struct clk_duty {
>>   *             separately via calls to .set_parent and .set_rate.
>>   *             Returns 0 on success, -EERROR otherwise.
>>   *
>> + * @set_spread_spectrum: Optional callback used to configure the spread
>> + *             spectrum modulation frequency, percentage, and method
>> + *             to reduce EMI by spreading the clock frequency over a
>> + *             wider range.
>> + *             Returns 0 on success, -EERROR otherwise.
>> + *
>>   * @recalc_accuracy: Recalculate the accuracy of this clock. The clock accuracy
>>   *             is expressed in ppb (parts per billion). The parent accuracy is
>>   *             an input parameter.
>> @@ -255,6 +274,8 @@ struct clk_ops {
>>         int             (*set_rate_and_parent)(struct clk_hw *hw,
>>                                     unsigned long rate,
>>                                     unsigned long parent_rate, u8 index);
>> +       int             (*set_spread_spectrum)(struct clk_hw *hw,
>> +                                              struct clk_spread_spectrum *clk_ss);
>
>const clk_ss pointer? And is it actually 'conf' or 'ss_conf'?

I will use "const clk_spread_spectrum *ss_conf" in v5.

>
>>         unsigned long   (*recalc_accuracy)(struct clk_hw *hw,
>>                                            unsigned long parent_accuracy);
>>         int             (*get_phase)(struct clk_hw *hw);
>> @@ -1430,6 +1451,7 @@ void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
>>                            unsigned long *max_rate);
>>  void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>>                            unsigned long max_rate);
>> +int clk_hw_set_spread_spectrum(struct clk_hw *hw, struct clk_spread_spectrum *conf);
>
>const conf? And 'ss_conf' again?

yeah. Fix in v5.

Thanks,
Peng