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

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(-)

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

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

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

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