All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism
@ 2012-01-23 14:41 Fabio Estevam
  2012-01-23 21:18 ` Wolfram Sang
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Fabio Estevam @ 2012-01-23 14:41 UTC (permalink / raw)
  To: linux-arm-kernel

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)
+{
+	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)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism
  2012-01-23 14:41 [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism Fabio Estevam
@ 2012-01-23 21:18 ` Wolfram Sang
  2012-01-23 21:31   ` Fabio Estevam
  2012-01-26 12:28 ` Shawn Guo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2012-01-23 21:18 UTC (permalink / raw)
  To: linux-arm-kernel

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120123/6bdaf43a/attachment.sig>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism
  2012-01-23 21:18 ` Wolfram Sang
@ 2012-01-23 21:31   ` Fabio Estevam
  0 siblings, 0 replies; 8+ messages in thread
From: Fabio Estevam @ 2012-01-23 21:31 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism
  2012-01-23 14:41 [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism Fabio Estevam
  2012-01-23 21:18 ` Wolfram Sang
@ 2012-01-26 12:28 ` Shawn Guo
  2012-01-27 14:56   ` Fabio Estevam
  2012-01-27 14:42 ` [PATCH v2] " Fabio Estevam
  2012-01-27 14:44 ` [PATCH v3] ARM: mxs: " Fabio Estevam
  3 siblings, 1 reply; 8+ messages in thread
From: Shawn Guo @ 2012-01-26 12:28 UTC (permalink / raw)
  To: linux-arm-kernel

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;
> +}

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2] ARM: mx28: clock-mx28: Use a proper timeout mechanism
  2012-01-23 14:41 [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism Fabio Estevam
  2012-01-23 21:18 ` Wolfram Sang
  2012-01-26 12:28 ` Shawn Guo
@ 2012-01-27 14:42 ` Fabio Estevam
  2012-01-27 14:44 ` [PATCH v3] ARM: mxs: " Fabio Estevam
  3 siblings, 0 replies; 8+ messages in thread
From: Fabio Estevam @ 2012-01-27 14:42 UTC (permalink / raw)
  To: linux-arm-kernel

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>
---
Changes since v1:
- Removed empty lines
- Removed _func_ from error message
- Put mxs_clkctrl_timeout in common file
- Change mx23 too
- Fixed comment about jiffy

 arch/arm/mach-mxs/clock-mx23.c          |   34 +++------------------
 arch/arm/mach-mxs/clock-mx28.c          |   49 ++++---------------------------
 arch/arm/mach-mxs/include/mach/common.h |    2 +
 arch/arm/mach-mxs/system.c              |   16 ++++++++++
 4 files changed, 29 insertions(+), 72 deletions(-)

diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
index e12e112..293958b 100644
--- a/arch/arm/mach-mxs/clock-mx23.c
+++ b/arch/arm/mach-mxs/clock-mx23.c
@@ -223,7 +223,6 @@ static int cpu_clk_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;
 
 	parent_rate = clk_get_rate(clk->parent);
 
@@ -275,14 +274,7 @@ static int cpu_clk_set_rate(struct clk *clk, unsigned long rate)
 	reg |= div << BP_CLKCTRL_CPU_DIV_CPU;
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_CPU);
 
-	for (i = 10000; i; i--)
-		if (!(__raw_readl(CLKCTRL_BASE_ADDR +
-					HW_CLKCTRL_CPU) & bm_busy))
-			break;
-	if (!i)	{
-		pr_err("%s: divider writing timeout\n", __func__);
-		return -ETIMEDOUT;
-	}
+	mxs_clkctrl_timeout(HW_CLKCTRL_CPU, bm_busy);
 
 	return 0;
 }
@@ -292,7 +284,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;	\
@@ -310,15 +301,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;					\
-	}								\
-									\
+	mxs_clkctrl_timeout(HW_CLKCTRL_##dr, BM_CLKCTRL_##dr##_BUSY);	\
 	return 0;							\
 }
 
@@ -461,7 +444,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);
@@ -510,14 +493,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_BUSY))
-			break;
-	if (!i) {
-		pr_err("%s: divider writing timeout\n", __func__);
-		return -ETIMEDOUT;
-	}
+	ret = mxs_clkctrl_timeout(HW_CLKCTRL_HBUS, BM_CLKCTRL_HBUS_BUSY);
 
 	/* Gate off cpu clock in WFI for power saving */
 	__raw_writel(BM_CLKCTRL_CPU_INTERRUPT_WAIT,
@@ -532,7 +508,7 @@ static int clk_misc_init(void)
 	reg |= 30 << BP_CLKCTRL_FRAC_IOFRAC;
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_FRAC);
 
-	return 0;
+	return ret;
 }
 
 int __init mx23_clocks_init(void)
diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index 5d68e41..f91b8fb 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -322,7 +322,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 +395,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 +411,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 +428,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 +441,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 +458,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 +646,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 +726,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 +753,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)
diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
index e1237ab..c50c3ea 100644
--- a/arch/arm/mach-mxs/include/mach/common.h
+++ b/arch/arm/mach-mxs/include/mach/common.h
@@ -31,4 +31,6 @@ extern void mx28_init_irq(void);
 
 extern void icoll_init_irq(void);
 
+extern int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask);
+
 #endif /* __MACH_MXS_COMMON_H__ */
diff --git a/arch/arm/mach-mxs/system.c b/arch/arm/mach-mxs/system.c
index 54f91ad..98e2de0 100644
--- a/arch/arm/mach-mxs/system.c
+++ b/arch/arm/mach-mxs/system.c
@@ -37,6 +37,8 @@
 #define MXS_MODULE_CLKGATE		(1 << 30)
 #define MXS_MODULE_SFTRST		(1 << 31)
 
+#define CLKCTRL_TIMEOUT		10	/* 10 ms */
+
 static void __iomem *mxs_clkctrl_reset_addr;
 
 /*
@@ -137,3 +139,17 @@ error:
 	return -ETIMEDOUT;
 }
 EXPORT_SYMBOL(mxs_reset_block);
+
+int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(CLKCTRL_TIMEOUT);
+	while (readl_relaxed(MXS_IO_ADDRESS(MXS_CLKCTRL_BASE_ADDR)
+						+ reg_offset) & mask) {
+		if (time_after(jiffies, timeout)) {
+			pr_err("Timeout@CLKCTRL + 0x%x\n", reg_offset);
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v3] ARM: mxs: Use a proper timeout mechanism
  2012-01-23 14:41 [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism Fabio Estevam
                   ` (2 preceding siblings ...)
  2012-01-27 14:42 ` [PATCH v2] " Fabio Estevam
@ 2012-01-27 14:44 ` Fabio Estevam
  2012-01-31 15:28   ` Shawn Guo
  3 siblings, 1 reply; 8+ messages in thread
From: Fabio Estevam @ 2012-01-27 14:44 UTC (permalink / raw)
  To: linux-arm-kernel

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>
---
Changes since v2:
- Update Subject
Changes since v1:
- Removed empty lines
- Removed _func_ from error message
- Put mxs_clkctrl_timeout in common file
- Change mx23 too
- Fixed comment about jiffy

 arch/arm/mach-mxs/clock-mx23.c          |   34 +++------------------
 arch/arm/mach-mxs/clock-mx28.c          |   49 ++++---------------------------
 arch/arm/mach-mxs/include/mach/common.h |    2 +
 arch/arm/mach-mxs/system.c              |   16 ++++++++++
 4 files changed, 29 insertions(+), 72 deletions(-)

diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
index e12e112..293958b 100644
--- a/arch/arm/mach-mxs/clock-mx23.c
+++ b/arch/arm/mach-mxs/clock-mx23.c
@@ -223,7 +223,6 @@ static int cpu_clk_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;
 
 	parent_rate = clk_get_rate(clk->parent);
 
@@ -275,14 +274,7 @@ static int cpu_clk_set_rate(struct clk *clk, unsigned long rate)
 	reg |= div << BP_CLKCTRL_CPU_DIV_CPU;
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_CPU);
 
-	for (i = 10000; i; i--)
-		if (!(__raw_readl(CLKCTRL_BASE_ADDR +
-					HW_CLKCTRL_CPU) & bm_busy))
-			break;
-	if (!i)	{
-		pr_err("%s: divider writing timeout\n", __func__);
-		return -ETIMEDOUT;
-	}
+	mxs_clkctrl_timeout(HW_CLKCTRL_CPU, bm_busy);
 
 	return 0;
 }
@@ -292,7 +284,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;	\
@@ -310,15 +301,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;					\
-	}								\
-									\
+	mxs_clkctrl_timeout(HW_CLKCTRL_##dr, BM_CLKCTRL_##dr##_BUSY);	\
 	return 0;							\
 }
 
@@ -461,7 +444,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);
@@ -510,14 +493,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_BUSY))
-			break;
-	if (!i) {
-		pr_err("%s: divider writing timeout\n", __func__);
-		return -ETIMEDOUT;
-	}
+	ret = mxs_clkctrl_timeout(HW_CLKCTRL_HBUS, BM_CLKCTRL_HBUS_BUSY);
 
 	/* Gate off cpu clock in WFI for power saving */
 	__raw_writel(BM_CLKCTRL_CPU_INTERRUPT_WAIT,
@@ -532,7 +508,7 @@ static int clk_misc_init(void)
 	reg |= 30 << BP_CLKCTRL_FRAC_IOFRAC;
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_FRAC);
 
-	return 0;
+	return ret;
 }
 
 int __init mx23_clocks_init(void)
diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index 5d68e41..f91b8fb 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -322,7 +322,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 +395,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 +411,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 +428,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 +441,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 +458,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 +646,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 +726,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 +753,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)
diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
index e1237ab..c50c3ea 100644
--- a/arch/arm/mach-mxs/include/mach/common.h
+++ b/arch/arm/mach-mxs/include/mach/common.h
@@ -31,4 +31,6 @@ extern void mx28_init_irq(void);
 
 extern void icoll_init_irq(void);
 
+extern int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask);
+
 #endif /* __MACH_MXS_COMMON_H__ */
diff --git a/arch/arm/mach-mxs/system.c b/arch/arm/mach-mxs/system.c
index 54f91ad..98e2de0 100644
--- a/arch/arm/mach-mxs/system.c
+++ b/arch/arm/mach-mxs/system.c
@@ -37,6 +37,8 @@
 #define MXS_MODULE_CLKGATE		(1 << 30)
 #define MXS_MODULE_SFTRST		(1 << 31)
 
+#define CLKCTRL_TIMEOUT		10	/* 10 ms */
+
 static void __iomem *mxs_clkctrl_reset_addr;
 
 /*
@@ -137,3 +139,17 @@ error:
 	return -ETIMEDOUT;
 }
 EXPORT_SYMBOL(mxs_reset_block);
+
+int mxs_clkctrl_timeout(unsigned int reg_offset, unsigned int mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(CLKCTRL_TIMEOUT);
+	while (readl_relaxed(MXS_IO_ADDRESS(MXS_CLKCTRL_BASE_ADDR)
+						+ reg_offset) & mask) {
+		if (time_after(jiffies, timeout)) {
+			pr_err("Timeout@CLKCTRL + 0x%x\n", reg_offset);
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism
  2012-01-26 12:28 ` Shawn Guo
@ 2012-01-27 14:56   ` Fabio Estevam
  0 siblings, 0 replies; 8+ messages in thread
From: Fabio Estevam @ 2012-01-27 14:56 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v3] ARM: mxs: Use a proper timeout mechanism
  2012-01-27 14:44 ` [PATCH v3] ARM: mxs: " Fabio Estevam
@ 2012-01-31 15:28   ` Shawn Guo
  0 siblings, 0 replies; 8+ messages in thread
From: Shawn Guo @ 2012-01-31 15:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jan 27, 2012 at 12:44:29PM -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>

Applied, thanks.

-- 
Regards,
Shawn

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-01-31 15:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-23 14:41 [PATCH] ARM: mx28: clock-mx28: Use a proper timeout mechanism Fabio Estevam
2012-01-23 21:18 ` Wolfram Sang
2012-01-23 21:31   ` Fabio Estevam
2012-01-26 12:28 ` Shawn Guo
2012-01-27 14:56   ` Fabio Estevam
2012-01-27 14:42 ` [PATCH v2] " Fabio Estevam
2012-01-27 14:44 ` [PATCH v3] ARM: mxs: " Fabio Estevam
2012-01-31 15:28   ` Shawn Guo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.