[PATCH 0/2] improve imx6q arm_clk set rate

[PATCH 0/2] improve imx6q arm_clk set rate

[Richard Zhao]

arm_clk rate changing should also change pll1 rate.

Richard Zhao (2):
  ARM: imx6q: add pll round_rate support
  ARM: imx6q: change pll1 rate when change arm_clk rate

 arch/arm/mach-imx/clock-imx6q.c |  161 +++++++++++++++++++++++++++++++++++---
 1 files changed, 148 insertions(+), 13 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-imx/clock-imx6q.c |   85 +++++++++++++++++++++++++++++++++-----
 1 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
index e4e4f5e..3d5dc56 100644
--- a/arch/arm/mach-imx/clock-imx6q.c
+++ b/arch/arm/mach-imx/clock-imx6q.c
@@ -519,6 +519,18 @@ static int pll1_sys_set_rate(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static unsigned long pll1_sys_round_rate(struct clk *clk, unsigned long rate)
+{
+	u32 div;
+	unsigned long parent_rate = clk_get_rate(clk->parent);
+
+	rate = rate < FREQ_650M ? FREQ_650M : rate;
+	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
+
+	div = rate * 2 / parent_rate;
+	return parent_rate * div / 2;
+}
+
 static unsigned long pll8_enet_get_rate(struct clk *clk)
 {
 	u32 div = (readl_relaxed(PLL8_ENET) & BM_PLL_ENET_DIV_SELECT) >>
@@ -567,6 +579,20 @@ static int pll8_enet_set_rate(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static unsigned long pll8_enet_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (rate >= 125000000)
+		rate = 125000000;
+	else if (rate >= 100000000)
+		rate = 100000000;
+	else if (rate >= 50000000)
+		rate = 50000000;
+	else
+		rate = 25000000;
+	return rate;
+}
+
+
 static unsigned long pll_av_get_rate(struct clk *clk)
 {
 	void __iomem *reg = (clk == &pll4_audio) ? PLL4_AUDIO : PLL5_VIDEO;
@@ -606,6 +632,25 @@ static int pll_av_set_rate(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static unsigned long pll_av_round_rate(struct clk *clk, unsigned long rate)
+{
+	unsigned long parent_rate = clk_get_rate(clk->parent);
+	u32 div;
+	u32 mfn, mfd = 1000000;
+	s64 temp64;
+
+	rate = rate < FREQ_650M ? FREQ_650M : rate;
+	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
+
+	div = rate / parent_rate;
+	temp64 = (u64) (parent_rate - (div * parent_rate));
+        temp64 *= mfd;
+        do_div(temp64, parent_rate);
+        mfn = temp64;
+
+        return (parent_rate * div) + ((parent_rate / mfd) * mfn);
+}
+
 static void __iomem *pll_get_div_reg_bit(struct clk *clk, u32 *bp, u32 *bm)
 {
 	void __iomem *reg;
@@ -662,18 +707,33 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
-#define pll2_bus_get_rate	pll_get_rate
-#define pll2_bus_set_rate	pll_set_rate
-#define pll3_usb_otg_get_rate	pll_get_rate
-#define pll3_usb_otg_set_rate	pll_set_rate
-#define pll7_usb_host_get_rate	pll_get_rate
-#define pll7_usb_host_set_rate	pll_set_rate
-#define pll4_audio_get_rate	pll_av_get_rate
-#define pll4_audio_set_rate	pll_av_set_rate
-#define pll5_video_get_rate	pll_av_get_rate
-#define pll5_video_set_rate	pll_av_set_rate
-#define pll6_mlb_get_rate	NULL
-#define pll6_mlb_set_rate	NULL
+static unsigned long pll_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (rate >= FREQ_528M)
+		rate = FREQ_528M;
+	else
+		rate = FREQ_480M;
+	return rate;
+}
+
+#define pll2_bus_get_rate		pll_get_rate
+#define pll2_bus_set_rate		pll_set_rate
+#define pll2_bus_round_rate		pll_round_rate
+#define pll3_usb_otg_get_rate		pll_get_rate
+#define pll3_usb_otg_set_rate		pll_set_rate
+#define pll3_usb_otg_round_rate		pll_round_rate
+#define pll7_usb_host_get_rate		pll_get_rate
+#define pll7_usb_host_set_rate		pll_set_rate
+#define pll7_usb_host_round_rate	pll_round_rate
+#define pll4_audio_get_rate		pll_av_get_rate
+#define pll4_audio_set_rate		pll_av_set_rate
+#define pll4_audio_round_rate		pll_av_round_rate
+#define pll5_video_get_rate		pll_av_get_rate
+#define pll5_video_set_rate		pll_av_set_rate
+#define pll5_video_round_rate		pll_av_round_rate
+#define pll6_mlb_get_rate		NULL
+#define pll6_mlb_set_rate		NULL
+#define pll6_mlb_round_rate		NULL
 
 #define DEF_PLL(name)					\
 	static struct clk name = {			\
@@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
 		.disable	= pll_disable,		\
 		.get_rate	= name##_get_rate,	\
 		.set_rate	= name##_set_rate,	\
+		.round_rate	= name##_round_rate,	\
 		.parent		= &osc_clk,		\
 	}
 
-- 
1.7.5.4

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Sascha Hauer]

On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/mach-imx/clock-imx6q.c |   85 +++++++++++++++++++++++++++++++++-----
>  1 files changed, 73 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
> index e4e4f5e..3d5dc56 100644
> --- a/arch/arm/mach-imx/clock-imx6q.c
> +++ b/arch/arm/mach-imx/clock-imx6q.c
> @@ -519,6 +519,18 @@ static int pll1_sys_set_rate(struct clk *clk, unsigned long rate)
>  	return 0;
>  }
>  
> +static unsigned long pll1_sys_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	u32 div;
> +	unsigned long parent_rate = clk_get_rate(clk->parent);
> +
> +	rate = rate < FREQ_650M ? FREQ_650M : rate;
> +	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
> +
> +	div = rate * 2 / parent_rate;
> +	return parent_rate * div / 2;
> +}
> +
>  static unsigned long pll8_enet_get_rate(struct clk *clk)
>  {
>  	u32 div = (readl_relaxed(PLL8_ENET) & BM_PLL_ENET_DIV_SELECT) >>
> @@ -567,6 +579,20 @@ static int pll8_enet_set_rate(struct clk *clk, unsigned long rate)
>  	return 0;
>  }
>  
> +static unsigned long pll8_enet_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (rate >= 125000000)
> +		rate = 125000000;
> +	else if (rate >= 100000000)
> +		rate = 100000000;
> +	else if (rate >= 50000000)
> +		rate = 50000000;
> +	else
> +		rate = 25000000;
> +	return rate;
> +}
> +
> +
>  static unsigned long pll_av_get_rate(struct clk *clk)
>  {
>  	void __iomem *reg = (clk == &pll4_audio) ? PLL4_AUDIO : PLL5_VIDEO;
> @@ -606,6 +632,25 @@ static int pll_av_set_rate(struct clk *clk, unsigned long rate)
>  	return 0;
>  }
>  
> +static unsigned long pll_av_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	unsigned long parent_rate = clk_get_rate(clk->parent);
> +	u32 div;
> +	u32 mfn, mfd = 1000000;
> +	s64 temp64;
> +
> +	rate = rate < FREQ_650M ? FREQ_650M : rate;
> +	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
> +
> +	div = rate / parent_rate;
> +	temp64 = (u64) (parent_rate - (div * parent_rate));
> +        temp64 *= mfd;
> +        do_div(temp64, parent_rate);
> +        mfn = temp64;
> +
> +        return (parent_rate * div) + ((parent_rate / mfd) * mfn);
> +}
> +
>  static void __iomem *pll_get_div_reg_bit(struct clk *clk, u32 *bp, u32 *bm)
>  {
>  	void __iomem *reg;
> @@ -662,18 +707,33 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
>  	return 0;
>  }
>  
> -#define pll2_bus_get_rate	pll_get_rate
> -#define pll2_bus_set_rate	pll_set_rate
> -#define pll3_usb_otg_get_rate	pll_get_rate
> -#define pll3_usb_otg_set_rate	pll_set_rate
> -#define pll7_usb_host_get_rate	pll_get_rate
> -#define pll7_usb_host_set_rate	pll_set_rate
> -#define pll4_audio_get_rate	pll_av_get_rate
> -#define pll4_audio_set_rate	pll_av_set_rate
> -#define pll5_video_get_rate	pll_av_get_rate
> -#define pll5_video_set_rate	pll_av_set_rate
> -#define pll6_mlb_get_rate	NULL
> -#define pll6_mlb_set_rate	NULL
> +static unsigned long pll_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (rate >= FREQ_528M)
> +		rate = FREQ_528M;
> +	else
> +		rate = FREQ_480M;
> +	return rate;
> +}
> +
> +#define pll2_bus_get_rate		pll_get_rate
> +#define pll2_bus_set_rate		pll_set_rate
> +#define pll2_bus_round_rate		pll_round_rate
> +#define pll3_usb_otg_get_rate		pll_get_rate
> +#define pll3_usb_otg_set_rate		pll_set_rate
> +#define pll3_usb_otg_round_rate		pll_round_rate
> +#define pll7_usb_host_get_rate		pll_get_rate
> +#define pll7_usb_host_set_rate		pll_set_rate
> +#define pll7_usb_host_round_rate	pll_round_rate
> +#define pll4_audio_get_rate		pll_av_get_rate
> +#define pll4_audio_set_rate		pll_av_set_rate
> +#define pll4_audio_round_rate		pll_av_round_rate
> +#define pll5_video_get_rate		pll_av_get_rate
> +#define pll5_video_set_rate		pll_av_set_rate
> +#define pll5_video_round_rate		pll_av_round_rate
> +#define pll6_mlb_get_rate		NULL
> +#define pll6_mlb_set_rate		NULL
> +#define pll6_mlb_round_rate		NULL
>  
>  #define DEF_PLL(name)					\
>  	static struct clk name = {			\
> @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
>  		.disable	= pll_disable,		\
>  		.get_rate	= name##_get_rate,	\
>  		.set_rate	= name##_set_rate,	\
> +		.round_rate	= name##_round_rate,	\

I hope this ## stuff is gone soon with the generic clock framework. It
is so ugly and inefficient.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Richard Zhao]

On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  arch/arm/mach-imx/clock-imx6q.c |   85 +++++++++++++++++++++++++++++++++-----
> >  1 files changed, 73 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
> > index e4e4f5e..3d5dc56 100644
> > --- a/arch/arm/mach-imx/clock-imx6q.c
> > +++ b/arch/arm/mach-imx/clock-imx6q.c
> > @@ -519,6 +519,18 @@ static int pll1_sys_set_rate(struct clk *clk, unsigned long rate)
> >  	return 0;
> >  }
> >  
> > +static unsigned long pll1_sys_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	u32 div;
> > +	unsigned long parent_rate = clk_get_rate(clk->parent);
> > +
> > +	rate = rate < FREQ_650M ? FREQ_650M : rate;
> > +	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
> > +
> > +	div = rate * 2 / parent_rate;
> > +	return parent_rate * div / 2;
> > +}
> > +
> >  static unsigned long pll8_enet_get_rate(struct clk *clk)
> >  {
> >  	u32 div = (readl_relaxed(PLL8_ENET) & BM_PLL_ENET_DIV_SELECT) >>
> > @@ -567,6 +579,20 @@ static int pll8_enet_set_rate(struct clk *clk, unsigned long rate)
> >  	return 0;
> >  }
> >  
> > +static unsigned long pll8_enet_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	if (rate >= 125000000)
> > +		rate = 125000000;
> > +	else if (rate >= 100000000)
> > +		rate = 100000000;
> > +	else if (rate >= 50000000)
> > +		rate = 50000000;
> > +	else
> > +		rate = 25000000;
> > +	return rate;
> > +}
> > +
> > +
> >  static unsigned long pll_av_get_rate(struct clk *clk)
> >  {
> >  	void __iomem *reg = (clk == &pll4_audio) ? PLL4_AUDIO : PLL5_VIDEO;
> > @@ -606,6 +632,25 @@ static int pll_av_set_rate(struct clk *clk, unsigned long rate)
> >  	return 0;
> >  }
> >  
> > +static unsigned long pll_av_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	unsigned long parent_rate = clk_get_rate(clk->parent);
> > +	u32 div;
> > +	u32 mfn, mfd = 1000000;
> > +	s64 temp64;
> > +
> > +	rate = rate < FREQ_650M ? FREQ_650M : rate;
> > +	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
> > +
> > +	div = rate / parent_rate;
> > +	temp64 = (u64) (parent_rate - (div * parent_rate));
> > +        temp64 *= mfd;
> > +        do_div(temp64, parent_rate);
> > +        mfn = temp64;
> > +
> > +        return (parent_rate * div) + ((parent_rate / mfd) * mfn);
> > +}
> > +
> >  static void __iomem *pll_get_div_reg_bit(struct clk *clk, u32 *bp, u32 *bm)
> >  {
> >  	void __iomem *reg;
> > @@ -662,18 +707,33 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> >  	return 0;
> >  }
> >  
> > -#define pll2_bus_get_rate	pll_get_rate
> > -#define pll2_bus_set_rate	pll_set_rate
> > -#define pll3_usb_otg_get_rate	pll_get_rate
> > -#define pll3_usb_otg_set_rate	pll_set_rate
> > -#define pll7_usb_host_get_rate	pll_get_rate
> > -#define pll7_usb_host_set_rate	pll_set_rate
> > -#define pll4_audio_get_rate	pll_av_get_rate
> > -#define pll4_audio_set_rate	pll_av_set_rate
> > -#define pll5_video_get_rate	pll_av_get_rate
> > -#define pll5_video_set_rate	pll_av_set_rate
> > -#define pll6_mlb_get_rate	NULL
> > -#define pll6_mlb_set_rate	NULL
> > +static unsigned long pll_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +	if (rate >= FREQ_528M)
> > +		rate = FREQ_528M;
> > +	else
> > +		rate = FREQ_480M;
> > +	return rate;
> > +}
> > +
> > +#define pll2_bus_get_rate		pll_get_rate
> > +#define pll2_bus_set_rate		pll_set_rate
> > +#define pll2_bus_round_rate		pll_round_rate
> > +#define pll3_usb_otg_get_rate		pll_get_rate
> > +#define pll3_usb_otg_set_rate		pll_set_rate
> > +#define pll3_usb_otg_round_rate		pll_round_rate
> > +#define pll7_usb_host_get_rate		pll_get_rate
> > +#define pll7_usb_host_set_rate		pll_set_rate
> > +#define pll7_usb_host_round_rate	pll_round_rate
> > +#define pll4_audio_get_rate		pll_av_get_rate
> > +#define pll4_audio_set_rate		pll_av_set_rate
> > +#define pll4_audio_round_rate		pll_av_round_rate
> > +#define pll5_video_get_rate		pll_av_get_rate
> > +#define pll5_video_set_rate		pll_av_set_rate
> > +#define pll5_video_round_rate		pll_av_round_rate
> > +#define pll6_mlb_get_rate		NULL
> > +#define pll6_mlb_set_rate		NULL
> > +#define pll6_mlb_round_rate		NULL
> >  
> >  #define DEF_PLL(name)					\
> >  	static struct clk name = {			\
> > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> >  		.disable	= pll_disable,		\
> >  		.get_rate	= name##_get_rate,	\
> >  		.set_rate	= name##_set_rate,	\
> > +		.round_rate	= name##_round_rate,	\
> 
> I hope this ## stuff is gone soon with the generic clock framework. It
> is so ugly and inefficient.
I hope this doesn't prevent this two patches go in.

Thanks
Richard
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Sascha Hauer]

On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  
> > >  #define DEF_PLL(name)					\
> > >  	static struct clk name = {			\
> > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > >  		.disable	= pll_disable,		\
> > >  		.get_rate	= name##_get_rate,	\
> > >  		.set_rate	= name##_set_rate,	\
> > > +		.round_rate	= name##_round_rate,	\
> > 
> > I hope this ## stuff is gone soon with the generic clock framework. It
> > is so ugly and inefficient.
> I hope this doesn't prevent this two patches go in.

Given that we are short from getting a generic clock framework (and I
think this time it's for real) and that you are not mention in any words
what these patches fix I don't see a reason for merging them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Shawn Guo]

On 15 March 2012 23:00, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
>> On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
>> > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
>> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>> > > ---
>> > >
>> > > ?#define DEF_PLL(name) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> > > ? static struct clk name = { ? ? ? ? ? ? ? ? ? ? ?\
>> > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
>> > > ? ? ? ? ? .disable ? ? ? ?= pll_disable, ? ? ? ? ?\
>> > > ? ? ? ? ? .get_rate ? ? ? = name##_get_rate, ? ? ?\
>> > > ? ? ? ? ? .set_rate ? ? ? = name##_set_rate, ? ? ?\
>> > > + ? ? ? ? .round_rate ? ? = name##_round_rate, ? ?\
>> >
>> > I hope this ## stuff is gone soon with the generic clock framework. It
>> > is so ugly and inefficient.
>> I hope this doesn't prevent this two patches go in.
>
> Given that we are short from getting a generic clock framework (and I
> think this time it's for real) and that you are not mention in any words
> what these patches fix I don't see a reason for merging them.
>
And I agree with Sascha.  Since I'm resuming my imx6 common clk
migration, you can keep an eye on the patches to ensure the changes
you are proposing here get rolled in.

Regards,
Shawn

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Richard Zhao]

On Thu, Mar 15, 2012 at 04:00:48PM +0100, Sascha Hauer wrote:
> On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> > On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > ---
> > > >  
> > > >  #define DEF_PLL(name)					\
> > > >  	static struct clk name = {			\
> > > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > > >  		.disable	= pll_disable,		\
> > > >  		.get_rate	= name##_get_rate,	\
> > > >  		.set_rate	= name##_set_rate,	\
> > > > +		.round_rate	= name##_round_rate,	\
> > > 
> > > I hope this ## stuff is gone soon with the generic clock framework. It
> > > is so ugly and inefficient.
> > I hope this doesn't prevent this two patches go in.
> 
> Given that we are short from getting a generic clock framework (and I
> think this time it's for real) and that you are not mention in any words
> what these patches fix I don't see a reason for merging them.
I think the cpu clock is a great challenge for generic clock framework.
When it set_rate, it includes reparent, and change parent's parent's rate.
I don't see any upstream code like that till now. and it's really what
we need.

And it also block cpufreq from upstreaming.


Thanks
Richard
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Sascha Hauer]

On Thu, Mar 15, 2012 at 11:15:23PM +0800, Richard Zhao wrote:
> On Thu, Mar 15, 2012 at 04:00:48PM +0100, Sascha Hauer wrote:
> > On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> > > On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > > > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > ---
> > > > >  
> > > > >  #define DEF_PLL(name)					\
> > > > >  	static struct clk name = {			\
> > > > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > > > >  		.disable	= pll_disable,		\
> > > > >  		.get_rate	= name##_get_rate,	\
> > > > >  		.set_rate	= name##_set_rate,	\
> > > > > +		.round_rate	= name##_round_rate,	\
> > > > 
> > > > I hope this ## stuff is gone soon with the generic clock framework. It
> > > > is so ugly and inefficient.
> > > I hope this doesn't prevent this two patches go in.
> > 
> > Given that we are short from getting a generic clock framework (and I
> > think this time it's for real) and that you are not mention in any words
> > what these patches fix I don't see a reason for merging them.
> I think the cpu clock is a great challenge for generic clock framework.
> When it set_rate, it includes reparent, and change parent's parent's rate.
> I don't see any upstream code like that till now. and it's really what
> we need.

Then do a clk_set_parent, clk_set_rate and a clk_set_parent again
in your cpufreq driver. The notifying mechanism even allows you to
block any concurrent change in between these three steps if necessary.
Noone says that these three steps have to be encapsulated in a single
clk_set_rate call like you did in your patch.

> 
> And it also block cpufreq from upstreaming.

Yes, and that's a good way to tell your management why you have to
invest some time into porting i.MX6 to the generic clock framework ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Richard Zhao]

Hi Sascha,

On Thu, Mar 15, 2012 at 07:50:04PM +0100, Sascha Hauer wrote:
> On Thu, Mar 15, 2012 at 11:15:23PM +0800, Richard Zhao wrote:
> > On Thu, Mar 15, 2012 at 04:00:48PM +0100, Sascha Hauer wrote:
> > > On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> > > > On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > > > > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > ---
> > > > > >  
> > > > > >  #define DEF_PLL(name)					\
> > > > > >  	static struct clk name = {			\
> > > > > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > > > > >  		.disable	= pll_disable,		\
> > > > > >  		.get_rate	= name##_get_rate,	\
> > > > > >  		.set_rate	= name##_set_rate,	\
> > > > > > +		.round_rate	= name##_round_rate,	\
> > > > > 
> > > > > I hope this ## stuff is gone soon with the generic clock framework. It
> > > > > is so ugly and inefficient.
> > > > I hope this doesn't prevent this two patches go in.
> > > 
> > > Given that we are short from getting a generic clock framework (and I
> > > think this time it's for real) and that you are not mention in any words
> > > what these patches fix I don't see a reason for merging them.
> > I think the cpu clock is a great challenge for generic clock framework.
> > When it set_rate, it includes reparent, and change parent's parent's rate.
> > I don't see any upstream code like that till now. and it's really what
> > we need.
> 
> Then do a clk_set_parent, clk_set_rate and a clk_set_parent again
> in your cpufreq driver.
No, it's platform code, and supposed to be in arch/arm. What the cpufre
driver know is to get cpu clk, rather not how cpu clk change its rate.
And cpu clk/arm_clk is a real clock wires in SoC.
> The notifying mechanism even allows you to
> block any concurrent change in between these three steps if necessary.
> Noone says that these three steps have to be encapsulated in a single
> clk_set_rate call like you did in your patch.
If you're reviwing the patch, why not take a step advance? :)
> 
> > 
> > And it also block cpufreq from upstreaming.
> 
> Yes, and that's a good way to tell your management why you have to
> invest some time into porting i.MX6 to the generic clock framework ;)
It's Shawn doing the work. So I think it's better get the patches in
and let everyone notice the tricky things.

In my opinion, it might not be good to close the window for summiting
patch of basic modules, which is used nearly by every driver. There
might be more things pending thant you thought.

Thanks
Richard
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Sascha Hauer]

On Fri, Mar 16, 2012 at 09:10:07AM +0800, Richard Zhao wrote:
> Hi Sascha,
> 
> On Thu, Mar 15, 2012 at 07:50:04PM +0100, Sascha Hauer wrote:
> > On Thu, Mar 15, 2012 at 11:15:23PM +0800, Richard Zhao wrote:
> > > On Thu, Mar 15, 2012 at 04:00:48PM +0100, Sascha Hauer wrote:
> > > > On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> > > > > On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > > > > > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > > ---
> > > > > > >  
> > > > > > >  #define DEF_PLL(name)					\
> > > > > > >  	static struct clk name = {			\
> > > > > > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > > > > > >  		.disable	= pll_disable,		\
> > > > > > >  		.get_rate	= name##_get_rate,	\
> > > > > > >  		.set_rate	= name##_set_rate,	\
> > > > > > > +		.round_rate	= name##_round_rate,	\
> > > > > > 
> > > > > > I hope this ## stuff is gone soon with the generic clock framework. It
> > > > > > is so ugly and inefficient.
> > > > > I hope this doesn't prevent this two patches go in.
> > > > 
> > > > Given that we are short from getting a generic clock framework (and I
> > > > think this time it's for real) and that you are not mention in any words
> > > > what these patches fix I don't see a reason for merging them.
> > > I think the cpu clock is a great challenge for generic clock framework.
> > > When it set_rate, it includes reparent, and change parent's parent's rate.
> > > I don't see any upstream code like that till now. and it's really what
> > > we need.
> > 
> > Then do a clk_set_parent, clk_set_rate and a clk_set_parent again
> > in your cpufreq driver.
> No, it's platform code, and supposed to be in arch/arm. What the cpufre
> driver know is to get cpu clk, rather not how cpu clk change its rate.
> And cpu clk/arm_clk is a real clock wires in SoC.

The job of the clock framework is to give a consistent view on the
clocks and to handle parent/rate changes properly. It even the
CLK_SET_RATE_GATE for ensuring that your PLL can only change its rate
when it's gated. It's not the job of the clock framework to dynamically
reorganize the clock tree on a rate change.

Besides, do you really want to reprogram the PLL with a cpufreq change?
You have a divider behind that PLL which you apparently can change
without having to reparent the PLL. Doesn't this give you enough choices
for the cpu frequency?

> > The notifying mechanism even allows you to
> > block any concurrent change in between these three steps if necessary.
> > Noone says that these three steps have to be encapsulated in a single
> > clk_set_rate call like you did in your patch.
> If you're reviwing the patch, why not take a step advance? :)

I have prepared the patches for converting i.MX1/21/25/27 and I have
(older) patches for the i.MX31/51/53 which I want to rebase on the
current clock framework. That gives me enough to do until the next merge
window. Additionally I'm not yet very familiar with the i.MX6.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Richard Zhao]

On Sat, Mar 17, 2012 at 01:28:31PM +0100, Sascha Hauer wrote:
> On Fri, Mar 16, 2012 at 09:10:07AM +0800, Richard Zhao wrote:
> > Hi Sascha,
> > 
> > On Thu, Mar 15, 2012 at 07:50:04PM +0100, Sascha Hauer wrote:
> > > On Thu, Mar 15, 2012 at 11:15:23PM +0800, Richard Zhao wrote:
> > > > On Thu, Mar 15, 2012 at 04:00:48PM +0100, Sascha Hauer wrote:
> > > > > On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> > > > > > On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > > > > > > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > > > ---
> > > > > > > >  
> > > > > > > >  #define DEF_PLL(name)					\
> > > > > > > >  	static struct clk name = {			\
> > > > > > > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > > > > > > >  		.disable	= pll_disable,		\
> > > > > > > >  		.get_rate	= name##_get_rate,	\
> > > > > > > >  		.set_rate	= name##_set_rate,	\
> > > > > > > > +		.round_rate	= name##_round_rate,	\
> > > > > > > 
> > > > > > > I hope this ## stuff is gone soon with the generic clock framework. It
> > > > > > > is so ugly and inefficient.
> > > > > > I hope this doesn't prevent this two patches go in.
> > > > > 
> > > > > Given that we are short from getting a generic clock framework (and I
> > > > > think this time it's for real) and that you are not mention in any words
> > > > > what these patches fix I don't see a reason for merging them.
> > > > I think the cpu clock is a great challenge for generic clock framework.
> > > > When it set_rate, it includes reparent, and change parent's parent's rate.
> > > > I don't see any upstream code like that till now. and it's really what
> > > > we need.
> > > 
> > > Then do a clk_set_parent, clk_set_rate and a clk_set_parent again
> > > in your cpufreq driver.
> > No, it's platform code, and supposed to be in arch/arm. What the cpufre
> > driver know is to get cpu clk, rather not how cpu clk change its rate.
> > And cpu clk/arm_clk is a real clock wires in SoC.
> 
> The job of the clock framework is to give a consistent view on the
> clocks and to handle parent/rate changes properly. It even the
> CLK_SET_RATE_GATE for ensuring that your PLL can only change its rate
> when it's gated. It's not the job of the clock framework to dynamically
> reorganize the clock tree on a rate change.
The reparent in set_rate is in mutex lock. Looks like, we only need
the unlocked version clk functions.
> 
> Besides, do you really want to reprogram the PLL with a cpufreq change?
> You have a divider behind that PLL which you apparently can change
> without having to reparent the PLL. Doesn't this give you enough choices
> for the cpu frequency?
No. The 3bit cpu divider is not enough. For example, if pll is 1G Hz, the
next lower freq will be 500M, which is a too large step.
> 
> > > The notifying mechanism even allows you to
> > > block any concurrent change in between these three steps if necessary.
> > > Noone says that these three steps have to be encapsulated in a single
> > > clk_set_rate call like you did in your patch.
> > If you're reviwing the patch, why not take a step advance? :)
> 
> I have prepared the patches for converting i.MX1/21/25/27 and I have
> (older) patches for the i.MX31/51/53 which I want to rebase on the
> current clock framework. 
You might notice I ever sent out serveral versions of MX5x clock porting
patches based on your first version. If you want to take it over, it's ok.
> That gives me enough to do until the next merge
> window. Additionally I'm not yet very familiar with the i.MX6.
So, Shawn, would you review it and consider the tricky things when
migrate to new framework?

Thanks
Richard
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Shawn Guo]

On Mon, Mar 19, 2012 at 09:51:43AM +0800, Richard Zhao wrote:
...
> So, Shawn, would you review it and consider the tricky things when
> migrate to new framework?
> 
Sure.  I'm working on it.  Just give me some time.

-- 
Regards,
Shawn

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Sascha Hauer]

On Mon, Mar 19, 2012 at 09:51:43AM +0800, Richard Zhao wrote:
> On Sat, Mar 17, 2012 at 01:28:31PM +0100, Sascha Hauer wrote:
> > On Fri, Mar 16, 2012 at 09:10:07AM +0800, Richard Zhao wrote:
> > > Hi Sascha,
> > > 
> > > On Thu, Mar 15, 2012 at 07:50:04PM +0100, Sascha Hauer wrote:
> > > > On Thu, Mar 15, 2012 at 11:15:23PM +0800, Richard Zhao wrote:
> > > > > On Thu, Mar 15, 2012 at 04:00:48PM +0100, Sascha Hauer wrote:
> > > > > > On Thu, Mar 15, 2012 at 10:46:04PM +0800, Richard Zhao wrote:
> > > > > > > On Wed, Mar 14, 2012 at 09:52:28AM +0100, Sascha Hauer wrote:
> > > > > > > > On Wed, Mar 14, 2012 at 04:22:58PM +0800, Richard Zhao wrote:
> > > > > > > > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  
> > > > > > > > >  #define DEF_PLL(name)					\
> > > > > > > > >  	static struct clk name = {			\
> > > > > > > > > @@ -681,6 +741,7 @@ static int pll_set_rate(struct clk *clk, unsigned long rate)
> > > > > > > > >  		.disable	= pll_disable,		\
> > > > > > > > >  		.get_rate	= name##_get_rate,	\
> > > > > > > > >  		.set_rate	= name##_set_rate,	\
> > > > > > > > > +		.round_rate	= name##_round_rate,	\
> > > > > > > > 
> > > > > > > > I hope this ## stuff is gone soon with the generic clock framework. It
> > > > > > > > is so ugly and inefficient.
> > > > > > > I hope this doesn't prevent this two patches go in.
> > > > > > 
> > > > > > Given that we are short from getting a generic clock framework (and I
> > > > > > think this time it's for real) and that you are not mention in any words
> > > > > > what these patches fix I don't see a reason for merging them.
> > > > > I think the cpu clock is a great challenge for generic clock framework.
> > > > > When it set_rate, it includes reparent, and change parent's parent's rate.
> > > > > I don't see any upstream code like that till now. and it's really what
> > > > > we need.
> > > > 
> > > > Then do a clk_set_parent, clk_set_rate and a clk_set_parent again
> > > > in your cpufreq driver.
> > > No, it's platform code, and supposed to be in arch/arm. What the cpufre
> > > driver know is to get cpu clk, rather not how cpu clk change its rate.
> > > And cpu clk/arm_clk is a real clock wires in SoC.
> > 
> > The job of the clock framework is to give a consistent view on the
> > clocks and to handle parent/rate changes properly. It even the
> > CLK_SET_RATE_GATE for ensuring that your PLL can only change its rate
> > when it's gated. It's not the job of the clock framework to dynamically
> > reorganize the clock tree on a rate change.
> The reparent in set_rate is in mutex lock. Looks like, we only need
> the unlocked version clk functions.

Again, you don't want to drill holes into the clock framework to
reparent in a set_rate op.

> > 
> > Besides, do you really want to reprogram the PLL with a cpufreq change?
> > You have a divider behind that PLL which you apparently can change
> > without having to reparent the PLL. Doesn't this give you enough choices
> > for the cpu frequency?
> No. The 3bit cpu divider is not enough. For example, if pll is 1G Hz, the
> next lower freq will be 500M, which is a too large step.
> > 
> > > > The notifying mechanism even allows you to
> > > > block any concurrent change in between these three steps if necessary.
> > > > Noone says that these three steps have to be encapsulated in a single
> > > > clk_set_rate call like you did in your patch.
> > > If you're reviwing the patch, why not take a step advance? :)
> > 
> > I have prepared the patches for converting i.MX1/21/25/27 and I have
> > (older) patches for the i.MX31/51/53 which I want to rebase on the
> > current clock framework. 
> You might notice I ever sent out serveral versions of MX5x clock porting
> patches based on your first version. If you want to take it over, it's ok.

No, I don't want to use the static initializers.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] ARM: imx6q: add pll round_rate support

[Richard Zhao]

> +static unsigned long pll_av_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	unsigned long parent_rate = clk_get_rate(clk->parent);
> +	u32 div;
> +	u32 mfn, mfd = 1000000;
> +	s64 temp64;
> +
> +	rate = rate < FREQ_650M ? FREQ_650M : rate;
> +	rate = rate > FREQ_1300M ? FREQ_1300M : rate;
> +
> +	div = rate / parent_rate;
> +	temp64 = (u64) (parent_rate - (div * parent_rate));
Be			rate here.
Sorry.
> +        temp64 *= mfd;
> +        do_div(temp64, parent_rate);
> +        mfn = temp64;
> +
> +        return (parent_rate * div) + ((parent_rate / mfd) * mfn);
> +}
> +
 
Thanks
Richard

[PATCH 2/2] ARM: imx6q: change pll1 rate when change arm_clk rate

[Richard Zhao]

Add arm_clk's own get_rate/set_rate/round_rate functions.

set_rate steps:
 - reparent pll1_sw_clk to pll2_pfd_400m or osc_clk
 - disable pll1_sys
 - set pll1_sys rate
 - enable pll1_sys
 - reparent pll1_sw_clk back to pll1_sys

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-imx/clock-imx6q.c |   76 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 75 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
index 3d5dc56..e601f52 100644
--- a/arch/arm/mach-imx/clock-imx6q.c
+++ b/arch/arm/mach-imx/clock-imx6q.c
@@ -1782,7 +1782,6 @@ DEF_NG_CLK(periph2_pre_clk,	&pll2_bus);
 DEF_NG_CLK(periph2_clk,		&periph2_pre_clk);
 DEF_NG_CLK(axi_clk,		&periph_clk);
 DEF_NG_CLK(emi_clk,		&axi_clk);
-DEF_NG_CLK(arm_clk,		&pll1_sw_clk);
 DEF_NG_CLK(ahb_clk,		&periph_clk);
 DEF_NG_CLK(ipg_clk,		&ahb_clk);
 DEF_NG_CLK(ipg_perclk,		&ipg_clk);
@@ -1873,6 +1872,81 @@ DEF_CLK(emi_slow_clk,	  CCGR6, CG5,  &axi_clk,	  NULL);
 DEF_CLK(vdo_axi_clk,	  CCGR6, CG6,  &axi_clk,	  NULL);
 DEF_CLK(vpu_clk,	  CCGR6, CG7,  &axi_clk,	  NULL);
 
+static unsigned long arm_clk_get_rate(struct clk *clk)
+{
+	unsigned long parent_rate = clk_get_rate(clk->parent);
+	u32 reg, div;
+
+	reg = readl_relaxed(CACRR);
+	div = (reg & BM_CACRR_ARM_PODF) >> BP_CACRR_ARM_PODF;
+
+	return parent_rate / (div + 1);
+}
+static int arm_clk_bestdiv(unsigned long rate, unsigned long *prate)
+{
+	u32 i, maxdiv, bestdiv = 0;
+	unsigned long parent_rate, best = 0, now;
+
+	*prate = 0;
+	maxdiv = (BM_CACRR_ARM_PODF >> BP_CACRR_ARM_PODF) + 1;
+	for (i = 1; i <= maxdiv; i++) {
+		int div;
+		parent_rate = clk_round_rate(&pll1_sys, rate * i);
+		div = parent_rate / rate;
+		div = div > maxdiv ? maxdiv : div;
+		div = div < 1 ? 1 : div;
+		now = parent_rate / div;
+
+		if (now <= rate && now >= best) {
+			bestdiv = div;
+			best = now;
+			*prate = parent_rate;
+			if (now == rate)
+				break;
+		}
+	}
+	return bestdiv;
+}
+
+static int arm_clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	u32 reg, div;
+	unsigned long parent_rate;
+	div = arm_clk_bestdiv(rate, &parent_rate);
+
+	if (pll2_pfd_400m.usecount > 0)
+		pll1_sw_clk.set_parent(&pll1_sw_clk, &pll2_pfd_400m);
+	else
+		pll1_sw_clk.set_parent(&pll1_sw_clk, &osc_clk);
+	pll1_sys.disable(&pll1_sys);
+	pll1_sys.set_rate(&pll1_sys, parent_rate);
+	pll1_sys.enable(&pll1_sys);
+	pll1_sw_clk.set_parent(&pll1_sw_clk, &pll1_sys);
+
+	reg = readl_relaxed(CACRR);
+	reg &= ~BM_CACRR_ARM_PODF;
+	reg |= (div - 1) << BP_CACRR_ARM_PODF;
+	writel_relaxed(reg, CACRR);
+
+	return 0;
+}
+
+static unsigned long arm_clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	unsigned long div, parent_rate;
+
+	div = arm_clk_bestdiv(rate, &parent_rate);
+	return parent_rate / div;
+}
+
+static struct clk arm_clk = {
+	.get_rate	= arm_clk_get_rate,
+	.set_rate	= arm_clk_set_rate,
+	.round_rate	= arm_clk_round_rate,
+	.set_parent	= _clk_set_parent,
+	.parent		= &pll1_sw_clk,
+};
+
 static int pcie_clk_enable(struct clk *clk)
 {
 	u32 val;
-- 
1.7.5.4