[U-Boot] [PATCH v3 00/15] env: Multiple env support and env transition for sunxi

[U-Boot] [PATCH v3 00/15] env: Multiple env support and env transition for sunxi

[Maxime Ripard]

Hi,

Here is a second attempt at transitioning away from the MMC raw environment
to a FAT-based one for Allwinner SoCs. Since the RFC was quite well
received, I actually tested it and fixed a few rough edges.

You'll find the first RFC here for reference:
https://lists.denx.de/pipermail/u-boot/2017-October/310111.html

And the second that originated in this series:
https://lists.denx.de/pipermail/u-boot/2017-November/311608.html

I gave it a travis run, and it passes all tests:
https://travis-ci.org/mripard/u-boot

Thanks!
Maxime

Original description of the patches:

The fundamental issue I'm trying to adress is that we've had for a
very long time the assumption that the main U-Boot binary wouldn't
exceed around 500 bytes.

However, we're starting to get real close to that limit, and are
running out of silver bullets to deal with the consequences of having
a bigger U-Boot binary, the main consequence being that we would
have some overlap between the environment and U-Boot.

One way to address this that has been suggested by Tom is to move away
from the raw MMC environment to a FAT-based one. This would allow us
to slowly migrate away, and eventually remove the MMC-raw option
entirely to reclaim that space for the binary.

That cannot be done in a single release however, since we might have
environments in the wild already that people rely on. And since we
always encouraged people to use the raw MMC environment, noone would
expect that.

This is even worse since some platforms are using the U-Boot
environment to deal with implement their upgrade mechanism, such as
mender.io, and force moving the environment would break any further
upgrade.

The suggested implementation is to allow U-Boot to compile multiple
environments backend at once, based on the work done by Simon. The
default behaviour shouldn't change obviously. We can then piggy-back
on this to tweak on a per-board basis the environment lookup algorithm
to always favour the FAT-based environment and then fallback to the
MMC. It will allow us to migrate a raw-MMC user to a FAT based
solution as soon as they update their environment (assuming that there
is a bootable FAT partition in the system).

This has just been compile tested on sunxi so far, and I'd like
general comments on the approach taken. Obviously, this will need to
work properly before being merged.

Changes from v2:
  - Fix a bug showing up on travis that resulted in the vexpress_ca_15 DHCP
    unit test failing. This was due to an improper return of ENVL_UNKNOWN.
  - Added Simon reviewed-by tag

Changes from v1:
  - Collect tags
  - Rebased on v2018.01
  - Fixed build failures on a couple of boards
  - Added back the message and the error code when an environment is
    failing
  - Added some comments about the printf in environments
  - Added a build time check about the number of our locations array to see
    if we're overflowing the location variable
  - Fixed the drv->init return code being ignored
  - Added helpers to manage the init status
  - Changed the ENVO prefix for the operations to ENVOP
  - Added some comments and documentation

Changes from the RFC:
  - Added more useful messages to see where we're loading / saving
  - Init all the environments no matter what, and the deal with whatever
    env we want to pick at load time
  - Added the various tags collected

Maxime Ripard (15):
  cmd: nvedit: Get rid of the env lookup
  env: Rename env_driver_lookup_default and env_get_default_location
  env: Pass additional parameters to the env lookup function
  env: Make the env save message a bit more explicit
  env: Make it explicit where we're loading our environment from
  env: fat: Make the debug messages play a little nicer
  env: mmc: Make the debug messages play a little nicer
  env: common: Make the debug messages play a little nicer
  env: Support multiple environments
  env: Initialise all the environments
  env: mmc: depends on the MMC framework
  env: Allow to build multiple environments in Kconfig
  env: Mark env_get_location as weak
  sunxi: Transition from the MMC to a FAT-based environment
  env: sunxi: Enable FAT-based environment support by default

 board/sunxi/board.c                   |  16 ++-
 cmd/nvedit.c                          |   4 +-
 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                           |  70 ++++----
 env/common.c                          |   2 +-
 env/env.c                             | 251 +++++++++++++++++++--------
 env/fat.c                             |  25 ++-
 env/mmc.c                             |   1 +-
 include/asm-generic/global_data.h     |   1 +-
 include/environment.h                 |  15 +-
 13 files changed, 266 insertions(+), 123 deletions(-)

base-commit: f3dd87e0b98999a78e500e8c6d2b063ebadf535a
-- 
git-series 0.9.1

[U-Boot] [PATCH v3 01/15] cmd: nvedit: Get rid of the env lookup

[Maxime Ripard]

The nvedit command is the only user of env_driver_lookup_default outside of
the environment code itself, and it uses it only to print the environment
it's about to save to during env save.

As we're about to rework the environment to be able to handle multiple
environment sources, we might not have an idea of what environment backend
is going to be used before trying (and possibly failing for some).

Therefore, it makes sense to remove that message and move it to the
env_save function itself. As a side effect, we also can get rid of the call
to env_driver_lookup_default that is also about to get refactored.

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>
---
 cmd/nvedit.c          | 4 ----
 env/env.c             | 4 +++-
 include/environment.h | 7 -------
 3 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 4e79d03856fe..a690d743cd46 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -708,10 +708,6 @@ ulong env_get_ulong(const char *name, int base, ulong default_val)
 static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc,
 		       char * const argv[])
 {
-	struct env_driver *env = env_driver_lookup_default();
-
-	printf("Saving Environment to %s...\n", env->name);
-
 	return env_save() ? 1 : 0;
 }
 
diff --git a/env/env.c b/env/env.c
index 76a5608628fc..094538ff5b62 100644
--- a/env/env.c
+++ b/env/env.c
@@ -52,7 +52,7 @@ static enum env_location env_get_default_location(void)
 		return ENVL_UNKNOWN;
 }
 
-struct env_driver *env_driver_lookup_default(void)
+static struct env_driver *env_driver_lookup_default(void)
 {
 	enum env_location loc = env_get_default_location();
 	struct env_driver *drv;
@@ -115,6 +115,8 @@ int env_save(void)
 		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__,
diff --git a/include/environment.h b/include/environment.h
index d29f82cb5d6f..a2015c299aa9 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -293,13 +293,6 @@ int env_import_redund(const char *buf1, const char *buf2);
 #endif
 
 /**
- * env_driver_lookup_default() - Look up the default environment driver
- *
- * @return pointer to driver, or NULL if none (which should not happen)
- */
-struct env_driver *env_driver_lookup_default(void);
-
-/**
  * env_get_char() - Get a character from the early environment
  *
  * This reads from the pre-relocation environemnt
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 01/15] cmd: nvedit: Get rid of the env lookup

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:50PM +0100, Maxime Ripard wrote:

> The nvedit command is the only user of env_driver_lookup_default outside of
> the environment code itself, and it uses it only to print the environment
> it's about to save to during env save.
> 
> As we're about to rework the environment to be able to handle multiple
> environment sources, we might not have an idea of what environment backend
> is going to be used before trying (and possibly failing for some).
> 
> Therefore, it makes sense to remove that message and move it to the
> env_save function itself. As a side effect, we also can get rid of the call
> to env_driver_lookup_default that is also about to get refactored.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/1ccd7bc5/attachment.sig>

[U-Boot] [PATCH v3 02/15] env: Rename env_driver_lookup_default and env_get_default_location

[Maxime Ripard]

The env_driver_lookup_default and env_get_default_location functions are
about to get refactored to support loading from multiple environment.

The name is therefore not really well suited anymore. Drop the default
part to be a bit more relevant.

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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/env/env.c b/env/env.c
index 094538ff5b62..97ada5b5a6fd 100644
--- a/env/env.c
+++ b/env/env.c
@@ -10,7 +10,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static struct env_driver *env_driver_lookup(enum env_location loc)
+static struct env_driver *_env_driver_lookup(enum env_location loc)
 {
 	struct env_driver *drv;
 	const int n_ents = ll_entry_count(struct env_driver, env_driver);
@@ -26,7 +26,7 @@ static struct env_driver *env_driver_lookup(enum env_location loc)
 	return NULL;
 }
 
-static enum env_location env_get_default_location(void)
+static enum env_location env_get_location(void)
 {
 	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
 		return ENVL_EEPROM;
@@ -52,12 +52,12 @@ static enum env_location env_get_default_location(void)
 		return ENVL_UNKNOWN;
 }
 
-static struct env_driver *env_driver_lookup_default(void)
+static struct env_driver *env_driver_lookup(void)
 {
-	enum env_location loc = env_get_default_location();
+	enum env_location loc = env_get_location();
 	struct env_driver *drv;
 
-	drv = env_driver_lookup(loc);
+	drv = _env_driver_lookup(loc);
 	if (!drv) {
 		debug("%s: No environment driver for location %d\n", __func__,
 		      loc);
@@ -69,7 +69,7 @@ static struct env_driver *env_driver_lookup_default(void)
 
 int env_get_char(int index)
 {
-	struct env_driver *drv = env_driver_lookup_default();
+	struct env_driver *drv = env_driver_lookup();
 	int ret;
 
 	if (gd->env_valid == ENV_INVALID)
@@ -89,7 +89,7 @@ int env_get_char(int index)
 
 int env_load(void)
 {
-	struct env_driver *drv = env_driver_lookup_default();
+	struct env_driver *drv = env_driver_lookup();
 	int ret = 0;
 
 	if (!drv)
@@ -108,7 +108,7 @@ int env_load(void)
 
 int env_save(void)
 {
-	struct env_driver *drv = env_driver_lookup_default();
+	struct env_driver *drv = env_driver_lookup();
 	int ret;
 
 	if (!drv)
@@ -129,7 +129,7 @@ int env_save(void)
 
 int env_init(void)
 {
-	struct env_driver *drv = env_driver_lookup_default();
+	struct env_driver *drv = env_driver_lookup();
 	int ret = -ENOENT;
 
 	if (!drv)
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 02/15] env: Rename env_driver_lookup_default and env_get_default_location

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:51PM +0100, Maxime Ripard wrote:

> The env_driver_lookup_default and env_get_default_location functions are
> about to get refactored to support loading from multiple environment.
> 
> The name is therefore not really well suited anymore. Drop the default
> part to be a bit more relevant.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/6c7d511e/attachment.sig>

[U-Boot] [PATCH v3 03/15] env: Pass additional parameters to the env lookup function

[Maxime Ripard]

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;
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 03/15] env: Pass additional parameters to the env lookup function

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:52PM +0100, 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/882478f2/attachment.sig>

[U-Boot] [PATCH v3 03/15] env: Pass additional parameters to the env lookup function

[Simon Goldschmidt]

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;

[U-Boot] [PATCH v3 04/15] env: Make the env save message a bit more explicit

[Maxime Ripard]

Since we'll soon have support for multiple environments, the environment
saving message might end up being printed multiple times if the higher
priority environment cannot be used.

That might confuse the user, so let's make it explicit if the operation
failed or not.

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 73da149fd8ca..11667a3cbc71 100644
--- a/env/env.c
+++ b/env/env.c
@@ -166,8 +166,9 @@ int env_save(void)
 		if (!drv->save)
 			continue;
 
-		printf("Saving Environment to %s...\n", drv->name);
+		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
+		printf("%s\n", ret ? "Failed" : "OK");
 		if (!ret)
 			return 0;
 
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 04/15] env: Make the env save message a bit more explicit

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:53PM +0100, Maxime Ripard wrote:

> Since we'll soon have support for multiple environments, the environment
> saving message might end up being printed multiple times if the higher
> priority environment cannot be used.
> 
> That might confuse the user, so let's make it explicit if the operation
> failed or not.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/b3709319/attachment.sig>

[U-Boot] [PATCH v3 05/15] env: Make it explicit where we're loading our environment from

[Maxime Ripard]

Since we can have multiple environments now, it's better to provide a
decent indication on what environments were tried and which were the one to
fail and succeed.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/env/env.c b/env/env.c
index 11667a3cbc71..906f28ee50a1 100644
--- a/env/env.c
+++ b/env/env.c
@@ -144,12 +144,15 @@ int env_load(void)
 		if (!drv->load)
 			continue;
 
+		printf("Loading Environment from %s... ", drv->name);
 		ret = drv->load();
+		if (ret)
+			printf("Failed (%d)\n", ret);
+		else
+			printf("OK\n");
+
 		if (!ret)
 			return 0;
-
-		debug("%s: Environment %s failed to load (err=%d)\n", __func__,
-		      drv->name, ret);
 	}
 
 	return -ENODEV;
@@ -168,12 +171,13 @@ int env_save(void)
 
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
-		printf("%s\n", ret ? "Failed" : "OK");
+		if (ret)
+			printf("Failed (%d)\n", ret);
+		else
+			printf("OK\n");
+
 		if (!ret)
 			return 0;
-
-		debug("%s: Environment %s failed to save (err=%d)\n", __func__,
-		      drv->name, ret);
 	}
 
 	return -ENODEV;
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 05/15] env: Make it explicit where we're loading our environment from

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:54PM +0100, Maxime Ripard wrote:

> Since we can have multiple environments now, it's better to provide a
> decent indication on what environments were tried and which were the one to
> fail and succeed.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/5be141c5/attachment.sig>

[U-Boot] [PATCH v3 06/15] env: fat: Make the debug messages play a little nicer

[Maxime Ripard]

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 FAT 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>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/fat.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/env/fat.c b/env/fat.c
index ec49c3905369..158a9a34357b 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -55,7 +55,11 @@ static int env_fat_save(void)
 
 	dev = dev_desc->devnum;
 	if (fat_set_blk_dev(dev_desc, &info) != 0) {
-		printf("\n** Unable to use %s %d:%d for saveenv **\n",
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to use %s %d:%d... ",
 		       CONFIG_ENV_FAT_INTERFACE, dev, part);
 		return 1;
 	}
@@ -63,12 +67,15 @@ static int env_fat_save(void)
 	err = file_fat_write(CONFIG_ENV_FAT_FILE, (void *)&env_new, 0, sizeof(env_t),
 			     &size);
 	if (err == -1) {
-		printf("\n** Unable to write \"%s\" from %s%d:%d **\n",
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to write \"%s\" from %s%d:%d... ",
 			CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part);
 		return 1;
 	}
 
-	puts("done\n");
 	return 0;
 }
 #endif /* CMD_SAVEENV */
@@ -90,14 +97,22 @@ static int env_fat_load(void)
 
 	dev = dev_desc->devnum;
 	if (fat_set_blk_dev(dev_desc, &info) != 0) {
-		printf("\n** Unable to use %s %d:%d for loading the env **\n",
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to use %s %d:%d... ",
 		       CONFIG_ENV_FAT_INTERFACE, dev, part);
 		goto err_env_relocate;
 	}
 
 	err = file_fat_read(CONFIG_ENV_FAT_FILE, buf, CONFIG_ENV_SIZE);
 	if (err == -1) {
-		printf("\n** Unable to read \"%s\" from %s%d:%d **\n",
+		/*
+		 * This printf is embedded in the messages from env_save that
+		 * will calling it. The missing \n is intentional.
+		 */
+		printf("Unable to read \"%s\" from %s%d:%d... ",
 			CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part);
 		goto err_env_relocate;
 	}
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 06/15] env: fat: Make the debug messages play a little nicer

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:55PM +0100, 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 FAT 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>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/f9ca1ec4/attachment.sig>

[U-Boot] [PATCH v3 07/15] env: mmc: Make the debug messages play a little nicer

[Maxime Ripard]

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>
---
 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
-- 
git-series 0.9.1

[U-Boot] [PATCH v3 07/15] env: mmc: Make the debug messages play a little nicer

[Jaehoon Chung]

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
> 

[U-Boot] [U-Boot, v3, 07/15] env: mmc: Make the debug messages play a little nicer

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:56PM +0100, 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/58c7acd5/attachment.sig>

[U-Boot] [PATCH v3 08/15] env: common: Make the debug messages play a little nicer

[Maxime Ripard]

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 the common code play a little nicer by removing all the extra output
in the standard case.

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/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 8167ea299264..c633502d68bb 100644
--- a/env/common.c
+++ b/env/common.c
@@ -78,7 +78,7 @@ void set_default_env(const char *s)
 			puts(s);
 		}
 	} else {
-		puts("Using default environment\n\n");
+		debug("Using default environment\n");
 	}
 
 	if (himport_r(&env_htab, (char *)default_environment,
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 08/15] env: common: Make the debug messages play a little nicer

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:57PM +0100, 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 the common code play a little nicer by removing all the extra output
> in the standard case.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/eff143f8/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

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.

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;
 }
 
 
-- 
git-series 0.9.1

[U-Boot] [U-Boot,v3,09/15] env: Support multiple environments

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:58PM +0100, 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, and we'll use the same one without lookup during writes. This
> should implement the same behaviour than we currently have.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/aa232caf/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

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

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[York Sun]

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.

York

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[York Sun]

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?

York

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[York Sun]

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?
> 

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;

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?

York

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

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
>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

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.

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

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/bcecb269/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[York Sun]

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

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

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.

I get more build errors testing this feature: there's a global variable 
'env_ptr' declared in flash, nand, nvram and remote env drivers 
(declared as extern in envrionment.h). From reading the code, it seems 
like these could just be changed to static, since 'env_ptr' is not used 
outside these drivers?

Simon

>
> 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.
>
> 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;
>   }
>   
>   

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Joakim Tjernlund]

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

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?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/6f9f7ff2/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Joakim Tjernlund]

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.

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

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?

Simon


>
>   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 at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Joakim Tjernlund]

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

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

On Wed, Feb 07, 2018 at 08:45:46AM +0000, Joakim Tjernlund wrote:
> 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.

It's exactly the point of this entire serie though, and why we merged
it.

You just need to override env_get_location in your board, and return
the preferred location.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/ab92d14d/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

On Tue, Feb 06, 2018 at 09:09:07AM +0100, 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.
> 
> I get more build errors testing this feature: there's a global variable
> 'env_ptr' declared in flash, nand, nvram and remote env drivers (declared as
> extern in envrionment.h). From reading the code, it seems like these could
> just be changed to static, since 'env_ptr' is not used outside these
> drivers?

Given Joakim's comment, I guess we should keep them !static, rename
them to $env_env_ptr, and remove the definition in the
include/environment that doesn't seem used anywhere.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/e071e98b/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

On 07.02.2018 09:19, Maxime Ripard wrote:
> On Tue, Feb 06, 2018 at 09:09:07AM +0100, 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.
>> I get more build errors testing this feature: there's a global variable
>> 'env_ptr' declared in flash, nand, nvram and remote env drivers (declared as
>> extern in envrionment.h). From reading the code, it seems like these could
>> just be changed to static, since 'env_ptr' is not used outside these
>> drivers?
> Given Joakim's comment, I guess we should keep them !static, rename
> them to $env_env_ptr, and remove the definition in the
> include/environment that doesn't seem used anywhere.

That's OK for me, I just wanted to point out the build error.

However, I do think that having unnecessary non-static global variables 
is not really good coding style as you risk name clashes. I'd really be 
interested in a reason for this.

Simon

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

1;5002;0c
On Wed, Feb 07, 2018 at 09:25:55AM +0100, Simon Goldschmidt wrote:
> On 07.02.2018 09:19, Maxime Ripard wrote:
> > On Tue, Feb 06, 2018 at 09:09:07AM +0100, 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.
> > > I get more build errors testing this feature: there's a global variable
> > > 'env_ptr' declared in flash, nand, nvram and remote env drivers (declared as
> > > extern in envrionment.h). From reading the code, it seems like these could
> > > just be changed to static, since 'env_ptr' is not used outside these
> > > drivers?
> > Given Joakim's comment, I guess we should keep them !static, rename
> > them to $env_env_ptr, and remove the definition in the
> > include/environment that doesn't seem used anywhere.
> 
> That's OK for me, I just wanted to point out the build error.
> 
> However, I do think that having unnecessary non-static global variables is
> not really good coding style as you risk name clashes. I'd really be
> interested in a reason for this.

If the relocation works with a static variable, I'm all for it.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/a44d255e/attachment.sig>

[U-Boot] [PATCH v3 10/15] env: Initialise all the environments

[Maxime Ripard]

Since we want to have multiple environments, we will need to initialise
all the environments since we don't know at init time what drivers might
fail when calling load.

