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

[U-Boot] [RFC PATCH 00/10] 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.

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

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.

Let me know what you think,
Maxime

Maxime Ripard (10):
  cmd: nvedit: Get rid of the env lookup
  env: Make env_driver_lookup_default private
  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: Support multiple environments
  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 --
 env/Kconfig           |  69 +++++++++---------
 env/env.c             | 194 ++++++++++++++++++++++++++++++++------------------
 include/environment.h |  14 ++--
 5 files changed, 182 insertions(+), 115 deletions(-)

-- 
2.14.3

[U-Boot] [RFC PATCH 01/10] 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.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 cmd/nvedit.c | 4 ----
 env/env.c    | 2 ++
 2 files changed, 2 insertions(+), 4 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..84c12e27bc3f 100644
--- a/env/env.c
+++ b/env/env.c
@@ -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__,
-- 
2.14.3

[U-Boot] [RFC PATCH 02/10] env: Make env_driver_lookup_default private

[Maxime Ripard]

No that there's no users of env_driver_lookup_default outside of env/env.c,
we can mark that function static and remove it from the environment header.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c             | 2 +-
 include/environment.h | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/env/env.c b/env/env.c
index 84c12e27bc3f..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;
diff --git a/include/environment.h b/include/environment.h
index 7b9821638960..226e3ef2d23a 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -302,13 +302,6 @@ int env_export(env_t *env_out);
 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
  *
-- 
2.14.3

[U-Boot] [RFC PATCH 03/10] 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.

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

[U-Boot] [RFC PATCH 04/10] 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             | 122 ++++++++++++++++++++++++++++++--------------------
 include/environment.h |   7 +++
 2 files changed, 80 insertions(+), 49 deletions(-)

diff --git a/env/env.c b/env/env.c
index 97ada5b5a6fd..673bfa6ba41b 100644
--- a/env/env.c
+++ b/env/env.c
@@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
 	return NULL;
 }
 
-static enum env_location env_get_location(void)
+static enum env_location env_get_location(enum env_operation op, int prio)
 {
+	if (prio >= 1)
+		return ENVL_UNKNOWN;
+
 	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
 		return ENVL_EEPROM;
 	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
@@ -52,11 +55,14 @@ static enum env_location env_get_location(void)
 		return ENVL_UNKNOWN;
 }
 
