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

[Simon Glass]

Hi Pali,

On Wed, 28 Dec 2022 at 11:51, Pali Rohár <pali at kernel.org> wrote:
>
> 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

Just a note on this...that file is produced by all boards. If a board
needs to create another file, e.g. u-boot-rockchip.bin then that is
fine. But the u-boot.bin file should remain.

> 2) Rename u-boot-socrates.bin to u-boot.bin

Please don't do that!

>
> 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)

Regards,
Simon