[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[Heiko Schocher]

since:
commit: f05ad9ba "Add a way to skip relocation"

tqm5200s board fails to boot. Reason is that
board_init_f has a function parameter bootflag,
which is not setup in
in arch/powerpc/cpu/mpc5xxx/start.S _start

So board_init_f gets a undefined bootflag,
currently the gd pointer address. Unfortunately
this address sets the GD_FLG_SKIP_RELOC bit,
so u-boot code gets not relocated and u-boot
does not boot ...

Init bootflag with 0, and tqm5200 boots fine again.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
I added a nightly build to:
http://xeidos.ddns.net/buildbot/waterfall
http://xeidos.ddns.net/buildbot/builders/tqm5200s_ml_ub

What does this nightly build:
- checkout current u-boot mainline code
- compile u-boot for tqm5200s board
- install u-boot image on the board with bdi
- run the image, and look if u-boot boots
ToDo:
- add more tbot [1] tests for this board

Currently failing, as this patch is not in mainline,
but it should go to green, if this patch is applied,
cross your fingers ;-)

[1] https://github.com/hsdenx/tbot

 arch/powerpc/cpu/mpc5xxx/start.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/cpu/mpc5xxx/start.S b/arch/powerpc/cpu/mpc5xxx/start.S
index 94eb0d3..54793f0 100644
--- a/arch/powerpc/cpu/mpc5xxx/start.S
+++ b/arch/powerpc/cpu/mpc5xxx/start.S
@@ -91,6 +91,7 @@ _start:
 	li	r5,GD_SIZE	/* parameter 3:	 count			*/
 	bl	memset
 
+	li	r3, 0		/* parameter 1:	 bootflag		*/
 	bl	board_init_f	/* run 1st part of board init code (in Flash)*/
 	/* NOTREACHED - board_init_f() does not return */
 #else
@@ -169,6 +170,7 @@ lowboot_reentry:
 	/* r3: IMMR */
 	bl	cpu_init_f	/* run low-level CPU init code (in Flash)*/
 
+	li	r3, 0		/* parameter 1:	 bootflag		*/
 	bl	board_init_f	/* run 1st part of board init code (in Flash)*/
 
 	/* NOTREACHED - board_init_f() does not return */
-- 
2.1.0

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[Simon Glass]

On 25 November 2015 at 01:46, Heiko Schocher <hs@denx.de> wrote:
> since:
> commit: f05ad9ba "Add a way to skip relocation"
>
> tqm5200s board fails to boot. Reason is that
> board_init_f has a function parameter bootflag,
> which is not setup in
> in arch/powerpc/cpu/mpc5xxx/start.S _start
>
> So board_init_f gets a undefined bootflag,
> currently the gd pointer address. Unfortunately
> this address sets the GD_FLG_SKIP_RELOC bit,
> so u-boot code gets not relocated and u-boot
> does not boot ...
>
> Init bootflag with 0, and tqm5200 boots fine again.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> I added a nightly build to:
> http://xeidos.ddns.net/buildbot/waterfall
> http://xeidos.ddns.net/buildbot/builders/tqm5200s_ml_ub
>
> What does this nightly build:
> - checkout current u-boot mainline code
> - compile u-boot for tqm5200s board
> - install u-boot image on the board with bdi
> - run the image, and look if u-boot boots
> ToDo:
> - add more tbot [1] tests for this board
>
> Currently failing, as this patch is not in mainline,
> but it should go to green, if this patch is applied,
> cross your fingers ;-)
>
> [1] https://github.com/hsdenx/tbot
>
>  arch/powerpc/cpu/mpc5xxx/start.S | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/cpu/mpc5xxx/start.S b/arch/powerpc/cpu/mpc5xxx/start.S
> index 94eb0d3..54793f0 100644
> --- a/arch/powerpc/cpu/mpc5xxx/start.S
> +++ b/arch/powerpc/cpu/mpc5xxx/start.S
> @@ -91,6 +91,7 @@ _start:
>         li      r5,GD_SIZE      /* parameter 3:  count                  */
>         bl      memset
>
> +       li      r3, 0           /* parameter 1:  bootflag               */
>         bl      board_init_f    /* run 1st part of board init code (in Flash)*/
>         /* NOTREACHED - board_init_f() does not return */
>  #else
> @@ -169,6 +170,7 @@ lowboot_reentry:
>         /* r3: IMMR */
>         bl      cpu_init_f      /* run low-level CPU init code (in Flash)*/
>
> +       li      r3, 0           /* parameter 1:  bootflag               */
>         bl      board_init_f    /* run 1st part of board init code (in Flash)*/
>
>         /* NOTREACHED - board_init_f() does not return */
> --
> 2.1.0
>

Funny, these latent bugs!

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[Heiko Schocher]

Hello Simon,

Am 26.11.2015 um 18:49 schrieb Simon Glass:
> On 25 November 2015 at 01:46, Heiko Schocher <hs@denx.de> wrote:
>> since:
>> commit: f05ad9ba "Add a way to skip relocation"
>>
>> tqm5200s board fails to boot. Reason is that
>> board_init_f has a function parameter bootflag,
>> which is not setup in
>> in arch/powerpc/cpu/mpc5xxx/start.S _start
>>
>> So board_init_f gets a undefined bootflag,
>> currently the gd pointer address. Unfortunately
>> this address sets the GD_FLG_SKIP_RELOC bit,
>> so u-boot code gets not relocated and u-boot
>> does not boot ...
>>
>> Init bootflag with 0, and tqm5200 boots fine again.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>> I added a nightly build to:
>> http://xeidos.ddns.net/buildbot/waterfall
>> http://xeidos.ddns.net/buildbot/builders/tqm5200s_ml_ub
>>
>> What does this nightly build:
>> - checkout current u-boot mainline code
>> - compile u-boot for tqm5200s board
>> - install u-boot image on the board with bdi
>> - run the image, and look if u-boot boots
>> ToDo:
>> - add more tbot [1] tests for this board
>>
>> Currently failing, as this patch is not in mainline,
>> but it should go to green, if this patch is applied,
>> cross your fingers ;-)
>>
>> [1] https://github.com/hsdenx/tbot
>>
>>   arch/powerpc/cpu/mpc5xxx/start.S | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/cpu/mpc5xxx/start.S b/arch/powerpc/cpu/mpc5xxx/start.S
>> index 94eb0d3..54793f0 100644
>> --- a/arch/powerpc/cpu/mpc5xxx/start.S
>> +++ b/arch/powerpc/cpu/mpc5xxx/start.S
>> @@ -91,6 +91,7 @@ _start:
>>          li      r5,GD_SIZE      /* parameter 3:  count                  */
>>          bl      memset
>>
>> +       li      r3, 0           /* parameter 1:  bootflag               */
>>          bl      board_init_f    /* run 1st part of board init code (in Flash)*/
>>          /* NOTREACHED - board_init_f() does not return */
>>   #else
>> @@ -169,6 +170,7 @@ lowboot_reentry:
>>          /* r3: IMMR */
>>          bl      cpu_init_f      /* run low-level CPU init code (in Flash)*/
>>
>> +       li      r3, 0           /* parameter 1:  bootflag               */
>>          bl      board_init_f    /* run 1st part of board init code (in Flash)*/
>>
>>          /* NOTREACHED - board_init_f() does not return */
>> --
>> 2.1.0
>>
>
> Funny, these latent bugs!

That;s the salt in our programming life ;-)

BTW: I wonder if this bootflag is really used?

> Reviewed-by: Simon Glass <sjg@chromium.org>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[Simon Glass]

Hi Heiko,

On 26 November 2015 at 21:23, Heiko Schocher <hs@denx.de> wrote:
> Hello Simon,
>
>
> Am 26.11.2015 um 18:49 schrieb Simon Glass:
>>
>> On 25 November 2015 at 01:46, Heiko Schocher <hs@denx.de> wrote:
>>>
>>> since:
>>> commit: f05ad9ba "Add a way to skip relocation"
>>>
>>> tqm5200s board fails to boot. Reason is that
>>> board_init_f has a function parameter bootflag,
>>> which is not setup in
>>> in arch/powerpc/cpu/mpc5xxx/start.S _start
>>>
>>> So board_init_f gets a undefined bootflag,
>>> currently the gd pointer address. Unfortunately
>>> this address sets the GD_FLG_SKIP_RELOC bit,
>>> so u-boot code gets not relocated and u-boot
>>> does not boot ...
>>>
>>> Init bootflag with 0, and tqm5200 boots fine again.
>>>
>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>> ---
>>> I added a nightly build to:
>>> http://xeidos.ddns.net/buildbot/waterfall
>>> http://xeidos.ddns.net/buildbot/builders/tqm5200s_ml_ub
>>>
>>> What does this nightly build:
>>> - checkout current u-boot mainline code
>>> - compile u-boot for tqm5200s board
>>> - install u-boot image on the board with bdi
>>> - run the image, and look if u-boot boots
>>> ToDo:
>>> - add more tbot [1] tests for this board
>>>
>>> Currently failing, as this patch is not in mainline,
>>> but it should go to green, if this patch is applied,
>>> cross your fingers ;-)
>>>
>>> [1] https://github.com/hsdenx/tbot
>>>
>>>   arch/powerpc/cpu/mpc5xxx/start.S | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/cpu/mpc5xxx/start.S
>>> b/arch/powerpc/cpu/mpc5xxx/start.S
>>> index 94eb0d3..54793f0 100644
>>> --- a/arch/powerpc/cpu/mpc5xxx/start.S
>>> +++ b/arch/powerpc/cpu/mpc5xxx/start.S
>>> @@ -91,6 +91,7 @@ _start:
>>>          li      r5,GD_SIZE      /* parameter 3:  count
>>> */
>>>          bl      memset
>>>
>>> +       li      r3, 0           /* parameter 1:  bootflag
>>> */
>>>          bl      board_init_f    /* run 1st part of board init code (in
>>> Flash)*/
>>>          /* NOTREACHED - board_init_f() does not return */
>>>   #else
>>> @@ -169,6 +170,7 @@ lowboot_reentry:
>>>          /* r3: IMMR */
>>>          bl      cpu_init_f      /* run low-level CPU init code (in
>>> Flash)*/
>>>
>>> +       li      r3, 0           /* parameter 1:  bootflag
>>> */
>>>          bl      board_init_f    /* run 1st part of board init code (in
>>> Flash)*/
>>>
>>>          /* NOTREACHED - board_init_f() does not return */
>>> --
>>> 2.1.0
>>>
>>
>> Funny, these latent bugs!
>
>
> That;s the salt in our programming life ;-)
>
> BTW: I wonder if this bootflag is really used?

