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

[Sean Anderson]

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 *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 at 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 at 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
>