[PATCH v3 0/2] rtc: Add fallbacks for dm functions
This adds some fallbacks for DM RTC consumers, so they don't have to ifdef out their RTC code.
There's some funkiness going on with FSP on x86, but I don't know what the correct behavior is. See patch 1 for details.
Changes in v3: - Select SPL_DM_RTC for HAVE_FSP. Before this series, even if just DM_RTC was selected, the DM RTC functions would be forward-declared. This probably would result in funky things at runtime.
Changes in v2: - Include linux/errno.h for ENOSYS. This is needed for some mips boards when CONFIG_BLK is enabled for whatever reason.
Sean Anderson (2): x86: fsp: Depend on DM_RTC rtc: Add fallbacks for dm functions
arch/x86/Kconfig | 2 ++ include/rtc.h | 32 +++++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-)
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 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
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?
+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
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
On 24.10.22 18: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?
+Stefan
I don't have access to this FSP x86 target any more, so can't test anything any more.
Thanks, 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
Viele Grüße, Stefan Roese
Hi,
On Mon, 24 Oct 2022 at 22:57, Stefan Roese sr@denx.de wrote:
On 24.10.22 18: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?
I think it is better to select these options rather than rely on boards to do so. I suspect that 'moveconfig.py -s' will remove some things from defconfigs.
Reviewed-by: Simon Glass sjg@chromium.org
+Stefan
I don't have access to this FSP x86 target any more, so can't test anything any more.
Thanks, 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
Regards, Simon
Hi Sean,
On Wed, Oct 26, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 24 Oct 2022 at 22:57, Stefan Roese sr@denx.de wrote:
On 24.10.22 18: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")
Please drop this "Fixes" tag.
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?
I think it is better to select these options rather than rely on boards to do so. I suspect that 'moveconfig.py -s' will remove some things from defconfigs.
Reviewed-by: Simon Glass sjg@chromium.org
+Stefan
I don't have access to this FSP x86 target any more, so can't test anything any more.
Thanks, Stefan
Regards, Bin
On 10/25/22 20:52, Bin Meng wrote:
Hi Sean,
On Wed, Oct 26, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 24 Oct 2022 at 22:57, Stefan Roese sr@denx.de wrote:
On 24.10.22 18: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")
Please drop this "Fixes" tag.
IMO the existing driver is buggy. I still have not gotten an answer about where the bug lies, but either
- fsp_save_s3_stack is not called by SPL, in which case it should not be compiled in. this indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot"). - fsp_save_s3_stack *is* called by SPL, but it is missing support for non-DM RTC. This indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot"). If this is the case, this patch will break things. - fsp_save_s3_stack *is* called by SPL, but it is expected that SPL_DM_RTC is selected by the defconfig. This indicates a bug in a1d6dc3f840 ("x86: Add chromebook_coral").
--Sean
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?
I think it is better to select these options rather than rely on boards to do so. I suspect that 'moveconfig.py -s' will remove some things from defconfigs.
Reviewed-by: Simon Glass sjg@chromium.org
+Stefan
I don't have access to this FSP x86 target any more, so can't test anything any more.
Thanks, Stefan
Regards, Bin
On 10/26/22 11:58, Sean Anderson wrote:
On 10/25/22 20:52, Bin Meng wrote:
Hi Sean,
On Wed, Oct 26, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 24 Oct 2022 at 22:57, Stefan Roese sr@denx.de wrote:
On 24.10.22 18: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")
Please drop this "Fixes" tag.
IMO the existing driver is buggy. I still have not gotten an answer about where the bug lies, but either
- fsp_save_s3_stack is not called by SPL, in which case it should not be compiled in. this indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot").
- fsp_save_s3_stack *is* called by SPL, but it is missing support for non-DM RTC. This indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot"). If this is the case, this patch will break things.
err, it won't break anything, because the existing code doesn't work, but this patch doesn't address the problem.
- fsp_save_s3_stack *is* called by SPL, but it is expected that SPL_DM_RTC is selected by the defconfig. This indicates a bug in a1d6dc3f840 ("x86: Add chromebook_coral").
--Sean
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?
I think it is better to select these options rather than rely on boards to do so. I suspect that 'moveconfig.py -s' will remove some things from defconfigs.
Reviewed-by: Simon Glass sjg@chromium.org
+Stefan
I don't have access to this FSP x86 target any more, so can't test anything any more.
Thanks, Stefan
Regards, Bin
On 10/26/22 12:01, Sean Anderson wrote:
On 10/26/22 11:58, Sean Anderson wrote:
On 10/25/22 20:52, Bin Meng wrote:
Hi Sean,
On Wed, Oct 26, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 24 Oct 2022 at 22:57, Stefan Roese sr@denx.de wrote:
On 24.10.22 18: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")
Please drop this "Fixes" tag.
IMO the existing driver is buggy. I still have not gotten an answer about where the bug lies, but either
- fsp_save_s3_stack is not called by SPL, in which case it should not be compiled in. this indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot").
- fsp_save_s3_stack *is* called by SPL, but it is missing support for non-DM RTC. This indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot"). If this is the case, this patch will break things.
err, it won't break anything, because the existing code doesn't work, but this patch doesn't address the problem.
- fsp_save_s3_stack *is* called by SPL, but it is expected that SPL_DM_RTC is selected by the defconfig. This indicates a bug in a1d6dc3f840 ("x86: Add chromebook_coral").
ping?
Bin, do you know which of the above scenarios is correct?
--Sean
--Sean
> 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?
I think it is better to select these options rather than rely on boards to do so. I suspect that 'moveconfig.py -s' will remove some things from defconfigs.
Reviewed-by: Simon Glass sjg@chromium.org
+Stefan
I don't have access to this FSP x86 target any more, so can't test anything any more.
Thanks, Stefan
Regards, Bin
Hi Sean,
On Fri, Nov 18, 2022 at 5:16 AM Sean Anderson sean.anderson@seco.com wrote:
On 10/26/22 12:01, Sean Anderson wrote:
On 10/26/22 11:58, Sean Anderson wrote:
On 10/25/22 20:52, Bin Meng wrote:
Hi Sean,
On Wed, Oct 26, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 24 Oct 2022 at 22:57, Stefan Roese sr@denx.de wrote:
On 24.10.22 18: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")
Please drop this "Fixes" tag.
IMO the existing driver is buggy. I still have not gotten an answer about where the bug lies, but either
- fsp_save_s3_stack is not called by SPL, in which case it should not be compiled in. this indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot").
fsp_save_s3_stack() was originally introduced on the BayTrail platform, and is not called by SPL. DM_RTC is implied by the x86 cpu Kconfig so there is no build error.
- fsp_save_s3_stack *is* called by SPL, but it is missing support for non-DM RTC. This indicates a bug in ba65808e7d0 ("x86: fsp: save stack address to cmos for next s3 boot"). If this is the case, this patch will break things.
There is no need to support non-DM RTC on x86. Lots of x86 U-Boot codes were born to support DM on day 1.
err, it won't break anything, because the existing code doesn't work, but this patch doesn't address the problem.
- fsp_save_s3_stack *is* called by SPL, but it is expected that SPL_DM_RTC is selected by the defconfig. This indicates a bug in a1d6dc3f840 ("x86: Add chromebook_coral").
fsp_save_s3_stack() is not called by SPL, even on chromebook_coral. Simon can you confirm?
ping?
Bin, do you know which of the above scenarios is correct?
So I don't think there is a bug anyway, either from build perspective or runtime perspective.
Your fix of adding the explicit DM_RTC dependency to HAVE_FSP makes sense. I was concerned about the use of the "Fixes" tag. I would restrict the 'Fixes:' tag to real bug regressions, as it might help downstream distributions to filter commits to cherry-pick.
In this particular use-case I would use an example like:
When adding the ACPI S3 support in commit ba65808e7d0 ("x86: fsp: Save stack address to CMOS for next S3 boot"), the dependency to RTC was expressed by x86 in arch/Kconfig. It might be better to declare the dependency explicitly in HAVE_FSP. Do it now.
Regards, Bin
This adds fallbacks for the various dm_rtc_* functions. This allows common code to use these functions without ifdefs.
Fixes: c8ce7ba87d1 ("misc: Add support for nvmem cells") Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Sean Anderson sean.anderson@seco.com ---
(no changes since v2)
Changes in v2: - Include linux/errno.h for ENOSYS. This is needed for some mips boards when CONFIG_BLK is enabled for whatever reason.
include/rtc.h | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/include/rtc.h b/include/rtc.h index 10104e3bf5a..b6fdbb60dc2 100644 --- a/include/rtc.h +++ b/include/rtc.h @@ -15,13 +15,12 @@
#include <bcd.h> #include <rtc_def.h> +#include <linux/errno.h>
typedef int64_t time64_t; - -#ifdef CONFIG_DM_RTC - struct udevice;
+#if CONFIG_IS_ENABLED(DM_RTC) struct rtc_ops { /** * get() - get the current time @@ -222,6 +221,33 @@ int rtc_enable_32khz_output(int busnum, int chip_addr); #endif
#else +static inline int dm_rtc_get(struct udevice *dev, struct rtc_time *time) +{ + return -ENOSYS; +} + +static inline int dm_rtc_set(struct udevice *dev, struct rtc_time *time) +{ + return -ENOSYS; +} + +static inline int dm_rtc_reset(struct udevice *dev) +{ + return -ENOSYS; +} + +static inline int dm_rtc_read(struct udevice *dev, unsigned int reg, u8 *buf, + unsigned int len) +{ + return -ENOSYS; +} + +static inline int dm_rtc_write(struct udevice *dev, unsigned int reg, + const u8 *buf, unsigned int len) +{ + return -ENOSYS; +} + int rtc_get (struct rtc_time *); int rtc_set (struct rtc_time *); void rtc_reset (void);
participants (4)
-
Bin Meng -
Sean Anderson -
Simon Glass -
Stefan Roese