[PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Kever Yang]

we are going to make a clock usage solution for rk3288:
1. CPLL and GPLL always not change after assign init;
2. NPLL default as 500MHz, may used for most scene;
3. NPLL may be changed by VOP(HDMI) clock for some special
   frequency requirement.

    I test it with rk3288 evb on top of Heiko's clk-for-next


Kever Yang (5):
  clk: rockchip: add some clock rate into rate table for rk3288
  clk: divider: make clk_divider_recalc/set_rate available
  clk: rockchip: introduce the div_ops handling for composite branches
  clk: rockchip: add the vop_determine_rate for vop dclock
  clk: rockchip: change DCLK_VOP0 to use new COMPOSITE_DIVOPS

 drivers/clk/clk-divider.c         |  4 +--
 drivers/clk/rockchip/clk-rk3288.c | 76 ++++++++++++++++++++++++++++++++++++++-
 drivers/clk/rockchip/clk.c        | 13 ++++---
 drivers/clk/rockchip/clk.h        | 24 +++++++++++++
 include/linux/clk-provider.h      |  4 +++
 5 files changed, 114 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/5] clk: rockchip: add some clock rate into rate table for rk3288

[Kever Yang]

We are going to support all the HDMI resolutions which need a bunch
of accurate clock rates. This patch makes the pll rate table can
provide all the option clock rate that HDMI controller may needed.

Here's what HDMI needs:
resolutions 		Pixel clock(Mhz)
1920x1080p 60/50	148.5
1920x1080i 100/120 	148.5
1920x1080i 60/50	74.25
1280x720p 60/50/30/25 	74.25
720x576p 50		27
720x480p 60		27
1440x480i 60		27
1440x576i 50 		27
1680x1050 60 		146.25
1280x1024 60		108
1280x960 60 		108
1440x900 60 		106.5
1280x800 60 		83.5
1024x768 60 		65
800x600 60 		40
800x600 56 		36
640x480 60 		25.175

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

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

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 279a662..48412e9 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -73,6 +73,7 @@ struct rockchip_pll_rate_table rk3288_pll_rates[] = {
 	RK3066_PLL_RATE(1128000000, 1, 47, 1),
 	RK3066_PLL_RATE(1104000000, 1, 46, 1),
 	RK3066_PLL_RATE(1008000000, 1, 84, 2),
+	RK3066_PLL_RATE(1007000000, 12, 1007, 2),
 	RK3066_PLL_RATE( 912000000, 1, 76, 2),
 	RK3066_PLL_RATE( 891000000, 8, 594, 2),
 	RK3066_PLL_RATE( 888000000, 1, 74, 2),
@@ -84,6 +85,7 @@ struct rockchip_pll_rate_table rk3288_pll_rates[] = {
 	RK3066_PLL_RATE( 696000000, 1, 58, 2),
 	RK3066_PLL_RATE( 600000000, 1, 50, 2),
 	RK3066_PLL_RATE_BWADJ(594000000, 1, 198, 8, 1),
+	RK3066_PLL_RATE( 585000000, 4, 195, 2),
 	RK3066_PLL_RATE( 552000000, 1, 46, 2),
 	RK3066_PLL_RATE( 504000000, 1, 84, 4),
 	RK3066_PLL_RATE( 500000000, 3, 125, 2),
@@ -97,6 +99,7 @@ struct rockchip_pll_rate_table rk3288_pll_rates[] = {
 	RK3066_PLL_RATE( 297000000, 2, 198, 8),
 	RK3066_PLL_RATE( 252000000, 1, 84, 8),
 	RK3066_PLL_RATE( 216000000, 1, 72, 8),
+	RK3066_PLL_RATE( 167000000, 3, 167, 8),
 	RK3066_PLL_RATE( 148500000, 2, 99, 8),
 	RK3066_PLL_RATE( 126000000, 1, 84, 16),
 	RK3066_PLL_RATE(  48000000, 1, 64, 32),
-- 
1.9.1

[PATCH 2/5] clk: divider: make clk_divider_recalc/set_rate available

[Kever Yang]

This patch makes these two function available for other file,
which may help to make costom usage of clock divider type.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/clk/clk-divider.c    | 4 ++--
 include/linux/clk-provider.h | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 18a9de2..f3f55a8 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -109,7 +109,7 @@ static unsigned int _get_val(struct clk_divider *divider, unsigned int div)
 	return div - 1;
 }
 
-static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
+unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 		unsigned long parent_rate)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
@@ -318,7 +318,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 	return DIV_ROUND_UP(*prate, div);
 }
 
-static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long parent_rate)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index be21af1..7947fe9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -555,6 +555,10 @@ struct clk *__clk_lookup(const char *name);
 long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
 			      unsigned long *best_parent_rate,
 			      struct clk **best_parent_p);
+unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate);
+int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+			 unsigned long parent_rate);
 
 /*
  * FIXME clock api without lock protection
-- 
1.9.1

[PATCH 3/5] clk: rockchip: introduce the div_ops handling for composite branches

[Kever Yang]

Rockchip Socs have a lot of clock node registered as composite
branch which include mux, divider and gate, most of them use
the same ops handling callback, we still need special ops
handling for some special clock node and this patch make it
possible.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/clk/rockchip/clk.c | 13 +++++++++----
 drivers/clk/rockchip/clk.h | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 1e68bff..0917c2b 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -42,6 +42,7 @@ static struct clk *rockchip_clk_register_branch(const char *name,
 		const char **parent_names, u8 num_parents, void __iomem *base,
 		int muxdiv_offset, u8 mux_shift, u8 mux_width, u8 mux_flags,
 		u8 div_shift, u8 div_width, u8 div_flags,
+		const struct clk_ops *divops,
 		struct clk_div_table *div_table, int gate_offset,
 		u8 gate_shift, u8 gate_flags, unsigned long flags,
 		spinlock_t *lock)
@@ -90,9 +91,12 @@ static struct clk *rockchip_clk_register_branch(const char *name,
 		div->width = div_width;
 		div->lock = lock;
 		div->table = div_table;
-		div_ops = (div_flags & CLK_DIVIDER_READ_ONLY)
-						? &clk_divider_ro_ops
-						: &clk_divider_ops;
+		if (divops)
+			div_ops = divops;
+		else if (div_flags & CLK_DIVIDER_READ_ONLY)
+			div_ops = &clk_divider_ro_ops;
+		else
+			div_ops = &clk_divider_ops;
 	}
 
 	clk = clk_register_composite(NULL, name, parent_names, num_parents,
@@ -275,7 +279,8 @@ void __init rockchip_clk_register_branches(
 				reg_base, list->muxdiv_offset, list->mux_shift,
 				list->mux_width, list->mux_flags,
 				list->div_shift, list->div_width,
-				list->div_flags, list->div_table,
+				list->div_flags,
+				list->div_ops, list->div_table,
 				list->gate_offset, list->gate_shift,
 				list->gate_flags, flags, &clk_lock);
 			break;
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 6baf665..2cf263b 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -185,6 +185,7 @@ struct rockchip_clk_branch {
 	u8				div_shift;
 	u8				div_width;
 	u8				div_flags;
+	const struct clk_ops		*div_ops;
 	struct clk_div_table		*div_table;
 	int				gate_offset;
 	u8				gate_shift;
@@ -212,6 +213,29 @@ struct rockchip_clk_branch {
 		.gate_flags	= gf,				\
 	}
 
+#define COMPOSITE_DIVOPS(_id, cname, pnames, f, mo, ms, mw, mf, \
+			ds, dw, df, dops, go, gs, gf)		\
+	{							\
+		.id		= _id,				\
+		.branch_type	= branch_composite,		\
+		.name		= cname,			\
+		.parent_names	= pnames,			\
+		.num_parents	= ARRAY_SIZE(pnames),		\
+		.flags		= f,				\
+		.muxdiv_offset	= mo,				\
+		.mux_shift	= ms,				\
+		.mux_width	= mw,				\
+		.mux_flags	= mf,				\
+		.div_shift	= ds,				\
+		.div_width	= dw,				\
+		.div_flags	= df,				\
+		.div_ops	= dops,				\
+		.gate_offset	= go,				\
+		.gate_shift	= gs,				\
+		.gate_flags	= gf,				\
+	}
+
+
 #define COMPOSITE_NOMUX(_id, cname, pname, f, mo, ds, dw, df,	\
 			go, gs, gf)				\
 	{							\
-- 
1.9.1

[PATCH 4/5] clk: rockchip: add the vop_determine_rate for vop dclock

[Kever Yang]

Rk3288 has 5 PLLs(APLL, DPLL, CPLL, GPLL, NPLL),
APLL is for CPU clock only and DPLL is for DRAM clock only,
and other 3 PLls used for all other peripherals.
We have to make a total solution for how to campatible all
kinds of clock requirement by on chip peripheral controllers.

Some controllers like I2S and HDMI need accurate frequency while
others controllers accept clock rate with margin.

According to our experience on rk3288, we prefer to use CPLL and GPLL fixed
at 400MHz and 594MHz for general use for most peripheral.

The fraction divider should be enough for I2S controller.

The HDMI is the most diffical one if we have to support all the
resolution requirement for frequency. Most people use 720p and
1080 i/p resolution with 74.25MHz/148.5MHz, which can get clock
rate from 594MHz(maybe from GPLL). some other resolution like
640*480 will use 25.175MHz, which is hard to get from general
used PLLs.

So it is better to make HDMI controller has the right to change
the PLL frequency and get the clock rate it wants.

We set NPLL to 500MHz as default, if HDMI can get what it need
from existent clock provider, then change its divider and switch
to that parent; if not, we have to change the NPLL's output
and always make CPLL&GPLL not change.

This patch add vop_determinate_rate as a div_ops to handle
the HDMI clock things.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

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

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 48412e9..0151140 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -14,6 +14,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk-private.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <dt-bindings/clock/rk3288-cru.h>
@@ -25,6 +26,7 @@
 enum rk3288_plls {
 	apll, dpll, cpll, gpll, npll,
 };
+const struct clk_ops dclk_vop_ops;
 
 struct rockchip_pll_rate_table rk3288_pll_rates[] = {
 	RK3066_PLL_RATE(2208000000, 1, 92, 1),
@@ -766,6 +768,73 @@ static const char *rk3288_critical_clocks[] __initconst = {
 	"aclk_peri",
 	"hclk_peri",
 };
+#define DCLK_VOP_PARENT_NPLL 2
+
+long dclk_vop_determine_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long *best_parent_rate,
+					struct clk **best_parent_p)
+{
+	struct clk *clk = hw->clk, *parent;
+	unsigned long parent_rate, best = 0;
+	int num_parents = clk->num_parents;
+	int i;
+
+	/*
+	 * check if one of the generic plls can provide a cleanly dividable
+	 * rate without changing them.
+	 */
+	for (i = 0; i < (num_parents - 1); i++) {
+		parent = clk_get_parent_by_index(clk, i);
+		parent_rate = __clk_get_rate(parent);
+		if (parent_rate % rate == 0) {
+			*best_parent_p = parent;
+			*best_parent_rate = parent_rate;
+			return rate;
+		}
+	}
+
+	/* take the npll and set its rate to something suitable */
+	for (i = 0; rk3288_pll_rates[i].rate != 0; i++) {
+		if (rk3288_pll_rates[i].rate % rate == 0) {
+			*best_parent_p = clk_get_parent_by_index(clk,
+						DCLK_VOP_PARENT_NPLL);
+			*best_parent_rate = rk3288_pll_rates[i].rate;
+			return rk3288_pll_rates[i].rate;
+		}
+	}
+
+	/*
+	 * We were not able to find a matching rate, so falling back
+	 * to finding the fastest rate < rate.
+	 * We allow the npll to change its rate while the other plls
+	 * are not allowed to change.
+	 */
+	for (i = 0; i < num_parents; i++) {
+		parent = clk_get_parent_by_index(clk, i);
+		if (!parent)
+			continue;
+
+		if (i == DCLK_VOP_PARENT_NPLL)
+			parent_rate = __clk_round_rate(parent, rate);
+		else
+			parent_rate = __clk_get_rate(parent);
+		if (parent_rate <= rate && parent_rate > best) {
+			int div = DIV_ROUND_UP(parent_rate, rate);
+			*best_parent_p = parent;
+			*best_parent_rate = parent_rate;
+			best = DIV_ROUND_UP(parent_rate, div);
+		}
+	}
+
+	return best;
+}
+
+
+const struct clk_ops dclk_vop_ops = {
+	.recalc_rate = clk_divider_recalc_rate,
+	.set_rate = clk_divider_set_rate,
+	.determine_rate = dclk_vop_determine_rate,
+};
 
 static void __init rk3288_clk_init(struct device_node *np)
 {
-- 
1.9.1

[PATCH 5/5] clk: rockchip: change DCLK_VOP0 to use new COMPOSITE_DIVOPS

[Kever Yang]

The HDMI clock is actually provide by DCLK_VOP0, so we need this
patch to handle the HDMI clock correctly

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/clk/rockchip/clk-rk3288.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 0151140..073a719 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -396,8 +396,10 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 			RK3288_CLKSEL_CON(30), 14, 2, MFLAGS, 8, 5, DFLAGS,
 			RK3288_CLKGATE_CON(3), 4, GFLAGS),
 
-	COMPOSITE(DCLK_VOP0, "dclk_vop0", mux_pll_src_cpll_gpll_npll_p, 0,
+	COMPOSITE_DIVOPS(DCLK_VOP0, "dclk_vop0", mux_pll_src_cpll_gpll_npll_p,
+			(CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT),
 			RK3288_CLKSEL_CON(27), 0, 2, MFLAGS, 8, 8, DFLAGS,
+			&dclk_vop_ops,
 			RK3288_CLKGATE_CON(3), 1, GFLAGS),
 	COMPOSITE(DCLK_VOP1, "dclk_vop1", mux_pll_src_cpll_gpll_npll_p, 0,
 			RK3288_CLKSEL_CON(29), 6, 2, MFLAGS, 8, 8, DFLAGS,
-- 
1.9.1

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Heiko Stübner]

Hi Kever,

Am Dienstag, 4. November 2014, 15:52:34 schrieb Kever Yang:
> we are going to make a clock usage solution for rk3288:
> 1. CPLL and GPLL always not change after assign init;
> 2. NPLL default as 500MHz, may used for most scene;
> 3. NPLL may be changed by VOP(HDMI) clock for some special
>    frequency requirement.
> 
>     I test it with rk3288 evb on top of Heiko's clk-for-next

In general I'm not really sure if allowing one component to arbitarily change 
a shared clock wouldn't result in trouble.

At the moment only dclk_vop0 is included in your series, while the hdmi 
controller can connect to both vop0 and vop1.
And as Doug mentioned the gpu also has the npll as one possible source.

Looking through the clock-tree there are a lot more components possibly using 
(or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp, hevc, 
gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow reserving 
the npll for VOP0 alone.

But I also don't see a different way to get these frequencies right now.


Heiko

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Kever Yang]

Hi Heiko,

On 11/07/2014 05:06 AM, Heiko Stübner wrote:
> Hi Kever,
>
> Am Dienstag, 4. November 2014, 15:52:34 schrieb Kever Yang:
>> we are going to make a clock usage solution for rk3288:
>> 1. CPLL and GPLL always not change after assign init;
>> 2. NPLL default as 500MHz, may used for most scene;
>> 3. NPLL may be changed by VOP(HDMI) clock for some special
>>     frequency requirement.
>>
>>      I test it with rk3288 evb on top of Heiko's clk-for-next
> In general I'm not really sure if allowing one component to arbitarily change
> a shared clock wouldn't result in trouble.
>
> At the moment only dclk_vop0 is included in your series, while the hdmi
> controller can connect to both vop0 and vop1.
> And as Doug mentioned the gpu also has the npll as one possible source.
I think the problem GPU HANGs with 480MHz clock from usbphy has
been fixed with my patch to gerrit:
https://chromium-review.googlesource.com/#/c/229554/
>
> Looking through the clock-tree there are a lot more components possibly using
> (or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp, hevc,
> gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow reserving
> the npll for VOP0 alone.
It's true that I customized the usage of npll for VOP0 when we need some
very special frequency, but it doesn't means other modules can't use the
npll, it will always decided by clock core for module clocks that which 
parent
is the best.

I'll be very happy if there is a better solution for this situation, and any
suggestion is welcome.

- Kever
>
> But I also don't see a different way to get these frequencies right now.
>
>
> Heiko
>
>
>

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Doug Anderson]

Hi,

On Thu, Nov 13, 2014 at 12:52 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Heiko,
>
> On 11/07/2014 05:06 AM, Heiko Stübner wrote:
>>
>> Hi Kever,
>>
>> Am Dienstag, 4. November 2014, 15:52:34 schrieb Kever Yang:
>>>
>>> we are going to make a clock usage solution for rk3288:
>>> 1. CPLL and GPLL always not change after assign init;
>>> 2. NPLL default as 500MHz, may used for most scene;
>>> 3. NPLL may be changed by VOP(HDMI) clock for some special
>>>     frequency requirement.
>>>
>>>      I test it with rk3288 evb on top of Heiko's clk-for-next
>>
>> In general I'm not really sure if allowing one component to arbitarily
>> change
>> a shared clock wouldn't result in trouble.
>>
>> At the moment only dclk_vop0 is included in your series, while the hdmi
>> controller can connect to both vop0 and vop1.
>> And as Doug mentioned the gpu also has the npll as one possible source.
>
> I think the problem GPU HANGs with 480MHz clock from usbphy has
> been fixed with my patch to gerrit:
> https://chromium-review.googlesource.com/#/c/229554/
>>
>>
>> Looking through the clock-tree there are a lot more components possibly
>> using
>> (or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp,
>> hevc,
>> gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow
>> reserving
>> the npll for VOP0 alone.
>
> It's true that I customized the usage of npll for VOP0 when we need some
> very special frequency, but it doesn't means other modules can't use the
> npll, it will always decided by clock core for module clocks that which
> parent
> is the best.

We will just need to be very careful.  As I've mentioned in the past
we'll need to think about what happens to other clocks that happen to
be parented by NPLL whenever we change it.

So if we do this:

1. NPLL happens to be 500MHz.
2. We set GPU to be 500MHz and it picks NPLL.
3. We change NPLL to a different speed (like 600MHz).

...I believe in this scenario the GPU would start running at 600MHz
immediately.  We'd need to add special code to watch out for this and
pick an alternate clock for the GPU (like the USB 480) before the NPLL
change.  If NPLL later changes back to 500MHz and the GPU still wanted
500MHz, we'd have to decide what to do.


The summary is: it's pretty complicated

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Kever Yang]

Hi

On 11/14/2014 09:46 AM, Mike Turquette wrote:
>>>> Looking through the clock-tree there are a lot more components possibly
>>>> > >>using
>>>> > >>(or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp,
>>>> > >>hevc,
>>>> > >>gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow
>>>> > >>reserving
>>>> > >>the npll for VOP0 alone.
I understand the concern for other module clocks which may use NPLL as
parent, we have to make sure all these clocks can get what they want even
if NPLL is not available for them. So, let's have a look at them(without 
NPLL):
with CPLL 400M and GPLL 594M,

GPU/HEVC/ISP/TSP can get 100/200/400 from CPLL, and ~300/600 from GPLL,
        and ~500 from usbphy480m, these should be enough for those modules.

DCLK_VOP1 for eDP/MIPI/LVDS, these clock can accept maybe more than 5%
margin, we can get enough frequency for them now, we can change the CPLL
to 800MHz for more option if it's needed one day.

GMAC can get 50M from CPLL, usually we use external clock for GMAC.

uart0 have frac divider, so we don't need to worry too much for it.



>>> > >
>>> > >It's true that I customized the usage of npll for VOP0 when we need some
>>> > >very special frequency, but it doesn't means other modules can't use the
>>> > >npll, it will always decided by clock core for module clocks that which
>>> > >parent
>>> > >is the best.
>> >
>> >We will just need to be very careful.  As I've mentioned in the past
>> >we'll need to think about what happens to other clocks that happen to
>> >be parented by NPLL whenever we change it.
>> >
>> >So if we do this:
>> >
>> >1. NPLL happens to be 500MHz.
>> >2. We set GPU to be 500MHz and it picks NPLL.
>> >3. We change NPLL to a different speed (like 600MHz).
>> >
>> >...I believe in this scenario the GPU would start running at 600MHz
>> >immediately.  We'd need to add special code to watch out for this and
>> >pick an alternate clock for the GPU (like the USB 480) before the NPLL
>> >change.  If NPLL later changes back to 500MHz and the GPU still wanted
>> >500MHz, we'd have to decide what to do.
>> >
>> >
>> >The summary is: it's pretty complicated
It's complicated if there are more than one clock share the PLL and
one of the clock wants to change the PLL output rate.

Most of module clocks can't be changed during they are work, and it
is better to route those clocks to a parent that would not change.

Some of PLLs should not be changed after system initialized and they
can be source for most of module clocks while some of PLLs have to
change for some special requirement like HDMI that we can know
the required frequency only when the device is plug in at run time.

To make it simple, we can use the NPLL exclusively for HDMI/VOP0,
just like what we do for APLL for cpu and DPLL for DDR, although what
I though was share NPLL with other clocks in most of time, maybe we
can use this case in the product like OTT BOX which will always have
HDMI pluged.

Maybe we can add an attribute for clock like NPLL in this way?
There is an owner children for NPLL(for example VOP0) which can change 
NPLL's rate,
and there it a fixed_rate attribute to describe if this clock is fixed 
or not.

If VOP0 is not enabled, NPLL output is unfixed, other children clock can
decide if they want to use a parent with unfixed output or another 
parent with
fixed output.

If VOP0 is enabled, then the NPLL's output has decide by the VOP0 and it 
become
a fixed output parent.

> +Stephen Boyd & Tomeu Vizoso
>
> Managing shared clocks is a subset of the general problems with clock
> constraints. Maybe Stephen or Tomeu have some comment here?

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Tomeu Vizoso]

On 13 November 2014 at 23:59, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Nov 13, 2014 at 12:52 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Heiko,
>>
>> On 11/07/2014 05:06 AM, Heiko Stübner wrote:
>>>
>>> Hi Kever,
>>>
>>> Am Dienstag, 4. November 2014, 15:52:34 schrieb Kever Yang:
>>>>
>>>> we are going to make a clock usage solution for rk3288:
>>>> 1. CPLL and GPLL always not change after assign init;
>>>> 2. NPLL default as 500MHz, may used for most scene;
>>>> 3. NPLL may be changed by VOP(HDMI) clock for some special
>>>>     frequency requirement.
>>>>
>>>>      I test it with rk3288 evb on top of Heiko's clk-for-next
>>>
>>> In general I'm not really sure if allowing one component to arbitarily
>>> change
>>> a shared clock wouldn't result in trouble.
>>>
>>> At the moment only dclk_vop0 is included in your series, while the hdmi
>>> controller can connect to both vop0 and vop1.
>>> And as Doug mentioned the gpu also has the npll as one possible source.
>>
>> I think the problem GPU HANGs with 480MHz clock from usbphy has
>> been fixed with my patch to gerrit:
>> https://chromium-review.googlesource.com/#/c/229554/
>>>
>>>
>>> Looking through the clock-tree there are a lot more components possibly
>>> using
>>> (or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp,
>>> hevc,
>>> gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow
>>> reserving
>>> the npll for VOP0 alone.
>>
>> It's true that I customized the usage of npll for VOP0 when we need some
>> very special frequency, but it doesn't means other modules can't use the
>> npll, it will always decided by clock core for module clocks that which
>> parent
>> is the best.
>
> We will just need to be very careful.  As I've mentioned in the past
> we'll need to think about what happens to other clocks that happen to
> be parented by NPLL whenever we change it.
>
> So if we do this:
>
> 1. NPLL happens to be 500MHz.
> 2. We set GPU to be 500MHz and it picks NPLL.
> 3. We change NPLL to a different speed (like 600MHz).
>
> ...I believe in this scenario the GPU would start running at 600MHz
> immediately.  We'd need to add special code to watch out for this and
> pick an alternate clock for the GPU (like the USB 480) before the NPLL
> change.  If NPLL later changes back to 500MHz and the GPU still wanted
> 500MHz, we'd have to decide what to do.
>
> The summary is: it's pretty complicated

Hello,

I have heard that this is still an issue in RK3288 support in mainline
so have given a look and wonder if you have considered having the
consumers of a shared clock (which could be a child clock) use
PRE_RATE_CHANGE notifications to adapt to changes in a parent's rate,
or to abort them if that's not possible.

Alternatively, a consumer can set min and max constraints if it
doesn't want for others to mess with its provider, and let the other
consumer deal with failed rate changes (probably by choosing another
provider).

Have I missed any problems with that?

Thanks,

Tomeu

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Doug Anderson]

Tomeu,

On Tue, Jan 19, 2016 at 4:02 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> On 13 November 2014 at 23:59, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Thu, Nov 13, 2014 at 12:52 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
>>> Hi Heiko,
>>>
>>> On 11/07/2014 05:06 AM, Heiko Stübner wrote:
>>>>
>>>> Hi Kever,
>>>>
>>>> Am Dienstag, 4. November 2014, 15:52:34 schrieb Kever Yang:
>>>>>
>>>>> we are going to make a clock usage solution for rk3288:
>>>>> 1. CPLL and GPLL always not change after assign init;
>>>>> 2. NPLL default as 500MHz, may used for most scene;
>>>>> 3. NPLL may be changed by VOP(HDMI) clock for some special
>>>>>     frequency requirement.
>>>>>
>>>>>      I test it with rk3288 evb on top of Heiko's clk-for-next
>>>>
>>>> In general I'm not really sure if allowing one component to arbitarily
>>>> change
>>>> a shared clock wouldn't result in trouble.
>>>>
>>>> At the moment only dclk_vop0 is included in your series, while the hdmi
>>>> controller can connect to both vop0 and vop1.
>>>> And as Doug mentioned the gpu also has the npll as one possible source.
>>>
>>> I think the problem GPU HANGs with 480MHz clock from usbphy has
>>> been fixed with my patch to gerrit:
>>> https://chromium-review.googlesource.com/#/c/229554/
>>>>
>>>>
>>>> Looking through the clock-tree there are a lot more components possibly
>>>> using
>>>> (or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp,
>>>> hevc,
>>>> gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow
>>>> reserving
>>>> the npll for VOP0 alone.
>>>
>>> It's true that I customized the usage of npll for VOP0 when we need some
>>> very special frequency, but it doesn't means other modules can't use the
>>> npll, it will always decided by clock core for module clocks that which
>>> parent
>>> is the best.
>>
>> We will just need to be very careful.  As I've mentioned in the past
>> we'll need to think about what happens to other clocks that happen to
>> be parented by NPLL whenever we change it.
>>
>> So if we do this:
>>
>> 1. NPLL happens to be 500MHz.
>> 2. We set GPU to be 500MHz and it picks NPLL.
>> 3. We change NPLL to a different speed (like 600MHz).
>>
>> ...I believe in this scenario the GPU would start running at 600MHz
>> immediately.  We'd need to add special code to watch out for this and
>> pick an alternate clock for the GPU (like the USB 480) before the NPLL
>> change.  If NPLL later changes back to 500MHz and the GPU still wanted
>> 500MHz, we'd have to decide what to do.
>>
>> The summary is: it's pretty complicated
>
> Hello,
>
> I have heard that this is still an issue in RK3288 support in mainline
> so have given a look and wonder if you have considered having the
> consumers of a shared clock (which could be a child clock) use
> PRE_RATE_CHANGE notifications to adapt to changes in a parent's rate,
> or to abort them if that's not possible.
>
> Alternatively, a consumer can set min and max constraints if it
> doesn't want for others to mess with its provider, and let the other
> consumer deal with failed rate changes (probably by choosing another
> provider).
>
> Have I missed any problems with that?

I don't think it's really that simple, but I certainly could be wrong.

Summary of the system:

* There are two VOPs (video processors) on rk3288: the big one and the
little one.

* The big VOP can handle higher resolutions than the little one but
uses more power.

* Both VOPs two can be sourced from CPLL, GPLL, or NPLL.

* Both VOPs can be setup to process video for either HDMI, eDP, LVDS, ...

* NPLL is the "new" PLL and is kinda like an extra PLL that's not used
for any of the core peripherals.  It's intended to be one that's
changed dynamically based on system needs.

* GPLL appears intended (in current designs) to run at 594 MHz on
rk3288 using a special "low jitter" setting.  This is intended to be
the source clock for HDMI when running the very common 148.5 and 74.25
rates.  It also it intended to be the source clock for various other
peripherals like i2c, eMMC, SD, etc.  This also runs the GPU when the
GPU is running at ~600MHz or ~300MHz.

* CPLL appears intended (in current designs) to run at 400MHz.  There
are some peripherals that run off this plus the GPU at 400MHz, 200MHz,
or 100MHz.


Let's imagine that we're booting up on a laptop where the builtin
panel is hooked up via eDP.  Let's say that the panel really wants
76.42 MHz but that will work just fine with 74.25 MHz too.  If this is
a system that has no other graphics needs, we'd want eDP to take NPLL
and grab 76.42 MHz.  Make the best rate, right?

...but what about if it's a system with an HDMI port?  The eDP panel
is built in and we know it works with 74.25 MHz, but the user could
plug into all sorts of HDMI devices and they may require some very
special PLL rates to get there.  So if we happen to take NPLL for eDP
we'll need to know to switch away from it once something is plugged
into HDMI.  We don't want to block the HDMI request since we know we
can run eDP at 74.25 off GPLL.  Presumably we could see the HDMI
request and then try to remux the eDP on the fly.  ...but what decides
the HDMI is more important to eDP?  You might want different rules if
the eDP port is somehow exposed and the user might plug different
panels into it...


-Doug

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Tomeu Vizoso]

On 20 January 2016 at 17:50, Doug Anderson <dianders@chromium.org> wrote:
> Tomeu,
>
> On Tue, Jan 19, 2016 at 4:02 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>> On 13 November 2014 at 23:59, Doug Anderson <dianders@chromium.org> wrote:
>>> Hi,
>>>
>>> On Thu, Nov 13, 2014 at 12:52 AM, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>> Hi Heiko,
>>>>
>>>> On 11/07/2014 05:06 AM, Heiko Stübner wrote:
>>>>>
>>>>> Hi Kever,
>>>>>
>>>>> Am Dienstag, 4. November 2014, 15:52:34 schrieb Kever Yang:
>>>>>>
>>>>>> we are going to make a clock usage solution for rk3288:
>>>>>> 1. CPLL and GPLL always not change after assign init;
>>>>>> 2. NPLL default as 500MHz, may used for most scene;
>>>>>> 3. NPLL may be changed by VOP(HDMI) clock for some special
>>>>>>     frequency requirement.
>>>>>>
>>>>>>      I test it with rk3288 evb on top of Heiko's clk-for-next
>>>>>
>>>>> In general I'm not really sure if allowing one component to arbitarily
>>>>> change
>>>>> a shared clock wouldn't result in trouble.
>>>>>
>>>>> At the moment only dclk_vop0 is included in your series, while the hdmi
>>>>> controller can connect to both vop0 and vop1.
>>>>> And as Doug mentioned the gpu also has the npll as one possible source.
>>>>
>>>> I think the problem GPU HANGs with 480MHz clock from usbphy has
>>>> been fixed with my patch to gerrit:
>>>> https://chromium-review.googlesource.com/#/c/229554/
>>>>>
>>>>>
>>>>> Looking through the clock-tree there are a lot more components possibly
>>>>> using
>>>>> (or wanting to use) the npll: of course the VOPs, the edp, hdmi, isp,
>>>>> hevc,
>>>>> gpu, tsp uart0 and gmac. So I'm slightly uncomfortable with somehow
>>>>> reserving
>>>>> the npll for VOP0 alone.
>>>>
>>>> It's true that I customized the usage of npll for VOP0 when we need some
>>>> very special frequency, but it doesn't means other modules can't use the
>>>> npll, it will always decided by clock core for module clocks that which
>>>> parent
>>>> is the best.
>>>
>>> We will just need to be very careful.  As I've mentioned in the past
>>> we'll need to think about what happens to other clocks that happen to
>>> be parented by NPLL whenever we change it.
>>>
>>> So if we do this:
>>>
>>> 1. NPLL happens to be 500MHz.
>>> 2. We set GPU to be 500MHz and it picks NPLL.
>>> 3. We change NPLL to a different speed (like 600MHz).
>>>
>>> ...I believe in this scenario the GPU would start running at 600MHz
>>> immediately.  We'd need to add special code to watch out for this and
>>> pick an alternate clock for the GPU (like the USB 480) before the NPLL
>>> change.  If NPLL later changes back to 500MHz and the GPU still wanted
>>> 500MHz, we'd have to decide what to do.
>>>
>>> The summary is: it's pretty complicated
>>
>> Hello,
>>
>> I have heard that this is still an issue in RK3288 support in mainline
>> so have given a look and wonder if you have considered having the
>> consumers of a shared clock (which could be a child clock) use
>> PRE_RATE_CHANGE notifications to adapt to changes in a parent's rate,
>> or to abort them if that's not possible.
>>
>> Alternatively, a consumer can set min and max constraints if it
>> doesn't want for others to mess with its provider, and let the other
>> consumer deal with failed rate changes (probably by choosing another
>> provider).
>>
>> Have I missed any problems with that?
>
> I don't think it's really that simple, but I certainly could be wrong.
>
> Summary of the system:
>
> * There are two VOPs (video processors) on rk3288: the big one and the
> little one.
>
> * The big VOP can handle higher resolutions than the little one but
> uses more power.
>
> * Both VOPs two can be sourced from CPLL, GPLL, or NPLL.
>
> * Both VOPs can be setup to process video for either HDMI, eDP, LVDS, ...
>
> * NPLL is the "new" PLL and is kinda like an extra PLL that's not used
> for any of the core peripherals.  It's intended to be one that's
> changed dynamically based on system needs.
>
> * GPLL appears intended (in current designs) to run at 594 MHz on
> rk3288 using a special "low jitter" setting.  This is intended to be
> the source clock for HDMI when running the very common 148.5 and 74.25
> rates.  It also it intended to be the source clock for various other
> peripherals like i2c, eMMC, SD, etc.  This also runs the GPU when the
> GPU is running at ~600MHz or ~300MHz.
>
> * CPLL appears intended (in current designs) to run at 400MHz.  There
> are some peripherals that run off this plus the GPU at 400MHz, 200MHz,
> or 100MHz.
>
>
> Let's imagine that we're booting up on a laptop where the builtin
> panel is hooked up via eDP.  Let's say that the panel really wants
> 76.42 MHz but that will work just fine with 74.25 MHz too.  If this is
> a system that has no other graphics needs, we'd want eDP to take NPLL
> and grab 76.42 MHz.  Make the best rate, right?
>
> ...but what about if it's a system with an HDMI port?  The eDP panel
> is built in and we know it works with 74.25 MHz, but the user could
> plug into all sorts of HDMI devices and they may require some very
> special PLL rates to get there.  So if we happen to take NPLL for eDP
> we'll need to know to switch away from it once something is plugged
> into HDMI.  We don't want to block the HDMI request since we know we
> can run eDP at 74.25 off GPLL.  Presumably we could see the HDMI
> request and then try to remux the eDP on the fly.  ...but what decides
> the HDMI is more important to eDP?  You might want different rules if
> the eDP port is somehow exposed and the user might plug different
> panels into it...

So we have a mechanism for detecting a conflict in the clock
hierarchy, and a mechanism to solve it, but we are missing a way for
userspace to communicate policy regarding which clocks should be given
priority when solving such a conflict?

Regards,

Tomeu

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Doug Anderson]

Hi,

On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> So we have a mechanism for detecting a conflict in the clock
> hierarchy, and a mechanism to solve it, but we are missing a way for
> userspace to communicate policy regarding which clocks should be given
> priority when solving such a conflict?

Hrmmm, I guess it could be userspace that makes the decision.  It does
seem a little odd to force it to userspace in all cases, though.  For
a particular laptop that is designed with a specific panel connected
up eDP it seems less than ideal to push this into userspace.  If the
kernel could just work in the expected sane way (or at least work that
way by default) it would be ideal.

