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

[Chuan Liu <chuan.liu@amlogic.com>]

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
>