Dear Terry Lv,
In message 12574070132761-git-send-email-r65388@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