Patchwork [RFC,v2] ARM: i.MX pllv1: Implement set_rate

login
register
mail settings
Submitter Alexander Shiyan
Date July 20, 2013, 8:29 a.m.
Message ID <1374308975-20596-1-git-send-email-shc_work@mail.ru>
Download mbox | patch
Permalink /patch/260422/
State New
Headers show

Comments

Alexander Shiyan - July 20, 2013, 8:29 a.m.
This patch implements frequency change for i.MX pllv1.
Selection of the values of the registers is not optimal, since is
made by simple enumeration of all possible values.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/mach-imx/clk-imx27.c |   2 +-
 arch/arm/mach-imx/clk-pllv1.c | 118 +++++++++++++++++++++++++++++++++++-------
 arch/arm/mach-imx/mx27.h      |   7 ++-
 3 files changed, 106 insertions(+), 21 deletions(-)
Sascha Hauer - July 20, 2013, 11:16 a.m.
On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> This patch implements frequency change for i.MX pllv1.
> Selection of the values of the registers is not optimal, since is
> made by simple enumeration of all possible values.

You do not mention why you want to adjust the rate. Normally we've been
happy with the static values from the bootloader.

> -static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
> -		unsigned long parent_rate)
> +static unsigned long _clk_pllv1_recalc_rate(unsigned int mfi, unsigned int mfn,
> +					    unsigned int mfd, unsigned int pd,
> +					    unsigned long parent_rate)
>  {
> -	struct clk_pllv1 *pll = to_clk_pllv1(hw);
> -	long long ll;
> -	int mfn_abs;
> -	unsigned int mfi, mfn, mfd, pd;
> -	u32 reg;
>  	unsigned long rate;
> -
> -	reg = readl(pll->base);
> +	int mfn_abs = mfn;
> +	long long ll;
>  
>  	/*
>  	 * Get the resulting clock rate from a PLL register value and the input
> @@ -47,15 +45,6 @@ static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
>  	 *                        pd + 1
>  	 */
>  
> -	mfi = (reg >> 10) & 0xf;
> -	mfn = reg & 0x3ff;
> -	mfd = (reg >> 16) & 0x3ff;
> -	pd =  (reg >> 26) & 0xf;
> -
> -	mfi = mfi <= 5 ? 5 : mfi;
> -
> -	mfn_abs = mfn;
> -
>  	/*
>  	 * On all i.MXs except i.MX1 and i.MX21 mfn is a 10bit
>  	 * 2's complements number
> @@ -78,8 +67,99 @@ static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
>  	return ll;
>  }
>  
> +static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
> +		unsigned long parent_rate)
> +{
> +	struct clk_pllv1 *pll = to_clk_pllv1(hw);
> +	unsigned int mfi, mfn, mfd, pd;
> +	u32 reg = readl(pll->base);
> +
> +	/* Avoid i.MX27 TO2 SPCTL0 bug */
> +	if (cpu_is_mx27() && (mx27_revision() == IMX_CHIP_REVISION_2_0))
> +		writel(reg, pll->base);

I remember this was fixed and got lost. If you want to have fixed it
again this should be a separate paatch along with a description of the
bug in the commit log.

> +
> +	mfi = (reg >> 10) & 0xf;
> +	mfn = reg & 0x3ff;
> +	mfd = (reg >> 16) & 0x3ff;
> +	pd =  (reg >> 26) & 0xf;
> +
> +	mfi = mfi <= 5 ? 5 : mfi;
> +
> +	return _clk_pllv1_recalc_rate(mfi, mfn, mfd, pd, parent_rate);
> +}
> +
> +static void _clk_pllv1_set_rate(unsigned long rate, unsigned long parent_rate,
> +				unsigned int *reg)
> +{
> +	unsigned int pd, mfi, mfn, mfd, mfn_offs = 0;
> +	long tmp, res, best = 0;
> +
> +	pd = (parent_rate * 2 * 5) / rate;
> +	if (pd > 15)
> +		pd = 15;
> +	mfi = rate / (parent_rate * 2 * (pd + 1));
> +	if (mfi < 5)
> +		mfi = 5;
> +	if (mfi < 15) {
> +		tmp = parent_rate * 2;
> +		tmp /= pd + 1;
> +		tmp *= mfi;
> +		if (tmp / parent_rate) {
> +			mfi++;
> +			/* Use negative values */
> +			mfn_offs = 0x200;
> +		}
> +	} else if (mfi > 15)
> +		mfi = 15;

Use braces on both branches here.

Sascha
Alexander Shiyan - July 20, 2013, 11:31 a.m.
On Sat, 20 Jul 2013 13:16:39 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > This patch implements frequency change for i.MX pllv1.
> > Selection of the values of the registers is not optimal, since is
> > made by simple enumeration of all possible values.
> 
> You do not mention why you want to adjust the rate. Normally we've been
> happy with the static values from the bootloader.

For CPUfreq.
Only one barrier to make it workable.

[...]
> > +	/* Avoid i.MX27 TO2 SPCTL0 bug */
> > +	if (cpu_is_mx27() && (mx27_revision() == IMX_CHIP_REVISION_2_0))
> > +		writel(reg, pll->base);
> 
> I remember this was fixed and got lost. If you want to have fixed it
> again this should be a separate paatch along with a description of the
> bug in the commit log.

This part of the patch can be completely removed ;)
I'm more interested in a more efficient algorithm for finding the best
values for the PLL.

