| Message ID | 20171116092231.27740-7-maxime.ripard@free-electrons.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: Multiple env support and env transition for sunxi | expand |
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@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)? Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
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@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?
On Fri, 17 Nov 2017 08:41:05 -0500 Tom Rini <trini@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@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. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
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@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@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 */ } 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? Thanks! Maxime
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@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@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? > > Thanks! > Maxime > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
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@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@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
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)
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@free-electrons.com> --- env/env.c | 75 ++++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 25 deletions(-)