[PATCH 0/5] Board specific runtime determined default env

[PATCH 0/5] Board specific runtime determined default env

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Hello Simon, Stefan, Pali,

this series adds support for board specific runtime determined
default environment variables.

IMPORTANT: This series depends on the series
  http://patchwork.ozlabs.org/project/uboot/list/?series=268452

Currently the
  env default [-a]
command uses the default_environment[] buffer to get the default
env variables.

Sometimes it makes sense to have runtime determined default env
settings.

For example the ESPRESSObin board has 4 variants
([ddr3 vs ddr4] x [emmc vs sd]), and each uses different device tree.
Thus the `fdtfile` env variable has different values for these 4
variants. (We can't set this variable via env_set() in some board init
function, because then the user would be unable to overwrite it.)
In order for the command
  env default fdtfile
to work as the user would expect, we need to support overwriting default
environment in runtime.

Pali solved this for ESPRESSObin by declaring the default_environment[]
buffer read-write, instead of read-only, and adding ad-hoc code into
board_late_init() that writes into the default_environment[] buffer.

This ad-hoc code works, but it would be better to have a generic API
for this, since there are other boards which could benefit.

The first 3 patches in this series fix and simplify code in
env/common.c.

The 4th patch adds support for board specific runtime determined
default environment in such a way that if a board code defines function

  const char *board_special_default_env(unsigned i, const char **name);

The default weak implementation of this function is trivial and just
returns NULL.
This function is to be defined in board code, and when defined, it must
return the value of the i-th runtime determined default env variable,
while storing its name into *name.

For example:
  const char *board_special_default_env(unsigned i, const char **name)
  {
    switch (i) {
    case 0:
      *name = "board";
      return is_ddr4() ? "my_board_ddr4" : "my_board";
    case 1:
      *name = "fdtfile";
      return is_ddr4() ? "my-board-ddr4.dtb" : "my-board.dtb";
    default:
      return NULL;
  }

The last patch (NOT TESTED) converts the ESPRESSObin ad-hoc code to
use this API.

Marek

Marek Behún (5):
  env: Don't set ready flag if import failed in env_set_default()
  env: Fix env_get() when returning empty string using env_get_f()
  env: Simplify env_get_default()
  env: Add support for board specific special default environment
  arm: mvebu: Espressobin: Use new API for setting default env at
    runtime

 board/Marvell/mvebu_armada-37xx/board.c     | 120 ++++++++++------
 configs/mvebu_espressobin-88f3720_defconfig |   1 -
 env/common.c                                | 150 ++++++++++++++++----
 include/configs/mvebu_armada-37xx.h         |  17 +--
 include/env_default.h                       |   2 -
 include/env_internal.h                      |   4 -
 6 files changed, 199 insertions(+), 95 deletions(-)

-- 
2.32.0

[PATCH 1/5] env: Don't set ready flag if import failed in env_set_default()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Do not set GD_FLG_ENV_READY nor GD_FLG_ENV_DEFAULT if failed importing
in env_set_default().

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 env/common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 99729ca002..2aa23545ba 100644
--- a/env/common.c
+++ b/env/common.c
@@ -261,9 +261,11 @@ void env_set_default(const char *s, int flags)
 	flags |= H_DEFAULT;
 	if (himport_r(&env_htab, default_environment,
 			sizeof(default_environment), '\0', flags, 0,
-			0, NULL) == 0)
+			0, NULL) == 0) {
 		pr_err("## Error: Environment import failed: errno = %d\n",
 		       errno);
+		return;
+	}
 
 	gd->flags |= GD_FLG_ENV_READY;
 	gd->flags |= GD_FLG_ENV_DEFAULT;
-- 
2.32.0

[PATCH 2/5] env: Fix env_get() when returning empty string using env_get_f()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

The env_get_f() function returns -1 on failure. Returning 0 means that
the variable exists, and is empty string.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 env/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 2aa23545ba..757c5f9ecd 100644
--- a/env/common.c
+++ b/env/common.c
@@ -125,7 +125,7 @@ char *env_get(const char *name)
 	}
 
 	/* restricted capabilities before import */
-	if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
+	if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) >= 0)
 		return (char *)(gd->env_buf);
 
 	return NULL;
-- 
2.32.0

[PATCH 3/5] env: Simplify env_get_default()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Instead of pretending that we don't have environment to force searching
default environment in env_get_default(), get the data from the
default_environment[] buffer directly.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 env/common.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/env/common.c b/env/common.c
index 757c5f9ecd..208e2adaa0 100644
--- a/env/common.c
+++ b/env/common.c
@@ -148,12 +148,10 @@ char *from_env(const char *envvar)
 	return ret;
 }
 
-/*
- * Look up variable from environment for restricted C runtime env.
- */
-int env_get_f(const char *name, char *buf, unsigned len)
+static int env_get_from_linear(const char *env, const char *name, char *buf,
+			       unsigned len)
 {
-	const char *env, *p, *end;
+	const char *p, *end;
 	size_t name_len;
 
 	if (name == NULL || *name == '\0')
@@ -161,11 +159,6 @@ int env_get_f(const char *name, char *buf, unsigned len)
 
 	name_len = strlen(name);
 
-	if (gd->env_valid == ENV_INVALID)
-		env = default_environment;
-	else
-		env = (const char *)gd->env_addr;
-
 	for (p = env; *p != '\0'; p = end + 1) {
 		const char *value;
 		unsigned res;
@@ -193,6 +186,21 @@ int env_get_f(const char *name, char *buf, unsigned len)
 	return -1;
 }
 
+/*
+ * Look up variable from environment for restricted C runtime env.
+ */
+int env_get_f(const char *name, char *buf, unsigned len)
+{
+	const char *env;
+
+	if (gd->env_valid == ENV_INVALID)
+		env = default_environment;
+	else
+		env = (const char *)gd->env_addr;
+
+	return env_get_from_linear(env, name, buf, len);
+}
+
 /**
  * Decode the integer value of an environment variable and return it.
  *
@@ -232,17 +240,12 @@ int env_get_yesno(const char *var)
  */
 char *env_get_default(const char *name)
 {
-	char *ret_val;
-	unsigned long really_valid = gd->env_valid;
-	unsigned long real_gd_flags = gd->flags;
-
-	/* Pretend that the image is bad. */
-	gd->flags &= ~GD_FLG_ENV_READY;
-	gd->env_valid = ENV_INVALID;
-	ret_val = env_get(name);
-	gd->env_valid = really_valid;
-	gd->flags = real_gd_flags;
-	return ret_val;
+	if (env_get_from_linear(default_environment, name,
+				(char *)(gd->env_buf),
+				sizeof(gd->env_buf)) >= 0)
+		return (char *)(gd->env_buf);
+
+	return NULL;
 }
 
 void env_set_default(const char *s, int flags)
-- 
2.32.0

[PATCH 4/5] env: Add support for board specific special default environment

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

The default_environment[] buffer is built at compile time, but sometimes
it makes sense for some default environment variables to be determined
at runtime, for example:
- one board code may support different boards, and needs that
    fdtfile, board, board_name
  are set appropriately when command
    env default -a
  is executed
- some boards may want to prohibit the
    env default -a
  command to remove device MAC addresses stored in
    ethaddr, ethNaddr.
  This is the case for the ESPRESSObin board code, for example, where
  currently the board_late_init() function rewrites the default
  environment array to achieve this.

Add a new board specific function,

  const char *board_special_default_env(unsigned i, const char **name);

which returns the value of i-th board special default environemnt
variable, while storing it's name to *name.

Add default weak implementation of this functions returning NULL.

Add code to default environemnt handlers in env/common.c, which iterate
these special board default environment variables and get it's values in
precedence to values in the default_environment[] buffer.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 env/common.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 96 insertions(+), 3 deletions(-)

diff --git a/env/common.c b/env/common.c
index 208e2adaa0..d157ca562a 100644
--- a/env/common.c
+++ b/env/common.c
@@ -148,6 +148,41 @@ char *from_env(const char *envvar)
 	return ret;
 }
 
+__weak const char *board_special_default_env(unsigned i, const char **name)
+{
+	return NULL;
+}
+
+static int board_special_default_env_get(const char *var, char *buf,
+					 unsigned len)
+{
+	int i;
+
+	/*
+	 * Iterate all board special default env variables, and if one
+	 * exists with the requested name, return it.
+	 */
+	for (i = 0; ; ++i) {
+		const char *name, *value;
+
+		value = board_special_default_env(i, &name);
+		if (!value)
+			break;
+
+		if (!strcmp(var, name)) {
+			unsigned res = strlen(value);
+
+			memcpy(buf, value, min(len, res + 1));
+			if (len <= res)
+				buf[len - 1] = '\0';
+
+			return res;
+		}
+	}
+
+	return -1;
+}
+
 static int env_get_from_linear(const char *env, const char *name, char *buf,
 			       unsigned len)
 {
@@ -157,6 +192,17 @@ static int env_get_from_linear(const char *env, const char *name, char *buf,
 	if (name == NULL || *name == '\0')
 		return -1;
 
+	if (env == default_environment) {
+		int res = board_special_default_env_get(name, buf, len);
+
+		/*
+		 * Board special default envs take precedence over the
+		 * default_environment[] array.
+		 */
+		if (res >= 0)
+			return res;
+	}
+
 	name_len = strlen(name);
 
 	for (p = env; *p != '\0'; p = end + 1) {
@@ -248,6 +294,41 @@ char *env_get_default(const char *name)
 	return NULL;
 }
 
+static int import_board_special_default_envs(bool all, int nvars,
+					     char * const vars[], int flags)
+{
+	int i;
+
+	for (i = 0; ; ++i) {
+		const char *name, *value;
+		struct env_entry e, *ep;
+
+		value = board_special_default_env(i, &name);
+		if (!value)
+			break;
+
+		if (!all) {
+			int j;
+
+			/* If name is not in vars, skip */
+
+			for (j = 0; j < nvars; ++j)
+				if (!strcmp(name, vars[j]))
+					break;
+			if (j == nvars)
+				continue;
+		}
+
+		e.key = name;
+		e.data = (char *)value;
+
+		if (!hsearch_r(e, ENV_ENTER, &ep, &env_htab, flags))
+			return -1;
+	}
+
+	return 0;
+}
+
 void env_set_default(const char *s, int flags)
 {
 	if (s) {
@@ -270,6 +351,12 @@ void env_set_default(const char *s, int flags)
 		return;
 	}
 
+	if (import_board_special_default_envs(true, 0, NULL, flags) < 0) {
+		pr_err("## Error: Board special default environment import failed: errno = %d\n",
+		       errno);
+		return;
+	}
+
 	gd->flags |= GD_FLG_ENV_READY;
 	gd->flags |= GD_FLG_ENV_DEFAULT;
 }
@@ -278,14 +365,20 @@ void env_set_default(const char *s, int flags)
 /* [re]set individual variables to their value in the default environment */
 int env_set_default_vars(int nvars, char * const vars[], int flags)
 {
+	int res;
+
 	/*
 	 * Special use-case: import from default environment
 	 * (and use \0 as a separator)
 	 */
 	flags |= H_NOCLEAR | H_DEFAULT;
-	return himport_r(&env_htab, default_environment,
-				sizeof(default_environment), '\0',
-				flags, 0, nvars, vars);
+	res = himport_r(&env_htab, default_environment,
+			sizeof(default_environment), '\0', flags, 0, nvars,
+			vars);
+	if (!res)
+		return res;
+
+	return import_board_special_default_envs(false, nvars, vars, flags);
 }
 
 /*
-- 
2.32.0

[PATCH 5/5] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

ESPRESSObin's board code uses an ad-hoc solution for ensuring that
ethaddrs are not overwritten by `env default -a` command and that the
`fdtfile` is set to correct value when `env default -a` is called.

This ad-hoc solution is overwriting the default_environment[] buffer in
board_late_init().

Since now we have a specific API for overwriting default environment,
convert this ad-hoc code to this new API.

Since the default_environment[] buffer is not overwritten anymore by any
board, remove support for read-write default_environment[].

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/Marvell/mvebu_armada-37xx/board.c     | 120 ++++++++++++--------
 configs/mvebu_espressobin-88f3720_defconfig |   1 -
 include/configs/mvebu_armada-37xx.h         |  17 +--
 include/env_default.h                       |   2 -
 include/env_internal.h                      |   4 -
 5 files changed, 75 insertions(+), 69 deletions(-)

diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
index d7b6ecafbf..5464482423 100644
--- a/board/Marvell/mvebu_armada-37xx/board.c
+++ b/board/Marvell/mvebu_armada-37xx/board.c
@@ -84,43 +84,16 @@ int board_init(void)
 	return 0;
 }
 
-#ifdef CONFIG_BOARD_LATE_INIT
-int board_late_init(void)
+static bool ram_is_ddr4(void)
 {
-	char *ptr = &default_environment[0];
-	struct udevice *dev;
-	struct mmc *mmc_dev;
-	bool ddr4, emmc;
-	const char *mac;
-	char eth[10];
-	int i;
-
-	if (!of_machine_is_compatible("globalscale,espressobin"))
-		return 0;
-
-	/* Find free buffer in default_environment[] for new variables */
-	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
-	ptr += 2;
-
-	/*
-	 * Ensure that 'env default -a' does not erase permanent MAC addresses
-	 * stored in env variables: $ethaddr, $eth1addr, $eth2addr and $eth3addr
-	 */
-
-	mac = env_get("ethaddr");
-	if (mac && strlen(mac) <= 17)
-		ptr += sprintf(ptr, "ethaddr=%s", mac) + 1;
-
-	for (i = 1; i <= 3; i++) {
-		sprintf(eth, "eth%daddr", i);
-		mac = env_get(eth);
-		if (mac && strlen(mac) <= 17)
-			ptr += sprintf(ptr, "%s=%s", eth, mac) + 1;
-	}
-
-	/* If the memory controller has been configured for DDR4, we're running on v7 */
-	ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
+	return ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
 		& A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4;
+}
+
+static bool has_emmc(void)
+{
+	struct mmc *mmc_dev;
+	bool emmc;
 
 	/* eMMC is mmc dev num 1 */
 	mmc_dev = find_mmc_device(1);
@@ -128,24 +101,79 @@ int board_late_init(void)
 
 	/* if eMMC is not present then remove it from DM */
 	if (!emmc && mmc_dev) {
+		struct udevice *dev;
 		dev = mmc_dev->dev;
 		device_remove(dev, DM_REMOVE_NORMAL);
 		device_unbind(dev);
 	}
 
-	/* Ensure that 'env default -a' set correct value to $fdtfile */
-	if (ddr4 && emmc)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7-emmc.dtb");
-	else if (ddr4)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7.dtb");
-	else if (emmc)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-emmc.dtb");
-	else
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin.dtb");
+	return emmc;
+}
 
-	return 0;
+const char *board_special_default_env(unsigned i, const char **name)
+{
+	static unsigned nvars;
+	static bool built;
+	static struct {
+		const char *name;
+		char *value;
+	} vars[5];
+
+	if (!of_machine_is_compatible("globalscale,espressobin"))
+		return NULL;
+
+	/*
+	 * We can access ethNaddr variables only when environment is valid.
+	 * We can access mmc only if relocated (initr_env() is called after
+	 * initr_mmc(), so at this point mmc device is present.
+	 */
+	if (gd->env_valid != ENV_VALID || !(gd->flags & GD_FLG_RELOC))
+		return NULL;
+
+	if (!built) {
+		const char *names[4] = { "ethaddr", "eth1addr", "eth2addr",
+					 "eth3addr" };
+		bool ddr4, emmc;
+
+		for (i = 0; i < 4; ++i) {
+			const char *mac;
+
+			mac = env_get(names[i]);
+			if (mac && strlen(mac) <= 17) {
+				vars[nvars].name = names[i];
+				vars[nvars].value = strdup(mac);
+				++nvars;
+			}
+		}
+
+		/*
+		 * If the memory controller has been configured for DDR4, we're
+		 * running on v7
+		 */
+		ddr4 = ram_is_ddr4();
+
+		emmc = has_emmc();
+
+		vars[nvars].name = "fdtfile";
+		if (ddr4 && emmc)
+			vars[nvars].value = "marvell/armada-3720-espressobin-v7-emmc.dtb";
+		else if (ddr4)
+			vars[nvars].value = "marvell/armada-3720-espressobin-v7.dtb";
+		else if (emmc)
+			vars[nvars].value = "marvell/armada-3720-espressobin-emmc.dtb";
+		else
+			vars[nvars].value = "marvell/armada-3720-espressobin.dtb";
+		++nvars;
+
+		built = true;
+	}
+
+	if (i >= nvars)
+		return NULL;
+
+	*name = vars[i].name;
+	return vars[i].value;
 }
-#endif
 
 /* Board specific AHCI / SATA enable code */
 int board_ahci_enable(void)
diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
index 3a69954fcd..3dc6da04f8 100644
--- a/configs/mvebu_espressobin-88f3720_defconfig
+++ b/configs/mvebu_espressobin-88f3720_defconfig
@@ -24,7 +24,6 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_ARCH_EARLY_INIT_R=y
 CONFIG_BOARD_EARLY_INIT_F=y
-CONFIG_BOARD_LATE_INIT=y
 # CONFIG_CMD_FLASH is not set
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
index e7f7e772fc..1669eaf715 100644
--- a/include/configs/mvebu_armada-37xx.h
+++ b/include/configs/mvebu_armada-37xx.h
@@ -35,11 +35,6 @@
 /* End of 16M scrubbed by training in bootrom */
 #define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_TEXT_BASE + 0xFF0000)
 
-/*
- * Environment
- */
-#define DEFAULT_ENV_IS_RW		/* required for configuring default fdtfile= */
-
 /*
  * Ethernet Driver configuration
  */
@@ -70,15 +65,6 @@
 
 #include <config_distro_bootcmd.h>
 
-/* filler for default values filled by board_early_init_f() */
-#define ENV_RW_FILLER \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for ethaddr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth1addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth2addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth3addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for fdtfile= */ \
-	""
-
 /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */
 #define CONFIG_EXTRA_ENV_SETTINGS	\
 	"scriptaddr=0x6d00000\0"	\
@@ -88,7 +74,6 @@
 	"kernel_addr=0x7000000\0"	\
 	"kernel_addr_r=0x7000000\0"	\
 	"ramdisk_addr_r=0xa000000\0"	\
-	BOOTENV \
-	ENV_RW_FILLER
+	BOOTENV
 
 #endif /* _CONFIG_MVEBU_ARMADA_37XX_H */
diff --git a/include/env_default.h b/include/env_default.h
index 23430dc70d..a1bdf97d38 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -19,8 +19,6 @@ env_t embedded_environment __UBOOT_ENV_SECTION__(environment) = {
 	{
 #elif defined(DEFAULT_ENV_INSTANCE_STATIC)
 static char default_environment[] = {
-#elif defined(DEFAULT_ENV_IS_RW)
-char default_environment[] = {
 #else
 const char default_environment[] = {
 #endif
diff --git a/include/env_internal.h b/include/env_internal.h
index f74927cd64..e0fc3e0070 100644
--- a/include/env_internal.h
+++ b/include/env_internal.h
@@ -111,11 +111,7 @@ typedef struct environment_s {
 extern env_t embedded_environment;
 #endif /* ENV_IS_EMBEDDED */
 
-#ifdef DEFAULT_ENV_IS_RW
-extern char default_environment[];
-#else
 extern const char default_environment[];
-#endif
 
 #ifndef DO_DEPS_ONLY
 
-- 
2.32.0

Re: [PATCH 0/5] Board specific runtime determined default env

[Simon Glass]

Hi Marek,

On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Hello Simon, Stefan, Pali,
>
> this series adds support for board specific runtime determined

grammarian here

board-specific, runtime-determined

> default environment variables.
>
> IMPORTANT: This series depends on the series
>   http://patchwork.ozlabs.org/project/uboot/list/?series=268452
>
> Currently the
>   env default [-a]
> command uses the default_environment[] buffer to get the default
> env variables.
>
> Sometimes it makes sense to have runtime determined default env
> settings.
>
> For example the ESPRESSObin board has 4 variants
> ([ddr3 vs ddr4] x [emmc vs sd]), and each uses different device tree.
> Thus the `fdtfile` env variable has different values for these 4
> variants. (We can't set this variable via env_set() in some board init
> function, because then the user would be unable to overwrite it.)
> In order for the command
>   env default fdtfile
> to work as the user would expect, we need to support overwriting default
> environment in runtime.
>
> Pali solved this for ESPRESSObin by declaring the default_environment[]
> buffer read-write, instead of read-only, and adding ad-hoc code into
> board_late_init() that writes into the default_environment[] buffer.
>
> This ad-hoc code works, but it would be better to have a generic API
> for this, since there are other boards which could benefit.
>
> The first 3 patches in this series fix and simplify code in
> env/common.c.
>
> The 4th patch adds support for board specific runtime determined
> default environment in such a way that if a board code defines function
>
>   const char *board_special_default_env(unsigned i, const char **name);
>
> The default weak implementation of this function is trivial and just
> returns NULL.
> This function is to be defined in board code, and when defined, it must
> return the value of the i-th runtime determined default env variable,
> while storing its name into *name.
>
> For example:
>   const char *board_special_default_env(unsigned i, const char **name)
>   {
>     switch (i) {
>     case 0:
>       *name = "board";
>       return is_ddr4() ? "my_board_ddr4" : "my_board";
>     case 1:
>       *name = "fdtfile";
>       return is_ddr4() ? "my-board-ddr4.dtb" : "my-board.dtb";
>     default:
>       return NULL;
>   }
>
> The last patch (NOT TESTED) converts the ESPRESSObin ad-hoc code to
> use this API.
>
> Marek
>
> Marek Behún (5):
>   env: Don't set ready flag if import failed in env_set_default()
>   env: Fix env_get() when returning empty string using env_get_f()
>   env: Simplify env_get_default()
>   env: Add support for board specific special default environment
>   arm: mvebu: Espressobin: Use new API for setting default env at
>     runtime
>
>  board/Marvell/mvebu_armada-37xx/board.c     | 120 ++++++++++------
>  configs/mvebu_espressobin-88f3720_defconfig |   1 -
>  env/common.c                                | 150 ++++++++++++++++----
>  include/configs/mvebu_armada-37xx.h         |  17 +--
>  include/env_default.h                       |   2 -
>  include/env_internal.h                      |   4 -
>  6 files changed, 199 insertions(+), 95 deletions(-)
>
> --
> 2.32.0
>

Regards,
Simon

Re: [PATCH 1/5] env: Don't set ready flag if import failed in env_set_default()

[Simon Glass]

On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Do not set GD_FLG_ENV_READY nor GD_FLG_ENV_DEFAULT if failed importing
> in env_set_default().
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  env/common.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

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

Re: [PATCH 2/5] env: Fix env_get() when returning empty string using env_get_f()

[Simon Glass]

Hi Marek,

On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> The env_get_f() function returns -1 on failure. Returning 0 means that
> the variable exists, and is empty string.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  env/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

But it isn't normally possible to set an env var to an empty string.
How does this happen?

>
> diff --git a/env/common.c b/env/common.c
> index 2aa23545ba..757c5f9ecd 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -125,7 +125,7 @@ char *env_get(const char *name)
>         }
>
>         /* restricted capabilities before import */
> -       if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
> +       if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) >= 0)
>                 return (char *)(gd->env_buf);
>
>         return NULL;
> --
> 2.32.0
>

Regards,
Simon

Re: [PATCH 3/5] env: Simplify env_get_default()

[Simon Glass]

On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Instead of pretending that we don't have environment to force searching
> default environment in env_get_default(), get the data from the
> default_environment[] buffer directly.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  env/common.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)

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

Re: [PATCH 4/5] env: Add support for board specific special default environment

[Simon Glass]

Hi Marek,

On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> The default_environment[] buffer is built at compile time, but sometimes
> it makes sense for some default environment variables to be determined
> at runtime, for example:
> - one board code may support different boards, and needs that
>     fdtfile, board, board_name
>   are set appropriately when command
>     env default -a
>   is executed
> - some boards may want to prohibit the
>     env default -a
>   command to remove device MAC addresses stored in
>     ethaddr, ethNaddr.
>   This is the case for the ESPRESSObin board code, for example, where
>   currently the board_late_init() function rewrites the default
>   environment array to achieve this.
>
> Add a new board specific function,
>
>   const char *board_special_default_env(unsigned i, const char **name);
>
> which returns the value of i-th board special default environemnt
> variable, while storing it's name to *name.
>
> Add default weak implementation of this functions returning NULL.
>
> Add code to default environemnt handlers in env/common.c, which iterate
> these special board default environment variables and get it's values in
> precedence to values in the default_environment[] buffer.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  env/common.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 96 insertions(+), 3 deletions(-)

What do you think about using a sysinfo driver for this? There are
already a lot of weak functions in U-Boot.

Regards,
Simon

Re: [PATCH 5/5] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Simon Glass]

Hi Marek,

On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> ethaddrs are not overwritten by `env default -a` command and that the
> `fdtfile` is set to correct value when `env default -a` is called.
>
> This ad-hoc solution is overwriting the default_environment[] buffer in
> board_late_init().
>
> Since now we have a specific API for overwriting default environment,
> convert this ad-hoc code to this new API.
>
> Since the default_environment[] buffer is not overwritten anymore by any
> board, remove support for read-write default_environment[].
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  board/Marvell/mvebu_armada-37xx/board.c     | 120 ++++++++++++--------
>  configs/mvebu_espressobin-88f3720_defconfig |   1 -
>  include/configs/mvebu_armada-37xx.h         |  17 +--
>  include/env_default.h                       |   2 -
>  include/env_internal.h                      |   4 -
>  5 files changed, 75 insertions(+), 69 deletions(-)

Can you split out the generic changes into their own patch?

Regards,
Simon

Re: [PATCH 2/5] env: Fix env_get() when returning empty string using env_get_f()

[Marek Behún]

On Thu, 28 Oct 2021 21:17:38 -0600
Simon Glass <sjg@chromium.org> wrote:

> Hi Marek,
> 
> On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > The env_get_f() function returns -1 on failure. Returning 0 means that
> > the variable exists, and is empty string.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  env/common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)  
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But it isn't normally possible to set an env var to an empty string.
> How does this happen?

Hmm, I don't see code in cmd/nvedit.c's _do_env_set() that would
prohibit empty string as value...

Why isn't it possible?

Re: [PATCH 4/5] env: Add support for board specific special default environment

[Marek Behún]

On Thu, 28 Oct 2021 21:17:41 -0600
Simon Glass <sjg@chromium.org> wrote:

> Hi Marek,
> 
> On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > The default_environment[] buffer is built at compile time, but sometimes
> > it makes sense for some default environment variables to be determined
> > at runtime, for example:
> > - one board code may support different boards, and needs that
> >     fdtfile, board, board_name
> >   are set appropriately when command
> >     env default -a
> >   is executed
> > - some boards may want to prohibit the
> >     env default -a
> >   command to remove device MAC addresses stored in
> >     ethaddr, ethNaddr.
> >   This is the case for the ESPRESSObin board code, for example, where
> >   currently the board_late_init() function rewrites the default
> >   environment array to achieve this.
> >
> > Add a new board specific function,
> >
> >   const char *board_special_default_env(unsigned i, const char **name);
> >
> > which returns the value of i-th board special default environemnt
> > variable, while storing it's name to *name.
> >
> > Add default weak implementation of this functions returning NULL.
> >
> > Add code to default environemnt handlers in env/common.c, which iterate
> > these special board default environment variables and get it's values in
> > precedence to values in the default_environment[] buffer.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  env/common.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 96 insertions(+), 3 deletions(-)  
> 
> What do you think about using a sysinfo driver for this? There are
> already a lot of weak functions in U-Boot.

I confess I am looking at sysinfo API for the first time just now. It
seems like a reasonable thing, but how to combine with default
environment for this?

The thing is that Pali's code for ESPRESSObin needs to prohibit
deleting the ethaddrs from env with "env default -a", because there
were issues with the mac address being lost...

Marek

Re: [PATCH 2/5] env: Fix env_get() when returning empty string using env_get_f()

[Pali Rohár]

On Thursday 28 October 2021 21:17:38 Simon Glass wrote:
> Hi Marek,
> 
> On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > The env_get_f() function returns -1 on failure. Returning 0 means that
> > the variable exists, and is empty string.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  env/common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But it isn't normally possible to set an env var to an empty string.
> How does this happen?

IIRC you can set variable to empty string via e.g.:

setenv abc ''

> >
> > diff --git a/env/common.c b/env/common.c
> > index 2aa23545ba..757c5f9ecd 100644
> > --- a/env/common.c
> > +++ b/env/common.c
> > @@ -125,7 +125,7 @@ char *env_get(const char *name)
> >         }
> >
> >         /* restricted capabilities before import */
> > -       if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
> > +       if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) >= 0)
> >                 return (char *)(gd->env_buf);
> >
> >         return NULL;
> > --
> > 2.32.0
> >
> 
> Regards,
> Simon

