Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Stephen Warren]

On 01/04/2013 10:22 AM, Stephen Warren wrote:
> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>> This patchset does following:
>> 1. Decompose single tegra clock structure into multiple clocks.
>> 2. Try to use standard clock types supported by common clock framework.
>> 3. Use dynamic initialization.
>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>> 6. Remove all legacy clock code from mach-tegra.
> 
> I think there are bugs here. I applied all your clock patches on top of
> Tegra's for-next (see list below), and found that the following don't
> work on Springbank:
> 
> * HDMI display
> * Audio playback
> * WiFi

(BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@ instead...)

Prashant, some updated testing results based off the "dev/ccf" branch
you sent me on our internal git server:

The following work, on both Tegra20/30 unless otherwise specified:

* MMC/SD, I2C, audio, HDMI display (Tegra20 only; on Springbank), USB
(Tegra20 only; not implemented on Tegra30).

I didn't test the following yet:

* LCD display on Tegra20.

The following are broken:

1)

SPI on either chip. This is because the SPI drivers call clk_get() with
a specific clock name, and the DT patches you created don't have a
clk-names property for any of the SPI nodes, hence this fails. However,
since there's only a single clock to the SPI controllers, I think the
drivers should be fixed not to request a specific clock name. The
following makes SPI work on TrimSlice (Tegra20) and Cardhu (Tegra30):

> diff --git a/drivers/spi/spi-tegra20-sflash.c b/drivers/spi/spi-tegra20-sflash.c
> index 02feaa5..e5dce91 100644
> --- a/drivers/spi/spi-tegra20-sflash.c
> +++ b/drivers/spi/spi-tegra20-sflash.c
> @@ -525,7 +525,7 @@ static int tegra_sflash_probe(struct platform_device *pdev)
>                 goto exit_free_master;
>         }
>  
> -       tsd->clk = devm_clk_get(&pdev->dev, "spi");
> +       tsd->clk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(tsd->clk)) {
>                 dev_err(&pdev->dev, "can not get clock\n");
>                 ret = PTR_ERR(tsd->clk);
> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
> index fa208a5..e255e7a 100644
> --- a/drivers/spi/spi-tegra20-slink.c
> +++ b/drivers/spi/spi-tegra20-slink.c
> @@ -1191,7 +1191,7 @@ static int tegra_slink_probe(struct platform_device *pdev)
>                 goto exit_free_master;
>         }
>  
> -       tspi->clk = devm_clk_get(&pdev->dev, "slink");
> +       tspi->clk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(tspi->clk)) {
>                 dev_err(&pdev->dev, "can not get clock\n");
>                 ret = PTR_ERR(tspi->clk);

I think you should include that in your series before the patches which
add clocks into the DT.

2)

HDMI and LCD display are both broken on Cardhu/Tegra30.

I found that the DT lists the HDMI clock's parent as pll_d2_out0, yet
the clock driver lists pll_d2 as a valid parent. I think the following
patch fixes this correctly?

> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> index a52b80c..661b9e1 100644
> --- a/drivers/clk/tegra/clk-tegra30.c
> +++ b/drivers/clk/tegra/clk-tegra30.c
> @@ -1385,8 +1385,8 @@ static const char *mux_pllpc_clkm_clk32k[] = { "pll_p", "pll_c", "clk_m",
>  static const char *mux_pllmcpa[] = { "pll_m", "pll_c", "pll_p", "pll_a" };
>  static const char *mux_pllpdc_clkm[] = { "pll_p", "pll_d", "pll_c", "clk_m" };
>  static const char *mux_pllp_clkm[] = { "pll_p", "unused", "unused", "clk_m" };
> -static const char *mux_pllpmdacd2_clkm[] = { "pll_p", "pll_m", "pll_d",
> -                                            "pll_a", "pll_c", "pll_d2",
> +static const char *mux_pllpmdacd2_clkm[] = { "pll_p", "pll_m", "pll_d_out0",
> +                                            "pll_a", "pll_c", "pll_d2_out0",
>                                              "clk_m" };
>  static const char *mux_plla_clk32k_pllp_clkm_plle[] = { "pll_a_out0",
>                                                         "clk_32k", "pll_p",

(are there any other similar problems in the code?)

After that, if I comment out:

late_initcall(clk_disable_unused);

... then either HDMI or LCD output on Cardhu work OK (or rather in
HDMI's case, as well as can be expected due incorrect memory controller
programming and hence display underflow at the moment). I haven't had a
chance to investigate why that initcall is causing problems (the problem
being a HW hang during boot in the HDMI case, and simply no visible
output in the LCD case, but the system otherwise works OK).

For reference, the patches to enable LCD/HDMI on Cardhu can be found in:

git://nv-tegra.nvidia.com/user/swarren/linux-2.6 linux-next_common

3)

WiFi doesn't work on either chip (Springbank or Cardhu, but I expect
Ventana has the same issue). I haven't been able to investigate why yet.

4)

