Patchwork ARM: mx28: clock-mx28: Use a proper timeout mechanism

login
register
mail settings
Submitter Fabio Estevam
Date Jan. 23, 2012, 2:41 p.m.
Message ID <1327329667-25512-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/137365/
State New
Headers show

Comments

Fabio Estevam - Jan. 23, 2012, 2:41 p.m.
Introduce a function for checking the busy bits of CLKCTRL register that 
uses a proper timeout mechanism.

Remove parts of code that use busy loops and replace them with the 
mxs_clkctrl_timeout() function.

Tested on a mx28evk by performing audio playback.

Suggested-by: Wolfram Sang <w.sang@pengutronix.de>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-mxs/clock-mx28.c |   64 +++++++++++++---------------------------
 1 files changed, 21 insertions(+), 43 deletions(-)
Wolfram Sang - Jan. 23, 2012, 9:18 p.m.
Hi Fabio,

On Mon, Jan 23, 2012 at 12:41:07PM -0200, Fabio Estevam wrote:
> Introduce a function for checking the busy bits of CLKCTRL register that 
> uses a proper timeout mechanism.
> 
> Remove parts of code that use busy loops and replace them with the 
> mxs_clkctrl_timeout() function.
> 
> Tested on a mx28evk by performing audio playback.
> 
> Suggested-by: Wolfram Sang <w.sang@pengutronix.de>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Is there a difference in the two versions you sent?

> ---
>  arch/arm/mach-mxs/clock-mx28.c |   64 +++++++++++++---------------------------
>  1 files changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index 5d68e41..ad5482d 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -38,6 +38,7 @@
>  #define DIGCTRL_BASE_ADDR	MX28_IO_ADDRESS(MX28_DIGCTL_BASE_ADDR)
>  
>  #define PARENT_RATE_SHIFT	8
> +#define CLKCTRL_TIMEOUT		10	/* 10 ms = 1 jiffy */

I'd remove the jiffy-part of the comment to prevent belief that the
relationship will always be true.

>  static struct clk pll2_clk;
>  static struct clk cpu_clk;
> @@ -127,6 +128,20 @@ static unsigned long pll2_clk_get_rate(struct clk *clk)
>  	return 50000000;
>  }
>  
> +static int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(CLKCTRL_TIMEOUT);
> +	while (readl_relaxed(CLKCTRL_BASE_ADDR + reg_offset) & mask) {

Hmm, this is a lot better, yet not perfect. You could be scheduled away here
for 10ms and then you had only checked at t=0. However, I think it makes more
sense to introduce a generic timeout-loop somehow and then convert to it
later. This is my homework, though...

> +		if (time_after(jiffies, timeout)) {
> +			pr_err("%s: divider writing timeout\n", __func__);

__func__ is probably not useful here. Perhaps reg and mask?

> +			return -ETIMEDOUT;
> +		}
> +

Remove empty line.

> +	}
> +
> +	return 0;
> +}
> +

Rest looked good from a glimpse.

Regards,

   Wolfram
Fabio Estevam - Jan. 23, 2012, 9:31 p.m.
Hi Wolfram,

On Mon, Jan 23, 2012 at 7:18 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:

> Is there a difference in the two versions you sent?

No,  I sent the same patch twice by mistake.

>> +static int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask)
>> +{
>> +     unsigned long timeout = jiffies + msecs_to_jiffies(CLKCTRL_TIMEOUT);
>> +     while (readl_relaxed(CLKCTRL_BASE_ADDR + reg_offset) & mask) {
>
> Hmm, this is a lot better, yet not perfect. You could be scheduled away here
> for 10ms and then you had only checked at t=0. However, I think it makes more
> sense to introduce a generic timeout-loop somehow and then convert to it
> later. This is my homework, though...

Ok, will address your comments and keep the current
mxs_clkctrl_timeout version for v2.

Thanks for your review.

Regards,

Fabio Estevam
Shawn Guo - Jan. 26, 2012, 12:28 p.m.
On Mon, Jan 23, 2012 at 12:41:07PM -0200, Fabio Estevam wrote:
> Introduce a function for checking the busy bits of CLKCTRL register that 
> uses a proper timeout mechanism.
> 
> Remove parts of code that use busy loops and replace them with the 
> mxs_clkctrl_timeout() function.
> 
> Tested on a mx28evk by performing audio playback.
> 
> Suggested-by: Wolfram Sang <w.sang@pengutronix.de>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-mxs/clock-mx28.c |   64 +++++++++++++---------------------------
>  1 files changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index 5d68e41..ad5482d 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -38,6 +38,7 @@
>  #define DIGCTRL_BASE_ADDR	MX28_IO_ADDRESS(MX28_DIGCTL_BASE_ADDR)
>  
>  #define PARENT_RATE_SHIFT	8
> +#define CLKCTRL_TIMEOUT		10	/* 10 ms = 1 jiffy */
>  
>  static struct clk pll2_clk;
>  static struct clk cpu_clk;
> @@ -127,6 +128,20 @@ static unsigned long pll2_clk_get_rate(struct clk *clk)
>  	return 50000000;
>  }
>  
> +static int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask)

Name it mxs_clkctrl_divider_timeout to be more precise?

As the function name indicates, it's not a mx28 specific call, and
should be put in some place common to mx23 and mx28, so that
the divider timeout code in clock-mx23.c can also be fixed up.  (Yes,
I would expect the patch fixes mx23 as well.)

> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(CLKCTRL_TIMEOUT);
> +	while (readl_relaxed(CLKCTRL_BASE_ADDR + reg_offset) & mask) {
> +		if (time_after(jiffies, timeout)) {
> +			pr_err("%s: divider writing timeout\n", __func__);
> +			return -ETIMEDOUT;
> +		}
> +
Unnecessary blank line.

Regards,
Shawn

> +	}
> +
> +	return 0;
> +}
Fabio Estevam - Jan. 27, 2012, 2:56 p.m.
On 1/26/12, Shawn Guo <shawn.guo@linaro.org> wrote:

> Name it mxs_clkctrl_divider_timeout to be more precise?

I kept the original name because if I use the name you suggested I
would have to break several lines for keeping them within 80 columns.

As this functions is called within several macros, it would not be
very readable after breaking  the lines.


Regards,

Fabio Estevam

Patch

diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index 5d68e41..ad5482d 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -38,6 +38,7 @@ 
 #define DIGCTRL_BASE_ADDR	MX28_IO_ADDRESS(MX28_DIGCTL_BASE_ADDR)
 
 #define PARENT_RATE_SHIFT	8
+#define CLKCTRL_TIMEOUT		10	/* 10 ms = 1 jiffy */
 
 static struct clk pll2_clk;
 static struct clk cpu_clk;
@@ -127,6 +128,20 @@  static unsigned long pll2_clk_get_rate(struct clk *clk)
 	return 50000000;
 }
 
+static int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(CLKCTRL_TIMEOUT);
+	while (readl_relaxed(CLKCTRL_BASE_ADDR + reg_offset) & mask) {
+		if (time_after(jiffies, timeout)) {
+			pr_err("%s: divider writing timeout\n", __func__);
+			return -ETIMEDOUT;
+		}
+
+	}
+
+	return 0;
+}
+
 #define _CLK_ENABLE_PLL(name, r, g)					\
 static int name##_enable(struct clk *clk)				\
 {									\
@@ -322,7 +337,6 @@  static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 {									\
 	u32 reg, bm_busy, div_max, d, f, div, frac;			\
 	unsigned long diff, parent_rate, calc_rate;			\
-	int i;								\
 									\
 	div_max = BM_CLKCTRL_##dr##_DIV >> BP_CLKCTRL_##dr##_DIV;	\
 	bm_busy = BM_CLKCTRL_##dr##_BUSY;				\
@@ -396,16 +410,7 @@  static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 	}								\
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##dr);		\
 									\
-	for (i = 10000; i; i--)						\
-		if (!(__raw_readl(CLKCTRL_BASE_ADDR +			\
-			HW_CLKCTRL_##dr) & bm_busy))			\
-			break;						\
-	if (!i)	{							\
-		pr_err("%s: divider writing timeout\n", __func__);	\
-		return -ETIMEDOUT;					\
-	}								\
-									\
-	return 0;							\
+	return mxs_clkctrl_timeout(HW_CLKCTRL_##dr, bm_busy);		\
 }
 
 _CLK_SET_RATE(cpu_clk, CPU, FRAC0, CPU)
@@ -421,7 +426,6 @@  static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 {									\
 	u32 reg, div_max, div;						\
 	unsigned long parent_rate;					\
-	int i;								\
 									\
 	parent_rate = clk_get_rate(clk->parent);			\
 	div_max = BM_CLKCTRL_##dr##_DIV >> BP_CLKCTRL_##dr##_DIV;	\
@@ -439,16 +443,7 @@  static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 	}								\
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##dr);		\
 									\
-	for (i = 10000; i; i--)						\
-		if (!(__raw_readl(CLKCTRL_BASE_ADDR +			\
-			HW_CLKCTRL_##dr) & BM_CLKCTRL_##dr##_BUSY))	\
-			break;						\
-	if (!i)	{							\
-		pr_err("%s: divider writing timeout\n", __func__);	\
-		return -ETIMEDOUT;					\
-	}								\
-									\
-	return 0;							\
+	return mxs_clkctrl_timeout(HW_CLKCTRL_##dr, BM_CLKCTRL_##dr##_BUSY);\
 }
 
 _CLK_SET_RATE1(xbus_clk, XBUS)
@@ -461,7 +456,6 @@  static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 	u32 reg;							\
 	u64 lrate;							\
 	unsigned long parent_rate;					\
-	int i;								\
 									\
 	parent_rate = clk_get_rate(clk->parent);			\
 	if (rate > parent_rate)						\
@@ -479,16 +473,7 @@  static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
 									\
-	for (i = 10000; i; i--)						\
-		if (!(__raw_readl(CLKCTRL_BASE_ADDR +			\
-			HW_CLKCTRL_##rs) & BM_CLKCTRL_##rs##_BUSY))	\
-			break;						\
-	if (!i) {							\
-		pr_err("%s: divider writing timeout\n", __func__);	\
-		return -ETIMEDOUT;					\
-	}								\
-									\
-	return 0;							\
+	return mxs_clkctrl_timeout(HW_CLKCTRL_##rs, BM_CLKCTRL_##rs##_BUSY);\
 }
 
 _CLK_SET_RATE_SAIF(saif0_clk, SAIF0)
@@ -676,7 +661,7 @@  static struct clk_lookup lookups[] = {
 static int clk_misc_init(void)
 {
 	u32 reg;
-	int i;
+	int ret;
 
 	/* Fix up parent per register setting */
 	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_CLKSEQ);
@@ -756,14 +741,7 @@  static int clk_misc_init(void)
 	reg |= 3 << BP_CLKCTRL_HBUS_DIV;
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_HBUS);
 
-	for (i = 10000; i; i--)
-		if (!(__raw_readl(CLKCTRL_BASE_ADDR +
-			HW_CLKCTRL_HBUS) & BM_CLKCTRL_HBUS_ASM_BUSY))
-			break;
-	if (!i) {
-		pr_err("%s: divider writing timeout\n", __func__);
-		return -ETIMEDOUT;
-	}
+	ret = mxs_clkctrl_timeout(HW_CLKCTRL_HBUS, BM_CLKCTRL_HBUS_ASM_BUSY);
 
 	/* Gate off cpu clock in WFI for power saving */
 	__raw_writel(BM_CLKCTRL_CPU_INTERRUPT_WAIT,
@@ -790,7 +768,7 @@  static int clk_misc_init(void)
 	reg |= 30 << BP_CLKCTRL_FRAC0_IO0FRAC;
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_FRAC0);
 
-	return 0;
+	return ret;
 }
 
 int __init mx28_clocks_init(void)