[PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

For a little over a year, U-Boot has configured the flow controller to
perform automatic RAM re-repair on off->on power transitions of the CPU
rail1]. This is mandatory for correct operation of Tegra124. However, RAM
re-repair relies on certain clocks, which the kernel must enable and
leave running. The fuse clock is one of those clocks. Enable this clock
so that LP1 power mode (system suspend) operates correctly.

[1] 3cc7942a4ae5 ARM: tegra: implement RAM repair

Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
Cc: stable@vger.kernel.org
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/clk/tegra/clk-tegra124.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
index 0224fdc4766f..f53f6315c646 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
 };
 
 static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
+	{ TEGRA124_CLK_FUSE, -1, 0, 1 },
 	{ TEGRA124_CLK_SOC_THERM, TEGRA124_CLK_PLL_P, 51000000, 0 },
 	{ TEGRA124_CLK_CCLK_G, TEGRA124_CLK_CLK_MAX, 0, 1 },
 	{ TEGRA124_CLK_HDA, TEGRA124_CLK_PLL_P, 102000000, 0 },
-- 
2.23.0

[PATCH 2/4] ARM: tegra: Enable PLLP bypass during Tegra124 LP1

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

For a little over a year, U-Boot has configured the flow controller to
perform automatic RAM re-repair on off->on power transitions of the CPU
rail1]. This is mandatory for correct operation of Tegra124. However, RAM
re-repair relies on certain clocks, which the kernel must enable and
leave running. PLLP is one of those clocks. This clock is shut down
during LP1 in order to save power. Enable bypass (which I believe routes
osc_div_clk, essentially the crystal clock, to the PLL output) so that
this clock signal toggles even though the PLL is not active. This is
required so that LP1 power mode (system suspend) operates correctly.

The bypass configuration must then be undone when resuming from LP1, so
that all peripheral clocks run at the expected rate. Without this, many
peripherals won't work correctly; for example, the UART baud rate would
be incorrect.

NVIDIA's downstream kernel code only does this if not compiled for
Tegra30, so the added code is made conditional upon the chip ID. NVIDIA's
downstream code makes this change conditional upon the active CPU
cluster. The upstream kernel currently doesn't support cluster switching,
so this patch doesn't test the active CPU cluster ID.

[1] 3cc7942a4ae5 ARM: tegra: implement RAM repair

Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
Cc: stable@vger.kernel.org
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/sleep-tegra30.S | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
index b408fa56eb89..6922dd8d3e2d 100644
--- a/arch/arm/mach-tegra/sleep-tegra30.S
+++ b/arch/arm/mach-tegra/sleep-tegra30.S
@@ -370,6 +370,14 @@ _pll_m_c_x_done:
 	pll_locked r1, r0, CLK_RESET_PLLC_BASE
 	pll_locked r1, r0, CLK_RESET_PLLX_BASE
 
+	tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
+	cmp	r1, #TEGRA30
+	beq	1f
+	ldr	r1, [r0, #CLK_RESET_PLLP_BASE]
+	bic	r1, r1, #(1<<31)	@ disable PllP bypass
+	str	r1, [r0, #CLK_RESET_PLLP_BASE]
+1:
+
 	mov32	r7, TEGRA_TMRUS_BASE
 	ldr	r1, [r7]
 	add	r1, r1, #LOCK_DELAY
@@ -630,7 +638,10 @@ tegra30_switch_cpu_to_clk32k:
 	str	r0, [r4, #PMC_PLLP_WB0_OVERRIDE]
 
 	/* disable PLLP, PLLA, PLLC and PLLX */
+	tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
+	cmp	r1, #TEGRA30
 	ldr	r0, [r5, #CLK_RESET_PLLP_BASE]
+	orrne	r0, r0, #(1 << 31)	@ enable PllP bypass on fast cluster
 	bic	r0, r0, #(1 << 30)
 	str	r0, [r5, #CLK_RESET_PLLP_BASE]
 	ldr	r0, [r5, #CLK_RESET_PLLA_BASE]
-- 
2.23.0

[PATCH 3/4] ARM: tegra: modify reshift divider during LP1

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The reshift hardware module implements the RAM re-repair process. This
module uses PLLP as an input clock during LP1 resume. The input divider
for this clock is typically set for PLLP's normal rate. During LP1
resume, PLLP is bypassed and so runs at the crystal rate, which is much
slower. Consequently, decrease the divider so that the reshift module
runs at a reasonable rate during LP1 resume.

NVIDIA's downstream kernel code only does this if not compiled for
Tegra30, so the added code is made conditional upon the chip ID.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/sleep-tegra30.S | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
index 6922dd8d3e2d..6191f9456288 100644
--- a/arch/arm/mach-tegra/sleep-tegra30.S
+++ b/arch/arm/mach-tegra/sleep-tegra30.S
@@ -59,6 +59,9 @@
 #define CLK_RESET_PLLX_MISC3_IDDQ	3
 #define CLK_RESET_PLLM_MISC_IDDQ	5
 #define CLK_RESET_PLLC_MISC_IDDQ	26
+#define CLK_RESET_PLLP_RESHIFT		0x528
+#define CLK_RESET_PLLP_RESHIFT_DEFAULT	0x3b
+#define CLK_RESET_PLLP_RESHIFT_ENABLE	0x3
 
 #define CLK_RESET_CLK_SOURCE_MSELECT	0x3b4
 
@@ -373,9 +376,13 @@ _pll_m_c_x_done:
 	tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
 	cmp	r1, #TEGRA30
 	beq	1f
+
 	ldr	r1, [r0, #CLK_RESET_PLLP_BASE]
 	bic	r1, r1, #(1<<31)	@ disable PllP bypass
 	str	r1, [r0, #CLK_RESET_PLLP_BASE]
+
+	mov	r1, #CLK_RESET_PLLP_RESHIFT_DEFAULT
+	str	r1, [r0, #CLK_RESET_PLLP_RESHIFT]
 1:
 
 	mov32	r7, TEGRA_TMRUS_BASE
@@ -644,6 +651,10 @@ tegra30_switch_cpu_to_clk32k:
 	orrne	r0, r0, #(1 << 31)	@ enable PllP bypass on fast cluster
 	bic	r0, r0, #(1 << 30)
 	str	r0, [r5, #CLK_RESET_PLLP_BASE]
+	beq	1f
+	mov	r0, #CLK_RESET_PLLP_RESHIFT_ENABLE
+	str	r0, [r5, #CLK_RESET_PLLP_RESHIFT]
+1:
 	ldr	r0, [r5, #CLK_RESET_PLLA_BASE]
 	bic	r0, r0, #(1 << 30)
 	str	r0, [r5, #CLK_RESET_PLLA_BASE]
-- 
2.23.0

PATCH 4/4] ARM: tegra: use clk_m CPU on Tegra124 LP1 resume

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Configure the clock controller to set an alternate clock for the CPU when
it receives an IRQ during LP1 (system suspend). Specifically, use clk_m
(the crystal) rather than clk_s (a 32KHz clock). Such an IRQ will be the
LP1 wake event. This reduces the amount of time taken to resume from LP1.

NVIDIA's downstream kernel executes this code on both Tegra30 and
Tegra124, so it appears OK to make this change unconditionally.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/mach-tegra/sleep-tegra30.S | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
index 6191f9456288..ba5e9c07d1b6 100644
--- a/arch/arm/mach-tegra/sleep-tegra30.S
+++ b/arch/arm/mach-tegra/sleep-tegra30.S
@@ -670,8 +670,12 @@ tegra30_switch_cpu_to_clk32k:
 	pll_iddq_entry r1, r5, CLK_RESET_PLLX_MISC3, CLK_RESET_PLLX_MISC3_IDDQ
 _no_pll_in_iddq:
 
-	/* switch to CLKS */
-	mov	r0, #0	/* brust policy = 32KHz */
+	/*
+	 * Switch to clk_s (32KHz); bits 28:31=0
+	 * Enable burst on CPU IRQ; bit 24=1
+	 * Set IRQ burst clock source to clk_m; bits 10:8=0
+	 */
+	mov	r0, #(1 << 24)
 	str	r0, [r5, #CLK_RESET_SCLK_BURST]
 
 	ret	lr
-- 
2.23.0

Re: [PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Thierry Reding]

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

On Tue, Oct 01, 2019 at 03:13:43PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> For a little over a year, U-Boot has configured the flow controller to
> perform automatic RAM re-repair on off->on power transitions of the CPU
> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
> re-repair relies on certain clocks, which the kernel must enable and
> leave running. The fuse clock is one of those clocks. Enable this clock
> so that LP1 power mode (system suspend) operates correctly.
> 
> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
> 
> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/clk/tegra/clk-tegra124.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> index 0224fdc4766f..f53f6315c646 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
>  };
>  
>  static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
> +	{ TEGRA124_CLK_FUSE, -1, 0, 1 },

I think the correct way to do this these days is to mark the clock as
CRITICAL. Not sure if there's an easy way to do that given that the
clock init table doesn't allow storing flags.

Do you have any good ideas on how to achieve this with the critical flag
instead of forcing the refcount to 1?

Perhaps something like the below would work?

Thierry

--- >8 ---
diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
index 0224fdc4766f..bba12d8308d3 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -838,7 +838,7 @@ static struct tegra_clk tegra124_clks[tegra_clk_max] __initdata = {
 	[tegra_clk_spdif_out] = { .dt_id = TEGRA124_CLK_SPDIF_OUT, .present = true },
 	[tegra_clk_vi_9] = { .dt_id = TEGRA124_CLK_VI, .present = true },
 	[tegra_clk_vi_sensor_8] = { .dt_id = TEGRA124_CLK_VI_SENSOR, .present = true },
-	[tegra_clk_fuse] = { .dt_id = TEGRA124_CLK_FUSE, .present = true },
+	[tegra_clk_fuse] = { .dt_id = TEGRA124_CLK_FUSE, .present = false },
 	[tegra_clk_fuse_burn] = { .dt_id = TEGRA124_CLK_FUSE_BURN, .present = true },
 	[tegra_clk_clk_32k] = { .dt_id = TEGRA124_CLK_CLK_32K, .present = true },
 	[tegra_clk_clk_m] = { .dt_id = TEGRA124_CLK_CLK_M, .present = true },
@@ -1033,6 +1033,12 @@ static __init void tegra124_periph_clk_init(void __iomem *clk_base,
 	clk_register_clkdev(clk, "cml1", NULL);
 	clks[TEGRA124_CLK_CML1] = clk;
 
+	clk = tegra_clk_register_periph_gate("fuse", "clk_m",
+					     TEGRA_PERIPH_ON_APB, clk_base,
+					     CLK_IS_CRITICAL, 39,
+					     periph_clk_enb_refcnt);
+	clks[TEGRA124_CLK_FUSE] = clk;
+
 	tegra_periph_clk_init(clk_base, pmc_base, tegra124_clks, &pll_p_params);
 }
 

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

Re: [PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Stephen Warren]

On 10/2/19 5:04 AM, Thierry Reding wrote:
> On Tue, Oct 01, 2019 at 03:13:43PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> For a little over a year, U-Boot has configured the flow controller to
>> perform automatic RAM re-repair on off->on power transitions of the CPU
>> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
>> re-repair relies on certain clocks, which the kernel must enable and
>> leave running. The fuse clock is one of those clocks. Enable this clock
>> so that LP1 power mode (system suspend) operates correctly.
>>
>> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
>>
>> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>   drivers/clk/tegra/clk-tegra124.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
>> index 0224fdc4766f..f53f6315c646 100644
>> --- a/drivers/clk/tegra/clk-tegra124.c
>> +++ b/drivers/clk/tegra/clk-tegra124.c
>> @@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
>>   };
>>   
>>   static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
>> +	{ TEGRA124_CLK_FUSE, -1, 0, 1 },
> 
> I think the correct way to do this these days is to mark the clock as
> CRITICAL. Not sure if there's an easy way to do that given that the
> clock init table doesn't allow storing flags.
> 
> Do you have any good ideas on how to achieve this with the critical flag
> instead of forcing the refcount to 1?
> 
> Perhaps something like the below would work?
 > ...

The following works for me; does this seem like a reasonable approach? 
It does set the critical flag for all SoCs, including any that don't 
require RAM re-repair. I'm not sure which do; I know it's more than just 
Tegra124, but I'm not sure how far back/forward the requirement goes.

> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
> index 1ed85f120a1b..76dd91eebd13 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -785,7 +785,7 @@ static struct tegra_periph_init_data gate_clks[] = {
>         GATE("ahbdma", "hclk", 33, 0, tegra_clk_ahbdma, 0),
>         GATE("apbdma", "pclk", 34, 0, tegra_clk_apbdma, 0),
>         GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
> -       GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
> +       GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, CLK_IS_CRITICAL),
>         GATE("fuse_burn", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse_burn, 0),
>         GATE("kfuse", "clk_m", 40, TEGRA_PERIPH_ON_APB, tegra_clk_kfuse, 0),
>         GATE("apbif", "clk_m", 107, TEGRA_PERIPH_ON_APB, tegra_clk_apbif, 0),

Re: [PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Dmitry Osipenko]

02.10.2019 00:13, Stephen Warren пишет:
> From: Stephen Warren <swarren@nvidia.com>
> 
> For a little over a year, U-Boot has configured the flow controller to
> perform automatic RAM re-repair on off->on power transitions of the CPU
> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
> re-repair relies on certain clocks, which the kernel must enable and
> leave running. The fuse clock is one of those clocks. Enable this clock
> so that LP1 power mode (system suspend) operates correctly.
> 
> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
> 
> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/clk/tegra/clk-tegra124.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> index 0224fdc4766f..f53f6315c646 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
>  };
>  
>  static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
> +	{ TEGRA124_CLK_FUSE, -1, 0, 1 },
>  	{ TEGRA124_CLK_SOC_THERM, TEGRA124_CLK_PLL_P, 51000000, 0 },
>  	{ TEGRA124_CLK_CCLK_G, TEGRA124_CLK_CLK_MAX, 0, 1 },
>  	{ TEGRA124_CLK_HDA, TEGRA124_CLK_PLL_P, 102000000, 0 },
> 

Hello Stephen,

Does this mean that devices which are using older U-Boot version were always in a trouble?
It sounds to me that the RAM re-repair should be also enabled by the kernel's flow
controller driver in a case if bootloader did not enable it.
If enabling RAM re-repair is a change that won't be easily backportable to stable kernels,
then may be it's worth to simply force-disable LP1 on T124 for the older kernels.

Re: [PATCH 2/4] ARM: tegra: Enable PLLP bypass during Tegra124 LP1

[Dmitry Osipenko]

02.10.2019 00:13, Stephen Warren пишет:
> From: Stephen Warren <swarren@nvidia.com>
> 
> For a little over a year, U-Boot has configured the flow controller to
> perform automatic RAM re-repair on off->on power transitions of the CPU
> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
> re-repair relies on certain clocks, which the kernel must enable and
> leave running. PLLP is one of those clocks. This clock is shut down
> during LP1 in order to save power. Enable bypass (which I believe routes
> osc_div_clk, essentially the crystal clock, to the PLL output) so that
> this clock signal toggles even though the PLL is not active. This is
> required so that LP1 power mode (system suspend) operates correctly.
> 
> The bypass configuration must then be undone when resuming from LP1, so
> that all peripheral clocks run at the expected rate. Without this, many
> peripherals won't work correctly; for example, the UART baud rate would
> be incorrect.
> 
> NVIDIA's downstream kernel code only does this if not compiled for
> Tegra30, so the added code is made conditional upon the chip ID. NVIDIA's
> downstream code makes this change conditional upon the active CPU
> cluster. The upstream kernel currently doesn't support cluster switching,
> so this patch doesn't test the active CPU cluster ID.
> 
> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
> 
> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  arch/arm/mach-tegra/sleep-tegra30.S | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
> index b408fa56eb89..6922dd8d3e2d 100644
> --- a/arch/arm/mach-tegra/sleep-tegra30.S
> +++ b/arch/arm/mach-tegra/sleep-tegra30.S
> @@ -370,6 +370,14 @@ _pll_m_c_x_done:
>  	pll_locked r1, r0, CLK_RESET_PLLC_BASE
>  	pll_locked r1, r0, CLK_RESET_PLLX_BASE
>  
> +	tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
> +	cmp	r1, #TEGRA30
> +	beq	1f

What about T114, or does it need enabled PLLP as well?

> +	ldr	r1, [r0, #CLK_RESET_PLLP_BASE]
> +	bic	r1, r1, #(1<<31)	@ disable PllP bypass
> +	str	r1, [r0, #CLK_RESET_PLLP_BASE]
> +1:
> +
>  	mov32	r7, TEGRA_TMRUS_BASE
>  	ldr	r1, [r7]
>  	add	r1, r1, #LOCK_DELAY
> @@ -630,7 +638,10 @@ tegra30_switch_cpu_to_clk32k:
>  	str	r0, [r4, #PMC_PLLP_WB0_OVERRIDE]
>  
>  	/* disable PLLP, PLLA, PLLC and PLLX */
> +	tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
> +	cmp	r1, #TEGRA30
>  	ldr	r0, [r5, #CLK_RESET_PLLP_BASE]
> +	orrne	r0, r0, #(1 << 31)	@ enable PllP bypass on fast cluster
>  	bic	r0, r0, #(1 << 30)
>  	str	r0, [r5, #CLK_RESET_PLLP_BASE]
>  	ldr	r0, [r5, #CLK_RESET_PLLA_BASE]
> 

Re: [PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Stephen Warren]

On 10/3/19 5:23 AM, Dmitry Osipenko wrote:
> 02.10.2019 00:13, Stephen Warren пишет:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> For a little over a year, U-Boot has configured the flow controller to
>> perform automatic RAM re-repair on off->on power transitions of the CPU
>> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
>> re-repair relies on certain clocks, which the kernel must enable and
>> leave running. The fuse clock is one of those clocks. Enable this clock
>> so that LP1 power mode (system suspend) operates correctly.
>>
>> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
>>
>> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>   drivers/clk/tegra/clk-tegra124.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
>> index 0224fdc4766f..f53f6315c646 100644
>> --- a/drivers/clk/tegra/clk-tegra124.c
>> +++ b/drivers/clk/tegra/clk-tegra124.c
>> @@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
>>   };
>>   
>>   static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
>> +	{ TEGRA124_CLK_FUSE, -1, 0, 1 },
>>   	{ TEGRA124_CLK_SOC_THERM, TEGRA124_CLK_PLL_P, 51000000, 0 },
>>   	{ TEGRA124_CLK_CCLK_G, TEGRA124_CLK_CLK_MAX, 0, 1 },
>>   	{ TEGRA124_CLK_HDA, TEGRA124_CLK_PLL_P, 102000000, 0 },
>>
> 
> Hello Stephen,
> 
> Does this mean that devices which are using older U-Boot version were always in a trouble?

Yes. RAM re-repair wouldn't have been enabled, so in theory any device 
could fail after an LP1 resume, or indeed anything that caused the CPU 
complex rail to be gated.

> It sounds to me that the RAM re-repair should be also enabled by the kernel's flow
> controller driver in a case if bootloader did not enable it.

Yes, that might be a good idea too.

> If enabling RAM re-repair is a change that won't be easily backportable to stable kernels,
> then may be it's worth to simply force-disable LP1 on T124 for the older kernels.

The first two patches in this series are all that's strictly required, 
and the change are pretty small and isolated, so it should be easy to 
back-port.

Re: [PATCH 2/4] ARM: tegra: Enable PLLP bypass during Tegra124 LP1

[Stephen Warren]

On 10/3/19 5:27 AM, Dmitry Osipenko wrote:
> 02.10.2019 00:13, Stephen Warren пишет:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> For a little over a year, U-Boot has configured the flow controller to
>> perform automatic RAM re-repair on off->on power transitions of the CPU
>> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
>> re-repair relies on certain clocks, which the kernel must enable and
>> leave running. PLLP is one of those clocks. This clock is shut down
>> during LP1 in order to save power. Enable bypass (which I believe routes
>> osc_div_clk, essentially the crystal clock, to the PLL output) so that
>> this clock signal toggles even though the PLL is not active. This is
>> required so that LP1 power mode (system suspend) operates correctly.
>>
>> The bypass configuration must then be undone when resuming from LP1, so
>> that all peripheral clocks run at the expected rate. Without this, many
>> peripherals won't work correctly; for example, the UART baud rate would
>> be incorrect.
>>
>> NVIDIA's downstream kernel code only does this if not compiled for
>> Tegra30, so the added code is made conditional upon the chip ID. NVIDIA's
>> downstream code makes this change conditional upon the active CPU
>> cluster. The upstream kernel currently doesn't support cluster switching,
>> so this patch doesn't test the active CPU cluster ID.
>>
>> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
>>
>> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>   arch/arm/mach-tegra/sleep-tegra30.S | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
>> index b408fa56eb89..6922dd8d3e2d 100644
>> --- a/arch/arm/mach-tegra/sleep-tegra30.S
>> +++ b/arch/arm/mach-tegra/sleep-tegra30.S
>> @@ -370,6 +370,14 @@ _pll_m_c_x_done:
>>   	pll_locked r1, r0, CLK_RESET_PLLC_BASE
>>   	pll_locked r1, r0, CLK_RESET_PLLX_BASE
>>   
>> +	tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
>> +	cmp	r1, #TEGRA30
>> +	beq	1f
> 
> What about T114, or does it need enabled PLLP as well?

I'm nowhere near as familiar with T114 as T124, so I can't be 100% sure. 
However, a very quick look at the CAR section in the T114 TRM does show 
the same gate/mux structure around a reshift and fuse clock, so I assume 
the requirement is identical there.

Also, NVIDIA's downstream kernel has a compile-time ifdef around the 
code I've added here. It's not compiled for T30 specifically, and is 
compiled for anything else, which I believe means both T114 and T124.

In patch 1 in this series, I only enabled the fuse clock for T124, since 
I don't have a T114 system to test any more. However, the revised patch 
1 that Thierry and I are discussing would enable the fuse clock on all 
SoCs, and hence make the code work identically on T114 as it does on T124.

Re: [PATCH 2/4] ARM: tegra: Enable PLLP bypass during Tegra124 LP1

[Dmitry Osipenko]

03.10.2019 19:34, Stephen Warren пишет:
> On 10/3/19 5:27 AM, Dmitry Osipenko wrote:
>> 02.10.2019 00:13, Stephen Warren пишет:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> For a little over a year, U-Boot has configured the flow controller to
>>> perform automatic RAM re-repair on off->on power transitions of the CPU
>>> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
>>> re-repair relies on certain clocks, which the kernel must enable and
>>> leave running. PLLP is one of those clocks. This clock is shut down
>>> during LP1 in order to save power. Enable bypass (which I believe routes
>>> osc_div_clk, essentially the crystal clock, to the PLL output) so that
>>> this clock signal toggles even though the PLL is not active. This is
>>> required so that LP1 power mode (system suspend) operates correctly.
>>>
>>> The bypass configuration must then be undone when resuming from LP1, so
>>> that all peripheral clocks run at the expected rate. Without this, many
>>> peripherals won't work correctly; for example, the UART baud rate would
>>> be incorrect.
>>>
>>> NVIDIA's downstream kernel code only does this if not compiled for
>>> Tegra30, so the added code is made conditional upon the chip ID. NVIDIA's
>>> downstream code makes this change conditional upon the active CPU
>>> cluster. The upstream kernel currently doesn't support cluster switching,
>>> so this patch doesn't test the active CPU cluster ID.
>>>
>>> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
>>>
>>> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>>   arch/arm/mach-tegra/sleep-tegra30.S | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S
>>> index b408fa56eb89..6922dd8d3e2d 100644
>>> --- a/arch/arm/mach-tegra/sleep-tegra30.S
>>> +++ b/arch/arm/mach-tegra/sleep-tegra30.S
>>> @@ -370,6 +370,14 @@ _pll_m_c_x_done:
>>>       pll_locked r1, r0, CLK_RESET_PLLC_BASE
>>>       pll_locked r1, r0, CLK_RESET_PLLX_BASE
>>>   +    tegra_get_soc_id TEGRA_APB_MISC_BASE, r1
>>> +    cmp    r1, #TEGRA30
>>> +    beq    1f
>>
>> What about T114, or does it need enabled PLLP as well?
> 
> I'm nowhere near as familiar with T114 as T124, so I can't be 100% sure. However, a very
> quick look at the CAR section in the T114 TRM does show the same gate/mux structure around
> a reshift and fuse clock, so I assume the requirement is identical there.

Indeed, T114 TRM suggests that it has the same requirement.

> Also, NVIDIA's downstream kernel has a compile-time ifdef around the code I've added here.
> It's not compiled for T30 specifically, and is compiled for anything else, which I believe
> means both T114 and T124.
> 
> In patch 1 in this series, I only enabled the fuse clock for T124, since I don't have a
> T114 system to test any more. However, the revised patch 1 that Thierry and I are
> discussing would enable the fuse clock on all SoCs, and hence make the code work
> identically on T114 as it does on T124.

Thanks for the clarification.

Re: [PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Thierry Reding]

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

On Wed, Oct 02, 2019 at 02:59:03PM -0600, Stephen Warren wrote:
> On 10/2/19 5:04 AM, Thierry Reding wrote:
> > On Tue, Oct 01, 2019 at 03:13:43PM -0600, Stephen Warren wrote:
> > > From: Stephen Warren <swarren@nvidia.com>
> > > 
> > > For a little over a year, U-Boot has configured the flow controller to
> > > perform automatic RAM re-repair on off->on power transitions of the CPU
> > > rail1]. This is mandatory for correct operation of Tegra124. However, RAM
> > > re-repair relies on certain clocks, which the kernel must enable and
> > > leave running. The fuse clock is one of those clocks. Enable this clock
> > > so that LP1 power mode (system suspend) operates correctly.
> > > 
> > > [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
> > > 
> > > Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > > ---
> > >   drivers/clk/tegra/clk-tegra124.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> > > index 0224fdc4766f..f53f6315c646 100644
> > > --- a/drivers/clk/tegra/clk-tegra124.c
> > > +++ b/drivers/clk/tegra/clk-tegra124.c
> > > @@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
> > >   };
> > >   static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
> > > +	{ TEGRA124_CLK_FUSE, -1, 0, 1 },
> > 
> > I think the correct way to do this these days is to mark the clock as
> > CRITICAL. Not sure if there's an easy way to do that given that the
> > clock init table doesn't allow storing flags.
> > 
> > Do you have any good ideas on how to achieve this with the critical flag
> > instead of forcing the refcount to 1?
> > 
> > Perhaps something like the below would work?
> > ...
> 
> The following works for me; does this seem like a reasonable approach? It
> does set the critical flag for all SoCs, including any that don't require
> RAM re-repair. I'm not sure which do; I know it's more than just Tegra124,
> but I'm not sure how far back/forward the requirement goes.
> 
> > diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
> > index 1ed85f120a1b..76dd91eebd13 100644
> > --- a/drivers/clk/tegra/clk-tegra-periph.c
> > +++ b/drivers/clk/tegra/clk-tegra-periph.c
> > @@ -785,7 +785,7 @@ static struct tegra_periph_init_data gate_clks[] = {
> >         GATE("ahbdma", "hclk", 33, 0, tegra_clk_ahbdma, 0),
> >         GATE("apbdma", "pclk", 34, 0, tegra_clk_apbdma, 0),
> >         GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
> > -       GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
> > +       GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, CLK_IS_CRITICAL),
> >         GATE("fuse_burn", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse_burn, 0),
> >         GATE("kfuse", "clk_m", 40, TEGRA_PERIPH_ON_APB, tegra_clk_kfuse, 0),
> >         GATE("apbif", "clk_m", 107, TEGRA_PERIPH_ON_APB, tegra_clk_apbif, 0),

It's probably fine to do this. The patch I proposed would've restricted
the change to just Tegra124. But if we need this on other generations, I
don't think the extra complexity is justified, especially since I can't
imagine that the FUSE clock remaining always on would consume a lot of
extra power.

Thierry

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

Re: [PATCH 1/4] clk: tegra: Enable fuse clock on Tegra124

[Stephen Warren]

On 10/4/19 6:18 AM, Thierry Reding wrote:
> On Wed, Oct 02, 2019 at 02:59:03PM -0600, Stephen Warren wrote:
>> On 10/2/19 5:04 AM, Thierry Reding wrote:
>>> On Tue, Oct 01, 2019 at 03:13:43PM -0600, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> For a little over a year, U-Boot has configured the flow controller to
>>>> perform automatic RAM re-repair on off->on power transitions of the CPU
>>>> rail1]. This is mandatory for correct operation of Tegra124. However, RAM
>>>> re-repair relies on certain clocks, which the kernel must enable and
>>>> leave running. The fuse clock is one of those clocks. Enable this clock
>>>> so that LP1 power mode (system suspend) operates correctly.
>>>>
>>>> [1] 3cc7942a4ae5 ARM: tegra: implement RAM repair
>>>>
>>>> Reported-by: Jonathan Hunter <jonathanh@nvidia.com>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>> ---
>>>>    drivers/clk/tegra/clk-tegra124.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
>>>> index 0224fdc4766f..f53f6315c646 100644
>>>> --- a/drivers/clk/tegra/clk-tegra124.c
>>>> +++ b/drivers/clk/tegra/clk-tegra124.c
>>>> @@ -1291,6 +1291,7 @@ static struct tegra_clk_init_table common_init_table[] __initdata = {
>>>>    };
>>>>    static struct tegra_clk_init_table tegra124_init_table[] __initdata = {
>>>> +	{ TEGRA124_CLK_FUSE, -1, 0, 1 },
>>>
>>> I think the correct way to do this these days is to mark the clock as
>>> CRITICAL. Not sure if there's an easy way to do that given that the
>>> clock init table doesn't allow storing flags.
>>>
>>> Do you have any good ideas on how to achieve this with the critical flag
>>> instead of forcing the refcount to 1?
>>>
>>> Perhaps something like the below would work?
>>> ...
>>
>> The following works for me; does this seem like a reasonable approach? It
>> does set the critical flag for all SoCs, including any that don't require
>> RAM re-repair. I'm not sure which do; I know it's more than just Tegra124,
>> but I'm not sure how far back/forward the requirement goes.
>>
>>> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
>>> index 1ed85f120a1b..76dd91eebd13 100644
>>> --- a/drivers/clk/tegra/clk-tegra-periph.c
>>> +++ b/drivers/clk/tegra/clk-tegra-periph.c
>>> @@ -785,7 +785,7 @@ static struct tegra_periph_init_data gate_clks[] = {
>>>          GATE("ahbdma", "hclk", 33, 0, tegra_clk_ahbdma, 0),
>>>          GATE("apbdma", "pclk", 34, 0, tegra_clk_apbdma, 0),
>>>          GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
>>> -       GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
>>> +       GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, CLK_IS_CRITICAL),
>>>          GATE("fuse_burn", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse_burn, 0),
>>>          GATE("kfuse", "clk_m", 40, TEGRA_PERIPH_ON_APB, tegra_clk_kfuse, 0),
>>>          GATE("apbif", "clk_m", 107, TEGRA_PERIPH_ON_APB, tegra_clk_apbif, 0),
> 
> It's probably fine to do this. The patch I proposed would've restricted
> the change to just Tegra124. But if we need this on other generations, I
> don't think the extra complexity is justified, especially since I can't
> imagine that the FUSE clock remaining always on would consume a lot of
> extra power.

T114/T124/T132/T210 all require it. T20/T30 I'm not sure since the TRM 
doesn't mention RAM repair, but that could just be missing 
documentation. I think it was introduced in T114 though. The T186 and 
T194s TRM mention RAM repair, but so much changed in those SoCs I'm not 
certain if it works in the same way and hence relies on fuse clock; 
probably though.