I noticed that the PCIe driver (on Trimslice/Tegra20) was complaining
about clk_get() failing or similar. I haven't been able to investigate
why yet.

For reference, all of these problems are all introduced by your commit
"arm: tegra: Migrate to new clock code"; everything works immediately
before that.

Let me know if you find any solutions to these; I guess just update your
branch. Otherwise, I'll keep investigating tomorrow.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Prashant Gaikwad]

On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>> This patchset does following:
>>> 1. Decompose single tegra clock structure into multiple clocks.
>>> 2. Try to use standard clock types supported by common clock framework.
>>> 3. Use dynamic initialization.
>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>> 6. Remove all legacy clock code from mach-tegra.
>> I think there are bugs here. I applied all your clock patches on top of
>> Tegra's for-next (see list below), and found that the following don't
>> work on Springbank:
>>
>> * HDMI display
>> * Audio playback
>> * WiFi
> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@ instead...)
>
> Prashant, some updated testing results based off the "dev/ccf" branch
> you sent me on our internal git server:

Stephen, thanks a lot for the extensive testing and fix!!

> The following work, on both Tegra20/30 unless otherwise specified:
>
> * MMC/SD, I2C, audio, HDMI display (Tegra20 only; on Springbank), USB
> (Tegra20 only; not implemented on Tegra30).
>
> I didn't test the following yet:
>
> * LCD display on Tegra20.
>
> The following are broken:
>
> 1)
>
> SPI on either chip. This is because the SPI drivers call clk_get() with
> a specific clock name, and the DT patches you created don't have a
> clk-names property for any of the SPI nodes, hence this fails. However,
> since there's only a single clock to the SPI controllers, I think the
> drivers should be fixed not to request a specific clock name. The
> following makes SPI work on TrimSlice (Tegra20) and Cardhu (Tegra30):
>
>> diff --git a/drivers/spi/spi-tegra20-sflash.c b/drivers/spi/spi-tegra20-sflash.c
>> index 02feaa5..e5dce91 100644
>> --- a/drivers/spi/spi-tegra20-sflash.c
>> +++ b/drivers/spi/spi-tegra20-sflash.c
>> @@ -525,7 +525,7 @@ static int tegra_sflash_probe(struct platform_device *pdev)
>>                  goto exit_free_master;
>>          }
>>   
>> -       tsd->clk = devm_clk_get(&pdev->dev, "spi");
>> +       tsd->clk = devm_clk_get(&pdev->dev, NULL);
>>          if (IS_ERR(tsd->clk)) {
>>                  dev_err(&pdev->dev, "can not get clock\n");
>>                  ret = PTR_ERR(tsd->clk);
>> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
>> index fa208a5..e255e7a 100644
>> --- a/drivers/spi/spi-tegra20-slink.c
>> +++ b/drivers/spi/spi-tegra20-slink.c
>> @@ -1191,7 +1191,7 @@ static int tegra_slink_probe(struct platform_device *pdev)
>>                  goto exit_free_master;
>>          }
>>   
>> -       tspi->clk = devm_clk_get(&pdev->dev, "slink");
>> +       tspi->clk = devm_clk_get(&pdev->dev, NULL);
>>          if (IS_ERR(tspi->clk)) {
>>                  dev_err(&pdev->dev, "can not get clock\n");
>>                  ret = PTR_ERR(tspi->clk);
> I think you should include that in your series before the patches which
> add clocks into the DT.

Included.

> 2)
>
> HDMI and LCD display are both broken on Cardhu/Tegra30.
>
> I found that the DT lists the HDMI clock's parent as pll_d2_out0, yet
> the clock driver lists pll_d2 as a valid parent. I think the following
> patch fixes this correctly?
>
>> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
>> index a52b80c..661b9e1 100644
>> --- a/drivers/clk/tegra/clk-tegra30.c
>> +++ b/drivers/clk/tegra/clk-tegra30.c
>> @@ -1385,8 +1385,8 @@ static const char *mux_pllpc_clkm_clk32k[] = { "pll_p", "pll_c", "clk_m",
>>   static const char *mux_pllmcpa[] = { "pll_m", "pll_c", "pll_p", "pll_a" };
>>   static const char *mux_pllpdc_clkm[] = { "pll_p", "pll_d", "pll_c", "clk_m" };
>>   static const char *mux_pllp_clkm[] = { "pll_p", "unused", "unused", "clk_m" };
>> -static const char *mux_pllpmdacd2_clkm[] = { "pll_p", "pll_m", "pll_d",
>> -                                            "pll_a", "pll_c", "pll_d2",
>> +static const char *mux_pllpmdacd2_clkm[] = { "pll_p", "pll_m", "pll_d_out0",
>> +                                            "pll_a", "pll_c", "pll_d2_out0",
>>                                               "clk_m" };
>>   static const char *mux_plla_clk32k_pllp_clkm_plle[] = { "pll_a_out0",
>>                                                          "clk_32k", "pll_p",
> (are there any other similar problems in the code?)

Included fix and reviewed code for more instances. There were some.

> After that, if I comment out:
>
> late_initcall(clk_disable_unused);
>
> ... then either HDMI or LCD output on Cardhu work OK (or rather in
> HDMI's case, as well as can be expected due incorrect memory controller
> programming and hence display underflow at the moment). I haven't had a
> chance to investigate why that initcall is causing problems (the problem
> being a HW hang during boot in the HDMI case, and simply no visible
> output in the LCD case, but the system otherwise works OK).
>
> For reference, the patches to enable LCD/HDMI on Cardhu can be found in:
>
> git://nv-tegra.nvidia.com/user/swarren/linux-2.6 linux-next_common

Not able to test LCD/HDMI even using above branch. Not sure what is missing.
In kernel log I can see that Tegra DRM is initialized properly. Tried 
"dmesg > /dev/fb0".

> 3)
>
> WiFi doesn't work on either chip (Springbank or Cardhu, but I expect
> Ventana has the same issue). I haven't been able to investigate why yet.

Probable fix included, can not test.

> 4)
>
> I noticed that the PCIe driver (on Trimslice/Tegra20) was complaining
> about clk_get() failing or similar. I haven't been able to investigate
> why yet.

