| 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: Mon, 13 Apr 2015 22:35:19 +0300
From: Mikko Perttunen <mikko.perttunen@...si.fi>
To: Michael Turquette <mturquette@...aro.org>,
Tomeu Vizoso <tomeu.vizoso@...labora.com>
CC: Stephen Warren <swarren@...dotorg.org>,
Thierry Reding <thierry.reding@...il.com>,
Alexandre Courbot <gnurou@...il.com>,
Peter De Schrijver <pdeschrijver@...dia.com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Viresh Kumar <viresh.kumar@...aro.org>, pwalmsley@...dia.com,
Vince Hsu <vinceh@...dia.com>,
Prashant Gaikwad <pgaikwad@...dia.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, tuomas.tynkkynen@....fi
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@...si.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@...si.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@...r.kernel.org
>>> 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@...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