lkml.org 
[lkml]   [2013]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 2/7] cpufreq: Add boost frequency support in core
    On Thu, 27 Jun 2013 14:32:57 +0530, Viresh Kumar wrote:
    > On 26 June 2013 18:24, Lukasz Majewski <l.majewski@samsung.com> wrote:
    > > 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:
    >
    > >> > +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.
    >
    > I said that as this will be more inclined towards the purpose of
    > this routine. This routine should store boost as show_boost()
    > is returning it. So, what would be better is if you just return
    > 0 or err from cpufreq_boost_trigger_state() and then set boost
    > here. This will also solve your problem where you revert back
    > to older boost value for failure cases.

    I thought about this idea, but at cpufreq_boost_trigger_state_sw()
    I iterate through all available policies and call
    cpufreq_frequency_table_cpuinfo()[*] on them. In this routine [*] I use
    cpufreq_boost_enabled() [**] route to search for maximal (boost)
    frequency.
    The [**] reads boost_enabled flag, which shall be updated before. When
    this search fails, then I restore the old value of boost_enabled.

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

    OK.

    >
    > >> > + else
    > >> > + ret = cpufreq_boost_trigger_state_sw();
    >
    > then why not enable_boost_sw() here? that would be more
    > relevant.

    Could you be more specific here?

    The distinction here is done on purpose:

    You can either call cpufreq_driver->enable_boost for HW controlled
    boost or cpufreq_boost_trigger_state_sw() for SW controlled one.

    I could write:
    if (cpufreq_driver->enable_boost)
    ret = cpufreq_driver->enable_boost(state);

    ret = cpufreq_boost_trigger_state_sw();

    But then for Intel CPUs I will iterate over its policies to seek for
    CPUFREQ_BOOST_FREQ marked frequencies without any purpose, since HW is
    taking care of boosting.

    >
    > > 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");
    >
    > So, you will not print error but current state? Probably
    > printing error is better.

    I will change it to:

    write_unlock_irqrestore(&cpufreq_driver_lock, flags);
    if (ret)
    pr_err("%s: BOOST cannot enable (%d)\n",
    __func__, ret);

    return ret;

    I want to avoid time consuming operations (like print) with holding
    lock (and boost_enabled shall be modified under lock).

    --
    Best regards,

    Lukasz Majewski

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


    \
     
     \ /
      Last update: 2013-06-27 12:41    [W:4.187 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site