Messages in this thread | | | From | Ulf Hansson <> | Date | Thu, 27 Jul 2023 13:37:50 +0200 | Subject | Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain |
| |
[...]
> > > > > > Is it not possible that, once registered, GenPD can decide, at some point > > > in the future, to try act on some of these domains associated with a CPU ? > > > (like Clock framework does at the end of boot trying to disable unused > > > clocks...not familiar with internals of GenPD, though) > > > > The "magic" that exists in genpd is to save/restore the performance > > state at genpd_runtime_suspend|resume(). > > > > That means the consumer device needs to be attached and runtime PM > > enabled, otherwise genpd will just leave the performance level > > unchanged. In other words, the control is entirely at the consumer > > driver (scmi cpufreq driver). > > > > Ok, so if the DT is well formed and a CPU-related perf domain is not > wrongly referred from a driver looking for a device perf-domain, the > genPD subsystem wont act on the CPUs domains.
Yes, correct!
> > > > > > > > + scmi_pd->domain_id = i; > > > > + scmi_pd->perf_ops = perf_ops; > > > > + scmi_pd->ph = ph; > > > > + scmi_pd->genpd.name = scmi_pd->info->name; > > > > + scmi_pd->genpd.flags = GENPD_FLAG_OPP_TABLE_FW; > > > > + scmi_pd->genpd.set_performance_state = scmi_pd_set_perf_state; > > > > + > > > > + ret = perf_ops->level_get(ph, i, &perf_level, false); > > > > + if (ret) { > > > > + dev_dbg(dev, "Failed to get perf level for %s", > > > > + scmi_pd->genpd.name); > > > > + perf_level = 0; > > > > + } > > > > + > > > > + /* Let the perf level indicate the power-state too. */ > > > > + ret = pm_genpd_init(&scmi_pd->genpd, NULL, perf_level == 0); > > > > > > In SCMI world PERF levels should have nothing to do with the Power > > > state of a domain: you have the POWER protocol for that, so you should > > > not assume that perf level 0 means OFF, but you can use the POWER protocol > > > operation .state_get() to lookup the power state. (and you can grab both > > > perf and power ops from the same driver) > > > > Well, I think this may be SCMI FW implementation specific, but > > honestly I don't know exactly what the spec says about this. In any > > case, I don't think it's a good idea to mix this with the POWER > > domain, as that's something that is entirely different. We have no > > clue of those relationships (domain IDs). > > > > My main idea behind this, is just to give the genpd internals a > > reasonably defined value for its power state. > > > > The thing is that in the SCMI world you cannot assume that perf_level 0 > means powered off, the current SCP/SCMI platform fw, as an example, wont > advertise a 0-perf-level and wont act on such a request, because you are > supposed to use POWER protocol to get/set the power-state of a device.
Right, thanks for clarifying this!
> > So it could be fine, as long as genPD wont try to set the level to 0 > expecting the domain to be as a consequence also powered off and as > long as it is fine for these genpd domains to be always initialized > as ON. (since perf_level could never be found as zero..)
Okay, so to me, it sounds like that we should set GENPD_FLAG_ALWAYS_ON for the genpd. This makes it clear that the domain can't be powered off.
Moreover, we should prevent genpd internals from setting the performance state to 0 for the scmi performance domain. Let me look into how to best deal with that.
Kind regards Uffe
| |