lkml.org 
[lkml]   [2014]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    SubjectRe: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
    From
    On 30 May 2014 21:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
    > I believe the issue is this:
    >
    > We can't change the rate of pll_x while it's being used as a source for
    > something. I'm not 100% sure why. I assume the PLL output simply isn't
    > stable enough while changing rates; perhaps it can go out-of-range, or
    > generate glitches.
    >
    > This means we must switch the CPU clock source to something else (we use
    > pll_p) while changing the pll_x rate.
    >
    > Since the CPU is the only thing that uses pll_x, if we switch the CPU to
    > some other clock source, pll_x will become unused, so it will be
    > automatically disabled.
    >
    > Disabling the PLL, and then re-enabling it later when switching the CPU
    > back to it, presumably takes some extra time (e.g. waiting for PLL lock
    > when it starts back up), so the code takes an extra reference to the
    > clock (prepare_enable) before switching the CPU away from it, and drops
    > that (disable_unprepare) after switching the CPU back to it.
    >
    > The only case pll_x is disabled is when we use a cpufreq of 216MHz; that
    > frequency is provided by pll_p itself, so we never switch back to pll_x
    > in this case, and do want to shut down pll_x to save some power.
    >
    > Now, in your patch when switching from 216MHz to pll_x, the initial
    > prepare_enable(pll_x) never happens, then the CPU is switched back to
    > pll_x which turns it on, then the unpaired disable_unprepare(pll_x)
    > happens which turns off pll_x even though the CPU is using it as clock
    > source. Arguably the clock API has a bug in that it shouldn't allow
    > these unpaired calls to break the reference counting, but that's the way
    > the API is right now.

    Okay, that was very helpful..

    What about this ? (Attached for testing) :

    Author: Viresh Kumar <viresh.kumar@linaro.org>
    Date: Fri May 16 14:22:40 2014 +0530

    cpufreq: Tegra: implement intermediate frequency callbacks

    Tegra had always been switching to intermediate frequency (pll_p_clk) since
    ever. CPUFreq core has better support for handling notifications for these
    frequencies and so we can adapt Tegra's driver to it.

    Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we
    should have atleast restored to earlier frequency on error.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
    ---
    drivers/cpufreq/tegra-cpufreq.c | 97
    ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------
    1 file changed, 62 insertions(+), 35 deletions(-)

    diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
    index 6e774c6..25c145a 100644
    --- a/drivers/cpufreq/tegra-cpufreq.c
    +++ b/drivers/cpufreq/tegra-cpufreq.c
    @@ -45,46 +45,51 @@ static struct clk *cpu_clk;
    static struct clk *pll_x_clk;
    static struct clk *pll_p_clk;
    static struct clk *emc_clk;
    +static int pll_p_clk_count;

    -static int tegra_cpu_clk_set_rate(unsigned long rate)
    +static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
    + unsigned int index)
    +{
    + unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
    +
    + /*
    + * Don't switch to intermediate freq if:
    + * - we are already at it, i.e. policy->cur == ifreq
    + * - index corresponds to ifreq
    + */
    + if ((freq_table[index].frequency == ifreq) || (policy->cur == ifreq))
    + return 0;
    +
    + return ifreq;
    +}
    +
    +static int tegra_target_intermediate(struct cpufreq_policy *policy,
    + unsigned int index)
    {
    int ret;

    /*
    * Take an extra reference to the main pll so it doesn't turn
    - * off when we move the cpu off of it
    + * off when we move the cpu off of it as enabling it again while we
    + * switch to it from tegra_target() would take additional time. Though
    + * when target-freq is intermediate freq, we don't need to take this
    + * reference.
    */
    clk_prepare_enable(pll_x_clk);

    ret = clk_set_parent(cpu_clk, pll_p_clk);
    - if (ret) {
    - pr_err("Failed to switch cpu to clock pll_p\n");
    - goto out;
    - }
    -
    - if (rate == clk_get_rate(pll_p_clk))
    - goto out;
    -
    - ret = clk_set_rate(pll_x_clk, rate);
    - if (ret) {
    - pr_err("Failed to change pll_x to %lu\n", rate);
    - goto out;
    - }
    -
    - ret = clk_set_parent(cpu_clk, pll_x_clk);
    - if (ret) {
    - pr_err("Failed to switch cpu to clock pll_x\n");
    - goto out;
    - }
    + if (ret)
    + clk_disable_unprepare(pll_x_clk);
    + else
    + pll_p_clk_count++;

    -out:
    - clk_disable_unprepare(pll_x_clk);
    return ret;
    }

    static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
    {
    unsigned long rate = freq_table[index].frequency;
    + unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
    int ret = 0;

    /*
    @@ -98,10 +103,30 @@ static int tegra_target(struct cpufreq_policy
    *policy, unsigned int index)
    else
    clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */

    - ret = tegra_cpu_clk_set_rate(rate * 1000);
    + /*
    + * target freq == pll_p, don't need to take extra reference to pll_x_clk
    + * as it isn't used anymore.
    + */
    + if (rate == ifreq)
    + return clk_set_parent(cpu_clk, pll_p_clk);
    +
    + ret = clk_set_rate(pll_x_clk, rate * 1000);
    + /* Restore to earlier frequency on error, i.e. pll_x */
    if (ret)
    - pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
    - rate);
    + pr_err("Failed to change pll_x to %lu\n", rate);
    +
    + ret = clk_set_parent(cpu_clk, pll_x_clk);
    + /* This shouldn't fail while changing or restoring */
    + WARN_ON(ret);
    +
    + /*
    + * Drop count to pll_x clock only if we switched to intermediate freq
    + * earlier while transitioning to a target frequency.
    + */
    + if (pll_p_clk_count) {
    + clk_disable_unprepare(pll_x_clk);
    + pll_p_clk_count--;
    + }

    return ret;
    }
    @@ -137,16 +162,18 @@ static int tegra_cpu_exit(struct cpufreq_policy *policy)
    }

    static struct cpufreq_driver tegra_cpufreq_driver = {
    - .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
    - .verify = cpufreq_generic_frequency_table_verify,
    - .target_index = tegra_target,
    - .get = cpufreq_generic_get,
    - .init = tegra_cpu_init,
    - .exit = tegra_cpu_exit,
    - .name = "tegra",
    - .attr = cpufreq_generic_attr,
    + .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
    + .verify = cpufreq_generic_frequency_table_verify,
    + .get_intermediate = tegra_get_intermediate,
    + .target_intermediate = tegra_target_intermediate,
    + .target_index = tegra_target,
    + .get = cpufreq_generic_get,
    + .init = tegra_cpu_init,
    + .exit = tegra_cpu_exit,
    + .name = "tegra",
    + .attr = cpufreq_generic_attr,
    #ifdef CONFIG_PM
    - .suspend = cpufreq_generic_suspend,
    + .suspend = cpufreq_generic_suspend,
    #endif
    };
    From fb2d82a52d95bbd20e4eb75b3b79f74efa4c1a75 Mon Sep 17 00:00:00 2001
    Message-Id: <fb2d82a52d95bbd20e4eb75b3b79f74efa4c1a75.1401703573.git.viresh.kumar@linaro.org>
    From: Viresh Kumar <viresh.kumar@linaro.org>
    Date: Fri, 16 May 2014 14:22:40 +0530
    Subject: [PATCH] cpufreq: Tegra: implement intermediate frequency callbacks

    Tegra had always been switching to intermediate frequency (pll_p_clk) since
    ever. CPUFreq core has better support for handling notifications for these
    frequencies and so we can adapt Tegra's driver to it.

    Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we
    should have atleast restored to earlier frequency on error.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
    ---
    drivers/cpufreq/tegra-cpufreq.c | 97 ++++++++++++++++++++++++++---------------
    1 file changed, 62 insertions(+), 35 deletions(-)

    diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
    index 6e774c6..25c145a 100644
    --- a/drivers/cpufreq/tegra-cpufreq.c
    +++ b/drivers/cpufreq/tegra-cpufreq.c
    @@ -45,46 +45,51 @@ static struct clk *cpu_clk;
    static struct clk *pll_x_clk;
    static struct clk *pll_p_clk;
    static struct clk *emc_clk;
    +static int pll_p_clk_count;

    -static int tegra_cpu_clk_set_rate(unsigned long rate)
    +static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
    + unsigned int index)
    +{
    + unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
    +
    + /*
    + * Don't switch to intermediate freq if:
    + * - we are already at it, i.e. policy->cur == ifreq
    + * - index corresponds to ifreq
    + */
    + if ((freq_table[index].frequency == ifreq) || (policy->cur == ifreq))
    + return 0;
    +
    + return ifreq;
    +}
    +
    +static int tegra_target_intermediate(struct cpufreq_policy *policy,
    + unsigned int index)
    {
    int ret;

    /*
    * Take an extra reference to the main pll so it doesn't turn
    - * off when we move the cpu off of it
    + * off when we move the cpu off of it as enabling it again while we
    + * switch to it from tegra_target() would take additional time. Though
    + * when target-freq is intermediate freq, we don't need to take this
    + * reference.
    */
    clk_prepare_enable(pll_x_clk);

    ret = clk_set_parent(cpu_clk, pll_p_clk);
    - if (ret) {
    - pr_err("Failed to switch cpu to clock pll_p\n");
    - goto out;
    - }
    -
    - if (rate == clk_get_rate(pll_p_clk))
    - goto out;
    -
    - ret = clk_set_rate(pll_x_clk, rate);
    - if (ret) {
    - pr_err("Failed to change pll_x to %lu\n", rate);
    - goto out;
    - }
    -
    - ret = clk_set_parent(cpu_clk, pll_x_clk);
    - if (ret) {
    - pr_err("Failed to switch cpu to clock pll_x\n");
    - goto out;
    - }
    + if (ret)
    + clk_disable_unprepare(pll_x_clk);
    + else
    + pll_p_clk_count++;

    -out:
    - clk_disable_unprepare(pll_x_clk);
    return ret;
    }

    static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
    {
    unsigned long rate = freq_table[index].frequency;
    + unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
    int ret = 0;

    /*
    @@ -98,10 +103,30 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
    else
    clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */

    - ret = tegra_cpu_clk_set_rate(rate * 1000);
    + /*
    + * target freq == pll_p, don't need to take extra reference to pll_x_clk
    + * as it isn't used anymore.
    + */
    + if (rate == ifreq)
    + return clk_set_parent(cpu_clk, pll_p_clk);
    +
    + ret = clk_set_rate(pll_x_clk, rate * 1000);
    + /* Restore to earlier frequency on error, i.e. pll_x */
    if (ret)
    - pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
    - rate);
    + pr_err("Failed to change pll_x to %lu\n", rate);
    +
    + ret = clk_set_parent(cpu_clk, pll_x_clk);
    + /* This shouldn't fail while changing or restoring */
    + WARN_ON(ret);
    +
    + /*
    + * Drop count to pll_x clock only if we switched to intermediate freq
    + * earlier while transitioning to a target frequency.
    + */
    + if (pll_p_clk_count) {
    + clk_disable_unprepare(pll_x_clk);
    + pll_p_clk_count--;
    + }

    return ret;
    }
    @@ -137,16 +162,18 @@ static int tegra_cpu_exit(struct cpufreq_policy *policy)
    }

    static struct cpufreq_driver tegra_cpufreq_driver = {
    - .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
    - .verify = cpufreq_generic_frequency_table_verify,
    - .target_index = tegra_target,
    - .get = cpufreq_generic_get,
    - .init = tegra_cpu_init,
    - .exit = tegra_cpu_exit,
    - .name = "tegra",
    - .attr = cpufreq_generic_attr,
    + .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
    + .verify = cpufreq_generic_frequency_table_verify,
    + .get_intermediate = tegra_get_intermediate,
    + .target_intermediate = tegra_target_intermediate,
    + .target_index = tegra_target,
    + .get = cpufreq_generic_get,
    + .init = tegra_cpu_init,
    + .exit = tegra_cpu_exit,
    + .name = "tegra",
    + .attr = cpufreq_generic_attr,
    #ifdef CONFIG_PM
    - .suspend = cpufreq_generic_suspend,
    + .suspend = cpufreq_generic_suspend,
    #endif
    };

    --
    2.0.0.rc2
    \
     
     \ /
      Last update: 2014-06-02 12:41    [W:4.219 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site