[...]
Sascha Hauer - July 20, 2013, 11:58 a.m.
On Sat, Jul 20, 2013 at 03:31:04PM +0400, Alexander Shiyan wrote:
> On Sat, 20 Jul 2013 13:16:39 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > > This patch implements frequency change for i.MX pllv1.
> > > Selection of the values of the registers is not optimal, since is
> > > made by simple enumeration of all possible values.
> > 
> > You do not mention why you want to adjust the rate. Normally we've been
> > happy with the static values from the bootloader.
> 
> For CPUfreq.
> Only one barrier to make it workable.

Isn't it better to adjust the dividers only? I haven't made good
experience with adjusting the PLLs.

Sascha
Alexander Shiyan - July 20, 2013, 12:18 p.m.
On Sat, 20 Jul 2013 13:58:29 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> > > On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > > > This patch implements frequency change for i.MX pllv1.
> > > > Selection of the values of the registers is not optimal, since is
> > > > made by simple enumeration of all possible values.
> > > 
> > > You do not mention why you want to adjust the rate. Normally we've been
> > > happy with the static values from the bootloader.
> > 
> > For CPUfreq.
> > Only one barrier to make it workable.
> 
> Isn't it better to adjust the dividers only? I haven't made good
> experience with adjusting the PLLs.

The frequency value after booting can be quite arbitrary.

Since we define possible frequencies for the CPU (266 and 399 MHz),
I can offer as variant to use a fixed constant values for PLL.
Sascha Hauer - July 21, 2013, 12:37 p.m.
On Sat, Jul 20, 2013 at 04:18:01PM +0400, Alexander Shiyan wrote:
> On Sat, 20 Jul 2013 13:58:29 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > > > On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > > > > This patch implements frequency change for i.MX pllv1.
> > > > > Selection of the values of the registers is not optimal, since is
> > > > > made by simple enumeration of all possible values.
> > > > 
> > > > You do not mention why you want to adjust the rate. Normally we've been
> > > > happy with the static values from the bootloader.
> > > 
> > > For CPUfreq.
> > > Only one barrier to make it workable.
> > 
> > Isn't it better to adjust the dividers only? I haven't made good
> > experience with adjusting the PLLs.
> 
> The frequency value after booting can be quite arbitrary.
> 
> Since we define possible frequencies for the CPU (266 and 399 MHz),
> I can offer as variant to use a fixed constant values for PLL.

AFAIK the PLL should run at 800MHz. With dividers of 2, 3 and 6 we get
400, 266 and 133MHz. I don't think having more fine grained control
about the CPU frequency is necessary from a power saving point of view.
Let's keep the variants to a minimum to make the whole stuff more
robust.

Sascha
Alexander Shiyan - July 22, 2013, 3:18 p.m.
On Sun, 21 Jul 2013 14:37:59 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Sat, Jul 20, 2013 at 04:18:01PM +0400, Alexander Shiyan wrote:
> > On Sat, 20 Jul 2013 13:58:29 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > > > > On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > > > > > This patch implements frequency change for i.MX pllv1.
> > > > > > Selection of the values of the registers is not optimal, since is
> > > > > > made by simple enumeration of all possible values.
> > > > > 
> > > > > You do not mention why you want to adjust the rate. Normally we've been
> > > > > happy with the static values from the bootloader.
> > > > 
> > > > For CPUfreq.
> > > > Only one barrier to make it workable.
> > > 
> > > Isn't it better to adjust the dividers only? I haven't made good
> > > experience with adjusting the PLLs.
> > 
> > The frequency value after booting can be quite arbitrary.
> > 
> > Since we define possible frequencies for the CPU (266 and 399 MHz),
> > I can offer as variant to use a fixed constant values for PLL.
> 
> AFAIK the PLL should run at 800MHz. With dividers of 2, 3 and 6 we get
> 400, 266 and 133MHz. I don't think having more fine grained control
> about the CPU frequency is necessary from a power saving point of view.
> Let's keep the variants to a minimum to make the whole stuff more
> robust.

What about switching the source?
In any case, I have already made an efficient algorithm to calculate
the PLL and after some tests, I will introduce it.
Thanks.
Lucas Stach - July 22, 2013, 3:24 p.m.
Am Montag, den 22.07.2013, 19:18 +0400 schrieb Alexander Shiyan:
> On Sun, 21 Jul 2013 14:37:59 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Sat, Jul 20, 2013 at 04:18:01PM +0400, Alexander Shiyan wrote:
> > > On Sat, 20 Jul 2013 13:58:29 +0200
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > 
> > > > > > On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > > > > > > This patch implements frequency change for i.MX pllv1.
> > > > > > > Selection of the values of the registers is not optimal, since is
> > > > > > > made by simple enumeration of all possible values.
> > > > > > 
> > > > > > You do not mention why you want to adjust the rate. Normally we've been
> > > > > > happy with the static values from the bootloader.
> > > > > 
> > > > > For CPUfreq.
> > > > > Only one barrier to make it workable.
> > > > 
> > > > Isn't it better to adjust the dividers only? I haven't made good
> > > > experience with adjusting the PLLs.
> > > 
> > > The frequency value after booting can be quite arbitrary.
> > > 
> > > Since we define possible frequencies for the CPU (266 and 399 MHz),
> > > I can offer as variant to use a fixed constant values for PLL.
> > 
> > AFAIK the PLL should run at 800MHz. With dividers of 2, 3 and 6 we get
> > 400, 266 and 133MHz. I don't think having more fine grained control
> > about the CPU frequency is necessary from a power saving point of view.
> > Let's keep the variants to a minimum to make the whole stuff more
> > robust.
> 
> What about switching the source?
> In any case, I have already made an efficient algorithm to calculate
> the PLL and after some tests, I will introduce it.
> Thanks.
> 
Both changing the divider or changing source should be glitchfree in the
CPU clock path. Using the divider should be enough to allow to scale the
CPU frequency with the accuracy needed for simple CPUfreq.

Using the PLL to scale the frequency is a really bad idea, as it's both
not glitchfree on it's own plus it adds considerable delay to the clock
change. In order to use the PLL for clock scaling you have to first
switch the CPU away from the PLL (making sure you have an alternative
clock path that can satisfy the CPUs requirements), then reprogram PLL,
wait for the PLL to lock, then switch back to the PLL. Sounds utterly
complex for a simple clock change that could be done by just flipping
the divider bits, isn't it?

Regards,
Lucas
Alexander Shiyan - July 22, 2013, 3:40 p.m.
On Mon, 22 Jul 2013 17:24:35 +0200
Lucas Stach <l.stach@pengutronix.de> wrote:

> Am Montag, den 22.07.2013, 19:18 +0400 schrieb Alexander Shiyan:
> > On Sun, 21 Jul 2013 14:37:59 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > > On Sat, Jul 20, 2013 at 04:18:01PM +0400, Alexander Shiyan wrote:
> > > > On Sat, 20 Jul 2013 13:58:29 +0200
> > > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > > 
> > > > > > > On Sat, Jul 20, 2013 at 12:29:35PM +0400, Alexander Shiyan wrote:
> > > > > > > > This patch implements frequency change for i.MX pllv1.
> > > > > > > > Selection of the values of the registers is not optimal, since is
> > > > > > > > made by simple enumeration of all possible values.
> > > > > > > 
> > > > > > > You do not mention why you want to adjust the rate. Normally we've been
> > > > > > > happy with the static values from the bootloader.
> > > > > > 
> > > > > > For CPUfreq.
> > > > > > Only one barrier to make it workable.
> > > > > 
> > > > > Isn't it better to adjust the dividers only? I haven't made good
> > > > > experience with adjusting the PLLs.
> > > > 
> > > > The frequency value after booting can be quite arbitrary.
> > > > 
> > > > Since we define possible frequencies for the CPU (266 and 399 MHz),
> > > > I can offer as variant to use a fixed constant values for PLL.
> > > 
> > > AFAIK the PLL should run at 800MHz. With dividers of 2, 3 and 6 we get
> > > 400, 266 and 133MHz. I don't think having more fine grained control
> > > about the CPU frequency is necessary from a power saving point of view.
> > > Let's keep the variants to a minimum to make the whole stuff more
> > > robust.
> > 
> > What about switching the source?
> > In any case, I have already made an efficient algorithm to calculate
> > the PLL and after some tests, I will introduce it.
> > Thanks.
> > 
> Both changing the divider or changing source should be glitchfree in the
> CPU clock path. Using the divider should be enough to allow to scale the
> CPU frequency with the accuracy needed for simple CPUfreq.
> 
> Using the PLL to scale the frequency is a really bad idea, as it's both
> not glitchfree on it's own plus it adds considerable delay to the clock
> change. In order to use the PLL for clock scaling you have to first
> switch the CPU away from the PLL (making sure you have an alternative
> clock path that can satisfy the CPUs requirements), then reprogram PLL,
> wait for the PLL to lock, then switch back to the PLL. Sounds utterly
> complex for a simple clock change that could be done by just flipping
> the divider bits, isn't it?

It sounds logical, but the datasheet (i.MX27) says nothing about this order:

The i.MX27 device provides a way for software to save power under different
operating conditions.
- Software determines whether to change the operation frequency or not.
- Software uses DPTC to determine whether to reduce or increase the power supply.
- After the power supply has been changed, software can update MPLL configuration.
- It restarts the MPLL and operates with new frequency.

Patch

diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
index c3cfa41..2b363d9 100644
--- a/arch/arm/mach-imx/clk-imx27.c
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -125,7 +125,7 @@  int __init mx27_clocks_init(unsigned long fref)
 	clk[vpu_sel] = imx_clk_mux("vpu_sel", CCM_CSCR, 21, 1, vpu_sel_clks, ARRAY_SIZE(vpu_sel_clks));
 	clk[vpu_div] = imx_clk_divider("vpu_div", "vpu_sel", CCM_PCDR0, 10, 6);
 	clk[usb_div] = imx_clk_divider("usb_div", "spll_gate", CCM_CSCR, 28, 3);
-	clk[cpu_sel] = imx_clk_mux("cpu_sel", CCM_CSCR, 15, 1, cpu_sel_clks, ARRAY_SIZE(cpu_sel_clks));
+	clk[cpu_sel] = imx_clk_mux_flags("cpu_sel", CCM_CSCR, 15, 1, cpu_sel_clks, ARRAY_SIZE(cpu_sel_clks), CLK_SET_RATE_PARENT);
 	clk[clko_sel] = imx_clk_mux("clko_sel", CCM_CCSR, 0, 5, clko_sel_clks, ARRAY_SIZE(clko_sel_clks));
 	if (mx27_revision() >= IMX_CHIP_REVISION_2_0)
 		clk[cpu_div] = imx_clk_divider("cpu_div", "cpu_sel", CCM_CSCR, 12, 2);
diff --git a/arch/arm/mach-imx/clk-pllv1.c b/arch/arm/mach-imx/clk-pllv1.c
index c1eaee3..722fea2 100644
--- a/arch/arm/mach-imx/clk-pllv1.c
+++ b/arch/arm/mach-imx/clk-pllv1.c
@@ -9,6 +9,8 @@ 
 #include "common.h"
 #include "hardware.h"
 
+#include "mx27.h"
+
 /**
  * pll v1
  *
@@ -25,17 +27,13 @@  struct clk_pllv1 {
 
 #define to_clk_pllv1(clk) (container_of(clk, struct clk_pllv1, clk))
 
-static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
-		unsigned long parent_rate)
+static unsigned long _clk_pllv1_recalc_rate(unsigned int mfi, unsigned int mfn,
+					    unsigned int mfd, unsigned int pd,
+					    unsigned long parent_rate)
 {
-	struct clk_pllv1 *pll = to_clk_pllv1(hw);
-	long long ll;
-	int mfn_abs;
-	unsigned int mfi, mfn, mfd, pd;
-	u32 reg;
 	unsigned long rate;
-
-	reg = readl(pll->base);
+	int mfn_abs = mfn;
+	long long ll;
 
 	/*
 	 * Get the resulting clock rate from a PLL register value and the input
@@ -47,15 +45,6 @@  static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
 	 *                        pd + 1
 	 */
 
