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. -- To view, visit https://review.coreboot.org/c/coreboot/+/48769 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icbc18914670f87f0943b371400c509ff0eeacf6a Gerrit-Change-Number: 48769 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 21 Dec 2020 16:12:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment