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

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

[Dan Carpenter]

On Fri, Jan 24, 2025 at 10:25:17PM +0800, Peng Fan (OSS) wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>  
> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> +			    unsigned int spreadpercent, unsigned int method,
> +			    bool enable)
> +{
> +	struct clk_spread_spectrum clk_ss;
> +	struct clk_core *core;
> +	int ret = 0;
> +
> +	if (!clk || !clk->core)
> +		return 0;
> +
> +	clk_ss.modfreq = modfreq;
> +	clk_ss.spreadpercent = spreadpercent;
> +	clk_ss.method = method;
> +	clk_ss.enable = enable;
> +
> +	clk_prepare_lock();
> +
> +	core = clk->core;
> +
> +	if (core->prepare_count) {
> +		ret = -EBUSY;
> +		goto fail;

fail is too vague to be useful as a name.  Also this should be
goto unlock;

> +	}
> +
> +	ret = clk_pm_runtime_get(core);
> +	if (ret)
> +		goto fail;
> +
> +	if (core->ops->set_spread_spectrum)
> +		ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
> +
> +	clk_pm_runtime_put(core);
unlock:

> +	clk_prepare_unlock();
> +fail:
> +	return ret;
> +}

regards,
dan carpenter

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

[Peng Fan (OSS)]

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

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

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

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

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

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

[PATCH 1/3] clk: Introduce clk_set_spread_spectrum

[Peng Fan (OSS)]

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

Add clk_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            | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h | 22 ++++++++++++++++++++++
 include/linux/clk.h          | 22 ++++++++++++++++++++++
 3 files changed, 83 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
 }
 EXPORT_SYMBOL_GPL(clk_set_max_rate);
 
+int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
+			    unsigned int spreadpercent, unsigned int method,
+			    bool enable)
+{
+	struct clk_spread_spectrum clk_ss;
+	struct clk_core *core;
+	int ret = 0;
+
+	if (!clk || !clk->core)
+		return 0;
+
+	clk_ss.modfreq = modfreq;
+	clk_ss.spreadpercent = spreadpercent;
+	clk_ss.method = method;
+	clk_ss.enable = enable;
+
+	clk_prepare_lock();
+
+	core = clk->core;
+
+	if (core->prepare_count) {
+		ret = -EBUSY;
+		goto fail;
+	}
+
+	ret = clk_pm_runtime_get(core);
+	if (ret)
+		goto fail;
+
+	if (core->ops->set_spread_spectrum)
+		ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
+
+	clk_pm_runtime_put(core);
+	clk_prepare_unlock();
+fail:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_spread_spectrum);
+
 /**
  * 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..478005f4d53ed0698ea17331730c755e08ea7984 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -84,6 +84,21 @@ struct clk_duty {
 	unsigned int den;
 };
 
+/**
+ * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock
+ *
+ * @modfreq:		Modulation frequency
+ * @spreadpercent:	Modulation percent
+ * @method:		Modulation method
+ * @enable:		enable or disable modulation
+ */
+struct clk_spread_spectrum {
+	unsigned int modfreq;
+	unsigned int spreadpercent;
+	unsigned int 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 +193,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 +275,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);
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.35.3

[PATCH 2/3] 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 spreadpercent
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 | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 303a0bb26e54a95655ce094a35b989c97ebc6fd8..1ca4222caa829223d65dd37c7a776d43dc421944 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -155,6 +155,70 @@ 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], spreadpercent = sscs[index * 3 + 1];
+		u32 method = sscs[index * 3 + 2];
+
+		if (modfreq || spreadpercent || 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);
+			}
+
+			rc = clk_set_spread_spectrum(clk, modfreq, spreadpercent, 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, spreadpercent, 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 +238,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.35.3

[PATCH 3/3] clk: scmi: Support spread spectrum

[Peng Fan (OSS)]

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

Support Spread Spectrum for i.MX95 with adding
scmi_clk_set_spread_spectrum_imx

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

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 15510c2ff21c0335f5cb30677343bd4ef59c0738..e43902aea6bee3633f8328acddcf54eef907b640 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -98,6 +98,35 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index)
 	return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index);
 }
 
