[PATCH 1/2] clk: tegra: remove USB from clk init table

[PATCH 1/2] clk: tegra: remove USB from clk init table

[Lucas Stach]

The USB clocks are just clock gates, so no need to set a specific clock.
In fact trying to set a specific clock is just a NOP if the requested
clockrate is the same as those of the parent (clk_m) or will trigger a
WARN_ON() if rates don't match up.

As we are not setting a specific rate, nor activating the clocks at
init, there is no point in keeping the the usb entries in the clock init
table.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
---
Trace produced by system with 13MHz clk_m:

tegra_init_from_table: Failed to set rate 12000000 of usbd
------------[ cut here ]------------
WARNING: at drivers/clk/tegra/clk.c:64
tegra_init_from_table+0xc0/0x158()
Modules linked in:
[<c0013a84>] (unwind_backtrace+0x0/0xf8) from
[<c0021a24>](warn_slowpath_common+0x4c/0x64)
[<c0021a24>] (warn_slowpath_common+0x4c/0x64) from [<c0021a58>]
(warn_slowpath_null+0x1c/0x24)
[<c0021a58>] (warn_slowpath_null+0x1c/0x24) from [<c069343c>]
(tegra_init_from_table+0xc0/0x158)
[<c069343c>] (tegra_init_from_table+0xc0/0x158) from [<c0694878>]
(tegra20_clock_init+0x1398/0x13d4)
[<c0694878>] (tegra20_clock_init+0x1398/0x13d4) from [<c0693298>]
(of_clk_init+0x30/0x58)
[<c0693298>] (of_clk_init+0x30/0x58) from [<c0681e5c>]
(tegra_dt_init_irq+0x8/0x1c)
[<c0681e5c>] (tegra_dt_init_irq+0x8/0x1c) from [<c067d334>]
(init_IRQ+0x14/0x1c)
[<c067d334>] (init_IRQ+0x14/0x1c) from [<c067b6b4>]
(start_kernel+0x1a0/0x2f8)
[<c067b6b4>] (start_kernel+0x1a0/0x2f8) from [<0000807c>] (0x807c)
---[ end trace 1b75b31a2719ed1c ]---
---
 drivers/clk/tegra/clk-tegra20.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 5d41569..f08cffc 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1253,9 +1253,6 @@ static __initdata struct tegra_clk_init_table init_table[] = {
 	{cclk, clk_max, 0, 1},
 	{uarta, pll_p, 0, 1},
 	{uartd, pll_p, 0, 1},
-	{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},
 	{cdev1, clk_max, 0, 1},
-- 
1.8.1

[PATCH 2/2] clk: tegra: add ac97 controller clock

[Lucas Stach]

AC97 controller clock is hardwired to pll_a_out0.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
---
 drivers/clk/tegra/clk-tegra20.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index f08cffc..1be8c23 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -894,6 +894,14 @@ static void __init tegra20_periph_clk_init(void)
 	struct clk *clk;
 	int i;
 
+	/* ac97 */
+	clk = tegra_clk_register_periph_gate("ac97", "pll_a_out0",
+				    TEGRA_PERIPH_ON_APB,
+				    clk_base, 0, 3, &periph_l_regs,
+				    periph_clk_enb_refcnt);
+	clk_register_clkdev(clk, NULL, "tegra20-ac97");
+	clks[ac97] = clk;
+
 	/* apbdma */
 	clk = tegra_clk_register_periph_gate("apbdma", "pclk", 0, clk_base,
 				    0, 34, &periph_h_regs,
-- 
1.8.1

Re: [PATCH 1/2] clk: tegra: remove USB from clk init table

[Stephen Warren]

On 01/27/2013 03:17 PM, Lucas Stach wrote:
> The USB clocks are just clock gates, so no need to set a specific clock.
> In fact trying to set a specific clock is just a NOP if the requested
> clockrate is the same as those of the parent (clk_m) or will trigger a
> WARN_ON() if rates don't match up.
> 
> As we are not setting a specific rate, nor activating the clocks at
> init, there is no point in keeping the the usb entries in the clock init
> table.

I'm not convinced here; aren't the USB clocks supposed to be driven by
PLL U?

Prashant, Peter, Venu, can you please comment here.

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Stephen Warren]

On 01/27/2013 03:17 PM, Lucas Stach wrote:
> AC97 controller clock is hardwired to pll_a_out0.

I would like Prashant's/Peter's review here too.

Re: [PATCH 1/2] clk: tegra: remove USB from clk init table

[Lucas Stach]

Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren:
> On 01/27/2013 03:17 PM, Lucas Stach wrote:
> > The USB clocks are just clock gates, so no need to set a specific clock.
> > In fact trying to set a specific clock is just a NOP if the requested
> > clockrate is the same as those of the parent (clk_m) or will trigger a
> > WARN_ON() if rates don't match up.
> > 
> > As we are not setting a specific rate, nor activating the clocks at
> > init, there is no point in keeping the the usb entries in the clock init
> > table.
> 
> I'm not convinced here; aren't the USB clocks supposed to be driven by
> PLL U?
> 
> Prashant, Peter, Venu, can you please comment here.
> 
I was a bit confused at first, too. But what I am removing here is the
clockgate init for the USB controllers. The clocks driven by PLL_U are
the USB PHY clocks, which are a separate set of clocks.

Regards,
Lucas

Re: [PATCH 1/2] clk: tegra: remove USB from clk init table

[Stephen Warren]

On 01/28/2013 11:45 AM, Lucas Stach wrote:
> Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren:
>> On 01/27/2013 03:17 PM, Lucas Stach wrote:
>>> The USB clocks are just clock gates, so no need to set a specific clock.
>>> In fact trying to set a specific clock is just a NOP if the requested
>>> clockrate is the same as those of the parent (clk_m) or will trigger a
>>> WARN_ON() if rates don't match up.
>>>
>>> As we are not setting a specific rate, nor activating the clocks at
>>> init, there is no point in keeping the the usb entries in the clock init
>>> table.
>>
>> I'm not convinced here; aren't the USB clocks supposed to be driven by
>> PLL U?
>>
>> Prashant, Peter, Venu, can you please comment here.
>
> I was a bit confused at first, too. But what I am removing here is the
> clockgate init for the USB controllers. The clocks driven by PLL_U are
> the USB PHY clocks, which are a separate set of clocks.

I don't think these are always separate.

If you look at the USB driver drivers/usb/phy/tegra_usb_phy.c, you'll
see that for UTMI there's a clk_get_sys("utmip-pad"), which per
drivers/clk/tegra/clk-tegra20.c is an alias for clock "usbd" which is
the USB1 controller clock (it's also aliased to "tegra-ehci.0").
However, for ULPI, there's a clk_get_sys(ulpi_config->clk), which is
"cdev2", which is separate from any USB controller clock.

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Stephen Warren]

On 01/27/2013 03:17 PM, Lucas Stach wrote:
> AC97 controller clock is hardwired to pll_a_out0.

One more comment here; don't you need to add this clock into
audio_parents[]?

Re: [PATCH 1/2] clk: tegra: remove USB from clk init table

[Lucas Stach]

Am Montag, den 28.01.2013, 12:13 -0700 schrieb Stephen Warren:
> On 01/28/2013 11:45 AM, Lucas Stach wrote:
> > Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren:
> >> On 01/27/2013 03:17 PM, Lucas Stach wrote:
> >>> The USB clocks are just clock gates, so no need to set a specific clock.
> >>> In fact trying to set a specific clock is just a NOP if the requested
> >>> clockrate is the same as those of the parent (clk_m) or will trigger a
> >>> WARN_ON() if rates don't match up.
> >>>
> >>> As we are not setting a specific rate, nor activating the clocks at
> >>> init, there is no point in keeping the the usb entries in the clock init
> >>> table.
> >>
> >> I'm not convinced here; aren't the USB clocks supposed to be driven by
> >> PLL U?
> >>
> >> Prashant, Peter, Venu, can you please comment here.
> >
> > I was a bit confused at first, too. But what I am removing here is the
> > clockgate init for the USB controllers. The clocks driven by PLL_U are
> > the USB PHY clocks, which are a separate set of clocks.
> 
> I don't think these are always separate.
> 
> If you look at the USB driver drivers/usb/phy/tegra_usb_phy.c, you'll
> see that for UTMI there's a clk_get_sys("utmip-pad"), which per
> drivers/clk/tegra/clk-tegra20.c is an alias for clock "usbd" which is
> the USB1 controller clock (it's also aliased to "tegra-ehci.0").
> However, for ULPI, there's a clk_get_sys(ulpi_config->clk), which is
> "cdev2", which is separate from any USB controller clock.
> 
I'm not sure here. The TRM is not really clear on that one, but if you
look at the schematic diagram of the USB complex PLL_U is really only
used for the PHYs, not the controllers itself. Though I don't know if
the controller gets it's clock from the PHY in the UTMI case (like
ULPI). So I also would like some NVIDIA clock expert to comment on this.

In either case the explicit init isn't needed.

Regards,
Lucas

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Lucas Stach]

