Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

[Daniel Lezcano]

On 22/11/2018 05:29, Viresh Kumar wrote:
> On 21-11-18, 23:12, Daniel Lezcano wrote:
>> On 30/10/2018 09:58, Viresh Kumar wrote:
>>> s/dmpis/dmips/ in $subject
>>>
>>> On 29-10-18, 17:23, Daniel Lezcano wrote:
>>>> In the case of assymetric SoC with the same micro-architecture, we
>>>
>>>                  asymmetric ?
>>>
>>>> have a group of CPUs with smaller OPPs than the other group. One
>>>> example is the 96boards dragonboard 820c. There is no dmips/MHz
>>>> difference between both groups, so no need to specify the values in
>>>> the DT. Unfortunately, without these defined, there is no scaling
>>>> capacity comutation triggered, so we need to write
>>>
>>>            computation
>>>
>>>> 'capacity-dmips-mhz' for each CPU with the same value in order to
>>>> force the scaled capacity computation.
>>>>
>>>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
>>>> 'capacity-dmips-mhz' is defined in the DT.
>>>>
>>>> This was tested on db820c:
>>>>  - specified values in the DT (correct results)
>>>>  - partial values defined in the DT (error + fallback to defaults)
>>>>  - no specified values in the DT (correct results)
>>>>
>>>> correct results are:
>>>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>>>>    758
>>>>    758
>>>>   1024
>>>>   1024
>>>>
>>>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
>>>>
>>>> That reflects the capacity for the max frequencies 1593600 and 2150400.
>>>>
>>>> Cc: Chris Redpath <chris.redpath@xxxxxxxxxx>
>>>> Cc: Quentin Perret <quentin.perret@xxxxxxxxxx>
>>>> Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>>>> Cc: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
>>>> Cc: Nicolas Dechesne <nicolas.dechesne@xxxxxxxxxx>
>>>> Cc: Niklas Cassel <niklas.cassel@xxxxxxxxxx>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>>>> ---
>>>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>>>> index 7311641..7d594a6 100644
>>>> --- a/drivers/base/arch_topology.c
>>>> +++ b/drivers/base/arch_topology.c
>>>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>>>>  	.notifier_call = init_cpu_capacity_callback,
>>>>  };
>>>>  
>>>> +static int topology_set_default_capacity(void)
>>>> +{
>>>> +	int cpu;
>>>> +
>>>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
>>>> +			       GFP_KERNEL);
>>>> +	if (!raw_capacity)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for_each_possible_cpu(cpu)
>>>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
>>>
>>> This isn't actually required as the value of raw_capacity isn't used
>>> anymore after this point in code. Rather it is forcefully updated in
>>> init_cpu_capacity_callback():
>>>
>>> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
>>>                     policy->cpuinfo.max_freq / 1000UL;
>>>
>>> Maybe it is better to allocate raw_capacity once at boot and use
>>> another global variable as flag (raw_capacity is used as a flag right
>>> now at many places).
>>
>> Can we keep the proposed change as is to simply fix the default value?
>>
>> I want to do a separate change with a raw_capacity rewrite and remove
>> the workqueue freeing it.
> 
> Sure. But you still don't need to update raw_capacity[cpu] above as I pointed
> out earlier. You can drop those lines at least.

Oh ... actually raw_capacity is not needed at all!



-- 
 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog