1. Patchew
  2. linux
  3. [PATCH v2 0/5] clk: amlogic: optimize the PLL driver
  4. View patch

[PATCH v2 3/5] clk: amlogic: Add handling for PLL lock failure

Chuan Liu via B4 Relay posted 5 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v2 3/5] clk: amlogic: Add handling for PLL lock failure
Posted by Chuan Liu via B4 Relay 1 month, 2 weeks ago 
From: Chuan Liu <chuan.liu@amlogic.com>

If the PLL fails to lock, it should be disabled, This makes the logic
more complete, and also helps save unnecessary power consumption when
the PLL is malfunctioning.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/clk-pll.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index f81ebf6cc981..6c794adb8ccd 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -353,6 +353,23 @@ static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
 	return -EIO;
 }
 
+static void meson_clk_pll_disable(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
+
+	/* Put the pll is in reset */
+	if (MESON_PARM_APPLICABLE(&pll->rst))
+		meson_parm_write(clk->map, &pll->rst, 1);
+
+	/* Disable the pll */
+	meson_parm_write(clk->map, &pll->en, 0);
+
+	/* Disable PLL internal self-adaption current module */
+	if (MESON_PARM_APPLICABLE(&pll->current_en))
+		meson_parm_write(clk->map, &pll->current_en, 0);
+}
+
 static int meson_clk_pll_enable(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
@@ -397,29 +414,17 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 		meson_parm_write(clk->map, &pll->l_detect, 0);
 	}
 
-	if (meson_clk_pll_wait_lock(hw))
+	if (meson_clk_pll_wait_lock(hw)) {
+		/* disable PLL when PLL lock failed. */
+		meson_clk_pll_disable(hw);
+		pr_warn("%s: PLL lock failed!!!\n", clk_hw_get_name(hw));
+
 		return -EIO;
+	}
 
 	return 0;
 }
 
-static void meson_clk_pll_disable(struct clk_hw *hw)
-{
-	struct clk_regmap *clk = to_clk_regmap(hw);
-	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-
-	/* Put the pll is in reset */
-	if (MESON_PARM_APPLICABLE(&pll->rst))
-		meson_parm_write(clk->map, &pll->rst, 1);
-
-	/* Disable the pll */
-	meson_parm_write(clk->map, &pll->en, 0);
-
-	/* Disable PLL internal self-adaption current module */
-	if (MESON_PARM_APPLICABLE(&pll->current_en))
-		meson_parm_write(clk->map, &pll->current_en, 0);
-}
-
 static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long parent_rate)
 {

-- 
2.42.0
Re: [PATCH v2 3/5] clk: amlogic: Add handling for PLL lock failure
Posted by Jerome Brunet 1 month, 2 weeks ago 
On Thu 30 Oct 2025 at 13:24, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> From: Chuan Liu <chuan.liu@amlogic.com>
>
> If the PLL fails to lock, it should be disabled, This makes the logic
> more complete, and also helps save unnecessary power consumption when
> the PLL is malfunctioning.
>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/clk-pll.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index f81ebf6cc981..6c794adb8ccd 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -353,6 +353,23 @@ static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
>  	return -EIO;
>  }
>  
> +static void meson_clk_pll_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +
> +	/* Put the pll is in reset */
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 1);
> +
> +	/* Disable the pll */
> +	meson_parm_write(clk->map, &pll->en, 0);
> +
> +	/* Disable PLL internal self-adaption current module */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en))
> +		meson_parm_write(clk->map, &pll->current_en, 0);
> +}
> +
>  static int meson_clk_pll_enable(struct clk_hw *hw)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
> @@ -397,29 +414,17 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  		meson_parm_write(clk->map, &pll->l_detect, 0);
>  	}
>  
> -	if (meson_clk_pll_wait_lock(hw))
> +	if (meson_clk_pll_wait_lock(hw)) {
> +		/* disable PLL when PLL lock failed. */
> +		meson_clk_pll_disable(hw);
> +		pr_warn("%s: PLL lock failed!!!\n", clk_hw_get_name(hw));
> +

This is something that's likely to spam, so pr_warn_once() (if you must warn)
and I don't think 3 "!" are necessary to convey the message.

>  		return -EIO;
> +	}
>  
>  	return 0;
>  }
>  
> -static void meson_clk_pll_disable(struct clk_hw *hw)
> -{
> -	struct clk_regmap *clk = to_clk_regmap(hw);
> -	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> -
> -	/* Put the pll is in reset */
> -	if (MESON_PARM_APPLICABLE(&pll->rst))
> -		meson_parm_write(clk->map, &pll->rst, 1);
> -
> -	/* Disable the pll */
> -	meson_parm_write(clk->map, &pll->en, 0);
> -
> -	/* Disable PLL internal self-adaption current module */
> -	if (MESON_PARM_APPLICABLE(&pll->current_en))
> -		meson_parm_write(clk->map, &pll->current_en, 0);
> -}
> -
>  static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  				  unsigned long parent_rate)
>  {

-- 
Jerome
Re: [PATCH v2 3/5] clk: amlogic: Add handling for PLL lock failure
Posted by Chuan Liu 1 month, 2 weeks ago 
Hi Jerome,

On 10/30/2025 4:32 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On Thu 30 Oct 2025 at 13:24, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
> 
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> If the PLL fails to lock, it should be disabled, This makes the logic
>> more complete, and also helps save unnecessary power consumption when
>> the PLL is malfunctioning.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/meson/clk-pll.c | 41 +++++++++++++++++++++++------------------
>>   1 file changed, 23 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index f81ebf6cc981..6c794adb8ccd 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -353,6 +353,23 @@ static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
>>        return -EIO;
>>   }
>>
>> +static void meson_clk_pll_disable(struct clk_hw *hw)
>> +{
>> +     struct clk_regmap *clk = to_clk_regmap(hw);
>> +     struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>> +
>> +     /* Put the pll is in reset */
>> +     if (MESON_PARM_APPLICABLE(&pll->rst))
>> +             meson_parm_write(clk->map, &pll->rst, 1);
>> +
>> +     /* Disable the pll */
>> +     meson_parm_write(clk->map, &pll->en, 0);
>> +
>> +     /* Disable PLL internal self-adaption current module */
>> +     if (MESON_PARM_APPLICABLE(&pll->current_en))
>> +             meson_parm_write(clk->map, &pll->current_en, 0);
>> +}
>> +
>>   static int meson_clk_pll_enable(struct clk_hw *hw)
>>   {
>>        struct clk_regmap *clk = to_clk_regmap(hw);
>> @@ -397,29 +414,17 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>                meson_parm_write(clk->map, &pll->l_detect, 0);
>>        }
>>
>> -     if (meson_clk_pll_wait_lock(hw))
>> +     if (meson_clk_pll_wait_lock(hw)) {
>> +             /* disable PLL when PLL lock failed. */
>> +             meson_clk_pll_disable(hw);
>> +             pr_warn("%s: PLL lock failed!!!\n", clk_hw_get_name(hw));
>> +
> 
> This is something that's likely to spam, so pr_warn_once() (if you must warn)
> and I don't think 3 "!" are necessary to convey the message.
> 

In the next version, I'll remove the "!", and replace pr_warn to
pr_warn_once.

Some drivers that reference the clock may not check the function’s
return value (even though that's not ideal), so adding this warning
makes the issue more noticeable.

>>                return -EIO;
>> +     }
>>
>>        return 0;
>>   }
>>
>> -static void meson_clk_pll_disable(struct clk_hw *hw)
>> -{
>> -     struct clk_regmap *clk = to_clk_regmap(hw);
>> -     struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>> -
>> -     /* Put the pll is in reset */
>> -     if (MESON_PARM_APPLICABLE(&pll->rst))
>> -             meson_parm_write(clk->map, &pll->rst, 1);
>> -
>> -     /* Disable the pll */
>> -     meson_parm_write(clk->map, &pll->en, 0);
>> -
>> -     /* Disable PLL internal self-adaption current module */
>> -     if (MESON_PARM_APPLICABLE(&pll->current_en))
>> -             meson_parm_write(clk->map, &pll->current_en, 0);
>> -}
>> -
>>   static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                  unsigned long parent_rate)
>>   {
> 
> --
> Jerome
Re: [PATCH v2 3/5] clk: amlogic: Add handling for PLL lock failure
Posted by Chuan Liu 1 month, 2 weeks ago 
Hi Jerome,

On 10/30/2025 5:15 PM, Chuan Liu wrote:
> Hi Jerome,
> 
> On 10/30/2025 4:32 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Thu 30 Oct 2025 at 13:24, Chuan Liu via B4 Relay 
>> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>>
>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>
>>> If the PLL fails to lock, it should be disabled, This makes the logic
>>> more complete, and also helps save unnecessary power consumption when
>>> the PLL is malfunctioning.
>>>
>>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/clk-pll.c | 41 ++++++++++++++++++++++ 
>>> +------------------
>>>   1 file changed, 23 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>> index f81ebf6cc981..6c794adb8ccd 100644
>>> --- a/drivers/clk/meson/clk-pll.c
>>> +++ b/drivers/clk/meson/clk-pll.c
>>> @@ -353,6 +353,23 @@ static int meson_clk_pcie_pll_enable(struct 
>>> clk_hw *hw)
>>>        return -EIO;
>>>   }
>>>
>>> +static void meson_clk_pll_disable(struct clk_hw *hw)
>>> +{
>>> +     struct clk_regmap *clk = to_clk_regmap(hw);
>>> +     struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>> +
>>> +     /* Put the pll is in reset */
>>> +     if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +             meson_parm_write(clk->map, &pll->rst, 1);
>>> +
>>> +     /* Disable the pll */
>>> +     meson_parm_write(clk->map, &pll->en, 0);
>>> +
>>> +     /* Disable PLL internal self-adaption current module */
>>> +     if (MESON_PARM_APPLICABLE(&pll->current_en))
>>> +             meson_parm_write(clk->map, &pll->current_en, 0);
>>> +}
>>> +
>>>   static int meson_clk_pll_enable(struct clk_hw *hw)
>>>   {
>>>        struct clk_regmap *clk = to_clk_regmap(hw);
>>> @@ -397,29 +414,17 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>>                meson_parm_write(clk->map, &pll->l_detect, 0);
>>>        }
>>>
>>> -     if (meson_clk_pll_wait_lock(hw))
>>> +     if (meson_clk_pll_wait_lock(hw)) {
>>> +             /* disable PLL when PLL lock failed. */
>>> +             meson_clk_pll_disable(hw);
>>> +             pr_warn("%s: PLL lock failed!!!\n", clk_hw_get_name(hw));
>>> +
>>
>> This is something that's likely to spam, so pr_warn_once() (if you 
>> must warn)

Different PLLs or different timing conditions may trigger PLL lock
failures repeatedly, so using pr_warn_once() might not achieve the
intended effect.

Do you mind keeping pr_warn() here?

>> and I don't think 3 "!" are necessary to convey the message.
>>
> 
> In the next version, I'll remove the "!", and replace pr_warn to
> pr_warn_once.
> 
> Some drivers that reference the clock may not check the function’s
> return value (even though that's not ideal), so adding this warning
> makes the issue more noticeable.
> 
>>>                return -EIO;
>>> +     }
>>>
>>>        return 0;
>>>   }
>>>
>>> -static void meson_clk_pll_disable(struct clk_hw *hw)
>>> -{
>>> -     struct clk_regmap *clk = to_clk_regmap(hw);
>>> -     struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>> -
>>> -     /* Put the pll is in reset */
>>> -     if (MESON_PARM_APPLICABLE(&pll->rst))
>>> -             meson_parm_write(clk->map, &pll->rst, 1);
>>> -
>>> -     /* Disable the pll */
>>> -     meson_parm_write(clk->map, &pll->en, 0);
>>> -
>>> -     /* Disable PLL internal self-adaption current module */
>>> -     if (MESON_PARM_APPLICABLE(&pll->current_en))
>>> -             meson_parm_write(clk->map, &pll->current_en, 0);
>>> -}
>>> -
>>>   static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long 
>>> rate,
>>>                                  unsigned long parent_rate)
>>>   {
>>
>> -- 
>> Jerome
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.