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