diff mbox

[4/8] ARM: vexpress: use generic cpu idle function for wfi

Message ID 1369665166-13049-5-git-send-email-sanjay.rawat@linaro.org
State New
Headers show

Commit Message

Sanjay Singh Rawat May 27, 2013, 2:32 p.m. UTC
use cpu_do_idle for entering the wfi mode.

Signed-off-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
---
 arch/arm/mach-vexpress/hotplug.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Sanjay Singh Rawat June 4, 2013, 3:23 p.m. UTC | #1
hello Russell, Nicolas,

can you please review this patch?


On Monday 27 May 2013 08:02 PM, Sanjay Singh Rawat wrote:
> use cpu_do_idle for entering the wfi mode.
>
> Signed-off-by: Sanjay Singh Rawat<sanjay.rawat@linaro.org>
> ---
>   arch/arm/mach-vexpress/hotplug.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
> index f0ce6b8..b3dffc2 100644
> --- a/arch/arm/mach-vexpress/hotplug.c
> +++ b/arch/arm/mach-vexpress/hotplug.c
> @@ -60,7 +60,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   	 * code will have already disabled interrupts
>   	 */
>   	for (;;) {
> -		wfi();
> +		/* enter WFI mode */
> +		cpu_do_idle();
>
>   		if (pen_release == cpu_logical_map(cpu)) {
>   			/*
Linus Walleij June 7, 2013, 9:17 a.m. UTC | #2
On Tue, Jun 4, 2013 at 5:23 PM, Sanjay Singh Rawat
<sanjay.rawat@linaro.org> wrote:

> hello Russell, Nicolas,
>
> can you please review this patch?

I think you may want Pawel to look at this, he usually looks
after the Vexpress.

Yours,
Linus Walleij
Pawel Moll June 7, 2013, 3:11 p.m. UTC | #3
On Mon, 2013-05-27 at 15:32 +0100, Sanjay Singh Rawat wrote:
> use cpu_do_idle for entering the wfi mode.
> 
> Signed-off-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
> ---
>  arch/arm/mach-vexpress/hotplug.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
> index f0ce6b8..b3dffc2 100644
> --- a/arch/arm/mach-vexpress/hotplug.c
> +++ b/arch/arm/mach-vexpress/hotplug.c
> @@ -60,7 +60,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>  	 * code will have already disabled interrupts
>  	 */
>  	for (;;) {
> -		wfi();
> +		/* enter WFI mode */
> +		cpu_do_idle();
>  
>  		if (pen_release == cpu_logical_map(cpu)) {
>  			/*

I probably don't get the whole picture, but may I ask what is the
rationale behind this change? As in: why cpu_do_idle() is better?

Paweł
Sanjay Singh Rawat June 11, 2013, 5:33 a.m. UTC | #4
commit: 8553cb67d2318db327071018fc81084cbabccc46 explains that

-sanjay

On Friday 07 June 2013 08:41 PM, Pawel Moll wrote:
> On Mon, 2013-05-27 at 15:32 +0100, Sanjay Singh Rawat wrote:
>> use cpu_do_idle for entering the wfi mode.
>>
>> Signed-off-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
>> ---
>>   arch/arm/mach-vexpress/hotplug.c |    3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
>> index f0ce6b8..b3dffc2 100644
>> --- a/arch/arm/mach-vexpress/hotplug.c
>> +++ b/arch/arm/mach-vexpress/hotplug.c
>> @@ -60,7 +60,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   	 * code will have already disabled interrupts
>>   	 */
>>   	for (;;) {
>> -		wfi();
>> +		/* enter WFI mode */
>> +		cpu_do_idle();
>>
>>   		if (pen_release == cpu_logical_map(cpu)) {
>>   			/*
>
> I probably don't get the whole picture, but may I ask what is the
> rationale behind this change? As in: why cpu_do_idle() is better?
>
> Paweł
>
>
Pawel Moll June 11, 2013, 10:07 a.m. UTC | #5
On Tue, 2013-06-11 at 06:33 +0100, Sanjay Singh Rawat wrote:
> >> use cpu_do_idle for entering the wfi mode.
> >>
> >> Signed-off-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
> >> ---
> >>   arch/arm/mach-vexpress/hotplug.c |    3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
> >> index f0ce6b8..b3dffc2 100644
> >> --- a/arch/arm/mach-vexpress/hotplug.c
> >> +++ b/arch/arm/mach-vexpress/hotplug.c
> >> @@ -60,7 +60,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
> >>   	 * code will have already disabled interrupts
> >>   	 */
> >>   	for (;;) {
> >> -		wfi();
> >> +		/* enter WFI mode */
> >> +		cpu_do_idle();
> >>
> >>   		if (pen_release == cpu_logical_map(cpu)) {
> >>   			/*
> >
> > I probably don't get the whole picture, but may I ask what is the
> > rationale behind this change? As in: why cpu_do_idle() is better?
>
> commit: 8553cb67d2318db327071018fc81084cbabccc46 explains that

Oh, I've noticed the dsb there, I'm just asking why is it necessary in
the cpu_die() case? The core is going down anyway, it left the coherency
domain and we could cut the power now (if only it was possible). All
cache maintenance (which undoubtedly included dsb) has been long done.

Now, don't get me wrong - I'm not saying it's wrong to do the dsb. I
simply would like to know if and why is required (I'm admitting my
ignorance here ;-) But if you simply grep-ed for "wfi()" in the whole
kernel and automatically replaced them with "cpu_do_idle()", it doesn't
make sense.

Regards

Pawel
Nicolas Pitre June 11, 2013, 3:30 p.m. UTC | #6
On Tue, 11 Jun 2013, Pawel Moll wrote:

> On Tue, 2013-06-11 at 06:33 +0100, Sanjay Singh Rawat wrote:
> > >> use cpu_do_idle for entering the wfi mode.
> > >>
> > >> Signed-off-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
> > >> ---
> > >>   arch/arm/mach-vexpress/hotplug.c |    3 ++-
> > >>   1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
> > >> index f0ce6b8..b3dffc2 100644
> > >> --- a/arch/arm/mach-vexpress/hotplug.c
> > >> +++ b/arch/arm/mach-vexpress/hotplug.c
> > >> @@ -60,7 +60,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
> > >>   	 * code will have already disabled interrupts
> > >>   	 */
> > >>   	for (;;) {
> > >> -		wfi();
> > >> +		/* enter WFI mode */
> > >> +		cpu_do_idle();
> > >>
> > >>   		if (pen_release == cpu_logical_map(cpu)) {
> > >>   			/*
> > >
> > > I probably don't get the whole picture, but may I ask what is the
> > > rationale behind this change? As in: why cpu_do_idle() is better?
> >
> > commit: 8553cb67d2318db327071018fc81084cbabccc46 explains that
> 
> Oh, I've noticed the dsb there, I'm just asking why is it necessary in
> the cpu_die() case? The core is going down anyway, it left the coherency
> domain and we could cut the power now (if only it was possible). All
> cache maintenance (which undoubtedly included dsb) has been long done.
> 
> Now, don't get me wrong - I'm not saying it's wrong to do the dsb. I
> simply would like to know if and why is required (I'm admitting my
> ignorance here ;-) But if you simply grep-ed for "wfi()" in the whole
> kernel and automatically replaced them with "cpu_do_idle()", it doesn't
> make sense.

That's my feeling too.  There is a semantic difference between calling 
cpu_do_idle() and executing a WFI inline.  I'd use cpu_do_idle() only in 
those cases where the call is expected to return and keep the inlined 
WFI otherwise (with the added DSB when necessary which is not all cases 
as explained above).

Nicolas
Sanjay Singh Rawat June 12, 2013, 5:34 a.m. UTC | #7
Pawel, Nicolas,

Thanks for the clarification :), dropping.

sanjay

On Tuesday 11 June 2013 09:00 PM, Nicolas Pitre wrote:
> On Tue, 11 Jun 2013, Pawel Moll wrote:
>
>> On Tue, 2013-06-11 at 06:33 +0100, Sanjay Singh Rawat wrote:
>>>>> use cpu_do_idle for entering the wfi mode.
>>>>>
>>>>> Signed-off-by: Sanjay Singh Rawat <sanjay.rawat@linaro.org>
>>>>> ---
>>>>>    arch/arm/mach-vexpress/hotplug.c |    3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
>>>>> index f0ce6b8..b3dffc2 100644
>>>>> --- a/arch/arm/mach-vexpress/hotplug.c
>>>>> +++ b/arch/arm/mach-vexpress/hotplug.c
>>>>> @@ -60,7 +60,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>>>>    	 * code will have already disabled interrupts
>>>>>    	 */
>>>>>    	for (;;) {
>>>>> -		wfi();
>>>>> +		/* enter WFI mode */
>>>>> +		cpu_do_idle();
>>>>>
>>>>>    		if (pen_release == cpu_logical_map(cpu)) {
>>>>>    			/*
>>>>
>>>> I probably don't get the whole picture, but may I ask what is the
>>>> rationale behind this change? As in: why cpu_do_idle() is better?
>>>
>>> commit: 8553cb67d2318db327071018fc81084cbabccc46 explains that
>>
>> Oh, I've noticed the dsb there, I'm just asking why is it necessary in
>> the cpu_die() case? The core is going down anyway, it left the coherency
>> domain and we could cut the power now (if only it was possible). All
>> cache maintenance (which undoubtedly included dsb) has been long done.
>>
>> Now, don't get me wrong - I'm not saying it's wrong to do the dsb. I
>> simply would like to know if and why is required (I'm admitting my
>> ignorance here ;-) But if you simply grep-ed for "wfi()" in the whole
>> kernel and automatically replaced them with "cpu_do_idle()", it doesn't
>> make sense.
>
> That's my feeling too.  There is a semantic difference between calling
> cpu_do_idle() and executing a WFI inline.  I'd use cpu_do_idle() only in
> those cases where the call is expected to return and keep the inlined
> WFI otherwise (with the added DSB when necessary which is not all cases
> as explained above).
>
> Nicolas
>
diff mbox

Patch

diff --git a/arch/arm/mach-vexpress/hotplug.c b/arch/arm/mach-vexpress/hotplug.c
index f0ce6b8..b3dffc2 100644
--- a/arch/arm/mach-vexpress/hotplug.c
+++ b/arch/arm/mach-vexpress/hotplug.c
@@ -60,7 +60,8 @@  static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 	 * code will have already disabled interrupts
 	 */
 	for (;;) {
-		wfi();
+		/* enter WFI mode */
+		cpu_do_idle();
 
 		if (pen_release == cpu_logical_map(cpu)) {
 			/*