[PATCH v3 1/2] x86: fsp: Depend on DM_RTC

[Sean Anderson]

On 10/24/22 12:49, Bin Meng wrote:
> On Mon, Oct 24, 2022 at 11:42 PM Sean Anderson <sean.anderson at 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 at 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_RTC
>> +       select 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