Hi Pali, On Wed, 28 Dec 2022 at 11:51, Pali Rohár <[email protected]> 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/[email protected]/ > > > 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
