Re: [RESEND PATCH v2 4/8] clk: rockchip: rk3399: fix incorrect GATE bits for {c, g}pll_aclk_perihp_src

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

[-- Attachment #1: Type: text/plain, Size: 4674 bytes --]

On 2016?08?02? 11:01, Xing Zheng wrote:
> On 2016?08?02? 04:20, Brian Norris wrote:
>> On Mon, Aug 01, 2016 at 05:53:39PM +0800, Xing Zheng wrote:
>>> Sorry to refer incorrect clock diagram, we double check it that the
>>> bits configuration of the Xpll_aclk_perihp_src need to be fixed:
>>> bit 1 - shows aclk_perihp_cpll_src_en
>>> bit 0 - shows aclk_perihp_gpll_src_en
>> Last time, you confirmed with the IC designers that we had things right.
>> What's different this time? Have you, for instance, done more thorough
>> testing, to show that the logical parent structure this driver outputs
>> is actually what the hardware is doing?
> I tried to proof it throught the testing that plug/unplug the USB 
> ethernet cable on kevin board.
>
> 1. the hclk_host0 and hclk_host1 are endpoint clocks.
> cpll --> G5[1] --> aclk_perihp_cpll_src --\                   |--> 
> hclk_host0
>                                                                 | --> 
> ... ---> |
> gpll --> G5[0] --> aclk_perihp_gpll_src --/                   |--> 
> hclk_host1
>
> 2. on the kevin, there is no clock below the cpll_aclk_perihp_src, and 
> the hclk_hostX are below the gpll_aclk_perihp_src:
>     pll_cpll                              1            1   
> 800000000          0 0
>        cpll                               7           19   
> 800000000          0 0
>           cpll_aclk_perihp_src            0            0   
> 800000000          0 0
>
> ...
>     pll_gpll                              1            1   
> 594000000          0 0
>        gpll                              10           10   
> 594000000          0 0
>           gpll_aclk_perihp_src            2            2   
> 594000000          0 0
>                 hclk_perihp               5            5    
> 74250000          0 0
>                    hclk_host1_arb           2            2    
> 74250000          0 0
>                    hclk_host1             2            2    
> 74250000          0 0
>                    hclk_host0_arb           2            2    
> 74250000          0 0
>                    hclk_host0             2            2    
> 74250000          0 0
>
> 3. by default, G5[0] and G5[1] are enabled:
> localhost ~ # mem r 0xff760314
> 0x000003e0
>
> 4. close the G5[1] (aclk_perihp_cpll_src), and plug/unplug USB 
> ethernet cable, the DUT still works well:
> localhost ~ # mem w 0xff760314 0xffff03e2
> localhost ~ # mem r 0xff760314
> 0x000003e2
> plug/unplug ok
>
> 5. close the G5[0] (aclk_perihp_gpll_src), , and plug/unplug USB 
> ethernet cable, the DUT will be crashed:
> localhost ~ # mem w 0xff760314 0xffff03e1
> localhost ~ # mem r 0xff760314
> 0x000003e1
> plug/unplug crashed
>
> Hence:
> bit 1 - shows aclk_perihp_cpll_src_en
> bit 0 - shows aclk_perihp_gpll_src_en
>
>> I think maybe this would qualify as:
>>
>> Fixes: 3bd14ae9da91 ("clk: rockchip: fix incorrect parent for rk3399's {c,g}pll_aclk_perihp_src")
>>
>> The above is not exactly a pure regression (we really want both
>> changes), but apparently it wasn't a complete fix.
> OK, I will add the commit message
> "Fixes: 3bd14ae9da91 ("clk: rockchip: fix incorrect parent for 
> rk3399's {c,g}pll_aclk_perihp_src")"
> at next version.
>>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
>>> ---
>>>
>>> Changes in v2: None
>> Uhh, isn't this patch brand new in v2? I don't think that means
>> "Changes ... None".
> Yes, I will add change log into this patch at next version.
>
> Thanks. 
Sorry to change my mind. This patch is only started to be added in 
patchset_v2, not be included in my patchset_v1, so I think it should be 
"Changes ... None".

Thanks.
>> Brian
>>
>>>   drivers/clk/rockchip/clk-rk3399.c |    4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
>>> index 2182391..8bf0d19 100644
>>> --- a/drivers/clk/rockchip/clk-rk3399.c
>>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>>> @@ -833,9 +833,9 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>>>
>>>   	/* perihp */
>>>   	GATE(0, "cpll_aclk_perihp_src", "cpll", CLK_IGNORE_UNUSED,
>>> -			RK3399_CLKGATE_CON(5), 0, GFLAGS),
>>> -	GATE(0, "gpll_aclk_perihp_src", "gpll", CLK_IGNORE_UNUSED,
>>>   			RK3399_CLKGATE_CON(5), 1, GFLAGS),
>>> +	GATE(0, "gpll_aclk_perihp_src", "gpll", CLK_IGNORE_UNUSED,
>>> +			RK3399_CLKGATE_CON(5), 0, GFLAGS),
>>>   	COMPOSITE(ACLK_PERIHP, "aclk_perihp", mux_aclk_perihp_p, CLK_IGNORE_UNUSED,
>>>   			RK3399_CLKSEL_CON(14), 7, 1, MFLAGS, 0, 5, DFLAGS,
>>>   			RK3399_CLKGATE_CON(5), 2, GFLAGS),
>>> -- 
>>> 1.7.9.5
>>>
>>>
>>
>
>
> -- 
> - Xing Zheng


-- 
- Xing Zheng


[-- Attachment #2: Type: text/html, Size: 11537 bytes --]