cpuidle: exynos: Unify target residency for AFTR and coupled AFTR states
diff mbox series

Message ID 20190328133358.6922-1-m.szyprowski@samsung.com
State Awaiting Upstream
Delegated to: Rafael Wysocki
Headers show
Series
  • cpuidle: exynos: Unify target residency for AFTR and coupled AFTR states
Related show

Commit Message

Marek Szyprowski March 28, 2019, 1:33 p.m. UTC
Since commit 45f1ff59e27c ("cpuidle: Return nohz hint from
cpuidle_select()") Exynos CPUidle driver stopped entering C1 (AFTR) mode
on Exynos4412-based Trats2 board.

Further analysis revealed that the CPUidle framework changed the way
it handles predicted timer ticks and reported target residency for the
given idle states. As a result, the C1 (AFTR) state was not chosen
anymore on completely idle device. The main issue was to high target
residency value. The similar C1 (AFTR) state for 'coupled' CPUidle
version used 10 times lower value for the target residency, despite
the fact that it is the same state from the hardware perspective.

The 100000us value for standard C1 (AFTR) mode is there from the begining
of the support for this idle state, added by the commit 67173ca492ab
("ARM: EXYNOS: Add support AFTR mode on EXYNOS4210"). That commit doesn't
give any reason for it, instead it looks like it was blindly copied from
the WFI/IDLE state of the same driver that time. That time, that value
was probably not really used by the framework for any critical decision,
so it didn't matter that much.

Now it turned out to be an issue, so unify the target residency with the
'coupled' version, as it seems to better match the real use case values
and restores the operation of the Exynos CPUidle driver on the idle
device.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/cpuidle/cpuidle-exynos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Krzysztof Kozlowski March 28, 2019, 1:37 p.m. UTC | #1
On Thu, 28 Mar 2019 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Since commit 45f1ff59e27c ("cpuidle: Return nohz hint from
> cpuidle_select()") Exynos CPUidle driver stopped entering C1 (AFTR) mode
> on Exynos4412-based Trats2 board.
>
> Further analysis revealed that the CPUidle framework changed the way
> it handles predicted timer ticks and reported target residency for the
> given idle states. As a result, the C1 (AFTR) state was not chosen
> anymore on completely idle device. The main issue was to high target
> residency value. The similar C1 (AFTR) state for 'coupled' CPUidle
> version used 10 times lower value for the target residency, despite
> the fact that it is the same state from the hardware perspective.
>
> The 100000us value for standard C1 (AFTR) mode is there from the begining
> of the support for this idle state, added by the commit 67173ca492ab
> ("ARM: EXYNOS: Add support AFTR mode on EXYNOS4210"). That commit doesn't
> give any reason for it, instead it looks like it was blindly copied from
> the WFI/IDLE state of the same driver that time. That time, that value
> was probably not really used by the framework for any critical decision,
> so it didn't matter that much.
>
> Now it turned out to be an issue, so unify the target residency with the
> 'coupled' version, as it seems to better match the real use case values
> and restores the operation of the Exynos CPUidle driver on the idle
> device.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/cpuidle/cpuidle-exynos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof
Daniel Lezcano March 28, 2019, 1:41 p.m. UTC | #2
On 28/03/2019 14:33, Marek Szyprowski wrote:
> Since commit 45f1ff59e27c ("cpuidle: Return nohz hint from
> cpuidle_select()") Exynos CPUidle driver stopped entering C1 (AFTR) mode
> on Exynos4412-based Trats2 board.
> 
> Further analysis revealed that the CPUidle framework changed the way
> it handles predicted timer ticks and reported target residency for the
> given idle states. As a result, the C1 (AFTR) state was not chosen
> anymore on completely idle device. The main issue was to high target
> residency value. The similar C1 (AFTR) state for 'coupled' CPUidle
> version used 10 times lower value for the target residency, despite
> the fact that it is the same state from the hardware perspective.
> 
> The 100000us value for standard C1 (AFTR) mode is there from the begining
> of the support for this idle state, added by the commit 67173ca492ab
> ("ARM: EXYNOS: Add support AFTR mode on EXYNOS4210"). That commit doesn't
> give any reason for it, instead it looks like it was blindly copied from
> the WFI/IDLE state of the same driver that time. That time, that value
> was probably not really used by the framework for any critical decision,
> so it didn't matter that much.
> 
> Now it turned out to be an issue, so unify the target residency with the
> 'coupled' version, as it seems to better match the real use case values
> and restores the operation of the Exynos CPUidle driver on the idle
> device.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Bartlomiej Zolnierkiewicz March 28, 2019, 1:42 p.m. UTC | #3
On 03/28/2019 02:33 PM, Marek Szyprowski wrote:
> Since commit 45f1ff59e27c ("cpuidle: Return nohz hint from
> cpuidle_select()") Exynos CPUidle driver stopped entering C1 (AFTR) mode
> on Exynos4412-based Trats2 board.
> 
> Further analysis revealed that the CPUidle framework changed the way
> it handles predicted timer ticks and reported target residency for the
> given idle states. As a result, the C1 (AFTR) state was not chosen
> anymore on completely idle device. The main issue was to high target
> residency value. The similar C1 (AFTR) state for 'coupled' CPUidle
> version used 10 times lower value for the target residency, despite
> the fact that it is the same state from the hardware perspective.
> 
> The 100000us value for standard C1 (AFTR) mode is there from the begining
> of the support for this idle state, added by the commit 67173ca492ab
> ("ARM: EXYNOS: Add support AFTR mode on EXYNOS4210"). That commit doesn't
> give any reason for it, instead it looks like it was blindly copied from
> the WFI/IDLE state of the same driver that time. That time, that value
> was probably not really used by the framework for any critical decision,
> so it didn't matter that much.
> 
> Now it turned out to be an issue, so unify the target residency with the
> 'coupled' version, as it seems to better match the real use case values
> and restores the operation of the Exynos CPUidle driver on the idle
> device.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Thank you for fixing this!

> ---
>  drivers/cpuidle/cpuidle-exynos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index a95c7907a1fb..7284495d4869 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -84,7 +84,7 @@ static struct cpuidle_driver exynos_idle_driver = {
>  		[1] = {
>  			.enter			= exynos_enter_lowpower,
>  			.exit_latency		= 300,
> -			.target_residency	= 100000,
> +			.target_residency	= 10000,
>  			.name			= "C1",
>  			.desc			= "ARM power down",
>  		},
> 

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Rafael J. Wysocki April 2, 2019, 8:41 a.m. UTC | #4
On Thursday, March 28, 2019 2:33:58 PM CEST Marek Szyprowski wrote:
> Since commit 45f1ff59e27c ("cpuidle: Return nohz hint from
> cpuidle_select()") Exynos CPUidle driver stopped entering C1 (AFTR) mode
> on Exynos4412-based Trats2 board.
> 
> Further analysis revealed that the CPUidle framework changed the way
> it handles predicted timer ticks and reported target residency for the
> given idle states. As a result, the C1 (AFTR) state was not chosen
> anymore on completely idle device. The main issue was to high target
> residency value. The similar C1 (AFTR) state for 'coupled' CPUidle
> version used 10 times lower value for the target residency, despite
> the fact that it is the same state from the hardware perspective.
> 
> The 100000us value for standard C1 (AFTR) mode is there from the begining
> of the support for this idle state, added by the commit 67173ca492ab
> ("ARM: EXYNOS: Add support AFTR mode on EXYNOS4210"). That commit doesn't
> give any reason for it, instead it looks like it was blindly copied from
> the WFI/IDLE state of the same driver that time. That time, that value
> was probably not really used by the framework for any critical decision,
> so it didn't matter that much.
> 
> Now it turned out to be an issue, so unify the target residency with the
> 'coupled' version, as it seems to better match the real use case values
> and restores the operation of the Exynos CPUidle driver on the idle
> device.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/cpuidle/cpuidle-exynos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index a95c7907a1fb..7284495d4869 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -84,7 +84,7 @@ static struct cpuidle_driver exynos_idle_driver = {
>  		[1] = {
>  			.enter			= exynos_enter_lowpower,
>  			.exit_latency		= 300,
> -			.target_residency	= 100000,
> +			.target_residency	= 10000,
>  			.name			= "C1",
>  			.desc			= "ARM power down",
>  		},
> 

Applied, thanks!

Patch
diff mbox series

diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
index a95c7907a1fb..7284495d4869 100644
--- a/drivers/cpuidle/cpuidle-exynos.c
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -84,7 +84,7 @@  static struct cpuidle_driver exynos_idle_driver = {
 		[1] = {
 			.enter			= exynos_enter_lowpower,
 			.exit_latency		= 300,
-			.target_residency	= 100000,
+			.target_residency	= 10000,
 			.name			= "C1",
 			.desc			= "ARM power down",
 		},