Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30360 )
Change subject: src/mainboard/libretrend/lt1000: Initial commit ......................................................................
Patch Set 13:
(18 comments)
Nice, thank you for upstreaming this.
Already use SPDX headers?
https://review.coreboot.org/c/coreboot/+/30360/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30360/13//COMMIT_MSG@7 PS13, Line 7: src/mainboard/libretrend/lt1000: Initial commit Please use just `mb/libretrend/lt1000:`, and use a statement by adding a verb in imperative mood.
mb/libretrend/lt1000: Do initial commit
or
mb/libretrend: Add KBL based Libretrend LT1000 (Librebox)
https://review.coreboot.org/c/coreboot/+/30360/13//COMMIT_MSG@8 PS13, Line 8: Please add a note, how you created that board.
Note, that Libretrend contracted(?) the work?
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... File Documentation/mainboard/libretrend/lt1000.md:
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 23: Kabylake Kaby Lake
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 25: *3rdparty/fsp* submodule. Mark up as code `3rdparty/fsp`.
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 27: microcode Microcode updates are …
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 28: from the *3rdparty/intel-microcode* submodule. Mark up as code/monospace.
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 42: , whose datasheet : can be found [here][W25Q64FV] Just do [datasheet][W25Q64FV].
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 47: Fastboot What is that?
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 1: /* Looks like a lot of boards use that, so it should be put in some common location.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 21: Name (PBST, One) What is this good for?
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/bootblock.c:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 42: */ What about port 2?
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/mainboard.c:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 24: {0} Please add spaces around the 0.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 47: MAX_SERIAL_LENGTH); Fits in 96 characters.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/ramstage.c:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 24: * Remove.
https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/romstage.c:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 38: /* DQS CPU<>DRAM map https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 50: /* Rcomp resistor */ Unneeded comment.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 53: sizeof(RcompResistor)); One line.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 77: */ https://doc.coreboot.org/coding_style.html#commenting