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

[Bin Meng]

Hi Sean,

On Fri, Nov 18, 2022 at 5:16 AM Sean Anderson <sean.anderson at 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 at chromium.org> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On Mon, 24 Oct 2022 at 22:57, Stefan Roese <sr at 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 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")
> >>>
> >>> 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