[RFC] cpufreq: mark duplicate frequencies as invalid and continue as normal
diff mbox series

Message ID 20191022173215.13350-1-sudeep.holla@arm.com
State RFC, archived
Headers show
Series
  • [RFC] cpufreq: mark duplicate frequencies as invalid and continue as normal
Related show

Commit Message

Sudeep Holla Oct. 22, 2019, 5:32 p.m. UTC
Currently if we encounter duplicate frequency table entries, we abort
the validation and return error immediately. Instead of failing, we
can mark the entry as invalid and continue to function normal.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpufreq/freq_table.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Hi Viresh,

Since commit da0c6dc00c69 ("cpufreq: Handle sorted frequency tables more
efficiently"), I seem to have modified the firmware entry on my TC2 to
drop 500MHz and had not seen the issue with duplicate entries and had
totally forgotten about it.

Recently I reverted back to original setting as I corrupted it and
started seeing this issues. I don't know the background for raising
duplicates as fatal error but we did allow it when we add arm_big_little.c
and hence this RFC. If there are known issues with this approach, I can
continue with changed firmware config.

With switcher, we have:
(little cluster)
Virt: 175 MHz, 200 MHz, 250 MHz, 300 MHz, 350 MHz, 400 MHz, 450 MHz, 500 MHz
Actu: 350 MHz, 400 MHz, 500 MHz, 600 MHz, 700 MHz, 800 MHz, 900 MHz, 1000 MHz
(big cluster)
500 MHz, 600 MHz, 700 MHz, 800 MHz, 900 MHz, 1000 MHz, 1.10 GHz, 1.20 GHz

with 500 MHz duplicate in merged table.

Regards,
Sudeep

Comments

Viresh Kumar Oct. 23, 2019, 3:26 a.m. UTC | #1
On 22-10-19, 18:32, Sudeep Holla wrote:
> Currently if we encounter duplicate frequency table entries, we abort
> the validation and return error immediately. Instead of failing, we
> can mark the entry as invalid and continue to function normal.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/freq_table.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Hi Viresh,
> 
> Since commit da0c6dc00c69 ("cpufreq: Handle sorted frequency tables more
> efficiently"), I seem to have modified the firmware entry on my TC2 to
> drop 500MHz and had not seen the issue with duplicate entries and had
> totally forgotten about it.
> 
> Recently I reverted back to original setting as I corrupted it and
> started seeing this issues. I don't know the background for raising
> duplicates as fatal error but we did allow it when we add arm_big_little.c
> and hence this RFC. If there are known issues with this approach, I can
> continue with changed firmware config.
> 
> With switcher, we have:
> (little cluster)
> Virt: 175 MHz, 200 MHz, 250 MHz, 300 MHz, 350 MHz, 400 MHz, 450 MHz, 500 MHz
> Actu: 350 MHz, 400 MHz, 500 MHz, 600 MHz, 700 MHz, 800 MHz, 900 MHz, 1000 MHz
> (big cluster)
> 500 MHz, 600 MHz, 700 MHz, 800 MHz, 900 MHz, 1000 MHz, 1.10 GHz, 1.20 GHz
> 
> with 500 MHz duplicate in merged table.
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index ded427e0a488..e9bf287846d6 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -305,9 +305,10 @@ static int set_freq_table_sorted(struct cpufreq_policy *policy)
>  		}
>  
>  		if (pos->frequency == prev->frequency) {
> -			pr_warn("Duplicate freq-table entries: %u\n",
> +			pr_warn("Duplicate freq-table entries: %u marking it invalid\n,",
>  				pos->frequency);
> -			return -EINVAL;
> +			pos->frequency = CPUFREQ_ENTRY_INVALID;
> +			continue;
>  		}
>  
>  		/* Frequency increased from prev to pos */

Of course we can do this, but I don't see why we shouldn't force
people to fix the freq-tables instead. What's the point of allowing
people to have duplicate entries instead ? This shouldn't happen with
OPP tables as we check for duplicate entries there as well.
Sudeep Holla Oct. 23, 2019, 9:06 a.m. UTC | #2
On Wed, Oct 23, 2019 at 08:56:08AM +0530, Viresh Kumar wrote:
> On 22-10-19, 18:32, Sudeep Holla wrote:
> > Currently if we encounter duplicate frequency table entries, we abort
> > the validation and return error immediately. Instead of failing, we
> > can mark the entry as invalid and continue to function normal.
> >
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/cpufreq/freq_table.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > Hi Viresh,
> >
> > Since commit da0c6dc00c69 ("cpufreq: Handle sorted frequency tables more
> > efficiently"), I seem to have modified the firmware entry on my TC2 to
> > drop 500MHz and had not seen the issue with duplicate entries and had
> > totally forgotten about it.
> >
> > Recently I reverted back to original setting as I corrupted it and
> > started seeing this issues. I don't know the background for raising
> > duplicates as fatal error but we did allow it when we add arm_big_little.c
> > and hence this RFC. If there are known issues with this approach, I can
> > continue with changed firmware config.
> >
> > With switcher, we have:
> > (little cluster)
> > Virt: 175 MHz, 200 MHz, 250 MHz, 300 MHz, 350 MHz, 400 MHz, 450 MHz, 500 MHz
> > Actu: 350 MHz, 400 MHz, 500 MHz, 600 MHz, 700 MHz, 800 MHz, 900 MHz, 1000 MHz
> > (big cluster)
> > 500 MHz, 600 MHz, 700 MHz, 800 MHz, 900 MHz, 1000 MHz, 1.10 GHz, 1.20 GHz
> >
> > with 500 MHz duplicate in merged table.
> >
> > Regards,
> > Sudeep
> >
> > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> > index ded427e0a488..e9bf287846d6 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -305,9 +305,10 @@ static int set_freq_table_sorted(struct cpufreq_policy *policy)
> >  		}
> >
> >  		if (pos->frequency == prev->frequency) {
> > -			pr_warn("Duplicate freq-table entries: %u\n",
> > +			pr_warn("Duplicate freq-table entries: %u marking it invalid\n,",
> >  				pos->frequency);
> > -			return -EINVAL;
> > +			pos->frequency = CPUFREQ_ENTRY_INVALID;
> > +			continue;
> >  		}
> >
> >  		/* Frequency increased from prev to pos */
>
> Of course we can do this, but I don't see why we shouldn't force
> people to fix the freq-tables instead.

Agreed. We still have warning :)

> What's the point of allowing people to have duplicate entries instead ?

I agree, we shouldn't. But this is in IKS which is built in the merged
table by the driver. We can fix that, but found this easier and simple.
And since it was allowed when the driver was merged, just thought of
checking the details with you.

> This shouldn't happen with OPP tables as we check for duplicate entries
> there as well.
>

Yes, but OPPs are per cpu cluster and they don't have duplicates in this
case.

--
Regards,
Sudeep
Viresh Kumar Oct. 23, 2019, 9:14 a.m. UTC | #3
On 23-10-19, 10:06, Sudeep Holla wrote:
> I agree, we shouldn't. But this is in IKS which is built in the merged
> table by the driver. We can fix that, but found this easier and simple.
> And since it was allowed when the driver was merged, just thought of
> checking the details with you.

That was 7 years back :)

Yeah, lets fix IKS tables then instead of adding that to the core.

Patch
diff mbox series

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index ded427e0a488..e9bf287846d6 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -305,9 +305,10 @@  static int set_freq_table_sorted(struct cpufreq_policy *policy)
 		}
 
 		if (pos->frequency == prev->frequency) {
-			pr_warn("Duplicate freq-table entries: %u\n",
+			pr_warn("Duplicate freq-table entries: %u marking it invalid\n,",
 				pos->frequency);
-			return -EINVAL;
+			pos->frequency = CPUFREQ_ENTRY_INVALID;
+			continue;
 		}
 
 		/* Frequency increased from prev to pos */