On Thu, Dec 22, 2022 at 06:13:06PM +0100, Pali Rohár wrote:
On Thursday 22 December 2022 09:29:27 Tom Rini wrote:
On Thu, Dec 22, 2022 at 08:49:47AM +0100, Pali Rohár wrote:
On Wednesday 21 December 2022 21:54:15 Tom Rini wrote:
On Fri, Dec 16, 2022 at 10:56:39PM +0100, Pali Rohár wrote:
On Friday 05 August 2022 16:21:24 Pali Rohár wrote:
Broken is also commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2. Seems that all kconfig migration changes done after that commit are broken.
I really do not have energy to investigate what and how was broken due to incorrect kconfig migration.
I did simple test. Applied following change:
diff --git a/include/configs/p1_p2_rdb_pc.h b/include/configs/p1_p2_rdb_pc.h index a6523753d5ca..489f24df0ab1 100644 --- a/include/configs/p1_p2_rdb_pc.h +++ b/include/configs/p1_p2_rdb_pc.h @@ -624,3 +624,7 @@ __stringify(__PCIE_RST_CMD)"\0" "bootm $norbootaddr - $norfdtaddr"
#endif /* __CONFIG_H */
+#ifdef CONFIG_SDCARD +#error +#endif
And then called:
make CROSS_COMPILE=powerpc-linux-gnuspe- P2020RDB-PC_defconfig u-boot.bin
And it failed, even when this defconfig file is not SD card builds.
Tom, that commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2 is yours. Could you please look at it? Because it is a regressions which made P1 and P2 broken. Based on the past experience it really does not make sense to wait for somebody who promised to do something as same situation is just repeating.
Above diff is a simple check to verify if code conversion is correct or not. If _before_ conversion CONFIG_SDCARD was not defined then also _after_ conversion this macro must not be defined. Right?
I dug through all of this again. I thought I understood what the right answer was again for a moment, but I don't. You, however, understand what platforms don't use PBL and what they use instead. I understand half of the fix, which is to change: choice prompt "Freescale PBL load location" depends on RAMBOOT_PBL || ((TARGET_P1010RDB_PA || TARGET_P1010RDB_PB \ || TARGET_P1020RDB_PC || TARGET_P1020RDB_PD || TARGET_P2020RDB) \ && !CMD_NAND)
To, I think: choice prompt "Freescale PBL load location" depends on RAMBOOT_PBL
Then introduce some new, not "SDCARD" symbol, for the P1/P2 platforms that don't use PBL but instead the FSL_PREPBL_ESDHC_BOOT_SECTOR logic you introduced before.
P2020RDB-PC_defconfig is for booting from FLASH NOR, not from SD card. CONFIG_SDCARD for P1/P2 must be defined when booting from SD card.
Before commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2 everything worked fine and CONFIG_SDCARD was not defined for P2020RDB-PC_defconfig. After commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2, symbol CONFIG_SDCARD is defined.
You can check it by adding into config.h:
+#ifdef CONFIG_SDCARD +#error +#endif
I say I almost thought I had it because I thought this would work: diff --git a/arch/powerpc/cpu/mpc85xx/Kconfig b/arch/powerpc/cpu/mpc85xx/Kconfig index 24d3f1f20c25..59740b173b11 100644 --- a/arch/powerpc/cpu/mpc85xx/Kconfig +++ b/arch/powerpc/cpu/mpc85xx/Kconfig @@ -15,7 +15,7 @@ config CMD_ERRATA config FSL_PREPBL_ESDHC_BOOT_SECTOR bool "Generate QorIQ pre-PBL eSDHC boot sector" depends on MPC85xx
- depends on SDCARD
- depends on TARGET_P1020RDB_PC || TARGET_P1020RDB_PD || TARGET_P2020RDB
No, original code was OK. As is written in description FSL_PREPBL_ESDHC_BOOT_SECTOR is for writing bootsector to SD card. And it is optional as there is other way how to generate it, as described in some doc/ file.
But if you choose to compile u-boot for P2020RDB-PC_defconfig (NOR FLASH) then there is no SD card booting and hence FSL_PREPBL_ESDHC_BOOT_SECTOR for P2020RDB-PC_defconfig must never be generated. So FSL_PREPBL_ESDHC_BOOT_SECTOR must depends on SDCARD.
help With this option final image would have prepended QorIQ pre-PBL eSDHC boot sector suitable for SD card images. This boot sector instruct diff --git a/boot/Kconfig b/boot/Kconfig index 4a001bcee851..d1c9c5f25067 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -725,8 +725,7 @@ config RAMBOOT_PBL
choice prompt "Freescale PBL load location"
- depends on RAMBOOT_PBL || ((TARGET_P1010RDB_PA || TARGET_P1010RDB_PB \
|| TARGET_P1020RDB_PC || TARGET_P1020RDB_PD || TARGET_P2020RDB) \
- depends on RAMBOOT_PBL || ((TARGET_P1010RDB_PA || TARGET_P1010RDB_PB) \ && !CMD_NAND)
config SDCARD
But no one enables CONFIG_FSL_PREPBL_ESDHC_BOOT_SECTOR.
It is optional _user_ symbol. During compilation of sd card version of u-boot, user can enable it.
For turris 1.x board there is waiting patch on the list which uses it. No review yet?
But maybe that just needs to be enabled on the platforms in question, and then the first dependency change above is just dropping the SDCARD line? I really don't know, and I'd be equally happy to just remove all of the P1*/P2* boards if they don't boot and no one cares to fix them.
-- Tom
Just fix the conversion process. The rule is simple: if config.h did not have enabled CONFIG_SDCARD then after conversion config.h must not have it enabled.
Compare git checkout d433c74eecdce1e4952ef4e8c712a9289c0dfcc2~1 and git checkout d433c74eecdce1e4952ef4e8c712a9289c0dfcc2
Because it worked fine before commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2.
Thanks for explaining. Since you clearly understand what it should do, and I do not, please submit something to implement the correct questions in Kconfig. There was some overloading of a symbol before which I didn't understand, and still don't exactly. Otherwise, since I believe you're saying the Turris platform is fine, once Marek merges it (you will likely need to rebase on next, next week, once I finish merging all of the CONFIG migration stuff), I'll just delete the P1/P2 platforms sometime in the next release if no one actually cares to fix them.
-- Tom
In lot of projects it is common that developer who introduced broken commit, is responsible to either fix it or revert it. I really do not have a time and power to fix every one broken commit behind every one developer. I sent lot of other fixes and in this case I at least wrote what is broken. Moreover it my was not my decision to use Kconfig for this stuff, which is unsuitable tool here. So do not take me wrong but I do not have motivation to fight with another tool for things for which it was not designed and try to hunt bugs in it.
It should have been pretty simple migration from tool A to tool B as it does not introduce any new feature nor code change. It should produce same u-boot.bin binary before and after change. And also it is not needed to fully understand what which option means. And if binaries are not same then conversion was wrong. No need for HW, just compiling and unit testing.
I have rebased turris 1.x patches at least 3 times. This is not enough?? Why should I rebase it again? Note that it depends on P1/P2 because turris 1.x is based on P2.
Also I have sent tons of patches and fixes for P1/P2 platforms and the only thing which you are going to do is to delete this platform?
You should have wrote this statement year ago and I would never take care of P1/P2 and fixing it. This is absolutely rude decision to show active developers: go away, we do not care about you, your time and your contributions.
Had I realized 10 years ago that Freescale was overloading CONFIG_SDCARD they way they were, I'd have rejected the changes then. So yes, that's on me. But this also wasn't a simple migration, or things wouldn't be broken right now. That's part of the problem. And I can't functionally test things. Which is why I've repeatedly asked if you can pick up and "own" these parts of the system which you understand and can test, and I cannot. At the time I likely figured the size change on a few platforms was one of the cases where the migration fixed an inconsistency, rather than breakage, as that has also happened as these changes go in.
I'm sorry you're frustrated here. I'm also frustrated here because the #ifdef games that PowerPC used, in a number of places have been very hard to un-wrap so that we can have something other than a home-grown build system. And I've tried to take your other feedback in to consideration, which has resulted in a large number of symbols being moved to CFG_... instead of Kconfig, as you're right, it wasn't the right mechanism for them.
So, is it really just the 3 platforms that use p1_p2_rdb.h that need neither SDCARD nor SPIFLASH for the NAND and no extra suffix defconfigs? Or is it the P1010RDB ones too?