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

[PATCH v2 0/4] clk: Support spread spectrum and use it in clk-pll144x and 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;

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

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Rename to clk_hw_set_spread_spectrum and not export it as consumer API.
- Fix error handling
- The enable parameter is still kept, because 0% is valid per
  https://www.ti.com/lit/an/scaa103/scaa103.pdf?ts=1738667308903
  https://www.synopsys.com/blogs/chip-design/understanding-pcie-spread-spectrum-clocking.html
- Include the i.MX clk pll14xx which was an effort to enable SSC on
i.MX8MN from https://lore.kernel.org/all/20250118124044.157308-1-dario.binacchi@amarulasolutions.com/
  With this patchset, things could be simplied a lot.
- Update the clk-scmi extconfig, marked as not apply, because spec not settle down.
- Link to v1: https://lore.kernel.org/linux-clk/20250124-clk-ssc-v1-0-2d39f6baf2af@nxp.com/T/#mce926ef10d3d9c1c960c21867c2f1509f1f87cb9

---
Peng Fan (4):
      clk: Introduce clk_hw_set_spread_spectrum
      clk: conf: Support assigned-clock-sscs
      clk: imx: pll14xx: support spread spectrum clock generation
      [NOT APPLY] clk: scmi: Support spread spectrum

 drivers/clk/clk-conf.c        | 70 +++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-scmi.c        | 47 +++++++++++++++++++++++++++--
 drivers/clk/clk.c             | 34 +++++++++++++++++++++
 drivers/clk/imx/clk-pll14xx.c | 66 ++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h  | 32 ++++++++++++++++++++
 include/linux/clk.h           | 22 ++++++++++++++
 include/linux/scmi_protocol.h |  6 ++++
 7 files changed, 275 insertions(+), 2 deletions(-)
---
base-commit: 40b8e93e17bff4a4e0cc129e04f9fdf5daa5397e
change-id: 20250124-clk-ssc-f3d70fb6cd1c

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

[PATCH v2 1/4] clk: Introduce clk_hw_set_spread_spectrum

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

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            | 34 ++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h | 32 ++++++++++++++++++++++++++++++++
 include/linux/clk.h          | 22 ++++++++++++++++++++++
 3 files changed, 88 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cf7720b9172ff223d86227aad144e15375ddfd86..e11f9615e683af52c719d4c8419bd30f369f301b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2790,6 +2790,40 @@ 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,
+			       unsigned int spreaddepth, enum clk_ssc_method method,
+			       bool enable)
+{
+	struct clk_spread_spectrum clk_ss;
+	struct clk_core *core;
+	int ret;
+
+	if (!hw)
+		return 0;
+
+	core = hw->core;
+
+	clk_ss.modfreq = modfreq;
+	clk_ss.spreaddepth = spreaddepth;
+	clk_ss.method = method;
+	clk_ss.enable = enable;
+
+	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 2e6e603b749342931c0d0693c3e72b62c000791b..ac0270cc9ec133954b1f8dcffed015723bd1ff5d 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -84,6 +84,28 @@ struct clk_duty {
 	unsigned int den;
 };
 
+/* Aligned with dtschema/schemas/clock/clock.yaml */
+enum clk_ssc_method {
+	CLK_SSC_CENTER_SPREAD,
+	CLK_SSC_UP_SPREAD,
+	CLK_SSC_DOWN_SPREAD,
+};
+
+/**
+ * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock
+ *
+ * @modfreq:		Modulation frequency
+ * @spreadpercent:	Modulation percent
+ * @method:		Modulation method
+ * @enable:		Modulation enable or disable
+ */
+struct clk_spread_spectrum {
+	unsigned int modfreq;
+	unsigned int spreaddepth;
+	enum clk_ssc_method method;
+	bool enable;
+};
+
 /**
  * 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 +200,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 +282,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);
@@ -1404,6 +1433,9 @@ 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,
+			       unsigned int spreaddepth, enum clk_ssc_method method,
+			       bool enable);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b607482ca77e987b9344c38f25ebb5c8d35c1d39..49a7f7eb8b03233e11cd3b92768896c4e45c4e7c 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -858,6 +858,21 @@ int clk_set_rate(struct clk *clk, unsigned long rate);
  */
 int clk_set_rate_exclusive(struct clk *clk, unsigned long rate);
 
+/**
+ * clk_set_spread_spectrum - set the spread spectrum for a clock
+ * @clk: clock source
+ * @modfreq: modulation freq
+ * @spreadpercent: modulation percentage
+ * @method: down spread, up spread, center spread or else
+ * @enable: enable or disable
+ *
+ * Configure the spread spectrum parameters for a clock.
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
+			    unsigned int spreadpercent, unsigned int method,
+			    bool enable);
 /**
  * clk_has_parent - check if a clock is a possible parent for another
  * @clk: clock source
@@ -1088,6 +1103,13 @@ static inline int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static inline int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
+					  unsigned int spreadpercent,
+					  unsigned int method, bool enable)
+{
+	return 0;
+}
+
 static inline long clk_round_rate(struct clk *clk, unsigned long rate)
 {
 	return 0;

-- 
2.37.1

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

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

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..9046a7710236839a30a72dc15c5fcebdbed5e9c6 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 = sscs[index * 3], spreaddepth = sscs[index * 3 + 1];
+		u32 method = sscs[index * 3 + 2];
+		struct clk_hw *hw;
+
+		if (modfreq || spreaddepth || 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, spreaddepth, method, true);
+			if (rc < 0)
+				pr_err("clk: couldn't set %s clk spread spectrum %u %u %u: %d\n",
+				       __clk_get_name(clk), modfreq, spreaddepth, 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 v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

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

Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-pll14xx.c | 66 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb048dcbf58db396af9d3aaf 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -20,6 +20,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)
@@ -31,15 +33,26 @@
 #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_MASK	GENMASK(1, 0)
 
 #define LOCK_TIMEOUT_US		10000
 
+enum imx_pll14xx_ssc_mod_type {
+	IMX_PLL14XX_SSC_DOWN_SPREAD,
+	IMX_PLL14XX_SSC_UP_SPREAD,
+	IMX_PLL14XX_SSC_CENTER_SPREAD,
+};
+
 struct clk_pll14xx {
 	struct clk_hw			hw;
 	void __iomem			*base;
 	enum imx_pll14xx_type		type;
 	const struct imx_pll14xx_rate_table *rate_table;
 	int rate_count;
+	struct clk_spread_spectrum	ssc_conf;
 };
 
 #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
@@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	return 0;
 }
 
+static void clk_pll1443x_enable_ssc(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->ssc_conf;
+	u32 sscg_ctrl, mfr, mrr, mod_type;
+
+	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 * pdiv * (1 << 5));
+	mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 * mfr);
+
+	switch (conf->method) {
+	case CLK_SSC_CENTER_SPREAD:
+		mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
+		break;
+	case CLK_SSC_UP_SPREAD:
+		mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
+		break;
+	case CLK_SSC_DOWN_SPREAD:
+		mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
+		break;
+	default:
+		mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
+		break;
+	}
+
+	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
+		FIELD_PREP(MRAT_CTL_MASK, mrr) |
+		FIELD_PREP(SEL_PF_MASK, mod_type);
+
+	writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
+}
+
 static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 				 unsigned long prate)
 {
@@ -370,6 +419,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);
 
+		if (pll->ssc_conf.enable)
+			clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
+
 		return 0;
 	}
 
@@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	gnrl_ctl &= ~BYPASS_MASK;
 	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
+	if (pll->ssc_conf.enable)
+		clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
+
 	return 0;
 }
 
@@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct clk_hw *hw)
 	writel_relaxed(val, pll->base + GNRL_CTL);
 }
 
+static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
+					    struct clk_spread_spectrum *clk_ss)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
+
+	memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
+
+	return 0;
+}
+
 static const struct clk_ops clk_pll1416x_ops = {
 	.prepare	= clk_pll14xx_prepare,
 	.unprepare	= clk_pll14xx_unprepare,
@@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops = {
 	.recalc_rate	= clk_pll14xx_recalc_rate,
 	.round_rate	= clk_pll1443x_round_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.37.1

[PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Support Spread Spectrum with adding scmi_clk_set_spread_spectrum

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

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 15510c2ff21c0335f5cb30677343bd4ef59c0738..56b9d0166b0170807c1a83fff391033fecee2159 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -23,6 +23,7 @@ enum scmi_clk_feats {
 	SCMI_CLK_RATE_CTRL_SUPPORTED,
 	SCMI_CLK_PARENT_CTRL_SUPPORTED,
 	SCMI_CLK_DUTY_CYCLE_SUPPORTED,
+	SCMI_CLK_SSC_SUPPORTED,
 	SCMI_CLK_FEATS_COUNT
 };
 
@@ -98,6 +99,36 @@ 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_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 (KHz)
+	 * extConfigValue[24]    - Enable/Disable
+	 * extConfigValue[31:25] - Modulation method
+	 */
+	val = FIELD_PREP(SCMI_CLOCK_EXT_SS_PERCENTAGE_MASK, clk_ss->spreaddepth);
+	val |= FIELD_PREP(SCMI_CLOCK_EXT_SS_MOD_FREQ_MASK, clk_ss->modfreq);
+	val |= FIELD_PREP(SCMI_CLOCK_EXT_SS_METHOD_MASK, clk_ss->method);
+	if (clk_ss->enable)
+		val |= SCMI_CLOCK_EXT_SS_ENABLE_MASK;
+	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
+						 SCMI_CLOCK_CFG_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, clk_ss->spreaddepth, 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,9 +347,17 @@ 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_SSC_SUPPORTED))
+		ops->set_spread_spectrum = scmi_clk_set_spread_spectrum;
+
 	return ops;
 }
 
