Re: [PATCH v2 1/3] cpufreq:boost: CPU frequency boost code unification for software and hardware solutions

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Viresh,

> Hi,
> 
> Change subject to: "cpufreq: Add boost frequency support in core"

Ok.

> 
> On 11 June 2013 14:33, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > This commit adds support for software based frequency boosting.
> 
> No. It adds support for both software and hardware boosting. So just
> write: This commit adds boost frequency support in cpufreq core
> (Hardware & Software).

Ok.
> 
> > Some SoC (like Exynos4 - e.g. 4x12) allow setting frequency above
> > its normal condition limits. Such a change shall be only done for a
> > short
> 
> s/condition/operation
> s/change/mode
> s/done/used
> 

Ok.

> > time.
> >
> > Overclocking (boost) support is essentially provided by platform
> > dependent cpufreq driver.
> >
> > This commit unifies support for SW and HW (Intel) over clocking
> > solutions in the core cpufreq driver. Previously the "boost" sysfs
> > attribute was defined at acpi driver code.
> > By default boost is disabled. One global attribute is available at:
> > /sys/devices/system/cpu/cpufreq/boost.
> 
> Enter a blank line here.

Ok

> 
> > It only shows up when cpufreq driver supports overclocking.
> > Under the hood frequencies dedicated for boosting are marked with a
> > special flag (CPUFREQ_BOOST_FREQ) at driver's frequency table.
> > It is the user's concern to enable/disable overclocking with proper
> > call to sysfs.
> 
> Good.
> 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
> >
> > --- 
    ^^^^
[*] this --- was added manually by me.

> > Changes for v2:
> > - Removal of cpufreq_boost structure and move its fields to
> > cpufreq_driver structure
> > - Flag to indicate if global boost attribute is already defined
> > - Extent the pr_{err|debbug} functions to show current function
> > names ---
> 
> 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.

> 
> >  drivers/cpufreq/cpufreq.c    |   69
> > ++++++++++++++++++++++++++++++++++++++++++
> > drivers/cpufreq/freq_table.c |   57
> > ++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h      |
> > 12 ++++++++ 3 files changed, 136 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 1b8a48e..98ba5f1 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -40,6 +40,7 @@
> >   * also protects the cpufreq_cpu_data array.
> >   */
> >  static struct cpufreq_driver *cpufreq_driver;
> > +static bool cpufreq_boost_sysfs_defined;
> >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> >  #ifdef CONFIG_HOTPLUG_CPU
> >  /* This one keeps track of the previously set governor of a
> > removed CPU */ @@ -315,6 +316,33 @@
> > EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /*********************************************************************
> >   *                          SYSFS
> > INTERFACE                          *
> > *********************************************************************/
> > +ssize_t show_boost_status(struct kobject *kobj,
> > +                                struct attribute *attr, char *buf)
> > +{
> > +       return sprintf(buf, "%d\n", cpufreq_driver->boost_en);
> 
> This isn't correct. It shows if cpufreq driver supports boost or
> not and it should show if boost is enabled from sysfs when
> cpufreq driver supports boost.

The cpufreq_driver->low_level_boost() is only valid when cpufreq driver
supports boost. Otherwise it is NULL. Thereof you will not see those
attributes exported to /sysfs until cpufreq driver supports boost. 

When "boost" attribute is exported - then it returns state of boosting
(if it is enabled or not).

> 
> > +}
> > +
> > +static ssize_t store_boost_status(struct kobject *kobj, struct
> > attribute *attr,
> > +                                 const char *buf, size_t count)
> > +{
> > +       int ret, enable;
> > +
> > +       ret = sscanf(buf, "%d", &enable);
> > +       if (ret != 1 || enable < 0 || enable > 1)
> > +               return -EINVAL;
> > +
> > +       if (cpufreq_boost_trigger_state(enable)) {
> > +               pr_err("%s: Cannot %sable boost!\n", __func__,
> > +                      enable ? "en" : "dis");
> 
> %sable doesn't look very much readable. Use complete strings:
> "enable" and "disable".

Ok.

> 
> > +               return -EINVAL;
> > +       }
> > +
> > +       return count;
> > +}
> > +
> > +static struct global_attr global_boost = __ATTR(boost, 0644,
> > +                                               show_boost_status,
> > +                                               store_boost_status);
> 
> User define_one_global_rw.

Ok, will reuse available macros (this code was taken directly from
acpi-cpufreq.c file).

