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

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

[Maxime Ripard]

Hi,

Here is an attempt at transitioning away from the MMC raw environment to a
FAT-based one. 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

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

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 (14):
  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: 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/common.c                      |   2 +-
 env/env.c                         | 198 +++++++++++++++++++------------
 env/fat.c                         |   9 +-
 env/mmc.c                         |   1 +-
 include/asm-generic/global_data.h |   1 +-
 include/environment.h             |  14 +-
 9 files changed, 192 insertions(+), 122 deletions(-)

base-commit: c253573f3e269fd9a24ee6684d87dd91106018a5
-- 
git-series 0.9.1

[U-Boot] [PATCH 01/14] 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: Lukasz Majewski <lukma@denx.de>
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 7b9821638960..226e3ef2d23a 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -303,13 +303,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] [PATCH 02/14] 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: Lukasz Majewski <lukma@denx.de>
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] [PATCH 03/14] 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;
-- 
git-series 0.9.1

[U-Boot] [PATCH 04/14] 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: Lukasz Majewski <lukma@denx.de>
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;
 
-- 
git-series 0.9.1

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

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

diff --git a/env/env.c b/env/env.c
index 1d13220aa79b..44f9908e2c2d 100644
--- a/env/env.c
+++ b/env/env.c
@@ -109,12 +109,11 @@ int env_load(void)
 		if (!drv->load)
 			continue;
 
+		printf("Loading Environment from %s... ", drv->name);
 		ret = drv->load();
+		printf("%s\n", ret ? "Failed" : "OK");
 		if (!ret)
 			return 0;
-
-		debug("%s: Environment %s failed to load (err=%d)\n", __func__,
-		      drv->name, ret);
 	}
 
 	return -ENODEV;
-- 
git-series 0.9.1

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

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

diff --git a/env/fat.c b/env/fat.c
index ec49c3905369..51c4cedda6be 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -55,7 +55,7 @@ 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",
+		printf("Unable to use %s %d:%d... ",
 		       CONFIG_ENV_FAT_INTERFACE, dev, part);
 		return 1;
 	}
@@ -63,12 +63,11 @@ 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",
+		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 +89,14 @@ 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",
+		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",
+		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] [PATCH 07/14] 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.

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 3f3092d97560..885e000307db 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -179,7 +179,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 08/14] 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.

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 70715bb6e756..fb66432bdc81 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\n");
 	}
 
 	if (himport_r(&env_htab, (char *)default_environment,
-- 
git-series 0.9.1

[U-Boot] [PATCH 09/14] 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 44f9908e2c2d..5176700133d3 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)
-- 
git-series 0.9.1

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c                         | 19 ++++++++++++-------
 include/asm-generic/global_data.h |  1 +
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/env/env.c b/env/env.c
index 5176700133d3..b4d8886e7a69 100644
--- a/env/env.c
+++ b/env/env.c
@@ -112,6 +112,9 @@ int env_get_char(int index)
 		if (!drv->get_char)
 			continue;
 
+		if (!(gd->env_has_init & BIT(drv->location)))
+			continue;
+
 		ret = drv->get_char(index);
 		if (!ret)
 			return 0;
@@ -134,6 +137,9 @@ int env_load(void)
 		if (!drv->load)
 			continue;
 
+		if (!(gd->env_has_init & BIT(drv->location)))
+			continue;
+
 		printf("Loading Environment from %s... ", drv->name);
 		ret = drv->load();
 		printf("%s\n", ret ? "Failed" : "OK");
@@ -155,6 +161,9 @@ int env_save(void)
 		if (!drv->save)
 			continue;
 
+		if (!(gd->env_has_init & BIT(drv->location)))
+			continue;
+
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
 		printf("%s\n", ret ? "Failed" : "OK");
@@ -175,14 +184,10 @@ int env_init(void)
 	int prio;
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
-		if (!drv->init)
-			continue;
-
-		ret = drv->init();
-		if (!ret)
-			return 0;
+		if (!drv->init || !drv->init())
+			gd->env_has_init |= BIT(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 944f58195caf..1d0611fe9498 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] [PATCH 11/14] 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: Lukasz Majewski <lukma@denx.de>
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
-- 
git-series 0.9.1

[U-Boot] [PATCH 12/14] 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: Lukasz Majewski <lukma@denx.de>
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 b4d8886e7a69..9b8b38cf3c2b 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:
-- 
git-series 0.9.1

[U-Boot] [PATCH 13/14] 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] [PATCH 14/14] 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: 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 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
-- 
git-series 0.9.1

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

[Quentin Schulz]

Hi Maxime,

On 28/11/2017 11:24, 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c                         | 19 ++++++++++++-------
>  include/asm-generic/global_data.h |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 5176700133d3..b4d8886e7a69 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -112,6 +112,9 @@ int env_get_char(int index)
>  		if (!drv->get_char)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		ret = drv->get_char(index);
>  		if (!ret)
>  			return 0;
> @@ -134,6 +137,9 @@ int env_load(void)
>  		if (!drv->load)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Loading Environment from %s... ", drv->name);
>  		ret = drv->load();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -155,6 +161,9 @@ int env_save(void)
>  		if (!drv->save)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Saving Environment to %s... ", drv->name);
>  		ret = drv->save();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -175,14 +184,10 @@ int env_init(void)
>  	int prio;
>  
>  	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> -		if (!drv->init)
> -			continue;
> -
> -		ret = drv->init();
> -		if (!ret)
> -			return 0;
> +		if (!drv->init || !drv->init())
> +			gd->env_has_init |= BIT(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);

You're not setting ret.

And you're printing that init env is always successful, and that's not
always the case as drv could have an init and call to drv->init() could
return an error code.

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

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

[Maxime Ripard]

On Tue, Nov 28, 2017 at 01:24:45PM +0100, Quentin Schulz wrote:
> Hi Maxime,
> 
> On 28/11/2017 11:24, 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.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  env/env.c                         | 19 ++++++++++++-------
> >  include/asm-generic/global_data.h |  1 +
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/env/env.c b/env/env.c
> > index 5176700133d3..b4d8886e7a69 100644
> > --- a/env/env.c
> > +++ b/env/env.c
> > @@ -112,6 +112,9 @@ int env_get_char(int index)
> >  		if (!drv->get_char)
> >  			continue;
> >  
> > +		if (!(gd->env_has_init & BIT(drv->location)))
> > +			continue;
> > +
> >  		ret = drv->get_char(index);
> >  		if (!ret)
> >  			return 0;
> > @@ -134,6 +137,9 @@ int env_load(void)
> >  		if (!drv->load)
> >  			continue;
> >  
> > +		if (!(gd->env_has_init & BIT(drv->location)))
> > +			continue;
> > +
> >  		printf("Loading Environment from %s... ", drv->name);
> >  		ret = drv->load();
> >  		printf("%s\n", ret ? "Failed" : "OK");
> > @@ -155,6 +161,9 @@ int env_save(void)
> >  		if (!drv->save)
> >  			continue;
> >  
> > +		if (!(gd->env_has_init & BIT(drv->location)))
> > +			continue;
> > +
> >  		printf("Saving Environment to %s... ", drv->name);
> >  		ret = drv->save();
> >  		printf("%s\n", ret ? "Failed" : "OK");
> > @@ -175,14 +184,10 @@ int env_init(void)
> >  	int prio;
> >  
> >  	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> > -		if (!drv->init)
> > -			continue;
> > -
> > -		ret = drv->init();
> > -		if (!ret)
> > -			return 0;
> > +		if (!drv->init || !drv->init())
> > +			gd->env_has_init |= BIT(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);
> 
> You're not setting ret.

Ah, right.

> And you're printing that init env is always successful

Not really. The message says that it's been done, not that it's been
done successfully.

> and that's not always the case as drv could have an init and call to
> drv->init() could return an error code.

Right, but I'm not really sure what could be done about that error
code beside printing it.

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/20171128/c7a75737/attachment.sig>

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

[Andre Przywara]

On 28/11/17 10:24, 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: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  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 7b9821638960..226e3ef2d23a 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -303,13 +303,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
> 

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

[Andre Przywara]

On 28/11/17 10:24, 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: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

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

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

[Andre Przywara]

Hi Maxime,

On 28/11/17 10:24, 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.

I wonder if this is a bit too generic. Wouldn't a simple:
	bool is_save_op (or some more clever name)
be sufficient? At least this is actually all we need, if I understand
the code correctly.
But it's up to you, I can see advantages for both approaches.

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

Did you deliberately remove this fallback from the new code below?

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

Is this change to the algorithm acceptable? Formerly we returned 0 on
finding a drv->load pointer to be NULL, now it's -ENODEV, plus we try to
find other sources first.
Or is that solved later somehow with the env_has_init bitmap?

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

Same as for env_load().

Cheers,
Andre

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

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

[Andre Przywara]

On 28/11/17 10:24, 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: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

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

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

[Andre Przywara]

On 28/11/17 10:24, 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 1d13220aa79b..44f9908e2c2d 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -109,12 +109,11 @@ int env_load(void)
>  		if (!drv->load)
>  			continue;
>  
> +		printf("Loading Environment from %s... ", drv->name);
>  		ret = drv->load();
> +		printf("%s\n", ret ? "Failed" : "OK");

This looses the error code that was printed with debug() below. Might be
worth to keep it? Either always or as a debug?
		printf("%s (%d)\n", ...);

Cheers,
Andre.

>  		if (!ret)
>  			return 0;
> -
> -		debug("%s: Environment %s failed to load (err=%d)\n", __func__,
> -		      drv->name, ret);
>  	}
>  
>  	return -ENODEV;
> 

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

[Andre Przywara]

On 28/11/17 10:24, 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

One nit below...

> ---
>  env/fat.c |  9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/env/fat.c b/env/fat.c
> index ec49c3905369..51c4cedda6be 100644
> --- a/env/fat.c
> +++ b/env/fat.c
> @@ -55,7 +55,7 @@ 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",
> +		printf("Unable to use %s %d:%d... ",

It might be worth to note here that those prints are embedded in
messages from the caller, which takes care of the linefeed.

Cheers,
Andre.

>  		       CONFIG_ENV_FAT_INTERFACE, dev, part);
>  		return 1;
>  	}
> @@ -63,12 +63,11 @@ 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",
> +		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 +89,14 @@ 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",
> +		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",
> +		printf("Unable to read \"%s\" from %s%d:%d... ",
>  			CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part);
>  		goto err_env_relocate;
>  	}
> 

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

[Andre Przywara]

On 28/11/17 10:24, 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  env/mmc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 3f3092d97560..885e000307db 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -179,7 +179,6 @@ static int env_mmc_save(void)
>  		goto fini;
>  	}
>  
> -	puts("done\n");
>  	ret = 0;
>  
>  #ifdef CONFIG_ENV_OFFSET_REDUND
> 

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

[Andre Przywara]

On 28/11/17 10:24, 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

One nit below ...

> ---
>  env/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/env/common.c b/env/common.c
> index 70715bb6e756..fb66432bdc81 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\n");

Do we still need the extra empty line?

Cheers,
Andre

>  	}
>  
>  	if (himport_r(&env_htab, (char *)default_environment,
> 

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

[Andre Przywara]

On 28/11/17 10:24, 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.
> 
> 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 44f9908e2c2d..5176700133d3 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];

That looks a bit fishy. Can you please make this two lines?

Otherwise: Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> +
> +	case ENVO_SAVE:
> +		return env_load_location;
> +	}
> +
> +	return ENVL_UNKNOWN;
>  }
>  
>  static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> 

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

[Andre Przywara]

Hi,

On 28/11/17 10:24, 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c                         | 19 ++++++++++++-------
>  include/asm-generic/global_data.h |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 5176700133d3..b4d8886e7a69 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -112,6 +112,9 @@ int env_get_char(int index)
>  		if (!drv->get_char)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		ret = drv->get_char(index);
>  		if (!ret)
>  			return 0;
> @@ -134,6 +137,9 @@ int env_load(void)
>  		if (!drv->load)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Loading Environment from %s... ", drv->name);
>  		ret = drv->load();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -155,6 +161,9 @@ int env_save(void)
>  		if (!drv->save)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Saving Environment to %s... ", drv->name);
>  		ret = drv->save();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -175,14 +184,10 @@ int env_init(void)
>  	int prio;
>  

Becoming a bit desperate now, because I am struggling to find actual
issues in this series ;-), so:

Maybe we should have a build time check should the env_locations array
ever grow beyond 32?
	BUILD_BUG_ON(ARRAY_SIZE(env_locations) > BITS_PER_LONG);

>  	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> -		if (!drv->init)
> -			continue;
> -
> -		ret = drv->init();
> -		if (!ret)
> -			return 0;
> +		if (!drv->init || !drv->init())
> +			gd->env_has_init |= BIT(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 944f58195caf..1d0611fe9498 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 */

struct env_location? Do you mean the env_location array? Or the enum?

Cheers,
Andre.

>  	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
>  	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
> 

[U-Boot] [PATCH 11/14] env: Allow to build multiple environments in Kconfig

[Andre Przywara]

Hi,

On 28/11/17 10:24, 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: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Looks good, but doesn't apply cleanly to mainline anymore, because
CONFIG_ENV_AES was removed meanwhile (c6831c74a9e9).

Otherwise:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

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

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Andre Przywara]

Hi,

On 28/11/17 10:24, Maxime Ripard wrote:
> Allow boards and architectures to override the default environment lookup
> code by overriding env_get_location.
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  env/env.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/env/env.c b/env/env.c
> index b4d8886e7a69..9b8b38cf3c2b 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:
> 

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

[Andre Przywara]

Hi,

On 28/11/17 10:24, 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>
> ---
>  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;

So even though the actual u-boot.bin for 64-bit boards is still somewhat
below the limit (~480KB), adding the ATF image (~32KB) pushes it over
the edge. So since v2017.11 u-boot.itb is already too big for the
traditional MMC env location.
So shall this "case 1:" be guarded by #ifndef CONFIG_ARM64, to not even
consider MMC for sunxi64 anymore?

Cheers,
Andre.

> +
> +	default:
> +		return ENVL_UNKNOWN;
> +	}
> +}
> +#endif
> +
>  /* add board specific code here */
>  int board_init(void)
>  {
> 

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

[Andre Przywara]

Hi,

On 28/11/17 10:24, Maxime Ripard wrote:
> Now that we have everything in place to implement the transition scheme,
> let's enable it by default.
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

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

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

[Tom Rini]

On Tue, Nov 28, 2017 at 11:24:35AM +0100, Maxime Ripard wrote:

> Hi,
> 
> Here is an attempt at transitioning away from the MMC raw environment to a
> FAT-based one. 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
> 
> 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,

I think this is the right direction to head in.  Can you please address
the few outstanding points / questions and have something posted shortly
after v2018.01 releases, and I'll apply it for inclusion in v2018.03?
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/20171207/5d4e52c6/attachment.sig>

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

[Maxime Ripard]

Hi Andre,

On Tue, Dec 05, 2017 at 10:09:04AM +0000, Andre Przywara wrote:
> On 28/11/17 10:24, 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.
> 
> I wonder if this is a bit too generic. Wouldn't a simple:
> 	bool is_save_op (or some more clever name)
> be sufficient? At least this is actually all we need, if I understand
> the code correctly.
> But it's up to you, I can see advantages for both approaches.

I'm not really sure what you meant, but I'll try to explain the
various constraints we have:

  - We need to init all the environments, no matter their priority,
    without any particular ordering

  - We need to load the environments by descending order of priority,
    until a valid one is found. I guess get_char is in the same
    situation, but I'm not really sure what it's supposed to be doing.

  - We need to store the environments. Now, this gets tricky. The
    current code only implement two cases: you store the environment
    where you loaded it from, which is the current case, and forcing
    always the same order, like we did on sunxi.

    But you might envision without pulling your hairs too much a
    situation for example that we would have loaded our environment
    from a read-only location (either because it is physically
    read-only, or because you cannot modify it (ie. signed
    environments)). In this case, you will need to implement a
    fallback strategy that is probably going to be by board, probably
    along the line of trying to save in our environments by ascending
    order, starting with the environment we loaded from.

tl; dr: we might need that flexibility for future use cases that are
not clear yet.

> >  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;
> 
> Is this change to the algorithm acceptable? Formerly we returned 0 on
> finding a drv->load pointer to be NULL, now it's -ENODEV, plus we try to
> find other sources first.

I couldn't really find a better way to deal with this.

You're right that the current algorithm is that if we don't have a
driver, return ENODEV, and if we don't have a load function, return 0.

When dealing with multiple enviroments, you would obviously have to
return -ENODEV if you didn't find any, but I couldn't find a proper
way to decide when we would return 0: should we return 0 when no
environment has a load function? only the first one we encounter?

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/20171208/c6622849/attachment.sig>

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

[Maxime Ripard]

Hi, Andre,

On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> Hi,
> 
> On 28/11/17 10:24, 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>
> > ---
> >  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;
> 
> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> the edge. So since v2017.11 u-boot.itb is already too big for the
> traditional MMC env location.

Even with the alignment fix you merged recently?

> So shall this "case 1:" be guarded by #ifndef CONFIG_ARM64, to not even
> consider MMC for sunxi64 anymore?

I guess it's even simpler: you just don't enable ENV_IS_IN_MMC if
ARM64 is set :)

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/20171208/6aeecc9d/attachment.sig>

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

[Maxime Ripard]

Hi Tom,

On Thu, Dec 07, 2017 at 03:09:31PM -0500, Tom Rini wrote:
> > Here is an attempt at transitioning away from the MMC raw environment to a
> > FAT-based one. 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
> > 
> > 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,
> 
> I think this is the right direction to head in.  Can you please address
> the few outstanding points / questions

This is done already.

> and have something posted shortly after v2018.01 releases, and I'll
> apply it for inclusion in v2018.03?

And I was waiting for your feedback :)

I'll do that, 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: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171208/6d26d680/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> the edge. So since v2017.11 u-boot.itb is already too big for the
> traditional MMC env location.

So I've had a quick look about what could go possibly go away in our
current armv8 config (using the pine64+ defconfig). Let me know if
some are actually vitals:

 - FIT_ENABLE_SHA256_SUPPORT
 - CONSOLE_MUX
 - CMD_CRC32
 - CMD_LZMADEC
 - CMD_UNZIP
 - CMD_LOADB
 - CMD_LOADS
 - CMD_MISC (actually implementing the command sleep)
 - ISO_PARTITION (yes. For CDROMs.)
 - VIDEO_BPP8, VIDEO_BPP16
 - VIDEO_ANSI
 - SHA256
 - LZMA

Removing those options make the u-boot.itb binary size going from
516kB to 478kB, making it functional again *and* allowing us to enable
the DT overlays that seem way more important than any feature
mentionned above (and bumps the size to 483kB).

Let me know what you think,
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/20171219/7898e986/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Tom Rini]

On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > the edge. So since v2017.11 u-boot.itb is already too big for the
> > traditional MMC env location.
> 
> So I've had a quick look about what could go possibly go away in our
> current armv8 config (using the pine64+ defconfig). Let me know if
> some are actually vitals:
> 
>  - FIT_ENABLE_SHA256_SUPPORT
>  - CONSOLE_MUX
>  - CMD_CRC32
>  - CMD_LZMADEC
>  - CMD_UNZIP
>  - CMD_LOADB
>  - CMD_LOADS
>  - CMD_MISC (actually implementing the command sleep)
>  - ISO_PARTITION (yes. For CDROMs.)
>  - VIDEO_BPP8, VIDEO_BPP16
>  - VIDEO_ANSI
>  - SHA256
>  - LZMA
> 
> Removing those options make the u-boot.itb binary size going from
> 516kB to 478kB, making it functional again *and* allowing us to enable
> the DT overlays that seem way more important than any feature
> mentionned above (and bumps the size to 483kB).

You want to keep sha256 support in FIT images, we only have dropping
that allowed for some very odd use-cases.  And while I don't have a
strong opinion about unzip, lzma is one of the valid/likelu compressions
for an Image (and aside, I, or someone, needs to look into having
'booti' recognize various compression algorithms and automatically
decompress them) so I don't think we should get rid of that.

-- 
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/20171219/95c002f0/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

1;5002;0c
On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
> On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > traditional MMC env location.
> > 
> > So I've had a quick look about what could go possibly go away in our
> > current armv8 config (using the pine64+ defconfig). Let me know if
> > some are actually vitals:
> > 
> >  - FIT_ENABLE_SHA256_SUPPORT
> >  - CONSOLE_MUX
> >  - CMD_CRC32
> >  - CMD_LZMADEC
> >  - CMD_UNZIP
> >  - CMD_LOADB
> >  - CMD_LOADS
> >  - CMD_MISC (actually implementing the command sleep)
> >  - ISO_PARTITION (yes. For CDROMs.)
> >  - VIDEO_BPP8, VIDEO_BPP16
> >  - VIDEO_ANSI
> >  - SHA256
> >  - LZMA
> > 
> > Removing those options make the u-boot.itb binary size going from
> > 516kB to 478kB, making it functional again *and* allowing us to enable
> > the DT overlays that seem way more important than any feature
> > mentionned above (and bumps the size to 483kB).
> 
> You want to keep sha256 support in FIT images, we only have dropping
> that allowed for some very odd use-cases.  And while I don't have a
> strong opinion about unzip, lzma is one of the valid/likelu compressions
> for an Image (and aside, I, or someone, needs to look into having
> 'booti' recognize various compression algorithms and automatically
> decompress them) so I don't think we should get rid of that.

So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
499kB. Still tight, but it fits.

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/20171219/dd813a75/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Alexander Graf]

On 12/19/2017 02:12 PM, Maxime Ripard wrote:
> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>> the edge. So since v2017.11 u-boot.itb is already too big for the
>> traditional MMC env location.
> So I've had a quick look about what could go possibly go away in our
> current armv8 config (using the pine64+ defconfig). Let me know if
> some are actually vitals:
>
>   - FIT_ENABLE_SHA256_SUPPORT
>   - CONSOLE_MUX
>   - CMD_CRC32

We use this in travis tests, but since no Allwinner systems are present 
there, I guess we don't care.

>   - CMD_LZMADEC
>   - CMD_UNZIP
>   - CMD_LOADB
>   - CMD_LOADS
>   - CMD_MISC (actually implementing the command sleep)
>   - ISO_PARTITION (yes. For CDROMs.)

This one is needed to boot openSUSE or SLES images, since they come as 
.iso with el torito (which then gets booted using distro + bootefi).

>   - VIDEO_BPP8, VIDEO_BPP16
>   - VIDEO_ANSI
>   - SHA256
>   - LZMA
>
> Removing those options make the u-boot.itb binary size going from
> 516kB to 478kB, making it functional again *and* allowing us to enable
> the DT overlays that seem way more important than any feature
> mentionned above (and bumps the size to 483kB).
>
> Let me know what you think,

Out of the ones above, only ISO_PARTITION is the one I would definitely 
care about; please don't remove that one ;)


Alex

[U-Boot] ARM64 Allwinner Binary Size

[Tom Rini]

On Tue, Dec 19, 2017 at 02:26:40PM +0100, Maxime Ripard wrote:
> 1;5002;0c
> On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
> > On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> > > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > > traditional MMC env location.
> > > 
> > > So I've had a quick look about what could go possibly go away in our
> > > current armv8 config (using the pine64+ defconfig). Let me know if
> > > some are actually vitals:
> > > 
> > >  - FIT_ENABLE_SHA256_SUPPORT
> > >  - CONSOLE_MUX
> > >  - CMD_CRC32
> > >  - CMD_LZMADEC
> > >  - CMD_UNZIP
> > >  - CMD_LOADB
> > >  - CMD_LOADS
> > >  - CMD_MISC (actually implementing the command sleep)
> > >  - ISO_PARTITION (yes. For CDROMs.)
> > >  - VIDEO_BPP8, VIDEO_BPP16
> > >  - VIDEO_ANSI
> > >  - SHA256
> > >  - LZMA
> > > 
> > > Removing those options make the u-boot.itb binary size going from
> > > 516kB to 478kB, making it functional again *and* allowing us to enable
> > > the DT overlays that seem way more important than any feature
> > > mentionned above (and bumps the size to 483kB).
> > 
> > You want to keep sha256 support in FIT images, we only have dropping
> > that allowed for some very odd use-cases.  And while I don't have a
> > strong opinion about unzip, lzma is one of the valid/likelu compressions
> > for an Image (and aside, I, or someone, needs to look into having
> > 'booti' recognize various compression algorithms and automatically
> > decompress them) so I don't think we should get rid of that.
> 
> So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
> 499kB. Still tight, but it fits.

Looking over myself, perhaps drop CMD_BOOTD and move LOGLEVEL to 3?

-- 
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/20171219/1819fc8e/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Tom Rini]

On Tue, Dec 19, 2017 at 02:28:03PM +0100, Alexander Graf wrote:
> On 12/19/2017 02:12 PM, Maxime Ripard wrote:
> >On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> >>So even though the actual u-boot.bin for 64-bit boards is still somewhat
> >>below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> >>the edge. So since v2017.11 u-boot.itb is already too big for the
> >>traditional MMC env location.
> >So I've had a quick look about what could go possibly go away in our
> >current armv8 config (using the pine64+ defconfig). Let me know if
> >some are actually vitals:
> >
> >  - FIT_ENABLE_SHA256_SUPPORT
> >  - CONSOLE_MUX
> >  - CMD_CRC32
> 
> We use this in travis tests, but since no Allwinner systems are present
> there, I guess we don't care.

