Patchwork [U-Boot] mpc83xx: remove some dead code, saving space

login
register
mail settings
Submitter Joakim Tjernlund
Date Nov. 23, 2010, 6:33 p.m.
Message ID <1290537223-12160-1-git-send-email-Joakim.Tjernlund@transmode.se>
Download mbox | patch
Permalink /patch/72713/
State Accepted
Delegated to: Kim Phillips
Headers show

Comments

Joakim Tjernlund - Nov. 23, 2010, 6:33 p.m.
Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 arch/powerpc/cpu/mpc83xx/start.S |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)
Kim Phillips - Nov. 28, 2010, 3:14 p.m.
On Tue, 23 Nov 2010 19:33:43 +0100
Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote:

> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---

applied.

Thanks,

Kim
Wolfgang Denk - Nov. 30, 2010, 8:07 p.m.
Dear Joakim Tjernlund,

In message <1290537223-12160-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  arch/powerpc/cpu/mpc83xx/start.S |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S
> index f7da14b..121c276 100644
> --- a/arch/powerpc/cpu/mpc83xx/start.S
> +++ b/arch/powerpc/cpu/mpc83xx/start.S
> @@ -967,13 +967,6 @@ relocate_code:
>  30:	li r3, 0
>  	blr
>  
> -2:	slwi	r0,r0,2 /* re copy in reverse order ... y do we needed it? */
> -	add	r8,r4,r0
> -	add	r7,r3,r0
> -3:	lwzu	r0,-4(r8)
> -	stwu	r0,-4(r7)
> -	bdnz	3b
> -

Why do you think this is dead code?

I see this about 30 lines above:

 ...
 851         cmplw   cr1,r3,r4
 852         addi    r0,r5,3
 853         srwi.   r0,r0,2
 854         beq     cr1,4f          /* In place copy is not necessary
*/
 855         beq     7f              /* Protect against 0 count
*/
 856         mtctr   r0
 857         bge     cr1,2f
 ...

With your removal of the code, the "bge     cr1,2f" will jump right
into the relocation loop.  I don't think this is intended?

Best regards,

Wolfgang Denk
Joakim Tjernlund - Nov. 30, 2010, 8:45 p.m.
>
> Dear Joakim Tjernlund,
>
> In message <1290537223-12160-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  arch/powerpc/cpu/mpc83xx/start.S |    7 -------
> >  1 files changed, 0 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S
> > index f7da14b..121c276 100644
> > --- a/arch/powerpc/cpu/mpc83xx/start.S
> > +++ b/arch/powerpc/cpu/mpc83xx/start.S
> > @@ -967,13 +967,6 @@ relocate_code:
> >  30:   li r3, 0
> >     blr
> >
> > -2:   slwi   r0,r0,2 /* re copy in reverse order ... y do we needed it? */
> > -   add   r8,r4,r0
> > -   add   r7,r3,r0
> > -3:   lwzu   r0,-4(r8)
> > -   stwu   r0,-4(r7)
> > -   bdnz   3b
> > -
>
> Why do you think this is dead code?
>
> I see this about 30 lines above:
>
>  ...
>  851         cmplw   cr1,r3,r4
>  852         addi    r0,r5,3
>  853         srwi.   r0,r0,2
>  854         beq     cr1,4f          /* In place copy is not necessary
> */
>  855         beq     7f              /* Protect against 0 count
> */
>  856         mtctr   r0
>  857         bge     cr1,2f
>  ...
>
> With your removal of the code, the "bge     cr1,2f" will jump right
> into the relocation loop.  I don't think this is intended?

Ah, sharp eyes! Didn't notice that branch. I do think that
bge could be removed as well. It will only matter if the src and dst overlap
and the distance between them is < 4 bytes.
Anyhow, we don't need to remove this code now.

On a related note, I am not sure why the I and D cache needs to be flushed,
aren't they coherent?

 Jocke
Scott Wood - Nov. 30, 2010, 8:50 p.m.
On Tue, 30 Nov 2010 21:45:19 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> On a related note, I am not sure why the I and D cache needs to be flushed,
> aren't they coherent?

They are not.

-Scott
Joakim Tjernlund - Nov. 30, 2010, 9:13 p.m.
Scott Wood <scottwood@freescale.com> wrote on 2010/11/30 21:50:52:
>
> On Tue, 30 Nov 2010 21:45:19 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> > On a related note, I am not sure why the I and D cache needs to be flushed,
> > aren't they coherent?
>
> They are not.

Ah, I figured they would be these days.
I doubt one needs to invalidate the icache though?
Better safe than sorry I guess, one could probably optimize it a
little bit though.

 Jocke
Scott Wood - Nov. 30, 2010, 9:17 p.m.
On Tue, 30 Nov 2010 22:13:32 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Scott Wood <scottwood@freescale.com> wrote on 2010/11/30 21:50:52:
> >
> > On Tue, 30 Nov 2010 21:45:19 +0100
> > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> >
> > > On a related note, I am not sure why the I and D cache needs to be flushed,
> > > aren't they coherent?
> >
> > They are not.
> 
> Ah, I figured they would be these days.

Nope, I've seen weird stuff happen when I forget to sync the icache
even on very recent chips (albeit 85xx and not 83xx).

> I doubt one needs to invalidate the icache though?
> Better safe than sorry I guess, one could probably optimize it a
> little bit though.

You need to flush the dcache and then invalidate the icache.

-Scott
Joakim Tjernlund - Nov. 30, 2010, 9:25 p.m.
Scott Wood <scottwood@freescale.com> wrote on 2010/11/30 22:17:31:
>
> On Tue, 30 Nov 2010 22:13:32 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> > Scott Wood <scottwood@freescale.com> wrote on 2010/11/30 21:50:52:
> > >
> > > On Tue, 30 Nov 2010 21:45:19 +0100
> > > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> > >
> > > > On a related note, I am not sure why the I and D cache needs to be flushed,
> > > > aren't they coherent?
> > >
> > > They are not.
> >
> > Ah, I figured they would be these days.
>
> Nope, I've seen weird stuff happen when I forget to sync the icache
> even on very recent chips (albeit 85xx and not 83xx).
>
> > I doubt one needs to invalidate the icache though?
> > Better safe than sorry I guess, one could probably optimize it a
> > little bit though.
>
> You need to flush the dcache and then invalidate the icache.

but if one haven't executed any insn's at the new location yet
there should not be any cache lines in the icache that
match the new location.
Scott Wood - Dec. 1, 2010, 12:21 a.m.
On Tue, 30 Nov 2010 22:25:11 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Scott Wood <scottwood@freescale.com> wrote on 2010/11/30 22:17:31:
> >
> > On Tue, 30 Nov 2010 22:13:32 +0100
> > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> >
> > > Scott Wood <scottwood@freescale.com> wrote on 2010/11/30 21:50:52:
> > > >
> > > > On Tue, 30 Nov 2010 21:45:19 +0100
> > > > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> > > >
> > > > > On a related note, I am not sure why the I and D cache needs to be flushed,
> > > > > aren't they coherent?
> > > >
> > > > They are not.
> > >
> > > Ah, I figured they would be these days.
> >
> > Nope, I've seen weird stuff happen when I forget to sync the icache
> > even on very recent chips (albeit 85xx and not 83xx).
> >
> > > I doubt one needs to invalidate the icache though?
> > > Better safe than sorry I guess, one could probably optimize it a
> > > little bit though.
> >
> > You need to flush the dcache and then invalidate the icache.
> 
> but if one haven't executed any insn's at the new location yet
> there should not be any cache lines in the icache that
> match the new location.

Ah.  That's a special case. :-)

Still, better to set a good example.

-Scott

Patch

diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S
index f7da14b..121c276 100644
--- a/arch/powerpc/cpu/mpc83xx/start.S
+++ b/arch/powerpc/cpu/mpc83xx/start.S
@@ -967,13 +967,6 @@  relocate_code:
 30:	li r3, 0
 	blr
 
-2:	slwi	r0,r0,2 /* re copy in reverse order ... y do we needed it? */
-	add	r8,r4,r0
-	add	r7,r3,r0
-3:	lwzu	r0,-4(r8)
-	stwu	r0,-4(r7)
-	bdnz	3b
-
 /*
  * Now flush the cache: note that we must start from a cache aligned
  * address. Otherwise we might miss one cache line.