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

[Andy Fleming]

On Wed, Apr 28, 2010 at 2:52 AM, Terry Lv <r65388 at freescale.com> wrote:
> This patch is to save environment data to mmc card.
> It uses interfaces defined in generic mmc.
>
> Signed-off-by: Terry Lv <r65388 at freescale.com>


> diff --git a/common/env_mmc.c b/common/env_mmc.c
> new file mode 100644
> index 0000000..202370e
> --- /dev/null
> +++ b/common/env_mmc.c
> @@ -0,0 +1,160 @@
> +/*
> + * (C) Copyright 2008-2010 Freescale Semiconductor, Inc.
> +
> + * (C) Copyright 2000-2006
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> + *
> + * (C) Copyright 2001 Sysgo Real-Time Solutions, GmbH <www.elinos.com>
> + * Andreas Heppel <aheppel at sysgo.de>

Are these non-Freescale copyrights here for a reason, or were they
just copied from somewhere?

> +#ifdef ENV_IS_EMBEDDED
> +extern uchar environment[];
> +env_t *env_ptr = (env_t *)(&environment[0]);
> +#else /* ! ENV_IS_EMBEDDED */
> +env_t *env_ptr;
> +#endif /* ENV_IS_EMBEDDED */

You should probably initialize env_ptr to NULL, here.

> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define BLOCK_ALIGN(x) ((x & (0x200 - 1)) \
> +                        ? ((x >> 9) + 1) : (x >> 9))

No need to reinvent the wheel, here.  This is the same as:

ALIGN(x, 0x200);

See: include/common.h

Also, are you sure the block size is 0x200?  That's a very common
block size, but the MMC spec allows a whole bunch of alternate ones.
You should use the actual value of mmc->read_bl_len and
mmc->write_bl_len;


> +int env_init(void)
> +{
> +       /* use default */
> +       gd->env_addr = (ulong)&default_environment[0];
> +       gd->env_valid = 1;
> +
> +       return 0;
> +}
> +
> +inline int init_mmc_for_env(struct mmc *mmc)

Why is this inlined?  Don't inline a function if there's no good reason.

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

> +inline int write_env(struct mmc *mmc, unsigned long size,
> +                                               unsigned long offset, const void *buffer)
> +{
> +       uint blk_start = 0, blk_cnt = 0, n = 0;

Don't initialize variables to zero right before assigning them.

> +
> +       blk_start = BLOCK_ALIGN(offset);
> +       blk_cnt   = BLOCK_ALIGN(size);
> +
> +       n = mmc->block_dev.block_write(CONFIG_SYS_MMC_ENV_DEV, blk_start , blk_cnt, (u_char *)buffer);
> +
> +       return (n == blk_cnt) ? 0 : -1;
> +}
> +
> +int saveenv(void)
> +{
> +       struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV);
> +
> +       if (init_mmc_for_env(mmc))
> +               return 1;
> +
> +       printf("Writing to MMC(%d)... ", CONFIG_SYS_MMC_ENV_DEV);
> +       if (write_env(mmc, CONFIG_ENV_SIZE, \

You don't need that "\".  C does that for you automatically.



> +inline int read_env(struct mmc *mmc, unsigned long size,
> +                                               unsigned long offset, const void *buffer)
> +{
> +       uint blk_start = 0, blk_cnt = 0, n = 0;

Again, don't bother zeroing out values like this.

> +
> +       blk_start = BLOCK_ALIGN(offset);
> +       blk_cnt   = BLOCK_ALIGN(size);
> +
> +       n = mmc->block_dev.block_read(CONFIG_SYS_MMC_ENV_DEV, blk_start, blk_cnt, (uchar *)buffer);
> +
> +       return (n == blk_cnt) ? 0 : -1;
> +}
> +
> +void env_relocate_spec(void)
> +{
> +#if !defined(ENV_IS_EMBEDDED)
> +       struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV);
> +
> +       if (init_mmc_for_env(mmc))
> +               return;
> +
> +       if (read_env(mmc, CONFIG_ENV_SIZE, CONFIG_ENV_OFFSET, env_ptr))
> +               return use_default();


This is broken.  env_ptr hasn't been initialized to a value, yet!
This writes to random memory!  env_ptr needs to be malloc'ed.


Andy