[U-Boot] [PATCH 0/4] Make u-boot true PIC for ppc
This series adds link_off(), a function to calculate the difference between load address and link address.
Using this function it is possible to make u-boot 100% PIC by wrapping global data accesses LINK_OFF() calls. Plenty of examples in the code to show how to use it.
All start.S needs to be updated too and I have done so with mpc83xx.
-DCONFIG_LINK_OFF in your platform config.mk to make use of this function.
Needs the -ffixed-r14 patches I just sent or you can fix the conflicts manually.
Would really appreciate some testing by 83xx owners. Scott?
Note: this is a resend.
Jocke
Joakim Tjernlund (4): ppc: Add const void *link_off(const void *addr) Use LINK_OFF to access global data Use LINK_OFF in enviroment too ppc: Make mpc83xx start.S relative.
common/cmd_nvedit.c | 2 + common/console.c | 12 ++++++-- common/env_common.c | 2 +- common/env_flash.c | 65 ++++++++++++++++++++++++---------------- cpu/mpc83xx/cpu.c | 10 +++--- cpu/mpc83xx/cpu_init.c | 38 ++++++++++++----------- cpu/mpc83xx/speed.c | 28 +++++++----------- cpu/mpc83xx/start.S | 35 +++++++++++++++++---- drivers/serial/serial.c | 21 +++++++------ include/common.h | 7 ++++ 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 ++- lib_ppc/reloc.S | 21 +++++++++++++ tools/updater/ctype.c | 2 +- 18 files changed, 177 insertions(+), 100 deletions(-)
Calculates the offset between global data link address and where the data is actually loaded. Add this offset to 'addr' arg and return the result. Useful for true PIC and when relocating code to RAM. --- include/common.h | 7 +++++++ lib_ppc/reloc.S | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/include/common.h b/include/common.h index 7df9afa..b37cb1d 100644 --- a/include/common.h +++ b/include/common.h @@ -108,6 +108,13 @@ typedef volatile unsigned char vu_char; #include <asm/blackfin.h> #endif
+#ifdef CONFIG_LINK_OFF +const void * link_off(const void * addr); +#else +#define link_off(addr) ((const void *) (addr)) +#endif +#define LINK_OFF(x) ((__typeof__(&(x)[0]))link_off(x)) + #include <part.h> #include <flash.h> #include <image.h> diff --git a/lib_ppc/reloc.S b/lib_ppc/reloc.S index 50f9a83..3ec26cc 100644 --- a/lib_ppc/reloc.S +++ b/lib_ppc/reloc.S @@ -47,3 +47,24 @@ trap_reloc: blr .size trap_reloc, .-trap_reloc #endif + +#ifdef CONFIG_LINK_OFF + /* Calculate the offset between global data link address + * and where the data is actually loaded. Add this offset + * to 'addr' arg and return the result. + * Useful for true PIC and when relocating code to RAM */ + .globl link_off + .type link_off, @function +link_off: /* const void * link_off(const void * addr) */ + /* In asm as we cannot use the stack */ + mflr r5 + bl _GLOBAL_OFFSET_TABLE_@local-4 + mflr r4 + mtlr r5 + addi r4,r4,12 /* find first .got2 entry */ + lwz r5,0(r4) /* get link address */ + subf r4,r5,r4 /* r4 = r4 - r5 */ + add r3,r3,r4 /* r3 = r4 + r3 */ + blr + .size link_off, .-link_off +#endif
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(-)
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 9f8d531..182c6fe 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -512,6 +512,7 @@ char *getenv (char *name) { int i, nxt;
+ name = LINK_OFF(name); WATCHDOG_RESET();
for (i=0; env_get_char(i) != '\0'; i=nxt+1) { @@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len) { int i, nxt;
+ name = LINK_OFF(name); for (i=0; env_get_char(i) != '\0'; i=nxt+1) { int val, n;
diff --git a/common/console.c b/common/console.c index dc0d13b..afda83a 100644 --- a/common/console.c +++ b/common/console.c @@ -346,7 +346,7 @@ void putc(const char c) } }
-void puts(const char *s) +static void printf_puts(const char *s) { #ifdef CONFIG_SILENT_CONSOLE if (gd->flags & GD_FLG_SILENT) @@ -367,12 +367,18 @@ void puts(const char *s) } }
+void puts(const char *s) +{ + printf_puts(LINK_OFF(s)); +} + void printf(const char *fmt, ...) { va_list args; uint i; char printbuffer[CONFIG_SYS_PBSIZE];
+ fmt = LINK_OFF(fmt); va_start(args, fmt);
/* For this to work, printbuffer must be larger than @@ -382,7 +388,7 @@ void printf(const char *fmt, ...) va_end(args);
/* Print the string */ - puts(printbuffer); + printf_puts(printbuffer); }
void vprintf(const char *fmt, va_list args) @@ -396,7 +402,7 @@ void vprintf(const char *fmt, va_list args) i = vsprintf(printbuffer, fmt, args);
/* Print the string */ - puts(printbuffer); + printf_puts(printbuffer); }
/* test if ctrl-c was pressed */ diff --git a/common/env_common.c b/common/env_common.c index 439a4a9..107e711 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -153,7 +153,7 @@ static uchar env_get_char_init (int index) { c = env_get_char_spec(index); } else { - c = default_environment[index]; + c = LINK_OFF(default_environment)[index]; }
return (c); diff --git a/cpu/mpc83xx/cpu.c b/cpu/mpc83xx/cpu.c index e38a372..12a2a84 100644 --- 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; u32 partid; } cpu_type_list [] = { CPU_TYPE_ENTRY(8311), @@ -72,6 +72,7 @@ int checkcpu(void) CPU_TYPE_ENTRY(8378), CPU_TYPE_ENTRY(8379), }; + const struct cpu_type *cpu_ptr = LINK_OFF(cpu_type_list);
immr = (immap_t *)CONFIG_SYS_IMMR;
@@ -99,11 +100,10 @@ int checkcpu(void) }
spridr = immr->sysconf.spridr; - for (i = 0; i < ARRAY_SIZE(cpu_type_list); i++) - if (cpu_type_list[i].partid == PARTID_NO_E(spridr)) { + if (cpu_ptr[i].partid == PARTID_NO_E(spridr)) { puts("MPC"); - puts(cpu_type_list[i].name); + puts(cpu_ptr[i].name); if (IS_E_PROCESSOR(spridr)) puts("E"); if (REVID_MAJOR(spridr) >= 2) diff --git a/cpu/mpc83xx/cpu_init.c b/cpu/mpc83xx/cpu_init.c index 031e8d5..e1f9018 100644 --- a/cpu/mpc83xx/cpu_init.c +++ b/cpu/mpc83xx/cpu_init.c @@ -42,13 +42,14 @@ static void config_qe_ioports(void) u8 port, pin; int dir, open_drain, assign; int i; - - for (i = 0; qe_iop_conf_tab[i].assign != QE_IOP_TAB_END; i++) { - port = qe_iop_conf_tab[i].port; - pin = qe_iop_conf_tab[i].pin; - dir = qe_iop_conf_tab[i].dir; - open_drain = qe_iop_conf_tab[i].open_drain; - assign = qe_iop_conf_tab[i].assign; + qe_iop_conf_t *qe_ptr = LINK_OFF(qe_iop_conf_tab); + + for (i = 0; qe_ptr[i].assign != QE_IOP_TAB_END; i++) { + port = qe_ptr[i].port; + pin = qe_ptr[i].pin; + dir = qe_ptr[i].dir; + open_drain = qe_ptr[i].open_drain; + assign = qe_ptr[i].assign; qe_config_iopin(port, pin, dir, open_drain, assign); } } @@ -378,7 +379,7 @@ int cpu_init_r (void) #if defined(CONFIG_DISPLAY_AER_FULL) static int print_83xx_arb_event(int force) { - static char* event[] = { + static const char* event[] = { "Address Time Out", "Data Time Out", "Address Only Transfer Type", @@ -388,7 +389,7 @@ static int print_83xx_arb_event(int force) "reserved", "reserved" }; - static char* master[] = { + static const char* master[] = { "e300 Core Data Transaction", "reserved", "e300 Core Instruction Fetch", @@ -422,7 +423,7 @@ static int print_83xx_arb_event(int force) "PCI Express 2", "TDM-DMAC" }; - static char *transfer[] = { + static const char *transfer[] = { "Address-only, Clean Block", "Address-only, lwarx reservation set", "Single-beat or Burst write", @@ -473,11 +474,11 @@ static int print_83xx_arb_event(int force)
puts("Arbiter Event Status:\n"); printf(" Event Address: 0x%08lX\n", gd->arbiter_event_address); - printf(" Event Type: 0x%1x = %s\n", etype, event[etype]); - printf(" Master ID: 0x%02x = %s\n", mstr_id, master[mstr_id]); + printf(" Event Type: 0x%1x = %s\n", etype, LINK_OFF(event)[etype]); + printf(" Master ID: 0x%02x = %s\n", mstr_id, LINK_OFF(master)[mstr_id]); printf(" Transfer Size: 0x%1x = %d bytes\n", (tbst<<3) | tsize, tbst ? (tsize ? tsize : 8) : 16 + 8 * tsize); - printf(" Transfer Type: 0x%02x = %s\n", ttype, transfer[ttype]); + printf(" Transfer Type: 0x%02x = %s\n", ttype, LINK_OFF(transfer)[ttype]);
return gd->arbiter_event_address; } @@ -501,7 +502,7 @@ static int print_83xx_arb_event(int force) */ int prt_83xx_rsr(void) { - static struct { + static const struct reset_type { ulong mask; char *desc; } bits[] = { @@ -515,7 +516,7 @@ int prt_83xx_rsr(void) RSR_SRS, "External/Internal Soft"}, { RSR_HRS, "External/Internal Hard"} }; - static int n = sizeof bits / sizeof bits[0]; + const struct reset_type *bp = LINK_OFF(bits); ulong rsr = gd->reset_status; int i; char *sep; @@ -523,9 +524,10 @@ int prt_83xx_rsr(void) puts("Reset Status:");
sep = " "; - for (i = 0; i < n; i++) - if (rsr & bits[i].mask) { - printf("%s%s", sep, bits[i].desc); + for (i = 0; i < ARRAY_SIZE(bits); i++) + if (rsr & bp[i].mask) { + puts(sep); + puts(bp[i].desc); sep = ", "; } puts("\n"); diff --git a/cpu/mpc83xx/speed.c b/cpu/mpc83xx/speed.c index bde7e92..001387a 100644 --- a/cpu/mpc83xx/speed.c +++ b/cpu/mpc83xx/speed.c @@ -98,7 +98,8 @@ int get_clocks(void) u32 corecnf_tab_index; u8 corepll; u32 lcrr; - + corecnf_t *cnf_tab; + int csb_r; u32 csb_clk; #if defined(CONFIG_MPC834x) || defined(CONFIG_MPC831x) || defined(CONFIG_MPC837x) u32 tsec1_clk; @@ -413,28 +414,21 @@ int get_clocks(void) /* corecnf_tab_index is too high, possibly worng value */ return -11; } - 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 */ + if (csb_r == _byp || csb_r == _x1 || csb_r == _1x) core_clk = csb_clk; - break; - case _1_5x: + else if (csb_r == _1_5x) core_clk = (3 * csb_clk) / 2; - break; - case _2x: + else if (csb_r == _2x) core_clk = 2 * csb_clk; - break; - case _2_5x: + else if (csb_r == _2_5x) core_clk = (5 * csb_clk) / 2; - break; - case _3x: + else if (csb_r == _3x) core_clk = 3 * csb_clk; - break; - default: - /* unkown core to csb ratio */ + else /* unkown core to csb ratio */ return -13; - }
#if defined(CONFIG_MPC8360) || defined(CONFIG_MPC832x) qepmf = (im->reset.rcwl & HRCWL_CEPMF) >> HRCWL_CEPMF_SHIFT; diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c index dd5f332..4ccfb56 100644 --- 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
#if defined(CONFIG_SERIAL_MULTI) @@ -159,26 +159,27 @@ static int calc_divisor (NS16550_t port) int serial_init (void) { int clock_divisor; + NS16550_t * sp = LINK_OFF(serial_ports);
#ifdef CONFIG_NS87308 initialise_ns87308(); #endif
#ifdef CONFIG_SYS_NS16550_COM1 - clock_divisor = calc_divisor(serial_ports[0]); - NS16550_init(serial_ports[0], clock_divisor); + clock_divisor = calc_divisor(sp[0]); + NS16550_init(sp[0], clock_divisor); #endif #ifdef CONFIG_SYS_NS16550_COM2 - clock_divisor = calc_divisor(serial_ports[1]); - NS16550_init(serial_ports[1], clock_divisor); + clock_divisor = calc_divisor(sp[1]); + NS16550_init(sp[1], clock_divisor); #endif #ifdef CONFIG_SYS_NS16550_COM3 - clock_divisor = calc_divisor(serial_ports[2]); - NS16550_init(serial_ports[2], clock_divisor); + clock_divisor = calc_divisor(sp[2]); + NS16550_init(sp[2], clock_divisor); #endif #ifdef CONFIG_SYS_NS16550_COM4 - clock_divisor = calc_divisor(serial_ports[3]); - NS16550_init(serial_ports[3], clock_divisor); + clock_divisor = calc_divisor(sp[3]); + NS16550_init(sp[3], clock_divisor); #endif
return (0); diff --git a/include/linux/ctype.h b/include/linux/ctype.h index afa3639..5873c55 100644 --- a/include/linux/ctype.h +++ b/include/linux/ctype.h @@ -1,6 +1,6 @@ #ifndef _LINUX_CTYPE_H #define _LINUX_CTYPE_H - +#include <common.h> /* * NOTE! This ctype does not handle EOF like the standard C * library is required to. @@ -15,9 +15,9 @@ #define _X 0x40 /* hex digit */ #define _SP 0x80 /* hard space (0x20) */
-extern unsigned char _ctype[]; +extern const unsigned char _ctype[];
-#define __ismask(x) (_ctype[(int)(unsigned char)(x)]) +#define __ismask(x) (LINK_OFF(_ctype)[(int)(unsigned char)(x)])
#define isalnum(c) ((__ismask(c)&(_U|_L|_D)) != 0) #define isalpha(c) ((__ismask(c)&(_U|_L)) != 0) diff --git a/lib_generic/crc32.c b/lib_generic/crc32.c index b27048c..2e11548 100644 --- a/lib_generic/crc32.c +++ b/lib_generic/crc32.c @@ -148,7 +148,7 @@ const uint32_t * ZEXPORT get_crc_table() #endif
/* ========================================================================= */ -#define DO1(buf) crc = crc_table[((int)crc ^ (*buf++)) & 0xff] ^ (crc >> 8); +#define DO1(buf) crc = crc_tab[((int)crc ^ (*buf++)) & 0xff] ^ (crc >> 8); #define DO2(buf) DO1(buf); DO1(buf); #define DO4(buf) DO2(buf); DO2(buf); #define DO8(buf) DO4(buf); DO4(buf); @@ -156,6 +156,11 @@ const uint32_t * ZEXPORT get_crc_table() /* ========================================================================= */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) { +#ifdef LINK_OFF + const uint32_t *crc_tab = LINK_OFF(crc_table); +#else + const uint32_t *crc_tab = crc_table; +#endif #ifdef DYNAMIC_CRC_TABLE if (crc_table_empty) make_crc_table(); diff --git a/lib_generic/ctype.c b/lib_generic/ctype.c index 6ed0468..dffe563 100644 --- a/lib_generic/ctype.c +++ b/lib_generic/ctype.c @@ -29,7 +29,7 @@
#include <linux/ctype.h>
-unsigned char _ctype[] = { +const unsigned char _ctype[] = { _C,_C,_C,_C,_C,_C,_C,_C, /* 0-7 */ _C,_C|_S,_C|_S,_C|_S,_C|_S,_C|_S,_C,_C, /* 8-15 */ _C,_C,_C,_C,_C,_C,_C,_C, /* 16-23 */ diff --git a/lib_generic/display_options.c b/lib_generic/display_options.c index 2dc2567..6b9fba2 100644 --- a/lib_generic/display_options.c +++ b/lib_generic/display_options.c @@ -31,9 +31,9 @@ int display_options (void) extern char version_string[];
#if defined(BUILD_TAG) - printf ("\n\n%s, Build: %s\n\n", version_string, BUILD_TAG); + printf ("\n\n%s, Build: %s\n\n", LINK_OFF(version_string), LINK_OFF(BUILD_TAG)); #else - printf ("\n\n%s\n\n", version_string); + printf ("\n\n%s\n\n", LINK_OFF(version_string)); #endif return 0; } @@ -49,6 +49,7 @@ void print_size (phys_size_t size, const char *s) phys_size_t d = 1 << 30; /* 1 GB */ char c = 'G';
+ s = LINK_OFF(s); if (size < d) { /* try MB */ c = 'M'; d = 1 << 20; diff --git a/lib_generic/vsprintf.c b/lib_generic/vsprintf.c index 3d95728..b779f56 100644 --- a/lib_generic/vsprintf.c +++ b/lib_generic/vsprintf.c @@ -37,8 +37,8 @@ extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
const char hex_asc[] = "0123456789abcdef"; -#define hex_asc_lo(x) hex_asc[((x) & 0x0f)] -#define hex_asc_hi(x) hex_asc[((x) & 0xf0) >> 4] +#define hex_asc_lo(x) LINK_OFF(hex_asc)[((x) & 0x0f)] +#define hex_asc_hi(x) LINK_OFF(hex_asc)[((x) & 0xf0) >> 4]
static inline char *pack_hex_byte(char *buf, u8 byte) { @@ -303,7 +303,7 @@ static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int pr int shift = 3; if (base == 16) shift = 4; do { - tmp[i++] = (digits[((unsigned char)num) & mask] | locase); + tmp[i++] = (LINK_OFF(digits)[((unsigned char)num) & mask] | locase); num >>= shift; } while (num); } else { /* base 10 */ @@ -675,6 +675,7 @@ int sprintf(char * buf, const char *fmt, ...) va_list args; int i;
+ fmt = LINK_OFF(fmt); va_start(args, fmt); i=vsprintf(buf,fmt,args); va_end(args); @@ -684,6 +685,8 @@ int sprintf(char * buf, const char *fmt, ...) void panic(const char *fmt, ...) { va_list args; + + fmt = LINK_OFF(fmt); va_start(args, fmt); vprintf(fmt, args); putc('\n'); diff --git a/lib_ppc/board.c b/lib_ppc/board.c index 765f97a..f9a7d30 100644 --- a/lib_ppc/board.c +++ b/lib_ppc/board.c @@ -384,8 +384,9 @@ void board_init_f (ulong bootflag) memset ((void *) gd, 0, sizeof (gd_t)); #endif
- for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) { - if ((*init_fnc_ptr) () != 0) { + for (init_fnc_ptr = LINK_OFF(init_sequence); *init_fnc_ptr; + ++init_fnc_ptr) { + if (((init_fnc_t *)link_off(*init_fnc_ptr)) () != 0) { hang (); } } diff --git a/tools/updater/ctype.c b/tools/updater/ctype.c index 6ed0468..dffe563 100644 --- a/tools/updater/ctype.c +++ b/tools/updater/ctype.c @@ -29,7 +29,7 @@
#include <linux/ctype.h>
-unsigned char _ctype[] = { +const unsigned char _ctype[] = { _C,_C,_C,_C,_C,_C,_C,_C, /* 0-7 */ _C,_C|_S,_C|_S,_C|_S,_C|_S,_C|_S,_C,_C, /* 8-15 */ _C,_C,_C,_C,_C,_C,_C,_C, /* 16-23 */
This is the most complex change. Keep this one as a separate commit for now. --- common/env_flash.c | 65 +++++++++++++++++++++++++++++++-------------------- 1 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/common/env_flash.c b/common/env_flash.c index b860c48..64882d2 100644 --- a/common/env_flash.c +++ b/common/env_flash.c @@ -52,27 +52,28 @@ DECLARE_GLOBAL_DATA_PTR;
char * env_name_spec = "Flash";
+static int flash_env_swapped; + #ifdef ENV_IS_EMBEDDED
extern uchar environment[]; env_t *env_ptr = (env_t *)(&environment[0]);
#ifdef CMD_SAVEENV -/* static env_t *flash_addr = (env_t *)(&environment[0]);-broken on ARM-wd-*/ -static env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR; +#define flash_addr f_flash_addr() #endif
#else /* ! ENV_IS_EMBEDDED */
env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; #ifdef CMD_SAVEENV -static env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR; +#define flash_addr f_flash_addr() #endif
#endif /* ENV_IS_EMBEDDED */
#ifdef CONFIG_ENV_ADDR_REDUND -static env_t *flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND; +#define flash_addr_new f_flash_addr_new()
/* CONFIG_ENV_ADDR is supposed to be on sector boundary */ static ulong end_addr = CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE - 1; @@ -80,10 +81,35 @@ static ulong end_addr_new = CONFIG_ENV_ADDR_REDUND + CONFIG_ENV_SECT_SIZE - 1;
#define ACTIVE_FLAG 1 #define OBSOLETE_FLAG 0 + +static void flash_env_swap(void) +{ + ulong ltmp = end_addr; + + flash_env_swapped = !flash_env_swapped; + + end_addr = end_addr_new; + end_addr_new = ltmp; +} + +static env_t * f_flash_addr_new(void) +{ + if (!*(LINK_OFF(&flash_env_swapped))) + return (env_t *)CONFIG_ENV_ADDR_REDUND; + else + return (env_t *)CONFIG_ENV_ADDR; +} #endif /* CONFIG_ENV_ADDR_REDUND */
extern uchar default_environment[];
+static env_t * f_flash_addr(void) +{ + if (!(*LINK_OFF(&flash_env_swapped))) + return (env_t *)CONFIG_ENV_ADDR; + else + return (env_t *)CONFIG_ENV_ADDR_REDUND; +}
uchar env_get_char_spec (int index) { @@ -99,7 +125,7 @@ int env_init(void) uchar flag1 = flash_addr->flags; uchar flag2 = flash_addr_new->flags;
- ulong addr_default = (ulong)&default_environment[0]; + ulong addr_default = (ulong)LINK_OFF(default_environment); ulong addr1 = (ulong)&(flash_addr->data); ulong addr2 = (ulong)&(flash_addr_new->data);
@@ -218,14 +244,7 @@ int saveenv(void) } #endif { - env_t * etmp = flash_addr; - ulong ltmp = end_addr; - - flash_addr = flash_addr_new; - flash_addr_new = etmp; - - end_addr = end_addr_new; - end_addr_new = ltmp; + flash_env_swap(); }
rc = 0; @@ -245,13 +264,15 @@ Done:
int env_init(void) { - if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { - gd->env_addr = (ulong)&(env_ptr->data); + env_t *ep = *LINK_OFF(&env_ptr); + + if (crc32(0, ep->data, ENV_SIZE) == ep->crc) { + gd->env_addr = (ulong)&(ep->data); gd->env_valid = 1; return(0); }
- gd->env_addr = (ulong)&default_environment[0]; + gd->env_addr = (ulong)LINK_OFF(default_environment); gd->env_valid = 0; return (0); } @@ -334,16 +355,8 @@ void env_relocate_spec (void) { #if !defined(ENV_IS_EMBEDDED) || defined(CONFIG_ENV_ADDR_REDUND) #ifdef CONFIG_ENV_ADDR_REDUND - if (gd->env_addr != (ulong)&(flash_addr->data)) { - env_t * etmp = flash_addr; - ulong ltmp = end_addr; - - flash_addr = flash_addr_new; - flash_addr_new = etmp; - - end_addr = end_addr_new; - end_addr_new = ltmp; - } + if (gd->env_addr != (ulong)&(flash_addr->data)) + flash_env_swap();
if (flash_addr_new->flags != OBSOLETE_FLAG && crc32(0, flash_addr_new->data, ENV_SIZE) ==
To use a different link address than load address, start.S must not make any absolute accesses. This makes it so. Use link_off(), if defined, to calculate the difference in load and link address. --- cpu/mpc83xx/start.S | 35 ++++++++++++++++++++++++++++------- 1 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/cpu/mpc83xx/start.S b/cpu/mpc83xx/start.S index 68bb620..bc17a60 100644 --- a/cpu/mpc83xx/start.S +++ b/cpu/mpc83xx/start.S @@ -240,9 +240,23 @@ boot_warm: /* time t 5 */ /* there and deflate the flash size back to minimal size */ /*------------------------------------------------------------*/ bl map_flash_by_law1 - lis r4, (CONFIG_SYS_MONITOR_BASE)@h - ori r4, r4, (CONFIG_SYS_MONITOR_BASE)@l - addi r5, r4, in_flash - _start + EXC_OFF_SYS_RESET + + /* Calculate address in flash and jump there */ + bl 1f +1: mflr r5 /* get current address */ + addi r5, r5, in_flash - 1b + /* Check if already inside flash address space. */ + /* if so, do not add CONFIG_SYS_FLASH_BASE */ + lis r4, (CONFIG_SYS_FLASH_BASE)@h + ori r4, r4, (CONFIG_SYS_FLASH_BASE)@l + cmplw cr0, r5, r4 + ble cr0, 2f /* r5 < r4 ? */ + lis r6, (CONFIG_SYS_FLASH_BASE+CONFIG_SYS_FLASH_SIZE)@h + ori r6, r6, (CONFIG_SYS_FLASH_BASE+CONFIG_SYS_FLASH_SIZE)@l + cmplw cr0, r5, r6 + bgt cr0, 3f /* r5 > r6 ? */ +2: add r5,r5,r4 +3: mtlr r5 blr in_flash: @@ -831,11 +845,18 @@ relocate_code: mr r10, r5 /* Save copy of Destination Address */
GET_GOT - mr r3, r5 /* Destination Address */ - lis r4, CONFIG_SYS_MONITOR_BASE@h /* Source Address */ - ori r4, r4, CONFIG_SYS_MONITOR_BASE@l + li r3, 0 +#ifdef CONFIG_LINK_OFF + bl link_off /* const void * link_off(const void *) */ +#endif + lwz r4, GOT(_start) /* Source Address */ + add r4, r4, r3 + addi r4, r4, -EXC_OFF_SYS_RESET lwz r5, GOT(__bss_start) - sub r5, r5, r4 + add r5, r5, r3 + mr r3, r10 /* Destination Address */ + + sub r5, r5, r4 /* r4 - r5 */ li r6, CONFIG_SYS_CACHELINE_SIZE /* Cache Line Size */
/*
On Wednesday 30 December 2009 10:08:30 Joakim Tjernlund wrote:
--- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -512,6 +512,7 @@ char *getenv (char *name) { int i, nxt;
name = LINK_OFF(name); WATCHDOG_RESET();
for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
@@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len) { int i, nxt;
- name = LINK_OFF(name); for (i=0; env_get_char(i) != '\0'; i=nxt+1) { int val, n;
you have no guarantee that getenv() is called with a const string which is in the .rodata section. there's code that generates the env name in a buffer on the stack and gives that to getenv(). does LINK_OFF() still work then ?
--- a/common/console.c +++ b/common/console.c @@ -346,7 +346,7 @@ void putc(const char c) } }
-void puts(const char *s) +static void printf_puts(const char *s) { #ifdef CONFIG_SILENT_CONSOLE if (gd->flags & GD_FLG_SILENT) @@ -367,12 +367,18 @@ void puts(const char *s) } }
+void puts(const char *s) +{
- printf_puts(LINK_OFF(s));
+}
and if CONFIG_LINK_OFF isnt defined, does gcc correctly inline this ? if not, i think there needs to be #ifdef CONFIG_LINK_OFF handling here.
--- a/include/linux/ctype.h +++ b/include/linux/ctype.h
-extern unsigned char _ctype[]; +extern const unsigned char _ctype[];
--- a/lib_generic/ctype.c +++ b/lib_generic/ctype.c
-unsigned char _ctype[] = { +const unsigned char _ctype[] = {
--- a/tools/updater/ctype.c +++ b/tools/updater/ctype.c
-unsigned char _ctype[] = { +const unsigned char _ctype[] = {
seems like these const changes should be split and pushed separately
--- a/lib_generic/crc32.c +++ b/lib_generic/crc32.c @@ -156,6 +156,11 @@ */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) { +#ifdef LINK_OFF
- const uint32_t *crc_tab = LINK_OFF(crc_table);
+#else
- const uint32_t *crc_tab = crc_table;
+#endif
the patch 1/4 you posted always defines LINK_OFF. it's CONFIG_LINK_OFF which is dynamic. -mike
Mike Frysinger vapier@gentoo.org wrote on 31/12/2009 19:44:40:
Happy new year :)
On Wednesday 30 December 2009 10:08:30 Joakim Tjernlund wrote:
--- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -512,6 +512,7 @@ char *getenv (char *name) { int i, nxt;
name = LINK_OFF(name); WATCHDOG_RESET();
for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
@@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len) { int i, nxt;
- name = LINK_OFF(name); for (i=0; env_get_char(i) != '\0'; i=nxt+1) { int val, n;
you have no guarantee that getenv() is called with a const string which is in the .rodata section. there's code that generates the env name in a buffer on the stack and gives that to getenv(). does LINK_OFF() still work then ?
True. LINK_OFF will not work iff link addr != load addr and name isn't a const string. Basically if you want to use the LINK_OFF feature you have to use a const string.
--- a/common/console.c +++ b/common/console.c @@ -346,7 +346,7 @@ void putc(const char c) } }
-void puts(const char *s) +static void printf_puts(const char *s) { #ifdef CONFIG_SILENT_CONSOLE if (gd->flags & GD_FLG_SILENT) @@ -367,12 +367,18 @@ void puts(const char *s) } }
+void puts(const char *s) +{
- printf_puts(LINK_OFF(s));
+}
and if CONFIG_LINK_OFF isnt defined, does gcc correctly inline this ? if not, i think there needs to be #ifdef CONFIG_LINK_OFF handling here.
Possibly, however LINK_OFF is is a NOP if CONFIG_LINK_OFF isn't defined.
--- a/include/linux/ctype.h +++ b/include/linux/ctype.h
-extern unsigned char _ctype[]; +extern const unsigned char _ctype[];
--- a/lib_generic/ctype.c +++ b/lib_generic/ctype.c
-unsigned char _ctype[] = { +const unsigned char _ctype[] = {
--- a/tools/updater/ctype.c +++ b/tools/updater/ctype.c
-unsigned char _ctype[] = { +const unsigned char _ctype[] = {
seems like these const changes should be split and pushed separately
Yeah, can't remember if this was required or if this was just something I noticed while adding LINK_OFF
--- a/lib_generic/crc32.c +++ b/lib_generic/crc32.c @@ -156,6 +156,11 @@ */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) { +#ifdef LINK_OFF
- const uint32_t *crc_tab = LINK_OFF(crc_table);
+#else
- const uint32_t *crc_tab = crc_table;
+#endif
the patch 1/4 you posted always defines LINK_OFF. it's CONFIG_LINK_OFF which is dynamic.
Yes, but LINK_OFF will work too. I can change this though, it looks better.
The bigger question is if the LINN_OFF changes in general are acceptable to u-boot. Any board/arch that doesn't want this functionality should not notice I think.
Jocke
On Thursday 31 December 2009 20:39:10 Joakim Tjernlund wrote:
Mike Frysinger vapier@gentoo.org wrote on 31/12/2009 19:44:40:
On Wednesday 30 December 2009 10:08:30 Joakim Tjernlund wrote:
--- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -512,6 +512,7 @@ char *getenv (char *name) { int i, nxt;
name = LINK_OFF(name); WATCHDOG_RESET();
for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
@@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len) { int i, nxt;
- name = LINK_OFF(name); for (i=0; env_get_char(i) != '\0'; i=nxt+1) { int val, n;
you have no guarantee that getenv() is called with a const string which is in the .rodata section. there's code that generates the env name in a buffer on the stack and gives that to getenv(). does LINK_OFF() still work then ?
True. LINK_OFF will not work iff link addr != load addr and name isn't a const string. Basically if you want to use the LINK_OFF feature you have to use a const string.
some of the other functions in these patch sets fall into the same issue ... like the output functions
--- a/common/console.c +++ b/common/console.c @@ -346,7 +346,7 @@ void putc(const char c) } }
-void puts(const char *s) +static void printf_puts(const char *s) { #ifdef CONFIG_SILENT_CONSOLE if (gd->flags & GD_FLG_SILENT) @@ -367,12 +367,18 @@ void puts(const char *s) } }
+void puts(const char *s) +{
- printf_puts(LINK_OFF(s));
+}
and if CONFIG_LINK_OFF isnt defined, does gcc correctly inline this ? if not, i think there needs to be #ifdef CONFIG_LINK_OFF handling here.
Possibly, however LINK_OFF is is a NOP if CONFIG_LINK_OFF isn't defined.
yes, but that doesnt mean gcc takes care of inlining all of printf_puts() into the puts() and all the new call sites go to puts()
--- a/lib_generic/crc32.c +++ b/lib_generic/crc32.c @@ -156,6 +156,11 @@ */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) { +#ifdef LINK_OFF
- const uint32_t *crc_tab = LINK_OFF(crc_table);
+#else
- const uint32_t *crc_tab = crc_table;
+#endif
the patch 1/4 you posted always defines LINK_OFF. it's CONFIG_LINK_OFF which is dynamic.
Yes, but LINK_OFF will work too. I can change this though, it looks better.
my point is that it should either be checking CONFIG_LINK_OFF or always using LINK_OFF (since it nops when the config is off as you point out)
The bigger question is if the LINN_OFF changes in general are acceptable to u-boot. Any board/arch that doesn't want this functionality should not notice I think.
i dont have any plans on wanting this, and it seems pretty invasive ... and easy to introduce new code that breaks PIC people but no one else really notices ... -mike
Mike Frysinger vapier@gentoo.org wrote on 01/01/2010 07:18:44:
From: Mike Frysinger vapier@gentoo.org To: Joakim Tjernlund joakim.tjernlund@transmode.se Cc: u-boot@lists.denx.de Date: 01/01/2010 07:19 Subject: Re: [U-Boot] [PATCH 2/4] Use LINK_OFF to access global data
On Thursday 31 December 2009 20:39:10 Joakim Tjernlund wrote:
Mike Frysinger vapier@gentoo.org wrote on 31/12/2009 19:44:40:
On Wednesday 30 December 2009 10:08:30 Joakim Tjernlund wrote:
--- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -512,6 +512,7 @@ char *getenv (char *name) { int i, nxt;
name = LINK_OFF(name); WATCHDOG_RESET();
for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
@@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len) { int i, nxt;
- name = LINK_OFF(name); for (i=0; env_get_char(i) != '\0'; i=nxt+1) { int val, n;
you have no guarantee that getenv() is called with a const string which is in the .rodata section. there's code that generates the env name in a buffer on the stack and gives that to getenv(). does LINK_OFF() still work then ?
True. LINK_OFF will not work iff link addr != load addr and name isn't a const string. Basically if you want to use the LINK_OFF feature you have to use a const string.
some of the other functions in these patch sets fall into the same issue ... like the output functions
Yes, I wish gcc would have an option to access string literals relative the text segment so one would not access these via the GOT.
--- a/common/console.c +++ b/common/console.c @@ -346,7 +346,7 @@ void putc(const char c) } }
-void puts(const char *s) +static void printf_puts(const char *s) { #ifdef CONFIG_SILENT_CONSOLE if (gd->flags & GD_FLG_SILENT) @@ -367,12 +367,18 @@ void puts(const char *s) } }
+void puts(const char *s) +{
- printf_puts(LINK_OFF(s));
+}
and if CONFIG_LINK_OFF isnt defined, does gcc correctly inline this ? if not, i think there needs to be #ifdef CONFIG_LINK_OFF handling here.
Possibly, however LINK_OFF is is a NOP if CONFIG_LINK_OFF isn't defined.
yes, but that doesnt mean gcc takes care of inlining all of printf_puts() into the puts() and all the new call sites go to puts()
Sure, gcc might not inline the current code in this case. I guess you don't want the extra size this would incur or is there some else you are concerned about?
--- a/lib_generic/crc32.c +++ b/lib_generic/crc32.c @@ -156,6 +156,11 @@ */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) { +#ifdef LINK_OFF
- const uint32_t *crc_tab = LINK_OFF(crc_table);
+#else
- const uint32_t *crc_tab = crc_table;
+#endif
the patch 1/4 you posted always defines LINK_OFF. it's CONFIG_LINK_OFF which is dynamic.
Yes, but LINK_OFF will work too. I can change this though, it looks better.
my point is that it should either be checking CONFIG_LINK_OFF or always using LINK_OFF (since it nops when the config is off as you point out)
Yes, I will change this in the next spin.
The bigger question is if the LINN_OFF changes in general are acceptable to u-boot. Any board/arch that doesn't want this functionality should not notice I think.
i dont have any plans on wanting this, and it seems pretty invasive ... and easy to introduce new code that breaks PIC people but no one else really notices ...
Yes, it is a bit invasive hence the question if this is acceptable to u-boot. I have been looking for other ways to do this but there isn't one sans modifying gcc. You are sort of an gcc guy, what do you think the odds are that gcc would add a few new options mainly useful for smaller embedded progs like u-boot (and possible the kernel too)?
If this is accepted a few boards could be changed to use this new function by default and then one would detect breakage earlier.
Jocke
On Friday 01 January 2010 11:29:41 Joakim Tjernlund wrote:
Mike Frysinger vapier@gentoo.org wrote on 01/01/2010 07:18:44:
yes, but that doesnt mean gcc takes care of inlining all of printf_puts() into the puts() and all the new call sites go to puts()
Sure, gcc might not inline the current code in this case. I guess you don't want the extra size this would incur or is there some else you are concerned about?
the extra size
i dont have any plans on wanting this, and it seems pretty invasive ... and easy to introduce new code that breaks PIC people but no one else really notices ...
Yes, it is a bit invasive hence the question if this is acceptable to u-boot. I have been looking for other ways to do this but there isn't one sans modifying gcc. You are sort of an gcc guy, what do you think the odds are that gcc would add a few new options mainly useful for smaller embedded progs like u-boot (and possible the kernel too)?
i'm not really a gcc guy ... i just sometimes fake it. i honestly dont have an idea here how they would respond. -mike
Dear Joakim Tjernlund,
in message OF7EEC72D7.43E7C3B7-ONC125769E.00078331-C125769E.0009144C@transmode.se you wrote:
Happy new year :)
Thanks, and same to you and everybody else.
The bigger question is if the LINN_OFF changes in general are acceptable to u-boot. Any board/arch that doesn't want this functionality should not notice I think.
What's the impact? So far it seems to me that there are pretty heavy changes to lots of global code anyway, so we can then enable it for all boards as well. Otherwise we just add a lot of #ifdef's and make the code even harder to read and to understand. Which does not mean that I understood what you are doing :-(
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 02/01/2010 19:17:52:
Dear Joakim Tjernlund,
in message <OF7EEC72D7.43E7C3B7-ONC125769E.00078331-C125769E. 0009144C@transmode.se> you wrote:
Happy new year :)
Thanks, and same to you and everybody else.
The bigger question is if the LINN_OFF changes in general are acceptable to u-boot. Any board/arch that doesn't want this functionality should not notice I think.
What's the impact? So far it seems to me that there are pretty heavy changes to lots of global code anyway, so we can then enable it for all boards as well. Otherwise we just add a lot of #ifdef's and make the code even harder to read and to understand. Which does not mean that I understood what you are doing :-(
We don't have to add #ifdefs. I #defined a version a LINK_OFF for non users too. That way we don't have to litter the code with #ifdefs and we get some type checking from gcc too even for boards that don't use it.
I don't think we can enable this for all boards at once, there will be too much that break. My goal was to get 83xx fully functional and then other boards could follow.
Jocke
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.
+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
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
Dear Joakim Tjernlund,
In message OF3EBC8B6E.2C26AE15-ONC12576A0.003379DE-C12576A0.0039F518@transmode.se you wrote:
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)).
Well, I understand this is not only puts(), but also all places where a constant strings are used. And this is a lot of places and a lot of functions.
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.
Ouch.
I selected the changes I needed to for my 83xx board and then some. I don't think serial.c needs any more changes.
Understood. But that means we don't see the real scope of this change yet, as adapting a new board to use this featue will become a trial-and-error procedure, adding incrementally more and more of these changes (unless someone performs a thorough review and catches most of these places in an initial commit).
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.
I have to admit that I dislike the impact of this change, and I'm not sure if we really want to take this route.
Comments from others are welcome and needed here!
I think as follows:
In the past, the majority of systems supported by U-Boot where booting from NOR flash or other memory devices. This made it easy to use common code (like library functions) both before and after relocation to the final location in RAM. For your current changes this means that we have a large number of places where we have to add this LINK_OFF stuff. This makes the code harder to read, much harder to understand (especially if it's not working during the initial bringup on new hardware), and harder to debug in general.
If I try to see trends in the development of U-Boot I notice a growing number of systems that boot from NAND flash, DataFlash or that come with on-chip ROM code to load images from SDCard and other storage media. Such systems cannot make real benefit from the original design of U-Boot, as here U-Boot is inherently a second-stage boot loader which gets loaded by some other means. Even for NAND booting systems where we have the NAND boot code included within the U-Boot source tree we often cannot share much of the code between the primary and the secondary loader stages as there are usually tight restrictions on the maximum size for the primary loader image. Here a sharper separation of "primary" and "secondary" boot code within U-Boot would be benefical.
I feel (but this is really just a feeling, and I definitely would like to hear what others think about this!) your PIC changes would be (or have been) useful for the former usage mode, but they come at a pretty heavy cost as they are really invasive to the code. For the second usage mode they are not usable, or at least not useful. This makes me wonder if we really should continue to work in this direction.
Comments welcome...
Best regards,
Wolfgang Denk
Wolfgang Denk a écrit :
I think as follows:
In the past, the majority of systems supported by U-Boot where booting from NOR flash or other memory devices. This made it easy to use common code (like library functions) both before and after relocation to the final location in RAM. For your current changes this means that we have a large number of places where we have to add this LINK_OFF stuff. This makes the code harder to read, much harder to understand (especially if it's not working during the initial bringup on new hardware), and harder to debug in general.
If I try to see trends in the development of U-Boot I notice a growing number of systems that boot from NAND flash, DataFlash or that come with on-chip ROM code to load images from SDCard and other storage media. Such systems cannot make real benefit from the original design of U-Boot, as here U-Boot is inherently a second-stage boot loader which gets loaded by some other means. Even for NAND booting systems where we have the NAND boot code included within the U-Boot source tree we often cannot share much of the code between the primary and the secondary loader stages as there are usually tight restrictions on the maximum size for the primary loader image. Here a sharper separation of "primary" and "secondary" boot code within U-Boot would be benefical.
I feel (but this is really just a feeling, and I definitely would like to hear what others think about this!) your PIC changes would be (or have been) useful for the former usage mode, but they come at a pretty heavy cost as they are really invasive to the code. For the second usage mode they are not usable, or at least not useful. This makes me wonder if we really should continue to work in this direction.
Comments welcome...
Best regards,
Wolfgang Denk
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Why not make the same two-stage separation systematic, even on NOR-based devices and others where U-boot is currently the one executed at power-up? Split the current U-boot into a small primary bootloader (U1?) and a fuller secondary bootloader (U2?). U1 would initialize RAM (and a console?) and U2 would initialize everything else. Each stage would only run from a fixed location and type of memory, removing the need for PIC.
Comment given off the top of my head, so feel free to open fire. :)
Amicalement,
Dear Albert ARIBAUD,
In message 4B40F8DB.1090509@free.fr you wrote:
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
This scenario is interesting for a lot of other use cases, for example to load the new version to some free location in RAM, verify that it works, then copy it (eventually by itself) to persistent storage; this is especially useful for systems when your JTAG debugger does not support programming images to NAND or DataFlash or SPI flash or whatever storage device is used to store the image.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Assume you have systems with different RAM size configurations. Being able to load the same image to any address is beneficial then, too. [And the NAND bootstrap code often does not allow for additional code as needed for example for relocation.]
Why not make the same two-stage separation systematic, even on NOR-based devices and others where U-boot is currently the one executed at power-up? Split the current U-boot into a small primary bootloader (U1?)
There are many reasons: ease of porting and debugging, minimization of boot time, etc.
and a fuller secondary bootloader (U2?). U1 would initialize RAM (and a console?) and U2 would initialize everything else. Each stage would only run from a fixed location and type of memory, removing the need for PIC.
If you use a console in U1, you will need to share a LOT of code with U2 - things like printf() and all it's dependencies, most of the str*() and mem*() functions, etc. And especially for such complicted and error prone actions like initializing the RAM you _do_ want to be able to use a console port to print error messages and debug information.
Thsi is _exactly_ where the current design is coming from.
Comment given off the top of my head, so feel free to open fire. :)
No, of course not. This is a complicated issue, where it's easy to overlook one thing or another, so every comment is appreciated.
Best regards,
Wolfgang Denk
Wolfgang Denk a écrit :
Dear Albert ARIBAUD,
In message 4B40F8DB.1090509@free.fr you wrote:
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
This scenario is interesting for a lot of other use cases, for example to load the new version to some free location in RAM, verify that it works, then copy it (eventually by itself) to persistent storage; this is especially useful for systems when your JTAG debugger does not support programming images to NAND or DataFlash or SPI flash or whatever storage device is used to store the image.
There is a way out of this one -- I used it myself -- where you build both versions of U-boot, the RAM-located one and the FLASH-located one. You load the RAM one, run it, and it loads and flashes the FLASH one.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Assume you have systems with different RAM size configurations. Being able to load the same image to any address is beneficial then, too. [And the NAND bootstrap code often does not allow for additional code as needed for example for relocation.]
The U1 bootloader might be given the ability to relocate the U2 code. that's probably far-fetched, but when linking U2, a map file could be generated and a script could produce a relocation table for U1 to use. The table could be put in NAND along with the U2 code, so U1 might not need to be regenerated for every new U2 build.
Why not make the same two-stage separation systematic, even on NOR-based devices and others where U-boot is currently the one executed at power-up? Split the current U-boot into a small primary bootloader (U1?)
There are many reasons: ease of porting and debugging, minimization of boot time, etc.
Granted you'd have some added effort there, but possibly not so much in the porting and boot time departments, as executing U1 then U2 would roughly be the same as running today's full U-Boot, and equally, porting effort would be split rather than duplicated.
and a fuller secondary bootloader (U2?). U1 would initialize RAM (and a console?) and U2 would initialize everything else. Each stage would only run from a fixed location and type of memory, removing the need for PIC.
If you use a console in U1, you will need to share a LOT of code with U2 - things like printf() and all it's dependencies, most of the str*() and mem*() functions, etc. And especially for such complicted and error prone actions like initializing the RAM you _do_ want to be able to use a console port to print error messages and debug information.
Nothing prevents linking in the same source code in U1 and U2, I think. Of course that would make U1 bigger, but you'd need the code in there so that's the price to pay -- and it would be a temporary use of RAM, as the RAM for U1 could be reused freed when U2 comes into play.
Thsi is _exactly_ where the current design is coming from.
But obviously your call for comments also calls for a revision of the current design, doesn't it?
Amicalement,
Dear Albert ARIBAUD,
In message 4B4100F3.8080507@free.fr you wrote:
This scenario is interesting for a lot of other use cases, for example to load the new version to some free location in RAM, verify that it works, then copy it (eventually by itself) to persistent storage; this is especially useful for systems when your JTAG debugger does not support programming images to NAND or DataFlash or SPI flash or whatever storage device is used to store the image.
There is a way out of this one -- I used it myself -- where you build both versions of U-boot, the RAM-located one and the FLASH-located one. You load the RAM one, run it, and it loads and flashes the FLASH one.
Of course this works, but this is then _another_ image, which was not tested yet. This is bad both from the administration point of view (need of two differently built images), and bad for the nerves (as you install an untested image).
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Assume you have systems with different RAM size configurations. Being able to load the same image to any address is beneficial then, too. [And the NAND bootstrap code often does not allow for additional code as needed for example for relocation.]
The U1 bootloader might be given the ability to relocate the U2 code. that's probably far-fetched, but when linking U2, a map file could be generated and a script could produce a relocation table for U1 to use. The table could be put in NAND along with the U2 code, so U1 might not need to be regenerated for every new U2 build.
Keep in mind that U1 often has to fit in as little memory as 4 KB or so...
If you use a console in U1, you will need to share a LOT of code with U2 - things like printf() and all it's dependencies, most of the str*() and mem*() functions, etc. And especially for such complicted and error prone actions like initializing the RAM you _do_ want to be able to use a console port to print error messages and debug information.
Nothing prevents linking in the same source code in U1 and U2, I think. Of course that would make U1 bigger, but you'd need the code in there so that's the price to pay -- and it would be a temporary use of RAM, as the RAM for U1 could be reused freed when U2 comes into play.
Creating twobig parts (U1 and U2) is bad for the overal flash memory footprint, and for the boot time. And usually iut doesn;t work at all on NAND etc.
But obviously your call for comments also calls for a revision of the current design, doesn't it?
Sure.
Best regards,
Wolfgang Denk
Wolfgang Denk a écrit :
Dear Albert ARIBAUD,
In message 4B4100F3.8080507@free.fr you wrote:
This scenario is interesting for a lot of other use cases, for example to load the new version to some free location in RAM, verify that it works, then copy it (eventually by itself) to persistent storage; this is especially useful for systems when your JTAG debugger does not support programming images to NAND or DataFlash or SPI flash or whatever storage device is used to store the image.
There is a way out of this one -- I used it myself -- where you build both versions of U-boot, the RAM-located one and the FLASH-located one. You load the RAM one, run it, and it loads and flashes the FLASH one.
Of course this works, but this is then _another_ image, which was not tested yet. This is bad both from the administration point of view (need of two differently built images), and bad for the nerves (as you install an untested image).
Well, testing the FLASH version of U-boot will require that you flash it, right? Based on the hypothesis that JTAG cannot flash, then you have no choice but to flash it some other way.
What I did with U-boot in this case (worse yet: in a case where I did *not* have JTAG equipment at all) was to start from a u-boot with a proven low level init code, build it for RAM by disabling the low level code, test its flashing capability on unused sectors of FLASH, and once validated, build it also for FLASH. That was risky, but if you cannot flash with anything but U-boot, there was little else I could do.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Assume you have systems with different RAM size configurations. Being able to load the same image to any address is beneficial then, too. [And the NAND bootstrap code often does not allow for additional code as needed for example for relocation.]
The U1 bootloader might be given the ability to relocate the U2 code. that's probably far-fetched, but when linking U2, a map file could be generated and a script could produce a relocation table for U1 to use. The table could be put in NAND along with the U2 code, so U1 might not need to be regenerated for every new U2 build.
Keep in mind that U1 often has to fit in as little memory as 4 KB or so...
If it is tightly limited then it cannot take the console code with itself, so it'll have to fetch it from NAND before being able to log anything. And for that, it needs RAM access. so basically it won't be able to log RAM activation when it happens (unless you think of loading extra code in IRAM, but if there is IRAM, it probably contains U1 at this stage). Better have a thoroughly JTAG-tested minimal U1 which does not log (or if logging is required, U1 logs to a buffer, and U2 will print the log once started, a bit like Linux early logging).
If you use a console in U1, you will need to share a LOT of code with U2 - things like printf() and all it's dependencies, most of the str*() and mem*() functions, etc. And especially for such complicted and error prone actions like initializing the RAM you _do_ want to be able to use a console port to print error messages and debug information.
Nothing prevents linking in the same source code in U1 and U2, I think. Of course that would make U1 bigger, but you'd need the code in there so that's the price to pay -- and it would be a temporary use of RAM, as the RAM for U1 could be reused freed when U2 comes into play.
Creating twobig parts (U1 and U2) is bad for the overal flash memory footprint, and for the boot time. And usually iut doesn;t work at all on NAND etc.
Then there could be a three-part split: U1, U2 and Ulib. Link them all as relocatable code, and let U1's core load and relocate Ulib, and U2's core relocate its own references to U1's loaded Ulib location. Easily said, I know, but we're still in brainstorming stage here so let's give it a look. Agreed, this three-way split is going to cost some extra execution time but it could keep overall footprint minimal.
Amicalement,
On Mon, Jan 4, 2010 at 7:41 AM, Albert ARIBAUD albert.aribaud@free.fr wrote:
Wolfgang Denk a écrit :
Dear Albert ARIBAUD,
In message 4B40F8DB.1090509@free.fr you wrote:
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
This scenario is interesting for a lot of other use cases, for example to load the new version to some free location in RAM, verify that it works, then copy it (eventually by itself) to persistent storage; this is especially useful for systems when your JTAG debugger does not support programming images to NAND or DataFlash or SPI flash or whatever storage device is used to store the image.
This is a very interesting use-case - I hadn't though about using U-Boot version A running at location x to bootstrap into U-Boot version B running at location y before U-Boot version B copies itself to Flash
There is a way out of this one -- I used it myself -- where you build both versions of U-boot, the RAM-located one and the FLASH-located one. You load the RAM one, run it, and it loads and flashes the FLASH one.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Assume you have systems with different RAM size configurations. Being able to load the same image to any address is beneficial then, too. [And the NAND bootstrap code often does not allow for additional code as needed for example for relocation.]
The U1 bootloader might be given the ability to relocate the U2 code. that's probably far-fetched, but when linking U2, a map file could be generated and a script could produce a relocation table for U1 to use. The table could be put in NAND along with the U2 code, so U1 might not need to be regenerated for every new U2 build.
Have a look at my x86 relocation patch series - There is no need to do this as there is enough information left in the binary if you use the right gcc/ld options. I have a feeling that what I have done for x86 is portable to other arches.
While the above mentioned patch series allows relocation to an arbitrary RAM location, it does not allow loading into an abitrarty ROM location. Considering that there is a very limited range of functions that need to run from an arbitrary ROM location (primarily RAM init and basic console) I think we could get away with using this LINK_OFF scheme for a minimal set of pre-RAM functions.
Why not make the same two-stage separation systematic, even on NOR-based devices and others where U-boot is currently the one executed at power-up? Split the current U-boot into a small primary bootloader (U1?)
I support the greater seperation of U-Boot into a two stage boot loader. I find it somewhat clumsy that ROM and RAM targeted code is mixed up together in the same source files (board.c etc). I think we may now have a unified path for relocation (without the -mrelocatable PPC only stuff). If this is the case, we can finish the task of making all arches relocatable and split U-Boot cleanly down the ROM/RAM divide.
For those of us who don't care about PIC in ROM, life is easy. For those that want full PIC, they just need to put in a little more effort into their ROM based code in U1.
There are many reasons: ease of porting and debugging, minimization of boot time, etc.
This opens up a new option for the previously mentioned use-case. We could totally split U1 and U2 into seperate make targets. Once you have a stable U1/U2 you flash them onto your board and then tweak versions of U2 and load them into a seperate location in Flash (into seperate banks to avoid erasing your known good U1/U2). At boot, you could have a console input to load 'last known good config' if your new U2 is a complete dud.
Granted you'd have some added effort there, but possibly not so much in the porting and boot time departments, as executing U1 then U2 would roughly be the same as running today's full U-Boot, and equally, porting effort would be split rather than duplicated.
and a fuller secondary bootloader (U2?). U1 would initialize RAM (and a console?) and U2 would initialize everything else. Each stage would only run from a fixed location and type of memory, removing the need for PIC.
If you use a console in U1, you will need to share a LOT of code with U2 - things like printf() and all it's dependencies, most of the str*() and mem*() functions, etc. And especially for such complicted and error prone actions like initializing the RAM you _do_ want to be able to use a console port to print error messages and debug information.
Nothing prevents linking in the same source code in U1 and U2, I think. Of course that would make U1 bigger, but you'd need the code in there so that's the price to pay -- and it would be a temporary use of RAM, as the RAM for U1 could be reused freed when U2 comes into play.
Thsi is _exactly_ where the current design is coming from.
But obviously your call for comments also calls for a revision of the current design, doesn't it?
Regards,
G
On Sun, Jan 03, 2010 at 09:06:51PM +0100, Albert ARIBAUD wrote:
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Note that the first-stage NAND loader still needs to be able to relocate itself to RAM in order to free up the NAND buffer for loading more data.
-Scott
u-boot-bounces@lists.denx.de wrote on 05/01/2010 21:20:32:
From: Scott Wood scottwood@freescale.com To: Albert ARIBAUD albert.aribaud@free.fr Cc: u-boot@lists.denx.de Date: 05/01/2010 21:22 Subject: Re: [U-Boot] [PATCH 2/4] Use LINK_OFF to access global data Sent by: u-boot-bounces@lists.denx.de
On Sun, Jan 03, 2010 at 09:06:51PM +0100, Albert ARIBAUD wrote:
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Note that the first-stage NAND loader still needs to be able to relocate itself to RAM in order to free up the NAND buffer for loading more data.
Hmm, does that mean that the LINK_OFF patches are useful to you or not?
Joakim Tjernlund wrote:
u-boot-bounces@lists.denx.de wrote on 05/01/2010 21:20:32:
From: Scott Wood scottwood@freescale.com To: Albert ARIBAUD albert.aribaud@free.fr Cc: u-boot@lists.denx.de Date: 05/01/2010 21:22 Subject: Re: [U-Boot] [PATCH 2/4] Use LINK_OFF to access global data Sent by: u-boot-bounces@lists.denx.de
On Sun, Jan 03, 2010 at 09:06:51PM +0100, Albert ARIBAUD wrote:
Hmm... PIC is interesting only if you want the same binary to run from two places, like NOR then RAM, which is the case when U-boot is the code which gets run in NOR at power-up and ends up running in RAM later.
For NAND-based boards, the NAND bootloader will load U-boot to RAM, and U-boot will never run from anywhere else but its intended RAM location.
Note that the first-stage NAND loader still needs to be able to relocate itself to RAM in order to free up the NAND buffer for loading more data.
Hmm, does that mean that the LINK_OFF patches are useful to you or not?
I was just responding to a suggestion that a split similar to the NAND loader might eliminate the need for relocation/PIC support.
I am a bit nervous about this stuff, though -- why is it needed? We just got rid of the need for manual relocations, and now we're adding them back (pre-reloc instead of post-reloc). :-(
-Scott
Wolfgang Denk wd@denx.de wrote on 03/01/2010 20:51:53:
Dear Joakim Tjernlund,
In message <OF3EBC8B6E.2C26AE15-ONC12576A0.003379DE-C12576A0. 0039F518@transmode.se> you wrote:
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)).
Well, I understand this is not only puts(), but also all places where a constant strings are used. And this is a lot of places and a lot of functions.
It is not too bad when you limit the scope to pre RAM functions. Consider I got my fairly complex 83xx board working with these patches and that wasn't too bad was it?
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.
Ouch.
Yeah, I whish ppc at least could access strings relative to text :(
I selected the changes I needed to for my 83xx board and then some. I don't think serial.c needs any more changes.
Understood. But that means we don't see the real scope of this change yet, as adapting a new board to use this featue will become a
True.
trial-and-error procedure, adding incrementally more and more of these changes (unless someone performs a thorough review and catches most of these places in an initial commit).
So much else is trial-and-error so I don't think this a killer. After all, this is a fairly advanced feature so it is no surprise it will cost some effort to add support for a board.
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.
I have to admit that I dislike the impact of this change, and I'm not sure if we really want to take this route.
This was exactly my thinking too. In the end I had to impl. it to see how bad it would be. After seeing the result I decided it was worth it.
Comments from others are welcome and needed here!
I think as follows:
In the past, the majority of systems supported by U-Boot where booting from NOR flash or other memory devices. This made it easy to use common code (like library functions) both before and after relocation to the final location in RAM. For your current changes this means that we have a large number of places where we have to add this LINK_OFF stuff. This makes the code harder to read, much harder
You don't HAVE to add LINK_OFF all over. Only boards that whishes to use this feature needs to add some more LINK_OFF calls.
to understand (especially if it's not working during the initial bringup on new hardware), and harder to debug in general.
If I try to see trends in the development of U-Boot I notice a growing number of systems that boot from NAND flash, DataFlash or that come with on-chip ROM code to load images from SDCard and other storage media. Such systems cannot make real benefit from the original design of U-Boot, as here U-Boot is inherently a second-stage boot loader which gets loaded by some other means. Even for NAND booting systems where we have the NAND boot code included within the U-Boot source tree we often cannot share much of the code between the primary and the secondary loader stages as there are usually tight restrictions on the maximum size for the primary loader image. Here a sharper separation of "primary" and "secondary" boot code within U-Boot would be benefical.
I feel (but this is really just a feeling, and I definitely would like to hear what others think about this!) your PIC changes would be (or have been) useful for the former usage mode, but they come at a pretty heavy cost as they are really invasive to the code. For the second usage mode they are not usable, or at least not useful. This makes me wonder if we really should continue to work in this direction.
I have not worked with NAND based boards yet so I can't really say if this is useful to such systems. I do think though that NOR based board will be common for quite some time to come so this new feature will be useful for a long time. Who knows, perhaps some new designs will be NOR based just because u-boot has this feature.
FWIW, I do agree with you to not split u-boot into a 2 stage loader, for pretty much the same reasons you listed.
Jocke
On Sunday 03 January 2010 14:51:53 Wolfgang Denk wrote:
If I try to see trends in the development of U-Boot I notice a growing number of systems that boot from NAND flash, DataFlash or that come with on-chip ROM code to load images from SDCard and other storage media. Such systems cannot make real benefit from the original design of U-Boot, as here U-Boot is inherently a second-stage boot loader which gets loaded by some other means. Even for NAND booting systems where we have the NAND boot code included within the U-Boot source tree we often cannot share much of the code between the primary and the secondary loader stages as there are usually tight restrictions on the maximum size for the primary loader image. Here a sharper separation of "primary" and "secondary" boot code within U-Boot would be benefical.
I feel (but this is really just a feeling, and I definitely would like to hear what others think about this!) your PIC changes would be (or have been) useful for the former usage mode, but they come at a pretty heavy cost as they are really invasive to the code. For the second usage mode they are not usable, or at least not useful. This makes me wonder if we really should continue to work in this direction.
the Blackfin processors are going the direction of an on-chip ROM handles loading from some storage (nor/nand/spi/usb/uart/sd/whatever) into RAM. there really isnt any usage model of executing in NOR for some time before running out of RAM. so generally, all this additional support is seen largely as overhead on Blackfin parts since there isnt any need/desire for them. -mike
participants (7)
-
Albert ARIBAUD -
Graeme Russ -
Joakim Tjernlund -
Joakim Tjernlund
-
Mike Frysinger -
Scott Wood -
Wolfgang Denk