[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Nicolas Pitre]

The core idle loop now takes care of it.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/powerpc/platforms/powernv/setup.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index 21166f65c9..a932feb290 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -26,7 +26,6 @@
 #include <linux/of_fdt.h>
 #include <linux/interrupt.h>
 #include <linux/bug.h>
-#include <linux/cpuidle.h>
 
 #include <asm/machdep.h>
 #include <asm/firmware.h>
@@ -217,16 +216,6 @@ static int __init pnv_probe(void)
 	return 1;
 }
 
-void powernv_idle(void)
-{
-	/* Hook to cpuidle framework if available, else
-	 * call on default platform idle code
-	 */
-	if (cpuidle_idle_call()) {
-		power7_idle();
-	}
-}
-
 define_machine(powernv) {
 	.name			= "PowerNV",
 	.probe			= pnv_probe,
@@ -236,7 +225,7 @@ define_machine(powernv) {
 	.show_cpuinfo		= pnv_show_cpuinfo,
 	.progress		= pnv_progress,
 	.machine_shutdown	= pnv_shutdown,
-	.power_save             = powernv_idle,
+	.power_save             = power7_idle,
 	.calibrate_decr		= generic_calibrate_decr,
 #ifdef CONFIG_KEXEC
 	.kexec_cpu_down		= pnv_kexec_cpu_down,
-- 
1.8.4.108.g55ea5f6

[PATCH 2/2] ARM64: powernv: remove redundant cpuidle_idle_call()

[Nicolas Pitre]

The core idle loop now takes care of it.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm64/kernel/process.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 1c0a9be2ff..9cce0098f4 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -33,7 +33,6 @@
 #include <linux/kallsyms.h>
 #include <linux/init.h>
 #include <linux/cpu.h>
-#include <linux/cpuidle.h>
 #include <linux/elfcore.h>
 #include <linux/pm.h>
 #include <linux/tick.h>
@@ -94,10 +93,8 @@ void arch_cpu_idle(void)
 	 * This should do all the clock switching and wait for interrupt
 	 * tricks
 	 */
-	if (cpuidle_idle_call()) {
-		cpu_do_idle();
-		local_irq_enable();
-	}
+	cpu_do_idle();
+	local_irq_enable();
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
-- 
1.8.4.108.g55ea5f6

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Thomas Gleixner]

An Thu, 6 Feb 2014, Nicolas Pitre wrote:

> The core idle loop now takes care of it.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 2/2] ARM64: powernv: remove redundant cpuidle_idle_call()

[Thomas Gleixner]

On Thu, 6 Feb 2014, Nicolas Pitre wrote:

> The core idle loop now takes care of it.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

On 02/06/2014 07:46 PM, Nicolas Pitre wrote:
> The core idle loop now takes care of it.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index 21166f65c9..a932feb290 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -26,7 +26,6 @@
>  #include <linux/of_fdt.h>
>  #include <linux/interrupt.h>
>  #include <linux/bug.h>
> -#include <linux/cpuidle.h>
> 
>  #include <asm/machdep.h>
>  #include <asm/firmware.h>
> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>  	return 1;
>  }
> 
> -void powernv_idle(void)
> -{
> -	/* Hook to cpuidle framework if available, else
> -	 * call on default platform idle code
> -	 */
> -	if (cpuidle_idle_call()) {
> -		power7_idle();
> -	}
> -}
> -
>  define_machine(powernv) {
>  	.name			= "PowerNV",
>  	.probe			= pnv_probe,
> @@ -236,7 +225,7 @@ define_machine(powernv) {
>  	.show_cpuinfo		= pnv_show_cpuinfo,
>  	.progress		= pnv_progress,
>  	.machine_shutdown	= pnv_shutdown,
> -	.power_save             = powernv_idle,
> +	.power_save             = power7_idle,
>  	.calibrate_decr		= generic_calibrate_decr,
>  #ifdef CONFIG_KEXEC
>  	.kexec_cpu_down		= pnv_kexec_cpu_down,
> 

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

[PATCH 2/2] ARM64: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

Hi Nicolas,

powernv in the subject of the patch?

Regards
Preeti U Murthy
On 02/06/2014 07:46 PM, Nicolas Pitre wrote:
> The core idle loop now takes care of it.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/arm64/kernel/process.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1c0a9be2ff..9cce0098f4 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -33,7 +33,6 @@
>  #include <linux/kallsyms.h>
>  #include <linux/init.h>
>  #include <linux/cpu.h>
> -#include <linux/cpuidle.h>
>  #include <linux/elfcore.h>
>  #include <linux/pm.h>
>  #include <linux/tick.h>
> @@ -94,10 +93,8 @@ void arch_cpu_idle(void)
>  	 * This should do all the clock switching and wait for interrupt
>  	 * tricks
>  	 */
> -	if (cpuidle_idle_call()) {
> -		cpu_do_idle();
> -		local_irq_enable();
> -	}
> +	cpu_do_idle();
> +	local_irq_enable();
>  }
> 
>  #ifdef CONFIG_HOTPLUG_CPU
> 

[PATCH 2/2] ARM64: powernv: remove redundant cpuidle_idle_call()

[Nicolas Pitre]

On Thu, 6 Feb 2014, Preeti U Murthy wrote:

> Hi Nicolas,
> 
> powernv in the subject of the patch?

Crap.  You're right.

That's what you get when posting patches while attending a meeting.



> 
> Regards
> Preeti U Murthy
> On 02/06/2014 07:46 PM, Nicolas Pitre wrote:
> > The core idle loop now takes care of it.
> > 
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > ---
> >  arch/arm64/kernel/process.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 1c0a9be2ff..9cce0098f4 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -33,7 +33,6 @@
> >  #include <linux/kallsyms.h>
> >  #include <linux/init.h>
> >  #include <linux/cpu.h>
> > -#include <linux/cpuidle.h>
> >  #include <linux/elfcore.h>
> >  #include <linux/pm.h>
> >  #include <linux/tick.h>
> > @@ -94,10 +93,8 @@ void arch_cpu_idle(void)
> >  	 * This should do all the clock switching and wait for interrupt
> >  	 * tricks
> >  	 */
> > -	if (cpuidle_idle_call()) {
> > -		cpu_do_idle();
> > -		local_irq_enable();
> > -	}
> > +	cpu_do_idle();
> > +	local_irq_enable();
> >  }
> > 
> >  #ifdef CONFIG_HOTPLUG_CPU
> > 
> 

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

Hi Daniel,

On 02/06/2014 09:55 PM, Daniel Lezcano wrote:
> Hi Nico,
> 
> 
> On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> 
>> The core idle loop now takes care of it.
>>
>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>> ---
>>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/setup.c
>> b/arch/powerpc/platforms/powernv/setup.c
>> index 21166f65c9..a932feb290 100644
>> --- a/arch/powerpc/platforms/powernv/setup.c
>> +++ b/arch/powerpc/platforms/powernv/setup.c
>> @@ -26,7 +26,6 @@
>>  #include <linux/of_fdt.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/bug.h>
>> -#include <linux/cpuidle.h>
>>
>>  #include <asm/machdep.h>
>>  #include <asm/firmware.h>
>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>>         return 1;
>>  }
>>
>> -void powernv_idle(void)
>> -{
>> -       /* Hook to cpuidle framework if available, else
>> -        * call on default platform idle code
>> -        */
>> -       if (cpuidle_idle_call()) {
>> -               power7_idle();
>> -       }
>>
> 
> The cpuidle_idle_call is called from arch_cpu_idle in
> arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section.
> Shouldn't the cpuidle-powernv driver call these functions when entering
> idle ?

Yes they should, I will send out a patch that does that ontop of this.
There have been cpuidle driver cleanups for powernv and pseries in this
merge window. While no change would be required in the pseries cpuidle
driver as a result of Nicolas's cleanup, we would need to add the
ppc64_runlatch_on and off functions before and after the entry into the
powernv idle states.

Thanks

Regards
Preeti U Murthy
> 
>   -- Daniel
> 
> 
>> -}
>> -
>>  define_machine(powernv) {
>>         .name                   = "PowerNV",
>>         .probe                  = pnv_probe,
>> @@ -236,7 +225,7 @@ define_machine(powernv) {
>>         .show_cpuinfo           = pnv_show_cpuinfo,
>>         .progress               = pnv_progress,
>>         .machine_shutdown       = pnv_shutdown,
>> -       .power_save             = powernv_idle,
>> +       .power_save             = power7_idle,
>>         .calibrate_decr         = generic_calibrate_decr,
>>  #ifdef CONFIG_KEXEC
>>         .kexec_cpu_down         = pnv_kexec_cpu_down,
>> --
>> 1.8.4.108.g55ea5f6
>>
>>
> 

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Nicolas Pitre]

On Thu, 6 Feb 2014, Preeti U Murthy wrote:

> Hi Daniel,
> 
> On 02/06/2014 09:55 PM, Daniel Lezcano wrote:
> > Hi Nico,
> > 
> > 
> > On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > 
> >> The core idle loop now takes care of it.
> >>
> >> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> >> ---
> >>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
> >>  1 file changed, 1 insertion(+), 12 deletions(-)
> >>
> >> diff --git a/arch/powerpc/platforms/powernv/setup.c
> >> b/arch/powerpc/platforms/powernv/setup.c
> >> index 21166f65c9..a932feb290 100644
> >> --- a/arch/powerpc/platforms/powernv/setup.c
> >> +++ b/arch/powerpc/platforms/powernv/setup.c
> >> @@ -26,7 +26,6 @@
> >>  #include <linux/of_fdt.h>
> >>  #include <linux/interrupt.h>
> >>  #include <linux/bug.h>
> >> -#include <linux/cpuidle.h>
> >>
> >>  #include <asm/machdep.h>
> >>  #include <asm/firmware.h>
> >> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
> >>         return 1;
> >>  }
> >>
> >> -void powernv_idle(void)
> >> -{
> >> -       /* Hook to cpuidle framework if available, else
> >> -        * call on default platform idle code
> >> -        */
> >> -       if (cpuidle_idle_call()) {
> >> -               power7_idle();
> >> -       }
> >>
> > 
> > The cpuidle_idle_call is called from arch_cpu_idle in
> > arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section.
> > Shouldn't the cpuidle-powernv driver call these functions when entering
> > idle ?
> 
> Yes they should, I will send out a patch that does that ontop of this.
> There have been cpuidle driver cleanups for powernv and pseries in this
> merge window. While no change would be required in the pseries cpuidle
> driver as a result of Nicolas's cleanup, we would need to add the
> ppc64_runlatch_on and off functions before and after the entry into the
> powernv idle states.

What about creating arch_cpu_idle_enter() and arch_cpu_idle_exit() in 
arch/powerpc/kernel/idle.c and calling ppc64_runlatch_off() and 
ppc64_runlatch_on() respectively from there instead?  Would that work?  
That would make the idle consolidation much easier afterwards.


Nicolas

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

Hi Nicolas,

On 02/07/2014 06:47 AM, Nicolas Pitre wrote:
> On Thu, 6 Feb 2014, Preeti U Murthy wrote:
> 
>> Hi Daniel,
>>
>> On 02/06/2014 09:55 PM, Daniel Lezcano wrote:
>>> Hi Nico,
>>>
>>>
>>> On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>>>
>>>> The core idle loop now takes care of it.
>>>>
>>>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>>>> ---
>>>>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>>>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/platforms/powernv/setup.c
>>>> b/arch/powerpc/platforms/powernv/setup.c
>>>> index 21166f65c9..a932feb290 100644
>>>> --- a/arch/powerpc/platforms/powernv/setup.c
>>>> +++ b/arch/powerpc/platforms/powernv/setup.c
>>>> @@ -26,7 +26,6 @@
>>>>  #include <linux/of_fdt.h>
>>>>  #include <linux/interrupt.h>
>>>>  #include <linux/bug.h>
>>>> -#include <linux/cpuidle.h>
>>>>
>>>>  #include <asm/machdep.h>
>>>>  #include <asm/firmware.h>
>>>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>>>>         return 1;
>>>>  }
>>>>
>>>> -void powernv_idle(void)
>>>> -{
>>>> -       /* Hook to cpuidle framework if available, else
>>>> -        * call on default platform idle code
>>>> -        */
>>>> -       if (cpuidle_idle_call()) {
>>>> -               power7_idle();
>>>> -       }
>>>>
>>>
>>> The cpuidle_idle_call is called from arch_cpu_idle in
>>> arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section.
>>> Shouldn't the cpuidle-powernv driver call these functions when entering
>>> idle ?
>>
>> Yes they should, I will send out a patch that does that ontop of this.
>> There have been cpuidle driver cleanups for powernv and pseries in this
>> merge window. While no change would be required in the pseries cpuidle
>> driver as a result of Nicolas's cleanup, we would need to add the
>> ppc64_runlatch_on and off functions before and after the entry into the
>> powernv idle states.
> 
> What about creating arch_cpu_idle_enter() and arch_cpu_idle_exit() in 
> arch/powerpc/kernel/idle.c and calling ppc64_runlatch_off() and 
> ppc64_runlatch_on() respectively from there instead?  Would that work?  
> That would make the idle consolidation much easier afterwards.

I would not suggest doing this. The ppc64_runlatch_*() routines need to
be called when we are sure that the cpu is about to enter or has exit an
idle state. Moving the ppc64_runlatch_on() routine to
arch_cpu_idle_enter() for instance is not a good idea because there are
places where the cpu can decide not to enter any idle state before the
call to cpuidle_idle_call() itself. In that case communicating
prematurely that we are in an idle state would not be a good idea.

So its best to add the ppc64_runlatch_* calls in the powernv cpuidle
driver IMO. We could however create idle_loop_prologue/epilogue()
variants inside it so that in addition to the runlatch routines we could
potentially add more such similar routines that are powernv specific.
  If there are cases where there is work to be done prior to and post an
entry into an idle state common to both pseries and powernv, we will
probably put them in arch_cpu_idle_enter/exit(). But the runlatch
routines are not suitable to be moved there as far as I can see.

Thank you

Regards
Preeti U Murthy
> 
> 
> Nicolas
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

Hi Nicolas,

Find below the patch that will need to be squashed with this one.
This patch is based on the mainline.Adding Deepthi, the author of
the patch which introduced the powernv cpuidle driver. Deepthi,
do you think the below patch looks right? We do not need to do an
explicit local_irq_enable() since we are in the call path of
cpuidle driver and that explicitly enables irqs on exit from
idle states.

On 02/07/2014 06:47 AM, Nicolas Pitre wrote:
> On Thu, 6 Feb 2014, Preeti U Murthy wrote:
> 
>> Hi Daniel,
>>
>> On 02/06/2014 09:55 PM, Daniel Lezcano wrote:
>>> Hi Nico,
>>>
>>>
>>> On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>>>
>>>> The core idle loop now takes care of it.
>>>>
>>>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>>>> ---
>>>>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>>>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/platforms/powernv/setup.c
>>>> b/arch/powerpc/platforms/powernv/setup.c
>>>> index 21166f65c9..a932feb290 100644
>>>> --- a/arch/powerpc/platforms/powernv/setup.c
>>>> +++ b/arch/powerpc/platforms/powernv/setup.c
>>>> @@ -26,7 +26,6 @@
>>>>  #include <linux/of_fdt.h>
>>>>  #include <linux/interrupt.h>
>>>>  #include <linux/bug.h>
>>>> -#include <linux/cpuidle.h>
>>>>
>>>>  #include <asm/machdep.h>
>>>>  #include <asm/firmware.h>
>>>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>>>>         return 1;
>>>>  }
>>>>
>>>> -void powernv_idle(void)
>>>> -{
>>>> -       /* Hook to cpuidle framework if available, else
>>>> -        * call on default platform idle code
>>>> -        */
>>>> -       if (cpuidle_idle_call()) {
>>>> -               power7_idle();
>>>> -       }
>>>>

 drivers/cpuidle/cpuidle-powernv.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 78fd174..130f081 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -31,11 +31,13 @@ static int snooze_loop(struct cpuidle_device *dev,
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
 	while (!need_resched()) {
+		ppc64_runlatch_off();
 		HMT_low();
 		HMT_very_low();
 	}
 
 	HMT_medium();
+	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
 	smp_mb();
 	return index;
@@ -45,7 +47,9 @@ static int nap_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			int index)
 {
+	ppc64_runlatch_off();
 	power7_idle();
+	ppc64_runlatch_on();
 	return index;
 }
 
Thanks

Regards
Preeti U Murthy

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Deepthi Dharwar]

Hi Preeti,

Thanks for the patch.

On 02/07/2014 12:31 PM, Preeti U Murthy wrote:
> Hi Nicolas,
> 
> Find below the patch that will need to be squashed with this one.
> This patch is based on the mainline.Adding Deepthi, the author of
> the patch which introduced the powernv cpuidle driver. Deepthi,
> do you think the below patch looks right? We do not need to do an
> explicit local_irq_enable() since we are in the call path of
> cpuidle driver and that explicitly enables irqs on exit from
> idle states.

Yes, We enable irqs explicitly while entering snooze loop and we always
have interrupts enabled in the snooze state.
For NAP state, we exit out of this state with interrupts enabled so we
do not need an explicit enable of irqs.

> On 02/07/2014 06:47 AM, Nicolas Pitre wrote:
>> On Thu, 6 Feb 2014, Preeti U Murthy wrote:
>>
>>> Hi Daniel,
>>>
>>> On 02/06/2014 09:55 PM, Daniel Lezcano wrote:
>>>> Hi Nico,
>>>>
>>>>
>>>> On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>>>>
>>>>> The core idle loop now takes care of it.
>>>>>
>>>>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>>>>> ---
>>>>>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>>>>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/platforms/powernv/setup.c
>>>>> b/arch/powerpc/platforms/powernv/setup.c
>>>>> index 21166f65c9..a932feb290 100644
>>>>> --- a/arch/powerpc/platforms/powernv/setup.c
>>>>> +++ b/arch/powerpc/platforms/powernv/setup.c
>>>>> @@ -26,7 +26,6 @@
>>>>>  #include <linux/of_fdt.h>
>>>>>  #include <linux/interrupt.h>
>>>>>  #include <linux/bug.h>
>>>>> -#include <linux/cpuidle.h>
>>>>>
>>>>>  #include <asm/machdep.h>
>>>>>  #include <asm/firmware.h>
>>>>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>>>>>         return 1;
>>>>>  }
>>>>>
>>>>> -void powernv_idle(void)
>>>>> -{
>>>>> -       /* Hook to cpuidle framework if available, else
>>>>> -        * call on default platform idle code
>>>>> -        */
>>>>> -       if (cpuidle_idle_call()) {
>>>>> -               power7_idle();
>>>>> -       }
>>>>>
> 
>  drivers/cpuidle/cpuidle-powernv.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 78fd174..130f081 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -31,11 +31,13 @@ static int snooze_loop(struct cpuidle_device *dev,
>  	set_thread_flag(TIF_POLLING_NRFLAG);
> 
>  	while (!need_resched()) {
> +		ppc64_runlatch_off();
                ^^^^^^^^^^^^^^^
We could move this before the while() loop.
It would ideal to turn off latch when we enter snooze and
turn it on when we are about to exit, rather than doing
it over and over in the while loop.

>  		HMT_low();
>  		HMT_very_low();
>  	}
> 
>  	HMT_medium();
> +	ppc64_runlatch_on();
>  	clear_thread_flag(TIF_POLLING_NRFLAG);
>  	smp_mb();
>  	return index;
> @@ -45,7 +47,9 @@ static int nap_loop(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,
>  			int index)
>  {
> +	ppc64_runlatch_off();
>  	power7_idle();
> +	ppc64_runlatch_on();
>  	return index;
>  }
> 
> Thanks
> 
> Regards
> Preeti U Murthy
> 

Regards,
Deepthi

[PATCH 2/2] ARM64: powernv: remove redundant cpuidle_idle_call()

[Catalin Marinas]

On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> The core idle loop now takes care of it.
>
> Signed-off-by: Nicolas Pitre <nico@linaro.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

Hi Deepthi,

On 02/07/2014 03:15 PM, Deepthi Dharwar wrote:
> Hi Preeti,
> 
> Thanks for the patch.
> 
> On 02/07/2014 12:31 PM, Preeti U Murthy wrote:
>> Hi Nicolas,
>>
>> Find below the patch that will need to be squashed with this one.
>> This patch is based on the mainline.Adding Deepthi, the author of
>> the patch which introduced the powernv cpuidle driver. Deepthi,
>> do you think the below patch looks right? We do not need to do an
>> explicit local_irq_enable() since we are in the call path of
>> cpuidle driver and that explicitly enables irqs on exit from
>> idle states.
> 
> Yes, We enable irqs explicitly while entering snooze loop and we always
> have interrupts enabled in the snooze state.
> For NAP state, we exit out of this state with interrupts enabled so we
> do not need an explicit enable of irqs.
> 
>> On 02/07/2014 06:47 AM, Nicolas Pitre wrote:
>>> On Thu, 6 Feb 2014, Preeti U Murthy wrote:
>>>
>>>> Hi Daniel,
>>>>
>>>> On 02/06/2014 09:55 PM, Daniel Lezcano wrote:
>>>>> Hi Nico,
>>>>>
>>>>>
>>>>> On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>>>>>
>>>>>> The core idle loop now takes care of it.
>>>>>>
>>>>>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>>>>>> ---
>>>>>>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>>>>>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/platforms/powernv/setup.c
>>>>>> b/arch/powerpc/platforms/powernv/setup.c
>>>>>> index 21166f65c9..a932feb290 100644
>>>>>> --- a/arch/powerpc/platforms/powernv/setup.c
>>>>>> +++ b/arch/powerpc/platforms/powernv/setup.c
>>>>>> @@ -26,7 +26,6 @@
>>>>>>  #include <linux/of_fdt.h>
>>>>>>  #include <linux/interrupt.h>
>>>>>>  #include <linux/bug.h>
>>>>>> -#include <linux/cpuidle.h>
>>>>>>
>>>>>>  #include <asm/machdep.h>
>>>>>>  #include <asm/firmware.h>
>>>>>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>>>>>>         return 1;
>>>>>>  }
>>>>>>
>>>>>> -void powernv_idle(void)
>>>>>> -{
>>>>>> -       /* Hook to cpuidle framework if available, else
>>>>>> -        * call on default platform idle code
>>>>>> -        */
>>>>>> -       if (cpuidle_idle_call()) {
>>>>>> -               power7_idle();
>>>>>> -       }
>>>>>>
>>
>>  drivers/cpuidle/cpuidle-powernv.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
>> index 78fd174..130f081 100644
>> --- a/drivers/cpuidle/cpuidle-powernv.c
>> +++ b/drivers/cpuidle/cpuidle-powernv.c
>> @@ -31,11 +31,13 @@ static int snooze_loop(struct cpuidle_device *dev,
>>  	set_thread_flag(TIF_POLLING_NRFLAG);
>>
>>  	while (!need_resched()) {
>> +		ppc64_runlatch_off();
>                 ^^^^^^^^^^^^^^^
> We could move this before the while() loop.
> It would ideal to turn off latch when we enter snooze and
> turn it on when we are about to exit, rather than doing
> it over and over in the while loop.

You are right, this can be moved out of the loop.

Thanks

Regards
Preeti U Murthy

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Preeti U Murthy]

Hi Nicolas,

You will have to include the below patch with yours. You
could squash the two I guess, I have added the changelog
just for clarity. And you also might want to change the subject to
cpuidle/powernv. It gives a better picture.

Thanks

Regards
Preeti U Murthy


cpuidle/powernv: Add ppc64_runlatch_off/on() to idle routines

Following moving of cpuidle_idle_call() to the generic idle loop, we need to
add the runlatch functions to the idle routines on powernv which was earlier
taken care of by the arch specific idle routine.

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-powernv.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 78fd174..f48607c 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -14,6 +14,7 @@
 
 #include <asm/machdep.h>
 #include <asm/firmware.h>
+#include <asm/runlatch.h>
 
 struct cpuidle_driver powernv_idle_driver = {
 	.name             = "powernv_idle",
@@ -30,12 +31,14 @@ static int snooze_loop(struct cpuidle_device *dev,
 	local_irq_enable();
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
+	ppc64_runlatch_off();
 	while (!need_resched()) {
 		HMT_low();
 		HMT_very_low();
 	}
 
 	HMT_medium();
+	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
 	smp_mb();
 	return index;
@@ -45,7 +48,9 @@ static int nap_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			int index)
 {
+	ppc64_runlatch_off();
 	power7_idle();
+	ppc64_runlatch_on();
 	return index;
 }

 
On 02/06/2014 07:46 PM, Nicolas Pitre wrote:
> The core idle loop now takes care of it.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index 21166f65c9..a932feb290 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -26,7 +26,6 @@
>  #include <linux/of_fdt.h>
>  #include <linux/interrupt.h>
>  #include <linux/bug.h>
> -#include <linux/cpuidle.h>
> 
>  #include <asm/machdep.h>
>  #include <asm/firmware.h>
> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>  	return 1;
>  }
> 
> -void powernv_idle(void)
> -{
> -	/* Hook to cpuidle framework if available, else
> -	 * call on default platform idle code
> -	 */
> -	if (cpuidle_idle_call()) {
> -		power7_idle();
> -	}
> -}
> -
>  define_machine(powernv) {
>  	.name			= "PowerNV",
>  	.probe			= pnv_probe,
> @@ -236,7 +225,7 @@ define_machine(powernv) {
>  	.show_cpuinfo		= pnv_show_cpuinfo,
>  	.progress		= pnv_progress,
>  	.machine_shutdown	= pnv_shutdown,
> -	.power_save             = powernv_idle,
> +	.power_save             = power7_idle,
>  	.calibrate_decr		= generic_calibrate_decr,
>  #ifdef CONFIG_KEXEC
>  	.kexec_cpu_down		= pnv_kexec_cpu_down,
> 

[PATCH 1/2] PPC: powernv: remove redundant cpuidle_idle_call()

[Deepthi Dharwar]

On 02/14/2014 04:47 PM, Preeti U Murthy wrote:
> Hi Nicolas,
> 
> You will have to include the below patch with yours. You
> could squash the two I guess, I have added the changelog
> just for clarity. And you also might want to change the subject to
> cpuidle/powernv. It gives a better picture.
> 
> Thanks
> 
> Regards
> Preeti U Murthy
> 
> 
> cpuidle/powernv: Add ppc64_runlatch_off/on() to idle routines
> 
> Following moving of cpuidle_idle_call() to the generic idle loop, we need to
> add the runlatch functions to the idle routines on powernv which was earlier
> taken care of by the arch specific idle routine.
> 
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>

> ---
>  drivers/cpuidle/cpuidle-powernv.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 78fd174..f48607c 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -14,6 +14,7 @@
> 
>  #include <asm/machdep.h>
>  #include <asm/firmware.h>
> +#include <asm/runlatch.h>
> 
>  struct cpuidle_driver powernv_idle_driver = {
>  	.name             = "powernv_idle",
> @@ -30,12 +31,14 @@ static int snooze_loop(struct cpuidle_device *dev,
>  	local_irq_enable();
>  	set_thread_flag(TIF_POLLING_NRFLAG);
> 
> +	ppc64_runlatch_off();
>  	while (!need_resched()) {
>  		HMT_low();
>  		HMT_very_low();
>  	}
> 
>  	HMT_medium();
> +	ppc64_runlatch_on();
>  	clear_thread_flag(TIF_POLLING_NRFLAG);
>  	smp_mb();
>  	return index;
> @@ -45,7 +48,9 @@ static int nap_loop(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,
>  			int index)
>  {
> +	ppc64_runlatch_off();
>  	power7_idle();
> +	ppc64_runlatch_on();
>  	return index;
>  }
> 
> 
> On 02/06/2014 07:46 PM, Nicolas Pitre wrote:
>> The core idle loop now takes care of it.
>>
>> Signed-off-by: Nicolas Pitre <nico@linaro.org>
>> ---
>>  arch/powerpc/platforms/powernv/setup.c | 13 +------------
>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
>> index 21166f65c9..a932feb290 100644
>> --- a/arch/powerpc/platforms/powernv/setup.c
>> +++ b/arch/powerpc/platforms/powernv/setup.c
>> @@ -26,7 +26,6 @@
>>  #include <linux/of_fdt.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/bug.h>
>> -#include <linux/cpuidle.h>
>>
>>  #include <asm/machdep.h>
>>  #include <asm/firmware.h>
>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void)
>>  	return 1;
>>  }
>>
>> -void powernv_idle(void)
>> -{
>> -	/* Hook to cpuidle framework if available, else
>> -	 * call on default platform idle code
>> -	 */
>> -	if (cpuidle_idle_call()) {
>> -		power7_idle();
>> -	}
>> -}
>> -
>>  define_machine(powernv) {
>>  	.name			= "PowerNV",
>>  	.probe			= pnv_probe,
>> @@ -236,7 +225,7 @@ define_machine(powernv) {
>>  	.show_cpuinfo		= pnv_show_cpuinfo,
>>  	.progress		= pnv_progress,
>>  	.machine_shutdown	= pnv_shutdown,
>> -	.power_save             = powernv_idle,
>> +	.power_save             = power7_idle,
>>  	.calibrate_decr		= generic_calibrate_decr,
>>  #ifdef CONFIG_KEXEC
>>  	.kexec_cpu_down		= pnv_kexec_cpu_down,
>>
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>