-	mfi = (reg >> 10) & 0xf;
-	mfn = reg & 0x3ff;
-	mfd = (reg >> 16) & 0x3ff;
-	pd =  (reg >> 26) & 0xf;
-
-	mfi = mfi <= 5 ? 5 : mfi;
-
-	mfn_abs = mfn;
-
 	/*
 	 * On all i.MXs except i.MX1 and i.MX21 mfn is a 10bit
 	 * 2's complements number
@@ -78,8 +67,99 @@  static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
 	return ll;
 }
 
+static unsigned long clk_pllv1_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	struct clk_pllv1 *pll = to_clk_pllv1(hw);
+	unsigned int mfi, mfn, mfd, pd;
+	u32 reg = readl(pll->base);
+
+	/* Avoid i.MX27 TO2 SPCTL0 bug */
+	if (cpu_is_mx27() && (mx27_revision() == IMX_CHIP_REVISION_2_0))
+		writel(reg, pll->base);
+
+	mfi = (reg >> 10) & 0xf;
+	mfn = reg & 0x3ff;
+	mfd = (reg >> 16) & 0x3ff;
+	pd =  (reg >> 26) & 0xf;
+
+	mfi = mfi <= 5 ? 5 : mfi;
+
+	return _clk_pllv1_recalc_rate(mfi, mfn, mfd, pd, parent_rate);
+}
+
+static void _clk_pllv1_set_rate(unsigned long rate, unsigned long parent_rate,
+				unsigned int *reg)
+{
+	unsigned int pd, mfi, mfn, mfd, mfn_offs = 0;
+	long tmp, res, best = 0;
+
+	pd = (parent_rate * 2 * 5) / rate;
+	if (pd > 15)
+		pd = 15;
+	mfi = rate / (parent_rate * 2 * (pd + 1));
+	if (mfi < 5)
+		mfi = 5;
+	if (mfi < 15) {
+		tmp = parent_rate * 2;
+		tmp /= pd + 1;
+		tmp *= mfi;
+		if (tmp / parent_rate) {
+			mfi++;
+			/* Use negative values */
+			mfn_offs = 0x200;
+		}
+	} else if (mfi > 15)
+		mfi = 15;
+
+	for (mfd = 1; mfd < 0x400; mfd++)
+		for (mfn = 0; (mfn < mfd) && (mfn <= 0x1fe); mfn++) {
+			res = _clk_pllv1_recalc_rate(mfi, mfn_offs + mfn, mfd,
+						     pd, parent_rate);
+			if (!best || (abs(rate - res) < abs(rate - best))) {
+				*reg = mfn_offs + mfn;
+				*reg |= mfi << 10;
+				*reg |= mfd << 16;
+				*reg |= pd << 26;
+				if (res == rate)
+					return;
+				best = res;
+			}
+		}
+}
+
+static int clk_pllv1_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct clk_pllv1 *pll = to_clk_pllv1(hw);
+	u32 reg;
+
+	_clk_pllv1_set_rate(rate, parent_rate, &reg);
+	writel(reg, pll->base);
+
+	return 0;
+}
+
+static long clk_pllv1_round_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *prate)
+{
+	unsigned int pd, mfi, mfn, mfd;
+	u32 reg;
+
+	_clk_pllv1_set_rate(rate, *prate, &reg);
+
+	mfi = (reg >> 10) & 0xf;
+	mfn = reg & 0x3ff;
+	mfd = (reg >> 16) & 0x3ff;
+	pd =  (reg >> 26) & 0xf;
+
+	return _clk_pllv1_recalc_rate(mfi, mfn, mfd, pd, *prate);
+}
+
 static struct clk_ops clk_pllv1_ops = {
-	.recalc_rate = clk_pllv1_recalc_rate,
+	.recalc_rate	= clk_pllv1_recalc_rate,
+	.round_rate	= clk_pllv1_round_rate,
+	.set_rate	= clk_pllv1_set_rate,
 };
 
 struct clk *imx_clk_pllv1(const char *name, const char *parent,
diff --git a/arch/arm/mach-imx/mx27.h b/arch/arm/mach-imx/mx27.h
index 8a65f19..c64632b 100644
--- a/arch/arm/mach-imx/mx27.h
+++ b/arch/arm/mach-imx/mx27.h
@@ -231,8 +231,13 @@ 
 #define MX27_DMA_REQ_SDHC3	36
 #define MX27_DMA_REQ_NFC	37
 
-#ifndef __ASSEMBLY__
+#ifdef CONFIG_SOC_IMX27
 extern int mx27_revision(void);
+#else
+static int mx27_revision(void)
+{
+	return -1;
+}
 #endif
 
 #endif /* ifndef __MACH_MX27_H__ */