Re: [PATCH 4/7] sunxi: use boot source for determining environment location

[Andre Przywara <andre.przywara@arm.com>]

On Fri, 15 Apr 2022 12:28:22 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

Hi Chris,

> On Tue, Jan 11, 2022 at 12:46:04PM +0000, Andre Przywara wrote:
> > Currently we only support to load the environment from raw MMC or FAT
> > locations on Allwinner boards. With the advent of SPI flash we probably
> > also want to support using the environment there, so we need to become
> > a bit more flexible.
> > 
> > Change the environment priority function to take the boot source into
> > account. When booted from eMMC or SD card, we use FAT or MMC, if
> > configured, as before.
> > If we are booted from SPI flash, we try to use the environment from
> > there, if possible. The same is true for NAND flash booting, although
> > this is somewhat theoretical right now (as untested).
> > 
> > This way we can use the same image for SD and SPI flash booting, which
> > allows us to simply copy a booted image from SD card to the SPI flash,
> > for instance.  
> 
> Unfortunately after a git-bisect I can confirm this patch breaks
> booting in FEL mode whenever the environment is set to
> CONFIG_ENV_IS_NOWHERE. Tested and reproducible on an NTC CHIP or a
> Source Parts Popcorn (both Allwinner R8 chips).
> 
> The curious thing though is that I could never see env_get_location
> called, but whenever I either set the environment to something like
> FAT or comment out this change (which I believe causes the non-board
> specific version of env_get_location to be used) it would boot again.
> So while I can confirm this patch breaks booting FEL when the env is
> set to nowhere, I was not able to figure out why that is.

Alright, thanks for the report. I see the problem: The default
fallback for the new env_get_location() is ENVL_FAT, as 157 out of the
160 Allwinner boards define CONFIG_ENV_IS_IN_FAT.
However the C.H.I.P. boards are one of the three exceptions, and
returning FAT when the FAT environment driver is not compiled in will
not end well. I don't have any CHIP board, but simulated this by
configuring a normal board with just defining CONFIG_ENV_IS_NOWHERE.

I made a patch that tries to choose a matching fallback. It's not very
elegant, but seems to fix the problem for me. Please test this once
posted.

Cheers,
Andre

> 
> Thank you.
> 
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  board/sunxi/board.c | 51 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 43 insertions(+), 8 deletions(-)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 2790a0f9e8..2472343d00 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -170,21 +170,56 @@ void i2c_init_board(void)
> >  #endif
> >  }
> >  
> > -#if defined(CONFIG_ENV_IS_IN_MMC) && defined(CONFIG_ENV_IS_IN_FAT)
> > +/*
> > + * Try to use the environment from the boot source first.
> > + * For MMC, this means a FAT partition on the boot device (SD or eMMC).
> > + * If the raw MMC environment is also enabled, this is tried next.
> > + * SPI flash falls back to FAT (on SD card).
> > + */
> >  enum env_location env_get_location(enum env_operation op, int prio)
> >  {
> > -	switch (prio) {
> > -	case 0:
> > -		return ENVL_FAT;
> > +	enum env_location boot_loc = ENVL_FAT;
> >  
> > -	case 1:
> > -		return ENVL_MMC;
> > +	gd->env_load_prio = prio;
> >  
> > +	switch (sunxi_get_boot_device()) {
> > +	case BOOT_DEVICE_MMC1:
> > +	case BOOT_DEVICE_MMC2:
> > +		boot_loc = ENVL_FAT;
> > +		break;
> > +	case BOOT_DEVICE_NAND:
> > +		if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND))
> > +			boot_loc = ENVL_NAND;
> > +		break;
> > +	case BOOT_DEVICE_SPI:
> > +		if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH))
> > +			boot_loc = ENVL_SPI_FLASH;
> > +		break;
> > +	case BOOT_DEVICE_BOARD:
> > +		break;
> >  	default:
> > -		return ENVL_UNKNOWN;
> > +		break;
> > +	}
> > +
> > +	/* Always try to access the environment on the boot device first. */
> > +	if (prio == 0)
> > +		return boot_loc;
> > +
> > +	if (prio == 1) {
> > +		switch (boot_loc) {
> > +		case ENVL_SPI_FLASH:
> > +			return ENVL_FAT;
> > +		case ENVL_FAT:
> > +			if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC))
> > +				return ENVL_MMC;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> >  	}
> > +
> > +	return ENVL_UNKNOWN;
> >  }
> > -#endif
> >  
> >  #ifdef CONFIG_DM_MMC
> >  static void mmc_pinmux_setup(int sdc);