[PATCH] clk: tegra: Don't enable PLLs during early boot

[PATCH] clk: tegra: Don't enable PLLs during early boot

[Peter De Schrijver]

The PLL code relies on udelay() which is not available when CCF is
initialized. Hence we can't enable any PLL during this phase.

Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

--

Stephen,

Can you confirm this is ok for the audio drivers?

We used to be lucky that this has worked up to now, but I will introduce some
changes to the pll lock check code which cause this to fail due to the
slight differences in timing.

---
 drivers/clk/tegra/clk-tegra20.c |    4 ++--
 drivers/clk/tegra/clk-tegra30.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index b92d48b..7cc76b0 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1230,8 +1230,8 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{usbd, clk_max, 12000000, 0},
 	{usb2, clk_max, 12000000, 0},
 	{usb3, clk_max, 12000000, 0},
-	{pll_a, clk_max, 56448000, 1},
-	{pll_a_out0, clk_max, 11289600, 1},
+	{pll_a, clk_max, 56448000, 0},
+	{pll_a_out0, clk_max, 11289600, 0},
 	{cdev1, clk_max, 0, 1},
 	{blink, clk_max, 32768, 1},
 	{i2s1, pll_a_out0, 11289600, 0},
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index ba6f51b..b705408 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1882,11 +1882,11 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{uartc, pll_p, 408000000, 0},
 	{uartd, pll_p, 408000000, 0},
 	{uarte, pll_p, 408000000, 0},
-	{pll_a, clk_max, 564480000, 1},
-	{pll_a_out0, clk_max, 11289600, 1},
-	{extern1, pll_a_out0, 0, 1},
+	{pll_a, clk_max, 564480000, 0},
+	{pll_a_out0, clk_max, 11289600, 0},
+	{extern1, pll_a_out0, 0, 0},
 	{clk_out_1_mux, extern1, 0, 0},
-	{clk_out_1, clk_max, 0, 1},
+	{clk_out_1, clk_max, 0, 0},
 	{blink, clk_max, 0, 1},
 	{i2s0, pll_a_out0, 11289600, 0},
 	{i2s1, pll_a_out0, 11289600, 0},
-- 
1.7.7.rc0.72.g4b5ea.dirty

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Stephen Warren]

On 03/22/2013 05:54 AM, Peter De Schrijver wrote:
> The PLL code relies on udelay() which is not available when CCF is
> initialized. Hence we can't enable any PLL during this phase.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> --
> 
> Stephen,
> 
> Can you confirm this is ok for the audio drivers?
> 
> We used to be lucky that this has worked up to now, but I will introduce some
> changes to the pll lock check code which cause this to fail due to the
> slight differences in timing.

No, this won't work for the audio drivers; they assume the clock is
enabled when they start.

This assumption was made long ago. I know drivers are supposed to assume
that clocks are disabled when they're probed, but historically that
wasn't always the case, so if the audio drivers assumed that, and then
did clk_enable() as the first thing, they got a warning due to the
enabling an already enabled clock and/or later attempts to disable the
clocks wouldn't actually disable them. Perhaps this has changed now, but
either way, audio driver changes would be needed to support this change.

Perhaps this is due to initializing the Tegra clock driver in the
machine descriptor's init_irq function, rather than in the init_machine
function? Can this be moved?

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Peter De Schrijver]

On Fri, Mar 22, 2013 at 04:48:11PM +0100, Stephen Warren wrote:
> On 03/22/2013 05:54 AM, Peter De Schrijver wrote:
> > The PLL code relies on udelay() which is not available when CCF is
> > initialized. Hence we can't enable any PLL during this phase.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> > 
> > --
> > 
> > Stephen,
> > 
> > Can you confirm this is ok for the audio drivers?
> > 
> > We used to be lucky that this has worked up to now, but I will introduce some
> > changes to the pll lock check code which cause this to fail due to the
> > slight differences in timing.
> 
> No, this won't work for the audio drivers; they assume the clock is
> enabled when they start.
> 
> This assumption was made long ago. I know drivers are supposed to assume
> that clocks are disabled when they're probed, but historically that
> wasn't always the case, so if the audio drivers assumed that, and then
> did clk_enable() as the first thing, they got a warning due to the
> enabling an already enabled clock and/or later attempts to disable the

