[U-Boot] ALERT! >90% of all board configurations BROKEN

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Wolfgang Denk]

Hello,

this is an ALERT!

I just noticed (when debugging a strage phenomenon) that most of the
board configurations (170 out of 191 for PowerPC, _ALL_ for ARM) are
broken, because the size of struct global_data has grown but
CONFIG_SYS_GBL_DATA_SIZE has not been adapted (commit 91a7675 added
32 bytes - and yes, I am to blame for that, where is that brown paper
bag...)

Fact is, the overwhelming majority of boards has currently problems
like this:

sizeof(struct global_data) = 136, CONFIG_SYS_GBL_DATA_SIZE = 128


This may appear to work, or may cause "funny" errors, or simply hangs
the board hard very early in booting.

If you are debugging strange problems, check this first!

In the mean time I will try to come up with a clean solution for that.

Um... and sorry.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No question is too silly to ask. Of course, some  questions  are  too
silly to to answer...  - L. Wall & R. L. Schwartz, _Programming Perl_

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Reinhard Meyer]

Dear Wolfgang Denk,
> this is an ALERT!
>
> I just noticed (when debugging a strage phenomenon) that most of the
> board configurations (170 out of 191 for PowerPC, _ALL_ for ARM) are
> broken, because the size of struct global_data has grown but
> CONFIG_SYS_GBL_DATA_SIZE has not been adapted (commit 91a7675 added
> 32 bytes - and yes, I am to blame for that, where is that brown paper
> bag...)
>
> Fact is, the overwhelming majority of boards has currently problems
> like this:
>
> sizeof(struct global_data) = 136, CONFIG_SYS_GBL_DATA_SIZE = 128
>
>
> This may appear to work, or may cause "funny" errors, or simply hangs
> the board hard very early in booting.

Grep-ing for CONFIG_SYS_GBL_DATA_SIZE in *.[chsS] Makefile *.ld it
seems to me that with "ELF-reloc" active that define is not used
anywhere at least in ARM.

Or did I miss a place?

Best Regards,
Reinhard

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC5E865.70003@emk-elektronik.de> you wrote:
>
> Grep-ing for CONFIG_SYS_GBL_DATA_SIZE in *.[chsS] Makefile *.ld it
> seems to me that with "ELF-reloc" active that define is not used
> anywhere at least in ARM.

Are you sure? My very first smaple sees:

"arch/arm/cpu/arm926ejs/start.S"

346         /* Set up the stack                                                 */
347 stack_setup:
348         ldr     r0, _TEXT_BASE          /* upper 128 KiB: relocated uboot   */
349         sub     sp, r0, #128            /* leave 32 words for abort-stack   */
350 #ifndef CONFIG_PRELOADER
351         sub     r0, r0, #CONFIG_SYS_MALLOC_LEN  /* malloc area                      */
352         sub     r0, r0, #CONFIG_SYS_GBL_DATA_SIZE /* bdinfo                        */


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is a multi-legged creature crawling on your shoulder.
	-- Spock, "A Taste of Armageddon", stardate 3193.9

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Reinhard Meyer]

Dear Wolfgang Denk,
> In message<4CC5E865.70003@emk-elektronik.de>  you wrote:
>>
>> Grep-ing for CONFIG_SYS_GBL_DATA_SIZE in *.[chsS] Makefile *.ld it
>> seems to me that with "ELF-reloc" active that define is not used
>> anywhere at least in ARM.
>
> Are you sure? My very first smaple sees:
>
> "arch/arm/cpu/arm926ejs/start.S"
>
> 346         /* Set up the stack                                                 */
> 347 stack_setup:
> 348         ldr     r0, _TEXT_BASE          /* upper 128 KiB: relocated uboot   */
> 349         sub     sp, r0, #128            /* leave 32 words for abort-stack   */
> 350 #ifndef CONFIG_PRELOADER
> 351         sub     r0, r0, #CONFIG_SYS_MALLOC_LEN  /* malloc area                      */
> 352         sub     r0, r0, #CONFIG_SYS_GBL_DATA_SIZE /* bdinfo                        */

That is #ifdef-ed away in case of ARM-relocation. Perhaps we should remove
all code that pertains to "WITHOUT_RELOC"... Would make the rest of the code
less obscure...

I changed my board.config like this:
...
/*#define CONFIG_SYS_GBL_DATA_SIZE 128*/	/* 128 bytes for initial data */
...
#define	CONFIG_SYS_INIT_SP_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x1000 /*- CONFIG_SYS_GBL_DATA_SIZE*/)
...
and it still compiles. The second line is the only use of GBL_DATA_SIZE,
since that is often set to somewhere in SRAM, it is still ok - even if
set to the end of SRAM it will most likely wrap into the repeated SRAM
image on most SoCs.

in start.S:

/* Set stackpointer in internal RAM to call board_init_f */
call_board_init_f:
	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
	ldr	r0,=0x00000000
	bl	board_init_f

in board.c:

void board_init_f (ulong bootflag)
{
	bd_t *bd;
	init_fnc_t **init_fnc_ptr;
	gd_t *id;
	ulong addr, addr_sp;

	/* Pointer is writable since we allocated a register for it */
	gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);

So global data is at CONFIG_SYS_INIT_SP_ADDR, stack goes down from
CONFIG_SYS_INIT_SP_ADDR.

Board maintainers for ARM should be aware that CONFIG_SYS_INIT_SP_ADDR
should point to RAM with enough space for global data above and enough
stack space below.

Best Regards,
Reinhard

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Albert ARIBAUD]

