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

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

[Alim Akhtar]

This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
required before accessing registers of these blocks.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/clk/samsung/clk-exynos7.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
index ad68d46..7013aa7 100644
--- a/drivers/clk/samsung/clk-exynos7.c
+++ b/drivers/clk/samsung/clk-exynos7.c
@@ -8,6 +8,7 @@
  *
 */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 
@@ -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);
 }
 
 CLK_OF_DECLARE(exynos7_clk_topc, "samsung,exynos7-clock-topc",
@@ -573,7 +578,11 @@ static struct samsung_cmu_info top1_cmu_info __initdata = {
 
 static void __init exynos7_clk_top1_init(struct device_node *np)
 {
+	struct clk *clk;
+
 	samsung_cmu_register_one(np, &top1_cmu_info);
+	clk = __clk_lookup("aclk_fsys0_200");
+	clk_prepare_enable(clk);
 }
 
 CLK_OF_DECLARE(exynos7_clk_top1, "samsung,exynos7-clock-top1",
-- 
1.7.10.4

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

[Alim Akhtar]

This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
required before accessing registers of these blocks.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/clk/samsung/clk-exynos7.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
index ad68d46..7013aa7 100644
--- a/drivers/clk/samsung/clk-exynos7.c
+++ b/drivers/clk/samsung/clk-exynos7.c
@@ -8,6 +8,7 @@
  *
 */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 
@@ -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);
 }
 
 CLK_OF_DECLARE(exynos7_clk_topc, "samsung,exynos7-clock-topc",
@@ -573,7 +578,11 @@ static struct samsung_cmu_info top1_cmu_info __initdata = {
 
 static void __init exynos7_clk_top1_init(struct device_node *np)
 {
+	struct clk *clk;
+
 	samsung_cmu_register_one(np, &top1_cmu_info);
+	clk = __clk_lookup("aclk_fsys0_200");
+	clk_prepare_enable(clk);
 }
 
 CLK_OF_DECLARE(exynos7_clk_top1, "samsung,exynos7-clock-top1",
-- 
1.7.10.4

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

[Sylwester Nawrocki]

On 04/12/2016 01:07 PM, Alim Akhtar wrote:
> This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
> required before accessing registers of these blocks.

Patch applied, thanks!

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

[Sylwester Nawrocki]

On 04/12/2016 01:07 PM, Alim Akhtar wrote:
> This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
> required before accessing registers of these blocks.

Patch applied, thanks!

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

[Tomasz Figa]

2016-04-12 14:07 GMT+03:00 Alim Akhtar <alim.akhtar@samsung.com>:
> This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
> required before accessing registers of these blocks.
>
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos7.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
> index ad68d46..7013aa7 100644
> --- a/drivers/clk/samsung/clk-exynos7.c
> +++ b/drivers/clk/samsung/clk-exynos7.c
> @@ -8,6 +8,7 @@
>   *
>  */
>
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/of.h>
>
> @@ -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.

Best regards,
Tomasz

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

[Tomasz Figa]

2016-04-12 14:07 GMT+03:00 Alim Akhtar <alim.akhtar@samsung.com>:
> This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
> required before accessing registers of these blocks.
>
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos7.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
> index ad68d46..7013aa7 100644
> --- a/drivers/clk/samsung/clk-exynos7.c
> +++ b/drivers/clk/samsung/clk-exynos7.c
> @@ -8,6 +8,7 @@
>   *
>  */
>
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/of.h>
>
> @@ -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.

Best regards,
Tomasz

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

[Sylwester Nawrocki]

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.

The CMU registration order is enforced by listing input clocks to each
CMU in DT. 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.

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? 

--
Regards, 
Sylwester

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

[Sylwester Nawrocki]

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.

The CMU registration order is enforced by listing input clocks to each
CMU in DT. 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.

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? 

--
Regards, 
Sylwester

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

[Tomasz Figa]

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

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

[Tomasz Figa]

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

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

[Alim Akhtar]

Hi

On 04/13/2016 05:01 PM, Tomasz Figa wrote:
> 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. :)
>

I was not aware of this flag, Thanks for pointing this out.
This flag indeed solves my problem.
These clocks should not be gated since the CMU block using this clock 
for SFR access.
Please ignore the $SUBJECT patch, I will send a new patch which uses
CLK_IS_CRITICAL flag instead.

>> 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
>
>

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

[Alim Akhtar]

Hi

On 04/13/2016 05:01 PM, Tomasz Figa wrote:
> 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. :)
>

I was not aware of this flag, Thanks for pointing this out.
This flag indeed solves my problem.
These clocks should not be gated since the CMU block using this clock 
for SFR access.
Please ignore the $SUBJECT patch, I will send a new patch which uses
CLK_IS_CRITICAL flag instead.

>> 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
>
>