Hi Babic, Pls check my comments with keyword [Ty].
Thanks for reviewing the patch. Thanks~~ Yours Terry -----Original Message----- From: Stefano Babic [mailto:sba...@denx.de] Sent: 2010年4月29日 22:34 To: Lv Terry-R65388 Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] Save environment data to mmc. Terry Lv wrote: > This patch is to save environment data to mmc card. > It uses interfaces defined in generic mmc. Hi Terry, > > Signed-off-by: Terry Lv <r65...@freescale.com> > --- > arch/arm/lib/board.c | 10 ++-- > arch/powerpc/lib/board.c | 12 ++-- > common/Makefile | 1 + > common/cmd_nvedit.c | 1 + > common/env_mmc.c | 154 > ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 167 insertions(+), 11 deletions(-) create mode > 100644 common/env_mmc.c Could you set a version of your patch (something like [PATCH V*] in the subject, so it is easier to track changes ? This is the third version, but it is difficult to get it without searching in archive. [Ty]: OK, I will do that, thanks, :-) > > diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index > f5660a9..f62e0eb 100644 > --- a/arch/arm/lib/board.c > +++ b/arch/arm/lib/board.c > @@ -347,6 +347,11 @@ void start_armboot (void) > dataflash_print_info(); > #endif > > +#ifdef CONFIG_GENERIC_MMC > + puts ("MMC: "); > + mmc_initialize (gd->bd); > +#endif > + > /* initialize environment */ > env_relocate (); > > @@ -419,11 +424,6 @@ extern void davinci_eth_set_mac_addr (const u_int8_t > *addr); > board_late_init (); > #endif > > -#ifdef CONFIG_GENERIC_MMC > - puts ("MMC: "); > - mmc_initialize (gd->bd); > -#endif > - > #ifdef CONFIG_BITBANGMII > bb_miiphy_init(); > #endif Because it is required to move the initialization of the mmc before env_relocate(), we need probably to advise that there are some consequences. If someone implements the mmc support in board_late_init() (setting a pin multiplexer or something like that, for example), it does not work. I think we should at least write a comment to advise that the mmc/sd controller should work after board_init() is called. However, after a quick check in the arm boards, I have not found a board that is initializing the mmc controller in board_late_init(). Not sure for powerpc. [Ty]: I have moved mmc initialization in boards that uses generic mmc. I will add the comment, thanks. > diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index > eb89e9e..78f75fb 100644 > --- a/common/cmd_nvedit.c > +++ b/common/cmd_nvedit.c > @@ -59,6 +59,7 @@ DECLARE_GLOBAL_DATA_PTR; > !defined(CONFIG_ENV_IS_IN_FLASH) && \ > !defined(CONFIG_ENV_IS_IN_DATAFLASH) && \ > !defined(CONFIG_ENV_IS_IN_MG_DISK) && \ > + !defined(CONFIG_ENV_IS_IN_MMC) && \ > !defined(CONFIG_ENV_IS_IN_NAND) && \ > !defined(CONFIG_ENV_IS_IN_NVRAM) && \ > !defined(CONFIG_ENV_IS_IN_ONENAND) && \ From the first version you remove the changes in the error string: # error Define one of CONFIG_ENV_IS_IN_{EEPROM|FLASH|DATAFLASH|ONENAND|\ -SPI_FLASH|MG_DISK|NVRAM|NOWHERE} +SPI_FLASH|MG_DISK|NVRAM|MMC|NOWHERE} I know it is only an error message, but the change seems correct. I have not found a comment against it ;) [Ty]: I will add this. > +#else /* ! ENV_IS_EMBEDDED */ > +env_t *env_ptr; > +#endif /* ENV_IS_EMBEDDED */ You missed Andy's comment. You have to initialize env_ptr. [Ty]: This is a global variable. I think that compiler will set it to zero for me. Correct me if I'm wrong. Anyway, I will set it to NULL. > + if (read_env(mmc, CONFIG_ENV_SIZE, CONFIG_ENV_OFFSET, env_ptr)) > + return use_default(); This is still broken, as found by Andy. I cannot find a line where env_ptr is initialized and then you pass the pointer to the mmc read function. [Ty]: env_ptr is defined in env_mmc.c and the value is assigned in env_common.c. Correct me if I'm wrong. Best 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