Re: [PATCH] cpufreq: arm_big_little: set lowest frequcency for cluster when no cpus in it

[Sudeep Holla <sudeep.holla@arm.com>]

On 26/06/14 16:38, Viresh Kumar wrote:
> On 26 June 2014 20:38, Thomas Abraham <thomas.ab@samsung.com> wrote:
>> From: Thomas Abraham <thomas.ab@samsung.com>
>>
>> If there are no active cpus in a cluster, the clock frequency of the cluster
>> can be lowered to the lowest possible frequency for that cluster. This can
>> help reduce the output clock speed of the PLL clocking the cluster and
>> save power.
>>
>> The get_table_min() function is also moved with this change to avoid forward
>> declaration. The function get_table_max() is also moved along with
>> get_table_min() so that these two functions are adjacent the code.
>
> Makes sense as my poor mind also thought this over an year and
> half back.
>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>   drivers/cpufreq/arm_big_little.c |   51 +++++++++++++++++++++-----------------
>>   1 file changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
>> index 1f4d4e3..4b1431f 100644
>> --- a/drivers/cpufreq/arm_big_little.c
>> +++ b/drivers/cpufreq/arm_big_little.c
>> @@ -75,6 +75,28 @@ static inline int cpu_to_cluster(int cpu)
>>                  MAX_CLUSTERS : raw_cpu_to_cluster(cpu);
>>   }
>>
>> +/* get the minimum frequency in the cpufreq_frequency_table */
>> +static inline u32 get_table_min(struct cpufreq_frequency_table *table)
>> +{
>> +       struct cpufreq_frequency_table *pos;
>> +       uint32_t min_freq = ~0;
>> +       cpufreq_for_each_entry(pos, table)
>> +               if (pos->frequency < min_freq)
>> +                       min_freq = pos->frequency;
>> +       return min_freq;
>> +}
>> +
>> +/* get the maximum frequency in the cpufreq_frequency_table */
>> +static inline u32 get_table_max(struct cpufreq_frequency_table *table)
>> +{
>> +       struct cpufreq_frequency_table *pos;
>> +       uint32_t max_freq = 0;
>> +       cpufreq_for_each_entry(pos, table)
>> +               if (pos->frequency > max_freq)
>> +                       max_freq = pos->frequency;
>> +       return max_freq;
>> +}
>> +
>>   static unsigned int find_cluster_maxfreq(int cluster)
>>   {
>>          int j;
>> @@ -170,9 +192,14 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>>
>>                  mutex_lock(&cluster_lock[old_cluster]);
>>
>> -               /* Set freq of old cluster if there are cpus left on it */
>> +               /*
>> +                * Set freq of old cluster. If there are no cpus left on it,
>> +                * set the lowest possible frequency for that cluster.
>> +                */
>>                  new_rate = find_cluster_maxfreq(old_cluster);
>>                  new_rate = ACTUAL_FREQ(old_cluster, new_rate);
>> +               if (!new_rate)
>> +                       new_rate = get_table_min(freq_table[old_cluster]);
>
> Have you hit this code? How did you test it?
>
> In case you have hit this code, you must be using IKS as in MP it
> doesn't make sense.
>
> Two things:
> - First in case this patch gets in, you can check !new_rate just after
>    find_cluster_maxfreq() as it will be zero there as well..
>
> - The counter argument given to me from Sudeep at that time was,
>    "it is just a waste of time". Because if we aren't using the cluster
>    anymore, it will be switched off very soon and there is no point
>    wasting time changing freq to lowest..
>

I was just writing the same thing again, you saved my time :)
Since in IKS the cluster will be powered down if no CPU is active, I see no
point in doing this. bL_switch_request would initiate cluster powerdown and
trying to reset the clk to lowest might unnecessarily wake it up(though
the exact behaviour depends on the firmware/code managing it)

Regards,
Sudeep