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: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
