linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time
@ 2019-07-07 23:08 Dmitry Osipenko
  2019-07-18  9:45 ` Jon Hunter
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Osipenko @ 2019-07-07 23:08 UTC (permalink / raw)
  To: Thierry Reding, Jonathan Hunter, Peter De Schrijver
  Cc: linux-tegra, linux-kernel

The PCLK clock is running off SCLK, which is a critical clock that is
very unlikely to randomly change its rate. It's also a bit clumsy (and
apparently incorrect) to query the clock's rate with interrupts being
disabled because clk_get_rate() takes a mutex and that's the case during
suspend/cpuidle entering. Lastly, it's better to always fully reprogram
PMC state because it's not obvious whether it could be changed after SC7.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/pmc.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 9f9c1c677cf4..532e0ada012b 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1433,6 +1433,7 @@ void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode)
 void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
 {
 	unsigned long long rate = 0;
+	u64 ticks;
 	u32 value;
 
 	switch (mode) {
@@ -1441,7 +1442,7 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
 		break;
 
 	case TEGRA_SUSPEND_LP2:
-		rate = clk_get_rate(pmc->clk);
+		rate = pmc->rate;
 		break;
 
 	default:
@@ -1451,26 +1452,20 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
 	if (WARN_ON_ONCE(rate == 0))
 		rate = 100000000;
 
-	if (rate != pmc->rate) {
-		u64 ticks;
-
-		ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
-		do_div(ticks, USEC_PER_SEC);
-		tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
-
-		ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
-		do_div(ticks, USEC_PER_SEC);
-		tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
+	ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
+	do_div(ticks, USEC_PER_SEC);
+	tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
 
-		wmb();
-
-		pmc->rate = rate;
-	}
+	ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
+	do_div(ticks, USEC_PER_SEC);
+	tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
 
 	value = tegra_pmc_readl(pmc, PMC_CNTRL);
 	value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
 	value |= PMC_CNTRL_CPU_PWRREQ_OE;
 	tegra_pmc_writel(pmc, value, PMC_CNTRL);
+
+	wmb();
 }
 #endif
 
@@ -2082,6 +2077,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		pmc->clk = NULL;
 	}
 
+	pmc->rate = clk_get_rate(pmc->clk);
 	pmc->dev = &pdev->dev;
 
 	tegra_pmc_init(pmc);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time
  2019-07-07 23:08 [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time Dmitry Osipenko
@ 2019-07-18  9:45 ` Jon Hunter
  2019-07-18  9:53   ` Jon Hunter
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jon Hunter @ 2019-07-18  9:45 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding, Peter De Schrijver
  Cc: linux-tegra, linux-kernel


On 08/07/2019 00:08, Dmitry Osipenko wrote:
> The PCLK clock is running off SCLK, which is a critical clock that is
> very unlikely to randomly change its rate. It's also a bit clumsy (and
> apparently incorrect) to query the clock's rate with interrupts being
> disabled because clk_get_rate() takes a mutex and that's the case during
> suspend/cpuidle entering. Lastly, it's better to always fully reprogram
> PMC state because it's not obvious whether it could be changed after SC7.

I agree with the first part, but I would drop the last sentence because
I see no evidence of this. Maybe Peter can confirm.

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/soc/tegra/pmc.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 9f9c1c677cf4..532e0ada012b 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -1433,6 +1433,7 @@ void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode)
>  void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>  {
>  	unsigned long long rate = 0;
> +	u64 ticks;
>  	u32 value;
>  
>  	switch (mode) {
> @@ -1441,7 +1442,7 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>  		break;
>  
>  	case TEGRA_SUSPEND_LP2:
> -		rate = clk_get_rate(pmc->clk);
> +		rate = pmc->rate;

There is another call to clk_get_rate() that could be removed as well.

>  		break;
>  
>  	default:
> @@ -1451,26 +1452,20 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>  	if (WARN_ON_ONCE(rate == 0))
>  		rate = 100000000;
>  
> -	if (rate != pmc->rate) {
> -		u64 ticks;
> -
> -		ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
> -		do_div(ticks, USEC_PER_SEC);
> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
> -
> -		ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
> -		do_div(ticks, USEC_PER_SEC);
> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
> +	ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
> +	do_div(ticks, USEC_PER_SEC);
> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);

You could go a step further and update the cpu_good_time/cpu_off_time to
be ticks and calculated once during probe and recalculated if
tegra_pmc_set_suspend_mode is called. I am not sure why we really need
to pass mode to tegra_pmc_enter_suspend_mode() seeing as the mode is
stored in the pmc struct.

>  
> -		wmb();
> -
> -		pmc->rate = rate;
> -	}
> +	ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
> +	do_div(ticks, USEC_PER_SEC);
> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>  
>  	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>  	value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
>  	value |= PMC_CNTRL_CPU_PWRREQ_OE;
>  	tegra_pmc_writel(pmc, value, PMC_CNTRL);
> +
> +	wmb();
>  }
>  #endif
>  
> @@ -2082,6 +2077,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>  		pmc->clk = NULL;
>  	}
>  
> +	pmc->rate = clk_get_rate(pmc->clk);

You should check the value returned is not 0 here.

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time
  2019-07-18  9:45 ` Jon Hunter
@ 2019-07-18  9:53   ` Jon Hunter
  2019-07-18 18:24     ` Dmitry Osipenko
  2019-07-18 18:12   ` Dmitry Osipenko
  2019-07-25  9:41   ` Peter De Schrijver
  2 siblings, 1 reply; 6+ messages in thread
From: Jon Hunter @ 2019-07-18  9:53 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding, Peter De Schrijver
  Cc: linux-tegra, linux-kernel


On 18/07/2019 10:45, Jon Hunter wrote:
> 
> On 08/07/2019 00:08, Dmitry Osipenko wrote:
>> The PCLK clock is running off SCLK, which is a critical clock that is
>> very unlikely to randomly change its rate. It's also a bit clumsy (and
>> apparently incorrect) to query the clock's rate with interrupts being
>> disabled because clk_get_rate() takes a mutex and that's the case during
>> suspend/cpuidle entering. Lastly, it's better to always fully reprogram
>> PMC state because it's not obvious whether it could be changed after SC7.
> 
> I agree with the first part, but I would drop the last sentence because
> I see no evidence of this. Maybe Peter can confirm.
> 
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 26 +++++++++++---------------
>>  1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 9f9c1c677cf4..532e0ada012b 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -1433,6 +1433,7 @@ void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode)
>>  void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>  {
>>  	unsigned long long rate = 0;
>> +	u64 ticks;
>>  	u32 value;
>>  
>>  	switch (mode) {
>> @@ -1441,7 +1442,7 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>  		break;
>>  
>>  	case TEGRA_SUSPEND_LP2:
>> -		rate = clk_get_rate(pmc->clk);
>> +		rate = pmc->rate;
> 
> There is another call to clk_get_rate() that could be removed as well.
> 
>>  		break;
>>  
>>  	default:
>> @@ -1451,26 +1452,20 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>  	if (WARN_ON_ONCE(rate == 0))
>>  		rate = 100000000;
>>  
>> -	if (rate != pmc->rate) {
>> -		u64 ticks;
>> -
>> -		ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
>> -		do_div(ticks, USEC_PER_SEC);
>> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
>> -
>> -		ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
>> -		do_div(ticks, USEC_PER_SEC);
>> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>> +	ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
>> +	do_div(ticks, USEC_PER_SEC);
>> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
> 
> You could go a step further and update the cpu_good_time/cpu_off_time to
> be ticks and calculated once during probe and recalculated if
> tegra_pmc_set_suspend_mode is called. I am not sure why we really need
> to pass mode to tegra_pmc_enter_suspend_mode() seeing as the mode is
> stored in the pmc struct.
> 
>>  
>> -		wmb();
>> -
>> -		pmc->rate = rate;
>> -	}
>> +	ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
>> +	do_div(ticks, USEC_PER_SEC);
>> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>>  
>>  	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>  	value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
>>  	value |= PMC_CNTRL_CPU_PWRREQ_OE;
>>  	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>> +
>> +	wmb();

I would not move the barrier unless there is a good reason. Maybe it is
intentional that this happens before the other writes.

Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time
  2019-07-18  9:45 ` Jon Hunter
  2019-07-18  9:53   ` Jon Hunter
@ 2019-07-18 18:12   ` Dmitry Osipenko
  2019-07-25  9:41   ` Peter De Schrijver
  2 siblings, 0 replies; 6+ messages in thread
From: Dmitry Osipenko @ 2019-07-18 18:12 UTC (permalink / raw)
  To: Jon Hunter, Thierry Reding, Peter De Schrijver; +Cc: linux-tegra, linux-kernel

18.07.2019 12:45, Jon Hunter пишет:
> 
> On 08/07/2019 00:08, Dmitry Osipenko wrote:
>> The PCLK clock is running off SCLK, which is a critical clock that is
>> very unlikely to randomly change its rate. It's also a bit clumsy (and
>> apparently incorrect) to query the clock's rate with interrupts being
>> disabled because clk_get_rate() takes a mutex and that's the case during
>> suspend/cpuidle entering. Lastly, it's better to always fully reprogram
>> PMC state because it's not obvious whether it could be changed after SC7.
> 
> I agree with the first part, but I would drop the last sentence because
> I see no evidence of this. Maybe Peter can confirm.

Okay.

>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 26 +++++++++++---------------
>>  1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 9f9c1c677cf4..532e0ada012b 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -1433,6 +1433,7 @@ void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode)
>>  void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>  {
>>  	unsigned long long rate = 0;
>> +	u64 ticks;
>>  	u32 value;
>>  
>>  	switch (mode) {
>> @@ -1441,7 +1442,7 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>  		break;
>>  
>>  	case TEGRA_SUSPEND_LP2:
>> -		rate = clk_get_rate(pmc->clk);
>> +		rate = pmc->rate;
> 
> There is another call to clk_get_rate() that could be removed as well.

Indeed!

>>  		break;
>>  
>>  	default:
>> @@ -1451,26 +1452,20 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>  	if (WARN_ON_ONCE(rate == 0))
>>  		rate = 100000000;
>>  
>> -	if (rate != pmc->rate) {
>> -		u64 ticks;
>> -
>> -		ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
>> -		do_div(ticks, USEC_PER_SEC);
>> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
>> -
>> -		ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
>> -		do_div(ticks, USEC_PER_SEC);
>> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>> +	ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
>> +	do_div(ticks, USEC_PER_SEC);
>> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
> 
> You could go a step further and update the cpu_good_time/cpu_off_time to
> be ticks and calculated once during probe and recalculated if
> tegra_pmc_set_suspend_mode is called. I am not sure why we really need
> to pass mode to tegra_pmc_enter_suspend_mode() seeing as the mode is
> stored in the pmc struct.

The mode will differ depending on the idling mode. The system suspend
could be LP1, while CPUIDLE is LP2. Hence the mode need to be
reconfigured dynamically and thus the ticks.

>>  
>> -		wmb();
>> -
>> -		pmc->rate = rate;
>> -	}
>> +	ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
>> +	do_div(ticks, USEC_PER_SEC);
>> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>>  
>>  	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>  	value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
>>  	value |= PMC_CNTRL_CPU_PWRREQ_OE;
>>  	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>> +
>> +	wmb();
>>  }
>>  #endif
>>  
>> @@ -2082,6 +2077,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>>  		pmc->clk = NULL;
>>  	}
>>  
>> +	pmc->rate = clk_get_rate(pmc->clk);
> 
> You should check the value returned is not 0 here.

Good point!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time
  2019-07-18  9:53   ` Jon Hunter
@ 2019-07-18 18:24     ` Dmitry Osipenko
  0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Osipenko @ 2019-07-18 18:24 UTC (permalink / raw)
  To: Jon Hunter, Thierry Reding, Peter De Schrijver; +Cc: linux-tegra, linux-kernel

18.07.2019 12:53, Jon Hunter пишет:
> 
> On 18/07/2019 10:45, Jon Hunter wrote:
>>
>> On 08/07/2019 00:08, Dmitry Osipenko wrote:
>>> The PCLK clock is running off SCLK, which is a critical clock that is
>>> very unlikely to randomly change its rate. It's also a bit clumsy (and
>>> apparently incorrect) to query the clock's rate with interrupts being
>>> disabled because clk_get_rate() takes a mutex and that's the case during
>>> suspend/cpuidle entering. Lastly, it's better to always fully reprogram
>>> PMC state because it's not obvious whether it could be changed after SC7.
>>
>> I agree with the first part, but I would drop the last sentence because
>> I see no evidence of this. Maybe Peter can confirm.
>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/soc/tegra/pmc.c | 26 +++++++++++---------------
>>>  1 file changed, 11 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>> index 9f9c1c677cf4..532e0ada012b 100644
>>> --- a/drivers/soc/tegra/pmc.c
>>> +++ b/drivers/soc/tegra/pmc.c
>>> @@ -1433,6 +1433,7 @@ void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode)
>>>  void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>>  {
>>>  	unsigned long long rate = 0;
>>> +	u64 ticks;
>>>  	u32 value;
>>>  
>>>  	switch (mode) {
>>> @@ -1441,7 +1442,7 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>>  		break;
>>>  
>>>  	case TEGRA_SUSPEND_LP2:
>>> -		rate = clk_get_rate(pmc->clk);
>>> +		rate = pmc->rate;
>>
>> There is another call to clk_get_rate() that could be removed as well.
>>
>>>  		break;
>>>  
>>>  	default:
>>> @@ -1451,26 +1452,20 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>>>  	if (WARN_ON_ONCE(rate == 0))
>>>  		rate = 100000000;
>>>  
>>> -	if (rate != pmc->rate) {
>>> -		u64 ticks;
>>> -
>>> -		ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
>>> -		do_div(ticks, USEC_PER_SEC);
>>> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
>>> -
>>> -		ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
>>> -		do_div(ticks, USEC_PER_SEC);
>>> -		tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>>> +	ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
>>> +	do_div(ticks, USEC_PER_SEC);
>>> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
>>
>> You could go a step further and update the cpu_good_time/cpu_off_time to
>> be ticks and calculated once during probe and recalculated if
>> tegra_pmc_set_suspend_mode is called. I am not sure why we really need
>> to pass mode to tegra_pmc_enter_suspend_mode() seeing as the mode is
>> stored in the pmc struct.
>>
>>>  
>>> -		wmb();
>>> -
>>> -		pmc->rate = rate;
>>> -	}
>>> +	ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
>>> +	do_div(ticks, USEC_PER_SEC);
>>> +	tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>>>  
>>>  	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>  	value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
>>>  	value |= PMC_CNTRL_CPU_PWRREQ_OE;
>>>  	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>> +
>>> +	wmb();
> 
> I would not move the barrier unless there is a good reason. Maybe it is
> intentional that this happens before the other writes.

Looking at 'git blame', the barrier was copied by Thierry while he moved
PMC driver form mach-tegra/ to soc/. Originally the barrier appeared in
d552920a02759cdc45d8507868de10ac2f5b9a18 (ARM: tegra30: cpuidle: add
powered-down state for CPU0).

But really there is no good justification for that barrier at all
because PMC logic kicks-in when CPU enters power-gated state and at that
point all CPU accesses guaranteed to be finished no matter what, hence
this barrier just doesn't make much sense and can be removed safely.
I'll make a separate commit for that.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time
  2019-07-18  9:45 ` Jon Hunter
  2019-07-18  9:53   ` Jon Hunter
  2019-07-18 18:12   ` Dmitry Osipenko
@ 2019-07-25  9:41   ` Peter De Schrijver
  2 siblings, 0 replies; 6+ messages in thread
From: Peter De Schrijver @ 2019-07-25  9:41 UTC (permalink / raw)
  To: Jon Hunter; +Cc: Dmitry Osipenko, Thierry Reding, linux-tegra, linux-kernel

On Thu, Jul 18, 2019 at 10:45:31AM +0100, Jon Hunter wrote:
> 
> On 08/07/2019 00:08, Dmitry Osipenko wrote:
> > The PCLK clock is running off SCLK, which is a critical clock that is
> > very unlikely to randomly change its rate. It's also a bit clumsy (and
> > apparently incorrect) to query the clock's rate with interrupts being
> > disabled because clk_get_rate() takes a mutex and that's the case during
> > suspend/cpuidle entering. Lastly, it's better to always fully reprogram
> > PMC state because it's not obvious whether it could be changed after SC7.
> 
> I agree with the first part, but I would drop the last sentence because
> I see no evidence of this. Maybe Peter can confirm.
> 

SCLK and PCLK certainly can change rate at runtime, although the changes
for this haven't made it upstream yet. It's indeed not very obvious, but
certainly doable.

Peter.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-07-25  9:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-07 23:08 [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time Dmitry Osipenko
2019-07-18  9:45 ` Jon Hunter
2019-07-18  9:53   ` Jon Hunter
2019-07-18 18:24     ` Dmitry Osipenko
2019-07-18 18:12   ` Dmitry Osipenko
2019-07-25  9:41   ` Peter De Schrijver

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).