| Message ID | 1290537223-12160-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | Accepted |
| Delegated to: | Kim Phillips |
| Headers | show |
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
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
> > 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
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
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
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
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.
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
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.
Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> --- arch/powerpc/cpu/mpc83xx/start.S | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)