| Message ID | 1448441179-6214-1-git-send-email-hs@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | York Sun |
| Headers | show |
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>
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
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
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
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
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
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 */
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(+)