[PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
This change allows board code to modify default_environment[] array when compile option DEFAULT_ENV_IS_RW is specified in board config file.
Some board default variables depend on runtime configuration which is not known at compile time. Therefore allow to set default_environment[] array as non-const and allow board code to modify it when it is needed.
Signed-off-by: Pali Rohár pali@kernel.org --- include/env_default.h | 2 ++ include/env_internal.h | 4 ++++ 2 files changed, 6 insertions(+)
diff --git a/include/env_default.h b/include/env_default.h index 8a0c3057f0..ea31a8eddf 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -19,6 +19,8 @@ env_t embedded_environment __UBOOT_ENV_SECTION__(environment) = { { #elif defined(DEFAULT_ENV_INSTANCE_STATIC) static char default_environment[] = { +#elif defined(DEFAULT_ENV_IS_RW) +uchar default_environment[] = { #else const uchar default_environment[] = { #endif diff --git a/include/env_internal.h b/include/env_internal.h index b26dc6239c..708c833a55 100644 --- a/include/env_internal.h +++ b/include/env_internal.h @@ -111,7 +111,11 @@ typedef struct environment_s { extern env_t embedded_environment; #endif /* ENV_IS_EMBEDDED */
+#ifdef DEFAULT_ENV_IS_RW +extern unsigned char default_environment[]; +#else extern const unsigned char default_environment[]; +#endif
#ifndef DO_DEPS_ONLY
On Espressobin board value for $fdtfile cannot be determined at compile time and is calculated at board runtime code. This change uses a new option DEFAULT_ENV_IS_RW to allow modifying default_environment[] array at runtime and set into it correct value.
This change also ensure that 'env default -a' set correct value to $fdtfile.
Signed-off-by: Pali Rohár pali@kernel.org --- board/Marvell/mvebu_armada-37xx/board.c | 22 +++++++++++++++------- include/configs/mvebu_armada-37xx.h | 13 ++++++++++++- 2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c index f67b04b78c..9297ea0f90 100644 --- a/board/Marvell/mvebu_armada-37xx/board.c +++ b/board/Marvell/mvebu_armada-37xx/board.c @@ -6,6 +6,7 @@ #include <common.h> #include <dm.h> #include <env.h> +#include <env_internal.h> #include <i2c.h> #include <init.h> #include <mmc.h> @@ -84,15 +85,17 @@ int board_init(void) #ifdef CONFIG_BOARD_LATE_INIT int board_late_init(void) { + char *ptr = (char *)&default_environment[0]; struct mmc *mmc_dev; bool ddr4, emmc;
- if (env_get("fdtfile")) - return 0; - if (!of_machine_is_compatible("globalscale,espressobin")) return 0;
+ /* Find free buffer in default_environment[] for new variables */ + while (*ptr != '\0' && *(ptr+1) != '\0') ptr++; + ptr += 2; + /* If the memory controller has been configured for DDR4, we're running on v7 */ ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS) & A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4; @@ -101,14 +104,19 @@ int board_late_init(void) mmc_dev = find_mmc_device(1); emmc = (mmc_dev && mmc_init(mmc_dev) == 0);
+ /* Ensure that 'env default -a' set correct value to $fdtfile */ if (ddr4 && emmc) - env_set("fdtfile", "marvell/armada-3720-espressobin-v7-emmc.dtb"); + strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7-emmc.dtb"); else if (ddr4) - env_set("fdtfile", "marvell/armada-3720-espressobin-v7.dtb"); + strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7.dtb"); else if (emmc) - env_set("fdtfile", "marvell/armada-3720-espressobin-emmc.dtb"); + strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-emmc.dtb"); else - env_set("fdtfile", "marvell/armada-3720-espressobin.dtb"); + strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin.dtb"); + + /* If $fdtfile was not set explicitly by user then set default value */ + if (!env_get("fdtfile")) + env_set("fdtfile", ptr + sizeof("fdtfile="));
return 0; } diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h index 0d585606a7..6df702367c 100644 --- a/include/configs/mvebu_armada-37xx.h +++ b/include/configs/mvebu_armada-37xx.h @@ -57,6 +57,11 @@ */ #define CONFIG_MTD_PARTITIONS /* required for UBI partition support */
+/* + * Environment + */ +#define DEFAULT_ENV_IS_RW /* required for configuring default fdtfile= */ + /* * Ethernet Driver configuration */ @@ -87,6 +92,11 @@
#include <config_distro_bootcmd.h>
+/* filler for default values filled by board_early_init_f() */ +#define ENV_RW_FILLER \ + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for fdtfile= */ \ + "" + /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ #define CONFIG_EXTRA_ENV_SETTINGS \ "scriptaddr=0x6d00000\0" \ @@ -96,6 +106,7 @@ "kernel_addr=0x7000000\0" \ "kernel_addr_r=0x7000000\0" \ "ramdisk_addr_r=0xa000000\0" \ - BOOTENV + BOOTENV \ + ENV_RW_FILLER
#endif /* _CONFIG_MVEBU_ARMADA_37XX_H */
On Espressobin board are MAC addresses stored in U-Boot env area. Therefore they are not present in default_environment[] array constructed at compile time.
This change puts permanent MAC addresses into default_environment[] array at board runtime. Espressobin board has enabled DEFAULT_ENV_IS_RW option and therefore can modify this array.
This change ensure that 'env default -a' does not delete permanent MAC addresses from Espressobin env storage area.
Signed-off-by: Pali Rohár pali@kernel.org --- board/Marvell/mvebu_armada-37xx/board.c | 19 +++++++++++++++++++ include/configs/mvebu_armada-37xx.h | 4 ++++ 2 files changed, 23 insertions(+)
diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c index 9297ea0f90..dabd4eefd6 100644 --- a/board/Marvell/mvebu_armada-37xx/board.c +++ b/board/Marvell/mvebu_armada-37xx/board.c @@ -88,6 +88,9 @@ int board_late_init(void) char *ptr = (char *)&default_environment[0]; struct mmc *mmc_dev; bool ddr4, emmc; + const char *mac; + char eth[10]; + int i;
if (!of_machine_is_compatible("globalscale,espressobin")) return 0; @@ -96,6 +99,22 @@ int board_late_init(void) while (*ptr != '\0' && *(ptr+1) != '\0') ptr++; ptr += 2;
+ /* + * Ensure that 'env default -a' does not erase permanent MAC addresses + * stored in env variables: $ethaddr, $eth1addr, $eth2addr and $eth3addr + */ + + mac = env_get("ethaddr"); + if (mac && strlen(mac) <= 17) + ptr += sprintf(ptr, "ethaddr=%s", mac) + 1; + + for (i = 1; i <= 3; i++) { + sprintf(eth, "eth%daddr", i); + mac = env_get(eth); + if (mac && strlen(mac) <= 17) + ptr += sprintf(ptr, "%s=%s", eth, mac) + 1; + } + /* If the memory controller has been configured for DDR4, we're running on v7 */ ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS) & A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4; diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h index 6df702367c..2ad4325eaf 100644 --- a/include/configs/mvebu_armada-37xx.h +++ b/include/configs/mvebu_armada-37xx.h @@ -94,6 +94,10 @@
/* filler for default values filled by board_early_init_f() */ #define ENV_RW_FILLER \ + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for ethaddr= */ \ + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth1addr= */ \ + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth2addr= */ \ + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth3addr= */ \ "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for fdtfile= */ \ ""
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
On Wednesday 23 December 2020 12:21:27 Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
-- 2.20.1
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind: - board provides list of immutable vars - env_set_default() backs up these vars - env_set_default() imports default_environment - env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
Maybe the first step can be solved with ENV_FLAGS_VAR, a new immutable flag, and boards just making use of CONFIG_ENV_FLAGS_LIST_DEFAULT to declare those. But I fail to find an example in-tree.
Thanks, Andre
On Wednesday 23 December 2020 12:21:27 Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
-- 2.20.1
On 12/01/2021 09:18, Andre Heider wrote:
...
Maybe the first step can be solved with ENV_FLAGS_VAR, a new immutable flag, and boards just making use of CONFIG_ENV_FLAGS_LIST_DEFAULT to declare those. But I fail to find an example in-tree.
Found one, see ENV_FLAGS_LIST_STATIC. That even has ETHADDR_WILDCARD, so when enabling CONFIG_REGEX it could look like this for espressobin:
CONFIG_ENV_FLAGS_LIST_DEFAULT="eth\d*addr:mX,fdtfile:X"
where "X" is the immutable flag.
Hello!
On Tuesday 12 January 2021 09:18:44 Andre Heider wrote:
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind:
- board provides list of immutable vars
- env_set_default() backs up these vars
- env_set_default() imports default_environment
- env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
This could probably work for $ethNaddr variables.
But there is still an issue how to handle $fdtfile. There is basically default value for this variable, but value itself cannot be determined at compile time, only at runtime. And for it variable flags do not help, we just need an mechanism how to set default variable values not only at compile time but also runtime.
That is why I chosen for now solution with modifying default_environment[] array as it solve issue for both $fdtfile and $ethNaddr variables.
Maybe the first step can be solved with ENV_FLAGS_VAR, a new immutable flag, and boards just making use of CONFIG_ENV_FLAGS_LIST_DEFAULT to declare those. But I fail to find an example in-tree.
Thanks, Andre
On Wednesday 23 December 2020 12:21:27 Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
-- 2.20.1
Hi Pali, Hi Andre,
On 12.01.21 10:24, Pali Rohár wrote:
Hello!
On Tuesday 12 January 2021 09:18:44 Andre Heider wrote:
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind:
- board provides list of immutable vars
- env_set_default() backs up these vars
- env_set_default() imports default_environment
- env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
This could probably work for $ethNaddr variables.
But there is still an issue how to handle $fdtfile. There is basically default value for this variable, but value itself cannot be determined at compile time, only at runtime. And for it variable flags do not help, we just need an mechanism how to set default variable values not only at compile time but also runtime.
That is why I chosen for now solution with modifying default_environment[] array as it solve issue for both $fdtfile and $ethNaddr variables.
So what is the outcome of this discussion? Andre, do you see any hindering points in this patch series, apart from it not winning a "beauty contest"? ;)
I tend to pull it in shortly, if nobody objects.
Thanks, Stefan
Maybe the first step can be solved with ENV_FLAGS_VAR, a new immutable flag, and boards just making use of CONFIG_ENV_FLAGS_LIST_DEFAULT to declare those. But I fail to find an example in-tree.
Thanks, Andre
On Wednesday 23 December 2020 12:21:27 Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
-- 2.20.1
Viele Grüße, Stefan
On Tuesday 02 February 2021 16:09:03 Stefan Roese wrote:
Hi Pali, Hi Andre,
On 12.01.21 10:24, Pali Rohár wrote:
Hello!
On Tuesday 12 January 2021 09:18:44 Andre Heider wrote:
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind:
- board provides list of immutable vars
- env_set_default() backs up these vars
- env_set_default() imports default_environment
- env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
This could probably work for $ethNaddr variables.
But there is still an issue how to handle $fdtfile. There is basically default value for this variable, but value itself cannot be determined at compile time, only at runtime. And for it variable flags do not help, we just need an mechanism how to set default variable values not only at compile time but also runtime.
That is why I chosen for now solution with modifying default_environment[] array as it solve issue for both $fdtfile and $ethNaddr variables.
So what is the outcome of this discussion? Andre, do you see any hindering points in this patch series, apart from it not winning a "beauty contest"? ;)
Hello! I have looked at it again and I can say that implementing a new "immutable" bit for ethNaddr env variables would be better / cleaner solution. But as I wrote for fdtfile env variable that immutable bit does not help and we need some option how to set default value of this variable at the runtime.
I tend to pull it in shortly, if nobody objects.
Andre thought that other u-boot developers would not like this approach. But I have not received any response, so I do not know if just nobody looked at this patch or more people looked at it and did not have objections.
Anyway, Andre are you going to look & implement for ethNaddr env variables that approach via immutable bits?
I think current implementation can be changed at anytime in future.
Thanks, Stefan
Maybe the first step can be solved with ENV_FLAGS_VAR, a new immutable flag, and boards just making use of CONFIG_ENV_FLAGS_LIST_DEFAULT to declare those. But I fail to find an example in-tree.
Thanks, Andre
On Wednesday 23 December 2020 12:21:27 Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
-- 2.20.1
Viele Grüße, Stefan
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
On 02/02/2021 16:09, Stefan Roese wrote:
Hi Pali, Hi Andre,
On 12.01.21 10:24, Pali Rohár wrote:
Hello!
On Tuesday 12 January 2021 09:18:44 Andre Heider wrote:
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind:
- board provides list of immutable vars
- env_set_default() backs up these vars
- env_set_default() imports default_environment
- env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
This could probably work for $ethNaddr variables.
But there is still an issue how to handle $fdtfile. There is basically default value for this variable, but value itself cannot be determined at compile time, only at runtime. And for it variable flags do not help, we just need an mechanism how to set default variable values not only at compile time but also runtime.
That is why I chosen for now solution with modifying default_environment[] array as it solve issue for both $fdtfile and $ethNaddr variables.
So what is the outcome of this discussion? Andre, do you see any hindering points in this patch series, apart from it not winning a "beauty contest"? ;)
Hehe, nope, only aesthetic concerns, no hinderung points to block this going in.
Thanks, Andre
On 02.02.21 17:13, Andre Heider wrote:
On 02/02/2021 16:09, Stefan Roese wrote:
Hi Pali, Hi Andre,
On 12.01.21 10:24, Pali Rohár wrote:
Hello!
On Tuesday 12 January 2021 09:18:44 Andre Heider wrote:
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind:
- board provides list of immutable vars
- env_set_default() backs up these vars
- env_set_default() imports default_environment
- env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
This could probably work for $ethNaddr variables.
But there is still an issue how to handle $fdtfile. There is basically default value for this variable, but value itself cannot be determined at compile time, only at runtime. And for it variable flags do not help, we just need an mechanism how to set default variable values not only at compile time but also runtime.
That is why I chosen for now solution with modifying default_environment[] array as it solve issue for both $fdtfile and $ethNaddr variables.
So what is the outcome of this discussion? Andre, do you see any hindering points in this patch series, apart from it not winning a "beauty contest"? ;)
Hehe, nope, only aesthetic concerns, no hinderung points to block this going in.
I see. Then if appropriate, please send any matching tag(s) to these patches.
Thanks, Stefan
On 02/02/2021 17:32, Stefan Roese wrote:
On 02.02.21 17:13, Andre Heider wrote:
On 02/02/2021 16:09, Stefan Roese wrote:
Hi Pali, Hi Andre,
On 12.01.21 10:24, Pali Rohár wrote:
Hello!
On Tuesday 12 January 2021 09:18:44 Andre Heider wrote:
Hi Pali,
On 11/01/2021 11:51, Pali Rohár wrote:
Hello Stefan and Andre!
Could you please look at this patch series and tell me what do you think about it? If it is fine or needs to take different approach?
I like the idea very much, and I bet there're quite some boards which could make good use of "immutable envvars".
The obvious review point is the filler thing and its dependency on DEFAULT_ENV_IS_RW, which probably won't win a beauty contest :) Maybe a nicer integration would help in getting it merged?
I don't think it would take too much effort, first thing that comes to mind:
- board provides list of immutable vars
- env_set_default() backs up these vars
- env_set_default() imports default_environment
- env_set_default() imports backup on top
The last step should be easy, see env_set_default_vars().
This could probably work for $ethNaddr variables.
But there is still an issue how to handle $fdtfile. There is basically default value for this variable, but value itself cannot be determined at compile time, only at runtime. And for it variable flags do not help, we just need an mechanism how to set default variable values not only at compile time but also runtime.
That is why I chosen for now solution with modifying default_environment[] array as it solve issue for both $fdtfile and $ethNaddr variables.
So what is the outcome of this discussion? Andre, do you see any hindering points in this patch series, apart from it not winning a "beauty contest"? ;)
Hehe, nope, only aesthetic concerns, no hinderung points to block this going in.
I see. Then if appropriate, please send any matching tag(s) to these patches.
I didn't test it, so this set is at least: Acked-by: Andre Heider a.heider@gmail.com
Thanks, Andre
Stefan, could you please look at this patch series, what do you think about it?
On Wednesday 23 December 2020 12:21:27 Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
-- 2.20.1
On 23.12.20 12:21, Pali Rohár wrote:
This patch series set default env values of $fdtfile and $ethNaddr for Espressobin board at runtime.
It fixes two main issues on Espressobin board that 'env default -a' completely erases permanent board MAC addresses and also erase $fdtfile variable which is needed for booting Linux kernel via distro boot.
Lot of people were complaining about erasing permanent MAC addresses by U-boot on this board and due to this issue some linux distributions started using static hardcoded MAC addresses for all Espressobin boards to workaround this issue. Apparently erase of MAC addresses or usage of static hardcoded value caused more issues on network (e.g. inability to connect two of these boards to the same network).
Pali Rohár (3): env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW arm: mvebu: Espressobin: Set default value for $fdtfile env variable arm: mvebu: Espressobin: Set default value for $ethNaddr env variable
board/Marvell/mvebu_armada-37xx/board.c | 41 ++++++++++++++++++++----- include/configs/mvebu_armada-37xx.h | 17 +++++++++- include/env_default.h | 2 ++ include/env_internal.h | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-)
Applied to u-boot-marvell/master
Thanks, Stefan
participants (3)
-
Andre Heider -
Pali Rohár -
Stefan Roese