[PATCH v2 0/5] clk: amlogic: optimize the PLL driver

[PATCH v2 0/5] clk: amlogic: optimize the PLL driver

[Chuan Liu via B4 Relay]

This patch series consists of four topics involving the amlogic PLL
driver:
- Fix out-of-range PLL frequency setting
- Improve the issue of PLL lock failures
- Add handling for PLL lock failure
- Optimize PLL enable timing
- Change the active level of l_detect

For easier review and management, these are submitted as a single
patch series.

The PLL timing optimization changes were merged into our internal
repository quite some time ago and have been verified on a large
number of SoCs :
- Already supported upstream: G12A, G12B, SM1, S4, A1, C3
- Planned for upstream support: T7, A5, A4, S7, S7D, S6, etc.

Based on the upstream code base, I have performed functional testing
on G12A, A1, A5, A4, T7, S7, S7D, and S6, all of which passed.

Additionally, stress testing using scripts was conducted on A5 and
A1, with over 40,000 and 50,000 iterations respectively, and no
abnormalities were observed. Below is a portion of the stress test
log (CLOCK_ALLOW_WRITE_DEBUGFS has been manually enabled):

- For A5:
  # echo 491520000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
  # cnt=0
  # while true; do
  >     echo "------------ cnt=$cnt -----------"
  >     echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
  >     en=$(cat /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable 2>/dev/null)
  >     if [ "$en" != "1" ]; then
  >         echo "[ERROR] PLL enable test failed! (clk_prepare_enable=$en)"
  >         break
  >     fi
  > 
  >     echo 0 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
  >     cnt=$((cnt + 1))
  >     echo -e "sleep time: 1 s."
  >     sleep 1
  > done &
  # ------------ cnt=0 -----------
  sleep time: 1 s.
  ------------ cnt=1 -----------
  sleep time: 1 s.
  ------------ cnt=2 -----------
  sleep time: 1 s.
  ...
  ------------ cnt=42076 -----------
  sleep time: 1 s.

- For A1:
  # echo 983040000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
  # cnt=0
  # while true; do
  >     echo "------------ cnt=$cnt -----------"
  >     echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
  >     en=$(cat /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable 2>/dev/null)
  >     if [ "$en" != "1" ]; then
  >         echo "[ERROR] PLL enable test failed! (clk_prepare_enable=$en)"
  >         break
  >     fi
  > 
  >     echo 0 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
  >     cnt=$((cnt + 1))
  >     echo -e "sleep time: 1 s."
  >     sleep 1
  > done &
  # ------------ cnt=0 -----------
  sleep time: 1 s.
  ------------ cnt=1 -----------
  sleep time: 1 s.
  ------------ cnt=2 -----------
  sleep time: 1 s.
  ...
  ------------ cnt=55051 -----------
  sleep time: 1 s.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
Changes in v2:
- Modify the judgment condition of 'm' out of range.
- Split the PLL timing optimization patch to make it easier to review.
- Link to v1: https://lore.kernel.org/r/20251022-optimize_pll_driver-v1-0-a275722fb6f4@amlogic.com

---
Chuan Liu (5):
      clk: amlogic: Fix out-of-range PLL frequency setting
      clk: amlogic: Improve the issue of PLL lock failures
      clk: amlogic: Add handling for PLL lock failure
      clk: amlogic: Optimize PLL enable timing
      clk: amlogic: Change the active level of l_detect

 drivers/clk/meson/a1-pll.c  |  1 +
 drivers/clk/meson/clk-pll.c | 76 ++++++++++++++++++++++++++++-----------------
 drivers/clk/meson/clk-pll.h |  2 ++
 3 files changed, 51 insertions(+), 28 deletions(-)
---
base-commit: 01f3a6d1d59b8e25a6de243b0d73075cf0415eaf
change-id: 20251020-optimize_pll_driver-7bef91876c41

Best regards,
-- 
Chuan Liu <chuan.liu@amlogic.com>

[PATCH v2 1/5] clk: amlogic: Fix out-of-range PLL frequency setting

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

If the calculated 'm' falls into the range:

    pll->range->max < m < (1 << pll->m.width)

Here an incorrect 'm' value could be obtained, so an additional
condition is added to ensure that the calculated 'm' stays within a
valid range.

Fixes: 8eed1db1adec6 ("clk: meson: pll: update driver for the g12a")
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/clk-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 1ea6579a760f..629f6af18ea1 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -191,7 +191,7 @@ static int meson_clk_get_pll_range_index(unsigned long rate,
 	*m = meson_clk_get_pll_range_m(rate, parent_rate, *n, pll);
 
 	/* the pre-divider gives a multiplier too big - stop */
-	if (*m >= (1 << pll->m.width))
+	if (*m > pll->range->max || *m >= (1 << pll->m.width))
 		return -EINVAL;
 
 	return 0;

-- 
2.42.0

[PATCH v2 2/5] clk: amlogic: Improve the issue of PLL lock failures

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

Due to factors such as temperature and process variations, the
internal circuits of the PLL may require a longer time to reach a
steady state, which can result in occasional lock failures on some
SoCs under low-temperature conditions.

After enabling the PLL and releasing its reset, a 20 us delay is
added at each step to provide enough time for the internal PLL
circuit to stabilize, thus reducing the probability of PLL lock
failure.

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

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 629f6af18ea1..f81ebf6cc981 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -368,11 +368,16 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 
 	/* Enable the pll */
 	meson_parm_write(clk->map, &pll->en, 1);
+	/* Wait for Bandgap and LDO to power up and stabilize */
+	udelay(20);
 
 	/* Take the pll out reset */
 	if (MESON_PARM_APPLICABLE(&pll->rst))
 		meson_parm_write(clk->map, &pll->rst, 0);
 
+	/* Wait for PLL loop stabilization */
+	udelay(20);
+
 	/*
 	 * Compared with the previous SoCs, self-adaption current module
 	 * is newly added for A1, keep the new power-on sequence to enable the

-- 
2.42.0

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

[Chuan Liu via B4 Relay]

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

[PATCH v2 4/5] clk: amlogic: Optimize PLL enable timing

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

l_detect controls the enablement of the PLL lock detection module.
It should remain disabled while the internal PLL circuits are
reaching a steady state; otherwise, the lock signal may be falsely
triggered high.

Before enabling the internal power supply of the PLL, l_detect should
be disabled. After the PLL’s internal circuits have stabilized,
l_detect should be enabled to prevent false lock signal triggers.

Currently, only A1 supports both l_detect and current_en, so this
patch will only affect A1.

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

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 6c794adb8ccd..c6eebde1f516 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -383,36 +383,38 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 	if (MESON_PARM_APPLICABLE(&pll->rst))
 		meson_parm_write(clk->map, &pll->rst, 1);
 
+	/* Disable the PLL lock-detect module */
+	if (MESON_PARM_APPLICABLE(&pll->l_detect))
+		meson_parm_write(clk->map, &pll->l_detect, 1);
+
 	/* Enable the pll */
 	meson_parm_write(clk->map, &pll->en, 1);
 	/* Wait for Bandgap and LDO to power up and stabilize */
 	udelay(20);
 
-	/* Take the pll out reset */
-	if (MESON_PARM_APPLICABLE(&pll->rst))
-		meson_parm_write(clk->map, &pll->rst, 0);
-
-	/* Wait for PLL loop stabilization */
-	udelay(20);
-
 	/*
 	 * Compared with the previous SoCs, self-adaption current module
 	 * is newly added for A1, keep the new power-on sequence to enable the
 	 * PLL. The sequence is:
-	 * 1. enable the pll, delay for 10us
+	 * 1. enable the pll, delay for 20us
 	 * 2. enable the pll self-adaption current module, delay for 40us
 	 * 3. enable the lock detect module
 	 */
 	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
-		udelay(10);
 		meson_parm_write(clk->map, &pll->current_en, 1);
-		udelay(40);
+		udelay(20);
 	}
 
-	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
-		meson_parm_write(clk->map, &pll->l_detect, 1);
+	/* Take the pll out reset */
+	if (MESON_PARM_APPLICABLE(&pll->rst))
+		meson_parm_write(clk->map, &pll->rst, 0);
+
+	/* Wait for PLL loop stabilization */
+	udelay(20);
+
+	/* Enable the lock-detect module */
+	if (MESON_PARM_APPLICABLE(&pll->l_detect))
 		meson_parm_write(clk->map, &pll->l_detect, 0);
-	}
 
 	if (meson_clk_pll_wait_lock(hw)) {
 		/* disable PLL when PLL lock failed. */

-- 
2.42.0

[PATCH v2 5/5] clk: amlogic: Change the active level of l_detect

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

l_detect controls the enable/disable of the PLL lock-detect module.

The enable signal is normally active-high. This design ensures that
the module remains disabled during the power-on process, preventing
power fluctuations from affecting its operating state.

For A1, the l_detect signal is active-low:
0 -> Enable lock-detect module;
1 -> Disable lock-detect module.

Here, a flag CLK_MESON_PLL_L_DETECT_N is added to handle cases like
A1, where the signal is active-low.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/a1-pll.c  |  1 +
 drivers/clk/meson/clk-pll.c | 16 ++++++++++++----
 drivers/clk/meson/clk-pll.h |  2 ++
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
index 1f82e9c7c14e..bfe559c71402 100644
--- a/drivers/clk/meson/a1-pll.c
+++ b/drivers/clk/meson/a1-pll.c
@@ -137,6 +137,7 @@ static struct clk_regmap a1_hifi_pll = {
 		.range = &a1_hifi_pll_range,
 		.init_regs = a1_hifi_pll_init_regs,
 		.init_count = ARRAY_SIZE(a1_hifi_pll_init_regs),
+		.flags = CLK_MESON_PLL_L_DETECT_N
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "hifi_pll",
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index c6eebde1f516..d729e933aa1c 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -384,8 +384,12 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 		meson_parm_write(clk->map, &pll->rst, 1);
 
 	/* Disable the PLL lock-detect module */
-	if (MESON_PARM_APPLICABLE(&pll->l_detect))
-		meson_parm_write(clk->map, &pll->l_detect, 1);
+	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
+		if (pll->flags & CLK_MESON_PLL_L_DETECT_N)
+			meson_parm_write(clk->map, &pll->l_detect, 1);
+		else
+			meson_parm_write(clk->map, &pll->l_detect, 0);
+	}
 
 	/* Enable the pll */
 	meson_parm_write(clk->map, &pll->en, 1);
@@ -413,8 +417,12 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 	udelay(20);
 
 	/* Enable the lock-detect module */
-	if (MESON_PARM_APPLICABLE(&pll->l_detect))
-		meson_parm_write(clk->map, &pll->l_detect, 0);
+	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
+		if (pll->flags & CLK_MESON_PLL_L_DETECT_N)
+			meson_parm_write(clk->map, &pll->l_detect, 0);
+		else
+			meson_parm_write(clk->map, &pll->l_detect, 1);
+	}
 
 	if (meson_clk_pll_wait_lock(hw)) {
 		/* disable PLL when PLL lock failed. */
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index 949157fb7bf5..83295a24721f 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -29,6 +29,8 @@ struct pll_mult_range {
 
 #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
 #define CLK_MESON_PLL_NOINIT_ENABLED	BIT(1)
+/* l_detect signal is active-low */
+#define CLK_MESON_PLL_L_DETECT_N	BIT(2)
 
 struct meson_clk_pll_data {
 	struct parm en;

-- 
2.42.0

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

[Jerome Brunet]

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 4/5] clk: amlogic: Optimize PLL enable timing

[Jerome Brunet]

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>
>
> l_detect controls the enablement of the PLL lock detection module.
> It should remain disabled while the internal PLL circuits are
> reaching a steady state; otherwise, the lock signal may be falsely
> triggered high.
>
> Before enabling the internal power supply of the PLL, l_detect should
> be disabled. After the PLL’s internal circuits have stabilized,
> l_detect should be enabled to prevent false lock signal triggers.

You to reformat this description. It feel that both paragraph are saying
the same thing.

>
> Currently, only A1 supports both l_detect and current_en, so this
> patch will only affect A1.
>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/clk-pll.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 6c794adb8ccd..c6eebde1f516 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -383,36 +383,38 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  	if (MESON_PARM_APPLICABLE(&pll->rst))
>  		meson_parm_write(clk->map, &pll->rst, 1);
>  
> +	/* Disable the PLL lock-detect module */
> +	if (MESON_PARM_APPLICABLE(&pll->l_detect))
> +		meson_parm_write(clk->map, &pll->l_detect, 1);
> +
>  	/* Enable the pll */
>  	meson_parm_write(clk->map, &pll->en, 1);
>  	/* Wait for Bandgap and LDO to power up and stabilize */
>  	udelay(20);
>  
> -	/* Take the pll out reset */
> -	if (MESON_PARM_APPLICABLE(&pll->rst))
> -		meson_parm_write(clk->map, &pll->rst, 0);

Why is the reset moving around ? nothing is said in the description about
that

> -
> -	/* Wait for PLL loop stabilization */
> -	udelay(20);
> -
>  	/*
>  	 * Compared with the previous SoCs, self-adaption current module
>  	 * is newly added for A1, keep the new power-on sequence to enable the
>  	 * PLL. The sequence is:
> -	 * 1. enable the pll, delay for 10us
> +	 * 1. enable the pll, delay for 20us
>  	 * 2. enable the pll self-adaption current module, delay for 40us
>  	 * 3. enable the lock detect module
>  	 */
>  	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> -		udelay(10);
>  		meson_parm_write(clk->map, &pll->current_en, 1);
> -		udelay(40);
> +		udelay(20);
>  	}
>  
> -	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
> -		meson_parm_write(clk->map, &pll->l_detect, 1);
> +	/* Take the pll out reset */
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 0);
> +
> +	/* Wait for PLL loop stabilization */
> +	udelay(20);
> +
> +	/* Enable the lock-detect module */
> +	if (MESON_PARM_APPLICABLE(&pll->l_detect))
>  		meson_parm_write(clk->map, &pll->l_detect, 0);
> -	}
>  
>  	if (meson_clk_pll_wait_lock(hw)) {
>  		/* disable PLL when PLL lock failed. */

-- 
Jerome

Re: [PATCH v2 5/5] clk: amlogic: Change the active level of l_detect

[Jerome Brunet]

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>
>
> l_detect controls the enable/disable of the PLL lock-detect module.
>
> The enable signal is normally active-high. This design ensures that
> the module remains disabled during the power-on process, preventing
> power fluctuations from affecting its operating state.
>
> For A1, the l_detect signal is active-low:
> 0 -> Enable lock-detect module;
> 1 -> Disable lock-detect module.
>
> Here, a flag CLK_MESON_PLL_L_DETECT_N is added to handle cases like
> A1, where the signal is active-low.

Given that A1 is the only ship supporting l_detect, this change is not
necessary.

Not now at least.

>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/a1-pll.c  |  1 +
>  drivers/clk/meson/clk-pll.c | 16 ++++++++++++----
>  drivers/clk/meson/clk-pll.h |  2 ++
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> index 1f82e9c7c14e..bfe559c71402 100644
> --- a/drivers/clk/meson/a1-pll.c
> +++ b/drivers/clk/meson/a1-pll.c
> @@ -137,6 +137,7 @@ static struct clk_regmap a1_hifi_pll = {
>  		.range = &a1_hifi_pll_range,
>  		.init_regs = a1_hifi_pll_init_regs,
>  		.init_count = ARRAY_SIZE(a1_hifi_pll_init_regs),
> +		.flags = CLK_MESON_PLL_L_DETECT_N
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "hifi_pll",
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index c6eebde1f516..d729e933aa1c 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -384,8 +384,12 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  		meson_parm_write(clk->map, &pll->rst, 1);
>  
>  	/* Disable the PLL lock-detect module */
> -	if (MESON_PARM_APPLICABLE(&pll->l_detect))
> -		meson_parm_write(clk->map, &pll->l_detect, 1);
> +	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
> +		if (pll->flags & CLK_MESON_PLL_L_DETECT_N)
> +			meson_parm_write(clk->map, &pll->l_detect, 1);
> +		else
> +			meson_parm_write(clk->map, &pll->l_detect, 0);
> +	}
>  
>  	/* Enable the pll */
>  	meson_parm_write(clk->map, &pll->en, 1);
> @@ -413,8 +417,12 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  	udelay(20);
>  
>  	/* Enable the lock-detect module */
> -	if (MESON_PARM_APPLICABLE(&pll->l_detect))
> -		meson_parm_write(clk->map, &pll->l_detect, 0);
> +	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
> +		if (pll->flags & CLK_MESON_PLL_L_DETECT_N)
> +			meson_parm_write(clk->map, &pll->l_detect, 0);
> +		else
> +			meson_parm_write(clk->map, &pll->l_detect, 1);
> +	}
>  
>  	if (meson_clk_pll_wait_lock(hw)) {
>  		/* disable PLL when PLL lock failed. */
> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
> index 949157fb7bf5..83295a24721f 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -29,6 +29,8 @@ struct pll_mult_range {
>  
>  #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
>  #define CLK_MESON_PLL_NOINIT_ENABLED	BIT(1)
> +/* l_detect signal is active-low */
> +#define CLK_MESON_PLL_L_DETECT_N	BIT(2)
>  
>  struct meson_clk_pll_data {
>  	struct parm en;

-- 
Jerome

Re: [PATCH v2 2/5] clk: amlogic: Improve the issue of PLL lock failures

[Jerome Brunet]

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>
>
> Due to factors such as temperature and process variations, the
> internal circuits of the PLL may require a longer time to reach a
> steady state, which can result in occasional lock failures on some
> SoCs under low-temperature conditions.
>
> After enabling the PLL and releasing its reset, a 20 us delay is
> added at each step to provide enough time for the internal PLL
> circuit to stabilize, thus reducing the probability of PLL lock
> failure.
>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/clk-pll.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 629f6af18ea1..f81ebf6cc981 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -368,11 +368,16 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  
>  	/* Enable the pll */
>  	meson_parm_write(clk->map, &pll->en, 1);

New line

> +	/* Wait for Bandgap and LDO to power up and stabilize */
> +	udelay(20);
>  
>  	/* Take the pll out reset */
>  	if (MESON_PARM_APPLICABLE(&pll->rst))
>  		meson_parm_write(clk->map, &pll->rst, 0);
>  
> +	/* Wait for PLL loop stabilization */
> +	udelay(20);
> +
>  	/*
>  	 * Compared with the previous SoCs, self-adaption current module
>  	 * is newly added for A1, keep the new power-on sequence to enable the

-- 
Jerome

Re: [PATCH v2 2/5] clk: amlogic: Improve the issue of PLL lock failures

[Chuan Liu]

Hi Jerome,

On 10/30/2025 4:41 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>
>>
>> Due to factors such as temperature and process variations, the
>> internal circuits of the PLL may require a longer time to reach a
>> steady state, which can result in occasional lock failures on some
>> SoCs under low-temperature conditions.
>>
>> After enabling the PLL and releasing its reset, a 20 us delay is
>> added at each step to provide enough time for the internal PLL
>> circuit to stabilize, thus reducing the probability of PLL lock
>> failure.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/meson/clk-pll.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index 629f6af18ea1..f81ebf6cc981 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -368,11 +368,16 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>
>>        /* Enable the pll */
>>        meson_parm_write(clk->map, &pll->en, 1);
> 
> New line

Ok, fix it in the next version.

> 
>> +     /* Wait for Bandgap and LDO to power up and stabilize */
>> +     udelay(20);
>>
>>        /* Take the pll out reset */
>>        if (MESON_PARM_APPLICABLE(&pll->rst))
>>                meson_parm_write(clk->map, &pll->rst, 0);
>>
>> +     /* Wait for PLL loop stabilization */
>> +     udelay(20);
>> +
>>        /*
>>         * Compared with the previous SoCs, self-adaption current module
>>         * is newly added for A1, keep the new power-on sequence to enable the
> 
> --
> Jerome

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

[Chuan Liu]

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 4/5] clk: amlogic: Optimize PLL enable timing

[Chuan Liu]

Hi Jerome,

On 10/30/2025 4:38 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>
>>
>> l_detect controls the enablement of the PLL lock detection module.
>> It should remain disabled while the internal PLL circuits are
>> reaching a steady state; otherwise, the lock signal may be falsely
>> triggered high.
>>
>> Before enabling the internal power supply of the PLL, l_detect should
>> be disabled. After the PLL’s internal circuits have stabilized,
>> l_detect should be enabled to prevent false lock signal triggers.
> 
> You to reformat this description. It feel that both paragraph are saying
> the same thing.
> 

Ok, drop it.

>>
>> Currently, only A1 supports both l_detect and current_en, so this
>> patch will only affect A1.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/meson/clk-pll.c | 28 +++++++++++++++-------------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index 6c794adb8ccd..c6eebde1f516 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -383,36 +383,38 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>        if (MESON_PARM_APPLICABLE(&pll->rst))
>>                meson_parm_write(clk->map, &pll->rst, 1);
>>
>> +     /* Disable the PLL lock-detect module */
>> +     if (MESON_PARM_APPLICABLE(&pll->l_detect))
>> +             meson_parm_write(clk->map, &pll->l_detect, 1);
>> +
>>        /* Enable the pll */
>>        meson_parm_write(clk->map, &pll->en, 1);
>>        /* Wait for Bandgap and LDO to power up and stabilize */
>>        udelay(20);
>>
>> -     /* Take the pll out reset */
>> -     if (MESON_PARM_APPLICABLE(&pll->rst))
>> -             meson_parm_write(clk->map, &pll->rst, 0);
> 
> Why is the reset moving around ? nothing is said in the description about
> that
> 

The function of current_en in the PLL is similar to en. It's more
reliable to assert the reset signal after current_en is set high.

I'll update the commit description accordingly in the next version.

>> -
>> -     /* Wait for PLL loop stabilization */
>> -     udelay(20);
>> -
>>        /*
>>         * Compared with the previous SoCs, self-adaption current module
>>         * is newly added for A1, keep the new power-on sequence to enable the
>>         * PLL. The sequence is:
>> -      * 1. enable the pll, delay for 10us
>> +      * 1. enable the pll, delay for 20us
>>         * 2. enable the pll self-adaption current module, delay for 40us
>>         * 3. enable the lock detect module
>>         */
>>        if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>> -             udelay(10);
>>                meson_parm_write(clk->map, &pll->current_en, 1);
>> -             udelay(40);
>> +             udelay(20);
>>        }
>>
>> -     if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
>> -             meson_parm_write(clk->map, &pll->l_detect, 1);
>> +     /* Take the pll out reset */
>> +     if (MESON_PARM_APPLICABLE(&pll->rst))
>> +             meson_parm_write(clk->map, &pll->rst, 0);
>> +
>> +     /* Wait for PLL loop stabilization */
>> +     udelay(20);
>> +
>> +     /* Enable the lock-detect module */
>> +     if (MESON_PARM_APPLICABLE(&pll->l_detect))
>>                meson_parm_write(clk->map, &pll->l_detect, 0);
>> -     }
>>
>>        if (meson_clk_pll_wait_lock(hw)) {
>>                /* disable PLL when PLL lock failed. */
> 
> --
> Jerome

Re: [PATCH v2 5/5] clk: amlogic: Change the active level of l_detect

[Chuan Liu]

Hi Jerome,

On 10/30/2025 4:40 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>
>>
>> l_detect controls the enable/disable of the PLL lock-detect module.
>>
>> The enable signal is normally active-high. This design ensures that
>> the module remains disabled during the power-on process, preventing
>> power fluctuations from affecting its operating state.
>>
>> For A1, the l_detect signal is active-low:
>> 0 -> Enable lock-detect module;
>> 1 -> Disable lock-detect module.
>>
>> Here, a flag CLK_MESON_PLL_L_DETECT_N is added to handle cases like
>> A1, where the signal is active-low.
> 
> Given that A1 is the only ship supporting l_detect, this change is not
> necessary.
> 
> Not now at least.
> 

The SoCs queued for upstreaming later will depend on this patch, so I
included it in this patchset.

If that’s not appropriate, would it be acceptable for me to include
it in the A4 submission instead?

>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/meson/a1-pll.c  |  1 +
>>   drivers/clk/meson/clk-pll.c | 16 ++++++++++++----
>>   drivers/clk/meson/clk-pll.h |  2 ++
>>   3 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
>> index 1f82e9c7c14e..bfe559c71402 100644
>> --- a/drivers/clk/meson/a1-pll.c
>> +++ b/drivers/clk/meson/a1-pll.c
>> @@ -137,6 +137,7 @@ static struct clk_regmap a1_hifi_pll = {
>>                .range = &a1_hifi_pll_range,
>>                .init_regs = a1_hifi_pll_init_regs,
>>                .init_count = ARRAY_SIZE(a1_hifi_pll_init_regs),
>> +             .flags = CLK_MESON_PLL_L_DETECT_N
>>        },
>>        .hw.init = &(struct clk_init_data){
>>                .name = "hifi_pll",
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index c6eebde1f516..d729e933aa1c 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -384,8 +384,12 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>                meson_parm_write(clk->map, &pll->rst, 1);
>>
>>        /* Disable the PLL lock-detect module */
>> -     if (MESON_PARM_APPLICABLE(&pll->l_detect))
>> -             meson_parm_write(clk->map, &pll->l_detect, 1);
>> +     if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
>> +             if (pll->flags & CLK_MESON_PLL_L_DETECT_N)
>> +                     meson_parm_write(clk->map, &pll->l_detect, 1);
>> +             else
>> +                     meson_parm_write(clk->map, &pll->l_detect, 0);
>> +     }
>>
>>        /* Enable the pll */
>>        meson_parm_write(clk->map, &pll->en, 1);
>> @@ -413,8 +417,12 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>        udelay(20);
>>
>>        /* Enable the lock-detect module */
>> -     if (MESON_PARM_APPLICABLE(&pll->l_detect))
>> -             meson_parm_write(clk->map, &pll->l_detect, 0);
>> +     if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
>> +             if (pll->flags & CLK_MESON_PLL_L_DETECT_N)
>> +                     meson_parm_write(clk->map, &pll->l_detect, 0);
>> +             else
>> +                     meson_parm_write(clk->map, &pll->l_detect, 1);
>> +     }
>>
>>        if (meson_clk_pll_wait_lock(hw)) {
>>                /* disable PLL when PLL lock failed. */
>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>> index 949157fb7bf5..83295a24721f 100644
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -29,6 +29,8 @@ struct pll_mult_range {
>>
>>   #define CLK_MESON_PLL_ROUND_CLOSEST  BIT(0)
>>   #define CLK_MESON_PLL_NOINIT_ENABLED BIT(1)
>> +/* l_detect signal is active-low */
>> +#define CLK_MESON_PLL_L_DETECT_N     BIT(2)
>>
>>   struct meson_clk_pll_data {
>>        struct parm en;
> 
> --
> Jerome

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

[Chuan Liu]

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
>