Am Montag, den 28.01.2013, 12:15 -0700 schrieb Stephen Warren:
> On 01/27/2013 03:17 PM, Lucas Stach wrote:
> > AC97 controller clock is hardwired to pll_a_out0.
> 
> One more comment here; don't you need to add this clock into
> audio_parents[]?
> 
Yeah it's documented that AC97 can be used as audio_sync_clock, but for
one I haven't tested this and also ac97 clock == pll_a_out0, so you can
just as well use the pll out in that case.

Regards,
Lucas

Re: [PATCH 1/2] clk: tegra: remove USB from clk init table

[Stephen Warren]

On 01/28/2013 12:23 PM, Lucas Stach wrote:
> Am Montag, den 28.01.2013, 12:13 -0700 schrieb Stephen Warren:
>> On 01/28/2013 11:45 AM, Lucas Stach wrote:
>>> Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren:
>>>> On 01/27/2013 03:17 PM, Lucas Stach wrote:
>>>>> The USB clocks are just clock gates, so no need to set a specific clock.
>>>>> In fact trying to set a specific clock is just a NOP if the requested
>>>>> clockrate is the same as those of the parent (clk_m) or will trigger a
>>>>> WARN_ON() if rates don't match up.
>>>>>
>>>>> As we are not setting a specific rate, nor activating the clocks at
>>>>> init, there is no point in keeping the the usb entries in the clock init
>>>>> table.
>>>>
>>>> I'm not convinced here; aren't the USB clocks supposed to be driven by
>>>> PLL U?
>>>>
>>>> Prashant, Peter, Venu, can you please comment here.
>>>
>>> I was a bit confused at first, too. But what I am removing here is the
>>> clockgate init for the USB controllers. The clocks driven by PLL_U are
>>> the USB PHY clocks, which are a separate set of clocks.
>>
>> I don't think these are always separate.
>>
>> If you look at the USB driver drivers/usb/phy/tegra_usb_phy.c, you'll
>> see that for UTMI there's a clk_get_sys("utmip-pad"), which per
>> drivers/clk/tegra/clk-tegra20.c is an alias for clock "usbd" which is
>> the USB1 controller clock (it's also aliased to "tegra-ehci.0").
>> However, for ULPI, there's a clk_get_sys(ulpi_config->clk), which is
>> "cdev2", which is separate from any USB controller clock.
>>
> I'm not sure here. The TRM is not really clear on that one, but if you
> look at the schematic diagram of the USB complex PLL_U is really only
> used for the PHYs, not the controllers itself. Though I don't know if
> the controller gets it's clock from the PHY in the UTMI case (like
> ULPI). So I also would like some NVIDIA clock expert to comment on this.

