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

[Cristian Marussi <cristian.marussi@arm.com>]

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