Messages in this thread | | | From | Ulf Hansson <> | Date | Wed, 26 Jul 2023 14:01:21 +0200 | Subject | Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain |
| |
On Wed, 19 Jul 2023 at 17:59, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 19, 2023 at 03:51:45PM +0100, Cristian Marussi wrote: > > On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote: > > [...] > > > > + scmi_pd_data = devm_kzalloc(dev, sizeof(*scmi_pd_data), GFP_KERNEL); > > > + if (!scmi_pd_data) > > > + return -ENOMEM; > > > + > > > + domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL); > > > + if (!domains) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < num_domains; i++, scmi_pd++) { > > > + scmi_pd->info = perf_ops->domain_info_get(ph, i); > > > > So here you are grabbing all the performance domains exposed by the > > platform via PERF protocol and then a few lines down below you are > > registering them with pm_genpd_init(), but the list of domains obtained > > from the platform will contain NOT only devices but also CPUs possibly, > > already managed by the SCMI CPUFreq driver. > > > > Agreed, I pointed out briefly in the previous patch I think. I am not sure > how will that work if the performance and power domains are not 1-1 mapping > or if they are CPUs then this might confusing ? Not sure but looks like > we might be creating a spaghetti here :(.
I assume the discussions around the DT bindings are making it more clear on how this should work. The scmi performance-domain and the scmi power-domain are two separate providers.
> > > In fact the SCMI CPUFreq driver, on his side, takes care to pick only > > domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT > > parsing) but here you are registering all domains with GenPD upfront. > > > > +1 > > > 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 ? > > IIRC, all unused genpd are turned off right. It may not impact here but still > super confusing as we will be creating power domains for the set of domains > actually advertised as power domains by the firmware. This will add another > set. > > > (like Clock framework does at the end of boot trying to disable unused > > clocks...not familiar with internals of GenPD, though) > > > > Ah, I am reading too much serialised. Just agreed and wrote the same above. > > > > + 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) > > > > The tricky part would be to match the PERF domain at hand with the > > related POWER domain to query the state for, I suppose. > > > > I wanted to ask the same. E.g. on juno, GPU has perf domain 2 and power domain > 9. It would be good if we can how it would work there ? What is expected > from the gpu driver in terms of managing perf and power ? Does it need > to specify 2 power domains now and specify which is perf and which power in > its bindings ?
Yes, correct.
Note that, we already have plenty of consumer devices/drivers that are managing multiple PM domains. They use dev_pm_domain_attach_by_id|name() to attach their devices to their corresponding domain(s). In addition, they often use device_link_add() to simplify runtime PM management.
That said, we should expect to see some boilerplate code in consumer drivers that deals with this attaching/detaching of multiple PM domains. That's a separate problem we may want to address later on. In fact, it's already been discussed earlier at LKML (I can't find the link right now).
[...]
Kind regards Uffe
| |