-static struct env_driver *env_driver_lookup(void)
+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 +75,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(ENVO_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(ENVO_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(ENVO_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(ENVO_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 226e3ef2d23a..7fa8b98cc0db 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -215,6 +215,13 @@ enum env_location {
 	ENVL_UNKNOWN,
 };
 
+enum env_operation {
+	ENVO_GET_CHAR,
+	ENVO_INIT,
+	ENVO_LOAD,
+	ENVO_SAVE,
+};
+
 struct env_driver {
 	const char *name;
 	enum env_location location;
-- 
2.14.3

[U-Boot] [RFC PATCH 05/10] 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.

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 673bfa6ba41b..1d13220aa79b 100644
--- a/env/env.c
+++ b/env/env.c
@@ -131,8 +131,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;
 
-- 
2.14.3

[U-Boot] [RFC PATCH 06/10] 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.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c | 75 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/env/env.c b/env/env.c
index 1d13220aa79b..6af9f897b0ae 100644
--- a/env/env.c
+++ b/env/env.c
@@ -26,33 +26,58 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
 	return NULL;
 }
 
+static enum env_location env_locations[] = {
+#ifdef CONFIG_ENV_IS_IN_EEPROM
+	ENVL_EEPROM,
+#endif
+#ifdef CONFIG_ENV_IS_IN_FAT
+	ENVL_FAT,
+#endif
+#ifdef CONFIG_ENV_IS_IN_FLASH
+	ENVL_FLASH,
+#endif
+#ifdef CONFIG_ENV_IS_IN_MMC
+	ENVL_MMC,
+#endif
+#ifdef CONFIG_ENV_IS_IN_NAND
+	ENVL_NAND,
+#endif
+#ifdef CONFIG_ENV_IS_IN_NVRAM
+	ENVL_NVRAM,
+#endif
+#ifdef CONFIG_ENV_IS_IN_REMOTE
+	ENVL_REMOTE,
+#endif
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
+	ENVL_SPI_FLASH,
+#endif
+#ifdef CONFIG_ENV_IS_IN_UBI
+	ENVL_UBI,
+#endif
+#ifdef CONFIG_ENV_IS_NOWHERE
+	ENVL_NOWHERE,
+#endif
+	ENVL_UNKNOWN,
+};
+
+static enum env_location env_load_location;
+
 static enum env_location env_get_location(enum env_operation op, int prio)
 {
-	if (prio >= 1)
-		return ENVL_UNKNOWN;
-
-	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
-		return ENVL_EEPROM;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
-		return ENVL_FAT;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
-		return ENVL_FLASH;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
-		return ENVL_MMC;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
-		return ENVL_NAND;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
-		return ENVL_NVRAM;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
-		return ENVL_REMOTE;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
-		return ENVL_SPI_FLASH;
-	else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
-		return ENVL_UBI;
-	else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
-		return ENVL_NOWHERE;
-	else
-		return ENVL_UNKNOWN;
+	switch (op) {
+	case ENVO_GET_CHAR:
+	case ENVO_INIT:
+	case ENVO_LOAD:
+		if (prio >= ARRAY_SIZE(env_locations))
+			return -ENODEV;
+
+		return env_load_location = env_locations[prio];
+
+	case ENVO_SAVE:
+		return env_load_location;
+	}
+
+	return ENVL_UNKNOWN;
 }
 
 static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
-- 
2.14.3

[U-Boot] [RFC PATCH 07/10] 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.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/Kconfig | 65 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index 8c9d800f485f..bf6eab6b4ace 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.
@@ -156,6 +145,11 @@ config ENV_IS_IN_FLASH
 config ENV_IS_IN_MMC
 	bool "Environment in an MMC device"
 	depends on !CHAIN_OF_TRUST
+	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.
@@ -293,6 +287,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.
@@ -358,8 +359,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_AES
 	bool "AES-128 encryption for stored environment (DEPRECATED)"
 	help
-- 
2.14.3

[U-Boot] [RFC PATCH 08/10] 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.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c
index 6af9f897b0ae..aceb77d4073c 100644
--- a/env/env.c
+++ b/env/env.c
@@ -62,7 +62,7 @@ static enum env_location env_locations[] = {
 
 static enum env_location env_load_location;
 
-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 ENVO_GET_CHAR:
-- 
2.14.3

[U-Boot] [RFC PATCH 09/10] 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.

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)
 {
-- 
2.14.3

[U-Boot] [RFC PATCH 10/10] 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.

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 bf6eab6b4ace..19524638e6e1 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
 	default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS
 	select FAT_WRITE
 	help
@@ -370,6 +371,7 @@ config ENV_AES
 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.
@@ -379,6 +381,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
-- 
2.14.3

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

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:22 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  cmd/nvedit.c | 4 ----
>  env/env.c    | 2 ++
>  2 files changed, 2 insertions(+), 4 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..84c12e27bc3f 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -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__,

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/bbf7eacf/attachment.sig>

[U-Boot] [RFC PATCH 02/10] env: Make env_driver_lookup_default private

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:23 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> No that there's no users of env_driver_lookup_default outside of
> env/env.c, we can mark that function static and remove it from the
> environment header.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c             | 2 +-
>  include/environment.h | 7 -------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 84c12e27bc3f..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;
> diff --git a/include/environment.h b/include/environment.h
> index 7b9821638960..226e3ef2d23a 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -302,13 +302,6 @@ int env_export(env_t *env_out);
>  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
>   *

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 484 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/f6b4426f/attachment.sig>

[U-Boot] [RFC PATCH 03/10] env: Rename env_driver_lookup_default and env_get_default_location

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:24 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> 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.
> 
> 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)

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/03a28cb6/attachment.sig>

[U-Boot] [RFC PATCH 04/10] env: Pass additional parameters to the env lookup function

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:25 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> 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             | 122
> ++++++++++++++++++++++++++++++--------------------
> include/environment.h |   7 +++ 2 files changed, 80 insertions(+), 49
> deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 97ada5b5a6fd..673bfa6ba41b 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum
> env_location loc) return NULL;
>  }
>  
> -static enum env_location env_get_location(void)
> +static enum env_location env_get_location(enum env_operation op, int
> prio) {
> +	if (prio >= 1)
> +		return ENVL_UNKNOWN;

Is this correct? 

Above you mentioned that prio = 0 is the default.

With this condition you may only allow prio <= 0...

> +
>  	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
>  		return ENVL_EEPROM;
>  	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> @@ -52,11 +55,14 @@ static enum env_location env_get_location(void)
>  		return ENVL_UNKNOWN;
>  }
>  
> -static struct env_driver *env_driver_lookup(void)
> +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 +75,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(ENVO_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(ENVO_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(ENVO_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(ENVO_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 226e3ef2d23a..7fa8b98cc0db 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -215,6 +215,13 @@ enum env_location {
>  	ENVL_UNKNOWN,
>  };
>  
> +enum env_operation {
> +	ENVO_GET_CHAR,
> +	ENVO_INIT,
> +	ENVO_LOAD,
> +	ENVO_SAVE,
> +};
> +
>  struct env_driver {
>  	const char *name;
>  	enum env_location location;



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/2672c739/attachment.sig>

[U-Boot] [RFC PATCH 05/10] env: Make the env save message a bit more explicit

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:26 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> 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.
> 
> 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 673bfa6ba41b..1d13220aa79b 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -131,8 +131,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;
>  

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/e283940f/attachment.sig>

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

[Lukasz Majewski]

Hi Maxime,

> Now that we have everything in place to support multiple environment,
> let's make sure the current code can use it.
> 
> The priority used between the various environment is the same one
> that was used in the code previously.
> 
> At read / init times, the highest priority environment is going to be
> detected, and we'll use the same one without lookup during writes.
> This should implement the same behaviour than we currently have.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 75
> ++++++++++++++++++++++++++++++++++++++++++--------------------- 1
> file changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 1d13220aa79b..6af9f897b0ae 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,33 +26,58 @@ static struct env_driver *_env_driver_lookup(enum
> env_location loc) return NULL;
>  }
>  
> +static enum env_location env_locations[] = {
> +#ifdef CONFIG_ENV_IS_IN_EEPROM
> +	ENVL_EEPROM,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_FAT
> +	ENVL_FAT,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_FLASH
> +	ENVL_FLASH,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_MMC
> +	ENVL_MMC,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_NAND
> +	ENVL_NAND,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_NVRAM
> +	ENVL_NVRAM,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_REMOTE
> +	ENVL_REMOTE,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> +	ENVL_SPI_FLASH,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_UBI
> +	ENVL_UBI,
> +#endif
> +#ifdef CONFIG_ENV_IS_NOWHERE
> +	ENVL_NOWHERE,
> +#endif
> +	ENVL_UNKNOWN,
> +};
> +
> +static enum env_location env_load_location;
> +
>  static enum env_location env_get_location(enum env_operation op, int
> prio) {
> -	if (prio >= 1)
> -		return ENVL_UNKNOWN;
> -
> -	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> -		return ENVL_EEPROM;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> -		return ENVL_FAT;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> -		return ENVL_FLASH;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> -		return ENVL_MMC;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> -		return ENVL_NAND;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> -		return ENVL_NVRAM;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> -		return ENVL_REMOTE;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> -		return ENVL_SPI_FLASH;
> -	else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> -		return ENVL_UBI;
> -	else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> -		return ENVL_NOWHERE;
> -	else
> -		return ENVL_UNKNOWN;
> +	switch (op) {
> +	case ENVO_GET_CHAR:
> +	case ENVO_INIT:
> +	case ENVO_LOAD:
> +		if (prio >= ARRAY_SIZE(env_locations))
> +			return -ENODEV;
> +
> +		return env_load_location = env_locations[prio];
> +
> +	case ENVO_SAVE:
> +		return env_load_location;
> +	}
> +
> +	return ENVL_UNKNOWN;
>  }
>  
>  static struct env_driver *env_driver_lookup(enum env_operation op,
> int prio)

I'm just wondering...

Do you think that it may be helpful to have a way to force particular
environment to be used (despite of the priority)?

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/5339cb85/attachment.sig>

[U-Boot] [RFC PATCH 07/10] env: Allow to build multiple environments in Kconfig

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:28 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Now that we have everything in place in the code, let's allow to build
> multiple environments backend through Kconfig.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/Kconfig | 65
> ++++++++++++++++++++++++++++++------------------------------- 1 file
> changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/env/Kconfig b/env/Kconfig
> index 8c9d800f485f..bf6eab6b4ace 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.
> @@ -156,6 +145,11 @@ config ENV_IS_IN_FLASH
>  config ENV_IS_IN_MMC
>  	bool "Environment in an MMC device"
>  	depends on !CHAIN_OF_TRUST
> +	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.
> @@ -293,6 +287,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.
> @@ -358,8 +359,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_AES
>  	bool "AES-128 encryption for stored environment (DEPRECATED)"
>  	help

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/fda532ee/attachment.sig>

[U-Boot] [RFC PATCH 08/10] env: Mark env_get_location as weak

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:29 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Allow boards and architectures to override the default environment
> lookup code by overriding env_get_location.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 6af9f897b0ae..aceb77d4073c 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {
>  
>  static enum env_location env_load_location;
>  
> -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 ENVO_GET_CHAR:

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/d4bb9fe1/attachment.sig>

[U-Boot] [RFC PATCH 09/10] sunxi: Transition from the MMC to a FAT-based environment

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:30 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> 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.
> 
> 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)
>  {

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/ef81c215/attachment.sig>

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

[Lukasz Majewski]

On Thu, 16 Nov 2017 10:22:31 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Now that we have everything in place to implement the transition
> scheme, let's enable it by default.
> 
> 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 bf6eab6b4ace..19524638e6e1 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
>  	default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS
>  	select FAT_WRITE
>  	help
> @@ -370,6 +371,7 @@ config ENV_AES
>  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. @@ -379,6 +381,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

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/0d0905ab/attachment.sig>

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

[Tom Rini]

On Fri, Nov 17, 2017 at 10:24:37AM +0100, Lukasz Majewski wrote:
> Hi Maxime,
> 
> > Now that we have everything in place to support multiple environment,
> > let's make sure the current code can use it.
> > 
> > The priority used between the various environment is the same one
> > that was used in the code previously.
> > 
> > At read / init times, the highest priority environment is going to be
> > detected, and we'll use the same one without lookup during writes.
> > This should implement the same behaviour than we currently have.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  env/env.c | 75
> > ++++++++++++++++++++++++++++++++++++++++++--------------------- 1
> > file changed, 50 insertions(+), 25 deletions(-)
> > 
> > diff --git a/env/env.c b/env/env.c
> > index 1d13220aa79b..6af9f897b0ae 100644
> > --- a/env/env.c
> > +++ b/env/env.c
> > @@ -26,33 +26,58 @@ static struct env_driver *_env_driver_lookup(enum
> > env_location loc) return NULL;
> >  }
> >  
> > +static enum env_location env_locations[] = {
> > +#ifdef CONFIG_ENV_IS_IN_EEPROM
> > +	ENVL_EEPROM,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_FAT
> > +	ENVL_FAT,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_FLASH
> > +	ENVL_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_MMC
> > +	ENVL_MMC,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NAND
> > +	ENVL_NAND,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NVRAM
> > +	ENVL_NVRAM,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_REMOTE
> > +	ENVL_REMOTE,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > +	ENVL_SPI_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_UBI
> > +	ENVL_UBI,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> > +	ENVL_NOWHERE,
> > +#endif
> > +	ENVL_UNKNOWN,
> > +};
> > +
> > +static enum env_location env_load_location;
> > +
> >  static enum env_location env_get_location(enum env_operation op, int
> > prio) {
> > -	if (prio >= 1)
> > -		return ENVL_UNKNOWN;
> > -
> > -	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> > -		return ENVL_EEPROM;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> > -		return ENVL_FAT;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> > -		return ENVL_FLASH;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> > -		return ENVL_MMC;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> > -		return ENVL_NAND;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> > -		return ENVL_NVRAM;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> > -		return ENVL_REMOTE;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> > -		return ENVL_SPI_FLASH;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> > -		return ENVL_UBI;
> > -	else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> > -		return ENVL_NOWHERE;
> > -	else
> > -		return ENVL_UNKNOWN;
> > +	switch (op) {
> > +	case ENVO_GET_CHAR:
> > +	case ENVO_INIT:
> > +	case ENVO_LOAD:
> > +		if (prio >= ARRAY_SIZE(env_locations))
> > +			return -ENODEV;
> > +
> > +		return env_load_location = env_locations[prio];
> > +
> > +	case ENVO_SAVE:
> > +		return env_load_location;
> > +	}
> > +
> > +	return ENVL_UNKNOWN;
> >  }
> >  
> >  static struct env_driver *env_driver_lookup(enum env_operation op,
> > int prio)
> 
> I'm just wondering...
> 
> Do you think that it may be helpful to have a way to force particular
> environment to be used (despite of the priority)?

That's one of the hard parts of this, yes.  I think I had suggested
before that we handle that in a follow up series?

-- 
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/20171117/f84bb900/attachment.sig>

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

[Lukasz Majewski]

On Fri, 17 Nov 2017 08:41:05 -0500
Tom Rini <trini@konsulko.com> wrote:

> On Fri, Nov 17, 2017 at 10:24:37AM +0100, Lukasz Majewski wrote:
> > Hi Maxime,
> >   
> > > Now that we have everything in place to support multiple
> > > environment, let's make sure the current code can use it.
> > > 
> > > The priority used between the various environment is the same one
> > > that was used in the code previously.
> > > 
> > > At read / init times, the highest priority environment is going
> > > to be detected, and we'll use the same one without lookup during
> > > writes. This should implement the same behaviour than we
> > > currently have.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > >  env/env.c | 75
> > > ++++++++++++++++++++++++++++++++++++++++++--------------------- 1
> > > file changed, 50 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/env/env.c b/env/env.c
> > > index 1d13220aa79b..6af9f897b0ae 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -26,33 +26,58 @@ static struct env_driver
> > > *_env_driver_lookup(enum env_location loc) return NULL;
> > >  }
> > >  
> > > +static enum env_location env_locations[] = {
> > > +#ifdef CONFIG_ENV_IS_IN_EEPROM
> > > +	ENVL_EEPROM,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_FAT
> > > +	ENVL_FAT,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_FLASH
> > > +	ENVL_FLASH,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_MMC
> > > +	ENVL_MMC,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_NAND
> > > +	ENVL_NAND,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_NVRAM
> > > +	ENVL_NVRAM,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_REMOTE
> > > +	ENVL_REMOTE,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > > +	ENVL_SPI_FLASH,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_IN_UBI
> > > +	ENVL_UBI,
> > > +#endif
> > > +#ifdef CONFIG_ENV_IS_NOWHERE
> > > +	ENVL_NOWHERE,
> > > +#endif
> > > +	ENVL_UNKNOWN,
> > > +};
> > > +
> > > +static enum env_location env_load_location;
> > > +
> > >  static enum env_location env_get_location(enum env_operation op,
> > > int prio) {
> > > -	if (prio >= 1)
> > > -		return ENVL_UNKNOWN;
> > > -
> > > -	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> > > -		return ENVL_EEPROM;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> > > -		return ENVL_FAT;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> > > -		return ENVL_FLASH;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> > > -		return ENVL_MMC;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> > > -		return ENVL_NAND;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> > > -		return ENVL_NVRAM;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> > > -		return ENVL_REMOTE;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> > > -		return ENVL_SPI_FLASH;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> > > -		return ENVL_UBI;
> > > -	else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> > > -		return ENVL_NOWHERE;
> > > -	else
> > > -		return ENVL_UNKNOWN;
> > > +	switch (op) {
> > > +	case ENVO_GET_CHAR:
> > > +	case ENVO_INIT:
> > > +	case ENVO_LOAD:
> > > +		if (prio >= ARRAY_SIZE(env_locations))
> > > +			return -ENODEV;
> > > +
> > > +		return env_load_location = env_locations[prio];
> > > +
> > > +	case ENVO_SAVE:
> > > +		return env_load_location;
> > > +	}
> > > +
> > > +	return ENVL_UNKNOWN;
> > >  }
> > >  
> > >  static struct env_driver *env_driver_lookup(enum env_operation
> > > op, int prio)  
> > 
> > I'm just wondering...
> > 
> > Do you think that it may be helpful to have a way to force
> > particular environment to be used (despite of the priority)?  
> 
> That's one of the hard parts of this, yes.  I think I had suggested
> before that we handle that in a follow up series?
> 

I don't want to say that this must be done now. I just can imagine that
we may need such feature in some point.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/ac8a0903/attachment.sig>

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

[Maxime Ripard]

Hi Lukasz, Tom,

On Fri, Nov 17, 2017 at 03:00:15PM +0100, Lukasz Majewski wrote:
> On Fri, 17 Nov 2017 08:41:05 -0500
> Tom Rini <trini@konsulko.com> wrote:
> 
> > On Fri, Nov 17, 2017 at 10:24:37AM +0100, Lukasz Majewski wrote:
> > > Hi Maxime,
> > >   
> > > > Now that we have everything in place to support multiple
> > > > environment, let's make sure the current code can use it.
> > > > 
> > > > The priority used between the various environment is the same one
> > > > that was used in the code previously.
> > > > 
> > > > At read / init times, the highest priority environment is going
> > > > to be detected, and we'll use the same one without lookup during
> > > > writes. This should implement the same behaviour than we
> > > > currently have.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > ---
> > > >  env/env.c | 75
> > > > ++++++++++++++++++++++++++++++++++++++++++--------------------- 1
> > > > file changed, 50 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/env/env.c b/env/env.c
> > > > index 1d13220aa79b..6af9f897b0ae 100644
> > > > --- a/env/env.c
> > > > +++ b/env/env.c
> > > > @@ -26,33 +26,58 @@ static struct env_driver
> > > > *_env_driver_lookup(enum env_location loc) return NULL;
> > > >  }
> > > >  
> > > > +static enum env_location env_locations[] = {
> > > > +#ifdef CONFIG_ENV_IS_IN_EEPROM
> > > > +	ENVL_EEPROM,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_FAT
> > > > +	ENVL_FAT,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_FLASH
> > > > +	ENVL_FLASH,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_MMC
> > > > +	ENVL_MMC,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_NAND
> > > > +	ENVL_NAND,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_NVRAM
> > > > +	ENVL_NVRAM,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_REMOTE
> > > > +	ENVL_REMOTE,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > > > +	ENVL_SPI_FLASH,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_IN_UBI
> > > > +	ENVL_UBI,
> > > > +#endif
> > > > +#ifdef CONFIG_ENV_IS_NOWHERE
> > > > +	ENVL_NOWHERE,
> > > > +#endif
> > > > +	ENVL_UNKNOWN,
> > > > +};
> > > > +
> > > > +static enum env_location env_load_location;
> > > > +
> > > >  static enum env_location env_get_location(enum env_operation op,
> > > > int prio) {
> > > > -	if (prio >= 1)
> > > > -		return ENVL_UNKNOWN;
> > > > -
> > > > -	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> > > > -		return ENVL_EEPROM;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> > > > -		return ENVL_FAT;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> > > > -		return ENVL_FLASH;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> > > > -		return ENVL_MMC;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> > > > -		return ENVL_NAND;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> > > > -		return ENVL_NVRAM;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> > > > -		return ENVL_REMOTE;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> > > > -		return ENVL_SPI_FLASH;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> > > > -		return ENVL_UBI;
> > > > -	else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> > > > -		return ENVL_NOWHERE;
> > > > -	else
> > > > -		return ENVL_UNKNOWN;
> > > > +	switch (op) {
> > > > +	case ENVO_GET_CHAR:
> > > > +	case ENVO_INIT:
> > > > +	case ENVO_LOAD:
> > > > +		if (prio >= ARRAY_SIZE(env_locations))
> > > > +			return -ENODEV;
> > > > +
> > > > +		return env_load_location = env_locations[prio];
> > > > +
> > > > +	case ENVO_SAVE:
> > > > +		return env_load_location;
> > > > +	}
> > > > +
> > > > +	return ENVL_UNKNOWN;
> > > >  }
> > > >  
> > > >  static struct env_driver *env_driver_lookup(enum env_operation
> > > > op, int prio)  
> > > 
> > > I'm just wondering...
> > > 
> > > Do you think that it may be helpful to have a way to force
> > > particular environment to be used (despite of the priority)?  
> > 
> > That's one of the hard parts of this, yes.  I think I had suggested
> > before that we handle that in a follow up series?
> > 
> 
> I don't want to say that this must be done now. I just can imagine that
> we may need such feature in some point.

So I guess this question is also part of the comment Lukasz had on the
other patch.

The current implementation is effectively hardcoding the environment
by doing two things:
  - Rejecting any priority other than 0, which is the highest. It
    basically says to the code that there is no further alternative
    and that its loop should stop there.
  - Returning the first environment in the array.

If one wanted to hardcode the environment to something else than the
default behaviour, you can always implement env_get_location in your
board by doing something like:

enum env_location env_get_location (enum env_operation op, int prio)
{
	if (prio)
		return ENVL_UNKNOWN;

	return ENVL_MMC; /* Or whatever you want */
}

Like I was saying, the default implementation already kind of does
that. Or maybe we want the default one to not hardcode it and just
deal with alternatives?

Thanks!
Maxime

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

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

[Lukasz Majewski]

Hi Maxime,

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

The env_get_location defined per board would work for me.

> 
> Like I was saying, the default implementation already kind of does
> that. Or maybe we want the default one to not hardcode it and just
> deal with alternatives?

I think that having the code hardcoded (as it is now) is acceptable for
making the feature way to main line. I suppose that
you would add the priorities handling in further patches?

> 
> Thanks!
> Maxime
> 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171117/09f8ddd4/attachment.sig>

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

[Maxime Ripard]

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

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

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

Maxime

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

[U-Boot] [RFC PATCH 02/10] env: Make env_driver_lookup_default private

[Andre Przywara]

Hi,

On 16/11/17 09:22, Maxime Ripard wrote:
> No that there's no users of env_driver_lookup_default outside of env/env.c,
> we can mark that function static and remove it from the environment header.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Looks good to me, but I believe you should merge this into the previous
patch. This makes it immediately obvious why we don't need this function
anymore.

Cheers,
Andre.

> ---
>  env/env.c             | 2 +-
>  include/environment.h | 7 -------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 84c12e27bc3f..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;
> diff --git a/include/environment.h b/include/environment.h
> index 7b9821638960..226e3ef2d23a 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -302,13 +302,6 @@ int env_export(env_t *env_out);
>  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
>   *
> 

[U-Boot] [RFC PATCH 04/10] env: Pass additional parameters to the env lookup function

[Quentin Schulz]

Hi Maxime,

On 16/11/2017 10:22, 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             | 122 ++++++++++++++++++++++++++++++--------------------
>  include/environment.h |   7 +++
>  2 files changed, 80 insertions(+), 49 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 97ada5b5a6fd..673bfa6ba41b 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
>  	return NULL;
>  }
>  
> -static enum env_location env_get_location(void)
> +static enum env_location env_get_location(enum env_operation op, int prio)
>  {
> +	if (prio >= 1)
> +		return ENVL_UNKNOWN;
> +
>  	if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
>  		return ENVL_EEPROM;
>  	else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> @@ -52,11 +55,14 @@ static enum env_location env_get_location(void)
>  		return ENVL_UNKNOWN;
>  }
>  
> -static struct env_driver *env_driver_lookup(void)
> +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 +75,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(ENVO_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(ENVO_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(ENVO_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(ENVO_INIT, prio)); prio++) {
> +		if (!drv->init)
> +			continue;
>  
> -	if (!drv)
> -		return -ENODEV;
> -	if (drv->init)
>  		ret = drv->init();
> +		if (!ret)
> +			return 0;
> +

Shouldn't we init all drivers that can be found?

I think it is perfectly plausible that an env that you could init could
fail to save/load its environment. Then we would try to save/load from
the next one but with the current code, the next one wouldn't be
initialized.

Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com