[U-Boot] [PATCH 1/3] x86: ivybridge: Fix saving mrc cache and enable it
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; + } ret = write_seeds_to_cmos(pei_data); if (ret) debug("Failed to write seeds to CMOS: %d\n", ret);
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 +}
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
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!
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
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
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!
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.
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
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
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@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
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@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
participants (2)
-
Bin Meng -
Simon Glass