[coreboot-gerrit] [S] Change in coreboot[master]: drivers/intel/fsp2_0: Introduce early MRC cache store

[Subrata Banik (Code Review)]

Attention is currently required from: Andrey Petrov, Felix Held, Fred Reitberger, Jason Glenesk, Kapil Porwal, Lean Sheng Tan, Martin L Roth, Matt DeVillier, Nico Huber, Paul Menzel, Raul Rangel.

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77556?usp=email )

Change subject: drivers/intel/fsp2_0: Introduce early MRC cache store
......................................................................

Patch Set 3:

(3 comments)

File src/drivers/mrc_cache/Kconfig:

https://review.coreboot.org/c/coreboot/+/77556/comment/dbbb6812_3ea6cdbe :
PS2, Line 24: MRC_STASH_TO_CBMEM
> > are you suggesting to keep MRC_WRITE_NV_LATE and MRC_STASH_TO_CBMEM ? or saying rename MRC_STASH_T […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/77556/comment/40ee41d6_8dbf7612 :
PS2, Line 35: boot
            : 	  states
> seems a bit unclear as written, so I was suggesting to drop "boot states" so it reads: ... […]

Acknowledged

File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/77556/comment/123dd2a2_a4c2c7fb :
PS1, Line 742: BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_ENTRY, finalize_mrc_cache, NULL);
> > > > This changes the order for devices with MRC_STASH_TO_CBMEM but without
> > > > MRC_WRITE_NV_LATE, e.g. those with BOOT_DEVICE_SPI_FLASH_NO_EARLY_WRITES.
> > > >
> > > > It's theoretically possible that this breaks. For instance, when the SPI
> > > > flash is locked between enumerate/on_exit and resume_check/on_entry. So
> > > > that would need extensive review or testing.
> > >
> > > So far we have three different ways to store the MRC cache below and different SoC vendors select the applicable config as per the platform need.
> > >
> > > w/o this CL:
> > > 1. NV Write late (AMD)- 
> > >
> > > save_mrc_data - BS_PRE_DEVICE_ENTRY
> > > finalize_mrc_cache - BS_OS_RESUME_CHECK_ENTRY
> > > lock - BS_OS_RESUME_CHECK_EXIT
> > >
> > >
> > > 2. MRC_STASH_TO_CBMEM aka w/ BOOT_DEVICE_SPI_FLASH_NO_EARLY_WRITES (BSW, BYT)
> > >
> > > save_mrc_data - BS_PRE_DEVICE_ENTRY
> > > finalize_mrc_cache - BS_DEV_ENUMERATE_EXIT
> > > lock - BS_DEV_RESOURCES_ENTRY
> > >
> > > 3. SAVE_MRC_AFTER_FSPS (XEON-SP)
> > >
> > > save_mrc_data - BS_DEV_INIT_CHIPS_EXIT
> > > finalize_mrc_cache - BS_DEV_ENUMERATE_EXIT
> > > lock - BS_DEV_RESOURCES_ENTRY
> >
> > These are just three special cases that defer the writing into
> > ramstage. Currently also FSP 2.0 platforms do it in ramstage and
> > there are many more that currently do it in romstage.
> > > w/ this cl:
> > >
> > > simplified using 2-ways to store the MRC cache below and how it can meet all the needs.
> > > 1. NV Write late/SAVE_MRC_AFTER_FSPS -> MRC_STASH_TO_CBMEM (also for BOOT_DEVICE_SPI_FLASH_NO_EARLY_WRITES) (AMD, XEON_SP, BSW and BYT)
> > >
> > > save_mrc_data - BS_DEV_INIT_CHIPS_EXIT
> > > finalize_mrc_cache - BS_OS_RESUME_CHECK_ENTRY
> > > lock - BS_OS_RESUME_CHECK_EXIT
> > >
> > >
> > > 2. MRC cache early (INTEL)
> > >
> > > save_mrc_data - post FSP-M
> > > finalize_mrc_cache - BS_DEV_ENUMERATE_EXIT
> > > lock - BS_DEV_RESOURCES_ENTRY
> > >
> > > In summary,
> > > - the order of the execution still remains the same (w/ and w/o this CL) between saving_mrc_cache later finalizes the mrc cache and finally locking the boot media.
> > >
> > > - I don't see any scenario where the we are attempting to write into the SPI but it's getting locked as you have highlighted (when the SPI flash is locked between enumerate/on_exit and resume_check/on_entry) 
> >
> > I agree to have only two paths. 1. that defers the write to ramstage,
> > and 2. that does it directly in romstage. However, changing the
> > boot-state hooks in ramstage--when the write is exactly done--requires
> > more careful evaluation.
> >
> > So how about we don't change the boot-state hooks too much right now,
> > and focus on the code paths?
> >
> > This could roughly happen in the following order:
> > Make the changes of CB:67669 optional (adding an interim Kconfig for this)
> > Could happen in parallel:
> > Let SAVE_MRC_AFTER_FSPS use MRC_STASH_TO_CBMEM instead of the `save_mrc_data.c` path
> > Let other FSP2.0 platforms directly write in romstage
>
> You beat me on this. I'm about to propose this. Lets SAVE_MRC_AFTER_FSPS and MRC_STASH_TO_CBMEM use the same boot states except I need to save mrc_cache a little late (after FSP-S exit) to unify both paths.
> > Remove `save_mrc_data.c` path
>
> I'm sure if we can remove this file otherwise i need to copy the save_mrc_cache API into memory_init.c file. And call it from there. may be keep this file as is bt include if MRC_CACHE_EARLY ?
> > Then only two major code paths would be left (if I didn't miss any):
> > One with and one without MRC_STASH_TO_CBMEM. Then we could still look
> > into unifying the boot-state hooks.
> > Btw. I don't see the Intel/AMD distinction that you seem to imply.
> > Most AMD platforms seem to simply follow the default FSP2.0 code path.
>
> Agree but I don't know why AMD platform would like to finalize_mrc_cache that late like BS_OS_RESUME_CHECK_ENTRY. hence, we ended up keeping both MRC_STASH_TO_CBMEM and MRC_CACHE_LATE

Please take a look at the latest patchset