[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Anson Huang]

For those clk gates which hold share count, since its is_enabled
callback is only checking the share count rather than reading
the hardware register setting, in the late phase of kernel bootup,
the clk_disable_unused action will NOT handle the scenario of
share_count is 0 but the hardware setting is enabled, actually,
uboot normally enables all clk gates, then those shared clk gates
will be always enabled until they are used by some modules.

So the problem would be: when kernel boot up, the usecount cat
from clk tree is 0, but the clk gates actually is enabled in
hardware register, it will confuse user and bring unnecessary power
consumption, take i.MX6SX for example, the ESAI clk info
is as below, the use count is 0, but the hardware register read
from CCM_CCGR1_CG8 is ungated.

cat /sys/kernel/debug/clk/clk_summary | grep esai

esai_sel            0            0   393216000          0 0
   esai_pred           0            0   196608000          0 0
      esai_podf           0            0    24576000          0 0
         esai_extal           0            0    24576000          0 0
      esai_mem           0            0   132000000          0 0
      esai_ipg           0            0   132000000          0 0

Read CCM_CCGR1:

0x020C406C:  F33FFF00

This patch disables all those clk gates which hold share count
before registering them, then the hardware status will match the
clk tree info.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 arch/arm/mach-imx/clk-imx6q.c  |    6 ++++++
 arch/arm/mach-imx/clk-imx6sl.c |    2 ++
 arch/arm/mach-imx/clk-imx6sx.c |    6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 5951660..fe53473 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -369,6 +369,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 
 	/*                                            name             parent_name          reg         shift */
 	clk[IMX6QDL_CLK_APBH_DMA]     = imx_clk_gate2("apbh_dma",      "usdhc3",            base + 0x68, 4);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x68) & (~(0x3 << 6)), base + 0x68);
 	clk[IMX6QDL_CLK_ASRC]         = imx_clk_gate2_shared("asrc",         "asrc_podf",   base + 0x68, 6, &share_count_asrc);
 	clk[IMX6QDL_CLK_ASRC_IPG]     = imx_clk_gate2_shared("asrc_ipg",     "ahb",         base + 0x68, 6, &share_count_asrc);
 	clk[IMX6QDL_CLK_ASRC_MEM]     = imx_clk_gate2_shared("asrc_mem",     "ahb",         base + 0x68, 6, &share_count_asrc);
@@ -385,6 +387,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	else
 		clk[IMX6Q_CLK_ECSPI5] = imx_clk_gate2("ecspi5",        "ecspi_root",        base + 0x6c, 8);
 	clk[IMX6QDL_CLK_ENET]         = imx_clk_gate2("enet",          "ipg",               base + 0x6c, 10);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x6c) & (~(0x3 << 16)), base + 0x6c);
 	clk[IMX6QDL_CLK_ESAI_EXTAL]   = imx_clk_gate2_shared("esai_extal",   "esai_podf",   base + 0x6c, 16, &share_count_esai);
 	clk[IMX6QDL_CLK_ESAI_IPG]     = imx_clk_gate2_shared("esai_ipg",   "ipg",           base + 0x6c, 16, &share_count_esai);
 	clk[IMX6QDL_CLK_ESAI_MEM]     = imx_clk_gate2_shared("esai_mem", "ahb",             base + 0x6c, 16, &share_count_esai);
@@ -443,6 +447,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_SDMA]         = imx_clk_gate2("sdma",          "ahb",               base + 0x7c, 6);
 	clk[IMX6QDL_CLK_SPBA]         = imx_clk_gate2("spba",          "ipg",               base + 0x7c, 12);
 	clk[IMX6QDL_CLK_SPDIF]        = imx_clk_gate2("spdif",         "spdif_podf",        base + 0x7c, 14);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x7c) & (~(0x3f << 18)), base + 0x7c);
 	clk[IMX6QDL_CLK_SSI1_IPG]     = imx_clk_gate2_shared("ssi1_ipg",      "ipg",        base + 0x7c, 18, &share_count_ssi1);
 	clk[IMX6QDL_CLK_SSI2_IPG]     = imx_clk_gate2_shared("ssi2_ipg",      "ipg",        base + 0x7c, 20, &share_count_ssi2);
 	clk[IMX6QDL_CLK_SSI3_IPG]     = imx_clk_gate2_shared("ssi3_ipg",      "ipg",        base + 0x7c, 22, &share_count_ssi3);
diff --git a/arch/arm/mach-imx/clk-imx6sl.c b/arch/arm/mach-imx/clk-imx6sl.c
index e982ebe..6af14a8 100644
--- a/arch/arm/mach-imx/clk-imx6sl.c
+++ b/arch/arm/mach-imx/clk-imx6sl.c
@@ -396,6 +396,8 @@ static void __init imx6sl_clocks_init(struct device_node *ccm_node)
 	clks[IMX6SL_CLK_SDMA]         = imx_clk_gate2("sdma",         "ipg",               base + 0x7c, 6);
 	clks[IMX6SL_CLK_SPBA]         = imx_clk_gate2("spba",         "ipg",               base + 0x7c, 12);
 	clks[IMX6SL_CLK_SPDIF]        = imx_clk_gate2("spdif",        "spdif0_podf",       base + 0x7c, 14);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x7c) & (~(0x3f << 18)), base + 0x7c);
 	clks[IMX6SL_CLK_SSI1_IPG]     = imx_clk_gate2_shared("ssi1_ipg",     "ipg",        base + 0x7c, 18, &share_count_ssi1);
 	clks[IMX6SL_CLK_SSI2_IPG]     = imx_clk_gate2_shared("ssi2_ipg",     "ipg",        base + 0x7c, 20, &share_count_ssi2);
 	clks[IMX6SL_CLK_SSI3_IPG]     = imx_clk_gate2_shared("ssi3_ipg",     "ipg",        base + 0x7c, 22, &share_count_ssi3);
diff --git a/arch/arm/mach-imx/clk-imx6sx.c b/arch/arm/mach-imx/clk-imx6sx.c
index 17354a1..e42052e 100644
--- a/arch/arm/mach-imx/clk-imx6sx.c
+++ b/arch/arm/mach-imx/clk-imx6sx.c
@@ -379,6 +379,8 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 	clks[IMX6SX_CLK_AIPS_TZ1]     = imx_clk_gate2("aips_tz1",      "ahb",               base + 0x68, 0);
 	clks[IMX6SX_CLK_AIPS_TZ2]     = imx_clk_gate2("aips_tz2",      "ahb",               base + 0x68, 2);
 	clks[IMX6SX_CLK_APBH_DMA]     = imx_clk_gate2("apbh_dma",      "usdhc3",            base + 0x68, 4);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x68) & (~(0x3 << 6)), base + 0x68);
 	clks[IMX6SX_CLK_ASRC_MEM]     = imx_clk_gate2_shared("asrc_mem", "ahb",             base + 0x68, 6, &share_count_asrc);
 	clks[IMX6SX_CLK_ASRC_IPG]     = imx_clk_gate2_shared("asrc_ipg", "ahb",             base + 0x68, 6, &share_count_asrc);
 	clks[IMX6SX_CLK_CAAM_MEM]     = imx_clk_gate2("caam_mem",      "ahb",               base + 0x68, 8);
@@ -400,6 +402,8 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 	clks[IMX6SX_CLK_ECSPI5]       = imx_clk_gate2("ecspi5",        "ecspi_podf",        base + 0x6c, 8);
 	clks[IMX6SX_CLK_EPIT1]        = imx_clk_gate2("epit1",         "perclk",            base + 0x6c, 12);
 	clks[IMX6SX_CLK_EPIT2]        = imx_clk_gate2("epit2",         "perclk",            base + 0x6c, 14);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x6c) & (~(0x3 << 16)), base + 0x6c);
 	clks[IMX6SX_CLK_ESAI_EXTAL]   = imx_clk_gate2_shared("esai_extal", "esai_podf",     base + 0x6c, 16, &share_count_esai);
 	clks[IMX6SX_CLK_ESAI_IPG]     = imx_clk_gate2_shared("esai_ipg",   "ahb",           base + 0x6c, 16, &share_count_esai);
 	clks[IMX6SX_CLK_ESAI_MEM]     = imx_clk_gate2_shared("esai_mem",   "ahb",           base + 0x6c, 16, &share_count_esai);
