Broken commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2
Tom Rini
trini at konsulko.com
Thu Dec 22 18:33:32 CET 2022
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?
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20221222/ebebaf3d/attachment.sig>
More information about the U-Boot
mailing list