diff mbox

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

Message ID 52F484D9.6020604@linux.vnet.ibm.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

preeti Feb. 7, 2014, 7:01 a.m. UTC
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(+)

Thanks

Regards
Preeti U Murthy

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

Comments

Deepthi Dharwar Feb. 7, 2014, 9:45 a.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
preeti Feb. 7, 2014, 11 a.m. UTC | #2
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/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;
 }