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

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

[Bin Meng]

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 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;
+		}
 		ret = write_seeds_to_cmos(pei_data);
 		if (ret)
 			debug("Failed to write seeds to CMOS: %d\n", ret);
-- 
1.8.2.1

[U-Boot] [PATCH 2/3] x86: quark: Implement mrc cache

[Bin Meng]

Using existing mrccache library to implement mrc cache support
for Intel Quark.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 arch/x86/cpu/quark/dram.c  | 52 +++++++++++++++++++++++++++++++++++++++-------
 arch/x86/cpu/quark/quark.c | 19 +++++++++++++++++
 2 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/arch/x86/cpu/quark/dram.c b/arch/x86/cpu/quark/dram.c
index 1b89376..40c830a 100644
--- a/arch/x86/cpu/quark/dram.c
+++ b/arch/x86/cpu/quark/dram.c
@@ -7,6 +7,8 @@
 #include <common.h>
 #include <errno.h>
 #include <fdtdec.h>
+#include <malloc.h>
+#include <asm/mrccache.h>
 #include <asm/mtrr.h>
 #include <asm/post.h>
 #include <asm/arch/mrc.h>
@@ -15,6 +17,29 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static __maybe_unused int prepare_mrc_cache(struct mrc_params *mrc_params)
+{
+	struct mrc_data_container *cache;
+	struct mrc_region entry;
+	int ret;
+
+	ret = mrccache_get_region(NULL, &entry);
+	if (ret)
+		return ret;
+
+	cache = mrccache_find_current(&entry);
+	if (!cache)
+		return -ENOENT;
+
+	debug("%s: mrc cache at %p, size %x checksum %04x\n", __func__,
+	      cache->data, cache->data_size, cache->checksum);
+
+	/* copy mrc cache to the mrc_params */
+	memcpy(&mrc_params->timings, cache->data, cache->data_size);
+
+	return 0;
+}
+
 static int mrc_configure_params(struct mrc_params *mrc_params)
 {
 	const void *blob = gd->fdt_blob;
@@ -27,14 +52,15 @@ static int mrc_configure_params(struct mrc_params *mrc_params)
 		return -EINVAL;
 	}
 
-	/*
-	 * TODO:
-	 *
-	 * We need support fast boot (MRC cache) in the future.
-	 *
-	 * Set boot mode to cold boot for now
-	 */
+#ifdef CONFIG_ENABLE_MRC_CACHE
+	mrc_params->boot_mode = prepare_mrc_cache(mrc_params);
+	if (mrc_params->boot_mode)
+		mrc_params->boot_mode = BM_COLD;
+	else
+		mrc_params->boot_mode = BM_FAST;
+#else
 	mrc_params->boot_mode = BM_COLD;
+#endif
 
 	/*
 	 * TODO:
@@ -98,6 +124,9 @@ static int mrc_configure_params(struct mrc_params *mrc_params)
 int dram_init(void)
 {
 	struct mrc_params mrc_params;
+#ifdef CONFIG_ENABLE_MRC_CACHE
+	char *cache;
+#endif
 	int ret;
 
 	memset(&mrc_params, 0, sizeof(struct mrc_params));
@@ -121,6 +150,15 @@ int dram_init(void)
 		       (~(gd->ram_size - 1)) | MTRR_PHYS_MASK_VALID);
 	enable_caches();
 
+#ifdef CONFIG_ENABLE_MRC_CACHE
+	cache = malloc(sizeof(struct mrc_timings));
+	if (cache) {
+		memcpy(cache, &mrc_params.timings, sizeof(struct mrc_timings));
+		gd->arch.mrc_output = cache;
+		gd->arch.mrc_output_len = sizeof(struct mrc_timings);
+	}
+#endif
+
 	return 0;
 }
 
diff --git a/arch/x86/cpu/quark/quark.c b/arch/x86/cpu/quark/quark.c
index 77d644a..f737e19 100644
--- a/arch/x86/cpu/quark/quark.c
+++ b/arch/x86/cpu/quark/quark.c
@@ -8,6 +8,7 @@
 #include <mmc.h>
 #include <asm/io.h>
 #include <asm/irq.h>
+#include <asm/mrccache.h>
 #include <asm/mtrr.h>
 #include <asm/pci.h>
 #include <asm/post.h>
@@ -368,6 +369,15 @@ void cpu_irq_init(void)
 
 int arch_misc_init(void)
 {
+#ifdef CONFIG_ENABLE_MRC_CACHE
+	/*
+	 * We intend not to check any return value here, as even MRC cache
+	 * is not saved successfully, it is not a severe error that will
+	 * prevent system from continuing to boot.
+	 */
+	mrccache_save();
+#endif
+
 	return pirq_init();
 }
 