@@ -455,6 +459,8 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 	clks[IMX6SX_CLK_ROM]          = imx_clk_gate2("rom",           "ahb",               base + 0x7c, 0);
 	clks[IMX6SX_CLK_SDMA]         = imx_clk_gate2("sdma",          "ahb",               base + 0x7c, 6);
 	clks[IMX6SX_CLK_SPBA]         = imx_clk_gate2("spba",          "ipg",               base + 0x7c, 12);
+	/* make sure the initial clock gate setting of share_count is off */
+	writel_relaxed(readl_relaxed(base + 0x7c) & (~(0x3 << 14)) & (~(0x3f << 18)), base + 0x7c);
 	clks[IMX6SX_CLK_AUDIO]        = imx_clk_gate2_shared("audio",  "audio_podf",        base + 0x7c, 14, &share_count_audio);
 	clks[IMX6SX_CLK_SPDIF]        = imx_clk_gate2_shared("spdif",  "spdif_podf",        base + 0x7c, 14, &share_count_audio);
 	clks[IMX6SX_CLK_SSI1_IPG]     = imx_clk_gate2_shared("ssi1_ipg",      "ipg",        base + 0x7c, 18, &share_count_ssi1);
-- 
1.7.9.5

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Sascha Hauer]

Hi,

On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote:
> For those clk gates which hold share count, since its is_enabled
> callback is only checking the share count rather than reading
> the hardware register setting, in the late phase of kernel bootup,
> the clk_disable_unused action will NOT handle the scenario of
> share_count is 0 but the hardware setting is enabled, actually,
> uboot normally enables all clk gates, then those shared clk gates
> will be always enabled until they are used by some modules.
> 
> So the problem would be: when kernel boot up, the usecount cat
> from clk tree is 0, but the clk gates actually is enabled in
> hardware register, it will confuse user and bring unnecessary power
> consumption, take i.MX6SX for example, the ESAI clk info
> is as below, the use count is 0, but the hardware register read
> from CCM_CCGR1_CG8 is ungated.

I believe the problem is that the shared count is not increased
correctly during registration. When it would be increased during
registration for each clock that is enabled in hardware clock_disable_unused
could do its job like intended.

Could you test the attached patch?

-------------------------------8<--------------------------

>From 8095ee6e407f33887b2afa504767d00fcca4f10c Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 9 Dec 2014 09:39:06 +0100
Subject: [PATCH] ARM: i.MX: clk-gate2: Fix shared clock enable counting