> 
> >  static struct cpufreq_governor *__find_governor(const char
> > *str_governor) {
> > @@ -754,6 +782,17 @@ static int cpufreq_add_dev_interface(unsigned
> > int cpu,
> 
> Why not do this in cpufreq_register_driver()?

Good point. I will move this code to cpufreq_register_driver.

> 
> >                         goto err_out_kobj_put;
> >                 drv_attr++;
> >         }
> > +       if (cpufreq_driver->low_level_boost
> > && !cpufreq_boost_sysfs_defined) {
> 
> I thought low_level_boost() is a function which will only be supported
> for drivers
> using hardware boost feature, like intel.

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. 

> And so we must have used
> boost_en here.

boost_en has two meanings:
0 - either boost disabled or not supported (when low_level_boost=NULL).
1 - boost is enabled.

> 
> > +               ret = sysfs_create_file(cpufreq_global_kobject,
> > +                                       &(global_boost.attr));
> 
> cpufreq_sysfs_create_file(), check
> 2361be23666232dbb4851a527f466c4cbf5340fc for details.

Ok, I will rebase those changes to newest
kernel/git/rafael/linux-pm.git ,branch
kernel_pm/pm-cpufreq

> 
> > +               if (ret) {
> > +                       pr_err("%s: cannot register global boost
> > sysfs file\n",
> > +                               __func__);
> > +                       goto err_out_kobj_put;
> > +               }
> > +               cpufreq_boost_sysfs_defined = 1;
> > +       }
> > +
> >         if (cpufreq_driver->get) {
> >                 ret = sysfs_create_file(&policy->kobj,
> > &cpuinfo_cur_freq.attr); if (ret)
> > @@ -1853,6 +1892,30 @@ static struct notifier_block __refdata
> > cpufreq_cpu_notifier = { };
> >
> >  /*********************************************************************
> > + *
> > BOOST                                              *
> > +
> > *********************************************************************/
> > +int cpufreq_boost_trigger_state(int state) +{
> > +       int ret = 0;
> > +
> > +       if (!cpufreq_driver->low_level_boost)
> > +               return -ENODEV;
> 
> I am certainly not aligned with your design. What's the
> use of this field? 

I had to rewrite a bit :-) patches since we decided to drop struct
cpufreq_boost.

I've added two fields to cpufreq_driver:
 - low_level_boost: 
	* When boost is not supported (default) it is set to NULL. This
	  field has two purposes: Indicates if boost is available on
	  the system and provides address of low level callback
	* When cpufreq driver assigns pointer to this field,
	  it means that it is supported

 - boost_en:
	* It shows if boost is enabled or disabled/not supported to the
	  rest of the system.

> And please update documentation too for these
> new entries in cpufreq_driver structure.

Ok I will extend it.

> 
> > +       if (cpufreq_driver->boost_en != state) {
> 
> So, you are using boost_en to see if boost is enabled from sysfs?
> Then you have put it at wrong place.

I check if boost is already enabled. 

> 
> I thought there would be three variables:
> - cpufreq_driver->boost_supported: boost is enabled for driver

For this purpose I check the low_level_boost pointer if it's NULL or
not.

> - cpufreq_driver->low_level_boost(): to set desired boost state
> (Only for hardware boosting)

It has two purposes (as described above) and can be used (defined) by
software and hardware boost solutions.

> - boost_enabled: global variable in cpufreq.c file, used only if
> cpufreq_driver->boost_supported is true.

I think that boost enabled flag shall be defined at cpufreq driver
(please look into acpi_cpufreq.c - which used another set of global
flags). 
It will then provide one flag for cpufreq.c (core) and drivers.

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

> 
> 
> > diff --git a/drivers/cpufreq/freq_table.c
> > b/drivers/cpufreq/freq_table.c index d7a7966..4e4f692 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -20,6 +20,27 @@
> >   *                     FREQUENCY TABLE
> > HELPERS                       *
> > *********************************************************************/
> >
> > +unsigned int
> > +cpufreq_frequency_table_max(struct cpufreq_frequency_table
> > *freq_table,
> > +                           int boost)
> > +{
> > +       int i = 0, boost_freq_max = 0, freq_max = 0;
> > +
> > +       for (; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
> > +               if (freq_table[i].index == CPUFREQ_BOOST_FREQ) {
> > +                       if (freq_table[i].frequency >
> > boost_freq_max)
> > +                               boost_freq_max =
> > freq_table[i].frequency;
> 
> Do above only when boost==true and below when boost==false.

Ok.

> 
> > +               } else {
> > +                       if (freq_table[i].frequency > freq_max)
> > +                               freq_max = freq_table[i].frequency;
> > +               }
> > +       }
> > +
> > +       return boost ? boost_freq_max : freq_max;
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_max);
> > +
> >  int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> >                                     struct cpufreq_frequency_table
> > *table) {
> > @@ -171,7 +192,8 @@ static DEFINE_PER_CPU(struct
> > cpufreq_frequency_table *, cpufreq_show_table); /**
> >   * show_available_freqs - show available frequencies for the
> > specified CPU */
> > -static ssize_t show_available_freqs(struct cpufreq_policy *policy,
> > 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.


> 
> >                 count += sprintf(&buf[count], "%d ",
> > table[i].frequency); }
> >         count += sprintf(&buf[count], "\n");
> >
> >         return count;
> > +}
> >
> > +/**
> > + * show_available_normal_freqs - show normal boost frequencies for
> > + * the specified CPU
> > + */
> > +static ssize_t show_available_normal_freqs(struct cpufreq_policy
> > *policy,
> > +                                          char *buf)
> > +{
> > +       return show_available_freqs(policy, buf, 0);
> >  }
> >
> >  struct freq_attr cpufreq_freq_attr_scaling_available_freqs = {
> >         .attr = { .name = "scaling_available_frequencies",
> >                   .mode = 0444,
> >                 },
> > -       .show = show_available_freqs,
> > +       .show = show_available_normal_freqs,
> >  };
> >  EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs);
> >
> > +/**
> > + * show_available_boost_freqs - show available boost frequencies
> > for
> > + * the specified CPU
> > + */
> > +static ssize_t show_available_boost_freqs(struct cpufreq_policy
> > *policy,
> > +                                         char *buf)
> > +{
> > +       return show_available_freqs(policy, buf, 1);
> > +}
> > +
> > +struct freq_attr cpufreq_freq_attr_boost_available_freqs = {
> > +       .attr = { .name = "scaling_boost_frequencies",
> > +                 .mode = 0444,
> > +               },
> > +       .show = show_available_boost_freqs,
> > +};
> > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_boost_available_freqs);
> > +
> 
> Code redundancy can be reduced by creating a macro for declaring
> **_availabe_freqs, its attributes and export symbol.

Yes, you are right here. Those two structures only differ with
different names.

> 
> >  /*
> >   * if you use these, you must assure that the frequency table is
> > valid
> >   * all the time between get_attr and put_attr!
> 
> 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.


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group