17.12.2019 17:19, Jon Hunter пишет:
On 10/12/2019 20:29, Dmitry Osipenko wrote:
10.12.2019 22:28, Dmitry Osipenko пишет:
Hello Jon,
10.12.2019 13:37, Jon Hunter пишет:
The suspend entry and exit code for 32-bit Tegra devices assumes that the PLLM (which is used to provide the clock for external memory) is always enabled on entry to suspend. Hence, the current code always disables the PLLM on entry to suspend and re-enables the PLLM on exit from suspend.
Since the introduction of the Tegra124 EMC driver by commit 73a7f0a90641 ("memory: tegra: Add EMC (external memory controller) driver"), which is used to scale the EMC frequency, PLLM may not be the current clock source for the EMC on entry to suspend and hence may not be enabled. Always enabling the PLLM on exit from suspend can cause the actual status on the PLL to be different from that reported by the common clock framework.
On kernels prior to v4.5, the code to set the rate of the PLLM had a test to verify if the PLL was enabled and if the PLL was enabled, setting the rate would fail. Since commit 267b62a96951 ("clk: tegra: pll: Update PLLM handling") the test to see if PLLM is enabled was removed.
With these earlier kernels, if the PLLM is disabled on entering suspend and the EMC driver attempts to set the parent of the EMC clock to the PLLM on exiting suspend, then the set rate for the PLLM will fail and in turn cause the resume to fail.
We should not be re-enabling the PLLM on resume from suspend unless it was enabled on entry to suspend. Therefore, fix this by saving the state of PLLM on entry to suspend and only re-enable it, if it was already enabled.
Fixes: 73a7f0a90641 ("memory: tegra: Add EMC (external memory controller) driver") Cc: stable@vger.kernel.org
Signed-off-by: Jon Hunter jonathanh@nvidia.com
arch/arm/mach-tegra/sleep-tegra30.S | 33 +++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S index 3341a12bbb9c..c2f0793a424f 100644 --- a/arch/arm/mach-tegra/sleep-tegra30.S +++ b/arch/arm/mach-tegra/sleep-tegra30.S @@ -337,26 +337,42 @@ ENTRY(tegra30_lp1_reset) add r1, r1, #2 wait_until r1, r7, r3
- /* enable PLLM via PMC */
- /* restore PLLM state */ mov32 r2, TEGRA_PMC_BASE
- adr r7, tegra_pllm_status
- ldr r1, [r7]
- cmp r2, #(1 << 12)
- bne _skip_pllm
- ldr r1, [r2, #PMC_PLLP_WB0_OVERRIDE] orr r1, r1, #(1 << 12) str r1, [r2, #PMC_PLLP_WB0_OVERRIDE]
pll_enable r1, r0, CLK_RESET_PLLM_BASE, 0
- pll_locked r1, r0, CLK_RESET_PLLM_BASE
+_skip_pllm: pll_enable r1, r0, CLK_RESET_PLLC_BASE, 0 pll_enable r1, r0, CLK_RESET_PLLX_BASE, 0 b _pll_m_c_x_done _no_pll_iddq_exit:
- /* enable PLLM via PMC */
- /* restore PLLM state */ mov32 r2, TEGRA_PMC_BASE
- adr r7, tegra_pllm_status
- ldr r1, [r7]
- cmp r2, #(1 << 12)
- bne _skip_pllm_no_iddq
- ldr r1, [r2, #PMC_PLLP_WB0_OVERRIDE] orr r1, r1, #(1 << 12) str r1, [r2, #PMC_PLLP_WB0_OVERRIDE]
pll_enable r1, r0, CLK_RESET_PLLM_BASE, CLK_RESET_PLLM_MISC
- pll_locked r1, r0, CLK_RESET_PLLM_BASE
+_skip_pllm_no_iddq: pll_enable r1, r0, CLK_RESET_PLLC_BASE, CLK_RESET_PLLC_MISC pll_enable r1, r0, CLK_RESET_PLLX_BASE, CLK_RESET_PLLX_MISC @@ -364,7 +380,6 @@ _pll_m_c_x_done: pll_enable r1, r0, CLK_RESET_PLLP_BASE, CLK_RESET_PLLP_MISC pll_enable r1, r0, CLK_RESET_PLLA_BASE, CLK_RESET_PLLA_MISC
- pll_locked r1, r0, CLK_RESET_PLLM_BASE pll_locked r1, r0, CLK_RESET_PLLP_BASE pll_locked r1, r0, CLK_RESET_PLLA_BASE pll_locked r1, r0, CLK_RESET_PLLC_BASE
@@ -526,6 +541,8 @@ __no_dual_emc_chanl: ENDPROC(tegra30_lp1_reset) .align L1_CACHE_SHIFT +tegra_pllm_status:
- .word 0
tegra30_sdram_pad_address: .word TEGRA_EMC_BASE + EMC_CFG @0x0 .word TEGRA_EMC_BASE + EMC_ZCAL_INTERVAL @0x4 @@ -624,10 +641,14 @@ tegra30_switch_cpu_to_clk32k: add r1, r1, #2 wait_until r1, r7, r9
- /* disable PLLM via PMC in LP1 */
- /* disable PLLM, if enabled, via PMC in LP1 */
- adr r1, tegra_pllm_status ldr r0, [r4, #PMC_PLLP_WB0_OVERRIDE]
- bic r0, r0, #(1 << 12)
- str r0, [r4, #PMC_PLLP_WB0_OVERRIDE]
- and r2, r0, #(1 << 12)
- str r2, [r1]
- cmp r2, #(1 << 12)
- biceq r0, r0, #(1 << 12)
- streq r0, [r4, #PMC_PLLP_WB0_OVERRIDE]
/* disable PLLP, PLLA, PLLC and PLLX */ ldr r0, [r5, #CLK_RESET_PLLP_BASE]
PLLM's enable-status could be defined either by PMC or CaR. Thus at first you need to check whether PMC overrides CaR's enable and then judge the enable state based on PMC or CaR state respectively.
Actually, now I think that it doesn't make sense to check PMC WB0 state at all. IIUC, PLLM's state of the WB0 register defines whether Boot ROM should enable PLLM on resume from suspend. Thus it will be correct to check only the CaR's enable-state of PLLM.
Thanks for pointing this out and sorry for the delay. However, I am not sure I agree that we should not check this at all. If the override bit is set, then we do want to check the state from the PMC register and if it is not then we should just use the PLLM register itself.
Sorry if I wasn't clear.. my point is that the PMC's override register bit doesn't reflect the PLLM's enable-state. The PLLM could be disabled while PMC_PLLP_WB0_OVERRIDE_PLLM_ENABLE bit is set.
The CaR's PLLM enable-state reflects the actual hardware state. At least that's what I see on T30.
Looks like it is a bit of nonsense that clk_pll_is_enabled() checks PMC_PLLP_WB0_OVERRIDE_PLLM_ENABLE for judging of the enable-state. This is not the first time I'm getting confused by it, perhaps will be worthwhile to clean up that part of the clk driver's code (if I'm not missing something).
That code looks fine to me. I just think this code entering and exiting suspend needs to be fixed. I will re-work this fix.
Jon