Attention is currently required from: Maulik V Vaghela, Mario Scheithauer, Subrata Banik, Lean Sheng Tan, Patrick Rudolph. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55355 )
Change subject: soc/intel/elkhartlake: Enable PCH GBE ......................................................................
Patch Set 3:
(3 comments)
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/55355/comment/749fde18_940f75e7 PS3, Line 386: Speed_2_5Gbps, : Speed_1Gbps, This is an enum of type TsnGbeLinkSpeed already, so we all are aware that we are dealing with "Speed" here. Shall we just name it tsn_2_5_Gbps and tsn_1_Gbps instead? Would be more meaningful, at least to me.
https://review.coreboot.org/c/coreboot/+/55355/comment/a9d80238_818aac25 PS3, Line 389: /* PCH TSN GBE Link Speed: Disable (0) / Enable (1) */ : bool PchTsnGbeLinkSpeed; This looks starnge to me. Usually, speed is not a bool. It looks like you try to transport a 0 or 1 integer value here , or? If yes, I would stick with a char here and, once it is used and you really need 0 or 1, do something like this: 'param = !!config->PchTsnGbeLinkSpeed' This will make sure that only values of 0 and 1 are passed into param, depending of config->PchTsnGbeLinkSpeed being 0 or != 0
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55355/comment/07fcab1c_6d12a2c3 PS3, Line 377: config->PchTsnGbeLinkSpeed; See previous comment.
-
Werner Zeh (Code Review)