| Message ID | 20191022173215.13350-1-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] cpufreq: mark duplicate frequencies as invalid and continue as normal | expand |
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. -- viresh
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
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. -- viresh
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 */
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 -- 2.17.1