Not that I have noticed. If PowerPC could get a crt0.S like ARM (i.e.
rationalise all the copies of similar assembly code) then it would be
easier to drop it, or change it.

Regards,
Simon

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[york sun]

On 11/25/2015 12:46 AM, Heiko Schocher wrote:
> since:
> commit: f05ad9ba "Add a way to skip relocation"
> 
> tqm5200s board fails to boot. Reason is that
> board_init_f has a function parameter bootflag,
> which is not setup in
> in arch/powerpc/cpu/mpc5xxx/start.S _start
> 
> So board_init_f gets a undefined bootflag,
> currently the gd pointer address. Unfortunately
> this address sets the GD_FLG_SKIP_RELOC bit,
> so u-boot code gets not relocated and u-boot
> does not boot ...
> 
> Init bootflag with 0, and tqm5200 boots fine again.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---

Applied to u-boot-mpc85x. Merged upstream.

York

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[Wolfgang Denk]

Dear York,

In message <AM4PR0401MB1732D7EBA1C64E62F3A403449AF50@AM4PR0401MB1732.eurprd04.prod.outlook.com> you wrote:
>
> > So board_init_f gets a undefined bootflag,
> > currently the gd pointer address. Unfortunately
> > this address sets the GD_FLG_SKIP_RELOC bit,
> > so u-boot code gets not relocated and u-boot
> > does not boot ...
> > 
> > Init bootflag with 0, and tqm5200 boots fine again.
...
> Applied to u-boot-mpc85x. Merged upstream.

Thanmks - but I think this needs a follow-up patch - Heiko is already
looking into that.  On closer inspection it appears as if the whole
passing around of "bootflag" variables is just dead code - there is no
place anywhere in U-Boot that makes real use of this data.  We should
remove all of the related code, which will make the code cleaner and
smaller - which might be especially welcome in a number of SPL
configurations.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
Quotation, n. The act of repeating erroneously the words of  another.
The  words  erroneously  repeated.
                            - Ambrose Bierce _The Devil's Dictionary_

[U-Boot] [PATCH] powerpc, mpc5xxx: fix missing bootflag init

[Heiko Schocher]

Hello Wolfgang, York,

Am 07.01.2016 um 20:30 schrieb Wolfgang Denk:
> Dear York,
>
> In message <AM4PR0401MB1732D7EBA1C64E62F3A403449AF50@AM4PR0401MB1732.eurprd04.prod.outlook.com> you wrote:
>>
>>> So board_init_f gets a undefined bootflag,
>>> currently the gd pointer address. Unfortunately
>>> this address sets the GD_FLG_SKIP_RELOC bit,
>>> so u-boot code gets not relocated and u-boot
>>> does not boot ...
>>>
>>> Init bootflag with 0, and tqm5200 boots fine again.
> ...
>> Applied to u-boot-mpc85x. Merged upstream.
>
> Thanmks - but I think this needs a follow-up patch - Heiko is already
> looking into that.  On closer inspection it appears as if the whole
> passing around of "bootflag" variables is just dead code - there is no
> place anywhere in U-Boot that makes real use of this data.  We should
> remove all of the related code, which will make the code cleaner and
> smaller - which might be especially welcome in a number of SPL
> configurations.

Yes, I am trying to look into it ...

BTW: Now with this patch applied, automated tbot test [1] is now green
-> tqm5200 board works again with current mainline :-D

bye,
Heiko
[1] http://xeidos.ddns.net/buildbot/builders/tqm5200s_ml_ub
     log:
     http://xeidos.ddns.net/buildbot/builders/tqm5200s_ml_ub/builds/20/steps/shell/logs/tbotlog
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany