Attention is currently required from: Patrick Rudolph, Simon Chou, Paul Menzel, Shuming Chu (Shuming), Arthur Heymans, Juan Sanchez.
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71968 )
Change subject: mb/intel: add Archer City CRB support ......................................................................
Patch Set 28:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/71968/comment/a47b6341_f2c8a03a PS27, Line 11:
What hardware configuration and what payloads were tested?
Done
File src/mainboard/intel/archercity_crb/romstage.c:
https://review.coreboot.org/c/coreboot/+/71968/comment/33c4fac9_b56c1a02 PS27, Line 76: /* Disable CXL header bypass */
Redundant comment, as it does not add any more information.
Done
https://review.coreboot.org/c/coreboot/+/71968/comment/c29f550a_d7b8e6a5 PS27, Line 79: /* Set DFX CXL security level to fully trusted */
Redundant comment, as it does not add any more information.
Done
https://review.coreboot.org/c/coreboot/+/71968/comment/94d39a69_3587723b PS27, Line 83: mupd->FspmConfig.DelayAfterPCIeLinkTraining = 2000;
Maybe just add a comment `/* ms */` at the end to document the unit? […]
I think the above comment already has mentioned ms.
https://review.coreboot.org/c/coreboot/+/71968/comment/f763f6b7_a55d4ed4 PS27, Line 96: "SerialIoUartDebugEnable to %d\n", FSP_LOG, FSP_LOG_DEFAULT);
Output strings should be on one line.
Ack. Put into online would exceed 96 characters.
https://review.coreboot.org/c/coreboot/+/71968/comment/580fed7a_f0e2bc70 PS27, Line 102: /* Enable - Portions of memory reference code will be skipped */ : /* when possible to increase boot speed on warm boots.*/
Please use the comment styles from the coding style.
Done
https://review.coreboot.org/c/coreboot/+/71968/comment/a23649b8_543e0e06 PS27, Line 107: /* Set Attempt Fast Cold Boot to enable. */
Redundant.
Done
https://review.coreboot.org/c/coreboot/+/71968/comment/c8258e33_52beff0a PS27, Line 108: /* Enable - Portions of memory reference code will be skipped */ : /* when possible to increase boot speed on cold boots. */ : /* Disable - Disables this feature. */ : /* Auto - Sets it to the MRC default setting. */
Aren’t these comments in the header file?
Done
https://review.coreboot.org/c/coreboot/+/71968/comment/ed706695_3f5e0128 PS27, Line 126: /* Disable FSP memory train results*/
- Missing space at the end. […]
Done
-
Johnny Lin (Code Review)