Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost

Hi,

On 6 June 2013 12:37, Lukasz Majewski l.majewski@samsung.com wrote:

This commit adds support for software based frequency boosting. Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal condition limits. This can be done for some short time.

Overclocking (boost) support came from cpufreq driver (which is platform dependent). Hence the data structure describing it is defined at its file.

To allow support for either SW and HW (Intel) based boosting, the cpufreq core code has been extended to support both solutions.

The main boost switch has been put at /sys/devices/system/cpu/cpufreq/boost.

Log requires some better paragraphs but I am not concerned about it for now.

Signed-off-by: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Myungjoo Ham myungjoo.ham@samsung.com

---

drivers/cpufreq/cpufreq.c | 156 ++++++++++++++++++++++++++++++++++++++++++ drivers/cpufreq/freq_table.c | 87 ++++++++++++++++++++++- include/linux/cpufreq.h | 35 +++++++++- 3 files changed, 275 insertions(+), 3 deletions(-)

My initial view on this patch is: "It is overly engineered and needs to get simplified"

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index ca74e27..8cf9a92 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -133,6 +133,11 @@ bool have_governor_per_policy(void) return cpufreq_driver->have_governor_per_policy; }

+/**

• • Global definition of cpufreq_boost structure
• */

+struct cpufreq_boost *cpufreq_boost;

Probably just a 'static bool' here cpufreq_boost_enabled. Which takes care of selection from sysfs.

static struct cpufreq_governor *__find_governor(const char *str_governor) { @@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu, if (cpufreq_set_drv_attr_files(policy, cpufreq_driver->attr)) goto err_out_kobj_put;

•   if (cpufreq_driver->boost) {
  

•           if (sysfs_create_file(cpufreq_global_kobject,
  

•                                 &(global_boost.attr)))
  

This will report error for systems where we have two policy structures. As we are creating something already present.

•                   pr_warn("could not register global boost sysfs file\n");
  

•           else
  

•                   pr_debug("registered global boost sysfs file\n");
  

Please make all your prints to print function name too:

pr_debug("%s: foo\n", __func__, foo);

•           if (cpufreq_set_drv_attr_files(policy,
  

•                                          cpufreq_driver->boost->attr))
  

Why is this required? Why do we want platforms to add some files in sysfs?

/*********************************************************************

• •           BOOST                                              *
    

• *********************************************************************/

+int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state) +{

•   struct cpufreq_boost *boost;
  

•   unsigned long flags;
  

•   int ret = 0;
  

•   if (!policy || !policy->boost || !policy->boost->low_level_boost)
  

•           return -ENODEV;
  

•   boost = policy->boost;
  

•   write_lock_irqsave(&cpufreq_driver_lock, flags);
  

•   if (boost->status != state) {
  

•           policy->boost->status = state;
  

•           ret = boost->low_level_boost(policy, state);
  

•           if (ret) {
  

•                   pr_err("BOOST cannot %sable low level code (%d)\n",
  

•                          state ? "en" : "dis", ret);
  

•                   return ret;
  

•           }
  

•   }
  

•   write_unlock_irqrestore(&cpufreq_driver_lock, flags);
  

•   pr_debug("cpufreq BOOST %sabled\n", state ? "en" : "dis");
  

•   return 0;
  

+}

+/**

• • cpufreq_boost_notifier - notifier callback for cpufreq policy change.
• • <at> nb: struct notifier_block * with callback info.
• • <at> event: value showing cpufreq event for which this function invoked.
• • <at> data: callback-specific data
• */

+static int cpufreq_boost_notifier(struct notifier_block *nb,

•                             unsigned long event, void *data)
  

+{

•   struct cpufreq_policy *policy = data;
  

•   if (event == CPUFREQ_INCOMPATIBLE) {
  

•           if (!policy || !policy->boost)
  

•                   return -ENODEV;
  

•           if (policy->boost->status == CPUFREQ_BOOST_EN) {
  

•                   pr_info("NOTIFIER BOOST: MAX: %d e:%lu cpu: %d\n",
  

•                           policy->max, event, policy->cpu);
  

•                   cpufreq_boost_trigger_state(policy, 0);
  

•           }
  

•   }
  

•   return 0;
  

+}

+/* Notifier for cpufreq policy change */ +static struct notifier_block cpufreq_boost_notifier_block = {

•   .notifier_call = cpufreq_boost_notifier,
  

+};

+int cpufreq_boost_init(struct cpufreq_policy *policy) +{

•   int ret = 0;
  

•   if (!policy)
  

•           return -EINVAL;
  

Heh, policy can't be NULL here.

•   if (!cpufreq_driver->boost) {
  

•           pr_err("Boost mode not supported on this device\n");
  

Wow!! You want to screw everybody else's logs with this message. Its not a crime if you don't have boost mode supported :)

Actually this routine must be called only if cpufreq_driver->boost is valid.

•           return -ENODEV;
  

•   }
  

•   policy->boost = cpufreq_boost = cpufreq_driver->boost;
  

Why are we copying same pointer to policy->boost? Driver is passing pointer to a single memory location, just save it globally.

•   /* disable boost for newly created policy - as we e.g. change
  

•      governor */
  

•   policy->boost->status = CPUFREQ_BOOST_DIS;
  

Drivers supporting boost may want boost to be enabled by default, maybe without any sysfs calls.

•   /* register policy notifier */
  

•   ret = cpufreq_register_notifier(&cpufreq_boost_notifier_block,
  

•                                   CPUFREQ_POLICY_NOTIFIER);
  

•   if (ret) {
  

•           pr_err("CPUFREQ BOOST notifier not registered.\n");
  

•           return ret;
  

•   }
  

•   /* add policy to policies list headed at struct cpufreq_boost */
  

•   list_add_tail(&policy->boost_list, &cpufreq_boost->policies);
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(cpufreq_boost_init);

Why do we need to maintain a list of boost here? notifiers? complex :(

+/*********************************************************************

•           REGISTER / UNREGISTER CPUFREQ DRIVER                *
  

*********************************************************************/

@@ -1954,6 +2106,10 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) pr_debug("unregistering driver %s\n", driver->name);

