ARM: tegra: Fix restoration of PLLM when exiting suspend
diff mbox series

Message ID 20191210103708.7023-1-jonathanh@nvidia.com
State New
Headers show
Series
  • ARM: tegra: Fix restoration of PLLM when exiting suspend
Related show

Commit Message

Jon Hunter Dec. 10, 2019, 10:37 a.m. UTC
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(-)

Comments

Thierry Reding Dec. 10, 2019, 12:09 p.m. UTC | #1
On Tue, Dec 10, 2019 at 10:37:08AM +0000, Jon Hunter wrote:
> 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(-)

Looks good to me. If I understand correctly we really only need this on
v4.4 and earlier because the issue doesn't happen on later kernels
because of that PLLM handling update change that you mentioned, right?

At the same time, this is the correct thing to do even on more recent
kernels because we currently rely on the PLLM status check being absent
for this to work.

So it seems like the safest option going forward is to apply this patch
to all versions, so that we don't rely on any assumptions.

Do you agree?

Thierry

> 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]
> -- 
> 2.17.1
>
Jon Hunter Dec. 10, 2019, 2:29 p.m. UTC | #2
On 10/12/2019 12:09, Thierry Reding wrote:
> On Tue, Dec 10, 2019 at 10:37:08AM +0000, Jon Hunter wrote:
>> 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(-)
> 
> Looks good to me. If I understand correctly we really only need this on
> v4.4 and earlier because the issue doesn't happen on later kernels
> because of that PLLM handling update change that you mentioned, right?

Yes.

> At the same time, this is the correct thing to do even on more recent
> kernels because we currently rely on the PLLM status check being absent
> for this to work.

Yes exactly.

> So it seems like the safest option going forward is to apply this patch
> to all versions, so that we don't rely on any assumptions.
> 
> Do you agree?

Yes, my feeling is that we should apply to mainline and then it should
be picked-up for stable.

Cheers
Jon
Jon Hunter Dec. 10, 2019, 2:32 p.m. UTC | #3
On 10/12/2019 14:29, Jon Hunter wrote:
> 
> On 10/12/2019 12:09, Thierry Reding wrote:
>> On Tue, Dec 10, 2019 at 10:37:08AM +0000, Jon Hunter wrote:
>>> 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(-)
>>
>> Looks good to me. If I understand correctly we really only need this on
>> v4.4 and earlier because the issue doesn't happen on later kernels
>> because of that PLLM handling update change that you mentioned, right?
> 
> Yes.

However, although we don't see any failures so far on mainline, it is
possible for the CCF status for PLLM to be incorrect following suspend.

Jon
Dmitry Osipenko Dec. 10, 2019, 7:28 p.m. UTC | #4
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.
Dmitry Osipenko Dec. 10, 2019, 8:29 p.m. UTC | #5
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.

I'm not sure what's the idea of WB0 overriding, maybe to resume faster.
Peter, could you please clarify that?

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).
Peter De Schrijver Dec. 11, 2019, 8:50 a.m. UTC | #6
On Tue, Dec 10, 2019 at 11:29:42PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 10.12.2019 22:28, Dmitry Osipenko пишет:
> > Hello Jon,
> >
> > 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.
> 
> I'm not sure what's the idea of WB0 overriding, maybe to resume faster.
> Peter, could you please clarify that?

I don't know why these overriding bits exist. The code for them was in
the downstream driver so I implemented the same in the upstream driver
:)

Peter.
Dmitry Osipenko Dec. 12, 2019, 10:18 p.m. UTC | #7
11.12.2019 11:50, Peter De Schrijver пишет:
> On Tue, Dec 10, 2019 at 11:29:42PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 10.12.2019 22:28, Dmitry Osipenko пишет:
>>> Hello Jon,
>>>
>>> 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.
>>
>> I'm not sure what's the idea of WB0 overriding, maybe to resume faster.
>> Peter, could you please clarify that?
> 
> I don't know why these overriding bits exist. The code for them was in
> the downstream driver so I implemented the same in the upstream driver
> :)

Okay, I'll try to figure out how to clean up it properly.
Jon Hunter Dec. 17, 2019, 2:19 p.m. UTC | #8
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.

> 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
Dmitry Osipenko Dec. 17, 2019, 2:28 p.m. UTC | #9
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
>
Dmitry Osipenko June 12, 2020, 3:20 p.m. UTC | #10
17.12.2019 17:28, Dmitry Osipenko пишет:
> 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.

Hello, Jon! Do you have any plans to continue working on this patch? A
day ago I sent out patch that improves PLLM handling within the clk
driver [1], will be great if the resume from suspend could be improved
as well! :)

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/20200610163738.29304-1-digetx@gmail.com/

Patch
diff mbox series

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]