Hi, Terry, > -----Original Message----- > From: u-boot-boun...@lists.denx.de > [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Lv Terry-R65388 > Sent: 2010年3月4日 11:01 > To: Stefano Babic > Cc: u-boot@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:sba...@denx.de] > Sent: 2010年3月3日 1:03 > To: Lv Terry-R65388 > Cc: u-boot@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: > off...@denx.de > ===================================================================== > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
