[1/4] clk: tegra: Enable fuse clock on Tegra124
diff mbox series

Message ID 20191001211346.104400-1-swarren@wwwdotorg.org
State New
Headers show
Series
  • [1/4] clk: tegra: Enable fuse clock on Tegra124
Related show

Commit Message

Stephen Warren Oct. 1, 2019, 9:13 p.m. UTC
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(+)

Comments

Thierry Reding Oct. 2, 2019, 11:04 a.m. UTC | #1
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);
 }
Stephen Warren Oct. 2, 2019, 8:59 p.m. UTC | #2
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),
Dmitry Osipenko Oct. 3, 2019, 11:23 a.m. UTC | #3
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.
Stephen Warren Oct. 3, 2019, 4:28 p.m. UTC | #4
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.
Thierry Reding Oct. 4, 2019, 12:18 p.m. UTC | #5
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
Stephen Warren Oct. 4, 2019, 4:07 p.m. UTC | #6
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.

Patch
diff mbox series

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 },