On 12 June 2013 14:39, Lukasz Majewski [off-list ref] wrote:

On Wed, 12 Jun 2013 13:39:18 +0530, Viresh Kumar wrote:

quoted

On 12 June 2013 13:09, Lukasz Majewski [off-list ref] wrote:

quoted

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.

Its not how I prefer it, but how everybody does it :)

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.

Yes, I am still asking you to follow the same steps:

git send-email --annotate ***patches***

and then write whatever you don't want to be logged by git am after
the "---" already present in the patch.

quoted

quoted

quoted

quoted

 drivers/cpufreq/cpufreq.c    |   69
++++++++++++++++++++++++++++++++++++++++++
drivers/cpufreq/freq_table.c |   57
++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h      |
12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-)

quoted

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.

Driver shouldn't play with boost freqs at runtime. This has to be handled
by cpufreq core. The only thing cpufreq driver must be doing in
low_level_boost or set_boost_freq (as what I suggested it to be), is
to set the boost frequency requested by core. And so this routine would
only be defined by drivers that have a special way of setting boost
frequencies. For others ->target() routine should be able to set all
freqs, boost or non boost.

Without this function [*] defined, we cannot enable frequency boosting.

why?

quoted

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?

Yeah, sorry :(

Ok, boost_supported seems needed. In my opinion it shall be defined at
cpufreq_driver structure (since it provides boosting infrastructure
anyway).

that's what I asked.

quoted

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).

Not really. Driver should only care about if it supports boost or not.
If it is enabled/disabled by sysfs or not should be kept inside core
in a global variable.

Moreover, if we have 5-6 cpufreq drivers compiled in (for multi-arch
compiled kernels), we will save memory wasted by this variable if
it is present in cpufreq_driver.

Moreover, boost_enable flag is already defined at
acpi-cpufreq.c (as static). We will have two flags for the same
purpose.

No, we don't have to. Just expose another API from cpufreq core
to get status of boost.

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

Yes.

quoted

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

quoted

LAB, where freq_boost structure was modified and boost frequencies were

either valid or invalid.

Yes.

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)

Drivers aren't supposed to set policy->max. It should be taken care
of by core or freq_table.c file.

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.

Yes, the core must be aware of it and must take the right decision
here. So, we need to take care of boost freq in freq_table.c