Merge branch '2023-01-10-env-mmc-improvements-and-corrections'

To quote the author:

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 c0364ce1c6 ("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"

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

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

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_USE_SERVERIP=y
 CONFIG_SERVERIP="192.168.1.1"

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_USE_SERVERIP=y
 CONFIG_SERVERIP="192.168.1.1"

env/Kconfig

@@ -244,6 +244,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
@@ -652,6 +659,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

env/mmc.c

@@ -21,8 +21,23 @@
 #include <errno.h>
 #include <dm/ofnode.h>
 
-#define __STR(X) #X
+#define ENV_MMC_INVALID_OFFSET ((s64)-1)
-#define STR(X) __STR(X)
+
+#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
+
+#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
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -35,7 +50,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)
@@ -56,8 +71,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 */
@@ -92,28 +117,34 @@ 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);
 	}
 
-	defvalue = CONFIG_ENV_OFFSET;
+	/* 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;
 
-#if defined(CONFIG_ENV_OFFSET_REDUND)
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && copy) {
-	if (copy) {
+		defvalue = ENV_MMC_OFFSET_REDUND;
-		defvalue = CONFIG_ENV_OFFSET_REDUND;
 		propname = dt_prop.offset_redund;
 	}
-#endif
+
 	return ofnode_conf_read_int(propname, defvalue);
 }
 #else
 static inline s64 mmc_offset(int copy)
 {
-	s64 offset = CONFIG_ENV_OFFSET;
+	s64 offset = ENV_MMC_OFFSET;
+
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && copy)
+		offset = ENV_MMC_OFFSET_REDUND;
 
-#if defined(CONFIG_ENV_OFFSET_REDUND)
-	if (copy)
-		offset = CONFIG_ENV_OFFSET_REDUND;
-#endif
 	return offset;
 }
 #endif
@@ -122,6 +153,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;
 
@@ -149,8 +185,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)
@@ -167,8 +219,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_init(mmc))
-	if (mmc_set_env_part(mmc, mmc_get_env_part(mmc)))
 		return "MMC partition switch failed";
 
 	return NULL;
@@ -176,11 +227,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
+	mmc_set_env_part_restore(mmc);
-	int dev = mmc_get_env_dev();
-
-	blk_select_hwpart_devnum(UCLASS_MMC, dev, env_mmc_orig_hwpart);
-#endif
 }
 
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
@@ -217,21 +264,20 @@ static int env_mmc_save(void)
 	if (ret)
 		goto fini;
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
-	if (gd->env_valid == ENV_VALID)
+		if (gd->env_valid == ENV_VALID)
-		copy = 1;
+			copy = 1;
 
-#ifdef ENV_MMC_HWPART_REDUND
+		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
-	ret = mmc_set_env_part(mmc, copy + 1);
+			ret = mmc_set_env_part(mmc, copy + 1);
-	if (ret)
+			if (ret)
-		goto fini;
+				goto fini;
-#endif
+		}
 
-#endif
+		if (mmc_get_env_addr(mmc, copy, &offset)) {
-
+			ret = 1;
-	if (mmc_get_env_addr(mmc, copy, &offset)) {
+			goto fini;
-		ret = 1;
+		}
-		goto fini;
 	}
 
 	printf("Writing to %sMMC(%d)... ", copy ? "redundant " : "", dev);
@@ -243,12 +289,12 @@ static int env_mmc_save(void)
 
 	ret = 0;
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
-#endif
 
 fini:
 	fini_mmc_for_env(mmc);
+
 	return ret;
 }
 
@@ -292,23 +338,23 @@ static int env_mmc_erase(void)
 	printf("\n");
 	ret = erase_env(mmc, CONFIG_ENV_SIZE, offset);
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
-	copy = 1;
+		copy = 1;
 
-#ifdef ENV_MMC_HWPART_REDUND
+		if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
-	ret = mmc_set_env_part(mmc, copy + 1);
+			ret = mmc_set_env_part(mmc, copy + 1);
-	if (ret)
+			if (ret)
-		goto fini;
+				goto fini;
-#endif
+		}
 
-	if (mmc_get_env_addr(mmc, copy, &offset)) {
+		if (mmc_get_env_addr(mmc, copy, &offset)) {
-		ret = CMD_RET_FAILURE;
+			ret = CMD_RET_FAILURE;
-		goto fini;
+			goto fini;
+		}
+
+		ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
 	}
 
-	ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
-#endif
-
 fini:
 	fini_mmc_for_env(mmc);
 	return ret;
@@ -329,10 +375,14 @@ static inline int read_env(struct mmc *mmc, unsigned long size,
 	return (n == blk_cnt) ? 0 : -1;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
+#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;
@@ -359,19 +409,19 @@ static int env_mmc_load(void)
 		goto fini;
 	}
 
-#ifdef ENV_MMC_HWPART_REDUND
+	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
-	ret = mmc_set_env_part(mmc, 1);
+		ret = mmc_set_env_part(mmc, 1);
-	if (ret)
+		if (ret)
-		goto fini;
+			goto fini;
-#endif
+	}
 
 	read1_fail = read_env(mmc, CONFIG_ENV_SIZE, offset1, tmp_env1);
 
-#ifdef ENV_MMC_HWPART_REDUND
+	if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
-	ret = mmc_set_env_part(mmc, 2);
+		ret = mmc_set_env_part(mmc, 2);
-	if (ret)
+		if (ret)
-		goto fini;
+			goto fini;
-#endif
+	}
 
 	read2_fail = read_env(mmc, CONFIG_ENV_SIZE, offset2, tmp_env2);
 
@@ -384,13 +434,11 @@ err:
 	if (ret)
 		env_set_default(errmsg, 0);
 
-#endif
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+#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;
@@ -429,10 +477,10 @@ fini:
 err:
 	if (ret)
 		env_set_default(errmsg, 0);
-#endif
+
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
+#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 
 U_BOOT_ENV_LOCATION(mmc) = {
 	.location	= ENVL_MMC,