That should be ok. You can enable a clock more than once.

> clocks wouldn't actually disable them. Perhaps this has changed now, but
> either way, audio driver changes would be needed to support this change.
> 

But indeed, the clock won't be disabled then when the driver does
clk_disable(). The reference count will just be decremented. That's however
not a functional problem, just not optimal from a power consumption point of
view. So we could change the drivers first and keep the clocks disabled at
boottime in a second phase.

> Perhaps this is due to initializing the Tegra clock driver in the
> machine descriptor's init_irq function, rather than in the init_machine
> function? Can this be moved?

Maybe. But we need the clockframework before the timers are initialized...
So I have to check the dependencies.

Cheers,

Peter.

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Prashant Gaikwad]

On Monday 25 March 2013 03:45 PM, Peter De Schrijver wrote:
> On Fri, Mar 22, 2013 at 04:48:11PM +0100, Stephen Warren wrote:
>> On 03/22/2013 05:54 AM, Peter De Schrijver wrote:
>>> The PLL code relies on udelay() which is not available when CCF is
>>> initialized. Hence we can't enable any PLL during this phase.
>>>
>>> Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> --
>>>
>>> Stephen,
>>>
>>> Can you confirm this is ok for the audio drivers?
>>>
>>> We used to be lucky that this has worked up to now, but I will introduce some
>>> changes to the pll lock check code which cause this to fail due to the
>>> slight differences in timing.
>> No, this won't work for the audio drivers; they assume the clock is
>> enabled when they start.
>>
>> This assumption was made long ago. I know drivers are supposed to assume
>> that clocks are disabled when they're probed, but historically that
>> wasn't always the case, so if the audio drivers assumed that, and then
>> did clk_enable() as the first thing, they got a warning due to the
>> enabling an already enabled clock and/or later attempts to disable the
> That should be ok. You can enable a clock more than once.
>
>> clocks wouldn't actually disable them. Perhaps this has changed now, but
>> either way, audio driver changes would be needed to support this change.
>>
> But indeed, the clock won't be disabled then when the driver does
> clk_disable(). The reference count will just be decremented. That's however
> not a functional problem, just not optimal from a power consumption point of
> view. So we could change the drivers first and keep the clocks disabled at
> boottime in a second phase.
>
>> Perhaps this is due to initializing the Tegra clock driver in the
>> machine descriptor's init_irq function, rather than in the init_machine
>> function? Can this be moved?
> Maybe. But we need the clockframework before the timers are initialized...
> So I have to check the dependencies.

I have moved the initialization after slab allocator is initialized and 
before timer initialization.
This is later that it used to happen with our legacy framework or which 
happens in our downstream kernel.

Isn't this problem observed in downstream kernel?

May be we can split the clock initialization and move clock init from 
table to some later stage.

> Cheers,
>
> Peter.

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Stephen Warren]

On 03/25/2013 04:15 AM, Peter De Schrijver wrote:
> On Fri, Mar 22, 2013 at 04:48:11PM +0100, Stephen Warren wrote:
>> On 03/22/2013 05:54 AM, Peter De Schrijver wrote:
>>> The PLL code relies on udelay() which is not available when CCF is
>>> initialized. Hence we can't enable any PLL during this phase.
...
>>> Can you confirm this is ok for the audio drivers?
>>>
>>> We used to be lucky that this has worked up to now, but I will introduce some
>>> changes to the pll lock check code which cause this to fail due to the
>>> slight differences in timing.
>>
>> No, this won't work for the audio drivers; they assume the clock is
>> enabled when they start.
...
>> Perhaps this is due to initializing the Tegra clock driver in the
>> machine descriptor's init_irq function, rather than in the init_machine
>> function? Can this be moved?
> 
> Maybe. But we need the clockframework before the timers are initialized...
> So I have to check the dependencies.

