Re: [PATCH] clk: rockchip: rk3036: Add apll as the critical clock

[Xing Zheng <zhengxing@rock-chips.com>]

Hi Heiko,

On 2016年01月21日 17:21, Heiko Stuebner wrote:
> Hi Xing,
>
> Am Mittwoch, 20. Januar 2016, 16:37:17 schrieb Xing Zheng:
>> The apll may be closed if there are some child clock nodes below
>> it when the device startup. Therefore, the apll should be keep
>> critical.
>>
>> The apll tree like this:
>>      pll_apll
>>         apll
>>            armclk
>>               pclk_dbg
>>               aclk_core_pre
>>            aclk_hvec
>>            uart_pll_clk
>>               uart2_src
>>                  uart2_frac
>>               uart1_src
>>                  uart1_frac
>>               uart0_src
>>                  uart0_frac
> can you find out which of those clocks does cause your hang?
> Because things like the uart-clocks for example should be handled by their
> driver already, at the time the clk_disable_unused runs(). So I'd really
> like the critical clock to be the actually needed clock.
>
> Thanks
> Heiko

It looks like that we call the rockchip_rk3036_pll_disable cause the 
apll is diabled.
I think the diabled tracing like this:
1. All of uart0_frac~uart2_frac are branch_fraction_divider type, they 
have CLK_SET_RATE_UNGATE flag,
2. I enable cpufreq configs on the rk3036_defconfig, the default cpu 
freq is 600MHz durning loader, when startup it is 816MHz with default DTS.
Therefore, cpu freq will be change rate 600MHz to 816MHz then call 
clk_change_rate.
3. With the flag CLK_SET_RATE_UNGATE, triggering call clk_core_disable. 
In here, it will recursively close all of uart gates, finally, to call 
the root
parent diable callback that is rockchip_rk3036_pll_disable.

The disble log:
[ 1.074186] clk_change_rate -- CLK_SET_RATE_UNGATE name: uart2_frac, 
parent: uart2_src, core->flags = 0x00000424
[ 1.105722] clk_gate_endisable -- name: uart2_frac, parent: uart2_src, 
enable = 0
[ 1.110125] clk_gate_endisable -- name: uart2_src, parent: uart_pll_clk, 
enable = 0
[ 2.604445] rockchip_rk3036_pll_disable -- name: pll_apll, parent: xin24m

Therefore, I am considering uart_pll_clk hang onto gpll, or add it into 
the critical clock replace using apll...

If there are some mistake, please correct me. :-)

Thanks.

>
>
>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
>> ---
>>
>>   drivers/clk/rockchip/clk-rk3036.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3036.c
>> b/drivers/clk/rockchip/clk-rk3036.c index ebce980..483913b 100644
>> --- a/drivers/clk/rockchip/clk-rk3036.c
>> +++ b/drivers/clk/rockchip/clk-rk3036.c
>> @@ -425,6 +425,7 @@ static struct rockchip_clk_branch
>> rk3036_clk_branches[] __initdata = { };
>>
>>   static const char *const rk3036_critical_clocks[] __initconst = {
>> +	"apll",
>>   	"aclk_cpu",
>>   	"aclk_peri",
>>   	"hclk_peri",
>
>
>