clk: tegra: Don't enable already enabled PLLs

PATCH In review Dmitry Osipenko 4 participants 11 messages Aug 30, 2018 – Dec 10, 2018

Subsystems (3)

Common clk Tegra architecture support Tegra clock driver

lore

Share & tools

lore

Dmitry Osipenko

author

2018-08-30 18:42 UTC #0 lore raw

PATCH v2 1/2 clk: tegra: Don't enable already enabled PLLs

Initially Common Clock Framework isn't aware of the clock-enable status, this results in enabling of clocks that were enabled by bootloader. This is not a big deal for a regular clock-gates, but for PLL's it may have some unpleasant consequences. Thus re-enabling PLLX (the main CPU parent clock) may result in extra long period of PLL re-locking.

Signed-off-by Dmitry Osipenko <[email protected]>

---

Changelog:

v2: No change.

 drivers/clk/tegra/clk-pll.c | 50 +++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 13 deletions(-)

drivers/clk/tegra/clk-pll.c +37 ~~-13~~

index 830d1c87fa7c..ddb431247f08 100644
@@ -444,6 +444,9 @@ static int clk_pll_enable(struct clk_hw *hw)
 	unsigned long flags = 0;
 	int ret;
 
+	if (clk_pll_is_enabled(hw))
+		return 0;
+
 	if (pll->lock)
 		spin_lock_irqsave(pll->lock, flags);

@@ -939,11 +942,16 @@ static int clk_plle_training(struct tegra_clk_pll *pll)
 static int clk_plle_enable(struct clk_hw *hw)
 {
 	struct tegra_clk_pll *pll = to_clk_pll(hw);
-	unsigned long input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
 	struct tegra_clk_pll_freq_table sel;
+	unsigned long input_rate;
 	u32 val;
 	int err;
 
+	if (clk_pll_is_enabled(hw))
+		return 0;
+
+	input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+
 	if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
 		return -EINVAL;

@@ -1354,6 +1362,9 @@ static int clk_pllc_enable(struct clk_hw *hw)
 	int ret;
 	unsigned long flags = 0;
 
+	if (clk_pll_is_enabled(hw))
+		return 0;
+
 	if (pll->lock)
 		spin_lock_irqsave(pll->lock, flags);

@@ -1566,7 +1577,12 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
 	u32 val;
 	int ret;
 	unsigned long flags = 0;
-	unsigned long input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+	unsigned long input_rate;
+
+	if (clk_pll_is_enabled(hw))
+		return 0;
+
+	input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
 
 	if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
 		return -EINVAL;

@@ -1703,6 +1719,9 @@ static int clk_pllu_tegra114_enable(struct clk_hw *hw)
 		return -EINVAL;
 	}
 
+	if (clk_pll_is_enabled(hw))
+		return 0;
+
 	input_rate = clk_hw_get_rate(__clk_get_hw(osc));
 
 	if (pll->lock)

@@ -2378,6 +2397,16 @@ struct clk *tegra_clk_register_pllre_tegra210(const char *name,
 	return clk;
 }
 
+static int clk_plle_tegra210_is_enabled(struct clk_hw *hw)
+{
+	struct tegra_clk_pll *pll = to_clk_pll(hw);
+	u32 val;
+
+	val = pll_readl_base(pll);
+
+	return val & PLLE_BASE_ENABLE ? 1 : 0;
+}
+
 static int clk_plle_tegra210_enable(struct clk_hw *hw)
 {
 	struct tegra_clk_pll *pll = to_clk_pll(hw);

@@ -2385,7 +2414,12 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw)
 	u32 val;
 	int ret = 0;
 	unsigned long flags = 0;
-	unsigned long input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+	unsigned long input_rate;
+
+	if (clk_plle_tegra210_is_enabled(hw))
+		return 0;
+
+	input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
 
 	if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
 		return -EINVAL;

@@ -2496,16 +2530,6 @@ static void clk_plle_tegra210_disable(struct clk_hw *hw)
 		spin_unlock_irqrestore(pll->lock, flags);
 }
 
-static int clk_plle_tegra210_is_enabled(struct clk_hw *hw)
-{
-	struct tegra_clk_pll *pll = to_clk_pll(hw);
-	u32 val;
-
-	val = pll_readl_base(pll);
-
-	return val & PLLE_BASE_ENABLE ? 1 : 0;
-}
-
 static const struct clk_ops tegra_clk_plle_tegra210_ops = {
 	.is_enabled =  clk_plle_tegra210_is_enabled,
 	.enable = clk_plle_tegra210_enable,

Signature · 1 line

2.18.0

Dmitry Osipenko

author

2018-08-30 18:42 UTC #1 lore raw

PATCH v2 2/2 clk: tegra20: Enable lock-status polling for PLLs

Currently all PLL's on Tegra20 use a hardcoded delay despite of having a lock-status bit. The lock-status polling was disabled ~7 years ago because PLLE was failing to lock and was a suspicion that other PLLs might be faulty too. Other PLLs are okay, hence enable the lock-status polling for them. This reduces delay of any operation that require PLL to lock.

Signed-off-by Dmitry Osipenko <[email protected]>

---

Changelog:

v2: Don't enable polling for PLLE as it known to not being able to lock.

 drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/clk/tegra/clk-tegra20.c +13 -7

index cc857d4d4a86..cfde3745a0db 100644
@@ -298,7 +298,8 @@ static struct tegra_clk_pll_params pll_c_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_c_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_m_params = {

@@ -314,7 +315,8 @@ static struct tegra_clk_pll_params pll_m_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_m_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_p_params = {

@@ -331,7 +333,7 @@ static struct tegra_clk_pll_params pll_p_params = {
 	.lock_delay = 300,
 	.freq_table = pll_p_freq_table,
 	.flags = TEGRA_PLL_FIXED | TEGRA_PLL_HAS_CPCON |
-		 TEGRA_PLL_HAS_LOCK_ENABLE,
+		 TEGRA_PLL_HAS_LOCK_ENABLE | TEGRA_PLL_USE_LOCK,
 	.fixed_rate =  216000000,
 };

@@ -348,7 +350,8 @@ static struct tegra_clk_pll_params pll_a_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_a_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_d_params = {

@@ -364,7 +367,8 @@ static struct tegra_clk_pll_params pll_d_params = {
 	.lock_enable_bit_idx = PLLDU_MISC_LOCK_ENABLE,
 	.lock_delay = 1000,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static const struct pdiv_map pllu_p[] = {

@@ -387,7 +391,8 @@ static struct tegra_clk_pll_params pll_u_params = {
 	.lock_delay = 1000,
 	.pdiv_tohw = pllu_p,
 	.freq_table = pll_u_freq_table,
-	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_x_params = {

@@ -403,7 +408,8 @@ static struct tegra_clk_pll_params pll_x_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_x_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_e_params = {

Signature · 1 line

2.18.0

Peter De Schrijver

2018-08-31 09:29 UTC #2 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

Quoted 7 lines · Dmitry Osipenko

Currently all PLL's on Tegra20 use a hardcoded delay despite of having a lock-status bit. The lock-status polling was disabled ~7 years ago because PLLE was failing to lock and was a suspicion that other PLLs might be faulty too. Other PLLs are okay, hence enable the lock-status polling for them. This reduces delay of any operation that require PLL to lock.

Quoted 7 lines

Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:
v2: Don't enable polling for PLLE as it known to not being able to lock.

This isn't correct. The lock bit of PLLE can declare lock too early, but the PLL itself does lock.

Quoted 79 lines

 drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/clk/tegra/clk-tegra20.c +13 -7

index cc857d4d4a86..cfde3745a0db 100644
@@ -298,7 +298,8 @@ static struct tegra_clk_pll_params pll_c_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_c_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_m_params = {

@@ -314,7 +315,8 @@ static struct tegra_clk_pll_params pll_m_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_m_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_p_params = {

@@ -331,7 +333,7 @@ static struct tegra_clk_pll_params pll_p_params = {
 	.lock_delay = 300,
 	.freq_table = pll_p_freq_table,
 	.flags = TEGRA_PLL_FIXED | TEGRA_PLL_HAS_CPCON |
-		 TEGRA_PLL_HAS_LOCK_ENABLE,
+		 TEGRA_PLL_HAS_LOCK_ENABLE | TEGRA_PLL_USE_LOCK,
 	.fixed_rate =  216000000,
 };

@@ -348,7 +350,8 @@ static struct tegra_clk_pll_params pll_a_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_a_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_d_params = {

@@ -364,7 +367,8 @@ static struct tegra_clk_pll_params pll_d_params = {
 	.lock_enable_bit_idx = PLLDU_MISC_LOCK_ENABLE,
 	.lock_delay = 1000,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static const struct pdiv_map pllu_p[] = {

@@ -387,7 +391,8 @@ static struct tegra_clk_pll_params pll_u_params = {
 	.lock_delay = 1000,
 	.pdiv_tohw = pllu_p,
 	.freq_table = pll_u_freq_table,
-	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_x_params = {

@@ -403,7 +408,8 @@ static struct tegra_clk_pll_params pll_x_params = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_x_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_e_params = {

Signature · 1 line

2.18.0

Dmitry Osipenko

author

2018-08-31 09:45 UTC #3 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

Quoted 11 lines · Peter De Schrijver

Quoted 10 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having a lock-status bit. The lock-status polling was disabled ~7 years ago because PLLE was failing to lock and was a suspicion that other PLLs might be faulty too. Other PLLs are okay, hence enable the lock-status polling for them. This reduces delay of any operation that require PLL to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---

Quoted 7 lines

Quoted 4 lines
Changelog:
v2: Don't enable polling for PLLE as it known to not being able to lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the PLL itself does lock.

Indeed, it locks but can't be polled for the lock-status as it doesn't have the lock-status bit.

Do you want me to adjust the commit description or it is fine as is?

It is also a bit odd that PLLE has "lock_delay = 0", is it correct?

Peter De Schrijver

2018-09-03 08:01 UTC #4 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

Quoted 17 lines · Dmitry Osipenko

Quoted 16 lines · Peter De Schrijver
Quoted 14 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having a lock-status bit. The lock-status polling was disabled ~7 years ago because PLLE was failing to lock and was a suspicion that other PLLs might be faulty too. Other PLLs are okay, hence enable the lock-status polling for them. This reduces delay of any operation that require PLL to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:
v2: Don't enable polling for PLLE as it known to not being able to lock.

Quoted 8 lines

Quoted 2 lines
This isn't correct. The lock bit of PLLE can declare lock too early, but the PLL itself does lock.
Indeed, it locks but can't be polled for the lock-status as it doesn't have the lock-status bit.
Do you want me to adjust the commit description or it is fine as is?

I think it's better to adjust it.

Quoted 1 line

It is also a bit odd that PLLE has "lock_delay = 0", is it correct?

That seems odd yes..

Peter.

Dmitry Osipenko

author

2018-09-04 09:06 UTC #5 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

Quoted 21 lines · On Monday 03 September 2018 11:01:11 Peter De Schrijver wrote:

Quoted 20 lines · Dmitry Osipenko
Quoted 18 lines · Peter De Schrijver
Quoted 7 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having a lock-status bit. The lock-status polling was disabled ~7 years ago because PLLE was failing to lock and was a suspicion that other PLLs might be faulty too. Other PLLs are okay, hence enable the lock-status polling for them. This reduces delay of any operation that require PLL to lock.
Quoted 7 lines
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:
v2: Don't enable polling for PLLE as it known to not being able to
lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the PLL itself does lock.

Quoted 6 lines

Quoted 4 lines
Indeed, it locks but can't be polled for the lock-status as it doesn't have the lock-status bit.
Do you want me to adjust the commit description or it is fine as is?
I think it's better to adjust it.

Okay. I expect to get a review from you for the other clock (and related) patches too and will send the new version once all the current patches will be reviewed. Please take a look at them once you'll have some free time, thanks.

Quoted 3 lines

Quoted 1 line
It is also a bit odd that PLLE has "lock_delay = 0", is it correct?
That seems odd yes..

Dmitry Osipenko

author

2018-10-17 11:52 UTC #6 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

In reply to Dmitry Osipenko

Quoted 14 lines · Dmitry Osipenko

Quoted 13 lines · Peter De Schrijver
Quoted 12 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having a lock-status bit. The lock-status polling was disabled ~7 years ago because PLLE was failing to lock and was a suspicion that other PLLs might be faulty too. Other PLLs are okay, hence enable the lock-status polling for them. This reduces delay of any operation that require PLL to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
v2: Don't enable polling for PLLE as it known to not being able to lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the PLL itself does lock.
Indeed, it locks but can't be polled for the lock-status as it doesn't have the lock-status bit.

Actually it has lock-status bit. Not sure how I managed to miss it before.

Marcel Ziswiler

2018-09-06 12:14 UTC #7 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

In reply to Peter De Schrijver

Quoted 18 lines · Peter De Schrijver

Quoted 17 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having
a lock-status bit. The lock-status polling was disabled ~7 years ago
because PLLE was failing to lock and was a suspicion that other PLLs
might be faulty too. Other PLLs are okay, hence enable the lock- status
polling for them. This reduces delay of any operation that require PLL
to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:

Quoted 7 lines

Quoted 3 lines
v2: Don't enable polling for PLLE as it known to not being
able to lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.

Is there an errata documenting this? As I could not really find any mentioning of this anywhere at least up to the v11 from Dec 21, 2010 I still have access to.

BTW: It looks like also PLLA may not always lock properly with those changes. Is there anything known about that as well? Here is what I get on various Colibri T20 modules (while random other ones seem to work fine):

[    0.232591] clk_pll_wait_for_lock: Timed out waiting for pll pll_a
lock
[    0.232614] tegra_init_from_table: Failed to enable pll_a
[    0.232627] ------------[ cut here ]------------
[    0.232655] WARNING: CPU: 0 PID: 1 at
/run/media/zim/Build/Sources/linux-next.git/drivers/clk/tegra/clk.c:285 
tegra_init_from_table+0x168/0x174
[    0.232676] Modules linked in:
[    0.232696] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc2-
next-20180903-00214-g5618a0514cf1-dirty #183
[    0.232714] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    0.232753] [<c0113644>] (unwind_backtrace) from [<c010dc0c>]
(show_stack+0x10/0x14)
[    0.232783] [<c010dc0c>] (show_stack) from [<c0aa66f4>]
(dump_stack+0x8c/0xa0)
[    0.232808] [<c0aa66f4>] (dump_stack) from [<c0125428>]
(__warn+0xe0/0xf8)
[    0.232828] [<c0125428>] (__warn) from [<c0125558>]
(warn_slowpath_null+0x40/0x48)
[    0.232849] [<c0125558>] (warn_slowpath_null) from [<c0f2f284>]
(tegra_init_from_table+0x168/0x174)
[    0.232874] [<c0f2f284>] (tegra_init_from_table) from [<c0f2f03c>]
(tegra_clocks_apply_init_table+0x1c/0x2c)
[    0.232901] [<c0f2f03c>] (tegra_clocks_apply_init_table) from
[<c0102fa0>] (do_one_initcall+0x54/0x278)
[    0.232926] [<c0102fa0>] (do_one_initcall) from [<c0f01128>]
(kernel_init_freeable+0x2c0/0x354)
[    0.232949] [<c0f01128>] (kernel_init_freeable) from [<c0abb1f0>]
(kernel_init+0x8/0x10c)
[    0.232970] [<c0abb1f0>] (kernel_init) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    0.232987] Exception stack(0xc4c8ffb0 to 0xc4c8fff8)
[    0.233001] ffa0:                                     00000000
00000000 00000000 00000000
[    0.233021] ffc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    0.233040] ffe0: 00000000 00000000 00000000 00000000 00000013
00000000
[    0.233059] ---[ end trace 3f40fa49530610b9 ]---

Quoted 88 lines

 drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/clk/tegra/clk-tegra20.c +2 -1

index cc857d4d4a86..cfde3745a0db 100644
@@ -298,7 +298,8 @@ static struct tegra_clk_pll_params pll_c_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_c_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_m_params = {

drivers/clk/tegra/clk-tegra20.c +2 -1

@@ -314,7 +315,8 @@ static struct tegra_clk_pll_params pll_m_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_m_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_p_params = {

drivers/clk/tegra/clk-tegra20.c +3 -2

@@ -331,7 +333,7 @@ static struct tegra_clk_pll_params pll_p_params
 = {
 	.lock_delay = 300,
 	.freq_table = pll_p_freq_table,
 	.flags = TEGRA_PLL_FIXED | TEGRA_PLL_HAS_CPCON |
-		 TEGRA_PLL_HAS_LOCK_ENABLE,
+		 TEGRA_PLL_HAS_LOCK_ENABLE | TEGRA_PLL_USE_LOCK,
 	.fixed_rate =  216000000,
 };

@@ -348,7 +350,8 @@ static struct tegra_clk_pll_params pll_a_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_a_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_d_params = {

drivers/clk/tegra/clk-tegra20.c +2 -1

@@ -364,7 +367,8 @@ static struct tegra_clk_pll_params pll_d_params
 = {
 	.lock_enable_bit_idx = PLLDU_MISC_LOCK_ENABLE,
 	.lock_delay = 1000,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static const struct pdiv_map pllu_p[] = {

drivers/clk/tegra/clk-tegra20.c +1 -1

@@ -387,7 +391,8 @@ static struct tegra_clk_pll_params pll_u_params
 = {
 	.lock_delay = 1000,
 	.pdiv_tohw = pllu_p,
 	.freq_table = pll_u_freq_table,
-	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON |
 TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON |

TEGRA_PLL_HAS_LOCK_ENABLE |

drivers/clk/tegra/clk-tegra20.c +3 -1

+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_x_params = {

@@ -403,7 +408,8 @@ static struct tegra_clk_pll_params pll_x_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_x_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_e_params = {

Signature · 1 line

2.18.0

Marcel Ziswiler

2018-10-17 10:59 UTC #8 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

In reply to Peter De Schrijver

Quoted 18 lines · Peter De Schrijver

Quoted 17 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having
a lock-status bit. The lock-status polling was disabled ~7 years ago
because PLLE was failing to lock and was a suspicion that other PLLs
might be faulty too. Other PLLs are okay, hence enable the lock- status
polling for them. This reduces delay of any operation that require PLL
to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:

Quoted 7 lines

Quoted 3 lines
v2: Don't enable polling for PLLE as it known to not being
able to lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.

Is there an errata documenting this? As I could not really find any mentioning of this anywhere at least up to the v11 from Dec 21, 2010 I still have access to.

[    0.232591] clk_pll_wait_for_lock: Timed out waiting for pll pll_a
lock
[    0.232614] tegra_init_from_table: Failed to enable pll_a
[    0.232627] ------------[ cut here ]------------
[    0.232655] WARNING: CPU: 0 PID: 1 at
/run/media/zim/Build/Sources/linux-
next.git/drivers/clk/tegra/clk.c:285 
tegra_init_from_table+0x168/0x174
[    0.232676] Modules linked in:
[    0.232696] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc2-
next-20180903-00214-g5618a0514cf1-dirty #183
[    0.232714] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    0.232753] [<c0113644>] (unwind_backtrace) from [<c010dc0c>]
(show_stack+0x10/0x14)
[    0.232783] [<c010dc0c>] (show_stack) from [<c0aa66f4>]
(dump_stack+0x8c/0xa0)
[    0.232808] [<c0aa66f4>] (dump_stack) from [<c0125428>]
(__warn+0xe0/0xf8)
[    0.232828] [<c0125428>] (__warn) from [<c0125558>]
(warn_slowpath_null+0x40/0x48)
[    0.232849] [<c0125558>] (warn_slowpath_null) from [<c0f2f284>]
(tegra_init_from_table+0x168/0x174)
[    0.232874] [<c0f2f284>] (tegra_init_from_table) from [<c0f2f03c>]
(tegra_clocks_apply_init_table+0x1c/0x2c)
[    0.232901] [<c0f2f03c>] (tegra_clocks_apply_init_table) from
[<c0102fa0>] (do_one_initcall+0x54/0x278)
[    0.232926] [<c0102fa0>] (do_one_initcall) from [<c0f01128>]
(kernel_init_freeable+0x2c0/0x354)
[    0.232949] [<c0f01128>] (kernel_init_freeable) from [<c0abb1f0>]
(kernel_init+0x8/0x10c)
[    0.232970] [<c0abb1f0>] (kernel_init) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    0.232987] Exception stack(0xc4c8ffb0 to 0xc4c8fff8)
[    0.233001] ffa0:                                     00000000
00000000 00000000 00000000
[    0.233021] ffc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    0.233040] ffe0: 00000000 00000000 00000000 00000000 00000013
00000000
[    0.233059] ---[ end trace 3f40fa49530610b9 ]---

Quoted 88 lines

 drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/clk/tegra/clk-tegra20.c +2 -1

index cc857d4d4a86..cfde3745a0db 100644
@@ -298,7 +298,8 @@ static struct tegra_clk_pll_params pll_c_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_c_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_m_params = {

drivers/clk/tegra/clk-tegra20.c +2 -1

@@ -314,7 +315,8 @@ static struct tegra_clk_pll_params pll_m_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_m_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_p_params = {

drivers/clk/tegra/clk-tegra20.c +3 -2

@@ -331,7 +333,7 @@ static struct tegra_clk_pll_params pll_p_params
 = {
 	.lock_delay = 300,
 	.freq_table = pll_p_freq_table,
 	.flags = TEGRA_PLL_FIXED | TEGRA_PLL_HAS_CPCON |
-		 TEGRA_PLL_HAS_LOCK_ENABLE,
+		 TEGRA_PLL_HAS_LOCK_ENABLE | TEGRA_PLL_USE_LOCK,
 	.fixed_rate =  216000000,
 };

@@ -348,7 +350,8 @@ static struct tegra_clk_pll_params pll_a_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_a_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_d_params = {

drivers/clk/tegra/clk-tegra20.c +2 -1

@@ -364,7 +367,8 @@ static struct tegra_clk_pll_params pll_d_params
 = {
 	.lock_enable_bit_idx = PLLDU_MISC_LOCK_ENABLE,
 	.lock_delay = 1000,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static const struct pdiv_map pllu_p[] = {

drivers/clk/tegra/clk-tegra20.c +1 -1

@@ -387,7 +391,8 @@ static struct tegra_clk_pll_params pll_u_params
 = {
 	.lock_delay = 1000,
 	.pdiv_tohw = pllu_p,
 	.freq_table = pll_u_freq_table,
-	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON |
 TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON |

TEGRA_PLL_HAS_LOCK_ENABLE |

drivers/clk/tegra/clk-tegra20.c +3 -1

+		 TEGRA_PLL_USE_LOCK,
 };
 
 static struct tegra_clk_pll_params pll_x_params = {

@@ -403,7 +408,8 @@ static struct tegra_clk_pll_params pll_x_params
 = {
 	.lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
 	.lock_delay = 300,
 	.freq_table = pll_x_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
+		 TEGRA_PLL_USE_LOCK,
 };

static struct tegra_clk_pll_params pll_e_params = {

Signature · 1 line

2.18.0

Dmitry Osipenko

author

2018-10-17 11:41 UTC #9 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

Quoted 27 lines · Marcel Ziswiler

Quoted 18 lines · Peter De Schrijver
Quoted 17 lines · Dmitry Osipenko
Currently all PLL's on Tegra20 use a hardcoded delay despite of having
a lock-status bit. The lock-status polling was disabled ~7 years ago
because PLLE was failing to lock and was a suspicion that other PLLs
might be faulty too. Other PLLs are okay, hence enable the lock- status
polling for them. This reduces delay of any operation that require PLL
to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:
Quoted 7 lines
Quoted 3 lines
v2: Don't enable polling for PLLE as it known to not being
able to lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.

Quoted 8 lines

Is there an errata documenting this? As I could not really find any mentioning of this anywhere at least up to the v11 from Dec 21, 2010 I still have access to.
BTW: It looks like also PLLA may not always lock properly with those changes. Is there anything known about that as well? Here is what I get on various Colibri T20 modules (while random other ones seem to work fine):

Could you please try to increase the timeout value?

Dmitry Osipenko

author

2018-12-10 00:55 UTC #10 lore raw

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

В Wed, 17 Oct 2018 14:41:35 +0300
Dmitry Osipenko <[email protected]> пишет:

Quoted 32 lines · Marcel Ziswiler

Quoted 31 lines · Peter De Schrijver
Quoted 18 lines · Dmitry Osipenko
Quoted 17 lines
Currently all PLL's on Tegra20 use a hardcoded delay despite of having
a lock-status bit. The lock-status polling was disabled ~7 years ago
because PLLE was failing to lock and was a suspicion that other PLLs
might be faulty too. Other PLLs are okay, hence enable the lock- status
polling for them. This reduces delay of any operation that require PLL
to lock.
Signed-off-by Dmitry Osipenko <[email protected]>
---
Changelog:
Quoted 7 lines
Quoted 3 lines
v2: Don't enable polling for PLLE as it known to not being
able to lock.
This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.
Is there an errata documenting this? As I could not really find any mentioning of this anywhere at least up to the v11 from Dec 21, 2010 I still have access to.

Quoted 5 lines

Quoted 4 lines
BTW: It looks like also PLLA may not always lock properly with those changes. Is there anything known about that as well? Here is what I get on various Colibri T20 modules (while random other ones seem to work fine):
Could you please try to increase the timeout value?

Hello Marcel,

Do you have any update on the PLL-lock-failure problem?