From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Thu, 06 Nov 2014 13:10:33 +0100
Subject: [U-Boot] arch/arm/lib/board.c - uninitialized vars
In-Reply-To: <20141106112841.78173382347@gemini.denx.de>
References: <20141106112841.78173382347@gemini.denx.de>
Message-ID: <545B6539.9090309@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Wolfgang,

Am 06.11.2014 12:28, schrieb Wolfgang Denk:
> Hi,
>
> I'm trying to clean up some warnings/errors detected when running
> "cppcheck" on the U-Boot source tree.  For arch/arm/lib/board.c
> I get this:
>
> [arch/arm/lib/board.c:445]: (error) Uninitialized variable: id
> [arch/arm/lib/board.c:422]: (error) Uninitialized variable: addr_sp
>
> The problem is not usually detected by GCC depending on which macros
> are active, here especially CONFIG_SPL_BUILD
>
> The relevant code was last touched / introduced by commit f1d2b313:
> "ARM: add relocation support" some two years ago...
>
> I have some questions regarding the CONFIG_SPL_BUILD "else" case
> (i. e. when building with CONFIG_SPL_BUILD defined):
>
> 422         addr_sp += 128; /* leave 32 words for abort-stack   */
>
> Is this correct?  The stack is growing downward, so should the '+' not
> be replaced by a '-', like we do a few lines above:

No, this is a typo ... it must be a "-"

>
> 412         /* leave 3 words for abort-stack    */
> 413         addr_sp -= 12;
>
> Why do we need 128 words in the CONFIG_SPL_BUILD case, but only 3
> otherwise?

Good question ...

> Should we not move the "alignment for ABI compliance" part outside the
> CONFIG_SPL_BUILD if/else case, i. e. should this not always be done?

Yes, I think thats correct.

But looking into common/board_f.c ... there it is also done only
for the not SPL case ...

going back in history ...

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/arm926ejs/start.S;h=cf40ce12928359c5238d02a32ed361ccc153c101;hb=a59e27997637a2395ae2cc7f809127f24119a167
here I see:

leave 32 words for abort-stack
leave 3 words for abort-stack
8-byte alignment for ABI compliance

without SPL (former PRELOADER) define mess ...

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/arm1136/start.S;h=41eb82dae246b9509545f051daac605a02b7db05;hb=a59e27997637a2395ae2cc7f809127f24119a167#l179
here:

#ifdef CONFIG_PRELOADER
leave 32 words for abort-stack
#else
leave 3 words for abort-stack
#endif
8-byte alignment for ABI compliance

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/pxa/start.S;h=e07c8c2e0e70744c45152c1649cef65f87825708;hb=a59e27997637a2395ae2cc7f809127f24119a167
here
leave 3 words for abort-stack
8-byte alignment for ABI compliance

no "leave 32 words for abort-stack"

... :-(

> And of course, how should we correctly initialize the "id" and
> "addr_sp" variable in both cases?

Hmm.. they are initialized for the !SPL case ... and in the
SPL case, board__init_f is used from ./arch/arm/lib/spl.c
or overwritten from SoC dependend code, like done in:

./arch/arm/cpu/arm926ejs/davinci/spl.c

-> so I think, we can drop the CONFIG_SPL_BUILD in
u-boot:/arch/arm/lib/board.c board_init_f(), as it is not used, thats
maybe the reason, why this issue never poped up!

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