Karthik Ramasubramanian 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 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39477/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39477/6/src/mainboard/google/dedede... PS6, Line 10: __weak void variant_isst_override(void)
Do we need this weak function? Why not put the code inside mainboard_config_isst() directly?
Only 2 variant boards and that too only in initial board phases use early revisions of Silicon. Also not every variant board that does not have the board version populated use the early Silicon revision. Hence the weak override is added.
https://review.coreboot.org/c/coreboot/+/39477/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/waddledee/variant.c:
https://review.coreboot.org/c/coreboot/+/39477/6/src/mainboard/google/dedede... PS6, Line 17:
Do we really required two varinat files, one under waddledee/variant.c and waddledoo/variant.c […]
Only 2 variant boards and that too only in initial board phases use early revisions of Silicon. Also not every variant board that does not have the board version populated use the early Silicon revision. Hence the weak override is added.
Moving it to the baseboard will apply for all new boards, which is not expected.