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
| ||
|
Date: Fri, 30 May 2014 07:26:55 +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 29 May 2014 23:10, Stephen Warren <swarren@...dotorg.org> wrote: > This patch breaks Tegra. The reason is below. Lets see what blunder I made :) >> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c > >> -static int tegra_cpu_clk_set_rate(unsigned long rate) >> +static unsigned int >> +tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index) > > (BTW, can we please not put the return type on a separate line; it's > inconsistent with the rest of the code in this file) Sure. >> +{ >> + 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; > > If policy->cur == ifreq here, then tegra_target_intermediate() isn't > called by the cpufreq core, so ... > >> +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 >> */ >> clk_prepare_enable(pll_x_clk); > > ... that reference isn't added... > >> @@ -98,10 +96,23 @@ 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 */ >> + if (rate * 1000 == clk_get_rate(pll_p_clk)) { >> + ret = tegra_target_intermediate(policy, index); >> + goto disable_pll_x; >> + } > > ... and this code doesn't call it either, since we could be switching > from the pll_p rate to something faster ... > >> + >> + 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); >> + >> +disable_pll_x: >> + clk_disable_unprepare(pll_x_clk); > > ... so this turns off pll_x even though we're running from it. Can you describe the role of the enable/disable of this pll_x_clk please? Which all clocks depend on it, etc? So that I understand why its important to enable it and for which clocks. And also if we need to disable it after changing to any freq.. > It would be simpler if Tegra *always* used an intermediate frequency, > and hence the core *always* called tegra_target_intermediate(). > Admittedly, this would result in tegra_target() sometimes (when > switching CPU clock rate to the pll_p rate) doing nothing other than > removing the extra reference on pll_x, but I think that the code would > be simpler to follow and more robust. Ok, will check what suits best. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists