linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
@ 2016-06-12  9:48 Xing Zheng
  2016-06-12 10:46 ` Yakir Yang
  2016-06-13 22:46 ` Heiko Stübner
  0 siblings, 2 replies; 8+ messages in thread
From: Xing Zheng @ 2016-06-12  9:48 UTC (permalink / raw)
  To: heiko
  Cc: dianders, elaine.zhang, huangtao, briannorris, ykk, Xing Zheng,
	Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel,
	linux-rockchip, linux-kernel

The functions and features VOP0 more complete than VOP1's, we need to
use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
screen.

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

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

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 7ecb12c3..6affa75 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -1157,7 +1157,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 	GATE(HCLK_VOP0_NOC, "hclk_vop0_noc", "hclk_vop0_pre", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(28), 0, GFLAGS),
 
-	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, 0,
+	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, CLK_SET_RATE_PARENT,
 			RK3399_CLKSEL_CON(49), 8, 2, MFLAGS, 0, 8, DFLAGS,
 			RK3399_CLKGATE_CON(10), 12, GFLAGS),
 
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-12  9:48 [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399 Xing Zheng
@ 2016-06-12 10:46 ` Yakir Yang
  2016-06-13 18:37   ` Brian Norris
  2016-06-13 22:46 ` Heiko Stübner
  1 sibling, 1 reply; 8+ messages in thread
From: Yakir Yang @ 2016-06-12 10:46 UTC (permalink / raw)
  To: Xing Zheng, heiko
  Cc: dianders, elaine.zhang, huangtao, briannorris, Michael Turquette,
	Stephen Boyd, linux-clk, linux-arm-kernel, linux-rockchip,
	linux-kernel

Xing,

On 06/12/2016 05:48 PM, Xing Zheng wrote:
> The functions and features VOP0 more complete than VOP1's, we need to
> use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
> screen.
>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>

Tested on RK3399 Kevin board, after apply this patch, my eDP panel light 
up normally. So

Tested-by: Yakir Yang <ykk@rock-chips.com>

BR,
- Yakir

> ---
>
>   drivers/clk/rockchip/clk-rk3399.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index 7ecb12c3..6affa75 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -1157,7 +1157,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>   	GATE(HCLK_VOP0_NOC, "hclk_vop0_noc", "hclk_vop0_pre", CLK_IGNORE_UNUSED,
>   			RK3399_CLKGATE_CON(28), 0, GFLAGS),
>   
> -	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, 0,
> +	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, CLK_SET_RATE_PARENT,
>   			RK3399_CLKSEL_CON(49), 8, 2, MFLAGS, 0, 8, DFLAGS,
>   			RK3399_CLKGATE_CON(10), 12, GFLAGS),
>   

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-12 10:46 ` Yakir Yang
@ 2016-06-13 18:37   ` Brian Norris
  2016-06-13 20:43     ` Doug Anderson
  0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2016-06-13 18:37 UTC (permalink / raw)
  To: Yakir Yang
  Cc: Xing Zheng, heiko, dianders, elaine.zhang, huangtao,
	Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel,
	linux-rockchip, linux-kernel

Hi,

On Sun, Jun 12, 2016 at 06:46:51PM +0800, Yakir Yang wrote:
> On 06/12/2016 05:48 PM, Xing Zheng wrote:
> >The functions and features VOP0 more complete than VOP1's, we need to
> >use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
> >screen.

Personally, I'd like a little better description that talks about the
rates, not just the differences between VOP0 and VOP1. Presumably the
clock rates needed by VOP0 are not achievable just by these dividers, so
we need to adjust the PLL?

FWIW, I haven't actually found this patch necessary in my own testing (I
have eDP running fine without this change), but perhaps with better
justification, this will make more sense.

> >Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> 
> Tested on RK3399 Kevin board, after apply this patch, my eDP panel
> light up normally. So
> 
> Tested-by: Yakir Yang <ykk@rock-chips.com>

For clarification: this patch isn't sufficient for that, right? Of
course eDP support hasn't fully landed upstream yet, but I presume you
have other changes to set the correct VOP frequencies?

