[PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Peng Fan (OSS)]

- Introduce clk_set_spread_spectrum to set the parameters for enabling
  spread spectrum of a clock.
- Parse 'assigned-clock-sscs' and configure it by default before using the
  clock. The pull request for this property is at [1]
  This property is parsed before parsing clock rate.

- Enable this feature for clk-scmi on i.MX95.
  This may not the best, since checking machine compatibles.
  I am thinking to provide an API scmi_get_vendor_info, then driver
  could use it for OEM stuff, such as
  if (scmi_get_vendor_info returns NXP_IMX)
      ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;

[1] https://github.com/devicetree-org/dt-schema/pull/154

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (3):
      clk: Introduce clk_set_spread_spectrum
      clk: conf: Support assigned-clock-sscs
      clk: scmi: Support spread spectrum

 drivers/clk/clk-conf.c        | 68 +++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-scmi.c        | 37 +++++++++++++++++++++++
 drivers/clk/clk.c             | 39 +++++++++++++++++++++++++
 include/linux/clk-provider.h  | 22 ++++++++++++++
 include/linux/clk.h           | 22 ++++++++++++++
 include/linux/scmi_protocol.h |  5 ++++
 6 files changed, 193 insertions(+)
---
base-commit: 5ffa57f6eecefababb8cbe327222ef171943b183
change-id: 20250124-clk-ssc-fccd4f60d7e5

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

Re: [PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Dario Binacchi]

On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> - Introduce clk_set_spread_spectrum to set the parameters for enabling
>   spread spectrum of a clock.
> - Parse 'assigned-clock-sscs' and configure it by default before using the
>   clock. The pull request for this property is at [1]
>   This property is parsed before parsing clock rate.
>
> - Enable this feature for clk-scmi on i.MX95.
>   This may not the best, since checking machine compatibles.
>   I am thinking to provide an API scmi_get_vendor_info, then driver
>   could use it for OEM stuff, such as
>   if (scmi_get_vendor_info returns NXP_IMX)
>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
>

I wonder if your solution is truly generic or merely a generalization
of your use case, which seems significantly simpler compared to what
happens on the i.MX8M platform, as discussed in thread
https://lore.kernel.org/lkml/PAXPR04MB8459537D7D2A49221D0E890D88E32@PAXPR04MB8459.eurprd04.prod.outlook.com/,
or on the STM32F platform, where parameters are not written directly
to registers but are instead used in calculations involving the
parent_rate and the PLL divider, for example.

I am the author of the patches that introduced spread spectrum
management for the AM33x and STM32Fx platforms, as well as the
series, still pending acceptance, for the i.MX8M.
From my perspective, this functionality varies significantly
from platform to platform, with key differences that must be
considered.

Thanks and regards,
Dario

> [1] https://github.com/devicetree-org/dt-schema/pull/154
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> Peng Fan (3):
>       clk: Introduce clk_set_spread_spectrum
>       clk: conf: Support assigned-clock-sscs
>       clk: scmi: Support spread spectrum
>
>  drivers/clk/clk-conf.c        | 68 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk-scmi.c        | 37 +++++++++++++++++++++++
>  drivers/clk/clk.c             | 39 +++++++++++++++++++++++++
>  include/linux/clk-provider.h  | 22 ++++++++++++++
>  include/linux/clk.h           | 22 ++++++++++++++
>  include/linux/scmi_protocol.h |  5 ++++
>  6 files changed, 193 insertions(+)
> ---
> base-commit: 5ffa57f6eecefababb8cbe327222ef171943b183
> change-id: 20250124-clk-ssc-fccd4f60d7e5
>
> Best regards,
> --
> Peng Fan <peng.fan@nxp.com>
>


-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Krzysztof Kozlowski]

On 25/01/2025 13:58, Dario Binacchi wrote:
> On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>
>> - Introduce clk_set_spread_spectrum to set the parameters for enabling
>>   spread spectrum of a clock.
>> - Parse 'assigned-clock-sscs' and configure it by default before using the
>>   clock. The pull request for this property is at [1]
>>   This property is parsed before parsing clock rate.
>>
>> - Enable this feature for clk-scmi on i.MX95.
>>   This may not the best, since checking machine compatibles.
>>   I am thinking to provide an API scmi_get_vendor_info, then driver
>>   could use it for OEM stuff, such as
>>   if (scmi_get_vendor_info returns NXP_IMX)
>>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
>>
> 
> I wonder if your solution is truly generic or merely a generalization
> of your use case, which seems significantly simpler compared to what