If the kernel doesn't try to do anything sane by default then you're
creating a requirement for everyone's userspace to somehow figure this
out.  Do you expect there to be UI here, or that this would be
something that would be figured out by the Linux distribution?
Certainly exposing UI on something like a laptop with a builtin panel
wouldn't make any sense to me, but it might make sense if you had an
eval board with different display connectors on it.  If there's no UI,
would the Linux distribution need to somehow identify which board we
were on and then have a big lookup table about how to configure
things?

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Tomeu Vizoso]

On 21 January 2016 at 21:11, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>> So we have a mechanism for detecting a conflict in the clock
>> hierarchy, and a mechanism to solve it, but we are missing a way for
>> userspace to communicate policy regarding which clocks should be given
>> priority when solving such a conflict?
>
> Hrmmm, I guess it could be userspace that makes the decision.  It does
> seem a little odd to force it to userspace in all cases, though.  For
> a particular laptop that is designed with a specific panel connected
> up eDP it seems less than ideal to push this into userspace.  If the
> kernel could just work in the expected sane way (or at least work that
> way by default) it would be ideal.

Ah, I was wrongly assuming that the kernel didn't have enough
information to make an informed decision in this case, sorry.

Guess the per-user rate limits don't help here because the consumer
with higher priority could work with frequencies other than the ideal.

And we cannot have a consumer listening for PRE_RATE_CHANGE and
aborting unwanted changes or rerouting the ancestors of the clocks of
other consumers because that would be a massive violation of
separation of concerns.

If we were to rearrange the clock topology from within the CCF, then
consumers need to have a way to communicate to the core that they are
more important than other consumers. clk_set_important(clk, true)
could be enough in this case, but would be insufficient in more
complex cases where more than two clocks could use the same PLL.

> If the kernel doesn't try to do anything sane by default then you're
> creating a requirement for everyone's userspace to somehow figure this
> out.  Do you expect there to be UI here, or that this would be
> something that would be figured out by the Linux distribution?
> Certainly exposing UI on something like a laptop with a builtin panel
> wouldn't make any sense to me, but it might make sense if you had an
> eval board with different display connectors on it.  If there's no UI,
> would the Linux distribution need to somehow identify which board we
> were on and then have a big lookup table about how to configure
> things?

If we don't actually need input from userspace for this use case, I
wouldn't go this way right now, because it seems to me like it could
be a really big timesink for little gain.

Once someone comes with a situation in which feedback from userspace
is really needed, that person can propose such an interface ;)

Regards,

Tomeu

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Doug Anderson]

Tomeu,

On Fri, Jan 22, 2016 at 6:00 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> On 21 January 2016 at 21:11, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>> So we have a mechanism for detecting a conflict in the clock
>>> hierarchy, and a mechanism to solve it, but we are missing a way for
>>> userspace to communicate policy regarding which clocks should be given
>>> priority when solving such a conflict?
>>
>> Hrmmm, I guess it could be userspace that makes the decision.  It does
>> seem a little odd to force it to userspace in all cases, though.  For
>> a particular laptop that is designed with a specific panel connected
>> up eDP it seems less than ideal to push this into userspace.  If the
>> kernel could just work in the expected sane way (or at least work that
>> way by default) it would be ideal.
>
> Ah, I was wrongly assuming that the kernel didn't have enough
> information to make an informed decision in this case, sorry.
>
> Guess the per-user rate limits don't help here because the consumer
> with higher priority could work with frequencies other than the ideal.
>
> And we cannot have a consumer listening for PRE_RATE_CHANGE and
> aborting unwanted changes or rerouting the ancestors of the clocks of
> other consumers because that would be a massive violation of
> separation of concerns.
>
> If we were to rearrange the clock topology from within the CCF, then
> consumers need to have a way to communicate to the core that they are
> more important than other consumers. clk_set_important(clk, true)
> could be enough in this case, but would be insufficient in more
> complex cases where more than two clocks could use the same PLL.

With something like the above I'd still expect some complexity
depending on the probe order.  If a less important device grabs the
clock first, it might be forced to re-think its clocks later.  That
might be disconcerting.

OK, so I was just involved in a change recently that made me realize
that maybe our original problems were tied to the fact that our
builtin panels were trying to specify a clock that was impossible to
achieve with CPLL / GPLL.  It was a known problem that the request
would be denied and the CCF would just pick the closest rate it could.
Probably the right thing is to solve _that_ problem first.  If using
simple panel you could do a change like
<https://chromium-review.googlesource.com/#/c/323211/> (though
presumably you'd have to handle people using the same panel in other
laptops).  You might also be able to do funny things to fixup the mode
like dbehr tried to do in
<https://chromium-review.googlesource.com/#/c/270017/>.  By doing this
and making sure that

With that being said, how about this as a solution:

1. Figure some way (presumably some type of device tree property?) to
make sure that the builtin panel chose a clock that was achievable by
dividing CPLL / GPLL on rk3288.  If you have eDP but no builtin panel
you wouldn't specify this and you'd just let the system pick whatever
it wanted.

2. We use "assigned-clocks" to (by default) make NPLL something silly
and unattractive.  This will keep the common clock framework from
picking it to use as the source if the clock could be made just as
well from CPLL / GPLL (or find some other way to make NPLL a lower
priority muxing option).

3. We use the "dibs" rule in that the first display driver to claim
NPLL blocks anyone else from changing things.


In the above on a laptop with a builtin panel and an HDMI port you'd
get the panel using CPLL / GPLL and HDMI getting CPLL, GPLL or NPLL
(and it would be able to change NPLL if needed).


In the above on a dev board with a lot of ports then the first device
probed would get the most flexibility.  Future devices would be
limited in their choices.

-Doug

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Tomeu Vizoso]

On 22 January 2016 at 18:07, Doug Anderson <dianders@chromium.org> wrote:
> Tomeu,
>
> On Fri, Jan 22, 2016 at 6:00 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>> On 21 January 2016 at 21:11, Doug Anderson <dianders@chromium.org> wrote:
>>> Hi,
>>>
>>> On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>>> So we have a mechanism for detecting a conflict in the clock
>>>> hierarchy, and a mechanism to solve it, but we are missing a way for
>>>> userspace to communicate policy regarding which clocks should be given
>>>> priority when solving such a conflict?
>>>
>>> Hrmmm, I guess it could be userspace that makes the decision.  It does
>>> seem a little odd to force it to userspace in all cases, though.  For
>>> a particular laptop that is designed with a specific panel connected
>>> up eDP it seems less than ideal to push this into userspace.  If the
>>> kernel could just work in the expected sane way (or at least work that
>>> way by default) it would be ideal.
>>
>> Ah, I was wrongly assuming that the kernel didn't have enough
>> information to make an informed decision in this case, sorry.
>>
>> Guess the per-user rate limits don't help here because the consumer
>> with higher priority could work with frequencies other than the ideal.
>>
>> And we cannot have a consumer listening for PRE_RATE_CHANGE and
>> aborting unwanted changes or rerouting the ancestors of the clocks of
>> other consumers because that would be a massive violation of
>> separation of concerns.
>>
>> If we were to rearrange the clock topology from within the CCF, then
>> consumers need to have a way to communicate to the core that they are
>> more important than other consumers. clk_set_important(clk, true)
>> could be enough in this case, but would be insufficient in more
>> complex cases where more than two clocks could use the same PLL.
>
> With something like the above I'd still expect some complexity
> depending on the probe order.  If a less important device grabs the
> clock first, it might be forced to re-think its clocks later.  That
> might be disconcerting.

How much disconcerting do you think this could be? Hopefully those
devices should probe quite close to each other, right?

> OK, so I was just involved in a change recently that made me realize
> that maybe our original problems were tied to the fact that our
> builtin panels were trying to specify a clock that was impossible to
> achieve with CPLL / GPLL.  It was a known problem that the request
> would be denied and the CCF would just pick the closest rate it could.
> Probably the right thing is to solve _that_ problem first.  If using
> simple panel you could do a change like
> <https://chromium-review.googlesource.com/#/c/323211/> (though
> presumably you'd have to handle people using the same panel in other
> laptops).  You might also be able to do funny things to fixup the mode
> like dbehr tried to do in
> <https://chromium-review.googlesource.com/#/c/270017/>.  By doing this
> and making sure that

Are we missing something here?

> With that being said, how about this as a solution:
>
> 1. Figure some way (presumably some type of device tree property?) to
> make sure that the builtin panel chose a clock that was achievable by
> dividing CPLL / GPLL on rk3288.  If you have eDP but no builtin panel
> you wouldn't specify this and you'd just let the system pick whatever
> it wanted.
>
> 2. We use "assigned-clocks" to (by default) make NPLL something silly
> and unattractive.  This will keep the common clock framework from
> picking it to use as the source if the clock could be made just as
> well from CPLL / GPLL (or find some other way to make NPLL a lower
> priority muxing option).
>
> 3. We use the "dibs" rule in that the first display driver to claim
> NPLL blocks anyone else from changing things.
>
>
> In the above on a laptop with a builtin panel and an HDMI port you'd
> get the panel using CPLL / GPLL and HDMI getting CPLL, GPLL or NPLL
> (and it would be able to change NPLL if needed).
>
>
> In the above on a dev board with a lot of ports then the first device
> probed would get the most flexibility.  Future devices would be
> limited in their choices.

Not sure of this, sounds a bit ad-hoc and may be avoiding to address
limitations in the CCF that need to be solved anyway? So far it still
looks to me like the CCF needs to get better at allocating limited
resources.

Mike, Stephen, what do you think?

Thanks,

Tomeu

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Doug Anderson]

Tomeu,

On Tue, Jan 26, 2016 at 12:28 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> On 22 January 2016 at 18:07, Doug Anderson <dianders@chromium.org> wrote:
>> Tomeu,
>>
>> On Fri, Jan 22, 2016 at 6:00 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>> On 21 January 2016 at 21:11, Doug Anderson <dianders@chromium.org> wrote:
>>>> Hi,
>>>>
>>>> On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>>>> So we have a mechanism for detecting a conflict in the clock
>>>>> hierarchy, and a mechanism to solve it, but we are missing a way for
>>>>> userspace to communicate policy regarding which clocks should be given
>>>>> priority when solving such a conflict?
>>>>
>>>> Hrmmm, I guess it could be userspace that makes the decision.  It does
>>>> seem a little odd to force it to userspace in all cases, though.  For
>>>> a particular laptop that is designed with a specific panel connected
>>>> up eDP it seems less than ideal to push this into userspace.  If the
>>>> kernel could just work in the expected sane way (or at least work that
>>>> way by default) it would be ideal.
>>>
>>> Ah, I was wrongly assuming that the kernel didn't have enough
>>> information to make an informed decision in this case, sorry.
>>>
>>> Guess the per-user rate limits don't help here because the consumer
>>> with higher priority could work with frequencies other than the ideal.
>>>
>>> And we cannot have a consumer listening for PRE_RATE_CHANGE and
>>> aborting unwanted changes or rerouting the ancestors of the clocks of
>>> other consumers because that would be a massive violation of
>>> separation of concerns.
>>>
>>> If we were to rearrange the clock topology from within the CCF, then
>>> consumers need to have a way to communicate to the core that they are
>>> more important than other consumers. clk_set_important(clk, true)
>>> could be enough in this case, but would be insufficient in more
>>> complex cases where more than two clocks could use the same PLL.
>>
>> With something like the above I'd still expect some complexity
>> depending on the probe order.  If a less important device grabs the
>> clock first, it might be forced to re-think its clocks later.  That
>> might be disconcerting.
>
> How much disconcerting do you think this could be? Hopefully those
> devices should probe quite close to each other, right?

Probe: probably, though with defers it could be several seconds.

...but remember that display interfaces tend to be hotplug.  That
might mean that the HDMI interface won't try to set the clock until
much, much later.


>> OK, so I was just involved in a change recently that made me realize
>> that maybe our original problems were tied to the fact that our
>> builtin panels were trying to specify a clock that was impossible to
>> achieve with CPLL / GPLL.  It was a known problem that the request
>> would be denied and the CCF would just pick the closest rate it could.
>> Probably the right thing is to solve _that_ problem first.  If using
>> simple panel you could do a change like
>> <https://chromium-review.googlesource.com/#/c/323211/> (though
>> presumably you'd have to handle people using the same panel in other
>> laptops).  You might also be able to do funny things to fixup the mode
>> like dbehr tried to do in
>> <https://chromium-review.googlesource.com/#/c/270017/>.  By doing this
>> and making sure that
>
> Are we missing something here?

Eh?

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Tomeu Vizoso]

On 26 January 2016 at 17:32, Doug Anderson <dianders@chromium.org> wrote:
> Tomeu,
>
> On Tue, Jan 26, 2016 at 12:28 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>> On 22 January 2016 at 18:07, Doug Anderson <dianders@chromium.org> wrote:
>>> Tomeu,
>>>
>>> On Fri, Jan 22, 2016 at 6:00 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>>> On 21 January 2016 at 21:11, Doug Anderson <dianders@chromium.org> wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>>>>> So we have a mechanism for detecting a conflict in the clock
>>>>>> hierarchy, and a mechanism to solve it, but we are missing a way for
>>>>>> userspace to communicate policy regarding which clocks should be given
>>>>>> priority when solving such a conflict?
>>>>>
>>>>> Hrmmm, I guess it could be userspace that makes the decision.  It does
>>>>> seem a little odd to force it to userspace in all cases, though.  For
>>>>> a particular laptop that is designed with a specific panel connected
>>>>> up eDP it seems less than ideal to push this into userspace.  If the
>>>>> kernel could just work in the expected sane way (or at least work that
>>>>> way by default) it would be ideal.
>>>>
>>>> Ah, I was wrongly assuming that the kernel didn't have enough
>>>> information to make an informed decision in this case, sorry.
>>>>
>>>> Guess the per-user rate limits don't help here because the consumer
>>>> with higher priority could work with frequencies other than the ideal.
>>>>
>>>> And we cannot have a consumer listening for PRE_RATE_CHANGE and
>>>> aborting unwanted changes or rerouting the ancestors of the clocks of
>>>> other consumers because that would be a massive violation of
>>>> separation of concerns.
>>>>
>>>> If we were to rearrange the clock topology from within the CCF, then
>>>> consumers need to have a way to communicate to the core that they are
>>>> more important than other consumers. clk_set_important(clk, true)
>>>> could be enough in this case, but would be insufficient in more
>>>> complex cases where more than two clocks could use the same PLL.
>>>
>>> With something like the above I'd still expect some complexity
>>> depending on the probe order.  If a less important device grabs the
>>> clock first, it might be forced to re-think its clocks later.  That
>>> might be disconcerting.
>>
>> How much disconcerting do you think this could be? Hopefully those
>> devices should probe quite close to each other, right?
>
> Probe: probably, though with defers it could be several seconds.
>
> ...but remember that display interfaces tend to be hotplug.  That
> might mean that the HDMI interface won't try to set the clock until
> much, much later.

