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