Brian

> BR,
> - Yakir
> 
> >---
> >
> >  drivers/clk/rockchip/clk-rk3399.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> >index 7ecb12c3..6affa75 100644
> >--- a/drivers/clk/rockchip/clk-rk3399.c
> >+++ b/drivers/clk/rockchip/clk-rk3399.c
> >@@ -1157,7 +1157,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
> >  	GATE(HCLK_VOP0_NOC, "hclk_vop0_noc", "hclk_vop0_pre", CLK_IGNORE_UNUSED,
> >  			RK3399_CLKGATE_CON(28), 0, GFLAGS),
> >-	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, 0,
> >+	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, CLK_SET_RATE_PARENT,
> >  			RK3399_CLKSEL_CON(49), 8, 2, MFLAGS, 0, 8, DFLAGS,
> >  			RK3399_CLKGATE_CON(10), 12, GFLAGS),
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-13 18:37   ` Brian Norris
@ 2016-06-13 20:43     ` Doug Anderson
  0 siblings, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2016-06-13 20:43 UTC (permalink / raw)
  To: Brian Norris
  Cc: Yakir Yang, Xing Zheng, Heiko Stübner, elaine zhang,
	Tao Huang, Michael Turquette, Stephen Boyd, linux-clk,
	linux-arm-kernel, open list:ARM/Rockchip SoC...,
	linux-kernel, Tomeu Vizoso, Chris, Tomasz Figa,
	Stéphane Marchesin, Dominik Behr

Hi,

On Mon, Jun 13, 2016 at 11:37 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi,
>
> On Sun, Jun 12, 2016 at 06:46:51PM +0800, Yakir Yang wrote:
>> On 06/12/2016 05:48 PM, Xing Zheng wrote:
>> >The functions and features VOP0 more complete than VOP1's, we need to
>> >use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
>> >screen.
>
> Personally, I'd like a little better description that talks about the
> rates, not just the differences between VOP0 and VOP1. Presumably the
> clock rates needed by VOP0 are not achievable just by these dividers, so
> we need to adjust the PLL?

The idea is that there is a "big" VOP (vop0) and a "little" VOP
(vop1).  The "big" VOP can support higher resolutions and more output
formats but draws a little more power.  The "little" VOP supports
lower resolutions and a more limited set of formats.  If you're
curious, chapter 1 of the rk3399 TRM has a summary of the VOP features
(big and little).

In general, I think the SoC allows dynamic assignment of the VOPs to
the various video devices (eDP, DP, MIPI, HDMI).  So you can output to
two places at once and you get to pick whichever VOP you want for each
output.

The VOPs have three PLL sources: VPLL, CPLL, and GPLL.  Those PLLs are
best described as:
* CPLL - The PLL that runs at 800MHz.
* GPLL - The PLL that runs at ~600MHz (actually 594 MHz).
* VPLL - The PLL that can change rates to make various pixels clocks.

Presumably:
* The little VOP has enough features that you'd want to use it for
most internal laptop / cellphone / tablet panels.
* The big VOP is a good choice for whatever external graphics
connector you have so you can support the widest range of devices.

So if you've got a laptop that happens to have an internal panel and
an external connector, presumably:

* You want to adjust your display timings (hblank, vblank, etc) to
make sure that the internal display can be driven by dividing 800 MHz
or 594 MHz by some integral amount.  As an example, for the Starry
panel I posted recently <https://patchwork.kernel.org/patch/9170139/>
you could make exactly 148.5 (594 / 4) by subtracting 4 from the
horizontal total and adding 15 to the vertical: 1250 * 1980 * 60 Hz =
148.5 MHz

* You want to make sure that the internal display gets assigned the
little VOP so save power / leave flexibility for the external
connector.

* You want to make sure that that the little VOP _doesn't_
accidentally get assigned VPLL even if (at boot) VPLL happens to be at
a rate that would be fine for the panel.  If you accidentally use VPLL
as a parent then you'll have a tougher time changing VPLL later when
an external display is plugged in.


NOTE: If you have things other than a laptop the decisions between
VOP0 and VOP1 get much tougher.


