On 10/24/22 12:49, Bin Meng wrote:
On Mon, Oct 24, 2022 at 11:42 PM Sean Anderson sean.anderson@seco.com wrote:
FSP support requires DM_RTC for rtc_write32. Select it.
Fixes: ba65808e7d0 ("x86: fsp: Save stack address to CMOS for next S3 boot") Signed-off-by: Sean Anderson sean.anderson@seco.com
This seems like it would never have worked. Does fsp_save_s3_stack even
This was working before. Did you test it on x86 that now it is broken?
Well, if it never got called in SPL it would never have failed.
Here is the error I was getting with the next patch applied:
../arch/x86/lib/fsp/fsp_common.c: In function ‘fsp_save_s3_stack’: ../arch/x86/lib/fsp/fsp_common.c:79:20: warning: passing argument 1 of ‘rtc_write32’ makes integer from pointer without a cast [-Wint-conversion] 79 | ret = rtc_write32(dev, CMOS_FSP_STACK_ADDR, gd->start_addr_sp); | ^~~ | | | struct udevice * In file included from ../arch/x86/lib/fsp/fsp_common.c:12: ../include/rtc.h:288:22: note: expected ‘int’ but argument is of type ‘struct udevice *’ 288 | void rtc_write32(int reg, u32 value); | ~~~~^~~ ../arch/x86/lib/fsp/fsp_common.c:79:8: error: too many arguments to function ‘rtc_write32’ 79 | ret = rtc_write32(dev, CMOS_FSP_STACK_ADDR, gd->start_addr_sp); | ^~~~~~~~~~~ In file included from ../arch/x86/lib/fsp/fsp_common.c:12: ../include/rtc.h:288:6: note: declared here 288 | void rtc_write32(int reg, u32 value); | ^~~~~~~~~~~ ../arch/x86/lib/fsp/fsp_common.c:79:6: error: void value not ignored as it ought to be 79 | ret = rtc_write32(dev, CMOS_FSP_STACK_ADDR, gd->start_addr_sp); | ^ make[4]: *** [../scripts/Makefile.build:257: spl/arch/x86/lib/fsp/fsp_common.o] Error 1 make[3]: *** [../scripts/Makefile.build:398: spl/arch/x86/lib/fsp] Error 2 make[2]: *** [../scripts/Makefile.spl:531: spl/arch/x86/lib] Error 2
You can see that the file is using the dm version of rtc_write32, but SPL_DM_RTC is not enabled. This was not caught before because the dm version of rtc_write32 was always declared when DM_RTC was enabled, even if SPL_DM_RTC was disabled. But of course, if you pass a pointer to non-dm rtc_write32, it will break.
So maybe the best solution here is to add a third patch converting FSP to use dm_rtc_write, and then remove the selects. That way we can just get an error if DM_RTC is disabled, instead of potentially writing to random registers.
--Sean
+Stefan
get called in SPL? Maybe it should be converted to use dm_rtc_write instead.
Changes in v3:
- New
arch/x86/Kconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 7cbfd6c9720..ed8216d9ad0 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -362,6 +362,8 @@ config HAVE_FSP depends on !EFI select USE_HOB select HAS_ROM
select DM_RTCselect SPL_DM_RTC help Select this option to add an Firmware Support Package binary to the resulting U-Boot image. It is a binary blob which U-Boot uses--
Regards, Bin