Messages in this thread | | | Date | Mon, 13 Apr 2015 22:35:19 +0300 | From | Mikko Perttunen <> | Subject | Re: [PATCH v8 10/18] clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent |
| |
On 04/13/2015 10:31 PM, Michael Turquette wrote: > Quoting Tomeu Vizoso (2015-04-13 05:17:01) >> On 11 April 2015 at 13:00, Mikko Perttunen <mikko.perttunen@kapsi.fi> wrote: >>> On 04/11/2015 12:08 AM, Michael Turquette wrote: >>>> >>>> Quoting Mikko Perttunen (2015-03-01 04:44:33) >>>>> >>>>> This patch moves the initialization of PLL_X to be slightly before >>>>> that of CCLK_G. This ensures that at boot, CCLK_G will immediately >>>>> have a parent and the common clock framework can determine its >>>>> clock rate correctly. >>>>> >>>>> Without this patch, calling clk_put on CCLK_G could cause the CCF >>>>> to set its rate to zero, hanging the system. >>>> >>>> >>>> Hi Mikko, >>>> >>>> Patch looks fine to me but I wanted to get more info on the behavior you >>>> mentioned above about clk_put. Is there some special circumstance that >>>> causes this for you? Why does calling clk_put adjust the rate of your >>>> clock? >>>> >>>> Thanks, >>>> Mike >>> >>> >>> Hi Mike, >>> >>> this is the chain of events: >>> - CCLK_G is registered. CCF stores its current rate, but since it doesn't >>> have a parent at this point, the rate is assumed zero. >>> - tegra cpufreq driver tries to probe, and clk_gets CCLK_G >>> - tegra dfll driver tries to probe, but fails >>> - tegra cpufreq driver's probe fails, and during unwinding clk_puts CCLK_G >>> - CCF attempts to restore CCLK_G's rate to what it was prior to the clk_get >>> (to revert possible changes due to clock constraints) >> >> The CCF will currently only do so if any constraints were set in the >> per-user clk that was destroyed, so this particular problem shouldn't >> happen any more: ec02ace clk: Only recalculate the rate if needed > > Precisely. clk_put shouldn't be setting rates unless we're releasing a > constraint. Can this re-ordering patch be dropped?
Yes.
I'll try to post a (hopefully) final version of the series tomorrow.
Thanks, Mikko.
> >> >>> - the stored rate was zero, so CCLK_G is set to zero. >>> >>> We did discuss it a bit on IRC with Tomeu and Peter and agreed that some fix >>> in CCF should be done, but we didn't get much further than that. >> >> Wonder if we could somehow make sure that the rate in the CCF matches >> the current state of the HW. > > If there is no constraint expressed by Linux software then we > should not care. > > Probably we need to track a few more things in the framework. Stephen > and I were discussing struct clk.hw_en which tracks the enable/disable > state of the hardware independently of the enable_count (e.g. gate clock > is enabled by bootloader but not enabled by Linux ccf). > > Tracking something like "is_clk_initialized" would be helpful. It is an > abstract concept but knowing that clock has been successfully hooked up > to its parent and that its rate has been properly calculated would be > very helpful for some corner cases. > > Regards, > Mike > >> >> Regards, >> >> Tomeu >> >>> Mikko >>> >>> >>>> >>>>> >>>>> Signed-off-by: Mikko Perttunen <mikko.perttunen@kapsi.fi> >>>>> --- >>>>> v8: >>>>> - Added >>>>> >>>>> drivers/clk/tegra/clk-tegra-super-gen4.c | 46 >>>>> ++++++++++++++++++-------------- >>>>> 1 file changed, 26 insertions(+), 20 deletions(-) >>>>> >>>>> diff --git a/drivers/clk/tegra/clk-tegra-super-gen4.c >>>>> b/drivers/clk/tegra/clk-tegra-super-gen4.c >>>>> index f1f4410..c5ea9ee 100644 >>>>> --- a/drivers/clk/tegra/clk-tegra-super-gen4.c >>>>> +++ b/drivers/clk/tegra/clk-tegra-super-gen4.c >>>>> @@ -104,6 +104,32 @@ void __init tegra_super_clk_gen4_init(void __iomem >>>>> *clk_base, >>>>> struct clk *clk; >>>>> struct clk **dt_clk; >>>>> >>>>> + /* >>>>> + * Register PLL_X first so that CCLK_G has a parent at >>>>> registration >>>>> + * time. This ensures that the common clock framework knows >>>>> CCLK_G's >>>>> + * rate. >>>>> + */ >>>>> + >>>>> +#if defined(CONFIG_ARCH_TEGRA_114_SOC) || >>>>> defined(CONFIG_ARCH_TEGRA_124_SOC) >>>>> + /* PLLX */ >>>>> + dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks); >>>>> + if (!dt_clk) >>>>> + return; >>>>> + >>>>> + clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base, >>>>> + pmc_base, CLK_IGNORE_UNUSED, params, NULL); >>>>> + *dt_clk = clk; >>>>> + >>>>> + /* PLLX_OUT0 */ >>>>> + >>>>> + dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks); >>>>> + if (!dt_clk) >>>>> + return; >>>>> + clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x", >>>>> + CLK_SET_RATE_PARENT, 1, 2); >>>>> + *dt_clk = clk; >>>>> +#endif >>>>> + >>>>> /* CCLKG */ >>>>> dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_g, tegra_clks); >>>>> if (dt_clk) { >>>>> @@ -127,25 +153,5 @@ void __init tegra_super_clk_gen4_init(void __iomem >>>>> *clk_base, >>>>> } >>>>> >>>>> tegra_sclk_init(clk_base, tegra_clks); >>>>> - >>>>> -#if defined(CONFIG_ARCH_TEGRA_114_SOC) || >>>>> defined(CONFIG_ARCH_TEGRA_124_SOC) >>>>> - /* PLLX */ >>>>> - dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks); >>>>> - if (!dt_clk) >>>>> - return; >>>>> - >>>>> - clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base, >>>>> - pmc_base, CLK_IGNORE_UNUSED, params, NULL); >>>>> - *dt_clk = clk; >>>>> - >>>>> - /* PLLX_OUT0 */ >>>>> - >>>>> - dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks); >>>>> - if (!dt_clk) >>>>> - return; >>>>> - clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x", >>>>> - CLK_SET_RATE_PARENT, 1, 2); >>>>> - *dt_clk = clk; >>>>> -#endif >>>>> } >>>>> >>>>> -- >>>>> 2.3.0 >>>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> Please read the FAQ at http://www.tux.org/lkml/
| |