[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[Jonas Jensen]

As it was already suggested by Russell King and Arnd Bergmann ( https://lkml.org/lkml/2013/5/16/133 ), moxart and gemini seem to be the only platforms using CPU_FA526, and instead of pointing arm_pm_idle to an empty function from platform code, it makes sense to remove WFI code from the processor specific idle function.

Note: moxart boots and prints to UART without this patch, but input is broken.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---
 arch/arm/mm/proc-fa526.S |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/proc-fa526.S b/arch/arm/mm/proc-fa526.S
index d217e97..85280b1 100644
--- a/arch/arm/mm/proc-fa526.S
+++ b/arch/arm/mm/proc-fa526.S
@@ -81,9 +81,7 @@ ENDPROC(cpu_fa526_reset)
  */
 	.align	4
 ENTRY(cpu_fa526_do_idle)
-	mcr	p15, 0, r0, c7, c0, 4		@ Wait for interrupt
-	mov	pc, lr
-
+	nop
 
 ENTRY(cpu_fa526_dcache_clean_area)
 1:	mcr	p15, 0, r0, c7, c10, 1		@ clean D entry
-- 
1.7.2.5

[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[Arnd Bergmann]

On Monday 17 June 2013 15:39:20 Jonas Jensen wrote:
> As it was already suggested by Russell King and Arnd Bergmann ( https://lkml.org/lkml/2013/5/16/133 ), moxart and gemini seem to be the only platforms using CPU_FA526, and instead of pointing arm_pm_idle to an empty function from platform code, it makes sense to remove WFI code from the processor specific idle function.
> 
> Note: moxart boots and prints to UART without this patch, but input is broken.

Please make sure you have proper line wraps. Lines in checkin
comments should be no longer than 70 characters normally.

> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Since this patch is independent from the actual platform support,
it should go through Russell's patch tracker at
http://www.arm.linux.org.uk/developer/patches/

>  arch/arm/mm/proc-fa526.S |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mm/proc-fa526.S b/arch/arm/mm/proc-fa526.S
> index d217e97..85280b1 100644
> --- a/arch/arm/mm/proc-fa526.S
> +++ b/arch/arm/mm/proc-fa526.S
> @@ -81,9 +81,7 @@ ENDPROC(cpu_fa526_reset)
>   */
>  	.align	4
>  ENTRY(cpu_fa526_do_idle)
> -	mcr	p15, 0, r0, c7, c0, 4		@ Wait for interrupt
> -	mov	pc, lr
> -
> +	nop
>  
>  ENTRY(cpu_fa526_dcache_clean_area)
>  1:	mcr	p15, 0, r0, c7, c10, 1		@ clean D entry

Adding Imre and Florian to Cc for Gemini. They might have a comment
about this. I think we should delete arch/arm/mach-gemini/idle.c
after this as well.

	Arnd

[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[Russell King - ARM Linux]

On Mon, Jun 17, 2013 at 04:14:03PM +0200, Arnd Bergmann wrote:
> On Monday 17 June 2013 15:39:20 Jonas Jensen wrote:
> > As it was already suggested by Russell King and Arnd Bergmann ( https://lkml.org/lkml/2013/5/16/133 ), moxart and gemini seem to be the only platforms using CPU_FA526, and instead of pointing arm_pm_idle to an empty function from platform code, it makes sense to remove WFI code from the processor specific idle function.
> > 
> > Note: moxart boots and prints to UART without this patch, but input is broken.
> 
> Please make sure you have proper line wraps. Lines in checkin
> comments should be no longer than 70 characters normally.
> 
> > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> Since this patch is independent from the actual platform support,
> it should go through Russell's patch tracker at
> http://www.arm.linux.org.uk/developer/patches/

Except... the patch is wrong.

> >  arch/arm/mm/proc-fa526.S |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mm/proc-fa526.S b/arch/arm/mm/proc-fa526.S
> > index d217e97..85280b1 100644
> > --- a/arch/arm/mm/proc-fa526.S
> > +++ b/arch/arm/mm/proc-fa526.S
> > @@ -81,9 +81,7 @@ ENDPROC(cpu_fa526_reset)
> >   */
> >  	.align	4
> >  ENTRY(cpu_fa526_do_idle)
> > -	mcr	p15, 0, r0, c7, c0, 4		@ Wait for interrupt
> > -	mov	pc, lr
> > -
> > +	nop

It replaces the WFI and return with a pure nop, so we will fall through
to cpu_fa526_dcache_clean_area().  Do we really want to clean a random
D cache entry depending on what r0 happened to hold at this point?  I
think not...

[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[Jonas Jensen]

On 17 June 2013 16:30, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> It replaces the WFI and return with a pure nop, so we will fall through
> to cpu_fa526_dcache_clean_area().  Do we really want to clean a random
> D cache entry depending on what r0 happened to hold at this point?  I
> think not...

I'm sorry for making the assumption that I know even the most basic
ARM assembler...

This is not the sort of thing I deal with (and probably shouldn't) on
a daily basis.

Can I make a guess, remove only mcr and replace it with nop?

Best regards,
Jonas

[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[Arnd Bergmann]

On Monday 17 June 2013 16:58:14 Jonas Jensen wrote:
> On 17 June 2013 16:30, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > It replaces the WFI and return with a pure nop, so we will fall through
> > to cpu_fa526_dcache_clean_area().  Do we really want to clean a random
> > D cache entry depending on what r0 happened to hold at this point?  I
> > think not...
> 
> I'm sorry for making the assumption that I know even the most basic
> ARM assembler...
> 
> This is not the sort of thing I deal with (and probably shouldn't) on
> a daily basis.
> 
> Can I make a guess, remove only mcr and replace it with nop?

No need for the nop, just remove the mcr instruction.

	Arnd

[PATCH] ARM: mm: cpu_fa526_do_idle: remove WFI

[Arnd Bergmann]

On Monday 17 June 2013 15:30:21 Russell King - ARM Linux wrote:
> > > diff --git a/arch/arm/mm/proc-fa526.S b/arch/arm/mm/proc-fa526.S
> > > index d217e97..85280b1 100644
> > > --- a/arch/arm/mm/proc-fa526.S
> > > +++ b/arch/arm/mm/proc-fa526.S
> > > @@ -81,9 +81,7 @@ ENDPROC(cpu_fa526_reset)
> > >   */
> > >     .align  4
> > >  ENTRY(cpu_fa526_do_idle)
> > > -   mcr     p15, 0, r0, c7, c0, 4           @ Wait for interrupt
> > > -   mov     pc, lr
> > > -
> > > +   nop
> 
> It replaces the WFI and return with a pure nop, so we will fall through
> to cpu_fa526_dcache_clean_area().  Do we really want to clean a random
> D cache entry depending on what r0 happened to hold at this point?  I
> think not...

Right, I should have actually read the change...

	Arnd