> In either case the explicit init isn't needed.

My suspicion is that the usb* clocks are actually driven by pll_u_out*
rather than clk_m as the clock driver currently states. In which case,
these initializations are probably intended to have the side-effect of
filtering up and configuring pll_u itself. Perhaps the answer is still
to remove those entries and replace them with a pll_u initialization.

Anyway, hopefully Prashant can shed some light here.

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Stephen Warren]

On 01/28/2013 12:25 PM, Lucas Stach wrote:
> Am Montag, den 28.01.2013, 12:15 -0700 schrieb Stephen Warren:
>> On 01/27/2013 03:17 PM, Lucas Stach wrote:
>>> AC97 controller clock is hardwired to pll_a_out0.
>>
>> One more comment here; don't you need to add this clock into
>> audio_parents[]?
>>
> Yeah it's documented that AC97 can be used as audio_sync_clock, but for
> one I haven't tested this

I expect there are quite a few of the clock driver table entries that
aren't tested. I still think we should add them though, once the clocks
they refer to actually exist.

> and also ac97 clock == pll_a_out0, so you can
> just as well use the pll out in that case.

I suspect that "ac97 clock == pll_a_out0" isn't categorically true; it's
quite possible that the TRM simply doesn't document the mux
register/field for the ac97 clock since the ac97 module is considered
deprecated. The diagram at the end of section 5.2.3 "Audio Clocks"
certainly implies that all of the i2s*, spdifout, and ac97 clocks have
the same structure.

Re: [PATCH 1/2] clk: tegra: remove USB from clk init table

[Peter De Schrijver]

On Mon, Jan 28, 2013 at 07:37:55PM +0100, Stephen Warren wrote:
> On 01/27/2013 03:17 PM, Lucas Stach wrote:
> > The USB clocks are just clock gates, so no need to set a specific clock.
> > In fact trying to set a specific clock is just a NOP if the requested
> > clockrate is the same as those of the parent (clk_m) or will trigger a
> > WARN_ON() if rates don't match up.
> > 
> > As we are not setting a specific rate, nor activating the clocks at
> > init, there is no point in keeping the the usb entries in the clock init
> > table.
> 
> I'm not convinced here; aren't the USB clocks supposed to be driven by
> PLL U?
> 
> Prashant, Peter, Venu, can you please comment here.

According to our downstream code, they are indeed just clockgates with clk_m
as the parent. I couldn't find any confirmation in the TRM though.

Cheers,

Peter.

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Peter De Schrijver]

> 
> > and also ac97 clock == pll_a_out0, so you can
> > just as well use the pll out in that case.
> 
> I suspect that "ac97 clock == pll_a_out0" isn't categorically true; it's
> quite possible that the TRM simply doesn't document the mux
> register/field for the ac97 clock since the ac97 module is considered
> deprecated. The diagram at the end of section 5.2.3 "Audio Clocks"
> certainly implies that all of the i2s*, spdifout, and ac97 clocks have
> the same structure.

It indeed does, but I can't find any reference to a mux control register for
ac97 in any document...

Cheers,

Peter.

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Stephen Warren]

On 01/30/2013 02:35 AM, Peter De Schrijver wrote:
>>
>>> and also ac97 clock == pll_a_out0, so you can
>>> just as well use the pll out in that case.
>>
>> I suspect that "ac97 clock == pll_a_out0" isn't categorically true; it's
>> quite possible that the TRM simply doesn't document the mux
>> register/field for the ac97 clock since the ac97 module is considered
>> deprecated. The diagram at the end of section 5.2.3 "Audio Clocks"
>> certainly implies that all of the i2s*, spdifout, and ac97 clocks have
>> the same structure.
> 
> It indeed does, but I can't find any reference to a mux control register for
> ac97 in any document...

