[U-Boot] [PATCH] Save environment data to mmc.

[Wolfgang Denk]

Dear Terry Lv,

In message <12574070132761-git-send-email-r65388 at freescale.com> you wrote:
> This patch is to save environment data to mmc card.
> It uses interfaces defined in generic mmc.
...
> +	if (!crc1_ok && !crc2_ok)
> +		gd->env_valid = 0;
> +	else if (crc1_ok && !crc2_ok)
> +		gd->env_valid = 1;
> +	else if (!crc1_ok && crc2_ok)
> +		gd->env_valid = 2;
> +	else {
> +		/* both ok - check serial */
> +		if (tmp_env1->flags == 255 && tmp_env2->flags == 0)
> +			gd->env_valid = 2;
> +		else if (tmp_env2->flags == 255 && tmp_env1->flags == 0)
> +			gd->env_valid = 1;
> +		else if (tmp_env1->flags > tmp_env2->flags)
> +			gd->env_valid = 1;
> +		else if (tmp_env2->flags > tmp_env1->flags)
> +			gd->env_valid = 2;
> +		else /* flags are equal - almost impossible */
> +			gd->env_valid = 1;
> +	}

Please check if this logic is really working on MMC - do you really "erase"
(i. e. fill with 0xFF bytes) the data blocks when "erasing"?

...
> +	if (!mmc) {
> +		puts("No MMC card found\n");
> +		return;
> +	}
> +
> +	if (mmc_init(mmc)) {
> +		puts("MMC init failed\n");
> +		return 1;
> +	}

This code repeats a number of times. Make it a (inline?) function?

> +	tmp_env1 = (env_t *)malloc(CONFIG_ENV_SIZE);
> +	if (!tmp_env1) {
> +		puts("Not enough memory!\n");
> +		goto use_default;
> +	}

Please print where we are, and how many bytes you attempted to
allocate.

> +	if (!tmp_env2) {
> +		puts("Not enough memory!\n");
> +		goto use_default;
> +	}

Ditto . Again - repeated code!

> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -94,6 +94,24 @@
>  # endif
>  #endif /* CONFIG_ENV_IS_IN_MG_DISK */
>  
> +#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 */

Does this make sense on MMC?

Would it not be better to define block numbers instead?

> +#ifdef CONFIG_GENERIC_MMC
> +        puts ("MMC:   ");
> +        mmc_initialize (gd->bd);
> +#endif

Indentation by TABs, please.

Are you sure movin the MMC initialization here has no negative impact
on any of the existing boards?

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

Ditto, for both remarks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The only way you could make a happy  marriage  is  by  cuttin'  their
heads  off  as  soon  as  they say `I do', yes? You can't make happi-
ness...                           - Terry Pratchett, _Witches Abroad_