+static const char * const scmi_clk_imxlist[] = {
+	"fsl,imx95",
+	NULL
+};
+
 /**
  * scmi_clk_ops_select() - Select a proper set of clock operations
  * @sclk: A reference to an SCMI clock descriptor
@@ -370,8 +409,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)
-		feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
+	if (ci->extended_config) {
+		if (of_machine_compatible_match(scmi_clk_imxlist))
+			feats_key |= BIT(SCMI_CLK_SSC_SUPPORTED);
+		else
+			feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
+	}
 
 	if (WARN_ON(feats_key >= db_size))
 		return NULL;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816247d24704f7ba109667a14226b67..a02a6d55568898ad0b5deed954e432415936dde2 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -79,10 +79,16 @@ struct scmi_protocol_handle;
 enum scmi_clock_oem_config {
 	SCMI_CLOCK_CFG_DUTY_CYCLE = 0x1,
 	SCMI_CLOCK_CFG_PHASE,
+	SCMI_CLOCK_CFG_SSC,
 	SCMI_CLOCK_CFG_OEM_START = 0x80,
 	SCMI_CLOCK_CFG_OEM_END = 0xFF,
 };
 
+#define SCMI_CLOCK_EXT_SS_PERCENTAGE_MASK	GENMASK(7, 0)
+#define SCMI_CLOCK_EXT_SS_MOD_FREQ_MASK		GENMASK(23, 8)
+#define SCMI_CLOCK_EXT_SS_ENABLE_MASK		BIT(24)
+#define SCMI_CLOCK_EXT_SS_METHOD_MASK		GENMASK(31, 25)
+
 /**
  * struct scmi_clk_proto_ops - represents the various operations provided
  *	by SCMI Clock Protocol

-- 
2.37.1

Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Dario Binacchi]

Hi Peng,

On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> Add support for spread spectrum clock (SSC) generation to the pll14xxx
> driver.
>
> Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

It doesn’t seem right to me.
You can’t take 90% of my patch, where the SSC management was actually
implemented,
add 10%, and consider yourself the author of the patch.
Please correct it in version 3.

Thanks and regards,
Dario

> ---
>  drivers/clk/imx/clk-pll14xx.c | 66 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb048dcbf58db396af9d3aaf 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -20,6 +20,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)
> @@ -31,15 +33,26 @@
>  #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_MASK    GENMASK(1, 0)
>
>  #define LOCK_TIMEOUT_US                10000
>
> +enum imx_pll14xx_ssc_mod_type {
> +       IMX_PLL14XX_SSC_DOWN_SPREAD,
> +       IMX_PLL14XX_SSC_UP_SPREAD,
> +       IMX_PLL14XX_SSC_CENTER_SPREAD,
> +};
> +
>  struct clk_pll14xx {
>         struct clk_hw                   hw;
>         void __iomem                    *base;
>         enum imx_pll14xx_type           type;
>         const struct imx_pll14xx_rate_table *rate_table;
>         int rate_count;
> +       struct clk_spread_spectrum      ssc_conf;
>  };
>
>  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
>         return 0;
>  }
>
> +static void clk_pll1443x_enable_ssc(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->ssc_conf;
> +       u32 sscg_ctrl, mfr, mrr, mod_type;
> +
> +       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 * pdiv * (1 << 5));
> +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 * mfr);
> +
> +       switch (conf->method) {
> +       case CLK_SSC_CENTER_SPREAD:
> +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
> +               break;
> +       case CLK_SSC_UP_SPREAD:
> +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
> +               break;
> +       case CLK_SSC_DOWN_SPREAD:
> +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> +               break;
> +       default:
> +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> +               break;
> +       }
> +
> +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
> +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
> +               FIELD_PREP(SEL_PF_MASK, mod_type);
> +
> +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
> +}
> +
>  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>                                  unsigned long prate)
>  {
> @@ -370,6 +419,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);
>
> +               if (pll->ssc_conf.enable)
> +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
> +
>                 return 0;
>         }
>
> @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>         gnrl_ctl &= ~BYPASS_MASK;
>         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>
> +       if (pll->ssc_conf.enable)
> +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
> +
>         return 0;
>  }
>
> @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct clk_hw *hw)
>         writel_relaxed(val, pll->base + GNRL_CTL);
>  }
>
> +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> +                                           struct clk_spread_spectrum *clk_ss)
> +{
> +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +
> +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
> +
> +       return 0;
> +}
> +
>  static const struct clk_ops clk_pll1416x_ops = {
>         .prepare        = clk_pll14xx_prepare,
>         .unprepare      = clk_pll14xx_unprepare,
> @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops = {
>         .recalc_rate    = clk_pll14xx_recalc_rate,
>         .round_rate     = clk_pll1443x_round_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.37.1
>


-- 

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 v2 1/4] clk: Introduce clk_hw_set_spread_spectrum

[Marco Felsch]

Hi Peng,

On 25-02-05, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> 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            | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h | 32 ++++++++++++++++++++++++++++++++
>  include/linux/clk.h          | 22 ++++++++++++++++++++++
>  3 files changed, 88 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172ff223d86227aad144e15375ddfd86..e11f9615e683af52c719d4c8419bd30f369f301b 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2790,6 +2790,40 @@ 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,
> +			       unsigned int spreaddepth, enum clk_ssc_method method,
> +			       bool enable)
> +{
> +	struct clk_spread_spectrum clk_ss;
> +	struct clk_core *core;
> +	int ret;
> +
> +	if (!hw)
> +		return 0;
> +
> +	core = hw->core;
> +
> +	clk_ss.modfreq = modfreq;
> +	clk_ss.spreaddepth = spreaddepth;
> +	clk_ss.method = method;
> +	clk_ss.enable = enable;
> +
> +	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 2e6e603b749342931c0d0693c3e72b62c000791b..ac0270cc9ec133954b1f8dcffed015723bd1ff5d 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -84,6 +84,28 @@ struct clk_duty {
>  	unsigned int den;
>  };
>  
> +/* Aligned with dtschema/schemas/clock/clock.yaml */
> +enum clk_ssc_method {
> +	CLK_SSC_CENTER_SPREAD,
> +	CLK_SSC_UP_SPREAD,
> +	CLK_SSC_DOWN_SPREAD,
> +};
> +
> +/**
> + * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock
> + *
> + * @modfreq:		Modulation frequency
> + * @spreadpercent:	Modulation percent
> + * @method:		Modulation method
> + * @enable:		Modulation enable or disable
> + */
> +struct clk_spread_spectrum {
> +	unsigned int modfreq;
> +	unsigned int spreaddepth;

Please use per mil as unit since I noticed that 0.x% is a common value
too.

Regards,
  Marco


> +	enum clk_ssc_method method;
> +	bool enable;
> +};
> +
>  /**
>   * 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 +200,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 +282,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);
> @@ -1404,6 +1433,9 @@ 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,
> +			       unsigned int spreaddepth, enum clk_ssc_method method,
> +			       bool enable);
>  
>  static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
>  {
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index b607482ca77e987b9344c38f25ebb5c8d35c1d39..49a7f7eb8b03233e11cd3b92768896c4e45c4e7c 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -858,6 +858,21 @@ int clk_set_rate(struct clk *clk, unsigned long rate);
>   */
>  int clk_set_rate_exclusive(struct clk *clk, unsigned long rate);
>  
> +/**
> + * clk_set_spread_spectrum - set the spread spectrum for a clock
> + * @clk: clock source
> + * @modfreq: modulation freq
> + * @spreadpercent: modulation percentage
> + * @method: down spread, up spread, center spread or else
> + * @enable: enable or disable
> + *
> + * Configure the spread spectrum parameters for a clock.
> + *
> + * Returns success (0) or negative errno.
> + */
> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> +			    unsigned int spreadpercent, unsigned int method,
> +			    bool enable);
>  /**
>   * clk_has_parent - check if a clock is a possible parent for another
>   * @clk: clock source
> @@ -1088,6 +1103,13 @@ static inline int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
>  	return 0;
>  }
>  
> +static inline int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> +					  unsigned int spreadpercent,
> +					  unsigned int method, bool enable)
> +{
> +	return 0;
> +}
> +
>  static inline long clk_round_rate(struct clk *clk, unsigned long rate)
>  {
>  	return 0;
> 
> -- 
> 2.37.1
> 
> 
> 

RE: [PATCH v2 1/4] clk: Introduce clk_hw_set_spread_spectrum

[Peng Fan]

> Subject: Re: [PATCH v2 1/4] clk: Introduce
> clk_hw_set_spread_spectrum
> 
> Hi Peng,
> 
> On 25-02-05, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > 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            | 34 ++++++++++++++++++++++++++++++++++
> >  include/linux/clk-provider.h | 32
> ++++++++++++++++++++++++++++++++
> >  include/linux/clk.h          | 22 ++++++++++++++++++++++
> >  3 files changed, 88 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> >
> cf7720b9172ff223d86227aad144e15375ddfd86..e11f9615e683af52c7
> 19d4c8419b
> > d30f369f301b 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2790,6 +2790,40 @@ 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,
> > +			       unsigned int spreaddepth, enum
> clk_ssc_method method,
> > +			       bool enable)
> > +{
> > +	struct clk_spread_spectrum clk_ss;
> > +	struct clk_core *core;
> > +	int ret;
> > +
> > +	if (!hw)
> > +		return 0;
> > +
> > +	core = hw->core;
> > +
> > +	clk_ss.modfreq = modfreq;
> > +	clk_ss.spreaddepth = spreaddepth;
> > +	clk_ss.method = method;
> > +	clk_ss.enable = enable;
> > +
> > +	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
> >
> 2e6e603b749342931c0d0693c3e72b62c000791b..ac0270cc9ec13395
> 4b1f8dcffed0
> > 15723bd1ff5d 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -84,6 +84,28 @@ struct clk_duty {
> >  	unsigned int den;
> >  };
> >
> > +/* Aligned with dtschema/schemas/clock/clock.yaml */ enum
> > +clk_ssc_method {
> > +	CLK_SSC_CENTER_SPREAD,
> > +	CLK_SSC_UP_SPREAD,
> > +	CLK_SSC_DOWN_SPREAD,
> > +};
> > +
> > +/**
> > + * struct clk_spread_spectrum - Structure encoding spread spectrum
> of
> > +a clock
> > + *
> > + * @modfreq:		Modulation frequency
> > + * @spreadpercent:	Modulation percent
> > + * @method:		Modulation method
> > + * @enable:		Modulation enable or disable
> > + */
> > +struct clk_spread_spectrum {
> > +	unsigned int modfreq;
> > +	unsigned int spreaddepth;
> 
> Please use per mil as unit since I noticed that 0.x% is a common value
> too.

I use " The modulation depth in permyriad " in dt-schema, since
Dario said ST chips has permyriad.

So change it to moddepth_per_myriad?

Thanks,
Peng.

> 
> Regards,
>   Marco
> 
> 
> > +	enum clk_ssc_method method;
> > +	bool enable;
> > +};
> > +
> >  /**
> >   * 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 +200,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 +282,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);
> > @@ -1404,6 +1433,9 @@ 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,
> > +			       unsigned int spreaddepth, enum
> clk_ssc_method method,
> > +			       bool enable);
> >
> >  static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw
> > *src)  { diff --git a/include/linux/clk.h b/include/linux/clk.h index
> >
> b607482ca77e987b9344c38f25ebb5c8d35c1d39..49a7f7eb8b03233e
> 11cd3b927688
> > 96c4e45c4e7c 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -858,6 +858,21 @@ int clk_set_rate(struct clk *clk, unsigned
> long rate);
> >   */
> >  int clk_set_rate_exclusive(struct clk *clk, unsigned long rate);
> >
> > +/**
> > + * clk_set_spread_spectrum - set the spread spectrum for a clock
> > + * @clk: clock source
> > + * @modfreq: modulation freq
> > + * @spreadpercent: modulation percentage
> > + * @method: down spread, up spread, center spread or else
> > + * @enable: enable or disable
> > + *
> > + * Configure the spread spectrum parameters for a clock.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> > +			    unsigned int spreadpercent, unsigned int
> method,
> > +			    bool enable);
> >  /**
> >   * clk_has_parent - check if a clock is a possible parent for another
> >   * @clk: clock source
> > @@ -1088,6 +1103,13 @@ static inline int
> clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
> >  	return 0;
> >  }
> >
> > +static inline int clk_set_spread_spectrum(struct clk *clk, unsigned
> int modfreq,
> > +					  unsigned int spreadpercent,
> > +					  unsigned int method, bool
> enable) {
> > +	return 0;
> > +}
> > +
> >  static inline long clk_round_rate(struct clk *clk, unsigned long
> > rate)  {
> >  	return 0;
> >
> > --
> > 2.37.1
> >
> >
> >

RE: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Peng Fan]

> Subject: Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum
> clock generation
> 
> Hi Peng,
> 
> On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS)
> <peng.fan@oss.nxp.com> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add support for spread spectrum clock (SSC) generation to the
> pll14xxx
> > driver.
> >
> > Co-developed-by: Dario Binacchi
> <dario.binacchi@amarulasolutions.com>
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> It doesn’t seem right to me.
> You can’t take 90% of my patch, where the SSC management was
> actually implemented, add 10%, and consider yourself the author of
> the patch.
> Please correct it in version 3.

Ah. But if you look into the patches, 10% is not accurate
per lines I change.
you could see more changes compared with your patch
https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/.

1. Use set_spread_spectrm ops
2. Use clk_spread_spectrum to replace imx_pll14xx_ssc
3. Drop imx_clk_pll14xx_ssc_parse_dt and clk_pll14xx_ssc_mod_type. This one would
 count over 50% changes. 

The logic that I only did minor update is the function
clk_pll1443x_enable_ssc with switching to use clk_spread_sectrum

If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
I take this patch in the patchset, mainly to ease your work and make
assigned-clock-sscs moving forward, considering SCMI spec needs update
(clk-scmi.c changes will not land soon).

Regards
Peng.

> 
> Thanks and regards,
> Dario
> 
> > ---
> >  drivers/clk/imx/clk-pll14xx.c | 66
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk-pll14xx.c
> > b/drivers/clk/imx/clk-pll14xx.c index
> >
> f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb
> 048dcbf58d
> > b396af9d3aaf 100644
> > --- a/drivers/clk/imx/clk-pll14xx.c
> > +++ b/drivers/clk/imx/clk-pll14xx.c
> > @@ -20,6 +20,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)
> > @@ -31,15 +33,26 @@
> >  #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_MASK    GENMASK(1, 0)
> >
> >  #define LOCK_TIMEOUT_US                10000
> >
> > +enum imx_pll14xx_ssc_mod_type {
> > +       IMX_PLL14XX_SSC_DOWN_SPREAD,
> > +       IMX_PLL14XX_SSC_UP_SPREAD,
> > +       IMX_PLL14XX_SSC_CENTER_SPREAD, };
> > +
> >  struct clk_pll14xx {
> >         struct clk_hw                   hw;
> >         void __iomem                    *base;
> >         enum imx_pll14xx_type           type;
> >         const struct imx_pll14xx_rate_table *rate_table;
> >         int rate_count;
> > +       struct clk_spread_spectrum      ssc_conf;
> >  };
> >
> >  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> > @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct
> clk_hw *hw, unsigned long drate,
> >         return 0;
> >  }
> >
> > +static void clk_pll1443x_enable_ssc(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->ssc_conf;
> > +       u32 sscg_ctrl, mfr, mrr, mod_type;
> > +
> > +       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 * pdiv * (1 << 5));
> > +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 *
> > + mfr);
> > +
> > +       switch (conf->method) {
> > +       case CLK_SSC_CENTER_SPREAD:
> > +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
> > +               break;
> > +       case CLK_SSC_UP_SPREAD:
> > +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
> > +               break;
> > +       case CLK_SSC_DOWN_SPREAD:
> > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > +               break;
> > +       default:
> > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > +               break;
> > +       }
> > +
> > +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK,
> mfr) |
> > +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
> > +               FIELD_PREP(SEL_PF_MASK, mod_type);
> > +
> > +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL); }
> > +
> >  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long
> drate,
> >                                  unsigned long prate)  { @@ -370,6
> > +419,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);
> >
> > +               if (pll->ssc_conf.enable)
> > +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > + rate.mdiv);
> > +
> >                 return 0;
> >         }
> >
> > @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct
> clk_hw *hw, unsigned long drate,
> >         gnrl_ctl &= ~BYPASS_MASK;
> >         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
> >
> > +       if (pll->ssc_conf.enable)
> > +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > + rate.mdiv);
> > +
> >         return 0;
> >  }
> >
> > @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct
> clk_hw *hw)
> >         writel_relaxed(val, pll->base + GNRL_CTL);  }
> >
> > +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> > +                                           struct clk_spread_spectrum
> > +*clk_ss) {
> > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > +
> > +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
> > +
> > +       return 0;
> > +}
> > +
> >  static const struct clk_ops clk_pll1416x_ops = {
> >         .prepare        = clk_pll14xx_prepare,
> >         .unprepare      = clk_pll14xx_unprepare,
> > @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops
> = {
> >         .recalc_rate    = clk_pll14xx_recalc_rate,
> >         .round_rate     = clk_pll1443x_round_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.37.1
> >
> 
> 
> --
> 
> 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
> 
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
> com%7Cbeaf5bdcc6694a5a1a1708dd45d701db%7C686ea1d3bc2b4c6
> fa92cd99c5c301635%7C0%7C0%7C638743511953067272%7CUnkno
> wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
> wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
> %7C%7C&sdata=UFgy1bS7QJ7qenzKFTkPBNfOGn0V89CGR9NLOBka0U
> 8%3D&reserved=0

Re: [PATCH v2 1/4] clk: Introduce clk_hw_set_spread_spectrum

[Marco Felsch]

On 25-02-06, Peng Fan wrote:
> > Subject: Re: [PATCH v2 1/4] clk: Introduce
> > clk_hw_set_spread_spectrum
> > 
> > Hi Peng,
> > 
> > On 25-02-05, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > 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            | 34 ++++++++++++++++++++++++++++++++++
> > >  include/linux/clk-provider.h | 32
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/clk.h          | 22 ++++++++++++++++++++++
> > >  3 files changed, 88 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> > >
> > cf7720b9172ff223d86227aad144e15375ddfd86..e11f9615e683af52c7
> > 19d4c8419b
> > > d30f369f301b 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -2790,6 +2790,40 @@ 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,
> > > +			       unsigned int spreaddepth, enum
> > clk_ssc_method method,
> > > +			       bool enable)
> > > +{
> > > +	struct clk_spread_spectrum clk_ss;
> > > +	struct clk_core *core;
> > > +	int ret;
> > > +
> > > +	if (!hw)
> > > +		return 0;
> > > +
> > > +	core = hw->core;
> > > +
> > > +	clk_ss.modfreq = modfreq;
> > > +	clk_ss.spreaddepth = spreaddepth;
> > > +	clk_ss.method = method;
> > > +	clk_ss.enable = enable;
> > > +
> > > +	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
> > >
> > 2e6e603b749342931c0d0693c3e72b62c000791b..ac0270cc9ec13395
> > 4b1f8dcffed0
> > > 15723bd1ff5d 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -84,6 +84,28 @@ struct clk_duty {
> > >  	unsigned int den;
> > >  };
> > >
> > > +/* Aligned with dtschema/schemas/clock/clock.yaml */ enum
> > > +clk_ssc_method {
> > > +	CLK_SSC_CENTER_SPREAD,
> > > +	CLK_SSC_UP_SPREAD,
> > > +	CLK_SSC_DOWN_SPREAD,
> > > +};
> > > +
> > > +/**
> > > + * struct clk_spread_spectrum - Structure encoding spread spectrum
> > of
> > > +a clock
> > > + *
> > > + * @modfreq:		Modulation frequency
> > > + * @spreadpercent:	Modulation percent
> > > + * @method:		Modulation method
> > > + * @enable:		Modulation enable or disable
> > > + */
> > > +struct clk_spread_spectrum {
> > > +	unsigned int modfreq;
> > > +	unsigned int spreaddepth;
> > 
> > Please use per mil as unit since I noticed that 0.x% is a common value
> > too.
> 
> I use " The modulation depth in permyriad " in dt-schema, since
> Dario said ST chips has permyriad.

Ah okay, thanks for the explanation, this wasn't clear since also the
comment says: "Modulation percent". I wouldn't call it permyriad or
per_myriad since this unit is rather rare. Instead I would call it "basis
point" so the variable name could be 'spread_bp'. While on it, we could
rename modfreq to modfreq_hz to make it clear.

Regards,
  Marco

> 
> So change it to moddepth_per_myriad?
> 
> Thanks,
> Peng.
> 
> > 
> > Regards,
> >   Marco
> > 
> > 
> > > +	enum clk_ssc_method method;
> > > +	bool enable;
> > > +};
> > > +
> > >  /**
> > >   * 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 +200,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 +282,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);
> > > @@ -1404,6 +1433,9 @@ 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,
> > > +			       unsigned int spreaddepth, enum
> > clk_ssc_method method,
> > > +			       bool enable);
> > >
> > >  static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw
> > > *src)  { diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > >
> > b607482ca77e987b9344c38f25ebb5c8d35c1d39..49a7f7eb8b03233e
> > 11cd3b927688
> > > 96c4e45c4e7c 100644
> > > --- a/include/linux/clk.h
> > > +++ b/include/linux/clk.h
> > > @@ -858,6 +858,21 @@ int clk_set_rate(struct clk *clk, unsigned
> > long rate);
> > >   */
> > >  int clk_set_rate_exclusive(struct clk *clk, unsigned long rate);
> > >
> > > +/**
> > > + * clk_set_spread_spectrum - set the spread spectrum for a clock
> > > + * @clk: clock source
> > > + * @modfreq: modulation freq
> > > + * @spreadpercent: modulation percentage
> > > + * @method: down spread, up spread, center spread or else
> > > + * @enable: enable or disable
> > > + *
> > > + * Configure the spread spectrum parameters for a clock.
> > > + *
> > > + * Returns success (0) or negative errno.
> > > + */
> > > +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> > > +			    unsigned int spreadpercent, unsigned int
> > method,
> > > +			    bool enable);
> > >  /**
> > >   * clk_has_parent - check if a clock is a possible parent for another
> > >   * @clk: clock source
> > > @@ -1088,6 +1103,13 @@ static inline int
> > clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
> > >  	return 0;
> > >  }
> > >
> > > +static inline int clk_set_spread_spectrum(struct clk *clk, unsigned
> > int modfreq,
> > > +					  unsigned int spreadpercent,
> > > +					  unsigned int method, bool
> > enable) {
> > > +	return 0;
> > > +}
> > > +
> > >  static inline long clk_round_rate(struct clk *clk, unsigned long
> > > rate)  {
> > >  	return 0;
> > >
> > > --
> > > 2.37.1
> > >
> > >
> > >
> 

Re: [PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Cristian Marussi]

On Wed, Feb 05, 2025 at 05:49:54PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Support Spread Spectrum with adding scmi_clk_set_spread_spectrum
> 

Hi,

I forwarded ATG with our latest exchange on the possibility of using a
standard OEM type instead of Vendor one if it is general enough....

...waiting for their feedback on this before reviewing further...BUT
just one comment down below

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/clk-scmi.c        | 47 +++++++++++++++++++++++++++++++++++++++++--
>  include/linux/scmi_protocol.h |  6 ++++++
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 15510c2ff21c0335f5cb30677343bd4ef59c0738..56b9d0166b0170807c1a83fff391033fecee2159 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -23,6 +23,7 @@ enum scmi_clk_feats {
>  	SCMI_CLK_RATE_CTRL_SUPPORTED,
>  	SCMI_CLK_PARENT_CTRL_SUPPORTED,
>  	SCMI_CLK_DUTY_CYCLE_SUPPORTED,
> +	SCMI_CLK_SSC_SUPPORTED,
>  	SCMI_CLK_FEATS_COUNT
>  };
>  
> @@ -98,6 +99,36 @@ 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_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 (KHz)
> +	 * extConfigValue[24]    - Enable/Disable
> +	 * extConfigValue[31:25] - Modulation method
> +	 */
> +	val = FIELD_PREP(SCMI_CLOCK_EXT_SS_PERCENTAGE_MASK, clk_ss->spreaddepth);
> +	val |= FIELD_PREP(SCMI_CLOCK_EXT_SS_MOD_FREQ_MASK, clk_ss->modfreq);
> +	val |= FIELD_PREP(SCMI_CLOCK_EXT_SS_METHOD_MASK, clk_ss->method);
> +	if (clk_ss->enable)
> +		val |= SCMI_CLOCK_EXT_SS_ENABLE_MASK;
> +	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
> +						 SCMI_CLOCK_CFG_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, clk_ss->spreaddepth, 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,9 +347,17 @@ 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_SSC_SUPPORTED))
> +		ops->set_spread_spectrum = scmi_clk_set_spread_spectrum;
> +
>  	return ops;
>  }
>  
> +static const char * const scmi_clk_imxlist[] = {
> +	"fsl,imx95",
> +	NULL
> +};
> +
>  /**
>   * scmi_clk_ops_select() - Select a proper set of clock operations
>   * @sclk: A reference to an SCMI clock descriptor
> @@ -370,8 +409,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)
> -		feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
> +	if (ci->extended_config) {
> +		if (of_machine_compatible_match(scmi_clk_imxlist))

... please NOT this also here if we use a standard OEM type :D..if it
won't be a vendor thing anymore, you should query with CONFIG_GET, OR we
should think also about adding some way in the spec to query the support
for extended configs like we do for other clock features...

Thanks,
Cristian

Re: [PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Peng Fan]

On Thu, Feb 06, 2025 at 12:26:32PM +0000, Cristian Marussi wrote:
>On Wed, Feb 05, 2025 at 05:49:54PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> Support Spread Spectrum with adding scmi_clk_set_spread_spectrum
>> 
>
>Hi,
>
>I forwarded ATG with our latest exchange on the possibility of using a
>standard OEM type instead of Vendor one if it is general enough....
>
>...waiting for their feedback on this before reviewing further...BUT
>just one comment down below
>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/clk/clk-scmi.c        | 47 +++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/scmi_protocol.h |  6 ++++++
>>  2 files changed, 51 insertions(+), 2 deletions(-)
>> 
>>  		feats_key |= BIT(SCMI_CLK_PARENT_CTRL_SUPPORTED);
>>  
>> -	if (ci->extended_config)
>> -		feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
>> +	if (ci->extended_config) {
>> +		if (of_machine_compatible_match(scmi_clk_imxlist))
>
>... please NOT this also here if we use a standard OEM type :D..if it
>won't be a vendor thing anymore, you should query with CONFIG_GET, OR we
>should think also about adding some way in the spec to query the support
>for extended configs like we do for other clock features...

I see, and I marked the title as NOT APPLY. CONFIG_GET would be heavy
for each clock. The clock attributes is better to send back what OEM type
is supported, not just a single OEM extension flag.

I posted out v2 mainly for "assigned-clock-sscs" changes, and not block
i.MX8M family spread spectrum patchset.
Also I hope patch [1,2] could got R-b or A-b from Maintainers. Then in NXP
downstream, I could pick patch [1,2] and do downstream implementation
for patch 4.

Thanks,
Peng.
>
>Thanks,
>Cristian

Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Dario Binacchi]

Hi Peng,

On Thu, Feb 6, 2025 at 1:53 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum
> > clock generation
> >
> > Hi Peng,
> >
> > On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS)
> > <peng.fan@oss.nxp.com> wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add support for spread spectrum clock (SSC) generation to the
> > pll14xxx
> > > driver.
> > >
> > > Co-developed-by: Dario Binacchi
> > <dario.binacchi@amarulasolutions.com>
> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >
> > It doesn’t seem right to me.
> > You can’t take 90% of my patch, where the SSC management was
> > actually implemented, add 10%, and consider yourself the author of
> > the patch.
> > Please correct it in version 3.
>
> Ah. But if you look into the patches, 10% is not accurate
> per lines I change.
> you could see more changes compared with your patch
> https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/.
>
> 1. Use set_spread_spectrm ops
> 2. Use clk_spread_spectrum to replace imx_pll14xx_ssc
> 3. Drop imx_clk_pll14xx_ssc_parse_dt and clk_pll14xx_ssc_mod_type. This one would
>  count over 50% changes.
>
> The logic that I only did minor update is the function
> clk_pll1443x_enable_ssc with switching to use clk_spread_sectrum

Sorry if I miscounted the lines, but here we are not considering who
actually implemented
the algorithmic part of the SSC management and all the time spent
testing the code on more
than one platform/board with each submission of the series for all 9 versions.

[1] https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/

Your changes, which are unnecessary for the clk-scmi.c changes, only
serve to support the
DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:

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

you should have proposed during the review of series [1]. You are the
NXP reviewer.

>
> If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
> I take this patch in the patchset, mainly to ease your work and make

Sorry for quoting Krzysztof again, but:
"Three months iMX8 patchsets, multiple reviews and no single comment
from you till January!"

So please, if you really want to ease my work, then remove this patch
from this series and resume
reviewing series [1].

Thanks and regards,
Dario

> assigned-clock-sscs moving forward, considering SCMI spec needs update
> (clk-scmi.c changes will not land soon).
>
> Regards
> Peng.
>
> >
> > Thanks and regards,
> > Dario
> >
> > > ---
> > >  drivers/clk/imx/clk-pll14xx.c | 66
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk-pll14xx.c
> > > b/drivers/clk/imx/clk-pll14xx.c index
> > >
> > f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb
> > 048dcbf58d
> > > b396af9d3aaf 100644
> > > --- a/drivers/clk/imx/clk-pll14xx.c
> > > +++ b/drivers/clk/imx/clk-pll14xx.c
> > > @@ -20,6 +20,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)
> > > @@ -31,15 +33,26 @@
> > >  #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_MASK    GENMASK(1, 0)
> > >
> > >  #define LOCK_TIMEOUT_US                10000
> > >
> > > +enum imx_pll14xx_ssc_mod_type {
> > > +       IMX_PLL14XX_SSC_DOWN_SPREAD,
> > > +       IMX_PLL14XX_SSC_UP_SPREAD,
> > > +       IMX_PLL14XX_SSC_CENTER_SPREAD, };
> > > +
> > >  struct clk_pll14xx {
> > >         struct clk_hw                   hw;
> > >         void __iomem                    *base;
> > >         enum imx_pll14xx_type           type;
> > >         const struct imx_pll14xx_rate_table *rate_table;
> > >         int rate_count;
> > > +       struct clk_spread_spectrum      ssc_conf;
> > >  };
> > >
> > >  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> > > @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct
> > clk_hw *hw, unsigned long drate,
> > >         return 0;
> > >  }
> > >
> > > +static void clk_pll1443x_enable_ssc(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->ssc_conf;
> > > +       u32 sscg_ctrl, mfr, mrr, mod_type;
> > > +
> > > +       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 * pdiv * (1 << 5));
> > > +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 *
> > > + mfr);
> > > +
> > > +       switch (conf->method) {
> > > +       case CLK_SSC_CENTER_SPREAD:
> > > +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
> > > +               break;
> > > +       case CLK_SSC_UP_SPREAD:
> > > +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
> > > +               break;
> > > +       case CLK_SSC_DOWN_SPREAD:
> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > > +               break;
> > > +       default:
> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > > +               break;
> > > +       }
> > > +
> > > +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK,
> > mfr) |
> > > +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
> > > +               FIELD_PREP(SEL_PF_MASK, mod_type);
> > > +
> > > +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL); }
> > > +
> > >  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long
> > drate,
> > >                                  unsigned long prate)  { @@ -370,6
> > > +419,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);
> > >
> > > +               if (pll->ssc_conf.enable)
> > > +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > > + rate.mdiv);
> > > +
> > >                 return 0;
> > >         }
> > >
> > > @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct
> > clk_hw *hw, unsigned long drate,
> > >         gnrl_ctl &= ~BYPASS_MASK;
> > >         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
> > >
> > > +       if (pll->ssc_conf.enable)
> > > +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > > + rate.mdiv);
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct
> > clk_hw *hw)
> > >         writel_relaxed(val, pll->base + GNRL_CTL);  }
> > >
> > > +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> > > +                                           struct clk_spread_spectrum
> > > +*clk_ss) {
> > > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > > +
> > > +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static const struct clk_ops clk_pll1416x_ops = {
> > >         .prepare        = clk_pll14xx_prepare,
> > >         .unprepare      = clk_pll14xx_unprepare,
> > > @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops
> > = {
> > >         .recalc_rate    = clk_pll14xx_recalc_rate,
> > >         .round_rate     = clk_pll1443x_round_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.37.1
> > >
> >
> >
> > --
> >
> > 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
> >
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> > www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
> > com%7Cbeaf5bdcc6694a5a1a1708dd45d701db%7C686ea1d3bc2b4c6
> > fa92cd99c5c301635%7C0%7C0%7C638743511953067272%7CUnkno
> > wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
> > wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
> > %7C%7C&sdata=UFgy1bS7QJ7qenzKFTkPBNfOGn0V89CGR9NLOBka0U
> > 8%3D&reserved=0



-- 

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 v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Sudeep Holla]

Hi Peng,

I apologise in advance for exploiting this thread to make my point.

On Thu, Feb 06, 2025 at 04:31:46PM +0100, Dario Binacchi wrote:
>
> Sorry if I miscounted the lines, but here we are not considering who
> actually implemented
> the algorithmic part of the SSC management and all the time spent
> testing the code on more
> than one platform/board with each submission of the series for all 9 versions.
>
> [1] https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/
>
> Your changes, which are unnecessary for the clk-scmi.c changes, only
> serve to support the
> DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:
>
> https://github.com/devicetree-org/dt-schema/pull/154
>
> you should have proposed during the review of series [1]. You are the
> NXP reviewer.
>
> >
> > If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
> > I take this patch in the patchset, mainly to ease your work and make
>
> Sorry for quoting Krzysztof again, but:
> "Three months iMX8 patchsets, multiple reviews and no single comment
> from you till January!"
>
> So please, if you really want to ease my work, then remove this patch
> from this series and resume
> reviewing series [1].
>

I had complained once in the past. I am repeating that again. You are not
new to the kernel development, yet at times I get really surprised with
the way you manage your patches and create so much confusion. It gets
extremely difficult to track what is happening if one doesn't follow all
your patches for a week(week is too lenient IMO, you manage sometime to
create same amount of confusion in just 2 days).

And as usually you ignore merge window and post a whole set of new series
on the first day of merge window. Which is fine especially if you are new
to kernel development(not true in your case though) or even otherwise if
you don't regularly track upstream cycle so much because of corporate
commitments(which may be true in your case and I am fine with that). But
you need to wait at-least a few days after the merge window so you give
every one a chance to follow your work.

And in this case, I would have avoided scmi changes are you have non-scmi
specific driver to get the core clock changes review first and then added
SCMI as it is OEM specific and we need to analyse it without other things
in flux or under discussion.

--
Regards,
Sudeep

Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Peng Fan]

Hi,

On Thu, Feb 06, 2025 at 04:31:46PM +0100, Dario Binacchi wrote:
>Hi Peng,
>
>On Thu, Feb 6, 2025 at 1:53 AM Peng Fan <peng.fan@nxp.com> wrote:
>>
>> > Subject: Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum
>> > clock generation
>> >
>> > Hi Peng,
>> >
>> > On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS)
>> > <peng.fan@oss.nxp.com> wrote:
>> > >
>> > > From: Peng Fan <peng.fan@nxp.com>
>> > >
>> > > Add support for spread spectrum clock (SSC) generation to the
>> > pll14xxx
>> > > driver.
>> > >
>> > > Co-developed-by: Dario Binacchi
>> > <dario.binacchi@amarulasolutions.com>
>> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> >
>> > It doesn’t seem right to me.
>> > You can’t take 90% of my patch, where the SSC management was
>> > actually implemented, add 10%, and consider yourself the author of
>> > the patch.
>> > Please correct it in version 3.
>>
>> Ah. But if you look into the patches, 10% is not accurate
>> per lines I change.
>> you could see more changes compared with your patch
>> https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/.
>>
>> 1. Use set_spread_spectrm ops
>> 2. Use clk_spread_spectrum to replace imx_pll14xx_ssc
>> 3. Drop imx_clk_pll14xx_ssc_parse_dt and clk_pll14xx_ssc_mod_type. This one would
>>  count over 50% changes.
>>
>> The logic that I only did minor update is the function
>> clk_pll1443x_enable_ssc with switching to use clk_spread_sectrum
>
>Sorry if I miscounted the lines, but here we are not considering who
>actually implemented
>the algorithmic part of the SSC management and all the time spent
>testing the code on more
>than one platform/board with each submission of the series for all 9 versions.
>
>[1] https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/
>
>Your changes, which are unnecessary for the clk-scmi.c changes, only
>serve to support the
>DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:
>
>https://github.com/devicetree-org/dt-schema/pull/154
>
>you should have proposed during the review of series [1]. You are the
>NXP reviewer.
>
>>
>> If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
>> I take this patch in the patchset, mainly to ease your work and make
>
>Sorry for quoting Krzysztof again, but:
>"Three months iMX8 patchsets, multiple reviews and no single comment
>from you till January!"


Sigh! I feel depressed, frustrated on such conclusion.

My previous reviewing work got ignored.

For the i.MX clk patches that changes dt-bindings. Only when the dt-binding
patches got R-b/A-b or not have big issues, I start to review the driver
changes.

So in your V1/V2, I not engage, because Krzysztof has major
comments on your bindings that needs big driver changes.

In you V3,
Krzysztof is still not happy with the binding part, so I start to propose
a potential solution to split anatop and ccm. This is in 2024 Nov-8th which is
two days just after you posted the patchset.

In your V4, you still have binding changes required from Krzysztof.
In your V6, after you got R-b/A-b from Krzysztof and addressed some
binding comments, I provided my R-b for some patches. This is in 2024 Dec-24th.

In your V8, you got my R-b for all the changes related to driver changes.
This is 2024 Jan-6th, which is 7 days after you posted the patchset.

In your V9, you added extra 5 patches. I not continue my reviewing for
the extra 5 patches.

>
>So please, if you really want to ease my work, then remove this patch
>from this series and resume
>reviewing series [1].

Krzysztof raised that "assigned-clock-sscs" makes the vendor properties
legacy, so I will not review the extra 5 patches unless the
'assigned-clock-sscs' stuff got rejected. Sorry.

I had similar situation that my access-controller v8 patch got a comment
that needs big changes. Complain does not make things good. This may not
be common, but it could suddenly jump in.

So please not blame me.

I will drop this patch in future version of this patchset.

Thanks,
Peng.

>
>Thanks and regards,
>Dario
>
>> assigned-clock-sscs moving forward, considering SCMI spec needs update
>> (clk-scmi.c changes will not land soon).
>>
>> Regards
>> Peng.
>>
>> >
>> > Thanks and regards,
>> > Dario
>> >
>> > > ---
>> > >  drivers/clk/imx/clk-pll14xx.c | 66
>> > > +++++++++++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 66 insertions(+)
>> > >
>> > > diff --git a/drivers/clk/imx/clk-pll14xx.c
>> > > b/drivers/clk/imx/clk-pll14xx.c index
>> > >
>> > f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb
>> > 048dcbf58d
>> > > b396af9d3aaf 100644
>> > > --- a/drivers/clk/imx/clk-pll14xx.c
>> > > +++ b/drivers/clk/imx/clk-pll14xx.c
>> > > @@ -20,6 +20,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)
>> > > @@ -31,15 +33,26 @@
>> > >  #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_MASK    GENMASK(1, 0)
>> > >
>> > >  #define LOCK_TIMEOUT_US                10000
>> > >
>> > > +enum imx_pll14xx_ssc_mod_type {
>> > > +       IMX_PLL14XX_SSC_DOWN_SPREAD,
>> > > +       IMX_PLL14XX_SSC_UP_SPREAD,
>> > > +       IMX_PLL14XX_SSC_CENTER_SPREAD, };
>> > > +
>> > >  struct clk_pll14xx {
>> > >         struct clk_hw                   hw;
>> > >         void __iomem                    *base;
>> > >         enum imx_pll14xx_type           type;
>> > >         const struct imx_pll14xx_rate_table *rate_table;
>> > >         int rate_count;
>> > > +       struct clk_spread_spectrum      ssc_conf;
>> > >  };
>> > >
>> > >  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
>> > > @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct
>> > clk_hw *hw, unsigned long drate,
>> > >         return 0;
>> > >  }
>> > >
>> > > +static void clk_pll1443x_enable_ssc(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->ssc_conf;
>> > > +       u32 sscg_ctrl, mfr, mrr, mod_type;
>> > > +
>> > > +       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 * pdiv * (1 << 5));
>> > > +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 *
>> > > + mfr);
>> > > +
>> > > +       switch (conf->method) {
>> > > +       case CLK_SSC_CENTER_SPREAD:
>> > > +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
>> > > +               break;
>> > > +       case CLK_SSC_UP_SPREAD:
>> > > +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
>> > > +               break;
>> > > +       case CLK_SSC_DOWN_SPREAD:
>> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
>> > > +               break;
>> > > +       default:
>> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
>> > > +               break;
>> > > +       }
>> > > +
>> > > +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK,
>> > mfr) |
>> > > +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
>> > > +               FIELD_PREP(SEL_PF_MASK, mod_type);
>> > > +
>> > > +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL); }
>> > > +
>> > >  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long
>> > drate,
>> > >                                  unsigned long prate)  { @@ -370,6
>> > > +419,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);
>> > >
>> > > +               if (pll->ssc_conf.enable)
>> > > +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
>> > > + rate.mdiv);
>> > > +
>> > >                 return 0;
>> > >         }
>> > >
>> > > @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct
>> > clk_hw *hw, unsigned long drate,
>> > >         gnrl_ctl &= ~BYPASS_MASK;
>> > >         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>> > >
>> > > +       if (pll->ssc_conf.enable)
>> > > +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
>> > > + rate.mdiv);
>> > > +
>> > >         return 0;
>> > >  }
>> > >
>> > > @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct
>> > clk_hw *hw)
>> > >         writel_relaxed(val, pll->base + GNRL_CTL);  }
>> > >
>> > > +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
>> > > +                                           struct clk_spread_spectrum
>> > > +*clk_ss) {
>> > > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
>> > > +
>> > > +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
>> > > +
>> > > +       return 0;
>> > > +}
>> > > +
>> > >  static const struct clk_ops clk_pll1416x_ops = {
>> > >         .prepare        = clk_pll14xx_prepare,
>> > >         .unprepare      = clk_pll14xx_unprepare,
>> > > @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops
>> > = {
>> > >         .recalc_rate    = clk_pll14xx_recalc_rate,
>> > >         .round_rate     = clk_pll1443x_round_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.37.1
>> > >
>> >
>> >
>> > --
>> >
>> > 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
>> >
>> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
>> > www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
>> > com%7Cbeaf5bdcc6694a5a1a1708dd45d701db%7C686ea1d3bc2b4c6
>> > fa92cd99c5c301635%7C0%7C0%7C638743511953067272%7CUnkno
>> > wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
>> > wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
>> > %7C%7C&sdata=UFgy1bS7QJ7qenzKFTkPBNfOGn0V89CGR9NLOBka0U
>> > 8%3D&reserved=0
>
>
>
>-- 
>
>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 v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Peng Fan]

Hi Sudeep,

On Thu, Feb 06, 2025 at 04:16:58PM +0000, Sudeep Holla wrote:
>Hi Peng,
>
>I apologise in advance for exploiting this thread to make my point.
>
>On Thu, Feb 06, 2025 at 04:31:46PM +0100, Dario Binacchi wrote:
>>
>> Sorry if I miscounted the lines, but here we are not considering who
>> actually implemented
>> the algorithmic part of the SSC management and all the time spent
>> testing the code on more
>> than one platform/board with each submission of the series for all 9 versions.
>>
>> [1] https://lore.kernel.org/all/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/
>>
>> Your changes, which are unnecessary for the clk-scmi.c changes, only
>> serve to support the
>> DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:
>>
>> https://github.com/devicetree-org/dt-schema/pull/154
>>
>> you should have proposed during the review of series [1]. You are the
>> NXP reviewer.
>>
>> >
>> > If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
>> > I take this patch in the patchset, mainly to ease your work and make
>>
>> Sorry for quoting Krzysztof again, but:
>> "Three months iMX8 patchsets, multiple reviews and no single comment
>> from you till January!"
>>
>> So please, if you really want to ease my work, then remove this patch
>> from this series and resume
>> reviewing series [1].
>>
>
>I had complained once in the past. I am repeating that again. You are not
>new to the kernel development, yet at times I get really surprised with
>the way you manage your patches and create so much confusion. It gets
>extremely difficult to track what is happening if one doesn't follow all
>your patches for a week(week is too lenient IMO, you manage sometime to
>create same amount of confusion in just 2 days).

V2 is actually 2 weeks after V1. So after addressing the comments
from Stephen and Dan, also updated clk-scmi.c to use a non-vendor
changes, I posted out V2.

2 days, this is just after got Cristian's comments. Then I posted V2.
I try to follow your working style on handling scmi patches, but seems you are
not active, so I mainly count on Cristian's comments and update patches.

The i.MX pll patches in V2 is orthogonal to clk scmi, I did not expect
complains. But ...

>
>And as usually you ignore merge window and post a whole set of new series
>on the first day of merge window. Which is fine especially if you are new
>to kernel development(not true in your case though) or even otherwise if
>you don't regularly track upstream cycle so much because of corporate
>commitments(which may be true in your case and I am fine with that). But
>you need to wait at-least a few days after the merge window so you give
>every one a chance to follow your work.

In my view, maintainers have patchwork to maintain patches. patches send
out in merge window will not be reviewed in short time or surely not
picked up, I understand this. patches could just be marked new in patchwork.
If new version is out, old version just marked as not apply.
And I use b4 to manage patchset, and each revision has changelog.

Indeed I not track merge window since I am not maintainer role. I was
not aware this would introduce complain (: I will track the cycle
in following days.

>
>And in this case, I would have avoided scmi changes are you have non-scmi
>specific driver to get the core clock changes review first and then added
>SCMI as it is OEM specific and we need to analyse it without other things
>in flux or under discussion.

the pll changes is orthogonal to clk scmi.

it is just like common changes + several driver changes in a patchset
and this is just an early version (V2). Such as people use devm_x to 
simplify various drivers.
1. Introduce devm_x
2. driver1: use devm_x
3. driver2: use devm_x

2 and 3 not impact each other.

My initial idea to introduce pll changes in V2 is to ease Dario's work,
but seems not. Not expect that would introduce confusion to you.

The goal is still to use assigned-clock-sscs and let clk scmi support 
spread spectrum.

Thanks,
Peng.

>
>--
>Regards,
>Sudeep

Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation

[Sudeep Holla]

On Fri, Feb 07, 2025 at 07:26:22PM +0800, Peng Fan wrote:
> Hi Sudeep,
>
> V2 is actually 2 weeks after V1. So after addressing the comments
> from Stephen and Dan, also updated clk-scmi.c to use a non-vendor
> changes, I posted out V2.
>

Sure, but as I said you posted on the very first day of the merge window.
So 2 weeks just cover the end of merge window.

> 2 days, this is just after got Cristian's comments. Then I posted V2.
> I try to follow your working style on handling scmi patches, but seems you are
> not active, so I mainly count on Cristian's comments and update patches.
>

Yes, his comments were for more discussions internally and externally.
Not to churn up another patch set.

> The i.MX pll patches in V2 is orthogonal to clk scmi, I did not expect
> complains. But ...
>

Sorry if I overlooked, but with not all the platform specific
knowledge it is just too much info to consume at once. Again it is
fine if you don't make it hard but churning newer versions. So please
give time.

> In my view, maintainers have patchwork to maintain patches. patches send
> out in merge window will not be reviewed in short time or surely not
> picked up, I understand this. patches could just be marked new in patchwork.
> If new version is out, old version just marked as not apply.

Though I don't use patchwork(probably I should not your problem).
However, sometimes I see all versions to understand the changes and
evolution sometimes. And it just gets hard if there are too many
versions in short duration.

> And I use b4 to manage patchset, and each revision has changelog.
>

Good.

> Indeed I not track merge window since I am not maintainer role. I was
> not aware this would introduce complain (: I will track the cycle
> in following days.
>

I don't say it is a must. But good if you manage to.

I will look at all the pending patches from you soon, give me until
middle of next week.

--
Regards,
Sudeep

Re: [PATCH v2 1/4] clk: Introduce clk_hw_set_spread_spectrum

[Geert Uytterhoeven]

Hi Peng,

On Wed, 5 Feb 2025 at 10:51, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> 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>

Thanks for your patch!

> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -84,6 +84,28 @@ struct clk_duty {
>         unsigned int den;
>  };
>
> +/* Aligned with dtschema/schemas/clock/clock.yaml */
> +enum clk_ssc_method {
> +       CLK_SSC_CENTER_SPREAD,
> +       CLK_SSC_UP_SPREAD,
> +       CLK_SSC_DOWN_SPREAD,
> +};
> +
> +/**
> + * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock
> + *
> + * @modfreq:           Modulation frequency
> + * @spreadpercent:     Modulation percent

E.g. Renesas R-Car V4M also supports 0.5%, 1.5%, and 2.5%.

> + * @method:            Modulation method
> + * @enable:            Modulation enable or disable
> + */
> +struct clk_spread_spectrum {
> +       unsigned int modfreq;
> +       unsigned int spreaddepth;
> +       enum clk_ssc_method method;
> +       bool enable;

Do you envision a use case for having a separate enable flag?
The alternative would be to add an extra enum value above:

    CLK_SSC_NO_SPREAD = 0,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 0/4] clk: Support spread spectrum and use it in clk-pll144x and clk-scmi

[Peng Fan (OSS)]

Hi Rob, Stephen,

> Subject: [PATCH v2 0/4] clk: Support spread spectrum and use it in clk-
> pll144x and clk-scmi

Do you have time to give a look on patch 1,2 and the bindings
part in https://github.com/devicetree-org/dt-schema/pull/154

I would like to see if you agree on this approach or not, then
I could work on next step to explore new method or else

Thanks,
Peng.

> 
> - 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;
> 
> [1] https://github.com/devicetree-org/dt-schema/pull/154
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> Changes in v2:
> - Rename to clk_hw_set_spread_spectrum and not export it as
> consumer API.
> - Fix error handling
> - The enable parameter is still kept, because 0% is valid per
>   https://www.ti.com/lit/an/scaa103/scaa103.pdf?ts=1738667308903
>   https://www.synopsys.com/blogs/chip-design/understanding-pcie-
> spread-spectrum-clocking.html
> - Include the i.MX clk pll14xx which was an effort to enable SSC on
> i.MX8MN from https://lore.kernel.org/all/20250118124044.157308-1-
> dario.binacchi@amarulasolutions.com/
>   With this patchset, things could be simplied a lot.
> - Update the clk-scmi extconfig, marked as not apply, because spec not
> settle down.
> - Link to v1: https://lore.kernel.org/linux-clk/20250124-clk-ssc-v1-0-
> 2d39f6baf2af@nxp.com/T/#mce926ef10d3d9c1c960c21867c2f1509f1
> f87cb9
> 
> ---
> Peng Fan (4):
>       clk: Introduce clk_hw_set_spread_spectrum
>       clk: conf: Support assigned-clock-sscs
>       clk: imx: pll14xx: support spread spectrum clock generation
>       [NOT APPLY] clk: scmi: Support spread spectrum
> 
>  drivers/clk/clk-conf.c        | 70
> +++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk-scmi.c        | 47 +++++++++++++++++++++++++++--
>  drivers/clk/clk.c             | 34 +++++++++++++++++++++
>  drivers/clk/imx/clk-pll14xx.c | 66
> ++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h  | 32 ++++++++++++++++++++
>  include/linux/clk.h           | 22 ++++++++++++++
>  include/linux/scmi_protocol.h |  6 ++++
>  7 files changed, 275 insertions(+), 2 deletions(-)
> ---
> base-commit: 40b8e93e17bff4a4e0cc129e04f9fdf5daa5397e
> change-id: 20250124-clk-ssc-f3d70fb6cd1c
> 
> Best regards,
> --
> Peng Fan <peng.fan@nxp.com>

Re: [PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Peng Fan]

Hi Cristian,

On Thu, Feb 06, 2025 at 12:26:32PM +0000, Cristian Marussi wrote:
>On Wed, Feb 05, 2025 at 05:49:54PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> Support Spread Spectrum with adding scmi_clk_set_spread_spectrum
>> 
>
>Hi,
>
>I forwarded ATG with our latest exchange on the possibility of using a
>standard OEM type instead of Vendor one if it is general enough....

Do you have any update?

Thanks,
Peng

>
>...waiting for their feedback on this before reviewing further...BUT
>just one comment down below
>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/clk/clk-scmi.c        | 47 +++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/scmi_protocol.h |  6 ++++++
>>  2 files changed, 51 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
>> index 15510c2ff21c0335f5cb30677343bd4ef59c0738..56b9d0166b0170807c1a83fff391033fecee2159 100644
>> --- a/drivers/clk/clk-scmi.c
>> +++ b/drivers/clk/clk-scmi.c
>> @@ -23,6 +23,7 @@ enum scmi_clk_feats {
>>  	SCMI_CLK_RATE_CTRL_SUPPORTED,
>>  	SCMI_CLK_PARENT_CTRL_SUPPORTED,
>>  	SCMI_CLK_DUTY_CYCLE_SUPPORTED,
>> +	SCMI_CLK_SSC_SUPPORTED,
>>  	SCMI_CLK_FEATS_COUNT
>>  };
>>  
>> @@ -98,6 +99,36 @@ 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_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 (KHz)
>> +	 * extConfigValue[24]    - Enable/Disable
>> +	 * extConfigValue[31:25] - Modulation method
>> +	 */
>> +	val = FIELD_PREP(SCMI_CLOCK_EXT_SS_PERCENTAGE_MASK, clk_ss->spreaddepth);
>> +	val |= FIELD_PREP(SCMI_CLOCK_EXT_SS_MOD_FREQ_MASK, clk_ss->modfreq);
>> +	val |= FIELD_PREP(SCMI_CLOCK_EXT_SS_METHOD_MASK, clk_ss->method);
>> +	if (clk_ss->enable)
>> +		val |= SCMI_CLOCK_EXT_SS_ENABLE_MASK;
>> +	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
>> +						 SCMI_CLOCK_CFG_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, clk_ss->spreaddepth, 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,9 +347,17 @@ 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_SSC_SUPPORTED))
>> +		ops->set_spread_spectrum = scmi_clk_set_spread_spectrum;
>> +
>>  	return ops;
>>  }
>>  
>> +static const char * const scmi_clk_imxlist[] = {
>> +	"fsl,imx95",
>> +	NULL
>> +};
>> +
>>  /**
>>   * scmi_clk_ops_select() - Select a proper set of clock operations
>>   * @sclk: A reference to an SCMI clock descriptor
>> @@ -370,8 +409,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)
>> -		feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
>> +	if (ci->extended_config) {
>> +		if (of_machine_compatible_match(scmi_clk_imxlist))
>
>... please NOT this also here if we use a standard OEM type :D..if it
>won't be a vendor thing anymore, you should query with CONFIG_GET, OR we
>should think also about adding some way in the spec to query the support
>for extended configs like we do for other clock features...
>
>Thanks,
>Cristian

Re: [PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Cristian Marussi]

On Mon, Mar 03, 2025 at 12:11:25PM +0800, Peng Fan wrote:
> Hi Cristian,
> 
> On Thu, Feb 06, 2025 at 12:26:32PM +0000, Cristian Marussi wrote:
> >On Wed, Feb 05, 2025 at 05:49:54PM +0800, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >> 
> >> Support Spread Spectrum with adding scmi_clk_set_spread_spectrum
> >> 
> >
> >Hi,
> >
> >I forwarded ATG with our latest exchange on the possibility of using a
> >standard OEM type instead of Vendor one if it is general enough....
> 
> Do you have any update?
> 

Yes I think you can go on with your original plan of using vendor OEM
types: as of now we are not gonna standardize a new commmon SCMI type
for Clock-SS, given there is really just one SCMI user of such clock
features...maybe in the future if more users shows up...

Thanks,
Cristian

Re: [PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Peng Fan]

On Wed, Mar 05, 2025 at 05:29:54PM +0000, Cristian Marussi wrote:
>On Mon, Mar 03, 2025 at 12:11:25PM +0800, Peng Fan wrote:
>> Hi Cristian,
>> 
>> On Thu, Feb 06, 2025 at 12:26:32PM +0000, Cristian Marussi wrote:
>> >On Wed, Feb 05, 2025 at 05:49:54PM +0800, Peng Fan (OSS) wrote:
>> >> From: Peng Fan <peng.fan@nxp.com>
>> >> 
>> >> Support Spread Spectrum with adding scmi_clk_set_spread_spectrum
>> >> 
>> >
>> >Hi,
>> >
>> >I forwarded ATG with our latest exchange on the possibility of using a
>> >standard OEM type instead of Vendor one if it is general enough....
>> 
>> Do you have any update?
>> 
>
>Yes I think you can go on with your original plan of using vendor OEM
>types: as of now we are not gonna standardize a new commmon SCMI type
>for Clock-SS, given there is really just one SCMI user of such clock
>features...maybe in the future if more users shows up...

Thanks for updating me. Back to how to add extensions in clk-scmi.c,
do you have any suggestions? I am thinking to provide vendor/sub-vendor
for clk-scmi.c and use vendor "NXP" sub-vedor "IMX" for spread spectrum.

Thanks,
Peng


>
>Thanks,
>Cristian

Re: [PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum

[Cristian Marussi]

On Mon, Mar 10, 2025 at 04:16:35PM +0800, Peng Fan wrote:
> On Wed, Mar 05, 2025 at 05:29:54PM +0000, Cristian Marussi wrote:
> >On Mon, Mar 03, 2025 at 12:11:25PM +0800, Peng Fan wrote:
> >> Hi Cristian,
> >> 
> >> On Thu, Feb 06, 2025 at 12:26:32PM +0000, Cristian Marussi wrote:
> >> >On Wed, Feb 05, 2025 at 05:49:54PM +0800, Peng Fan (OSS) wrote:
> >> >> From: Peng Fan <peng.fan@nxp.com>
> >> >> 
> >> >> Support Spread Spectrum with adding scmi_clk_set_spread_spectrum
> >> >> 
> >> >
> >> >Hi,
> >> >
> >> >I forwarded ATG with our latest exchange on the possibility of using a
> >> >standard OEM type instead of Vendor one if it is general enough....
> >> 
> >> Do you have any update?
> >> 
> >
> >Yes I think you can go on with your original plan of using vendor OEM
> >types: as of now we are not gonna standardize a new commmon SCMI type
> >for Clock-SS, given there is really just one SCMI user of such clock
> >features...maybe in the future if more users shows up...
> 
> Thanks for updating me. Back to how to add extensions in clk-scmi.c,
> do you have any suggestions? I am thinking to provide vendor/sub-vendor
> for clk-scmi.c and use vendor "NXP" sub-vedor "IMX" for spread spectrum.
> 

Definitely based on vendors subvendors, not sure about how, I have not
really though about the details: you also shoudl consider that the new
clk ops for spread spectrum should be registered ONLY when you are on a
supported platform (of course) AND the OEM types nnamespace is per-vendor:
like vendor protocols your NXP OEM config type 0x80 must coexist with any
future possible OTHER_VENDOR OEM ConfigType 0x80.

Thanks,
Cristian

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

[Peng Fan]

I am still waiting for a yes or no, or any suggestions.

Thanks,
Peng

On Mon, Feb 24, 2025 at 01:09:20PM +0000, Peng Fan (OSS) wrote:
>Hi Rob, Stephen,
>
>> Subject: [PATCH v2 0/4] clk: Support spread spectrum and use it in clk-
>> pll144x and clk-scmi
>
>Do you have time to give a look on patch 1,2 and the bindings
>part in https://github.com/devicetree-org/dt-schema/pull/154
>
>I would like to see if you agree on this approach or not, then
>I could work on next step to explore new method or else

>
>Thanks,
>Peng.
>
>> 
>> - 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;
>> 
>> [1] https://github.com/devicetree-org/dt-schema/pull/154
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>> Changes in v2:
>> - Rename to clk_hw_set_spread_spectrum and not export it as
>> consumer API.
>> - Fix error handling
>> - The enable parameter is still kept, because 0% is valid per
>>   https://www.ti.com/lit/an/scaa103/scaa103.pdf?ts=1738667308903
>>   https://www.synopsys.com/blogs/chip-design/understanding-pcie-
>> spread-spectrum-clocking.html
>> - Include the i.MX clk pll14xx which was an effort to enable SSC on
>> i.MX8MN from https://lore.kernel.org/all/20250118124044.157308-1-
>> dario.binacchi@amarulasolutions.com/
>>   With this patchset, things could be simplied a lot.
>> - Update the clk-scmi extconfig, marked as not apply, because spec not
>> settle down.
>> - Link to v1: https://lore.kernel.org/linux-clk/20250124-clk-ssc-v1-0-
>> 2d39f6baf2af@nxp.com/T/#mce926ef10d3d9c1c960c21867c2f1509f1
>> f87cb9
>> 
>> ---
>> Peng Fan (4):
>>       clk: Introduce clk_hw_set_spread_spectrum
>>       clk: conf: Support assigned-clock-sscs
>>       clk: imx: pll14xx: support spread spectrum clock generation
>>       [NOT APPLY] clk: scmi: Support spread spectrum
>> 
>>  drivers/clk/clk-conf.c        | 70
>> +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/clk-scmi.c        | 47 +++++++++++++++++++++++++++--
>>  drivers/clk/clk.c             | 34 +++++++++++++++++++++
>>  drivers/clk/imx/clk-pll14xx.c | 66
>> ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/clk-provider.h  | 32 ++++++++++++++++++++
>>  include/linux/clk.h           | 22 ++++++++++++++
>>  include/linux/scmi_protocol.h |  6 ++++
>>  7 files changed, 275 insertions(+), 2 deletions(-)
>> ---
>> base-commit: 40b8e93e17bff4a4e0cc129e04f9fdf5daa5397e
>> change-id: 20250124-clk-ssc-f3d70fb6cd1c
>> 
>> Best regards,
>> --
>> Peng Fan <peng.fan@nxp.com>
>