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

Hi Simon,

On Mon, Oct 19, 2015 at 4:27 AM, Simon Glass sjg@chromium.org wrote:

Hi Bin,

On 12 October 2015 at 02:30, Bin Meng bmeng.cn@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@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@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.

So where does this pei_data->mrc_input point to? Is it some place that is malloced by the MRC itself and in a place that does not get overwritten?

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

Regards, Bin