From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Subject: Re: [PATCH v2 1/3] cpufreq:boost: CPU frequency boost code unification for software and hardware solutions Date: Wed, 12 Jun 2013 11:09:15 +0200 Message-ID: <20130612110915.64cfdb1d@amdc308.digital.local> References: <1370502472-7249-1-git-send-email-l.majewski@samsung.com> <1370941408-29304-1-git-send-email-l.majewski@samsung.com> <1370941408-29304-2-git-send-email-l.majewski@samsung.com> <20130612093938.1cf73c9a@amdc308.digital.local> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-reply-to: Sender: cpufreq-owner@vger.kernel.org To: Viresh Kumar Cc: "Rafael J. Wysocky" , "cpufreq@vger.kernel.org" , Linux PM list , Vincent Guittot , Jonghwa Lee , Myungjoo Ham , linux-kernel , Lukasz Majewski , Andre Przywara , Daniel Lezcano , Kukjin Kim List-Id: linux-pm@vger.kernel.org On Wed, 12 Jun 2013 13:39:18 +0530, Viresh Kumar wrote: Hi Viresh, > On 12 June 2013 13:09, Lukasz Majewski wrote: > > Hi Viresh, > > Hi Lukasz, > > >> Hi, > > Please don't remove the line which says, who wrote last mail at what > time. These >, >>, >>>, >>>>, ... have a meaning. They help us > understand who wrote what in bottom posting. And as you removed my > line, nobody can see who wrote above "Hi" to you :) Ok, mailer adjusted. > > >> You don't have to manually add "---" here. Just keep a blank line > >> instead. > > > > One "---" is added by git automatically. The [*] was added to > > distinct the changelog from rest of the commit. At least older > > versions of GIT required this to not include changelog to commit > > messages. > > You don't have to add an extra "---" line. Just write your changelog > after "---" added by git and give a blank line between your last > changelog line and below ones (probably just to make it more > readable and not a must, but i am not sure). I got your point. If you prefer, I will stick to it. No problem. As a side note: In the other open source project (u-boot) we use the pattern which I've used previously. It has one big advantage - I can edit change log at git gui (just below sign-of-by). It is simply more convenient for me :-). Those changes between "---" are simply skipped by git am afterwards. > > >> > drivers/cpufreq/cpufreq.c | 69 > >> > ++++++++++++++++++++++++++++++++++++++++++ > >> > drivers/cpufreq/freq_table.c | 57 > >> > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | > >> > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) > > > I think that we shall give users some flexibility and don't assume > > that low_level_boost is only used for one solution/vendor. > > > > It is also needed with software controlled boost. Please refer to > > patch 3/3. > > You didn't get me. I am not asking to keep it only for Intel. But keep > this variable as is (s/low_level_boost/set_boost_freq), and make it > optional. So, few drivers can implement it but not everybody is > required to. The low_level_boost (set_boost_freq)[*] is optional. However it seems to me, that the burden of changing available set of frequencies (when boost is enabled) must be put to cpufreq driver anyway. Without this function [*] defined, we cannot enable frequency boosting. > > So, Add another variable: boost_supported, which will tell cpufreq > core that boost is supported by governor or not. ^^^^^^^shouldn't it be cpufreq driver? Ok, boost_supported seems needed. In my opinion it shall be defined at cpufreq_driver structure (since it provides boosting infrastructure anyway). > > And a global variable in cpufreq.c boost_enabled to track status of > what user has requested. I think, that boost_enable shall be also defined at cpufreq driver (as proposed in the patch). We keep pointer to cpufreq driver at cpufreq.c anyway. Moreover, boost_enable flag is already defined at acpi-cpufreq.c (as static). We will have two flags for the same purpose. > > > However I've added one global flag: cpufreq_boost_sysfs_defined > > which indicates if /sys/devices/system/cpu/cpufreq/boost attribute > > has been already defined (to prevent multiple definitions attempts). > > You don't need this variable anymore as sysfs create file is now > moved to cpufreq_register_driver(), so this can't be called twice. Yes, in this situation the cpufreq_boost_sysfs_defined flag is redundant. > > >> > char *buf) +static ssize_t show_available_freqs(struct > >> > cpufreq_policy *policy, char *buf, > >> > + int show_boost) > >> > { > >> > unsigned int i = 0; > >> > unsigned int cpu = policy->cpu; > >> > @@ -186,22 +208,53 @@ static ssize_t show_available_freqs(struct > >> > cpufreq_policy *policy, char *buf) for (i = 0; > >> > (table[i].frequency != CPUFREQ_TABLE_END); i++) { if > >> > (table[i].frequency == CPUFREQ_ENTRY_INVALID) continue; > >> > + if (show_boost) > >> > + if (table[i].index != CPUFREQ_BOOST_FREQ) > >> > + continue; > >> > + > >> > >> Looks wrong. You will show boost freqs when show_boost is false. > > > > My purpose here is to display frequencies only tagged with > > CPUFREQ_BOOST_FREQ and when show_boost is true. > > > > When show_boost is false, the operation of the function is > > unchanged. > > Which is wrong. When show_boost is false, it means that user don't > want to see boost frequencies and so you should skip them. So we want as follows: show_boost = 1 ---> show only frequencies tagged as CPUFREQ_BOOST_FREQ show_boost = 0 ---> show only "normal" (non boost) frequencies > > >> With this patch alone, we would be using boost frequencies even in > >> normal cases where we haven't enabled boost. > > > > Correct me if I'm wrong here, but the > > cpufreq_freq_attr_boost_available_freqs will be added to cpufreq > > driver's freq_attr table (i.e. *exynos_cpufreq_attr[]). > > It is cpufreq driver's responsibility to add this attribute. By > > default all other drivers add only > > cpufreq_freq_attr_boost_available_freqs. > > You are just talking about showing boost freqs in sysfs. I am talking > about the frequencies that governors will select when boost is > disabled from sysfs. Because we don't skip boost frequencies in > target() routines, we will set them as and when governor requests > them. I think, that it is the main issue here and it shall be cleared out: Frequencies marked as: CPUFREQ_BOOST_FREQ are added permanently to the freq_table. That is the distinction to the original overclocking patch posted with LAB, where freq_boost structure was modified and boost frequencies were either valid or invalid. Then we can in SW control boost in two ways: 1. change policy->max value (to the maximal boost frequency) - as it is done now (v3) at Exynos. This is the simple solution (patch 3/3) 2. Modify all freq_table helper functions to be aware of boost and skip boost frequencies when boost_enable = 0. (as it was done at v2). This requires code modification at freq_table.c and reevaluation of policy. Maybe you have any other idea? -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group