Message-ID: <79711a6c-8b59-43c9-bbbc-369be1608a49@broadcom.com>
Date: Fri, 15 Aug 2025 09:46:22 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: linux-kernel@...r.kernel.org, james.quinlan@...adcom.com,
 Sudeep Holla <sudeep.holla@....com>, "Rafael J. Wysocki"
 <rafael@...nel.org>, Viresh Kumar <viresh.kumar@...aro.org>,
 Peng Fan <peng.fan@....com>, Mike Tipton <quic_mdtipton@...cinc.com>,
 arm-scmi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in
 scmi_dev_used_by_cpus()

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@....com>
>> Fixes: 6c9bb8692272 ("cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs")
>> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.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

Powered by blists - more mailing lists