| Message ID | 20221024154204.2133777-2-sean.anderson@seco.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | rtc: Add fallbacks for dm functions | expand |
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_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
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_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
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_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 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_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 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
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
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(+)