    subsys_interface_unregister(&cpufreq_interface);

•   if (cpufreq_driver->boost)
  

•           sysfs_remove_file(cpufreq_global_kobject, &(global_boost.attr));
  

You haven't removed this from policy. Memory leak.

    unregister_hotcpu_notifier(&cpufreq_cpu_notifier);

    write_lock_irqsave(&cpufreq_driver_lock, flags);

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c index d7a7966..0e95524 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -3,6 +3,8 @@

• Copyright (C) 2002 - 2003 Dominik Brodowski

• • Copyright (C) 2013 Lukasz Majewski l.majewski@samsung.com

You shouldn't add it unless you did some major work on this file. You aren't maintaining this file in 2013.

+static int cpufreq_frequency_table_skip_boost(struct cpufreq_policy *policy,

•                                         unsigned int index)
  

+{

•   if (index == CPUFREQ_BOOST)
  

•           if (!policy->boost ||
  

This shouldn't be true. If index has got CPUFREQ_BOOST, then driver has to support boost.

•               policy->boost->status == CPUFREQ_BOOST_DIS)
  

•                   return 1;
  

•   return 0;
  

+}

+unsigned int +cpufreq_frequency_table_boost_max(struct cpufreq_frequency_table *freq_table) +{

•   int index, boost_freq_max;
  

•   for (index = 0, boost_freq_max = 0;
  

•           freq_table[index].frequency != CPUFREQ_TABLE_END; index++)
  

•           if (freq_table[index].index == CPUFREQ_BOOST) {
  

•                   if (freq_table[index].frequency > boost_freq_max)
  

•                           boost_freq_max = freq_table[index].frequency;
  

•           }
  

•   return boost_freq_max;
  

+} +EXPORT_SYMBOL_GPL(cpufreq_frequency_table_boost_max);

why do we need this?

/*

• if you use these, you must assure that the frequency table is valid
• all the time between get_attr and put_attr!

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..1294c8c 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -88,6 +88,25 @@ struct cpufreq_real_policy { struct cpufreq_governor *governor; /* see below */ };

+#define CPUFREQ_BOOST_DIS (0) +#define CPUFREQ_BOOST_EN (1)

You don't need these.. Just create variable as bool and 0 & 1 would be fine.

+struct cpufreq_policy; +struct cpufreq_boost {

•   unsigned int            max_boost_freq; /* maximum value of
  

•                                            * boosted freq */
  

•   unsigned int            max_normal_freq; /* non boost max freq */
  

•   int                     status; /* status of boost */
  

•   /* boost sysfs attributies */
  

•   struct freq_attr        **attr;
  

•   /* low-level trigger for boost */
  

•   int (*low_level_boost) (struct cpufreq_policy *policy, int state);
  

•   struct list_head        policies;
  

+};

We don't need it. Just add two more fields to cpufreq_driver: - have_boost_freqs and low_level_boost (maybe a better name. What's its use?)

struct cpufreq_policy { /* CPUs sharing clock, require sw coordination */ cpumask_var_t cpus; /* Online CPUs only */ @@ -113,6 +132,9 @@ struct cpufreq_policy {

    struct cpufreq_real_policy      user_policy;

•   struct cpufreq_boost    *boost;
  

•   struct list_head        boost_list;
  

We don't need both of these.

    struct kobject          kobj;
    struct completion       kobj_unregister;

};

@@ -277,7 +302,6 @@ struct cpufreq_driver { int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);

??

void cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state);

@@ -403,6 +427,9 @@ extern struct cpufreq_governor cpufreq_gov_conservative; #define CPUFREQ_ENTRY_INVALID ~0 #define CPUFREQ_TABLE_END ~1

+/* Define index for boost frequency */ +#define CPUFREQ_BOOST ~2

s/CPUFREQ_BOOST/CPUFREQ_BOOST_FREQ