[PATCH 0/8] env: mmc: improvements and corrections

[PATCH 0/8] env: mmc: improvements and corrections

[Patrick Delaunay]

Update in U-Boot env mmc backend with several cosmetic changes or
corrections and 2 new features:

1/ CONFIG_ENV_MMC_USE_DT = no more use CONFIG_ENV_OFFSET
   in the mmc ENV backend when this config is activated.

   Requested by the STM32MP STMicroelectronics boards which activate
   several ENV_IS_IN_XXX; the value of CONFIG_ENV_OFFSET is invalid for
   SD-Card / eMMC boot; this offset should only used in SPIFlash backend
   (sf.c) for SPI-NOR boot.

   If this offset is used on mmc backend, when partition name in GPT is
   not aligned with  U-Boot DT: "u-boot,mmc-env-partition", the behavior
   is difficult to debug: a partition is corrupted on 'env save' command.

2/ selects the GPT env partition by the "u-boot-env" type GUID introduced
   by the commit c0364ce1c695 ("doc/README.gpt: define partition
   type GUID for U-Boot environment")

   This feature can also avoid issue when 'u-boot-env' partition name
   change in GPT partitioning but not in the U-Boot DT with
   "u-boot,mmc-env-partition"

Few check patch warnings remained in the series,
but after check I can't remove them :

- IS_ENABLED(ENV_MMC_HWPART_REDUND) is normally used as
  IS_ENABLED(CONFIG_ENV_MMC_HWPART_REDUND)
  => ENV_MMC_HWPART_REDUND is locally defined in this file it is not
     a real CONFIG but I can use the IS_ENABLED() macro as it is defined
     to 1

- Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where
  possible
  + CONFIG_PARTITION_TYPE_GUID => info.type_guid existence
  + CONFIG_ENV_OFFSET_REDUND and CONFIG_ENV_MMC_USE_DT => only for define

As I miss the merge window, not targeted for v2023.01 but for next
v2023.04.

Regards

Patrick



Patrick Delaunay (8):
  env: mmc: introduced ENV_MMC_OFFSET
  env: mcc: Drop unnecessary #ifdefs
  env: mcc: fix compilation error with ENV_IS_EMBEDDED
  env: mmc: add CONFIG_ENV_MMC_USE_DT
  configs: stm32mp: activate CONFIG_ENV_MMC_USE_DT
  env: mmc: select GPT env partition by type guid
  env: mmc: add debug message when mmc-env-partition is not found
  env: mmc: cosmetic: remove unused macro STR(X)

 configs/stm32mp13_defconfig         |   1 +
 configs/stm32mp15_basic_defconfig   |   1 +
 configs/stm32mp15_defconfig         |   1 +
 configs/stm32mp15_trusted_defconfig |   1 +
 env/Kconfig                         |  16 +++
 env/mmc.c                           | 182 ++++++++++++++++++----------
 6 files changed, 135 insertions(+), 67 deletions(-)

-- 
2.25.1

[PATCH 1/8] env: mmc: introduced ENV_MMC_OFFSET

[Patrick Delaunay]

Introduce ENV_MMC_OFFSET defines.
It is a preliminary step to the next patches to simplify the code.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 env/mmc.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/env/mmc.c b/env/mmc.c
index c28f4c6c6dc0..42bcf7e775cc 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -24,6 +24,17 @@
 #define __STR(X) #X
 #define STR(X) __STR(X)
 
+#define ENV_MMC_INVALID_OFFSET ((s64)-1)
+
+/* Default ENV offset when not defined in Device Tree */
+#define ENV_MMC_OFFSET		CONFIG_ENV_OFFSET
+
+#if defined(CONFIG_ENV_OFFSET_REDUND)
+#define ENV_MMC_OFFSET_REDUND	CONFIG_ENV_OFFSET_REDUND
+#else
+#define ENV_MMC_OFFSET_REDUND	ENV_MMC_INVALID_OFFSET
+#endif
+
 DECLARE_GLOBAL_DATA_PTR;
 
 /*
@@ -94,12 +105,12 @@ static inline s64 mmc_offset(int copy)
 			return val;
 	}
 
-	defvalue = CONFIG_ENV_OFFSET;
+	defvalue = ENV_MMC_OFFSET;
 	propname = dt_prop.offset;
 
 #if defined(CONFIG_ENV_OFFSET_REDUND)
 	if (copy) {
-		defvalue = CONFIG_ENV_OFFSET_REDUND;
+		defvalue = ENV_MMC_OFFSET_REDUND;
 		propname = dt_prop.offset_redund;
 	}
 #endif
@@ -108,11 +119,11 @@ static inline s64 mmc_offset(int copy)
 #else
 static inline s64 mmc_offset(int copy)
 {
-	s64 offset = CONFIG_ENV_OFFSET;
+	s64 offset = ENV_MMC_OFFSET;
 
 #if defined(CONFIG_ENV_OFFSET_REDUND)
 	if (copy)
-		offset = CONFIG_ENV_OFFSET_REDUND;
+		offset = ENV_MMC_OFFSET_REDUND;
 #endif
 	return offset;
 }
@@ -122,6 +133,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
 {
 	s64 offset = mmc_offset(copy);
 
+	if (offset == ENV_MMC_INVALID_OFFSET) {
+		printf("Invalid ENV offset in MMC, copy=%d\n", copy);
+		return -ENOENT;
+	}
+
 	if (offset < 0)
 		offset += mmc->capacity;
 
-- 
2.25.1

[PATCH 2/8] env: mcc: Drop unnecessary #ifdefs

[Patrick Delaunay]

This file has a lot of conditional code and much of it is unnecessary.
Clean this up to reduce the number of build combinations.

This patch replaces the test on CONFIG_ENV_OFFSET_REDUND for the
more coherent CONFIG_SYS_REDUNDAND_ENVIRONMENT.

This patch also corrects a compilation issue in init_mmc_for_env()
when CONFIG_SYS_MMC_ENV_PART is not activated, env_mmc_orig_hwpart is
not defined.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 env/mmc.c | 120 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 64 insertions(+), 56 deletions(-)

diff --git a/env/mmc.c b/env/mmc.c
index 42bcf7e775cc..b36bd9ad77ee 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -46,7 +46,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
      (CONFIG_SYS_MMC_ENV_PART == 1) && \
      (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
-#define ENV_MMC_HWPART_REDUND
+#define ENV_MMC_HWPART_REDUND	1
 #endif
 
 #if CONFIG_IS_ENABLED(OF_CONTROL)
@@ -108,12 +108,11 @@ static inline s64 mmc_offset(int copy)
 	defvalue = ENV_MMC_OFFSET;
 	propname = dt_prop.offset;
 
-#if defined(CONFIG_ENV_OFFSET_REDUND)
-	if (copy) {
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && copy) {
 		defvalue = ENV_MMC_OFFSET_REDUND;
 		propname = dt_prop.offset_redund;
 	}
-#endif
+
 	return ofnode_conf_read_int(propname, defvalue);
 }
 #else
@@ -121,10 +120,9 @@ static inline s64 mmc_offset(int copy)
 {
 	s64 offset = ENV_MMC_OFFSET;
 
-#if defined(CONFIG_ENV_OFFSET_REDUND)
-	if (copy)
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && copy)
 		offset = ENV_MMC_OFFSET_REDUND;
-#endif
+
 	return offset;
 }
 #endif
@@ -165,8 +163,24 @@ static int mmc_set_env_part(struct mmc *mmc, uint part)
 
 	return ret;
 }
+
+static bool mmc_set_env_part_init(struct mmc *mmc)
+{
+	env_mmc_orig_hwpart = mmc_get_blk_desc(mmc)->hwpart;
+	if (mmc_set_env_part(mmc, mmc_get_env_part(mmc)))
+		return false;
+
+	return true;
+}
+
+static int mmc_set_env_part_restore(struct mmc *mmc)
+{
+	return mmc_set_env_part(mmc, env_mmc_orig_hwpart);
+}
 #else
 static inline int mmc_set_env_part(struct mmc *mmc, uint part) {return 0; };
+static bool mmc_set_env_part_init(struct mmc *mmc) {return true; }
+static inline int mmc_set_env_part_restore(struct mmc *mmc) {return 0; };
 #endif
 
 static const char *init_mmc_for_env(struct mmc *mmc)
@@ -183,8 +197,7 @@ static const char *init_mmc_for_env(struct mmc *mmc)
 	if (mmc_init(mmc))
 		return "MMC init failed";
 #endif
-	env_mmc_orig_hwpart = mmc_get_blk_desc(mmc)->hwpart;
-	if (mmc_set_env_part(mmc, mmc_get_env_part(mmc)))
+	if (!mmc_set_env_part_init(mmc))
 		return "MMC partition switch failed";
 
 	return NULL;
@@ -192,11 +205,7 @@ static const char *init_mmc_for_env(struct mmc *mmc)
 
 static void fini_mmc_for_env(struct mmc *mmc)
 {
-#ifdef CONFIG_SYS_MMC_ENV_PART
-	int dev = mmc_get_env_dev();
-
-	blk_select_hwpart_devnum(UCLASS_MMC, dev, env_mmc_orig_hwpart);
-#endif
+	mmc_set_env_part_restore(mmc);
 }
 
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
@@ -233,21 +242,20 @@ static int env_mmc_save(void)
 	if (ret)
 		goto fini;
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-	if (gd->env_valid == ENV_VALID)
-		copy = 1;
-
-#ifdef ENV_MMC_HWPART_REDUND
-	ret = mmc_set_env_part(mmc, copy + 1);
-	if (ret)
-		goto fini;
-#endif
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
+		if (gd->env_valid == ENV_VALID)
+			copy = 1;
 
-#endif
+		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+			ret = mmc_set_env_part(mmc, copy + 1);
+			if (ret)
+				goto fini;
+		}
 
-	if (mmc_get_env_addr(mmc, copy, &offset)) {
-		ret = 1;
-		goto fini;
+		if (mmc_get_env_addr(mmc, copy, &offset)) {
+			ret = 1;
+			goto fini;
+		}
 	}
 
 	printf("Writing to %sMMC(%d)... ", copy ? "redundant " : "", dev);
@@ -259,12 +267,12 @@ static int env_mmc_save(void)
 
 	ret = 0;
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
-#endif
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
 
 fini:
 	fini_mmc_for_env(mmc);
+
 	return ret;
 }
 
@@ -308,22 +316,22 @@ static int env_mmc_erase(void)
 	printf("\n");
 	ret = erase_env(mmc, CONFIG_ENV_SIZE, offset);
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-	copy = 1;
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
+		copy = 1;
 
-#ifdef ENV_MMC_HWPART_REDUND
-	ret = mmc_set_env_part(mmc, copy + 1);
-	if (ret)
-		goto fini;
-#endif
+		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+			ret = mmc_set_env_part(mmc, copy + 1);
+			if (ret)
+				goto fini;
+		}
 
-	if (mmc_get_env_addr(mmc, copy, &offset)) {
-		ret = CMD_RET_FAILURE;
-		goto fini;
-	}
+		if (mmc_get_env_addr(mmc, copy, &offset)) {
+			ret = CMD_RET_FAILURE;
+			goto fini;
+		}
 
-	ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
-#endif
+		ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
+	}
 
 fini:
 	fini_mmc_for_env(mmc);
@@ -345,7 +353,7 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
+#if defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
 static int env_mmc_load(void)
 {
 #if !defined(ENV_IS_EMBEDDED)
@@ -375,19 +383,19 @@ static int env_mmc_load(void)
 		goto fini;
 	}
 
-#ifdef ENV_MMC_HWPART_REDUND
-	ret = mmc_set_env_part(mmc, 1);
-	if (ret)
-		goto fini;
-#endif
+	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+		ret = mmc_set_env_part(mmc, 1);
+		if (ret)
+			goto fini;
+	}
 
 	read1_fail = read_env(mmc, CONFIG_ENV_SIZE, offset1, tmp_env1);
 
-#ifdef ENV_MMC_HWPART_REDUND
-	ret = mmc_set_env_part(mmc, 2);
-	if (ret)
-		goto fini;
-#endif
+	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
+		ret = mmc_set_env_part(mmc, 2);
+		if (ret)
+			goto fini;
+	}
 
 	read2_fail = read_env(mmc, CONFIG_ENV_SIZE, offset2, tmp_env2);
 
@@ -403,7 +411,7 @@ err:
 #endif
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 static int env_mmc_load(void)
 {
 #if !defined(ENV_IS_EMBEDDED)
@@ -448,7 +456,7 @@ err:
 #endif
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
+#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 
 U_BOOT_ENV_LOCATION(mmc) = {
 	.location	= ENVL_MMC,
-- 
2.25.1

[PATCH 3/8] env: mcc: fix compilation error with ENV_IS_EMBEDDED

[Patrick Delaunay]

When ENV_IS_EMBEDDED is enabled, ret is not defined but is used as a
return value in env_mmc_load().
This patch correct this issue and simplify the existing code, test only
one time #if defined(ENV_IS_EMBEDDED) and not in the function.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 env/mmc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/env/mmc.c b/env/mmc.c
index b36bd9ad77ee..661a268ea07d 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -353,10 +353,14 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-#if defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
+#if defined(ENV_IS_EMBEDDED)
+static int env_mmc_load(void)
+{
+	return 0;
+}
+#elif defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
 static int env_mmc_load(void)
 {
-#if !defined(ENV_IS_EMBEDDED)
 	struct mmc *mmc;
 	u32 offset1, offset2;
 	int read1_fail = 0, read2_fail = 0;
@@ -408,13 +412,11 @@ err:
 	if (ret)
 		env_set_default(errmsg, 0);
 
-#endif
 	return ret;
 }
 #else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 static int env_mmc_load(void)
 {
-#if !defined(ENV_IS_EMBEDDED)
 	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
 	struct mmc *mmc;
 	u32 offset;
@@ -453,7 +455,7 @@ fini:
 err:
 	if (ret)
 		env_set_default(errmsg, 0);
-#endif
+
 	return ret;
 }
 #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
-- 
2.25.1

[PATCH 4/8] env: mmc: add CONFIG_ENV_MMC_USE_DT

[Patrick Delaunay]

Add a new config CONFIG_ENV_MMC_USE_DT to force configuration of the
U-Boot environment offset with device tree config node.

This patch avoids issues when several CONFIG_ENV_IS_IN_XXX are activated,
the defconfig file uses the same value for CONFIG_ENV_OFFSET or
CONFIG_ENV_OFFSET_REDUND for the several ENV backends (SPI_FLASH, EEPROM
NAND, SATA, MMC).

After this patch a bad offset value is not possible when the selected
partition in device tree is not found.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 env/Kconfig | 16 ++++++++++++++++
 env/mmc.c   |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/env/Kconfig b/env/Kconfig
index 24111dfaf47b..f8ee99052b97 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -242,6 +242,13 @@ config ENV_IS_IN_MMC
 	  This value is also in units of bytes, but must also be aligned to
 	  an MMC sector boundary.
 
+	  CONFIG_ENV_MMC_USE_DT (optional):
+
+	  These define forces the configuration by the config node in device
+	  tree with partition name: "u-boot,mmc-env-partition" or with
+	  offset: "u-boot,mmc-env-offset", "u-boot,mmc-env-offset-redundant".
+	  CONFIG_ENV_OFFSET and CONFIG_ENV_OFFSET_REDUND are not used.
+
 config ENV_IS_IN_NAND
 	bool "Environment in a NAND device"
 	depends on !CHAIN_OF_TRUST
@@ -650,6 +657,15 @@ config SYS_MMC_ENV_PART
 	  partition 0 or the first boot partition, which is 1 or some other defined
 	  partition.
 
+config ENV_MMC_USE_DT
+	bool "Read partition name and offset in DT"
+	depends on ENV_IS_IN_MMC && OF_CONTROL
+	help
+	  Only use the device tree to get the environment location in MMC
+	  device, with partition name or with offset.
+	  The 2 defines CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND
+	  are not used as fallback.
+
 config USE_DEFAULT_ENV_FILE
 	bool "Create default environment from file"
 	help
diff --git a/env/mmc.c b/env/mmc.c
index 661a268ea07d..1894b6483220 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -26,6 +26,12 @@
 
 #define ENV_MMC_INVALID_OFFSET ((s64)-1)
 
+#if defined(CONFIG_ENV_MMC_USE_DT)
+/* ENV offset is invalid when not defined in Device Tree */
+#define ENV_MMC_OFFSET		ENV_MMC_INVALID_OFFSET
+#define ENV_MMC_OFFSET_REDUND	ENV_MMC_INVALID_OFFSET
+
+#else
 /* Default ENV offset when not defined in Device Tree */
 #define ENV_MMC_OFFSET		CONFIG_ENV_OFFSET
 
@@ -34,6 +40,7 @@
 #else
 #define ENV_MMC_OFFSET_REDUND	ENV_MMC_INVALID_OFFSET
 #endif
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
-- 
2.25.1

[PATCH 5/8] configs: stm32mp: activate CONFIG_ENV_MMC_USE_DT

[Patrick Delaunay]

Activate by default CONFIG_ENV_MMC_USE_DT as "u-boot,mmc-env-partition"
should be always use in STMicroelectronics boards device tree to locate
the environment for mmc backend. The 2 defines:
  CONFIG_ENV_OFFSET=0x280000
  CONFIG_ENV_OFFSET_REDUND=0x2C0000
are only valid for spi-nor and not for SD-Card or eMMC.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 configs/stm32mp13_defconfig         | 1 +
 configs/stm32mp15_basic_defconfig   | 1 +
 configs/stm32mp15_defconfig         | 1 +
 configs/stm32mp15_trusted_defconfig | 1 +
 4 files changed, 4 insertions(+)

diff --git a/configs/stm32mp13_defconfig b/configs/stm32mp13_defconfig
index af6b1839d039..4cab07647349 100644
--- a/configs/stm32mp13_defconfig
+++ b/configs/stm32mp13_defconfig
@@ -46,6 +46,7 @@ CONFIG_ENV_IS_IN_MMC=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_SYS_MMC_ENV_DEV=-1
+CONFIG_ENV_MMC_USE_DT=y
 CONFIG_CLK_SCMI=y
 CONFIG_GPIO_HOG=y
 CONFIG_DM_I2C=y
diff --git a/configs/stm32mp15_basic_defconfig b/configs/stm32mp15_basic_defconfig
index 86ebbef0a6c8..4a96ad22bcc8 100644
--- a/configs/stm32mp15_basic_defconfig
+++ b/configs/stm32mp15_basic_defconfig
@@ -91,6 +91,7 @@ CONFIG_ENV_UBI_VOLUME="uboot_config"
 CONFIG_ENV_UBI_VOLUME_REDUND="uboot_config_r"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_SYS_MMC_ENV_DEV=-1
+CONFIG_ENV_MMC_USE_DT=y
 # CONFIG_SPL_ENV_IS_NOWHERE is not set
 # CONFIG_SPL_ENV_IS_IN_SPI_FLASH is not set
 CONFIG_TFTP_TSIZE=y
diff --git a/configs/stm32mp15_defconfig b/configs/stm32mp15_defconfig
index caa79e68834f..151981849de9 100644
--- a/configs/stm32mp15_defconfig
+++ b/configs/stm32mp15_defconfig
@@ -65,6 +65,7 @@ CONFIG_ENV_UBI_VOLUME="uboot_config"
 CONFIG_ENV_UBI_VOLUME_REDUND="uboot_config_r"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_SYS_MMC_ENV_DEV=-1
+CONFIG_ENV_MMC_USE_DT=y
 CONFIG_TFTP_TSIZE=y
 CONFIG_STM32_ADC=y
 CONFIG_CLK_SCMI=y
diff --git a/configs/stm32mp15_trusted_defconfig b/configs/stm32mp15_trusted_defconfig
index 3309c2e79246..098eedc9b727 100644
--- a/configs/stm32mp15_trusted_defconfig
+++ b/configs/stm32mp15_trusted_defconfig
@@ -66,6 +66,7 @@ CONFIG_ENV_UBI_VOLUME="uboot_config"
 CONFIG_ENV_UBI_VOLUME_REDUND="uboot_config_r"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_SYS_MMC_ENV_DEV=-1
+CONFIG_ENV_MMC_USE_DT=y
 CONFIG_TFTP_TSIZE=y
 CONFIG_STM32_ADC=y
 CONFIG_CLK_SCMI=y
-- 
2.25.1

[PATCH 6/8] env: mmc: select GPT env partition by type guid

[Patrick Delaunay]

Since commit c0364ce1c695 ("doc/README.gpt: define partition type GUID for
U-Boot environment"), a specific type GUID can be used to indicate
the U-Boot environment partition on the device with GPT partition table.

This patch uses this type GUID to found the env partition as fallback
when the partition name property "u-boot,mmc-env-partition" is not present
in config node or if the indicated partition name is not found.

The mmc_offset_try_partition() function is reused, it selects the first
partition with the correct type GUID when the parameter 'str' is NULL.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

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

diff --git a/env/mmc.c b/env/mmc.c
index 1894b6483220..bd7d51e6b633 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -74,8 +74,18 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
 		if (ret < 0)
 			return ret;
 
-		if (!strncmp((const char *)info.name, str, sizeof(info.name)))
+		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
 			break;
+#ifdef CONFIG_PARTITION_TYPE_GUID
+		if (!str) {
+			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
+			efi_guid_t type_guid;
+
+			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
+			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
+				break;
+		}
+#endif
 	}
 
 	/* round up to info.blksz */
@@ -112,6 +122,13 @@ static inline s64 mmc_offset(int copy)
 			return val;
 	}
 
