Re: [PATCH 1/4] arm: topology: remove cpu_efficiency

[Vincent Guittot]

On 6 September 2017 at 13:43, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> wrote:
> Hi Vincent,
>
> On 04/09/17 08:49, Vincent Guittot wrote:
>> Hi Dietmar,
>>
>> Removing cpu effificiency table looks good to me. Nevertheless, i have
>> some comments below for this patch.
>
> Thanks for the review!
>
>> On 30 August 2017 at 16:41, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> wrote:
>>> Remove the 'cpu_efficiency/clock-frequency dt property' based solution
>>> to set cpu capacity which was only working for Cortex-A15/A7 arm
>>> big.LITTLE systems.
>>>
>>> I.e. the 'capacity-dmips-mhz' based solution is now the only one. It is
>>> shared between arm and arm64 and works for every big.LITTLE system no
>>> matter which core types it consists of.
>>>
>>> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
>>> Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
>>> Cc: Juri Lelli <juri.lelli@xxxxxxx>
>>> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
>>> ---
>>>  arch/arm/kernel/topology.c | 113 ++-------------------------------------------
>>>  1 file changed, 3 insertions(+), 110 deletions(-)
>
> [...]
>
>>> @@ -115,73 +70,13 @@ static void __init parse_dt_topology(void)
>>>                         of_node_put(cn);
>>>                         continue;
>>
>> AFAICT, this continue is now useless as it was there to skipe the cpu
>> table efficiency method
>
> You're right ... will remove it.
>
> [...]
>
>>> -       if (cap_from_dt)
>>> -               topology_normalize_cpu_scale();
>>
>> Why have you moved the call to topology_normalize_cpu_scale() from
>> parse_dt_topology() to update_cpu_capacity() ?
>
> Didn't move it ? It's still called from parse_dt_topology().
>
>> You should keep it in parse_dt_topology() as itis part of the dt
>> parsing sequence
>
> Yes, this should be the case.
>
> [...]
>
>>> -/*
>>> - * Look for a customed capacity of a CPU in the cpu_capacity table during the
>>> - * boot. The update of all CPUs is in O(n^2) for heteregeneous system but the
>>> - * function returns directly for SMP system.
>>> - */
>>> -static void update_cpu_capacity(unsigned int cpu)
>>> -{
>>> -       if (!cpu_capacity(cpu) || cap_from_dt)
>>> -               return;
>>> -
>>> -       topology_set_cpu_scale(cpu, cpu_capacity(cpu) / middle_capacity);
>>> -
>>> -       pr_info("CPU%u: update cpu_capacity %lu\n",
>>> -               cpu, topology_get_cpu_scale(NULL, cpu));
>>> +       topology_normalize_cpu_scale();
>>>  }
>>
>> You can probably just removed update_cpu_capacity()
>
> I did remove update_cpu_capacity(). Maybe the patch layout is confusing?

yes you're right I have been confused by the layout

>
> [...]