From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752570AbcD1MsO (ORCPT ); Thu, 28 Apr 2016 08:48:14 -0400 Received: from foss.arm.com ([217.140.101.70]:43057 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751539AbcD1MsK (ORCPT ); Thu, 28 Apr 2016 08:48:10 -0400 Subject: Re: [PATCH 2/2] cpufreq: arm_big_little: use generic OPP functions for {init,free}_opp_table To: Viresh Kumar References: <1461839114-29857-1-git-send-email-sudeep.holla@arm.com> <1461839114-29857-2-git-send-email-sudeep.holla@arm.com> <20160428112657.GD2915@vireshk-i7> Cc: Sudeep Holla , linux-kernel@vger.kernel.org, Viresh Kumar , "Rafael J. Wysocki" , linux-pm@vger.kernel.org From: Sudeep Holla Organization: ARM Message-ID: <57220686.9050507@arm.com> Date: Thu, 28 Apr 2016 13:48:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160428112657.GD2915@vireshk-i7> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> Cc: "Rafael J. Wysocki" >> Cc: linux-pm@vger.kernel.org >> Signed-off-by: Sudeep Holla >> --- >> 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