[U-Boot] [PATCH 2/4] Use LINK_OFF to access global data

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <1262185712-11890-3-git-send-email-Joakim.Tjernlund at 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.

> +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?

> --- 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?

> -	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?

Why can we use "if" but not "switch/case" ?

> --- 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?) ???

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"355/113 -- Not the famous irrational number PI,  but  an  incredible
simulation!"