Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38463 )
Change subject: mainboard/system76: Add System76 Lemur Pro (lemp9) ......................................................................
Patch Set 7:
(19 comments)
https://review.coreboot.org/c/coreboot/+/38463/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38463/6//COMMIT_MSG@29 PS6, Line 29: GOP driver is recommended, VBT is provided
assume display init works with both FSP/GOP init and a separate GOP driver in Tianocore?
Display works with FSP GOP regardless of using GOP driver in Tianocore. Performance is better also using GOP driver in Tianocore.
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/Kconfig:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 24: Fix failure to boot GRUB
Is this comment accurate?
It unfortunately is. GRUB systems do not boot correctly without the 8254 timer. Input does not work.
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi/ac.asl:
PS6:
I think we also use tabs to indent stuff in . […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 1: /*
given you are running your own EC code, is the EC ACPI actually board specific? If not, perhaps bett […]
Seeing as this is the only mainboard so far in upstream coreboot, I would prefer to do the work of generalizing the ACPI tables later on.
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 18: Scope (_SB) {
This is redundant. This file is already inside the `_SB` scope when included by `dsdt. […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi_tables.c:
PS6:
Is this blank file needed? I think it is not a requirement anymore
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 11: Enable
Or just drop the comment altogether, so that it can't rot away :)
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 20: # CPU (soc/intel/cannonlake/cpu.c)
Are these comments not indented on purpose? it looks a bit odd
These comments identify where the setting is used, which I have found helpful when things break and I need to fix up the SoC code
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 98: Used by card reader
Is it, actually? […]
The pins are used by PCIe as you have stated
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 28: // Some generic macros
Please drop this comment. I removed all instances in a recent change.
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 34: // CPU
I don't think this comment adds much value. I'd drop it.
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 36: : Scope (_SB) { : Device (PCI0) : {
The distinct placement of the open braces is odd. In any case, this can be simplified: […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 45: // Chipset specific sleep states
This was also dropped, as the sleepstates. […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 49: // PS/2 bus
Mixed tabs and spaces?
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 16: #ifndef HDA_VERB_H : #define HDA_VERB_H
Really? Header guards in a . […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 24: 0x15581401, /* Subsystem ID */
Nit: align the comments here with tabs
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 38: Kabylake
But is it Kaby Lake? […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/romstage.c:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 90: Enabled
But is it actually enabled? […]
Done
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/spd/Makefile.inc:
PS6:
Note: there's a common implementation of the "add SPD files to CBFS" code.
Is there an example of a mainboard doing this?