+static int scmi_clk_set_spread_spectrum_imx(struct clk_hw *hw,
+					    struct clk_spread_spectrum *clk_ss)
+{
+	struct scmi_clk *clk = to_scmi_clk(hw);
+	int ret;
+	u32 val;
+
+	/* SCMI OEM Duty Cycle is expressed as a percentage */
+	/*
+	 * extConfigValue[7:0]   - spread percentage (%)
+	 * extConfigValue[23:8]  - Modulation Frequency (KHz)
+	 * extConfigValue[24]    - Enable/Disable
+	 * extConfigValue[31:25] - Reserved
+	 */
+	val = FIELD_PREP(IMX_CLOCK_EXT_SS_PERCENTAGE_MASK, clk_ss->spreadpercent);
+	val |= FIELD_PREP(IMX_CLOCK_EXT_SS_MOD_FREQ_MASK, clk_ss->modfreq);
+	val |= IMX_CLOCK_EXT_SS_ENABLE_MASK;
+	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
+						 SCMI_CLOCK_CFG_NXP_IMX_SSC,
+						 val, false);
+	if (ret)
+		dev_warn(clk->dev,
+			 "Failed to set spread spectrum(%u,%u,%u) for clock ID %d\n",
+			 clk_ss->modfreq, clk_ss->spreadpercent, 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);
@@ -266,6 +295,11 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
  * Return: A pointer to the allocated and configured clk_ops on success,
  *	   or NULL on allocation failure.
  */
+static const char * const scmi_clk_ssc_allowlist[] = {
+	"fsl,imx95",
+	NULL
+};
+
 static const struct clk_ops *
 scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
 {
@@ -316,6 +350,9 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
 		ops->set_duty_cycle = scmi_clk_set_duty_cycle;
 	}
 
+	if (of_machine_compatible_match(scmi_clk_ssc_allowlist))
+		ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
+
 	return ops;
 }
 
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816247d24704f7ba109667a14226b67..7012d5efef00eb7b52f17d0f3d8d69f3d0063557 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -80,9 +80,14 @@ enum scmi_clock_oem_config {
 	SCMI_CLOCK_CFG_DUTY_CYCLE = 0x1,
 	SCMI_CLOCK_CFG_PHASE,
 	SCMI_CLOCK_CFG_OEM_START = 0x80,
+	SCMI_CLOCK_CFG_NXP_IMX_SSC = 0x80,
 	SCMI_CLOCK_CFG_OEM_END = 0xFF,
 };
 
+#define IMX_CLOCK_EXT_SS_PERCENTAGE_MASK	GENMASK(7, 0)
+#define IMX_CLOCK_EXT_SS_MOD_FREQ_MASK		GENMASK(23, 8)
+#define IMX_CLOCK_EXT_SS_ENABLE_MASK		BIT(24)
+
 /**
  * struct scmi_clk_proto_ops - represents the various operations provided
  *	by SCMI Clock Protocol

-- 
2.35.3

Re: [PATCH 3/3] clk: scmi: Support spread spectrum

[kernel test robot]

Hi Peng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5ffa57f6eecefababb8cbe327222ef171943b183]

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/clk-Introduce-clk_set_spread_spectrum/20250124-212050
base:   5ffa57f6eecefababb8cbe327222ef171943b183
patch link:    https://lore.kernel.org/r/20250124-clk-ssc-v1-3-2d39f6baf2af%40nxp.com
patch subject: [PATCH 3/3] clk: scmi: Support spread spectrum
config: i386-buildonly-randconfig-005-20250125 (https://download.01.org/0day-ci/archive/20250125/202501250520.evxxfDdY-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250125/202501250520.evxxfDdY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501250520.evxxfDdY-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clk/clk-scmi.c:298: warning: cannot understand function prototype: 'const char * const scmi_clk_ssc_allowlist[] = '


vim +298 drivers/clk/clk-scmi.c

   286	
   287	/**
   288	 * scmi_clk_ops_alloc() - Alloc and configure clock operations
   289	 * @dev: A device reference for devres
   290	 * @feats_key: A bitmap representing the desired clk_ops capabilities
   291	 *
   292	 * Allocate and configure a proper set of clock operations depending on the
   293	 * specifically required SCMI clock features.
   294	 *
   295	 * Return: A pointer to the allocated and configured clk_ops on success,
   296	 *	   or NULL on allocation failure.
   297	 */
 > 298	static const char * const scmi_clk_ssc_allowlist[] = {
   299		"fsl,imx95",
   300		NULL
   301	};
   302	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Dario Binacchi]

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

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

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

Thanks and regards,
Dario

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


-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

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

www.amarulasolutions.com

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

[Krzysztof Kozlowski]

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

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

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

So what exactly varies? Come with specifics.

To me look addressing exactly the same.

Best regards,
Krzysztof

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

[Dario Binacchi]

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

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

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

Thanks and regards,
Dario

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



-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

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

www.amarulasolutions.com

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

[Krzysztof Kozlowski]

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

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

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

Best regards,
Krzysztof

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

[Dario Binacchi]

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

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

Thanks and regards,
Dario

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



-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

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

www.amarulasolutions.com

Re: [PATCH 3/3] clk: scmi: Support spread spectrum

[Cristian Marussi]

On Fri, Jan 24, 2025 at 10:25:19PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Support Spread Spectrum for i.MX95 with adding
> scmi_clk_set_spread_spectrum_imx
> 

[CC: Souvik from ATG]

Hi Peng,

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/clk-scmi.c        | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h |  5 +++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 15510c2ff21c0335f5cb30677343bd4ef59c0738..e43902aea6bee3633f8328acddcf54eef907b640 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -98,6 +98,35 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index)
>  	return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index);
>  }
>  
> +static int scmi_clk_set_spread_spectrum_imx(struct clk_hw *hw,
> +					    struct clk_spread_spectrum *clk_ss)
> +{
> +	struct scmi_clk *clk = to_scmi_clk(hw);
> +	int ret;
> +	u32 val;
> +
> +	/* SCMI OEM Duty Cycle is expressed as a percentage */
> +	/*
> +	 * extConfigValue[7:0]   - spread percentage (%)
> +	 * extConfigValue[23:8]  - Modulation Frequency (KHz)
> +	 * extConfigValue[24]    - Enable/Disable
> +	 * extConfigValue[31:25] - Reserved
> +	 */
> +	val = FIELD_PREP(IMX_CLOCK_EXT_SS_PERCENTAGE_MASK, clk_ss->spreadpercent);
> +	val |= FIELD_PREP(IMX_CLOCK_EXT_SS_MOD_FREQ_MASK, clk_ss->modfreq);
> +	val |= IMX_CLOCK_EXT_SS_ENABLE_MASK;
> +	ret = scmi_proto_clk_ops->config_oem_set(clk->ph, clk->id,
> +						 SCMI_CLOCK_CFG_NXP_IMX_SSC,

If this is determined to be general enough (as per other mail in this
thread), since it effectively provides a new general clock framework
callback, I wonder if we should not try to make this straight away one
of the standard SCMI Clock Extended config types by adding it as a new
0x3 Extended config type value in the SCMI v3.2 Table 16 (with the above
extConfigValue synatx too)...

...that would mean having 0x3 reserved already for this in the upcoming
v3.3....but of course ATG has to agree on this so I copied Souvik.

In this way we could just get rid of the Vendor customization...if NOT I
would certainly base this Vendor OEM type extension on the SCMI FW-provided
vendor_info as you mentioned in the cover-letter, instead of compatibles.

Either way, it would also be wise to check if the specific Extended
config type is supported by the specific FW version (despite the version)
before registering a callback that could then always fail due to a missing
feature; currently, in fact, we do NOT take this precaution for for Duty
cycle callbacks and just assume that if SCMI Clocks extended configs are
suppported, all the standard ones are supported: this seems NOT right
BUT the only way to assure that an Extended config type is supported, as
of now, would be to query the current extended_config with CLOCK_CONFIG_GET
and see what the FW replies...this would allow us to avoid registering
unsupported features (like DutyCycle or SSC) with the core Clock framework
if NOT really supported by the running SCMI server...which in turn would
mean,potentially, 1 more SCMI message exchange per-clock at initialization
time, and I know this overhead is not always welcomed :D

> +						 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->spreadpercent, 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);
> @@ -266,6 +295,11 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
>   * Return: A pointer to the allocated and configured clk_ops on success,
>   *	   or NULL on allocation failure.
>   */
> +static const char * const scmi_clk_ssc_allowlist[] = {
> +	"fsl,imx95",
> +	NULL
> +};

Fw vednor info would be better as you said, if we stick to a Vendor
implementation...

> +
>  static const struct clk_ops *
>  scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>  {
> @@ -316,6 +350,9 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>  		ops->set_duty_cycle = scmi_clk_set_duty_cycle;
>  	}
>  
> +	if (of_machine_compatible_match(scmi_clk_ssc_allowlist))
> +		ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
> +
>  	return ops;
>  }
>  
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 688466a0e816247d24704f7ba109667a14226b67..7012d5efef00eb7b52f17d0f3d8d69f3d0063557 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -80,9 +80,14 @@ enum scmi_clock_oem_config {
>  	SCMI_CLOCK_CFG_DUTY_CYCLE = 0x1,
>  	SCMI_CLOCK_CFG_PHASE,
>  	SCMI_CLOCK_CFG_OEM_START = 0x80,
> +	SCMI_CLOCK_CFG_NXP_IMX_SSC = 0x80,

If using a Vendor OEM type, I feel this should be somehow defined
per-vendor....you cannot just grab 0x80 Extended config type for NXP
because you arrived first :P

>  	SCMI_CLOCK_CFG_OEM_END = 0xFF,
>  };
>  
> +#define IMX_CLOCK_EXT_SS_PERCENTAGE_MASK	GENMASK(7, 0)
> +#define IMX_CLOCK_EXT_SS_MOD_FREQ_MASK		GENMASK(23, 8)
> +#define IMX_CLOCK_EXT_SS_ENABLE_MASK		BIT(24)
> +

Same...I feel the best would be to just add a standard 0x3 SSC Extended
type as said...lets see what Souvik says and if we can assume such 0x3
AND the above extConfigValue syntax to be reserved for this usage BEFORE
v3.3 is out...

