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