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