Thans,
Cristian

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

[Stephen Boyd]

Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>  
> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> +                           unsigned int spreadpercent, unsigned int method,
> +                           bool enable)
> +{
> +       struct clk_spread_spectrum clk_ss;
> +       struct clk_core *core;
> +       int ret = 0;

The assignment looks unnecessary.

> +
> +       if (!clk || !clk->core)

How do you not have clk->core?

> +               return 0;
> +
> +       clk_ss.modfreq = modfreq;
> +       clk_ss.spreadpercent = spreadpercent;
> +       clk_ss.method = method;
> +       clk_ss.enable = enable;
> +
> +       clk_prepare_lock();
> +
> +       core = clk->core;

Why do we need to get the core under the lock?

> +
> +       if (core->prepare_count) {

Why does prepare count matter?

> +               ret = -EBUSY;
> +               goto fail;

We just left without releasing the lock.

> +       }
> +
> +       ret = clk_pm_runtime_get(core);
> +       if (ret)
> +               goto fail;

We just left without releasing the lock.

> +
> +       if (core->ops->set_spread_spectrum)
> +               ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
> +
> +       clk_pm_runtime_put(core);
> +       clk_prepare_unlock();
> +fail:
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_spread_spectrum);
> +
> 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

Did we get cut off?

> + * @enable: enable or disable

Isn't 'disable' equal to spread_percent of zero?

> + *
> + * 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,

Does this need to be a consumer API at all? Usually SSC is figured out
when making a board and you have to pass some certification testing
because some harmonics are interfering. Is the DT property sufficient
for now and then we can do it when the driver probes in the framework?

> +                           unsigned int spreadpercent, unsigned int method,

I'd assume 'method' would be some sort of enum?

> +                           bool enable);
>  /**
>   * clk_has_parent - check if a clock is a possible parent for another
>   * @clk: clock source

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

[Peng Fan]

On Tue, Jan 28, 2025 at 12:25:28PM -0800, Stephen Boyd wrote:
>Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>>  
>> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
>> +                           unsigned int spreadpercent, unsigned int method,
>> +                           bool enable)
>> +{
>> +       struct clk_spread_spectrum clk_ss;
>> +       struct clk_core *core;
>> +       int ret = 0;
>
>The assignment looks unnecessary.

To avoid uninitialized variable warning.

>
>> +
>> +       if (!clk || !clk->core)
>
>How do you not have clk->core?
>
>> +               return 0;
>> +
>> +       clk_ss.modfreq = modfreq;
>> +       clk_ss.spreadpercent = spreadpercent;
>> +       clk_ss.method = method;
>> +       clk_ss.enable = enable;
>> +
>> +       clk_prepare_lock();
>> +
>> +       core = clk->core;
>
>Why do we need to get the core under the lock?

Drop in v2.

>
>> +
>> +       if (core->prepare_count) {
>
>Why does prepare count matter?

I was thinking to configure Spread Spectrum(SS) before
prepare/enable a clock. But it should be fine to not
check prepare count.

>
>> +               ret = -EBUSY;
>> +               goto fail;
>
>We just left without releasing the lock.

True. Dan also reported this. Fix in V2.

>
>> +       }
>> +
>> +       ret = clk_pm_runtime_get(core);
>> +       if (ret)
>> +               goto fail;
>
>We just left without releasing the lock.
>
>> +
>> +       if (core->ops->set_spread_spectrum)
>> +               ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
>> +
>> +       clk_pm_runtime_put(core);
>> +       clk_prepare_unlock();
>> +fail:
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_spread_spectrum);
>> +
>> 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
>
>Did we get cut off?

Sorry I not get this point.

>
>> + * @enable: enable or disable
>
>Isn't 'disable' equal to spread_percent of zero?

yeah. Drop the last parameter.

>
>> + *
>> + * 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,
>
>Does this need to be a consumer API at all? Usually SSC is figured out
>when making a board and you have to pass some certification testing
>because some harmonics are interfering. Is the DT property sufficient
>for now and then we can do it when the driver probes in the framework?

I suppose 'DT property' you are refering the stm32 and i.MX8M SSC patchsets.
I am proposing a generic interface for drivers to enable SSC.
Otherwise we need to introduce vendor properties for each vendor.
And looking at clk-scmi.c, we need a generic way to enable SSC, I think SCMI
maintainers not agree to add vendor properties for it.

>
>> +                           unsigned int spreadpercent, unsigned int method,
>
>I'd assume 'method' would be some sort of enum?

sure. Fix in V2.

Thanks,
Peng

>
>> +                           bool enable);
>>  /**
>>   * clk_has_parent - check if a clock is a possible parent for another
>>   * @clk: clock source

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

[Cristian Marussi]

On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote:
> On Tue, Jan 28, 2025 at 12:25:28PM -0800, Stephen Boyd wrote:
> >Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
> >>  }
> >>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
> >>  
> >> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
> >> +                           unsigned int spreadpercent, unsigned int method,
> >> +                           bool enable)
> >> +{
> >> +       struct clk_spread_spectrum clk_ss;
> >> +       struct clk_core *core;
> >> +       int ret = 0;
> >
> >The assignment looks unnecessary.
> 
> To avoid uninitialized variable warning.
> 
> >
> >> +
> >> +       if (!clk || !clk->core)
> >
> >How do you not have clk->core?
> >
> >> +               return 0;
> >> +
> >> +       clk_ss.modfreq = modfreq;
> >> +       clk_ss.spreadpercent = spreadpercent;
> >> +       clk_ss.method = method;
> >> +       clk_ss.enable = enable;
> >> +
> >> +       clk_prepare_lock();
> >> +
> >> +       core = clk->core;
> >
> >Why do we need to get the core under the lock?
> 
> Drop in v2.
> 
> >
> >> +
> >> +       if (core->prepare_count) {
> >
> >Why does prepare count matter?
> 
> I was thinking to configure Spread Spectrum(SS) before
> prepare/enable a clock. But it should be fine to not
> check prepare count.
> 
> >
> >> +               ret = -EBUSY;
> >> +               goto fail;
> >
> >We just left without releasing the lock.
> 
> True. Dan also reported this. Fix in V2.
> 
> >
> >> +       }
> >> +
> >> +       ret = clk_pm_runtime_get(core);
> >> +       if (ret)
> >> +               goto fail;
> >
> >We just left without releasing the lock.
> >
> >> +
> >> +       if (core->ops->set_spread_spectrum)
> >> +               ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
> >> +
> >> +       clk_pm_runtime_put(core);
> >> +       clk_prepare_unlock();
> >> +fail:
> >> +       return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(clk_set_spread_spectrum);
> >> +
> >> 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
> >
> >Did we get cut off?
> 
> Sorry I not get this point.
> 
> >
> >> + * @enable: enable or disable
> >
> >Isn't 'disable' equal to spread_percent of zero?
> 
> yeah. Drop the last parameter.
> 
> >
> >> + *
> >> + * 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,
> >
> >Does this need to be a consumer API at all? Usually SSC is figured out
> >when making a board and you have to pass some certification testing
> >because some harmonics are interfering. Is the DT property sufficient
> >for now and then we can do it when the driver probes in the framework?
> 
> I suppose 'DT property' you are refering the stm32 and i.MX8M SSC patchsets.
> I am proposing a generic interface for drivers to enable SSC.
> Otherwise we need to introduce vendor properties for each vendor.
> And looking at clk-scmi.c, we need a generic way to enable SSC, I think SCMI
> maintainers not agree to add vendor properties for it.
> 

To clarify, from the SCMI point of view, I expressed the idea that it
would make sense to have a common SSC interface on the SCMI backend too
instead of a custom NXP since you are adding a common CLK framework feature,
BUT only if it turns out, from this discussion, that a common general way of
configuring SSC can be found...and I dont know that, so I am waiting to see
what this discussion with CLK framework and iMX maintainers goes before
excluding the SCMI CLK vendor OEM types scenario...it would be ideal and
easier NOT to use SCMI vendor extensions BUT ONLY if this NXP SSC/config generic
solution is deemed to be really generic and usable by any other vendor.

Thanks,
Cristian

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

[Cristian Marussi]

On Mon, Feb 03, 2025 at 07:47:22PM +0800, Peng Fan wrote:
> On Mon, Feb 03, 2025 at 09:43:39AM +0000, Cristian Marussi wrote:
> >On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote:
> >> On Tue, Jan 28, 2025 at 12:25:28PM -0800, Stephen Boyd wrote:
> >> >Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
> >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> >> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
> >> >> --- a/drivers/clk/clk.c
> >> >> +++ b/drivers/clk/clk.c

Hi Peng,

> >> >> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(clk_set_max_rate);

[snip]

> >> >
> >> >> + *
> >> >> + * 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,
> >> >
> >> >Does this need to be a consumer API at all? Usually SSC is figured out
> >> >when making a board and you have to pass some certification testing
> >> >because some harmonics are interfering. Is the DT property sufficient
> >> >for now and then we can do it when the driver probes in the framework?
> >> 
> >> I suppose 'DT property' you are refering the stm32 and i.MX8M SSC patchsets.
> >> I am proposing a generic interface for drivers to enable SSC.
> >> Otherwise we need to introduce vendor properties for each vendor.
> >> And looking at clk-scmi.c, we need a generic way to enable SSC, I think SCMI
> >> maintainers not agree to add vendor properties for it.
> >> 
> >
> >To clarify, from the SCMI point of view, I expressed the idea that it
> >would make sense to have a common SSC interface on the SCMI backend too
> >instead of a custom NXP since you are adding a common CLK framework feature,
> >BUT only if it turns out, from this discussion, that a common general way of
> >configuring SSC can be found...and I dont know that, so I am waiting to see
> >what this discussion with CLK framework and iMX maintainers goes before
> >excluding the SCMI CLK vendor OEM types scenario...it would be ideal and
> >easier NOT to use SCMI vendor extensions BUT ONLY if this NXP SSC/config generic
> >solution is deemed to be really generic and usable by any other vendor.
> 
> You may misunderstand. Using DT properties for clk-scmi.c to configure SSC
> of a single clock means to add properties under clk scmi node, such
> as:
> "arm,modfreq = <x>, <y>, <z>;
>  arm,moddepth = <a>, <b>, <c>;
>  arm,modmethod = <j>, <l>, <m>;"
> 
> And during probe in clk-scmi.c, the properties needs to be parsed and when
> clk is configured, the ssc settings need to be passed to scmi platform.
> 
> But per the i.MX8M SSC patchset that DT maintainers raised, 
> the properties(fsl,modfreq and etc) needs to in consumer side,
> not provider side.
> 
> So it is not feasible to add properties here.
> 

Thanks for the clarification.

> "assigned-clock-sscs" could be put under consumer node and clocks
> could be configured with SSC when needed. This is a generic property.
> 

Yes I understood this, the property that describes SSC that you are
adding is generic BUT are also the related params needed to describe
effectively the SSC

IOW, if we add, as desirable, a generic new SSC type in extended configs
instead of using a vendor oem, will these info down below passed to the SCMI:

+       /*
+        * extConfigValue[7:0]   - spread percentage (%)
+        * extConfigValue[23:8]  - Modulation Frequency (KHz)
+        * extConfigValue[24]    - Enable/Disable
+        * extConfigValue[31:25] - Reserved
+        */


