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.1On 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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.