Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39477 )
Change subject: mb/google/dedede: Enable Intel Speed Shift Technology ......................................................................
Patch Set 5:
(2 comments)
Patch Set 5:
Not to be too much of a stickler, but this is the whole reason for mainboard_silicon_init_params(FSP_S_CONFIG *params). It gives you a chance to update FSP_S UPDs before FSP-S runs. And if 'dee and 'doo are all the variants you have right now, you could just leave the check and setting of ISST in mainboard.c for now.
I think what Karthik identified was yyou
https://review.coreboot.org/c/coreboot/+/39477/5/src/mainboard/google/dedede... File src/mainboard/google/dedede/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39477/5/src/mainboard/google/dedede... PS5, Line 12: /* Nothing to override */ It would be helpful to add a comment indicating when this is required to be implemented by a variant. It would be helpful as we add new variants to ensure that they don't end up copying waddledee/waddledoo.
https://review.coreboot.org/c/coreboot/+/39477/5/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/waddledee/variant.c:
https://review.coreboot.org/c/coreboot/+/39477/5/src/mainboard/google/dedede... PS5, Line 13: where board version is not populated.
If EC returns error, get board version returns -1. […]
Thanks for checking Karthik!