... be enough to describe the required SSC config to any generic SCMI server
from any vendor using any HW ?

... or is it plausible and maybe frequent/usual that other vendors could
need additional specific params to be fed to the server, so that we
will end up using the new standard SSC only for NXP ?

IOW the property is generic, agreed, but are the described params above
generic enough too ? ... that was my concern from an UN-educated point
of view...of course, I am talking about the most common scenarios, if
some other vendor ends up needing some arcane/magic specific param that
cannot fit the above, they will be relegated to the OEM custom spaces
even if this common SSC is available.

> Back to NXP SCMI vendor extension, if SCMI spec could include SSC, NXP
> SCMI could move to align with spec and not use vendor extension.
> 

Agreed, conditional to the above doubts.

Apologies if I have asked dumb/obvious questions, but I am not familiar
with the subject matter enough...

Thanks,
Cristian

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

[Peng Fan]

On Mon, Feb 03, 2025 at 09:43:39AM +0000, Cristian Marussi wrote:
>On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote:
>> On Tue, Jan 28, 2025 at 12:25:28PM -0800, Stephen Boyd wrote:
>> >Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
>> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
>> >> --- a/drivers/clk/clk.c
>> >> +++ b/drivers/clk/clk.c
>> >> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>> >>  
>> >> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
>> >> +                           unsigned int spreadpercent, unsigned int method,
>> >> +                           bool enable)
>> >> +{
>> >> +       struct clk_spread_spectrum clk_ss;
>> >> +       struct clk_core *core;
>> >> +       int ret = 0;
>> >
>> >The assignment looks unnecessary.
>> 
>> To avoid uninitialized variable warning.
>> 
>> >
>> >> +
>> >> +       if (!clk || !clk->core)
>> >
>> >How do you not have clk->core?
>> >
>> >> +               return 0;
>> >> +
>> >> +       clk_ss.modfreq = modfreq;
>> >> +       clk_ss.spreadpercent = spreadpercent;
>> >> +       clk_ss.method = method;
>> >> +       clk_ss.enable = enable;
>> >> +
>> >> +       clk_prepare_lock();
>> >> +
>> >> +       core = clk->core;
>> >
>> >Why do we need to get the core under the lock?
>> 
>> Drop in v2.
>> 
>> >
>> >> +
>> >> +       if (core->prepare_count) {
>> >
>> >Why does prepare count matter?
>> 
>> I was thinking to configure Spread Spectrum(SS) before
>> prepare/enable a clock. But it should be fine to not
>> check prepare count.
>> 
>> >
>> >> +               ret = -EBUSY;
>> >> +               goto fail;
>> >
>> >We just left without releasing the lock.
>> 
>> True. Dan also reported this. Fix in V2.
>> 
>> >
>> >> +       }
>> >> +
>> >> +       ret = clk_pm_runtime_get(core);
>> >> +       if (ret)
>> >> +               goto fail;
>> >
>> >We just left without releasing the lock.
>> >
>> >> +
>> >> +       if (core->ops->set_spread_spectrum)
>> >> +               ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
>> >> +
>> >> +       clk_pm_runtime_put(core);
>> >> +       clk_prepare_unlock();
>> >> +fail:
>> >> +       return ret;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(clk_set_spread_spectrum);
>> >> +
>> >> 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
>> >
>> >Did we get cut off?
>> 
>> Sorry I not get this point.
>> 
>> >
>> >> + * @enable: enable or disable
>> >
>> >Isn't 'disable' equal to spread_percent of zero?
>> 
>> yeah. Drop the last parameter.
>> 
>> >
>> >> + *
>> >> + * 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,
>> >
>> >Does this need to be a consumer API at all? Usually SSC is figured out
>> >when making a board and you have to pass some certification testing
>> >because some harmonics are interfering. Is the DT property sufficient
>> >for now and then we can do it when the driver probes in the framework?
>> 
>> I suppose 'DT property' you are refering the stm32 and i.MX8M SSC patchsets.
>> I am proposing a generic interface for drivers to enable SSC.
>> Otherwise we need to introduce vendor properties for each vendor.
>> And looking at clk-scmi.c, we need a generic way to enable SSC, I think SCMI
>> maintainers not agree to add vendor properties for it.
>> 
>
>To clarify, from the SCMI point of view, I expressed the idea that it
>would make sense to have a common SSC interface on the SCMI backend too
>instead of a custom NXP since you are adding a common CLK framework feature,
>BUT only if it turns out, from this discussion, that a common general way of
>configuring SSC can be found...and I dont know that, so I am waiting to see
>what this discussion with CLK framework and iMX maintainers goes before
>excluding the SCMI CLK vendor OEM types scenario...it would be ideal and
>easier NOT to use SCMI vendor extensions BUT ONLY if this NXP SSC/config generic
>solution is deemed to be really generic and usable by any other vendor.

You may misunderstand. Using DT properties for clk-scmi.c to configure SSC
of a single clock means to add properties under clk scmi node, such
as:
"arm,modfreq = <x>, <y>, <z>;
 arm,moddepth = <a>, <b>, <c>;
 arm,modmethod = <j>, <l>, <m>;"

And during probe in clk-scmi.c, the properties needs to be parsed and when
clk is configured, the ssc settings need to be passed to scmi platform.

But per the i.MX8M SSC patchset that DT maintainers raised, 
the properties(fsl,modfreq and etc) needs to in consumer side,
not provider side.

So it is not feasible to add properties here.

"assigned-clock-sscs" could be put under consumer node and clocks
could be configured with SSC when needed. This is a generic property.

Back to NXP SCMI vendor extension, if SCMI spec could include SSC, NXP
SCMI could move to align with spec and not use vendor extension.

Thanks,
Peng.

>
>Thanks,
>Cristian

RE: [PATCH 1/3] clk: Introduce clk_set_spread_spectrum

[Peng Fan]

> Subject: Re: [PATCH 1/3] clk: Introduce clk_set_spread_spectrum
> 
> On Mon, Feb 03, 2025 at 07:47:22PM +0800, Peng Fan wrote:
> > On Mon, Feb 03, 2025 at 09:43:39AM +0000, Cristian Marussi wrote:
> > >On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote:
> > >> On Tue, Jan 28, 2025 at 12:25:28PM -0800, Stephen Boyd wrote:
> > >> >Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
> > >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> > >> >>
> cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b7
> 36e3c
> > >> >> 2751eeb38dc4577b1f 100644
> > >> >> --- a/drivers/clk/clk.c
> > >> >> +++ b/drivers/clk/clk.c
> 
> Hi Peng,
> 
> > >> >> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk,
> > >> >> unsigned long rate)  }  EXPORT_SYMBOL_GPL(clk_set_max_rate);
> 
> [snip]
> 
> > >> >
> > >> >> + *
> > >> >> + * 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,
> > >> >
> > >> >Does this need to be a consumer API at all? Usually SSC is figured
> out
> > >> >when making a board and you have to pass some certification
> testing
> > >> >because some harmonics are interfering. Is the DT property
> sufficient
> > >> >for now and then we can do it when the driver probes in the
> framework?
> > >>
> > >> I suppose 'DT property' you are refering the stm32 and i.MX8M
> SSC patchsets.
> > >> I am proposing a generic interface for drivers to enable SSC.
> > >> Otherwise we need to introduce vendor properties for each
> vendor.
> > >> And looking at clk-scmi.c, we need a generic way to enable SSC, I
> think SCMI
> > >> maintainers not agree to add vendor properties for it.
> > >>
> > >
> > >To clarify, from the SCMI point of view, I expressed the idea that it
> > >would make sense to have a common SSC interface on the SCMI
> backend too
> > >instead of a custom NXP since you are adding a common CLK
> framework feature,
> > >BUT only if it turns out, from this discussion, that a common general
> way of
> > >configuring SSC can be found...and I dont know that, so I am waiting
> to see
> > >what this discussion with CLK framework and iMX maintainers goes
> before
> > >excluding the SCMI CLK vendor OEM types scenario...it would be
> ideal and
> > >easier NOT to use SCMI vendor extensions BUT ONLY if this NXP
> SSC/config generic
> > >solution is deemed to be really generic and usable by any other
> vendor.
> >
> > You may misunderstand. Using DT properties for clk-scmi.c to
> configure SSC
> > of a single clock means to add properties under clk scmi node, such
> > as:
> > "arm,modfreq = <x>, <y>, <z>;
> >  arm,moddepth = <a>, <b>, <c>;
> >  arm,modmethod = <j>, <l>, <m>;"
> >
> > And during probe in clk-scmi.c, the properties needs to be parsed and
> when
> > clk is configured, the ssc settings need to be passed to scmi platform.
> >
> > But per the i.MX8M SSC patchset that DT maintainers raised,
> > the properties(fsl,modfreq and etc) needs to in consumer side,
> > not provider side.
> >
> > So it is not feasible to add properties here.
> >
> 
> Thanks for the clarification.
> 
> > "assigned-clock-sscs" could be put under consumer node and clocks
> > could be configured with SSC when needed. This is a generic property.
> >
> 
> Yes I understood this, the property that describes SSC that you are
> adding is generic BUT are also the related params needed to describe
> effectively the SSC
> 
> IOW, if we add, as desirable, a generic new SSC type in extended
> configs
> instead of using a vendor oem, will these info down below passed to
> the SCMI:
> 
> +       /*
> +        * extConfigValue[7:0]   - spread percentage (%)
> +        * extConfigValue[23:8]  - Modulation Frequency (KHz)
> +        * extConfigValue[24]    - Enable/Disable
> +        * extConfigValue[31:25] - Reserved
> +        */

From SSC view, spread percent(depth),  modulation freq, modulation
method is required to be passed to SCMI server. Enable maybe
optional(depth set to 0 should be same as disable).

The upper encodings using extConfig is NXP defined, we could
update following spec.

> 
> 
> ... be enough to describe the required SSC config to any generic SCMI
> server
> from any vendor using any HW ?
> 
> ... or is it plausible and maybe frequent/usual that other vendors could
> need additional specific params to be fed to the server, so that we
> will end up using the new standard SSC only for NXP ?

I checked TI/STM32/Renesas spec.
https://www.ti.com/lit/an/spraby0a/spraby0a.pdf?ts=1738492934158&ref_url=https%253A%252F%252Fwww.google.com.hk%252F
https://www.st.com/resource/en/application_note/an4850-stm32-mcus-spreadspectrum-clock-generation-principles-properties-and-implementation-stmicroelectronics.pdf
https://www.renesas.com/en/products/clocks-timing/application-specific-clocks/spread-spectrum-clocks?srsltid=AfmBOoqSceW72dYO41RZVhT1YxhyKeXNhWUTfSrSCpfZt2A2cy7JgaGv

same parameters are required.

From ADI:
https://www.analog.com/en/resources/technical-articles/clock-generation-with-spread-spectrum.html
"
Creating a spread-spectrum CLK by dithering the CLK frequency is not as
straightforward as it might appear. We begin by defining parameters that
comprise a spread-spectrum CLK: spreading rate, spreading style,
modulation rate, and modulation waveform. Spreading rate is the ratio
of the range of dithering (or spreading) frequency over the original CLK
frequency, fC. Spreading style is down-spreading, center-spreading, or up-spreading.
If we assume that the spreading frequency range is Δf, the spreading rate, δ, is defined as:

Down spreading: δ = -Δf /fC x 100%

Center spreading: δ = ±1/2Δf/fC x 100%

Up spreading: δ = Δf/fC x 100%
"

fC is same as spread percentage/depth.

> 
> IOW the property is generic, agreed, but are the described params
> above
> generic enough too ? ... 

I think so.

Thanks,
Peng.

that was my concern from an UN-educated
> point
> of view...of course, I am talking about the most common scenarios, if
> some other vendor ends up needing some arcane/magic specific param
> that
> cannot fit the above, they will be relegated to the OEM custom spaces
> even if this common SSC is available.
> 
> > Back to NXP SCMI vendor extension, if SCMI spec could include SSC,
> NXP
> > SCMI could move to align with spec and not use vendor extension.
> >
> 
> Agreed, conditional to the above doubts.
> 
> Apologies if I have asked dumb/obvious questions, but I am not
> familiar
> with the subject matter enough...
> 
> Thanks,
> Cristian