Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30360 )
Change subject: mb/libretrend/lt1000: Add Libretrend LT1000 board support ......................................................................
Patch Set 14:
(29 comments)
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. […]
Done
https://review.coreboot.org/c/coreboot/+/30360/13//COMMIT_MSG@8 PS13, Line 8:
Please add a note, how you created that board. […]
Yes it was contracted, but only partially. Upstreaming this is my/3mdeb own initiative.
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
Done
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 25: *3rdparty/fsp* submodule.
Mark up as code `3rdparty/fsp`.
Done
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 27: microcode
Microcode updates are …
Done
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.
Done
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].
Done
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 61: onboard
Capitalize this: "Onboard"
Done
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 67: InfraRed
infrared
Done
https://review.coreboot.org/c/coreboot/+/30360/13/Documentation/mainboard/li... PS13, Line 102: Super I/O, EC
Kind of. It is Super I/O with integrated Environmental Controller (EC). […]
Removed EC.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/Kconfig:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 11: select MAINBOARD_HAS_LPC_TPM : select INTEL_GMA_HAVE_VBT : select MAINBOARD_HAS_LIBGFXINIT
any reason not to alphabetize these?
Done
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: /*
IIRC there was a discussion some time ago about correctness of adding power button in ACPI. […]
Noticed some board also have LIDs and sleep buttons. Some mainboards do not have power button (physical button) so I wonder if it should be included in common location.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 21: Name (PBST, One)
Probably nothing. Copy-paste error.
Removed.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/acpi_tables.c:
PS13:
Definitely.
Done
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 38: devictree
devicetree
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 41: devictree
here too
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 65: 3
There's an enum for this
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 28: // Some generic macros
This comment got dropped
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 34: // CPU
This comment is not useful
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 37: Scope (_SB) { : Device (PCI0)
Device (_SB. […]
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 45: // Chipset specific sleep states
This got dropped as well.
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... File src/mainboard/libretrend/lt1000/mainboard.c:
PS13:
this can be dropped/replaced by selection of DRIVERS_GENERIC_CBFS_SERIAL in Kconfig
dropped as it is not needed
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 24: {0}
Please add spaces around the 0.
File was removed.
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 47: MAX_SERIAL_LENGTH);
Fits in 96 characters.
File was removed.
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. […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 50: /* Rcomp resistor */
Unneeded comment.
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 53: sizeof(RcompResistor));
One line.
Done
https://review.coreboot.org/c/coreboot/+/30360/13/src/mainboard/libretrend/l... PS13, Line 77: */
Done