Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48769 )
Change subject: mb/yanling: Add Yanling YL-KBR6L mainboard + doc ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG@9 PS1, Line 9: Yanling YL-KBR6L Do you have an URL for that? (I know, some folks do not like URLs in commit messages.)
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG@15 PS1, Line 15: SeaBIOS, Linux and FreeBSD For the record, can you please also note down the used version of these?
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG@17 PS1, Line 17: VGA Option ROM Extracted how?
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG@18 PS1, Line 18: flashrom What version?
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG@25 PS1, Line 25: Crucial CT2K32G4SFD8266 Is that one DIMM with 64 GB?
https://review.coreboot.org/c/coreboot/+/48769/1//COMMIT_MSG@32 PS1, Line 32: Removed Present tense: Remove
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... File Documentation/mainboard/yanling/yl-kbr6l.md:
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 3: Protectli FW6 Please link to the page.
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 33: VGA Option ROM is not required to boot, but if one needs graphics in pre-OS : stage, it should be included (if not using libgfxinit). Maybe:
Graphics initialization is not required in firmware for graphics in the OS. But for graphics in pre-OS stage (payload), libgfxinit or the VGA Option ROM are required.
Is the GOP driver also supported (by FSP)?
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 40: The main SPI flash can be accessed using [flashrom]. Maybe clarify, that this also applies to the vendor firmware.
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 56: - assume the same as for Protectli FW6: Remove the “bullet point”?
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 63: return returns
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 64: or : temporarily disconnect or *to* temporarily disconnect
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 66: dim What does that mean?
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 70: - assume the same as for Protectli FW6: Remove the “bullet point”?
https://review.coreboot.org/c/coreboot/+/48769/1/Documentation/mainboard/yan... PS1, Line 133: [Yanling N18]:https://www.ylipc.com/product/network_server_network_server/N18_Firewall_Min... Please add a space after the colon.