Wolfgang Denk wd@denx.de wrote on 02/01/2010 19:13:29:
Dear Joakim Tjernlund,
In message 1262185712-11890-3-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Accessing global data before relocation needs special handling if link address != load address. Use LINK_OFF to calculate the difference.
common/cmd_nvedit.c | 2 ++ common/console.c | 12 +++++++++--- common/env_common.c | 2 +- cpu/mpc83xx/cpu.c | 10 +++++----- cpu/mpc83xx/cpu_init.c | 38 ++++++++++++++++++++------------------ cpu/mpc83xx/speed.c | 28 +++++++++++----------------- drivers/serial/serial.c | 21 +++++++++++---------- include/linux/ctype.h | 6 +++--- lib_generic/crc32.c | 7 ++++++- lib_generic/ctype.c | 2 +- lib_generic/display_options.c | 5 +++-- lib_generic/vsprintf.c | 9 ++++++--- lib_ppc/board.c | 5 +++-- tools/updater/ctype.c | 2 +- 14 files changed, 82 insertions(+), 67 deletions(-)
...
-void puts(const char *s) +static void printf_puts(const char *s)
The name "printf_puts" makes my (remaining) hair stand on end. Either we have printf(), or we have puts(), but please let's never use "printf_puts" - I fear there might be a "printf_getchar" coming, too.
OK, I can fix the name easily.
+void puts(const char *s) +{
- printf_puts(LINK_OFF(s));
+}
Will such changes be needed to all functions that work on (constant?) strings? Or why here?
Only those that work on constant strings before relocation to RAM. One alternative is to change all call sites to puts(LINK_OFF(s)). I this particular case I need an intermediate function as puts() are used by printf() too and printf() don't want the LINK_OFF transformation.
--- a/cpu/mpc83xx/cpu.c +++ b/cpu/mpc83xx/cpu.c @@ -51,8 +51,8 @@ int checkcpu(void) char buf[32]; int i;
- const struct cpu_type {
char name[15];
- static const struct cpu_type {
char *name;I guess we have similar constructs in many other places as well. Do all of these need to be changed? Really?
I guess I could have changed the code below: - puts(cpu_type_list[i].name); + puts(cpu_ptr[i].name); to something else instead but the change I did do felt best as we hardly need name to be 15 chars wide and we don't need to initialise it at runtime. but yes, all such constructs in cpu_init like code needs to be looked upon iff one wants to use this feature.
- switch (corecnf_tab[corecnf_tab_index].core_csb_ratio) {
- case _byp:
- case _x1:
- case _1x:
- cnf_tab = LINK_OFF(corecnf_tab);
- csb_r = cnf_tab[corecnf_tab_index].core_csb_ratio;
- /* Cannot use a switch stmt here, it uses linked address */
Yet another of these really, really invasive changes. Is this really unavoidable?
Yes, global data and strings accessed before relocation to RAM needs to be wrapped with LINK_OFF to calculate the real address.
Why can we use "if" but not "switch/case" ?
For some reason, the code generated by gcc wasn't true PIC. The jump table that the switch stmt generated accessed the GOT. This may very well be a gcc bug.
--- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -85,9 +85,9 @@ static NS16550_t serial_ports[4] = { #endif };
-#define PORT serial_ports[port-1] +#define PORT (LINK_OFF(serial_ports)[port-1]) #if defined(CONFIG_CONS_INDEX) -#define CONSOLE (serial_ports[CONFIG_CONS_INDEX-1]) +#define CONSOLE (LINK_OFF(serial_ports)[CONFIG_CONS_INDEX-1]) #endif
I wonder how you selected the places where such changes were needed, and where not. There is certainly lots of similar looking code that you don't touch. Why not? Because it's not needed (then why do we need these changes here?), or because it just did not result in errors in your tests so far (then what's the estimate for the full amount of changes?) ???
I selected the changes I needed to for my 83xx board and then some. I don't think serial.c needs any more changes. Remember you only have to consider code that uses global data and is executed before relocation to RAM and that limits the scope quite a lot. The places to look at you find by following board.c's init_sequence. I think my changes are fairly complete for PowerPC,83xx. There are missing bits to do, mainly in other archs I think, but boards that doesn't need/want this functionality don't have to change anything.
Jocke