env: Multiple env support and env transition for sunxi

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 <[email protected]>

---
 env/env.c             | 135 +++++++++++++++++++++++++++----------------
 include/environment.h |   8 +++-
 2 files changed, 94 insertions(+), 49 deletions(-)

env/env.c +0 -0

index 97ada5b5a6fd..4dc39b384c1e 100644

Hi Maxime,

Quoted 34 lines · Maxime Ripard

In preparation for the multiple environment support, let's introduce two new parameters to the environment driver lookup function: the priority and operation.

The operation parameter is meant to identify, obviously, the operation you might want to perform on the environment.

The priority is a number passed to identify the environment priority you want to retrieve. The lowest priority parameter (0) will be the primary source.

Combining the two parameters allow you to support multiple environments through different priorities, and to change those priorities between read and writes operations.

This is especially useful to implement migration mechanisms where you want to always use the same environment first, be it to read or write, while the common case is more likely to use the same environment it has read from to write it to.

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c             | 135 +++++++++++++++++++++++++++----------------
 include/environment.h |   8 +++-
 2 files changed, 94 insertions(+), 49 deletions(-)

env/env.c +0 -0

index 97ada5b5a6fd..4dc39b384c1e 100644
@@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
        return NULL;
 }
 

env/env.c static struct env_driver *_env_driver_lookup(enum env_location loc) · line 29

Quoted 2 lines

env/env.c +1 -1

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

env/env.c static struct env_driver *_env_driver_lookup(enum env_location loc) · line 32

Quoted 4 lines

env/env.c +2 -0

 {
+       if (prio >= 1)
+               return ENVL_UNKNOWN;

What is this for? Can you please add a comment?

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 <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/fat.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Hi Maxime,

Quoted 6 lines · Maxime Ripard

Since we have global messages to indicate what's going on, the custom messages in the environment drivers only make the output less readable.

Make FAT play a little nicer by removing all the extra \n and formatting that is redundant with the global output.

Quoted 5 lines

Reviewed-by Andre Przywara <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/fat.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Do you have a change log for this patch?

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 <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/mmc.c | 1 -
 1 file changed, 1 deletion(-)

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 <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 <[email protected]>

---
 env/env.c                         | 36 +++++++++++++++++++++++++-------
 include/asm-generic/global_data.h |  1 +-
 2 files changed, 30 insertions(+), 7 deletions(-)

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 <[email protected]>

---
 env/Kconfig | 1 +
 1 file changed, 1 insertion(+)

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

env/env.c +19 -2

index 75da2b921804..6d0ab8ebe1a4 100644
@@ -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

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

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

Hi Simon,

Quoted 13 lines · Simon Glass

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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.

Quoted 19 lines · Simon Glass

Quoted 13 lines

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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?

Quoted 4 lines

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.

Hi,

Quoted 24 lines · Simon Glass

Quoted 19 lines · Simon Glass

Quoted 13 lines

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 39 lines · Maxime Ripard

Hi,

Quoted 24 lines · Simon Glass

Quoted 19 lines · Simon Glass

Quoted 13 lines

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 44 lines · Tom Rini

Quoted 42 lines · Maxime Ripard

Hi,

Quoted 24 lines · Simon Glass

Quoted 19 lines · Simon Glass

Quoted 13 lines

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 49 lines · Maxime Ripard

Quoted 47 lines · Tom Rini

Quoted 35 lines · Maxime Ripard

Hi,

Quoted 24 lines · Simon Glass

Quoted 19 lines · Simon Glass

Quoted 13 lines

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 3 lines

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,

Quoted 50 lines · Tom Rini

Quoted 49 lines · Maxime Ripard

Quoted 47 lines · Tom Rini

Quoted 35 lines · Maxime Ripard

Hi,

Quoted 24 lines · Simon Glass

Quoted 19 lines · Simon Glass

Quoted 13 lines

Quoted 10 lines · Maxime Ripard

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

Reviewed-by Andre Przywara <[email protected]>

Reviewed-by Lukasz Majewski <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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 <[email protected]>

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?

Quoted 12 lines · Maxime Ripard

Hi,

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

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

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

Quoted 6 lines

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,

Quoted 20 lines · Tom Rini

Quoted 12 lines · Maxime Ripard

Hi,

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

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

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

Quoted 6 lines

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?

Quoted 6 lines

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

Quoted 30 lines · Maxime Ripard

Hi,

Quoted 26 lines · Tom Rini

Quoted 12 lines · Maxime Ripard

Hi,

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

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

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

Quoted 6 lines

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'

Quoted 3 lines

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.

Quoted 3 lines

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.