[PATCH v3 0/7] fix and optimize some clock configuration for the RK3399 platfom

[PATCH v3 0/7] fix and optimize some clock configuration for the RK3399 platfom

[Xing Zheng]

Hi:
  In the development work, we found that some of the previous
incorrect clock configuration on the RK3399 platform, we should
fix and optimize them.

Changes in v3:
- list more details of the testing steps
- add the regresson message "Fixes: 3bd14ae9da91 ..." to track the previous commit
- remove the patch "clk: rockchip: rk3399: fix incorrect parent for rk3399's {c, g}pll_aclk_perihp_src"
- add "Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>"

Changes in v2:
- add this patch " clk: rockchip: rk3399: fix incorrect GATE bits for {c, g}pll_aclk_perihp_src" into the patchset

Elaine Zhang (1):
  clk: rockchip: rk3399: delete the CLK_IGNORE_UNUSED for aclk_pcie

Xing Zheng (6):
  clk: rockchip: rk3399: export USBPHYx_480M_SRC clock IDs
  clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1
  clk: rockchip: rk3399: fix incorrect GATE bits for {c,
    g}pll_aclk_perihp_src
  clk: rockchip: rk3399: fix incorrect aclk_emmc source gate bits
  clk: rockchip: rk3399: add 65MHz and 106.5MHz clocks for HDMI
  clk: rockchip: rk3399: Add support frac mode frequencies

 drivers/clk/rockchip/clk-rk3399.c      |   39 ++++++++++++++++++++++++--------
 include/dt-bindings/clock/rk3399-cru.h |    2 ++
 2 files changed, 32 insertions(+), 9 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/7] clk: rockchip: rk3399: export USBPHYx_480M_SRC clock IDs

[Xing Zheng]

We export some clock IDs for the usb phy 480m source clocks.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

---

Changes in v3: None
Changes in v2: None

 include/dt-bindings/clock/rk3399-cru.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/dt-bindings/clock/rk3399-cru.h b/include/dt-bindings/clock/rk3399-cru.h
index 50a44cf..c4d8311 100644
--- a/include/dt-bindings/clock/rk3399-cru.h
+++ b/include/dt-bindings/clock/rk3399-cru.h
@@ -131,6 +131,8 @@
 #define SCLK_DPHY_RX0_CFG		165
 #define SCLK_RMII_SRC			166
 #define SCLK_PCIEPHY_REF100M		167
+#define SCLK_USBPHY0_480M_SRC		168
+#define SCLK_USBPHY1_480M_SRC		169
 
 #define DCLK_VOP0			180
 #define DCLK_VOP1			181
-- 
1.7.9.5

[PATCH v3 2/7] clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1

[Xing Zheng]

Export these source clocks for usbphy.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

---

Changes in v3: None
Changes in v2: None

 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 8059a8d..316f5f4 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -403,9 +403,9 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 	GATE(SCLK_USB2PHY1_REF, "clk_usb2phy1_ref", "xin24m", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(6), 6, GFLAGS),
 