Please come with specific arguments why this is not generic enough, not
just FUD. Does it fit your case? If not, what would had to be changed?
These are the comments needed to actually work on generic solution.

> happens on the i.MX8M platform, as discussed in thread
> https://lore.kernel.org/lkml/PAXPR04MB8459537D7D2A49221D0E890D88E32@PAXPR04MB8459.eurprd04.prod.outlook.com/,
> or on the STM32F platform, where parameters are not written directly
> to registers but are instead used in calculations involving the
> parent_rate and the PLL divider, for example.
> 
> I am the author of the patches that introduced spread spectrum
> management for the AM33x and STM32Fx platforms, as well as the
> series, still pending acceptance, for the i.MX8M.
> From my perspective, this functionality varies significantly
> from platform to platform, with key differences that must be
> considered.

So what exactly varies? Come with specifics.

To me look addressing exactly the same.

Best regards,
Krzysztof

Re: [PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Dario Binacchi]

On Mon, Jan 27, 2025 at 8:42 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 25/01/2025 13:58, Dario Binacchi wrote:
> > On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> >>
> >> - Introduce clk_set_spread_spectrum to set the parameters for enabling
> >>   spread spectrum of a clock.
> >> - Parse 'assigned-clock-sscs' and configure it by default before using the
> >>   clock. The pull request for this property is at [1]
> >>   This property is parsed before parsing clock rate.
> >>
> >> - Enable this feature for clk-scmi on i.MX95.
> >>   This may not the best, since checking machine compatibles.
> >>   I am thinking to provide an API scmi_get_vendor_info, then driver
> >>   could use it for OEM stuff, such as
> >>   if (scmi_get_vendor_info returns NXP_IMX)
> >>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
> >>
> >
> > I wonder if your solution is truly generic or merely a generalization
> > of your use case, which seems significantly simpler compared to what
>
> Please come with specific arguments why this is not generic enough, not
> just FUD. Does it fit your case? If not, what would had to be changed?
> These are the comments needed to actually work on generic solution.
>
> > happens on the i.MX8M platform, as discussed in thread
> > https://lore.kernel.org/lkml/PAXPR04MB8459537D7D2A49221D0E890D88E32@PAXPR04MB8459.eurprd04.prod.outlook.com/,
> > or on the STM32F platform, where parameters are not written directly
> > to registers but are instead used in calculations involving the
> > parent_rate and the PLL divider, for example.
> >
> > I am the author of the patches that introduced spread spectrum
> > management for the AM33x and STM32Fx platforms, as well as the
> > series, still pending acceptance, for the i.MX8M.
> > From my perspective, this functionality varies significantly
> > from platform to platform, with key differences that must be
> > considered.
>
> So what exactly varies? Come with specifics.

In all the cases I implemented, I enabled spread spectrum within
the set_rate of the clock/PLL in question, as information such as
the parent rate or the divisor used was necessary to perform the
calculations needed to extract the data for setting the SSCG
register bitfields.

If I'm not mistaken, I think this is not the case implemented by this
series.

Thanks and regards,
Dario

> To me look addressing exactly the same.
>
> Best regards,
> Krzysztof



-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Krzysztof Kozlowski]

