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

[u.kleine-koenig@pengutronix.de (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/  |