[U-Boot] [PATCH 1/2] env_mmc: make board configurable the partition for the environment

[U-Boot] [PATCH 1/2] env_mmc: make board configurable the partition for the environment

[Hector Palacios]

This complements commit 9404a5fc7cb58 "env_mmc: allow environment to be
in an eMMC partition" by allowing boards to accommodate the partition
to use for the environment in different scenarios (similarly to what is
done with the mmc dev number). Depending on the detected boot media,
boards may decide to store the environment in a different partition.

The __weak function also allows to remove some ifdefs from the code.
If CONFIG_SYS_MMC_ENV_PART is not defined, partition 0 is assumed
(default value for U-Boot when a partition is not provided).

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
CC: Stephen Warren <swarren@nvidia.com>
CC: Andy Fleming <afleming@freescale.com>
---
 common/env_mmc.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/common/env_mmc.c b/common/env_mmc.c
index 78c2bc7a1f08..d569b070e005 100644
--- a/common/env_mmc.c
+++ b/common/env_mmc.c
@@ -64,6 +64,13 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
 __weak int mmc_get_env_devno(void)
 {
 	return CONFIG_SYS_MMC_ENV_DEV;
+
+__weak int mmc_get_env_partno(void)
+{
+#ifdef CONFIG_SYS_MMC_ENV_PART
+	return CONFIG_SYS_MMC_ENV_PART;
+#endif
+	return 0;
 }
 
 int env_init(void)
@@ -77,6 +84,9 @@ int env_init(void)
 
 static int init_mmc_for_env(struct mmc *mmc)
 {
+	int mmc_env_devno = mmc_get_env_devno();
+	int mmc_env_partno = mmc_get_env_partno();
+
 	if (!mmc) {
 		puts("No MMC card found\n");
 		return -1;
@@ -87,30 +97,23 @@ static int init_mmc_for_env(struct mmc *mmc)
 		return -1;
 	}
 
-#ifdef CONFIG_SYS_MMC_ENV_PART
-	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {
-		int mmc_env_devno = mmc_get_env_devno();
-
-		if (mmc_switch_part(mmc_env_devno,
-				    CONFIG_SYS_MMC_ENV_PART)) {
+	if (mmc_env_partno != mmc->part_num) {
+		if (mmc_switch_part(mmc_env_devno, mmc_env_partno)) {
 			puts("MMC partition switch failed\n");
 			return -1;
 		}
 	}
-#endif
 
 	return 0;
 }
 
 static void fini_mmc_for_env(struct mmc *mmc)
 {
-#ifdef CONFIG_SYS_MMC_ENV_PART
 	int mmc_env_devno = mmc_get_env_devno();
+	int mmc_env_partno = mmc_get_env_partno();
 
-	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
-		mmc_switch_part(mmc_env_devno,
-				mmc->part_num);
-#endif
+	if (mmc_env_partno != mmc->part_num)
+		mmc_switch_part(mmc_env_devno, mmc->part_num);
 }
 
 #ifdef CONFIG_CMD_SAVEENV

[U-Boot] [PATCH 2/2] env_mmc: default to 0 if CONFIG_SYS_MMC_ENV_DEV not defined

[Hector Palacios]

Since function mmc_get_env_devno is __weak and can be overridden by
board code, boards do not necessarily need to define
CONFIG_SYS_MMC_ENV_DEV. If the constant is not defined, return 0 by
default.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 common/env_mmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/env_mmc.c b/common/env_mmc.c
index d569b070e005..bd3bc1d50b15 100644
--- a/common/env_mmc.c
+++ b/common/env_mmc.c
@@ -63,7 +63,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
 
 __weak int mmc_get_env_devno(void)
 {
+#ifdef CONFIG_SYS_MMC_ENV_DEV
 	return CONFIG_SYS_MMC_ENV_DEV;
+#endif
+	return 0;
+}
 
 __weak int mmc_get_env_partno(void)
 {

[U-Boot] [PATCH 2/2] env_mmc: default to 0 if CONFIG_SYS_MMC_ENV_DEV not defined

[Otavio Salvador]

On Wed, Jan 15, 2014 at 8:53 AM, Hector Palacios
<hector.palacios@digi.com>wrote:

> Since function mmc_get_env_devno is __weak and can be overridden by
> board code, boards do not necessarily need to define
> CONFIG_SYS_MMC_ENV_DEV. If the constant is not defined, return 0 by
> default.
>
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>

Maybe use same default define, if not set?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 2/2] env_mmc: default to 0 if CONFIG_SYS_MMC_ENV_DEV not defined

[Stephen Warren]

On 01/15/2014 03:53 AM, Hector Palacios wrote:
> Since function mmc_get_env_devno is __weak and can be overridden by
> board code, boards do not necessarily need to define
> CONFIG_SYS_MMC_ENV_DEV. If the constant is not defined, return 0 by
> default.

Ah, I see that's a better justification for allowing the define not to
be set.


> diff --git a/common/env_mmc.c b/common/env_mmc.c

>  __weak int mmc_get_env_devno(void)
>  {
> +#ifdef CONFIG_SYS_MMC_ENV_DEV
>  	return CONFIG_SYS_MMC_ENV_DEV;
> +#endif

Same comment here; need a #else here, and #endif below.
> +	return 0;
> +}

Re: Hector's comments: Perhaps the following in
include/config_fallbacks.h would be appropriate?

#ifndef CONFIG_SYS_MMC_ENV_DEV
#define CONFIG_SYS_MMC_ENV_DEV 0
#endif

?

[U-Boot] [PATCH 2/2] env_mmc: default to 0 if CONFIG_SYS_MMC_ENV_DEV not defined

[Hector Palacios]

Hi Stephen,

On 01/15/2014 06:40 PM, Stephen Warren wrote:
> On 01/15/2014 03:53 AM, Hector Palacios wrote:
>> Since function mmc_get_env_devno is __weak and can be overridden by
>> board code, boards do not necessarily need to define
>> CONFIG_SYS_MMC_ENV_DEV. If the constant is not defined, return 0 by
>> default.
>
> Ah, I see that's a better justification for allowing the define not to
> be set.
>
>
>> diff --git a/common/env_mmc.c b/common/env_mmc.c
>
>>   __weak int mmc_get_env_devno(void)
>>   {
>> +#ifdef CONFIG_SYS_MMC_ENV_DEV
>>   	return CONFIG_SYS_MMC_ENV_DEV;
>> +#endif
>
> Same comment here; need a #else here, and #endif below.
>> +	return 0;
>> +}
>
> Re: Hector's comments: Perhaps the following in
> include/config_fallbacks.h would be appropriate?
>
> #ifndef CONFIG_SYS_MMC_ENV_DEV
> #define CONFIG_SYS_MMC_ENV_DEV 0
> #endif

This define is only used in common/env_mmc.c so I think the default definition should 
stay on that file, rather than in include/config_fallbacks.h.

Best regards,
--
Hector Palacios

[U-Boot] [PATCH 1/2] env_mmc: make board configurable the partition for the environment

[Otavio Salvador]

Hello Hector,

On Wed, Jan 15, 2014 at 8:53 AM, Hector Palacios
<hector.palacios@digi.com>wrote:

> This complements commit 9404a5fc7cb58 "env_mmc: allow environment to be
> in an eMMC partition" by allowing boards to accommodate the partition
> to use for the environment in different scenarios (similarly to what is
> done with the mmc dev number). Depending on the detected boot media,
> boards may decide to store the environment in a different partition.
>
> The __weak function also allows to remove some ifdefs from the code.
> If CONFIG_SYS_MMC_ENV_PART is not defined, partition 0 is assumed
> (default value for U-Boot when a partition is not provided).
>
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> CC: Stephen Warren <swarren@nvidia.com>
> CC: Andy Fleming <afleming@freescale.com>
> ---
>  common/env_mmc.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/common/env_mmc.c b/common/env_mmc.c
> index 78c2bc7a1f08..d569b070e005 100644
> --- a/common/env_mmc.c
> +++ b/common/env_mmc.c
> @@ -64,6 +64,13 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy,
> u32 *env_addr)
>  __weak int mmc_get_env_devno(void)
>  {
>         return CONFIG_SYS_MMC_ENV_DEV;
> +
> +__weak int mmc_get_env_partno(void)
> +{
> +#ifdef CONFIG_SYS_MMC_ENV_PART
> +       return CONFIG_SYS_MMC_ENV_PART;
> +#endif
> +       return 0;
>

Maybe:

#ifndef CONFIG_SYS_MMC_ENV_PART
#define CONFIG_SYS_MMC_ENV_PART 0
#endif

__weak int mmc_get_env_partno(void)
{
    return CONFIG_SYS_MMC_ENV_PART;
}

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/2] env_mmc: make board configurable the partition for the environment

[Hector Palacios]

Hello Otavio,

On 01/15/2014 12:09 PM, Otavio Salvador wrote:
> Hello Hector,
>
> On Wed, Jan 15, 2014 at 8:53 AM, Hector Palacios <hector.palacios@digi.com
> <mailto:hector.palacios@digi.com>> wrote:
>
>     This complements commit 9404a5fc7cb58 "env_mmc: allow environment to be
>     in an eMMC partition" by allowing boards to accommodate the partition
>     to use for the environment in different scenarios (similarly to what is
>     done with the mmc dev number). Depending on the detected boot media,
>     boards may decide to store the environment in a different partition.
>
>     The __weak function also allows to remove some ifdefs from the code.
>     If CONFIG_SYS_MMC_ENV_PART is not defined, partition 0 is assumed
>     (default value for U-Boot when a partition is not provided).
>
>     Signed-off-by: Hector Palacios <hector.palacios@digi.com
>     <mailto:hector.palacios@digi.com>>
>     CC: Stephen Warren <swarren at nvidia.com <mailto:swarren@nvidia.com>>
>     CC: Andy Fleming <afleming at freescale.com <mailto:afleming@freescale.com>>
>     ---
>       common/env_mmc.c | 27 +++++++++++++++------------
>       1 file changed, 15 insertions(+), 12 deletions(-)
>
>     diff --git a/common/env_mmc.c b/common/env_mmc.c
>     index 78c2bc7a1f08..d569b070e005 100644
>     --- a/common/env_mmc.c
>     +++ b/common/env_mmc.c
>     @@ -64,6 +64,13 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32
>     *env_addr)
>       __weak int mmc_get_env_devno(void)
>       {
>              return CONFIG_SYS_MMC_ENV_DEV;
>     +
>     +__weak int mmc_get_env_partno(void)
>     +{
>     +#ifdef CONFIG_SYS_MMC_ENV_PART
>     +       return CONFIG_SYS_MMC_ENV_PART;
>     +#endif
>     +       return 0;
>
>
> Maybe:
>
> #ifndef CONFIG_SYS_MMC_ENV_PART
> #define CONFIG_SYS_MMC_ENV_PART 0
> #endif
>
> __weak int mmc_get_env_partno(void)
> {
>      return CONFIG_SYS_MMC_ENV_PART;
> }

Much better. I'll do this for both patches. Thanks.

Best regards,
--
Hector Palacios

[U-Boot] [PATCH 1/2] env_mmc: make board configurable the partition for the environment

[Stephen Warren]

On 01/15/2014 03:53 AM, Hector Palacios wrote:
> This complements commit 9404a5fc7cb58 "env_mmc: allow environment to be
> in an eMMC partition" by allowing boards to accommodate the partition
> to use for the environment in different scenarios (similarly to what is
> done with the mmc dev number). Depending on the detected boot media,
> boards may decide to store the environment in a different partition.
> 
> The __weak function also allows to remove some ifdefs from the code.
> If CONFIG_SYS_MMC_ENV_PART is not defined, partition 0 is assumed
> (default value for U-Boot when a partition is not provided).

One advantage of the ifdefs is that it means zero code size bloat for
people who haven't defined CONFIG_SYS_MMC_ENV_PART. I'm not that
concerned about the issue for this amount of code, so don't take this as
a nak from me, but I'm sure others will be.

Also, it seems like a good idea to explicitly force people to think
about the partition ID they want the environment stored in, rather than
assume some potentially incorrect default for it?

> diff --git a/common/env_mmc.c b/common/env_mmc.c

> +__weak int mmc_get_env_partno(void)
> +{
> +#ifdef CONFIG_SYS_MMC_ENV_PART
> +	return CONFIG_SYS_MMC_ENV_PART;
> +#endif

With this implementation, you'd want that to be #else

> +	return 0;

... and the #endif here, so you don't end up with two return statements
back-to-back in the function in one case.

>  }