In kernel 3.8, the initialization of the Tegra clock driver and the
processing of the "clock initialization table" were separate. The clock
driver used to be initialized in .init_early(), whereas the clock init
table was processed in .init_machine().

In 3.9, those two things have been conflated into .init_irq().

Is the solution here to separate those two initialization steps again;
leave the base clock driver init in .init_irq() since that's where it
needs to be for unrelated reasons, but move the processing of the clock
init table back into .init_machine() so that there are no restrictions
re: which clocks can actually be initialized?

That would probably require exposing some custom API from the Tegra clk
driver for the Tegra DT board file to call to process the clock init
table, but that should be pretty easy to do, and shouldn't cause any
scalability issues.

Perhaps that extra API can even be standardized later, along with some
way of representing the initial clock tree parenting/... setup in the
DT, and thus making it useful for any SoC.

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Russell King - ARM Linux]

On Fri, Mar 22, 2013 at 09:48:11AM -0600, Stephen Warren wrote:
> This assumption was made long ago. I know drivers are supposed to assume
> that clocks are disabled when they're probed, but historically that
> wasn't always the case, so if the audio drivers assumed that, and then
> did clk_enable() as the first thing, they got a warning due to the
> enabling an already enabled clock and/or later attempts to disable the
> clocks wouldn't actually disable them.

No.  clocks have always been able to be enabled multiple times.  You
should only get a warning if you try and disable it more times than you
enabled it, because that's a violation of the API.

Consider two drivers sharing the same input clock.  If one driver
disables the clock more times than it enables it, then it's treading
on the other drivers need to have the clock enabled.

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Stephen Warren]

On 03/25/2013 05:02 AM, Russell King - ARM Linux wrote:
> On Fri, Mar 22, 2013 at 09:48:11AM -0600, Stephen Warren wrote:
>> This assumption was made long ago. I know drivers are supposed to assume
>> that clocks are disabled when they're probed, but historically that
>> wasn't always the case, so if the audio drivers assumed that, and then
>> did clk_enable() as the first thing, they got a warning due to the
>> enabling an already enabled clock and/or later attempts to disable the
>> clocks wouldn't actually disable them.
> 
> No.  clocks have always been able to be enabled multiple times.  You
> should only get a warning if you try and disable it more times than you
> enabled it, because that's a violation of the API.

That may have been the issue; the clock may have been on at boot, and
hence the driver attempted to disable it without ever enabling it
itself, and this triggered an error/warning.

Equally, it's plausible this issue is long gone; the audio clocks were
set up this way due to issues over 2 years ago when the audio code was
upstreamed. At that time, Tegra's clock code was using very little
standard infra-structure, whereas now it's been cleaned up to use the
common clock framework etc.

[PATCH] clk: tegra: Don't enable PLLs during early boot

[Peter De Schrijver]

The PLL code relies on udelay() which is not available when CCF is
initialized. Hence we can't enable any PLL during this phase.

Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

--

Stephen,

Can you confirm this is ok for the audio drivers?

We used to be lucky that this has worked up to now, but I will introduce some
changes to the pll lock check code which cause this to fail due to the
slight differences in timing.

---
 drivers/clk/tegra/clk-tegra20.c |    4 ++--
 drivers/clk/tegra/clk-tegra30.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index b92d48b..7cc76b0 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1230,8 +1230,8 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{usbd, clk_max, 12000000, 0},
 	{usb2, clk_max, 12000000, 0},
 	{usb3, clk_max, 12000000, 0},
