Re: [PATCH 2/2] cpufreq: vexpress-spc: find and skip duplicates when merging frequencies

[Viresh Kumar]

On 23-10-19, 12:08, Sudeep Holla wrote:
> Currently the cpufreq core aborts the validation and return error
> immediately when it encounter duplicate frequency table entries.
> This change was introduced long back since commit da0c6dc00c69
> ("cpufreq: Handle sorted frequency tables more efficiently").
> 
> However, this missed the testing with modified firmware for long time.
> Inorder to make it work with default settings, we need to ensure the
> merged table for bL switcher contains no duplicates. Find the duplicates
> and skip them when merging the frequenct tables of A15 and A7 clusters.
> 
> Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> ---
>  drivers/cpufreq/vexpress-spc-cpufreq.c | 35 ++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c
> index 093ef8d3a8d4..921dbd42b3bb 100644
> --- a/drivers/cpufreq/vexpress-spc-cpufreq.c
> +++ b/drivers/cpufreq/vexpress-spc-cpufreq.c
> @@ -242,6 +242,19 @@ static inline u32 get_table_max(struct cpufreq_frequency_table *table)
>  	return max_freq;
>  }
>  
> +static bool search_frequency(struct cpufreq_frequency_table *table, int size,
> +			     unsigned int freq)
> +{
> +	int count;
> +
> +	for (count = 0; count < size; count++) {
> +		if (table[count].frequency == freq)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int merge_cluster_tables(void)
>  {
>  	int i, j, k = 0, count = 1;
> @@ -256,13 +269,21 @@ static int merge_cluster_tables(void)
>  
>  	freq_table[MAX_CLUSTERS] = table;
>  
> -	/* Add in reverse order to get freqs in increasing order */
> -	for (i = MAX_CLUSTERS - 1; i >= 0; i--) {
> -		for (j = 0; freq_table[i][j].frequency != CPUFREQ_TABLE_END;
> -		     j++, k++) {
> -			table[k].frequency =
> -				VIRT_FREQ(i, freq_table[i][j].frequency);
> -		}

I think we can still use this single loop, which already starts from
A7 cluster. Just that we can add an if (A15) block inside it as the
first line.

> +	/* Add A7_CLUSTER first to get freqs in increasing order */
> +	for (j = 0; freq_table[A7_CLUSTER][j].frequency != CPUFREQ_TABLE_END;
> +	     j++, k++) {
> +		table[k].frequency =
> +			VIRT_FREQ(A7_CLUSTER, freq_table[A7_CLUSTER][j].frequency);
> +	}
> +	count = k;
> +
> +	/* And then A15_CLUSTER checking for duplicates */
> +	for (j = 0; freq_table[A15_CLUSTER][j].frequency != CPUFREQ_TABLE_END;
> +	     j++) {
> +		if (search_frequency(table, count,
> +				     freq_table[A15_CLUSTER][j].frequency))
> +			continue; /* skip duplicates */
> +		table[k++].frequency = freq_table[A15_CLUSTER][j].frequency;
>  	}

How many duplicate entries are there anyway in the firmware? Or do we
really need to make it that generic? I mean, only the last of A7 and
first of A15 should be overlapping, in that case why search entire
table again ?

>  
>  	table[k].driver_data = k;
> -- 
> 2.17.1

-- 
viresh