Hi,

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

On 11 June 2013 14:33, Lukasz Majewski [off-list ref] 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).

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

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.

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 <redacted>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>

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

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.

+}
+
+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".

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

User define_one_global_rw.

Why not do this in 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. And so we must have used boost_en
here.

+               ret = sysfs_create_file(cpufreq_global_kobject,
+                                       &(global_boost.attr));

cpufreq_sysfs_create_file(), check 2361be23666232dbb4851a527f466c4cbf5340fc
for details.

I am certainly not aligned with your design. What's the
use of this field? And please update documentation too for these
new entries in cpufreq_driver structure.

+       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 thought there would be three variables:
- cpufreq_driver->boost_supported: boost is enabled for driver
- cpufreq_driver->low_level_boost(): to set desired boost state
(Only for hardware boosting)
- boost_enabled: global variable in cpufreq.c file, used only if
cpufreq_driver->boost_supported is true.

Do above only when boost==true and below when boost==false.

Looks wrong. You will show boost freqs when show_boost is false.

                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.

 /*
  * 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.