Wolfgang Denk <w...@denx.de> wrote on 02/01/2010 19:13:29:
>
> Dear Joakim Tjernlund,
>
> In message <1262185712-11890-3-git-send-email-joakim.tjernl...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