Let's init all of them, and only consider for further operations the ones
that have not reported any errors at init time.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c                         | 36 +++++++++++++++++++++++++-------
 include/asm-generic/global_data.h |  1 +-
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/env/env.c b/env/env.c
index 1182fdb545db..2ea581cbedd6 100644
--- a/env/env.c
+++ b/env/env.c
@@ -61,6 +61,23 @@ static enum env_location env_locations[] = {
 
 static enum env_location env_load_location = ENVL_UNKNOWN;
 
+static bool env_has_inited(enum env_location location)
+{
+	return gd->env_has_init & BIT(location);
+}
+
+static void env_set_inited(enum env_location location)
+{
+	/*
+	 * We're using a 32-bits bitmask stored in gd (env_has_init)
+	 * using the above enum value as the bit index. We need to
+	 * make sure that we're not overflowing it.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(env_locations) > BITS_PER_LONG);
+
+	gd->env_has_init |= BIT(location);
+}
+
 /**
  * env_get_location() - Returns the best env location for a board
  * @op: operations performed on the environment
@@ -142,6 +159,9 @@ int env_get_char(int index)
 		if (!drv->get_char)
 			continue;
 
+		if (!env_has_inited(drv->location))
+			continue;
+
 		ret = drv->get_char(index);
 		if (!ret)
 			return 0;
@@ -164,6 +184,9 @@ int env_load(void)
 		if (!drv->load)
 			continue;
 
+		if (!env_has_inited(drv->location))
+			continue;
+
 		printf("Loading Environment from %s... ", drv->name);
 		ret = drv->load();
 		if (ret)
@@ -189,6 +212,9 @@ int env_save(void)
 		if (!drv->save)
 			continue;
 
+		if (!env_has_inited(drv->location))
+			continue;
+
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
 		if (ret)
@@ -210,14 +236,10 @@ int env_init(void)
 	int prio;
 
 	for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
-		if (!drv->init)
-			continue;
-
-		ret = drv->init();
-		if (!ret)
-			return 0;
+		if (!drv->init || !(ret = drv->init()))
+			env_set_inited(drv->location);
 
-		debug("%s: Environment %s failed to init (err=%d)\n", __func__,
+		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
 	}
 
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 73e036d6fd4a..fd8cd45b050e 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -50,6 +50,7 @@ typedef struct global_data {
 #endif
 	unsigned long env_addr;		/* Address  of Environment struct */
 	unsigned long env_valid;	/* Environment valid? enum env_valid */
+	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
 
 	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
 	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 10/15] env: Initialise all the environments

[Tom Rini]

On Tue, Jan 23, 2018 at 09:16:59PM +0100, Maxime Ripard wrote:

> Since we want to have multiple environments, we will need to initialise
> all the environments since we don't know at init time what drivers might
> fail when calling load.
> 
> Let's init all of them, and only consider for further operations the ones
> that have not reported any errors at init time.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/f6d0d7e1/attachment.sig>

[U-Boot] [PATCH v3 11/15] env: mmc: depends on the MMC framework

[Maxime Ripard]

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>
---
 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.
-- 
git-series 0.9.1

[U-Boot] [PATCH v3 11/15] env: mmc: depends on the MMC framework

[Jaehoon Chung]

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.
> 

[U-Boot] [U-Boot, v3, 11/15] env: mmc: depends on the MMC framework

[Tom Rini]

On Tue, Jan 23, 2018 at 09:17:00PM +0100, 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/5804d732/attachment.sig>

[U-Boot] [PATCH v3 12/15] env: Allow to build multiple environments in Kconfig

[Maxime Ripard]

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>
---
 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 at 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 at 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
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 12/15] env: Allow to build multiple environments in Kconfig

[Tom Rini]

On Tue, Jan 23, 2018 at 09:17:01PM +0100, 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/7c063843/attachment.sig>

[U-Boot] [PATCH v3 12/15] env: Allow to build multiple environments in Kconfig

[Simon Goldschmidt]

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 at 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 at 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

[U-Boot] [PATCH v3 12/15] env: Allow to build multiple environments in Kconfig

[Maxime Ripard]

On Thu, Feb 01, 2018 at 11:06:14AM +0100, Simon Goldschmidt wrote:
> 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.

Is that happening in any of the current defconfig right now? Or is it
only when you add more environments?

> 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...

I'm not sure what it could be either, that file looks like it would
need a quite big rework, in order to be able to operate properly.

Do you actually need those? Maybe we can just disable those in Kconfig
to forbid such a combination?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180202/a2ec6fd6/attachment.sig>

[U-Boot] [PATCH v3 13/15] env: Mark env_get_location as weak

[Maxime Ripard]

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

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:
-- 
git-series 0.9.1

[U-Boot] [U-Boot,v3,13/15] env: Mark env_get_location as weak

[Tom Rini]

On Tue, Jan 23, 2018 at 09:17:02PM +0100, Maxime Ripard wrote:

> Allow boards and architectures to override the default environment lookup
> code by overriding env_get_location.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/3a114d6c/attachment.sig>

[U-Boot] [PATCH v3 13/15] env: Mark env_get_location as weak

[Simon Goldschmidt]

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:

[U-Boot] [PATCH v3 13/15] env: Mark env_get_location as weak

[Maxime Ripard]

On Tue, Jan 30, 2018 at 09:12:41AM +0100, Simon Goldschmidt wrote:
> 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.

Well, it's not really referenced, there's a build time boundary check
that we do not overflow our bitfield.

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: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180130/77b64dde/attachment.sig>

[U-Boot] [PATCH v3 14/15] sunxi: Transition from the MMC to a FAT-based environment

[Maxime Ripard]

The current environment has been hardcoded to an offset that starts to be
an issue given the current size of our main U-Boot binary.

By implementing a custom environment location routine, we can always favor
the FAT-based environment, and fallback to the MMC if we don't find
something in the FAT partition. We also implement the same order when
saving the environment, so that hopefully we can slowly migrate the users
over to FAT-based environment and away from the raw MMC one.

Eventually, and hopefully before we reach that limit again, we will have
most of our users using that setup, and we'll be able to retire the raw
environment, and gain more room for the U-Boot binary.

Reviewed-by: Lukasz Majewski <lukma@denx.de>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 board/sunxi/board.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index dcacdf3e626d..8891961dcc6b 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -173,6 +173,22 @@ void i2c_init_board(void)
 #endif
 }
 
+#if defined(CONFIG_ENV_IS_IN_MMC) && defined(CONFIG_ENV_IS_IN_FAT)
+enum env_location env_get_location(enum env_operation op, int prio)
+{
+	switch (prio) {
+	case 0:
+		return ENVL_FAT;
+
+	case 1:
+		return ENVL_MMC;
+
+	default:
+		return ENVL_UNKNOWN;
+	}
+}
+#endif
+
 /* add board specific code here */
 int board_init(void)
 {
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 14/15] sunxi: Transition from the MMC to a FAT-based environment

[Tom Rini]

On Tue, Jan 23, 2018 at 09:17:03PM +0100, Maxime Ripard wrote:

> The current environment has been hardcoded to an offset that starts to be
> an issue given the current size of our main U-Boot binary.
> 
> By implementing a custom environment location routine, we can always favor
> the FAT-based environment, and fallback to the MMC if we don't find
> something in the FAT partition. We also implement the same order when
> saving the environment, so that hopefully we can slowly migrate the users
> over to FAT-based environment and away from the raw MMC one.
> 
> Eventually, and hopefully before we reach that limit again, we will have
> most of our users using that setup, and we'll be able to retire the raw
> environment, and gain more room for the U-Boot binary.
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/2474c4ad/attachment.sig>

[U-Boot] [PATCH v3 15/15] env: sunxi: Enable FAT-based environment support by default

[Maxime Ripard]

Now that we have everything in place to implement the transition scheme,
let's enable it by default.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/env/Kconfig b/env/Kconfig
index 6e2fbf416c12..d1bf75ab41ca 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -55,6 +55,7 @@ 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 ARCH_SUNXI && MMC
 	default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS
 	select FAT_WRITE
 	help
@@ -363,6 +364,7 @@ config ENV_IS_IN_UBI
 config ENV_FAT_INTERFACE
 	string "Name of the block device for the environment"
 	depends on ENV_IS_IN_FAT
+	default "mmc" if ARCH_SUNXI
 	default "mmc" if TI_COMMON_CMD_OPTIONS || ARCH_ZYNQMP || ARCH_AT91
 	help
 	  Define this to a string that is the name of the block device.
@@ -372,6 +374,8 @@ config ENV_FAT_DEVICE_AND_PART
 	depends on ENV_IS_IN_FAT
 	default "0:1" if TI_COMMON_CMD_OPTIONS
 	default "0:auto" if ARCH_ZYNQMP
+	default "0:auto" if ARCH_SUNXI && MMC_SUNXI_SLOT_EXTRA = -1
+	default "1:auto" if ARCH_SUNXI && MMC_SUNXI_SLOT_EXTRA != -1
 	default "0" if ARCH_AT91
 	help
 	  Define this to a string to specify the partition of the device. It can
-- 
git-series 0.9.1

[U-Boot] [U-Boot, v3, 15/15] env: sunxi: Enable FAT-based environment support by default

[Tom Rini]

On Tue, Jan 23, 2018 at 09:17:04PM +0100, Maxime Ripard wrote:

> Now that we have everything in place to implement the transition scheme,
> let's enable it by default.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180127/919be3c2/attachment.sig>

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Goldschmidt Simon]

