Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29949 )
Change subject: mainboard/google/mistral: Add support for Mistral ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/chromeo... File src/mainboard/google/mistral/chromeos.fmd:
https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/chromeo... PS2, Line 38: RW_XBL_BUFFER_A@0x1E8000 0x4000 Please justify what you need all the non-standard sections in here for. This looks like it was copied from a very early version of Cheza code, where we later found out that a lot of it is unnecessary (e.g. this XBL thing).
Note that this is the latest FMAP for Cheza (which will plan to use, except that some section may still shrink or grow a bit): https://review.coreboot.org/c/coreboot/+/25208/62/src/mainboard/google/cheza... . I expect that Mistral should look exactly the same, except maybe without an FSG region (that one depends on whether you have an LTE modem or not).
https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/memlayo... File src/mainboard/google/mistral/memlayout.ld:
https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/memlayo... PS2, Line 16: There shouldn't be a space here (guess that's wrong in Cheza, too).
https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/romstag... File src/mainboard/google/mistral/romstage.c:
https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/romstag... PS2, Line 22: #include <arch/stages.h> In general, do not define functions until you need them (e.g. you don't need to define platform_romstage_main() yet... in fact, you don't need to add this whole file yet), and do not add headers unless some code that you're adding with this patch actually uses them.