Fix included, pcie_xclk clock was missing

> For reference, all of these problems are all introduced by your commit
> "arm: tegra: Migrate to new clock code"; everything works immediately
> before that.
>
> Let me know if you find any solutions to these; I guess just update your
> branch. Otherwise, I'll keep investigating tomorrow.

I have updated the internal branch with all the above mentioned fixes.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Stephen Warren]

On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>> This patchset does following:
>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>> 2. Try to use standard clock types supported by common clock framework.
>>>> 3. Use dynamic initialization.
>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>> 6. Remove all legacy clock code from mach-tegra.
>>> I think there are bugs here. I applied all your clock patches on top of
>>> Tegra's for-next (see list below), and found that the following don't
>>> work on Springbank:
>>>
>>> * HDMI display
>>> * Audio playback
>>> * WiFi
>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@ instead...)
>>
>> Prashant, some updated testing results based off the "dev/ccf" branch
>> you sent me on our internal git server:
...
> I have updated the internal branch with all the above mentioned fixes.

WiFi and SPI now work on both Tegra20/30.

PCIe still doesn't work on Tegra20. The reason is that
clk_plle_enable()'s call to _get_table_rate() fails, since
pll->fixed_rate is 0, and hence there's no matching table entry.

I simply commented out that call, and the later code that uses its
results to configure the PLL rate, and then everything worked - or at
least PCIe device enumeration and hence lspci worked, which is as much
as works right now with PCIe on Tegra...

Is the solution here to fix clk-tegra20.c's call to tegra_clk_plle() to
specify a valid fixed rate?

However, I'm puzzled why clk_plle_enable() is even touching the dividers
though; shouldn't clk_plle_set_rate() be setting up the rate, and
clk_plle_enable() /just/ be enabling/disabling the PLL? The old clock
driver appears to work the way I expect, and use the standard PLL
set_rate op for this clock, and has a very simple custom enable op for PLLe.

The remaining item is the display issue on Tegra30, which I'll go look
at now.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Stephen Warren]

On 01/08/2013 11:49 AM, Stephen Warren wrote:
> On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
>> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>>> This patchset does following:
>>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>>> 2. Try to use standard clock types supported by common clock framework.
>>>>> 3. Use dynamic initialization.
>>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>>> 6. Remove all legacy clock code from mach-tegra.
>>>> I think there are bugs here. I applied all your clock patches on top of
>>>> Tegra's for-next (see list below), and found that the following don't
>>>> work on Springbank:
>>>>
>>>> * HDMI display
>>>> * Audio playback
>>>> * WiFi
>>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@ instead...)
>>>
>>> Prashant, some updated testing results based off the "dev/ccf" branch
>>> you sent me on our internal git server:
> ...
>> I have updated the internal branch with all the above mentioned fixes.
...
> The remaining item is the display issue on Tegra30, which I'll go look
> at now.

The USB3 clock, which isn't used by any drivers on Tegra30, and hence
was disabled at boot, was set up incorrectly and ended up mapping to the
disp1 clock, and hence turned off the display. The following fixes it:

> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> index f2f526e..0bac07c 100644
> --- a/drivers/clk/tegra/clk-tegra30.c
> +++ b/drivers/clk/tegra/clk-tegra30.c
> @@ -1541,7 +1541,7 @@ static void __init tegra30_periph_clk_init(void)
>  
>  	/* usb3 */
>  	clk = tegra_clk_periph_gate("usb3", "clk_m", 0, clk_base, 0, 59,
> -				    &periph_l_regs, periph_clk_enb_refcnt);
> +				    &periph_h_regs, periph_clk_enb_refcnt);
>  	clk_register_clkdev(clk, NULL, "tegra-ehci.2");
>  	clks[usb3] = clk;

I wonder if there are any other similar issues?

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Prashant Gaikwad]

On Wednesday 09 January 2013 02:31 AM, Stephen Warren wrote:
> On 01/08/2013 11:49 AM, Stephen Warren wrote:
>> On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
>>> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>>>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>>>> This patchset does following:
>>>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>>>> 2. Try to use standard clock types supported by common clock framework.
>>>>>> 3. Use dynamic initialization.
>>>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>>>> 6. Remove all legacy clock code from mach-tegra.
>>>>> I think there are bugs here. I applied all your clock patches on top of
>>>>> Tegra's for-next (see list below), and found that the following don't
>>>>> work on Springbank:
>>>>>
>>>>> * HDMI display
>>>>> * Audio playback
>>>>> * WiFi
>>>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@ instead...)
>>>>
>>>> Prashant, some updated testing results based off the "dev/ccf" branch
>>>> you sent me on our internal git server:
>> ...
>>> I have updated the internal branch with all the above mentioned fixes.
> ...
>> The remaining item is the display issue on Tegra30, which I'll go look
>> at now.
> The USB3 clock, which isn't used by any drivers on Tegra30, and hence
> was disabled at boot, was set up incorrectly and ended up mapping to the
> disp1 clock, and hence turned off the display. The following fixes it:

Stephen, thanks for the fix!! I have included this and PLLE fix; updated 
internal branch.

Please let me know if it is good to merge now, I will send the patches.

>> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
>> index f2f526e..0bac07c 100644
>> --- a/drivers/clk/tegra/clk-tegra30.c
>> +++ b/drivers/clk/tegra/clk-tegra30.c
>> @@ -1541,7 +1541,7 @@ static void __init tegra30_periph_clk_init(void)
>>   
>>   	/* usb3 */
>>   	clk = tegra_clk_periph_gate("usb3", "clk_m", 0, clk_base, 0, 59,
>> -				    &periph_l_regs, periph_clk_enb_refcnt);
>> +				    &periph_h_regs, periph_clk_enb_refcnt);
>>   	clk_register_clkdev(clk, NULL, "tegra-ehci.2");
>>   	clks[usb3] = clk;
> I wonder if there are any other similar issues?

Reviewed, there were none.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Stephen Warren]

On 01/09/2013 03:59 AM, Prashant Gaikwad wrote:
> On Wednesday 09 January 2013 02:31 AM, Stephen Warren wrote:
>> On 01/08/2013 11:49 AM, Stephen Warren wrote:
>>> On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
>>>> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>>>>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>>>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>>>>> This patchset does following:
>>>>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>>>>> 2. Try to use standard clock types supported by common clock
>>>>>>> framework.
>>>>>>> 3. Use dynamic initialization.
>>>>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>>>>> 6. Remove all legacy clock code from mach-tegra.
>>>>>> I think there are bugs here. I applied all your clock patches on
>>>>>> top of
>>>>>> Tegra's for-next (see list below), and found that the following don't
>>>>>> work on Springbank:
>>>>>>
>>>>>> * HDMI display
>>>>>> * Audio playback
>>>>>> * WiFi
>>>>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@
>>>>> instead...)
>>>>>
>>>>> Prashant, some updated testing results based off the "dev/ccf" branch
>>>>> you sent me on our internal git server:
>>> ...
>>>> I have updated the internal branch with all the above mentioned fixes.
>> ...
>>> The remaining item is the display issue on Tegra30, which I'll go look
>>> at now.
>> The USB3 clock, which isn't used by any drivers on Tegra30, and hence
>> was disabled at boot, was set up incorrectly and ended up mapping to the
>> disp1 clock, and hence turned off the display. The following fixes it:
> 
> Stephen, thanks for the fix!! I have included this and PLLE fix; updated
> internal branch.

Almost everything works great now.

However, I don't see any fix for the PLLE issue in the code; if there is
one it certainly doesn't work. Applying my previous hack makes it work.

FYI, the branch I tested with is at
git://nv-tegra.nvidia.com/user/swarren/linux-2.6 test-ccf-rework-2

It's rebased onto the latest Tegra for-next.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Stephen Warren]

On 01/09/2013 10:34 AM, Stephen Warren wrote:
> On 01/09/2013 03:59 AM, Prashant Gaikwad wrote:
>> On Wednesday 09 January 2013 02:31 AM, Stephen Warren wrote:
>>> On 01/08/2013 11:49 AM, Stephen Warren wrote:
>>>> On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
>>>>> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>>>>>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>>>>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>>>>>> This patchset does following:
>>>>>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>>>>>> 2. Try to use standard clock types supported by common clock
>>>>>>>> framework.
>>>>>>>> 3. Use dynamic initialization.
>>>>>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>>>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>>>>>> 6. Remove all legacy clock code from mach-tegra.
>>>>>>> I think there are bugs here. I applied all your clock patches on
>>>>>>> top of
>>>>>>> Tegra's for-next (see list below), and found that the following don't
>>>>>>> work on Springbank:
>>>>>>>
>>>>>>> * HDMI display
>>>>>>> * Audio playback
>>>>>>> * WiFi
>>>>>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@
>>>>>> instead...)
>>>>>>
>>>>>> Prashant, some updated testing results based off the "dev/ccf" branch
>>>>>> you sent me on our internal git server:
>>>> ...
>>>>> I have updated the internal branch with all the above mentioned fixes.
>>> ...
>>>> The remaining item is the display issue on Tegra30, which I'll go look
>>>> at now.
>>> The USB3 clock, which isn't used by any drivers on Tegra30, and hence
>>> was disabled at boot, was set up incorrectly and ended up mapping to the
>>> disp1 clock, and hence turned off the display. The following fixes it:
>>
>> Stephen, thanks for the fix!! I have included this and PLLE fix; updated
>> internal branch.
> 
> Almost everything works great now.

I noticed a couple more issues.

First, the clock rate rounding behaviour changes with your patch series
for at least the I2C clocks.

With I2C, you specify how fast you want the bus to go. The bus shouldn't
run any faster than that since the rate matches the max specification
for the attached devices. So the clock framework should pick the
smallest divider that yields a clock that doesn't exceed the request
from clk_set_rate(). However, the new code seems to pick the smallest
divider that yields at least the requested clock. I can certainly see
that other clocks (say memory, CPU, or other internal
performance-related clocks) might want to round the other way though.

The I2C driver sets its module clock to 8 times the desired bus rate.
This yields a request of:

desired bus rate: 100 KHz
desired clock rate: 800 KHz
actual clock rate before your change: 800 KHz
actual clock rate after your change: 800 KHz
-> OK

desired bus rate: 400 KHz
desired clock rate: 3.2 MHz
actual clock rate before your change: 3 MHz (so 375 KHz bus rate, OK)
actual clock rate after your change: 4 MHZ (so 500 KHz bus rate, BAD)

