[PATCH v11 0/2] Support spread spectrum clocking for i.MX8M PLLs

[PATCH v11 0/2] Support spread spectrum clocking for i.MX8M PLLs

[Dario Binacchi]

The series adds support for spread spectrum clocking for i.MX8M{M,N,P}
PLLs (audio, video and DRAM). It has been tested for the video PLL on
boards using i.MX8MN and i.MX8MP.

Changes in v11:
 - Add patch "clk: validate spread spectrum configuration" to reject
   invalid parameters in the clk core before they reach the providers.
 - Fix issues claimed by Sashiko. See the details in the patch changelog.
 
Changes in v10:
- Drop patches 1-22, superseded by the generic spread spectrum clocking
  support from Peng Fan, merged in 7.3, so the i.MX8M specific infrastructure
  is no longer needed. This patch is now based on the following commits:
  - ae794aa6e88d ("dt-bindings: clock: Add spread spectrum definition")
  - c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
  - a7f16660af39 ("clk: conf: Support assigned-clock-sscs")
  - 7d98de6ffdab ("clk: Add KUnit tests for assigned-clock-sscs")
  https://lore.kernel.org/all/20260826165611.236321-1-sboyd@kernel.org/

Changes in v9:
- Add 'Reviewed-by' tag of Peng Fan for imx8mn platform patches
- Fix building warning raised by the kernel test robot for patch
  v8, 11/18 clk: imx: add support for i.MX8MN anatop clock driver
- Add patches for imx8m{m,p} platforms:
  - 23/23 clk: imx8mm: support spread spectrum clock generation
  - 22/23 clk: imx: add support for i.MX8MM anatop clock driver
  - 21/23 clk: imx8mp: support spread spectrum clock generation
  - 20/23 clk: imx8mp: rename ccm_base to base
  - 19/23 clk: imx: add support for i.MX8MP anatop clock driver

Changes in v8:
- Drop the patches added in version 7:
  - 10/23 dt-bindings: clock: imx8m-clock: add phandle to the anatop
  - 11/23 arm64: dts: imx8mm: add phandle to anatop within CCM
  - 12/23 arm64: dts: imx8mn: add phandle to anatop within CCM
  - 13/23 arm64: dts: imx8mp: add phandle to anatop within CCM
  - 14/23 arm64: dts: imx8mq: add phandle to anatop within CCM

Changes in v7:
- Add and manage fsl,anatop property as phandle to the anatop node with
  the new patches:
  - 10/23 dt-bindings: clock: imx8m-clock: add phandle to the anatop
  - 11/23 arm64: dts: imx8mm: add phandle to anatop within CCM
  - 12/23 arm64: dts: imx8mn: add phandle to anatop within CCM
  - 13/23 arm64: dts: imx8mp: add phandle to anatop within CCM
  - 14/23 arm64: dts: imx8mq: add phandle to anatop within CCM

Changes in v6:
- Merge patches:
  10/20 dt-bindings: clock: imx8mm: add binding definitions for anatop
  11/20 dt-bindings: clock: imx8mn: add binding definitions for anatop
  12/20 dt-bindings: clock: imx8mp: add binding definitions for anatop
  to
  05/20 dt-bindings: clock: imx8m-anatop: define clocks/clock-names
  now renamed
  05/18 dt-bindings: clock: imx8m-anatop: add oscillators and PLLs
- Split the patch
  15/20 dt-bindings-clock-imx8m-clock-support-spread-spectru.patch
  into
  12/18 dt-bindings: clock: imx8m-clock: add PLLs
  16/18 dt-bindings: clock: imx8m-clock: support spread spectrum clocking

Changes in v5:
- Fix compilation errors.
- Separate driver code from dt-bindings

Changes in v4:
- Add dt-bindings for anatop
- Add anatop driver
- Drop fsl,ssc-clocks from spread spectrum dt-bindings

Changes in v3:
- Patches 1/8 has been added in version 3. The dt-bindings have
  been moved from fsl,imx8m-anatop.yaml to imx8m-clock.yaml. The
  anatop device (fsl,imx8m-anatop.yaml) is indeed more or less a
  syscon, so it represents a memory area accessible by ccm
  (imx8m-clock.yaml) to setup the PLLs.
- Patches {3,5}/8 have been added in version 3.
- Patches {4,6,8}/8 use ccm device node instead of the anatop one.

Changes in v2:
- Add "allOf:" and place it after "required:" block, like in the
  example schema.
- Move the properties definition to the top-level.
- Drop unit types as requested by the "make dt_binding_check" command.

Dario Binacchi (2):
  clk: validate spread spectrum configuration
  clk: imx: pll14xx: support spread spectrum clock generation

 drivers/clk/clk.c             | 14 +++++++
 drivers/clk/imx/clk-pll14xx.c | 79 +++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

---
base-commit: 786262be6048deab760f68c8acc2c85607165894
branch: imx-pll14xx-spread-spectrum

-- 
2.43.0

[PATCH v11 1/2] clk: validate spread spectrum configuration

[Dario Binacchi]

The spread spectrum configuration is passed to the provider's
set_spread_spectrum() callback without any validation, as clk-conf.c
only skips all-zero triplets from "assigned-clock-sscs". An invalid
device tree can hand providers a zero modulation frequency or a spread
ratio above 100%, and each provider would have to add the same checks
to protect e.g. divisions in its rate computations.

Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 drivers/clk/clk.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fef87167a60b..208caf60eeb5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
 	if (!hw)
 		return 0;
 
+	switch (ss_conf->method) {
+	case CLK_SPREAD_NO:
+		break;
+	case CLK_SPREAD_CENTER:
+	case CLK_SPREAD_UP:
+	case CLK_SPREAD_DOWN:
+		if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
+		    ss_conf->spread_bp > 10000)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	core = hw->core;
 
 	clk_prepare_lock();
-- 
2.43.0

[PATCH v11 2/2] clk: imx: pll14xx: support spread spectrum clock generation

[Dario Binacchi]

Add support for spread spectrum clock (SSC) generation to the pll14xx
driver.

Tested on the video PLL of i.MX8MN and i.MX8MP based boards.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v11:
 - Drop the dt-bindings/clock/clock.h include and use the enum
   clk_ssc_method values from clk-provider.h, as clk-scmi-oem.c does.
 - Disable SSC and clear the modulation fields in SSCG_CTRL when no
   spread method is configured, so that a setup left enabled by the
   bootloader is not kept active.
 - Skip the SSC setup instead of dividing by zero when modfreq_hz is
   zero or mfr truncates to zero.
 - Reject mfr and mrr values that do not fit the MFREQ_CTL and MRAT_CTL
   register fields instead of letting FIELD_PREP silently truncate
   them. This also prevents the 10000 * mfr multiplication from
   overflowing.
 - Compute the mfr divisor in 64-bit arithmetic, as the 32-bit product
   could wrap with out of range modfreq_hz values.

Changes in v10:
- Drop 'Reviewed-by' tag of Peng Fan.
- Adapt the driver to the new infrastructure. Implement the
  set_spread_spectrum() and get the modulation parameters from
  struct clk_spread_spectrum.

Changes in v9:
- Add 'Reviewed-by' tag of Peng Fan.

 drivers/clk/imx/clk-pll14xx.c | 79 +++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index b6f1cc9f5700..0c2b812ba20c 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -13,6 +13,7 @@
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/math64.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/jiffies.h>
@@ -22,6 +23,8 @@
 #define GNRL_CTL	0x0
 #define DIV_CTL0	0x4
 #define DIV_CTL1	0x8
+#define SSCG_CTRL	0xc
+
 #define LOCK_STATUS	BIT(31)
 #define LOCK_SEL_MASK	BIT(29)
 #define CLKE_MASK	BIT(11)
@@ -33,6 +36,13 @@
 #define KDIV_MASK	GENMASK(15, 0)
 #define KDIV_MIN	SHRT_MIN
 #define KDIV_MAX	SHRT_MAX
+#define SSCG_ENABLE	BIT(31)
+#define MFREQ_CTL_MASK	GENMASK(19, 12)
+#define MRAT_CTL_MASK	GENMASK(9, 4)
+#define SEL_PF_DOWN_SPREAD	0
+#define SEL_PF_UP_SPREAD	1
+#define SEL_PF_CENTER_SPREAD	2
+#define SEL_PF_MASK	GENMASK(1, 0)
 
 #define LOCK_TIMEOUT_US		10000
 
