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

[Lv Terry-R65388]

Hi Babic,

	Thanks for reviewing the patch.

	I will send out a new patch to fix the problems you point out.

	For setting block numbers for MMC offset, I just don't want env mmc to be different. I want users to use it as similiar as other env devices.

	For the initalization for ARM and PPC, It will be added for all architectures.

	Thanks a lot ~~

Yours
Terry

-----Original Message-----
From: Stefano Babic [mailto:sbabic at denx.de] 
Sent: 2010年3月3日 1:03
To: Lv Terry-R65388
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH] Save environment data to mmc.

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 =====================================================================