Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37516 )
Change subject: mb/facebook/monolith: Add Facebook Monolith ......................................................................
Patch Set 6:
(7 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?
The external part only refers to programming using an external programmer. It doesn't refer to the location or type of the flash device itself. In this case both flashes are soldered down SOIC devices.
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
Done
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?
Yes I verified it and it is correct for this board.
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?
We have disabled this for now as we haven't implemented the Embedded Controller support yet to access the temperature sensors. This will be done in the next stage.
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 35: #register "ScsSdCardEnabled" = "2"
Commented-out code
Overlooked when upstreaming, removed it.
https://review.coreboot.org/c/coreboot/+/37516/5/src/mainboard/facebook/mono... PS5, Line 69: 0x02
These should be decimal constants for clarity
You are right I corrected this. I used the Intel Kaby Lake RVP as a base and used the same style. I will submit a patch for that Intel RVP later as this is the only one still using hex values.
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. […]
Done