@@ -44,6 +54,7 @@ struct clk_pll14xx {
 	int rate_count;
 	s16 delta_k;
 	spinlock_t lock;
+	struct clk_spread_spectrum ss_conf;
 };
 
 #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
@@ -366,6 +377,58 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	return 0;
 }
 
+static void __clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
+					       unsigned long parent_rate,
+					       unsigned int pdiv,
+					       unsigned int mdiv)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
+	struct clk_spread_spectrum *conf = &pll->ss_conf;
+	u32 sscg_ctrl, mfr, mrr, sel_pf;
+
+	sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
+	sscg_ctrl &= ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK | SEL_PF_MASK);
+
+	switch (conf->method) {
+	case CLK_SPREAD_CENTER:
+		sel_pf = SEL_PF_CENTER_SPREAD;
+		break;
+	case CLK_SPREAD_UP:
+		sel_pf = SEL_PF_UP_SPREAD;
+		break;
+	case CLK_SPREAD_DOWN:
+		sel_pf = SEL_PF_DOWN_SPREAD;
+		break;
+	default:
+		/* No spread: disable modulation and clear any stale state */
+		goto out;
+	}
+
+	if (!conf->modfreq_hz)
+		goto out;
+
+	mfr = div64_u64(parent_rate, (u64)conf->modfreq_hz * pdiv * BIT(5));
+	if (!mfr || mfr > FIELD_MAX(MFREQ_CTL_MASK)) {
+		pr_warn("%s: SSC disabled, modulation frequency (%u Hz) out of range\n",
+			clk_hw_get_name(hw), conf->modfreq_hz);
+		goto out;
+	}
+
+	mrr = (conf->spread_bp * mdiv * BIT(6)) / (10000 * mfr);
+	if (!mrr || mrr > FIELD_MAX(MRAT_CTL_MASK)) {
+		pr_warn("%s: SSC disabled, spread (%u permyriad) out of range\n",
+			clk_hw_get_name(hw), conf->spread_bp);
+		goto out;
+	}
+
+	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
+		FIELD_PREP(MRAT_CTL_MASK, mrr) |
+		FIELD_PREP(SEL_PF_MASK, sel_pf);
+
+out:
+	writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
+}
+
 static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 				 unsigned long prate)
 {
@@ -390,6 +453,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
 			       pll->base + DIV_CTL1);
 
+		__clk_pll1443x_set_spread_spectrum(hw, prate, rate.pdiv,
+						   rate.mdiv);
+
 		spin_unlock_irqrestore(&pll->lock, flags);
 
 		return 0;
@@ -411,6 +477,8 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 
 	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
 
