Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

From: Nicola Mazzucato <nicola.mazzucato@arm.com>
To: Viresh Kumar <viresh.kumar@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, rjw@rjwysocki.net,
	vireshk@kernel.org, robh+dt@kernel.org, sboyd@kernel.org,
	nm@ti.com, daniel.lezcano@linaro.org, morten.rasmussen@arm.com,
	chris.redpath@arm.com
Subject: Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM
Date: Wed, 9 Dec 2020 09:20:33 +0000	[thread overview]
Message-ID: <a70cfb32-1a5f-d12f-f466-321d60e58204@arm.com> (raw)
In-Reply-To: <20201209054502.ajomw6glcxx5hue2@vireshk-i7>

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