| Message ID | 1448032264-29622-1-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
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>
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)