[PATCH v3 0/8] ARM: Rockchip: add cpuclk handling - clock-tree part

[PATCH v3 0/8] ARM: Rockchip: add cpuclk handling - clock-tree part

[Heiko Stuebner]

This series implements cpu frequency-scaling for Rockchip SoCs.
The whole handling of the armclk frequency changes and therefore
the implementention is very similar to the recent series for
Samsung SoCs from Thomas Abraham.

Tested on a
- rk3066 Marsboard
- rk3188 Radxa Rock
- rk3288 Evaluation board

This is the part that would go through the clock tree and the patches
are build against current clk-next.

-------
On the arm-soc side we would need the ARMCLK id in an exported branch
from the clk tree so that the devicetree changes can use this new id.
-------

The series also is split, as clk-next and the arm-soc parts have diverged
so much, that it's not possible to create one series against one tree
and stay sane.

changes since v2:
- add patch removing the pll clk-notifier
- in cpuclk do parent change and alt_div setting in one go
changes since v1:
- address comments received concerning smaller issues
- split series into two, one for clk-next and one for arm-soc

Doug Anderson (1):
  clk: rockchip: change pll rate without a clk-notifier

Heiko Stuebner (6):
  clk: rockchip: fix rk3066 pll status register location
  clk: rockchip: reparent aclk_cpu_pre to the gpll
  clk: rockchip: make tightly bound armclk child-clocks read-only
  clk: rockchip: add new clock-type for the cpuclk
  clk: rockchip: add binding id for ARMCLK
  clk: rockchip: switch to using the new cpuclk type for armclk

Jianqun (1):
  clk: rockchip: fix rk3288 pll status register location

 drivers/clk/rockchip/Makefile                 |   1 +
 drivers/clk/rockchip/clk-cpu.c                | 330 ++++++++++++++++++++++++++
 drivers/clk/rockchip/clk-pll.c                |  63 +----
 drivers/clk/rockchip/clk-rk3188.c             | 161 +++++++++++--
 drivers/clk/rockchip/clk-rk3288.c             |  93 +++++++-
 drivers/clk/rockchip/clk.c                    |  20 ++
 drivers/clk/rockchip/clk.h                    |  36 +++
 include/dt-bindings/clock/rk3188-cru-common.h |   1 +
 include/dt-bindings/clock/rk3288-cru.h        |   1 +
 9 files changed, 628 insertions(+), 78 deletions(-)
 create mode 100644 drivers/clk/rockchip/clk-cpu.c

-- 
2.0.1

[PATCH v3 1/8] clk: rockchip: change pll rate without a clk-notifier

[Heiko Stuebner]

From: Doug Anderson <dianders@chromium.org>

The Rockchip PLL code switches into slow mode (AKA bypass more AKA
24MHz mode) before actually changing the PLL.  This keeps anyone from
using the PLL while it's changing.  However, in all known Rockchip
SoCs nobody should ever see the 24MHz when changing the PLL supplying
the armclk because we should reparent children to an alternate
(faster than 24MHz) PLL.

One problem is that the code to switch to an alternate parent was
running in PRE_RATE_CHANGE.  ...and the code to switch to slow mode
was _also_ running in PRE_RATE_CHANGE.  That meant there was no real
guarantee that we would switch to an alternate parent before switching
to 24MHz mode.

Let's move the switch to "slow mode" straight into
rockchip_rk3066_pll_set_rate().  That means we're guaranteed that the
24MHz is really a last-resort.

Note that without this change on real systems we were the code to
switch to an alternate parent at 24MHz.  In some older versions of
that code we'd appy a (temporary) / 5 to the 24MHz causing us to run
at 4.8MHz.  That wasn't enough to service USB interrupts in some cases
and could lead to a system hang.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-pll.c | 63 +++++++++---------------------------------
 1 file changed, 13 insertions(+), 50 deletions(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index f2a1c7a..a3e886a 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -34,7 +34,6 @@ struct rockchip_clk_pll {
 	const struct clk_ops	*pll_mux_ops;
 
 	struct notifier_block	clk_nb;
-	bool			rate_change_remuxed;
 
 	void __iomem		*reg_base;
 	int			lock_offset;
@@ -109,38 +108,6 @@ static int rockchip_pll_wait_lock(struct rockchip_clk_pll *pll)
 }
 
 /**
- * Set pll mux when changing the pll rate.
- * This makes sure to move the pll mux away from the actual pll before
- * changing its rate and back to the original parent after the change.
- */
-static int rockchip_pll_notifier_cb(struct notifier_block *nb,
-					unsigned long event, void *data)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll_nb(nb);
-	struct clk_mux *pll_mux = &pll->pll_mux;
-	const struct clk_ops *pll_mux_ops = pll->pll_mux_ops;
-	int cur_parent;
-
-	switch (event) {
-	case PRE_RATE_CHANGE:
-		cur_parent = pll_mux_ops->get_parent(&pll_mux->hw);
-		if (cur_parent == PLL_MODE_NORM) {
-			pll_mux_ops->set_parent(&pll_mux->hw, PLL_MODE_SLOW);
-			pll->rate_change_remuxed = 1;
-		}
-		break;
-	case POST_RATE_CHANGE:
-		if (pll->rate_change_remuxed) {
-			pll_mux_ops->set_parent(&pll_mux->hw, PLL_MODE_NORM);
-			pll->rate_change_remuxed = 0;
-		}
-		break;
-	}
-
-	return NOTIFY_OK;
-}
-
-/**
  * PLL used in RK3066, RK3188 and RK3288
  */
 
@@ -194,6 +161,10 @@ static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 	const struct rockchip_pll_rate_table *rate;
 	unsigned long old_rate = rockchip_rk3066_pll_recalc_rate(hw, prate);
 	struct regmap *grf = rockchip_clk_get_grf();
+	struct clk_mux *pll_mux = &pll->pll_mux;
+	const struct clk_ops *pll_mux_ops = pll->pll_mux_ops;
+	int rate_change_remuxed = 0;
+	int cur_parent;
 	int ret;
 
 	if (IS_ERR(grf)) {
@@ -216,6 +187,12 @@ static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 	pr_debug("%s: rate settings for %lu (nr, no, nf): (%d, %d, %d)\n",
 		 __func__, rate->rate, rate->nr, rate->no, rate->nf);
 
+	cur_parent = pll_mux_ops->get_parent(&pll_mux->hw);
+	if (cur_parent == PLL_MODE_NORM) {
+		pll_mux_ops->set_parent(&pll_mux->hw, PLL_MODE_SLOW);
+		rate_change_remuxed = 1;
+	}
+
 	/* enter reset mode */
 	writel(HIWORD_UPDATE(RK3066_PLLCON3_RESET, RK3066_PLLCON3_RESET, 0),
 	       pll->reg_base + RK3066_PLLCON(3));
@@ -247,6 +224,9 @@ static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 		rockchip_rk3066_pll_set_rate(hw, old_rate, prate);
 	}
 
+	if (rate_change_remuxed)
+		pll_mux_ops->set_parent(&pll_mux->hw, PLL_MODE_NORM);
+
 	return ret;
 }
 
@@ -310,7 +290,6 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 	struct clk_mux *pll_mux;
 	struct clk *pll_clk, *mux_clk;
 	char pll_name[20];
-	int ret;
 
 	if (num_parents != 2) {
 		pr_err("%s: needs two parent clocks\n", __func__);
@@ -367,7 +346,6 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 	pll->lock_offset = grf_lock_offset;
 	pll->lock_shift = lock_shift;
 	pll->lock = lock;
-	pll->clk_nb.notifier_call = rockchip_pll_notifier_cb;
 
 	pll_clk = clk_register(NULL, &pll->hw);
 	if (IS_ERR(pll_clk)) {
@@ -377,14 +355,6 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 		goto err_pll;
 	}
 
-	ret = clk_notifier_register(pll_clk, &pll->clk_nb);
-	if (ret) {
-		pr_err("%s: failed to register clock notifier for %s : %d\n",
-				__func__, name, ret);
-		mux_clk = ERR_PTR(ret);
-		goto err_pll_notifier;
-	}
-
 	/* create the mux on top of the real pll */
 	pll->pll_mux_ops = &clk_mux_ops;
 	pll_mux = &pll->pll_mux;
@@ -417,13 +387,6 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 	return mux_clk;
 
 err_mux:
-	ret = clk_notifier_unregister(pll_clk, &pll->clk_nb);
-	if (ret) {
-		pr_err("%s: could not unregister clock notifier in error path : %d\n",
-		       __func__, ret);
-		return mux_clk;
-	}
-err_pll_notifier:
 	clk_unregister(pll_clk);
 err_pll:
 	kfree(pll);
-- 
2.0.1

[PATCH v3 2/8] clk: rockchip: fix rk3066 pll status register location

[Heiko Stuebner]

The register providing the pll lock status is at a different address on the
rk3066. The error became apparent while working on cpufreq support for
the rockchip SoCs.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3188.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3188.c b/drivers/clk/rockchip/clk-rk3188.c
index 0147614..1c5e644 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -19,6 +19,7 @@
 #include <dt-bindings/clock/rk3188-cru-common.h>
 #include "clk.h"
 
+#define RK3066_GRF_SOC_STATUS	0x15c
 #define RK3188_GRF_SOC_STATUS	0xac
 
 enum rk3188_plls {
@@ -629,9 +630,6 @@ static void __init rk3188_common_clk_init(struct device_node *np)
 		pr_warn("%s: could not register clock usb480m: %ld\n",
 			__func__, PTR_ERR(clk));
 
-	rockchip_clk_register_plls(rk3188_pll_clks,
-				   ARRAY_SIZE(rk3188_pll_clks),
-				   RK3188_GRF_SOC_STATUS);
 	rockchip_clk_register_branches(common_clk_branches,
 				  ARRAY_SIZE(common_clk_branches));
 	rockchip_clk_protect_critical(rk3188_critical_clocks,
@@ -644,6 +642,9 @@ static void __init rk3188_common_clk_init(struct device_node *np)
 static void __init rk3066a_clk_init(struct device_node *np)
 {
 	rk3188_common_clk_init(np);
+	rockchip_clk_register_plls(rk3188_pll_clks,
+				   ARRAY_SIZE(rk3188_pll_clks),
+				   RK3066_GRF_SOC_STATUS);
 	rockchip_clk_register_branches(rk3066a_clk_branches,
 				  ARRAY_SIZE(rk3066a_clk_branches));
 }
@@ -652,6 +653,9 @@ CLK_OF_DECLARE(rk3066a_cru, "rockchip,rk3066a-cru", rk3066a_clk_init);
 static void __init rk3188a_clk_init(struct device_node *np)
 {
 	rk3188_common_clk_init(np);
+	rockchip_clk_register_plls(rk3188_pll_clks,
+				   ARRAY_SIZE(rk3188_pll_clks),
+				   RK3188_GRF_SOC_STATUS);
 	rockchip_clk_register_branches(rk3188_clk_branches,
 				  ARRAY_SIZE(rk3188_clk_branches));
 }
-- 
2.0.1

[PATCH v3 3/8] clk: rockchip: fix rk3288 pll status register location

[Heiko Stuebner]

From: Jianqun <jay.xu@rock-chips.com>

In RK3288, APLL lock status bit is in GRF_SOC_STATUS1,
but in RK3188, is GRFSOC_STATUS0.

Signed-off-by: Jianqun <jay.xu@rock-chips.com>

Also name the constant accordingly as GRF_SOC_STATUS1
to prevent confusion.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-rk3288.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index aa4f4b7..b30e147 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -20,7 +20,7 @@
 #include "clk.h"
 
 #define RK3288_GRF_SOC_CON(x)	(0x244 + x * 4)
-#define RK3288_GRF_SOC_STATUS	0x280
+#define RK3288_GRF_SOC_STATUS1	0x284
 
 enum rk3288_plls {
 	apll, dpll, cpll, gpll, npll,
@@ -713,7 +713,7 @@ static void __init rk3288_clk_init(struct device_node *np)
 
 	rockchip_clk_register_plls(rk3288_pll_clks,
 				   ARRAY_SIZE(rk3288_pll_clks),
-				   RK3288_GRF_SOC_STATUS);
+				   RK3288_GRF_SOC_STATUS1);
 	rockchip_clk_register_branches(rk3288_clk_branches,
 				  ARRAY_SIZE(rk3288_clk_branches));
 	rockchip_clk_protect_critical(rk3288_critical_clocks,
-- 
2.0.1

[PATCH v3 4/8] clk: rockchip: reparent aclk_cpu_pre to the gpll

[Heiko Stuebner]

aclk_cpu_pre on the rk3188 can either be sourced from the armclk or the gpll.
To reduce complexity on apll changes caused by cpufreq, reparent it always
to the gpll source.

If really necessary it could be reparented back on a per board level using
the assigned-clocks mechanism.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3188.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3188.c b/drivers/clk/rockchip/clk-rk3188.c
index 1c5e644..6a81bc9 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -652,12 +652,33 @@ CLK_OF_DECLARE(rk3066a_cru, "rockchip,rk3066a-cru", rk3066a_clk_init);
 
 static void __init rk3188a_clk_init(struct device_node *np)
 {
+	struct clk *clk1, *clk2;
+	unsigned long rate;
+	int ret;
+
 	rk3188_common_clk_init(np);
 	rockchip_clk_register_plls(rk3188_pll_clks,
 				   ARRAY_SIZE(rk3188_pll_clks),
 				   RK3188_GRF_SOC_STATUS);
 	rockchip_clk_register_branches(rk3188_clk_branches,
 				  ARRAY_SIZE(rk3188_clk_branches));
+
+	/* reparent aclk_cpu_pre from apll */
+	clk1 = __clk_lookup("aclk_cpu_pre");
+	clk2 = __clk_lookup("gpll");
+	if (clk1 && clk2) {
+		rate = clk_get_rate(clk1);
+
+		ret = clk_set_parent(clk1, clk2);
+		if (ret < 0)
+			pr_warn("%s: could not reparent aclk_cpu_pre to gpll\n",
+				__func__);
+
+		clk_set_rate(clk1, rate);
+	} else {
+		pr_warn("%s: missing clocks to reparent aclk_cpu_pre to gpll\n",
+			__func__);
+	}
 }
 CLK_OF_DECLARE(rk3188a_cru, "rockchip,rk3188a-cru", rk3188a_clk_init);
 
-- 
2.0.1

[PATCH v3 5/8] clk: rockchip: make tightly bound armclk child-clocks read-only

[Heiko Stuebner]

Rockchip SoCs contain clocks tightly bound to the armclk, where the best
rate / divider is supplied by the vendor after careful measuring.
Often this ideal rate may be greater than the current rate.

Therefore prevent the ccf from trying to set these dividers itself by
setting them to read-only.

In the case of the rk3066, this also includes the aclk_cpu, which makes it
necessary to also split its direct child-clocks (pclk_cpu, hclk_cpu, ...)
into individual definitions for rk3066 and rk3188.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-rk3188.c | 26 ++++++++++++++++++--------
 drivers/clk/rockchip/clk-rk3288.c | 18 +++++++++---------
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3188.c b/drivers/clk/rockchip/clk-rk3188.c
index 6a81bc9..adfbfef 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -174,17 +174,10 @@ static struct rockchip_clk_branch common_clk_branches[] __initdata = {
 	GATE(0, "aclk_cpu", "aclk_cpu_pre", 0,
 			RK2928_CLKGATE_CON(0), 3, GFLAGS),
 
-	DIV(0, "pclk_cpu_pre", "aclk_cpu_pre", 0,
-			RK2928_CLKSEL_CON(1), 12, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
 	GATE(0, "atclk_cpu", "pclk_cpu_pre", 0,
 			RK2928_CLKGATE_CON(0), 6, GFLAGS),
 	GATE(0, "pclk_cpu", "pclk_cpu_pre", 0,
 			RK2928_CLKGATE_CON(0), 5, GFLAGS),
-	DIV(0, "hclk_cpu_pre", "aclk_cpu_pre", 0,
-			RK2928_CLKSEL_CON(1), 8, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
-	COMPOSITE_NOMUX(0, "hclk_ahb2apb", "hclk_cpu_pre", 0,
-			RK2928_CLKSEL_CON(1), 14, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO,
-			RK2928_CLKGATE_CON(4), 9, GFLAGS),
 	GATE(0, "hclk_cpu", "hclk_cpu_pre", 0,
 			RK2928_CLKGATE_CON(0), 4, GFLAGS),
 
@@ -416,7 +409,17 @@ static struct rockchip_clk_branch rk3066a_clk_branches[] __initdata = {
 	COMPOSITE_NOGATE(0, "armclk", mux_armclk_p, 0,
 			RK2928_CLKSEL_CON(0), 8, 1, MFLAGS, 0, 5, DFLAGS),
 	DIVTBL(0, "aclk_cpu_pre", "armclk", 0,
-			RK2928_CLKSEL_CON(1), 0, 3, DFLAGS, div_aclk_cpu_t),
+			RK2928_CLKSEL_CON(1), 0, 3, DFLAGS | CLK_DIVIDER_READ_ONLY, div_aclk_cpu_t),
+	DIV(0, "pclk_cpu_pre", "aclk_cpu_pre", 0,
+			RK2928_CLKSEL_CON(1), 12, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO
+							    | CLK_DIVIDER_READ_ONLY),
+	DIV(0, "hclk_cpu_pre", "aclk_cpu_pre", 0,
+			RK2928_CLKSEL_CON(1), 8, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO
+							   | CLK_DIVIDER_READ_ONLY),
+	COMPOSITE_NOMUX(0, "hclk_ahb2apb", "hclk_cpu_pre", 0,
+			RK2928_CLKSEL_CON(1), 14, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO
+							    | CLK_DIVIDER_READ_ONLY,
+			RK2928_CLKGATE_CON(4), 9, GFLAGS),
 
 	GATE(CORE_L2C, "core_l2c", "aclk_cpu", 0,
 			RK2928_CLKGATE_CON(9), 4, GFLAGS),
@@ -534,6 +537,13 @@ static struct rockchip_clk_branch rk3188_clk_branches[] __initdata = {
 	/* do not source aclk_cpu_pre from the apll, to keep complexity down */
 	COMPOSITE_NOGATE(0, "aclk_cpu_pre", mux_aclk_cpu_p, CLK_SET_RATE_NO_REPARENT,
 			RK2928_CLKSEL_CON(0), 5, 1, MFLAGS, 0, 5, DFLAGS),
+	DIV(0, "pclk_cpu_pre", "aclk_cpu_pre", 0,
+			RK2928_CLKSEL_CON(1), 12, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
+	DIV(0, "hclk_cpu_pre", "aclk_cpu_pre", 0,
+			RK2928_CLKSEL_CON(1), 8, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
+	COMPOSITE_NOMUX(0, "hclk_ahb2apb", "hclk_cpu_pre", 0,
+			RK2928_CLKSEL_CON(1), 14, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO,
+			RK2928_CLKGATE_CON(4), 9, GFLAGS),
 
 	GATE(CORE_L2C, "core_l2c", "armclk", 0,
 			RK2928_CLKGATE_CON(9), 4, GFLAGS),
diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index b30e147..1d3036d 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -170,31 +170,31 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 			RK3288_CLKSEL_CON(0), 15, 1, MFLAGS, 8, 5, DFLAGS),
 
 	COMPOSITE_NOMUX(0, "armcore0", "armclk", 0,
-			RK3288_CLKSEL_CON(36), 0, 3, DFLAGS,
+			RK3288_CLKSEL_CON(36), 0, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 0, GFLAGS),
 	COMPOSITE_NOMUX(0, "armcore1", "armclk", 0,
-			RK3288_CLKSEL_CON(36), 4, 3, DFLAGS,
+			RK3288_CLKSEL_CON(36), 4, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 1, GFLAGS),
 	COMPOSITE_NOMUX(0, "armcore2", "armclk", 0,
-			RK3288_CLKSEL_CON(36), 8, 3, DFLAGS,
+			RK3288_CLKSEL_CON(36), 8, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 2, GFLAGS),
 	COMPOSITE_NOMUX(0, "armcore3", "armclk", 0,
-			RK3288_CLKSEL_CON(36), 12, 3, DFLAGS,
+			RK3288_CLKSEL_CON(36), 12, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 3, GFLAGS),
 	COMPOSITE_NOMUX(0, "l2ram", "armclk", 0,
-			RK3288_CLKSEL_CON(37), 0, 3, DFLAGS,
+			RK3288_CLKSEL_CON(37), 0, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 4, GFLAGS),
 	COMPOSITE_NOMUX(0, "aclk_core_m0", "armclk", 0,
-			RK3288_CLKSEL_CON(0), 0, 4, DFLAGS,
+			RK3288_CLKSEL_CON(0), 0, 4, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 5, GFLAGS),
 	COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", 0,
-			RK3288_CLKSEL_CON(0), 4, 4, DFLAGS,
+			RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 6, GFLAGS),
 	COMPOSITE_NOMUX(0, "atclk", "armclk", 0,
-			RK3288_CLKSEL_CON(37), 4, 5, DFLAGS,
+			RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 7, GFLAGS),
 	COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", 0,
-			RK3288_CLKSEL_CON(37), 9, 5, DFLAGS,
+			RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			RK3288_CLKGATE_CON(12), 8, GFLAGS),
 	GATE(0, "pclk_dbg", "pclk_dbg_pre", 0,
 			RK3288_CLKGATE_CON(12), 9, GFLAGS),
-- 
2.0.1

[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk

[Heiko Stuebner]

When changing the armclk on Rockchip SoCs it is supposed to be reparented
to an alternate parent before changing the underlying pll and back after
the change. Additionally there exist clocks that are very tightly bound to
the armclk whose divider values are set according to the armclk rate.

Add a special clock-type to handle all that. The rate table and divider
values will be supplied from the soc-specific clock controllers.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/Makefile  |   1 +
 drivers/clk/rockchip/clk-cpu.c | 330 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/rockchip/clk.c     |  20 +++
 drivers/clk/rockchip/clk.h     |  36 +++++
 4 files changed, 387 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-cpu.c

diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
index ee6b077..bd8514d 100644
--- a/drivers/clk/rockchip/Makefile
+++ b/drivers/clk/rockchip/Makefile
@@ -5,6 +5,7 @@
 obj-y	+= clk-rockchip.o
 obj-y	+= clk.o
 obj-y	+= clk-pll.o
+obj-y	+= clk-cpu.o
 obj-$(CONFIG_RESET_CONTROLLER)	+= softrst.o
 
 obj-y	+= clk-rk3188.o
diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c
new file mode 100644
index 0000000..8f0aaeb
--- /dev/null
+++ b/drivers/clk/rockchip/clk-cpu.c
@@ -0,0 +1,330 @@
+/*
+ * Copyright (c) 2014 MundoReader S.L.
+ * Author: Heiko Stuebner <heiko@sntech.de>
+ *
+ * based on clk/samsung/clk-cpu.c
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Thomas Abraham <thomas.ab@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This file contains the utility function to register CPU clock for Samsung
+ * Exynos platforms. A CPU clock is defined as a clock supplied to a CPU or a
+ * group of CPUs. The CPU clock is typically derived from a hierarchy of clock
+ * blocks which includes mux and divider blocks. There are a number of other
+ * auxiliary clocks supplied to the CPU domain such as the debug blocks and AXI
+ * clock for CPU domain. The rates of these auxiliary clocks are related to the
+ * CPU clock rate and this relation is usually specified in the hardware manual
+ * of the SoC or supplied after the SoC characterization.
+ *
+ * The below implementation of the CPU clock allows the rate changes of the CPU
+ * clock and the corresponding rate changes of the auxillary clocks of the CPU
+ * domain. The platform clock driver provides a clock register configuration
+ * for each configurable rate which is then used to program the clock hardware
+ * registers to acheive a fast co-oridinated rate change for all the CPU domain
+ * clocks.
+ *
+ * On a rate change request for the CPU clock, the rate change is propagated
+ * upto the PLL supplying the clock to the CPU domain clock blocks. While the
+ * CPU domain PLL is reconfigured, the CPU domain clocks are driven using an
+ * alternate clock source. If required, the alternate clock source is divided
+ * down in order to keep the output clock rate within the previous OPP limits.
+ */
+
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk-provider.h>
+#include "clk.h"
+
+/**
+ * struct rockchip_cpuclk: information about clock supplied to a CPU core.
+ * @hw:		handle between ccf and cpu clock.
+ * @alt_parent:	alternate parent clock to use when switching the speed
+ *		of the primary parent clock.
+ * @reg_base:	base register for cpu-clock values.
+ * @clk_nb:	clock notifier registered for changes in clock speed of the
+ *		primary parent clock.
+ * @rate_count:	number of rates in the rate_table
+ * @rate_table:	pll-rates and their associated dividers
+ * @reg_data:	cpu-specific register settings
+ * @lock:	clock lock
+ */
+struct rockchip_cpuclk {
+	struct clk_hw				hw;
+
+	struct clk_mux				cpu_mux;
+	const struct clk_ops			*cpu_mux_ops;
+
+	struct clk				*alt_parent;
+	void __iomem				*reg_base;
+	struct notifier_block			clk_nb;
+	unsigned int				rate_count;
+	struct rockchip_cpuclk_rate_table	*rate_table;
+	const struct rockchip_cpuclk_reg_data	*reg_data;
+	spinlock_t				*lock;
+};
+
+#define to_rockchip_cpuclk_hw(hw) container_of(hw, struct rockchip_cpuclk, hw)
+#define to_rockchip_cpuclk_nb(nb) \
+			container_of(nb, struct rockchip_cpuclk, clk_nb)
+
+static const struct rockchip_cpuclk_rate_table *rockchip_get_cpuclk_settings(
+			    struct rockchip_cpuclk *cpuclk, unsigned long rate)
+{
+	const struct rockchip_cpuclk_rate_table *rate_table =
+							cpuclk->rate_table;
+	int i;
+
+	for (i = 0; i < cpuclk->rate_count; i++) {
+		if (rate == rate_table[i].prate)
+			return &rate_table[i];
+	}
+
+	return NULL;
+}
+
+static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_hw(hw);
+	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
+	u32 clksel0 = readl_relaxed(cpuclk->reg_base + reg_data->core_reg);
+
+	clksel0 >>= reg_data->div_core_shift;
+	clksel0 &= reg_data->div_core_mask;
+	return parent_rate / (clksel0 + 1);
+}
+
+static const struct clk_ops rockchip_cpuclk_ops = {
+	.recalc_rate = rockchip_cpuclk_recalc_rate,
+};
+
+static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
+					   struct clk_notifier_data *ndata)
+{
+	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
+	const struct rockchip_cpuclk_rate_table *rate;
+	unsigned long alt_prate, alt_div;
+	int i;
+
+	rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for cpuclk\n",
+		       __func__, ndata->new_rate);
+		return -EINVAL;
+	}
+
+	alt_prate = clk_get_rate(cpuclk->alt_parent);
+
+	spin_lock(cpuclk->lock);
+
+	/*
+	 * If the old parent clock speed is less than the clock speed
+	 * of the alternate parent, then it should be ensured that at no point
+	 * the armclk speed is more than the old_rate until the dividers are
+	 * set.
+	 */
+	if (alt_prate > ndata->old_rate) {
+		/* calculate dividers */
+		alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
+		if (alt_div > reg_data->div_core_mask) {
+			pr_warn("%s: limiting alt-divider %lu to %d\n",
+				__func__, alt_div, reg_data->div_core_mask);
+			alt_div = reg_data->div_core_mask;
+		}
+
+		/*
+		 * Change parents and add dividers in a single transaction.
+		 *
+		 * NOTE: we do this in a single transaction so we're never
+		 * dividing the primary parent by the extra dividers that were
+		 * needed for the alt.
+		 */
+		pr_debug("%s: setting div %lu as alt-rate %lu > old-rate %lu\n",
+			 __func__, alt_div, alt_prate, ndata->old_rate);
+
+		writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
+					      reg_data->div_core_shift) |
+		       HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
+		       cpuclk->reg_base + reg_data->core_reg);
+	} else {
+		/* select alternate parent */
+		writel(HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
+			cpuclk->reg_base + reg_data->core_reg);
+	}
+
+	/* alternate parent is active now. set the dividers */
+	for (i = 0; i < ARRAY_SIZE(rate->divs); i++) {
+		const struct rockchip_cpuclk_clksel *clksel = &rate->divs[i];
+
+		if (!clksel->reg)
+			continue;
+
+		pr_debug("%s: setting reg 0x%x to 0x%x\n",
+			 __func__, clksel->reg, clksel->val);
+		writel(clksel->val , cpuclk->reg_base + clksel->reg);
+	}
+
+	spin_unlock(cpuclk->lock);
+	return 0;
+}
+
+static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
+					    struct clk_notifier_data *ndata)
+{
+	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
+
+	spin_lock(cpuclk->lock);
+
+	/*
+	 * post-rate change event, re-mux to primary parent and remove dividers.
+	 *
+	 * NOTE: we do this in a single transaction so we're never dividing the
+	 * primary parent by the extra dividers that were needed for the alt.
+	 */
+
+	writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
+				reg_data->div_core_shift) |
+	       HIWORD_UPDATE(0, 1, reg_data->mux_core_shift),
+	       cpuclk->reg_base + reg_data->core_reg);
+
+	spin_unlock(cpuclk->lock);
+	return 0;
+}
+
+/*
+ * This clock notifier is called when the frequency of the parent clock
+ * of cpuclk is to be changed. This notifier handles the setting up all
+ * the divider clocks, remux to temporary parent and handling the safe
+ * frequency levels when using temporary parent.
+ */
+static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
+	int ret = 0;
+
+	pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
+		 __func__, event, ndata->old_rate, ndata->new_rate);
+	if (event == PRE_RATE_CHANGE)
+		ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
+	else if (event == POST_RATE_CHANGE)
+		ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
+
+	return notifier_from_errno(ret);
+}
+
+struct clk *rockchip_clk_register_cpuclk(const char *name,
+			const char **parent_names, u8 num_parents,
+			const struct rockchip_cpuclk_reg_data *reg_data,
+			struct rockchip_cpuclk_rate_table *rate_table,
+			void __iomem *reg_base, spinlock_t *lock)
+{
+	struct rockchip_cpuclk *cpuclk;
+	struct clk_init_data init;
+	struct clk *clk, *cclk;
+	int ret;
+
+	if (!reg_data) {
+		pr_err("%s: no soc register information\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (num_parents != 2) {
+		pr_err("%s: needs two parent clocks\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
+	if (!cpuclk)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.parent_names = &parent_names[0];
+	init.num_parents = 1;
+	init.ops = &rockchip_cpuclk_ops;
+
+	/* only allow rate changes when we have a rate table */
+	init.flags = rate_table ? CLK_SET_RATE_PARENT : 0;
+
+	/* disallow automatic parent changes by ccf */
+	init.flags |= CLK_SET_RATE_NO_REPARENT;
+
+	init.flags |= CLK_GET_RATE_NOCACHE;
+
+	cpuclk->reg_base = reg_base;
+	cpuclk->lock = lock;
+	cpuclk->reg_data = reg_data;
+	cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
+	cpuclk->hw.init = &init;
+
+	cpuclk->alt_parent = __clk_lookup(parent_names[1]);
+	if (!cpuclk->alt_parent) {
+		pr_err("%s: could not lookup alternate parent\n",
+		       __func__);
+		ret = -EINVAL;
+		goto free_cpuclk;
+	}
+
+	ret = clk_prepare_enable(cpuclk->alt_parent);
+	if (ret) {
+		pr_err("%s: could not enable alternate parent\n",
+		       __func__);
+		goto free_cpuclk;
+	}
+
+	clk = __clk_lookup(parent_names[0]);
+	if (!clk) {
+		pr_err("%s: could not lookup parent clock %s\n",
+		       __func__, parent_names[0]);
+		ret = -EINVAL;
+		goto free_cpuclk;
+	}
+
+	ret = clk_notifier_register(clk, &cpuclk->clk_nb);
+	if (ret) {
+		pr_err("%s: failed to register clock notifier for %s\n",
+				__func__, name);
+		goto free_cpuclk;
+	}
+
+	if (rate_table) {
+		int nrates;
+
+		/* find count of rates in rate_table */
+		for (nrates = 0; rate_table[nrates].prate != 0; )
+			nrates++;
+
+		cpuclk->rate_count = nrates;
+		cpuclk->rate_table = kmemdup(rate_table,
+					     sizeof(*rate_table) * nrates,
+					     GFP_KERNEL);
+		if (!cpuclk->rate_table) {
+			pr_err("%s: could not allocate memory for cpuclk rates\n",
+			       __func__);
+			ret = -ENOMEM;
+			goto unregister_notifier;
+		}
+	}
+
+	cclk = clk_register(NULL, &cpuclk->hw);
+	if (IS_ERR(clk)) {
+		pr_err("%s: could not register cpuclk %s\n", __func__,	name);
+		ret = PTR_ERR(clk);
+		goto free_rate_table;
+	}
+
+	return cclk;
+
+free_rate_table:
+	kfree(cpuclk->rate_table);
+unregister_notifier:
+	clk_notifier_unregister(clk, &cpuclk->clk_nb);
+free_cpuclk:
+	kfree(cpuclk);
+	return ERR_PTR(ret);
+}
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index d9c6db2..f87ac4a 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -297,6 +297,26 @@ void __init rockchip_clk_register_branches(
 	}
 }
 
+void __init rockchip_clk_register_armclk(unsigned int lookup_id,
+			const char *name, const char **parent_names,
+			u8 num_parents,
+			const struct rockchip_cpuclk_reg_data *reg_data,
+			struct rockchip_cpuclk_rate_table *rate_table)
+{
+	struct clk *clk;
+
+	clk = rockchip_clk_register_cpuclk(name, parent_names, num_parents,
+					   reg_data, rate_table, reg_base,
+					   &clk_lock);
+	if (IS_ERR(clk)) {
+		pr_err("%s: failed to register clock %s: %ld\n",
+		       __func__, name, PTR_ERR(clk));
+		return;
+	}
+
+	rockchip_clk_add_lookup(clk, lookup_id);
+}
+
 void __init rockchip_clk_protect_critical(const char *clocks[], int nclocks)
 {
 	int i;
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 2b0bca1..e0ea61e 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -120,6 +120,38 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 		struct rockchip_pll_rate_table *rate_table,
 		spinlock_t *lock);
 
+struct rockchip_cpuclk_clksel {
+	int reg;
+	u32 val;
+};
+
+#define ROCKCHIP_CPUCLK_NUM_DIVIDERS	2
+struct rockchip_cpuclk_rate_table {
+	unsigned long prate;
+	struct rockchip_cpuclk_clksel divs[ROCKCHIP_CPUCLK_NUM_DIVIDERS];
+};
+
+/**
+ * struct rockchip_cpuclk_reg_data: describes register offsets and masks of the cpuclock
+ * @core_reg:		register offset of the core settings register
+ * @div_core_shift:	core divider offset used to divide the pll value
+ * @div_core_mask:	core divider mask
+ * @mux_core_shift:	offset of the core multiplexer
+ */
+struct rockchip_cpuclk_reg_data {
+	int		core_reg;
+	u8		div_core_shift;
+	u32		div_core_mask;
+	int		mux_core_reg;
+	u8		mux_core_shift;
+};
+
+struct clk *rockchip_clk_register_cpuclk(const char *name,
+			const char **parent_names, u8 num_parents,
+			const struct rockchip_cpuclk_reg_data *reg_data,
+			struct rockchip_cpuclk_rate_table *rate_table,
+			void __iomem *reg_base, spinlock_t *lock);
+
 #define PNAME(x) static const char *x[] __initconst
 
 enum rockchip_clk_branch_type {
@@ -329,6 +361,10 @@ void rockchip_clk_register_branches(struct rockchip_clk_branch *clk_list,
 				    unsigned int nr_clk);
 void rockchip_clk_register_plls(struct rockchip_pll_clock *pll_list,
 				unsigned int nr_pll, int grf_lock_offset);
+void rockchip_clk_register_armclk(unsigned int lookup_id, const char *name,
+			const char **parent_names, u8 num_parents,
+			const struct rockchip_cpuclk_reg_data *reg_data,
+			struct rockchip_cpuclk_rate_table *rate_table);
 void rockchip_clk_protect_critical(const char *clocks[], int nclocks);
 
 #define ROCKCHIP_SOFTRST_HIWORD_MASK	BIT(0)
-- 
2.0.1

[PATCH v3 7/8] clk: rockchip: add binding id for ARMCLK

[Heiko Stuebner]

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 include/dt-bindings/clock/rk3188-cru-common.h | 1 +
 include/dt-bindings/clock/rk3288-cru.h        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/dt-bindings/clock/rk3188-cru-common.h b/include/dt-bindings/clock/rk3188-cru-common.h
index 750ee60..6a37050 100644
--- a/include/dt-bindings/clock/rk3188-cru-common.h
+++ b/include/dt-bindings/clock/rk3188-cru-common.h
@@ -20,6 +20,7 @@
 #define PLL_GPLL		4
 #define CORE_PERI		5
 #define CORE_L2C		6
+#define ARMCLK			7
 
 /* sclk gates (special clocks) */
 #define SCLK_UART0		64
diff --git a/include/dt-bindings/clock/rk3288-cru.h b/include/dt-bindings/clock/rk3288-cru.h
index ebcb460..8f75bf0 100644
--- a/include/dt-bindings/clock/rk3288-cru.h
+++ b/include/dt-bindings/clock/rk3288-cru.h
@@ -19,6 +19,7 @@
 #define PLL_CPLL		3
 #define PLL_GPLL		4
 #define PLL_NPLL		5
+#define ARMCLK			6
 
 /* sclk gates (special clocks) */
 #define SCLK_GPU		64
-- 
2.0.1

[PATCH v3 8/8] clk: rockchip: switch to using the new cpuclk type for armclk

[Heiko Stuebner]

This adds the necessary soc-specific divider values and switches the armclk
to use the newly introduced cpuclk type.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3188.c | 104 ++++++++++++++++++++++++++++++++++++--
 drivers/clk/rockchip/clk-rk3288.c |  71 +++++++++++++++++++++++++-
 2 files changed, 169 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3188.c b/drivers/clk/rockchip/clk-rk3188.c
index adfbfef..db1a815 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -101,6 +101,100 @@ struct rockchip_pll_rate_table rk3188_pll_rates[] = {
 	{ /* sentinel */ },
 };
 
+#define RK3066_DIV_CORE_PERIPH_MASK	0x3
+#define RK3066_DIV_CORE_PERIPH_SHIFT	6
+#define RK3066_DIV_ACLK_CORE_MASK	0x7
+#define RK3066_DIV_ACLK_CORE_SHIFT	0
+#define RK3066_DIV_ACLK_HCLK_MASK	0x3
+#define RK3066_DIV_ACLK_HCLK_SHIFT	8
+#define RK3066_DIV_ACLK_PCLK_MASK	0x3
+#define RK3066_DIV_ACLK_PCLK_SHIFT	12
+#define RK3066_DIV_AHB2APB_MASK		0x3
+#define RK3066_DIV_AHB2APB_SHIFT	14
+
+#define RK3066_CLKSEL0(_core_peri)					\
+	{								\
+		.reg = RK2928_CLKSEL_CON(0),				\
+		.val = HIWORD_UPDATE(_core_peri, RK3066_DIV_CORE_PERIPH_MASK, \
+				RK3066_DIV_CORE_PERIPH_SHIFT)		\
+	}
+#define RK3066_CLKSEL1(_aclk_core, _aclk_hclk, _aclk_pclk, _ahb2apb)	\
+	{								\
+		.reg = RK2928_CLKSEL_CON(1),				\
+		.val = HIWORD_UPDATE(_aclk_core, RK3066_DIV_ACLK_CORE_MASK, \
+				RK3066_DIV_ACLK_CORE_SHIFT) |		\
+		       HIWORD_UPDATE(_aclk_hclk, RK3066_DIV_ACLK_HCLK_MASK, \
+				RK3066_DIV_ACLK_HCLK_SHIFT) |		\
+		       HIWORD_UPDATE(_aclk_pclk, RK3066_DIV_ACLK_PCLK_MASK, \
+				RK3066_DIV_ACLK_PCLK_SHIFT) |		\
+		       HIWORD_UPDATE(_ahb2apb, RK3066_DIV_AHB2APB_MASK,	\
+				RK3066_DIV_AHB2APB_SHIFT),		\
+	}
+
+#define RK3066_CPUCLK_RATE(_prate, _core_peri, _acore, _ahclk, _apclk, _h2p) \
+	{								\
+		.prate = _prate,					\
+		.divs = {						\
+			RK3066_CLKSEL0(_core_peri),			\
+			RK3066_CLKSEL1(_acore, _ahclk, _apclk, _h2p),	\
+		},							\
+	}
+
+static struct rockchip_cpuclk_rate_table rk3066_cpuclk_rates[] = {
+	RK3066_CPUCLK_RATE(1416000000, 2, 3, 1, 2, 1),
+	RK3066_CPUCLK_RATE(1200000000, 2, 3, 1, 2, 1),
+	RK3066_CPUCLK_RATE(1008000000, 2, 2, 1, 2, 1),
+	RK3066_CPUCLK_RATE( 816000000, 2, 2, 1, 2, 1),
+	RK3066_CPUCLK_RATE( 600000000, 1, 2, 1, 2, 1),
+	RK3066_CPUCLK_RATE( 504000000, 1, 1, 1, 2, 1),
+	RK3066_CPUCLK_RATE( 312000000, 0, 1, 1, 1, 0),
+	{ /* sentinel */ },
+};
+
+static const struct rockchip_cpuclk_reg_data rk3066_cpuclk_data = {
+	.core_reg = RK2928_CLKSEL_CON(0),
+	.div_core_shift = 0,
+	.div_core_mask = 0x1f,
+	.mux_core_shift = 8,
+};
+
+#define RK3188_DIV_ACLK_CORE_MASK	0x7
+#define RK3188_DIV_ACLK_CORE_SHIFT	3
+
+#define RK3188_CLKSEL1(_aclk_core)		\
+	{					\
+		.reg = RK2928_CLKSEL_CON(1),	\
+		.val = HIWORD_UPDATE(_aclk_core, RK3188_DIV_ACLK_CORE_MASK,\
+				 RK3188_DIV_ACLK_CORE_SHIFT) \
+	}
+#define RK3188_CPUCLK_RATE(_prate, _core_peri, _aclk_core)	\
+	{							\
+		.prate = _prate,				\
+		.divs = {					\
+			RK3066_CLKSEL0(_core_peri),		\
+			RK3188_CLKSEL1(_aclk_core),		\
+		},						\
+	}
+
+static struct rockchip_cpuclk_rate_table rk3188_cpuclk_rates[] = {
+	RK3188_CPUCLK_RATE(1608000000, 2, 3),
+	RK3188_CPUCLK_RATE(1416000000, 2, 3),
+	RK3188_CPUCLK_RATE(1200000000, 2, 3),
+	RK3188_CPUCLK_RATE(1008000000, 2, 3),
+	RK3188_CPUCLK_RATE( 816000000, 2, 3),
+	RK3188_CPUCLK_RATE( 600000000, 1, 3),
+	RK3188_CPUCLK_RATE( 504000000, 1, 3),
+	RK3188_CPUCLK_RATE( 312000000, 0, 1),
+	{ /* sentinel */ },
+};
+
+static const struct rockchip_cpuclk_reg_data rk3188_cpuclk_data = {
+	.core_reg = RK2928_CLKSEL_CON(0),
+	.div_core_shift = 9,
+	.div_core_mask = 0x1f,
+	.mux_core_shift = 8,
+};
+
 PNAME(mux_pll_p)		= { "xin24m", "xin32k" };
 PNAME(mux_armclk_p)		= { "apll", "gpll_armclk" };
 PNAME(mux_ddrphy_p)		= { "dpll", "gpll_ddr" };
@@ -406,8 +500,6 @@ static struct clk_div_table div_aclk_cpu_t[] = {
 };
 
 static struct rockchip_clk_branch rk3066a_clk_branches[] __initdata = {
-	COMPOSITE_NOGATE(0, "armclk", mux_armclk_p, 0,
-			RK2928_CLKSEL_CON(0), 8, 1, MFLAGS, 0, 5, DFLAGS),
 	DIVTBL(0, "aclk_cpu_pre", "armclk", 0,
 			RK2928_CLKSEL_CON(1), 0, 3, DFLAGS | CLK_DIVIDER_READ_ONLY, div_aclk_cpu_t),
 	DIV(0, "pclk_cpu_pre", "aclk_cpu_pre", 0,
@@ -528,8 +620,6 @@ PNAME(mux_hsicphy_p)		= { "sclk_otgphy0", "sclk_otgphy1",
 				    "gpll", "cpll" };
 
 static struct rockchip_clk_branch rk3188_clk_branches[] __initdata = {
-	COMPOSITE_NOGATE(0, "armclk", mux_armclk_p, 0,
-			RK2928_CLKSEL_CON(0), 8, 1, MFLAGS, 9, 5, DFLAGS),
 	COMPOSITE_NOMUX_DIVTBL(0, "aclk_core", "armclk", 0,
 			RK2928_CLKSEL_CON(1), 3, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
 			div_rk3188_aclk_core_t, RK2928_CLKGATE_CON(0), 7, GFLAGS),
@@ -657,6 +747,9 @@ static void __init rk3066a_clk_init(struct device_node *np)
 				   RK3066_GRF_SOC_STATUS);
 	rockchip_clk_register_branches(rk3066a_clk_branches,
 				  ARRAY_SIZE(rk3066a_clk_branches));
+	rockchip_clk_register_armclk(ARMCLK, "armclk",
+			mux_armclk_p, ARRAY_SIZE(mux_armclk_p),
+			&rk3066_cpuclk_data, rk3066_cpuclk_rates);
 }
 CLK_OF_DECLARE(rk3066a_cru, "rockchip,rk3066a-cru", rk3066a_clk_init);
 
@@ -672,6 +765,9 @@ static void __init rk3188a_clk_init(struct device_node *np)
 				   RK3188_GRF_SOC_STATUS);
 	rockchip_clk_register_branches(rk3188_clk_branches,
 				  ARRAY_SIZE(rk3188_clk_branches));
+	rockchip_clk_register_armclk(ARMCLK, "armclk",
+				  mux_armclk_p, ARRAY_SIZE(mux_armclk_p),
+				  &rk3188_cpuclk_data, rk3188_cpuclk_rates);
 
 	/* reparent aclk_cpu_pre from apll */
 	clk1 = __clk_lookup("aclk_cpu_pre");
diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 1d3036d..5fa7986 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -101,6 +101,71 @@ struct rockchip_pll_rate_table rk3288_pll_rates[] = {
 	{ /* sentinel */ },
 };
 
+#define RK3288_DIV_ACLK_CORE_M0_MASK	0xf
+#define RK3288_DIV_ACLK_CORE_M0_SHIFT	0
+#define RK3288_DIV_ACLK_CORE_MP_MASK	0xf
+#define RK3288_DIV_ACLK_CORE_MP_SHIFT	4
+#define RK3288_DIV_L2RAM_MASK		0x7
+#define RK3288_DIV_L2RAM_SHIFT		0
+#define RK3288_DIV_ATCLK_MASK		0x1f
+#define RK3288_DIV_ATCLK_SHIFT		4
+#define RK3288_DIV_PCLK_DBGPRE_MASK	0x1f
+#define RK3288_DIV_PCLK_DBGPRE_SHIFT	9
+
+#define RK3288_CLKSEL0(_core_m0, _core_mp)				\
+	{								\
+		.reg = RK3288_CLKSEL_CON(0),				\
+		.val = HIWORD_UPDATE(_core_m0, RK3288_DIV_ACLK_CORE_M0_MASK, \
+				RK3288_DIV_ACLK_CORE_M0_SHIFT) |	\
+		       HIWORD_UPDATE(_core_mp, RK3288_DIV_ACLK_CORE_MP_MASK, \
+				RK3288_DIV_ACLK_CORE_MP_SHIFT),		\
+	}
+#define RK3288_CLKSEL37(_l2ram, _atclk, _pclk_dbg_pre)			\
+	{								\
+		.reg = RK3288_CLKSEL_CON(37),				\
+		.val = HIWORD_UPDATE(_l2ram, RK3288_DIV_L2RAM_MASK,	\
+				RK3288_DIV_L2RAM_SHIFT) |		\
+		       HIWORD_UPDATE(_atclk, RK3288_DIV_ATCLK_MASK,	\
+				RK3288_DIV_ATCLK_SHIFT) |		\
+		       HIWORD_UPDATE(_pclk_dbg_pre,			\
+				RK3288_DIV_PCLK_DBGPRE_MASK,		\
+				RK3288_DIV_PCLK_DBGPRE_SHIFT),		\
+	}
+
+#define RK3288_CPUCLK_RATE(_prate, _core_m0, _core_mp, _l2ram, _atclk, _pdbg) \
+	{								\
+		.prate = _prate,					\
+		.divs = {						\
+			RK3288_CLKSEL0(_core_m0, _core_mp),		\
+			RK3288_CLKSEL37(_l2ram, _atclk, _pdbg),		\
+		},							\
+	}
+
+static struct rockchip_cpuclk_rate_table rk3288_cpuclk_rates[] = {
+	RK3288_CPUCLK_RATE(1800000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE(1704000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE(1608000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE(1512000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE(1416000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE(1200000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE(1008000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 816000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 696000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 600000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 408000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 312000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 216000000, 2, 4, 2, 4, 4),
+	RK3288_CPUCLK_RATE( 126000000, 2, 4, 2, 4, 4),
+	{ /* sentinel */ },
+};
+
+static const struct rockchip_cpuclk_reg_data rk3288_cpuclk_data = {
+	.core_reg = RK3288_CLKSEL_CON(0),
+	.div_core_shift = 8,
+	.div_core_mask = 0x1f,
+	.mux_core_shift = 15,
+};
+
 PNAME(mux_pll_p)		= { "xin24m", "xin32k" };
 PNAME(mux_armclk_p)		= { "apll_core", "gpll_core" };
 PNAME(mux_ddrphy_p)		= { "dpll_ddr", "gpll_ddr" };
@@ -166,8 +231,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 			RK3288_CLKGATE_CON(0), 1, GFLAGS),
 	GATE(0, "gpll_core", "gpll", 0,
 			RK3288_CLKGATE_CON(0), 2, GFLAGS),
-	COMPOSITE_NOGATE(0, "armclk", mux_armclk_p, 0,
-			RK3288_CLKSEL_CON(0), 15, 1, MFLAGS, 8, 5, DFLAGS),
 
 	COMPOSITE_NOMUX(0, "armcore0", "armclk", 0,
 			RK3288_CLKSEL_CON(36), 0, 3, DFLAGS | CLK_DIVIDER_READ_ONLY,
@@ -719,6 +782,10 @@ static void __init rk3288_clk_init(struct device_node *np)
 	rockchip_clk_protect_critical(rk3288_critical_clocks,
 				      ARRAY_SIZE(rk3288_critical_clocks));
 
+	rockchip_clk_register_armclk(ARMCLK, "armclk",
+			mux_armclk_p, ARRAY_SIZE(mux_armclk_p),
+			&rk3288_cpuclk_data, rk3288_cpuclk_rates);
+
 	rockchip_register_softrst(np, 9, reg_base + RK3288_SOFTRST_CON(0),
 				  ROCKCHIP_SOFTRST_HIWORD_MASK);
 }
-- 
2.0.1

[PATCH v3 1/8] clk: rockchip: change pll rate without a clk-notifier

[Doug Anderson]

Heiko,

On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> From: Doug Anderson <dianders@chromium.org>
>
> The Rockchip PLL code switches into slow mode (AKA bypass more AKA
> 24MHz mode) before actually changing the PLL.  This keeps anyone from
> using the PLL while it's changing.  However, in all known Rockchip
> SoCs nobody should ever see the 24MHz when changing the PLL supplying
> the armclk because we should reparent children to an alternate
> (faster than 24MHz) PLL.
>
> One problem is that the code to switch to an alternate parent was
> running in PRE_RATE_CHANGE.  ...and the code to switch to slow mode
> was _also_ running in PRE_RATE_CHANGE.  That meant there was no real
> guarantee that we would switch to an alternate parent before switching
> to 24MHz mode.
>
> Let's move the switch to "slow mode" straight into
> rockchip_rk3066_pll_set_rate().  That means we're guaranteed that the
> 24MHz is really a last-resort.
>
> Note that without this change on real systems we were the code to
> switch to an alternate parent at 24MHz.  In some older versions of
> that code we'd appy a (temporary) / 5 to the 24MHz causing us to run
> at 4.8MHz.  That wasn't enough to service USB interrupts in some cases
> and could lead to a system hang.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/rockchip/clk-pll.c | 63 +++++++++---------------------------------
>  1 file changed, 13 insertions(+), 50 deletions(-)

Thanks for adding my patch to your series (with the proper commit
message)!  I think you need your SoB on the patch too.  Andrew Morton
pointed to the docs in another patch I was involved in.  Specifically,
you were "on the patch delivery".  See Documentation/SubmittingPatches
section 12 (and 13).

-Doug

[PATCH v3 1/8] clk: rockchip: change pll rate without a clk-notifier

[Heiko Stübner]

Am Mittwoch, 17. September 2014, 15:46:08 schrieb Doug Anderson:
> Heiko,
> 
> On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > From: Doug Anderson <dianders@chromium.org>
> > 
> > The Rockchip PLL code switches into slow mode (AKA bypass more AKA
> > 24MHz mode) before actually changing the PLL.  This keeps anyone from
> > using the PLL while it's changing.  However, in all known Rockchip
> > SoCs nobody should ever see the 24MHz when changing the PLL supplying
> > the armclk because we should reparent children to an alternate
> > (faster than 24MHz) PLL.
> > 
> > One problem is that the code to switch to an alternate parent was
> > running in PRE_RATE_CHANGE.  ...and the code to switch to slow mode
> > was _also_ running in PRE_RATE_CHANGE.  That meant there was no real
> > guarantee that we would switch to an alternate parent before switching
> > to 24MHz mode.
> > 
> > Let's move the switch to "slow mode" straight into
> > rockchip_rk3066_pll_set_rate().  That means we're guaranteed that the
> > 24MHz is really a last-resort.
> > 
> > Note that without this change on real systems we were the code to
> > switch to an alternate parent at 24MHz.  In some older versions of
> > that code we'd appy a (temporary) / 5 to the 24MHz causing us to run
> > at 4.8MHz.  That wasn't enough to service USB interrupts in some cases
> > and could lead to a system hang.
> > 
> > Signed-off-by: Doug Anderson <dianders@chromium.org>
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/rockchip/clk-pll.c | 63
> >  +++++++++--------------------------------- 1 file changed, 13
> >  insertions(+), 50 deletions(-)
> 
> Thanks for adding my patch to your series (with the proper commit
> message)!  I think you need your SoB on the patch too.  Andrew Morton
> pointed to the docs in another patch I was involved in.  Specifically,
> you were "on the patch delivery".  See Documentation/SubmittingPatches
> section 12 (and 13).

ok ... Mike can you add the

Signed-off-by: Heiko Stuebner <heiko@sntech.de>

to the patch, or do you want a respin [if no other issue appears]


Heiko

[PATCH v3 7/8] clk: rockchip: add binding id for ARMCLK

[Doug Anderson]

Heiko,

On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  include/dt-bindings/clock/rk3188-cru-common.h | 1 +
>  include/dt-bindings/clock/rk3288-cru.h        | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk

[Doug Anderson]

Heiko,

On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> When changing the armclk on Rockchip SoCs it is supposed to be reparented
> to an alternate parent before changing the underlying pll and back after
> the change. Additionally there exist clocks that are very tightly bound to
> the armclk whose divider values are set according to the armclk rate.
>
> Add a special clock-type to handle all that. The rate table and divider
> values will be supplied from the soc-specific clock controllers.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/rockchip/Makefile  |   1 +
>  drivers/clk/rockchip/clk-cpu.c | 330 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/rockchip/clk.c     |  20 +++
>  drivers/clk/rockchip/clk.h     |  36 +++++
>  4 files changed, 387 insertions(+)
>  create mode 100644 drivers/clk/rockchip/clk-cpu.c
>
> diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
> index ee6b077..bd8514d 100644
> --- a/drivers/clk/rockchip/Makefile
> +++ b/drivers/clk/rockchip/Makefile
> @@ -5,6 +5,7 @@
>  obj-y  += clk-rockchip.o
>  obj-y  += clk.o
>  obj-y  += clk-pll.o
> +obj-y  += clk-cpu.o
>  obj-$(CONFIG_RESET_CONTROLLER) += softrst.o
>
>  obj-y  += clk-rk3188.o
> diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c
> new file mode 100644
> index 0000000..8f0aaeb
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-cpu.c
> @@ -0,0 +1,330 @@
> +/*
> + * Copyright (c) 2014 MundoReader S.L.
> + * Author: Heiko Stuebner <heiko@sntech.de>
> + *
> + * based on clk/samsung/clk-cpu.c
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Author: Thomas Abraham <thomas.ab@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This file contains the utility function to register CPU clock for Samsung

I think on the last review someone pointed out that this part of the
comment is wrong.  Perhaps you could remove it or fix it?
Specifically, this is not for Samsung Exynos...

> + * Exynos platforms. A CPU clock is defined as a clock supplied to a CPU or a
> + * group of CPUs. The CPU clock is typically derived from a hierarchy of clock
> + * blocks which includes mux and divider blocks. There are a number of other
> + * auxiliary clocks supplied to the CPU domain such as the debug blocks and AXI
> + * clock for CPU domain. The rates of these auxiliary clocks are related to the
> + * CPU clock rate and this relation is usually specified in the hardware manual
> + * of the SoC or supplied after the SoC characterization.
> + *
> + * The below implementation of the CPU clock allows the rate changes of the CPU
> + * clock and the corresponding rate changes of the auxillary clocks of the CPU
> + * domain. The platform clock driver provides a clock register configuration
> + * for each configurable rate which is then used to program the clock hardware
> + * registers to acheive a fast co-oridinated rate change for all the CPU domain
> + * clocks.
> + *
> + * On a rate change request for the CPU clock, the rate change is propagated
> + * upto the PLL supplying the clock to the CPU domain clock blocks. While the
> + * CPU domain PLL is reconfigured, the CPU domain clocks are driven using an
> + * alternate clock source. If required, the alternate clock source is divided
> + * down in order to keep the output clock rate within the previous OPP limits.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk-provider.h>
> +#include "clk.h"
> +
> +/**
> + * struct rockchip_cpuclk: information about clock supplied to a CPU core.
> + * @hw:                handle between ccf and cpu clock.
> + * @alt_parent:        alternate parent clock to use when switching the speed
> + *             of the primary parent clock.
> + * @reg_base:  base register for cpu-clock values.
> + * @clk_nb:    clock notifier registered for changes in clock speed of the
> + *             primary parent clock.
> + * @rate_count:        number of rates in the rate_table
> + * @rate_table:        pll-rates and their associated dividers
> + * @reg_data:  cpu-specific register settings
> + * @lock:      clock lock
> + */
> +struct rockchip_cpuclk {
> +       struct clk_hw                           hw;
> +
> +       struct clk_mux                          cpu_mux;
> +       const struct clk_ops                    *cpu_mux_ops;
> +
> +       struct clk                              *alt_parent;
> +       void __iomem                            *reg_base;
> +       struct notifier_block                   clk_nb;
> +       unsigned int                            rate_count;
> +       struct rockchip_cpuclk_rate_table       *rate_table;
> +       const struct rockchip_cpuclk_reg_data   *reg_data;
> +       spinlock_t                              *lock;
> +};
> +
> +#define to_rockchip_cpuclk_hw(hw) container_of(hw, struct rockchip_cpuclk, hw)
> +#define to_rockchip_cpuclk_nb(nb) \
> +                       container_of(nb, struct rockchip_cpuclk, clk_nb)
> +
> +static const struct rockchip_cpuclk_rate_table *rockchip_get_cpuclk_settings(
> +                           struct rockchip_cpuclk *cpuclk, unsigned long rate)
> +{
> +       const struct rockchip_cpuclk_rate_table *rate_table =
> +                                                       cpuclk->rate_table;
> +       int i;
> +
> +       for (i = 0; i < cpuclk->rate_count; i++) {
> +               if (rate == rate_table[i].prate)
> +                       return &rate_table[i];
> +       }
> +
> +       return NULL;
> +}
> +
> +static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long parent_rate)
> +{
> +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_hw(hw);
> +       const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> +       u32 clksel0 = readl_relaxed(cpuclk->reg_base + reg_data->core_reg);
> +
> +       clksel0 >>= reg_data->div_core_shift;
> +       clksel0 &= reg_data->div_core_mask;
> +       return parent_rate / (clksel0 + 1);
> +}
> +
> +static const struct clk_ops rockchip_cpuclk_ops = {
> +       .recalc_rate = rockchip_cpuclk_recalc_rate,
> +};
> +
> +static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
> +                                          struct clk_notifier_data *ndata)
> +{
> +       const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> +       const struct rockchip_cpuclk_rate_table *rate;
> +       unsigned long alt_prate, alt_div;
> +       int i;
> +
> +       rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
> +       if (!rate) {
> +               pr_err("%s: Invalid rate : %lu for cpuclk\n",
> +                      __func__, ndata->new_rate);
> +               return -EINVAL;
> +       }
> +
> +       alt_prate = clk_get_rate(cpuclk->alt_parent);
> +
> +       spin_lock(cpuclk->lock);
> +
> +       /*
> +        * If the old parent clock speed is less than the clock speed
> +        * of the alternate parent, then it should be ensured that at no point
> +        * the armclk speed is more than the old_rate until the dividers are
> +        * set.
> +        */
> +       if (alt_prate > ndata->old_rate) {
> +               /* calculate dividers */
> +               alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
> +               if (alt_div > reg_data->div_core_mask) {
> +                       pr_warn("%s: limiting alt-divider %lu to %d\n",
> +                               __func__, alt_div, reg_data->div_core_mask);
> +                       alt_div = reg_data->div_core_mask;
> +               }
> +
> +               /*
> +                * Change parents and add dividers in a single transaction.
> +                *
> +                * NOTE: we do this in a single transaction so we're never
> +                * dividing the primary parent by the extra dividers that were
> +                * needed for the alt.
> +                */
> +               pr_debug("%s: setting div %lu as alt-rate %lu > old-rate %lu\n",
> +                        __func__, alt_div, alt_prate, ndata->old_rate);
> +
> +               writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
> +                                             reg_data->div_core_shift) |
> +                      HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
> +                      cpuclk->reg_base + reg_data->core_reg);
> +       } else {
> +               /* select alternate parent */
> +               writel(HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
> +                       cpuclk->reg_base + reg_data->core_reg);
> +       }
> +
> +       /* alternate parent is active now. set the dividers */
> +       for (i = 0; i < ARRAY_SIZE(rate->divs); i++) {
> +               const struct rockchip_cpuclk_clksel *clksel = &rate->divs[i];
> +
> +               if (!clksel->reg)
> +                       continue;
> +
> +               pr_debug("%s: setting reg 0x%x to 0x%x\n",
> +                        __func__, clksel->reg, clksel->val);
> +               writel(clksel->val , cpuclk->reg_base + clksel->reg);
> +       }

I'm not 100% certain that you can always set the dividers here and
still be safe.

Let's imagine that we're going 800MHz => 300MHz.  Let's say that we
have "divide by 3" for 800Mhz and "divide by 1" for 300Mhz.  Let's say
that the alternate PLL is 500Mhz.

...so at this point in the transition we've switched to the alternate
PLL (500Mhz) but we're now setting the dividers for 300MHz.  That
doesn't seem quite right.


I also _think_ that these dividers are based on the PLL, not based on
the armclock, right.  In other words:

200MHz -> 300MHz with 500Mhz alternate PLL will do:

1. Switch core PLL to 500MHz alt with "div by 3", so 166MHz arm clock.  OK.

2. ...but the "div by 3" doesn't apply to MP AXI clock divider, M0 AXI
clock divider, etc.  That means _those_ are now based on 500MHz and
are now running faster than they were.


None of that terribly matters for rk3288 which (currently) has the
same dividers for every rate point, but it seems like it might be
relevant for 3066 and 3188.  ...or if they ever need to change
dividers for rk3288.



> +
> +       spin_unlock(cpuclk->lock);
> +       return 0;
> +}
> +
> +static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
> +                                           struct clk_notifier_data *ndata)
> +{
> +       const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> +
> +       spin_lock(cpuclk->lock);
> +
> +       /*
> +        * post-rate change event, re-mux to primary parent and remove dividers.
> +        *
> +        * NOTE: we do this in a single transaction so we're never dividing the
> +        * primary parent by the extra dividers that were needed for the alt.
> +        */
> +
> +       writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
> +                               reg_data->div_core_shift) |
> +              HIWORD_UPDATE(0, 1, reg_data->mux_core_shift),
> +              cpuclk->reg_base + reg_data->core_reg);
> +
> +       spin_unlock(cpuclk->lock);
> +       return 0;
> +}
> +
> +/*
> + * This clock notifier is called when the frequency of the parent clock
> + * of cpuclk is to be changed. This notifier handles the setting up all
> + * the divider clocks, remux to temporary parent and handling the safe
> + * frequency levels when using temporary parent.
> + */
> +static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
> +                                       unsigned long event, void *data)
> +{
> +       struct clk_notifier_data *ndata = data;
> +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
> +       int ret = 0;
> +
> +       pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
> +                __func__, event, ndata->old_rate, ndata->new_rate);
> +       if (event == PRE_RATE_CHANGE)
> +               ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
> +       else if (event == POST_RATE_CHANGE)
> +               ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
> +
> +       return notifier_from_errno(ret);
> +}
> +
> +struct clk *rockchip_clk_register_cpuclk(const char *name,
> +                       const char **parent_names, u8 num_parents,
> +                       const struct rockchip_cpuclk_reg_data *reg_data,
> +                       struct rockchip_cpuclk_rate_table *rate_table,
> +                       void __iomem *reg_base, spinlock_t *lock)
> +{
> +       struct rockchip_cpuclk *cpuclk;
> +       struct clk_init_data init;
> +       struct clk *clk, *cclk;
> +       int ret;
> +
> +       if (!reg_data) {
> +               pr_err("%s: no soc register information\n", __func__);
> +               return ERR_PTR(-EINVAL);
> +       }

Do we really think it likely that we'll get passed a NULL reg_data?

> +
> +       if (num_parents != 2) {
> +               pr_err("%s: needs two parent clocks\n", __func__);
> +               return ERR_PTR(-EINVAL);
> +       }

I guess this is just future proofing things?  I notice that the most
recently posted exynos driver just takes two strings, the main parent
and an alternate parent.  That does seem slightly cleaner.

> +
> +       cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
> +       if (!cpuclk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       init.parent_names = &parent_names[0];
> +       init.num_parents = 1;
> +       init.ops = &rockchip_cpuclk_ops;
> +
> +       /* only allow rate changes when we have a rate table */
> +       init.flags = rate_table ? CLK_SET_RATE_PARENT : 0;
> +
> +       /* disallow automatic parent changes by ccf */
> +       init.flags |= CLK_SET_RATE_NO_REPARENT;
> +
> +       init.flags |= CLK_GET_RATE_NOCACHE;
> +
> +       cpuclk->reg_base = reg_base;
> +       cpuclk->lock = lock;
> +       cpuclk->reg_data = reg_data;
> +       cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
> +       cpuclk->hw.init = &init;
> +
> +       cpuclk->alt_parent = __clk_lookup(parent_names[1]);
> +       if (!cpuclk->alt_parent) {
> +               pr_err("%s: could not lookup alternate parent\n",
> +                      __func__);
> +               ret = -EINVAL;
> +               goto free_cpuclk;
> +       }
> +
> +       ret = clk_prepare_enable(cpuclk->alt_parent);
> +       if (ret) {
> +               pr_err("%s: could not enable alternate parent\n",
> +                      __func__);
> +               goto free_cpuclk;
> +       }
> +
> +       clk = __clk_lookup(parent_names[0]);
> +       if (!clk) {
> +               pr_err("%s: could not lookup parent clock %s\n",
> +                      __func__, parent_names[0]);
> +               ret = -EINVAL;
> +               goto free_cpuclk;
> +       }
> +
> +       ret = clk_notifier_register(clk, &cpuclk->clk_nb);
> +       if (ret) {
> +               pr_err("%s: failed to register clock notifier for %s\n",
> +                               __func__, name);
> +               goto free_cpuclk;
> +       }
> +
> +       if (rate_table) {
> +               int nrates;
> +
> +               /* find count of rates in rate_table */
> +               for (nrates = 0; rate_table[nrates].prate != 0; )
> +                       nrates++;
> +
> +               cpuclk->rate_count = nrates;
> +               cpuclk->rate_table = kmemdup(rate_table,
> +                                            sizeof(*rate_table) * nrates,
> +                                            GFP_KERNEL);

Ah, I think I see what Derek was saying earlier.  It's a little
awkward that you pass this in as a table with a sentinel at the end
and then at runtime convert it to an array with a stored length.
Given that all of these are compiled-in tables, it wouldn't be crazy
to just pass in nrates (which is just ARRAY_SIZE in all callers) and
get rid of the sentinel.  ...but I won't insist on it.


> +               if (!cpuclk->rate_table) {
> +                       pr_err("%s: could not allocate memory for cpuclk rates\n",
> +                              __func__);
> +                       ret = -ENOMEM;
> +                       goto unregister_notifier;
> +               }
> +       }
> +
> +       cclk = clk_register(NULL, &cpuclk->hw);
> +       if (IS_ERR(clk)) {
> +               pr_err("%s: could not register cpuclk %s\n", __func__,  name);
> +               ret = PTR_ERR(clk);
> +               goto free_rate_table;
> +       }
> +
> +       return cclk;
> +
> +free_rate_table:
> +       kfree(cpuclk->rate_table);
> +unregister_notifier:
> +       clk_notifier_unregister(clk, &cpuclk->clk_nb);
> +free_cpuclk:
> +       kfree(cpuclk);
> +       return ERR_PTR(ret);
> +}
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index d9c6db2..f87ac4a 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -297,6 +297,26 @@ void __init rockchip_clk_register_branches(
>         }
>  }
>
> +void __init rockchip_clk_register_armclk(unsigned int lookup_id,
> +                       const char *name, const char **parent_names,
> +                       u8 num_parents,
> +                       const struct rockchip_cpuclk_reg_data *reg_data,
> +                       struct rockchip_cpuclk_rate_table *rate_table)
> +{
> +       struct clk *clk;
> +
> +       clk = rockchip_clk_register_cpuclk(name, parent_names, num_parents,
> +                                          reg_data, rate_table, reg_base,
> +                                          &clk_lock);
> +       if (IS_ERR(clk)) {
> +               pr_err("%s: failed to register clock %s: %ld\n",
> +                      __func__, name, PTR_ERR(clk));
> +               return;
> +       }
> +
> +       rockchip_clk_add_lookup(clk, lookup_id);
> +}
> +
>  void __init rockchip_clk_protect_critical(const char *clocks[], int nclocks)
>  {
>         int i;
> diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
> index 2b0bca1..e0ea61e 100644
> --- a/drivers/clk/rockchip/clk.h
> +++ b/drivers/clk/rockchip/clk.h
> @@ -120,6 +120,38 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
>                 struct rockchip_pll_rate_table *rate_table,
>                 spinlock_t *lock);
>
> +struct rockchip_cpuclk_clksel {
> +       int reg;
> +       u32 val;
> +};
> +
> +#define ROCKCHIP_CPUCLK_NUM_DIVIDERS   2
> +struct rockchip_cpuclk_rate_table {
> +       unsigned long prate;
> +       struct rockchip_cpuclk_clksel divs[ROCKCHIP_CPUCLK_NUM_DIVIDERS];
> +};
> +
> +/**
> + * struct rockchip_cpuclk_reg_data: describes register offsets and masks of the cpuclock
> + * @core_reg:          register offset of the core settings register
> + * @div_core_shift:    core divider offset used to divide the pll value
> + * @div_core_mask:     core divider mask
> + * @mux_core_shift:    offset of the core multiplexer
> + */
> +struct rockchip_cpuclk_reg_data {
> +       int             core_reg;
> +       u8              div_core_shift;
> +       u32             div_core_mask;
> +       int             mux_core_reg;
> +       u8              mux_core_shift;
> +};
> +
> +struct clk *rockchip_clk_register_cpuclk(const char *name,
> +                       const char **parent_names, u8 num_parents,
> +                       const struct rockchip_cpuclk_reg_data *reg_data,
> +                       struct rockchip_cpuclk_rate_table *rate_table,
> +                       void __iomem *reg_base, spinlock_t *lock);
> +
>  #define PNAME(x) static const char *x[] __initconst
>
>  enum rockchip_clk_branch_type {
> @@ -329,6 +361,10 @@ void rockchip_clk_register_branches(struct rockchip_clk_branch *clk_list,
>                                     unsigned int nr_clk);
>  void rockchip_clk_register_plls(struct rockchip_pll_clock *pll_list,
>                                 unsigned int nr_pll, int grf_lock_offset);
> +void rockchip_clk_register_armclk(unsigned int lookup_id, const char *name,
> +                       const char **parent_names, u8 num_parents,
> +                       const struct rockchip_cpuclk_reg_data *reg_data,
> +                       struct rockchip_cpuclk_rate_table *rate_table);
>  void rockchip_clk_protect_critical(const char *clocks[], int nclocks);
>
>  #define ROCKCHIP_SOFTRST_HIWORD_MASK   BIT(0)
> --
> 2.0.1
>

[PATCH v3 8/8] clk: rockchip: switch to using the new cpuclk type for armclk

[Doug Anderson]

Heiko,

On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> +static struct rockchip_cpuclk_rate_table rk3288_cpuclk_rates[] = {
> +       RK3288_CPUCLK_RATE(1800000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE(1704000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE(1608000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE(1512000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE(1416000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE(1200000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE(1008000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 816000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 696000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 600000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 408000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 312000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 216000000, 2, 4, 2, 4, 4),
> +       RK3288_CPUCLK_RATE( 126000000, 2, 4, 2, 4, 4),
> +       { /* sentinel */ },
> +};

As Derek Basehore pointed out in another thread, this could be init
const.  ...but that could also happen in a followup patch.

...some of my review feedback on the "clk-cpu" patch might require
changes to this file but they would be minor, I think.  On rk3288 I've
stressed this series and things look good.  I've also generally
reviewed the code at a high level.

Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>

[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk

[Heiko Stübner]

Am Montag, 22. September 2014, 10:47:25 schrieb Doug Anderson:
> Heiko,
> 
> On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > When changing the armclk on Rockchip SoCs it is supposed to be reparented
> > to an alternate parent before changing the underlying pll and back after
> > the change. Additionally there exist clocks that are very tightly bound to
> > the armclk whose divider values are set according to the armclk rate.
> > 
> > Add a special clock-type to handle all that. The rate table and divider
> > values will be supplied from the soc-specific clock controllers.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/rockchip/Makefile  |   1 +
> >  drivers/clk/rockchip/clk-cpu.c | 330
> >  +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c    
> >  |  20 +++
> >  drivers/clk/rockchip/clk.h     |  36 +++++
> >  4 files changed, 387 insertions(+)
> >  create mode 100644 drivers/clk/rockchip/clk-cpu.c
> > 
> > diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
> > index ee6b077..bd8514d 100644
> > --- a/drivers/clk/rockchip/Makefile
> > +++ b/drivers/clk/rockchip/Makefile
> > @@ -5,6 +5,7 @@
> > 
> >  obj-y  += clk-rockchip.o
> >  obj-y  += clk.o
> >  obj-y  += clk-pll.o
> > 
> > +obj-y  += clk-cpu.o
> > 
> >  obj-$(CONFIG_RESET_CONTROLLER) += softrst.o
> >  
> >  obj-y  += clk-rk3188.o
> > 
> > diff --git a/drivers/clk/rockchip/clk-cpu.c
> > b/drivers/clk/rockchip/clk-cpu.c new file mode 100644
> > index 0000000..8f0aaeb
> > --- /dev/null
> > +++ b/drivers/clk/rockchip/clk-cpu.c
> > @@ -0,0 +1,330 @@
> > +/*
> > + * Copyright (c) 2014 MundoReader S.L.
> > + * Author: Heiko Stuebner <heiko@sntech.de>
> > + *
> > + * based on clk/samsung/clk-cpu.c
> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> > + * Author: Thomas Abraham <thomas.ab@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This file contains the utility function to register CPU clock for
> > Samsung
> I think on the last review someone pointed out that this part of the
> comment is wrong.  Perhaps you could remove it or fix it?
> Specifically, this is not for Samsung Exynos...
> 
> > + * Exynos platforms. A CPU clock is defined as a clock supplied to a CPU
> > or a + * group of CPUs. The CPU clock is typically derived from a
> > hierarchy of clock + * blocks which includes mux and divider blocks.
> > There are a number of other + * auxiliary clocks supplied to the CPU
> > domain such as the debug blocks and AXI + * clock for CPU domain. The
> > rates of these auxiliary clocks are related to the + * CPU clock rate and
> > this relation is usually specified in the hardware manual + * of the SoC
> > or supplied after the SoC characterization.
> > + *
> > + * The below implementation of the CPU clock allows the rate changes of
> > the CPU + * clock and the corresponding rate changes of the auxillary
> > clocks of the CPU + * domain. The platform clock driver provides a clock
> > register configuration + * for each configurable rate which is then used
> > to program the clock hardware + * registers to acheive a fast
> > co-oridinated rate change for all the CPU domain + * clocks.
> > + *
> > + * On a rate change request for the CPU clock, the rate change is
> > propagated + * upto the PLL supplying the clock to the CPU domain clock
> > blocks. While the + * CPU domain PLL is reconfigured, the CPU domain
> > clocks are driven using an + * alternate clock source. If required, the
> > alternate clock source is divided + * down in order to keep the output
> > clock rate within the previous OPP limits. + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/io.h>
> > +#include <linux/clk-provider.h>
> > +#include "clk.h"
> > +
> > +/**
> > + * struct rockchip_cpuclk: information about clock supplied to a CPU
> > core.
> > + * @hw:                handle between ccf and cpu clock.
> > + * @alt_parent:        alternate parent clock to use when switching the
> > speed + *             of the primary parent clock.
> > + * @reg_base:  base register for cpu-clock values.
> > + * @clk_nb:    clock notifier registered for changes in clock speed of
> > the
> > + *             primary parent clock.
> > + * @rate_count:        number of rates in the rate_table
> > + * @rate_table:        pll-rates and their associated dividers
> > + * @reg_data:  cpu-specific register settings
> > + * @lock:      clock lock
> > + */
> > +struct rockchip_cpuclk {
> > +       struct clk_hw                           hw;
> > +
> > +       struct clk_mux                          cpu_mux;
> > +       const struct clk_ops                    *cpu_mux_ops;
> > +
> > +       struct clk                              *alt_parent;
> > +       void __iomem                            *reg_base;
> > +       struct notifier_block                   clk_nb;
> > +       unsigned int                            rate_count;
> > +       struct rockchip_cpuclk_rate_table       *rate_table;
> > +       const struct rockchip_cpuclk_reg_data   *reg_data;
> > +       spinlock_t                              *lock;
> > +};
> > +
> > +#define to_rockchip_cpuclk_hw(hw) container_of(hw, struct
> > rockchip_cpuclk, hw) +#define to_rockchip_cpuclk_nb(nb) \
> > +                       container_of(nb, struct rockchip_cpuclk, clk_nb)
> > +
> > +static const struct rockchip_cpuclk_rate_table
> > *rockchip_get_cpuclk_settings( +                           struct
> > rockchip_cpuclk *cpuclk, unsigned long rate) +{
> > +       const struct rockchip_cpuclk_rate_table *rate_table =
> > +                                                      
> > cpuclk->rate_table;
> > +       int i;
> > +
> > +       for (i = 0; i < cpuclk->rate_count; i++) {
> > +               if (rate == rate_table[i].prate)
> > +                       return &rate_table[i];
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
> > +                                       unsigned long parent_rate)
> > +{
> > +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_hw(hw);
> > +       const struct rockchip_cpuclk_reg_data *reg_data =
> > cpuclk->reg_data;
> > +       u32 clksel0 = readl_relaxed(cpuclk->reg_base +
> > reg_data->core_reg);
> > +
> > +       clksel0 >>= reg_data->div_core_shift;
> > +       clksel0 &= reg_data->div_core_mask;
> > +       return parent_rate / (clksel0 + 1);
> > +}
> > +
> > +static const struct clk_ops rockchip_cpuclk_ops = {
> > +       .recalc_rate = rockchip_cpuclk_recalc_rate,
> > +};
> > +
> > +static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk
> > *cpuclk,
> > +                                          struct clk_notifier_data
> > *ndata)
> > +{
> > +       const struct rockchip_cpuclk_reg_data *reg_data =
> > cpuclk->reg_data;
> > +       const struct rockchip_cpuclk_rate_table *rate;
> > +       unsigned long alt_prate, alt_div;
> > +       int i;
> > +
> > +       rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
> > +       if (!rate) {
> > +               pr_err("%s: Invalid rate : %lu for cpuclk\n",
> > +                      __func__, ndata->new_rate);
> > +               return -EINVAL;
> > +       }
> > +
> > +       alt_prate = clk_get_rate(cpuclk->alt_parent);
> > +
> > +       spin_lock(cpuclk->lock);
> > +
> > +       /*
> > +        * If the old parent clock speed is less than the clock speed
> > +        * of the alternate parent, then it should be ensured that at no
> > point +        * the armclk speed is more than the old_rate until the
> > dividers are +        * set.
> > +        */
> > +       if (alt_prate > ndata->old_rate) {
> > +               /* calculate dividers */
> > +               alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
> > +               if (alt_div > reg_data->div_core_mask) {
> > +                       pr_warn("%s: limiting alt-divider %lu to %d\n",
> > +                               __func__, alt_div,
> > reg_data->div_core_mask); +                       alt_div =
> > reg_data->div_core_mask;
> > +               }
> > +
> > +               /*
> > +                * Change parents and add dividers in a single
> > transaction.
> > +                *
> > +                * NOTE: we do this in a single transaction so we're never
> > +                * dividing the primary parent by the extra dividers that
> > were +                * needed for the alt.
> > +                */
> > +               pr_debug("%s: setting div %lu as alt-rate %lu > old-rate
> > %lu\n", +                        __func__, alt_div, alt_prate,
> > ndata->old_rate); +
> > +               writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
> > +                                             reg_data->div_core_shift) |
> > +                      HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
> > +                      cpuclk->reg_base + reg_data->core_reg);
> > +       } else {
> > +               /* select alternate parent */
> > +               writel(HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
> > +                       cpuclk->reg_base + reg_data->core_reg);
> > +       }
> > +
> > +       /* alternate parent is active now. set the dividers */
> > +       for (i = 0; i < ARRAY_SIZE(rate->divs); i++) {
> > +               const struct rockchip_cpuclk_clksel *clksel =
> > &rate->divs[i]; +
> > +               if (!clksel->reg)
> > +                       continue;
> > +
> > +               pr_debug("%s: setting reg 0x%x to 0x%x\n",
> > +                        __func__, clksel->reg, clksel->val);
> > +               writel(clksel->val , cpuclk->reg_base + clksel->reg);
> > +       }
> 
> I'm not 100% certain that you can always set the dividers here and
> still be safe.
> 
> Let's imagine that we're going 800MHz => 300MHz.  Let's say that we
> have "divide by 3" for 800Mhz and "divide by 1" for 300Mhz.  Let's say
> that the alternate PLL is 500Mhz.
> 
> ...so at this point in the transition we've switched to the alternate
> PLL (500Mhz) but we're now setting the dividers for 300MHz.  That
> doesn't seem quite right.

So setting dividers here when increasing the rate and in the post_rate notifier 
when lowering the rate?

I think the samsung cpuclk had the same discussion at some point, but am not 
sure what the outcome was.


> I also _think_ that these dividers are based on the PLL, not based on
> the armclock, right.  In other words:
> 
> 200MHz -> 300MHz with 500Mhz alternate PLL will do:
> 
> 1. Switch core PLL to 500MHz alt with "div by 3", so 166MHz arm clock.  OK.
> 
> 2. ...but the "div by 3" doesn't apply to MP AXI clock divider, M0 AXI
> clock divider, etc.  That means _those_ are now based on 500MHz and
> are now running faster than they were.
> 

Nope, the dividers are clocks that are really children of the armclk. In the 
rk3288 trm on page 44 you can see the mux from apll and gpll, followed by the 
divider we use to keep the temporary rate below the old apll rate.

The result of this is then supplied to the armclk [*] and the tightly bound 
clocks. So the "div by 3" when using the alternate parent will be used by both 
the arm-cores as well as the dependent clocks.



[*] There is a slight variance between rk3288 and the Cortex-A9 SoCs. On 
rk3188 and before the result of the mux+divider described above are fed 
directly and unmodified to the arm cores, while the core clocks on rk3288 could 
use another divider on a per-core basis (see clk_core0..clk_core3 on the page 
44 cited above).

But it also doesn't matter much for this, as the coupled clocks are dependent 
on the shared parent of clk_coreX.


> 
> None of that terribly matters for rk3288 which (currently) has the
> same dividers for every rate point, but it seems like it might be
> relevant for 3066 and 3188.  ...or if they ever need to change
> dividers for rk3288.
> 
> > +
> > +       spin_unlock(cpuclk->lock);
> > +       return 0;
> > +}
> > +
> > +static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk
> > *cpuclk, +                                           struct
> > clk_notifier_data *ndata) +{
> > +       const struct rockchip_cpuclk_reg_data *reg_data =
> > cpuclk->reg_data;
> > +
> > +       spin_lock(cpuclk->lock);
> > +
> > +       /*
> > +        * post-rate change event, re-mux to primary parent and remove
> > dividers. +        *
> > +        * NOTE: we do this in a single transaction so we're never
> > dividing the +        * primary parent by the extra dividers that were
> > needed for the alt. +        */
> > +
> > +       writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
> > +                               reg_data->div_core_shift) |
> > +              HIWORD_UPDATE(0, 1, reg_data->mux_core_shift),
> > +              cpuclk->reg_base + reg_data->core_reg);
> > +
> > +       spin_unlock(cpuclk->lock);
> > +       return 0;
> > +}
> > +
> > +/*
> > + * This clock notifier is called when the frequency of the parent clock
> > + * of cpuclk is to be changed. This notifier handles the setting up all
> > + * the divider clocks, remux to temporary parent and handling the safe
> > + * frequency levels when using temporary parent.
> > + */
> > +static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
> > +                                       unsigned long event, void *data)
> > +{
> > +       struct clk_notifier_data *ndata = data;
> > +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
> > +       int ret = 0;
> > +
> > +       pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
> > +                __func__, event, ndata->old_rate, ndata->new_rate);
> > +       if (event == PRE_RATE_CHANGE)
> > +               ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
> > +       else if (event == POST_RATE_CHANGE)
> > +               ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
> > +
> > +       return notifier_from_errno(ret);
> > +}
> > +
> > +struct clk *rockchip_clk_register_cpuclk(const char *name,
> > +                       const char **parent_names, u8 num_parents,
> > +                       const struct rockchip_cpuclk_reg_data *reg_data,
> > +                       struct rockchip_cpuclk_rate_table *rate_table,
> > +                       void __iomem *reg_base, spinlock_t *lock)
> > +{
> > +       struct rockchip_cpuclk *cpuclk;
> > +       struct clk_init_data init;
> > +       struct clk *clk, *cclk;
> > +       int ret;
> > +
> > +       if (!reg_data) {
> > +               pr_err("%s: no soc register information\n", __func__);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> 
> Do we really think it likely that we'll get passed a NULL reg_data?
> 
> > +
> > +       if (num_parents != 2) {
> > +               pr_err("%s: needs two parent clocks\n", __func__);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> 
> I guess this is just future proofing things?  I notice that the most
> recently posted exynos driver just takes two strings, the main parent
> and an alternate parent.  That does seem slightly cleaner.

As the regular clock register functions use the parent_names, num_parents form 
it somehow seemed cleaner to me to continue this, but I don't have a hard 
preference here.


> 
> > +
> > +       cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
> > +       if (!cpuclk)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       init.name = name;
> > +       init.parent_names = &parent_names[0];
> > +       init.num_parents = 1;
> > +       init.ops = &rockchip_cpuclk_ops;
> > +
> > +       /* only allow rate changes when we have a rate table */
> > +       init.flags = rate_table ? CLK_SET_RATE_PARENT : 0;
> > +
> > +       /* disallow automatic parent changes by ccf */
> > +       init.flags |= CLK_SET_RATE_NO_REPARENT;
> > +
> > +       init.flags |= CLK_GET_RATE_NOCACHE;
> > +
> > +       cpuclk->reg_base = reg_base;
> > +       cpuclk->lock = lock;
> > +       cpuclk->reg_data = reg_data;
> > +       cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
> > +       cpuclk->hw.init = &init;
> > +
> > +       cpuclk->alt_parent = __clk_lookup(parent_names[1]);
> > +       if (!cpuclk->alt_parent) {
> > +               pr_err("%s: could not lookup alternate parent\n",
> > +                      __func__);
> > +               ret = -EINVAL;
> > +               goto free_cpuclk;
> > +       }
> > +
> > +       ret = clk_prepare_enable(cpuclk->alt_parent);
> > +       if (ret) {
> > +               pr_err("%s: could not enable alternate parent\n",
> > +                      __func__);
> > +               goto free_cpuclk;
> > +       }
> > +
> > +       clk = __clk_lookup(parent_names[0]);
> > +       if (!clk) {
> > +               pr_err("%s: could not lookup parent clock %s\n",
> > +                      __func__, parent_names[0]);
> > +               ret = -EINVAL;
> > +               goto free_cpuclk;
> > +       }
> > +
> > +       ret = clk_notifier_register(clk, &cpuclk->clk_nb);
> > +       if (ret) {
> > +               pr_err("%s: failed to register clock notifier for %s\n",
> > +                               __func__, name);
> > +               goto free_cpuclk;
> > +       }
> > +
> > +       if (rate_table) {
> > +               int nrates;
> > +
> > +               /* find count of rates in rate_table */
> > +               for (nrates = 0; rate_table[nrates].prate != 0; )
> > +                       nrates++;
> > +
> > +               cpuclk->rate_count = nrates;
> > +               cpuclk->rate_table = kmemdup(rate_table,
> > +                                            sizeof(*rate_table) * nrates,
> > +                                            GFP_KERNEL);
> 
> Ah, I think I see what Derek was saying earlier.  It's a little
> awkward that you pass this in as a table with a sentinel at the end
> and then at runtime convert it to an array with a stored length.
> Given that all of these are compiled-in tables, it wouldn't be crazy
> to just pass in nrates (which is just ARRAY_SIZE in all callers) and
> get rid of the sentinel.  ...but I won't insist on it.

This would also be true for the pll rate tables then ... mixing concepts 
between such tables in clock drivers would be strange somehow.

I guess this was originally simply getting a lot of inpsiration from the 
samsung pll-clock driver, which does use the same style (including the 
copying). Not sure if there was a rationale behind the original.

Interestingly (I just looked it up) the new samsung cpuclk uses the rates + 
nrates approach.

So I guess I would be ok with using the other approach as well :-)


Heiko

[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk

[Doug Anderson]

Heiko,

On Mon, Sep 22, 2014 at 12:21 PM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Montag, 22. September 2014, 10:47:25 schrieb Doug Anderson:
>> Heiko,
>>
>> On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> > When changing the armclk on Rockchip SoCs it is supposed to be reparented
>> > to an alternate parent before changing the underlying pll and back after
>> > the change. Additionally there exist clocks that are very tightly bound to
>> > the armclk whose divider values are set according to the armclk rate.
>> >
>> > Add a special clock-type to handle all that. The rate table and divider
>> > values will be supplied from the soc-specific clock controllers.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  drivers/clk/rockchip/Makefile  |   1 +
>> >  drivers/clk/rockchip/clk-cpu.c | 330
>> >  +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c
>> >  |  20 +++
>> >  drivers/clk/rockchip/clk.h     |  36 +++++
>> >  4 files changed, 387 insertions(+)
>> >  create mode 100644 drivers/clk/rockchip/clk-cpu.c
>> >
>> > diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
>> > index ee6b077..bd8514d 100644
>> > --- a/drivers/clk/rockchip/Makefile
>> > +++ b/drivers/clk/rockchip/Makefile
>> > @@ -5,6 +5,7 @@
>> >
>> >  obj-y  += clk-rockchip.o
>> >  obj-y  += clk.o
>> >  obj-y  += clk-pll.o
>> >
>> > +obj-y  += clk-cpu.o
>> >
>> >  obj-$(CONFIG_RESET_CONTROLLER) += softrst.o
>> >
>> >  obj-y  += clk-rk3188.o
>> >
>> > diff --git a/drivers/clk/rockchip/clk-cpu.c
>> > b/drivers/clk/rockchip/clk-cpu.c new file mode 100644
>> > index 0000000..8f0aaeb
>> > --- /dev/null
>> > +++ b/drivers/clk/rockchip/clk-cpu.c
>> > @@ -0,0 +1,330 @@
>> > +/*
>> > + * Copyright (c) 2014 MundoReader S.L.
>> > + * Author: Heiko Stuebner <heiko@sntech.de>
>> > + *
>> > + * based on clk/samsung/clk-cpu.c
>> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> > + * Author: Thomas Abraham <thomas.ab@samsung.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + *
>> > + * This file contains the utility function to register CPU clock for
>> > Samsung
>> I think on the last review someone pointed out that this part of the
>> comment is wrong.  Perhaps you could remove it or fix it?
>> Specifically, this is not for Samsung Exynos...
>>
>> > + * Exynos platforms. A CPU clock is defined as a clock supplied to a CPU
>> > or a + * group of CPUs. The CPU clock is typically derived from a
>> > hierarchy of clock + * blocks which includes mux and divider blocks.
>> > There are a number of other + * auxiliary clocks supplied to the CPU
>> > domain such as the debug blocks and AXI + * clock for CPU domain. The
>> > rates of these auxiliary clocks are related to the + * CPU clock rate and
>> > this relation is usually specified in the hardware manual + * of the SoC
>> > or supplied after the SoC characterization.
>> > + *
>> > + * The below implementation of the CPU clock allows the rate changes of
>> > the CPU + * clock and the corresponding rate changes of the auxillary
>> > clocks of the CPU + * domain. The platform clock driver provides a clock
>> > register configuration + * for each configurable rate which is then used
>> > to program the clock hardware + * registers to acheive a fast
>> > co-oridinated rate change for all the CPU domain + * clocks.
>> > + *
>> > + * On a rate change request for the CPU clock, the rate change is
>> > propagated + * upto the PLL supplying the clock to the CPU domain clock
>> > blocks. While the + * CPU domain PLL is reconfigured, the CPU domain
>> > clocks are driven using an + * alternate clock source. If required, the
>> > alternate clock source is divided + * down in order to keep the output
>> > clock rate within the previous OPP limits. + */
>> > +
>> > +#include <linux/of.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/io.h>
>> > +#include <linux/clk-provider.h>
>> > +#include "clk.h"
>> > +
>> > +/**
>> > + * struct rockchip_cpuclk: information about clock supplied to a CPU
>> > core.
>> > + * @hw:                handle between ccf and cpu clock.
>> > + * @alt_parent:        alternate parent clock to use when switching the
>> > speed + *             of the primary parent clock.
>> > + * @reg_base:  base register for cpu-clock values.
>> > + * @clk_nb:    clock notifier registered for changes in clock speed of
>> > the
>> > + *             primary parent clock.
>> > + * @rate_count:        number of rates in the rate_table
>> > + * @rate_table:        pll-rates and their associated dividers
>> > + * @reg_data:  cpu-specific register settings
>> > + * @lock:      clock lock
>> > + */
>> > +struct rockchip_cpuclk {
>> > +       struct clk_hw                           hw;
>> > +
>> > +       struct clk_mux                          cpu_mux;
>> > +       const struct clk_ops                    *cpu_mux_ops;
>> > +
>> > +       struct clk                              *alt_parent;
>> > +       void __iomem                            *reg_base;
>> > +       struct notifier_block                   clk_nb;
>> > +       unsigned int                            rate_count;
>> > +       struct rockchip_cpuclk_rate_table       *rate_table;
>> > +       const struct rockchip_cpuclk_reg_data   *reg_data;
>> > +       spinlock_t                              *lock;
>> > +};
>> > +
>> > +#define to_rockchip_cpuclk_hw(hw) container_of(hw, struct
>> > rockchip_cpuclk, hw) +#define to_rockchip_cpuclk_nb(nb) \
>> > +                       container_of(nb, struct rockchip_cpuclk, clk_nb)
>> > +
>> > +static const struct rockchip_cpuclk_rate_table
>> > *rockchip_get_cpuclk_settings( +                           struct
>> > rockchip_cpuclk *cpuclk, unsigned long rate) +{
>> > +       const struct rockchip_cpuclk_rate_table *rate_table =
>> > +
>> > cpuclk->rate_table;
>> > +       int i;
>> > +
>> > +       for (i = 0; i < cpuclk->rate_count; i++) {
>> > +               if (rate == rate_table[i].prate)
>> > +                       return &rate_table[i];
>> > +       }
>> > +
>> > +       return NULL;
>> > +}
>> > +
>> > +static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
>> > +                                       unsigned long parent_rate)
>> > +{
>> > +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_hw(hw);
>> > +       const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +       u32 clksel0 = readl_relaxed(cpuclk->reg_base +
>> > reg_data->core_reg);
>> > +
>> > +       clksel0 >>= reg_data->div_core_shift;
>> > +       clksel0 &= reg_data->div_core_mask;
>> > +       return parent_rate / (clksel0 + 1);
>> > +}
>> > +
>> > +static const struct clk_ops rockchip_cpuclk_ops = {
>> > +       .recalc_rate = rockchip_cpuclk_recalc_rate,
>> > +};
>> > +
>> > +static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk
>> > *cpuclk,
>> > +                                          struct clk_notifier_data
>> > *ndata)
>> > +{
>> > +       const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +       const struct rockchip_cpuclk_rate_table *rate;
>> > +       unsigned long alt_prate, alt_div;
>> > +       int i;
>> > +
>> > +       rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
>> > +       if (!rate) {
>> > +               pr_err("%s: Invalid rate : %lu for cpuclk\n",
>> > +                      __func__, ndata->new_rate);
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       alt_prate = clk_get_rate(cpuclk->alt_parent);
>> > +
>> > +       spin_lock(cpuclk->lock);
>> > +
>> > +       /*
>> > +        * If the old parent clock speed is less than the clock speed
>> > +        * of the alternate parent, then it should be ensured that at no
>> > point +        * the armclk speed is more than the old_rate until the
>> > dividers are +        * set.
>> > +        */
>> > +       if (alt_prate > ndata->old_rate) {
>> > +               /* calculate dividers */
>> > +               alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
>> > +               if (alt_div > reg_data->div_core_mask) {
>> > +                       pr_warn("%s: limiting alt-divider %lu to %d\n",
>> > +                               __func__, alt_div,
>> > reg_data->div_core_mask); +                       alt_div =
>> > reg_data->div_core_mask;
>> > +               }
>> > +
>> > +               /*
>> > +                * Change parents and add dividers in a single
>> > transaction.
>> > +                *
>> > +                * NOTE: we do this in a single transaction so we're never
>> > +                * dividing the primary parent by the extra dividers that
>> > were +                * needed for the alt.
>> > +                */
>> > +               pr_debug("%s: setting div %lu as alt-rate %lu > old-rate
>> > %lu\n", +                        __func__, alt_div, alt_prate,
>> > ndata->old_rate); +
>> > +               writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
>> > +                                             reg_data->div_core_shift) |
>> > +                      HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
>> > +                      cpuclk->reg_base + reg_data->core_reg);
>> > +       } else {
>> > +               /* select alternate parent */
>> > +               writel(HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
>> > +                       cpuclk->reg_base + reg_data->core_reg);
>> > +       }
>> > +
>> > +       /* alternate parent is active now. set the dividers */
>> > +       for (i = 0; i < ARRAY_SIZE(rate->divs); i++) {
>> > +               const struct rockchip_cpuclk_clksel *clksel =
>> > &rate->divs[i]; +
>> > +               if (!clksel->reg)
>> > +                       continue;
>> > +
>> > +               pr_debug("%s: setting reg 0x%x to 0x%x\n",
>> > +                        __func__, clksel->reg, clksel->val);
>> > +               writel(clksel->val , cpuclk->reg_base + clksel->reg);
>> > +       }
>>
>> I'm not 100% certain that you can always set the dividers here and
>> still be safe.
>>
>> Let's imagine that we're going 800MHz => 300MHz.  Let's say that we
>> have "divide by 3" for 800Mhz and "divide by 1" for 300Mhz.  Let's say
>> that the alternate PLL is 500Mhz.
>>
>> ...so at this point in the transition we've switched to the alternate
>> PLL (500Mhz) but we're now setting the dividers for 300MHz.  That
>> doesn't seem quite right.
>
> So setting dividers here when increasing the rate and in the post_rate notifier
> when lowering the rate?

Yes.


> I think the samsung cpuclk had the same discussion at some point, but am not
> sure what the outcome was.

OK.  I didn't go try to read all of the exynos upstream discussion.  I
mostly just reviewed your patch as-is and also did some diffs to the
newest version of the exynos patch I could find.

It appears that the exynos patch does set some dividers in the post change.


>> I also _think_ that these dividers are based on the PLL, not based on
>> the armclock, right.  In other words:
>>
>> 200MHz -> 300MHz with 500Mhz alternate PLL will do:
>>
>> 1. Switch core PLL to 500MHz alt with "div by 3", so 166MHz arm clock.  OK.
>>
>> 2. ...but the "div by 3" doesn't apply to MP AXI clock divider, M0 AXI
>> clock divider, etc.  That means _those_ are now based on 500MHz and
>> are now running faster than they were.
>>
>
> Nope, the dividers are clocks that are really children of the armclk. In the
> rk3288 trm on page 44 you can see the mux from apll and gpll, followed by the
> divider we use to keep the temporary rate below the old apll rate.

Ah, perfect.  Yes, that diagram clears things up.  I was using the
register descriptions only where it was unclear.


> The result of this is then supplied to the armclk [*] and the tightly bound
> clocks. So the "div by 3" when using the alternate parent will be used by both
> the arm-cores as well as the dependent clocks.
>
>
>
> [*] There is a slight variance between rk3288 and the Cortex-A9 SoCs. On
> rk3188 and before the result of the mux+divider described above are fed
> directly and unmodified to the arm cores, while the core clocks on rk3288 could
> use another divider on a per-core basis (see clk_core0..clk_core3 on the page
> 44 cited above).
>
> But it also doesn't matter much for this, as the coupled clocks are dependent
> on the shared parent of clk_coreX.
>
>
>>
>> None of that terribly matters for rk3288 which (currently) has the
>> same dividers for every rate point, but it seems like it might be
>> relevant for 3066 and 3188.  ...or if they ever need to change
>> dividers for rk3288.
>>
>> > +
>> > +       spin_unlock(cpuclk->lock);
>> > +       return 0;
>> > +}
>> > +
>> > +static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk
>> > *cpuclk, +                                           struct
>> > clk_notifier_data *ndata) +{
>> > +       const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +
>> > +       spin_lock(cpuclk->lock);
>> > +
>> > +       /*
>> > +        * post-rate change event, re-mux to primary parent and remove
>> > dividers. +        *
>> > +        * NOTE: we do this in a single transaction so we're never
>> > dividing the +        * primary parent by the extra dividers that were
>> > needed for the alt. +        */
>> > +
>> > +       writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
>> > +                               reg_data->div_core_shift) |
>> > +              HIWORD_UPDATE(0, 1, reg_data->mux_core_shift),
>> > +              cpuclk->reg_base + reg_data->core_reg);
>> > +
>> > +       spin_unlock(cpuclk->lock);
>> > +       return 0;
>> > +}
>> > +
>> > +/*
>> > + * This clock notifier is called when the frequency of the parent clock
>> > + * of cpuclk is to be changed. This notifier handles the setting up all
>> > + * the divider clocks, remux to temporary parent and handling the safe
>> > + * frequency levels when using temporary parent.
>> > + */
>> > +static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
>> > +                                       unsigned long event, void *data)
>> > +{
>> > +       struct clk_notifier_data *ndata = data;
>> > +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
>> > +       int ret = 0;
>> > +
>> > +       pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
>> > +                __func__, event, ndata->old_rate, ndata->new_rate);
>> > +       if (event == PRE_RATE_CHANGE)
>> > +               ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
>> > +       else if (event == POST_RATE_CHANGE)
>> > +               ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
>> > +
>> > +       return notifier_from_errno(ret);
>> > +}
>> > +
>> > +struct clk *rockchip_clk_register_cpuclk(const char *name,
>> > +                       const char **parent_names, u8 num_parents,
>> > +                       const struct rockchip_cpuclk_reg_data *reg_data,
>> > +                       struct rockchip_cpuclk_rate_table *rate_table,
>> > +                       void __iomem *reg_base, spinlock_t *lock)
>> > +{
>> > +       struct rockchip_cpuclk *cpuclk;
>> > +       struct clk_init_data init;
>> > +       struct clk *clk, *cclk;
>> > +       int ret;
>> > +
>> > +       if (!reg_data) {
>> > +               pr_err("%s: no soc register information\n", __func__);
>> > +               return ERR_PTR(-EINVAL);
>> > +       }
>>
>> Do we really think it likely that we'll get passed a NULL reg_data?
>>
>> > +
>> > +       if (num_parents != 2) {
>> > +               pr_err("%s: needs two parent clocks\n", __func__);
>> > +               return ERR_PTR(-EINVAL);
>> > +       }
>>
>> I guess this is just future proofing things?  I notice that the most
>> recently posted exynos driver just takes two strings, the main parent
>> and an alternate parent.  That does seem slightly cleaner.
>
> As the regular clock register functions use the parent_names, num_parents form
> it somehow seemed cleaner to me to continue this, but I don't have a hard
> preference here.

I won't force the issue, so do what you feel is cleanest.


>> > +       cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
>> > +       if (!cpuclk)
>> > +               return ERR_PTR(-ENOMEM);
>> > +
>> > +       init.name = name;
>> > +       init.parent_names = &parent_names[0];
>> > +       init.num_parents = 1;
>> > +       init.ops = &rockchip_cpuclk_ops;
>> > +
>> > +       /* only allow rate changes when we have a rate table */
>> > +       init.flags = rate_table ? CLK_SET_RATE_PARENT : 0;
>> > +
>> > +       /* disallow automatic parent changes by ccf */
>> > +       init.flags |= CLK_SET_RATE_NO_REPARENT;
>> > +
>> > +       init.flags |= CLK_GET_RATE_NOCACHE;
>> > +
>> > +       cpuclk->reg_base = reg_base;
>> > +       cpuclk->lock = lock;
>> > +       cpuclk->reg_data = reg_data;
>> > +       cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
>> > +       cpuclk->hw.init = &init;
>> > +
>> > +       cpuclk->alt_parent = __clk_lookup(parent_names[1]);
>> > +       if (!cpuclk->alt_parent) {
>> > +               pr_err("%s: could not lookup alternate parent\n",
>> > +                      __func__);
>> > +               ret = -EINVAL;
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       ret = clk_prepare_enable(cpuclk->alt_parent);
>> > +       if (ret) {
>> > +               pr_err("%s: could not enable alternate parent\n",
>> > +                      __func__);
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       clk = __clk_lookup(parent_names[0]);
>> > +       if (!clk) {
>> > +               pr_err("%s: could not lookup parent clock %s\n",
>> > +                      __func__, parent_names[0]);
>> > +               ret = -EINVAL;
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       ret = clk_notifier_register(clk, &cpuclk->clk_nb);
>> > +       if (ret) {
>> > +               pr_err("%s: failed to register clock notifier for %s\n",
>> > +                               __func__, name);
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       if (rate_table) {
>> > +               int nrates;
>> > +
>> > +               /* find count of rates in rate_table */
>> > +               for (nrates = 0; rate_table[nrates].prate != 0; )
>> > +                       nrates++;
>> > +
>> > +               cpuclk->rate_count = nrates;
>> > +               cpuclk->rate_table = kmemdup(rate_table,
>> > +                                            sizeof(*rate_table) * nrates,
>> > +                                            GFP_KERNEL);
>>
>> Ah, I think I see what Derek was saying earlier.  It's a little
>> awkward that you pass this in as a table with a sentinel at the end
>> and then at runtime convert it to an array with a stored length.
>> Given that all of these are compiled-in tables, it wouldn't be crazy
>> to just pass in nrates (which is just ARRAY_SIZE in all callers) and
>> get rid of the sentinel.  ...but I won't insist on it.
>
> This would also be true for the pll rate tables then ... mixing concepts
> between such tables in clock drivers would be strange somehow.
>
> I guess this was originally simply getting a lot of inpsiration from the
> samsung pll-clock driver, which does use the same style (including the
> copying). Not sure if there was a rationale behind the original.
>
> Interestingly (I just looked it up) the new samsung cpuclk uses the rates +
> nrates approach.
>
> So I guess I would be ok with using the other approach as well :-)

Again, no hard requirement from me, so do as you see fit.  It does
seem nice to get rid of a little extra code though...

-Doug

[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk

[Thomas Abraham]

On Tue, Sep 23, 2014 at 12:51 AM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Montag, 22. September 2014, 10:47:25 schrieb Doug Anderson:
>> Heiko,
>>
>> On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> > When changing the armclk on Rockchip SoCs it is supposed to be reparented
>> > to an alternate parent before changing the underlying pll and back after
>> > the change. Additionally there exist clocks that are very tightly bound to
>> > the armclk whose divider values are set according to the armclk rate.
>> >
>> > Add a special clock-type to handle all that. The rate table and divider
>> > values will be supplied from the soc-specific clock controllers.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  drivers/clk/rockchip/Makefile  |   1 +
>> >  drivers/clk/rockchip/clk-cpu.c | 330
>> >  +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c
>> >  |  20 +++
>> >  drivers/clk/rockchip/clk.h     |  36 +++++
>> >  4 files changed, 387 insertions(+)
>> >  create mode 100644 drivers/clk/rockchip/clk-cpu.c
>> >
>> > diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
>> > index ee6b077..bd8514d 100644
>> > --- a/drivers/clk/rockchip/Makefile
>> > +++ b/drivers/clk/rockchip/Makefile
>> > @@ -5,6 +5,7 @@
>> >
>> >  obj-y  += clk-rockchip.o
>> >  obj-y  += clk.o
>> >  obj-y  += clk-pll.o
>> >
>> > +obj-y  += clk-cpu.o
>> >
>> >  obj-$(CONFIG_RESET_CONTROLLER) += softrst.o
>> >
>> >  obj-y  += clk-rk3188.o
>> >
>> > diff --git a/drivers/clk/rockchip/clk-cpu.c
>> > b/drivers/clk/rockchip/clk-cpu.c new file mode 100644
>> > index 0000000..8f0aaeb
>> > --- /dev/null
>> > +++ b/drivers/clk/rockchip/clk-cpu.c
>> > @@ -0,0 +1,330 @@
>> > +/*
>> > + * Copyright (c) 2014 MundoReader S.L.
>> > + * Author: Heiko Stuebner <heiko@sntech.de>
>> > + *
>> > + * based on clk/samsung/clk-cpu.c
>> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> > + * Author: Thomas Abraham <thomas.ab@samsung.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + *
>> > + * This file contains the utility function to register CPU clock for
>> > Samsung
>> I think on the last review someone pointed out that this part of the
>> comment is wrong.  Perhaps you could remove it or fix it?
>> Specifically, this is not for Samsung Exynos...
>>
>> > + * Exynos platforms. A CPU clock is defined as a clock supplied to a CPU
>> > or a + * group of CPUs. The CPU clock is typically derived from a
>> > hierarchy of clock + * blocks which includes mux and divider blocks.
>> > There are a number of other + * auxiliary clocks supplied to the CPU
>> > domain such as the debug blocks and AXI + * clock for CPU domain. The
>> > rates of these auxiliary clocks are related to the + * CPU clock rate and
>> > this relation is usually specified in the hardware manual + * of the SoC
>> > or supplied after the SoC characterization.
>> > + *
>> > + * The below implementation of the CPU clock allows the rate changes of
>> > the CPU + * clock and the corresponding rate changes of the auxillary
>> > clocks of the CPU + * domain. The platform clock driver provides a clock
>> > register configuration + * for each configurable rate which is then used
>> > to program the clock hardware + * registers to acheive a fast
>> > co-oridinated rate change for all the CPU domain + * clocks.
>> > + *
>> > + * On a rate change request for the CPU clock, the rate change is
>> > propagated + * upto the PLL supplying the clock to the CPU domain clock
>> > blocks. While the + * CPU domain PLL is reconfigured, the CPU domain
>> > clocks are driven using an + * alternate clock source. If required, the
>> > alternate clock source is divided + * down in order to keep the output
>> > clock rate within the previous OPP limits. + */
>> > +
>> > +#include <linux/of.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/io.h>
>> > +#include <linux/clk-provider.h>
>> > +#include "clk.h"
>> > +
>> > +/**
>> > + * struct rockchip_cpuclk: information about clock supplied to a CPU
>> > core.
>> > + * @hw:                handle between ccf and cpu clock.
>> > + * @alt_parent:        alternate parent clock to use when switching the
>> > speed + *             of the primary parent clock.
>> > + * @reg_base:  base register for cpu-clock values.
>> > + * @clk_nb:    clock notifier registered for changes in clock speed of
>> > the
>> > + *             primary parent clock.
>> > + * @rate_count:        number of rates in the rate_table
>> > + * @rate_table:        pll-rates and their associated dividers
>> > + * @reg_data:  cpu-specific register settings
>> > + * @lock:      clock lock
>> > + */
>> > +struct rockchip_cpuclk {
>> > +       struct clk_hw                           hw;
>> > +
>> > +       struct clk_mux                          cpu_mux;
>> > +       const struct clk_ops                    *cpu_mux_ops;
>> > +
>> > +       struct clk                              *alt_parent;
>> > +       void __iomem                            *reg_base;
>> > +       struct notifier_block                   clk_nb;
>> > +       unsigned int                            rate_count;
>> > +       struct rockchip_cpuclk_rate_table       *rate_table;
>> > +       const struct rockchip_cpuclk_reg_data   *reg_data;
>> > +       spinlock_t                              *lock;
>> > +};
>> > +
>> > +#define to_rockchip_cpuclk_hw(hw) container_of(hw, struct
>> > rockchip_cpuclk, hw) +#define to_rockchip_cpuclk_nb(nb) \
>> > +                       container_of(nb, struct rockchip_cpuclk, clk_nb)
>> > +
>> > +static const struct rockchip_cpuclk_rate_table
>> > *rockchip_get_cpuclk_settings( +                           struct
>> > rockchip_cpuclk *cpuclk, unsigned long rate) +{
>> > +       const struct rockchip_cpuclk_rate_table *rate_table =
>> > +
>> > cpuclk->rate_table;
>> > +       int i;
>> > +
>> > +       for (i = 0; i < cpuclk->rate_count; i++) {
>> > +               if (rate == rate_table[i].prate)
>> > +                       return &rate_table[i];
>> > +       }
>> > +
>> > +       return NULL;
>> > +}
>> > +
>> > +static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
>> > +                                       unsigned long parent_rate)
>> > +{
>> > +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_hw(hw);
>> > +       const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +       u32 clksel0 = readl_relaxed(cpuclk->reg_base +
>> > reg_data->core_reg);
>> > +
>> > +       clksel0 >>= reg_data->div_core_shift;
>> > +       clksel0 &= reg_data->div_core_mask;
>> > +       return parent_rate / (clksel0 + 1);
>> > +}
>> > +
>> > +static const struct clk_ops rockchip_cpuclk_ops = {
>> > +       .recalc_rate = rockchip_cpuclk_recalc_rate,
>> > +};
>> > +
>> > +static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk
>> > *cpuclk,
>> > +                                          struct clk_notifier_data
>> > *ndata)
>> > +{
>> > +       const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +       const struct rockchip_cpuclk_rate_table *rate;
>> > +       unsigned long alt_prate, alt_div;
>> > +       int i;
>> > +
>> > +       rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
>> > +       if (!rate) {
>> > +               pr_err("%s: Invalid rate : %lu for cpuclk\n",
>> > +                      __func__, ndata->new_rate);
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       alt_prate = clk_get_rate(cpuclk->alt_parent);
>> > +
>> > +       spin_lock(cpuclk->lock);
>> > +
>> > +       /*
>> > +        * If the old parent clock speed is less than the clock speed
>> > +        * of the alternate parent, then it should be ensured that at no
>> > point +        * the armclk speed is more than the old_rate until the
>> > dividers are +        * set.
>> > +        */
>> > +       if (alt_prate > ndata->old_rate) {
>> > +               /* calculate dividers */
>> > +               alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
>> > +               if (alt_div > reg_data->div_core_mask) {
>> > +                       pr_warn("%s: limiting alt-divider %lu to %d\n",
>> > +                               __func__, alt_div,
>> > reg_data->div_core_mask); +                       alt_div =
>> > reg_data->div_core_mask;
>> > +               }
>> > +
>> > +               /*
>> > +                * Change parents and add dividers in a single
>> > transaction.
>> > +                *
>> > +                * NOTE: we do this in a single transaction so we're never
>> > +                * dividing the primary parent by the extra dividers that
>> > were +                * needed for the alt.
>> > +                */
>> > +               pr_debug("%s: setting div %lu as alt-rate %lu > old-rate
>> > %lu\n", +                        __func__, alt_div, alt_prate,
>> > ndata->old_rate); +
>> > +               writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
>> > +                                             reg_data->div_core_shift) |
>> > +                      HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
>> > +                      cpuclk->reg_base + reg_data->core_reg);
>> > +       } else {
>> > +               /* select alternate parent */
>> > +               writel(HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
>> > +                       cpuclk->reg_base + reg_data->core_reg);
>> > +       }
>> > +
>> > +       /* alternate parent is active now. set the dividers */
>> > +       for (i = 0; i < ARRAY_SIZE(rate->divs); i++) {
>> > +               const struct rockchip_cpuclk_clksel *clksel =
>> > &rate->divs[i]; +
>> > +               if (!clksel->reg)
>> > +                       continue;
>> > +
>> > +               pr_debug("%s: setting reg 0x%x to 0x%x\n",
>> > +                        __func__, clksel->reg, clksel->val);
>> > +               writel(clksel->val , cpuclk->reg_base + clksel->reg);
>> > +       }
>>
>> I'm not 100% certain that you can always set the dividers here and
>> still be safe.
>>
>> Let's imagine that we're going 800MHz => 300MHz.  Let's say that we
>> have "divide by 3" for 800Mhz and "divide by 1" for 300Mhz.  Let's say
>> that the alternate PLL is 500Mhz.
>>
>> ...so at this point in the transition we've switched to the alternate
>> PLL (500Mhz) but we're now setting the dividers for 300MHz.  That
>> doesn't seem quite right.
>
> So setting dividers here when increasing the rate and in the post_rate notifier
> when lowering the rate?
>
> I think the samsung cpuclk had the same discussion at some point, but am not
> sure what the outcome was.

Hi Heiko,

In the samsung cpuclk implementation, the ratio between PLL output and
armclk is always 1:1. So the divider that divides the PLL output to
get the armclk clock is always set to zero (div by 1). That divider
gets a non-zero value only when using an alternate parent clock source
during rate change operations.

Thomas.

>
>
>> I also _think_ that these dividers are based on the PLL, not based on
>> the armclock, right.  In other words:
>>
>> 200MHz -> 300MHz with 500Mhz alternate PLL will do:
>>
>> 1. Switch core PLL to 500MHz alt with "div by 3", so 166MHz arm clock.  OK.
>>
>> 2. ...but the "div by 3" doesn't apply to MP AXI clock divider, M0 AXI
>> clock divider, etc.  That means _those_ are now based on 500MHz and
>> are now running faster than they were.
>>
>
> Nope, the dividers are clocks that are really children of the armclk. In the
> rk3288 trm on page 44 you can see the mux from apll and gpll, followed by the
> divider we use to keep the temporary rate below the old apll rate.
>
> The result of this is then supplied to the armclk [*] and the tightly bound
> clocks. So the "div by 3" when using the alternate parent will be used by both
> the arm-cores as well as the dependent clocks.
>
>
>
> [*] There is a slight variance between rk3288 and the Cortex-A9 SoCs. On
> rk3188 and before the result of the mux+divider described above are fed
> directly and unmodified to the arm cores, while the core clocks on rk3288 could
> use another divider on a per-core basis (see clk_core0..clk_core3 on the page
> 44 cited above).
>
> But it also doesn't matter much for this, as the coupled clocks are dependent
> on the shared parent of clk_coreX.
>
>
>>
>> None of that terribly matters for rk3288 which (currently) has the
>> same dividers for every rate point, but it seems like it might be
>> relevant for 3066 and 3188.  ...or if they ever need to change
>> dividers for rk3288.
>>
>> > +
>> > +       spin_unlock(cpuclk->lock);
>> > +       return 0;
>> > +}
>> > +
>> > +static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk
>> > *cpuclk, +                                           struct
>> > clk_notifier_data *ndata) +{
>> > +       const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +
>> > +       spin_lock(cpuclk->lock);
>> > +
>> > +       /*
>> > +        * post-rate change event, re-mux to primary parent and remove
>> > dividers. +        *
>> > +        * NOTE: we do this in a single transaction so we're never
>> > dividing the +        * primary parent by the extra dividers that were
>> > needed for the alt. +        */
>> > +
>> > +       writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
>> > +                               reg_data->div_core_shift) |
>> > +              HIWORD_UPDATE(0, 1, reg_data->mux_core_shift),
>> > +              cpuclk->reg_base + reg_data->core_reg);
>> > +
>> > +       spin_unlock(cpuclk->lock);
>> > +       return 0;
>> > +}
>> > +
>> > +/*
>> > + * This clock notifier is called when the frequency of the parent clock
>> > + * of cpuclk is to be changed. This notifier handles the setting up all
>> > + * the divider clocks, remux to temporary parent and handling the safe
>> > + * frequency levels when using temporary parent.
>> > + */
>> > +static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
>> > +                                       unsigned long event, void *data)
>> > +{
>> > +       struct clk_notifier_data *ndata = data;
>> > +       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
>> > +       int ret = 0;
>> > +
>> > +       pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
>> > +                __func__, event, ndata->old_rate, ndata->new_rate);
>> > +       if (event == PRE_RATE_CHANGE)
>> > +               ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
>> > +       else if (event == POST_RATE_CHANGE)
>> > +               ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
>> > +
>> > +       return notifier_from_errno(ret);
>> > +}
>> > +
>> > +struct clk *rockchip_clk_register_cpuclk(const char *name,
>> > +                       const char **parent_names, u8 num_parents,
>> > +                       const struct rockchip_cpuclk_reg_data *reg_data,
>> > +                       struct rockchip_cpuclk_rate_table *rate_table,
>> > +                       void __iomem *reg_base, spinlock_t *lock)
>> > +{
>> > +       struct rockchip_cpuclk *cpuclk;
>> > +       struct clk_init_data init;
>> > +       struct clk *clk, *cclk;
>> > +       int ret;
>> > +
>> > +       if (!reg_data) {
>> > +               pr_err("%s: no soc register information\n", __func__);
>> > +               return ERR_PTR(-EINVAL);
>> > +       }
>>
>> Do we really think it likely that we'll get passed a NULL reg_data?
>>
>> > +
>> > +       if (num_parents != 2) {
>> > +               pr_err("%s: needs two parent clocks\n", __func__);
>> > +               return ERR_PTR(-EINVAL);
>> > +       }
>>
>> I guess this is just future proofing things?  I notice that the most
>> recently posted exynos driver just takes two strings, the main parent
>> and an alternate parent.  That does seem slightly cleaner.
>
> As the regular clock register functions use the parent_names, num_parents form
> it somehow seemed cleaner to me to continue this, but I don't have a hard
> preference here.
>
>
>>
>> > +
>> > +       cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
>> > +       if (!cpuclk)
>> > +               return ERR_PTR(-ENOMEM);
>> > +
>> > +       init.name = name;
>> > +       init.parent_names = &parent_names[0];
>> > +       init.num_parents = 1;
>> > +       init.ops = &rockchip_cpuclk_ops;
>> > +
>> > +       /* only allow rate changes when we have a rate table */
>> > +       init.flags = rate_table ? CLK_SET_RATE_PARENT : 0;
>> > +
>> > +       /* disallow automatic parent changes by ccf */
>> > +       init.flags |= CLK_SET_RATE_NO_REPARENT;
>> > +
>> > +       init.flags |= CLK_GET_RATE_NOCACHE;
>> > +
>> > +       cpuclk->reg_base = reg_base;
>> > +       cpuclk->lock = lock;
>> > +       cpuclk->reg_data = reg_data;
>> > +       cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
>> > +       cpuclk->hw.init = &init;
>> > +
>> > +       cpuclk->alt_parent = __clk_lookup(parent_names[1]);
>> > +       if (!cpuclk->alt_parent) {
>> > +               pr_err("%s: could not lookup alternate parent\n",
>> > +                      __func__);
>> > +               ret = -EINVAL;
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       ret = clk_prepare_enable(cpuclk->alt_parent);
>> > +       if (ret) {
>> > +               pr_err("%s: could not enable alternate parent\n",
>> > +                      __func__);
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       clk = __clk_lookup(parent_names[0]);
>> > +       if (!clk) {
>> > +               pr_err("%s: could not lookup parent clock %s\n",
>> > +                      __func__, parent_names[0]);
>> > +               ret = -EINVAL;
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       ret = clk_notifier_register(clk, &cpuclk->clk_nb);
>> > +       if (ret) {
>> > +               pr_err("%s: failed to register clock notifier for %s\n",
>> > +                               __func__, name);
>> > +               goto free_cpuclk;
>> > +       }
>> > +
>> > +       if (rate_table) {
>> > +               int nrates;
>> > +
>> > +               /* find count of rates in rate_table */
>> > +               for (nrates = 0; rate_table[nrates].prate != 0; )
>> > +                       nrates++;
>> > +
>> > +               cpuclk->rate_count = nrates;
>> > +               cpuclk->rate_table = kmemdup(rate_table,
>> > +                                            sizeof(*rate_table) * nrates,
>> > +                                            GFP_KERNEL);
>>
>> Ah, I think I see what Derek was saying earlier.  It's a little
>> awkward that you pass this in as a table with a sentinel at the end
>> and then at runtime convert it to an array with a stored length.
>> Given that all of these are compiled-in tables, it wouldn't be crazy
>> to just pass in nrates (which is just ARRAY_SIZE in all callers) and
>> get rid of the sentinel.  ...but I won't insist on it.
>
> This would also be true for the pll rate tables then ... mixing concepts
> between such tables in clock drivers would be strange somehow.
>
> I guess this was originally simply getting a lot of inpsiration from the
> samsung pll-clock driver, which does use the same style (including the
> copying). Not sure if there was a rationale behind the original.
>
> Interestingly (I just looked it up) the new samsung cpuclk uses the rates +
> nrates approach.
>
> So I guess I would be ok with using the other approach as well :-)
>
>
> Heiko
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk

[Doug Anderson]

Thomas,

On Mon, Sep 22, 2014 at 10:25 PM, Thomas Abraham <ta.omasab@gmail.com> wrote:
> In the samsung cpuclk implementation, the ratio between PLL output and
> armclk is always 1:1. So the divider that divides the PLL output to
> get the armclk clock is always set to zero (div by 1). That divider
> gets a non-zero value only when using an alternate parent clock source
> during rate change operations.

The dividers we're discussing here are extra dividers.  In exynos
speak this would be things like "CPUD", "ATB", "PCLK_DBG".

-Doug

[PATCH v3 1/8] clk: rockchip: change pll rate without a clk-notifier

[Mike Turquette]

Quoting Heiko St?bner (2014-09-17 16:13:42)
> Am Mittwoch, 17. September 2014, 15:46:08 schrieb Doug Anderson:
> > Heiko,
> > 
> > On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > > From: Doug Anderson <dianders@chromium.org>
> > > 
> > > The Rockchip PLL code switches into slow mode (AKA bypass more AKA
> > > 24MHz mode) before actually changing the PLL.  This keeps anyone from
> > > using the PLL while it's changing.  However, in all known Rockchip
> > > SoCs nobody should ever see the 24MHz when changing the PLL supplying
> > > the armclk because we should reparent children to an alternate
> > > (faster than 24MHz) PLL.
> > > 
> > > One problem is that the code to switch to an alternate parent was
> > > running in PRE_RATE_CHANGE.  ...and the code to switch to slow mode
> > > was _also_ running in PRE_RATE_CHANGE.  That meant there was no real
> > > guarantee that we would switch to an alternate parent before switching
> > > to 24MHz mode.
> > > 
> > > Let's move the switch to "slow mode" straight into
> > > rockchip_rk3066_pll_set_rate().  That means we're guaranteed that the
> > > 24MHz is really a last-resort.
> > > 
> > > Note that without this change on real systems we were the code to
> > > switch to an alternate parent at 24MHz.  In some older versions of
> > > that code we'd appy a (temporary) / 5 to the 24MHz causing us to run
> > > at 4.8MHz.  That wasn't enough to service USB interrupts in some cases
> > > and could lead to a system hang.
> > > 
> > > Signed-off-by: Doug Anderson <dianders@chromium.org>
> > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > > 
> > >  drivers/clk/rockchip/clk-pll.c | 63
> > >  +++++++++--------------------------------- 1 file changed, 13
> > >  insertions(+), 50 deletions(-)
> > 
> > Thanks for adding my patch to your series (with the proper commit
> > message)!  I think you need your SoB on the patch too.  Andrew Morton
> > pointed to the docs in another patch I was involved in.  Specifically,
> > you were "on the patch delivery".  See Documentation/SubmittingPatches
> > section 12 (and 13).
> 
> ok ... Mike can you add the
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> 
> to the patch, or do you want a respin [if no other issue appears]

I can add it, but do you plan to spin another version of this series
with the changes to the comments?

Regards,
Mike

> 
> 
> Heiko