From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Mon, 19 Oct 2015 10:44:27 +0800 Subject: [U-Boot] [PATCH 1/3] x86: ivybridge: Fix saving mrc cache and enable it In-Reply-To: References: <1444638643-13055-1-git-send-email-bmeng.cn@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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(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