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