+	__clk_pll1443x_set_spread_spectrum(hw, prate, rate.pdiv, rate.mdiv);
+
 	spin_unlock_irqrestore(&pll->lock, flags);
 
 	/*
@@ -437,6 +505,16 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	return 0;
 }
 
+static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
+					    const struct clk_spread_spectrum *ss_conf)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
+
+	memcpy(&pll->ss_conf, ss_conf, sizeof(pll->ss_conf));
+
+	return 0;
+}
+
 static int clk_pll14xx_prepare(struct clk_hw *hw)
 {
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
@@ -509,6 +587,7 @@ static const struct clk_ops clk_pll1443x_ops = {
 	.recalc_rate	= clk_pll14xx_recalc_rate,
 	.determine_rate = clk_pll1443x_determine_rate,
 	.set_rate	= clk_pll1443x_set_rate,
+	.set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
 };
 
 struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
-- 
2.43.0

Re: [PATCH v11 1/2] clk: validate spread spectrum configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer division truncates valid spread spectrum configurations to zero, silently disabling the feature.
--

commit f9da34cc1be223c5bb63d6e1c7898cf344a88cfe
Author: Dario Binacchi <dario.binacchi@amarulasolutions.com>
clk: validate spread spectrum configuration

This commit adds validation to the clk_hw_set_spread_spectrum function to
ensure that the modulation frequency is non-zero and the spread ratio does
not exceed 10000 basis points (100%). This prevents invalid device tree
configurations from passing incorrect values to clock providers.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
>  	if (!hw)
>  		return 0;
>  
> +	switch (ss_conf->method) {
> +	case CLK_SPREAD_NO:
> +		break;
> +	case CLK_SPREAD_CENTER:
> +	case CLK_SPREAD_UP:
> +	case CLK_SPREAD_DOWN:
> +		if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
> +		    ss_conf->spread_bp > 10000)

[Severity: High]
This isn't a bug introduced by this patch, but looking at
scmi_clk_imx_set_spread_spectrum() in drivers/clk/clk-scmi-oem.c:

scmi_clk_imx_set_spread_spectrum() {
    ...
    val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, ss_conf->spread_bp / 10000);
    ...
}

Does this integer division incorrectly assume spread_bp is a direct percentage
multiplier rather than basis points? For standard device tree configurations,
it seems this would truncate valid permyriad percentages (e.g., 100 for 1%)
to 0. Would this result in the hardware being programmed with a 0% spread
instead of the configured value, effectively disabling the feature silently?

> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	core = hw->core;
>  
>  	clk_prepare_lock();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901090912.585681-1-dario.binacchi@amarulasolutions.com?part=1

Re: [PATCH v11 2/2] clk: imx: pll14xx: support spread spectrum clock generation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] SSC settings fail to apply independently of rate changes, and bootloader settings are not cleared.
--

commit 144aa55ef6f15acf393fd40e2f5e663ff37e0de8
Author: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Date:   Tue Sep 1 09:05:18 2026 +0000

    clk: imx: pll14xx: support spread spectrum clock generation
    
    Add support for spread spectrum clock (SSC) generation to the pll14xx
    driver.
    
    Tested on the video PLL of i.MX8MN and i.MX8MP based boards.

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index b6f1cc9f57005..0c2b812ba20c7 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c

[ ... ]

> @@ -390,6 +453,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
>  			       pll->base + DIV_CTL1);
>  
> +		__clk_pll1443x_set_spread_spectrum(hw, prate, rate.pdiv,
> +						   rate.mdiv);
> +
>  		spin_unlock_irqrestore(&pll->lock, flags);
>  
>  		return 0;

[Severity: High]
Is deferring the hardware application of the SSC settings to the rate change
callback sufficient?

If the requested clock rate matches the current rate, CCF optimizes out the
call to clk_pll1443x_set_rate(). This means the hardware is never updated
with the new SSC settings if the rate remains unchanged.

[ ... ]

> @@ -437,6 +505,16 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>  	return 0;
>  }
>  
> +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> +					    const struct clk_spread_spectrum *ss_conf)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +
> +	memcpy(&pll->ss_conf, ss_conf, sizeof(pll->ss_conf));
> +
> +	return 0;
> +}

[Severity: High]
Does this callback need to program the actual hardware instead of just
caching the settings?

When CCF configures SSC via device tree (assigned-clock-sscs), it calls
clk_pll1443x_set_spread_spectrum(). By only caching the configuration in
pll->ss_conf without writing to hardware, the SSC application becomes
entirely dependent on unrelated rate changes.

[ ... ]

> @@ -509,6 +587,7 @@ static const struct clk_ops clk_pll1443x_ops = {
>  	.recalc_rate	= clk_pll14xx_recalc_rate,
>  	.determine_rate = clk_pll1443x_determine_rate,
>  	.set_rate	= clk_pll1443x_set_rate,
> +	.set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
>  };
>  
>  struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,

[Severity: High]
Should imx_dev_clk_hw_pll14xx() explicitly clear the SSCG_CTRL register
during initialization?

Since the driver currently relies on clk_pll1443x_set_rate() to disable any
bootloader SSC setups, it will fail to clear them if no spread method is
configured and the clock rate is unchanged. This can leave an active
bootloader configuration running, leading to incorrect clock behavior and
potential EMI issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901090912.585681-1-dario.binacchi@amarulasolutions.com?part=2

Re: [PATCH v11 1/2] clk: validate spread spectrum configuration

[Peng Fan]

On Tue, Sep 01, 2026 at 11:05:17AM +0200, Dario Binacchi wrote:
>The spread spectrum configuration is passed to the provider's
>set_spread_spectrum() callback without any validation, as clk-conf.c
>only skips all-zero triplets from "assigned-clock-sscs". An invalid
>device tree can hand providers a zero modulation frequency or a spread
>ratio above 100%, and each provider would have to add the same checks
>to protect e.g. divisions in its rate computations.
>
>Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
>Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>---
>
>(no changes since v1)
>
> drivers/clk/clk.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>index fef87167a60b..208caf60eeb5 100644
>--- a/drivers/clk/clk.c
>+++ b/drivers/clk/clk.c
>@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
> 	if (!hw)
> 		return 0;
> 
>+	switch (ss_conf->method) {
>+	case CLK_SPREAD_NO:
>+		break;
>+	case CLK_SPREAD_CENTER:
>+	case CLK_SPREAD_UP:
>+	case CLK_SPREAD_DOWN:
>+		if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
>+		    ss_conf->spread_bp > 10000)
>+			return -EINVAL;

I think sashiko comment is wrong.
In [1], we reached an agreement that spread "in permyriad, i.e. 0.01%".
So the input value must be the real value * 10000, saying 0.01% * 10000

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

Regards
Peng

>+		break;
>+	default:
>+		return -EINVAL;
>+	}
>+
> 	core = hw->core;
> 
> 	clk_prepare_lock();
>-- 
>2.43.0
>
>

Re: [PATCH v11 1/2] clk: validate spread spectrum configuration

[Dario Binacchi]

Hi Peng,

On Wed, Sep 2, 2026 at 10:33 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> On Tue, Sep 01, 2026 at 11:05:17AM +0200, Dario Binacchi wrote:
> >The spread spectrum configuration is passed to the provider's
> >set_spread_spectrum() callback without any validation, as clk-conf.c
> >only skips all-zero triplets from "assigned-clock-sscs". An invalid
> >device tree can hand providers a zero modulation frequency or a spread
> >ratio above 100%, and each provider would have to add the same checks
> >to protect e.g. divisions in its rate computations.
> >
> >Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
> >Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >---
> >
> >(no changes since v1)
> >
> > drivers/clk/clk.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> >diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >index fef87167a60b..208caf60eeb5 100644
> >--- a/drivers/clk/clk.c
> >+++ b/drivers/clk/clk.c
> >@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
> >       if (!hw)
> >               return 0;
> >
> >+      switch (ss_conf->method) {
> >+      case CLK_SPREAD_NO:
> >+              break;
> >+      case CLK_SPREAD_CENTER:
> >+      case CLK_SPREAD_UP:
> >+      case CLK_SPREAD_DOWN:
> >+              if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
> >+                  ss_conf->spread_bp > 10000)
> >+                      return -EINVAL;
>
> I think sashiko comment is wrong.
> In [1], we reached an agreement that spread "in permyriad, i.e. 0.01%".
> So the input value must be the real value * 10000, saying 0.01% * 10000

The DT cell is a u32 holding the integer permyriad value, i.e. the
ratio * 10000 as agreed in [1]: 1% is written as 100, 100% as 10000.
extConfigValue[7:0] takes an integer percentage, so the conversion
is a division by 100, not by 10000:

  depth   DT value   / 10000   / 100
  0.1%        10         0        0
  1%         100         0        1
  3%         300         0        3
  100%     10000         1      100

With integer arithmetic the current code programs 0% for any depth
below 100%. I am sending the fix as the first patch of v12.

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

Thanks and regards,
Dario
>
> [1] https://github.com/devicetree-org/dt-schema/pull/154
>
> Regards
> Peng
>
> >+              break;
> >+      default:
> >+              return -EINVAL;
> >+      }
> >+
> >       core = hw->core;
> >
> >       clk_prepare_lock();
> >--
> >2.43.0
> >
> >



-- 
Dario Binacchi
Senior Embedded Software Engineer
M. +39 328 0625246
dario.binacchi@amarulasolutions.com
―――――――――――――――
Amarula Solutions SRL
 Via Felice Cavallotti 25D, 41012 Carpi, MO, IT
info@amarulasolutions.com
www.amarulasolutions.com

RE: [PATCH v11 1/2] clk: validate spread spectrum configuration

[Peng Fan]

> Subject: Re: [PATCH v11 1/2] clk: validate spread spectrum
> configuration
>
> Hi Peng,
>

NXP Confidential
> On Wed, Sep 2, 2026 at 10:33 AM Peng Fan <peng.fan@oss.nxp.com>
> wrote:
> >
> > On Tue, Sep 01, 2026 at 11:05:17AM +0200, Dario Binacchi wrote:
> > >The spread spectrum configuration is passed to the provider's
> > >set_spread_spectrum() callback without any validation, as clk-conf.c
> > >only skips all-zero triplets from "assigned-clock-sscs". An invalid
> > >device tree can hand providers a zero modulation frequency or a
> > >spread ratio above 100%, and each provider would have to add the
> same
> > >checks to protect e.g. divisions in its rate computations.
> > >
> > >Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
> > >Signed-off-by: Dario Binacchi
> <dario.binacchi@amarulasolutions.com>
> > >---
> > >
> > >(no changes since v1)
> > >
> > > drivers/clk/clk.c | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > >diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> > >fef87167a60b..208caf60eeb5 100644
> > >--- a/drivers/clk/clk.c
> > >+++ b/drivers/clk/clk.c
> > >@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct
> clk_hw *hw, const struct clk_spread_spectr
> > >       if (!hw)
> > >               return 0;
> > >
> > >+      switch (ss_conf->method) {
> > >+      case CLK_SPREAD_NO:
> > >+              break;
> > >+      case CLK_SPREAD_CENTER:
> > >+      case CLK_SPREAD_UP:
> > >+      case CLK_SPREAD_DOWN:
> > >+              if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
> > >+                  ss_conf->spread_bp > 10000)
> > >+                      return -EINVAL;
> >
> > I think sashiko comment is wrong.
> > In [1], we reached an agreement that spread "in permyriad, i.e.
> 0.01%".
> > So the input value must be the real value * 10000, saying 0.01% *
> > 10000
>
> The DT cell is a u32 holding the integer permyriad value, i.e. the ratio *
> 10000 as agreed in [1]: 1% is written as 100, 100% as 10000.
> extConfigValue[7:0] takes an integer percentage, so the conversion is a
> division by 100, not by 10000:
>
>   depth   DT value   / 10000   / 100
>   0.1%        10         0        0
>   1%         100         0        1
>   3%         300         0        3
>   100%     10000         1      100
>
> With integer arithmetic the current code programs 0% for any depth
> below 100%. I am sending the fix as the first patch of v12.
>

ok, please include more pieces in your patch.

The spread_bp field is documented as "Modulation percent in permyriad"
(include/linux/clk-provider.h), where 1 unit = 0.01%, i.e. 10000 = 100%.
So the > 10000 check here correctly rejects spreads above 100%.
Actually 1000 should be enough.

However, the existing KUnit test data in kunit_clk_assigned_rates.h uses:

  #define ASSIGNED_SSCS_0_SPREAD    30000   /* 300% in permyriad */
  #define ASSIGNED_SSCS_1_SPREAD    40000   /* 400% in permyriad */

