All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
lock status is indicated by register PLL_STS (offset 0x80 inside AP
clock controller). We should poll the register to ensure the PLL
actually locks after enabling it.
Furthermore, a 30us delay is added after enabling the PLL, after which
the PLL could be considered stable as stated by vendor clock code.
Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
Signed-off-by: Yao Zi <ziyao@disroot.org>
---
drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index 71ad03a998e8..d870f0c665f8 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -8,11 +8,14 @@
#include <dt-bindings/clock/thead,th1520-clk-ap.h>
#include <linux/bitfield.h>
#include <linux/clk-provider.h>
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
+#define TH1520_PLL_STS 0x80
+
#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
#define TH1520_PLL_FBDIV GENMASK(19, 8)
@@ -23,6 +26,13 @@
#define TH1520_PLL_FRAC GENMASK(23, 0)
#define TH1520_PLL_FRAC_BITS 24
+/*
+ * All PLLs in TH1520 take 21250ns at maximum to lock, let's take its double
+ * for safety.
+ */
+#define TH1520_PLL_LOCK_TIMEOUT_US 44
+#define TH1520_PLL_STABLE_DELAY_US 30
+
struct ccu_internal {
u8 shift;
u8 width;
@@ -64,6 +74,7 @@ struct ccu_div {
struct ccu_pll {
struct ccu_common common;
+ u32 lock_sts_mask;
};
#define TH_CCU_ARG(_shift, _width) \
@@ -299,9 +310,21 @@ static void ccu_pll_disable(struct clk_hw *hw)
static int ccu_pll_enable(struct clk_hw *hw)
{
struct ccu_pll *pll = hw_to_ccu_pll(hw);
+ u32 reg;
+ int ret;
- return regmap_clear_bits(pll->common.map, pll->common.cfg1,
- TH1520_PLL_VCO_RST);
+ regmap_clear_bits(pll->common.map, pll->common.cfg1,
+ TH1520_PLL_VCO_RST);
+
+ ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
+ reg, reg & pll->lock_sts_mask,
+ 5, TH1520_PLL_LOCK_TIMEOUT_US);
+ if (ret)
+ return ret;
+
+ udelay(TH1520_PLL_STABLE_DELAY_US);
+
+ return 0;
}
static int ccu_pll_is_enabled(struct clk_hw *hw)
@@ -389,6 +412,7 @@ static struct ccu_pll cpu_pll0_clk = {
&clk_pll_ops,
CLK_IS_CRITICAL),
},
+ .lock_sts_mask = BIT(1),
};
static struct ccu_pll cpu_pll1_clk = {
@@ -401,6 +425,7 @@ static struct ccu_pll cpu_pll1_clk = {
&clk_pll_ops,
CLK_IS_CRITICAL),
},
+ .lock_sts_mask = BIT(4),
};
static struct ccu_pll gmac_pll_clk = {
@@ -413,6 +438,7 @@ static struct ccu_pll gmac_pll_clk = {
&clk_pll_ops,
CLK_IS_CRITICAL),
},
+ .lock_sts_mask = BIT(3),
};
static const struct clk_hw *gmac_pll_clk_parent[] = {
@@ -433,6 +459,7 @@ static struct ccu_pll video_pll_clk = {
&clk_pll_ops,
CLK_IS_CRITICAL),
},
+ .lock_sts_mask = BIT(7),
};
static const struct clk_hw *video_pll_clk_parent[] = {
@@ -453,6 +480,7 @@ static struct ccu_pll dpu0_pll_clk = {
&clk_pll_ops,
0),
},
+ .lock_sts_mask = BIT(8),
};
static const struct clk_hw *dpu0_pll_clk_parent[] = {
@@ -469,6 +497,7 @@ static struct ccu_pll dpu1_pll_clk = {
&clk_pll_ops,
0),
},
+ .lock_sts_mask = BIT(9),
};
static const struct clk_hw *dpu1_pll_clk_parent[] = {
@@ -485,6 +514,7 @@ static struct ccu_pll tee_pll_clk = {
&clk_pll_ops,
CLK_IS_CRITICAL),
},
+ .lock_sts_mask = BIT(10),
};
static const struct clk_parent_data c910_i0_parents[] = {
--
2.51.2On Thu, Nov 20, 2025 at 01:14:11PM +0000, Yao Zi wrote:
> All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
> lock status is indicated by register PLL_STS (offset 0x80 inside AP
> clock controller). We should poll the register to ensure the PLL
> actually locks after enabling it.
>
> Furthermore, a 30us delay is added after enabling the PLL, after which
> the PLL could be considered stable as stated by vendor clock code.
>
> Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
> drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
[...]
> +/*
> + * All PLLs in TH1520 take 21250ns at maximum to lock, let's take its double
> + * for safety.
> + */
> +#define TH1520_PLL_LOCK_TIMEOUT_US 44
> +#define TH1520_PLL_STABLE_DELAY_US 30
I'm taking a second look at this and I think it might be best to add a
define for the polling loop delay of 5. It could be helpful when other
people read the code later.
[...]
> + ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
> + reg, reg & pll->lock_sts_mask,
> + 5, TH1520_PLL_LOCK_TIMEOUT_US);
The loop delay is only used here but I think using a #define would make
it more readable.
Other than that:
Reviewed-by: Drew Fustini <fustini@kernel.org>
If no other changes are needed I could fix this up on apply. Let's see
what other comments there may be. It's too late for me to send a 6.19
clk pull request so this will have to target the next merge window. I
can put it into linux-next once 6.19-rc1 is released.
Thanks,
Drew
On Wed, Nov 26, 2025 at 08:52:00AM -0600, Drew Fustini wrote:
> On Thu, Nov 20, 2025 at 01:14:11PM +0000, Yao Zi wrote:
> > All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
> > lock status is indicated by register PLL_STS (offset 0x80 inside AP
> > clock controller). We should poll the register to ensure the PLL
> > actually locks after enabling it.
> >
> > Furthermore, a 30us delay is added after enabling the PLL, after which
> > the PLL could be considered stable as stated by vendor clock code.
> >
> > Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> > drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
> > 1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> [...]
> > +/*
> > + * All PLLs in TH1520 take 21250ns at maximum to lock, let's take its double
> > + * for safety.
> > + */
> > +#define TH1520_PLL_LOCK_TIMEOUT_US 44
> > +#define TH1520_PLL_STABLE_DELAY_US 30
>
> I'm taking a second look at this and I think it might be best to add a
> define for the polling loop delay of 5. It could be helpful when other
> people read the code later.
>
> [...]
> > + ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
> > + reg, reg & pll->lock_sts_mask,
> > + 5, TH1520_PLL_LOCK_TIMEOUT_US);
>
> The loop delay is only used here but I think using a #define would make
> it more readable.
There are TH1520_PLL_LOCK_TIMEOUT_US and TH1520_PLL_STABLE_DELAY_US
defined because they're meaningful constants, either specified by TRM or
implied by vendor code, however the 5us delay is only a randomly-picked
value, as what I've mentioned before.
Anyway, I'm fine with a separate definition. So please go ahead if it
looks better to you.
> Other than that:
> Reviewed-by: Drew Fustini <fustini@kernel.org>
>
> If no other changes are needed I could fix this up on apply. Let's see
> what other comments there may be. It's too late for me to send a 6.19
> clk pull request so this will have to target the next merge window. I
> can put it into linux-next once 6.19-rc1 is released.
Many thanks for it.
> Thanks,
> Drew
Best regards,
Yao Zi
On Thu, Nov 20, 2025 at 01:14:11PM +0000, Yao Zi wrote:
> All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
> lock status is indicated by register PLL_STS (offset 0x80 inside AP
> clock controller). We should poll the register to ensure the PLL
> actually locks after enabling it.
>
> Furthermore, a 30us delay is added after enabling the PLL, after which
> the PLL could be considered stable as stated by vendor clock code.
>
> Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
> drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
Thanks for working on this patch series.
[...]
> @@ -299,9 +310,21 @@ static void ccu_pll_disable(struct clk_hw *hw)
> static int ccu_pll_enable(struct clk_hw *hw)
> {
> struct ccu_pll *pll = hw_to_ccu_pll(hw);
> + u32 reg;
> + int ret;
>
> - return regmap_clear_bits(pll->common.map, pll->common.cfg1,
> - TH1520_PLL_VCO_RST);
> + regmap_clear_bits(pll->common.map, pll->common.cfg1,
> + TH1520_PLL_VCO_RST);
> +
> + ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
> + reg, reg & pll->lock_sts_mask,
> + 5, TH1520_PLL_LOCK_TIMEOUT_US);
Is there a reason for the specific value of 5 uS polling delay?
> + if (ret)
> + return ret;
> +
> + udelay(TH1520_PLL_STABLE_DELAY_US);
Is it the case that the 30 uS delay after the lock bit is set is just so
that it has the same behavior as the vendor's code? Or did you notice
stability problems without this?
Thanks,
Drew
On Mon, Nov 24, 2025 at 02:08:00PM -0800, Drew Fustini wrote:
> On Thu, Nov 20, 2025 at 01:14:11PM +0000, Yao Zi wrote:
> > All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
> > lock status is indicated by register PLL_STS (offset 0x80 inside AP
> > clock controller). We should poll the register to ensure the PLL
> > actually locks after enabling it.
> >
> > Furthermore, a 30us delay is added after enabling the PLL, after which
> > the PLL could be considered stable as stated by vendor clock code.
> >
> > Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> > drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
> > 1 file changed, 32 insertions(+), 2 deletions(-)
>
> Thanks for working on this patch series.
>
> [...]
> > @@ -299,9 +310,21 @@ static void ccu_pll_disable(struct clk_hw *hw)
> > static int ccu_pll_enable(struct clk_hw *hw)
> > {
> > struct ccu_pll *pll = hw_to_ccu_pll(hw);
> > + u32 reg;
> > + int ret;
> >
> > - return regmap_clear_bits(pll->common.map, pll->common.cfg1,
> > - TH1520_PLL_VCO_RST);
> > + regmap_clear_bits(pll->common.map, pll->common.cfg1,
> > + TH1520_PLL_VCO_RST);
> > +
> > + ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
> > + reg, reg & pll->lock_sts_mask,
> > + 5, TH1520_PLL_LOCK_TIMEOUT_US);
>
> Is there a reason for the specific value of 5 uS polling delay?
No, it was picked randomly. A smaller value would reduce latency of
PLL enabling, and I could tune it more carefully by some testing. But
it's hard to predict how much improvement it will bring.
> > + if (ret)
> > + return ret;
> > +
> > + udelay(TH1520_PLL_STABLE_DELAY_US);
>
> Is it the case that the 30 uS delay after the lock bit is set is just so
> that it has the same behavior as the vendor's code? Or did you notice
> stability problems without this?
This aligns with the vendor code, and I haven't yet observed stability
issues without the delay. But I think it's more safe to keep the
behavior similar since it's hard to test all working conditions.
> Thanks,
> Drew
Best regards,
Yao Zi
On Tue, Nov 25, 2025 at 03:19:30AM +0000, Yao Zi wrote:
> On Mon, Nov 24, 2025 at 02:08:00PM -0800, Drew Fustini wrote:
> > On Thu, Nov 20, 2025 at 01:14:11PM +0000, Yao Zi wrote:
> > > All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
> > > lock status is indicated by register PLL_STS (offset 0x80 inside AP
> > > clock controller). We should poll the register to ensure the PLL
> > > actually locks after enabling it.
> > >
> > > Furthermore, a 30us delay is added after enabling the PLL, after which
> > > the PLL could be considered stable as stated by vendor clock code.
> > >
> > > Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > > drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
> > > 1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > Thanks for working on this patch series.
> >
> > [...]
> > > @@ -299,9 +310,21 @@ static void ccu_pll_disable(struct clk_hw *hw)
> > > static int ccu_pll_enable(struct clk_hw *hw)
> > > {
> > > struct ccu_pll *pll = hw_to_ccu_pll(hw);
> > > + u32 reg;
> > > + int ret;
> > >
> > > - return regmap_clear_bits(pll->common.map, pll->common.cfg1,
> > > - TH1520_PLL_VCO_RST);
> > > + regmap_clear_bits(pll->common.map, pll->common.cfg1,
> > > + TH1520_PLL_VCO_RST);
> > > +
> > > + ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
> > > + reg, reg & pll->lock_sts_mask,
> > > + 5, TH1520_PLL_LOCK_TIMEOUT_US);
> >
> > Is there a reason for the specific value of 5 uS polling delay?
>
> No, it was picked randomly. A smaller value would reduce latency of
> PLL enabling, and I could tune it more carefully by some testing. But
> it's hard to predict how much improvement it will bring.
Okay, I was just curious. I think it is okay to stick with that current
value if it is working correctly.
> > > + if (ret)
> > > + return ret;
> > > +
> > > + udelay(TH1520_PLL_STABLE_DELAY_US);
> >
> > Is it the case that the 30 uS delay after the lock bit is set is just so
> > that it has the same behavior as the vendor's code? Or did you notice
> > stability problems without this?
>
> This aligns with the vendor code, and I haven't yet observed stability
> issues without the delay. But I think it's more safe to keep the
> behavior similar since it's hard to test all working conditions.
Okay, that seems reasonable to play it safe.
Thanks,
Drew
© 2016 - 2025 Red Hat, Inc.