I'm still having trouble grasping what's the impact to users. Is it
that if HDMI gets the contended clock first and the internal panel
device only probes after a second or so, then the user may notice a
change in frequency in the HDMI screen when the panel lights up?

Though in that particular case I'm not sure if the impact is that big
for the user, wonder if such rearranging of the clock hierarchy can
cause bigger problems in other situations.

The on-demand probing series could help here because a downstream that
cared about these issues could just rearrange the contents of the DT
(maybe with a script as part of the build process) so that the panel
is always probed first, but well, that one collided with an iceberg.

>>> OK, so I was just involved in a change recently that made me realize
>>> that maybe our original problems were tied to the fact that our
>>> builtin panels were trying to specify a clock that was impossible to
>>> achieve with CPLL / GPLL.  It was a known problem that the request
>>> would be denied and the CCF would just pick the closest rate it could.
>>> Probably the right thing is to solve _that_ problem first.  If using
>>> simple panel you could do a change like
>>> <https://chromium-review.googlesource.com/#/c/323211/> (though
>>> presumably you'd have to handle people using the same panel in other
>>> laptops).  You might also be able to do funny things to fixup the mode
>>> like dbehr tried to do in
>>> <https://chromium-review.googlesource.com/#/c/270017/>.  By doing this
>>> and making sure that
>>
>> Are we missing something here?
>
> Eh?

The sentence above seemed to have been cut in the middle and I was
wondering if there's something relevant I'm missing because of it.

Thanks,

Tomeu

Re: [PATCH 0/5] clk: rockchip: add full support for HDMI clock on rk3288

[Doug Anderson]

Tomeu,

On Wed, Jan 27, 2016 at 2:20 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> On 26 January 2016 at 17:32, Doug Anderson <dianders@chromium.org> wrote:
>> Tomeu,
>>
>> On Tue, Jan 26, 2016 at 12:28 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>> On 22 January 2016 at 18:07, Doug Anderson <dianders@chromium.org> wrote:
>>>> Tomeu,
>>>>
>>>> On Fri, Jan 22, 2016 at 6:00 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>>>> On 21 January 2016 at 21:11, Doug Anderson <dianders@chromium.org> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Jan 21, 2016 at 1:03 AM, Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>>>>>> So we have a mechanism for detecting a conflict in the clock
>>>>>>> hierarchy, and a mechanism to solve it, but we are missing a way for
>>>>>>> userspace to communicate policy regarding which clocks should be given
>>>>>>> priority when solving such a conflict?
>>>>>>
>>>>>> Hrmmm, I guess it could be userspace that makes the decision.  It does
>>>>>> seem a little odd to force it to userspace in all cases, though.  For
>>>>>> a particular laptop that is designed with a specific panel connected
>>>>>> up eDP it seems less than ideal to push this into userspace.  If the
>>>>>> kernel could just work in the expected sane way (or at least work that
>>>>>> way by default) it would be ideal.
>>>>>
>>>>> Ah, I was wrongly assuming that the kernel didn't have enough
>>>>> information to make an informed decision in this case, sorry.
>>>>>
>>>>> Guess the per-user rate limits don't help here because the consumer
>>>>> with higher priority could work with frequencies other than the ideal.
>>>>>
>>>>> And we cannot have a consumer listening for PRE_RATE_CHANGE and
>>>>> aborting unwanted changes or rerouting the ancestors of the clocks of
>>>>> other consumers because that would be a massive violation of
>>>>> separation of concerns.
>>>>>
>>>>> If we were to rearrange the clock topology from within the CCF, then
>>>>> consumers need to have a way to communicate to the core that they are
>>>>> more important than other consumers. clk_set_important(clk, true)
>>>>> could be enough in this case, but would be insufficient in more
>>>>> complex cases where more than two clocks could use the same PLL.
>>>>
>>>> With something like the above I'd still expect some complexity
>>>> depending on the probe order.  If a less important device grabs the
>>>> clock first, it might be forced to re-think its clocks later.  That
>>>> might be disconcerting.
>>>
>>> How much disconcerting do you think this could be? Hopefully those
>>> devices should probe quite close to each other, right?
>>
>> Probe: probably, though with defers it could be several seconds.
>>
>> ...but remember that display interfaces tend to be hotplug.  That
>> might mean that the HDMI interface won't try to set the clock until
>> much, much later.
>
> I'm still having trouble grasping what's the impact to users. Is it
> that if HDMI gets the contended clock first and the internal panel
> device only probes after a second or so, then the user may notice a
> change in frequency in the HDMI screen when the panel lights up?
>
> Though in that particular case I'm not sure if the impact is that big
> for the user, wonder if such rearranging of the clock hierarchy can
> cause bigger problems in other situations.
>
> The on-demand probing series could help here because a downstream that
> cared about these issues could just rearrange the contents of the DT
> (maybe with a script as part of the build process) so that the panel
> is always probed first, but well, that one collided with an iceberg.

Problem I'm imagining is this one:

1. EDP (used for builtin panel) is setup by userspace.  It requests a
pixel clock that's not quite achievable from CPLL / GPLL.  Let's say
it requests 67 MHz.  GPLL can make 66 MHz (594 / 9) and CPLL can make
66.67 (400 / 6).  ...but NPLL is sitting right there, so let's make
things exact and set NPLL to 1.608 GHz (67 MHz * 24).  Now we get
exactly 67 MHz.

2. An hour after bootup, the user plugs in HDMI.  The user wants to
use a 78.8 MHz pixel clock.

Option #1: deny HDMI because CPLL / GPLL are fixed and NPLL is in use
by eDP.  Not great for HDMI support.

Option #2: switch EDP away from NPLL to CPLL.  This presumably will
require disabling / re-enabling eDP and isn't super amazing, but could
be done if need be.  Since user space might care about the eDP refresh
rate (so it can properly schedule video frames) and this will tweak
the refresh rate by a bit, this might be cleanest as a full "unplug"
of eDP and re-plug of eDP.

Option #3: Somehow ahead of time know that on this particular device
that eDP shouldn't use NPLL.

--

The whole situation would presumably be different if eDP wasn't used
for the builtin panel.  Technically eDP can be hotplugged (I think).
If we're on a board where eDP could be hotplugged to different
monitors then all the rules might be different.


>>>> OK, so I was just involved in a change recently that made me realize
>>>> that maybe our original problems were tied to the fact that our
>>>> builtin panels were trying to specify a clock that was impossible to
>>>> achieve with CPLL / GPLL.  It was a known problem that the request
>>>> would be denied and the CCF would just pick the closest rate it could.
>>>> Probably the right thing is to solve _that_ problem first.  If using
>>>> simple panel you could do a change like
>>>> <https://chromium-review.googlesource.com/#/c/323211/> (though
>>>> presumably you'd have to handle people using the same panel in other
>>>> laptops).  You might also be able to do funny things to fixup the mode
>>>> like dbehr tried to do in
>>>> <https://chromium-review.googlesource.com/#/c/270017/>.  By doing this
>>>> and making sure that
>>>
>>> Are we missing something here?
>>
>> Eh?
>
> The sentence above seemed to have been cut in the middle and I was
> wondering if there's something relevant I'm missing because of it.

On the current systems eDP will actually request 72.5 MHz, which is
definitely not achievable from CPLL / GPLL.  You can try redoing the
exercise above where eDP requests 72.5 MHz if you want, but suffice to
say that the eDP refresh rate will change more drastically if it goes
from 72.5 MHz to 66.67 MHz.