Re: [PATCH] clk: samsung: exynos7: Enable clocks for CMU_CCORE and CMU_FSYS0 blocks

From: Tomasz Figa <tomasz.figa@gmail.com>
To: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: "Alim Akhtar" <alim.akhtar@samsung.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	"Thomas Abraham" <thomas.ab@samsung.com>,
	"Krzysztof Kozłowski" <k.kozlowski@samsung.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"linux-samsung-soc@vger.kernel.org"
	<linux-samsung-soc@vger.kernel.org>,
	linux-clk@vger.kernel.org, "Stephen Boyd" <sboyd@codeaurora.org>,
	"Michael Turquette" <mturquette@baylibre.com>
Subject: Re: [PATCH] clk: samsung: exynos7: Enable clocks for CMU_CCORE and CMU_FSYS0 blocks
Date: Wed, 13 Apr 2016 14:31:31 +0300	[thread overview]
Message-ID: <CA+Ln22Gf+aXnOzj-qYeNq5-MO8b+tjxrp0KPtG7Q2ceStk4ERA@mail.gmail.com> (raw)
In-Reply-To: <570E2123.1000107@samsung.com>

2016-04-13 13:36 GMT+03:00 Sylwester Nawrocki <s.nawrocki@samsung.com>:
> On 04/13/2016 08:26 AM, Tomasz Figa wrote:
>>> > @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = {
>>> >
>>> >  static void __init exynos7_clk_topc_init(struct device_node *np)
>>> >  {
>>> > +       struct clk *clk;
>>> > +
>>> >         samsung_cmu_register_one(np, &topc_cmu_info);
>>> > +       clk = __clk_lookup("aclk_ccore_133");
>>> > +       clk_prepare_enable(clk);
>>
>> Shouldn't this be rather done before calling
>> samsung_cmu_register_one()? I don't remember exactly, but wouldn't
>> clock registration trigger reading back current (mux, div) values from
>> registers?
>>
>> Also, do we have any guarantees on order of initialization of
>> particular CMUs? I believe this will happen in order of DT nodes and
>> so would be not any kind of guarantee at all.
>
> If these clocks need to be kept enabled perhaps it's better to just set
> CLK_IS_CRITICAL flag for them? Patches adding this flag are already in
> clk-next branch in the clk git tree. This way the clocks would get
> enabled within the clk_register() call.
>

Oh, I didn't know about this. Sounds like a good idea. :)

> The CMU registration order is enforced by listing input clocks to each
> CMU in DT.

Ah, right, I recall now, I even reviewed relevant patch. Thanks for
refreshing my memory.

> However, I wouldn't be concerned much about it in context
> of this patch. We are enabling here clocks which belong to same CMU.
> samsung_cmu_register_one() needs to be called first for subsequent
> __clk_lookup() calls to work.

I thought a clock from another CMU has to be enabled for the CPU to be
able to access the CMU being registered.

>
> Perhaps related bits need to be set manually in CMU registers before
> registering a clock provider for the CMU, to fulfil the requirements.
>
> Anyway, summary of the $subject patch seems not precise enough:
>
> "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
> required before accessing registers of these blocks."
>
> We need to enable selected clocks (i.e. access the CMU's registers)
> before accessing this CMU's registers?

Yeah, maybe the explanation could be a bit more precise.

Best regards,
Tomasz