Re: [PATCH v3 1/2] soc/tegra: pmc: Query PCLK clock rate at probe time

2019-08-02 Thread Dmitry Osipenko 
02.08.2019 16:38, Peter De Schrijver пишет:
> On Tue, Jul 30, 2019 at 08:40:19PM +0300, 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 is possible to get a
>> lockup if kernel decides to enter LP2 cpuidle from a clk-notifier, which
>> happens occasionally in a case of Tegra30 EMC driver that waits for the
>> clk-change event in the clk-notify handler, because CCF's 'prepare' mutex
>> in kept locked and thus clk_get_rate() wants to sleep with interrupts
>> being disabled.
>>
> 
> I don't think this is the right solution. Eventually we will want to
> scale sclk and pclk because the clock tree power of those is not
> insignificant. Maybe register a notifier which updates the PMC timer
> values when pclk changes?

I also had a thought about the notifier for pclk, but wasn't sure if
it's really worthwhile right now since there is no real use-case and
it's not obvious when such case will materialize. So, I'd say that
solution is correct but incomplete.

I'll make a v4 with the notifier, since you're asking about it. Thanks
for the review!

Re: [PATCH v3 1/2] soc/tegra: pmc: Query PCLK clock rate at probe time

2019-08-02 Thread Peter De Schrijver 
On Tue, Jul 30, 2019 at 08:40:19PM +0300, 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 is possible to get a
> lockup if kernel decides to enter LP2 cpuidle from a clk-notifier, which
> happens occasionally in a case of Tegra30 EMC driver that waits for the
> clk-change event in the clk-notify handler, because CCF's 'prepare' mutex
> in kept locked and thus clk_get_rate() wants to sleep with interrupts
> being disabled.
> 

I don't think this is the right solution. Eventually we will want to
scale sclk and pclk because the clock tree power of those is not
insignificant. Maybe register a notifier which updates the PMC timer
values when pclk changes?

Peter.

[PATCH v3 1/2] soc/tegra: pmc: Query PCLK clock rate at probe time

2019-07-30 Thread Dmitry Osipenko 
The PCLK clock is running off SCLK, which is a critical clock that is
very unlikely to randomly change its rate. It is possible to get a
lockup if kernel decides to enter LP2 cpuidle from a clk-notifier, which
happens occasionally in a case of Tegra30 EMC driver that waits for the
clk-change event in the clk-notify handler, because CCF's 'prepare' mutex
in kept locked and thus clk_get_rate() wants to sleep with interrupts
being disabled.

Signed-off-by: Dmitry Osipenko 
---

Changelog:

v3: Changed commit's message because I actually recalled what was the
initial reason for the patch, since the problem reoccurred once again.

v2: Addressed review comments that were made by Jon Hunter to v1 by
not moving the memory barrier, replacing one missed clk_get_rate()
with pmc->rate, handling possible clk_get_rate() error on probe and
slightly adjusting the commits message.

 drivers/soc/tegra/pmc.c | 34 --
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 9f9c1c677cf4..aba3396b2e73 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1192,7 +1192,7 @@ static int tegra_io_pad_prepare(struct tegra_pmc *pmc, 
enum tegra_io_pad id,
return err;
 
if (pmc->clk) {
-   rate = clk_get_rate(pmc->clk);
+   rate = pmc->rate;
if (!rate) {
dev_err(pmc->dev, "failed to get clock rate\n");
return -ENODEV;
@@ -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,31 +1442,22 @@ 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:
break;
}
 
-   if (WARN_ON_ONCE(rate == 0))
-   rate = 1;
+   ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
+   do_div(ticks, USEC_PER_SEC);
+   tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
 
-   if (rate != pmc->rate) {
-   u64 ticks;
+   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);
-
-   ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
-   do_div(ticks, USEC_PER_SEC);
-   tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
-
-   wmb();
-
-   pmc->rate = rate;
-   }
+   wmb();
 
value = tegra_pmc_readl(pmc, PMC_CNTRL);
value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
@@ -2082,8 +2074,14 @@ static int tegra_pmc_probe(struct platform_device *pdev)
pmc->clk = NULL;
}
 
+   pmc->rate = clk_get_rate(pmc->clk);
pmc->dev = >dev;
 
+   if (!pmc->rate) {
+   dev_err(>dev, "failed to get pclk rate\n");
+   pmc->rate = 1;
+   }
+
tegra_pmc_init(pmc);
 
tegra_pmc_init_tsense_reset(pmc);
-- 
2.22.0