On 27/01/2025 08:59, Dario Binacchi wrote:
> On Mon, Jan 27, 2025 at 8:42 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 25/01/2025 13:58, Dario Binacchi wrote:
>>> On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>>>
>>>> - Introduce clk_set_spread_spectrum to set the parameters for enabling
>>>>   spread spectrum of a clock.
>>>> - Parse 'assigned-clock-sscs' and configure it by default before using the
>>>>   clock. The pull request for this property is at [1]
>>>>   This property is parsed before parsing clock rate.
>>>>
>>>> - Enable this feature for clk-scmi on i.MX95.
>>>>   This may not the best, since checking machine compatibles.
>>>>   I am thinking to provide an API scmi_get_vendor_info, then driver
>>>>   could use it for OEM stuff, such as
>>>>   if (scmi_get_vendor_info returns NXP_IMX)
>>>>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
>>>>
>>>
>>> I wonder if your solution is truly generic or merely a generalization
>>> of your use case, which seems significantly simpler compared to what
>>
>> Please come with specific arguments why this is not generic enough, not
>> just FUD. Does it fit your case? If not, what would had to be changed?
>> These are the comments needed to actually work on generic solution.
>>
>>> happens on the i.MX8M platform, as discussed in thread
>>> https://lore.kernel.org/lkml/PAXPR04MB8459537D7D2A49221D0E890D88E32@PAXPR04MB8459.eurprd04.prod.outlook.com/,
>>> or on the STM32F platform, where parameters are not written directly
>>> to registers but are instead used in calculations involving the
>>> parent_rate and the PLL divider, for example.
>>>
>>> I am the author of the patches that introduced spread spectrum
>>> management for the AM33x and STM32Fx platforms, as well as the
>>> series, still pending acceptance, for the i.MX8M.
>>> From my perspective, this functionality varies significantly
>>> from platform to platform, with key differences that must be
>>> considered.
>>
>> So what exactly varies? Come with specifics.
> 
> In all the cases I implemented, I enabled spread spectrum within
> the set_rate of the clock/PLL in question, as information such as
> the parent rate or the divisor used was necessary to perform the
> calculations needed to extract the data for setting the SSCG
> register bitfields.
> 
> If I'm not mistaken, I think this is not the case implemented by this
> series.

It feels like you speak about driver, so I misunderstood the concerns. I
did not check the drivers at all, so here I do not claim patchsets are
compatible.

But the binding takes the same values - the main PLL/clock rate.

Best regards,
Krzysztof

Re: [PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Dario Binacchi]

On Mon, Jan 27, 2025 at 9:31 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 27/01/2025 08:59, Dario Binacchi wrote:
> > On Mon, Jan 27, 2025 at 8:42 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 25/01/2025 13:58, Dario Binacchi wrote:
> >>> On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> >>>>
> >>>> - Introduce clk_set_spread_spectrum to set the parameters for enabling
> >>>>   spread spectrum of a clock.
> >>>> - Parse 'assigned-clock-sscs' and configure it by default before using the
> >>>>   clock. The pull request for this property is at [1]
> >>>>   This property is parsed before parsing clock rate.
> >>>>
> >>>> - Enable this feature for clk-scmi on i.MX95.
> >>>>   This may not the best, since checking machine compatibles.
> >>>>   I am thinking to provide an API scmi_get_vendor_info, then driver
> >>>>   could use it for OEM stuff, such as
> >>>>   if (scmi_get_vendor_info returns NXP_IMX)
> >>>>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
> >>>>
> >>>
> >>> I wonder if your solution is truly generic or merely a generalization
> >>> of your use case, which seems significantly simpler compared to what
> >>
> >> Please come with specific arguments why this is not generic enough, not
> >> just FUD. Does it fit your case? If not, what would had to be changed?
> >> These are the comments needed to actually work on generic solution.
> >>
> >>> happens on the i.MX8M platform, as discussed in thread
> >>> https://lore.kernel.org/lkml/PAXPR04MB8459537D7D2A49221D0E890D88E32@PAXPR04MB8459.eurprd04.prod.outlook.com/,
> >>> or on the STM32F platform, where parameters are not written directly
> >>> to registers but are instead used in calculations involving the
> >>> parent_rate and the PLL divider, for example.
> >>>
> >>> I am the author of the patches that introduced spread spectrum
> >>> management for the AM33x and STM32Fx platforms, as well as the
> >>> series, still pending acceptance, for the i.MX8M.
> >>> From my perspective, this functionality varies significantly
> >>> from platform to platform, with key differences that must be
> >>> considered.
> >>
> >> So what exactly varies? Come with specifics.
> >
> > In all the cases I implemented, I enabled spread spectrum within
> > the set_rate of the clock/PLL in question, as information such as
> > the parent rate or the divisor used was necessary to perform the
> > calculations needed to extract the data for setting the SSCG
> > register bitfields.
> >
> > If I'm not mistaken, I think this is not the case implemented by this
> > series.
>
> It feels like you speak about driver, so I misunderstood the concerns. I
> did not check the drivers at all, so here I do not claim patchsets are
> compatible.

Yes, I commented on the driver.
For the dt-bindings I added a comment in the github PR
https://github.com/devicetree-org/dt-schema/pull/154

Thanks and regards,
Dario