When during registration of a shared clock the clock is enabled
in hardware the shared_count must be increased. This makes sure
that during clk_disable the shared_count is decreased first before
actually diabling the clock. When done like this the is_enabled
callback can return the hardware state of the clock, like it is
intended.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/clk-gate2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
index 5a75cdc..a673364 100644
--- a/arch/arm/mach-imx/clk-gate2.c
+++ b/arch/arm/mach-imx/clk-gate2.c
@@ -96,10 +96,7 @@ static int clk_gate2_is_enabled(struct clk_hw *hw)
 {
 	struct clk_gate2 *gate = to_clk_gate2(hw);
 
-	if (gate->share_count)
-		return !!__clk_get_enable_count(hw->clk);
-	else
-		return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx);
+	return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx);
 }
 
 static struct clk_ops clk_gate2_ops = {
@@ -129,6 +126,9 @@ struct clk *clk_register_gate2(struct device *dev, const char *name,
 	gate->lock = lock;
 	gate->share_count = share_count;
 
+	if (share_count && clk_gate2_reg_is_enabled(reg, bit_idx))
+		(*share_count)++;
+
 	init.name = name;
 	init.ops = &clk_gate2_ops;
 	init.flags = flags;
-- 
2.1.3


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Uwe Kleine-König]

Hallo Sascha,

On Tue, Dec 09, 2014 at 09:46:53AM +0100, Sascha Hauer wrote:
> On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote:
> > For those clk gates which hold share count, since its is_enabled
> > callback is only checking the share count rather than reading
> > the hardware register setting, in the late phase of kernel bootup,
> > the clk_disable_unused action will NOT handle the scenario of
> > share_count is 0 but the hardware setting is enabled, actually,
> > uboot normally enables all clk gates, then those shared clk gates
> > will be always enabled until they are used by some modules.
> > 
> > So the problem would be: when kernel boot up, the usecount cat
> > from clk tree is 0, but the clk gates actually is enabled in
> > hardware register, it will confuse user and bring unnecessary power
> > consumption, take i.MX6SX for example, the ESAI clk info
> > is as below, the use count is 0, but the hardware register read
> > from CCM_CCGR1_CG8 is ungated.
> 
> I believe the problem is that the shared count is not increased
> correctly during registration. When it would be increased during
> registration for each clock that is enabled in hardware clock_disable_unused
> could do its job like intended.
> 
> Could you test the attached patch?
> 
> -------------------------------8<--------------------------
> 
> From 8095ee6e407f33887b2afa504767d00fcca4f10c Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Tue, 9 Dec 2014 09:39:06 +0100
> Subject: [PATCH] ARM: i.MX: clk-gate2: Fix shared clock enable counting
> 
> When during registration of a shared clock the clock is enabled
> in hardware the shared_count must be increased. This makes sure
> that during clk_disable the shared_count is decreased first before
> actually diabling the clock. When done like this the is_enabled
> callback can return the hardware state of the clock, like it is
> intended.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-imx/clk-gate2.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
> index 5a75cdc..a673364 100644
> --- a/arch/arm/mach-imx/clk-gate2.c
> +++ b/arch/arm/mach-imx/clk-gate2.c
> @@ -96,10 +96,7 @@ static int clk_gate2_is_enabled(struct clk_hw *hw)
>  {
>  	struct clk_gate2 *gate = to_clk_gate2(hw);
>  
> -	if (gate->share_count)
> -		return !!__clk_get_enable_count(hw->clk);
> -	else
> -		return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx);
> +	return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx);
>  }
>  
>  static struct clk_ops clk_gate2_ops = {
> @@ -129,6 +126,9 @@ struct clk *clk_register_gate2(struct device *dev, const char *name,
>  	gate->lock = lock;
>  	gate->share_count = share_count;
>  
> +	if (share_count && clk_gate2_reg_is_enabled(reg, bit_idx))
> +		(*share_count)++;
> +
This is equivalent to calling clk_enable on this clk. So the result is
that clk_disable_unused now succeeds to disable if all related clks are
unused. But it introduces a new problem I think.

Consider that the bootloader starts linux with the clk on, so
assuming 3 shared clks share_count ends at 3 after the clocks are
registered. Then a driver makes use of one of them, calls
clk_prepare_enable on it, share_count becomes 4, enable_count = 1.
clk_disable_unused disables the two unused clks making share_count = 2.
After the driver now calls clk_disable the clk stays on. -> bad

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Uwe Kleine-König]

Hello Anson,

[Cc += Mike Turquette]

On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote:
> For those clk gates which hold share count, since its is_enabled
> callback is only checking the share count rather than reading
> the hardware register setting, in the late phase of kernel bootup,
> the clk_disable_unused action will NOT handle the scenario of
> share_count is 0 but the hardware setting is enabled, actually,
> uboot normally enables all clk gates, then those shared clk gates
s/uboot/U-Boot/ IIRC

> will be always enabled until they are used by some modules.
What is the purpose of the shared clocks? Just to have different names
for a single gate?

Wouldn't it be better to fix this problem at the framework level? Then
ideally clk_disable_unused would handle these shared clocks just fine.

I guess the problem is that this shared clk implementation doesn't
fulfill the expectations of the clk framework.

This is found in arch/arm/mach-imx/clk-imx6q.c
>  	clk[IMX6QDL_CLK_ASRC]         = imx_clk_gate2_shared("asrc",         "asrc_podf",   base + 0x68, 6, &share_count_asrc);
>  	clk[IMX6QDL_CLK_ASRC_IPG]     = imx_clk_gate2_shared("asrc_ipg",     "ahb",         base + 0x68, 6, &share_count_asrc);
>  	clk[IMX6QDL_CLK_ASRC_MEM]     = imx_clk_gate2_shared("asrc_mem",     "ahb",         base + 0x68, 6, &share_count_asrc);

For these clks is_enabled returns just enable_count instead of the
status of the corresponding bit in the corresponding register. The
reason for that probably is that "asrc" must not be disabled even if
unused if "asrc_ipg" is in use. So the problem is there are three struct
clk s with a common enable count for a single register bit. Interesting
enough these clocks have different parents (the 2nd parameter to
imx_clk_gate2_shared) which makes the easy solution to just make
asrc_ipg and asrc_mem children of asrc not applicable. Any idea for a
sane solution? A multi-parent-dummy-clk would do I think.

Other than that the following might work: Ignore share_count in the
is_enabled callback (i.e. report if the register bit is set as in the
share_count == NULL case) and implement clk->ops->disable_unused that
only unsets the bit if share_count == 0? Ugly but IMHO better and more
robust than the idea implemented in this patch. Still I'd prefer to make
the usual assumptions of the clk framework just work.

> So the problem would be: when kernel boot up, the usecount cat
> from clk tree is 0, but the clk gates actually is enabled in
> hardware register, it will confuse user and bring unnecessary power
> consumption, take i.MX6SX for example, the ESAI clk info
> is as below, the use count is 0, but the hardware register read
> from CCM_CCGR1_CG8 is ungated.
> 
> cat /sys/kernel/debug/clk/clk_summary | grep esai
Just a very minor nitpick: Unnecessary usage of cat,

	grep esai /sys/kernel/debug/clk/clk_summary

just produces the same result. :-)

