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

[Viresh Kumar]

On 30 May 2014 21:56, Stephen Warren <swarren@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
 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@xxxxxxxxxx>
From: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
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@xxxxxxxxxx>
---
 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