diff mbox

ARM: OMAP4: PM: Avoid expensive cpu_suspend() path for all CPU power states except off

Message ID 1360336306-18277-3-git-send-email-santosh.shilimkar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Santosh Shilimkar Feb. 8, 2013, 3:11 p.m. UTC
Current CPU PM code code make use of common cpu_suspend() path for all the
CPU power states which is not optimal. In fact cpu_suspend() path is needed
only when we put CPU power domain to off state where the CPU context is lost.

Update the code accordingly so that the expensive cpu_suspend() path
can be avoided for the shallow CPU power states like CPU PD INA/CSWR.

Cc: Kevin Hilman <khilman@deeprootsystems.com>

Reported-by: Richard Woodruff <r-woodruff2@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/mach-omap2/omap-mpuss-lowpower.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Felipe Balbi Feb. 8, 2013, 3:26 p.m. UTC | #1
On Fri, Feb 08, 2013 at 08:41:46PM +0530, Santosh Shilimkar wrote:
> Current CPU PM code code make use of common cpu_suspend() path for all the
> CPU power states which is not optimal. In fact cpu_suspend() path is needed
> only when we put CPU power domain to off state where the CPU context is lost.
> 
> Update the code accordingly so that the expensive cpu_suspend() path
> can be avoided for the shallow CPU power states like CPU PD INA/CSWR.
> 
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> 
> Reported-by: Richard Woodruff <r-woodruff2@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

some numbers showing the benefit would be cool...
Santosh Shilimkar Feb. 8, 2013, 3:34 p.m. UTC | #2
On Friday 08 February 2013 08:56 PM, Felipe Balbi wrote:
> On Fri, Feb 08, 2013 at 08:41:46PM +0530, Santosh Shilimkar wrote:
>> Current CPU PM code code make use of common cpu_suspend() path for all the
>> CPU power states which is not optimal. In fact cpu_suspend() path is needed
>> only when we put CPU power domain to off state where the CPU context is lost.
>>
>> Update the code accordingly so that the expensive cpu_suspend() path
>> can be avoided for the shallow CPU power states like CPU PD INA/CSWR.
>>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>
>> Reported-by: Richard Woodruff <r-woodruff2@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
> some numbers showing the benefit would be cool...
>
The numbers depends on CPU clock you are running generally. The main
issue is, for the shallow power states where we were not loosing
CPU context or Caches, we were doing all the book keeping. States
like INA are of 6-8 uS and above overhead is quite significant
their.

Regards
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Feb. 8, 2013, 9:19 p.m. UTC | #3
Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

> Current CPU PM code code make use of common cpu_suspend() path for all the
> CPU power states which is not optimal. In fact cpu_suspend() path is needed
> only when we put CPU power domain to off state where the CPU context is lost.
>
> Update the code accordingly so that the expensive cpu_suspend() path
> can be avoided for the shallow CPU power states like CPU PD INA/CSWR.
>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>
> Reported-by: Richard Woodruff <r-woodruff2@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Looks OK at first glance, but are you sure this is right for the
various ways both clusters might idle using coupled CPUidle?

Some description of the testing would be helpful here.

Thanks,

Kevin