On 02/07/2018 21:18, York Sun wrote:
> 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.

I still think we should get rid of the 'get_char' callback for
the env drivers. While that could have made sense for some boards
before the conversion to multiple environments (although I
doubt that, as the environment is *not* checked for
validity in this call), its meaning is totally lost when having
multiple env drivers active.

The whole purpose of multiple env drivers is that we select a
valid driver in the 'load' callback. How could we possibly know
that the 'get_char' callback of the highest prio env driver is
what we want?

I'd rather make 'env_get_char' weak and let boards decide if they
really want this behaviour.

A quick search through the current code base shows me *no* usage
of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:

    imx31_phycore_defconfig
    imx31_phycore_eet_defconfig
    km_kirkwood_128m16_defconfig
    km_kirkwood_defconfig
    km_kirkwood_pci_defconfig
    mgcoge3un_defconfig
    portl2_defconfig

Are these seven boards worth keeping this feature?

Simon

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

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[York Sun]

On 02/07/2018 12:45 PM, Goldschmidt Simon wrote:
> On 02/07/2018 21:18, York Sun wrote:
>> 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.

Actually I am not a big fun to using global data. It increases size for
everybody. But I don't see a way you can save the variable before
relocation.

>>>
>>>>> 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.
> 
> I still think we should get rid of the 'get_char' callback for
> the env drivers. While that could have made sense for some boards
> before the conversion to multiple environments (although I
> doubt that, as the environment is *not* checked for
> validity in this call), its meaning is totally lost when having
> multiple env drivers active.
> 
> The whole purpose of multiple env drivers is that we select a
> valid driver in the 'load' callback. How could we possibly know
> that the 'get_char' callback of the highest prio env driver is
> what we want?
> 
> I'd rather make 'env_get_char' weak and let boards decide if they
> really want this behaviour.
> 
> A quick search through the current code base shows me *no* usage
> of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
> defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:
> 
>     imx31_phycore_defconfig
>     imx31_phycore_eet_defconfig
>     km_kirkwood_128m16_defconfig
>     km_kirkwood_defconfig
>     km_kirkwood_pci_defconfig
>     mgcoge3un_defconfig
>     portl2_defconfig
> 
> Are these seven boards worth keeping this feature?
> 

Simon,

Adding multiple environments seems to be an improvement. But this fell
through the cracks. I don't know if other boards also read env before
relocation. All my boards reads hwconfig before relocation. Having to
create a new function for all of them doesn't look appealing to me.

York

[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

On 07.02.2018 22:18, York Sun wrote:
> On 02/07/2018 12:45 PM, Goldschmidt Simon wrote:
>> On 02/07/2018 21:18, York Sun wrote:
>>> 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.
> Actually I am not a big fun to using global data. It increases size for
> everybody. But I don't see a way you can save the variable before
> relocation.
>
>>>>>> 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.
>> I still think we should get rid of the 'get_char' callback for
>> the env drivers. While that could have made sense for some boards
>> before the conversion to multiple environments (although I
>> doubt that, as the environment is *not* checked for
>> validity in this call), its meaning is totally lost when having
>> multiple env drivers active.
>>
>> The whole purpose of multiple env drivers is that we select a
>> valid driver in the 'load' callback. How could we possibly know
>> that the 'get_char' callback of the highest prio env driver is
>> what we want?
>>
>> I'd rather make 'env_get_char' weak and let boards decide if they
>> really want this behaviour.
>>
>> A quick search through the current code base shows me *no* usage
>> of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
>> defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:
>>
>>      imx31_phycore_defconfig
>>      imx31_phycore_eet_defconfig
>>      km_kirkwood_128m16_defconfig
>>      km_kirkwood_defconfig
>>      km_kirkwood_pci_defconfig
>>      mgcoge3un_defconfig
>>      portl2_defconfig
>>
>> Are these seven boards worth keeping this feature?
>>
> Simon,
>
> Adding multiple environments seems to be an improvement. But this fell
> through the cracks. I don't know if other boards also read env before
> relocation. All my boards reads hwconfig before relocation. Having to
> create a new function for all of them doesn't look appealing to me.

The change I proposed would be to restore the old behavior but kick out 
the byte-by-byte reading from eeprom and nvram. My understanding was you 
are using flash and were reading from environment in ram, not in nvram 
or eeprom.

Simon