-	GATE(0, "clk_usbphy0_480m_src", "clk_usbphy0_480m", CLK_IGNORE_UNUSED,
+	GATE(SCLK_USBPHY0_480M_SRC, "clk_usbphy0_480m_src", "clk_usbphy0_480m", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(13), 12, GFLAGS),
-	GATE(0, "clk_usbphy1_480m_src", "clk_usbphy1_480m", CLK_IGNORE_UNUSED,
+	GATE(SCLK_USBPHY1_480M_SRC, "clk_usbphy1_480m_src", "clk_usbphy1_480m", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(13), 12, GFLAGS),
 	MUX(0, "clk_usbphy_480m", mux_usbphy_480m_p, CLK_IGNORE_UNUSED,
 			RK3399_CLKSEL_CON(14), 6, 1, MFLAGS),
-- 
1.7.9.5

[PATCH v3 3/7] clk: rockchip: rk3399: fix incorrect GATE bits for {c, g}pll_aclk_perihp_src

[Xing Zheng]

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

Through the testing that plug/unplug the USB ethernet cable on the RK3399 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. 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, the work statue is 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, the DUT is crashed

Summary:
bit 1 - shows aclk_perihp_cpll_src_en
bit 0 - shows aclk_perihp_gpll_src_en

Fixes: 3bd14ae9da91 ("clk: rockchip: fix incorrect parent for rk3399's {c,g}pll_aclk_perihp_src")
Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

---

Changes in v3:
- list more details of the testing steps
- add the regresson message "Fixes: 3bd14ae9da91 ..." to track the previous commit
- remove the patch "clk: rockchip: rk3399: fix incorrect parent for rk3399's {c, g}pll_aclk_perihp_src"

Changes in v2:
- add this patch " clk: rockchip: rk3399: fix incorrect GATE bits for {c, g}pll_aclk_perihp_src" into the patchset

 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 316f5f4..f511de6 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

[PATCH v3 4/7] clk: rockchip: rk3399: fix incorrect aclk_emmc source gate bits

[Xing Zheng]

Dues to incorrect diagram, we need to fix incorrect bits for
(c/g)pll_aclk_emmc_src:
cpll_aclk_emmc_src --> G6[13]
gpll_aclk_emmc_src --> G6[12]

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v3:
- add "Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>"

Changes in v2: None

 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 f511de6..e5fdac0 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -923,9 +923,9 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(6), 14, GFLAGS),
 
 	GATE(0, "cpll_aclk_emmc_src", "cpll", CLK_IGNORE_UNUSED,
-			RK3399_CLKGATE_CON(6), 12, GFLAGS),
-	GATE(0, "gpll_aclk_emmc_src", "gpll", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(6), 13, GFLAGS),
+	GATE(0, "gpll_aclk_emmc_src", "gpll", CLK_IGNORE_UNUSED,
+			RK3399_CLKGATE_CON(6), 12, GFLAGS),
 	COMPOSITE_NOGATE(ACLK_EMMC, "aclk_emmc", mux_aclk_emmc_p, CLK_IGNORE_UNUSED,
 			RK3399_CLKSEL_CON(21), 7, 1, MFLAGS, 0, 5, DFLAGS),
 	GATE(ACLK_EMMC_CORE, "aclk_emmccore", "aclk_emmc", CLK_IGNORE_UNUSED,
-- 
1.7.9.5

[PATCH v3 5/7] clk: rockchip: rk3399: add 65MHz and 106.5MHz clocks for HDMI

[Xing Zheng]

We need to add more clocks for supporting more display resolution
for HDMI.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

Changes in v3: None
Changes in v2: None

 drivers/clk/rockchip/clk-rk3399.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index e5fdac0..8032eba 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -100,8 +100,10 @@ static struct rockchip_pll_rate_table rk3399_pll_rates[] = {
 	RK3036_PLL_RATE( 297000000, 1, 99, 4, 2, 1, 0),
 	RK3036_PLL_RATE( 216000000, 1, 72, 4, 2, 1, 0),
 	RK3036_PLL_RATE( 148500000, 1, 99, 4, 4, 1, 0),
+	RK3036_PLL_RATE( 106500000, 1, 71, 4, 4, 1, 0),
 	RK3036_PLL_RATE(  96000000, 1, 64, 4, 4, 1, 0),
 	RK3036_PLL_RATE(  74250000, 2, 99, 4, 4, 1, 0),
+	RK3036_PLL_RATE(  65000000, 1, 65, 6, 4, 1, 0),
 	RK3036_PLL_RATE(  54000000, 1, 54, 6, 4, 1, 0),
 	RK3036_PLL_RATE(  27000000, 1, 27, 6, 4, 1, 0),
 	{ /* sentinel */ },
-- 
1.7.9.5

[PATCH v3 6/7] clk: rockchip: rk3399: delete the CLK_IGNORE_UNUSED for aclk_pcie

[Xing Zheng]

From: Elaine Zhang <zhangqing@rock-chips.com>

allow aclk_pcie and aclk_perf_pcie disabled when unused.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

---

Changes in v3: None
Changes in v2: None

 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 8032eba..2792404 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -848,9 +848,9 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKSEL_CON(14), 12, 2, DFLAGS,
 			RK3399_CLKGATE_CON(5), 4, GFLAGS),
 
-	GATE(ACLK_PERF_PCIE, "aclk_perf_pcie", "aclk_perihp", CLK_IGNORE_UNUSED,
+	GATE(ACLK_PERF_PCIE, "aclk_perf_pcie", "aclk_perihp", 0,
 			RK3399_CLKGATE_CON(20), 2, GFLAGS),
-	GATE(ACLK_PCIE, "aclk_pcie", "aclk_perihp", CLK_IGNORE_UNUSED,
+	GATE(ACLK_PCIE, "aclk_pcie", "aclk_perihp", 0,
 			RK3399_CLKGATE_CON(20), 10, GFLAGS),
 	GATE(0, "aclk_perihp_noc", "aclk_perihp", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(20), 12, GFLAGS),
-- 
1.7.9.5

[PATCH v3 7/7] clk: rockchip: rk3399: Add support frac mode frequencies

[Xing Zheng]

We need to support various display resolutions for external
display devices like HDMI/DP, the frac mode can help us to
acquire almost any frequencies, and need higher VCOs to reduce
clock jitters.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

Changes in v3: None
Changes in v2: None

 drivers/clk/rockchip/clk-rk3399.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 2792404..ddd209b 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -109,6 +109,25 @@ static struct rockchip_pll_rate_table rk3399_pll_rates[] = {
 	{ /* sentinel */ },
 };
 
+static struct rockchip_pll_rate_table rk3399_pll_frates[] = {
+	/* _mhz, _refdiv, _fbdiv, _postdiv1, _postdiv2, _dsmpd, _frac */
+	RK3036_PLL_RATE( 594000000, 1, 123, 5, 1, 0, 12582912),  /* vco = 2970000000 */
+	RK3036_PLL_RATE( 593406593, 1, 123, 5, 1, 0, 10508804),  /* vco = 2967032965 */
+	RK3036_PLL_RATE( 297000000, 1, 123, 5, 2, 0, 12582912),  /* vco = 2970000000 */
+	RK3036_PLL_RATE( 296703297, 1, 123, 5, 2, 0, 10508807),  /* vco = 2967032970 */
+	RK3036_PLL_RATE( 148500000, 1, 129, 7, 3, 0, 15728640),  /* vco = 3118500000 */
+	RK3036_PLL_RATE( 148351648, 1, 123, 5, 4, 0, 10508800),  /* vco = 2967032960 */
+	RK3036_PLL_RATE( 106500000, 1, 124, 7, 4, 0,  4194304),  /* vco = 2982000000 */
+	RK3036_PLL_RATE(  74250000, 1, 129, 7, 6, 0, 15728640),  /* vco = 3118500000 */
+	RK3036_PLL_RATE(  74175824, 1, 129, 7, 6, 0, 13550823),  /* vco = 3115384608 */
+	RK3036_PLL_RATE(  65000000, 1, 113, 7, 6, 0, 12582912),  /* vco = 2730000000 */
+	RK3036_PLL_RATE(  59340659, 1, 121, 7, 7, 0,  2581098),  /* vco = 2907692291 */
+	RK3036_PLL_RATE(  54000000, 1, 110, 7, 7, 0,  4194304),  /* vco = 2646000000 */
+	RK3036_PLL_RATE(  27000000, 1,  55, 7, 7, 0,  2097152),  /* vco = 1323000000 */
+	RK3036_PLL_RATE(  26973027, 1,  55, 7, 7, 0,  1173232),  /* vco = 1321678323 */
+	{ /* sentinel */ },
+};
+
 /* CRU parents */
 PNAME(mux_pll_p)				= { "xin24m", "xin32k" };
 
@@ -229,7 +248,7 @@ static struct rockchip_pll_clock rk3399_pll_clks[] __initdata = {
 	[npll] = PLL(pll_rk3399, PLL_NPLL, "npll",  mux_pll_p, 0, RK3399_PLL_CON(40),
 		     RK3399_PLL_CON(43), 8, 31, ROCKCHIP_PLL_SYNC_RATE, rk3399_pll_rates),
 	[vpll] = PLL(pll_rk3399, PLL_VPLL, "vpll",  mux_pll_p, 0, RK3399_PLL_CON(48),
-		     RK3399_PLL_CON(51), 8, 31, ROCKCHIP_PLL_SYNC_RATE, rk3399_pll_rates),
+		     RK3399_PLL_CON(51), 8, 31, ROCKCHIP_PLL_SYNC_RATE, rk3399_pll_frates),
 };
 
 static struct rockchip_pll_clock rk3399_pmu_pll_clks[] __initdata = {
-- 
1.7.9.5

Re: [PATCH v3 5/7] clk: rockchip: rk3399: add 65MHz and 106.5MHz clocks for HDMI

[Heiko Stübner]

Am Dienstag, 2. August 2016, 15:22:26 schrieb Xing Zheng:
> We need to add more clocks for supporting more display resolution
> for HDMI.
> 
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

applied to my clock-branch for 4.9


Thanks
Heiko

Re: [PATCH v3 6/7] clk: rockchip: rk3399: delete the CLK_IGNORE_UNUSED for aclk_pcie

[Heiko Stübner]

Am Dienstag, 2. August 2016, 15:22:49 schrieb Xing Zheng:
> From: Elaine Zhang <zhangqing@rock-chips.com>
> 
> allow aclk_pcie and aclk_perf_pcie disabled when unused.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

applied to my clock-branch for 4.9


Thanks
Heiko

Re: [PATCH v3 2/7] clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1

[Heiko Stübner]

Hi Xing,

Am Dienstag, 2. August 2016, 15:19:56 schrieb Xing Zheng:
> Export these source clocks for usbphy.
> 
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

can you please provide a rationale why you need manual control over that 
intermediate clock?

The two usbphys seem to use the  clk_usb2phyX_ref clocks, generate the 480m 
clocks, but do not seem to need the clk_usbphyX_480m_src gates.

The clk_usbphyX_480m_src clocks on the other hand only lead to the 
clk_usbphy_480m mux, so I'd like some explanation on what you want to achieve 
here :-)


Thanks
Heiko

Re: [PATCH v3 7/7] clk: rockchip: rk3399: Add support frac mode frequencies

[Heiko Stübner]

Hi Xing,

Am Dienstag, 2. August 2016, 15:22:59 schrieb Xing Zheng:
> We need to support various display resolutions for external
> display devices like HDMI/DP, the frac mode can help us to
> acquire almost any frequencies, and need higher VCOs to reduce
> clock jitters.
> 
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

why does this need to be a separate rate array and cannot live in the general 
pll rate array?

The plls are general purpose, so we shouldn't limit them arbitarily.

I currently only see some frequencies (594MHz, 297MHz, 54MHz) that are present 
in both arrays but have different settings. As your patch description says 
that these settings reduce clock jitter, wouldn't the general frequencies also 
profit from merging these new values into the general rate array?


Heiko

Re: [PATCH v3 7/7] clk: rockchip: rk3399: Add support frac mode frequencies

[Xing Zheng]

Hi Heiko,

On 2016年08月05日 03:19, Heiko Stübner wrote:
> Hi Xing,
>
> Am Dienstag, 2. August 2016, 15:22:59 schrieb Xing Zheng:
>> We need to support various display resolutions for external
>> display devices like HDMI/DP, the frac mode can help us to
>> acquire almost any frequencies, and need higher VCOs to reduce
>> clock jitters.
>>
>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
> why does this need to be a separate rate array and cannot live in the general
> pll rate array?
>
> The plls are general purpose, so we shouldn't limit them arbitarily.
Yes, I understand your mean. :-)
>
> I currently only see some frequencies (594MHz, 297MHz, 54MHz) that are present
> in both arrays but have different settings. As your patch description says
> that these settings reduce clock jitter, wouldn't the general frequencies also
> profit from merging these new values into the general rate array?
>
>
and here are some of our ideas:

"WIth the frac mode and higher VCO to reduce clock jitters" that 
suggestion is from IC designer.
There are many and various kinds resolution and needed frequencies for 
external disaplay devices. For example, the DP needs:
3840x2160 533250KHz
3840x2160 297000KHz
3840x2160 296703KHz
2560x1440 241500KHz
1920x1080 148500KHz
1920x1080 148352KHz
1680x1050 146250KHz
1600x900 108000KHz
1280x1024 135000KHz
1280x1024 108000KHz
... and so on

There some frequencies must be allocated with frac mode. We separate 
these frequencies that are only used for display (VPLL) from the general 
rate table, and put them to be classified into a frac mode table, we can 
reduce the frequency of the query time, the two rate tables will not 
interfere with each other. Because other PLLs don't need to assgin these 
various frequencies with frac mode.

Thanks

-- 
- Xing Zheng

Re: [PATCH v3 2/7] clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1

[Frank Wang]

Hi Heiko,

On 2016/8/5 3:10, Heiko Stübner wrote:
> Hi Xing,
>
> Am Dienstag, 2. August 2016, 15:19:56 schrieb Xing Zheng:
>> Export these source clocks for usbphy.
>>
>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> can you please provide a rationale why you need manual control over that
> intermediate clock?

Well, From below graph, you can see that 'clk_usbphyX_480m' is generated 
from usb2phy, and 'clk_usbphy_480m' which select from 
clk_usbphyX_480m_src via a gate (G13[12])  provided 480M clock to other 
modules.

   xin24m
       |__ clk_usb2phy0_ref
       |                 |__ clk_usbphy0_480m
       |                                   |__clk_usbphy0_480m_src
       | |__clk_usbphy_480m
       |         |__ ... ...
       |__ clk_usb2phy1_ref
                          |__ clk_usbphy1_480m
                                          |__clk_usbphy1_480m_src

> The two usbphys seem to use the  clk_usb2phyX_ref clocks, generate the 480m
> clocks, but do not seem to need the clk_usbphyX_480m_src gates.

Yeah, they used to be. However, the story went something like this,

Some PM suspend process related ehci/ohci controller are base on 480m 
clocks, unfortunately, usb2-phy suspended earlier than ehci/ohci 
(usb2-phy will be auto suspended if no devices plug-in), and the 
clk-480m provided by it was disabled if no module used. As a result, the 
PM suspend process was blocked when it run into ehci/ohci module.

Hence, we are planing to refer clk_usbphyX_480m_src into each ehci/ohci 
driver. Maybe you will challenge why not refer clk_usbphy_480m directly? 
because there are two ehci/ohci connected in the different usb2phy, and 
only one clk_usbphy_480m clock was selected in clock tree.

BR.
Frank

> The clk_usbphyX_480m_src clocks on the other hand only lead to the
> clk_usbphy_480m mux, so I'd like some explanation on what you want to achieve
> here :-)
>
>
> Thanks
> Heiko
>

Re: [PATCH v3 7/7] clk: rockchip: rk3399: Add support frac mode frequencies

[Heiko Stübner]

Hi Xing,

Am Freitag, 5. August 2016, 10:26:57 schrieb Xing Zheng:
> On 2016年08月05日 03:19, Heiko Stübner wrote:
> > Am Dienstag, 2. August 2016, 15:22:59 schrieb Xing Zheng:
> >> We need to support various display resolutions for external
> >> display devices like HDMI/DP, the frac mode can help us to
> >> acquire almost any frequencies, and need higher VCOs to reduce
> >> clock jitters.
> >> 
> >> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
> > 
> > why does this need to be a separate rate array and cannot live in the
> > general pll rate array?
> > 
> > The plls are general purpose, so we shouldn't limit them arbitarily.
> 
> Yes, I understand your mean. :-)
> 
> > I currently only see some frequencies (594MHz, 297MHz, 54MHz) that are
> > present in both arrays but have different settings. As your patch
> > description says that these settings reduce clock jitter, wouldn't the
> > general frequencies also profit from merging these new values into the
> > general rate array?
> 
> and here are some of our ideas:
> 
> "WIth the frac mode and higher VCO to reduce clock jitters" that
> suggestion is from IC designer.
> There are many and various kinds resolution and needed frequencies for
> external disaplay devices. For example, the DP needs:
> 3840x2160 533250KHz
> 3840x2160 297000KHz
> 3840x2160 296703KHz
> 2560x1440 241500KHz
> 1920x1080 148500KHz
> 1920x1080 148352KHz
> 1680x1050 146250KHz
> 1600x900 108000KHz
> 1280x1024 135000KHz
> 1280x1024 108000KHz
> ... and so on
> 
> There some frequencies must be allocated with frac mode. We separate
> these frequencies that are only used for display (VPLL) from the general
> rate table, and put them to be classified into a frac mode table, we can
> reduce the frequency of the query time, the two rate tables will not
> interfere with each other. Because other PLLs don't need to assgin these
> various frequencies with frac mode.

Hmm, you're adding 14 frequencies to that new table (4 or so of them 
duplicating existing frequencies). So even if the effective number of new 
frequencies goes from now 10 to 20, I don't think walking that table will take 
an excessive time longer than now.

After the patch introducing the automatic rate calculation, the rate table we 
need to walk, will even get smaller.

Other components might also profit from the updated standard frequencies with 
less jitter you're introducing here.

And of course there is also the possibility somebody might want to build some 
rk3399 device without any graphics output at all [arm-server seem to be the 
new hype :-) ], so may want to use the vpll for something else completely.

So I still don't see an argument why it needs to be a separate table, as I 
currently don't see a case were it will really hurt the other PLLs.


Heiko

Re: [PATCH v3 7/7] clk: rockchip: rk3399: Add support frac mode frequencies

[Xing Zheng]

Hi Heiko,

On 2016年08月05日 16:48, Heiko Stübner wrote:
> Hi Xing,
>
> Am Freitag, 5. August 2016, 10:26:57 schrieb Xing Zheng:
>> On 2016年08月05日 03:19, Heiko Stübner wrote:
>>> Am Dienstag, 2. August 2016, 15:22:59 schrieb Xing Zheng:
>>>> We need to support various display resolutions for external
>>>> display devices like HDMI/DP, the frac mode can help us to
>>>> acquire almost any frequencies, and need higher VCOs to reduce
>>>> clock jitters.
>>>>
>>>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
>>> why does this need to be a separate rate array and cannot live in the
>>> general pll rate array?
>>>
>>> The plls are general purpose, so we shouldn't limit them arbitarily.
>> Yes, I understand your mean. :-)
>>
>>> I currently only see some frequencies (594MHz, 297MHz, 54MHz) that are
>>> present in both arrays but have different settings. As your patch
>>> description says that these settings reduce clock jitter, wouldn't the
>>> general frequencies also profit from merging these new values into the
>>> general rate array?
>> and here are some of our ideas:
>>
>> "WIth the frac mode and higher VCO to reduce clock jitters" that
>> suggestion is from IC designer.
>> There are many and various kinds resolution and needed frequencies for
>> external disaplay devices. For example, the DP needs:
>> 3840x2160 533250KHz
>> 3840x2160 297000KHz
>> 3840x2160 296703KHz
>> 2560x1440 241500KHz
>> 1920x1080 148500KHz
>> 1920x1080 148352KHz
>> 1680x1050 146250KHz
>> 1600x900 108000KHz
>> 1280x1024 135000KHz
>> 1280x1024 108000KHz
>> ... and so on
>>
>> There some frequencies must be allocated with frac mode. We separate
>> these frequencies that are only used for display (VPLL) from the general
>> rate table, and put them to be classified into a frac mode table, we can
>> reduce the frequency of the query time, the two rate tables will not
>> interfere with each other. Because other PLLs don't need to assgin these
>> various frequencies with frac mode.
> Hmm, you're adding 14 frequencies to that new table (4 or so of them
> duplicating existing frequencies). So even if the effective number of new
> frequencies goes from now 10 to 20, I don't think walking that table will take
> an excessive time longer than now.
>
> After the patch introducing the automatic rate calculation, the rate table we
> need to walk, will even get smaller.
>
> Other components might also profit from the updated standard frequencies with
> less jitter you're introducing here.
>
> And of course there is also the possibility somebody might want to build some
> rk3399 device without any graphics output at all [arm-server seem to be the
> new hype :-) ], so may want to use the vpll for something else completely.
>
> So I still don't see an argument why it needs to be a separate table, as I
> currently don't see a case were it will really hurt the other PLLs.
>
>
> Heiko
>
Yes, sorry to this idea is not comprehensive. I will try to find a 
better way.

Thanks for your comments. :-)

-- 
- Xing Zheng

Re: [PATCH v3 7/7] clk: rockchip: rk3399: Add support frac mode frequencies

[Heiko Stübner]

Am Freitag, 5. August 2016, 21:23:14 schrieb Xing Zheng:
> Hi Heiko,
> 
> On 2016年08月05日 16:48, Heiko Stübner wrote:
> > Hi Xing,
> > 
> > Am Freitag, 5. August 2016, 10:26:57 schrieb Xing Zheng:
> >> On 2016年08月05日 03:19, Heiko Stübner wrote:
> >>> Am Dienstag, 2. August 2016, 15:22:59 schrieb Xing Zheng:
> >>>> We need to support various display resolutions for external
> >>>> display devices like HDMI/DP, the frac mode can help us to
> >>>> acquire almost any frequencies, and need higher VCOs to reduce
> >>>> clock jitters.
> >>>> 
> >>>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
> >>> 
> >>> why does this need to be a separate rate array and cannot live in the
> >>> general pll rate array?
> >>> 
> >>> The plls are general purpose, so we shouldn't limit them arbitarily.
> >> 
> >> Yes, I understand your mean. :-)
> >> 
> >>> I currently only see some frequencies (594MHz, 297MHz, 54MHz) that are
> >>> present in both arrays but have different settings. As your patch
> >>> description says that these settings reduce clock jitter, wouldn't the
> >>> general frequencies also profit from merging these new values into the
> >>> general rate array?
> >> 
> >> and here are some of our ideas:
> >> 
> >> "WIth the frac mode and higher VCO to reduce clock jitters" that
> >> suggestion is from IC designer.
> >> There are many and various kinds resolution and needed frequencies for
> >> external disaplay devices. For example, the DP needs:
> >> 3840x2160 533250KHz
> >> 3840x2160 297000KHz
> >> 3840x2160 296703KHz
> >> 2560x1440 241500KHz
> >> 1920x1080 148500KHz
> >> 1920x1080 148352KHz
> >> 1680x1050 146250KHz
> >> 1600x900 108000KHz
> >> 1280x1024 135000KHz
> >> 1280x1024 108000KHz
> >> ... and so on
> >> 
> >> There some frequencies must be allocated with frac mode. We separate
> >> these frequencies that are only used for display (VPLL) from the general
> >> rate table, and put them to be classified into a frac mode table, we can
> >> reduce the frequency of the query time, the two rate tables will not
> >> interfere with each other. Because other PLLs don't need to assgin these
> >> various frequencies with frac mode.
> > 
> > Hmm, you're adding 14 frequencies to that new table (4 or so of them
> > duplicating existing frequencies). So even if the effective number of new
> > frequencies goes from now 10 to 20, I don't think walking that table will
> > take an excessive time longer than now.
> > 
> > After the patch introducing the automatic rate calculation, the rate table
> > we need to walk, will even get smaller.
> > 
> > Other components might also profit from the updated standard frequencies
> > with less jitter you're introducing here.
> > 
> > And of course there is also the possibility somebody might want to build
> > some rk3399 device without any graphics output at all [arm-server seem to
> > be the new hype :-) ], so may want to use the vpll for something else
> > completely.
> > 
> > So I still don't see an argument why it needs to be a separate table, as I
> > currently don't see a case were it will really hurt the other PLLs.
> > 
> > 
> > Heiko
> 
> Yes, sorry to this idea is not comprehensive. I will try to find a
> better way.
> 
> Thanks for your comments. :-)

as I said, to me just merging the new clock rates into the existing pll rate 
array looks like it should work just fine :-)

Re: [PATCH v3 2/7] clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1

[Heiko Stübner]

Hi Frank,

Am Freitag, 5. August 2016, 16:34:42 schrieb Frank Wang:
> On 2016/8/5 3:10, Heiko Stübner wrote:
> > Am Dienstag, 2. August 2016, 15:19:56 schrieb Xing Zheng:
> >> Export these source clocks for usbphy.
> >> 
> >> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> > 
> > can you please provide a rationale why you need manual control over that
> > intermediate clock?
> 
> Well, From below graph, you can see that 'clk_usbphyX_480m' is generated
> from usb2phy, and 'clk_usbphy_480m' which select from
> clk_usbphyX_480m_src via a gate (G13[12])  provided 480M clock to other
> modules.
> 
>    xin24m
> 
>        |__ clk_usb2phy0_ref
>        |
>        |                 |__ clk_usbphy0_480m
>        |                 |
>        |                                   |__clk_usbphy0_480m_src
>        | |
>        | |__clk_usbphy_480m
>        | |
>        |         |__ ... ...
>        |
>        |__ clk_usb2phy1_ref
>        |
>                           |__ clk_usbphy1_480m
>                           |
>                                           |__clk_usbphy1_480m_src
> > 
> > The two usbphys seem to use the  clk_usb2phyX_ref clocks, generate the
> > 480m
> > clocks, but do not seem to need the clk_usbphyX_480m_src gates.
> 
> Yeah, they used to be. However, the story went something like this,
> 
> Some PM suspend process related ehci/ohci controller are base on 480m
> clocks, unfortunately, usb2-phy suspended earlier than ehci/ohci
> (usb2-phy will be auto suspended if no devices plug-in), and the
> clk-480m provided by it was disabled if no module used. As a result, the
> PM suspend process was blocked when it run into ehci/ohci module.