Note that tests that require CMD_CRC32 should be marked as such too.

-- 
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/20171219/96cd8952/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Andre Przywara]

Hi Maxime,

thanks for having a look!

On 19/12/17 13:12, Maxime Ripard wrote:
> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>> the edge. So since v2017.11 u-boot.itb is already too big for the
>> traditional MMC env location.
> 
> So I've had a quick look about what could go possibly go away in our
> current armv8 config (using the pine64+ defconfig). Let me know if
> some are actually vitals:
> 
>  - FIT_ENABLE_SHA256_SUPPORT
>  - CONSOLE_MUX
>  - CMD_CRC32
>  - CMD_LZMADEC
>  - CMD_UNZIP
>  - CMD_LOADB
>  - CMD_LOADS
>  - CMD_MISC (actually implementing the command sleep)
>  - ISO_PARTITION (yes. For CDROMs.)

As Alex mentioned, this is needed for some installer images, which come
as ISOs. So if possible, we should keep this in.

>  - VIDEO_BPP8, VIDEO_BPP16
>  - VIDEO_ANSI
>  - SHA256
>  - LZMA

From just looking at the names I am fine with the rest gone. But let me
test tonight if there are any side effects.

Some of them seem useful, but I would leave enabling them to the actual
users. If someone needs it, they can enable them and loose the raw MMC
environment. I think this is a fair trade-off.

> Removing those options make the u-boot.itb binary size going from
> 516kB to 478kB, making it functional again *and* allowing us to enable
> the DT overlays that seem way more important than any feature
> mentionned above (and bumps the size to 483kB).

How important is the raw MMC environment for the ARM64 boards, actually?
Most of the rationale for the 32-bit side seemed to apply to legacy use
cases only. Do we have reports/complaints from 64-bit users?

Cheers,
Andre.

[U-Boot] ARM64 Allwinner Binary Size

[Mark Kettenis]

> Date: Tue, 19 Dec 2017 14:12:02 +0100
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > the edge. So since v2017.11 u-boot.itb is already too big for the
> > traditional MMC env location.
> 
> So I've had a quick look about what could go possibly go away in our
> current armv8 config (using the pine64+ defconfig). Let me know if
> some are actually vitals:
> 
>  - FIT_ENABLE_SHA256_SUPPORT
>  - CONSOLE_MUX
>  - CMD_CRC32
>  - CMD_LZMADEC
>  - CMD_UNZIP
>  - CMD_LOADB
>  - CMD_LOADS
>  - CMD_MISC (actually implementing the command sleep)
>  - ISO_PARTITION (yes. For CDROMs.)
>  - VIDEO_BPP8, VIDEO_BPP16
>  - VIDEO_ANSI
>  - SHA256
>  - LZMA
> 
> Removing those options make the u-boot.itb binary size going from
> 516kB to 478kB, making it functional again *and* allowing us to enable
> the DT overlays that seem way more important than any feature
> mentionned above (and bumps the size to 483kB).

So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
console and usb keyboard are present?  Is the usb keyboard used when
it is plugged in but serial otherwise?

In any case, this would be just a stopgap measure for 2018.01 wouldn't
it?  We really have to move to the FAT-based environment or just bite
the bullet and move the location of the environment.  Personally I
don't cutsomize my environment beyond setting boot_targets.  So losing
my old settings upon upgrade is not a big issue for me.

[U-Boot] ARM64 Allwinner Binary Size

[Mark Kettenis]

> From: Andre Przywara <andre.przywara@arm.com>
> Date: Tue, 19 Dec 2017 13:38:59 +0000
> 
> Hi Maxime,
> 
> thanks for having a look!
> 
> On 19/12/17 13:12, Maxime Ripard wrote:
> > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> >> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> >> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> >> the edge. So since v2017.11 u-boot.itb is already too big for the
> >> traditional MMC env location.
> > 
> > So I've had a quick look about what could go possibly go away in our
> > current armv8 config (using the pine64+ defconfig). Let me know if
> > some are actually vitals:
> > 
> >  - FIT_ENABLE_SHA256_SUPPORT
> >  - CONSOLE_MUX
> >  - CMD_CRC32
> >  - CMD_LZMADEC
> >  - CMD_UNZIP
> >  - CMD_LOADB
> >  - CMD_LOADS
> >  - CMD_MISC (actually implementing the command sleep)
> >  - ISO_PARTITION (yes. For CDROMs.)
> 
> As Alex mentioned, this is needed for some installer images, which come
> as ISOs. So if possible, we should keep this in.
> 
> >  - VIDEO_BPP8, VIDEO_BPP16
> >  - VIDEO_ANSI
> >  - SHA256
> >  - LZMA
> 
> From just looking at the names I am fine with the rest gone. But let me
> test tonight if there are any side effects.
> 
> Some of them seem useful, but I would leave enabling them to the actual
> users. If someone needs it, they can enable them and loose the raw MMC
> environment. I think this is a fair trade-off.
> 
> > Removing those options make the u-boot.itb binary size going from
> > 516kB to 478kB, making it functional again *and* allowing us to enable
> > the DT overlays that seem way more important than any feature
> > mentionned above (and bumps the size to 483kB).
> 
> How important is the raw MMC environment for the ARM64 boards, actually?
> Most of the rationale for the 32-bit side seemed to apply to legacy use
> cases only. Do we have reports/complaints from 64-bit users?

For me/us (OpenBSD) the environment is still important.  I have many
setups where U-Boot lives on a uSD card but the installed OS lives on
a USB device.  In that scenario I set boot_targets to boot the EFI
bootloader and OS off the USB disk.  This is very helpfull for testing
new versions of U-Boot as I can simply swap the uSD card.  But for
some setups this is essential as OpenBSD doesn't support the SD/MCC
controller on all ARM hardware yet (but we do support it on
Allwinner).

Cheers,

Mark

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 19, 2017 at 08:30:31AM -0500, Tom Rini wrote:
> On Tue, Dec 19, 2017 at 02:26:40PM +0100, Maxime Ripard wrote:
> > 1;5002;0c
> > On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
> > > On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> > > > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > > > traditional MMC env location.
> > > > 
> > > > So I've had a quick look about what could go possibly go away in our
> > > > current armv8 config (using the pine64+ defconfig). Let me know if
> > > > some are actually vitals:
> > > > 
> > > >  - FIT_ENABLE_SHA256_SUPPORT
> > > >  - CONSOLE_MUX
> > > >  - CMD_CRC32
> > > >  - CMD_LZMADEC
> > > >  - CMD_UNZIP
> > > >  - CMD_LOADB
> > > >  - CMD_LOADS
> > > >  - CMD_MISC (actually implementing the command sleep)
> > > >  - ISO_PARTITION (yes. For CDROMs.)
> > > >  - VIDEO_BPP8, VIDEO_BPP16
> > > >  - VIDEO_ANSI
> > > >  - SHA256
> > > >  - LZMA
> > > > 
> > > > Removing those options make the u-boot.itb binary size going from
> > > > 516kB to 478kB, making it functional again *and* allowing us to enable
> > > > the DT overlays that seem way more important than any feature
> > > > mentionned above (and bumps the size to 483kB).
> > > 
> > > You want to keep sha256 support in FIT images, we only have dropping
> > > that allowed for some very odd use-cases.  And while I don't have a
> > > strong opinion about unzip, lzma is one of the valid/likelu compressions
> > > for an Image (and aside, I, or someone, needs to look into having
> > > 'booti' recognize various compression algorithms and automatically
> > > decompress them) so I don't think we should get rid of that.
> > 
> > So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
> > 499kB. Still tight, but it fits.
> 
> Looking over myself, perhaps drop CMD_BOOTD and move LOGLEVEL to 3?

And we're at 498kB now! :)

Is CMD_ELF useful as well?

Maxime

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

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 19, 2017 at 02:28:03PM +0100, Alexander Graf wrote:
> On 12/19/2017 02:12 PM, Maxime Ripard wrote:
> >   - VIDEO_BPP8, VIDEO_BPP16
> >   - VIDEO_ANSI
> >   - SHA256
> >   - LZMA
> > 
> > Removing those options make the u-boot.itb binary size going from
> > 516kB to 478kB, making it functional again *and* allowing us to enable
> > the DT overlays that seem way more important than any feature
> > mentionned above (and bumps the size to 483kB).
> > 
> > Let me know what you think,
> 
> Out of the ones above, only ISO_PARTITION is the one I would definitely care
> about; please don't remove that one ;)

Well, I guess you're never done with the 90s ;)

And who knows, maybe it'll be hype again in a few years :)

Maxime

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

[U-Boot] ARM64 Allwinner Binary Size

[Emmanuel Vadot]

On Tue, 19 Dec 2017 15:09:52 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> On Tue, Dec 19, 2017 at 08:30:31AM -0500, Tom Rini wrote:
> > On Tue, Dec 19, 2017 at 02:26:40PM +0100, Maxime Ripard wrote:
> > > 1;5002;0c
> > > On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
> > > > On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> > > > > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > > > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > > > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > > > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > > > > traditional MMC env location.
> > > > > 
> > > > > So I've had a quick look about what could go possibly go away in our
> > > > > current armv8 config (using the pine64+ defconfig). Let me know if
> > > > > some are actually vitals:
> > > > > 
> > > > >  - FIT_ENABLE_SHA256_SUPPORT
> > > > >  - CONSOLE_MUX
> > > > >  - CMD_CRC32
> > > > >  - CMD_LZMADEC
> > > > >  - CMD_UNZIP
> > > > >  - CMD_LOADB
> > > > >  - CMD_LOADS
> > > > >  - CMD_MISC (actually implementing the command sleep)
> > > > >  - ISO_PARTITION (yes. For CDROMs.)
> > > > >  - VIDEO_BPP8, VIDEO_BPP16
> > > > >  - VIDEO_ANSI
> > > > >  - SHA256
> > > > >  - LZMA
> > > > > 
> > > > > Removing those options make the u-boot.itb binary size going from
> > > > > 516kB to 478kB, making it functional again *and* allowing us to enable
> > > > > the DT overlays that seem way more important than any feature
> > > > > mentionned above (and bumps the size to 483kB).
> > > > 
> > > > You want to keep sha256 support in FIT images, we only have dropping
> > > > that allowed for some very odd use-cases.  And while I don't have a
> > > > strong opinion about unzip, lzma is one of the valid/likelu compressions
> > > > for an Image (and aside, I, or someone, needs to look into having
> > > > 'booti' recognize various compression algorithms and automatically
> > > > decompress them) so I don't think we should get rid of that.
> > > 
> > > So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
> > > 499kB. Still tight, but it fits.
> > 
> > Looking over myself, perhaps drop CMD_BOOTD and move LOGLEVEL to 3?
> 
> And we're at 498kB now! :)
> 
> Is CMD_ELF useful as well?

 Speaking for the BSDs, FreeBSD and OpenBSD are using EFI, NetBSD is
using uImage, CMD_ELF <might> be useful for test but people who run
test on u-boot should know how to configure it.

> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


-- 
Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>

[U-Boot] ARM64 Allwinner Binary Size

[Andre Przywara]

Hi,

On 19/12/17 13:51, Mark Kettenis wrote:
>> From: Andre Przywara <andre.przywara@arm.com>
>> Date: Tue, 19 Dec 2017 13:38:59 +0000
>>
>> Hi Maxime,
>>
>> thanks for having a look!
>>
>> On 19/12/17 13:12, Maxime Ripard wrote:
>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>> traditional MMC env location.
>>>
>>> So I've had a quick look about what could go possibly go away in our
>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>> some are actually vitals:
>>>
>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>  - CONSOLE_MUX
>>>  - CMD_CRC32
>>>  - CMD_LZMADEC
>>>  - CMD_UNZIP
>>>  - CMD_LOADB
>>>  - CMD_LOADS
>>>  - CMD_MISC (actually implementing the command sleep)
>>>  - ISO_PARTITION (yes. For CDROMs.)
>>
>> As Alex mentioned, this is needed for some installer images, which come
>> as ISOs. So if possible, we should keep this in.
>>
>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>  - VIDEO_ANSI
>>>  - SHA256
>>>  - LZMA
>>
>> From just looking at the names I am fine with the rest gone. But let me
>> test tonight if there are any side effects.
>>
>> Some of them seem useful, but I would leave enabling them to the actual
>> users. If someone needs it, they can enable them and loose the raw MMC
>> environment. I think this is a fair trade-off.
>>
>>> Removing those options make the u-boot.itb binary size going from
>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>> the DT overlays that seem way more important than any feature
>>> mentionned above (and bumps the size to 483kB).
>>
>> How important is the raw MMC environment for the ARM64 boards, actually?
>> Most of the rationale for the 32-bit side seemed to apply to legacy use
>> cases only. Do we have reports/complaints from 64-bit users?
> 
> For me/us (OpenBSD) the environment is still important.  I have many
> setups where U-Boot lives on a uSD card but the installed OS lives on
> a USB device.  In that scenario I set boot_targets to boot the EFI
> bootloader and OS off the USB disk.  This is very helpfull for testing
> new versions of U-Boot as I can simply swap the uSD card.  But for
> some setups this is essential as OpenBSD doesn't support the SD/MCC
> controller on all ARM hardware yet (but we do support it on
> Allwinner).

