Re: [PATCH v7 6/6] clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver

[Peng Fan <peng.fan@oss.nxp.com>]

Hi Cristian,

Thanks for reviewing!

On Sat, Feb 28, 2026 at 10:02:04AM +0000, Cristian Marussi wrote:
>On Wed, Dec 31, 2025 at 06:12:53PM +0800, Peng Fan (OSS) wrote:
>> +
>> +struct scmi_clk_oem_info {
>> +	char *vendor_id;
>> +	char *sub_vendor_id;
>> +	char *compatible;
>
>I suppose different OEM per impl_ver is overkill...and maybe better
>to be trated as a bug if it happens leveraging the Quirk framework..

Hope I not get you wrong. impl_ver is not used here.
compatible is machine compatible string.

The OEM matching introduced in this patch is strictly based
on SCMI vendor_id and sub_vendor_id as reported by the firmware. There is no
dependency on impl_ver, nor do we intend to distinguish OEM behavior based on
implementation version.

If in the future any firmware shows inconsistent behavior across different
impl_ver, we agree that this should be treated as a firmware bug and
handled through the existing SCMI quirk framework.

I could add a comment for the structure in next version:
/*
 * Selection is based on SCMI vendor_id/sub_vendor_id and optional machine
 * comaptible string, without involving impl_ver. impl_ver‑specific behavior
 * should be handled via the SCMI Quirk framework
 */

>
>> +	const void *data;
>> +};
>> +
>> +static int
>> +scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
>> +const struct scmi_clk_oem_info info[] = {
>
>Any reason why this cannot be made static too ?
>Given that you just embed it with set_drvdata and then get_drvdata
>back from the main clk driver...

Fix in next version.

Thanks,
Peng

>
>> +	{ SCMI_IMX_VENDOR, SCMI_IMX_SUBVENDOR, NULL, &scmi_clk_oem_imx },
>> +};
>> +
>> +int scmi_clk_oem_init(struct scmi_device *sdev)
>> +{
>> +	const struct scmi_handle *handle = sdev->handle;
>> +	int i, size = ARRAY_SIZE(info);
>> +
>> +	for (i = 0; i < size; i++) {
>> +		if (strcmp(handle->version->vendor_id, info[i].vendor_id) ||
>> +		    strcmp(handle->version->sub_vendor_id, info[i].sub_vendor_id))
>> +			continue;
>> +		if (info[i].compatible &&
>> +		    !of_machine_is_compatible(info[i].compatible))
>> +			continue;
>> +
>> +		break;
>> +	}
>> +
>> +	if (i < size)
>> +		dev_set_drvdata(&sdev->dev, (void *)info[i].data);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
>> index bf85924d61985eb9e596419349eb883e3817de73..1ed2091e3d4a951c8662db4c94dee4b9c98b8326 100644
>> --- a/drivers/clk/clk-scmi.c
>> +++ b/drivers/clk/clk-scmi.c
>> @@ -14,6 +14,8 @@
>>  #include <linux/scmi_protocol.h>
>>  #include <asm/div64.h>
>>  
>> +#include "clk-scmi.h"
>> +
>>  const struct scmi_clk_proto_ops *scmi_proto_clk_ops;
>>  
>>  static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
>> @@ -242,6 +244,7 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
>>  static const struct clk_ops *
>>  scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>>  {
>> +	struct scmi_clk_oem *oem_data = dev_get_drvdata(dev);
>>  	struct clk_ops *ops;
>>  
>>  	ops = devm_kzalloc(dev, sizeof(*ops), GFP_KERNEL);
>> @@ -288,11 +291,15 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>>  		ops->set_duty_cycle = scmi_clk_set_duty_cycle;
>>  	}
>>  
>> +	if (oem_data && (feats_key & BIT(SCMI_CLK_EXT_OEM_SSC_SUPPORTED)))
>> +		ops->set_spread_spectrum = oem_data->set_spread_spectrum;
>> +
>>  	return ops;
>>  }
>>  
>>  /**
>>   * scmi_clk_ops_select() - Select a proper set of clock operations
>> + * @sdev: pointer to the SCMI device
>>   * @sclk: A reference to an SCMI clock descriptor
>>   * @atomic_capable: A flag to indicate if atomic mode is supported by the
>>   *		    transport
>> @@ -317,8 +324,8 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>>   *	   NULL otherwise.
>>   */
>>  static const struct clk_ops *
>> -scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
>> -		    unsigned int atomic_threshold_us,
>> +scmi_clk_ops_select(struct scmi_device *sdev, struct scmi_clk *sclk,
>> +		    bool atomic_capable, unsigned int atomic_threshold_us,
>>  		    const struct clk_ops **clk_ops_db, size_t db_size)
>>  {
>>  	int ret;
>> @@ -326,6 +333,7 @@ scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
>>  	const struct scmi_clock_info *ci = sclk->info;
>>  	unsigned int feats_key = 0;
>>  	const struct clk_ops *ops;
>> +	struct scmi_clk_oem *oem_data = dev_get_drvdata(&sdev->dev);
>>  
>>  	/*
>>  	 * Note that when transport is atomic but SCMI protocol did not
>> @@ -350,6 +358,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 (oem_data && oem_data->query_ext_oem_feats)
>> +			oem_data->query_ext_oem_feats(sclk->ph, sclk->id, &feats_key);
>>  	}
>>  
>>  	if (WARN_ON(feats_key >= db_size))
>> @@ -407,6 +418,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>>  	clk_data->num = count;
>>  	hws = clk_data->hws;
>>  
>> +	scmi_clk_oem_init(sdev);
>> +
>>  	transport_is_atomic = handle->is_transport_atomic(handle,
>>  							  &atomic_threshold_us);
>>  
>> @@ -438,7 +451,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>>  		 * to avoid sharing the devm_ allocated clk_ops between multiple
>>  		 * SCMI clk driver instances.
>>  		 */
>> -		scmi_ops = scmi_clk_ops_select(sclk, transport_is_atomic,
>> +		scmi_ops = scmi_clk_ops_select(sdev, sclk, transport_is_atomic,
>>  					       atomic_threshold_us,
>>  					       scmi_clk_ops_db,
>>  					       ARRAY_SIZE(scmi_clk_ops_db));
>> diff --git a/drivers/clk/clk-scmi.h b/drivers/clk/clk-scmi.h
>> index 6ef6adc77c836dc2d599ff852cdc941f217ee388..d7f63f36c56d155f728325efd6bcf7fe2585b170 100644
>> --- a/drivers/clk/clk-scmi.h
>> +++ b/drivers/clk/clk-scmi.h
>> @@ -7,6 +7,7 @@
>>  #define __SCMI_CLK_H
>>  
>>  #include <linux/bits.h>
>> +#include <linux/clk-provider.h>
>>  #include <linux/scmi_protocol.h>
>>  #include <linux/types.h>
>>  
>> @@ -19,6 +20,7 @@ enum scmi_clk_feats {
>>  	SCMI_CLK_RATE_CTRL_SUPPORTED,
>>  	SCMI_CLK_PARENT_CTRL_SUPPORTED,
>>  	SCMI_CLK_DUTY_CYCLE_SUPPORTED,
>> +	SCMI_CLK_EXT_OEM_SSC_SUPPORTED,
>>  	SCMI_CLK_FEATS_COUNT
>>  };
>>  
>> @@ -37,4 +39,13 @@ struct scmi_clk {
>>  
>>  extern const struct scmi_clk_proto_ops *scmi_proto_clk_ops;
>>  
>> +struct scmi_clk_oem {
>> +	int (*query_ext_oem_feats)(const struct scmi_protocol_handle *ph,
>> +				   u32 id, unsigned int *feats_key);
>> +	int (*set_spread_spectrum)(struct clk_hw *hw,
>> +				   const struct clk_spread_spectrum *ss_conf);
>> +};
>> +
>> +int scmi_clk_oem_init(struct scmi_device *dev);
>> +
>>  #endif
>>
>
>Other than the above, LGTM.
>
>Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>
>Thanks,
>Cristian