[PATCH v10 0/1] Support spread spectrum clocking for i.MX8M PLLs

[PATCH v10 0/1] 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 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 (1):
  clk: imx: pll14xx: support spread spectrum clock generation

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

---
base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
branch: imx-pll14xx-spread-spectrum

-- 
2.43.0

[PATCH v10 1/1] 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 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 | 58 +++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index b6f1cc9f5700..27a637847f57 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -5,6 +5,7 @@
 
 #define pr_fmt(fmt) "pll14xx: " fmt
 
+#include <dt-bindings/clock/clock.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/clk-provider.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,37 @@ 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;
+
+	if (conf->method == CLK_SSC_CENTER_SPREAD)
+		sel_pf = SEL_PF_CENTER_SPREAD;
+	else if (conf->method == CLK_SSC_UP_SPREAD)
+		sel_pf = SEL_PF_UP_SPREAD;
+	else if (conf->method == CLK_SSC_DOWN_SPREAD)
+		sel_pf = SEL_PF_DOWN_SPREAD;
+	else
+		return;
+
+	sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
+	sscg_ctrl &= ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK | SEL_PF_MASK);
+
+	mfr = parent_rate / (conf->modfreq_hz * pdiv * BIT(5));
+	mrr = (conf->spread_bp * mdiv * BIT(6)) / (10000 * mfr);
+
+	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
+		FIELD_PREP(MRAT_CTL_MASK, mrr) |
+		FIELD_PREP(SEL_PF_MASK, sel_pf);
+
+	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 +432,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 +456,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 +484,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 +566,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 v10 1/1] clk: imx: pll14xx: support spread spectrum clock generation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing validation of spread spectrum parameters leads to division by zero panics.
- [High] Spread spectrum cannot be disabled and fails to clear stale hardware state.
- [Medium] Integer overflow in spread spectrum rate calculation (`mrr`).
- [Low] Missing clock infrastructure causes fatal compilation errors.
--

commit ab4effb250732b499e28c0bd53e1f0ee8a868e4c
Author: Dario Binacchi <dario.binacchi@amarulasolutions.com>

    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.
    
    Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index b6f1cc9f57005..27a637847f570 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -5,6 +5,7 @@
>  
>  #define pr_fmt(fmt) "pll14xx: " fmt
>  
> +#include <dt-bindings/clock/clock.h>

[Severity: Low]
Does this file build cleanly? It appears <dt-bindings/clock/clock.h> does
not exist. Additionally, struct clk_spread_spectrum added to struct
clk_pll14xx, and .set_spread_spectrum assigned in clk_pll1443x_ops are
undefined, which will cause fatal compilation errors.

[ ... ]

> @@ -366,6 +377,37 @@ 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;
> +
> +	if (conf->method == CLK_SSC_CENTER_SPREAD)
> +		sel_pf = SEL_PF_CENTER_SPREAD;
> +	else if (conf->method == CLK_SSC_UP_SPREAD)
> +		sel_pf = SEL_PF_UP_SPREAD;
> +	else if (conf->method == CLK_SSC_DOWN_SPREAD)
> +		sel_pf = SEL_PF_DOWN_SPREAD;
> +	else
> +		return;

[Severity: High]
Does this early return prevent spread spectrum from being disabled? If the
modulation method is unconfigured or set to disable, this path bypasses
clearing the SSCG_ENABLE bit.

If a bootloader had previously enabled spread spectrum, or if it is
dynamically disabled at runtime, the hardware may continue modulating using
stale configuration parameters alongside newly set PLL dividers.

> +
> +	sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
> +	sscg_ctrl &= ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK | SEL_PF_MASK);
> +
> +	mfr = parent_rate / (conf->modfreq_hz * pdiv * BIT(5));

[Severity: High]
Can this calculation result in a division by zero panic? If
conf->modfreq_hz is uninitialized (0), this will cause a direct division
by zero.

Furthermore, if parent_rate < conf->modfreq_hz * pdiv * 32 (which is
possible with large pdiv values and a standard 32kHz modfreq), integer
division truncates mfr to 0. This will subsequently cause a division by
zero panic during the mrr calculation while clock subsystem locks are held,
permanently deadlocking the system.

> +	mrr = (conf->spread_bp * mdiv * BIT(6)) / (10000 * mfr);

[Severity: Medium]
Does the calculation 10000 * mfr risk a 32-bit integer overflow? Since
mfr is a 32-bit unsigned integer and 10000 is a 32-bit signed literal, the
multiplication is performed using 32-bit arithmetic.

If an anomalous device tree input causes mfr to exceed 429,496, the
multiplication will silently overflow the 32-bit boundary, producing a
truncated denominator and an invalid mrr value being pushed to the
hardware.

> +
> +	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
> +		FIELD_PREP(MRAT_CTL_MASK, mrr) |
> +		FIELD_PREP(SEL_PF_MASK, sel_pf);
> +
> +	writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
> +}

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