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 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37516/7/src/mainboard/facebook/mono... File src/mainboard/facebook/monolith/onboard.h:
https://review.coreboot.org/c/coreboot/+/37516/7/src/mainboard/facebook/mono... PS7, Line 22: #define ITE8528_DATA_PORT 0x6F
off-topic: I can see these are defined the same way for fbg1701 and portwell/m107, and google/jecht […]
That is clear, originally these values were used in more instances in the board directory. It does make sense to either get these values from the device tree or define them on another level if they are needed somewhere else as well.
https://review.coreboot.org/c/coreboot/+/37516/7/src/mainboard/facebook/mono... File src/mainboard/facebook/monolith/spd/spd.h:
https://review.coreboot.org/c/coreboot/+/37516/7/src/mainboard/facebook/mono... PS7, Line 25: void mainboard_fill_rcomp_strength_data(void *rcomp_strength_ptr);
Looks like 4th or 5th copy of these prototypes in the tree, maybe have a look if there is need to de […]
You can't do this at the FSP 1.1 or 2.0 level. This is something that should be handled at the SoC level. The cannon_lake has a different approach that I think is better. Please have look.