[PATCH 1/6] bootstage: Fix 'stacked' typo
This should be 'stashed'. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/bootstage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/bootstage.h b/include/bootstage.h index f507271375..00c85fb86a 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -338,7 +338,7 @@ int bootstage_stash(void *base, int size); * @param base Base address of memory buffer * @param size Size of memory buffer (-1 if unknown) * @return 0 if unstashed ok, -ENOENT if bootstage info not found, -ENOSPC if - * there is not space for read the stacked data, or other error if + * there is not space for read the stashed data, or other error if * something else went wrong */ int bootstage_unstash(const void *base, int size);
At present if an optional Kconfig value needs to be used it must be bracketed by #ifdef. For example, with this Kconfig setup:
config WIBBLE bool "Support wibbles, the world needs more wibbles"
config WIBBLE_ADDR hex "Address of the wibble" depends on WIBBLE
then the following code must be used:
#ifdef CONFIG_WIBBLE static void handle_wibble(void) { int val = CONFIG_WIBBLE_ADDR;
... } #endif
static void init_machine() { ... #ifdef CONFIG_WIBBLE handle_wibble(); #endif }
Add a new IF_ENABLED_INT() to help with this. So now it is possible to write, without #ifdefs:
static void handle_wibble(void) { int val = IF_ENABLED_INT(CONFIG_WIBBLE, CONFIG_WIBBLE_ADDR);
... }
static void init_machine() { ... if (IS_ENABLED(CONFIG_WIBBLE)) handle_wibble(); }
The value will be 0 if CONFIG_WIBBLE is not defined, and CONFIG_WIBBLE_ADDR if it is. This allows us to reduce the use of #ifdef in the code, ensuring that the compiler still checks the code even if it is not ultimately used for a particular build.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/linux/kconfig.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index 3a2da738c4..86cd266540 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -79,6 +79,18 @@ */ #define CONFIG_VAL(option) config_val(option)
+/* This use a similar mechanism to config_enabled() above */ +#define config_opt_enabled(cfg, opt_cfg) _config_opt_enabled(cfg, opt_cfg) +#define _config_opt_enabled(cfg_val, opt_value) \ + __config_opt_enabled(__ARG_PLACEHOLDER_##cfg_val, opt_value) +#define __config_opt_enabled(arg1_or_junk, arg2) \ + ___config_opt_enabled(arg1_or_junk arg2, 0) +#define ___config_opt_enabled(__ignored, val, ...) val + +/* Evaluates to 0 if option is not defined, int_option if it is defined */ +#define IF_ENABLED_INT(option, int_option) \ + config_opt_enabled(option, int_option) + /* * CONFIG_IS_ENABLED(FOO) evaluates to * 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y' or 'm',
On Fri, May 22, 2020 at 11:02 AM Simon Glass sjg@chromium.org wrote:
At present if an optional Kconfig value needs to be used it must be bracketed by #ifdef. For example, with this Kconfig setup:
config WIBBLE bool "Support wibbles, the world needs more wibbles"
config WIBBLE_ADDR hex "Address of the wibble" depends on WIBBLE
I am not sure if this is a good idea.
If you want to always use CONFIG_WIBBLE_ADDR, you can get rid of 'depends on WIBBLE'.
then the following code must be used:
#ifdef CONFIG_WIBBLE static void handle_wibble(void) { int val = CONFIG_WIBBLE_ADDR;
...} #endif
static void init_machine() { ... #ifdef CONFIG_WIBBLE handle_wibble(); #endif }
Add a new IF_ENABLED_INT() to help with this. So now it is possible to write, without #ifdefs:
static void handle_wibble(void) { int val = IF_ENABLED_INT(CONFIG_WIBBLE, CONFIG_WIBBLE_ADDR);
...}
static void init_machine() { ... if (IS_ENABLED(CONFIG_WIBBLE)) handle_wibble(); }
The value will be 0 if CONFIG_WIBBLE is not defined, and CONFIG_WIBBLE_ADDR if it is. This allows us to reduce the use of #ifdef in the code, ensuring that the compiler still checks the code even if it is not ultimately used for a particular build.
Signed-off-by: Simon Glass sjg@chromium.org
include/linux/kconfig.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index 3a2da738c4..86cd266540 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -79,6 +79,18 @@ */ #define CONFIG_VAL(option) config_val(option)
+/* This use a similar mechanism to config_enabled() above */ +#define config_opt_enabled(cfg, opt_cfg) _config_opt_enabled(cfg, opt_cfg) +#define _config_opt_enabled(cfg_val, opt_value) \
__config_opt_enabled(__ARG_PLACEHOLDER_##cfg_val, opt_value)+#define __config_opt_enabled(arg1_or_junk, arg2) \
___config_opt_enabled(arg1_or_junk arg2, 0)+#define ___config_opt_enabled(__ignored, val, ...) val
+/* Evaluates to 0 if option is not defined, int_option if it is defined */ +#define IF_ENABLED_INT(option, int_option) \
config_opt_enabled(option, int_option)/*
- CONFIG_IS_ENABLED(FOO) evaluates to
- 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y' or 'm',
-- 2.27.0.rc0.183.gde8f92d652-goog
On 5/22/20 4:17 AM, Masahiro Yamada wrote:
On Fri, May 22, 2020 at 11:02 AM Simon Glass sjg@chromium.org wrote:
At present if an optional Kconfig value needs to be used it must be bracketed by #ifdef. For example, with this Kconfig setup:
config WIBBLE bool "Support wibbles, the world needs more wibbles"
config WIBBLE_ADDR hex "Address of the wibble" depends on WIBBLE
I am not sure if this is a good idea.
If you want to always use CONFIG_WIBBLE_ADDR, you can get rid of 'depends on WIBBLE'.
Hello Simon,
what is the effect on the code size if you eliminate the #ifdefs?
Isn't this move growing the size of the U-Boot binary?
Best regards
Heinrich
then the following code must be used:
#ifdef CONFIG_WIBBLE static void handle_wibble(void) { int val = CONFIG_WIBBLE_ADDR;
...} #endif
static void init_machine() { ... #ifdef CONFIG_WIBBLE handle_wibble(); #endif }
Add a new IF_ENABLED_INT() to help with this. So now it is possible to write, without #ifdefs:
static void handle_wibble(void) { int val = IF_ENABLED_INT(CONFIG_WIBBLE, CONFIG_WIBBLE_ADDR);
...}
static void init_machine() { ... if (IS_ENABLED(CONFIG_WIBBLE)) handle_wibble(); }
The value will be 0 if CONFIG_WIBBLE is not defined, and CONFIG_WIBBLE_ADDR if it is. This allows us to reduce the use of #ifdef in the code, ensuring that the compiler still checks the code even if it is not ultimately used for a particular build.
Signed-off-by: Simon Glass sjg@chromium.org
include/linux/kconfig.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index 3a2da738c4..86cd266540 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -79,6 +79,18 @@ */ #define CONFIG_VAL(option) config_val(option)
+/* This use a similar mechanism to config_enabled() above */ +#define config_opt_enabled(cfg, opt_cfg) _config_opt_enabled(cfg, opt_cfg) +#define _config_opt_enabled(cfg_val, opt_value) \
__config_opt_enabled(__ARG_PLACEHOLDER_##cfg_val, opt_value)+#define __config_opt_enabled(arg1_or_junk, arg2) \
___config_opt_enabled(arg1_or_junk arg2, 0)+#define ___config_opt_enabled(__ignored, val, ...) val
+/* Evaluates to 0 if option is not defined, int_option if it is defined */ +#define IF_ENABLED_INT(option, int_option) \
config_opt_enabled(option, int_option)/*
- CONFIG_IS_ENABLED(FOO) evaluates to
- 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y' or 'm',
-- 2.27.0.rc0.183.gde8f92d652-goog
Hi,
On Fri, 22 May 2020 at 00:18, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 5/22/20 4:17 AM, Masahiro Yamada wrote:
On Fri, May 22, 2020 at 11:02 AM Simon Glass sjg@chromium.org wrote:
At present if an optional Kconfig value needs to be used it must be bracketed by #ifdef. For example, with this Kconfig setup:
config WIBBLE bool "Support wibbles, the world needs more wibbles"
config WIBBLE_ADDR hex "Address of the wibble" depends on WIBBLE
I am not sure if this is a good idea.
If you want to always use CONFIG_WIBBLE_ADDR, you can get rid of 'depends on WIBBLE'.
Yes that's right.
But I worry that we end up clutting the Kconfig with unused things.
Another option would be for Kconfig to emit hex CONFIGs always, even if optional?
Hello Simon,
what is the effect on the code size if you eliminate the #ifdefs?
Isn't this move growing the size of the U-Boot binary?
No it is handled at compile time so the code doesn't make it into the image.
[..]
Regards, Simon
The current get_timer_us() uses 64-bit arithmetic. When implementing microsecond-level timeouts, 32-bits is plenty. Add a new function to support this.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/time.h | 11 +++++++++++ lib/time.c | 5 +++++ 2 files changed, 16 insertions(+)
diff --git a/include/time.h b/include/time.h index e99f9c8012..434e63b075 100644 --- a/include/time.h +++ b/include/time.h @@ -17,6 +17,17 @@ unsigned long get_timer(unsigned long base); unsigned long timer_get_us(void); uint64_t get_timer_us(uint64_t base);
+/** + * get_timer_us_long() - Get the number of elapsed microseconds + * + * This uses 32-bit arithmetic on 32-bit machines, which is enough to handle + * delays of over an hour. + * + *@base: Base time to consider + *@return elapsed time since @base + */ +unsigned long get_timer_us_long(unsigned long base); + /* * timer_test_add_offset() * diff --git a/lib/time.c b/lib/time.c index 65db0f6cda..47f8c84327 100644 --- a/lib/time.c +++ b/lib/time.c @@ -152,6 +152,11 @@ uint64_t __weak get_timer_us(uint64_t base) return tick_to_time_us(get_ticks()) - base; }
+unsigned long __weak get_timer_us_long(unsigned long base) +{ + return timer_get_us() - base; +} + unsigned long __weak notrace timer_get_us(void) { return tick_to_time(get_ticks() * 1000);
Enable this feature on chromebook_coral to speed up the display.
With this change, the time taken to print the environment to the display without CONFIG_CONSOLE_SCROLL_LINES is reduced from 1830ms to 62ms.
Signed-off-by: Simon Glass sjg@chromium.org ---
configs/chromebook_coral_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig index 2039ea6186..0bcdbea8d6 100644 --- a/configs/chromebook_coral_defconfig +++ b/configs/chromebook_coral_defconfig @@ -93,6 +93,7 @@ CONFIG_TPM2_CR50_I2C=y CONFIG_USB_XHCI_HCD=y CONFIG_USB_STORAGE=y CONFIG_USB_KEYBOARD=y +CONFIG_VIDEO_COPY=y CONFIG_SPL_FS_CBFS=y # CONFIG_SPL_USE_TINY_PRINTF is not set CONFIG_TPL_USE_TINY_PRINTF=y
At present this enables a few arch-specific members of the global_data struct which are otherwise not part of the struct. As a result we have to use #ifdef in various places.
The cost of always having these in the struct is small. Adjust things so that we can use compile-time code instead of #ifdefs.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/cpu/apollolake/cpu_spl.c | 13 +++++----- arch/x86/cpu/apollolake/fsp_s.c | 10 ++++---- arch/x86/cpu/baytrail/acpi.c | 2 -- arch/x86/cpu/broadwell/power_state.c | 5 ++-- arch/x86/cpu/cpu.c | 38 ++++++++++++++-------------- arch/x86/include/asm/global_data.h | 2 -- arch/x86/lib/coreboot_table.c | 6 ++--- arch/x86/lib/fsp/fsp_common.c | 2 -- arch/x86/lib/fsp/fsp_dram.c | 26 +++++++++++-------- arch/x86/lib/fsp1/fsp_common.c | 16 +++++++----- arch/x86/lib/fsp2/fsp_dram.c | 7 +++-- 11 files changed, 63 insertions(+), 64 deletions(-)
diff --git a/arch/x86/cpu/apollolake/cpu_spl.c b/arch/x86/cpu/apollolake/cpu_spl.c index 707ceb3e64..9f32f2e27e 100644 --- a/arch/x86/cpu/apollolake/cpu_spl.c +++ b/arch/x86/cpu/apollolake/cpu_spl.c @@ -247,12 +247,13 @@ static int arch_cpu_init_spl(void) ret = pmc_init(pmc); if (ret < 0) return log_msg_ret("Could not init PMC", ret); -#ifdef CONFIG_HAVE_ACPI_RESUME - ret = pmc_prev_sleep_state(pmc); - if (ret < 0) - return log_msg_ret("Could not get PMC sleep state", ret); - gd->arch.prev_sleep_state = ret; -#endif + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) { + ret = pmc_prev_sleep_state(pmc); + if (ret < 0) + return log_msg_ret("Could not get PMC sleep state", + ret); + gd->arch.prev_sleep_state = ret; + }
return 0; } diff --git a/arch/x86/cpu/apollolake/fsp_s.c b/arch/x86/cpu/apollolake/fsp_s.c index c5c953f2f8..76cadbe1ca 100644 --- a/arch/x86/cpu/apollolake/fsp_s.c +++ b/arch/x86/cpu/apollolake/fsp_s.c @@ -192,16 +192,16 @@ int arch_fsps_preinit(void)
int arch_fsp_init_r(void) { -#ifdef CONFIG_HAVE_ACPI_RESUME - bool s3wake = gd->arch.prev_sleep_state == ACPI_S3; -#else - bool s3wake = false; -#endif + bool s3wake; struct udevice *dev, *itss; int ret;
if (!ll_boot_init()) return 0; + + s3wake = IS_ENABLED(CONFIG_HAVE_ACPI_RESUME) && + gd->arch.prev_sleep_state == ACPI_S3; + /* * This must be called before any devices are probed. Put any probing * into arch_fsps_preinit() above. diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c index 65f2006a0a..b17bc62a2d 100644 --- a/arch/x86/cpu/baytrail/acpi.c +++ b/arch/x86/cpu/baytrail/acpi.c @@ -161,7 +161,6 @@ void acpi_create_gnvs(struct acpi_global_nvs *gnvs) gnvs->iuart_en = 0; }
-#ifdef CONFIG_HAVE_ACPI_RESUME /* * The following two routines are called at a very early stage, even before * FSP 2nd phase API fsp_init() is called. Registers off ACPI_BASE_ADDRESS @@ -204,4 +203,3 @@ void chipset_clear_sleep_state(void) pm1_cnt = inl(ACPI_BASE_ADDRESS + PM1_CNT); outl(pm1_cnt & ~(SLP_TYP), ACPI_BASE_ADDRESS + PM1_CNT); } -#endif diff --git a/arch/x86/cpu/broadwell/power_state.c b/arch/x86/cpu/broadwell/power_state.c index 99d6f72cf6..62fd2e8d2c 100644 --- a/arch/x86/cpu/broadwell/power_state.c +++ b/arch/x86/cpu/broadwell/power_state.c @@ -23,11 +23,10 @@ static int prev_sleep_state(struct chipset_power_state *ps)
if (ps->pm1_sts & WAK_STS) { switch ((ps->pm1_cnt & SLP_TYP) >> SLP_TYP_SHIFT) { -#if CONFIG_HAVE_ACPI_RESUME case SLP_TYP_S3: - prev_sleep_state = SLEEP_STATE_S3; + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) + prev_sleep_state = SLEEP_STATE_S3; break; -#endif case SLP_TYP_S5: prev_sleep_state = SLEEP_STATE_S5; break; diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index a814e7d7a6..23a4d633d2 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -163,10 +163,10 @@ int default_print_cpuinfo(void) cpu_has_64bit() ? "x86_64" : "x86", cpu_vendor_name(gd->arch.x86_vendor), gd->arch.x86_device);
-#ifdef CONFIG_HAVE_ACPI_RESUME - debug("ACPI previous sleep state: %s\n", - acpi_ss_string(gd->arch.prev_sleep_state)); -#endif + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) { + debug("ACPI previous sleep state: %s\n", + acpi_ss_string(gd->arch.prev_sleep_state)); + }
return 0; } @@ -191,12 +191,12 @@ int last_stage_init(void)
board_final_cleanup();
-#ifdef CONFIG_HAVE_ACPI_RESUME - fadt = acpi_find_fadt(); + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) { + fadt = acpi_find_fadt();
- if (fadt && gd->arch.prev_sleep_state == ACPI_S3) - acpi_resume(fadt); -#endif + if (fadt && gd->arch.prev_sleep_state == ACPI_S3) + acpi_resume(fadt); + }
write_tables();
@@ -277,17 +277,17 @@ int reserve_arch(void) high_table_reserve(); #endif
-#ifdef CONFIG_HAVE_ACPI_RESUME - acpi_s3_reserve(); + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) { + acpi_s3_reserve();
-#ifdef CONFIG_HAVE_FSP - /* - * Save stack address to CMOS so that at next S3 boot, - * we can use it as the stack address for fsp_contiue() - */ - fsp_save_s3_stack(); -#endif /* CONFIG_HAVE_FSP */ -#endif /* CONFIG_HAVE_ACPI_RESUME */ + if (IS_ENABLED(CONFIG_HAVE_FSP)) { + /* + * Save stack address to CMOS so that at next S3 boot, + * we can use it as the stack address for fsp_contiue() + */ + fsp_save_s3_stack(); + } + }
return 0; } diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h index 4aee2f3e8c..0e64c8a46d 100644 --- a/arch/x86/include/asm/global_data.h +++ b/arch/x86/include/asm/global_data.h @@ -116,10 +116,8 @@ struct arch_global_data { u32 high_table_ptr; u32 high_table_limit; #endif -#ifdef CONFIG_HAVE_ACPI_RESUME int prev_sleep_state; /* Previous sleep state ACPI_S0/1../5 */ ulong backup_mem; /* Backup memory address for S3 */ -#endif #ifdef CONFIG_FSP_VERSION2 struct fsp_header *fsp_s_hdr; /* Pointer to FSP-S header */ #endif diff --git a/arch/x86/lib/coreboot_table.c b/arch/x86/lib/coreboot_table.c index 331c1b7e5a..6cd3244301 100644 --- a/arch/x86/lib/coreboot_table.c +++ b/arch/x86/lib/coreboot_table.c @@ -21,11 +21,11 @@ int high_table_reserve(void) gd->arch.high_table_ptr = gd->start_addr_sp;
/* clear the memory */ -#ifdef CONFIG_HAVE_ACPI_RESUME - if (gd->arch.prev_sleep_state != ACPI_S3) -#endif + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME) && + gd->arch.prev_sleep_state != ACPI_S3) { memset((void *)gd->arch.high_table_ptr, 0, CONFIG_HIGH_TABLE_SIZE); + }
gd->start_addr_sp &= ~0xf;
diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c index cf32b3e512..8e3082d4c8 100644 --- a/arch/x86/lib/fsp/fsp_common.c +++ b/arch/x86/lib/fsp/fsp_common.c @@ -60,7 +60,6 @@ void board_final_cleanup(void) debug("OK\n"); }
-#ifdef CONFIG_HAVE_ACPI_RESUME int fsp_save_s3_stack(void) { struct udevice *dev; @@ -84,4 +83,3 @@ int fsp_save_s3_stack(void)
return 0; } -#endif diff --git a/arch/x86/lib/fsp/fsp_dram.c b/arch/x86/lib/fsp/fsp_dram.c index ad5a0f79ad..8f8cb5ede4 100644 --- a/arch/x86/lib/fsp/fsp_dram.c +++ b/arch/x86/lib/fsp/fsp_dram.c @@ -117,17 +117,21 @@ unsigned int install_e820_map(unsigned int max_entries, entries[num_entries].type = E820_RESERVED; num_entries++;
-#ifdef CONFIG_HAVE_ACPI_RESUME - /* - * Everything between U-Boot's stack and ram top needs to be - * reserved in order for ACPI S3 resume to work. - */ - entries[num_entries].addr = gd->start_addr_sp - CONFIG_STACK_SIZE; - entries[num_entries].size = gd->ram_top - gd->start_addr_sp + - CONFIG_STACK_SIZE; - entries[num_entries].type = E820_RESERVED; - num_entries++; -#endif + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) { + ulong stack_size; + + stack_size = IF_ENABLED_INT(CONFIG_HAVE_ACPI_RESUME, + CONFIG_STACK_SIZE); + /* + * Everything between U-Boot's stack and ram top needs to be + * reserved in order for ACPI S3 resume to work. + */ + entries[num_entries].addr = gd->start_addr_sp - stack_size; + entries[num_entries].size = gd->ram_top - gd->start_addr_sp + + stack_size; + entries[num_entries].type = E820_RESERVED; + num_entries++; + }
return num_entries; } diff --git a/arch/x86/lib/fsp1/fsp_common.c b/arch/x86/lib/fsp1/fsp_common.c index 43d32b7abe..da351cf097 100644 --- a/arch/x86/lib/fsp1/fsp_common.c +++ b/arch/x86/lib/fsp1/fsp_common.c @@ -46,10 +46,12 @@ int arch_fsp_init(void) void *nvs; int stack = CONFIG_FSP_TEMP_RAM_ADDR; int boot_mode = BOOT_FULL_CONFIG; -#ifdef CONFIG_HAVE_ACPI_RESUME - int prev_sleep_state = chipset_prev_sleep_state(); - gd->arch.prev_sleep_state = prev_sleep_state; -#endif + int prev_sleep_state; + + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)) { + prev_sleep_state = chipset_prev_sleep_state(); + gd->arch.prev_sleep_state = prev_sleep_state; + }
if (!gd->arch.hob_list) { if (IS_ENABLED(CONFIG_ENABLE_MRC_CACHE)) @@ -57,8 +59,8 @@ int arch_fsp_init(void) else nvs = NULL;
-#ifdef CONFIG_HAVE_ACPI_RESUME - if (prev_sleep_state == ACPI_S3) { + if (IS_ENABLED(CONFIG_HAVE_ACPI_RESUME) && + prev_sleep_state == ACPI_S3) { if (nvs == NULL) { /* If waking from S3 and no cache then */ debug("No MRC cache found in S3 resume path\n"); @@ -79,7 +81,7 @@ int arch_fsp_init(void) stack = cmos_read32(CMOS_FSP_STACK_ADDR); boot_mode = BOOT_ON_S3_RESUME; } -#endif + /* * The first time we enter here, call fsp_init(). * Note the execution does not return to this function, diff --git a/arch/x86/lib/fsp2/fsp_dram.c b/arch/x86/lib/fsp2/fsp_dram.c index 1c82b81831..c9f6402e6a 100644 --- a/arch/x86/lib/fsp2/fsp_dram.c +++ b/arch/x86/lib/fsp2/fsp_dram.c @@ -27,11 +27,10 @@ int dram_init(void) return 0; } if (spl_phase() == PHASE_SPL) { -#ifdef CONFIG_HAVE_ACPI_RESUME - bool s3wake = gd->arch.prev_sleep_state == ACPI_S3; -#else bool s3wake = false; -#endif + + s3wake = IS_ENABLED(CONFIG_HAVE_ACPI_RESUME) && + gd->arch.prev_sleep_state == ACPI_S3;
ret = fsp_memory_init(s3wake, IS_ENABLED(CONFIG_APL_BOOT_FROM_FAST_SPI_FLASH));
With DDR4, Intel SOCs take quite a long time to init their memory. During this time, if the user is watching, it looks like SPL has hung. Add a message in this case.
This works by adding a return code to fspm_update_config() that indicates whether MRC data was found and a new property to the device tree.
Also add one more debug message while starting.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/cpu/apollolake/fsp_m.c | 12 ++++++++-- arch/x86/dts/chromebook_coral.dts | 1 + arch/x86/include/asm/fsp2/fsp_internal.h | 3 ++- arch/x86/lib/fsp2/fsp_meminit.c | 24 +++++++++++++++---- .../fsp/fsp2/apollolake/fsp-m.txt | 3 +++ 5 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/arch/x86/cpu/apollolake/fsp_m.c b/arch/x86/cpu/apollolake/fsp_m.c index 1301100cd5..65461d85b8 100644 --- a/arch/x86/cpu/apollolake/fsp_m.c +++ b/arch/x86/cpu/apollolake/fsp_m.c @@ -16,10 +16,14 @@ int fspm_update_config(struct udevice *dev, struct fspm_upd *upd) { struct fsp_m_config *cfg = &upd->config; struct fspm_arch_upd *arch = &upd->arch; + int cache_ret = 0; ofnode node; + int ret;
arch->nvs_buffer_ptr = NULL; - prepare_mrc_cache(upd); + cache_ret = prepare_mrc_cache(upd); + if (cache_ret && cache_ret != -ENOENT) + return log_msg_ret("mrc", cache_ret); arch->stack_base = (void *)0xfef96000; arch->boot_loader_tolum_size = 0; arch->boot_mode = FSP_BOOT_WITH_FULL_CONFIGURATION; @@ -28,7 +32,11 @@ int fspm_update_config(struct udevice *dev, struct fspm_upd *upd) if (!ofnode_valid(node)) return log_msg_ret("fsp-m settings", -ENOENT);
- return fsp_m_update_config_from_dtb(node, cfg); + ret = fsp_m_update_config_from_dtb(node, cfg); + if (ret) + return log_msg_ret("dtb", cache_ret); + + return cache_ret; }
/* diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index dea35b73a0..aad12f2c4d 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -117,6 +117,7 @@ reg = <0x00000000 0 0 0 0>; compatible = "intel,apl-hostbridge"; pciex-region-size = <0x10000000>; + fspm,training-delay = <21>; /* * Parameters used by the FSP-S binary blob. This is * really unfortunate since these parameters mostly diff --git a/arch/x86/include/asm/fsp2/fsp_internal.h b/arch/x86/include/asm/fsp2/fsp_internal.h index f751fbf961..b4a4fbbd84 100644 --- a/arch/x86/include/asm/fsp2/fsp_internal.h +++ b/arch/x86/include/asm/fsp2/fsp_internal.h @@ -57,7 +57,8 @@ int arch_fsps_preinit(void); * * @dev: Hostbridge device containing config * @upd: Config data to fill in - * @return 0 if OK, -ve on error + * @return 0 if OK, -ENOENT if OK but no MRC-cache data was found, other -ve on + * error */ int fspm_update_config(struct udevice *dev, struct fspm_upd *upd);
diff --git a/arch/x86/lib/fsp2/fsp_meminit.c b/arch/x86/lib/fsp2/fsp_meminit.c index faf9c29aef..ce0b0aff76 100644 --- a/arch/x86/lib/fsp2/fsp_meminit.c +++ b/arch/x86/lib/fsp2/fsp_meminit.c @@ -9,6 +9,7 @@ #include <common.h> #include <binman.h> #include <bootstage.h> +#include <dm.h> #include <log.h> #include <asm/mrccache.h> #include <asm/fsp/fsp_infoheader.h> @@ -63,8 +64,10 @@ int fsp_memory_init(bool s3wake, bool use_spi_flash) struct fsp_header *hdr; struct hob_header *hob; struct udevice *dev; + int delay; int ret;
+ log_debug("Locating FSP\n"); ret = fsp_locate_fsp(FSP_M, &entry, use_spi_flash, &dev, &hdr, NULL); if (ret) return log_msg_ret("locate FSP", ret); @@ -76,21 +79,32 @@ int fsp_memory_init(bool s3wake, bool use_spi_flash) return log_msg_ret("Bad UPD signature", -EPERM); memcpy(&upd, fsp_upd, sizeof(upd));
+ delay = dev_read_u32_default(dev, "fspm,training-delay", 0); ret = fspm_update_config(dev, &upd); - if (ret) - return log_msg_ret("Could not setup config", ret); - - debug("SDRAM init..."); + if (ret) { + if (ret != -ENOENT) + return log_msg_ret("Could not setup config", ret); + } else { + delay = 0; + } + + if (delay) + printf("SDRAM training (%d seconds)...", delay); + else + log_debug("SDRAM init..."); bootstage_start(BOOTSTAGE_ID_ACCUM_FSP_M, "fsp-m"); func = (fsp_memory_init_func)(hdr->img_base + hdr->fsp_mem_init); ret = func(&upd, &hob); bootstage_accum(BOOTSTAGE_ID_ACCUM_FSP_M); cpu_reinit_fpu(); + if (delay) + printf("done\n"); + else + log_debug("done\n"); if (ret) return log_msg_ret("SDRAM init fail\n", ret);
gd->arch.hob_list = hob; - debug("done\n");
ret = fspm_done(dev); if (ret) diff --git a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt index 647a0862d4..67407fa935 100644 --- a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt +++ b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt @@ -17,6 +17,9 @@ values of the FSP-M are used. [2] https://github.com/IntelFsp/FSP/tree/master/ApolloLakeFspBinPkg/Docs
Optional properties: +- fspm,training-delay: Time taken to train DDR memory if there is no cached MRC + data, in seconds. This is used to show a message if possible. For Intel APL + this is typicaly 21 seconds. - fspm,serial-debug-port-address: Debug Serial Port Base address - fspm,serial-debug-port-type: Debug Serial Port Type 0: NONE
Hi Simon,
-----"Simon Glass" sjg@chromium.org schrieb: -----
Betreff: [PATCH 6/6] x86: fsp: Support a warning message when DRAM init is slow
With DDR4, Intel SOCs take quite a long time to init their memory. During this time, if the user is watching, it looks like SPL has hung. Add a message in this case.
This works by adding a return code to fspm_update_config() that indicates whether MRC data was found and a new property to the device tree.
Also add one more debug message while starting.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/cpu/apollolake/fsp_m.c | 12 ++++++++-- arch/x86/dts/chromebook_coral.dts | 1 + arch/x86/include/asm/fsp2/fsp_internal.h | 3 ++- arch/x86/lib/fsp2/fsp_meminit.c | 24 +++++++++++++++---- .../fsp/fsp2/apollolake/fsp-m.txt | 3 +++ 5 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/arch/x86/cpu/apollolake/fsp_m.c b/arch/x86/cpu/apollolake/fsp_m.c index 1301100cd5..65461d85b8 100644 --- a/arch/x86/cpu/apollolake/fsp_m.c +++ b/arch/x86/cpu/apollolake/fsp_m.c @@ -16,10 +16,14 @@ int fspm_update_config(struct udevice *dev, struct fspm_upd *upd) { struct fsp_m_config *cfg = &upd->config; struct fspm_arch_upd *arch = &upd->arch;
int cache_ret = 0; ofnode node;
int ret;
arch->nvs_buffer_ptr = NULL;
- prepare_mrc_cache(upd);
- cache_ret = prepare_mrc_cache(upd);
- if (cache_ret && cache_ret != -ENOENT)
return log_msg_ret("mrc", cache_ret);@@ -28,7 +32,11 @@ int fspm_update_config(struct udevice *dev, struct fspm_upd *upd) if (!ofnode_valid(node)) return log_msg_ret("fsp-m settings", -ENOENT);
- return fsp_m_update_config_from_dtb(node, cfg);
- ret = fsp_m_update_config_from_dtb(node, cfg);
- if (ret)
return log_msg_ret("dtb", cache_ret);- return cache_ret;
}
/* diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts index dea35b73a0..aad12f2c4d 100644 --- a/arch/x86/dts/chromebook_coral.dts +++ b/arch/x86/dts/chromebook_coral.dts @@ -117,6 +117,7 @@ reg = <0x00000000 0 0 0 0>; compatible = "intel,apl-hostbridge"; pciex-region-size = <0x10000000>;
fspm,training-delay = <21>; /* * Parameters used by the FSP-S binary blob. This is * really unfortunate since these parameters mostlydiff --git a/arch/x86/include/asm/fsp2/fsp_internal.h b/arch/x86/include/asm/fsp2/fsp_internal.h index f751fbf961..b4a4fbbd84 100644 --- a/arch/x86/include/asm/fsp2/fsp_internal.h +++ b/arch/x86/include/asm/fsp2/fsp_internal.h @@ -57,7 +57,8 @@ int arch_fsps_preinit(void);
- @dev: Hostbridge device containing config
- @upd: Config data to fill in
- @return 0 if OK, -ve on error
- @return 0 if OK, -ENOENT if OK but no MRC-cache data was found, other -ve on
*/
- error
int fspm_update_config(struct udevice *dev, struct fspm_upd *upd);
diff --git a/arch/x86/lib/fsp2/fsp_meminit.c b/arch/x86/lib/fsp2/fsp_meminit.c index faf9c29aef..ce0b0aff76 100644 --- a/arch/x86/lib/fsp2/fsp_meminit.c +++ b/arch/x86/lib/fsp2/fsp_meminit.c @@ -9,6 +9,7 @@ #include <common.h> #include <binman.h> #include <bootstage.h> +#include <dm.h> #include <log.h> #include <asm/mrccache.h> #include <asm/fsp/fsp_infoheader.h> @@ -63,8 +64,10 @@ int fsp_memory_init(bool s3wake, bool use_spi_flash) struct fsp_header *hdr; struct hob_header *hob; struct udevice *dev;
int delay; int ret;
log_debug("Locating FSP\n"); ret = fsp_locate_fsp(FSP_M, &entry, use_spi_flash, &dev, &hdr, NULL); if (ret) return log_msg_ret("locate FSP", ret);
@@ -76,21 +79,32 @@ int fsp_memory_init(bool s3wake, bool use_spi_flash) return log_msg_ret("Bad UPD signature", -EPERM); memcpy(&upd, fsp_upd, sizeof(upd));
- delay = dev_read_u32_default(dev, "fspm,training-delay", 0); ret = fspm_update_config(dev, &upd);
- if (ret)
return log_msg_ret("Could not setup config", ret);- debug("SDRAM init...");
if (ret) {
if (ret != -ENOENT)return log_msg_ret("Could not setup config", ret);} else {
delay = 0;}
if (delay)
printf("SDRAM training (%d seconds)...", delay);else
log_debug("SDRAM init...");bootstage_start(BOOTSTAGE_ID_ACCUM_FSP_M, "fsp-m"); func = (fsp_memory_init_func)(hdr->img_base + hdr->fsp_mem_init); ret = func(&upd, &hob); bootstage_accum(BOOTSTAGE_ID_ACCUM_FSP_M); cpu_reinit_fpu();
if (delay)
printf("done\n");else
log_debug("done\n");if (ret) return log_msg_ret("SDRAM init fail\n", ret);
gd->arch.hob_list = hob;
debug("done\n");
ret = fspm_done(dev); if (ret)
diff --git a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt index 647a0862d4..67407fa935 100644 --- a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt +++ b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt @@ -17,6 +17,9 @@ values of the FSP-M are used. [2] https://github.com/IntelFsp/FSP/tree/master/ApolloLakeFspBinPkg/Docs
Optional properties: +- fspm,training-delay: Time taken to train DDR memory if there is no cached MRC
- data, in seconds. This is used to show a message if possible. For Intel APL
- this is typicaly 21 seconds.
1) The required time is not APL specific, I think it depends on the memory configuration. I have tested it on an APL board with just 1 GB RAM, and there it only takes ~6 seconds.
How about rewording it as follows: "As an example, for Chromebook Coral this is typically 21 seconds."
2) Typo: typically
- fspm,serial-debug-port-address: Debug Serial Port Base address
- fspm,serial-debug-port-type: Debug Serial Port Type 0: NONE
-- 2.27.0.rc0.183.gde8f92d652-goog
Reviewed-by: Wolfgang Wallner wolfgang.wallner@br-automation.com
Tested-by: Wolfgang Wallner wolfgang.wallner@br-automation.com Tested on a custom Apollo Lake board
participants (4)
-
Heinrich Schuchardt -
Masahiro Yamada -
Simon Glass -
Wolfgang Wallner