Hi Marek,
Thank you for the patch.
On mar., févr. 11, 2025 at 14:31, Marek Vasut <ma...@denx.de> wrote:
> Introduce a new function mmc_env_is_redundant_in_both_boot_hwparts()
> which replaces IS_ENABLED(ENV_MMC_HWPART_REDUND) and internally does
> almost the same check as the macro which assigned ENV_MMC_HWPART_REDUND
> did, and call it in place of IS_ENABLED(ENV_MMC_HWPART_REDUND).
>
> The difference compared to IS_ENABLED(ENV_MMC_HWPART_REDUND) is
> in the last conditional, which does not do plain macro compare
> (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND), but instead does
> mmc_offset(mmc, 0) == mmc_offset(mmc, 1). If OF_CONTROL is not
> in use, this gets optimized back to original macro compare, but
> if OF_CONTROL is in use, this also takes into account the DT
> properties u-boot,mmc-env-offset and u-boot,mmc-env-offset-redundant.
>
> Signed-off-by: Marek Vasut <ma...@denx.de>
Reviewed-by: Mattijs Korpershoek <mkorpersh...@baylibre.com>
Some question below.
> ---
> Cc: Dragan Simic <dsi...@manjaro.org>
> Cc: Joe Hershberger <joe.hershber...@ni.com>
> Cc: Mattijs Korpershoek <mkorpersh...@baylibre.com>
> Cc: Quentin Schulz <quentin.sch...@cherry.de>
> Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
> Cc: Simon Glass <s...@chromium.org>
> Cc: Tom Rini <tr...@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
> V2: - Rename mmc_env_hwpart_redund() to
> mmc_env_is_redundant_in_both_boot_hwparts()
> - Return bool
> ---
> env/mmc.c | 37 +++++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/env/mmc.c b/env/mmc.c
> index 379f5ec9be7..c4467333263 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -40,18 +40,6 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -/*
> - * In case the environment is redundant, stored in eMMC hardware boot
> - * partition and the environment and redundant environment offsets are
> - * identical, store the environment and redundant environment in both
> - * eMMC boot partitions, one copy in each.
> - * */
> -#if (defined(CONFIG_SYS_REDUNDAND_ENVIRONMENT) && \
> - (CONFIG_SYS_MMC_ENV_PART == 1) && \
> - (CONFIG_ENV_OFFSET == CONFIG_ENV_OFFSET_REDUND))
> -#define ENV_MMC_HWPART_REDUND 1
> -#endif
> -
> #if CONFIG_IS_ENABLED(OF_CONTROL)
>
> static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
> @@ -217,6 +205,23 @@ static inline s64 mmc_offset(struct mmc *mmc, int copy)
> }
> #endif
>
> +static bool mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
> +{
> + /*
> + * In case the environment is redundant, stored in eMMC hardware boot
> + * partition and the environment and redundant environment offsets are
> + * identical, store the environment and redundant environment in both
> + * eMMC boot partitions, one copy in each.
> + */
> + if (!IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> + return false;
> +
> + if (CONFIG_SYS_MMC_ENV_PART != 1)
> + return false;
Looking at the description of this KConfig entry:
"""
CONFIG_SYS_MMC_ENV_PART (optional):
Specifies which MMC partition the environment is stored in. If not
set, defaults to partition 0, the user area. Common values might be
1 (first MMC boot partition), 2 (second MMC boot partition).
"""
Makes me wonder: does this work when the environment is stored in the
second MMC boot partition ?
I'm not sure.
This is also out of scope for the patch though, since the original
#ifdefery already has this.
> +
> + return mmc_offset(mmc, 0) == mmc_offset(mmc, 1);
> +}
> +
> __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
> {
> s64 offset = mmc_offset(mmc, copy);
> @@ -336,7 +341,7 @@ static int env_mmc_save(void)
> if (gd->env_valid == ENV_VALID)
> copy = 1;
>
> - if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> + if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
> ret = mmc_set_env_part(mmc, copy + 1);
> if (ret)
> goto fini;
> @@ -409,7 +414,7 @@ static int env_mmc_erase(void)
> if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
> copy = 1;
>
> - if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> + if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
> ret = mmc_set_env_part(mmc, copy + 1);
> if (ret)
> goto fini;
> @@ -477,7 +482,7 @@ static int env_mmc_load(void)
> goto fini;
> }
>
> - if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> + if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
> ret = mmc_set_env_part(mmc, 1);
> if (ret)
> goto fini;
> @@ -485,7 +490,7 @@ static int env_mmc_load(void)
>
> read1_fail = read_env(mmc, CONFIG_ENV_SIZE, offset1, tmp_env1);
>
> - if (IS_ENABLED(ENV_MMC_HWPART_REDUND)) {
> + if (mmc_env_is_redundant_in_both_boot_hwparts(mmc)) {
> ret = mmc_set_env_part(mmc, 2);
> if (ret)
> goto fini;
> --
> 2.47.2