>
> But the binding takes the same values - the main PLL/clock rate.
>
> Best regards,
> Krzysztof



-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

[PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Peng Fan]

Since the assigned-clock-sscs property [1] has been accepted into the device
tree schema, we can now support it in the Linux clock driver. Therefore,
I’ve picked up the previously submitted work [2] titled “clk: Support
spread spectrum and use it in clk-pll144x and clk-scmi.”
As more than six months have passed since [2] was posted, I’m treating this
patchset as a new submission rather than a v3.

- Introduce clk_set_spread_spectrum to set the parameters for enabling
  spread spectrum of a clock.
- Parse 'assigned-clock-sscs' and configure it by default before using the
  clock. This property is parsed before parsing clock rate.
- Enable this feature for clk-scmi on i.MX95.

Because SCMI spec will not include spread spectrum as a standard
extension, we still need to use NXP i.MX OEM extension.

[1] https://github.com/devicetree-org/dt-schema/pull/154
[2] https://lore.kernel.org/all/20250205-clk-ssc-v2-0-fa73083caa92@nxp.com/

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (3):
      clk: Introduce clk_hw_set_spread_spectrum
      clk: conf: Support assigned-clock-sscs
      clk: scmi: Support Spread Spectrum for NXP i.MX95

 drivers/clk/clk-conf.c        | 70 +++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-scmi.c        | 64 ++++++++++++++++++++++++++++++++++++---
 drivers/clk/clk.c             | 32 ++++++++++++++++++++
 include/linux/clk-provider.h  | 29 ++++++++++++++++++
 include/linux/scmi_protocol.h |  5 ++++
 5 files changed, 196 insertions(+), 4 deletions(-)
---
base-commit: b1549501188cc9eba732c25b033df7a53ccc341f
change-id: 20250812-clk-ssc-version1-acf6f6efbd96

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH 1/3] clk: Introduce clk_hw_set_spread_spectrum

[Peng Fan]

Add clk_hw_set_spread_spectrum to configure a clock to enable spread
spectrum feature. set_spread_spectrum ops is added for clk drivers to
have their own hardware specific implementation.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/clk.c            | 32 ++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b821b2cdb155331c85fafbd2fac8ab3703a08e4d..48c7a301b72b30fd824dae7ada2c44ee84d40867 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2802,6 +2802,38 @@ 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, unsigned int modfreq_hz,
+			       unsigned int spread_bp, unsigned int method)
+{
+	struct clk_spread_spectrum clk_ss;
+	struct clk_core *core;
+	int ret;
+
+	if (!hw)
+		return 0;
+
+	core = hw->core;
+
+	clk_ss.modfreq_hz = modfreq_hz;
+	clk_ss.spread_bp = spread_bp;
+	clk_ss.method = method;
+
+	clk_prepare_lock();
+
+	ret = clk_pm_runtime_get(core);
+	if (ret)
+		goto fail;
+
+	if (core->ops->set_spread_spectrum)
+		ret = core->ops->set_spread_spectrum(hw, &clk_ss);
+
+	clk_pm_runtime_put(core);
+
+fail:
+	clk_prepare_unlock();
+	return ret;
+}
+
 /**
  * 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..2b6cebe8b12268f537b3c92aa0bbadea601f0eb0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -84,6 +84,26 @@ struct clk_duty {
 	unsigned int den;
 };
 
+enum clk_ssc_method {
+	CLK_SSC_NO_SPREAD,
+	CLK_SSC_CENTER_SPREAD,
+	CLK_SSC_UP_SPREAD,
+	CLK_SSC_DOWN_SPREAD,
+};
+
+/**
+ * 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 {
+	unsigned int modfreq_hz;
+	unsigned int spread_bp;
+	enum clk_ssc_method method;
+};
+
 /**
  * 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 +198,11 @@ struct clk_duty {
  *		separately via calls to .set_parent and .set_rate.
  *		Returns 0 on success, -EERROR otherwise.
  *
+ * @set_spread_spectrum: Configure the modulation frequency, modulation percentage
+ *		and method. This callback is optional for clocks that does not
+ *		support spread spectrum feature or no need to enable this feature.
+ *		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 +280,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);
 	unsigned long	(*recalc_accuracy)(struct clk_hw *hw,
 					   unsigned long parent_accuracy);
 	int		(*get_phase)(struct clk_hw *hw);
@@ -1430,6 +1457,8 @@ 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, unsigned int modfreq_hz,
+			       unsigned int spread_bp, unsigned int method);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {

-- 
2.37.1

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

[Peng Fan]

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@nxp.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;
+
+	for (index = 0; index < count; index++) {
+		u32 modfreq_hz = sscs[index * 3], spread_bp = sscs[index * 3 + 1];
+		u32 method = sscs[index * 3 + 2];
+		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) {
+				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;
+}
+
 /**
  * of_clk_set_defaults() - parse and set assigned clocks configuration
  * @node: device node to apply clock settings for
@@ -174,6 +240,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
 	if (!node)
 		return 0;
 
+	rc = __set_clk_spread_spectrum(node, clk_supplier);
+	if (rc < 0)
+		return rc;
+
 	rc = __set_clk_parents(node, clk_supplier);
 	if (rc < 0)
 		return rc;

-- 
2.37.1

[PATCH 3/3] clk: scmi: Support Spread Spectrum for NXP i.MX95

[Peng Fan]

The PLL clocks on NXP i.MX95 SoCs support Spread Spectrum (SS).
This patch introduces scmi_clk_imx_set_spread_spectrum to pass SS
configuration to the SCMI firmware, which handles the actual
implementation.

To ensure this feature is only enabled on NXP platforms,
scmi_clk_imx_extended_config_oem is added. Since SS is only applicable
to PLL clocks, config_oem_get is used to verify SS support for a given
clock.

i.MX95 SCMI firmware Spread Spectrum extConfigValue definition is as
below, no modulation method because firmware forces to use down spread.
	 extConfigValue[7:0]   - spread percentage (%)
	 extConfigValue[23:8]  - Modulation Frequency (KHz)
	 extConfigValue[24]    - Enable/Disable
	 extConfigValue[31:25] - Reserved

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/clk-scmi.c        | 64 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/scmi_protocol.h |  5 ++++
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index d2408403283fc72f0cf902e65f4c08bcbc7b4b0b..bb5e20dab18e92932ab4b99192b496e0c4d96417 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -12,6 +12,7 @@
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
+#include <linux/scmi_imx_protocol.h>
 #include <asm/div64.h>
 
 #define NOT_ATOMIC	false
@@ -23,6 +24,7 @@ enum scmi_clk_feats {
 	SCMI_CLK_RATE_CTRL_SUPPORTED,
 	SCMI_CLK_PARENT_CTRL_SUPPORTED,
 	SCMI_CLK_DUTY_CYCLE_SUPPORTED,
+	SCMI_CLK_IMX_SSC_SUPPORTED,
 	SCMI_CLK_FEATS_COUNT
 };
 
@@ -98,6 +100,35 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index)
 	return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index);
 }
 
+static int scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
+					    struct clk_spread_spectrum *clk_ss)
+{
+	struct scmi_clk *clk = to_scmi_clk(hw);
+	int ret;
+	u32 val;
+
+	/*
+	 * extConfigValue[7:0]   - spread percentage (%)
+	 * extConfigValue[23:8]  - Modulation Frequency
+	 * extConfigValue[24]    - Enable/Disable
+	 * extConfigValue[31:25] - Reserved
+	 */
+	val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, clk_ss->spread_bp / 10000);
+	val |= FIELD_PREP(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK, clk_ss->modfreq_hz);
+	if (clk_ss->method != CLK_SSC_NO_SPREAD)
+		val |= SCMI_CLOCK_IMX_SS_ENABLE_MASK;
+	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
+						 SCMI_CLOCK_CFG_IMX_SSC,
+						 val, false);
+	if (ret)
+		dev_warn(clk->dev,
+			 "Failed to set spread spectrum(%u,%u,%u) for clock ID %d\n",
+			 clk_ss->modfreq_hz, clk_ss->spread_bp, clk_ss->method,
+			 clk->id);
+
+	return ret;
+}
+
 static u8 scmi_clk_get_parent(struct clk_hw *hw)
 {
 	struct scmi_clk *clk = to_scmi_clk(hw);
@@ -316,11 +347,33 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
 		ops->set_duty_cycle = scmi_clk_set_duty_cycle;
 	}
 
+	if (feats_key & BIT(SCMI_CLK_IMX_SSC_SUPPORTED))
+		ops->set_spread_spectrum = scmi_clk_imx_set_spread_spectrum;
+
 	return ops;
 }
 
