cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by_cpus()

[PATCH] cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by_cpus()

Posted by Florian Fainelli 1 month, 2 weeks ago

Broadcom STB platforms were early adopters of the SCMI framework and as
a result, not all deployed systems have a Device Tree entry where SCMI
protocol 0x13 (PERFORMANCE) is declared as a clock provider, nor are the
CPU Device Tree node(s) referencing protocol 0x13 as their clock
provider.

Leverage the quirks framework recently introduce to match on the
Broadcom SCMI vendor and in that case, disable the Device Tree
properties checks being done by scmi_dev_used_by_cpus().

Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
Fixes: 6c9bb8692272 ("cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/cpufreq/scmi-cpufreq.c     | 13 +++++++++++++
 drivers/firmware/arm_scmi/quirks.c |  2 ++
 drivers/firmware/arm_scmi/quirks.h |  1 +
 3 files changed, 16 insertions(+)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index ef078426bfd5..80647511d3c3 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -22,6 +22,8 @@
 #include <linux/types.h>
 #include <linux/units.h>
 
+#include "../drivers/firmware/arm_scmi/quirks.h"
+
 struct scmi_data {
 	int domain_id;
 	int nr_opp;
@@ -34,6 +36,7 @@ struct scmi_data {
 static struct scmi_protocol_handle *ph;
 static const struct scmi_perf_proto_ops *perf_ops;
 static struct cpufreq_driver scmi_cpufreq_driver;
+static bool __maybe_unused scmi_cpufreq_dt_props_check_disable;
 
 static unsigned int scmi_cpufreq_get_rate(unsigned int cpu)
 {
@@ -400,6 +403,9 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
 	struct device *cpu_dev;
 	int cpu, idx;
 
+	if (scmi_cpufreq_dt_props_check_disable)
+		return true;
+
 	if (!scmi_np)
 		return false;
 
@@ -427,12 +433,19 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
 	return false;
 }
 
+#define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS			\
+	({							\
+		scmi_cpufreq_dt_props_check_disable = true;	\
+	})
+
 static int scmi_cpufreq_probe(struct scmi_device *sdev)
 {
 	int ret;
 	struct device *dev = &sdev->dev;
 	const struct scmi_handle *handle;
 
+	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);
+
 	handle = sdev->handle;
 
 	if (!handle || !scmi_dev_used_by_cpus(dev))
diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
index 03960aca3610..aafc7b4b3294 100644
--- a/drivers/firmware/arm_scmi/quirks.c
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -171,6 +171,7 @@ struct scmi_quirk {
 /* Global Quirks Definitions */
 DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
 DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
+DEFINE_SCMI_QUIRK_EXPORTED(scmi_cpufreq_no_check_dt_props, "brcm-scmi", NULL, "0x2");
 
 /*
  * Quirks Pointers Array
@@ -181,6 +182,7 @@ DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
 static struct scmi_quirk *scmi_quirks_table[] = {
 	__DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
 	__DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
+	__DECLARE_SCMI_QUIRK_ENTRY(scmi_cpufreq_no_check_dt_props),
 	NULL
 };
 
diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
index a71fde85a527..64ac1f05d0d3 100644
--- a/drivers/firmware/arm_scmi/quirks.h
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -48,5 +48,6 @@ static inline void scmi_quirks_enable(struct device *dev, const char *vend,
 /* Quirk delarations */
 DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
 DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
+DECLARE_SCMI_QUIRK(scmi_cpufreq_no_check_dt_props);
 
 #endif /* _SCMI_QUIRKS_H */
-- 
2.34.1

Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by_cpus()

Posted by Cristian Marussi 1 month, 2 weeks ago

On Thu, Aug 14, 2025 at 03:51:55PM -0700, Florian Fainelli wrote:
> Broadcom STB platforms were early adopters of the SCMI framework and as
> a result, not all deployed systems have a Device Tree entry where SCMI
> protocol 0x13 (PERFORMANCE) is declared as a clock provider, nor are the
> CPU Device Tree node(s) referencing protocol 0x13 as their clock
> provider.
> 
> Leverage the quirks framework recently introduce to match on the
> Broadcom SCMI vendor and in that case, disable the Device Tree
> properties checks being done by scmi_dev_used_by_cpus().
> 

Hi,

> Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
> Fixes: 6c9bb8692272 ("cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c     | 13 +++++++++++++
>  drivers/firmware/arm_scmi/quirks.c |  2 ++
>  drivers/firmware/arm_scmi/quirks.h |  1 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index ef078426bfd5..80647511d3c3 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -22,6 +22,8 @@
>  #include <linux/types.h>
>  #include <linux/units.h>
>  
> +#include "../drivers/firmware/arm_scmi/quirks.h"
> +

I will post a patch to move this header up to avoid the uglyness of this
include....

>  struct scmi_data {
>  	int domain_id;
>  	int nr_opp;
> @@ -34,6 +36,7 @@ struct scmi_data {
>  static struct scmi_protocol_handle *ph;
>  static const struct scmi_perf_proto_ops *perf_ops;
>  static struct cpufreq_driver scmi_cpufreq_driver;
> +static bool __maybe_unused scmi_cpufreq_dt_props_check_disable;
>  

Not sure why you introduce an intermediate global bool to check...this
defeats a bit the whole idea of the quirks framework which is based on
static_keys and is supposed to be mostly transarent when quirks are not
enabled....

Couldn't you just move the quirk inside the get_rate ?
(maybe I am missing something around compiler behaviours..)

#define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS		\
({							\
	if (true)					\
		return true;				\
})

>  static unsigned int scmi_cpufreq_get_rate(unsigned int cpu)
>  {
> @@ -400,6 +403,9 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
>  	struct device *cpu_dev;
>  	int cpu, idx;
>  

+	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);

>  	if (!scmi_np)
>  		return false;
>  
> @@ -427,12 +433,19 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
>  	return false;
>  }
>  
> +#define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS			\
> +	({							\
> +		scmi_cpufreq_dt_props_check_disable = true;	\
> +	})
> +
>  static int scmi_cpufreq_probe(struct scmi_device *sdev)
>  {
>  	int ret;
>  	struct device *dev = &sdev->dev;
>  	const struct scmi_handle *handle;
>  

> +	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);
> +

...removing this of course

>  	handle = sdev->handle;
>  
>  	if (!handle || !scmi_dev_used_by_cpus(dev))
> diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
> index 03960aca3610..aafc7b4b3294 100644
> --- a/drivers/firmware/arm_scmi/quirks.c
> +++ b/drivers/firmware/arm_scmi/quirks.c
> @@ -171,6 +171,7 @@ struct scmi_quirk {
>  /* Global Quirks Definitions */
>  DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
>  DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
> +DEFINE_SCMI_QUIRK_EXPORTED(scmi_cpufreq_no_check_dt_props, "brcm-scmi", NULL, "0x2");

Also, are you sure about using version as "0x2" ? That is supposed to
indicate the (optional) SCMI FW Version to which this quirk will
apply...and with that it means whatever FW versioning you use in
Broadcom to identify build versions....it is NOT the SCMI Protocol
Version, so that also means that if/when you will change the advertised
version, this quirk wont apply anymore...or equally if there are older
version than 0x2 that are buggy they wont be quirked...

One more doubt I have (despite me having suggested this solution) is
that here you are quirking against a malformed deployed DT really,
not against some SCMI FW anomaly in the Broadcom FW, but using the
SCMI Quirks framework you are tying the quirk to the SCMI FW Vendor
and maybe some specific SCMI FW Version....

...so what will happen when you will update/fix your DT in the future ?
Will you also take care to bump the BRCM SCMI FW version to disable the
quirk in the DT deployed by your FW binary ?

Thanks,
Cristian

Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by_cpus()

Posted by Florian Fainelli 1 month, 2 weeks ago

On 8/15/25 01:08, Cristian Marussi wrote:
> On Thu, Aug 14, 2025 at 03:51:55PM -0700, Florian Fainelli wrote:
>> Broadcom STB platforms were early adopters of the SCMI framework and as
>> a result, not all deployed systems have a Device Tree entry where SCMI
>> protocol 0x13 (PERFORMANCE) is declared as a clock provider, nor are the
>> CPU Device Tree node(s) referencing protocol 0x13 as their clock
>> provider.
>>
>> Leverage the quirks framework recently introduce to match on the
>> Broadcom SCMI vendor and in that case, disable the Device Tree
>> properties checks being done by scmi_dev_used_by_cpus().
>>
> 
> Hi,
> 
>> Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
>> Fixes: 6c9bb8692272 ("cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs")
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   drivers/cpufreq/scmi-cpufreq.c     | 13 +++++++++++++
>>   drivers/firmware/arm_scmi/quirks.c |  2 ++
>>   drivers/firmware/arm_scmi/quirks.h |  1 +
>>   3 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index ef078426bfd5..80647511d3c3 100644
>> --- a/drivers/cpufreq/scmi-cpufreq.c
>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>> @@ -22,6 +22,8 @@
>>   #include <linux/types.h>
>>   #include <linux/units.h>
>>   
>> +#include "../drivers/firmware/arm_scmi/quirks.h"
>> +
> 
> I will post a patch to move this header up to avoid the uglyness of this
> include....

Sounds good, thanks!

> 
>>   struct scmi_data {
>>   	int domain_id;
>>   	int nr_opp;
>> @@ -34,6 +36,7 @@ struct scmi_data {
>>   static struct scmi_protocol_handle *ph;
>>   static const struct scmi_perf_proto_ops *perf_ops;
>>   static struct cpufreq_driver scmi_cpufreq_driver;
>> +static bool __maybe_unused scmi_cpufreq_dt_props_check_disable;
>>   
> 
> Not sure why you introduce an intermediate global bool to check...this
> defeats a bit the whole idea of the quirks framework which is based on
> static_keys and is supposed to be mostly transarent when quirks are not
> enabled....
> 
> Couldn't you just move the quirk inside the get_rate ?

Yes, I just had not realized that the execution of the quirk was already 
conditional, so it makes sense not to need any intermediate boolean.

> (maybe I am missing something around compiler behaviours..)
>   
> #define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS		\
> ({							\
> 	if (true)					\
> 		return true;				\
> })
> 
>>   static unsigned int scmi_cpufreq_get_rate(unsigned int cpu)
>>   {
>> @@ -400,6 +403,9 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
>>   	struct device *cpu_dev;
>>   	int cpu, idx;
>>   
> 
> +	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);
> 
>>   	if (!scmi_np)
>>   		return false;
>>   
>> @@ -427,12 +433,19 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
>>   	return false;
>>   }
>>   
>> +#define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS			\
>> +	({							\
>> +		scmi_cpufreq_dt_props_check_disable = true;	\
>> +	})
>> +
>>   static int scmi_cpufreq_probe(struct scmi_device *sdev)
>>   {
>>   	int ret;
>>   	struct device *dev = &sdev->dev;
>>   	const struct scmi_handle *handle;
>>   
> 
>> +	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);
>> +
> 
> ...removing this of course
> 
>>   	handle = sdev->handle;
>>   
>>   	if (!handle || !scmi_dev_used_by_cpus(dev))
>> diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
>> index 03960aca3610..aafc7b4b3294 100644
>> --- a/drivers/firmware/arm_scmi/quirks.c
>> +++ b/drivers/firmware/arm_scmi/quirks.c
>> @@ -171,6 +171,7 @@ struct scmi_quirk {
>>   /* Global Quirks Definitions */
>>   DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
>>   DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
>> +DEFINE_SCMI_QUIRK_EXPORTED(scmi_cpufreq_no_check_dt_props, "brcm-scmi", NULL, "0x2");
> 
> Also, are you sure about using version as "0x2" ? That is supposed to
> indicate the (optional) SCMI FW Version to which this quirk will
> apply...and with that it means whatever FW versioning you use in
> Broadcom to identify build versions....it is NOT the SCMI Protocol
> Version, so that also means that if/when you will change the advertised
> version, this quirk wont apply anymore...or equally if there are older
> version than 0x2 that are buggy they wont be quirked...

It's a good point, we should actually be matching on <= 0x2

> 
> One more doubt I have (despite me having suggested this solution) is
> that here you are quirking against a malformed deployed DT really,
> not against some SCMI FW anomaly in the Broadcom FW, but using the
> SCMI Quirks framework you are tying the quirk to the SCMI FW Vendor
> and maybe some specific SCMI FW Version....

Yes that is a very good point, maybe that's abusing the quirks framework 
so to speak.

> 
> ...so what will happen when you will update/fix your DT in the future ?
> Will you also take care to bump the BRCM SCMI FW version to disable the
> quirk in the DT deployed by your FW binary ?

Yes we would bump the version number to indicate that the Device Tree 
has always been fixed, both go hand in hand on our platforms. Or, as I 
suggested privately to address the stable back ports, maybe it would be 
better to do something like this:

@@ -430,6 +428,14 @@ static bool scmi_dev_used_by_cpus(struct device 
*scmi_dev)
                         return true;
         }

+       /* Older Broadcom STB chips had a "clocks" property that did not 
match
+        * the SCMI performance protocol Device Tree node, if we got 
there, it
+        * means we had such an older Device Tree, therefore return true in
+        * the interest of preserving backwards compatibility.
+        */
+       if (of_machine_is_compatible("brcm,brcmstb"))
+               return true;
+
         return false;
  }


which would be more in line with checking the Device Tree only, and it 
would also allow for unmodified backports to reach the stable trees. 
Contrary to what I suggested privately however, this check is done 
later, so we leave a chance for properly formed DT to return "true" 
earlier on.

What do you think? I am now leaning more towards that solution that 
leveraging the quirks as I agree it is somewhat unrelated.

Thanks!
-- 
Florian

Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by_cpus()

Posted by Sudeep Holla 1 month, 2 weeks ago

On Fri, Aug 15, 2025 at 09:46:22AM -0700, Florian Fainelli wrote:

[...]

> which would be more in line with checking the Device Tree only, and it would
> also allow for unmodified backports to reach the stable trees. Contrary to
> what I suggested privately however, this check is done later, so we leave a
> chance for properly formed DT to return "true" earlier on.
>

Completely agree on keeping the backports for stable simple without the
need to backport the quirk infrastructure.

> What do you think? I am now leaning more towards that solution that
> leveraging the quirks as I agree it is somewhat unrelated.
> 

I am confused here. Do you prefer not to have SCMI quirk based solution
upstream forever ? Or are you OK to revert the fix and move to quirks
say in v6.18 and above ?

-- 
Regards,
Sudeep