| Message ID | 1292955178-13018-3-git-send-email-monstr@monstr.eu |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Michal Simek, In message <1292955178-13018-3-git-send-email-monstr@monstr.eu> you wrote: > Patch "Replace CONFIG_SYS_GBL_DATA_SIZE by auto-generated value" > (sha1: 25ddd1fb0a2281b182529afbc8fda5de2dc16d96) > introduce GENERATED_GBL_DATA_SIZE which is sizeof aligned gd_t > (currently 0x40). > Microblaze configs used 0x40(128) because this place also contained > board info structure which lies on the top of ram. In the Subject: s/bdiinfo/bd_info/ > index eeef579..8232cf0 100644 > --- a/arch/microblaze/lib/board.c > +++ b/arch/microblaze/lib/board.c > @@ -91,15 +91,16 @@ void board_init (void) > bd_t *bd; > init_fnc_t **init_fnc_ptr; > gd = (gd_t *) CONFIG_SYS_GBL_DATA_OFFSET; > + bd = (bd_t *) CONFIG_SYS_GBL_DATA_OFFSET - GENERATED_BD_INFO_SIZE; This is actually wrong. You are using CONFIG_SYS_GBL_DATA_OFFSET as if it were CONFIG_SYS_GBL_DATA_ADDR, but it ain't so: it is an _offset_, and NOT and address. > - memset ((void *)gd, 0, GENERATED_GBL_DATA_SIZE); > - gd->bd = (bd_t *) (gd + 1); /* At end of global data */ > + memset ((void *)bd, 0, GENERATED_GBL_DATA_SIZE > + + GENERATED_BD_INFO_SIZE); Don't do this. Instead, use two separate memset() calls, one for gd and another one for bd. The stucts may be in a contiguous area now, but you would probably run into nasty bugs if this gets changed one day. Best regards, Wolfgang Denk
Wolfgang Denk wrote: > Dear Michal Simek, > > In message <1292955178-13018-3-git-send-email-monstr@monstr.eu> you wrote: >> Patch "Replace CONFIG_SYS_GBL_DATA_SIZE by auto-generated value" >> (sha1: 25ddd1fb0a2281b182529afbc8fda5de2dc16d96) >> introduce GENERATED_GBL_DATA_SIZE which is sizeof aligned gd_t >> (currently 0x40). >> Microblaze configs used 0x40(128) because this place also contained >> board info structure which lies on the top of ram. > > In the Subject: s/bdiinfo/bd_info/ > >> index eeef579..8232cf0 100644 >> --- a/arch/microblaze/lib/board.c >> +++ b/arch/microblaze/lib/board.c >> @@ -91,15 +91,16 @@ void board_init (void) >> bd_t *bd; >> init_fnc_t **init_fnc_ptr; >> gd = (gd_t *) CONFIG_SYS_GBL_DATA_OFFSET; >> + bd = (bd_t *) CONFIG_SYS_GBL_DATA_OFFSET - GENERATED_BD_INFO_SIZE; > > This is actually wrong. > > You are using CONFIG_SYS_GBL_DATA_OFFSET as if it were > CONFIG_SYS_GBL_DATA_ADDR, but it ain't so: it is an _offset_, and NOT > and address. I agree. BTW: Maybe nios2 and sparc use it too. > >> - memset ((void *)gd, 0, GENERATED_GBL_DATA_SIZE); >> - gd->bd = (bd_t *) (gd + 1); /* At end of global data */ >> + memset ((void *)bd, 0, GENERATED_GBL_DATA_SIZE >> + + GENERATED_BD_INFO_SIZE); > > Don't do this. Instead, use two separate memset() calls, one for gd > and another one for bd. The stucts may be in a contiguous area now, > but you would probably run into nasty bugs if this gets changed one > day. I just wanted to save some instructions and no problem to separate it. Thanks, Michal
Dear Michal Simek, In message <4D1111CF.5090002@monstr.eu> you wrote: > > >> @@ -91,15 +91,16 @@ void board_init (void) > >> bd_t *bd; > >> init_fnc_t **init_fnc_ptr; > >> gd = (gd_t *) CONFIG_SYS_GBL_DATA_OFFSET; > >> + bd = (bd_t *) CONFIG_SYS_GBL_DATA_OFFSET - GENERATED_BD_INFO_SIZE; > > > > This is actually wrong. > > > > You are using CONFIG_SYS_GBL_DATA_OFFSET as if it were > > CONFIG_SYS_GBL_DATA_ADDR, but it ain't so: it is an _offset_, and NOT > > and address. > > I agree. BTW: Maybe nios2 and sparc use it too. I see - I put the custodians on Cc:. > >> - memset ((void *)gd, 0, GENERATED_GBL_DATA_SIZE); > >> - gd->bd = (bd_t *) (gd + 1); /* At end of global data */ > >> + memset ((void *)bd, 0, GENERATED_GBL_DATA_SIZE > >> + + GENERATED_BD_INFO_SIZE); > > > > Don't do this. Instead, use two separate memset() calls, one for gd > > and another one for bd. The stucts may be in a contiguous area now, > > but you would probably run into nasty bugs if this gets changed one > > day. > > I just wanted to save some instructions and no problem to separate it. Yes, I understand this, but it's a dangerous thing to so, and robust and maintainable code is more important than a few bytes. Best regards, Wolfgang Denk
diff --git a/arch/microblaze/lib/board.c b/arch/microblaze/lib/board.c index eeef579..8232cf0 100644 --- a/arch/microblaze/lib/board.c +++ b/arch/microblaze/lib/board.c @@ -91,15 +91,16 @@ void board_init (void) bd_t *bd; init_fnc_t **init_fnc_ptr; gd = (gd_t *) CONFIG_SYS_GBL_DATA_OFFSET; + bd = (bd_t *) CONFIG_SYS_GBL_DATA_OFFSET - GENERATED_BD_INFO_SIZE; char *s; #if defined(CONFIG_CMD_FLASH) ulong flash_size = 0; #endif asm ("nop"); /* FIXME gd is not initialize - wait */ - memset ((void *)gd, 0, GENERATED_GBL_DATA_SIZE); - gd->bd = (bd_t *) (gd + 1); /* At end of global data */ + memset ((void *)bd, 0, GENERATED_GBL_DATA_SIZE + + GENERATED_BD_INFO_SIZE); + gd->bd = bd; gd->baudrate = CONFIG_BAUDRATE; - bd = gd->bd; bd->bi_baudrate = CONFIG_BAUDRATE; bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE; diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h index 75e4e07..fdfc0d8 100644 --- a/include/configs/microblaze-generic.h +++ b/include/configs/microblaze-generic.h @@ -142,9 +142,10 @@ /* monitor code */ #define SIZE 0x40000 -#define CONFIG_SYS_MONITOR_LEN (SIZE - GENERATED_GBL_DATA_SIZE) +#define CONFIG_SYS_MONITOR_LEN SIZE #define CONFIG_SYS_MONITOR_BASE \ - (CONFIG_SYS_GBL_DATA_OFFSET - CONFIG_SYS_MONITOR_LEN) + (CONFIG_SYS_GBL_DATA_OFFSET - CONFIG_SYS_MONITOR_LEN \ + - GENERATED_BD_INFO_SIZE) #define CONFIG_SYS_MONITOR_END \ (CONFIG_SYS_MONITOR_BASE + CONFIG_SYS_MONITOR_LEN) #define CONFIG_SYS_MALLOC_LEN SIZE
Patch "Replace CONFIG_SYS_GBL_DATA_SIZE by auto-generated value" (sha1: 25ddd1fb0a2281b182529afbc8fda5de2dc16d96) introduce GENERATED_GBL_DATA_SIZE which is sizeof aligned gd_t (currently 0x40). Microblaze configs used 0x40(128) because this place also contained board info structure which lies on the top of ram. U-Boot is placed to the top of the ram (for example 0xd7ffffff) and bd structure was moved out of ram. This patch is fixing this scheme with GENERATED_BD_INFO_SIZE which swap global data and board info structures. For example: Current: gd 0xd7ffffc0, bd 0xd8000000 Fixed: gd 0xd7ffffc0, bd 0xd7ffff90 Signed-off-by: Michal Simek <monstr@monstr.eu> --- arch/microblaze/lib/board.c | 7 ++++--- include/configs/microblaze-generic.h | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-)