Re: Add OEM extension for clk-scmi

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

On Tue, Sep 30, 2025 at 01:22:15PM +0000, Peng Fan wrote:
> Hi All,
> 

Hi,

> This is to discuss the spread spectrum support for clk-scmi.  I think
> it is hard to provide a generic framework from scmi core level.
> Because oem extension varies per protocol. And most of the
> code added is in consumer driver side saying clk-scmi.c not
> the firmware/arm_scmi/clock.c.
> 
> Based on above, I am thinking to follow what arm-smmu
> implements(arm_smmu_impl_init), by calling a function
> scmi_clk_oem_init in probe.
> 
> And define a generic feature macro in enum scmi_clk_feats:
> SCMI_CLK_EXT_OEM_SSC_SUPPORTED,
> 
> Also define a new structure for OEM stuff:
> +struct scmi_clk_plat {
> +       int (*query_oem_feats)(const struct scmi_protocol_handle *ph,
> +                              u32 id, unsigned long *feats_key);
> +       int (*set_spread_spectrum)(struct clk_hw *hw,
> +                                  const struct clk_spread_spectrum *ss_conf);
> +};
> +
> 
> Add a new file clk-scmi-oem for up plat hooks implementation.
> 
> And add clk-scmi.h to move some code that would be shared
> between clk-scmi.c and clk-scmi-oem.c
> 
> This is just an idea, more code pieces as below, this does not
> pass compiling, just an idea in my mind.
> 
> If you think this is feasible, I will do a formal patch.
> 
> BTW: I will be on holiday in the following week. I will check
> email from time to time, but may not respond in time.
>  
> drivers/clk/clk-scmi-oem.c
> static int scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
>                                             struct clk_spread_spectrum *ss_conf)
> {
>         /* TODO */
>         return 0;
> }
> 
> static int scmi_clk_imx_query_oem_feats(const struct scmi_protocol_handle *ph,
>                                         u32 id, unsigned long *feats_key)
> {
>         /* TODO */
>         if ( the clk supports SSC)
>               *feats_key |= SCMI_CLK_EXT_OEM_SSC_SUPPORTED;
>         return 0;
> }
> 
> static const struct scmi_clk_plat scmi_clk_imx_plat = {
>         .query_ext_oem_feats = scmi_clk_imx_query_oem_feats,
>         .set_spread_spectrum = scmi_clk_imx_set_spread_spectrum,
> };
> 
> int scmi_clk_oem_init(struct scmi_device *dev, struct scmi_clk_plat *data)
> {
>         if (!strcmp(handle->version->vendor_id, SCMI_IMX_VENDOR) &&
>             !strcmp(handle->version->sub_vendor_id, SCMI_IMX_SUBVENDOR)) {
>                 *data = scmi_clk_imx_plat;
>         } else {
>                 /* Other SoC vendors (TI) */
>         }
> 
>         return 0;
> }
> 
> In clk-scmi.c, using the hook to update oem feats
> @@ -375,6 +388,9 @@ scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
>                                                  &val, NULL, false);
>                 if (!ret)
>                         feats_key |= BIT(SCMI_CLK_DUTY_CYCLE_SUPPORTED);
> +
> +               if (data->query_ext_oem_feats)
> +                       data->query_ext_oem_feats(sclk->ph, sclk->id, &feats_key);
>         }
> 
> @@ -313,11 +322,15 @@ 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_EXT_OEM_SSC_SUPPORTED))
> +               ops->set_spread_spectrum = data->set_spread_spectrum;
> +
>         return ops;
>  }

In general seems a sensible approach...once you had a full series we can
reason about it better probably....and see how much more (or less) we
can/should generalize.

Thanks for this,
Cristian