Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34423 )
Change subject: soc/amd/picasso: Add FSP support for including AGESA ......................................................................
Patch Set 21:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34423/21/src/soc/amd/picasso/romsta... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/34423/21/src/soc/amd/picasso/romsta... PS21, Line 39: clear_agesa_mtrrs Wouldn't it be more clear if MTRR's are cleared in one function and restored (to some extend) in a different one?
https://review.coreboot.org/c/coreboot/+/34423/21/src/soc/amd/picasso/romsta... PS21, Line 83: TODO: make AGESA leave the MTRRs alone Looks like this has been a problem ever since AGESA has been integrated in coreboot...
https://review.coreboot.org/c/coreboot/+/34423/21/src/soc/amd/picasso/romsta... PS21, Line 91: mem_top - 16 * MiB needs to be aligned to its size.
https://review.coreboot.org/c/coreboot/+/34423/21/src/soc/amd/picasso/romsta... PS21, Line 103: tseg_base needs to be aligned to its size.
https://review.coreboot.org/c/coreboot/+/34423/21/src/soc/amd/picasso/romsta... PS21, Line 120: config != NULL Die earlier if that is not the case? That allows to reduce the indentation.