Sorry to warm up this old thread, but I think the implementation of env_save does not really work on error:

On my board, I have the environment stored in QSPI. If saving fails, env_save tries to continue to the next environment driver, but env_driver_lookup/env_get_location always returns the same driver for ENVOP_SAVE (gd->env_load_location).

The result is that if the env driver returns an error from its save function, env_save hangs in an infinite loop. Sorry I don't have a patch for this right now...

Simon

On 23.01.2018 21:16, Maxime Ripard wrote:

In preparation for the multiple environment support, let's introduce two new parameters to the environment driver lookup function: the priority and operation.

The operation parameter is meant to identify, obviously, the operation you might want to perform on the environment.

The priority is a number passed to identify the environment priority you want to retrieve. The lowest priority parameter (0) will be the primary source.

Combining the two parameters allow you to support multiple environments through different priorities, and to change those priorities between read and writes operations.

This is especially useful to implement migration mechanisms where you want to always use the same environment first, be it to read or write, while the common case is more likely to use the same environment it has read from to write it to.

Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

env/env.c | 157 +++++++++++++++++++++++++++++-------------- include/environment.h | 8 ++- 2 files changed, 116 insertions(+), 49 deletions(-)

diff --git a/env/env.c b/env/env.c index 97ada5b5a6fd..73da149fd8ca 100644 --- a/env/env.c +++ b/env/env.c @@ -26,8 +26,33 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) return NULL; }

-static enum env_location env_get_location(void) +/**

• • env_get_location() - Returns the best env location for a board
• • @op: operations performed on the environment
• • @prio: priority between the multiple environments, 0 being the

• •    highest priority
    

• • This will return the preferred environment for the given priority.
• • All implementations are free to use the operation, the priority and
• • any other data relevant to their choice, but must take into account
• • the fact that the lowest prority (0) is the most important location
• • in the system. The following locations should be returned by order
• • of descending priorities, from the highest to the lowest priority.
• • Returns:
• • an enum env_location value on success, a negative error code otherwise
• */

+static enum env_location env_get_location(enum env_operation op, int prio) {

• /*

• * We support a single environment, so any environment asked
  

• * with a priority that is not zero is out of our supported
  

• * bounds.
  

• */
  

• 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)

@@ -52,11 +77,27 @@ static enum env_location env_get_location(void) return ENVL_UNKNOWN; }

-static struct env_driver *env_driver_lookup(void)

+/**

• • env_driver_lookup() - Finds the most suited environment location
• • @op: operations performed on the environment
• • @prio: priority between the multiple environments, 0 being the

• •    highest priority
    

• • This will try to find the available environment with the highest
• • priority in the system.
• • Returns:
• • NULL on error, a pointer to a struct env_driver otherwise
• */

+static struct env_driver *env_driver_lookup(enum env_operation op, int prio) {

• enum env_location loc = env_get_location();

• enum env_location loc = env_get_location(op, prio); struct env_driver *drv;

• if (loc == ENVL_UNKNOWN)

• return NULL;
  

• drv = _env_driver_lookup(loc); if (!drv) { debug("%s: No environment driver for location %d\n", __func__,

@@ -69,83 +110,101 @@ static struct env_driver *env_driver_lookup(void)

int env_get_char(int index) {

• struct env_driver *drv = env_driver_lookup();
• int ret;

• struct env_driver *drv;

• int prio;

  if (gd->env_valid == ENV_INVALID) return default_environment[index];

• if (!drv)

• return -ENODEV;
  

• if (!drv->get_char)

• return *(uchar *)(gd->env_addr + index);
  

• ret = drv->get_char(index);
• if (ret < 0) {

• debug("%s: Environment failed to load (err=%d)\n",
  

•       __func__, ret);
  

• for (prio = 0; (drv = env_driver_lookup(ENVOP_GET_CHAR, prio)); prio++) {

• int ret;
  

• if (!drv->get_char)
  

• 	continue;
  

• ret = drv->get_char(index);
  

• if (!ret)
  

• 	return 0;
  

• debug("%s: Environment %s failed to load (err=%d)\n", __func__,
  

•       drv->name, ret);
  

  }

• return ret;

• return -ENODEV; }

  int env_load(void) {

• struct env_driver *drv = env_driver_lookup();
• int ret = 0;

• struct env_driver *drv;
• int prio;

• if (!drv)

• return -ENODEV;
  

• if (!drv->load)

• return 0;
  

• ret = drv->load();
• if (ret) {

• debug("%s: Environment failed to load (err=%d)\n", __func__,
  

•       ret);
  

• return ret;
  

• for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {

• int ret;
  

• if (!drv->load)
  

• 	continue;
  

• ret = drv->load();
  

• if (!ret)
  

• 	return 0;
  

• debug("%s: Environment %s failed to load (err=%d)\n", __func__,
  

•       drv->name, ret);
  

  }

• return 0;

• return -ENODEV; }

  int env_save(void) {

• struct env_driver *drv = env_driver_lookup();
• int ret;

• struct env_driver *drv;
• int prio;

• if (!drv)

• return -ENODEV;
  

• if (!drv->save)

• return -ENOSYS;
  

• printf("Saving Environment to %s...\n", drv->name);
• ret = drv->save();
• if (ret) {

• debug("%s: Environment failed to save (err=%d)\n", __func__,
  

•       ret);
  

• return ret;
  

• for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {

• int ret;
  

• if (!drv->save)
  

• 	continue;
  

• printf("Saving Environment to %s...\n", drv->name);
  

• ret = drv->save();
  

• if (!ret)
  

• 	return 0;
  

• debug("%s: Environment %s failed to save (err=%d)\n", __func__,
  

•       drv->name, ret);
  

  }

• return 0;

• return -ENODEV; }

  int env_init(void) {

• struct env_driver *drv = env_driver_lookup();

• struct env_driver *drv; int ret = -ENOENT;
• int prio;
• for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {

• if (!drv->init)
  

• 	continue;
  
  

• if (!drv)

• return -ENODEV;
  

• if (drv->init) ret = drv->init();

• if (!ret)
  

• 	return 0;
  

• debug("%s: Environment %s failed to init (err=%d)\n", __func__,
  

•       drv->name, ret);
  

• }

• if (!prio)

• return -ENODEV;
  

• if (ret == -ENOENT) { gd->env_addr = (ulong)&default_environment[0]; gd->env_valid = ENV_VALID;

  return 0;

• } else if (ret) {

• debug("%s: Environment failed to init (err=%d)\n", __func__,
  

•       ret);
  

• return ret;
  

  }

• return 0;

• return ret; }

diff --git a/include/environment.h b/include/environment.h index a2015c299aa9..a4060506fabb 100644 --- a/include/environment.h +++ b/include/environment.h @@ -205,6 +205,14 @@ enum env_location { ENVL_UNKNOWN, };

+/* value for the various operations we want to perform on the env */ +enum env_operation {

• ENVOP_GET_CHAR, /* we want to call the get_char function */
• ENVOP_INIT, /* we want to call the init function */
• ENVOP_LOAD, /* we want to call the load function */
• ENVOP_SAVE, /* we want to call the save function */

+};

• struct env_driver { const char *name; enum env_location location;

On 2018년 01월 24일 05:16, Maxime Ripard wrote:

Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.

Make MMC play a little nicer by removing all the extra \n and formatting that is redundant with the global output.

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

Reviewed-by: Jaehoon Chung jh80.chung@samsung.com

---

env/mmc.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/env/mmc.c b/env/mmc.c index ed7bcf16ae0e..528fbf978162 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -233,7 +233,6 @@ static int env_mmc_save(void) goto fini; }

• puts("done\n"); ret = 0;

#ifdef CONFIG_ENV_OFFSET_REDUND

On 23.01.2018 21:16, Maxime Ripard wrote:

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,

Does priority handling really work here? Most env drivers seem to ignore the return value of env_import and may thus return success although importing the environment failed (only reading the data from the device succeeded).

This is from reading the code, I haven't had a chance to test this, yet.

and we'll use the same one without lookup during writes. This should implement the same behaviour than we currently have.

Is there a way to save the environment to drivers other than the one selected at init time?

Regards, Simon

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

---

env/env.c | 80 +++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 30 deletions(-)

diff --git a/env/env.c b/env/env.c index 906f28ee50a1..1182fdb545db 100644 --- a/env/env.c +++ b/env/env.c @@ -26,6 +26,41 @@ 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 +};

+static enum env_location env_load_location = ENVL_UNKNOWN;

• /**
  • env_get_location() - Returns the best env location for a board
  • @op: operations performed on the environment

@@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) */ static enum env_location env_get_location(enum env_operation op, int prio) {

• /*

• * We support a single environment, so any environment asked
  

• * with a priority that is not zero is out of our supported
  

• * bounds.
  

• */
  

• 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 ENVOP_GET_CHAR:
• case ENVOP_INIT:
• case ENVOP_LOAD:

• if (prio >= ARRAY_SIZE(env_locations))
  

• 	return ENVL_UNKNOWN;
  

• env_load_location = env_locations[prio];
  

• return env_load_location;
  

• case ENVOP_SAVE:

• return env_load_location;
  

• }
• return ENVL_UNKNOWN; }

On 31.01.2018 00:02, York Sun wrote:

On 01/30/2018 12:16 PM, York Sun wrote:

...

On 01/30/2018 11:40 AM, York Sun wrote:

...

On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:

...

On 23.01.2018 21:16, Maxime Ripard wrote:

...

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,

Does priority handling really work here? Most env drivers seem to ignore the return value of env_import and may thus return success although importing the environment failed (only reading the data from the device succeeded).

This is from reading the code, I haven't had a chance to test this, yet.

It is broken on my LS1043ARDB with simply NOR flash. I am trying to determine what went wrong.

I found the problem. The variable "env_load_location" is static. It is probably not write-able during booting from flash. It is expected to be set during ENVOP_INIT. But if I print this variable, it has ENVL_UNKNOWN. I can make it work by moving env_load_location to global data structure.

That being said, this addition of multiple environments really slows down the booting process for me. I see every time env_get_char() is called, env_driver_lookup() runs. A simple call of env_get_f() gets slowed down dramatically. I didn't find out where the most time is spent yet.

Does anyone else experience this unbearable slowness?

Yes. Depending on CPU speed... :-)

On my board, that slowdown comes from the fact that env_get_f does not check the return value of env_get_char for an error. That leads to trying for CONFIG_ENV_SIZE times, which is of course slow. I'll post a fix for that.

I found the problem. In patch #3 in this series, the default get_char() was dropped so there is no driver for a plain NOR flash. A quick (and maybe dirty) fix is this

That patch #3 actually changed the behavior for all env drivers not providing .get_char (so all drivers except for eeprom and nvram) from returning what's behind gd->env_addr. Your patch below restores the old behaviour.

I can't tell what's the correct behaviour though: in my tests, env_get_f got called even after importing the environment, which is not what I would have expected...

diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret;

             if (!drv->get_char)

•                   continue;
  

•                   return *(uchar *)(gd->env_addr + index);
  
             if (!env_has_inited(drv->location))
                     continue;
  
  

With this temporary fix, my flash chip works again and I can boot all the way up in a timely manner. I still don't like to call env_driver_lookup() thousands of times to get a simple env variable.

That's not a thing Maxime has changed but a change that came in when adding environment drivers.

Simon

Can Maxime post a quick post soon?

York

On 02/07/2018 12:43 AM, Maxime Ripard wrote:

Hi,

On Tue, Jan 30, 2018 at 11:02:49PM +0000, York Sun wrote:

...

On 01/30/2018 12:16 PM, York Sun wrote:

...

On 01/30/2018 11:40 AM, York Sun wrote:

...

On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:

...

On 23.01.2018 21:16, Maxime Ripard wrote:

...

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,

Does priority handling really work here? Most env drivers seem to ignore the return value of env_import and may thus return success although importing the environment failed (only reading the data from the device succeeded).

This is from reading the code, I haven't had a chance to test this, yet.

It is broken on my LS1043ARDB with simply NOR flash. I am trying to determine what went wrong.

I found the problem. The variable "env_load_location" is static. It is probably not write-able during booting from flash. It is expected to be set during ENVOP_INIT. But if I print this variable, it has ENVL_UNKNOWN. I can make it work by moving env_load_location to global data structure.

That would work for me.

...

...

That being said, this addition of multiple environments really slows down the booting process for me. I see every time env_get_char() is called, env_driver_lookup() runs. A simple call of env_get_f() gets slowed down dramatically. I didn't find out where the most time is spent yet.

Does anyone else experience this unbearable slowness?

I found the problem. In patch #3 in this series, the default get_char() was dropped so there is no driver for a plain NOR flash. A quick (and maybe dirty) fix is this

diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret;

            if (!drv->get_char)

•                   continue;
  

•                   return *(uchar *)(gd->env_addr + index);
  
            if (!env_has_inited(drv->location))
                    continue;
  

And this too.

If you agree with this fix (actually revert your change earlier), I can send out a patch.

...

With this temporary fix, my flash chip works again and I can boot all the way up in a timely manner. I still don't like to call env_driver_lookup() thousands of times to get a simple env variable.

Can Maxime post a quick post soon?

Given that you already made all the debugging, and the patches, and I have no way to test, I guess it would make more sense if you did it :)

Yes, I have tested on my boards. I will send out this patch.

York

On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:

.....

...

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

---

env/env.c | 80 +++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 30 deletions(-)

diff --git a/env/env.c b/env/env.c index 906f28ee50a1..1182fdb545db 100644 --- a/env/env.c +++ b/env/env.c @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) return NULL; }

+static enum env_location env_locations[] = {

Don't use static/global variables. They cause a lot of relocation work/size and is less flexible. There is no way to #define ENVL_EEPROM to a function when a variable.

Jocke

...

+#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 +};

+static enum env_location env_load_location = ENVL_UNKNOWN;

• /**
  • env_get_location() - Returns the best env location for a board
  • @op: operations performed on the environment

@@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) */ static enum env_location env_get_location(enum env_operation op, int prio) {

• /*
  

•  * We support a single environment, so any environment asked
  

•  * with a priority that is not zero is out of our supported
  

•  * bounds.
  

•  */
  

• 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 ENVOP_GET_CHAR:
  

• case ENVOP_INIT:
  

• case ENVOP_LOAD:
  

•         if (prio >= ARRAY_SIZE(env_locations))
  

•                 return ENVL_UNKNOWN;
  

•         env_load_location = env_locations[prio];
  

•         return env_load_location;
  

• case ENVOP_SAVE:
  

•         return env_load_location;
  

• }
  

• return ENVL_UNKNOWN;
  

  }

---

U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

On Thu, 1970-01-01 at 00:00 +0000, Maxime Ripard wrote:

Hi,

On Tue, Feb 06, 2018 at 08:20:49AM +0000, Joakim Tjernlund wrote:

...

On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:

.....

...

...

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

---

env/env.c | 80 +++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 30 deletions(-)

diff --git a/env/env.c b/env/env.c index 906f28ee50a1..1182fdb545db 100644 --- a/env/env.c +++ b/env/env.c @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) return NULL; }

+static enum env_location env_locations[] = {

Don't use static/global variables. They cause a lot of relocation work/size and is less flexible. There is no way to #define ENVL_EEPROM to a function when a variable.

Is that last sentence truncated?

Can you elaborate a bit more on what is the source of the relocation issues you're mentionning? Is that because of the section it ends up in?

Mainly that its adds relocation entries that take up space, more space than doing a simple assignment directly in code.

On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On 06.02.2018 09:20, Joakim Tjernlund wrote:

...

On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:

.....

...

...

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

---

env/env.c | 80 +++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 30 deletions(-)

diff --git a/env/env.c b/env/env.c index 906f28ee50a1..1182fdb545db 100644 --- a/env/env.c +++ b/env/env.c @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) return NULL; }

+static enum env_location env_locations[] = {

Don't use static/global variables. They cause a lot of relocation work/size and is less flexible.

In this specific case, I think this array should be const anyway, would that prevent the relocation problems you see?

...

There is no way to #define ENVL_EEPROM to a function when a variable.

ENVL_EEPROM is an enum value, why would you define it to a function?

I got boards that very similar but differ in where/how env. is stored, like different flash so I need to be able to select at runtime how get my env., I haven't looked if this particular area is affected but ideally I would like if all env. related "constants" could be impl. with a function instead.

Also, using static/global vars takes more space than simple assignments in code, ideally everything needed early (before reloc/ in SPL) should avoid relocations to save space.

Jocke

On 2018년 01월 24일 05:17, Maxime Ripard wrote:

The raw MMC environment directly calls into the MMC framework. Make sure it's enabled before we can select it.

Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

Reviewed-by: Jaehoon Chung jh80.chung@samsung.com

---

env/Kconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/env/Kconfig b/env/Kconfig index bef6e89bfc3c..ad5ccc253762 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -156,6 +156,7 @@ config ENV_IS_IN_FLASH config ENV_IS_IN_MMC bool "Environment in an MMC device" depends on !CHAIN_OF_TRUST

• depends on MMC help Define this if you have an MMC device which you want to use for the environment.

On 23.01.2018 21:17, Maxime Ripard wrote:

Now that we have everything in place in the code, let's allow to build multiple environments backend through Kconfig.

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

I get a build error when enabling CONFIG_ENV_IS_IN_SPI_FLASH and CONFIG_ENV_IS_IN_MMC at the same time.

The build error is in host tools, not in U-Boot or SPL itself. In fact, this is not specific to CONFIG_ENV_IS_IN_SPI_FLASH but to the combination of CONFIG_ENV_IS_IN_MMC and any of the environments marked as ENVCRC- in tools/Makefile.

The actual error is that the compiler does not know standard types in efi.h and mmc.h, e.g.: In file included from include/blk.h:11:0,                  from include/part.h:10,                  from include/mmc.h:16,                  from include/environment.h:168,                  from ./tools/../env/embedded.c:16,                  from tools/env/embedded.c:2: include/efi.h:32:2: error: unknown type name ‘u8’   u8 b[16];   ^~

I can't think of a correct fix right now...

Simon

---

configs/MPC8313ERDB_NAND_33_defconfig | 1 +- configs/MPC8313ERDB_NAND_66_defconfig | 1 +- configs/cl-som-imx7_defconfig | 1 +- configs/microblaze-generic_defconfig | 1 +- env/Kconfig | 65 +++++++++++++--------------- 5 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/configs/MPC8313ERDB_NAND_33_defconfig b/configs/MPC8313ERDB_NAND_33_defconfig index 823001583447..b761516d126a 100644 --- a/configs/MPC8313ERDB_NAND_33_defconfig +++ b/configs/MPC8313ERDB_NAND_33_defconfig @@ -22,6 +22,7 @@ CONFIG_CMD_DATE=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDIDS_DEFAULT="nand0=e2800000.flash" CONFIG_MTDPARTS_DEFAULT="mtdparts=e2800000.flash:512k(uboot),128k(env),6m@1m(kernel),-(fs)" +# CONFIG_ENV_IS_IN_FLASH is not set CONFIG_ENV_IS_IN_NAND=y # CONFIG_MMC is not set CONFIG_MTD_NOR_FLASH=y diff --git a/configs/MPC8313ERDB_NAND_66_defconfig b/configs/MPC8313ERDB_NAND_66_defconfig index 2639926ab814..0f2a675ae2cf 100644 --- a/configs/MPC8313ERDB_NAND_66_defconfig +++ b/configs/MPC8313ERDB_NAND_66_defconfig @@ -22,6 +22,7 @@ CONFIG_CMD_DATE=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDIDS_DEFAULT="nand0=e2800000.flash" CONFIG_MTDPARTS_DEFAULT="mtdparts=e2800000.flash:512k(uboot),128k(env),6m@1m(kernel),-(fs)" +# CONFIG_ENV_IS_IN_FLASH is not set CONFIG_ENV_IS_IN_NAND=y # CONFIG_MMC is not set CONFIG_MTD_NOR_FLASH=y diff --git a/configs/cl-som-imx7_defconfig b/configs/cl-som-imx7_defconfig index d37c82cafac1..0c93159032e5 100644 --- a/configs/cl-som-imx7_defconfig +++ b/configs/cl-som-imx7_defconfig @@ -41,6 +41,7 @@ CONFIG_CMD_EXT4=y CONFIG_CMD_EXT4_WRITE=y CONFIG_CMD_FAT=y CONFIG_CMD_FS_GENERIC=y +# CONFIG_ENV_IS_IN_MMC is not set CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SPI_FLASH=y CONFIG_SPI_FLASH_STMICRO=y diff --git a/configs/microblaze-generic_defconfig b/configs/microblaze-generic_defconfig index 5254c0da790a..cc80e8a027c8 100644 --- a/configs/microblaze-generic_defconfig +++ b/configs/microblaze-generic_defconfig @@ -40,7 +40,6 @@ CONFIG_CMD_UBI=y # CONFIG_CMD_UBIFS is not set CONFIG_SPL_OF_CONTROL=y CONFIG_OF_EMBED=y -CONFIG_ENV_IS_IN_FLASH=y CONFIG_NETCONSOLE=y CONFIG_SPL_DM=y CONFIG_MTD_NOR_FLASH=y diff --git a/env/Kconfig b/env/Kconfig index ad5ccc253762..6e2fbf416c12 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -1,38 +1,18 @@ menu "Environment"

-choice

• prompt "Select the location of the environment"
• default ENV_IS_IN_MMC if ARCH_SUNXI
• default ENV_IS_IN_MMC if ARCH_EXYNOS4
• default ENV_IS_IN_MMC if MX6SX || MX7D
• default ENV_IS_IN_MMC if TEGRA30 || TEGRA124
• default ENV_IS_IN_MMC if TEGRA_ARMV8_COMMON
• default ENV_IS_IN_FLASH if ARCH_CINTEGRATOR
• default ENV_IS_IN_FLASH if ARCH_INTEGRATOR_CP
• default ENV_IS_IN_FLASH if M548x || M547x || M5282 || MCF547x_8x
• default ENV_IS_IN_FLASH if MCF532x || MCF52x2
• default ENV_IS_IN_FLASH if MPC86xx || MPC83xx
• default ENV_IS_IN_FLASH if ARCH_MPC8572 || ARCH_MPC8548 || ARCH_MPC8641
• default ENV_IS_IN_FLASH if SH && !CPU_SH4
• default ENV_IS_IN_SPI_FLASH if ARMADA_XP
• default ENV_IS_IN_SPI_FLASH if INTEL_BAYTRAIL
• default ENV_IS_IN_SPI_FLASH if INTEL_BRASWELL
• default ENV_IS_IN_SPI_FLASH if INTEL_BROADWELL
• default ENV_IS_IN_SPI_FLASH if NORTHBRIDGE_INTEL_IVYBRIDGE
• default ENV_IS_IN_SPI_FLASH if INTEL_QUARK
• default ENV_IS_IN_SPI_FLASH if INTEL_QUEENSBAY
• default ENV_IS_IN_FAT if ARCH_BCM283X
• default ENV_IS_IN_FAT if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS
• default ENV_IS_NOWHERE
• help

•  At present the environment can be stored in only one place. Use this
  

•  option to select the location. This is either a device (where the
  

•  environemnt information is simply written to a fixed location or
  

•  partition on the device) or a filesystem (where the environment
  

•  information is written to a file).
  

• config ENV_IS_NOWHERE bool "Environment is not stored"

• depends on !ENV_IS_IN_EEPROM
• depends on !ENV_IS_IN_FAT
• depends on !ENV_IS_IN_FLASH
• depends on !ENV_IS_IN_MMC
• depends on !ENV_IS_IN_NAND
• depends on !ENV_IS_IN_NVRAM
• depends on !ENV_IS_IN_ONENAND
• depends on !ENV_IS_IN_REMOTE
• depends on !ENV_IS_IN_SPI_FLASH
• depends on !ENV_IS_IN_UBI
• default y help Define this if you don't want to or can't have an environment stored on a storage medium. In this case the environemnt will still exist

@@ -74,6 +54,8 @@ config ENV_IS_IN_EEPROM config ENV_IS_IN_FAT bool "Environment is in a FAT filesystem" depends on !CHAIN_OF_TRUST

• default y if ARCH_BCM283X
• default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS select FAT_WRITE help Define this if you want to use the FAT file system for the environment.

@@ -84,6 +66,13 @@ config ENV_IS_IN_FAT config ENV_IS_IN_FLASH bool "Environment in flash memory" depends on !CHAIN_OF_TRUST

• default y if ARCH_CINTEGRATOR
• default y if ARCH_INTEGRATOR_CP
• default y if M548x || M547x || M5282 || MCF547x_8x
• default y if MCF532x || MCF52x2
• default y if MPC86xx || MPC83xx
• default y if ARCH_MPC8572 || ARCH_MPC8548 || ARCH_MPC8641
• default y if SH && !CPU_SH4 help Define this if you have a flash device which you want to use for the environment.

@@ -157,6 +146,11 @@ config ENV_IS_IN_MMC bool "Environment in an MMC device" depends on !CHAIN_OF_TRUST depends on MMC

• default y if ARCH_SUNXI
• default y if ARCH_EXYNOS4
• default y if MX6SX || MX7D
• default y if TEGRA30 || TEGRA124
• default y if TEGRA_ARMV8_COMMON help Define this if you have an MMC device which you want to use for the environment.

@@ -294,6 +288,13 @@ config ENV_IS_IN_REMOTE config ENV_IS_IN_SPI_FLASH bool "Environment is in SPI flash" depends on !CHAIN_OF_TRUST

• default y if ARMADA_XP
• default y if INTEL_BAYTRAIL
• default y if INTEL_BRASWELL
• default y if INTEL_BROADWELL
• default y if NORTHBRIDGE_INTEL_IVYBRIDGE
• default y if INTEL_QUARK
• default y if INTEL_QUEENSBAY help Define this if you have a SPI Flash memory device which you want to use for the environment.

@@ -359,8 +360,6 @@ config ENV_IS_IN_UBI You will probably want to define these to avoid a really noisy system when storing the env in UBI.

-endchoice

• config ENV_FAT_INTERFACE string "Name of the block device for the environment" depends on ENV_IS_IN_FAT

On 23.01.2018 21:17, Maxime Ripard wrote:

Allow boards and architectures to override the default environment lookup code by overriding env_get_location.

Sorry for missing that conversation a bit, but is it really enough to override env_get_location? Overriding this function should make the env_locations array unused, but env_set_inited still references it.

Regards, Simon

Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

---

env/env.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c index 2ea581cbedd6..9564522e76a5 100644 --- a/env/env.c +++ b/env/env.c @@ -85,6 +85,7 @@ static void env_set_inited(enum env_location location)

•    highest priority
  

• This will return the preferred environment for the given priority.

• • This is overridable by boards if they need to.
  • All implementations are free to use the operation, the priority and
  • any other data relevant to their choice, but must take into account

@@ -95,7 +96,7 @@ static void env_set_inited(enum env_location location)

• Returns:
• an enum env_location value on success, a negative error code otherwise

*/ -static enum env_location env_get_location(enum env_operation op, int prio) +__weak enum env_location env_get_location(enum env_operation op, int prio) { switch (op) { case ENVOP_GET_CHAR: