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

Florian Fainelli
2025-08-15T18:41:17Z

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