Re: [PATCH] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

Mike Tipton
2025-04-15T16:45:09Z

On Tue, Apr 15, 2025 at 05:06:55PM +0800, Peng Fan wrote:
> On Fri, Apr 11, 2025 at 02:29:41PM -0700, Mike Tipton wrote:
> >Currently, all SCMI devices with performance domains attempt to register
> >a cpufreq driver, even if their performance domains aren't used to
> >control the CPUs. The cpufreq framework only supports registering a
> >single driver, so only the first device will succeed. And if that device
> >isn't used for the CPUs, then cpufreq will scale the wrong domains.
> >
> >To avoid this, return early from scmi_cpufreq_probe() if the probing
> >SCMI device isn't referenced by the CPU device phandles.
> >
> >This keeps the existing assumption that all CPUs are controlled by a
> >single SCMI device.
> >
> >Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> >---
> > drivers/cpufreq/scmi-cpufreq.c | 29 +++++++++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> >diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> >index 944e899eb1be..7981a879974b 100644
> >--- a/drivers/cpufreq/scmi-cpufreq.c
> >+++ b/drivers/cpufreq/scmi-cpufreq.c
> >@@ -393,6 +393,32 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
> > .set_boost = cpufreq_boost_set_sw,
> > };
> >
> >+static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
> >+{
> >+ struct device_node *scmi_np = scmi_dev->of_node;
> >+ struct device_node *np;
> >+ struct device *cpu_dev;
> >+ int cpu, idx;
> >+
> >+ for_each_possible_cpu(cpu) {
> >+ cpu_dev = get_cpu_device(cpu);
> >+ if (!cpu_dev)
> >+ continue;
> >+
> >+ np = cpu_dev->of_node;
> >+
> >+ if (of_parse_phandle(np, "clocks", 0) == scmi_np)
> >+ return true;
> >+
> >+ idx = of_property_match_string(np, "power-domain-names", "perf");
> >+
> >+ if (of_parse_phandle(np, "power-domains", idx) == scmi_np)
> >+ return true;
> >+ }
> >+
> >+ return false;
> >+}
> >+
> > static int scmi_cpufreq_probe(struct scmi_device *sdev)
> > {
> > int ret;
> >@@ -404,6 +430,9 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
> > if (!handle)
> > return -ENODEV;
> >
> >+ if (!scmi_dev_used_by_cpus(dev))
> >+ return 0;
>
> Should 'return -ENOTSUPP' be used here?
> There is no need to mark the probe success.

Returning -ENOTSUPP will add noise in the logs from probe failures, for
example:

scmi-cpufreq scmi_dev.4: probe with driver scmi-cpufreq failed with error -524

These are "expected" failures, so this would be misleading. However, we
could return -ENODEV instead which doesn't log anything by default. It
uses a dev_dbg() in that case:

scmi-cpufreq scmi_dev.4: probe with driver scmi-cpufreq rejects match -19

Returning -ENODEV seems more appropriate. I can make that change.