Re: [PATCH 4/5] env: Add support for board specific special default environment

[Simon Glass]

Hi Marek,

On Fri, 29 Oct 2021 at 02:58, Marek Behún <kabel@kernel.org> wrote:
>
> On Thu, 28 Oct 2021 21:17:41 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Marek,
> >
> > On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > The default_environment[] buffer is built at compile time, but sometimes
> > > it makes sense for some default environment variables to be determined
> > > at runtime, for example:
> > > - one board code may support different boards, and needs that
> > >     fdtfile, board, board_name
> > >   are set appropriately when command
> > >     env default -a
> > >   is executed
> > > - some boards may want to prohibit the
> > >     env default -a
> > >   command to remove device MAC addresses stored in
> > >     ethaddr, ethNaddr.
> > >   This is the case for the ESPRESSObin board code, for example, where
> > >   currently the board_late_init() function rewrites the default
> > >   environment array to achieve this.
> > >
> > > Add a new board specific function,
> > >
> > >   const char *board_special_default_env(unsigned i, const char **name);
> > >
> > > which returns the value of i-th board special default environemnt
> > > variable, while storing it's name to *name.
> > >
> > > Add default weak implementation of this functions returning NULL.
> > >
> > > Add code to default environemnt handlers in env/common.c, which iterate
> > > these special board default environment variables and get it's values in
> > > precedence to values in the default_environment[] buffer.
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  env/common.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 96 insertions(+), 3 deletions(-)
> >
> > What do you think about using a sysinfo driver for this? There are
> > already a lot of weak functions in U-Boot.
>
> I confess I am looking at sysinfo API for the first time just now. It
> seems like a reasonable thing, but how to combine with default
> environment for this?
>
> The thing is that Pali's code for ESPRESSObin needs to prohibit
> deleting the ethaddrs from env with "env default -a", because there
> were issues with the mac address being lost...

The way you have done it seems reasonable to me, i.e. calling an
iterator function to get the strings. Could it be done with sysinfo's
get_str()? Perhaps we should add get_str_list() as a new method that
takes and id and also an index?

Regards,
Simon

Re: [PATCH 2/5] env: Fix env_get() when returning empty string using env_get_f()

[Simon Glass]

Hi,

On Fri, 29 Oct 2021 at 03:03, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 28 October 2021 21:17:38 Simon Glass wrote:
> > Hi Marek,
> >
> > On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > The env_get_f() function returns -1 on failure. Returning 0 means that
> > > the variable exists, and is empty string.
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  env/common.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But it isn't normally possible to set an env var to an empty string.
> > How does this happen?
>
> IIRC you can set variable to empty string via e.g.:
>
> setenv abc ''

Yes that works and I now see you are all right. In fact the command
handling for 'env set' does not use env_set().

It seems a bit inconsistent to me. Since a deleted variable is
considered empty, do we need to support empty vars?

Regards,
Simon

Re: [PATCH 2/5] env: Fix env_get() when returning empty string using env_get_f()

[Marek Behún]

On Sun, 31 Oct 2021 07:07:47 -0600
Simon Glass <sjg@chromium.org> wrote:

> Hi,
> 
> On Fri, 29 Oct 2021 at 03:03, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 28 October 2021 21:17:38 Simon Glass wrote:  
> > > Hi Marek,
> > >
> > > On Wed, 27 Oct 2021 at 21:28, Marek Behún <kabel@kernel.org> wrote:  
> > > >
> > > > From: Marek Behún <marek.behun@nic.cz>
> > > >
> > > > The env_get_f() function returns -1 on failure. Returning 0 means that
> > > > the variable exists, and is empty string.
> > > >
> > > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > > ---
> > > >  env/common.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)  
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > But it isn't normally possible to set an env var to an empty string.
> > > How does this happen?  
> >
> > IIRC you can set variable to empty string via e.g.:
> >
> > setenv abc ''  
> 
> Yes that works and I now see you are all right. In fact the command
> handling for 'env set' does not use env_set().
> 
> It seems a bit inconsistent to me. Since a deleted variable is
> considered empty, do we need to support empty vars?

Depends on what you mean by "support". I think that if the user
does

  setenv xyz ""

it shouldn't be an error. Whether it deletes the variable or not is
something different (although I would make it so that the variable is
not deleted...).

But env_get_f("xyz") shouldn't return -1 in this case, as that indicates
an error.

Marek

Re: [PATCH 5/5] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Pali Rohár]

On Thursday 28 October 2021 05:28:10 Marek Behún wrote:
> diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
> index 3a69954fcd..3dc6da04f8 100644
> --- a/configs/mvebu_espressobin-88f3720_defconfig
> +++ b/configs/mvebu_espressobin-88f3720_defconfig
> @@ -24,7 +24,6 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
>  CONFIG_DISPLAY_BOARDINFO_LATE=y
>  CONFIG_ARCH_EARLY_INIT_R=y
>  CONFIG_BOARD_EARLY_INIT_F=y
> -CONFIG_BOARD_LATE_INIT=y

This would not work. Detection of emmc is done in board_late_init()
function and cannot be done earlier as it depends on device
initialization. Also in this function is removal of emmc from DM if emmc
is not present.

>  # CONFIG_CMD_FLASH is not set
>  CONFIG_CMD_GPIO=y
>  CONFIG_CMD_GPT=y

Re: [PATCH 5/5] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Simon Glass]

On Sun, 31 Oct 2021 at 14:15, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 28 October 2021 05:28:10 Marek Behún wrote:
> > diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
> > index 3a69954fcd..3dc6da04f8 100644
> > --- a/configs/mvebu_espressobin-88f3720_defconfig
> > +++ b/configs/mvebu_espressobin-88f3720_defconfig
> > @@ -24,7 +24,6 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
> >  CONFIG_DISPLAY_BOARDINFO_LATE=y
> >  CONFIG_ARCH_EARLY_INIT_R=y
> >  CONFIG_BOARD_EARLY_INIT_F=y
> > -CONFIG_BOARD_LATE_INIT=y
>
> This would not work. Detection of emmc is done in board_late_init()
> function and cannot be done earlier as it depends on device
> initialization. Also in this function is removal of emmc from DM if emmc
> is not present.
>
> >  # CONFIG_CMD_FLASH is not set
> >  CONFIG_CMD_GPIO=y
> >  CONFIG_CMD_GPT=y

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

Re: [PATCH 5/5] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Pali Rohár]

On Tuesday 02 November 2021 08:57:51 Simon Glass wrote:
> On Sun, 31 Oct 2021 at 14:15, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 28 October 2021 05:28:10 Marek Behún wrote:
> > > diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
> > > index 3a69954fcd..3dc6da04f8 100644
> > > --- a/configs/mvebu_espressobin-88f3720_defconfig
> > > +++ b/configs/mvebu_espressobin-88f3720_defconfig
> > > @@ -24,7 +24,6 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
> > >  CONFIG_DISPLAY_BOARDINFO_LATE=y
> > >  CONFIG_ARCH_EARLY_INIT_R=y
> > >  CONFIG_BOARD_EARLY_INIT_F=y
> > > -CONFIG_BOARD_LATE_INIT=y
> >
> > This would not work. Detection of emmc is done in board_late_init()
> > function and cannot be done earlier as it depends on device
> > initialization. Also in this function is removal of emmc from DM if emmc
> > is not present.
> >
> > >  # CONFIG_CMD_FLASH is not set
> > >  CONFIG_CMD_GPIO=y
> > >  CONFIG_CMD_GPT=y
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

I was discussing with Marek and this patch needs to be reworked as in
current state it does not work.