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(-)
diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 830d1c87fa7c..ddb431247f08 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -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,
--
2.18.0
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(-)
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index cc857d4d4a86..cfde3745a0db 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -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 = {
--
2.18.0
On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> 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.
>
This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.
> drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index cc857d4d4a86..cfde3745a0db 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -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 = {
> --
> 2.18.0
>
On 8/31/18 12:29 PM, Peter De Schrijver wrote:
> On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
>> 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.
>>
>
> 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?
On Fri, Aug 31, 2018 at 12:45:17PM +0300, Dmitry Osipenko wrote:
> On 8/31/18 12:29 PM, Peter De Schrijver wrote:
> > On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> >> 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.
> >>
> >
> > 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.
> It is also a bit odd that PLLE has "lock_delay = 0", is it correct?
That seems odd yes..
Peter.
On Monday 03 September 2018 11:01:11 Peter De Schrijver wrote:
> On Fri, Aug 31, 2018 at 12:45:17PM +0300, Dmitry Osipenko wrote:
> > On 8/31/18 12:29 PM, Peter De Schrijver wrote:
> > > On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> > >> 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.
> > >
> > > 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.
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.
> > It is also a bit odd that PLLE has "lock_delay = 0", is it correct?
>
> That seems odd yes..
:)
On Fri, 2018-08-31 at 12:29 +0300, Peter De Schrijver wrote:
> On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> > 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.
> >
>
> 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 ]---
> > drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/clk-tegra20.c
> > b/drivers/clk/tegra/clk-tegra20.c
> > index cc857d4d4a86..cfde3745a0db 100644
> > --- a/drivers/clk/tegra/clk-tegra20.c
> > +++ b/drivers/clk/tegra/clk-tegra20.c
> > @@ -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 = {
> > --
> > 2.18.0
On Fri, 2018-08-31 at 12:29 +0300, Peter De Schrijver wrote:
> On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> > 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.
> >
>
> 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 ]---
> > drivers/clk/tegra/clk-tegra20.c | 20 +++++++++++++-------
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/clk-tegra20.c
> > b/drivers/clk/tegra/clk-tegra20.c
> > index cc857d4d4a86..cfde3745a0db 100644
> > --- a/drivers/clk/tegra/clk-tegra20.c
> > +++ b/drivers/clk/tegra/clk-tegra20.c
> > @@ -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 = {
> > --
> > 2.18.0
On 10/17/18 1:59 PM, Marcel Ziswiler wrote:
> On Fri, 2018-08-31 at 12:29 +0300, Peter De Schrijver wrote:
>> On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
>>> 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.
>>>
>>
>> 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):
Could you please try to increase the timeout value?
On 8/31/18 12:45 PM, Dmitry Osipenko wrote:
> On 8/31/18 12:29 PM, Peter De Schrijver wrote:
>> On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
>>> 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.
>>>
>>
>> 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.
В Wed, 17 Oct 2018 14:41:35 +0300
Dmitry Osipenko <[email protected]> пишет:
> On 10/17/18 1:59 PM, Marcel Ziswiler wrote:
> > On Fri, 2018-08-31 at 12:29 +0300, Peter De Schrijver wrote:
> >> On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> >>> 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.
> >>>
> >>
> >> 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):
> Could you please try to increase the timeout value?
Hello Marcel,
Do you have any update on the PLL-lock-failure problem?