Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

Viresh Kumar
2014-06-02T10:06:40Z

[-- Attachment #1: Type: text/plain, Size: 7435 bytes --]

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

[-- Attachment #2: 0001-cpufreq-Tegra-implement-intermediate-frequency-callb.patch --]
[-- Type: text/x-patch, Size: 4861 bytes --]

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