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

[Mikko Perttunen <mikko.perttunen@kapsi.fi>]

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/