Attention is currently required from: Andrey Petrov, Arthur Heymans, Kapil Porwal, Ronak Kanabar, Tarun Tuli.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75919?usp=email )
Change subject: driver/{intel,mrc_cache}: Have MRC cache ver. based on FSP-M/MRC ver
......................................................................
Patch Set 3:
(1 comment)
File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/75919/comment/cd4deb3a_cc9801a1 :
PS3, Line 14: help
This choice is user visible on ALL users of CACHE_MRC_SETTINGS, however not all users use FSP and the choice is not even meaningfull on all FSP platforms.
Is a user visible option even meaningful? The FSP either exposes extended header or it does not. The user should not have to figure this out. The platform should select the right option.
isn't the help msg is self explanatory ? i got your point but there is no need to give -1 for such reason (an unsolved comment itself is enough as in the latest patchset is not even +2'ed)
Yes but why should the user even have to read the help msg in the menu if it can be avoided.
I believe here the definition of the `user` is still a developer and not the real end user (i hope u agrees to this fact). And for the platform developer, we should have knowledge about what is the decision logic is used to store the MRC cache. For example: since last so many FSP generations of Intel based devices, we were using FSP version to associate with MRC cache because there was no way FSP would have provided the MRC version. What is the downside of this logic? the real end users are seeing some unnecessary black screen while we are updating the FW in-field devices.
The time we actually discovered that (since the mrc_cache being implemented till date we were not concerned about this), it took few months to land a proper solution (w/ help from Intel) which would help to improve the device experience so, my point is its still good to give some msgs/notifications to the developer to know what are the hidden underlying assumption while building the platform. And if develop is not sure, should leave this as is.
-1 gets removed on updating a patch iirc.
thanks for highlighting this.
--
To view, visit
https://review.coreboot.org/c/coreboot/+/75919?usp=email
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic3084d37ff1d3f9282d30f3be0ebf15db0ec0e6f
Gerrit-Change-Number: 75919
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik
subratabanik@google.com
Gerrit-Reviewer: Andrey Petrov
andrey.petrov@gmail.com
Gerrit-Reviewer: Arthur Heymans
arthur@aheymans.xyz
Gerrit-Reviewer: Kapil Porwal
kapilporwal@google.com
Gerrit-Reviewer: Ronak Kanabar
ronak.kanabar@intel.com
Gerrit-Reviewer: Tarun Tuli
taruntuli@google.com
Gerrit-Reviewer: build bot (Jenkins)
no-reply@coreboot.org
Gerrit-Attention: Tarun Tuli
taruntuli@google.com
Gerrit-Attention: Kapil Porwal
kapilporwal@google.com
Gerrit-Attention: Arthur Heymans
arthur@aheymans.xyz
Gerrit-Attention: Ronak Kanabar
ronak.kanabar@intel.com
Gerrit-Attention: Andrey Petrov
andrey.petrov@gmail.com
Gerrit-Comment-Date: Mon, 19 Jun 2023 18:04:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik
subratabanik@google.com
Comment-In-Reply-To: Arthur Heymans
arthur@aheymans.xyz
Gerrit-MessageType: comment