Broken commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2

[Pali Rohár]

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.