> 
> esai_sel            0            0   393216000          0 0
>    esai_pred           0            0   196608000          0 0
>       esai_podf           0            0    24576000          0 0
>          esai_extal           0            0    24576000          0 0
>       esai_mem           0            0   132000000          0 0
>       esai_ipg           0            0   132000000          0 0
> 
> Read CCM_CCGR1:
> 
> 0x020C406C:  F33FFF00
This is hardly useful if you don't have the reference manual at hand.
I'd believe the commit log even without the examples.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Anson.Huang at freescale.com]

Hi, Uwe

Best regards!
Anson Huang


-----Original Message-----
From: Uwe Kleine-K?nig [mailto:u.kleine-koenig at pengutronix.de] 
Sent: 2014-12-09 4:58 PM
To: Huang Yongcai-B20788
Cc: shawn.guo at linaro.org; kernel at pengutronix.de; linux-arm-kernel at lists.infradead.org; Mike Turquette
Subject: Re: [PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

Hello Anson,

[Cc += Mike Turquette]

On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote:
> For those clk gates which hold share count, since its is_enabled 
> callback is only checking the share count rather than reading the 
> hardware register setting, in the late phase of kernel bootup, the 
> clk_disable_unused action will NOT handle the scenario of share_count 
> is 0 but the hardware setting is enabled, actually, uboot normally 
> enables all clk gates, then those shared clk gates
s/uboot/U-Boot/ IIRC

> will be always enabled until they are used by some modules.
What is the purpose of the shared clocks? Just to have different names for a single gate?
[Anson] The shared clocks is to meet the case that where one clk gate control multi clks path in module, for example, in esai module, there are three clk paths, esai_core, esai_ipg and esai_mem, but they share same clk gate in CCM_CCGR, and their parents are different...No very good hardware design for us.

Wouldn't it be better to fix this problem at the framework level? Then ideally clk_disable_unused would handle these shared clocks just fine.
[Anson] Agree, handle it in framework is much better than doing it in every platform, my bad:(

I guess the problem is that this shared clk implementation doesn't fulfill the expectations of the clk framework.

This is found in arch/arm/mach-imx/clk-imx6q.c
>  	clk[IMX6QDL_CLK_ASRC]         = imx_clk_gate2_shared("asrc",         "asrc_podf",   base + 0x68, 6, &share_count_asrc);
>  	clk[IMX6QDL_CLK_ASRC_IPG]     = imx_clk_gate2_shared("asrc_ipg",     "ahb",         base + 0x68, 6, &share_count_asrc);
>  	clk[IMX6QDL_CLK_ASRC_MEM]     = imx_clk_gate2_shared("asrc_mem",     "ahb",         base + 0x68, 6, &share_count_asrc);

For these clks is_enabled returns just enable_count instead of the status of the corresponding bit in the corresponding register. The reason for that probably is that "asrc" must not be disabled even if unused if "asrc_ipg" is in use. So the problem is there are three struct clk s with a common enable count for a single register bit. Interesting enough these clocks have different parents (the 2nd parameter to
imx_clk_gate2_shared) which makes the easy solution to just make asrc_ipg and asrc_mem children of asrc not applicable. Any idea for a sane solution? A multi-parent-dummy-clk would do I think.

Other than that the following might work: Ignore share_count in the is_enabled callback (i.e. report if the register bit is set as in the share_count == NULL case) and implement clk->ops->disable_unused that only unsets the bit if share_count == 0? Ugly but IMHO better and more robust than the idea implemented in this patch. Still I'd prefer to make the usual assumptions of the clk framework just work.
[Anson] I think this solution is feasible for us, as disable_unused is only used in late phase of kernel boot up, I think adding such callback is acceptable.

> So the problem would be: when kernel boot up, the usecount cat from 
> clk tree is 0, but the clk gates actually is enabled in hardware 
> register, it will confuse user and bring unnecessary power 
> consumption, take i.MX6SX for example, the ESAI clk info is as below, 
> the use count is 0, but the hardware register read from CCM_CCGR1_CG8 
> is ungated.
> 
> cat /sys/kernel/debug/clk/clk_summary | grep esai
Just a very minor nitpick: Unnecessary usage of cat,

	grep esai /sys/kernel/debug/clk/clk_summary

just produces the same result. :-)
[Anson] Thanks for sharing such knowledge:)

