[U-Boot] [PATCH v2 00/15] env: Multiple env support and env transition for sunxi
Hi,
Here is a second attempt at transitioning away from the MMC raw environment to a FAT-based one for Allwinner SoCs. Since the RFC was quite well received, I actually tested it and fixed a few rough edges.
You'll find the first RFC here for reference: https://lists.denx.de/pipermail/u-boot/2017-October/310111.html
And the second that originated in this series: https://lists.denx.de/pipermail/u-boot/2017-November/311608.html
I gave it a travis run, and one test seems to fail in one test: https://travis-ci.org/mripard/u-boot/jobs/329229382
I really cannot really make any sense of why a change in the way the environment loads could affect the operation of a DHCP client. Is there a way to reproduce locally?
Thanks! Maxime
Original description of the patches:
The fundamental issue I'm trying to adress is that we've had for a very long time the assumption that the main U-Boot binary wouldn't exceed around 500 bytes.
However, we're starting to get real close to that limit, and are running out of silver bullets to deal with the consequences of having a bigger U-Boot binary, the main consequence being that we would have some overlap between the environment and U-Boot.
One way to address this that has been suggested by Tom is to move away from the raw MMC environment to a FAT-based one. This would allow us to slowly migrate away, and eventually remove the MMC-raw option entirely to reclaim that space for the binary.
That cannot be done in a single release however, since we might have environments in the wild already that people rely on. And since we always encouraged people to use the raw MMC environment, noone would expect that.
This is even worse since some platforms are using the U-Boot environment to deal with implement their upgrade mechanism, such as mender.io, and force moving the environment would break any further upgrade.
The suggested implementation is to allow U-Boot to compile multiple environments backend at once, based on the work done by Simon. The default behaviour shouldn't change obviously. We can then piggy-back on this to tweak on a per-board basis the environment lookup algorithm to always favour the FAT-based environment and then fallback to the MMC. It will allow us to migrate a raw-MMC user to a FAT based solution as soon as they update their environment (assuming that there is a bootable FAT partition in the system).
This has just been compile tested on sunxi so far, and I'd like general comments on the approach taken. Obviously, this will need to work properly before being merged.
Changes from v1: - Collect tags - Rebased on v2018.01 - Fixed build failures on a couple of boards - Added back the message and the error code when an environment is failing - Added some comments about the printf in environments - Added a build time check about the number of our locations array to see if we're overflowing the location variable - Fixed the drv->init return code being ignored - Added helpers to manage the init status - Changed the ENVO prefix for the operations to ENVOP - Added some comments and documentation
Changes from the RFC: - Added more useful messages to see where we're loading / saving - Init all the environments no matter what, and the deal with whatever env we want to pick at load time - Added the various tags collected
Maxime Ripard (15): cmd: nvedit: Get rid of the env lookup env: Rename env_driver_lookup_default and env_get_default_location env: Pass additional parameters to the env lookup function env: Make the env save message a bit more explicit env: Make it explicit where we're loading our environment from env: fat: Make the debug messages play a little nicer env: mmc: Make the debug messages play a little nicer env: common: Make the debug messages play a little nicer env: Support multiple environments env: Initialise all the environments env: mmc: depends on the MMC framework env: Allow to build multiple environments in Kconfig env: Mark env_get_location as weak sunxi: Transition from the MMC to a FAT-based environment env: sunxi: Enable FAT-based environment support by default
board/sunxi/board.c | 16 ++- cmd/nvedit.c | 4 +- configs/MPC8313ERDB_NAND_33_defconfig | 1 +- configs/MPC8313ERDB_NAND_66_defconfig | 1 +- configs/cl-som-imx7_defconfig | 1 +- configs/microblaze-generic_defconfig | 1 +- env/Kconfig | 70 ++++---- env/common.c | 2 +- env/env.c | 252 +++++++++++++++++++-------- env/fat.c | 25 ++- env/mmc.c | 1 +- include/asm-generic/global_data.h | 1 +- include/environment.h | 15 +- 13 files changed, 267 insertions(+), 123 deletions(-)
base-commit: f3dd87e0b98999a78e500e8c6d2b063ebadf535a
The nvedit command is the only user of env_driver_lookup_default outside of the environment code itself, and it uses it only to print the environment it's about to save to during env save.
As we're about to rework the environment to be able to handle multiple environment sources, we might not have an idea of what environment backend is going to be used before trying (and possibly failing for some).
Therefore, it makes sense to remove that message and move it to the env_save function itself. As a side effect, we also can get rid of the call to env_driver_lookup_default that is also about to get refactored.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- cmd/nvedit.c | 4 ---- env/env.c | 4 +++- include/environment.h | 7 ------- 3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 4e79d03856fe..a690d743cd46 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -708,10 +708,6 @@ ulong env_get_ulong(const char *name, int base, ulong default_val) static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - struct env_driver *env = env_driver_lookup_default(); - - printf("Saving Environment to %s...\n", env->name); - return env_save() ? 1 : 0; }
diff --git a/env/env.c b/env/env.c index 76a5608628fc..094538ff5b62 100644 --- a/env/env.c +++ b/env/env.c @@ -52,7 +52,7 @@ static enum env_location env_get_default_location(void) return ENVL_UNKNOWN; }
-struct env_driver *env_driver_lookup_default(void) +static struct env_driver *env_driver_lookup_default(void) { enum env_location loc = env_get_default_location(); struct env_driver *drv; @@ -115,6 +115,8 @@ int env_save(void) return -ENODEV; if (!drv->save) return -ENOSYS; + + printf("Saving Environment to %s...\n", drv->name); ret = drv->save(); if (ret) { debug("%s: Environment failed to save (err=%d)\n", __func__, diff --git a/include/environment.h b/include/environment.h index d29f82cb5d6f..a2015c299aa9 100644 --- a/include/environment.h +++ b/include/environment.h @@ -293,13 +293,6 @@ int env_import_redund(const char *buf1, const char *buf2); #endif
/** - * env_driver_lookup_default() - Look up the default environment driver - * - * @return pointer to driver, or NULL if none (which should not happen) - */ -struct env_driver *env_driver_lookup_default(void); - -/** * env_get_char() - Get a character from the early environment * * This reads from the pre-relocation environemnt
The env_driver_lookup_default and env_get_default_location functions are about to get refactored to support loading from multiple environment.
The name is therefore not really well suited anymore. Drop the default part to be a bit more relevant.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/env.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/env/env.c b/env/env.c index 094538ff5b62..97ada5b5a6fd 100644 --- a/env/env.c +++ b/env/env.c @@ -10,7 +10,7 @@
DECLARE_GLOBAL_DATA_PTR;
-static struct env_driver *env_driver_lookup(enum env_location loc) +static struct env_driver *_env_driver_lookup(enum env_location loc) { struct env_driver *drv; const int n_ents = ll_entry_count(struct env_driver, env_driver); @@ -26,7 +26,7 @@ static struct env_driver *env_driver_lookup(enum env_location loc) return NULL; }
-static enum env_location env_get_default_location(void) +static enum env_location env_get_location(void) { if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM) return ENVL_EEPROM; @@ -52,12 +52,12 @@ static enum env_location env_get_default_location(void) return ENVL_UNKNOWN; }
-static struct env_driver *env_driver_lookup_default(void) +static struct env_driver *env_driver_lookup(void) { - enum env_location loc = env_get_default_location(); + enum env_location loc = env_get_location(); struct env_driver *drv;
- drv = env_driver_lookup(loc); + drv = _env_driver_lookup(loc); if (!drv) { debug("%s: No environment driver for location %d\n", __func__, loc); @@ -69,7 +69,7 @@ static struct env_driver *env_driver_lookup_default(void)
int env_get_char(int index) { - struct env_driver *drv = env_driver_lookup_default(); + struct env_driver *drv = env_driver_lookup(); int ret;
if (gd->env_valid == ENV_INVALID) @@ -89,7 +89,7 @@ int env_get_char(int index)
int env_load(void) { - struct env_driver *drv = env_driver_lookup_default(); + struct env_driver *drv = env_driver_lookup(); int ret = 0;
if (!drv) @@ -108,7 +108,7 @@ int env_load(void)
int env_save(void) { - struct env_driver *drv = env_driver_lookup_default(); + struct env_driver *drv = env_driver_lookup(); int ret;
if (!drv) @@ -129,7 +129,7 @@ int env_save(void)
int env_init(void) { - struct env_driver *drv = env_driver_lookup_default(); + struct env_driver *drv = env_driver_lookup(); int ret = -ENOENT;
if (!drv)
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 | 135 +++++++++++++++++++++++++++---------------- include/environment.h | 8 +++- 2 files changed, 94 insertions(+), 49 deletions(-)
diff --git a/env/env.c b/env/env.c index 97ada5b5a6fd..4dc39b384c1e 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,27 @@ static enum env_location env_get_location(void) return ENVL_UNKNOWN; }
-static struct env_driver *env_driver_lookup(void) + +/** + * env_driver_lookup() - Finds the most suited environment location + * @op: operations performed on the environment + * @prio: priority between the multiple environments, 0 being the + * highest priority + * + * This will try to find the available environment with the highest + * priority in the system. + * + * Returns: + * NULL on error, a pointer to a struct env_driver otherwise + */ +static struct env_driver *env_driver_lookup(enum env_operation op, int prio) { - enum env_location loc = env_get_location(); + enum env_location loc = env_get_location(op, prio); struct env_driver *drv;
+ if (loc == ENVL_UNKNOWN) + return NULL; + drv = _env_driver_lookup(loc); if (!drv) { debug("%s: No environment driver for location %d\n", __func__, @@ -69,83 +88,101 @@ static struct env_driver *env_driver_lookup(void)
int env_get_char(int index) { - struct env_driver *drv = env_driver_lookup(); - int ret; + struct env_driver *drv; + int prio;
if (gd->env_valid == ENV_INVALID) return default_environment[index]; - if (!drv) - return -ENODEV; - if (!drv->get_char) - return *(uchar *)(gd->env_addr + index); - ret = drv->get_char(index); - if (ret < 0) { - debug("%s: Environment failed to load (err=%d)\n", - __func__, ret); + + for (prio = 0; (drv = env_driver_lookup(ENVOP_GET_CHAR, prio)); prio++) { + int ret; + + if (!drv->get_char) + continue; + + ret = drv->get_char(index); + if (!ret) + return 0; + + debug("%s: Environment %s failed to load (err=%d)\n", __func__, + drv->name, ret); }
- return ret; + return -ENODEV; }
int env_load(void) { - struct env_driver *drv = env_driver_lookup(); - int ret = 0; + struct env_driver *drv; + int prio;
- if (!drv) - return -ENODEV; - if (!drv->load) - return 0; - ret = drv->load(); - if (ret) { - debug("%s: Environment failed to load (err=%d)\n", __func__, - ret); - return ret; + for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { + int ret; + + if (!drv->load) + continue; + + ret = drv->load(); + if (!ret) + return 0; + + debug("%s: Environment %s failed to load (err=%d)\n", __func__, + drv->name, ret); }
- return 0; + return -ENODEV; }
int env_save(void) { - struct env_driver *drv = env_driver_lookup(); - int ret; + struct env_driver *drv; + int prio;
- if (!drv) - return -ENODEV; - if (!drv->save) - return -ENOSYS; - - printf("Saving Environment to %s...\n", drv->name); - ret = drv->save(); - if (ret) { - debug("%s: Environment failed to save (err=%d)\n", __func__, - ret); - return ret; + for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) { + int ret; + + if (!drv->save) + continue; + + printf("Saving Environment to %s...\n", drv->name); + ret = drv->save(); + if (!ret) + return 0; + + debug("%s: Environment %s failed to save (err=%d)\n", __func__, + drv->name, ret); }
- return 0; + return -ENODEV; }
int env_init(void) { - struct env_driver *drv = env_driver_lookup(); + struct env_driver *drv; int ret = -ENOENT; + int prio; + + for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { + if (!drv->init) + continue;
- if (!drv) - return -ENODEV; - if (drv->init) ret = drv->init(); + if (!ret) + return 0; + + debug("%s: Environment %s failed to init (err=%d)\n", __func__, + drv->name, ret); + } + + if (!prio) + return -ENODEV; + if (ret == -ENOENT) { gd->env_addr = (ulong)&default_environment[0]; gd->env_valid = ENV_VALID;
return 0; - } else if (ret) { - debug("%s: Environment failed to init (err=%d)\n", __func__, - ret); - return ret; }
- return 0; + return ret; } diff --git a/include/environment.h b/include/environment.h index a2015c299aa9..a4060506fabb 100644 --- a/include/environment.h +++ b/include/environment.h @@ -205,6 +205,14 @@ enum env_location { ENVL_UNKNOWN, };
+/* value for the various operations we want to perform on the env */ +enum env_operation { + ENVOP_GET_CHAR, /* we want to call the get_char function */ + ENVOP_INIT, /* we want to call the init function */ + ENVOP_LOAD, /* we want to call the load function */ + ENVOP_SAVE, /* we want to call the save function */ +}; + struct env_driver { const char *name; enum env_location location;
Hi Maxime,
On 16 January 2018 at 01:16, 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 | 135 +++++++++++++++++++++++++++---------------- include/environment.h | 8 +++- 2 files changed, 94 insertions(+), 49 deletions(-)
diff --git a/env/env.c b/env/env.c index 97ada5b5a6fd..4dc39b384c1e 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)
Please add a function comment, including why @op is needed.
{
if (prio >= 1)return ENVL_UNKNOWN;
What is this for? Can you please add a comment?
Regards, Simon
On Wed, Jan 17, 2018 at 03:03:40PM -0700, Simon Glass wrote:
Hi Maxime,
On 16 January 2018 at 01:16, 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 | 135 +++++++++++++++++++++++++++---------------- include/environment.h | 8 +++- 2 files changed, 94 insertions(+), 49 deletions(-)
diff --git a/env/env.c b/env/env.c index 97ada5b5a6fd..4dc39b384c1e 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)
Please add a function comment, including why @op is needed.
This was done in a later patch (the __weak one), but you're right that it makes more sense for it to be here.
{
if (prio >= 1)return ENVL_UNKNOWN;What is this for? Can you please add a comment?
Done, thanks! Maxime
Since we'll soon have support for multiple environments, the environment saving message might end up being printed multiple times if the higher priority environment cannot be used.
That might confuse the user, so let's make it explicit if the operation failed or not.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/env.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/env/env.c b/env/env.c index 4dc39b384c1e..afffab4c002a 100644 --- a/env/env.c +++ b/env/env.c @@ -144,8 +144,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;
Since we can have multiple environments now, it's better to provide a decent indication on what environments were tried and which were the one to fail and succeed.
Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/env.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/env/env.c b/env/env.c index afffab4c002a..9dd4d4cb4d11 100644 --- a/env/env.c +++ b/env/env.c @@ -122,12 +122,15 @@ int env_load(void) if (!drv->load) continue;
+ printf("Loading Environment from %s... ", drv->name); ret = drv->load(); + if (ret) + printf("Failed (%d)\n", ret); + else + printf("OK\n"); + if (!ret) return 0; - - debug("%s: Environment %s failed to load (err=%d)\n", __func__, - drv->name, ret); }
return -ENODEV; @@ -146,12 +149,13 @@ int env_save(void)
printf("Saving Environment to %s... ", drv->name); ret = drv->save(); - printf("%s\n", ret ? "Failed" : "OK"); + if (ret) + printf("Failed (%d)\n", ret); + else + printf("OK\n"); + if (!ret) return 0; - - debug("%s: Environment %s failed to save (err=%d)\n", __func__, - drv->name, ret); }
return -ENODEV;
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make FAT play a little nicer by removing all the extra \n and formatting that is redundant with the global output.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/fat.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/env/fat.c b/env/fat.c index ec49c3905369..158a9a34357b 100644 --- a/env/fat.c +++ b/env/fat.c @@ -55,7 +55,11 @@ static int env_fat_save(void)
dev = dev_desc->devnum; if (fat_set_blk_dev(dev_desc, &info) != 0) { - printf("\n** Unable to use %s %d:%d for saveenv **\n", + /* + * This printf is embedded in the messages from env_save that + * will calling it. The missing \n is intentional. + */ + printf("Unable to use %s %d:%d... ", CONFIG_ENV_FAT_INTERFACE, dev, part); return 1; } @@ -63,12 +67,15 @@ static int env_fat_save(void) err = file_fat_write(CONFIG_ENV_FAT_FILE, (void *)&env_new, 0, sizeof(env_t), &size); if (err == -1) { - printf("\n** Unable to write "%s" from %s%d:%d **\n", + /* + * This printf is embedded in the messages from env_save that + * will calling it. The missing \n is intentional. + */ + printf("Unable to write "%s" from %s%d:%d... ", CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part); return 1; }
- puts("done\n"); return 0; } #endif /* CMD_SAVEENV */ @@ -90,14 +97,22 @@ static int env_fat_load(void)
dev = dev_desc->devnum; if (fat_set_blk_dev(dev_desc, &info) != 0) { - printf("\n** Unable to use %s %d:%d for loading the env **\n", + /* + * This printf is embedded in the messages from env_save that + * will calling it. The missing \n is intentional. + */ + printf("Unable to use %s %d:%d... ", CONFIG_ENV_FAT_INTERFACE, dev, part); goto err_env_relocate; }
err = file_fat_read(CONFIG_ENV_FAT_FILE, buf, CONFIG_ENV_SIZE); if (err == -1) { - printf("\n** Unable to read "%s" from %s%d:%d **\n", + /* + * This printf is embedded in the messages from env_save that + * will calling it. The missing \n is intentional. + */ + printf("Unable to read "%s" from %s%d:%d... ", CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part); goto err_env_relocate; }
Hi Maxime,
On 16 January 2018 at 01:16, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make FAT play a little nicer by removing all the extra \n and formatting that is redundant with the global output.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/fat.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
Do you have a change log for this patch?
Regards, Simon
On Wed, Jan 17, 2018 at 03:03:47PM -0700, Simon Glass wrote:
Hi Maxime,
On 16 January 2018 at 01:16, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make FAT play a little nicer by removing all the extra \n and formatting that is redundant with the global output.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/fat.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
Do you have a change log for this patch?
Sorry, it was buried in my cover letter.
You have the diff here: http://code.bulix.org/g3emu3-259575
tl; dr: I added some comments around the printf's to make it obvious that the missing \n were intentional.
Maxime
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make MMC play a little nicer by removing all the extra \n and formatting that is redundant with the global output.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/mmc.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c index ed7bcf16ae0e..528fbf978162 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -233,7 +233,6 @@ static int env_mmc_save(void) goto fini; }
- puts("done\n"); ret = 0;
#ifdef CONFIG_ENV_OFFSET_REDUND
On 16 January 2018 at 01:16, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make MMC play a little nicer by removing all the extra \n and formatting that is redundant with the global output.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/mmc.c | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make the common code play a little nicer by removing all the extra output in the standard case.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/common.c b/env/common.c index 8167ea299264..c633502d68bb 100644 --- a/env/common.c +++ b/env/common.c @@ -78,7 +78,7 @@ void set_default_env(const char *s) puts(s); } } else { - puts("Using default environment\n\n"); + debug("Using default environment\n"); }
if (himport_r(&env_htab, (char *)default_environment,
On 16 January 2018 at 01:16, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.
Make the common code play a little nicer by removing all the extra output in the standard case.
Reviewed-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Now that we have everything in place to support multiple environment, let's make sure the current code can use it.
The priority used between the various environment is the same one that was used in the code previously.
At read / init times, the highest priority environment is going to be detected, and we'll use the same one without lookup during writes. This should implement the same behaviour than we currently have.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/env.c | 76 +++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 25 deletions(-)
diff --git a/env/env.c b/env/env.c index 9dd4d4cb4d11..333fd71aad53 100644 --- a/env/env.c +++ b/env/env.c @@ -26,33 +26,59 @@ 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 ENVOP_GET_CHAR: + case ENVOP_INIT: + case ENVOP_LOAD: + if (prio >= ARRAY_SIZE(env_locations)) + return -ENODEV; + + env_load_location = env_locations[prio]; + return env_load_location; + + case ENVOP_SAVE: + return env_load_location; + } + + return ENVL_UNKNOWN; }
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 | 36 +++++++++++++++++++++++++------- include/asm-generic/global_data.h | 1 +- 2 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/env/env.c b/env/env.c index 333fd71aad53..75da2b921804 100644 --- a/env/env.c +++ b/env/env.c @@ -60,6 +60,23 @@ static enum env_location env_locations[] = { ENVL_UNKNOWN, };
+static bool env_has_inited(enum env_location location) +{ + return gd->env_has_init & BIT(location); +} + +static void env_set_inited(enum env_location location) +{ + /* + * We're using a 32-bits bitmask stored in gd (env_has_init) + * using the above enum value as the bit index. We need to + * make sure that we're not overflowing it. + */ + BUILD_BUG_ON(ARRAY_SIZE(env_locations) > BITS_PER_LONG); + + gd->env_has_init |= BIT(location); +} + static enum env_location env_load_location;
static enum env_location env_get_location(enum env_operation op, int prio) @@ -126,6 +143,9 @@ int env_get_char(int index) if (!drv->get_char) continue;
+ if (!env_has_inited(drv->location)) + continue; + ret = drv->get_char(index); if (!ret) return 0; @@ -148,6 +168,9 @@ int env_load(void) if (!drv->load) continue;
+ if (!env_has_inited(drv->location)) + continue; + printf("Loading Environment from %s... ", drv->name); ret = drv->load(); if (ret) @@ -173,6 +196,9 @@ int env_save(void) if (!drv->save) continue;
+ if (!env_has_inited(drv->location)) + continue; + printf("Saving Environment to %s... ", drv->name); ret = drv->save(); if (ret) @@ -194,14 +220,10 @@ int env_init(void) int prio;
for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { - if (!drv->init) - continue; - - ret = drv->init(); - if (!ret) - return 0; + if (!drv->init || !(ret = drv->init())) + env_set_inited(drv->location);
- debug("%s: Environment %s failed to init (err=%d)\n", __func__, + debug("%s: Environment %s init done (ret=%d)\n", __func__, drv->name, ret); }
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 73e036d6fd4a..fd8cd45b050e 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -50,6 +50,7 @@ typedef struct global_data { #endif unsigned long env_addr; /* Address of Environment struct */ unsigned long env_valid; /* Environment valid? enum env_valid */ + unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */
unsigned long ram_top; /* Top address of RAM used by U-Boot */ unsigned long relocaddr; /* Start address of U-Boot in RAM */
On 16 January 2018 at 01:16, 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 | 36 +++++++++++++++++++++++++------- include/asm-generic/global_data.h | 1 +- 2 files changed, 30 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
The raw MMC environment directly calls into the MMC framework. Make sure it's enabled before we can select it.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/env/Kconfig b/env/Kconfig index bef6e89bfc3c..ad5ccc253762 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -156,6 +156,7 @@ config ENV_IS_IN_FLASH config ENV_IS_IN_MMC bool "Environment in an MMC device" depends on !CHAIN_OF_TRUST + depends on MMC help Define this if you have an MMC device which you want to use for the environment.
On 16 January 2018 at 01:16, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The raw MMC environment directly calls into the MMC framework. Make sure it's enabled before we can select it.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/Kconfig | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
Now that we have everything in place in the code, let's allow to build multiple environments backend through Kconfig.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- configs/MPC8313ERDB_NAND_33_defconfig | 1 +- configs/MPC8313ERDB_NAND_66_defconfig | 1 +- configs/cl-som-imx7_defconfig | 1 +- configs/microblaze-generic_defconfig | 1 +- env/Kconfig | 65 +++++++++++++--------------- 5 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/configs/MPC8313ERDB_NAND_33_defconfig b/configs/MPC8313ERDB_NAND_33_defconfig index 823001583447..b761516d126a 100644 --- a/configs/MPC8313ERDB_NAND_33_defconfig +++ b/configs/MPC8313ERDB_NAND_33_defconfig @@ -22,6 +22,7 @@ CONFIG_CMD_DATE=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDIDS_DEFAULT="nand0=e2800000.flash" CONFIG_MTDPARTS_DEFAULT="mtdparts=e2800000.flash:512k(uboot),128k(env),6m@1m(kernel),-(fs)" +# CONFIG_ENV_IS_IN_FLASH is not set CONFIG_ENV_IS_IN_NAND=y # CONFIG_MMC is not set CONFIG_MTD_NOR_FLASH=y diff --git a/configs/MPC8313ERDB_NAND_66_defconfig b/configs/MPC8313ERDB_NAND_66_defconfig index 2639926ab814..0f2a675ae2cf 100644 --- a/configs/MPC8313ERDB_NAND_66_defconfig +++ b/configs/MPC8313ERDB_NAND_66_defconfig @@ -22,6 +22,7 @@ CONFIG_CMD_DATE=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDIDS_DEFAULT="nand0=e2800000.flash" CONFIG_MTDPARTS_DEFAULT="mtdparts=e2800000.flash:512k(uboot),128k(env),6m@1m(kernel),-(fs)" +# CONFIG_ENV_IS_IN_FLASH is not set CONFIG_ENV_IS_IN_NAND=y # CONFIG_MMC is not set CONFIG_MTD_NOR_FLASH=y diff --git a/configs/cl-som-imx7_defconfig b/configs/cl-som-imx7_defconfig index d37c82cafac1..0c93159032e5 100644 --- a/configs/cl-som-imx7_defconfig +++ b/configs/cl-som-imx7_defconfig @@ -41,6 +41,7 @@ CONFIG_CMD_EXT4=y CONFIG_CMD_EXT4_WRITE=y CONFIG_CMD_FAT=y CONFIG_CMD_FS_GENERIC=y +# CONFIG_ENV_IS_IN_MMC is not set CONFIG_ENV_IS_IN_SPI_FLASH=y CONFIG_SPI_FLASH=y CONFIG_SPI_FLASH_STMICRO=y diff --git a/configs/microblaze-generic_defconfig b/configs/microblaze-generic_defconfig index 5254c0da790a..cc80e8a027c8 100644 --- a/configs/microblaze-generic_defconfig +++ b/configs/microblaze-generic_defconfig @@ -40,7 +40,6 @@ CONFIG_CMD_UBI=y # CONFIG_CMD_UBIFS is not set CONFIG_SPL_OF_CONTROL=y CONFIG_OF_EMBED=y -CONFIG_ENV_IS_IN_FLASH=y CONFIG_NETCONSOLE=y CONFIG_SPL_DM=y CONFIG_MTD_NOR_FLASH=y diff --git a/env/Kconfig b/env/Kconfig index ad5ccc253762..6e2fbf416c12 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -1,38 +1,18 @@ menu "Environment"
-choice - prompt "Select the location of the environment" - default ENV_IS_IN_MMC if ARCH_SUNXI - default ENV_IS_IN_MMC if ARCH_EXYNOS4 - default ENV_IS_IN_MMC if MX6SX || MX7D - default ENV_IS_IN_MMC if TEGRA30 || TEGRA124 - default ENV_IS_IN_MMC if TEGRA_ARMV8_COMMON - default ENV_IS_IN_FLASH if ARCH_CINTEGRATOR - default ENV_IS_IN_FLASH if ARCH_INTEGRATOR_CP - default ENV_IS_IN_FLASH if M548x || M547x || M5282 || MCF547x_8x - default ENV_IS_IN_FLASH if MCF532x || MCF52x2 - default ENV_IS_IN_FLASH if MPC86xx || MPC83xx - default ENV_IS_IN_FLASH if ARCH_MPC8572 || ARCH_MPC8548 || ARCH_MPC8641 - default ENV_IS_IN_FLASH if SH && !CPU_SH4 - default ENV_IS_IN_SPI_FLASH if ARMADA_XP - default ENV_IS_IN_SPI_FLASH if INTEL_BAYTRAIL - default ENV_IS_IN_SPI_FLASH if INTEL_BRASWELL - default ENV_IS_IN_SPI_FLASH if INTEL_BROADWELL - default ENV_IS_IN_SPI_FLASH if NORTHBRIDGE_INTEL_IVYBRIDGE - default ENV_IS_IN_SPI_FLASH if INTEL_QUARK - default ENV_IS_IN_SPI_FLASH if INTEL_QUEENSBAY - default ENV_IS_IN_FAT if ARCH_BCM283X - default ENV_IS_IN_FAT if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS - default ENV_IS_NOWHERE - help - At present the environment can be stored in only one place. Use this - option to select the location. This is either a device (where the - environemnt information is simply written to a fixed location or - partition on the device) or a filesystem (where the environment - information is written to a file). - config ENV_IS_NOWHERE bool "Environment is not stored" + depends on !ENV_IS_IN_EEPROM + depends on !ENV_IS_IN_FAT + depends on !ENV_IS_IN_FLASH + depends on !ENV_IS_IN_MMC + depends on !ENV_IS_IN_NAND + depends on !ENV_IS_IN_NVRAM + depends on !ENV_IS_IN_ONENAND + depends on !ENV_IS_IN_REMOTE + depends on !ENV_IS_IN_SPI_FLASH + depends on !ENV_IS_IN_UBI + default y help Define this if you don't want to or can't have an environment stored on a storage medium. In this case the environemnt will still exist @@ -74,6 +54,8 @@ config ENV_IS_IN_EEPROM config ENV_IS_IN_FAT bool "Environment is in a FAT filesystem" depends on !CHAIN_OF_TRUST + default y if ARCH_BCM283X + default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS select FAT_WRITE help Define this if you want to use the FAT file system for the environment. @@ -84,6 +66,13 @@ config ENV_IS_IN_FAT config ENV_IS_IN_FLASH bool "Environment in flash memory" depends on !CHAIN_OF_TRUST + default y if ARCH_CINTEGRATOR + default y if ARCH_INTEGRATOR_CP + default y if M548x || M547x || M5282 || MCF547x_8x + default y if MCF532x || MCF52x2 + default y if MPC86xx || MPC83xx + default y if ARCH_MPC8572 || ARCH_MPC8548 || ARCH_MPC8641 + default y if SH && !CPU_SH4 help Define this if you have a flash device which you want to use for the environment. @@ -157,6 +146,11 @@ config ENV_IS_IN_MMC bool "Environment in an MMC device" depends on !CHAIN_OF_TRUST depends on MMC + default y if ARCH_SUNXI + default y if ARCH_EXYNOS4 + default y if MX6SX || MX7D + default y if TEGRA30 || TEGRA124 + default y if TEGRA_ARMV8_COMMON help Define this if you have an MMC device which you want to use for the environment. @@ -294,6 +288,13 @@ config ENV_IS_IN_REMOTE config ENV_IS_IN_SPI_FLASH bool "Environment is in SPI flash" depends on !CHAIN_OF_TRUST + default y if ARMADA_XP + default y if INTEL_BAYTRAIL + default y if INTEL_BRASWELL + default y if INTEL_BROADWELL + default y if NORTHBRIDGE_INTEL_IVYBRIDGE + default y if INTEL_QUARK + default y if INTEL_QUEENSBAY help Define this if you have a SPI Flash memory device which you want to use for the environment. @@ -359,8 +360,6 @@ config ENV_IS_IN_UBI You will probably want to define these to avoid a really noisy system when storing the env in UBI.
-endchoice - config ENV_FAT_INTERFACE string "Name of the block device for the environment" depends on ENV_IS_IN_FAT
Allow boards and architectures to override the default environment lookup code by overriding env_get_location.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/env/env.c b/env/env.c index 75da2b921804..6d0ab8ebe1a4 100644 --- a/env/env.c +++ b/env/env.c @@ -79,7 +79,25 @@ static void env_set_inited(enum env_location location)
static enum env_location env_load_location;
-static enum env_location env_get_location(enum env_operation op, int prio) +/** + * env_get_location() - Returns the best env location for a board + * @op: operations performed on the environment + * @prio: priority between the multiple environments, 0 being the + * highest priority + * + * This will return the preferred environment for the given + * priority. This is overridable by boards if they need to. + * + * All implementations are free to use the operation, the priority and + * any other data relevant to their choice, but must take into account + * the fact that the lowest prority (0) is the most important location + * in the system. The following locations should be returned by order + * of descending priorities, from the highest to the lowest priority. + * + * Returns: + * an enum env_location value on success, a negative error code otherwise + */ +__weak enum env_location env_get_location(enum env_operation op, int prio) { switch (op) { case ENVOP_GET_CHAR:
Hi Maxime,
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
diff --git a/env/env.c b/env/env.c index 75da2b921804..6d0ab8ebe1a4 100644 --- a/env/env.c +++ b/env/env.c @@ -79,7 +79,25 @@ static void env_set_inited(enum env_location location)
static enum env_location env_load_location;
-static enum env_location env_get_location(enum env_operation op, int prio) +/**
- env_get_location() - Returns the best env location for a board
- @op: operations performed on the environment
- @prio: priority between the multiple environments, 0 being the
highest priority
- This will return the preferred environment for the given
- priority. This is overridable by boards if they need to.
- All implementations are free to use the operation, the priority and
- any other data relevant to their choice, but must take into account
- the fact that the lowest prority (0) is the most important location
- in the system. The following locations should be returned by order
- of descending priorities, from the highest to the lowest priority.
- Returns:
- an enum env_location value on success, a negative error code otherwise
- */
+__weak enum env_location env_get_location(enum env_operation op, int prio) { switch (op) { case ENVOP_GET_CHAR: -- git-series 0.9.1
Regards, Simon
Hi Simon,
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
Maxime
Hi Maxime,
On 18 January 2018 at 10:21, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Hi Simon,
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Regards, Simon
Hi,
On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Aren't we kind of stuck here?
On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it.
I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this.
Maxime
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
Hi,
On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Aren't we kind of stuck here?
On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it.
I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this.
Yes, I think this is where we want things to be weak, with a reasonable default. If we start to see that "everyone" does the same thing by and large we can re-evaluate.
On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
Hi,
On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
env/env.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Aren't we kind of stuck here?
On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it.
I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this.
Yes, I think this is where we want things to be weak, with a reasonable default. If we start to see that "everyone" does the same thing by and large we can re-evaluate.
Ok.
I've fixed the bug I mentionned the other day on IRC, should I send a PR?
Thanks! Maxime
On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:
On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
Hi,
On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com > Reviewed-by: Lukasz Majewski lukma@denx.de > Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com > --- > env/env.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) >
I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Aren't we kind of stuck here?
On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it.
I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this.
Yes, I think this is where we want things to be weak, with a reasonable default. If we start to see that "everyone" does the same thing by and large we can re-evaluate.
Ok.
I've fixed the bug I mentionned the other day on IRC, should I send a PR?
Lets give Simon a chance to provide any other feedback here, or another argument to convince me that no, we don't want to have this abstracted by a weak function but instead ..., thanks!
Hi,
On 22 January 2018 at 09:36, Tom Rini trini@konsulko.com wrote:
On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:
On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
Hi,
On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com > > Reviewed-by: Lukasz Majewski lukma@denx.de > > Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com > > --- > > env/env.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > I still don't really understand why this needs to be a weak function. > If the board knows the priority order, can it not put it into > global_data? We could have a little u8 array of 4 items with a > terminator?
Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Aren't we kind of stuck here?
On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it.
I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this.
Yes, I think this is where we want things to be weak, with a reasonable default. If we start to see that "everyone" does the same thing by and large we can re-evaluate.
Ok.
I've fixed the bug I mentionned the other day on IRC, should I send a PR?
Lets give Simon a chance to provide any other feedback here, or another argument to convince me that no, we don't want to have this abstracted by a weak function but instead ..., thanks!
I suspect there is a reason why this is better than what I propose. Perhaps when I try it out it will become apparent.
So let's go ahead and revisit later if we have new information.
Reviewed-by: Simon Glass sjg@chromium.org
Regards, Simon
On Mon, Jan 22, 2018 at 09:48:26AM -0700, Simon Glass wrote:
Hi,
On 22 January 2018 at 09:36, Tom Rini trini@konsulko.com wrote:
On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:
On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
Hi,
On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
> On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: >> On 16 January 2018 at 01:16, 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: Andre Przywara andre.przywara@arm.com >> > Reviewed-by: Lukasz Majewski lukma@denx.de >> > Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com >> > --- >> > env/env.c | 20 +++++++++++++++++++- >> > 1 file changed, 19 insertions(+), 1 deletion(-) >> > >> >> I still don't really understand why this needs to be a weak function. >> If the board knows the priority order, can it not put it into >> global_data? We could have a little u8 array of 4 items with a >> terminator? > > Sure that would be simpler, but that would also prevent us from doing > "smart" things based on data other than just whether the previous > environment is usable. Things based for example on a GPIO state, or a > custom algorithm to transition (or duplicate) the environment.
In that case the board could read the GPIO state, or the algorithm, and then set up the value.
Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback.
Aren't we kind of stuck here?
On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it.
I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this.
Yes, I think this is where we want things to be weak, with a reasonable default. If we start to see that "everyone" does the same thing by and large we can re-evaluate.
Ok.
I've fixed the bug I mentionned the other day on IRC, should I send a PR?
Lets give Simon a chance to provide any other feedback here, or another argument to convince me that no, we don't want to have this abstracted by a weak function but instead ..., thanks!
I suspect there is a reason why this is better than what I propose. Perhaps when I try it out it will become apparent.
So let's go ahead and revisit later if we have new information.
Reviewed-by: Simon Glass sjg@chromium.org
Thanks! Maxime, please re-spin with the bugfix (or wait another day or two for other feedback), repost and I'll take it in Thurs/Fri or so.
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) {
Now that we have everything in place to implement the transition scheme, let's enable it by default.
Reviewed-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Lukasz Majewski lukma@denx.de Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- env/Kconfig | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/env/Kconfig b/env/Kconfig index 6e2fbf416c12..d1bf75ab41ca 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -55,6 +55,7 @@ config ENV_IS_IN_FAT bool "Environment is in a FAT filesystem" depends on !CHAIN_OF_TRUST default y if ARCH_BCM283X + default y if ARCH_SUNXI && MMC default y if MMC_OMAP_HS && TI_COMMON_CMD_OPTIONS select FAT_WRITE help @@ -363,6 +364,7 @@ config ENV_IS_IN_UBI config ENV_FAT_INTERFACE string "Name of the block device for the environment" depends on ENV_IS_IN_FAT + default "mmc" if ARCH_SUNXI default "mmc" if TI_COMMON_CMD_OPTIONS || ARCH_ZYNQMP || ARCH_AT91 help Define this to a string that is the name of the block device. @@ -372,6 +374,8 @@ config ENV_FAT_DEVICE_AND_PART depends on ENV_IS_IN_FAT default "0:1" if TI_COMMON_CMD_OPTIONS default "0:auto" if ARCH_ZYNQMP + default "0:auto" if ARCH_SUNXI && MMC_SUNXI_SLOT_EXTRA = -1 + default "1:auto" if ARCH_SUNXI && MMC_SUNXI_SLOT_EXTRA != -1 default "0" if ARCH_AT91 help Define this to a string to specify the partition of the device. It can
On Tue, Jan 16, 2018 at 10:16:36AM +0100, Maxime Ripard wrote:
Hi,
Here is a second attempt at transitioning away from the MMC raw environment to a FAT-based one for Allwinner SoCs. Since the RFC was quite well received, I actually tested it and fixed a few rough edges.
You'll find the first RFC here for reference: https://lists.denx.de/pipermail/u-boot/2017-October/310111.html
And the second that originated in this series: https://lists.denx.de/pipermail/u-boot/2017-November/311608.html
I gave it a travis run, and one test seems to fail in one test: https://travis-ci.org/mripard/u-boot/jobs/329229382
I really cannot really make any sense of why a change in the way the environment loads could affect the operation of a DHCP client. Is there a way to reproduce locally?
In this case, roughly, clone https://github.com/swarren/uboot-test-hooks and then: $ export PATH=${PATH}:/path/to/uboot-test-hooks/bin $ export PYTHONPATH=/path/to/uboot-test-hooks/py/travis-ci:${PYTHONPATH} $ export UBOOT_TRAVIS_BUILD_DIR=`cd .. && pwd`/.bm-work/$1 $ ./test/py/test.py --bd vexpress_ca15_tc2 --build-dir "$UBOOT_TRAVIS_BUILD_DIR" -k 'net'
Hi,
On Tue, Jan 16, 2018 at 04:56:49PM -0500, Tom Rini wrote:
On Tue, Jan 16, 2018 at 10:16:36AM +0100, Maxime Ripard wrote:
Hi,
Here is a second attempt at transitioning away from the MMC raw environment to a FAT-based one for Allwinner SoCs. Since the RFC was quite well received, I actually tested it and fixed a few rough edges.
You'll find the first RFC here for reference: https://lists.denx.de/pipermail/u-boot/2017-October/310111.html
And the second that originated in this series: https://lists.denx.de/pipermail/u-boot/2017-November/311608.html
I gave it a travis run, and one test seems to fail in one test: https://travis-ci.org/mripard/u-boot/jobs/329229382
I really cannot really make any sense of why a change in the way the environment loads could affect the operation of a DHCP client. Is there a way to reproduce locally?
In this case, roughly, clone https://github.com/swarren/uboot-test-hooks and then: $ export PATH=${PATH}:/path/to/uboot-test-hooks/bin $ export PYTHONPATH=/path/to/uboot-test-hooks/py/travis-ci:${PYTHONPATH} $ export UBOOT_TRAVIS_BUILD_DIR=`cd .. && pwd`/.bm-work/$1 $ ./test/py/test.py --bd vexpress_ca15_tc2 --build-dir "$UBOOT_TRAVIS_BUILD_DIR" -k 'net'
Thanks, I had to tweak it a bit to add --id qemu, and copy the conf.vexpress_ca15_tc2_qemu file in a directory with my hostname in the uboot-test-hooks repo.
And this is really getting weird, since the tests passes on my machine: http://code.bulix.org/rp6z29-258577
Maxime
On Wed, Jan 17, 2018 at 09:58:53AM +0100, Maxime Ripard wrote:
Hi,
On Tue, Jan 16, 2018 at 04:56:49PM -0500, Tom Rini wrote:
On Tue, Jan 16, 2018 at 10:16:36AM +0100, Maxime Ripard wrote:
Hi,
Here is a second attempt at transitioning away from the MMC raw environment to a FAT-based one for Allwinner SoCs. Since the RFC was quite well received, I actually tested it and fixed a few rough edges.
You'll find the first RFC here for reference: https://lists.denx.de/pipermail/u-boot/2017-October/310111.html
And the second that originated in this series: https://lists.denx.de/pipermail/u-boot/2017-November/311608.html
I gave it a travis run, and one test seems to fail in one test: https://travis-ci.org/mripard/u-boot/jobs/329229382
I really cannot really make any sense of why a change in the way the environment loads could affect the operation of a DHCP client. Is there a way to reproduce locally?
In this case, roughly, clone https://github.com/swarren/uboot-test-hooks and then: $ export PATH=${PATH}:/path/to/uboot-test-hooks/bin $ export PYTHONPATH=/path/to/uboot-test-hooks/py/travis-ci:${PYTHONPATH} $ export UBOOT_TRAVIS_BUILD_DIR=`cd .. && pwd`/.bm-work/$1 $ ./test/py/test.py --bd vexpress_ca15_tc2 --build-dir "$UBOOT_TRAVIS_BUILD_DIR" -k 'net'
Thanks, I had to tweak it a bit to add --id qemu, and copy the conf.vexpress_ca15_tc2_qemu file in a directory with my hostname in the uboot-test-hooks repo.
Ah right, yes, oops. I quickly grabbed bits out of my local wrapper script around it.
And this is really getting weird, since the tests passes on my machine: http://code.bulix.org/rp6z29-258577
Does it repeatedly fail in travis (which, uh, stopped working for me yesterday, I haven't seen if it's un-wedged today)? You can restart a specific sub-job rather than the whole.
On Wed, Jan 17, 2018 at 08:01:34AM -0500, Tom Rini wrote:
On Wed, Jan 17, 2018 at 09:58:53AM +0100, Maxime Ripard wrote:
Hi,
On Tue, Jan 16, 2018 at 04:56:49PM -0500, Tom Rini wrote:
On Tue, Jan 16, 2018 at 10:16:36AM +0100, Maxime Ripard wrote:
Hi,
Here is a second attempt at transitioning away from the MMC raw environment to a FAT-based one for Allwinner SoCs. Since the RFC was quite well received, I actually tested it and fixed a few rough edges.
You'll find the first RFC here for reference: https://lists.denx.de/pipermail/u-boot/2017-October/310111.html
And the second that originated in this series: https://lists.denx.de/pipermail/u-boot/2017-November/311608.html
I gave it a travis run, and one test seems to fail in one test: https://travis-ci.org/mripard/u-boot/jobs/329229382
I really cannot really make any sense of why a change in the way the environment loads could affect the operation of a DHCP client. Is there a way to reproduce locally?
In this case, roughly, clone https://github.com/swarren/uboot-test-hooks and then: $ export PATH=${PATH}:/path/to/uboot-test-hooks/bin $ export PYTHONPATH=/path/to/uboot-test-hooks/py/travis-ci:${PYTHONPATH} $ export UBOOT_TRAVIS_BUILD_DIR=`cd .. && pwd`/.bm-work/$1 $ ./test/py/test.py --bd vexpress_ca15_tc2 --build-dir "$UBOOT_TRAVIS_BUILD_DIR" -k 'net'
Thanks, I had to tweak it a bit to add --id qemu, and copy the conf.vexpress_ca15_tc2_qemu file in a directory with my hostname in the uboot-test-hooks repo.
Ah right, yes, oops. I quickly grabbed bits out of my local wrapper script around it.
Don't worry, I was on a good enough path :)
And this is really getting weird, since the tests passes on my machine: http://code.bulix.org/rp6z29-258577
Does it repeatedly fail in travis (which, uh, stopped working for me yesterday, I haven't seen if it's un-wedged today)? You can restart a specific sub-job rather than the whole.
It was repeatedly failing for that commit, but I reordered a few commits since (and I tested the new branch on my machine). I triggered a new build on travis that is currently running, but is taking a long time to complete. I'll let you know the results.
Thanks! Maxime
participants (3)
-
Maxime Ripard -
Simon Glass -
Tom Rini