Re: [PATCH v8 10/18] clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent

From: Mikko Perttunen
Date: Mon Apr 13 2015 - 15:35:42 EST


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@xxxxxxxx> 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@xxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/