[PATCH] clk: meson: pll: adjust timeout in meson_clk_pll_wait_lock()

[PATCH] clk: meson: pll: adjust timeout in meson_clk_pll_wait_lock()

[Heiner Kallweit]

Currently we loop over meson_parm_read() up to 24mln times.
This results in a unpredictable timeout period. In my case
it's over 5s on a S905X4-based system. Make the timeout
period predictable and set it to 100ms.

Whilst we're at it: All callers of this function return -EIO
in case of failure, therefore we can return this value directly
in the timeout case.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/clk/meson/clk-pll.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index daa025b6d..d70bee331 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -277,17 +277,17 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-	int delay = 24000000;
+	int delay = 5000;
 
 	do {
-		/* Is the clock locked now ? */
+		/* Is the clock locked now ? Time out after 100ms. */
 		if (meson_parm_read(clk->map, &pll->l))
 			return 0;
 
-		delay--;
-	} while (delay > 0);
+		udelay(20);
+	} while (--delay);
 
-	return -ETIMEDOUT;
+	return -EIO;
 }
 
 static int meson_clk_pll_init(struct clk_hw *hw)
@@ -350,10 +350,7 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 	/* Take the pll out reset */
 	meson_parm_write(clk->map, &pll->rst, 0);
 
-	if (meson_clk_pll_wait_lock(hw))
-		return -EIO;
-
-	return 0;
+	return meson_clk_pll_wait_lock(hw);
 }
 
 static void meson_clk_pll_disable(struct clk_hw *hw)
-- 
2.37.2


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] clk: meson: pll: adjust timeout in meson_clk_pll_wait_lock()

[Jerome Brunet]

On Sun 14 Aug 2022 at 23:29, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> Currently we loop over meson_parm_read() up to 24mln times.
> This results in a unpredictable timeout period. In my case
> it's over 5s on a S905X4-based system. Make the timeout
> period predictable and set it to 100ms.
>
> Whilst we're at it: All callers of this function return -EIO
> in case of failure, therefore we can return this value directly
> in the timeout case.

I'm okay with this change but I'd prefer if one change addressed a
single topic. Please split this out.

>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/clk/meson/clk-pll.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index daa025b6d..d70bee331 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -277,17 +277,17 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> -	int delay = 24000000;
> +	int delay = 5000;
>  
>  	do {
> -		/* Is the clock locked now ? */
> +		/* Is the clock locked now ? Time out after 100ms. */
>  		if (meson_parm_read(clk->map, &pll->l))
>  			return 0;
>  
> -		delay--;
> -	} while (delay > 0);
> +		udelay(20);
> +	} while (--delay);
>  
> -	return -ETIMEDOUT;
> +	return -EIO;
>  }
>  
>  static int meson_clk_pll_init(struct clk_hw *hw)
> @@ -350,10 +350,7 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  	/* Take the pll out reset */
>  	meson_parm_write(clk->map, &pll->rst, 0);
>  
> -	if (meson_clk_pll_wait_lock(hw))
> -		return -EIO;
> -
> -	return 0;
> +	return meson_clk_pll_wait_lock(hw);
>  }
>  
>  static void meson_clk_pll_disable(struct clk_hw *hw)


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] clk: meson: pll: adjust timeout in meson_clk_pll_wait_lock()

[Heiner Kallweit]

On 29.08.2022 11:52, Jerome Brunet wrote:
> 
> On Sun 14 Aug 2022 at 23:29, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> Currently we loop over meson_parm_read() up to 24mln times.
>> This results in a unpredictable timeout period. In my case
>> it's over 5s on a S905X4-based system. Make the timeout
>> period predictable and set it to 100ms.
>>
>> Whilst we're at it: All callers of this function return -EIO
>> in case of failure, therefore we can return this value directly
>> in the timeout case.
> 
> I'm okay with this change but I'd prefer if one change addressed a
> single topic. Please split this out.
> 
OK, I'll send a v2.

>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/clk/meson/clk-pll.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index daa025b6d..d70bee331 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -277,17 +277,17 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
>>  {
>>  	struct clk_regmap *clk = to_clk_regmap(hw);
>>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>> -	int delay = 24000000;
>> +	int delay = 5000;
>>  
>>  	do {
>> -		/* Is the clock locked now ? */
>> +		/* Is the clock locked now ? Time out after 100ms. */
>>  		if (meson_parm_read(clk->map, &pll->l))
>>  			return 0;
>>  
>> -		delay--;
>> -	} while (delay > 0);
>> +		udelay(20);
>> +	} while (--delay);
>>  
>> -	return -ETIMEDOUT;
>> +	return -EIO;
>>  }
>>  
>>  static int meson_clk_pll_init(struct clk_hw *hw)
>> @@ -350,10 +350,7 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>  	/* Take the pll out reset */
>>  	meson_parm_write(clk->map, &pll->rst, 0);
>>  
>> -	if (meson_clk_pll_wait_lock(hw))
>> -		return -EIO;
>> -
>> -	return 0;
>> +	return meson_clk_pll_wait_lock(hw);
>>  }
>>  
>>  static void meson_clk_pll_disable(struct clk_hw *hw)
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic