Patchwork [U-Boot,3/3] microblaze: Fix bdiinfo pointer

login
register
mail settings
Submitter Michal Simek
Date Dec. 21, 2010, 6:12 p.m.
Message ID <1292955178-13018-3-git-send-email-monstr@monstr.eu>
Download mbox | patch
Permalink /patch/76310/
State Changes Requested
Headers show

Comments

Michal Simek - Dec. 21, 2010, 6:12 p.m.
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(-)
Wolfgang Denk - Dec. 21, 2010, 7:01 p.m.
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
Michal Simek - Dec. 21, 2010, 8:45 p.m.
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
Wolfgang Denk - Dec. 21, 2010, 9:15 p.m.
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

Patch

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