> 
> esai_sel            0            0   393216000          0 0
>    esai_pred           0            0   196608000          0 0
>       esai_podf           0            0    24576000          0 0
>          esai_extal           0            0    24576000          0 0
>       esai_mem           0            0   132000000          0 0
>       esai_ipg           0            0   132000000          0 0
> 
> Read CCM_CCGR1:
> 
> 0x020C406C:  F33FFF00
This is hardly useful if you don't have the reference manual at hand.
I'd believe the commit log even without the examples.
[Anson] Agreed, I will remove it in V2 patch.

Many thanks for reviewing my patch and very good advice:) Please help review V2 patch.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Uwe Kleine-König]

Hello,

On Wed, Dec 10, 2014 at 08:49:27AM +0000, Anson.Huang at freescale.com wrote:
> > 
> > esai_sel            0            0   393216000          0 0
> >    esai_pred           0            0   196608000          0 0
> >       esai_podf           0            0    24576000          0 0
> >          esai_extal           0            0    24576000          0 0
> >       esai_mem           0            0   132000000          0 0
> >       esai_ipg           0            0   132000000          0 0
> > 
> > Read CCM_CCGR1:
> > 
> > 0x020C406C:  F33FFF00
> This is hardly useful if you don't have the reference manual at hand.
> I'd believe the commit log even without the examples.
> [Anson] Agreed, I will remove it in V2 patch.
> 
> Many thanks for reviewing my patch and very good advice:) Please help review V2 patch.
>  
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 
If you want to do me (and most of the other readers of lakml I guess)
another favour: please fix your quoting style. The way you replied to my
mail is hard to parse in a console mail reader as you might be able to
see from the text I quoted above.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] arm: imx: correct the hardware clock gate setting for shared nodes

[Uwe Kleine-König]

Hello,

On Wed, Dec 10, 2014 at 05:11:26PM +0800, Anson Huang wrote:
> OK, very sorry for using OUTLOOK to reply such review mail, I will use console mail
> to reply it from now:)
Much better \o/. Note that Outlook can be tuned to behave better, too
(still not perfect AFAIK). Back when I was forced to use Outlook I liked
Outlook Quote Fix[1]. I hope this is not necessary any more today.

Best regards
Uwe

[1] http://home.in.tum.de/~jain/software/outlook-quotefix/

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |