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

[Liu Hui-R64343]

Hi, Terry,

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Lv Terry-R65388
> Sent: 2010年3月4日 11:01
> To: Stefano Babic
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] Save environment data to mmc.
> 
> 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 ‾‾

Had better not put comments on the top of the email, need follow the community style. 

> 
> 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 
> =====================================================================
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>