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

[Joakim Tjernlund]

Wolfgang Denk <wd at denx.de> wrote on 02/01/2010 19:13:29:
>
> 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.

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