[PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime

Pali Rohár-2
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

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] env: Allow to set default_environment[] from board code via compile option DEFAULT_ENV_IS_RW

Pali Rohár-2
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 <[hidden email]>
---
 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
 
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] arm: mvebu: Espressobin: Set default value for $fdtfile env variable

Pali Rohár-2
In reply to this post by Pali Rohár-2
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 <[hidden email]>
---
 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 */
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] arm: mvebu: Espressobin: Set default value for $ethNaddr env variable

Pali Rohár-2
In reply to this post by Pali Rohár-2
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 <[hidden email]>
---
 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= */ \
  ""
 
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime

Pali Rohár-2
In reply to this post by Pali Rohár-2
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
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime

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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime

Andre Heider
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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] arm: mvebu: Espressobin: Set default env values at runtime

Pali Rohár-2
In reply to this post by Andre Heider
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
> > >
>