soc/tegra: pmc: Query PCLK clock rate at probe time
PATCH 1/1 soc/tegra: pmc: Query PCLK clock rate at probe time
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.
--- drivers/soc/tegra/pmc.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
drivers/soc/tegra/pmc.c +11 -15
@@ -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);
Signature · 1 line
2.22.0
Jon Hunter
maintainerQuoted 6 lines · Dmitry Osipenko
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.
Quoted 18 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/soc/tegra/pmc.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)drivers/soc/tegra/pmc.c +1
-0@@ -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) {
drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1445
Quoted 5 lines
drivers/soc/tegra/pmc.c +1
-1@@ -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.
Quoted 10 lines
drivers/soc/tegra/pmc.c +0
-3break ;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; -
drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1457
Quoted 10 lines
drivers/soc/tegra/pmc.c +3
-7- 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.
Quoted 19 lines
drivers/soc/tegra/pmc.c +5
-4-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
drivers/soc/tegra/pmc.c static int tegra_pmc_probe(struct platform_device *pdev) · line 2080
Quoted 4 lines
drivers/soc/tegra/pmc.c +1
-0@@ -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
Signature · 1 line
nvpublic
Jon Hunter
maintainerQuoted 74 lines · Jon Hunter
Quoted 6 lines · Dmitry Osipenko
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.
Quoted 18 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/soc/tegra/pmc.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)drivers/soc/tegra/pmc.c +1
-0@@ -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) {drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1445
Quoted 5 lines
drivers/soc/tegra/pmc.c +1
-1@@ -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.Quoted 10 lines
drivers/soc/tegra/pmc.c +0
-3break ;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; -drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1457
Quoted 10 lines
drivers/soc/tegra/pmc.c +3
-7- 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_modeis called. I am not sure why we really need to pass mode totegra_pmc_enter_suspend_mode()seeing as the mode is stored in the pmc struct.Quoted 9 lines
drivers/soc/tegra/pmc.c +3
-4-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 );
Quoted 6 lines
drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1468
Quoted 6 lines
drivers/soc/tegra/pmc.c +2
-0value =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
Signature · 1 line
nvpublic
18.07.2019 12:53, Jon Hunter пишет:
Quoted 76 lines · Jon Hunter
Quoted 74 lines · Dmitry Osipenko
Quoted 6 lines
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.
Quoted 18 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/soc/tegra/pmc.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)drivers/soc/tegra/pmc.c +1
-0@@ -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) {drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1445
Quoted 5 lines
drivers/soc/tegra/pmc.c +1
-1@@ -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.Quoted 10 lines
drivers/soc/tegra/pmc.c +0
-3break ;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; -drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1457
Quoted 10 lines
drivers/soc/tegra/pmc.c +3
-7- 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_modeis called. I am not sure why we really need to pass mode totegra_pmc_enter_suspend_mode()seeing as the mode is stored in the pmc struct.Quoted 9 lines
drivers/soc/tegra/pmc.c +3
-4-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 );
Quoted 9 lines
Quoted 6 lines
drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1468
Quoted 6 lines
drivers/soc/tegra/pmc.c +2
-0value =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.
18.07.2019 12:45, Jon Hunter пишет:
Quoted 11 lines · Dmitry Osipenko
Quoted 6 lines
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.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/soc/tegra/pmc.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)drivers/soc/tegra/pmc.c +1
-0@@ -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 ;
Quoted 5 lines
drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1445
Quoted 3 lines
drivers/soc/tegra/pmc.c +1
-1case 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!
Quoted 18 lines
Quoted 18 lines
drivers/soc/tegra/pmc.c +1
-10break ;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 ;
Quoted 8 lines
drivers/soc/tegra/pmc.c void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode) · line 1457
Quoted 2 lines
drivers/soc/tegra/pmc.c +2
-0+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_modeis called. I am not sure why we really need to pass mode totegra_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.
Quoted 18 lines
Quoted 18 lines
drivers/soc/tegra/pmc.c +5
-4-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
Quoted 7 lines
drivers/soc/tegra/pmc.c static int tegra_pmc_probe(struct platform_device *pdev) · line 2080
Quoted 5 lines
drivers/soc/tegra/pmc.c +1
-0@@ -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!
Quoted 9 lines · Jon Hunter
Quoted 6 lines · Dmitry Osipenko
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.
Quoted 3 lines
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.