Hi Sean, On Fri, Nov 18, 2022 at 5:16 AM Sean Anderson <sean.ander...@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 <s...@chromium.org> wrote: > >>>> > >>>> Hi, > >>>> > >>>> On Mon, 24 Oct 2022 at 22:57, Stefan Roese <s...@denx.de> wrote: > >>>> > > >>>> > On 24.10.22 18:49, Bin Meng wrote: > >>>> > > On Mon, Oct 24, 2022 at 11:42 PM Sean Anderson > >>>> > > <sean.ander...@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