+	/* try the GPT partition with "U-Boot ENV" TYPE GUID */
+	if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) {
+		err = mmc_offset_try_partition(NULL, copy, &val);
+		if (!err)
+			return val;
+	}
+
 	defvalue = ENV_MMC_OFFSET;
 	propname = dt_prop.offset;
 
-- 
2.25.1

[PATCH 7/8] env: mmc: add debug message when mmc-env-partition is not found

[Patrick Delaunay]

Add a debug message to indicate a potential issue when
"u-boot,mmc-env-partition" is present in config node of device tree
but this partition name is not found in the mmc device.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 env/mmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/env/mmc.c b/env/mmc.c
index bd7d51e6b633..8941e0f5ff39 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -120,6 +120,7 @@ static inline s64 mmc_offset(int copy)
 		err = mmc_offset_try_partition(str, copy, &val);
 		if (!err)
 			return val;
+		debug("env partition '%s' not found (%d)", str, err);
 	}
 
 	/* try the GPT partition with "U-Boot ENV" TYPE GUID */
-- 
2.25.1

[PATCH 8/8] env: mmc: cosmetic: remove unused macro STR(X)

[Patrick Delaunay]

Remove the unused macro STR(X) since the commit 2b2f727500dc ("env: mmc:
allow support of mmc_get_env_dev with OF_CONTROL")

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

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

diff --git a/env/mmc.c b/env/mmc.c
index 8941e0f5ff39..85761417f283 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -21,9 +21,6 @@
 #include <errno.h>
 #include <dm/ofnode.h>
 
-#define __STR(X) #X
-#define STR(X) __STR(X)
-
 #define ENV_MMC_INVALID_OFFSET ((s64)-1)
 
 #if defined(CONFIG_ENV_MMC_USE_DT)
-- 
2.25.1

Re: [PATCH 0/8] env: mmc: improvements and corrections

[Simon Glass]

Hi Patrick,

On Thu, 10 Nov 2022 at 03:49, Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
>
> Update in U-Boot env mmc backend with several cosmetic changes or
> corrections and 2 new features:
>
> 1/ CONFIG_ENV_MMC_USE_DT = no more use CONFIG_ENV_OFFSET
>    in the mmc ENV backend when this config is activated.
>
>    Requested by the STM32MP STMicroelectronics boards which activate
>    several ENV_IS_IN_XXX; the value of CONFIG_ENV_OFFSET is invalid for
>    SD-Card / eMMC boot; this offset should only used in SPIFlash backend
>    (sf.c) for SPI-NOR boot.
>
>    If this offset is used on mmc backend, when partition name in GPT is
>    not aligned with  U-Boot DT: "u-boot,mmc-env-partition", the behavior
>    is difficult to debug: a partition is corrupted on 'env save' command.
>
> 2/ selects the GPT env partition by the "u-boot-env" type GUID introduced
>    by the commit c0364ce1c695 ("doc/README.gpt: define partition
>    type GUID for U-Boot environment")
>
>    This feature can also avoid issue when 'u-boot-env' partition name
>    change in GPT partitioning but not in the U-Boot DT with
>    "u-boot,mmc-env-partition"
>
> Few check patch warnings remained in the series,
> but after check I can't remove them :
>
> - IS_ENABLED(ENV_MMC_HWPART_REDUND) is normally used as
>   IS_ENABLED(CONFIG_ENV_MMC_HWPART_REDUND)
>   => ENV_MMC_HWPART_REDUND is locally defined in this file it is not
>      a real CONFIG but I can use the IS_ENABLED() macro as it is defined
>      to 1
>
> - Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where
>   possible
>   + CONFIG_PARTITION_TYPE_GUID => info.type_guid existence
>   + CONFIG_ENV_OFFSET_REDUND and CONFIG_ENV_MMC_USE_DT => only for define
>
> As I miss the merge window, not targeted for v2023.01 but for next
> v2023.04.

Shouldn't this all move to device tree? Using CONFIG options is such a
mess. We have the devices in DT so can indicate which ones have an
environment and what the parameters are for each.

Regards,
Simon

Re: [PATCH 0/8] env: mmc: improvements and corrections

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

On Thu, Nov 10, 2022 at 01:40:32PM -0700, Simon Glass wrote:
> Hi Patrick,
> 
> On Thu, 10 Nov 2022 at 03:49, Patrick Delaunay
> <patrick.delaunay@foss.st.com> wrote:
> >
> >
> > Update in U-Boot env mmc backend with several cosmetic changes or
> > corrections and 2 new features:
> >
> > 1/ CONFIG_ENV_MMC_USE_DT = no more use CONFIG_ENV_OFFSET
> >    in the mmc ENV backend when this config is activated.
> >
> >    Requested by the STM32MP STMicroelectronics boards which activate
> >    several ENV_IS_IN_XXX; the value of CONFIG_ENV_OFFSET is invalid for
> >    SD-Card / eMMC boot; this offset should only used in SPIFlash backend
> >    (sf.c) for SPI-NOR boot.
> >
> >    If this offset is used on mmc backend, when partition name in GPT is
> >    not aligned with  U-Boot DT: "u-boot,mmc-env-partition", the behavior
> >    is difficult to debug: a partition is corrupted on 'env save' command.
> >
> > 2/ selects the GPT env partition by the "u-boot-env" type GUID introduced
> >    by the commit c0364ce1c695 ("doc/README.gpt: define partition
> >    type GUID for U-Boot environment")
> >
> >    This feature can also avoid issue when 'u-boot-env' partition name
> >    change in GPT partitioning but not in the U-Boot DT with
> >    "u-boot,mmc-env-partition"
> >
> > Few check patch warnings remained in the series,
> > but after check I can't remove them :
> >
> > - IS_ENABLED(ENV_MMC_HWPART_REDUND) is normally used as
> >   IS_ENABLED(CONFIG_ENV_MMC_HWPART_REDUND)
> >   => ENV_MMC_HWPART_REDUND is locally defined in this file it is not
> >      a real CONFIG but I can use the IS_ENABLED() macro as it is defined
> >      to 1
> >
> > - Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where
> >   possible
> >   + CONFIG_PARTITION_TYPE_GUID => info.type_guid existence
> >   + CONFIG_ENV_OFFSET_REDUND and CONFIG_ENV_MMC_USE_DT => only for define
> >
> > As I miss the merge window, not targeted for v2023.01 but for next
> > v2023.04.
> 
> Shouldn't this all move to device tree? Using CONFIG options is such a
> mess. We have the devices in DT so can indicate which ones have an
> environment and what the parameters are for each.

And there's already the
Documentation/devicetree/bindings/nvmem/u-boot,env.yaml to build upon.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [Uboot-stm32] [PATCH 1/8] env: mmc: introduced ENV_MMC_OFFSET

[Patrice CHOTARD]

On 11/10/22 11:48, Patrick Delaunay wrote:
> Introduce ENV_MMC_OFFSET defines.
> It is a preliminary step to the next patches to simplify the code.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/mmc.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index c28f4c6c6dc0..42bcf7e775cc 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -24,6 +24,17 @@
>  #define __STR(X) #X
>  #define STR(X) __STR(X)
>  
> +#define ENV_MMC_INVALID_OFFSET ((s64)-1)
> +
> +/* Default ENV offset when not defined in Device Tree */
> +#define ENV_MMC_OFFSET		CONFIG_ENV_OFFSET
> +
> +#if defined(CONFIG_ENV_OFFSET_REDUND)
> +#define ENV_MMC_OFFSET_REDUND	CONFIG_ENV_OFFSET_REDUND
> +#else
> +#define ENV_MMC_OFFSET_REDUND	ENV_MMC_INVALID_OFFSET
> +#endif
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  /*
> @@ -94,12 +105,12 @@ static inline s64 mmc_offset(int copy)
>  			return val;
>  	}
>  
> -	defvalue = CONFIG_ENV_OFFSET;
> +	defvalue = ENV_MMC_OFFSET;
>  	propname = dt_prop.offset;
>  
>  #if defined(CONFIG_ENV_OFFSET_REDUND)
>  	if (copy) {
> -		defvalue = CONFIG_ENV_OFFSET_REDUND;
> +		defvalue = ENV_MMC_OFFSET_REDUND;
>  		propname = dt_prop.offset_redund;
>  	}
>  #endif
> @@ -108,11 +119,11 @@ static inline s64 mmc_offset(int copy)
>  #else
>  static inline s64 mmc_offset(int copy)
>  {
> -	s64 offset = CONFIG_ENV_OFFSET;
> +	s64 offset = ENV_MMC_OFFSET;
>  
>  #if defined(CONFIG_ENV_OFFSET_REDUND)
>  	if (copy)
> -		offset = CONFIG_ENV_OFFSET_REDUND;
> +		offset = ENV_MMC_OFFSET_REDUND;
>  #endif
>  	return offset;
>  }
> @@ -122,6 +133,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
>  {
>  	s64 offset = mmc_offset(copy);
>  
> +	if (offset == ENV_MMC_INVALID_OFFSET) {
> +		printf("Invalid ENV offset in MMC, copy=%d\n", copy);
> +		return -ENOENT;
> +	}
> +
>  	if (offset < 0)
>  		offset += mmc->capacity;
>  

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [Uboot-stm32] [PATCH 2/8] env: mcc: Drop unnecessary #ifdefs

[Patrice CHOTARD]

On 11/10/22 11:48, Patrick Delaunay wrote:
> This file has a lot of conditional code and much of it is unnecessary.
> Clean this up to reduce the number of build combinations.
> 
> This patch replaces the test on CONFIG_ENV_OFFSET_REDUND for the
> more coherent CONFIG_SYS_REDUNDAND_ENVIRONMENT.
> 
> This patch also corrects a compilation issue in init_mmc_for_env()
> when CONFIG_SYS_MMC_ENV_PART is not activated, env_mmc_orig_hwpart is
> not defined.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/mmc.c | 120 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 64 insertions(+), 56 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 42bcf7e775cc..b36bd9ad77ee 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -46,7 +46,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
>       (CONFIG_SYS_MMC_ENV_PART == 1) && \
>       (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
> -#define ENV_MMC_HWPART_REDUND
> +#define ENV_MMC_HWPART_REDUND	1
>  #endif
>  
>  #if CONFIG_IS_ENABLED(OF_CONTROL)
> @@ -108,12 +108,11 @@ static inline s64 mmc_offset(int copy)
>  	defvalue = ENV_MMC_OFFSET;
>  	propname = dt_prop.offset;
>  
> -#if defined(CONFIG_ENV_OFFSET_REDUND)
> -	if (copy) {
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && copy) {
>  		defvalue = ENV_MMC_OFFSET_REDUND;
>  		propname = dt_prop.offset_redund;
>  	}
> -#endif
> +
>  	return ofnode_conf_read_int(propname, defvalue);
>  }
>  #else
> @@ -121,10 +120,9 @@ static inline s64 mmc_offset(int copy)
>  {
>  	s64 offset = ENV_MMC_OFFSET;
>  
> -#if defined(CONFIG_ENV_OFFSET_REDUND)
> -	if (copy)
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && copy)
>  		offset = ENV_MMC_OFFSET_REDUND;
> -#endif
> +
>  	return offset;
>  }
>  #endif
> @@ -165,8 +163,24 @@ static int mmc_set_env_part(struct mmc *mmc, uint part)
>  
>  	return ret;
>  }
> +
> +static bool mmc_set_env_part_init(struct mmc *mmc)
> +{
> +	env_mmc_orig_hwpart = mmc_get_blk_desc(mmc)->hwpart;
> +	if (mmc_set_env_part(mmc, mmc_get_env_part(mmc)))
> +		return false;
> +
> +	return true;
> +}
> +
> +static int mmc_set_env_part_restore(struct mmc *mmc)
> +{
> +	return mmc_set_env_part(mmc, env_mmc_orig_hwpart);
> +}
>  #else
>  static inline int mmc_set_env_part(struct mmc *mmc, uint part) {return 0; };
> +static bool mmc_set_env_part_init(struct mmc *mmc) {return true; }
> +static inline int mmc_set_env_part_restore(struct mmc *mmc) {return 0; };
>  #endif
>  
>  static const char *init_mmc_for_env(struct mmc *mmc)
> @@ -183,8 +197,7 @@ static const char *init_mmc_for_env(struct mmc *mmc)
>  	if (mmc_init(mmc))
>  		return "MMC init failed";
>  #endif
> -	env_mmc_orig_hwpart = mmc_get_blk_desc(mmc)->hwpart;
> -	if (mmc_set_env_part(mmc, mmc_get_env_part(mmc)))
> +	if (!mmc_set_env_part_init(mmc))
>  		return "MMC partition switch failed";
>  
>  	return NULL;
> @@ -192,11 +205,7 @@ static const char *init_mmc_for_env(struct mmc *mmc)
>  
>  static void fini_mmc_for_env(struct mmc *mmc)
>  {
> -#ifdef CONFIG_SYS_MMC_ENV_PART
> -	int dev = mmc_get_env_dev();
> -
> -	blk_select_hwpart_devnum(UCLASS_MMC, dev, env_mmc_orig_hwpart);
> -#endif
> +	mmc_set_env_part_restore(mmc);
>  }
>  
>  #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
> @@ -233,21 +242,20 @@ static int env_mmc_save(void)
>  	if (ret)
>  		goto fini;
>  
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> -	if (gd->env_valid == ENV_VALID)
> -		copy = 1;
> -
> -#ifdef ENV_MMC_HWPART_REDUND
> -	ret = mmc_set_env_part(mmc, copy + 1);
> -	if (ret)
> -		goto fini;
> -#endif
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
> +		if (gd->env_valid == ENV_VALID)
> +			copy = 1;
>  
> -#endif
> +		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> +			ret = mmc_set_env_part(mmc, copy + 1);
> +			if (ret)
> +				goto fini;
> +		}
>  
> -	if (mmc_get_env_addr(mmc, copy, &offset)) {
> -		ret = 1;
> -		goto fini;
> +		if (mmc_get_env_addr(mmc, copy, &offset)) {
> +			ret = 1;
> +			goto fini;
> +		}
>  	}
>  
>  	printf("Writing to %sMMC(%d)... ", copy ? "redundant " : "", dev);
> @@ -259,12 +267,12 @@ static int env_mmc_save(void)
>  
>  	ret = 0;
>  
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> -	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
> -#endif
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> +		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>  
>  fini:
>  	fini_mmc_for_env(mmc);
> +
>  	return ret;
>  }
>  
> @@ -308,22 +316,22 @@ static int env_mmc_erase(void)
>  	printf("\n");
>  	ret = erase_env(mmc, CONFIG_ENV_SIZE, offset);
>  
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> -	copy = 1;
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
> +		copy = 1;
>  
> -#ifdef ENV_MMC_HWPART_REDUND
> -	ret = mmc_set_env_part(mmc, copy + 1);
> -	if (ret)
> -		goto fini;
> -#endif
> +		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> +			ret = mmc_set_env_part(mmc, copy + 1);
> +			if (ret)
> +				goto fini;
> +		}
>  
> -	if (mmc_get_env_addr(mmc, copy, &offset)) {
> -		ret = CMD_RET_FAILURE;
> -		goto fini;
> -	}
> +		if (mmc_get_env_addr(mmc, copy, &offset)) {
> +			ret = CMD_RET_FAILURE;
> +			goto fini;
> +		}
>  
> -	ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
> -#endif
> +		ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
> +	}
>  
>  fini:
>  	fini_mmc_for_env(mmc);
> @@ -345,7 +353,7 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
>  	return (n == blk_cnt) ? 0 : -1;
>  }
>  
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> +#if defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
>  static int env_mmc_load(void)
>  {
>  #if !defined(ENV_IS_EMBEDDED)
> @@ -375,19 +383,19 @@ static int env_mmc_load(void)
>  		goto fini;
>  	}
>  
> -#ifdef ENV_MMC_HWPART_REDUND
> -	ret = mmc_set_env_part(mmc, 1);
> -	if (ret)
> -		goto fini;
> -#endif
> +	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> +		ret = mmc_set_env_part(mmc, 1);
> +		if (ret)
> +			goto fini;
> +	}
>  
>  	read1_fail = read_env(mmc, CONFIG_ENV_SIZE, offset1, tmp_env1);
>  
> -#ifdef ENV_MMC_HWPART_REDUND
> -	ret = mmc_set_env_part(mmc, 2);
> -	if (ret)
> -		goto fini;
> -#endif
> +	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> +		ret = mmc_set_env_part(mmc, 2);
> +		if (ret)
> +			goto fini;
> +	}
>  
>  	read2_fail = read_env(mmc, CONFIG_ENV_SIZE, offset2, tmp_env2);
>  
> @@ -403,7 +411,7 @@ err:
>  #endif
>  	return ret;
>  }
> -#else /* ! CONFIG_ENV_OFFSET_REDUND */
> +#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
>  static int env_mmc_load(void)
>  {
>  #if !defined(ENV_IS_EMBEDDED)
> @@ -448,7 +456,7 @@ err:
>  #endif
>  	return ret;
>  }
> -#endif /* CONFIG_ENV_OFFSET_REDUND */
> +#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
>  
>  U_BOOT_ENV_LOCATION(mmc) = {
>  	.location	= ENVL_MMC,

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [Uboot-stm32] [PATCH 3/8] env: mcc: fix compilation error with ENV_IS_EMBEDDED

[Patrice CHOTARD]

On 11/10/22 11:49, Patrick Delaunay wrote:
> When ENV_IS_EMBEDDED is enabled, ret is not defined but is used as a
> return value in env_mmc_load().
> This patch correct this issue and simplify the existing code, test only
> one time #if defined(ENV_IS_EMBEDDED) and not in the function.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/mmc.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index b36bd9ad77ee..661a268ea07d 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -353,10 +353,14 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
>  	return (n == blk_cnt) ? 0 : -1;
>  }
>  
> -#if defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
> +#if defined(ENV_IS_EMBEDDED)
> +static int env_mmc_load(void)
> +{
> +	return 0;
> +}
> +#elif defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT)
>  static int env_mmc_load(void)
>  {
> -#if !defined(ENV_IS_EMBEDDED)
>  	struct mmc *mmc;
>  	u32 offset1, offset2;
>  	int read1_fail = 0, read2_fail = 0;
> @@ -408,13 +412,11 @@ err:
>  	if (ret)
>  		env_set_default(errmsg, 0);
>  
> -#endif
>  	return ret;
>  }
>  #else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
>  static int env_mmc_load(void)
>  {
> -#if !defined(ENV_IS_EMBEDDED)
>  	ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
>  	struct mmc *mmc;
>  	u32 offset;
> @@ -453,7 +455,7 @@ fini:
>  err:
>  	if (ret)
>  		env_set_default(errmsg, 0);
> -#endif
> +
>  	return ret;
>  }
>  #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [Uboot-stm32] [PATCH 4/8] env: mmc: add CONFIG_ENV_MMC_USE_DT

[Patrice CHOTARD]

On 11/10/22 11:49, Patrick Delaunay wrote:
> Add a new config CONFIG_ENV_MMC_USE_DT to force configuration of the
> U-Boot environment offset with device tree config node.
> 
> This patch avoids issues when several CONFIG_ENV_IS_IN_XXX are activated,
> the defconfig file uses the same value for CONFIG_ENV_OFFSET or
> CONFIG_ENV_OFFSET_REDUND for the several ENV backends (SPI_FLASH, EEPROM
> NAND, SATA, MMC).
> 
> After this patch a bad offset value is not possible when the selected
> partition in device tree is not found.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/Kconfig | 16 ++++++++++++++++
>  env/mmc.c   |  7 +++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/env/Kconfig b/env/Kconfig
> index 24111dfaf47b..f8ee99052b97 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -242,6 +242,13 @@ config ENV_IS_IN_MMC
>  	  This value is also in units of bytes, but must also be aligned to
>  	  an MMC sector boundary.
>  
> +	  CONFIG_ENV_MMC_USE_DT (optional):
> +
> +	  These define forces the configuration by the config node in device
> +	  tree with partition name: "u-boot,mmc-env-partition" or with
> +	  offset: "u-boot,mmc-env-offset", "u-boot,mmc-env-offset-redundant".
> +	  CONFIG_ENV_OFFSET and CONFIG_ENV_OFFSET_REDUND are not used.
> +
>  config ENV_IS_IN_NAND
>  	bool "Environment in a NAND device"
>  	depends on !CHAIN_OF_TRUST
> @@ -650,6 +657,15 @@ config SYS_MMC_ENV_PART
>  	  partition 0 or the first boot partition, which is 1 or some other defined
>  	  partition.
>  
> +config ENV_MMC_USE_DT
> +	bool "Read partition name and offset in DT"
> +	depends on ENV_IS_IN_MMC && OF_CONTROL
> +	help
> +	  Only use the device tree to get the environment location in MMC
> +	  device, with partition name or with offset.
> +	  The 2 defines CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND
> +	  are not used as fallback.
> +
>  config USE_DEFAULT_ENV_FILE
>  	bool "Create default environment from file"
>  	help
> diff --git a/env/mmc.c b/env/mmc.c
> index 661a268ea07d..1894b6483220 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -26,6 +26,12 @@
>  
>  #define ENV_MMC_INVALID_OFFSET ((s64)-1)
>  
> +#if defined(CONFIG_ENV_MMC_USE_DT)
> +/* ENV offset is invalid when not defined in Device Tree */
> +#define ENV_MMC_OFFSET		ENV_MMC_INVALID_OFFSET
> +#define ENV_MMC_OFFSET_REDUND	ENV_MMC_INVALID_OFFSET
> +
> +#else
>  /* Default ENV offset when not defined in Device Tree */
>  #define ENV_MMC_OFFSET		CONFIG_ENV_OFFSET
>  
> @@ -34,6 +40,7 @@
>  #else
>  #define ENV_MMC_OFFSET_REDUND	ENV_MMC_INVALID_OFFSET
>  #endif
> +#endif
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [Uboot-stm32] [PATCH 6/8] env: mmc: select GPT env partition by type guid

[Patrice CHOTARD]

On 11/10/22 11:49, Patrick Delaunay wrote:
> Since commit c0364ce1c695 ("doc/README.gpt: define partition type GUID for
> U-Boot environment"), a specific type GUID can be used to indicate
> the U-Boot environment partition on the device with GPT partition table.
> 
> This patch uses this type GUID to found the env partition as fallback
> when the partition name property "u-boot,mmc-env-partition" is not present
> in config node or if the indicated partition name is not found.
> 
> The mmc_offset_try_partition() function is reused, it selects the first
> partition with the correct type GUID when the parameter 'str' is NULL.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/mmc.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 1894b6483220..bd7d51e6b633 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -74,8 +74,18 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
>  		if (ret < 0)
>  			return ret;
>  
> -		if (!strncmp((const char *)info.name, str, sizeof(info.name)))
> +		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
>  			break;
> +#ifdef CONFIG_PARTITION_TYPE_GUID
> +		if (!str) {
> +			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
> +			efi_guid_t type_guid;
> +
> +			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
> +			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
> +				break;
> +		}
> +#endif
>  	}
>  
>  	/* round up to info.blksz */
> @@ -112,6 +122,13 @@ static inline s64 mmc_offset(int copy)
>  			return val;
>  	}
>  
> +	/* try the GPT partition with "U-Boot ENV" TYPE GUID */
> +	if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) {
> +		err = mmc_offset_try_partition(NULL, copy, &val);
> +		if (!err)
> +			return val;
> +	}
> +
>  	defvalue = ENV_MMC_OFFSET;
>  	propname = dt_prop.offset;
>  

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [Uboot-stm32] [PATCH 7/8] env: mmc: add debug message when mmc-env-partition is not found

[Patrice CHOTARD]

On 11/10/22 11:49, Patrick Delaunay wrote:
> Add a debug message to indicate a potential issue when
> "u-boot,mmc-env-partition" is present in config node of device tree
> but this partition name is not found in the mmc device.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/mmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index bd7d51e6b633..8941e0f5ff39 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -120,6 +120,7 @@ static inline s64 mmc_offset(int copy)
>  		err = mmc_offset_try_partition(str, copy, &val);
>  		if (!err)
>  			return val;
> +		debug("env partition '%s' not found (%d)", str, err);
>  	}
>  
>  	/* try the GPT partition with "U-Boot ENV" TYPE GUID */

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [Uboot-stm32] [PATCH 8/8] env: mmc: cosmetic: remove unused macro STR(X)

[Patrice CHOTARD]

On 11/10/22 11:49, Patrick Delaunay wrote:
> Remove the unused macro STR(X) since the commit 2b2f727500dc ("env: mmc:
> allow support of mmc_get_env_dev with OF_CONTROL")
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  env/mmc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 8941e0f5ff39..85761417f283 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -21,9 +21,6 @@
>  #include <errno.h>
>  #include <dm/ofnode.h>
>  
> -#define __STR(X) #X
> -#define STR(X) __STR(X)
> -
>  #define ENV_MMC_INVALID_OFFSET ((s64)-1)
>  
>  #if defined(CONFIG_ENV_MMC_USE_DT)

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH 5/8] configs: stm32mp: activate CONFIG_ENV_MMC_USE_DT

[Patrice CHOTARD]

On 11/10/22 11:49, Patrick Delaunay wrote:
> Activate by default CONFIG_ENV_MMC_USE_DT as "u-boot,mmc-env-partition"
> should be always use in STMicroelectronics boards device tree to locate
> the environment for mmc backend. The 2 defines:
>   CONFIG_ENV_OFFSET=0x280000
>   CONFIG_ENV_OFFSET_REDUND=0x2C0000
> are only valid for spi-nor and not for SD-Card or eMMC.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  configs/stm32mp13_defconfig         | 1 +
>  configs/stm32mp15_basic_defconfig   | 1 +
>  configs/stm32mp15_defconfig         | 1 +
>  configs/stm32mp15_trusted_defconfig | 1 +
>  4 files changed, 4 insertions(+)
> 
> diff --git a/configs/stm32mp13_defconfig b/configs/stm32mp13_defconfig
> index af6b1839d039..4cab07647349 100644
> --- a/configs/stm32mp13_defconfig
> +++ b/configs/stm32mp13_defconfig
> @@ -46,6 +46,7 @@ CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>  CONFIG_SYS_MMC_ENV_DEV=-1
> +CONFIG_ENV_MMC_USE_DT=y
>  CONFIG_CLK_SCMI=y
>  CONFIG_GPIO_HOG=y
>  CONFIG_DM_I2C=y
> diff --git a/configs/stm32mp15_basic_defconfig b/configs/stm32mp15_basic_defconfig
> index 86ebbef0a6c8..4a96ad22bcc8 100644
> --- a/configs/stm32mp15_basic_defconfig
> +++ b/configs/stm32mp15_basic_defconfig
> @@ -91,6 +91,7 @@ CONFIG_ENV_UBI_VOLUME="uboot_config"
>  CONFIG_ENV_UBI_VOLUME_REDUND="uboot_config_r"
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>  CONFIG_SYS_MMC_ENV_DEV=-1
> +CONFIG_ENV_MMC_USE_DT=y
>  # CONFIG_SPL_ENV_IS_NOWHERE is not set
>  # CONFIG_SPL_ENV_IS_IN_SPI_FLASH is not set
>  CONFIG_TFTP_TSIZE=y
> diff --git a/configs/stm32mp15_defconfig b/configs/stm32mp15_defconfig
> index caa79e68834f..151981849de9 100644
> --- a/configs/stm32mp15_defconfig
> +++ b/configs/stm32mp15_defconfig
> @@ -65,6 +65,7 @@ CONFIG_ENV_UBI_VOLUME="uboot_config"
>  CONFIG_ENV_UBI_VOLUME_REDUND="uboot_config_r"
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>  CONFIG_SYS_MMC_ENV_DEV=-1
> +CONFIG_ENV_MMC_USE_DT=y
>  CONFIG_TFTP_TSIZE=y
>  CONFIG_STM32_ADC=y
>  CONFIG_CLK_SCMI=y
> diff --git a/configs/stm32mp15_trusted_defconfig b/configs/stm32mp15_trusted_defconfig
> index 3309c2e79246..098eedc9b727 100644
> --- a/configs/stm32mp15_trusted_defconfig
> +++ b/configs/stm32mp15_trusted_defconfig
> @@ -66,6 +66,7 @@ CONFIG_ENV_UBI_VOLUME="uboot_config"
>  CONFIG_ENV_UBI_VOLUME_REDUND="uboot_config_r"
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>  CONFIG_SYS_MMC_ENV_DEV=-1
> +CONFIG_ENV_MMC_USE_DT=y
>  CONFIG_TFTP_TSIZE=y
>  CONFIG_STM32_ADC=y
>  CONFIG_CLK_SCMI=y

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

Re: [PATCH 0/8] env: mmc: improvements and corrections

[Tom Rini]

On Thu, 10 Nov 2022 11:48:57 +0100, Patrick Delaunay wrote:

> Update in U-Boot env mmc backend with several cosmetic changes or
> corrections and 2 new features:
> 
> 1/ CONFIG_ENV_MMC_USE_DT = no more use CONFIG_ENV_OFFSET
>    in the mmc ENV backend when this config is activated.
> 
>    Requested by the STM32MP STMicroelectronics boards which activate
>    several ENV_IS_IN_XXX; the value of CONFIG_ENV_OFFSET is invalid for
>    SD-Card / eMMC boot; this offset should only used in SPIFlash backend
>    (sf.c) for SPI-NOR boot.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom