Re: Broken commit de47ff536363289f92f85ed1e4901724d238432d

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3599 bytes --]

On Wed, Dec 28, 2022 at 05:50:43PM +0100, 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
> 
> 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)

Yes, I'll look at this again. My first observation is that the
exhaustive list of incorrectly migrated platforms listed there is ALSO
the list of platforms that had SDCARD/SPIFLASH enabled, when they
shouldn't have and now have NO_PBL set, So, that being set wrong meant
that the part here was wrong.  I think the answer might be to just fix
those configs, and then also add a "depends on
!MPC85XX_HAVE_RESET_VECTOR" to the *SYS_MPC85XX_NO_RESETVEC options.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]