diff mbox series

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

Message ID 20250205-clk-ssc-v2-4-fa73083caa92@nxp.com (mailing list archive)
State Under Review
Headers show
Series clk: Support spread spectrum and use it in clk-pll144x and clk-scmi | expand

Commit Message

Peng Fan Feb. 5, 2025, 9:49 a.m. UTC 
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(-)

Comments

Cristian Marussi Feb. 6, 2025, 12:26 p.m. UTC | #1 
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
Peng Fan Feb. 6, 2025, 2 p.m. UTC | #2 
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
diff mbox series

Patch

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