Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by_cpus()
From: Florian Fainelli
Date: Fri Aug 15 2025 - 12:51:58 EST
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@xxxxxxx>
Fixes: 6c9bb8692272 ("cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs")
Signed-off-by: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
---
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