[U-Boot] [PATCH] Save environment data to mmc.

[Stefano Babic <sbabic@denx.de>]

Terry Lv wrote:

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

> +#include <linux/stddef.h>
> +#include <malloc.h>
> +#include <mmc.h>
> +
> +#if defined(CONFIG_CMD_ENV) && defined(CONFIG_CMD_MMC)

This seems not correct. If not explicitely set, we get a compiler error.
Assuming you has taken this check from env_nand.c, this should be:

#if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_MMC)

> +#define CMD_SAVEENV
> +#elif defined(CONFIG_ENV_OFFSET_REDUND)
> +#error Cannot use CONFIG_ENV_OFFSET_REDUND without CONFIG_CMD_ENV & CONFIG_CMD_MMC

Line too long.

> +#endif
> +
> +#if defined(CONFIG_ENV_SIZE_REDUND) && (CONFIG_ENV_SIZE_REDUND < CONFIG_ENV_SIZE)

Ditto.


> +
> +#ifdef CMD_SAVEENV
> +
> +inline int write_env(struct mmc *mmc, unsigned long size,
> +						unsigned long offset, const void *buffer)

Line too long.

> +{
> +	uint blk_start = 0, blk_cnt = 0, n = 0;
> +
> +	blk_start = (offset % 512) ? ((offset / 512) + 1) : (offset / 512);
> +	blk_cnt = (size % 512) ? ((size / 512) + 1) : (size / 512);

The alignment to block size is repeated here and in the read function.
Should not better to set a macro (something like BLOCK_ALIGN) providing
the required alignment ?

> +int saveenv(void)
> +{
> +	struct mmc *mmc = find_mmc_device(0);

Why is the MMC device hard-coded ? At least should be configurable with
a CONFIG_ option. There are boards with more than one MMC controller and
you constraint to use always the first one.

> +	blk_start = (offset % 512) ? ((offset / 512) + 1) : (offset / 512);

Already said, a macro is much more readable to perform alignment.

> diff --git a/include/environment.h b/include/environment.h

> +#if defined(CONFIG_ENV_IS_IN_MMC)
> +# ifndef CONFIG_ENV_OFFSET
> +#  error "Need to define CONFIG_ENV_OFFSET when using CONFIG_ENV_IS_IN_MMC"
> +# endif
> +# ifndef CONFIG_ENV_ADDR
> +#  define CONFIG_ENV_ADDR	(CONFIG_ENV_OFFSET)
> +# endif
> +# ifndef CONFIG_ENV_OFFSET
> +#  define CONFIG_ENV_OFFSET (CONFIG_ENV_ADDR)
> +# endif
> +# ifdef CONFIG_ENV_OFFSET_REDUND
> +#  define CONFIG_SYS_REDUNDAND_ENVIRONMENT
> +# endif
> +# ifdef CONFIG_ENV_IS_EMBEDDED
> +#  define ENV_IS_EMBEDDED	1
> +# endif
> +#endif /* CONFIG_ENV_IS_IN_MMC */

You missed Wolfgang's comment. I think also that there is no reason to
set offset for the MMC and block numbers makes more sense.

> +
>  /* Embedded env is only supported for some flash types */
>  #ifdef CONFIG_ENV_IS_EMBEDDED
>  # if !defined(CONFIG_ENV_IS_IN_FLASH) && \
> diff --git a/lib_arm/board.c b/lib_arm/board.c
> index 5e3d7f6..f846d0d 100644

> +#ifdef CONFIG_GENERIC_MMC
> +	puts ("MMC:   ");
> +	mmc_initialize (gd->bd);
> +#endif

> diff --git a/lib_ppc/board.c b/lib_ppc/board.c
> index 765f97a..9b3f84c 100644
> --- a/lib_ppc/board.c
> +++ b/lib_ppc/board.c
> @@ -776,6 +776,12 @@ void board_init_r (gd_t *id, ulong dest_addr)
>  	nand_init();		/* go init the NAND */
>  #endif
>  
> +#ifdef CONFIG_GENERIC_MMC
> +	WATCHDOG_RESET ();
> +	puts ("MMC:  ");
> +	mmc_initialize (bd);
> +#endif

I am quite confused. You add the initialization only for ARM and PPC.
What about the other architectures ?

I tested your patch on mx51evk, environment is correctly read/written on
the SD situated on the back.

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================