Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38463 )
Change subject: src/mainboard/system76: Add System76 Lemur Pro (lemp9) ......................................................................
Patch Set 5:
(5 comments)
Thanks for the review, Matt. I will address your comments soon.
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/Kconfig:
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... PS5, Line 29: config VARIANT_DIR : string : default "lemp9" if BOARD_SYSTEM76_LEMP9
reason for this when there's no variant/baseboard setup? coming later?
The VARIANT_DIR is used by default to determine the path of the VBT. I suppose I could instead modify the VBT path
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... PS5, Line 97: #config DRIVER_TPM_SPI_BUS : # hex : # default 0x0 : : #config DRIVER_TPM_SPI_CHIP : # int : # default 2
same here
SPI chip select 2 is unsupported using the intel fast SPI driver. When support is added, this can be uncommented. I could remove it for now or add a comment explaining this
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... PS5, Line 2: ramstage-y += ramstage.c hda_verb.c
I think typically these are put on separate lines for readability
Will fix
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/romstage.c:
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... PS5, Line 19: //TODO: find correct values
still true?
It's all figured out now, I will remove
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38463/5/src/mainboard/system76/lemp... PS5, Line 1: ##
this device uses fixed RAM vs sodimms?
Actually both. It has soldered RAM for one channel and a slot for the other channel