| Message ID | 20190730174020.15878-1-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/2] soc/tegra: pmc: Query PCLK clock rate at probe time | expand |
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!
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);
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 <digetx@gmail.com> --- 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(-)