desired bus rate: 80 KHz (Toshiba AC100's nvec/I2C slave driver)
desired clock rate: 640 KHz
actual clock rate before your change: ~632 KHz (so ~79 KHz bus rate, OK)
actual clock rate after your change: ~706 KHz (so ~88 KHz bus rate, BAD)

Can this be fixed?

Second, the Toshiba AC100 uses an alternative driver for the I2C HW,
since it operates in I2C slave mode. So, the DT node for that driver
needs to include the clocks properties so the driver can get the clocks
through DT:

> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
> index edef66c..6495425 100644
> --- a/arch/arm/boot/dts/tegra20-paz00.dts
> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
> @@ -278,6 +278,8 @@
>                 clock-frequency = <50000>;
>                 request-gpios = <&gpio 170 0>; /* gpio PV2 */
>                 slave-addr = <138>;
> +               clocks = <&tegra_car 67>, <&tegra_car 124>;
> +               clock-names = "div-clk", "fast-clk";
>         };
>  
>         i2c@7000d000 {

Your changes don't actually cause the driver to break though, since it
abuses clk_get_sys() to retrieve clocks under a different driver name,
which matches what the clock driver provides. However, I think you
should also include the following patch at the end of your series to fix
this up, so the clock looking happens through device tree:

> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index d8826ed..6d44076 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -770,7 +770,7 @@ static int tegra_nvec_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>  
> -       i2c_clk = clk_get_sys("tegra-i2c.2", "div-clk");
> +       i2c_clk = clk_get(&pdev->dev, "div-clk");
>         if (IS_ERR(i2c_clk)) {
>                 dev_err(nvec->dev, "failed to get controller clock\n");
>                 return -ENODEV;

Thanks.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Prashant Gaikwad]

On Thursday 10 January 2013 02:14 AM, Stephen Warren wrote:
> On 01/09/2013 10:34 AM, Stephen Warren wrote:
>> On 01/09/2013 03:59 AM, Prashant Gaikwad wrote:
>>> On Wednesday 09 January 2013 02:31 AM, Stephen Warren wrote:
>>>> On 01/08/2013 11:49 AM, Stephen Warren wrote:
>>>>> On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
>>>>>> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>>>>>>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>>>>>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>>>>>>> This patchset does following:
>>>>>>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>>>>>>> 2. Try to use standard clock types supported by common clock
>>>>>>>>> framework.
>>>>>>>>> 3. Use dynamic initialization.
>>>>>>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>>>>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>>>>>>> 6. Remove all legacy clock code from mach-tegra.
>>>>>>>> I think there are bugs here. I applied all your clock patches on
>>>>>>>> top of
>>>>>>>> Tegra's for-next (see list below), and found that the following don't
>>>>>>>> work on Springbank:
>>>>>>>>
>>>>>>>> * HDMI display
>>>>>>>> * Audio playback
>>>>>>>> * WiFi
>>>>>>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@
>>>>>>> instead...)
>>>>>>>
>>>>>>> Prashant, some updated testing results based off the "dev/ccf" branch
>>>>>>> you sent me on our internal git server:
>>>>> ...
>>>>>> I have updated the internal branch with all the above mentioned fixes.
>>>> ...
>>>>> The remaining item is the display issue on Tegra30, which I'll go look
>>>>> at now.
>>>> The USB3 clock, which isn't used by any drivers on Tegra30, and hence
>>>> was disabled at boot, was set up incorrectly and ended up mapping to the
>>>> disp1 clock, and hence turned off the display. The following fixes it:
>>> Stephen, thanks for the fix!! I have included this and PLLE fix; updated
>>> internal branch.
>> Almost everything works great now.
> I noticed a couple more issues.
>
> First, the clock rate rounding behaviour changes with your patch series
> for at least the I2C clocks.
>
> With I2C, you specify how fast you want the bus to go. The bus shouldn't
> run any faster than that since the rate matches the max specification
> for the attached devices. So the clock framework should pick the
> smallest divider that yields a clock that doesn't exceed the request
> from clk_set_rate(). However, the new code seems to pick the smallest
> divider that yields at least the requested clock. I can certainly see
> that other clocks (say memory, CPU, or other internal
> performance-related clocks) might want to round the other way though.
>
> The I2C driver sets its module clock to 8 times the desired bus rate.
> This yields a request of:
>
> desired bus rate: 100 KHz
> desired clock rate: 800 KHz
> actual clock rate before your change: 800 KHz
> actual clock rate after your change: 800 KHz
> -> OK
>
> desired bus rate: 400 KHz
> desired clock rate: 3.2 MHz
> actual clock rate before your change: 3 MHz (so 375 KHz bus rate, OK)
> actual clock rate after your change: 4 MHZ (so 500 KHz bus rate, BAD)
>
> desired bus rate: 80 KHz (Toshiba AC100's nvec/I2C slave driver)
> desired clock rate: 640 KHz
> actual clock rate before your change: ~632 KHz (so ~79 KHz bus rate, OK)
> actual clock rate after your change: ~706 KHz (so ~88 KHz bus rate, BAD)
>
> Can this be fixed?

Fixed in latest patches.

> Second, the Toshiba AC100 uses an alternative driver for the I2C HW,
> since it operates in I2C slave mode. So, the DT node for that driver
> needs to include the clocks properties so the driver can get the clocks
> through DT:
>
>> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
>> index edef66c..6495425 100644
>> --- a/arch/arm/boot/dts/tegra20-paz00.dts
>> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
>> @@ -278,6 +278,8 @@
>>                  clock-frequency = <50000>;
>>                  request-gpios = <&gpio 170 0>; /* gpio PV2 */
>>                  slave-addr = <138>;
>> +               clocks = <&tegra_car 67>, <&tegra_car 124>;
>> +               clock-names = "div-clk", "fast-clk";
>>          };
>>   
>>          i2c@7000d000 {
> Your changes don't actually cause the driver to break though, since it
> abuses clk_get_sys() to retrieve clocks under a different driver name,
> which matches what the clock driver provides. However, I think you
> should also include the following patch at the end of your series to fix
> this up, so the clock looking happens through device tree:
>
>> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
>> index d8826ed..6d44076 100644
>> --- a/drivers/staging/nvec/nvec.c
>> +++ b/drivers/staging/nvec/nvec.c
>> @@ -770,7 +770,7 @@ static int tegra_nvec_probe(struct platform_device *pdev)
>>                  return -ENODEV;
>>          }
>>   
>> -       i2c_clk = clk_get_sys("tegra-i2c.2", "div-clk");
>> +       i2c_clk = clk_get(&pdev->dev, "div-clk");
>>          if (IS_ERR(i2c_clk)) {
>>                  dev_err(nvec->dev, "failed to get controller clock\n");
>>                  return -ENODEV;

Included in the latest patches sent.

> Thanks.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Prashant Gaikwad]

On Wednesday 09 January 2013 11:04 PM, Stephen Warren wrote:
> On 01/09/2013 03:59 AM, Prashant Gaikwad wrote:
>> On Wednesday 09 January 2013 02:31 AM, Stephen Warren wrote:
>>> On 01/08/2013 11:49 AM, Stephen Warren wrote:
>>>> On 01/08/2013 06:19 AM, Prashant Gaikwad wrote:
>>>>> On Tuesday 08 January 2013 05:40 AM, Stephen Warren wrote:
>>>>>> On 01/04/2013 10:22 AM, Stephen Warren wrote:
>>>>>>> On 01/04/2013 02:40 AM, Prashant Gaikwad wrote:
>>>>>>>> This patchset does following:
>>>>>>>> 1. Decompose single tegra clock structure into multiple clocks.
>>>>>>>> 2. Try to use standard clock types supported by common clock
>>>>>>>> framework.
>>>>>>>> 3. Use dynamic initialization.
>>>>>>>> 4. Move all clock code to drivers/clk/tegra from mach-tegra.
>>>>>>>> 5. Add device tree support for Tegra20 and Tegra30 clocks.
>>>>>>>> 6. Remove all legacy clock code from mach-tegra.
>>>>>>> I think there are bugs here. I applied all your clock patches on
>>>>>>> top of
>>>>>>> Tegra's for-next (see list below), and found that the following don't
>>>>>>> work on Springbank:
>>>>>>>
>>>>>>> * HDMI display
>>>>>>> * Audio playback
>>>>>>> * WiFi
>>>>>> (BTW, I stopped Cc'ing linux-kernel@, but added linux-tegra@
>>>>>> instead...)
>>>>>>
>>>>>> Prashant, some updated testing results based off the "dev/ccf" branch
>>>>>> you sent me on our internal git server:
>>>> ...
>>>>> I have updated the internal branch with all the above mentioned fixes.
>>> ...
>>>> The remaining item is the display issue on Tegra30, which I'll go look
>>>> at now.
>>> The USB3 clock, which isn't used by any drivers on Tegra30, and hence
>>> was disabled at boot, was set up incorrectly and ended up mapping to the
>>> disp1 clock, and hence turned off the display. The following fixes it:
>> Stephen, thanks for the fix!! I have included this and PLLE fix; updated
>> internal branch.
> Almost everything works great now.

Great!!

> However, I don't see any fix for the PLLE issue in the code; if there is
> one it certainly doesn't work. Applying my previous hack makes it work.

There is some difference between Tegra20 and Tegra30 PLLE implementation.
Fixed in latest patches sent.

> FYI, the branch I tested with is at
> git://nv-tegra.nvidia.com/user/swarren/linux-2.6 test-ccf-rework-2
>
> It's rebased onto the latest Tegra for-next.

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Marc Dietrich]

On Friday 11 January 2013 13:40:03 Prashant Gaikwad wrote:
> On Thursday 10 January 2013 02:14 AM, Stephen Warren wrote:
> > On 01/09/2013 10:34 AM, Stephen Warren wrote:

...

> > Second, the Toshiba AC100 uses an alternative driver for the I2C HW,
> > since it operates in I2C slave mode. So, the DT node for that driver
> > needs to include the clocks properties so the driver can get the clocks
> > 
> > through DT:
> >> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
> >> b/arch/arm/boot/dts/tegra20-paz00.dts index edef66c..6495425 100644
> >> --- a/arch/arm/boot/dts/tegra20-paz00.dts
> >> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
> >> @@ -278,6 +278,8 @@
> >> 
> >>                  clock-frequency = <50000>;
> >>                  request-gpios = <&gpio 170 0>; /* gpio PV2 */
> >>                  slave-addr = <138>;
> >> 
> >> +               clocks = <&tegra_car 67>, <&tegra_car 124>;
> >> +               clock-names = "div-clk", "fast-clk";
> >> 
> >>          };
> >>          
> >>          i2c@7000d000 {
> > 
> > Your changes don't actually cause the driver to break though, since it
> > abuses clk_get_sys() to retrieve clocks under a different driver name,
> > which matches what the clock driver provides. However, I think you
> > should also include the following patch at the end of your series to fix
> > 
> > this up, so the clock looking happens through device tree:
> >> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> >> index d8826ed..6d44076 100644
> >> --- a/drivers/staging/nvec/nvec.c
> >> +++ b/drivers/staging/nvec/nvec.c
> >> @@ -770,7 +770,7 @@ static int tegra_nvec_probe(struct platform_device
> >> *pdev)>> 
> >>                  return -ENODEV;
> >>          
> >>          }
> >> 
> >> -       i2c_clk = clk_get_sys("tegra-i2c.2", "div-clk");
> >> +       i2c_clk = clk_get(&pdev->dev, "div-clk");
> >> 
> >>          if (IS_ERR(i2c_clk)) {
> >>          
> >>                  dev_err(nvec->dev, "failed to get controller clock\n");
> >>                  return -ENODEV;
> 
> Included in the latest patches sent.

em, not yet in V4. Maybe you can also adjust the TODO (2nd entry) file now 
that this issue is fixed.

Thanks for taking care of this.

Marc

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Stephen Warren]

On 01/11/2013 08:59 AM, Marc Dietrich wrote:
> On Friday 11 January 2013 13:40:03 Prashant Gaikwad wrote:
>> On Thursday 10 January 2013 02:14 AM, Stephen Warren wrote:
>>> On 01/09/2013 10:34 AM, Stephen Warren wrote:
...
>>> Your changes don't actually cause the driver to break though, since it
>>> abuses clk_get_sys() to retrieve clocks under a different driver name,
>>> which matches what the clock driver provides. However, I think you
>>> should also include the following patch at the end of your series to fix
>>> this up, so the clock looking happens through device tree:
>>>
>>>> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
>>>> index d8826ed..6d44076 100644
>>>> --- a/drivers/staging/nvec/nvec.c
>>>> +++ b/drivers/staging/nvec/nvec.c
>>>> @@ -770,7 +770,7 @@ static int tegra_nvec_probe(struct platform_device
>>>> *pdev)>> 
>>>>                  return -ENODEV;
>>>>          
>>>>          }
>>>>
>>>> -       i2c_clk = clk_get_sys("tegra-i2c.2", "div-clk");
>>>> +       i2c_clk = clk_get(&pdev->dev, "div-clk");
>>>>
>>>>          if (IS_ERR(i2c_clk)) {
>>>>          
>>>>                  dev_err(nvec->dev, "failed to get controller clock\n");
>>>>                  return -ENODEV;
>>
>> Included in the latest patches sent.
> 
> em, not yet in V4.

It's in V2 of the other series Prashant posted which sits on top of the
CCF rework series.

> Maybe you can also adjust the TODO (2nd entry) file now 
> that this issue is fixed.

I'll try to remember to repost an updated version of the patch which
does that...

Re: [PATCH v3 0/9] Migrate Tegra to common clock framework

[Marc Dietrich]

On Friday 11 January 2013 11:23:56 Stephen Warren wrote:
> On 01/11/2013 08:59 AM, Marc Dietrich wrote:
> > On Friday 11 January 2013 13:40:03 Prashant Gaikwad wrote:
> >> On Thursday 10 January 2013 02:14 AM, Stephen Warren wrote:
> >>> On 01/09/2013 10:34 AM, Stephen Warren wrote:
> ...
> 
> >>> Your changes don't actually cause the driver to break though, since it
> >>> abuses clk_get_sys() to retrieve clocks under a different driver name,
> >>> which matches what the clock driver provides. However, I think you
> >>> should also include the following patch at the end of your series to fix
> >>> 
> >>> this up, so the clock looking happens through device tree:
> >>>> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> >>>> index d8826ed..6d44076 100644
> >>>> --- a/drivers/staging/nvec/nvec.c
> >>>> +++ b/drivers/staging/nvec/nvec.c
> >>>> @@ -770,7 +770,7 @@ static int tegra_nvec_probe(struct platform_device
> >>>> *pdev)>>
> >>>> 
> >>>>                  return -ENODEV;
> >>>>          
> >>>>          }
> >>>> 
> >>>> -       i2c_clk = clk_get_sys("tegra-i2c.2", "div-clk");
> >>>> +       i2c_clk = clk_get(&pdev->dev, "div-clk");
> >>>> 
> >>>>          if (IS_ERR(i2c_clk)) {
> >>>>          
> >>>>                  dev_err(nvec->dev, "failed to get controller
> >>>>                  clock\n");
> >>>>                  return -ENODEV;
> >> 
> >> Included in the latest patches sent.
> > 
> > em, not yet in V4.
> 
> It's in V2 of the other series Prashant posted which sits on top of the
> CCF rework series.

ahh, sorry - I didn't see it because of the changed subject line.

> > Maybe you can also adjust the TODO (2nd entry) file now
> > that this issue is fixed.
> 
> I'll try to remember to repost an updated version of the patch which
> does that...

no need if it just adds this change. I'll need to update the file anyway soon.

Thanks

Marc