diff mbox

[v2,4/9] clk: rockchip: add new clock type and controller for rk3036

Message ID 1442478540-15068-5-git-send-email-zhengxing@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

zhengxing Sept. 17, 2015, 8:28 a.m. UTC 
The rk3036's pll and clock are different with base on the rk3066(rk3188,
rk3288, rk3368 use it), there are different adjust foctors and control
registers, so these should be independent and separate from the series
of rk3066s.

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

Changes in v2: None

 drivers/clk/rockchip/clk-pll.c |  262 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 261 insertions(+), 1 deletion(-)

Comments

Heiko Stübner Sept. 17, 2015, 9:54 a.m. UTC | #1 
Hi,

Am Donnerstag, 17. September 2015, 16:28:55 schrieb Xing Zheng:
> The rk3036's pll and clock are different with base on the rk3066(rk3188,
> rk3288, rk3368 use it), there are different adjust foctors and control
> registers, so these should be independent and separate from the series
> of rk3066s.
> 
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/clk/rockchip/clk-pll.c |  262
> +++++++++++++++++++++++++++++++++++++++- 1 file changed, 261 insertions(+),
> 1 deletion(-)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index 7737a1d..25b066a 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -2,6 +2,9 @@
>   * Copyright (c) 2014 MundoReader S.L.
>   * Author: Heiko Stuebner <heiko@sntech.de>
>   *
> + * Copyright (c) 2015 Rockchip Electronics Co. Ltd.
> + * Author: Xing Zheng <zhengxing@rock-chips.com>
> + *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; either version 2 of the License, or
> @@ -19,6 +22,7 @@
>  #include <linux/delay.h>
>  #include <linux/clk-provider.h>
>  #include <linux/regmap.h>
> +#include <linux/clk.h>
>  #include "clk.h"
> 
>  #define PLL_MODE_MASK		0x3
> @@ -306,6 +310,256 @@ static void rockchip_rk3066_pll_init(struct clk_hw
> *hw) }
>  }
> 
> +/**
> + * PLL used in RK3036
> + */
> +
> +#define RK3036_PLLCON(i)			(i * 0x4)
> +#define RK3036_PLLCON0_FBDIV_MASK		0xfff
> +#define RK3036_PLLCON0_FBDIV_SHIFT		0
> +#define RK3036_PLLCON0_POSTDIV1_MASK		0x7
> +#define RK3036_PLLCON0_POSTDIV1_SHIFT		12
> +#define RK3036_PLLCON1_REFDIV_MASK		0x3f
> +#define RK3036_PLLCON1_REFDIV_SHIFT		0
> +#define RK3036_PLLCON1_POSTDIV2_MASK		0x7
> +#define RK3036_PLLCON1_POSTDIV2_SHIFT		6
> +#define RK3036_PLLCON1_DSMPD_MASK		0x1
> +#define RK3036_PLLCON1_DSMPD_SHIFT		12
> +#define RK3036_PLLCON2_FRAC_MASK		0xffffff
> +#define RK3036_PLLCON2_FRAC_SHIFT		0
> +
> +#define RK3036_PLLCON1_PWRDOWN			(1 << 13)
> +#define RK3036_PLLCON1_LOCK_STATUS		(1 << 10)
> +
> +static int rockchip_rk3036_pll_wait_lock(struct rockchip_clk_pll *pll)
> +{
> +	u32 pllcon;
> +	int delay = 24000000;
> +
> +	/* poll check the lock status in rk3036 xPLLCON1 */
> +	while (delay > 0) {
> +		pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(1));
> +		if (pllcon & RK3036_PLLCON1_LOCK_STATUS)
> +			return 0;
> +
> +		delay--;
> +	}
> +
> +	pr_err("%s: timeout waiting for pll to lock\n", __func__);
> +	return -ETIMEDOUT;
> +}

Just saw that you responded to this in the v1 thread.

I don't necessarily object to this new lock function ... but if both the 
PLLCON1 and GRF_SOC_STATUS0 register provide the same information, I'd prefer 
to use the already existing solution.

[...]

> @@ -363,7 +617,7 @@ struct clk *rockchip_clk_register_pll(enum
> rockchip_pll_type pll_type, pll_mux->lock = lock;
>  	pll_mux->hw.init = &init;
> 
> -	if (pll_type == pll_rk3066)
> +	if (pll_type == pll_rk3066 || pll_type == pll_rk3036)

ordering please :-) (3036 before 3066)

>  		pll_mux->flags |= CLK_MUX_HIWORD_MASK;
> 
>  	/* the actual muxing is xin24m, pll-output, xin32k */
> @@ -414,6 +668,12 @@ struct clk *rockchip_clk_register_pll(enum
> rockchip_pll_type pll_type, else
>  			init.ops = &rockchip_rk3066_pll_clk_ops;
>  		break;
> +	case pll_rk3036:
> +		if (!pll->rate_table)
> +			init.ops = &rockchip_rk3036_pll_clk_norate_ops;
> +		else
> +			init.ops = &rockchip_rk3036_pll_clk_ops;
> +		break;

same here, 3036 before 3066 please


>  	default:
>  		pr_warn("%s: Unknown pll type for pll clk %s\n",
>  			__func__, name);


apart from these small issues, this looks great :-)


Thanks
Heiko
Stephen Boyd Sept. 22, 2015, 10:41 p.m. UTC | #2 
On 09/17, Xing Zheng wrote:
> +
> +static void rockchip_rk3036_pll_init(struct clk_hw *hw)

init ops are "discouraged". Could we do this through assigned
rates instead?

> +{
> +	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
> +	const struct rockchip_pll_rate_table *rate;
> +	unsigned int fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac;
> +	unsigned long drate;
> +	u32 pllcon;
> +
> +	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
> +		return;

I don't understand what this one does though. This check isn't in
the set rate ops.
Heiko Stübner Sept. 22, 2015, 10:58 p.m. UTC | #3 
Hi Stephen,

Am Dienstag, 22. September 2015, 15:41:25 schrieb Stephen Boyd:
> On 09/17, Xing Zheng wrote:
> > +
> > +static void rockchip_rk3036_pll_init(struct clk_hw *hw)
> 
> init ops are "discouraged". Could we do this through assigned
> rates instead?

really? According to Mike that was a valid use-case when we looked for an 
initial place for that on the rk3288 :-) .


> > +{
> > +	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
> > +	const struct rockchip_pll_rate_table *rate;
> > +	unsigned int fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac;
> > +	unsigned long drate;
> > +	u32 pllcon;
> > +
> > +	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
> > +		return;
> 
> I don't understand what this one does though. This check isn't in
> the set rate ops.

And it shouldn't be :-)

The issue this whole thing is trying to solve is aligning the pll settings 
which what we have in the rate table, not what the bootloader set.

For example the bootloader could set up a pll at 594MHz with one set of 
parameters and after some time - when you don't want to exchange bootloaders 
on shipping devices anymore - it comes to light that a different set of 
parameters for the same frequency produces for example a more stable hdmi 
signal [I think that was the main reason for the initial change].

So we're not changing the frequency x -> y, which could be easily done [and is 
done already] via assigned-rates, but instead
	x {params a,b,c} -> x {params d,e,f}
so the rate itself stays the same, only the frequency generation is adapted.

As for the ROCKCHIP_PLL_SYNC_RATE param, we of course don't want to sync the 
ddr-pll for example for obvious hang-reasons.


Heiko
Stephen Boyd Sept. 22, 2015, 11:19 p.m. UTC | #4 
On 09/23, Heiko Stübner wrote:
> Hi Stephen,
> 
> Am Dienstag, 22. September 2015, 15:41:25 schrieb Stephen Boyd:
> > On 09/17, Xing Zheng wrote:
> > > +
> > > +static void rockchip_rk3036_pll_init(struct clk_hw *hw)
> > 
> > init ops are "discouraged". Could we do this through assigned
> > rates instead?
> 
> really? According to Mike that was a valid use-case when we looked for an 
> initial place for that on the rk3288 :-) .

A comment in clk.c indicates init ops are discouraged. Maybe this
is a valid use-case on other platforms so it was allowed, but
pretty much every time we see a new init op we have to think
about it and justify it. Hooray!

> 
> 
> > > +{
> > > +	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
> > > +	const struct rockchip_pll_rate_table *rate;
> > > +	unsigned int fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac;
> > > +	unsigned long drate;
> > > +	u32 pllcon;
> > > +
> > > +	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
> > > +		return;
> > 
> > I don't understand what this one does though. This check isn't in
> > the set rate ops.
> 
> And it shouldn't be :-)
> 
> The issue this whole thing is trying to solve is aligning the pll settings 
> which what we have in the rate table, not what the bootloader set.
> 
> For example the bootloader could set up a pll at 594MHz with one set of 
> parameters and after some time - when you don't want to exchange bootloaders 
> on shipping devices anymore - it comes to light that a different set of 
> parameters for the same frequency produces for example a more stable hdmi 
> signal [I think that was the main reason for the initial change].
> 
> So we're not changing the frequency x -> y, which could be easily done [and is 
> done already] via assigned-rates, but instead
> 	x {params a,b,c} -> x {params d,e,f}
> so the rate itself stays the same, only the frequency generation is adapted.

Ok. It would be nice if this sort of information was made into a
comment and put in the code. Or at least the commit text for the
change.

And is there any reason that we need to get the parent clock and
parent rate to align the PLL settings? It would be nice if we
avoided using clk_* APIs in here, by extracting the pll set rate
code into another function that we can call from init to make the
values the same without all the fallback to old rates, etc.
diff mbox

Patch

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 7737a1d..25b066a 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -2,6 +2,9 @@ 
  * Copyright (c) 2014 MundoReader S.L.
  * Author: Heiko Stuebner <heiko@sntech.de>
  *
+ * Copyright (c) 2015 Rockchip Electronics Co. Ltd.
+ * Author: Xing Zheng <zhengxing@rock-chips.com>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -19,6 +22,7 @@ 
 #include <linux/delay.h>
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
+#include <linux/clk.h>
 #include "clk.h"
 
 #define PLL_MODE_MASK		0x3
@@ -306,6 +310,256 @@  static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 	}
 }
 
+/**
+ * PLL used in RK3036
+ */
+
+#define RK3036_PLLCON(i)			(i * 0x4)
+#define RK3036_PLLCON0_FBDIV_MASK		0xfff
+#define RK3036_PLLCON0_FBDIV_SHIFT		0
+#define RK3036_PLLCON0_POSTDIV1_MASK		0x7
+#define RK3036_PLLCON0_POSTDIV1_SHIFT		12
+#define RK3036_PLLCON1_REFDIV_MASK		0x3f
+#define RK3036_PLLCON1_REFDIV_SHIFT		0
+#define RK3036_PLLCON1_POSTDIV2_MASK		0x7
+#define RK3036_PLLCON1_POSTDIV2_SHIFT		6
+#define RK3036_PLLCON1_DSMPD_MASK		0x1
+#define RK3036_PLLCON1_DSMPD_SHIFT		12
+#define RK3036_PLLCON2_FRAC_MASK		0xffffff
+#define RK3036_PLLCON2_FRAC_SHIFT		0
+
+#define RK3036_PLLCON1_PWRDOWN			(1 << 13)
+#define RK3036_PLLCON1_LOCK_STATUS		(1 << 10)
+
+static int rockchip_rk3036_pll_wait_lock(struct rockchip_clk_pll *pll)
+{
+	u32 pllcon;
+	int delay = 24000000;
+
+	/* poll check the lock status in rk3036 xPLLCON1 */
+	while (delay > 0) {
+		pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(1));
+		if (pllcon & RK3036_PLLCON1_LOCK_STATUS)
+			return 0;
+
+		delay--;
+	}
+
+	pr_err("%s: timeout waiting for pll to lock\n", __func__);
+	return -ETIMEDOUT;
+}
+
+static unsigned long rockchip_rk3036_pll_recalc_rate(struct clk_hw *hw,
+						     unsigned long prate)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	unsigned int fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac;
+	u64 rate64 = prate;
+	u32 pllcon;
+
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(0));
+	fbdiv = ((pllcon >> RK3036_PLLCON0_FBDIV_SHIFT) & RK3036_PLLCON0_FBDIV_MASK);
+	postdiv1 = ((pllcon >> RK3036_PLLCON0_POSTDIV1_SHIFT) & RK3036_PLLCON0_POSTDIV1_MASK);
+
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(1));
+	refdiv = ((pllcon >> RK3036_PLLCON1_REFDIV_SHIFT) & RK3036_PLLCON1_REFDIV_MASK);
+	postdiv2 = ((pllcon >> RK3036_PLLCON1_POSTDIV2_SHIFT) & RK3036_PLLCON1_POSTDIV2_MASK);
+	dsmpd = ((pllcon >> RK3036_PLLCON1_DSMPD_SHIFT) & RK3036_PLLCON1_DSMPD_MASK);
+
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(2));
+	frac = ((pllcon >> RK3036_PLLCON2_FRAC_SHIFT) & RK3036_PLLCON2_FRAC_MASK);
+
+	rate64 *= fbdiv;
+	do_div(rate64, refdiv);
+
+	if (dsmpd == 0) {
+		/* fractional mode */
+		u64 frac_rate64 = prate * frac;
+
+		do_div(frac_rate64, refdiv);
+		rate64 += frac_rate64 >> 24;
+	}
+
+	do_div(rate64, postdiv1);
+	do_div(rate64, postdiv2);
+
+	return (unsigned long)rate64;
+}
+
+static int rockchip_rk3036_pll_set_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long prate)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	const struct rockchip_pll_rate_table *rate;
+	unsigned long old_rate = rockchip_rk3036_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;
+	u32 pllcon;
+	int rate_change_remuxed = 0;
+	int cur_parent;
+	int ret;
+
+	if (IS_ERR(grf)) {
+		pr_debug("%s: grf regmap not available, aborting rate change\n",
+			 __func__);
+		return PTR_ERR(grf);
+	}
+
+	pr_debug("%s: changing %s from %lu to %lu with a parent rate of %lu\n",
+		 __func__, __clk_get_name(hw->clk), old_rate, drate, prate);
+
+	/* Get required rate settings from table */
+	rate = rockchip_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, __clk_get_name(hw->clk));
+		return -EINVAL;
+	}
+
+	pr_debug("%s: rate settings for %lu fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		__func__, rate->rate,
+		rate->fbdiv, rate->postdiv1, rate->refdiv, rate->postdiv2, rate->dsmpd, rate->frac);
+
+	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;
+	}
+
+	/* update pll values */
+	writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3036_PLLCON0_FBDIV_MASK,
+					  RK3036_PLLCON0_FBDIV_SHIFT) |
+		       HIWORD_UPDATE(rate->postdiv1, RK3036_PLLCON0_POSTDIV1_MASK,
+					     RK3036_PLLCON0_POSTDIV1_SHIFT),
+		       pll->reg_base + RK3036_PLLCON(0));
+
+	writel_relaxed(HIWORD_UPDATE(rate->refdiv, RK3036_PLLCON1_REFDIV_MASK,
+						   RK3036_PLLCON1_REFDIV_SHIFT) |
+		       HIWORD_UPDATE(rate->postdiv2, RK3036_PLLCON1_POSTDIV2_MASK,
+						     RK3036_PLLCON1_POSTDIV2_SHIFT) |
+		       HIWORD_UPDATE(rate->dsmpd, RK3036_PLLCON1_DSMPD_MASK,
+						  RK3036_PLLCON1_DSMPD_SHIFT),
+		       pll->reg_base + RK3036_PLLCON(1));
+
+	/* GPLL CON2 is not HIWORD_MASK */
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(2));
+	pllcon &= ~(RK3036_PLLCON2_FRAC_MASK << RK3036_PLLCON2_FRAC_SHIFT);
+	pllcon |= rate->frac << RK3036_PLLCON2_FRAC_SHIFT;
+	writel_relaxed(pllcon, pll->reg_base + RK3036_PLLCON(2));
+
+	/* wait for the pll to lock */
+	ret = rockchip_rk3036_pll_wait_lock(pll);
+	if (ret) {
+		pr_warn("%s: pll did not lock, trying to restore old rate %lu\n",
+			__func__, old_rate);
+		rockchip_rk3036_pll_set_rate(hw, old_rate, prate);
+	}
+
+	if (rate_change_remuxed)
+		pll_mux_ops->set_parent(&pll_mux->hw, PLL_MODE_NORM);
+
+	return ret;
+}
+
+static int rockchip_rk3036_pll_enable(struct clk_hw *hw)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+
+	writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 0),
+	       pll->reg_base + RK3036_PLLCON(1));
+
+	return 0;
+}
+
+static void rockchip_rk3036_pll_disable(struct clk_hw *hw)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+
+	writel(HIWORD_UPDATE(RK3036_PLLCON1_PWRDOWN,
+			     RK3036_PLLCON1_PWRDOWN, 0),
+	       pll->reg_base + RK3036_PLLCON(1));
+}
+
+static int rockchip_rk3036_pll_is_enabled(struct clk_hw *hw)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	u32 pllcon = readl(pll->reg_base + RK3036_PLLCON(1));
+
+	return !(pllcon & RK3036_PLLCON1_PWRDOWN);
+}
+
+static void rockchip_rk3036_pll_init(struct clk_hw *hw)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	const struct rockchip_pll_rate_table *rate;
+	unsigned int fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac;
+	unsigned long drate;
+	u32 pllcon;
+
+	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
+		return;
+
+	drate = clk_hw_get_rate(hw);
+	rate = rockchip_get_pll_settings(pll, drate);
+
+	/* when no rate setting for the current rate, rely on clk_set_rate */
+	if (!rate)
+		return;
+
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(0));
+	fbdiv = ((pllcon >> RK3036_PLLCON0_FBDIV_SHIFT) & RK3036_PLLCON0_FBDIV_MASK);
+	postdiv1 = ((pllcon >> RK3036_PLLCON0_POSTDIV1_SHIFT) & RK3036_PLLCON0_POSTDIV1_MASK);
+
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(1));
+	refdiv = ((pllcon >> RK3036_PLLCON1_REFDIV_SHIFT) & RK3036_PLLCON1_REFDIV_MASK);
+	postdiv2 = ((pllcon >> RK3036_PLLCON1_POSTDIV2_SHIFT) & RK3036_PLLCON1_POSTDIV2_MASK);
+	dsmpd = ((pllcon >> RK3036_PLLCON1_DSMPD_SHIFT) & RK3036_PLLCON1_DSMPD_MASK);
+
+	pllcon = readl_relaxed(pll->reg_base + RK3036_PLLCON(2));
+	frac = ((pllcon >> RK3036_PLLCON2_FRAC_SHIFT) & RK3036_PLLCON2_FRAC_MASK);
+
+	pr_debug("%s: pll %s@%lu: Hz\n", __func__, __clk_get_name(hw->clk), drate);
+	pr_debug("old - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac);
+	pr_debug("new - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		rate->fbdiv, rate->postdiv1, rate->refdiv, rate->postdiv2, rate->dsmpd, rate->frac);
+
+	if (rate->fbdiv != fbdiv || rate->postdiv1 != postdiv1 || rate->refdiv != refdiv ||
+		rate->postdiv2 != postdiv2 || rate->dsmpd != dsmpd || rate->frac != frac) {
+		struct clk *parent = clk_get_parent(hw->clk);
+		unsigned long prate;
+
+		if (!parent) {
+			pr_warn("%s: parent of %s not available\n",
+				__func__, __clk_get_name(hw->clk));
+			return;
+		}
+
+		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
+			 __func__, __clk_get_name(hw->clk));
+		prate = clk_get_rate(parent);
+		rockchip_rk3036_pll_set_rate(hw, drate, prate);
+	}
+}
+
+static const struct clk_ops rockchip_rk3036_pll_clk_norate_ops = {
+	.recalc_rate = rockchip_rk3036_pll_recalc_rate,
+	.enable = rockchip_rk3036_pll_enable,
+	.disable = rockchip_rk3036_pll_disable,
+	.is_enabled = rockchip_rk3036_pll_is_enabled,
+};
+
+static const struct clk_ops rockchip_rk3036_pll_clk_ops = {
+	.recalc_rate = rockchip_rk3036_pll_recalc_rate,
+	.round_rate = rockchip_pll_round_rate,
+	.set_rate = rockchip_rk3036_pll_set_rate,
+	.enable = rockchip_rk3036_pll_enable,
+	.disable = rockchip_rk3036_pll_disable,
+	.is_enabled = rockchip_rk3036_pll_is_enabled,
+	.init = rockchip_rk3036_pll_init,
+};
+
 static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = {
 	.recalc_rate = rockchip_rk3066_pll_recalc_rate,
 	.enable = rockchip_rk3066_pll_enable,
@@ -363,7 +617,7 @@  struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 	pll_mux->lock = lock;
 	pll_mux->hw.init = &init;
 
-	if (pll_type == pll_rk3066)
+	if (pll_type == pll_rk3066 || pll_type == pll_rk3036)
 		pll_mux->flags |= CLK_MUX_HIWORD_MASK;
 
 	/* the actual muxing is xin24m, pll-output, xin32k */
@@ -414,6 +668,12 @@  struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,
 		else
 			init.ops = &rockchip_rk3066_pll_clk_ops;
 		break;
+	case pll_rk3036:
+		if (!pll->rate_table)
+			init.ops = &rockchip_rk3036_pll_clk_norate_ops;
+		else
+			init.ops = &rockchip_rk3036_pll_clk_ops;
+		break;
 	default:
 		pr_warn("%s: Unknown pll type for pll clk %s\n",
 			__func__, name);