+static void scmi_clk_imx_extended_config_oem(const struct scmi_handle *handle,
+					     struct scmi_clk *sclk,
+					     unsigned int *feats_key)
+{
+	int ret;
+	u32 val;
+
+	if (strcmp(handle->version->vendor_id, SCMI_IMX_VENDOR) ||
+	    strcmp(handle->version->sub_vendor_id, SCMI_IMX_SUBVENDOR))
+		return;
+
+	ret = scmi_proto_clk_ops->config_oem_get(sclk->ph, sclk->id,
+						 SCMI_CLOCK_CFG_IMX_SSC,
+						 &val, NULL, false);
+	if (!ret)
+		*feats_key |= BIT(SCMI_CLK_IMX_SSC_SUPPORTED);
+}
+
 /**
  * scmi_clk_ops_select() - Select a proper set of clock operations
+ * @handle: A reference to an SCMI entity
  * @sclk: A reference to an SCMI clock descriptor
  * @atomic_capable: A flag to indicate if atomic mode is supported by the
  *		    transport
@@ -345,8 +398,8 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
  *	   NULL otherwise.
  */
 static const struct clk_ops *
-scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
-		    unsigned int atomic_threshold_us,
+scmi_clk_ops_select(const struct scmi_handle *handle, struct scmi_clk *sclk,
+		    bool atomic_capable, unsigned int atomic_threshold_us,
 		    const struct clk_ops **clk_ops_db, size_t db_size)
 {
 	const struct scmi_clock_info *ci = sclk->info;
@@ -370,9 +423,12 @@ scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
 	if (!ci->parent_ctrl_forbidden)
 		feats_key |= BIT(SCMI_CLK_PARENT_CTRL_SUPPORTED);
 
-	if (ci->extended_config)
+	if (ci->extended_config) {
 		feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
 
+		scmi_clk_imx_extended_config_oem(handle, sclk, &feats_key);
+	}
+
 	if (WARN_ON(feats_key >= db_size))
 		return NULL;
 
@@ -459,7 +515,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		 * to avoid sharing the devm_ allocated clk_ops between multiple
 		 * SCMI clk driver instances.
 		 */
-		scmi_ops = scmi_clk_ops_select(sclk, transport_is_atomic,
+		scmi_ops = scmi_clk_ops_select(handle, sclk, transport_is_atomic,
 					       atomic_threshold_us,
 					       scmi_clk_ops_db,
 					       ARRAY_SIZE(scmi_clk_ops_db));
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816247d24704f7ba109667a14226b67..6f9f197ee671540e38470a5666eb5ba8ec9de439 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -80,9 +80,14 @@ enum scmi_clock_oem_config {
 	SCMI_CLOCK_CFG_DUTY_CYCLE = 0x1,
 	SCMI_CLOCK_CFG_PHASE,
 	SCMI_CLOCK_CFG_OEM_START = 0x80,
+	SCMI_CLOCK_CFG_IMX_SSC = 0x80,
 	SCMI_CLOCK_CFG_OEM_END = 0xFF,
 };
 
+#define SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK	GENMASK(7, 0)
+#define SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK		GENMASK(23, 8)
+#define SCMI_CLOCK_IMX_SS_ENABLE_MASK		BIT(24)
+
 /**
  * struct scmi_clk_proto_ops - represents the various operations provided
  *	by SCMI Clock Protocol

-- 
2.37.1

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

[Dan Carpenter]

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@nxp.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;
> +}

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

[Dan Carpenter]

On Fri, Aug 15, 2025 at 04:50:42PM +0800, Peng Fan wrote:
> >> +		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>;
>   }
> 

Ah.  Great.  Thanks.

regards,
dan carpenter

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

[Peng Fan]

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@nxp.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;
>> +}
>

Re: [PATCH 0/3] clk: Support spread spectrum and use it in clk-scmi

[Peng Fan]

Hi Stephen, Sudeep, Cristian

On Tue, Aug 12, 2025 at 08:17:04PM +0800, Peng Fan wrote:
>Since the assigned-clock-sscs property [1] has been accepted into the device
>tree schema, we can now support it in the Linux clock driver. Therefore,
>I’ve picked up the previously submitted work [2] titled “clk: Support
>spread spectrum and use it in clk-pll144x and clk-scmi.”
>As more than six months have passed since [2] was posted, I’m treating this
>patchset as a new submission rather than a v3.
>
>- Introduce clk_set_spread_spectrum to set the parameters for enabling
>  spread spectrum of a clock.
>- Parse 'assigned-clock-sscs' and configure it by default before using the
>  clock. This property is parsed before parsing clock rate.
>- Enable this feature for clk-scmi on i.MX95.
>
>Because SCMI spec will not include spread spectrum as a standard
>extension, we still need to use NXP i.MX OEM extension.

I only got a comment from Dan until now, before I post V2, I would like
to see if CLK and SCMI maintainer's have any comments. Or I need to wait
a bit more time, please just drop me a message.

Thanks,
Peng

>
>[1] https://github.com/devicetree-org/dt-schema/pull/154
>[2] https://lore.kernel.org/all/20250205-clk-ssc-v2-0-fa73083caa92@nxp.com/
>
>Signed-off-by: Peng Fan <peng.fan@nxp.com>
>---
>Peng Fan (3):
>      clk: Introduce clk_hw_set_spread_spectrum
>      clk: conf: Support assigned-clock-sscs
>      clk: scmi: Support Spread Spectrum for NXP i.MX95
>
> drivers/clk/clk-conf.c        | 70 +++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/clk-scmi.c        | 64 ++++++++++++++++++++++++++++++++++++---
> drivers/clk/clk.c             | 32 ++++++++++++++++++++
> include/linux/clk-provider.h  | 29 ++++++++++++++++++
> include/linux/scmi_protocol.h |  5 ++++
> 5 files changed, 196 insertions(+), 4 deletions(-)
>---
>base-commit: b1549501188cc9eba732c25b033df7a53ccc341f
>change-id: 20250812-clk-ssc-version1-acf6f6efbd96
>
>Best regards,
>-- 
>Peng Fan <peng.fan@nxp.com>
>

Re: [PATCH 1/3] clk: Introduce clk_hw_set_spread_spectrum

[Brian Masney]

On Tue, Aug 12, 2025 at 08:17:05PM +0800, Peng Fan wrote:
> Add clk_hw_set_spread_spectrum to configure a clock to enable spread
> spectrum feature. set_spread_spectrum ops is added for clk drivers to
> have their own hardware specific implementation.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/clk.c            | 32 ++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b821b2cdb155331c85fafbd2fac8ab3703a08e4d..48c7a301b72b30fd824dae7ada2c44ee84d40867 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2802,6 +2802,38 @@ 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, unsigned int modfreq_hz,
> +			       unsigned int spread_bp, unsigned int method)
                                                       ^^^^^^^^^^^^
Should this be 'enum clk_ssc_method'?

Also can you add kernel docs for all of the parameters? I know it's
documented on 'struct clk_spread_spectrum' below.

What do you think about having this function take that struct instead as
a parameter to match what's on the clk op?

Brian

Re: [PATCH 1/3] clk: Introduce clk_hw_set_spread_spectrum

[Peng Fan]

On Wed, Aug 27, 2025 at 11:45:19AM -0400, Brian Masney wrote:
>On Tue, Aug 12, 2025 at 08:17:05PM +0800, Peng Fan wrote:
>> Add clk_hw_set_spread_spectrum to configure a clock to enable spread
>> spectrum feature. set_spread_spectrum ops is added for clk drivers to
>> have their own hardware specific implementation.
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/clk/clk.c            | 32 ++++++++++++++++++++++++++++++++
>>  include/linux/clk-provider.h | 29 +++++++++++++++++++++++++++++
>>  2 files changed, 61 insertions(+)
>> 
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b821b2cdb155331c85fafbd2fac8ab3703a08e4d..48c7a301b72b30fd824dae7ada2c44ee84d40867 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2802,6 +2802,38 @@ 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, unsigned int modfreq_hz,
>> +			       unsigned int spread_bp, unsigned int method)
>                                                       ^^^^^^^^^^^^
>Should this be 'enum clk_ssc_method'?
>
>Also can you add kernel docs for all of the parameters? I know it's
>documented on 'struct clk_spread_spectrum' below.
>
>What do you think about having this function take that struct instead as
>a parameter to match what's on the clk op?

Yeah. Dan has raised similar comment, I will change to use
struct clk_spread_spectrum as the 2nd param.

Thanks,
Peng

>
>Brian
>