Attention is currently required from: Maulik V Vaghela, Mario Scheithauer, Subrata Banik, Werner Zeh, Patrick Rudolph. Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55355 )
Change subject: soc/intel/elkhartlake: Enable PCH GBE ......................................................................
Patch Set 4:
(5 comments)
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/55355/comment/c67322c4_8d61d1d2 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 […]
Done
https://review.coreboot.org/c/coreboot/+/55355/comment/7a8e5239_90e267b0 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. […]
Done. comment updated also.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55355/comment/7b8f9a5a_6c59d268 PS3, Line 377: config->PchTsnGbeLinkSpeed;
See previous comment.
Done
https://review.coreboot.org/c/coreboot/+/55355/comment/f6714f6f_ffdf043c PS3, Line 375: switch (CONFIG_CPU_XTAL_HZ) { : case 24000000: : params->PchTsnGbeLinkSpeed = config->PchTsnGbeLinkSpeed; : break; : case 38400000: : params->PchTsnGbeLinkSpeed = (config->PchTsnGbeLinkSpeed) + 2; : break; : default: : printk(BIOS_ERROR, "ERROR: Unsupported CPU XTAL for GBE config.\n : Disabling PCH GBE.\n"); : params->PchTsnEnable = 0; : break;
For now CONFIG_CPU_XTAL_HZ is hard coded in Kconfig of the soc: […]
Yes you are right. but still, i added a safe check here in case someone mistakenly change the CPU xtal in future.
https://review.coreboot.org/c/coreboot/+/55355/comment/fced89f7_338d38b9 PS3, Line 383: printk(BIOS_ERROR, "ERROR: Unsupported CPU XTAL for GBE config.\n : Disabling PCH GBE.\n"); : params->PchTsnEnable = 0; : break;
Please take care of the indention and the missing curly bracket for case-switch. […]
Done
-
Lean Sheng Tan (Code Review)