[PATCH] clk: tegra: Fix bypassing of PLLs

[PATCH] clk: tegra: Fix bypassing of PLLs

[Jon Hunter]

The _clk_disable_pll() function will attempt to place a PLL into bypass
if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
by clearing the enable bit. To place the PLL into bypass, the bypass bit
needs to be set and not cleared. Fix this by setting the bypass bit and
not clearing it.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/clk/tegra/clk-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index d6d4ecb88e94..e5aa9c87df4c 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -312,7 +312,7 @@ static void _clk_pll_disable(struct clk_hw *hw)
 
 	val = pll_readl_base(pll);
 	if (pll->params->flags & TEGRA_PLL_BYPASS)
-		val &= ~PLL_BASE_BYPASS;
+		val |= PLL_BASE_BYPASS;
 	val &= ~PLL_BASE_ENABLE;
 	pll_writel_base(val, pll);
 
-- 
2.1.4

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Rhyland Klein]

On 11/20/2015 10:11 AM, Jon Hunter wrote:
> The _clk_disable_pll() function will attempt to place a PLL into bypass
> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
> by clearing the enable bit. To place the PLL into bypass, the bypass bit
> needs to be set and not cleared. Fix this by setting the bypass bit and
> not clearing it.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/clk/tegra/clk-pll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index d6d4ecb88e94..e5aa9c87df4c 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -312,7 +312,7 @@ static void _clk_pll_disable(struct clk_hw *hw)
>  
>  	val = pll_readl_base(pll);
>  	if (pll->params->flags & TEGRA_PLL_BYPASS)
> -		val &= ~PLL_BASE_BYPASS;
> +		val |= PLL_BASE_BYPASS;
>  	val &= ~PLL_BASE_ENABLE;
>  	pll_writel_base(val, pll);
>  
> 


Good catch.

Acked-by: Rhyland Klein <rklein@nvidia.com>


-- 
nvpublic

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Stephen Boyd]

On 11/20, Jon Hunter wrote:
> The _clk_disable_pll() function will attempt to place a PLL into bypass
> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
> by clearing the enable bit. To place the PLL into bypass, the bypass bit
> needs to be set and not cleared. Fix this by setting the bypass bit and
> not clearing it.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---

Fixes tag? It looks like this has been wrong from the beginning
of time.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Jon Hunter]

On 20/11/15 17:15, Stephen Boyd wrote:
> On 11/20, Jon Hunter wrote:
>> The _clk_disable_pll() function will attempt to place a PLL into bypass
>> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
>> by clearing the enable bit. To place the PLL into bypass, the bypass bit
>> needs to be set and not cleared. Fix this by setting the bypass bit and
>> not clearing it.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
> 
> Fixes tag? It looks like this has been wrong from the beginning
> of time.

Yes good point.

Thierry, I see you have put this in the -next branch for tegra. Do you
want to add the following?

Fixes: 8f8f484bf355 ("clk: tegra: add Tegra specific clocks")

Jon

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Tyler Baker]

Hi Jon,

On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
> The _clk_disable_pll() function will attempt to place a PLL into bypass
> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
> by clearing the enable bit. To place the PLL into bypass, the bypass bit
> needs to be set and not cleared. Fix this by setting the bypass bit and
> not clearing it.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
in the tegra tree. This boot failure has only been observed when
booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
this boot failure to this commit, and I confirmed reverting it on top
of the tegra for-next branch resolves the issue. The ramdisk[4] used
for booting is loaded with the modules from the build. It appears to
me that as the modules are being loaded in userspace by eudev the
jetson-tk1 locks up. I've sifted through the console logs a bit, and
found this splat to be most interesting[5].  Can you confirm this
issue on your end?

Cheers,

Tyler

[1] http://kernelci.org/soc/tegra/job/tegra/kernel/v4.4-rc1-60-gb924f95da320/
[2] http://kernelci.org/soc/tegra/job/tegra/
[3] http://hastebin.com/sekozibilo.lua
[4] http://storage.kernelci.org/images/rootfs/buildroot/armel/base/rootfs.cpio.gz
[5] http://hastebin.com/jomigahiro.coffee

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Jon Hunter]

Hi Tyler,

On 23/11/15 23:18, Tyler Baker wrote:
> Hi Jon,
> 
> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
>> The _clk_disable_pll() function will attempt to place a PLL into bypass
>> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
>> by clearing the enable bit. To place the PLL into bypass, the bypass bit
>> needs to be set and not cleared. Fix this by setting the bypass bit and
>> not clearing it.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
> in the tegra tree. This boot failure has only been observed when
> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
> this boot failure to this commit, and I confirmed reverting it on top
> of the tegra for-next branch resolves the issue. The ramdisk[4] used
> for booting is loaded with the modules from the build. It appears to
> me that as the modules are being loaded in userspace by eudev the
> jetson-tk1 locks up. I've sifted through the console logs a bit, and
> found this splat to be most interesting[5].  Can you confirm this
> issue on your end?

