Hi Simon, On Wed, Mar 22, 2017 at 4:06 AM, Simon Glass <s...@chromium.org> wrote: > Hi Bin, > > On 16 March 2017 at 08:26, Bin Meng <bmeng...@gmail.com> wrote: >> At the end of pre-relocation phase, save the new stack address >> to CMOS and use it as the stack on next S3 boot for fsp_init() >> continuation function. >> >> Signed-off-by: Bin Meng <bmeng...@gmail.com> >> --- >> >> arch/x86/cpu/cpu.c | 8 ++++++++ >> arch/x86/include/asm/cmos_layout.h | 31 +++++++++++++++++++++++++++++++ >> arch/x86/include/asm/u-boot-x86.h | 1 + >> arch/x86/lib/fsp/fsp_common.c | 30 +++++++++++++++++++++++++++++- >> 4 files changed, 69 insertions(+), 1 deletion(-) >> create mode 100644 arch/x86/include/asm/cmos_layout.h > > Reviewed-by: Simon Glass <s...@chromium.org> > > nits below > >> >> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c >> index afc8645..9e2aee2 100644 >> --- a/arch/x86/cpu/cpu.c >> +++ b/arch/x86/cpu/cpu.c >> @@ -278,6 +278,14 @@ int reserve_arch(void) >> high_table_reserve(); >> #endif >> >> +#if defined(CONFIG_HAVE_ACPI_RESUME) && defined(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 >> + >> return 0; >> } >> #endif >> diff --git a/arch/x86/include/asm/cmos_layout.h >> b/arch/x86/include/asm/cmos_layout.h >> new file mode 100644 >> index 0000000..0a0a51e >> --- /dev/null >> +++ b/arch/x86/include/asm/cmos_layout.h >> @@ -0,0 +1,31 @@ >> +/* >> + * Copyright (C) 2017, Bin Meng <bmeng...@gmail.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#ifndef __CMOS_LAYOUT_H >> +#define __CMOS_LAYOUT_H >> + >> +/* >> + * The RTC internal registers and RAM is organized as two banks of 128 bytes >> + * each, called the standard and extended banks. The first 14 bytes of the >> + * standard bank contain the RTC time and date information along with four >> + * registers, A - D, that are used for configuration of the RTC. The >> extended >> + * bank contains a full 128 bytes of battery backed SRAM. >> + * >> + * For simplicity in U-Boot we only support CMOS in the standard bank, and >> + * its base address starts from offset 0x10, which leaves us 112 bytes >> space. >> + */ >> +#define CMOS_BASE 0x10 >> + >> +/* >> + * The file records all offsets off CMOS_BASE that is currently used by >> + * U-Boot for various reasons. It is put in such a unified place in order >> + * to be consistent across platforms. >> + */ >> + >> +/* stack address for S3 boot in a FSP configuration, 4 bytes */ >> +#define CMOS_FSP_STACK_ADDR CMOS_BASE >> + >> +#endif /* __CMOS_LAYOUT_H */ >> diff --git a/arch/x86/include/asm/u-boot-x86.h >> b/arch/x86/include/asm/u-boot-x86.h >> index 4f901f9..024aaf4 100644 >> --- a/arch/x86/include/asm/u-boot-x86.h >> +++ b/arch/x86/include/asm/u-boot-x86.h >> @@ -56,6 +56,7 @@ u32 isa_map_rom(u32 bus_addr, int size); >> int video_bios_init(void); >> >> /* arch/x86/lib/fsp/... */ >> +int fsp_save_s3_stack(void); >> int x86_fsp_init(void); > > Function comment? > >> >> void board_init_f_r_trampoline(ulong) __attribute__ ((noreturn)); >> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c >> index 2b33fba..df73a2a 100644 >> --- a/arch/x86/lib/fsp/fsp_common.c >> +++ b/arch/x86/lib/fsp/fsp_common.c >> @@ -5,8 +5,12 @@ >> */ >> >> #include <common.h> >> +#include <dm.h> >> #include <errno.h> >> +#include <rtc.h> >> #include <asm/acpi_s3.h> >> +#include <asm/cmos_layout.h> >> +#include <asm/early_cmos.h> >> #include <asm/io.h> >> #include <asm/mrccache.h> >> #include <asm/post.h> >> @@ -71,9 +75,32 @@ static __maybe_unused void *fsp_prepare_mrc_cache(void) >> return cache->data; >> } >> >> +#ifdef CONFIG_HAVE_ACPI_RESUME >> +int fsp_save_s3_stack(void) >> +{ >> + struct udevice *dev; >> + int ret; >> + >> + if (gd->arch.prev_sleep_state == ACPI_S3) >> + return 0; >> + >> + ret = uclass_get_device(UCLASS_RTC, 0, &dev); > > You need to use uclass_get_device_err() to get an error I think.
There is no such API in current mainline. > >> + if (ret) { >> + debug("Cannot find RTC: err=%d\n", ret); >> + return -ENODEV; >> + } >> + >> + /* Save the stack address to CMOS */ >> + rtc_write32(dev, CMOS_FSP_STACK_ADDR, gd->start_addr_sp); > > Check error? > >> + >> + return 0; >> +} >> +#endif >> + >> int x86_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(); >> @@ -102,6 +129,7 @@ int x86_fsp_init(void) >> panic("Reboot System"); >> } >> >> + stack = cmos_read32(CMOS_FSP_STACK_ADDR); > > Perhaps you should comment why you are using this instead of rtc_...() ? > Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot