On Wed, Oct 23, 2019 at 04:55:37PM +0530, Viresh Kumar wrote:
> 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 <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > 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.
>
Yes, I can do that.
> > + /* 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 ?
>
Yes I thought about the same. But since one can play with the firmware
table, I thought it's better to keep it generic instead of assuming that.
Since I had changed the firmware table, I didn't notice this issue. I
don't want to get into similar situation again ;)
But if you still insist that we can assume and work only for default, I
am fine by that, just that someone else may face the same issue if they
have some modified/experimental firmware table.
--
Regards,
Sudeep