@@ -390,3 +400,12 @@ void board_final_cleanup(void)
 
 	return;
 }
+
+int reserve_arch(void)
+{
+#ifdef CONFIG_ENABLE_MRC_CACHE
+	return mrccache_reserve();
+#else
+	return 0;
+#endif
+}
-- 
1.8.2.1

[U-Boot] [PATCH 3/3] x86: galileo: Enable mrc cache

[Bin Meng]

Now that we have added MRC cache on quark support codes,
enable it on Intel Galileo board.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 arch/x86/dts/galileo.dts  | 4 ++++
 configs/galileo_defconfig | 1 +
 2 files changed, 5 insertions(+)

diff --git a/arch/x86/dts/galileo.dts b/arch/x86/dts/galileo.dts
index a4e1676..b49b1f5 100644
--- a/arch/x86/dts/galileo.dts
+++ b/arch/x86/dts/galileo.dts
@@ -132,6 +132,10 @@
 			reg = <0>;
 			compatible = "winbond,w25q64", "spi-flash";
 			memory-map = <0xff800000 0x00800000>;
+			rw-mrc-cache {
+				label = "rw-mrc-cache";
+				reg = <0x00010000 0x00010000>;
+			};
 		};
 	};
 
diff --git a/configs/galileo_defconfig b/configs/galileo_defconfig
index d1808a5..1e1ce95 100644
--- a/configs/galileo_defconfig
+++ b/configs/galileo_defconfig
@@ -2,6 +2,7 @@ CONFIG_X86=y
 CONFIG_VENDOR_INTEL=y
 CONFIG_DEFAULT_DEVICE_TREE="galileo"
 CONFIG_TARGET_GALILEO=y
+CONFIG_ENABLE_MRC_CACHE=y
 CONFIG_GENERATE_PIRQ_TABLE=y
 # CONFIG_CMD_IMLS is not set
 # CONFIG_CMD_FLASH is not set
-- 
1.8.2.1

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

[U-Boot] [PATCH 2/3] x86: quark: Implement mrc cache

[Simon Glass]

On 12 October 2015 at 02:30, Bin Meng <bmeng.cn@gmail.com> wrote:
> Using existing mrccache library to implement mrc cache support
> for Intel Quark.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/cpu/quark/dram.c  | 52 +++++++++++++++++++++++++++++++++++++++-------
>  arch/x86/cpu/quark/quark.c | 19 +++++++++++++++++
>  2 files changed, 64 insertions(+), 7 deletions(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/3] x86: galileo: Enable mrc cache

[Simon Glass]

On 12 October 2015 at 02:30, Bin Meng <bmeng.cn@gmail.com> wrote:
> Now that we have added MRC cache on quark support codes,
> enable it on Intel Galileo board.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/dts/galileo.dts  | 4 ++++
>  configs/galileo_defconfig | 1 +
>  2 files changed, 5 insertions(+)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/3] x86: quark: Implement mrc cache

[Simon Glass]

On 18 October 2015 at 14:27, Simon Glass <sjg@chromium.org> wrote:
> On 12 October 2015 at 02:30, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Using existing mrccache library to implement mrc cache support
>> for Intel Quark.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  arch/x86/cpu/quark/dram.c  | 52 +++++++++++++++++++++++++++++++++++++++-------
>>  arch/x86/cpu/quark/quark.c | 19 +++++++++++++++++
>>  2 files changed, 64 insertions(+), 7 deletions(-)
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-x86, thanks!

[U-Boot] [PATCH 3/3] x86: galileo: Enable mrc cache

[Simon Glass]

On 18 October 2015 at 14:27, Simon Glass <sjg@chromium.org> wrote:
> On 12 October 2015 at 02:30, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Now that we have added MRC cache on quark support codes,
>> enable it on Intel Galileo board.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  arch/x86/dts/galileo.dts  | 4 ++++
>>  configs/galileo_defconfig | 1 +
>>  2 files changed, 5 insertions(+)
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-x86, thanks!

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

[Bin Meng]

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

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

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

[Simon Glass]

Hi Bin,

On 18 October 2015 at 20:44, Bin Meng <bmeng.cn@gmail.com> wrote:
> 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 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.
>
> 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 it points into the part of CAR that is reserved for use by the MRC.

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

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

[Bin Meng]

Hi Simon,

On Mon, Oct 19, 2015 at 10:51 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 18 October 2015 at 20:44, Bin Meng <bmeng.cn@gmail.com> wrote:
>> 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 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.
>>
>> 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 it points into the part of CAR that is reserved for use by the MRC.
>

Thanks, this explains why malloc() is not needed, since it is reserved
and not the same stack that U-Boot uses in the CAR.

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