For both these two issues, I'll try and track down the relevant HW
engineers or other sources of documentation internally...

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Lucas Stach]

Am Mittwoch, den 30.01.2013, 11:27 -0700 schrieb Stephen Warren:
> On 01/30/2013 02:35 AM, Peter De Schrijver wrote:
> >>
> >>> and also ac97 clock == pll_a_out0, so you can
> >>> just as well use the pll out in that case.
> >>
> >> I suspect that "ac97 clock == pll_a_out0" isn't categorically true; it's
> >> quite possible that the TRM simply doesn't document the mux
> >> register/field for the ac97 clock since the ac97 module is considered
> >> deprecated. The diagram at the end of section 5.2.3 "Audio Clocks"
> >> certainly implies that all of the i2s*, spdifout, and ac97 clocks have
> >> the same structure.
> > 
> > It indeed does, but I can't find any reference to a mux control register for
> > ac97 in any document...
> 
> For both these two issues, I'll try and track down the relevant HW
> engineers or other sources of documentation internally...
> 
Ping.
I would really like to see those two patches in 3.9, as otherwise
Colibri T20 boots up with kernel warnings and a non-functional audio.

Regards,
Lucas

Re: [PATCH 2/2] clk: tegra: add ac97 controller clock

[Stephen Warren]

On 03/21/2013 08:01 AM, Lucas Stach wrote:
> Am Mittwoch, den 30.01.2013, 11:27 -0700 schrieb Stephen Warren:
>> On 01/30/2013 02:35 AM, Peter De Schrijver wrote:
>>>>
>>>>> and also ac97 clock == pll_a_out0, so you can
>>>>> just as well use the pll out in that case.
>>>>
>>>> I suspect that "ac97 clock == pll_a_out0" isn't categorically true; it's
>>>> quite possible that the TRM simply doesn't document the mux
>>>> register/field for the ac97 clock since the ac97 module is considered
>>>> deprecated. The diagram at the end of section 5.2.3 "Audio Clocks"
>>>> certainly implies that all of the i2s*, spdifout, and ac97 clocks have
>>>> the same structure.
>>>
>>> It indeed does, but I can't find any reference to a mux control register for
>>> ac97 in any document...
>>
>> For both these two issues, I'll try and track down the relevant HW
>> engineers or other sources of documentation internally...
>>
> Ping.
> I would really like to see those two patches in 3.9, as otherwise
> Colibri T20 boots up with kernel warnings and a non-functional audio.

Sorry, I seem to have forgotten about this, and it looks like I wasn't
able to get any kind of answer from HW. I expect it's far too late to
get this into 3.9.

I guess we can go ahead and add the clock without actually resolving
these issues; I don't think their resolution would affect the device
tree at all, so there should be no compatibility issues to worry about.

I suggest reposting to Mike Turqette (clock maintainer) to see if he'll
take it as a fix for 3.9, or ack it so I can. If not, if he can ack it,
I'll take it for 3.10 along with the Tegra clock driver changes.

[PATCH 2/2] clk: tegra: add ac97 controller clock

[Stephen Warren]

From: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>

AC97 controller clock is hardwired to pll_a_out0.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
Acked-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Prashant Gaikwad <pgaikwad-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-tegra20.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index ecfe532..2547bc0 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -872,6 +872,14 @@ static void __init tegra20_periph_clk_init(void)
 	struct clk *clk;
 	int i;
 
+	/* ac97 */
+	clk = tegra_clk_register_periph_gate("ac97", "pll_a_out0",
+				    TEGRA_PERIPH_ON_APB,
+				    clk_base, 0, 3, &periph_l_regs,
+				    periph_clk_enb_refcnt);
+	clk_register_clkdev(clk, NULL, "tegra20-ac97");
+	clks[ac97] = clk;
+
 	/* apbdma */
 	clk = tegra_clk_register_periph_gate("apbdma", "pclk", 0, clk_base,
 				    0, 34, &periph_h_regs,
-- 
1.7.10.4