Le 26/10/2010 02:11, Reinhard Meyer a ?crit :

> That is #ifdef-ed away in case of ARM-relocation. Perhaps we should remove
> all code that pertains to "WITHOUT_RELOC"... Would make the rest of the code
> less obscure...

> I changed my board.config like this:
> ...
> /*#define CONFIG_SYS_GBL_DATA_SIZE 128*/	/* 128 bytes for initial data */
> ...
> #define	CONFIG_SYS_INIT_SP_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x1000 /*- CONFIG_SYS_GBL_DATA_SIZE*/)
> ...
> and it still compiles. The second line is the only use of GBL_DATA_SIZE,
> since that is often set to somewhere in SRAM, it is still ok - even if
> set to the end of SRAM it will most likely wrap into the repeated SRAM
> image on most SoCs.

Not all SoCs have SRAM. While the kirkwood family always has it, as does 
the Marvell 5182, the 5281 doesn't, for instance.

Amicalement,
-- 
Albert.

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Heiko Schocher]

Hello Reinhard,

Reinhard Meyer wrote:
> Dear Wolfgang Denk,
>> this is an ALERT!
>>
>> I just noticed (when debugging a strage phenomenon) that most of the
>> board configurations (170 out of 191 for PowerPC, _ALL_ for ARM) are
>> broken, because the size of struct global_data has grown but
>> CONFIG_SYS_GBL_DATA_SIZE has not been adapted (commit 91a7675 added
>> 32 bytes - and yes, I am to blame for that, where is that brown paper
>> bag...)
>>
>> Fact is, the overwhelming majority of boards has currently problems
>> like this:
>>
>> sizeof(struct global_data) = 136, CONFIG_SYS_GBL_DATA_SIZE = 128
>>
>>
>> This may appear to work, or may cause "funny" errors, or simply hangs
>> the board hard very early in booting.
> 
> Grep-ing for CONFIG_SYS_GBL_DATA_SIZE in *.[chsS] Makefile *.ld it
> seems to me that with "ELF-reloc" active that define is not used
> anywhere at least in ARM.
> 
> Or did I miss a place?

Yep, for example I use it on the beagle3 board, look in:
include/configs/omap3_beagle.h

Checked sizeof(struct global_data) on beagle board, I get 92 bytes,
which is OK with the define in the boardconfig:

#define CONFIG_SYS_GBL_DATA_SIZE        128     /* bytes reserved for */
                                                /* initial data */

A fast look in "arch/arm/include/asm/global_data.h":

I think there should be only problems on arm boards, if
CONFIG_AT91FAMILY is defined, because this define adds + 44 bytes ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Reinhard Meyer]

Dear Albert ARIBAUD,

>> That is #ifdef-ed away in case of ARM-relocation. Perhaps we should remove
>> all code that pertains to "WITHOUT_RELOC"... Would make the rest of the code
>> less obscure...
> 
>> I changed my board.config like this:
>> ...
>> /*#define CONFIG_SYS_GBL_DATA_SIZE 128*/	/* 128 bytes for initial data */
>> ...
>> #define	CONFIG_SYS_INIT_SP_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x1000 /*- CONFIG_SYS_GBL_DATA_SIZE*/)

I must confess I copied that #define from other boards without much reasoning about it:)

>> ...
>> and it still compiles. The second line is the only use of GBL_DATA_SIZE,
>> since that is often set to somewhere in SRAM, it is still ok - even if
>> set to the end of SRAM it will most likely wrap into the repeated SRAM
>> image on most SoCs.
> 
> Not all SoCs have SRAM. While the kirkwood family always has it, as does 
> the Marvell 5182, the 5281 doesn't, for instance.

Point is, that probably there is enough space above CONFIG_SYS_INIT_SP_ADDR
to hold the global data before relocation without overwriting essential values
and without hitting an access fault, so the mistake did not hurt.

And as Heiko points out, the global data is well below 128 bytes in the usual
ARM cases.

With ELF relocation (on AT91) boards the single use of CONFIG_SYS_GBL_DATA_SIZE is to
reserve space above CONFIG_SYS_INIT_SP_ADDR.

To really document that relationship, I will reorder the defines in my <board>.h:

#define CONFIG_SYS_GBL_DATA_SIZE 256	/* 256 bytes for initial data */
#define	CONFIG_SYS_INIT_SP_ADDR	(CONFIG_SYS_SDRAM_BASE + 0x1000 - CONFIG_SYS_GBL_DATA_SIZE)


Reinhard

[U-Boot] ALERT! >90% of all board configurations BROKEN

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC6909F.4060503@emk-elektronik.de> you wrote:
> Dear Albert ARIBAUD,
> 
> >> That is #ifdef-ed away in case of ARM-relocation. Perhaps we should remove
> >> all code that pertains to "WITHOUT_RELOC"... Would make the rest of the code
> >> less obscure...
> > 
> >> I changed my board.config like this:
> >> ...
> >> /*#define CONFIG_SYS_GBL_DATA_SIZE 128*/	/* 128 bytes for initial data */
> >> ...
> >> #define	CONFIG_SYS_INIT_SP_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x1000 /*- CONFIG_SYS_GBL_DATA_SIZE*/)
> 
> I must confess I copied that #define from other boards without much reasoning about it:)

I just mentioned it in another mail: there is lots of confusion in
this area, and little common code. I will try to remove all of this.
All we need is the address and size of the initial RAM area; all the
rest can (now) be automatically calculated.  Patches are in the works.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Emotions are alien to me.  I'm a scientist.
	-- Spock, "This Side of Paradise", stardate 3417.3