| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKohpokxnyu5qk+Jb+LAfHsoFe0iFAg55k1DPN3PO7t+0obBkg@mail.gmail.com>
Date: Mon, 2 Jun 2014 15:36:37 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Stephen Warren <swarren@...dotorg.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Arvind Chauhan <arvind.chauhan@....com>,
Stephen Warren <swarren@...dia.com>,
Doug Anderson <dianders@...omium.org>,
Russell King - ARM Linux <linux@....linux.org.uk>,
Nicolas Pitre <nicolas.pitre@...aro.org>,
Thomas Abraham <thomas.abraham@...aro.org>,
Peter De Schrijver <pdeschrijver@...dia.com>
Subject: Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 30 May 2014 21:56, Stephen Warren <swarren@...dotorg.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@...aro.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@...aro.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
};
View attachment "0001-cpufreq-Tegra-implement-intermediate-frequency-callb.patch" of type "text/x-patch" (4861 bytes)
Powered by blists - more mailing lists