ah, so the ehci controller needs that 480m clock as well? Do you happen to 
have example patches for the ehci/ohci side already? I'd like to peak at what 
you mean with "some PM suspend process related" things.

Depending on what is actually needed, you could also pull the usbphy out of 
autosuspend in a pm-prepare callback of the phy driver itself ... see 
http://lxr.free-electrons.com/source/include/linux/pm.h#L86

Like 
- in the .prepare callback make sure to unsuspend the phy
  and deactivate the autosuspend
- ehci/ohci will poweroff the phy in it s suspend callback (already does that)
- suspend -> resume
- ehci/ohci will poweron the phy
- in the phy's .complete callback you can reactivate the autosuspend timer

Because it looks more like you actually need the phy and not the clock alone.
So it would be nicer to use mechanisms already in place instead of creating 
new dependencies.


> Hence, we are planing to refer clk_usbphyX_480m_src into each ehci/ohci
> driver. Maybe you will challenge why not refer clk_usbphy_480m directly?
> because there are two ehci/ohci connected in the different usb2phy, and
> only one clk_usbphy_480m clock was selected in clock tree.

Nope, no argument from me as I fully understand that each phy provides its own 
480m clock :-) .


Heiko

Re: [PATCH v3 2/7] clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1

[Frank Wang]

Hi Heiko,

On 2016/8/6 0:05, Heiko Stübner wrote:
> Hi Frank,
>
> Am Freitag, 5. August 2016, 16:34:42 schrieb Frank Wang:
>> On 2016/8/5 3:10, Heiko Stübner wrote:
>>> Am Dienstag, 2. August 2016, 15:19:56 schrieb Xing Zheng:
>>>> Export these source clocks for usbphy.
>>>>
>>>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>>> can you please provide a rationale why you need manual control over that
>>> intermediate clock?
>> Well, From below graph, you can see that 'clk_usbphyX_480m' is generated
>> from usb2phy, and 'clk_usbphy_480m' which select from
>> clk_usbphyX_480m_src via a gate (G13[12])  provided 480M clock to other
>> modules.
>>
>>     xin24m
>>       |__ clk_usb2phy0_ref
>>       |     |
>>       |     |__ clk_usbphy0_480m
>>       |           |
>>       |           |__clk_usbphy0_480m_src
>>       |                |
>>       |                |__clk_usbphy_480m
>>       |                     |__ ... ...
>>       |
>>       |__ clk_usb2phy1_ref
>>             |
>>             |__ clk_usbphy1_480m
>>                   |
>>                   |__clk_usbphy1_480m_src
>>> The two usbphys seem to use the  clk_usb2phyX_ref clocks, generate the
>>> 480m
>>> clocks, but do not seem to need the clk_usbphyX_480m_src gates.
>> Yeah, they used to be. However, the story went something like this,
>>
>> Some PM suspend process related ehci/ohci controller are base on 480m
>> clocks, unfortunately, usb2-phy suspended earlier than ehci/ohci
>> (usb2-phy will be auto suspended if no devices plug-in), and the
>> clk-480m provided by it was disabled if no module used. As a result, the
>> PM suspend process was blocked when it run into ehci/ohci module.
> ah, so the ehci controller needs that 480m clock as well? Do you happen to
> have example patches for the ehci/ohci side already? I'd like to peak at what
> you mean with "some PM suspend process related" things.

Actually, no patches for it, I just make below steps manually :-).
1. set two usb2-phy into suspend mode.
2. disable 480m clock on each usb2-phy (assume only usb2-phy used it).
3. press power button let system into PM suspend.

Then, the kernel will be blocked and you can see the following log from 
console.
... ....
[  123.763848] calling  usb6+ @ 166, parent: xhci-hcd.0.auto
[  123.764503] call usb6+ returned 0 after 163 usecs
[  123.765106] calling  usb5+ @ 166, parent: xhci-hcd.0.auto
[  123.765719] call usb5+ returned 0 after 121 usecs
[  123.766294] calling  usb4+ @ 166, parent: fe3e0000.usb
[  123.766917] calling  usb3+ @ 55, parent: fe3a0000.usb

>
> Depending on what is actually needed, you could also pull the usbphy out of
> autosuspend in a pm-prepare callback of the phy driver itself ... see
> http://lxr.free-electrons.com/source/include/linux/pm.h#L86
>
> Like
> - in the .prepare callback make sure to unsuspend the phy
>    and deactivate the autosuspend
> - ehci/ohci will poweroff the phy in it s suspend callback (already does that)

Hmm, do you remember that we have previously discussed there are some 
oddities in ehci/ohci driver? phy_power_on() gets called twice at 
ehci/ohci driver probe time, one is at pdata->power_on(); another is at 
usb_add_hcd(), then the power_count of phy increases to 2,  but 
phy_power_off() is just invoked one time when ehci/ohci goes to PM 
suspend, so phy->ops->power_off is never be invoked.

In this way,  the usb-phy maybe never go to suspend.

> - suspend -> resume
> - ehci/ohci will poweron the phy
> - in the phy's .complete callback you can reactivate the autosuspend timer
>
> Because it looks more like you actually need the phy and not the clock alone.
> So it would be nicer to use mechanisms already in place instead of creating
> new dependencies.
>

Theoretically, phy_init() will be invoked when ehci/ohci power on, and 
the sm_work will be reactivated (have already implemented) in 
phy->ops->init, but unfortunately, the same issue as phy_power_on() 
mentioned above, it never run there too .

>> Hence, we are planing to refer clk_usbphyX_480m_src into each ehci/ohci
>> driver. Maybe you will challenge why not refer clk_usbphy_480m directly?
>> because there are two ehci/ohci connected in the different usb2phy, and
>> only one clk_usbphy_480m clock was selected in clock tree.
> Nope, no argument from me as I fully understand that each phy provides its own
> 480m clock :-) .
>
>
> Heiko
>
>

Re: [PATCH v3 4/7] clk: rockchip: rk3399: fix incorrect aclk_emmc source gate bits

[Heiko Stübner]

Am Dienstag, 2. August 2016, 15:19:58 schrieb Xing Zheng:
> Dues to incorrect diagram, we need to fix incorrect bits for
> (c/g)pll_aclk_emmc_src:
> cpll_aclk_emmc_src --> G6[13]
> gpll_aclk_emmc_src --> G6[12]
> 
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

applied to my clk-fixes branch for 4.8


Thanks
Heiko

Re: [PATCH v3 3/7] clk: rockchip: rk3399: fix incorrect GATE bits for {c, g}pll_aclk_perihp_src

[Heiko Stübner]

