Broken socrates board (Was: Re: Broken commit de47ff536363289f92f85ed1e4901724d238432d)

[Pali Rohár]

On Wednesday 28 December 2022 17:50:43 Pali Rohár wrote:
> And back to this issue...
> 
> On Tuesday 02 August 2022 11:13:38 Pali Rohár wrote:
> > Hello Tom!
> > 
> > Your commit de47ff536363289f92f85ed1e4901724d238432d ("Convert
> > CONFIG_SYS_MPC85XX_NO_RESETVEC to Kconfig") seems to be broken.
> > 
> > If you look at P1020RDB-PD_defconfig file change in this commit there is:
> > 
> > --- a/configs/P1020RDB-PD_defconfig
> > +++ b/configs/P1020RDB-PD_defconfig
> > @@ -9,6 +9,7 @@ CONFIG_MPC85xx=y
> >  # CONFIG_CMD_ERRATA is not set
> >  CONFIG_TARGET_P1020RDB_PD=y
> >  CONFIG_MPC85XX_HAVE_RESET_VECTOR=y
> > +CONFIG_SYS_MPC85XX_NO_RESETVEC=y
> >  CONFIG_MP=y
> >  CONFIG_FIT=y
> >  CONFIG_FIT_VERBOSE=y
> > 
> > Which does not make sense to me.
> > 
> > First thing is that CONFIG_MPC85XX_HAVE_RESET_VECTOR and
> > CONFIG_SYS_MPC85XX_NO_RESETVEC are exclusive options. You can either
> > disable generating of reset vector in image or enable it. What is
> > expected from the result when you ask Kconfig to both enable and disable
> > it? First specified option win? Or last specified win? Or random of
> > those two options win? It is not really clear for me.
> 
> Experiments proved that CONFIG_SYS_MPC85XX_NO_RESETVEC wins over
> CONFIG_MPC85XX_HAVE_RESET_VECTOR. So problematic commit
> de47ff536363289f92f85ed1e4901724d238432d effectively disabled reset
> vectors in more defconfig files.
> 
> > Second thing is that reset vector is required for (parallel) NOR booting
> > and your change is adding CONFIG_SYS_MPC85XX_NO_RESETVEC=y to defconfig
> > for NOR, which to my guess make image non-bootable and broken.
> 
> And this is truth. CONFIG_SYS_MPC85XX_NO_RESETVEC=y in defconfig broke
> booting from parallel FLASH NOR memory. Without reset vector, u-boot
> from FLASH cannot be booted.
> 
> When I manually disabled CONFIG_SYS_MPC85XX_NO_RESETVEC for P2020 then
> together with CONFIG_SDCARD fix, I was able to boot U-Boot from FLASH.
> 
> So kconfig conversion in commit de47ff536363289f92f85ed1e4901724d238432d
> was done incorrectly. Because in 2022.04 CONFIG_SYS_MPC85XX_NO_RESETVEC
> was really not enabled in config.h for FLASH defconfigs.
> 
> > And seems that other defconfig files in that change have similar issues.
> 
> Tom, would you fix this commit de47ff536363289f92f85ed1e4901724d238432d
> too? I do not know how you did that kconfig conversion but fix could be
> straightforward. By boolean logic CONFIG_MPC85XX_HAVE_RESET_VECTOR xor
> CONFIG_SYS_MPC85XX_NO_RESETVEC can be defined. Not both at the same
> time.
> 
> By moveconfig.py following defconfigs are affected:
> 
> $ ./tools/moveconfig.py -f MPC85XX_HAVE_RESET_VECTOR SYS_MPC85XX_NO_RESETVEC
> 9 matches
> P1020RDB-PC P1010RDB-PB_36BIT_NOR P1010RDB-PA_36BIT_NOR P1020RDB-PC_36BIT P1010RDB-PA_NOR P1010RDB-PB_NOR P2020RDB-PC P2020RDB-PC_36BIT P1020RDB-PD

And there is one _interesting_ board:

$ for arch in `git grep 'config ARCH' arch/powerpc/cpu/mpc85xx/Kconfig | sed 's/.* //'`; do ./tools/moveconfig.py -f $arch ~MPC85XX_HAVE_RESET_VECTOR ~SYS_MPC85XX_NO_RESETVEC; done | grep -v ' matches\|^$'
socrates

(I do not know how to easily tell moveconfig.py to do OR, so I called in
more times in loop).

socrates_defconfig is the only one mpc85xx board which does not have
enabled reset vectors nor disabled reset vectors.

As it boots from flash too, it should have reset vector in its binary.
I have looked into compiled u-boot ELF binary and reset vector is there.
So good.

But in u-boot.bin binary it is not, so it is broken.

Checking with v2022.04 release and it is quite interesting. U-Boot build
process produce more *.bin binaries:

-rw-r--r-- 1 pali pali 530613 dec 28 17:57 u-boot.bin
-rw-r--r-- 1 pali pali 530613 dec 28 17:57 u-boot-dtb.bin
-rwxr-xr-x 1 pali pali 524288 dec 28 17:57 u-boot-nodtb.bin
-rw-r--r-- 1 pali pali 655360 dec 28 17:57 u-boot-socrates.bin

u-boot.bin and u-boot-dtb.bin are broken. u-boot-nodtb.bin is
intermediate and u-boot-socrates.bin seems to be correct (contains reset
vector at correct offset and also has DTB file in it).

U-Boot master build only broken u-boot.bin and does *not* build correct
u-boot-socrates.bin.

I bisected this issue why u-boot-socrates.bin stopped being building.
And reason is my commit 5af42eafd7e1 ("Makefile: Reduce usage of custom
mpc85xx u-boot.bin target"). It is binman who built "correct" binary and
that commit deselected it.

I looked again at CONFIG_MPC85XX_HAVE_RESET_VECTOR symbol and its
meaning is currently "build only intermediate binaries for binman,
separate one for u-boot main code, separate one for reset vector,
separate for DTB and let binman to merge them via standard powerpc
config binman file arch/powerpc/dts/u-boot.dtsi to output u-boot.bin."

>From the code, this socrates is expected to build final binary
u-boot-socrates.bin (not u-boot.bin) via binman, but not via common
powerpc u-boot.dtsi, but rather via arch/powerpc/dts/socrates-u-boot.dtsi
So it does not use CONFIG_MPC85XX_HAVE_RESET_VECTOR symbol (which is
just for common powerpc binman stage).

I'm going to send a fix for socrates, so build process start again
building u-boot-socrates.bin binary and let it to v2022.04 state.

The long term solution for socrates should be:
1) Stop building broken and unused u-boot.bin
2) Rename u-boot-socrates.bin to u-boot.bin

PS: Similarly is broken also keymile board, but for this one I had
realized that is "custom" and months ago I sent patch for it for
conversion to use common powerpc binman rule, even before commit
5af42eafd7e1 which broke both keymile and socrates:
https://patchwork.ozlabs.org/project/uboot/patch/20220803112049.4287-1-pali@kernel.org/

> All of these defconfigs (by their names) boot from FLASH nor, so they
> must have reset vector included and so *NO_RESETVEC* must *not* be
> enabled. (Hope it is clear even with too many negations)