* [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties
@ 2025-02-11 13:31 Marek Vasut
2025-02-11 13:47 ` Quentin Schulz
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Marek Vasut @ 2025-02-11 13:31 UTC (permalink / raw)
To: u-boot
Cc: Marek Vasut, Dragan Simic, Joe Hershberger, Mattijs Korpershoek,
Quentin Schulz, Rasmus Villemoes, Simon Glass, Tom Rini
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 <marex@denx.de>
---
Cc: Dragan Simic <dsimic@manjaro.org>
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Quentin Schulz <quentin.schulz@cherry.de>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@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;
+
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties
2025-02-11 13:31 [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties Marek Vasut
@ 2025-02-11 13:47 ` Quentin Schulz
2025-02-11 16:06 ` Marek Vasut
2025-02-13 8:30 ` Mattijs Korpershoek
2025-02-18 22:31 ` Tom Rini
2 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2025-02-11 13:47 UTC (permalink / raw)
To: Marek Vasut, u-boot
Cc: Dragan Simic, Joe Hershberger, Mattijs Korpershoek,
Rasmus Villemoes, Simon Glass, Tom Rini
Hi Marek,
On 2/11/25 2:31 PM, Marek Vasut 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 <marex@denx.de>
> ---
> Cc: Dragan Simic <dsimic@manjaro.org>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@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)
Oof what a terrible name for the actual meaning :/
MMC hardware partition device number on the platform where the
environment is stored. Note that this is not related to any
software
defined partition table but instead if we are in the user
area, which is
partition 0 or the first boot partition, which is 1 or some
other defined
partition.
Would benefit from a small comment making this less confusing in the code.
In any case,
Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Less obscure ifdefery makes me happy.
Thanks!
Quentin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties
2025-02-11 13:47 ` Quentin Schulz
@ 2025-02-11 16:06 ` Marek Vasut
2025-02-11 16:39 ` Quentin Schulz
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2025-02-11 16:06 UTC (permalink / raw)
To: Quentin Schulz, u-boot
Cc: Dragan Simic, Joe Hershberger, Mattijs Korpershoek,
Rasmus Villemoes, Simon Glass, Tom Rini
On 2/11/25 2:47 PM, Quentin Schulz wrote:
Hi,
>> + if (CONFIG_SYS_MMC_ENV_PART != 1)
>
> Oof what a terrible name for the actual meaning :/
>
> MMC hardware partition device number on the platform where the
> environment is stored. Note that this is not related to any
> software
> defined partition table but instead if we are in the user
> area, which is
> partition 0 or the first boot partition, which is 1 or some
> other defined
> partition.
>
> Would benefit from a small comment making this less confusing in the code.
Maybe CONFIG_SYS_MMC_ENV_PART should be renamed instead ... to some
CONFIG_ENV_MMC_HW_PART
> In any case,
>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>
> Less obscure ifdefery makes me happy.
Thank you
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties
2025-02-11 16:06 ` Marek Vasut
@ 2025-02-11 16:39 ` Quentin Schulz
0 siblings, 0 replies; 6+ messages in thread
From: Quentin Schulz @ 2025-02-11 16:39 UTC (permalink / raw)
To: Marek Vasut, u-boot
Cc: Dragan Simic, Joe Hershberger, Mattijs Korpershoek,
Rasmus Villemoes, Simon Glass, Tom Rini
Hi Marek,
On 2/11/25 5:06 PM, Marek Vasut wrote:
> On 2/11/25 2:47 PM, Quentin Schulz wrote:
>
> Hi,
>
>>> + if (CONFIG_SYS_MMC_ENV_PART != 1)
>>
>> Oof what a terrible name for the actual meaning :/
>>
>> MMC hardware partition device number on the platform where the
>> environment is stored. Note that this is not related to
>> any software
>> defined partition table but instead if we are in the user
>> area, which is
>> partition 0 or the first boot partition, which is 1 or some
>> other defined
>> partition.
>>
>> Would benefit from a small comment making this less confusing in the
>> code.
>
> Maybe CONFIG_SYS_MMC_ENV_PART should be renamed instead ... to some
> CONFIG_ENV_MMC_HW_PART
>
config ENV_MMC_IN_BOOT_PART
bool "env in eMMC boot partition"
help
Store the environment in eMMC boot partition (y) or in user
partition (n).
Maybe?
Out of scope for this patch though :)
Cheers,
Quentin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties
2025-02-11 13:31 [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties Marek Vasut
2025-02-11 13:47 ` Quentin Schulz
@ 2025-02-13 8:30 ` Mattijs Korpershoek
2025-02-18 22:31 ` Tom Rini
2 siblings, 0 replies; 6+ messages in thread
From: Mattijs Korpershoek @ 2025-02-13 8:30 UTC (permalink / raw)
To: Marek Vasut, u-boot
Cc: Marek Vasut, Dragan Simic, Joe Hershberger, Quentin Schulz,
Rasmus Villemoes, Simon Glass, Tom Rini
Hi Marek,
Thank you for the patch.
On mar., févr. 11, 2025 at 14:31, Marek Vasut <marex@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 <marex@denx.de>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Some question below.
> ---
> Cc: Dragan Simic <dsimic@manjaro.org>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties
2025-02-11 13:31 [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties Marek Vasut
2025-02-11 13:47 ` Quentin Schulz
2025-02-13 8:30 ` Mattijs Korpershoek
@ 2025-02-18 22:31 ` Tom Rini
2 siblings, 0 replies; 6+ messages in thread
From: Tom Rini @ 2025-02-18 22:31 UTC (permalink / raw)
To: Marek Vasut
Cc: u-boot, Dragan Simic, Joe Hershberger, Mattijs Korpershoek,
Quentin Schulz, Rasmus Villemoes, Simon Glass
[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]
On Tue, Feb 11, 2025 at 02:31:08PM +0100, Marek Vasut 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 <marex@denx.de>
This introduces a failure to build on a number of platforms including
verdin-am62_a53 with:
+(verdin-am62_a53) env/mmc.c:208:13: error: 'mmc_env_is_redundant_in_both_boot_hwparts' defined but not used [-Werror=unused-function]
+(verdin-am62_a53) 208 | static bool mmc_env_is_redundant_in_both_boot_hwparts(struct mmc *mmc)
+(verdin-am62_a53) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+(verdin-am62_a53) cc1: all warnings being treated as errors
+(verdin-am62_a53) make[3]: *** [scripts/Makefile.build:257: spl/env/mmc.o] Error 1
+(verdin-am62_a53) make[2]: *** [scripts/Makefile.xpl:550: spl/env] Error 2
+(verdin-am62_a53) make[1]: *** [Makefile:2115: spl/u-boot-spl] Error 2
+(verdin-am62_a53) make: *** [Makefile:177: sub-make] Error 2
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-18 22:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-11 13:31 [PATCH v2] env: mmc: Make redundant env in both eMMC boot partitions consider DT properties Marek Vasut
2025-02-11 13:47 ` Quentin Schulz
2025-02-11 16:06 ` Marek Vasut
2025-02-11 16:39 ` Quentin Schulz
2025-02-13 8:30 ` Mattijs Korpershoek
2025-02-18 22:31 ` Tom Rini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox