Re: [PATCH v4 2/7] cpufreq: Add boost frequency support in core

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

On Wed, 26 Jun 2013 16:24:32 +0530, Viresh Kumar wrote:
> On 19 June 2013 22:42, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > Boost sysfs attribute is always exported (to support legacy API). By
> > default boost is exported as read only. One global attribute is
> > available at: /sys/devices/system/cpu/cpufreq/boost.
> 
> I assume you are going to fix this as discussed in other threads.
Yes. Boost attribute will be visible only when boost is supported.

> 
> > Changes for v4:
> > - Remove boost parameter from cpufreq_frequency_table_cpuinfo()
> > function
> > - Introduce cpufreq_boost_supported() method
> > - Use of cpufreq_boost_supported() and cpufreq_boost_enabled() to
> > decide if frequency shall be skipped
> > - Rename set_boost_freq() to enable_boost()
> > - cpufreq_attr_available_freq() moved to freq_table.c
> > - Use policy list to get access to cpufreq policies
> > - Rename global boost flag (cpufreq_boost_enabled -> boost_enabled)
> > - pr_err corrected ( %sable)
> > - Remove sanity check at cpufreq_boost_trigger_state() entrance [to
> > test if boost is supported]
> > - Use either HW (boost_enable) callback or SW managed boost
> > - Introduce new cpufreq_boost_trigger_state_sw() method to handle
> > boost at SW.
> > - Protect boost_enabled manipulation with lock
> > - Always export boost attribute (preserve legacy behaviour). When
> > boost is not supported this attribute is read only
> 
> Very well written changelog. But write it after ---

I will stick to the rule proposed at patch 1/4, ver 4.

> 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 665e641..9141d33 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 boost_enabled;
> >  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 */ @@ -316,6 +317,29 @@
> > EXPORT_SYMBOL_GPL(cpufreq_notify_transition); /*********************************************************************
> >   *                          SYSFS
> > INTERFACE                          *
> > *********************************************************************/
> > +ssize_t show_boost(struct kobject *kobj,
> > +                                struct attribute *attr, char *buf)
> > +{
> > +       return sprintf(buf, "%d\n", boost_enabled);
> > +}
> > +
> > +static ssize_t store_boost(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 enable boost!\n", __func__);
> > +               return -EINVAL;
> > +       }
> 
> Probably do boost_enabled = true here.

I would prefer to set boot_enabled at
cpufreq_boost_trigger_state() method. It is closer to the 
cpufreq_driver->enable_boost and cpufreq_boost_trigger_state_sw();
functions, which do change the freq.

> 
> > +       return count;
> > +}
> > +define_one_global_rw(boost);
> 
> >  /*********************************************************************
> > + *
> > BOOST                                              *
> > +
> > *********************************************************************/
> > +static int cpufreq_boost_trigger_state_sw(void) +{
> > +       struct cpufreq_frequency_table *freq_table;
> > +       struct cpufreq_policy *policy;
> > +       int ret = -EINVAL;
> > +
> > +       list_for_each_entry(policy, &cpufreq_policy_list,
> > policy_list) {
> > +               freq_table =
> > cpufreq_frequency_get_table(policy->cpu);
> > +               if (freq_table)
> > +                       ret =
> > cpufreq_frequency_table_cpuinfo(policy,
> > +                                                       freq_table);
> > +       }
> > +
> > +       return ret;
> > +
> > +}
> 
> add blank line here.

OK.

> 
> > +int cpufreq_boost_trigger_state(int state)
> > +{
> > +       unsigned long flags;
> > +       int ret = 0;
> > +
> > +       if (boost_enabled != state) {
> > +               write_lock_irqsave(&cpufreq_driver_lock, flags);
> > +               boost_enabled = state;
> > +               if (cpufreq_driver->enable_boost)
> > +                       ret = cpufreq_driver->enable_boost(state);
						  ^^^^^^^^^^^^^
					I would prefer to change this
					name to enable_boost_hw
It is more informative, since it is tailored to hw based boost (Intel).

> > +               else
> > +                       ret = cpufreq_boost_trigger_state_sw();
> > +
> > +               if (ret) {
> > +                       boost_enabled = 0;
> > +
> > write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> > +                       pr_err("%s: BOOST cannot enable (%d)\n",
> > +                              __func__, ret);
> > +
> > +                       return ret;
> > +               }
> > +               write_unlock_irqrestore(&cpufreq_driver_lock,
> > flags);
> 
> You can rewrite if (ret) and unlock() code to make it less redundant.
> unlock and return ret at the end and write other stuff before it.

I will rewrite it as follow:

if (ret)
	boost_enabled = 0;
		
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
pr_debug("%s: cpufreq BOOST %s\n", __func__,
		 state ? "enabled" : "disabled");

return ret;

> 
> > +               pr_debug("%s: cpufreq BOOST %s\n", __func__,
> > +                        state ? "enabled" : "disabled");
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +int cpufreq_boost_supported(void)
> > +{
> > +       return cpufreq_driver->boost_supported;
> > +}
> > +
> > +int cpufreq_boost_enabled(void)
> > +{
> > +       return boost_enabled;
> > +}
> 
> EXPORT_SYMBOL_GPL ??

I will export cpufreq_boost_enabled() and cpufreq_boost_supported()

> 
> > +/*********************************************************************
> >   *               REGISTER / UNREGISTER CPUFREQ
> > DRIVER                *
> > *********************************************************************/
> >
> > @@ -1936,6 +2019,16 @@ int cpufreq_register_driver(struct
> > cpufreq_driver *driver_data) cpufreq_driver = driver_data;
> >         write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> >
> > +       if (!cpufreq_driver->boost_supported)
> > +               boost.attr.mode = 0444;
            Will be removed ^^^^^^^^^^^^^^^
> > +
> > +       ret = cpufreq_sysfs_create_file(&(boost.attr));
> > +       if (ret) {
> > +               pr_err("%s: cannot register global boost sysfs
> > file\n",
> > +                      __func__);
> > +               goto err_null_driver;
> > +       }
> 
> This would change.

This will be only exported when cpufreq_boost_supported() is true.

> 
> >         ret = subsys_interface_register(&cpufreq_interface);
> >         if (ret)
> >                 goto err_null_driver;
> > @@ -1992,6 +2085,8 @@ int cpufreq_unregister_driver(struct
> > cpufreq_driver *driver) pr_debug("unregistering driver %s\n",
> > driver->name);
> >
> >         subsys_interface_unregister(&cpufreq_interface);
> > +
> > +       cpufreq_sysfs_remove_file(&(boost.attr));
> >         unregister_hotcpu_notifier(&cpufreq_cpu_notifier);
> >
> >         list_del(&cpufreq_policy_list);
> > diff --git a/drivers/cpufreq/freq_table.c
> > b/drivers/cpufreq/freq_table.c index d7a7966..9c8e71e 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -34,6 +34,11 @@ int cpufreq_frequency_table_cpuinfo(struct
> > cpufreq_policy *policy,
> >
> >                         continue;
> >                 }
> > +               if (cpufreq_boost_supported())
> 
> Probably remove this check. Assume somebody while testing exynos,
> just sent boost_supported as false. Then you will not skip this
> frequency and may burn your chip :)

OK.

> 
> > +                       if (!cpufreq_boost_enabled()
> > +                           && table[i].index == CPUFREQ_BOOST_FREQ)
> > +                               continue;
> 
> This should be enough.

Let's only rely on the cpufreq_boost_enabled() test here.

> 
> >                 pr_debug("table entry %u: %u kHz, %u index\n",
> >                                         i, freq, table[i].index);
> >                 if (freq < min_freq)
> > @@ -171,7 +176,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,6 +192,9 @@ 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;
> 
> Add a comment here describing your complex logic.

OK.

> 
> > +               if (show_boost ^ (table[i].index ==
> > CPUFREQ_BOOST_FREQ))
> > +                       continue;
> > +
> >                 count += sprintf(&buf[count], "%d ",
> > table[i].frequency); }
> >         count += sprintf(&buf[count], "\n");
> > @@ -194,14 +203,34 @@ static ssize_t show_available_freqs(struct
> > cpufreq_policy *policy, char *buf)
> >
> >  }
> >
> > -struct freq_attr cpufreq_freq_attr_scaling_available_freqs = {
> > -       .attr = { .name = "scaling_available_frequencies",
> > -                 .mode = 0444,
> > -               },
> > -       .show = show_available_freqs,
> > -};
> > +#define cpufreq_attr_available_freq(_name)       \
> > +struct freq_attr cpufreq_freq_attr_##_name##_freqs =     \
> > +__ATTR_RO(_name##_frequencies)
> > +
> > +/**
> > + * show_scaling_available_frequencies - show normal boost
> > frequencies for
> 
> You missed this comment earlier. boost??

My mistake. This will be corrected.

> 
> > + * the specified CPU
> > + */
> > +static ssize_t scaling_available_frequencies_show(struct
> > cpufreq_policy *policy,
> > +                                                 char *buf)
> > +{
> > +       return show_available_freqs(policy, buf, 0);
> > +}
> > +cpufreq_attr_available_freq(scaling_available);
> >  EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_available_freqs);
> >
> > +/**
> > + * show_available_boost_freqs - show available boost frequencies
> > for
> > + * the specified CPU
> > + */
> > +static ssize_t scaling_boost_frequencies_show(struct
> > cpufreq_policy *policy,
> > +                                             char *buf)
> > +{
> > +       return show_available_freqs(policy, buf, 1);
> > +}
> > +cpufreq_attr_available_freq(scaling_boost);
> > +EXPORT_SYMBOL_GPL(cpufreq_freq_attr_scaling_boost_freqs);
> > +
> >  /*
> >   * 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 5348981..4783c4c 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -267,6 +267,10 @@ struct cpufreq_driver {
> >         int     (*suspend)      (struct cpufreq_policy *policy);
> >         int     (*resume)       (struct cpufreq_policy *policy);
> >         struct freq_attr        **attr;
> > +
> > +       /* platform specific boost support code */
> > +       bool                    boost_supported;
> > +       int (*enable_boost)    (int state);
> >  };
> >
> >  /* flags */
> > @@ -408,6 +412,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_FREQ    ~2
> > +
> >  struct cpufreq_frequency_table {
> >         unsigned int    index;     /* any */
> >         unsigned int    frequency; /* kHz - doesn't need to be in
> > ascending @@ -426,11 +433,15 @@ int
> > cpufreq_frequency_table_target(struct cpufreq_policy *policy,
> > unsigned int relation, unsigned int *index);
> >
> > +int cpufreq_boost_trigger_state(int state);
> 
> Why is this present here?

We had agreed to talk only about cpufreq :-).

This declaration will be removed.

-- 
Best regards,

Lukasz Majewski

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