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 <[email protected]>
---
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 = 100000000;
+ 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 = &pdev->dev;
+ if (!pmc->rate) {
+ dev_err(&pdev->dev, "failed to get pclk rate\n");
+ pmc->rate = 100000000;
+ }
+
tegra_pmc_init(pmc);
tegra_pmc_init_tsense_reset(pmc);
--
2.22.0
The removed barrier isn't needed because the writes/reads are strictly
ordered and even if PMC had separate ports for the writes, it wouldn't
matter since the hardware logic takes into effect after triggering CPU's
power-gating and at that point all CPU accesses are guaranteed to be
completed. Hence remove the barrier to eliminate the confusion.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
Changelog:
v3: No changes.
v2: New patch that was added after Jon's Hunter pointing that it's better
not to change the barrier's placement in the code. In fact the barrier
is not needed at all.
drivers/soc/tegra/pmc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index aba3396b2e73..3044809f1c10 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1457,8 +1457,6 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
do_div(ticks, USEC_PER_SEC);
tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
- wmb();
-
value = tegra_pmc_readl(pmc, PMC_CNTRL);
value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
value |= PMC_CNTRL_CPU_PWRREQ_OE;
--
2.22.0
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.
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!