Re: [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  wrote:
> Hi Bin,
>
> On 18 October 2015 at 20:44, Bin Meng  wrote:
>> Hi Simon,
>>
>> On Mon, Oct 19, 2015 at 4:27 AM, Simon Glass  wrote:
>>> Hi Bin,
>>>
>>> On 12 October 2015 at 02:30, Bin Meng  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 
 ---

  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(s...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  wrote:
> Hi Simon,
>
> On Mon, Oct 19, 2015 at 4:27 AM, Simon Glass  wrote:
>> Hi Bin,
>>
>> On 12 October 2015 at 02:30, Bin Meng  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 
>>> ---
>>>
>>>  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(s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  wrote:
> Hi Bin,
>
> On 12 October 2015 at 02:30, Bin Meng  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 
>> ---
>>
>>  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(s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  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 
> ---
>
>  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(s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

 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(s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot