Re: [PATCH v2 3/5] clk: amlogic: Add handling for PLL lock failure
From: Chuan Liu
Date: Thu Oct 30 2025 - 07:49:36 EST
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@xxxxxxxxxx> wrote:
From: Chuan Liu <chuan.liu@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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