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@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: office@denx.de =====================================================================