Am Dienstag, 2. August 2016, 15:19:57 schrieb Xing Zheng:
> 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
> 
> Through the testing that plug/unplug the USB ethernet cable on the RK3399
> 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. 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, the work statue is 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, the DUT is crashed
> 
> Summary:
> bit 1 - shows aclk_perihp_cpll_src_en
> bit 0 - shows aclk_perihp_gpll_src_en
> 
> Fixes: 3bd14ae9da91 ("clk: rockchip: fix incorrect parent for rk3399's
> {c,g}pll_aclk_perihp_src") Signed-off-by: Xing Zheng
> <zhengxing@rock-chips.com>

applied to my clk-fixes branch for 4.8


Thanks
Heiko

Re: [PATCH v3 2/7] clk: rockchip: rk3399: export 480M_SRC clock id for usbphy0/usbphy1

[Frank Wang]

Hi Heiko,

On 2016/8/8 17:55, Frank Wang wrote:
> Hi Heiko,
>
> On 2016/8/6 0:05, Heiko Stübner wrote:
>> Hi Frank,
>>
>> Am Freitag, 5. August 2016, 16:34:42 schrieb Frank Wang:
>>> On 2016/8/5 3:10, Heiko Stübner wrote:
>>>> Am Dienstag, 2. August 2016, 15:19:56 schrieb Xing Zheng:
>>>>> Export these source clocks for usbphy.
>>>>>
>>>>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>>>> can you please provide a rationale why you need manual control over 
>>>> that
>>>> intermediate clock?
>>> Well, From below graph, you can see that 'clk_usbphyX_480m' is 
>>> generated
>>> from usb2phy, and 'clk_usbphy_480m' which select from
>>> clk_usbphyX_480m_src via a gate (G13[12])  provided 480M clock to other
>>> modules.
>>>
>>>     xin24m
>>>       |__ clk_usb2phy0_ref
>>>       |     |
>>>       |     |__ clk_usbphy0_480m
>>>       |           |
>>>       |           |__clk_usbphy0_480m_src
>>>       |                |
>>>       |                |__clk_usbphy_480m
>>>       |                     |__ ... ...
>>>       |
>>>       |__ clk_usb2phy1_ref
>>>             |
>>>             |__ clk_usbphy1_480m
>>>                   |
>>>                   |__clk_usbphy1_480m_src
>>>> The two usbphys seem to use the clk_usb2phyX_ref clocks, generate the
>>>> 480m
>>>> clocks, but do not seem to need the clk_usbphyX_480m_src gates.
>>> Yeah, they used to be. However, the story went something like this,
>>>
>>> Some PM suspend process related ehci/ohci controller are base on 480m
>>> clocks, unfortunately, usb2-phy suspended earlier than ehci/ohci
>>> (usb2-phy will be auto suspended if no devices plug-in), and the
>>> clk-480m provided by it was disabled if no module used. As a result, 
>>> the
>>> PM suspend process was blocked when it run into ehci/ohci module.
>> ah, so the ehci controller needs that 480m clock as well? Do you 
>> happen to
>> have example patches for the ehci/ohci side already? I'd like to peak 
>> at what
>> you mean with "some PM suspend process related" things.
>
> Actually, no patches for it, I just make below steps manually :-).
> 1. set two usb2-phy into suspend mode.
> 2. disable 480m clock on each usb2-phy (assume only usb2-phy used it).
> 3. press power button let system into PM suspend.
>
> Then, the kernel will be blocked and you can see the following log 
> from console.
> ... ....
> [  123.763848] calling  usb6+ @ 166, parent: xhci-hcd.0.auto
> [  123.764503] call usb6+ returned 0 after 163 usecs
> [  123.765106] calling  usb5+ @ 166, parent: xhci-hcd.0.auto
> [  123.765719] call usb5+ returned 0 after 121 usecs
> [  123.766294] calling  usb4+ @ 166, parent: fe3e0000.usb
> [  123.766917] calling  usb3+ @ 55, parent: fe3a0000.usb
>

May I know have you tried reproducing on your board or not?

>>
>> Depending on what is actually needed, you could also pull the usbphy 
>> out of
>> autosuspend in a pm-prepare callback of the phy driver itself ... see
>> http://lxr.free-electrons.com/source/include/linux/pm.h#L86
>>
>> Like
>> - in the .prepare callback make sure to unsuspend the phy
>>    and deactivate the autosuspend
>> - ehci/ohci will poweroff the phy in it s suspend callback (already 
>> does that)
>
> Hmm, do you remember that we have previously discussed there are some 
> oddities in ehci/ohci driver? phy_power_on() gets called twice at 
> ehci/ohci driver probe time, one is at pdata->power_on(); another is 
> at usb_add_hcd(), then the power_count of phy increases to 2,  but 
> phy_power_off() is just invoked one time when ehci/ohci goes to PM 
> suspend, so phy->ops->power_off is never be invoked.
>
> In this way,  the usb-phy maybe never go to suspend.
>
>> - suspend -> resume
>> - ehci/ohci will poweron the phy
>> - in the phy's .complete callback you can reactivate the autosuspend 
>> timer
>>
>> Because it looks more like you actually need the phy and not the 
>> clock alone.
>> So it would be nicer to use mechanisms already in place instead of 
>> creating
>> new dependencies.
>>
>
> Theoretically, phy_init() will be invoked when ehci/ohci power on, and 
> the sm_work will be reactivated (have already implemented) in 
> phy->ops->init, but unfortunately, the same issue as phy_power_on() 
> mentioned above, it never run there too .
>

Would you like to give some comments on above two oddities please?

BR.
Frank

>>> Hence, we are planing to refer clk_usbphyX_480m_src into each ehci/ohci
>>> driver. Maybe you will challenge why not refer clk_usbphy_480m 
>>> directly?
>>> because there are two ehci/ohci connected in the different usb2phy, and
>>> only one clk_usbphy_480m clock was selected in clock tree.
>> Nope, no argument from me as I fully understand that each phy 
>> provides its own
>> 480m clock :-) .
>>
>>
>> Heiko
>>
>>