[U-Boot] [RFC PATCH 06/10] env: Support multiple environments

Fri Nov 17 17:07:15 UTC 2017

On Fri, Nov 17, 2017 at 03:40:51PM +0100, Lukasz Majewski wrote:
> Hi Maxime,
> 
> > Hi Lukasz, Tom,
> > 
> > On Fri, Nov 17, 2017 at 03:00:15PM +0100, Lukasz Majewski wrote:
> > > On Fri, 17 Nov 2017 08:41:05 -0500
> > > Tom Rini <trini at konsulko.com> wrote:
> > >   
> > > > On Fri, Nov 17, 2017 at 10:24:37AM +0100, Lukasz Majewski wrote:  
> > > > > Hi Maxime,
> > > > >     
> > > > > > Now that we have everything in place to support multiple
> > > > > > environment, let's make sure the current code can use it.
> > > > > > 
> > > > > > The priority used between the various environment is the same
> > > > > > one that was used in the code previously.
> > > > > > 
> > > > > > At read / init times, the highest priority environment is
> > > > > > going to be detected, and we'll use the same one without
> > > > > > lookup during writes. This should implement the same
> > > > > > behaviour than we currently have.
> > > > > > 
> > > > > > Signed-off-by: Maxime Ripard
> > > > > > <maxime.ripard at free-electrons.com> ---
> > > > > >  env/env.c | 75
> > > > > > ++++++++++++++++++++++++++++++++++++++++++---------------------
> > > > > > 1 file changed, 50 insertions(+), 25 deletions(-)
> > > > > > 
> > > > > > diff --git a/env/env.c b/env/env.c
> > > > > > index 1d13220aa79b..6af9f897b0ae 100644
> > > > > > --- a/env/env.c
> > > > > > +++ b/env/env.c
> > > > > > @@ -26,33 +26,58 @@ static struct env_driver
> > > > > > *_env_driver_lookup(enum env_location loc) return NULL;
> > > > > >  }
> > > > > >  
> > > > > > +static enum env_location env_locations[] = {
> > > > > > +#ifdef CONFIG_ENV_IS_IN_EEPROM
> > > > > > +	ENVL_EEPROM,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_FAT
> > > > > > +	ENVL_FAT,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_FLASH
> > > > > > +	ENVL_FLASH,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_MMC
> > > > > > +	ENVL_MMC,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_NAND
> > > > > > +	ENVL_NAND,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_NVRAM
> > > > > > +	ENVL_NVRAM,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_REMOTE
> > > > > > +	ENVL_REMOTE,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > > > > > +	ENVL_SPI_FLASH,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_IN_UBI
> > > > > > +	ENVL_UBI,
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_ENV_IS_NOWHERE
> > > > > > +	ENVL_NOWHERE,
> > > > > > +#endif
> > > > > > +	ENVL_UNKNOWN,
> > > > > > +};
> > > > > > +
> > > > > > +static enum env_location env_load_location;
> > > > > > +
> > > > > >  static enum env_location env_get_location(enum env_operation
> > > > > > op, int prio) {
> > > > > > -	if (prio >= 1)
> > > > > > -		return ENVL_UNKNOWN;
> > > > > > -
> > > > > > -	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> > > > > > -		return ENVL_EEPROM;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> > > > > > -		return ENVL_FAT;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> > > > > > -		return ENVL_FLASH;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> > > > > > -		return ENVL_MMC;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> > > > > > -		return ENVL_NAND;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> > > > > > -		return ENVL_NVRAM;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> > > > > > -		return ENVL_REMOTE;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> > > > > > -		return ENVL_SPI_FLASH;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> > > > > > -		return ENVL_UBI;
> > > > > > -	else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> > > > > > -		return ENVL_NOWHERE;
> > > > > > -	else
> > > > > > -		return ENVL_UNKNOWN;
> > > > > > +	switch (op) {
> > > > > > +	case ENVO_GET_CHAR:
> > > > > > +	case ENVO_INIT:
> > > > > > +	case ENVO_LOAD:
> > > > > > +		if (prio >= ARRAY_SIZE(env_locations))
> > > > > > +			return -ENODEV;
> > > > > > +
> > > > > > +		return env_load_location =
> > > > > > env_locations[prio]; +
> > > > > > +	case ENVO_SAVE:
> > > > > > +		return env_load_location;
> > > > > > +	}
> > > > > > +
> > > > > > +	return ENVL_UNKNOWN;
> > > > > >  }
> > > > > >  
> > > > > >  static struct env_driver *env_driver_lookup(enum
> > > > > > env_operation op, int prio)    
> > > > > 
> > > > > I'm just wondering...
> > > > > 
> > > > > Do you think that it may be helpful to have a way to force
> > > > > particular environment to be used (despite of the priority)?    
> > > > 
> > > > That's one of the hard parts of this, yes.  I think I had
> > > > suggested before that we handle that in a follow up series?
> > > >   
> > > 
> > > I don't want to say that this must be done now. I just can imagine
> > > that we may need such feature in some point.  
> > 
> > So I guess this question is also part of the comment Lukasz had on the
> > other patch.
> > 
> > The current implementation is effectively hardcoding the environment
> > by doing two things:
> >   - Rejecting any priority other than 0, which is the highest. It
> >     basically says to the code that there is no further alternative
> >     and that its loop should stop there.
> >   - Returning the first environment in the array.
> > 
> > If one wanted to hardcode the environment to something else than the
> > default behaviour, you can always implement env_get_location in your
> > board by doing something like:
> > 
> > enum env_location env_get_location (enum env_operation op, int prio)
> > {
> > 	if (prio)
> > 		return ENVL_UNKNOWN;
> > 
> > 	return ENVL_MMC; /* Or whatever you want */
> > }
> 
> The env_get_location defined per board would work for me.
> 
> > 
> > Like I was saying, the default implementation already kind of does
> > that. Or maybe we want the default one to not hardcode it and just
> > deal with alternatives?
> 
> I think that having the code hardcoded (as it is now) is acceptable for
> making the feature way to main line. I suppose that
> you would add the priorities handling in further patches?

Just adding the same priorities than right now is quite easy. What I'm
a bit afraid is how exactly that priority handling could be done in a
generic way, because I guess everyone is going to want something a bit
different.

And if we just decide to put it in the boards, then it's already
supported.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/ca2bb1e6/attachment.sig>