Thanks for the report. I have booted the latest next on the jetson-tk1
with the multi_v7_defconfig but I did not see this. However, the test
infrastructure is not loading those modules. I need to look at adding this.

I will see if I can reproduce this today and let you know what I find.

Cheers
Jon

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4466 bytes --]

On Mon, Nov 23, 2015 at 03:18:59PM -0800, Tyler Baker wrote:
> Hi Jon,
> 
> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
> > The _clk_disable_pll() function will attempt to place a PLL into bypass
> > if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
> > by clearing the enable bit. To place the PLL into bypass, the bypass bit
> > needs to be set and not cleared. Fix this by setting the bypass bit and
> > not clearing it.
> >
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
> in the tegra tree. This boot failure has only been observed when
> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
> this boot failure to this commit, and I confirmed reverting it on top
> of the tegra for-next branch resolves the issue. The ramdisk[4] used
> for booting is loaded with the modules from the build. It appears to
> me that as the modules are being loaded in userspace by eudev the
> jetson-tk1 locks up. I've sifted through the console logs a bit, and
> found this splat to be most interesting[5].  Can you confirm this
> issue on your end?

Let me quote one of your logs for ease of commenting. I've trimmed it
somewhat because it got fragmented in the middle and not everything is
relevant:

[    9.809636] tegra-emc 7001b000.emc: no timing for rate 4294967295
[    9.834995] Unable to handle kernel paging request at virtual address 00270300
...
[    9.836808] [] (__irq_svc) from [] (console_unlock+0x3ec/0x47c)
[    9.836854] [] (console_unlock) from [] (vprintk_emit+0x1bf/0x348)
[    9.836905] [] (vprintk_emit) from [] (dev_vprintk_emit+0x9f/0x124)
[    9.836951] [] (dev_vprintk_emit) from [] (dev_printk_emit+0x15/0x20)
[    9.836995] [] (dev_printk_emit) from [] (__dev_printk+0x29/0x48)
[    9.837036] [] (__dev_printk) from [] (dev_err+0x25/0x30)
[    9.837083] [] (dev_err) from [] (tegra_emc_find_timing+0x3d/0x4c)
[    9.837125] [] (tegra_emc_find_timing) from [] (tegra_emc_complete_timing_change+0x9/0x130)
[    9.837162] [] (tegra_emc_complete_timing_change) from [] (emc_set_timing+0xc3/0x158)
[    9.837190] [] (emc_set_timing) from [] (emc_set_rate+0xdb/0x150)
[    9.837221] [] (emc_set_rate) from [] (gpmc_calc_timings+0xb5/0x51c)
[    9.837261] [] (gpmc_calc_timings) from [] (clk_set_rate+0x15/0x20)
[    9.837307] [] (clk_set_rate) from [] (tegra_devfreq_target+0x40/0x58 [tegra_devfreq])
[    9.837350] [] (tegra_devfreq_target [tegra_devfreq]) from [] (update_devfreq+0x4b/0x9c)
[    9.837386] [] (update_devfreq) from [] (actmon_thread_isr+0x12/0x20 [tegra_devfreq])
[    9.837424] [] (actmon_thread_isr [tegra_devfreq]) from [] (irq_thread_dtor+0x77/0x78)
[    9.837456] [] (irq_thread_dtor) from [] (irq_thread+0xcf/0x16c)
[    9.837496] [] (irq_thread) from [] (kthread+0x93/0xac)
[    9.837541] [] (kthread) from [] (ret_from_fork+0x11/0x20)
[    9.837563] Code: bad PC value
[    9.837582] ---[ end trace 586d537b3212336d ]---