These values are above 10000 and would be rejected by the new
validation. When clk_hw_set_spread_spectrum() returns -EINVAL, clk-conf.c
prints an error but swallows it (rc = 0), and the dummy provider's
set_spread_spectrum callback is never called. So the test assertions like:

  KUNIT_EXPECT_EQ(test, ctx->clk0.sscs.spread_bp, ASSIGNED_SSCS_0_SPREAD);

will fail because ctx->clk0.sscs.spread_bp remains at its initial value.

I believe the test values were intended to represent 3% and 4% spread
(reasonable SSC values), which in permyriad should be 300 and 400,
not 30000 and 40000.

Similarly, the divisor in clk-scmi-oem.c:

  val = FIELD_PREP(..., ss_conf->spread_bp / 10000);

should be / 10.  ==> Here not / 100, because i.MX
use 10 as %1 in SCMI firmware.

The skip test data also has the same issue:

  .sscs = {50000, 60000, 3},

where 60000 should be 600 (6%) in permyriad.

Could you fold a fix for the test data and clk-scmi-oem.c divisor
into the series? Specifically:

  drivers/clk/kunit_clk_assigned_rates.h:
    ASSIGNED_SSCS_0_SPREAD: 30000 -> 300  (3%)
    ASSIGNED_SSCS_1_SPREAD: 40000 -> 400  (4%)

  drivers/clk/clk_test.c:
    .sscs = {50000, 60000, 3} -> {50000, 600, 3}  (all six instances)

  drivers/clk/clk-scmi-oem.c:
    ss_conf->spread_bp / 10000 -> ss_conf->spread_bp / 10

Thanks,
Peng.