Hi both, thanks for looking into this. On 12/9/20 5:45 AM, Viresh Kumar wrote: > On 08-12-20, 11:20, Sudeep Holla wrote: >> It is because of per-CPU vs per domain drama here. Imagine a system with >> 4 CPUs which the firmware puts in individual domains while they all are >> in the same perf domain and hence OPP is marked shared in DT. >> >> Since this probe gets called for all the cpus, we need to skip adding >> OPPs for the last 3(add only for 1st one and mark others as shared). > > Okay and this wasn't happening before this series because the firmware > was only returning the current CPU from scmi_get_sharing_cpus() ? yes > > Is this driver also used for the cases where we have multiple CPUs in > a policy ? Otherwise we won't be required to call > dev_pm_opp_set_sharing_cpus(). > > So I assume that we want to support both the cases here ? yes, we want to support existing platforms (n cpus in a policy) + the per-cpu case. > >> If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate >> OPP as we would have already marked it as shared table with the first cpu. >> Am I missing anything ? I suggested this as Nicola saw OPP duplicate >> warnings when he was hacking up this patch. > > The common stuff (for all the CPUs) is better moved to probe() in this > case, instead of the ->init() callback. Otherwise it will always be > messy. You can initialize the OPP and cpufreq tables in probe() > itself, save the pointer somewhere and then just use it here in > ->init(). > > Also do EM registration from there. > ok, will rework >>>> otherwise no need as they would be duplicated. >>>>> And we don't check the return value of >>>>> the below call anymore, moreover we have to call it twice now. >> >> Yes, that looks wrong, we need to add the check for non zero values, but .... >> >>>> >>>> This second get_opp_count is required such that we register em with the correct >>>> opp number after having added them. Without this the opp_count would not be correct. >>> >> >> ... I have a question here. Why do you need to call >> >> em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..) >> >> on each CPU ? Why can't that be done once for unique opp_shared_cpus ? >> >> The whole drama of per-CPU vs perf domain is to have energy model and >> if feeding it opp_shared_cpus once is not sufficient, then something is >> wrong or simply duplicated or just not necessary IMO. >> >>> What if the count is still 0 ? What about deferred probe we were doing earlier ? >> >> OK, you made me think with that question. I think the check was original >> added for deferred probe but then scmi core was changed to add the cpufreq >> device only after everything needed is ready. So the condition must never >> occur now. > > The deferred probe shall be handled in a different patch in that case. > > Nicola, please break the patch into multiple patches, with one patch > dealing only with one task. Sure, I had the doubt and thanks for confirming. will do, thanks > Cheers, Nicola
