[U-Boot] [PATCH 1/3] x86: ivybridge: Fix saving mrc cache and enable it

[Simon Glass]

Hi Bin,

On 12 October 2015 at 02:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> Currently sdram_initialise() saves pei_data->mrc_output directly to
> gd->arch.mrc_output. This is incorrect as pei_data->mrc_output points
> to an address on the stack whose content is no longer valid when we
> call mrccache_reserve(). To fix this, save it on the heap instead.
>
> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> ---
>
>  arch/x86/cpu/ivybridge/sdram.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/cpu/ivybridge/sdram.c b/arch/x86/cpu/ivybridge/sdram.c
> index fc66a3c..f3d97ca 100644
> --- a/arch/x86/cpu/ivybridge/sdram.c
> +++ b/arch/x86/cpu/ivybridge/sdram.c
> @@ -151,14 +151,8 @@ static int prepare_mrc_cache(struct pei_data *pei_data)
>         if (!mrc_cache)
>                 return -ENOENT;
>
> -       /*
> -        * TODO(sjg at chromium.org): Skip this for now as it causes boot
> -        * problems
> -        */
> -       if (0) {
> -               pei_data->mrc_input = mrc_cache->data;
> -               pei_data->mrc_input_len = mrc_cache->data_size;
> -       }
> +       pei_data->mrc_input = mrc_cache->data;
> +       pei_data->mrc_input_len = mrc_cache->data_size;
>         debug("%s: at %p, size %x checksum %04x\n", __func__,
>               pei_data->mrc_input, pei_data->mrc_input_len,
>               mrc_cache->checksum);
> @@ -289,6 +283,7 @@ int sdram_initialise(struct pei_data *pei_data)
>         unsigned version;
>         const char *data;
>         uint16_t done;
> +       char *cache;
>         int ret;
>
>         report_platform_info();
> @@ -386,8 +381,13 @@ int sdram_initialise(struct pei_data *pei_data)
>                  * This will be copied to SDRAM in reserve_arch(), then written
>                  * to SPI flash in mrccache_save()
>                  */
> -               gd->arch.mrc_output = (char *)pei_data->mrc_output;
> -               gd->arch.mrc_output_len = pei_data->mrc_output_len;
> +               cache = malloc(pei_data->mrc_output_len);
> +               if (cache) {
> +                       memcpy(cache, pei_data->mrc_output,
> +                              pei_data->mrc_output_len);
> +                       gd->arch.mrc_output = cache;
> +                       gd->arch.mrc_output_len = pei_data->mrc_output_len;
> +               }

This isn't really any better than what is there. The malloc() region
is in CAR memory, just a different part of it. The function
reserve_arch() copies it to SDRAM.

I think with FSP this does not work but for ivybridge it seems OK.

I'll resend your patch with this part removed. Your comments spurred
me to take another look at why MRC was broken on ivybridge, and I
found that car_uninit() was wrong. I'll send a series to fix it.

>                 ret = write_seeds_to_cmos(pei_data);
>                 if (ret)
>                         debug("Failed to write seeds to CMOS: %d\n", ret);
> --
> 1.8.2.1
>

Regards,
Simon