I see, but I wasn't arguing for dropping the environment altogether,
more for supporting FAT environments *only*.
So how important is preserving existing environments over a firmware
update in your scenario? I think this is the killer question here, isn't
it? I'm inclined to just drop raw MMC environment support from sunxi64
boards and then enjoy the ~450KB more worth of code, until we hit the
first MB boundary.
I have builds with all (DDR3) A64 board DTs in the binary [1], which
would be larger than 504K anyway.

Cheers,
Andre.

[1] https://github.com/apritzel/pine64/commit/ee12bea43

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 19, 2017 at 01:38:59PM +0000, Andre Przywara wrote:
> Hi Maxime,
> 
> thanks for having a look!
> 
> On 19/12/17 13:12, Maxime Ripard wrote:
> > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> >> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> >> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> >> the edge. So since v2017.11 u-boot.itb is already too big for the
> >> traditional MMC env location.
> > 
> > So I've had a quick look about what could go possibly go away in our
> > current armv8 config (using the pine64+ defconfig). Let me know if
> > some are actually vitals:
> > 
> >  - FIT_ENABLE_SHA256_SUPPORT
> >  - CONSOLE_MUX
> >  - CMD_CRC32
> >  - CMD_LZMADEC
> >  - CMD_UNZIP
> >  - CMD_LOADB
> >  - CMD_LOADS
> >  - CMD_MISC (actually implementing the command sleep)
> >  - ISO_PARTITION (yes. For CDROMs.)
> 
> As Alex mentioned, this is needed for some installer images, which come
> as ISOs. So if possible, we should keep this in.

So, with FIT_ENABLE_SHA256_SUPPORT, LZMADEC, ISO_PARTITION and the
overlay support, we're at 500kB.

Again, tight, but under the limit.

> >  - VIDEO_BPP8, VIDEO_BPP16
> >  - VIDEO_ANSI
> >  - SHA256
> >  - LZMA
> 
> From just looking at the names I am fine with the rest gone. But let me
> test tonight if there are any side effects.
> 
> Some of them seem useful, but I would leave enabling them to the actual
> users. If someone needs it, they can enable them and loose the raw MMC
> environment. I think this is a fair trade-off.

Yes, that's my view too.

> > Removing those options make the u-boot.itb binary size going from
> > 516kB to 478kB, making it functional again *and* allowing us to enable
> > the DT overlays that seem way more important than any feature
> > mentionned above (and bumps the size to 483kB).
> 
> How important is the raw MMC environment for the ARM64 boards, actually?
> Most of the rationale for the 32-bit side seemed to apply to legacy use
> cases only. Do we have reports/complaints from 64-bit users?

Pretty much as important as it is on arm I guess. We just have less
history, but the same use cases.

I'd really like to give at least one release for transition, which
would mean having a schedule like this:

  - in 2018.01, merge those config removals so that we have least have
    something that works quite fast

  - in 2018.03, merge the multiple environment patches. We seem to
    have reached a consensus here, so I'm not really concerned that we
    will have it merged.

  - in 2018.05, if needed, remove the raw MMC options and complete the
    transition to FAT.

Maxime

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

[U-Boot] ARM64 Allwinner Binary Size

[Tom Rini]

On Tue, Dec 19, 2017 at 03:09:52PM +0100, Maxime Ripard wrote:
> On Tue, Dec 19, 2017 at 08:30:31AM -0500, Tom Rini wrote:
> > On Tue, Dec 19, 2017 at 02:26:40PM +0100, Maxime Ripard wrote:
> > > 1;5002;0c
> > > On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
> > > > On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> > > > > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > > > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > > > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > > > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > > > > traditional MMC env location.
> > > > > 
> > > > > So I've had a quick look about what could go possibly go away in our
> > > > > current armv8 config (using the pine64+ defconfig). Let me know if
> > > > > some are actually vitals:
> > > > > 
> > > > >  - FIT_ENABLE_SHA256_SUPPORT
> > > > >  - CONSOLE_MUX
> > > > >  - CMD_CRC32
> > > > >  - CMD_LZMADEC
> > > > >  - CMD_UNZIP
> > > > >  - CMD_LOADB
> > > > >  - CMD_LOADS
> > > > >  - CMD_MISC (actually implementing the command sleep)
> > > > >  - ISO_PARTITION (yes. For CDROMs.)
> > > > >  - VIDEO_BPP8, VIDEO_BPP16
> > > > >  - VIDEO_ANSI
> > > > >  - SHA256
> > > > >  - LZMA
> > > > > 
> > > > > Removing those options make the u-boot.itb binary size going from
> > > > > 516kB to 478kB, making it functional again *and* allowing us to enable
> > > > > the DT overlays that seem way more important than any feature
> > > > > mentionned above (and bumps the size to 483kB).
> > > > 
> > > > You want to keep sha256 support in FIT images, we only have dropping
> > > > that allowed for some very odd use-cases.  And while I don't have a
> > > > strong opinion about unzip, lzma is one of the valid/likelu compressions
> > > > for an Image (and aside, I, or someone, needs to look into having
> > > > 'booti' recognize various compression algorithms and automatically
> > > > decompress them) so I don't think we should get rid of that.
> > > 
> > > So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
> > > 499kB. Still tight, but it fits.
> > 
> > Looking over myself, perhaps drop CMD_BOOTD and move LOGLEVEL to 3?
> 
> And we're at 498kB now! :)
> 
> Is CMD_ELF useful as well?

That's a good question.  In 32bit land, that's how you're going to
launch FreeRTOS and proprietary apps and so forth.  I don't know if for
aarch64 people will still be doing ELF applications or UEFI applications
for all of this.

-- 
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/20171219/01d0937e/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Andre Przywara]

Hi,

On 19/12/17 14:20, Tom Rini wrote:
> On Tue, Dec 19, 2017 at 03:09:52PM +0100, Maxime Ripard wrote:
>> On Tue, Dec 19, 2017 at 08:30:31AM -0500, Tom Rini wrote:
>>> On Tue, Dec 19, 2017 at 02:26:40PM +0100, Maxime Ripard wrote:
>>>> 1;5002;0c
>>>> On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
>>>>> On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
>>>>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>>>>> traditional MMC env location.
>>>>>>
>>>>>> So I've had a quick look about what could go possibly go away in our
>>>>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>>>>> some are actually vitals:
>>>>>>
>>>>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>>>>  - CONSOLE_MUX
>>>>>>  - CMD_CRC32
>>>>>>  - CMD_LZMADEC
>>>>>>  - CMD_UNZIP
>>>>>>  - CMD_LOADB
>>>>>>  - CMD_LOADS
>>>>>>  - CMD_MISC (actually implementing the command sleep)
>>>>>>  - ISO_PARTITION (yes. For CDROMs.)
>>>>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>>>>  - VIDEO_ANSI
>>>>>>  - SHA256
>>>>>>  - LZMA
>>>>>>
>>>>>> Removing those options make the u-boot.itb binary size going from
>>>>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>>>>> the DT overlays that seem way more important than any feature
>>>>>> mentionned above (and bumps the size to 483kB).
>>>>>
>>>>> You want to keep sha256 support in FIT images, we only have dropping
>>>>> that allowed for some very odd use-cases.  And while I don't have a
>>>>> strong opinion about unzip, lzma is one of the valid/likelu compressions
>>>>> for an Image (and aside, I, or someone, needs to look into having
>>>>> 'booti' recognize various compression algorithms and automatically
>>>>> decompress them) so I don't think we should get rid of that.
>>>>
>>>> So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
>>>> 499kB. Still tight, but it fits.
>>>
>>> Looking over myself, perhaps drop CMD_BOOTD and move LOGLEVEL to 3?
>>
>> And we're at 498kB now! :)
>>
>> Is CMD_ELF useful as well?
> 
> That's a good question.  In 32bit land, that's how you're going to
> launch FreeRTOS and proprietary apps and so forth.  I don't know if for
> aarch64 people will still be doing ELF applications or UEFI applications
> for all of this.

Eventually we will be, but for now (and the *defconfig*) I think it's
fine to drop it. If we stick to Maxime's plan, we can get it back in 6
months or so.

Cheers,
Andre.
> 

[U-Boot] ARM64 Allwinner Binary Size

[Tom Rini]

On Tue, Dec 19, 2017 at 02:22:59PM +0000, Andre Przywara wrote:
> Hi,
> 
> On 19/12/17 14:20, Tom Rini wrote:
> > On Tue, Dec 19, 2017 at 03:09:52PM +0100, Maxime Ripard wrote:
> >> On Tue, Dec 19, 2017 at 08:30:31AM -0500, Tom Rini wrote:
> >>> On Tue, Dec 19, 2017 at 02:26:40PM +0100, Maxime Ripard wrote:
> >>>> 1;5002;0c
> >>>> On Tue, Dec 19, 2017 at 08:15:31AM -0500, Tom Rini wrote:
> >>>>> On Tue, Dec 19, 2017 at 02:12:02PM +0100, Maxime Ripard wrote:
> >>>>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> >>>>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> >>>>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> >>>>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
> >>>>>>> traditional MMC env location.
> >>>>>>
> >>>>>> So I've had a quick look about what could go possibly go away in our
> >>>>>> current armv8 config (using the pine64+ defconfig). Let me know if
> >>>>>> some are actually vitals:
> >>>>>>
> >>>>>>  - FIT_ENABLE_SHA256_SUPPORT
> >>>>>>  - CONSOLE_MUX
> >>>>>>  - CMD_CRC32
> >>>>>>  - CMD_LZMADEC
> >>>>>>  - CMD_UNZIP
> >>>>>>  - CMD_LOADB
> >>>>>>  - CMD_LOADS
> >>>>>>  - CMD_MISC (actually implementing the command sleep)
> >>>>>>  - ISO_PARTITION (yes. For CDROMs.)
> >>>>>>  - VIDEO_BPP8, VIDEO_BPP16
> >>>>>>  - VIDEO_ANSI
> >>>>>>  - SHA256
> >>>>>>  - LZMA
> >>>>>>
> >>>>>> Removing those options make the u-boot.itb binary size going from
> >>>>>> 516kB to 478kB, making it functional again *and* allowing us to enable
> >>>>>> the DT overlays that seem way more important than any feature
> >>>>>> mentionned above (and bumps the size to 483kB).
> >>>>>
> >>>>> You want to keep sha256 support in FIT images, we only have dropping
> >>>>> that allowed for some very odd use-cases.  And while I don't have a
> >>>>> strong opinion about unzip, lzma is one of the valid/likelu compressions
> >>>>> for an Image (and aside, I, or someone, needs to look into having
> >>>>> 'booti' recognize various compression algorithms and automatically
> >>>>> decompress them) so I don't think we should get rid of that.
> >>>>
> >>>> So, adding back CMD_LZMADEC and FIT_ENABLE_SHA256_SUPPORT, we're at
> >>>> 499kB. Still tight, but it fits.
> >>>
> >>> Looking over myself, perhaps drop CMD_BOOTD and move LOGLEVEL to 3?
> >>
> >> And we're at 498kB now! :)
> >>
> >> Is CMD_ELF useful as well?
> > 
> > That's a good question.  In 32bit land, that's how you're going to
> > launch FreeRTOS and proprietary apps and so forth.  I don't know if for
> > aarch64 people will still be doing ELF applications or UEFI applications
> > for all of this.
> 
> Eventually we will be, but for now (and the *defconfig*) I think it's
> fine to drop it. If we stick to Maxime's plan, we can get it back in 6
> months or so.

If we have to, we have to.  But I am weary of dropping and then
re-adding functionality like that as you never know what version some
company/vendor/etc is going to pick up and run with.  If we can't keep
the limit without dropping ELF, well, then we can't.

-- 
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/20171219/fee2b129/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Andre Przywara]

Hi Maxime,

On 19/12/17 14:20, Maxime Ripard wrote:
> On Tue, Dec 19, 2017 at 01:38:59PM +0000, Andre Przywara wrote:
>> Hi Maxime,
>>
>> thanks for having a look!
>>
>> On 19/12/17 13:12, Maxime Ripard wrote:
>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>> traditional MMC env location.
>>>
>>> So I've had a quick look about what could go possibly go away in our
>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>> some are actually vitals:
>>>
>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>  - CONSOLE_MUX
>>>  - CMD_CRC32
>>>  - CMD_LZMADEC
>>>  - CMD_UNZIP
>>>  - CMD_LOADB
>>>  - CMD_LOADS
>>>  - CMD_MISC (actually implementing the command sleep)
>>>  - ISO_PARTITION (yes. For CDROMs.)
>>
>> As Alex mentioned, this is needed for some installer images, which come
>> as ISOs. So if possible, we should keep this in.
> 
> So, with FIT_ENABLE_SHA256_SUPPORT, LZMADEC, ISO_PARTITION and the
> overlay support, we're at 500kB.
> 
> Again, tight, but under the limit.

