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.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
Changelog:
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:
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 23, 2019 at 05:35:10AM +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'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.
>
SCLK and PCLK certainly can change rate at runtime, although the code to
handle this hasn't reached upstream yet.
Peter.
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
>
> Changelog:
>
> 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
>
25.07.2019 12:36, Peter De Schrijver пишет:
> On Tue, Jul 23, 2019 at 05:35:10AM +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'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.
>>
>
> SCLK and PCLK certainly can change rate at runtime, although the code to
> handle this hasn't reached upstream yet.
Okay, maybe this patch is indeed not very worthwhile then. I'm leaving
it up to you, guys, to decide.
25.07.2019 14:15, Dmitry Osipenko пишет:
> 25.07.2019 12:36, Peter De Schrijver пишет:
>> On Tue, Jul 23, 2019 at 05:35:10AM +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'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.
>>>
>>
>> SCLK and PCLK certainly can change rate at runtime, although the code to
>> handle this hasn't reached upstream yet.
>
> Okay, maybe this patch is indeed not very worthwhile then. I'm leaving
> it up to you, guys, to decide.
>
I now recalled what was the initial reason for this patch because
happened to bump into the problem once again.. it's really problematic
to call clk_get_rate() with the disabled preemption because some clk
notifier handler may block (EMC) and cause reschedule, hence the CCF
'prepare' mutex is kept locked during of CPUIDLE driver entering LP2
state and thus causing system lockup, since scheduling with the disabled
interrupts obviously won't work well.
So this patch actually is needed to be applied or some other solution
have to be provided. Since PCLK rate currently isn't altering anywhere
in the kernel, I'd suggest to imply apply this series. Please let me
know if you have any objections. I may re-iterate this patch with an
extended commit message, describing the resolved problem in a more
details, if necessary.
29.07.2019 16:07, Dmitry Osipenko пишет:
> 25.07.2019 14:15, Dmitry Osipenko пишет:
>> 25.07.2019 12:36, Peter De Schrijver пишет:
>>> On Tue, Jul 23, 2019 at 05:35:10AM +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'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.
>>>>
>>>
>>> SCLK and PCLK certainly can change rate at runtime, although the code to
>>> handle this hasn't reached upstream yet.
>>
>> Okay, maybe this patch is indeed not very worthwhile then. I'm leaving
>> it up to you, guys, to decide.
>>
>
> I now recalled what was the initial reason for this patch because
> happened to bump into the problem once again.. it's really problematic
> to call clk_get_rate() with the disabled preemption because some clk
> notifier handler may block (EMC) and cause reschedule, hence the CCF
> 'prepare' mutex is kept locked during of CPUIDLE driver entering LP2
> state and thus causing system lockup, since scheduling with the disabled
> interrupts obviously won't work well.
>
> So this patch actually is needed to be applied or some other solution
> have to be provided. Since PCLK rate currently isn't altering anywhere
> in the kernel, I'd suggest to imply apply this series. Please let me
> know if you have any objections. I may re-iterate this patch with an
> extended commit message, describing the resolved problem in a more
> details, if necessary.
>
I'll send v3 with a changed commit's message, please take a look at it.