Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37516 )
Change subject: mb/facebook/monolith: Add Facebook Monolith ......................................................................
Patch Set 5:
(11 comments)
https://review.coreboot.org/c/coreboot/+/37516/5/Documentation/mainboard/fac... File Documentation/mainboard/facebook/monolith.md:
https://review.coreboot.org/c/coreboot/+/37516/5/Documentation/mainboard/fac... PS5, Line 24: The system has an external flash chip which is a 16 MiB soldered SOIC-8 chip. If it's external, why is it soldered?
https://review.coreboot.org/c/coreboot/+/37516/5/Documentation/mainboard/fac... PS5, Line 33: - hardware monitor I'd prefer if all the elements of the list started with a capital letter
https://review.coreboot.org/c/coreboot/+/37516/5/Documentation/mainboard/fac... PS5, Line 43: graphics With what? Tianocore, FSP GOP?
https://review.coreboot.org/c/coreboot/+/37516/5/Documentation/mainboard/fac... PS5, Line 50: Trailing space
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... File src/mainboard/facebook/monolith/Kconfig:
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 39: config DEVICETREE That's the default value
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... File src/mainboard/facebook/monolith/acpi/dptf.asl:
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 25: Why three spaces?
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... File src/mainboard/facebook/monolith/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 3: # Enable deep Sx states Are you sure?
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 27: # Enable DPTF Are you sure? Also, why do you have ACPI code for DPTF if this is disabled?
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 35: #register "ScsSdCardEnabled" = "2" Commented-out code
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 69: 0x02 These should be decimal constants for clarity
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 94: register "domain_vr_config[VR_SYSTEM_AGENT]" = "{ Please don't use hex values here. The voltage limit, when written as decimal, is the value in millivolts.