| Message ID | 1360336306-18277-3-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
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
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
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
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
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
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
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(-)