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.

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