On 05/29/2014 07:56 PM, Viresh Kumar wrote:
On 29 May 2014 23:10, Stephen Warren swarren@wwwdotorg.org wrote:
This patch breaks Tegra. The reason is below.
...
+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..
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.