Make MPC83xx one step closer to full relocation.

Remove a few absolute references to CFG_MONITOR_BASE for ppc/mpc83xx and use GOT relative reference.

---
 cpu/mpc83xx/start.S |   11 +++++++----
 lib_ppc/board.c     |    3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

-----Original Message-----
From: Kim Phillips [mailto:kim.phillips at freescale.com]
Sent: den 29 mars 2008 00:33
To: Joakim Tjernlund
Cc: u-boot-users at lists.sourceforge.net
Subject: Re: [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 6 lines · On Fri, 28 Mar 2008 15:41:25 +0100 Joakim Tjernlund…

Remove a few absolute references to CFG_MONITOR_BASE for ppc/mpc83xx and use GOT relative reference.

---
 cpu/mpc83xx/start.S |   11 +++++++----
 lib_ppc/board.c     |    3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

Joakim,

this patch is missing your signoff, and doesn't apply cleanly (even adds whitespace).

btw, shouldn't it fix up the other ppc start.S files?

Kim

Quoted 17 lines

-----Original Message-----
From: Kim Phillips [mailto:kim.phillips at freescale.com]
Sent: den 29 mars 2008 00:33
To: Joakim Tjernlund
Cc: u-boot-users at lists.sourceforge.net
Subject: Re: [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 6 lines · On Fri, 28 Mar 2008 15:41:25 +0100 Joakim Tjernlund…

Remove a few absolute references to CFG_MONITOR_BASE for ppc/mpc83xx and use GOT relative reference.

---
 cpu/mpc83xx/start.S |   11 +++++++----
 lib_ppc/board.c     |    3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

Quoted 4 lines

Joakim,

this patch is missing your signoff, and doesn't apply cleanly (even adds whitespace).

It doesn't apply because you have been sitting on it for too long :) I sent long ago and you said would apply it in the next merge window, but seems to have forgotten about it. Sorry for the whitespace. I am still on the last u-boot release so I am afraid I can't do much about it ATM. I hope you can clean up the WS.

Signed-off-by Joakim Tjernlund <[email protected]>

-----Original Message-----
From: Kim Phillips [mailto:kim.phillips at freescale.com]
Sent: den 29 mars 2008 01:01
To: Joakim Tjernlund
Cc: u-boot-users at lists.sourceforge.net
Subject: Re: [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 28 lines · On Sat, 29 Mar 2008 00:47:01 +0100 "Joakim Tjernlund"…

Quoted 17 lines

-----Original Message-----
From: Kim Phillips [mailto:kim.phillips at freescale.com]
Sent: den 29 mars 2008 00:33
To: Joakim Tjernlund
Cc: u-boot-users at lists.sourceforge.net
Subject: Re: [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 6 lines · On Fri, 28 Mar 2008 15:41:25 +0100 Joakim Tjernlund…

Remove a few absolute references to CFG_MONITOR_BASE for ppc/mpc83xx and use GOT relative reference.

---
 cpu/mpc83xx/start.S |   11 +++++++----
 lib_ppc/board.c     |    3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

Quoted 4 lines

Joakim,

this patch is missing your signoff, and doesn't apply cleanly (even adds whitespace).

It doesn't apply because you have been sitting on it for too long :) I sent long ago and you said would apply it in the next merge window, but seems to have forgotten about it. Sorry for the whitespace. I am still on the last u-boot release so I am afraid I can't do much about it ATM. I hope you can clean up the WS.

Quoted 1 line

Signed-off-by Joakim Tjernlund <[email protected]>

ok, I can add it now.

Kim

Quoted 7 lines

Quoted 6 lines · Joakim Tjernlund

Remove a few absolute references to CFG_MONITOR_BASE for ppc/mpc83xx and use GOT relative reference.

---
 cpu/mpc83xx/start.S |   11 +++++++----
 lib_ppc/board.c     |    3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

Quoted 1 line

btw, shouldn't it fix up the other ppc start.S files?

The other ppc's won't break and I only got a 83xx, hence I only did it for 83xx, hoping it would serve as a guide for the rest.

Hi Joakim,

Quoted 7 lines · Joakim Tjernlund

Quoted 7 lines

Quoted 6 lines · Joakim Tjernlund

Remove a few absolute references to CFG_MONITOR_BASE for ppc/mpc83xx and use GOT relative reference.

---
 cpu/mpc83xx/start.S |   11 +++++++----
 lib_ppc/board.c     |    3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

<snip>

Quoted 4 lines

Quoted 1 line

btw, shouldn't it fix up the other ppc start.S files?

The other ppc's won't break and I only got a 83xx, hence I only did it for 83xx, hoping it would serve as a guide for the rest.

I'm afraid, but the "other ppc's" did break with this patch. At least 4xx does. With your patch applied relocation to SDRAM does not work anymore. Here's what I get:

CFG_MONITOR_BASE=fffa0000
(ulong)&_start + EXC_OFF_SYS_RESET=fffa2200
EXC_OFF_SYS_RESET=100

I haven't looked into it closer yet. Any idea on how to fix this?

Thanks.

Best regards,
Stefan

Quoted 10 lines

I'm afraid, but the "other ppc's" did break with this patch. At least 4xx does. With your patch applied relocation to SDRAM does not work anymore. Here's what I get:

CFG_MONITOR_BASE=fffa0000
(ulong)&_start + EXC_OFF_SYS_RESET=fffa2200
EXC_OFF_SYS_RESET=100

I haven't looked into it closer yet. Any idea on how to fix this?

Quoted 4 lines

Thanks.

Best regards,
Stefan

Oops, didn't see that coming. Your _start symbol in ppc4xx/start.S isn't pointing to your real start of execution. Seems like _start_440 is your real start but I can't be sure.

There are some strange code in there
that I don't understand.

Quoted 10 lines · Joakim Tjernlund

Quoted 10 lines

I'm afraid, but the "other ppc's" did break with this patch. At least 4xx does. With your patch applied relocation to SDRAM does not work anymore. Here's what I get:

CFG_MONITOR_BASE=fffa0000
(ulong)&_start + EXC_OFF_SYS_RESET=fffa2200
EXC_OFF_SYS_RESET=100

I haven't looked into it closer yet. Any idea on how to fix this?

Quoted 8 lines

Quoted 4 lines

Thanks.

Best regards,
Stefan

Oops, didn't see that coming. Your _start symbol in ppc4xx/start.S isn't pointing to your real start of execution. Seems like _start_440 is your real start but I can't be sure.

No, unfortunately it's not. _start_440 is loaded into the last 4k of bootrom (via linker script), since 440 has a shadow TLB upon startup to only map 4k of address space. After looking at System.map it seems that _start_of_vectors seems to be the way to go for 4xx:

fffa0004 T version_string
fffa0100 t CritcalInput
fffa0100 T _start_of_vectors

But I don't want to introduce some #ifdef here. Perhaps it would be better to introduce a common (PPC) label that points to the beginning of the U-Boot image here.

BTW: I think this version:

	len = (ulong)&_end - ((ulong)&_start - EXC_OFF_SYS_RESET);

instead of:

	len = (ulong)&_end - (ulong)&_start + EXC_OFF_SYS_RESET;

is better. Makes the transition from CFG_MONITOR_BASE clearer, don't you think so?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

CFG_MONITOR_BASE=fffa0000

I first did a new symbol for this but changed it to use _start as I didn't want to introduce yet another symbol. I would hope it is possible to rework 4xx to move the _start symbol to were it belongs? Not having the _start symbol where it should be could bite you some other day too.

Quoted 2 lines · In message <[email protected]> you wrote:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

CFG_MONITOR_BASE=fffa0000

...

Quoted 5 lines

I first did a new symbol for this but changed it to use _start as I didn't want to introduce yet another symbol. I would hope it is possible to rework 4xx to move the _start symbol to were it belongs? Not having the _start symbol where it should be could bite you some other day too.

I think you got this wrong. _start *is* where it should be, where the execution of the code begins. Where you set the breakpoint in gdb.

The "new symbol" you mention corresponds to CFG_MONITOR_BASE, but note that it has no fixed address as it depends where you place your image in flash.

Quoted 8 lines

Quoted 5 lines

I first did a new symbol for this but changed it to use _start as I didn't want to introduce yet another symbol. I would hope it is possible to rework 4xx to move the _start symbol to were it belongs? Not having the _start symbol where it should be could bite you some other day too.

I think you got this wrong. _start *is* where it should be, where the execution of the code begins. Where you set the breakpoint in gdb.

Ah, right. I *assumed* that _start was always the first symbol in the text segment too. On 4xx it isn't for some reason, but it should be possible to move it first.

Sidnote: Does execution really begin at _start for 4xx? I still looks like it begin at _start_440 and eventually end up at _start

Quoted 3 lines

The "new symbol" you mention corresponds to CFG_MONITOR_BASE, but note that it has no fixed address as it depends where you place your image in flash.

Yes, but the address is not important here, it is the difference _end - _start. I guess we could define a linker symbol too that calculates _end - _start for us and then just do
len = _uboot_reloc_size + EXC_OFF_SYS_RESET;

or define a new symbol that is initialised to CFG_MONITOR_BASE or let the linker place it at the beginning of .text, hopefully there is already a name reserved for the symbol although I don't know of such a name. I either case I think one needs to add that symbol to the GOT list in start.S

Quoted 13 lines · Joakim Tjernlund

Quoted 8 lines

Quoted 5 lines

I first did a new symbol for this but changed it to use _start as I didn't want to introduce yet another symbol. I would hope it is possible to rework 4xx to move the _start symbol to were it belongs? Not having the _start symbol where it should be could bite you some other day too.

I think you got this wrong. _start *is* where it should be, where the execution of the code begins. Where you set the breakpoint in gdb.

Ah, right. I *assumed* that _start was always the first symbol in the text segment too. On 4xx it isn't for some reason, but it should be possible to move it first.

Quoted 2 lines

Sidnote: Does execution really begin at _start for 4xx? I still looks like it begin at _start_440 and eventually end up at _start

On 4xx execution always starts at 0xfffffffc (last lword in 32bit address space). This location holds a jump to _start for 405 PPC's and to _start_440 for 440 PPC's. 440 PPC's need some extended initialization (TLB setup etc) and later jump to the 4xx common _start.

Quoted 10 lines

Quoted 4 lines

The "new symbol" you mention corresponds to CFG_MONITOR_BASE, but note that it has no fixed address as it depends where you place your image in flash.

Yes, but the address is not important here, it is the difference _end - _start. I guess we could define a linker symbol too that calculates _end - _start for us and then just do
len = _uboot_reloc_size + EXC_OFF_SYS_RESET;

Quoted 5 lines

or define a new symbol that is initialised to CFG_MONITOR_BASE or let the linker place it at the beginning of .text, hopefully there is already a name reserved for the symbol although I don't know of such a name. I either case I think one needs to add that symbol to the GOT list in start.S

I'm an linker script dyslexic. So no idea if we can handle this solely in the linker script or if we need a new common symbol in the PPC start.S's.

Quoted 4 lines

On 4xx execution always starts at 0xfffffffc (last lword in 32bit address space). This location holds a jump to _start for 405 PPC's and to _start_440 for 440 PPC's. 440 PPC's need some extended initialization (TLB setup etc) and later jump to the 4xx common _start.

OK, then it is like I suspected. What if you rename _start to _common_start. Make _start equal _common_start for 405 and rename _start_440 to _start, i.e make sure that _start is defined where you start executing after the jump.

Quoted 2 lines

I'm an linker script dyslexic. So no idea if we can handle this solely in the linker script or if we need a new common symbol in the PPC start.S's.

Both ways should be doable I think. A linker script would probably look something like(pseudo diff below):
.text :
{

(quoted diff) +2 -0

+    _monitor_base = . ;
+    PROVIDE (_monitor_base = .);
    cpu/mpc83xx/start.o	(.text)

And then add a
GOT_ENTRY(_monitor_base)
in start.S

-----Original Message-----
From: u-boot-users-bounces at lists.sourceforge.net [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf Of Stefan Roese
Sent: den 8 april 2008 21:53
To: joakim.tjernlund at transmode.se
Cc: u-boot-users at lists.sourceforge.net; 'Kim Phillips'; Wolfgang Denk
Subject: Re: [U-Boot-Users] [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 9 lines · Joakim Tjernlund

Quoted 4 lines

On 4xx execution always starts at 0xfffffffc (last lword in 32bit address space). This location holds a jump to _start for 405 PPC's and to _start_440 for 440 PPC's. 440 PPC's need some extended initialization (TLB setup etc) and later jump to the 4xx common _start.

OK, then it is like I suspected. What if you rename _start to _common_start. Make _start equal _common_start for 405 and rename _start_440 to _start, i.e make sure that _start is defined where you start executing after the jump.

As I mentioned earlier, _start_440 is mapped to 0xfffff000 via the linker script since the jump from 0xfffffffc can't be too long (because of the 4k shadow TLB entry). So renaming _start_440 to _start won't help here. It can be done, but frankly I don't have the time for it currently.

Quoted 12 lines

Quoted 3 lines

I'm an linker script dyslexic. So no idea if we can handle this solely in the linker script or if we need a new common symbol in the PPC start.S's.

Both ways should be doable I think. A linker script would probably look something like(pseudo diff below):
.text :
{

(quoted diff) +2 -0

+    _monitor_base = . ;
+    PROVIDE (_monitor_base = .);
    cpu/mpc83xx/start.o	(.text)

Quoted 3 lines

And then add a
GOT_ENTRY(_monitor_base)
in start.S

The disadvantage I see is that I need to change the linker scripts for all boards for such a solution. Doesn't sound like fun.

So for now, I would really like to see the old version with the ugly CFG_MONITOR_BASE back so that 4xx board can be used again.

Best regards,
Stefan

Both ways should be doable I think. A linker script would probably look something like(pseudo diff below):
.text :
{
+ _monitor_base = . ;

-----Original Message-----
From: wd at denx.de [mailto:wd at denx.de]
Sent: den 8 april 2008 22:42
To: joakim.tjernlund at transmode.se
Cc: Stefan Roese; u-boot-users at lists.sourceforge.net; 'Kim Phillips'
Subject: Re: [U-Boot-Users] [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 5 lines

Both ways should be doable I think. A linker script would probably look something like(pseudo diff below):
.text :
{
+ _monitor_base = . ;

There is no guarantee that this is the same as the start address of .text

Quoted 8 lines

Quoted 4 lines

.text :
{
+ _monitor_base = . ;

There is no guarantee that this is the same as the start address of .text

I am no link script expert, but I cannot see why that would not be the start of the .text segment?

Anyhow, we can go with the other solution instead, define the symbol in start.S instead, if one is needed.

-----Original Message-----
From: wd at denx.de [mailto:wd at denx.de]
Sent: den 8 april 2008 23:09
To: Joakim Tjernlund
Cc: 'Stefan Roese'; u-boot-users at lists.sourceforge.net; 'Kim Phillips'
Subject: Re: [U-Boot-Users] [PATCH] Make MPC83xx one step closer to full relocation.

Quoted 11 lines · In message <044901c899ba$83532450$89f96cf0$@[email protected]> you wrote:

Quoted 8 lines

Quoted 4 lines

.text :
{
+ _monitor_base = . ;

There is no guarantee that this is the same as the start address of .text

I am no link script expert, but I cannot see why that would not be the start of the .text segment?

There might be other segments at the start of the image...

See for example cpu/mpc8xx/start.S - while at the moment we really start with .text, it would technically be more approrpriate to change the first 256 bytes (from monitor base to EXC_OFF_SYS_RESET) into .rodata or something like this.

Quoted 3 lines

Anyhow, we can go with the other solution instead, define the symbol in start.S instead, if one is needed.

If it cannot be avoided ;-)

Quoted 4 lines

See for example cpu/mpc8xx/start.S - while at the moment we really start with .text, it would technically be more approrpriate to change the first 256 bytes (from monitor base to EXC_OFF_SYS_RESET) into .rodata or something like this.

Ahh, now I see what you mean and it gave me an idea ...

Quoted 4 lines

Quoted 2 lines

Anyhow, we can go with the other solution instead, define the symbol in start.S instead, if one is needed.

If it cannot be avoided ;-)

... Stefan, maybe you can #define EXC_OFF_SYS_RESET for 405 to something that fits with my relocation patch?

Since I'm really short in time currently, and I need U-Boot running on 4xx again, I vote for reverting this patch now. I'll try to come up with a "generic" solution for the next merge window.

Wolfgang? What do you think?

Quoted 6 lines · In message <[email protected]> you wrote:

Since I'm really short in time currently, and I need U-Boot running on 4xx again, I vote for reverting this patch now. I'll try to come up with a "generic" solution for the next merge window.

Wolfgang? What do you think?

I agree. This patch is too invasive at this point of the development cycle. Please revert it. We will handle this in the next merge window, then.

Quoted 5 lines · Wolfgang Denk

Quoted 4 lines · In message <[email protected]> you wrote:

Since I'm really short in time currently, and I need U-Boot running on 4xx again, I vote for reverting this patch now. I'll try to come up with a "generic" solution for the next merge window.

Quoted 5 lines

Quoted 1 line

Wolfgang? What do you think?

I agree. This patch is too invasive at this point of the development cycle. Please revert it. We will handle this in the next merge window, then.

I rather see a #ifdef for ppc4xx instead.

Quoted 4 lines

I agree. This patch is too invasive at this point of the development cycle. Please revert it. We will handle this in the next merge window, then.

I rather see a #ifdef for ppc4xx instead.

Oops, didn't see that coming. Your _start symbol in ppc4xx/start.S isn't pointing to your real start of execution. Seems like _start_440 is your real start but I can't be sure. There are some strange code in there that I don't understand.