> ---
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> index aac46bf..abdd0f6 100644
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -276,7 +276,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>  	/*
>  	 * Call low level function  with targeted low power state.
>  	 */
> -	cpu_suspend(save_state, omap4_finish_suspend);
> +	if (save_state)
> +		cpu_suspend(save_state, omap4_finish_suspend);
> +	else
> +		omap4_finish_suspend(save_state);
>  
>  	/*
>  	 * Restore the CPUx power state to ON otherwise CPUx
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar Feb. 12, 2013, 4:55 a.m. UTC | #4
On Saturday 09 February 2013 02:49 AM, Kevin Hilman wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
>> Current CPU PM code code make use of common cpu_suspend() path for all the
>> CPU power states which is not optimal. In fact cpu_suspend() path is needed
>> only when we put CPU power domain to off state where the CPU context is lost.
>>
>> Update the code accordingly so that the expensive cpu_suspend() path
>> can be avoided for the shallow CPU power states like CPU PD INA/CSWR.
>>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>
>> Reported-by: Richard Woodruff <r-woodruff2@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
> Looks OK at first glance, but are you sure this is right for the
> various ways both clusters might idle using coupled CPUidle?
>
Yes it is perfectly safe from all C-states. This patch has been part of
the OMAP4/OMAP5 product port for some time. I forgot to send it upstream
some how :(

> Some description of the testing would be helpful here.
>
Sorry. Should have mentioned it in first place.
Patch is being tested on OMAP4430 (idle/suspend) and OMAP5 with
few out of tree patches.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Feb. 12, 2013, 3:18 p.m. UTC | #5
Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

> On Saturday 09 February 2013 02:49 AM, Kevin Hilman wrote:
>> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>>
>>> Current CPU PM code code make use of common cpu_suspend() path for all the
>>> CPU power states which is not optimal. In fact cpu_suspend() path is needed
>>> only when we put CPU power domain to off state where the CPU context is lost.
>>>
>>> Update the code accordingly so that the expensive cpu_suspend() path
>>> can be avoided for the shallow CPU power states like CPU PD INA/CSWR.
>>>
>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>
>>> Reported-by: Richard Woodruff <r-woodruff2@ti.com>
>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>
>> Looks OK at first glance, but are you sure this is right for the
>> various ways both clusters might idle using coupled CPUidle?
>>
> Yes it is perfectly safe from all C-states. This patch has been part of
> the OMAP4/OMAP5 product port for some time. I forgot to send it upstream
> some how :(
>
>> Some description of the testing would be helpful here.
>>
> Sorry. Should have mentioned it in first place.
> Patch is being tested on OMAP4430 (idle/suspend) and OMAP5 with
> few out of tree patches.

OK, please update changelog with a brief description of how it was
tested, and on which platforms.

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar Feb. 13, 2013, 5:05 a.m. UTC | #6
On Tuesday 12 February 2013 08:48 PM, Kevin Hilman wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
>> On Saturday 09 February 2013 02:49 AM, Kevin Hilman wrote:
>>> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>>>
>>>> Current CPU PM code code make use of common cpu_suspend() path for all the
>>>> CPU power states which is not optimal. In fact cpu_suspend() path is needed
>>>> only when we put CPU power domain to off state where the CPU context is lost.
>>>>
>>>> Update the code accordingly so that the expensive cpu_suspend() path
>>>> can be avoided for the shallow CPU power states like CPU PD INA/CSWR.
>>>>
>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>>
>>>> Reported-by: Richard Woodruff <r-woodruff2@ti.com>
>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>
>>> Looks OK at first glance, but are you sure this is right for the
>>> various ways both clusters might idle using coupled CPUidle?
>>>
>> Yes it is perfectly safe from all C-states. This patch has been part of
>> the OMAP4/OMAP5 product port for some time. I forgot to send it upstream
>> some how :(
>>
>>> Some description of the testing would be helpful here.
>>>
>> Sorry. Should have mentioned it in first place.
>> Patch is being tested on OMAP4430 (idle/suspend) and OMAP5 with
>> few out of tree patches.
>
> OK, please update changelog with a brief description of how it was
> tested, and on which platforms.
>
Will update the changelog and post it

Regards,
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
index aac46bf..abdd0f6 100644
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -276,7 +276,10 @@  int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
 	/*
 	 * Call low level function  with targeted low power state.
 	 */
-	cpu_suspend(save_state, omap4_finish_suspend);
+	if (save_state)
+		cpu_suspend(save_state, omap4_finish_suspend);
+	else
+		omap4_finish_suspend(save_state);
 
 	/*
 	 * Restore the CPUx power state to ON otherwise CPUx