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

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

Hi Viresh,

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

The pointer to struct cpufreq_boost is needed for further reference
(as it is now done with struct cpufreq_driver pointer - *cpufreq_driver
- defined at cpufreq.c file).


> 
> >  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.
Good point, thanks.

> 
> > +                       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);

OK.

> 
> > +               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?

There are two kinds of attributes, which are exported by boost:

1. global boost (/sys/devices/system/cpu/cpufreq/boost)

2. attributes describing cpufreq abilities when boost is available
(/sys/devices/syste/cpu/cpu0/cpufreq/):
	- scaling_boost_frequencies - which will show over clocked
	  frequencies
	- the scaling_available_frequencies will also display boosted
	  frequency (when boost enabled)

Information from 2. is cpufreq_driver dependent. And that information
shouldn't been displayed when boost is not available



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

Extra precautions :-). I will remove it.

> 
> > +       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 :)

Hmm, I've exaggerated a bit here.... :)

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

This needs some explanation.

The sysfs entry presented at [1] doesn't bring any useful information
to reuse (like *policy). For this reason the global cpufreq_boost
pointer is needed.

However to efficiently manage the boost, it is necessary to keep per
policy pointer to the only struct cpufreq_boost instance (defined at
cpufreq_driver code).

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

This can be done by setting the struct cpufreq_boost status field to
CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
defined)

> 
> > +       /* 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 :(

Notifier is needed to disable boost when policy is changed (for
example when we change from ondemand/lab to performance governor).

I wanted to avoid the situation when boost stays enabled across
different governors.

The list of in system available policies is defined to allow boost
enable/disable for all policies available (by changing for example
policy->max field).

If we decide, that we will support only one policy (as it is now at
e.g. Exynos), the list is unnecessary here.

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

Yes, you are right.

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

OK, I will remove the entry.

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

Correct me if I'm wrong here, but in my understanding the boost shall be
only supported when both CPUFREQ_BOOST index is defined in a freq_table
and boost.state = CPUFREQ_BOOST_EN is set.

Setting of CPUFREQ_BOOST shouldn't by default allow to use over
clocking frequency. 

> 
> > +                   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?

To evaluate the maximal boost frequency from the frequency table. It is
then used as a delimiter (when LAB cooperates with thermal framework).

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

Yes, this seems to be a clearer solution.

> 
> > +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?)

The separate struct cpufreq_boost was created to explicitly separate
boost fields from cpufreq_driver structure. 

If in your opinion this structure is redundant, I can remove it and
extend cpufreq_driver structure.

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

*boost pointer is necessary when one wants to enable/disable boost from
e.g governor code (which operates mostly on struct cpufreq_policy
*policy pointers).

The boost_list is necessary to connect policies in a list. 

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

Ok, will be changed to something more descriptive. 

Thanks for thorough review :-)

-- 
Best regards,

Lukasz Majewski

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