Messages in this thread | | | Subject | Re: [PATCH 2/2] cpufreq: arm_big_little: use generic OPP functions for {init,free}_opp_table | From | Sudeep Holla <> | Date | Thu, 28 Apr 2016 13:48:06 +0100 |
| |
On 28/04/16 12:26, Viresh Kumar wrote: > On 28-04-16, 11:25, Sudeep Holla wrote: >> Currently when performing random hotplugs and suspend-to-ram(S2R) on >> systems using arm_big_little cpufreq driver, we get warnings similar to: >> >> cpu cpu1: _opp_add: duplicate OPPs detected. Existing: freq: 600000000, >> volt: 800000, enabled: 1. New: freq: 600000000, volt: 800000, enabled: 1 >> >> This is mainly because the OPPs for the shared cpus are not set. We can >> just use dev_pm_opp_of_cpumask_add_table in case the OPPs are obtained >> from DT(arm_big_little_dt.c) or use dev_pm_opp_set_sharing_cpus if the >> OPPs are obtained by other means like firmware(e.g. scpi-cpufreq.c) >> >> Also now that the generic dev_pm_opp_cpumask_remove_table can handle >> removal of opp table and entries for all associated CPUs, we can reuse >> dev_pm_opp_cpumask_remove_table as free_opp_table in cpufreq_arm_bL_ops. >> >> This patch makes necessary changes to reuse the generic OPP functions for >> {init,free}_opp_table and thereby eliminating the warnings. >> >> Cc: Viresh Kumar <vireshk@kernel.org> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Cc: linux-pm@vger.kernel.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/cpufreq/arm_big_little.c | 54 ++++++++++++++++++++----------------- >> drivers/cpufreq/arm_big_little.h | 4 +-- >> drivers/cpufreq/arm_big_little_dt.c | 21 ++------------- >> drivers/cpufreq/scpi-cpufreq.c | 24 ++++++++++++++--- >> 4 files changed, 54 insertions(+), 49 deletions(-) >> >> Hi Viresh, >> >> Why is dynamic OPPs not deleted in dev_pm_opp_{,cpumask_}remove_table ? >> It would remove some code in scpi-cpufreq.c but I would like to understand. >> Is there any use in not deleting them there ? > > That was intentional, consider this case: > - OPPs are added dynamically from platform code > - insmod cpufreq-dt.ko (add static (dt) OPPs as well, mostly fail) > - rmmod cpufreq-dt.ko (remove all OPPs) > > - insmod cpufreq-dt.ko (no more OPPs available, as we removed them both). > > That's bad ? > > Now, how to fix platforms which don't add dynamic OPPs from platform code? But > something like the scpi driver, which can get inserted/removed again. > > This is what I would suggest: > - Even if dev_pm_opp_of_cpumask_remove_table() isn't doing any OF specific > stuff, lets not update its name and move it out of the ifdef, as it will be > used only if the user has created OPPs using DT earlier. > - But rather make those two routines call two new routines in the core (which > don't depend on OF) and implement what these of-remove routines do. > - Add two more routines for removing OPPs created dynamically and call the two > routines created in previous step. > > I hope you followed that :) >
Yes I got it. I had thought of this initially, but somehow got convinced that we can delete dynamic OPPs too.
>> -static void scpi_free_opp_table(struct device *cpu_dev) >> +static void scpi_free_opp_table(cpumask_var_t cpumask) >> { >> + struct device *cpu_dev = get_cpu_device(cpumask_first(cpumask)); >> + >> + cpumask_clear_cpu(cpu_dev->id, cpumask); >> + dev_pm_opp_cpumask_remove_table(cpumask); >> + cpumask_set_cpu(cpu_dev->id, cpumask); >> + >> scpi_opp_table_ops(cpu_dev, true); >> } > > This was a *really* crazy idea :) >
I knew that :)
> And btw, I think you better move to cpufreq-dt, instead of struggling with this > one :) >
Yes that's next on my TODO, but since this is duplicate messages are getting reported now, it's better to fix that rather than wait for move to cpufreq-dt. BTW cpufreq-dt will be misnomer after I make it work with firmware driver DVFS.
Anyways, what ever change to fix these warning now will help to move to cpufreq-dt IMO.
-- Regards, Sudeep
| |