Phew! ;-)

> 
>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>  - VIDEO_ANSI
>>>  - SHA256
>>>  - LZMA
>>
>> From just looking at the names I am fine with the rest gone. But let me
>> test tonight if there are any side effects.
>>
>> Some of them seem useful, but I would leave enabling them to the actual
>> users. If someone needs it, they can enable them and loose the raw MMC
>> environment. I think this is a fair trade-off.
> 
> Yes, that's my view too.
> 
>>> Removing those options make the u-boot.itb binary size going from
>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>> the DT overlays that seem way more important than any feature
>>> mentionned above (and bumps the size to 483kB).
>>
>> How important is the raw MMC environment for the ARM64 boards, actually?
>> Most of the rationale for the 32-bit side seemed to apply to legacy use
>> cases only. Do we have reports/complaints from 64-bit users?
> 
> Pretty much as important as it is on arm I guess. We just have less
> history, but the same use cases.
> 
> I'd really like to give at least one release for transition, which
> would mean having a schedule like this:
> 
>   - in 2018.01, merge those config removals so that we have least have
>     something that works quite fast
> 
>   - in 2018.03, merge the multiple environment patches. We seem to
>     have reached a consensus here, so I'm not really concerned that we
>     will have it merged.
> 
>   - in 2018.05, if needed, remove the raw MMC options and complete the
>     transition to FAT.

That sounds very reasonable to me, thanks for writing this down!

This gives us an only intermediate shortage of features for defconfig.

We should encourage everyone who builds (and ships) firmware right now
to drop MMC env support already, if they tinker with the .config anyway.
That is, if there is no legacy usage to be concerned about.

Cheers,
Andre.

[U-Boot] ARM64 Allwinner Binary Size

[Jagan Teki]

On Tue, Dec 19, 2017 at 7:57 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Maxime,
>
> On 19/12/17 14:20, Maxime Ripard wrote:
>> On Tue, Dec 19, 2017 at 01:38:59PM +0000, Andre Przywara wrote:
>>> Hi Maxime,
>>>
>>> thanks for having a look!
>>>
>>> On 19/12/17 13:12, Maxime Ripard wrote:
>>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>>> traditional MMC env location.
>>>>
>>>> So I've had a quick look about what could go possibly go away in our
>>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>>> some are actually vitals:
>>>>
>>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>>  - CONSOLE_MUX
>>>>  - CMD_CRC32
>>>>  - CMD_LZMADEC
>>>>  - CMD_UNZIP
>>>>  - CMD_LOADB
>>>>  - CMD_LOADS
>>>>  - CMD_MISC (actually implementing the command sleep)
>>>>  - ISO_PARTITION (yes. For CDROMs.)
>>>
>>> As Alex mentioned, this is needed for some installer images, which come
>>> as ISOs. So if possible, we should keep this in.
>>
>> So, with FIT_ENABLE_SHA256_SUPPORT, LZMADEC, ISO_PARTITION and the
>> overlay support, we're at 500kB.
>>
>> Again, tight, but under the limit.
>
> Phew! ;-)
>
>>
>>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>>  - VIDEO_ANSI
>>>>  - SHA256
>>>>  - LZMA
>>>
>>> From just looking at the names I am fine with the rest gone. But let me
>>> test tonight if there are any side effects.
>>>
>>> Some of them seem useful, but I would leave enabling them to the actual
>>> users. If someone needs it, they can enable them and loose the raw MMC
>>> environment. I think this is a fair trade-off.
>>
>> Yes, that's my view too.
>>
>>>> Removing those options make the u-boot.itb binary size going from
>>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>>> the DT overlays that seem way more important than any feature
>>>> mentionned above (and bumps the size to 483kB).
>>>
>>> How important is the raw MMC environment for the ARM64 boards, actually?
>>> Most of the rationale for the 32-bit side seemed to apply to legacy use
>>> cases only. Do we have reports/complaints from 64-bit users?
>>
>> Pretty much as important as it is on arm I guess. We just have less
>> history, but the same use cases.
>>
>> I'd really like to give at least one release for transition, which
>> would mean having a schedule like this:
>>
>>   - in 2018.01, merge those config removals so that we have least have
>>     something that works quite fast
>>
>>   - in 2018.03, merge the multiple environment patches. We seem to
>>     have reached a consensus here, so I'm not really concerned that we
>>     will have it merged.
>>
>>   - in 2018.05, if needed, remove the raw MMC options and complete the
>>     transition to FAT.
>
> That sounds very reasonable to me, thanks for writing this down!
>
> This gives us an only intermediate shortage of features for defconfig.
>
> We should encourage everyone who builds (and ships) firmware right now
> to drop MMC env support already, if they tinker with the .config anyway.
> That is, if there is no legacy usage to be concerned about.

All these trimming(if it fits) seems to be nice for now, but what if
once driver-model MMC, reset, pinctrl, clk, regulator are IN?
of-course we can't do anything with SPL size because of SRAM
constraints, but U-Boot proper size? why can't we increase the u-boot
partition size?

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] ARM64 Allwinner Binary Size

[Chen-Yu Tsai]

On Tue, Dec 19, 2017 at 10:38 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> On Tue, Dec 19, 2017 at 7:57 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> Hi Maxime,
>>
>> On 19/12/17 14:20, Maxime Ripard wrote:
>>> On Tue, Dec 19, 2017 at 01:38:59PM +0000, Andre Przywara wrote:
>>>> Hi Maxime,
>>>>
>>>> thanks for having a look!
>>>>
>>>> On 19/12/17 13:12, Maxime Ripard wrote:
>>>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>>>> traditional MMC env location.
>>>>>
>>>>> So I've had a quick look about what could go possibly go away in our
>>>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>>>> some are actually vitals:
>>>>>
>>>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>>>  - CONSOLE_MUX
>>>>>  - CMD_CRC32
>>>>>  - CMD_LZMADEC
>>>>>  - CMD_UNZIP
>>>>>  - CMD_LOADB
>>>>>  - CMD_LOADS
>>>>>  - CMD_MISC (actually implementing the command sleep)
>>>>>  - ISO_PARTITION (yes. For CDROMs.)
>>>>
>>>> As Alex mentioned, this is needed for some installer images, which come
>>>> as ISOs. So if possible, we should keep this in.
>>>
>>> So, with FIT_ENABLE_SHA256_SUPPORT, LZMADEC, ISO_PARTITION and the
>>> overlay support, we're at 500kB.
>>>
>>> Again, tight, but under the limit.
>>
>> Phew! ;-)
>>
>>>
>>>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>>>  - VIDEO_ANSI
>>>>>  - SHA256
>>>>>  - LZMA
>>>>
>>>> From just looking at the names I am fine with the rest gone. But let me
>>>> test tonight if there are any side effects.
>>>>
>>>> Some of them seem useful, but I would leave enabling them to the actual
>>>> users. If someone needs it, they can enable them and loose the raw MMC
>>>> environment. I think this is a fair trade-off.
>>>
>>> Yes, that's my view too.
>>>
>>>>> Removing those options make the u-boot.itb binary size going from
>>>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>>>> the DT overlays that seem way more important than any feature
>>>>> mentionned above (and bumps the size to 483kB).
>>>>
>>>> How important is the raw MMC environment for the ARM64 boards, actually?
>>>> Most of the rationale for the 32-bit side seemed to apply to legacy use
>>>> cases only. Do we have reports/complaints from 64-bit users?
>>>
>>> Pretty much as important as it is on arm I guess. We just have less
>>> history, but the same use cases.
>>>
>>> I'd really like to give at least one release for transition, which
>>> would mean having a schedule like this:
>>>
>>>   - in 2018.01, merge those config removals so that we have least have
>>>     something that works quite fast
>>>
>>>   - in 2018.03, merge the multiple environment patches. We seem to
>>>     have reached a consensus here, so I'm not really concerned that we
>>>     will have it merged.
>>>
>>>   - in 2018.05, if needed, remove the raw MMC options and complete the
>>>     transition to FAT.
>>
>> That sounds very reasonable to me, thanks for writing this down!
>>
>> This gives us an only intermediate shortage of features for defconfig.
>>
>> We should encourage everyone who builds (and ships) firmware right now
>> to drop MMC env support already, if they tinker with the .config anyway.
>> That is, if there is no legacy usage to be concerned about.
>
> All these trimming(if it fits) seems to be nice for now, but what if
> once driver-model MMC, reset, pinctrl, clk, regulator are IN?
> of-course we can't do anything with SPL size because of SRAM
> constraints, but U-Boot proper size? why can't we increase the u-boot
> partition size?

That is a really big if, given no one is actively working on it.
I reckon we deal with it when someone starts having patches merged. :)

ChenYu

[U-Boot] ARM64 Allwinner Binary Size

[Andre Przywara]

Hi,

On 19/12/17 14:38, Jagan Teki wrote:
> On Tue, Dec 19, 2017 at 7:57 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> Hi Maxime,
>>
>> On 19/12/17 14:20, Maxime Ripard wrote:
>>> On Tue, Dec 19, 2017 at 01:38:59PM +0000, Andre Przywara wrote:
>>>> Hi Maxime,
>>>>
>>>> thanks for having a look!
>>>>
>>>> On 19/12/17 13:12, Maxime Ripard wrote:
>>>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>>>> traditional MMC env location.
>>>>>
>>>>> So I've had a quick look about what could go possibly go away in our
>>>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>>>> some are actually vitals:
>>>>>
>>>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>>>  - CONSOLE_MUX
>>>>>  - CMD_CRC32
>>>>>  - CMD_LZMADEC
>>>>>  - CMD_UNZIP
>>>>>  - CMD_LOADB
>>>>>  - CMD_LOADS
>>>>>  - CMD_MISC (actually implementing the command sleep)
>>>>>  - ISO_PARTITION (yes. For CDROMs.)
>>>>
>>>> As Alex mentioned, this is needed for some installer images, which come
>>>> as ISOs. So if possible, we should keep this in.
>>>
>>> So, with FIT_ENABLE_SHA256_SUPPORT, LZMADEC, ISO_PARTITION and the
>>> overlay support, we're at 500kB.
>>>
>>> Again, tight, but under the limit.
>>
>> Phew! ;-)
>>
>>>
>>>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>>>  - VIDEO_ANSI
>>>>>  - SHA256
>>>>>  - LZMA
>>>>
>>>> From just looking at the names I am fine with the rest gone. But let me
>>>> test tonight if there are any side effects.
>>>>
>>>> Some of them seem useful, but I would leave enabling them to the actual
>>>> users. If someone needs it, they can enable them and loose the raw MMC
>>>> environment. I think this is a fair trade-off.
>>>
>>> Yes, that's my view too.
>>>
>>>>> Removing those options make the u-boot.itb binary size going from
>>>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>>>> the DT overlays that seem way more important than any feature
>>>>> mentionned above (and bumps the size to 483kB).
>>>>
>>>> How important is the raw MMC environment for the ARM64 boards, actually?
>>>> Most of the rationale for the 32-bit side seemed to apply to legacy use
>>>> cases only. Do we have reports/complaints from 64-bit users?
>>>
>>> Pretty much as important as it is on arm I guess. We just have less
>>> history, but the same use cases.
>>>
>>> I'd really like to give at least one release for transition, which
>>> would mean having a schedule like this:
>>>
>>>   - in 2018.01, merge those config removals so that we have least have
>>>     something that works quite fast
>>>
>>>   - in 2018.03, merge the multiple environment patches. We seem to
>>>     have reached a consensus here, so I'm not really concerned that we
>>>     will have it merged.
>>>
>>>   - in 2018.05, if needed, remove the raw MMC options and complete the
>>>     transition to FAT.
>>
>> That sounds very reasonable to me, thanks for writing this down!
>>
>> This gives us an only intermediate shortage of features for defconfig.
>>
>> We should encourage everyone who builds (and ships) firmware right now
>> to drop MMC env support already, if they tinker with the .config anyway.
>> That is, if there is no legacy usage to be concerned about.
> 
> All these trimming(if it fits) seems to be nice for now, but what if
> once driver-model MMC, reset, pinctrl, clk, regulator are IN?

But that is what all of this is about? At the moment we can't go beyond
504K because the raw MMC U-Boot environment lives there on the SD card.
But with Maxime's plan for 2018.05, this will be gone and we should have
about 984KB (1MB - 40K).

Cheers,
Andre.

> of-course we can't do anything with SPL size because of SRAM
> constraints, but U-Boot proper size? why can't we increase the u-boot
> partition size?
> 
> thanks!
> 

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 19, 2017 at 10:41:23PM +0800, Chen-Yu Tsai wrote:
> > All these trimming(if it fits) seems to be nice for now, but what if
> > once driver-model MMC, reset, pinctrl, clk, regulator are IN?

I guess a better question would be: what are we doing to fix an issue
we had in a release already and will destroy the binary as soon as
someone does a saveenv?

> > of-course we can't do anything with SPL size because of SRAM
> > constraints, but U-Boot proper size? why can't we increase the u-boot
> > partition size?
> 
> That is a really big if, given no one is actively working on it.
> I reckon we deal with it when someone starts having patches merged. :)

+1.

Maxime

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

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 19, 2017 at 02:41:17PM +0100, Mark Kettenis wrote:
> > Date: Tue, 19 Dec 2017 14:12:02 +0100
> > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > traditional MMC env location.
> > 
> > So I've had a quick look about what could go possibly go away in our
> > current armv8 config (using the pine64+ defconfig). Let me know if
> > some are actually vitals:
> > 
> >  - FIT_ENABLE_SHA256_SUPPORT
> >  - CONSOLE_MUX
> >  - CMD_CRC32
> >  - CMD_LZMADEC
> >  - CMD_UNZIP
> >  - CMD_LOADB
> >  - CMD_LOADS
> >  - CMD_MISC (actually implementing the command sleep)
> >  - ISO_PARTITION (yes. For CDROMs.)
> >  - VIDEO_BPP8, VIDEO_BPP16
> >  - VIDEO_ANSI
> >  - SHA256
> >  - LZMA
> > 
> > Removing those options make the u-boot.itb binary size going from
> > 516kB to 478kB, making it functional again *and* allowing us to enable
> > the DT overlays that seem way more important than any feature
> > mentionned above (and bumps the size to 483kB).
> 
> So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
> console and usb keyboard are present?  Is the usb keyboard used when
> it is plugged in but serial otherwise?

That's actually a very good question, and I don't know, I never used a
USB keyboard with U-Boot.

This is enabled on 7 Allwinner boards, and no one complained so far,
so I would expect to work without that option activated. However, it
doesn't take that much space either, so it's not like we really need
to disable it either.

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/20171219/c637ae13/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Tue, Dec 19, 2017 at 02:27:57PM +0000, Andre Przywara wrote:
> >>> Removing those options make the u-boot.itb binary size going from
> >>> 516kB to 478kB, making it functional again *and* allowing us to enable
> >>> the DT overlays that seem way more important than any feature
> >>> mentionned above (and bumps the size to 483kB).
> >>
> >> How important is the raw MMC environment for the ARM64 boards, actually?
> >> Most of the rationale for the 32-bit side seemed to apply to legacy use
> >> cases only. Do we have reports/complaints from 64-bit users?
> > 
> > Pretty much as important as it is on arm I guess. We just have less
> > history, but the same use cases.
> > 
> > I'd really like to give at least one release for transition, which
> > would mean having a schedule like this:
> > 
> >   - in 2018.01, merge those config removals so that we have least have
> >     something that works quite fast

So, given the various feedback, the current diff for the pine64 is:
http://code.bulix.org/t7icrw-243561

With that, We're at 500kB with the ATF.

Does it work for everyone here?
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/20171219/e82378a2/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Tom Rini]

On Tue, Dec 19, 2017 at 04:36:13PM +0100, Maxime Ripard wrote:
> On Tue, Dec 19, 2017 at 02:27:57PM +0000, Andre Przywara wrote:
> > >>> Removing those options make the u-boot.itb binary size going from
> > >>> 516kB to 478kB, making it functional again *and* allowing us to enable
> > >>> the DT overlays that seem way more important than any feature
> > >>> mentionned above (and bumps the size to 483kB).
> > >>
> > >> How important is the raw MMC environment for the ARM64 boards, actually?
> > >> Most of the rationale for the 32-bit side seemed to apply to legacy use
> > >> cases only. Do we have reports/complaints from 64-bit users?
> > > 
> > > Pretty much as important as it is on arm I guess. We just have less
> > > history, but the same use cases.
> > > 
> > > I'd really like to give at least one release for transition, which
> > > would mean having a schedule like this:
> > > 
> > >   - in 2018.01, merge those config removals so that we have least have
> > >     something that works quite fast
> 
> So, given the various feedback, the current diff for the pine64 is:
> http://code.bulix.org/t7icrw-243561
> 
> With that, We're at 500kB with the ATF.
> 
> Does it work for everyone here?

Works for me, 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/20171219/65d0891a/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Jagan Teki]

On Tue, Dec 19, 2017 at 9:06 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Dec 19, 2017 at 02:27:57PM +0000, Andre Przywara wrote:
>> >>> Removing those options make the u-boot.itb binary size going from
>> >>> 516kB to 478kB, making it functional again *and* allowing us to enable
>> >>> the DT overlays that seem way more important than any feature
>> >>> mentionned above (and bumps the size to 483kB).
>> >>
>> >> How important is the raw MMC environment for the ARM64 boards, actually?
>> >> Most of the rationale for the 32-bit side seemed to apply to legacy use
>> >> cases only. Do we have reports/complaints from 64-bit users?
>> >
>> > Pretty much as important as it is on arm I guess. We just have less
>> > history, but the same use cases.
>> >
>> > I'd really like to give at least one release for transition, which
>> > would mean having a schedule like this:
>> >
>> >   - in 2018.01, merge those config removals so that we have least have
>> >     something that works quite fast
>
> So, given the various feedback, the current diff for the pine64 is:
> http://code.bulix.org/t7icrw-243561
>
> With that, We're at 500kB with the ATF.
>
> Does it work for everyone here?

496KB for a64-olinuxino, nanopi-a64

[U-Boot] ARM64 Allwinner Binary Size

[Mark Kettenis]

> Date: Tue, 19 Dec 2017 16:24:59 +0100
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> On Tue, Dec 19, 2017 at 02:41:17PM +0100, Mark Kettenis wrote:
> > > Date: Tue, 19 Dec 2017 14:12:02 +0100
> > > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > 
> > > On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > > So even though the actual u-boot.bin for 64-bit boards is still somewhat
> > > > below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > > the edge. So since v2017.11 u-boot.itb is already too big for the
> > > > traditional MMC env location.
> > > 
> > > So I've had a quick look about what could go possibly go away in our
> > > current armv8 config (using the pine64+ defconfig). Let me know if
> > > some are actually vitals:
> > > 
> > >  - FIT_ENABLE_SHA256_SUPPORT
> > >  - CONSOLE_MUX
> > >  - CMD_CRC32
> > >  - CMD_LZMADEC
> > >  - CMD_UNZIP
> > >  - CMD_LOADB
> > >  - CMD_LOADS
> > >  - CMD_MISC (actually implementing the command sleep)
> > >  - ISO_PARTITION (yes. For CDROMs.)
> > >  - VIDEO_BPP8, VIDEO_BPP16
> > >  - VIDEO_ANSI
> > >  - SHA256
> > >  - LZMA
> > > 
> > > Removing those options make the u-boot.itb binary size going from
> > > 516kB to 478kB, making it functional again *and* allowing us to enable
> > > the DT overlays that seem way more important than any feature
> > > mentionned above (and bumps the size to 483kB).
> > 
> > So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
> > console and usb keyboard are present?  Is the usb keyboard used when
> > it is plugged in but serial otherwise?
> 
> That's actually a very good question, and I don't know, I never used a
> USB keyboard with U-Boot.
> 
> This is enabled on 7 Allwinner boards, and no one complained so far,
> so I would expect to work without that option activated. However, it
> doesn't take that much space either, so it's not like we really need
> to disable it either.

I've used it on several board including my Orange Pi PC2.  With this
option enabled, all output from U-Boot and the EFI bootloader is
visible on both serial and hdmi, and I can interact with U-Boot and
the EFI bootloader over serial and using a USB keyboard.

That allows users to interact with U-Boot and/or the EFI bootloader
and configure whether the kernel sould use serial or "glass" console.

[U-Boot] ARM64 Allwinner Binary Size

[André Przywara]

On 19/12/17 15:24, Maxime Ripard wrote:
> On Tue, Dec 19, 2017 at 02:41:17PM +0100, Mark Kettenis wrote:
>>> Date: Tue, 19 Dec 2017 14:12:02 +0100
>>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>
>>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
>>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
>>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
>>>> the edge. So since v2017.11 u-boot.itb is already too big for the
>>>> traditional MMC env location.
>>>
>>> So I've had a quick look about what could go possibly go away in our
>>> current armv8 config (using the pine64+ defconfig). Let me know if
>>> some are actually vitals:
>>>
>>>  - FIT_ENABLE_SHA256_SUPPORT
>>>  - CONSOLE_MUX
>>>  - CMD_CRC32
>>>  - CMD_LZMADEC
>>>  - CMD_UNZIP
>>>  - CMD_LOADB
>>>  - CMD_LOADS
>>>  - CMD_MISC (actually implementing the command sleep)
>>>  - ISO_PARTITION (yes. For CDROMs.)
>>>  - VIDEO_BPP8, VIDEO_BPP16
>>>  - VIDEO_ANSI
>>>  - SHA256
>>>  - LZMA
>>>
>>> Removing those options make the u-boot.itb binary size going from
>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>> the DT overlays that seem way more important than any feature
>>> mentionned above (and bumps the size to 483kB).
>>
>> So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
>> console and usb keyboard are present?  Is the usb keyboard used when
>> it is plugged in but serial otherwise?
> 
> That's actually a very good question, and I don't know, I never used a
> USB keyboard with U-Boot.
> 
> This is enabled on 7 Allwinner boards, and no one complained so far,
> so I would expect to work without that option activated. However, it
> doesn't take that much space either, so it's not like we really need
> to disable it either.

Just tested it: without CONSOLE_MUX we don't use video and USB keyboard
automatically. It seems to default to serial, "setenv stdout vidconsole"
switches over (immediately). This is quite weird behaviour, and probably
quite unpleasant for the casual user.
With CONSOLE_MUX defined we get what we want: always serial console,
plus USB keyboard and HDMI, if connected. Output appears on both, input
is accepted from both.
So I would very much like to keep it in.

Cheers,
Andre.

[U-Boot] ARM64 Allwinner Binary Size

[André Przywara]

On 19/12/17 15:36, Maxime Ripard wrote:
> On Tue, Dec 19, 2017 at 02:27:57PM +0000, Andre Przywara wrote:
>>>>> Removing those options make the u-boot.itb binary size going from
>>>>> 516kB to 478kB, making it functional again *and* allowing us to enable
>>>>> the DT overlays that seem way more important than any feature
>>>>> mentionned above (and bumps the size to 483kB).
>>>>
>>>> How important is the raw MMC environment for the ARM64 boards, actually?
>>>> Most of the rationale for the 32-bit side seemed to apply to legacy use
>>>> cases only. Do we have reports/complaints from 64-bit users?
>>>
>>> Pretty much as important as it is on arm I guess. We just have less
>>> history, but the same use cases.
>>>
>>> I'd really like to give at least one release for transition, which
>>> would mean having a schedule like this:
>>>
>>>   - in 2018.01, merge those config removals so that we have least have
>>>     something that works quite fast
> 
> So, given the various feedback, the current diff for the pine64 is:
> http://code.bulix.org/t7icrw-243561
> 
> With that, We're at 500kB with the ATF.
> 
> Does it work for everyone here?

Yes, briefly tested with HDMI, USB keyboard, also UEFI boot (just grub).

The only downside is the missing "boot" command. Not a big deal (as one
can always type "run bootmcd"), but quite inconvenient for the casual
user. If one (accidentally) aborts the timeout, just typing "boot" does
the right thing. This add ~450 bytes on my setup, so I wonder if we can
keep it in?

Cheers,
Andre.

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Wed, Dec 20, 2017 at 01:55:51AM +0000, André Przywara wrote:
> On 19/12/17 15:24, Maxime Ripard wrote:
> > On Tue, Dec 19, 2017 at 02:41:17PM +0100, Mark Kettenis wrote:
> >>> Date: Tue, 19 Dec 2017 14:12:02 +0100
> >>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>>
> >>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> >>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> >>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> >>>> the edge. So since v2017.11 u-boot.itb is already too big for the
> >>>> traditional MMC env location.
> >>>
> >>> So I've had a quick look about what could go possibly go away in our
> >>> current armv8 config (using the pine64+ defconfig). Let me know if
> >>> some are actually vitals:
> >>>
> >>>  - FIT_ENABLE_SHA256_SUPPORT
> >>>  - CONSOLE_MUX
> >>>  - CMD_CRC32
> >>>  - CMD_LZMADEC
> >>>  - CMD_UNZIP
> >>>  - CMD_LOADB
> >>>  - CMD_LOADS
> >>>  - CMD_MISC (actually implementing the command sleep)
> >>>  - ISO_PARTITION (yes. For CDROMs.)
> >>>  - VIDEO_BPP8, VIDEO_BPP16
> >>>  - VIDEO_ANSI
> >>>  - SHA256
> >>>  - LZMA
> >>>
> >>> Removing those options make the u-boot.itb binary size going from
> >>> 516kB to 478kB, making it functional again *and* allowing us to enable
> >>> the DT overlays that seem way more important than any feature
> >>> mentionned above (and bumps the size to 483kB).
> >>
> >> So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
> >> console and usb keyboard are present?  Is the usb keyboard used when
> >> it is plugged in but serial otherwise?
> > 
> > That's actually a very good question, and I don't know, I never used a
> > USB keyboard with U-Boot.
> > 
> > This is enabled on 7 Allwinner boards, and no one complained so far,
> > so I would expect to work without that option activated. However, it
> > doesn't take that much space either, so it's not like we really need
> > to disable it either.
> 
> Just tested it: without CONSOLE_MUX we don't use video and USB keyboard
> automatically. It seems to default to serial, "setenv stdout vidconsole"
> switches over (immediately). This is quite weird behaviour, and probably
> quite unpleasant for the casual user.
> With CONSOLE_MUX defined we get what we want: always serial console,
> plus USB keyboard and HDMI, if connected. Output appears on both, input
> is accepted from both.
> So I would very much like to keep it in.

Sounds like we should even enable it for everyone then.

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/20171220/6320eb88/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Mark Kettenis]

> Date: Wed, 20 Dec 2017 08:15:46 +0100
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> On Wed, Dec 20, 2017 at 01:55:51AM +0000, Andr=E9 Przywara wrote:
> > On 19/12/17 15:24, Maxime Ripard wrote:
> > > On Tue, Dec 19, 2017 at 02:41:17PM +0100, Mark Kettenis wrote:
> > >>> Date: Tue, 19 Dec 2017 14:12:02 +0100
> > >>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > >>>
> > >>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > >>>> So even though the actual u-boot.bin for 64-bit boards is still some=
> what
> > >>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > >>>> the edge. So since v2017.11 u-boot.itb is already too big for the
> > >>>> traditional MMC env location.
> > >>>
> > >>> So I've had a quick look about what could go possibly go away in our
> > >>> current armv8 config (using the pine64+ defconfig). Let me know if
> > >>> some are actually vitals:
> > >>>
> > >>>  - FIT_ENABLE_SHA256_SUPPORT
> > >>>  - CONSOLE_MUX
> > >>>  - CMD_CRC32
> > >>>  - CMD_LZMADEC
> > >>>  - CMD_UNZIP
> > >>>  - CMD_LOADB
> > >>>  - CMD_LOADS
> > >>>  - CMD_MISC (actually implementing the command sleep)
> > >>>  - ISO_PARTITION (yes. For CDROMs.)
> > >>>  - VIDEO_BPP8, VIDEO_BPP16
> > >>>  - VIDEO_ANSI
> > >>>  - SHA256
> > >>>  - LZMA
> > >>>
> > >>> Removing those options make the u-boot.itb binary size going from
> > >>> 516kB to 478kB, making it functional again *and* allowing us to enable
> > >>> the DT overlays that seem way more important than any feature
> > >>> mentionned above (and bumps the size to 483kB).
> > >>
> > >> So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
> > >> console and usb keyboard are present?  Is the usb keyboard used when
> > >> it is plugged in but serial otherwise?
> > >=20
> > > That's actually a very good question, and I don't know, I never used a
> > > USB keyboard with U-Boot.
> > >=20
> > > This is enabled on 7 Allwinner boards, and no one complained so far,
> > > so I would expect to work without that option activated. However, it
> > > doesn't take that much space either, so it's not like we really need
> > > to disable it either.
> >=20
> > Just tested it: without CONSOLE_MUX we don't use video and USB keyboard
> > automatically. It seems to default to serial, "setenv stdout vidconsole"
> > switches over (immediately). This is quite weird behaviour, and probably
> > quite unpleasant for the casual user.
> > With CONSOLE_MUX defined we get what we want: always serial console,
> > plus USB keyboard and HDMI, if connected. Output appears on both, input
> > is accepted from both.
> > So I would very much like to keep it in.
> 
> Sounds like we should even enable it for everyone then.

common/Kconfig has:

config CONSOLE_MUX
        bool "Enable console multiplexing"
        default y if DM_VIDEO || VIDEO || LCD

so it is already enabled in the cases where it matters.

[U-Boot] ARM64 Allwinner Binary Size

[Maxime Ripard]

On Wed, Dec 20, 2017 at 08:42:22AM +0100, Mark Kettenis wrote:
> > Date: Wed, 20 Dec 2017 08:15:46 +0100
> > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > On Wed, Dec 20, 2017 at 01:55:51AM +0000, Andr=E9 Przywara wrote:
> > > On 19/12/17 15:24, Maxime Ripard wrote:
> > > > On Tue, Dec 19, 2017 at 02:41:17PM +0100, Mark Kettenis wrote:
> > > >>> Date: Tue, 19 Dec 2017 14:12:02 +0100
> > > >>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > >>>
> > > >>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> > > >>>> So even though the actual u-boot.bin for 64-bit boards is still some=
> > what
> > > >>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> > > >>>> the edge. So since v2017.11 u-boot.itb is already too big for the
> > > >>>> traditional MMC env location.
> > > >>>
> > > >>> So I've had a quick look about what could go possibly go away in our
> > > >>> current armv8 config (using the pine64+ defconfig). Let me know if
> > > >>> some are actually vitals:
> > > >>>
> > > >>>  - FIT_ENABLE_SHA256_SUPPORT
> > > >>>  - CONSOLE_MUX
> > > >>>  - CMD_CRC32
> > > >>>  - CMD_LZMADEC
> > > >>>  - CMD_UNZIP
> > > >>>  - CMD_LOADB
> > > >>>  - CMD_LOADS
> > > >>>  - CMD_MISC (actually implementing the command sleep)
> > > >>>  - ISO_PARTITION (yes. For CDROMs.)
> > > >>>  - VIDEO_BPP8, VIDEO_BPP16
> > > >>>  - VIDEO_ANSI
> > > >>>  - SHA256
> > > >>>  - LZMA
> > > >>>
> > > >>> Removing those options make the u-boot.itb binary size going from
> > > >>> 516kB to 478kB, making it functional again *and* allowing us to enable
> > > >>> the DT overlays that seem way more important than any feature
> > > >>> mentionned above (and bumps the size to 483kB).
> > > >>
> > > >> So without CONFIG_CONSOLE_MUX, what is the behaviour when both serial
> > > >> console and usb keyboard are present?  Is the usb keyboard used when
> > > >> it is plugged in but serial otherwise?
> > > >=20
> > > > That's actually a very good question, and I don't know, I never used a
> > > > USB keyboard with U-Boot.
> > > >=20
> > > > This is enabled on 7 Allwinner boards, and no one complained so far,
> > > > so I would expect to work without that option activated. However, it
> > > > doesn't take that much space either, so it's not like we really need
> > > > to disable it either.
> > >=20
> > > Just tested it: without CONSOLE_MUX we don't use video and USB keyboard
> > > automatically. It seems to default to serial, "setenv stdout vidconsole"
> > > switches over (immediately). This is quite weird behaviour, and probably
> > > quite unpleasant for the casual user.
> > > With CONSOLE_MUX defined we get what we want: always serial console,
> > > plus USB keyboard and HDMI, if connected. Output appears on both, input
> > > is accepted from both.
> > > So I would very much like to keep it in.
> > 
> > Sounds like we should even enable it for everyone then.
> 
> common/Kconfig has:
> 
> config CONSOLE_MUX
>         bool "Enable console multiplexing"
>         default y if DM_VIDEO || VIDEO || LCD
> 
> so it is already enabled in the cases where it matters.

Ok, then the next question is why a few of our boards are actually
setting it in their defconfig instead of just using that default :)

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/20171220/7fab0962/attachment.sig>

[U-Boot] ARM64 Allwinner Binary Size

[Mark Kettenis]

> From: Andre Przywara <andre.przywara@arm.com>
> Date: Tue, 19 Dec 2017 14:17:37 +0000
> 
> Hi,
> 
> On 19/12/17 13:51, Mark Kettenis wrote:
> >> From: Andre Przywara <andre.przywara@arm.com>
> >> Date: Tue, 19 Dec 2017 13:38:59 +0000
> >>
> >> Hi Maxime,
> >>
> >> thanks for having a look!
> >>
> >> On 19/12/17 13:12, Maxime Ripard wrote:
> >>> On Tue, Dec 05, 2017 at 10:28:20AM +0000, Andre Przywara wrote:
> >>>> So even though the actual u-boot.bin for 64-bit boards is still somewhat
> >>>> below the limit (~480KB), adding the ATF image (~32KB) pushes it over
> >>>> the edge. So since v2017.11 u-boot.itb is already too big for the
> >>>> traditional MMC env location.
> >>>
> >>> So I've had a quick look about what could go possibly go away in our
> >>> current armv8 config (using the pine64+ defconfig). Let me know if
> >>> some are actually vitals:
> >>>
> >>>  - FIT_ENABLE_SHA256_SUPPORT
> >>>  - CONSOLE_MUX
> >>>  - CMD_CRC32
> >>>  - CMD_LZMADEC
> >>>  - CMD_UNZIP
> >>>  - CMD_LOADB
> >>>  - CMD_LOADS
> >>>  - CMD_MISC (actually implementing the command sleep)
> >>>  - ISO_PARTITION (yes. For CDROMs.)
> >>
> >> As Alex mentioned, this is needed for some installer images, which come
> >> as ISOs. So if possible, we should keep this in.
> >>
> >>>  - VIDEO_BPP8, VIDEO_BPP16
> >>>  - VIDEO_ANSI
> >>>  - SHA256
> >>>  - LZMA
> >>
> >> From just looking at the names I am fine with the rest gone. But let me
> >> test tonight if there are any side effects.
> >>
> >> Some of them seem useful, but I would leave enabling them to the actual
> >> users. If someone needs it, they can enable them and loose the raw MMC
> >> environment. I think this is a fair trade-off.
> >>
> >>> Removing those options make the u-boot.itb binary size going from
> >>> 516kB to 478kB, making it functional again *and* allowing us to enable
> >>> the DT overlays that seem way more important than any feature
> >>> mentionned above (and bumps the size to 483kB).
> >>
> >> How important is the raw MMC environment for the ARM64 boards, actually?
> >> Most of the rationale for the 32-bit side seemed to apply to legacy use
> >> cases only. Do we have reports/complaints from 64-bit users?
> > 
> > For me/us (OpenBSD) the environment is still important.  I have many
> > setups where U-Boot lives on a uSD card but the installed OS lives on
> > a USB device.  In that scenario I set boot_targets to boot the EFI
> > bootloader and OS off the USB disk.  This is very helpfull for testing
> > new versions of U-Boot as I can simply swap the uSD card.  But for
> > some setups this is essential as OpenBSD doesn't support the SD/MCC
> > controller on all ARM hardware yet (but we do support it on
> > Allwinner).
> 
> I see, but I wasn't arguing for dropping the environment altogether,
> more for supporting FAT environments *only*.
> So how important is preserving existing environments over a firmware
> update in your scenario? I think this is the killer question here, isn't
> it? I'm inclined to just drop raw MMC environment support from sunxi64
> boards and then enjoy the ~450KB more worth of code, until we hit the
> first MB boundary.

I wouldn't consider preserving the environment as crucial.

> I have builds with all (DDR3) A64 board DTs in the binary [1], which
> would be larger than 504K anyway.

Oh, that's some cool work on the ATF side.  Need to check that out.

> Cheers,
> Andre.
> 
> [1] https://github.com/apritzel/pine64/commit/ee12bea43

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

[Simon Glass]

On 28 November 2017 at 03:24, 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.
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On 28 November 2017 at 03:24, 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.
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

Hi Maxime,

On 28 November 2017 at 03:24, 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;
> +
>         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)

Can you please document the function args? The operation of prio needs
to be described somewhere.

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

Why is this needed? Shouldn't _env_driver_lookup() return NULL in this
case anyway?

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

So we get no advice of error? I think it should print a message here,
like ...failed or ...done.

Also if no driver succeeded I suggest returning one of the errors
returned from drv-save().

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

comment here

> +enum env_operation {
> +       ENVO_GET_CHAR,

I think ENVOP would be a better prefix.

> +       ENVO_INIT,
> +       ENVO_LOAD,
> +       ENVO_SAVE,
> +};
> +
>  struct env_driver {
>         const char *name;
>         enum env_location location;
> --
> git-series 0.9.1

Regards,
Simon

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

[Simon Glass]

On 28 November 2017 at 03:24, 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.
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On 5 December 2017 at 03:09, Andre Przywara <andre.przywara@arm.com> wrote:
> On 28/11/17 10:24, 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.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>>  env/env.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index 1d13220aa79b..44f9908e2c2d 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -109,12 +109,11 @@ int env_load(void)
>>               if (!drv->load)
>>                       continue;
>>
>> +             printf("Loading Environment from %s... ", drv->name);
>>               ret = drv->load();
>> +             printf("%s\n", ret ? "Failed" : "OK");
>
> This looses the error code that was printed with debug() below. Might be
> worth to keep it? Either always or as a debug?
>                 printf("%s (%d)\n", ...);
>

Seems like a good idea. Other than that:

Reviewed-by: Simon Glass <sjg@chromium.org>


> Cheers,
> Andre.
>
>>               if (!ret)
>>                       return 0;
>> -
>> -             debug("%s: Environment %s failed to load (err=%d)\n", __func__,
>> -                   drv->name, ret);
>>       }
>>
>>       return -ENODEV;
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Simon Glass]

On 28 November 2017 at 03:24, Maxime Ripard
<maxime.ripard@free-electrons.com> 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.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 75 +++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 25 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

Hi Maxime,

On 28 November 2017 at 03:24, Maxime Ripard
<maxime.ripard@free-electrons.com> 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.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c                         | 19 ++++++++++++-------
>  include/asm-generic/global_data.h |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 5176700133d3..b4d8886e7a69 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -112,6 +112,9 @@ int env_get_char(int index)
>                 if (!drv->get_char)
>                         continue;
>
> +               if (!(gd->env_has_init & BIT(drv->location)))
> +                       continue;
> +

Little nit. Can you please put the access in functions like
env_has_inited() and env_set_inited()? This allows us to change the
implementation later, and also it more self-documenting.

>                 ret = drv->get_char(index);
>                 if (!ret)
>                         return 0;
> @@ -134,6 +137,9 @@ int env_load(void)
>                 if (!drv->load)
>                         continue;
>
> +               if (!(gd->env_has_init & BIT(drv->location)))
> +                       continue;
> +
>                 printf("Loading Environment from %s... ", drv->name);
>                 ret = drv->load();
>                 printf("%s\n", ret ? "Failed" : "OK");
> @@ -155,6 +161,9 @@ int env_save(void)
>                 if (!drv->save)
>                         continue;
>
> +               if (!(gd->env_has_init & BIT(drv->location)))
> +                       continue;
> +
>                 printf("Saving Environment to %s... ", drv->name);
>                 ret = drv->save();
>                 printf("%s\n", ret ? "Failed" : "OK");
> @@ -175,14 +184,10 @@ int env_init(void)
>         int prio;
>
>         for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> -               if (!drv->init)
> -                       continue;
> -
> -               ret = drv->init();
> -               if (!ret)
> -                       return 0;
> +               if (!drv->init || !drv->init())
> +                       gd->env_has_init |= BIT(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 944f58195caf..1d0611fe9498 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 mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Regards,
Simon

[U-Boot] [PATCH 11/14] env: Allow to build multiple environments in Kconfig

[Simon Glass]

On 28 November 2017 at 03:24, 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.
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/Kconfig | 65 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 33 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Simon Glass]

Hi Maxime,

On 28 November 2017 at 03:24, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Allow boards and architectures to override the default environment lookup
> code by overriding env_get_location.
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 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 b4d8886e7a69..9b8b38cf3c2b 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)

Is it possible instead to make this deterministic, so that the board
sets up the location during boot?

>  {
>         switch (op) {
>         case ENVO_GET_CHAR:
> --
> git-series 0.9.1
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Regards,
Simon

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

[Maxime Ripard]

Hi Simon,

On Thu, Dec 28, 2017 at 08:13:23PM -0700, Simon Glass wrote:
> > -static struct env_driver *env_driver_lookup(void)
> > +static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> 
> Can you please document the function args? The operation of prio needs
> to be described somewhere.

Will do.

> >  {
> > -       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;
> 
> Why is this needed? Shouldn't _env_driver_lookup() return NULL in this
> case anyway?

I just thought that making it obvious and explicit (especially when
the behaviour of _env_driver_lookup might change) would be a good
thing, but I can remove it if you want.

> > -       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;
> 
> So we get no advice of error? I think it should print a message here,
> like ...failed or ...done.

This is addressed in the next patch

> Also if no driver succeeded I suggest returning one of the errors
> returned from drv-save().

And this was one of the comments made in this version, so it's going
to be addressed in the next iteration.

> > --- a/include/environment.h
> > +++ b/include/environment.h
> > @@ -215,6 +215,13 @@ enum env_location {
> >         ENVL_UNKNOWN,
> >  };
> >
> 
> comment here
> 
> > +enum env_operation {
> > +       ENVO_GET_CHAR,
> 
> I think ENVOP would be a better prefix.

That works for me. 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: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180105/bceecf4a/attachment.sig>

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Maxime Ripard]

Hi Simon,

On Thu, Dec 28, 2017 at 08:13:42PM -0700, Simon Glass wrote:
> Hi Maxime,
> 
> On 28 November 2017 at 03:24, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Allow boards and architectures to override the default environment lookup
> > code by overriding env_get_location.
> >
> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > 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 b4d8886e7a69..9b8b38cf3c2b 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)
> 
> Is it possible instead to make this deterministic, so that the board
> sets up the location during boot?

I'm not really sure what you mean here. The default implementation is
deterministic, and the board implementations should make sure that it
is if it's of any value to them.

Do you want me to add a comment to make that clearer?

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

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Simon Glass]

Hi Maxime,

On 5 January 2018 at 02:29, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Simon,
>
> On Thu, Dec 28, 2017 at 08:13:42PM -0700, Simon Glass wrote:
>> Hi Maxime,
>>
>> On 28 November 2017 at 03:24, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > Allow boards and architectures to override the default environment lookup
>> > code by overriding env_get_location.
>> >
>> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
>> > 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 b4d8886e7a69..9b8b38cf3c2b 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)
>>
>> Is it possible instead to make this deterministic, so that the board
>> sets up the location during boot?
>
> I'm not really sure what you mean here. The default implementation is
> deterministic, and the board implementations should make sure that it
> is if it's of any value to them.
>
> Do you want me to add a comment to make that clearer?

I mean that the board presumably knows the location, so could set it
up (perhaps in global_data). Then env_get_location() can use it.

Making functions weak means everything becomes non-deterministic -
e.g. I cannot tell from the code what it is going to do.

>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Simon

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Maxime Ripard]

Hi,

On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:
> >> On 28 November 2017 at 03:24, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > Allow boards and architectures to override the default environment lookup
> >> > code by overriding env_get_location.
> >> >
> >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> >> > 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 b4d8886e7a69..9b8b38cf3c2b 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)
> >>
> >> Is it possible instead to make this deterministic, so that the board
> >> sets up the location during boot?
> >
> > I'm not really sure what you mean here. The default implementation is
> > deterministic, and the board implementations should make sure that it
> > is if it's of any value to them.
> >
> > Do you want me to add a comment to make that clearer?
> 
> I mean that the board presumably knows the location, so could set it
> up (perhaps in global_data). Then env_get_location() can use it.

Well, it's pretty much what happens when you only have a single
environment selected in Kconfig, except that you don't need to change
anything in all the boards, which would be the case with your solution
I guess?

> Making functions weak means everything becomes non-deterministic -
> e.g. I cannot tell from the code what it is going to do.

One of the point of this serie is to allow boards to select the
environment locations based on data of their choice, so it's kind of
expected that that function should be overriden I guess?

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/20180109/835d1f30/attachment.sig>

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Tom Rini]

On Tue, Jan 09, 2018 at 02:10:47PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:
> > >> On 28 November 2017 at 03:24, Maxime Ripard
> > >> <maxime.ripard@free-electrons.com> wrote:
> > >> > Allow boards and architectures to override the default environment lookup
> > >> > code by overriding env_get_location.
> > >> >
> > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > >> > 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 b4d8886e7a69..9b8b38cf3c2b 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)
> > >>
> > >> Is it possible instead to make this deterministic, so that the board
> > >> sets up the location during boot?
> > >
> > > I'm not really sure what you mean here. The default implementation is
> > > deterministic, and the board implementations should make sure that it
> > > is if it's of any value to them.
> > >
> > > Do you want me to add a comment to make that clearer?
> > 
> > I mean that the board presumably knows the location, so could set it
> > up (perhaps in global_data). Then env_get_location() can use it.
> 
> Well, it's pretty much what happens when you only have a single
> environment selected in Kconfig, except that you don't need to change
> anything in all the boards, which would be the case with your solution
> I guess?
> 
> > Making functions weak means everything becomes non-deterministic -
> > e.g. I cannot tell from the code what it is going to do.
> 
> One of the point of this serie is to allow boards to select the
> environment locations based on data of their choice, so it's kind of
> expected that that function should be overriden I guess?

I am generally a fan of using __weak functions as well, with the weak
version being clear about what an overriding version is supposed to do,
when it needs to be overridden.

-- 
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/20180109/af216d83/attachment.sig>

[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Maxime Ripard]

Hi,

On Tue, Jan 09, 2018 at 11:05:51AM -0500, Tom Rini wrote:
> On Tue, Jan 09, 2018 at 02:10:47PM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:
> > > >> On 28 November 2017 at 03:24, Maxime Ripard
> > > >> <maxime.ripard@free-electrons.com> wrote:
> > > >> > Allow boards and architectures to override the default environment lookup
> > > >> > code by overriding env_get_location.
> > > >> >
> > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> > > >> > 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 b4d8886e7a69..9b8b38cf3c2b 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)
> > > >>
> > > >> Is it possible instead to make this deterministic, so that the board
> > > >> sets up the location during boot?
> > > >
> > > > I'm not really sure what you mean here. The default implementation is
> > > > deterministic, and the board implementations should make sure that it
> > > > is if it's of any value to them.
> > > >
> > > > Do you want me to add a comment to make that clearer?
> > > 
> > > I mean that the board presumably knows the location, so could set it
> > > up (perhaps in global_data). Then env_get_location() can use it.
> > 
> > Well, it's pretty much what happens when you only have a single
> > environment selected in Kconfig, except that you don't need to change
> > anything in all the boards, which would be the case with your solution
> > I guess?
> > 
> > > Making functions weak means everything becomes non-deterministic -
> > > e.g. I cannot tell from the code what it is going to do.
> > 
> > One of the point of this serie is to allow boards to select the
> > environment locations based on data of their choice, so it's kind of
> > expected that that function should be overriden I guess?
> 
> I am generally a fan of using __weak functions as well, with the weak
> version being clear about what an overriding version is supposed to do,
> when it needs to be overridden.

I'll add a comment then to detail what is expected from this function,
and any function that might override it.

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

[U-Boot] ARM64 Allwinner Binary Size

[Duncan Hare]

 From: Jagan Teki <jagannadh.teki@gmail.com>
On Wed, Dec 20, 2017 at 11:22 PM, Duncan Hare <dh@synoia.com> wrote:
>> An observation: The Banana PI boards, allwinner based, I've use have a boot.scr file on the fat partition to direct booting.
>> A Suggestion: If this is widespread, the memory used by the u-boot image could be reduce by eliminating much of the
>> pre-defined boot hush scripts.

>not exactly are you trying to avoid env space to not define extra
>variable instead scripts? which board from Bpi are you using?
_______________________________________________________________________________________________
Jagan
We tried the B PI 2 and 3 believe.
We dropped them, because of the inability to manage over-scan. If there is documentation on this we could not find it, nor could the board manufacturer send it to us.

If u-boot is using boot.scr for a boot script, it appears nearly everything else in the environment, especially the predefined scripts, are not needed, and if there is a "size of u-boot issue", this might be a mechanism to reduce u-boot's overall size.

It was a suggestion, Not a recommendation.
 Duncan Hare

714 931 7952


   

[U-Boot] ARM64 Allwinner Binary Size

[Jagan Teki]

On Wed, Dec 20, 2017 at 11:22 PM, Duncan Hare <dh@synoia.com> wrote:
> An observation: The Banana PI boards, allwinner based, I've use have a boot.scr file on the fat partition to direct booting.
> A Suggestion: If this is widespread, the memory used by the u-boot image could be reduce by eliminating much of the
> pre-defined boot hush scripts.

not exactly are you trying to avoid env space to not define extra
variable instead scripts? which board from Bpi are you using?

[U-Boot] ARM64 Allwinner Binary Size

[Duncan Hare]

An observation: The Banana PI boards, allwinner based, I've use have a boot.scr file on the fat partition to direct booting.
A Suggestion: If this is widespread, the memory used by the u-boot image could be reduce by eliminating much of the 
pre-defined boot hush scripts.

I offer this as an observation and suggestion. It may have no merit. Duncan Hare

714 931 7952