Attention is currently required from: Alok Agarwal, Anil Kumar K, Intel coreboot Reviewers, Jayvik Desai, Julius Werner, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Vikrant L Jadeja.
Subrata Banik has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/85454?usp=email )
Change subject: soc/intel/pantherlake: Display Sign-of-Life during memory training ......................................................................
Patch Set 21:
(1 comment)
File src/soc/intel/pantherlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/85454/comment/4e552442_343b3a26?usp... : PS21, Line 387: m_cfg->VgaMessage = (efi_uintn_t)ux_locales_get_text(UX_LOCALE_MSG_MEMORY_TRAINING);
I have considered your point, and I agree with you that there is a slight leap of faith in the proposed refactoring of the code. The Firmware Support Package (FSP) engineers are not going to guarantee that there won't be any changes in the upcoming generations. However, I would argue that:
- `VbtSize` and `LidStatus` have been in existence for some time (since the Alder Lake generation, and possibly even earlier, in System-on-Chips (SoCs) not supported by coreboot).
- `VbtPtr`, `VgaInitControl`, and `VgaMessage` have already persisted through several generations (Meteor Lake and Panther Lake, but also Lunar Lake, and as far as I know, Wildcat Lake).
I do not think it is particularly detrimental to provide a common function at this time and see how long we can continue to use it.
Independent of how long we can go this route and then pull back, I'd recommend not to pursue this approach, as it relies on FSP UPD names. In the past, we have done this as well, but the common code principle discourages common things based on FSP UPD names, which can change without notice. I don't see any added value to trying to combine MTL/PTL FSP UPDs with the assumption that it will stay common. How about adding more eSOL UPDs in the future, like Battery Status, which won't be present inside a prior generation but we assume is added in the NVL timeline? We should disregard any need to common things based on FSP UPDs. FSP UPDs are meant to be SoC local, and trying to generalize these are simply overdoing it.