Broken commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2

Thu Dec 22 15:29:27 CET 2022

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
-------------- 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/f4bc6660/attachment.sig>