The part that's really weird in the above is the call to
gpmc_calc_timings(), because that function is from OMAP:

	$ git grep -n gpmc_calc_timings
	arch/arm/mach-omap2/gpmc-onenand.c:89:  gpmc_calc_timings(t, &onenand_async, &dev_t);
	arch/arm/mach-omap2/gpmc-onenand.c:266: gpmc_calc_timings(t, &onenand_sync, &dev_t);
	arch/arm/mach-omap2/usb-tusb6010.c:72:  gpmc_calc_timings(&t, &tusb_async, &dev_t);
	arch/arm/mach-omap2/usb-tusb6010.c:99:  gpmc_calc_timings(&t, &tusb_sync, &dev_t);
	drivers/memory/omap-gpmc.c:1539:int gpmc_calc_timings(struct gpmc_timings *gpmc_t,
	include/linux/omap-gpmc.h:152:extern int gpmc_calc_timings(struct gpmc_timings *gpmc_t,

So I'm not at all surprised that this breaks. While that seems unrelated
it's quite possible that there's some memory corruption going on which
would also explain the hang. It doesn't even have to be memory
corruption, but we've seen similar problems in the past where some
platform was unconditionally registering drivers that it shouldn't have
been registering and which then executed on a device where the device
wasn't there. That could be the case here as well.

Unfortunately I can't come up with any good explanation of why
gpmc_calc_timings() shows up in the call trace above. It's only ever
called as a result of USB or NAND operation, so why it would be called
from clk_set_rate() is beyond me.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Jon Hunter]

Hi Tyler,

On 23/11/15 23:18, Tyler Baker wrote:
> Hi Jon,
> 
> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
>> The _clk_disable_pll() function will attempt to place a PLL into bypass
>> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
>> by clearing the enable bit. To place the PLL into bypass, the bypass bit
>> needs to be set and not cleared. Fix this by setting the bypass bit and
>> not clearing it.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
> in the tegra tree. This boot failure has only been observed when
> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
> this boot failure to this commit, and I confirmed reverting it on top
> of the tegra for-next branch resolves the issue. The ramdisk[4] used
> for booting is loaded with the modules from the build. It appears to
> me that as the modules are being loaded in userspace by eudev the
> jetson-tk1 locks up. I've sifted through the console logs a bit, and
> found this splat to be most interesting[5].  Can you confirm this
> issue on your end?

It appears that the crash is occurring when the tegra-devfreq driver is
loaded and I have been able to narrow it down to the pllm pll that is
causing the problem. If I remove the bypass flag for pllm then I no
longer see the problem (see below). However, the bypass bit is valid for
this pll and so I need to see if there is another bug lurking in the
management of this pll. The pllm has an additional override feature and
I see another enable bit. I need to check this code.

Cheers
Jon

commit 1e4a77f9f08b34f63fc1d4768a31edd5070321a7
Author: Jon Hunter <jonathanh@nvidia.com>
Date:   Tue Nov 24 15:13:58 2015 +0000

    clk: tegra: Don't bypass pllm (TESTING ONLY)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index c7b5f039d283..bf809086c1e6 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -1788,7 +1788,6 @@ struct clk *tegra_clk_register_pllm(const char
*name, const char *parent_name,
                pll_params->vco_min = pll_params->adjust_vco(pll_params,
                                                             parent_rate);

-       pll_params->flags |= TEGRA_PLL_BYPASS;
        pll_params->flags |= TEGRA_PLLM;
        pll = _tegra_init_pll(clk_base, pmc, pll_params, lock);
        if (IS_ERR(pll))

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

On Mon, Nov 23, 2015 at 03:18:59PM -0800, Tyler Baker wrote:
> Hi Jon,
> 
> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
> > The _clk_disable_pll() function will attempt to place a PLL into bypass
> > if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
> > by clearing the enable bit. To place the PLL into bypass, the bypass bit
> > needs to be set and not cleared. Fix this by setting the bypass bit and
> > not clearing it.
> >
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
> in the tegra tree. This boot failure has only been observed when
> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
> this boot failure to this commit, and I confirmed reverting it on top
> of the tegra for-next branch resolves the issue. The ramdisk[4] used
> for booting is loaded with the modules from the build. It appears to
> me that as the modules are being loaded in userspace by eudev the
> jetson-tk1 locks up. I've sifted through the console logs a bit, and
> found this splat to be most interesting[5].  Can you confirm this
> issue on your end?

Just to close the loop on this: we've discussed this on IRC and came to
the conclusion that not using the bypass mode is safer (switching into
and out of bypass can glitch). I've dropped this patch for now and Jon
will be looking into a second revision of the patch which, in addition
to fixing bypass (the fix is legit, it just happens to break because of
the glitch, most likely), will also remove the BYPASS flag setting so
that bypass will not be used.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Tyler Baker]

On 25 November 2015 at 07:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Nov 23, 2015 at 03:18:59PM -0800, Tyler Baker wrote:
>> Hi Jon,
>>
>> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
>> > The _clk_disable_pll() function will attempt to place a PLL into bypass
>> > if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
>> > by clearing the enable bit. To place the PLL into bypass, the bypass bit
>> > needs to be set and not cleared. Fix this by setting the bypass bit and
>> > not clearing it.
>> >
>> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>
>> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
>> in the tegra tree. This boot failure has only been observed when
>> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
>> this boot failure to this commit, and I confirmed reverting it on top
>> of the tegra for-next branch resolves the issue. The ramdisk[4] used
>> for booting is loaded with the modules from the build. It appears to
>> me that as the modules are being loaded in userspace by eudev the
>> jetson-tk1 locks up. I've sifted through the console logs a bit, and
>> found this splat to be most interesting[5].  Can you confirm this
>> issue on your end?
>
> Just to close the loop on this: we've discussed this on IRC and came to
> the conclusion that not using the bypass mode is safer (switching into
> and out of bypass can glitch). I've dropped this patch for now and Jon
> will be looking into a second revision of the patch which, in addition
> to fixing bypass (the fix is legit, it just happens to break because of
> the glitch, most likely), will also remove the BYPASS flag setting so
> that bypass will not be used.

Thanks for the update, I appreciate you guys looking into this issue.
Please CC me on any fixes, I can re-test and give my tested-by.

Cheers,

Tyler

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Jon Hunter]

On 25/11/15 15:52, Tyler Baker wrote:
> On 25 November 2015 at 07:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>> On Mon, Nov 23, 2015 at 03:18:59PM -0800, Tyler Baker wrote:
>>> Hi Jon,
>>>
>>> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> The _clk_disable_pll() function will attempt to place a PLL into bypass
>>>> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
>>>> by clearing the enable bit. To place the PLL into bypass, the bypass bit
>>>> needs to be set and not cleared. Fix this by setting the bypass bit and
>>>> not clearing it.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>
>>> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
>>> in the tegra tree. This boot failure has only been observed when
>>> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
>>> this boot failure to this commit, and I confirmed reverting it on top
>>> of the tegra for-next branch resolves the issue. The ramdisk[4] used
>>> for booting is loaded with the modules from the build. It appears to
>>> me that as the modules are being loaded in userspace by eudev the
>>> jetson-tk1 locks up. I've sifted through the console logs a bit, and
>>> found this splat to be most interesting[5].  Can you confirm this
>>> issue on your end?
>>
>> Just to close the loop on this: we've discussed this on IRC and came to
>> the conclusion that not using the bypass mode is safer (switching into
>> and out of bypass can glitch). I've dropped this patch for now and Jon
>> will be looking into a second revision of the patch which, in addition
>> to fixing bypass (the fix is legit, it just happens to break because of
>> the glitch, most likely), will also remove the BYPASS flag setting so
>> that bypass will not be used.
> 
> Thanks for the update, I appreciate you guys looking into this issue.
> Please CC me on any fixes, I can re-test and give my tested-by.

No problem. On 2nd thoughts I am wondering if there is any value in
bypassing the PLL when disabling it. I will look into this and see what
I find out.

Cheers
Jon

Re: [PATCH] clk: tegra: Fix bypassing of PLLs

[Jon Hunter]

On 25/11/15 17:48, Jon Hunter wrote:
> 
> On 25/11/15 15:52, Tyler Baker wrote:
>> On 25 November 2015 at 07:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Mon, Nov 23, 2015 at 03:18:59PM -0800, Tyler Baker wrote:
>>>> Hi Jon,
>>>>
>>>> On 20 November 2015 at 07:11, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> The _clk_disable_pll() function will attempt to place a PLL into bypass
>>>>> if the TEGRA_PLL_BYPASS is specified for the PLL and then disable the PLL
>>>>> by clearing the enable bit. To place the PLL into bypass, the bypass bit
>>>>> needs to be set and not cleared. Fix this by setting the bypass bit and
>>>>> not clearing it.
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>
>>>> The kernelci.org bot recently detected a jetson-tk1 boot failure[1][2]
>>>> in the tegra tree. This boot failure has only been observed when
>>>> booting with a multi_v7_defconfig kernel variant. The bot bisected[3]
>>>> this boot failure to this commit, and I confirmed reverting it on top
>>>> of the tegra for-next branch resolves the issue. The ramdisk[4] used
>>>> for booting is loaded with the modules from the build. It appears to
>>>> me that as the modules are being loaded in userspace by eudev the
>>>> jetson-tk1 locks up. I've sifted through the console logs a bit, and
>>>> found this splat to be most interesting[5].  Can you confirm this
>>>> issue on your end?
>>>
>>> Just to close the loop on this: we've discussed this on IRC and came to
>>> the conclusion that not using the bypass mode is safer (switching into
>>> and out of bypass can glitch). I've dropped this patch for now and Jon
>>> will be looking into a second revision of the patch which, in addition
>>> to fixing bypass (the fix is legit, it just happens to break because of
>>> the glitch, most likely), will also remove the BYPASS flag setting so
>>> that bypass will not be used.
>>
>> Thanks for the update, I appreciate you guys looking into this issue.
>> Please CC me on any fixes, I can re-test and give my tested-by.
> 
> No problem. On 2nd thoughts I am wondering if there is any value in
> bypassing the PLL when disabling it. I will look into this and see what
> I find out.

So I got some feedback saying that setting the bypass bit is not
glitch-less for most PLLs and so we should avoid setting this. It may
look a bit odd from reviewing the code, but it is clear to me know. So
we should just drop this change. However, if you like we could add a
comment to document why we are doing this.

Cheers
Jon