> FWIW, I haven't actually found this patch necessary in my own testing (I
> have eDP running fine without this change), but perhaps with better
> justification, this will make more sense.

It is probable that firmware has already set the PLL up.  It would be
interested to hack your firmware to turn off the display and see if
your behavior changes.  Alternatively, try adding something like this
to hack the VOPs:

--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -816,7 +816,7 @@
                        <&cru ACLK_VOP1>, <&cru HCLK_VOP1>,
                        <&cru ARMCLKL>, <&cru ARMCLKB>,
                        <&cru PLL_GPLL>, <&cru PLL_CPLL>,
-                       <&cru PLL_NPLL>,
+                       <&cru PLL_NPLL>, <&cru PLL_VPLL>,
                        <&cru ACLK_PERIHP>, <&cru HCLK_PERIHP>,
                        <&cru PCLK_PERIHP>,
                        <&cru ACLK_PERILP0>, <&cru HCLK_PERILP0>,
@@ -827,7 +827,7 @@
                        <400000000>,  <200000000>,
                        <816000000>, <1008000000>,
                         <594000000>,  <800000000>,
-                       <1000000000>,
+                       <1000000000>,  <900000000>,
                         <150000000>,   <75000000>,
                          <37500000>,
                         <100000000>,  <100000000>,


NOTE also: it seems terribly unlikely that adding CLK_SET_RATE_PARENT
to "vop0" would help with eDP, which really ought to be using vop1,
right?  In testing on my board, I found that eDP is in fact using vop1
with my current patch stack.

---

To summarize all that, I think that the following things would work OK
for a laptop until a better solution comes along:

* Probably VOP0 and VOP1 should both be able to change their parent's rate.

* Somehow adjust the panel rate to one that could be produced by CPLL
/ GPLL.  Presumably we'd want some code to add these extra modes to
simple-panel (?) and some code to know which mode to pick (?).

* make sure VOP0 is assigned to the panel (make this already is forced somehow?)

* make sure VOP0 starts out with the right parent (CPLL / GPLL) using
assigned-clocks in the device tree, so CCF will leave things alone.


Anyway, maybe everything I said is wrong.  If so, please correct.  ;)


-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-12  9:48 [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399 Xing Zheng
  2016-06-12 10:46 ` Yakir Yang
@ 2016-06-13 22:46 ` Heiko Stübner
  2016-06-14 16:04   ` Doug Anderson
  2016-06-22  0:31   ` Doug Anderson
  1 sibling, 2 replies; 8+ messages in thread
From: Heiko Stübner @ 2016-06-13 22:46 UTC (permalink / raw)
  To: Xing Zheng
  Cc: dianders, elaine.zhang, huangtao, briannorris, ykk,
	Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel,
	linux-rockchip, linux-kernel

Am Sonntag, 12. Juni 2016, 17:48:48 schrieb Xing Zheng:
> The functions and features VOP0 more complete than VOP1's, we need to
> use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
> screen.
> 
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
> 
>  drivers/clk/rockchip/clk-rk3399.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3399.c
> b/drivers/clk/rockchip/clk-rk3399.c index 7ecb12c3..6affa75 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -1157,7 +1157,7 @@ static struct rockchip_clk_branch
> rk3399_clk_branches[] __initdata = { GATE(HCLK_VOP0_NOC, "hclk_vop0_noc",
> "hclk_vop0_pre", CLK_IGNORE_UNUSED, RK3399_CLKGATE_CON(28), 0, GFLAGS),
> 
> -	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, 0,
> +	COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p,
> CLK_SET_RATE_PARENT, RK3399_CLKSEL_CON(49), 8, 2, MFLAGS, 0, 8, DFLAGS,
>  			RK3399_CLKGATE_CON(10), 12, GFLAGS),

The vpll is a possible source for multiple clocks (cci, aclk_vop0, dclk_vop0, 
clk_vop0_pwm, aclk_vop1, dclk_vop1, clk_vop1_pwm), so allowing one clock to 
hog the rate setting, might influence the other consumers of the vpll as well.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-13 22:46 ` Heiko Stübner
@ 2016-06-14 16:04   ` Doug Anderson
  2016-06-22  0:31   ` Doug Anderson
  1 sibling, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2016-06-14 16:04 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: Xing Zheng, elaine zhang, Tao Huang, Brian Norris, Yakir Yang,
	Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel,
	open list:ARM/Rockchip SoC...,
	linux-kernel

Heiko,

On Mon, Jun 13, 2016 at 3:46 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Sonntag, 12. Juni 2016, 17:48:48 schrieb Xing Zheng:
>> The functions and features VOP0 more complete than VOP1's, we need to
>> use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
>> screen.
>>
>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>> ---
>>
>>  drivers/clk/rockchip/clk-rk3399.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c
>> b/drivers/clk/rockchip/clk-rk3399.c index 7ecb12c3..6affa75 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -1157,7 +1157,7 @@ static struct rockchip_clk_branch
>> rk3399_clk_branches[] __initdata = { GATE(HCLK_VOP0_NOC, "hclk_vop0_noc",
>> "hclk_vop0_pre", CLK_IGNORE_UNUSED, RK3399_CLKGATE_CON(28), 0, GFLAGS),
>>
>> -     COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, 0,
>> +     COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p,
>> CLK_SET_RATE_PARENT, RK3399_CLKSEL_CON(49), 8, 2, MFLAGS, 0, 8, DFLAGS,
>>                       RK3399_CLKGATE_CON(10), 12, GFLAGS),
>
> The vpll is a possible source for multiple clocks (cci, aclk_vop0, dclk_vop0,
> clk_vop0_pwm, aclk_vop1, dclk_vop1, clk_vop1_pwm), so allowing one clock to
> hog the rate setting, might influence the other consumers of the vpll as well.

Ah, right.  I think this gets back to your series:

  8993791 [RFC,1/3] clk: fix inconsistent use of req_rate
  8993801 [RFC,2/3] clk: adjust clocks to their requested rate after
parent changes
  8993811 [RFC,3/3] clk: rockchip: make rk3399 vop dclks keep their
rate on parent rate changes

Did you ever have any more ideas about that?  I think the last thing
in that series was a comment from me on patch #2/3.  If we can't come
up with a general CCF solution for this problem, perhaps we need to
register for notifications for all the relevant clocks that might
change?

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-13 22:46 ` Heiko Stübner
  2016-06-14 16:04   ` Doug Anderson
@ 2016-06-22  0:31   ` Doug Anderson
  2016-06-26 22:18     ` Heiko Stuebner
  1 sibling, 1 reply; 8+ messages in thread
From: Doug Anderson @ 2016-06-22  0:31 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: Xing Zheng, elaine zhang, Tao Huang, Brian Norris, Yakir Yang,
	Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel,
	open list:ARM/Rockchip SoC...,
	linux-kernel

Heiko,

On Mon, Jun 13, 2016 at 3:46 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Sonntag, 12. Juni 2016, 17:48:48 schrieb Xing Zheng:
>> The functions and features VOP0 more complete than VOP1's, we need to
>> use it dclk_vop0_div operate VPLLI, and let VOP0 as the default primary
>> screen.
>>
>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>> ---
>>
>>  drivers/clk/rockchip/clk-rk3399.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c
>> b/drivers/clk/rockchip/clk-rk3399.c index 7ecb12c3..6affa75 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -1157,7 +1157,7 @@ static struct rockchip_clk_branch
>> rk3399_clk_branches[] __initdata = { GATE(HCLK_VOP0_NOC, "hclk_vop0_noc",
>> "hclk_vop0_pre", CLK_IGNORE_UNUSED, RK3399_CLKGATE_CON(28), 0, GFLAGS),
>>
>> -     COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p, 0,
>> +     COMPOSITE(DCLK_VOP0_DIV, "dclk_vop0_div", mux_pll_src_vpll_cpll_gpll_p,
>> CLK_SET_RATE_PARENT, RK3399_CLKSEL_CON(49), 8, 2, MFLAGS, 0, 8, DFLAGS,
>>                       RK3399_CLKGATE_CON(10), 12, GFLAGS),
>
> The vpll is a possible source for multiple clocks (cci, aclk_vop0, dclk_vop0,
> clk_vop0_pwm, aclk_vop1, dclk_vop1, clk_vop1_pwm), so allowing one clock to
> hog the rate setting, might influence the other consumers of the vpll as well.

OK, so let's think this through.

1. If we want to be able to support a wide variety of devices plugged
into an external display connector, we need _some_ type of PLL that
can change dynamically when the system is up.  On rk3399 there are new
fractional clocks available (yay!) but only for low speed pixel clocks
(boo!).  On rk3288 even those weren't there.

2. There are no PLLs dedicated to be used only for the pixel clock.
No matter what PLL we choose to use we _might_ end up changing some
else's rate when we change the pixel clock since someone else could be
a child.

2a) If we are careful with our clock tree we actually _can_ totally
avoid changing people's rates.  On rk3288 there's nothing else in the
system that really _needs_ NPLL and on rk3388 nothing really _needs_
VPLL.

2b) We could also try to do something dynamic like you proposed in
<https://patchwork.kernel.org/patch/8993811>, but presumably if a
child of NPLL/VPLL is really flexible about its rate (and thus could
handle this new rate), we'd be better off just starting that child off
parented somewhere else.  This relies on the fact that nothing really
_needs_ NPLL/VPLL.  Note that in rk3399, all muxings that include vpll
also include at least cpll and gpll; in rk3288 all muxings that
include npll also include at least cpll and gpll

2c) So, this means that if we are on a system that needs to support a
wide range of pixels clocks, we should ahead of time reparent everyone
else on another PLL and leave NPLL/VPLL as dynamic.

---

Let's think about use cases.  As far as I can think of, we have these.
Note that I'll call VPLL (rk3399) / NPLL (rk3288) the "dynamic" PLL.


A) There is some other important non-pixel-clock use of the "dynamic"
PLL where it's sub-optimal (or impossible) to use another PLL.

B) No important non-pixel-clock use for the "dynamic PLL".  Support
for arbitrary pixel clocks not important at all.

C) No important non-pixel-clock use for the "dynamic PLL".  Support
for arbitrary pixel clocks important for one display, not for the
other.

D) No important non-pixel-clock use for the "dynamic PLL".  Support
for arbitrary pixel clocks important for two displays.


I don't know of anyone in use case A), but if you do then please yell.

For B) we might have other things currently parented from the
"dynamic" PLL, but presumably these clocks are either not used or
could be reparented elsewhere.

Case C) is a device with a builtin display where the builtin display
can run fine on a pixel clock derived from CPLL or GPLL.  We might
need something like
<https://chromium-review.googlesource.com/#/c/354165/2>

Case D) is a device with two external connectors which might be used
at the same time.  You either need to pick one connector to get the
dynamic pixel clock (at boot? first come first served? userspace?) or
somehow rejigger your PLLs to allow two PLLs to be dynamic (don't
laugh, potentially GPLL could run at 1188 MHz and then you could
divide by 3 and get roughly 396 MHz, so you could actually just parent
everything on GPLL and make CPLL a second dynamic PLL).

---

OK, so what do we do?  Personally I can't see us coming up with a
one-size fits all approach, can you?  That means some type of
configuration.  ...and, as seen by the assigned-clocks device tree
binding, _some_ types of configuration is allowed in the device tree
presuming it is well thought out and describing how the designers of
the hardware "intended" it to be used.

So maybe:

i) Unless there's a counter example, I see no reason to let any clocks
other than the VOP display clocks parent on the "dynamic" PLL.  If we
later find a counter example then presumably we should add a device
tree property on that board.  We could have code at boot time that
goes through all clocks parented on "dynamic" PLL and reparents them
(trying to keep the rate?), they disallows future muxing to the
"dynamic" PLL.

ii) It seems sane to me to add a device tree property to the board
that will effectively enable CLK_SET_RATE_PARENT on one of the
dclk_vop clocks and forcing it to the "dynamic" PLL (no
auto-remuxing).  Maybe we would add something to the HDMI node, for
instance, like "rockchip,intended-dclk-pll = <&...>".  Then somehow
this would need to affect whichever VOP was assigned to HDMI.

iii) If later someone can propose how to handle D) above, we can
handle it then.  Until a solution is proposed those boards would work
like today.

---

OK, what was long.  Thoughts?


-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399
  2016-06-22  0:31   ` Doug Anderson
@ 2016-06-26 22:18     ` Heiko Stuebner
  0 siblings, 0 replies; 8+ messages in thread
From: Heiko Stuebner @ 2016-06-26 22:18 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Xing Zheng, elaine zhang, Tao Huang, Brian Norris, Yakir Yang,
	Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel,
	open list:ARM/Rockchip SoC...,
	linux-kernel, tomasz.figa

Am Dienstag, 21. Juni 2016, 17:31:16 schrieb Doug Anderson:
> OK, so what do we do?  Personally I can't see us coming up with a
> one-size fits all approach, can you?  That means some type of
> configuration.  ...and, as seen by the assigned-clocks device tree
> binding, _some_ types of configuration is allowed in the device tree
> presuming it is well thought out and describing how the designers of
> the hardware "intended" it to be used.
> 
> So maybe:
> 
> i) Unless there's a counter example, I see no reason to let any clocks
> other than the VOP display clocks parent on the "dynamic" PLL.  If we
> later find a counter example then presumably we should add a device
> tree property on that board.  We could have code at boot time that
> goes through all clocks parented on "dynamic" PLL and reparents them
> (trying to keep the rate?), they disallows future muxing to the
> "dynamic" PLL.

Two problems:
- "disallowing future muxing to the pll" feels a bit far on the "policy"-
side on the policy<->hw-description scale, I still guess the kernel should 
be allowed to shoot itself in the foot :-)
- would probably include hacking up the clock parent-names, which won't work 
at runtime, as they are init params and get memcpy'd on clk creation.

But I guess this issue of depending clock maybe needing to adapt to a 
changed pll-rate here would be solvable with my rate-reassignment I still 
need to revisit.


> ii) It seems sane to me to add a device tree property to the board
> that will effectively enable CLK_SET_RATE_PARENT on one of the
> dclk_vop clocks and forcing it to the "dynamic" PLL (no
> auto-remuxing).  Maybe we would add something to the HDMI node, for
> instance, like "rockchip,intended-dclk-pll = <&...>".  Then somehow
> this would need to affect whichever VOP was assigned to HDMI.

Right now I see two issues:
- can the drm reassign vops at runtime [don't know enough about drm], but 
what would happen in such a case when hdmi jumps from one to the other vop.
- clock-flags [CLK_SET_RATE_PARENT and friends] are clk-init data, set when 
creating a clock, so there is no changing this at runtime - see above

I guess one option to handle this could be to not have anybody getting a 
real CLK_SET_RATE_PARENT to the vpll, but instead having the vops handle 
this special pll if necessary. Aka check if the requested rate is possible 
using the current sources (clk_round_rate on the dclk_vopX) and if not adapt 
the special pll to a suitable source rate and re-check.

And if there is no dclk-vpll, then simply skip that step and try to get the 
best rate possible.


> iii) If later someone can propose how to handle D) above, we can
> handle it then.  Until a solution is proposed those boards would work
> like today.

Having the "rockchip,dclk-pll = <&...>;" live in the display-subsystem node 
or so (=assigned to the global display-subsystem not one special vop) the 
available vops might even be able to work out between them who should get 
access to it depending on connected displays / modes?


Heiko

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-26 22:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-12  9:48 [PATCH] clk: rockchip: add flag CLK_SET_RATE_PARENT for dclk_vop0_div on RK3399 Xing Zheng
2016-06-12 10:46 ` Yakir Yang
2016-06-13 18:37   ` Brian Norris
2016-06-13 20:43     ` Doug Anderson
2016-06-13 22:46 ` Heiko Stübner
2016-06-14 16:04   ` Doug Anderson
2016-06-22  0:31   ` Doug Anderson
2016-06-26 22:18     ` Heiko Stuebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).