-	{pll_a, clk_max, 56448000, 1},
-	{pll_a_out0, clk_max, 11289600, 1},
+	{pll_a, clk_max, 56448000, 0},
+	{pll_a_out0, clk_max, 11289600, 0},
 	{cdev1, clk_max, 0, 1},
 	{blink, clk_max, 32768, 1},
 	{i2s1, pll_a_out0, 11289600, 0},
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index ba6f51b..b705408 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1882,11 +1882,11 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{uartc, pll_p, 408000000, 0},
 	{uartd, pll_p, 408000000, 0},
 	{uarte, pll_p, 408000000, 0},
-	{pll_a, clk_max, 564480000, 1},
-	{pll_a_out0, clk_max, 11289600, 1},
-	{extern1, pll_a_out0, 0, 1},
+	{pll_a, clk_max, 564480000, 0},
+	{pll_a_out0, clk_max, 11289600, 0},
+	{extern1, pll_a_out0, 0, 0},
 	{clk_out_1_mux, extern1, 0, 0},
-	{clk_out_1, clk_max, 0, 1},
+	{clk_out_1, clk_max, 0, 0},
 	{blink, clk_max, 0, 1},
 	{i2s0, pll_a_out0, 11289600, 0},
 	{i2s1, pll_a_out0, 11289600, 0},
-- 
1.7.7.rc0.72.g4b5ea.dirty

Re: [PATCH] clk: tegra: Don't enable PLLs during early boot

[Peter De Schrijver]

On Fri, Mar 22, 2013 at 01:02:33PM +0100, Peter De Schrijver wrote:
> The PLL code relies on udelay() which is not available when CCF is
> initialized. Hence we can't enable any PLL during this phase.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 

Sorry for accidently reposting this.

Cheers,

Peter.

[PATCH] clk: tegra: Don't enable PLLs during early boot

[Peter De Schrijver]

The PLL code relies on udelay() which is not available when CCF is
initialized. Hence we can't enable any PLL during this phase.

Signed-off-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

--

Stephen,

Can you confirm this is ok for the audio drivers?

We used to be lucky that this has worked up to now, but I will introduce some
changes to the pll lock check code which cause this to fail due to the
slight differences in timing.

---
 drivers/clk/tegra/clk-tegra20.c |    4 ++--
 drivers/clk/tegra/clk-tegra30.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index b92d48b..7cc76b0 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1230,8 +1230,8 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{usbd, clk_max, 12000000, 0},
 	{usb2, clk_max, 12000000, 0},
 	{usb3, clk_max, 12000000, 0},
-	{pll_a, clk_max, 56448000, 1},
-	{pll_a_out0, clk_max, 11289600, 1},
+	{pll_a, clk_max, 56448000, 0},
+	{pll_a_out0, clk_max, 11289600, 0},
 	{cdev1, clk_max, 0, 1},
 	{blink, clk_max, 32768, 1},
 	{i2s1, pll_a_out0, 11289600, 0},
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index ba6f51b..b705408 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1882,11 +1882,11 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{uartc, pll_p, 408000000, 0},
 	{uartd, pll_p, 408000000, 0},
 	{uarte, pll_p, 408000000, 0},
-	{pll_a, clk_max, 564480000, 1},
-	{pll_a_out0, clk_max, 11289600, 1},
-	{extern1, pll_a_out0, 0, 1},
+	{pll_a, clk_max, 564480000, 0},
+	{pll_a_out0, clk_max, 11289600, 0},
+	{extern1, pll_a_out0, 0, 0},
 	{clk_out_1_mux, extern1, 0, 0},
-	{clk_out_1, clk_max, 0, 1},
+	{clk_out_1, clk_max, 0, 0},
 	{blink, clk_max, 0, 1},
 	{i2s0, pll_a_out0, 11289600, 0},
 	{i2s1, pll_a_out0, 11289600, 0},
-- 
1.7.7.rc0.72.g4b5ea.dirty