Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33759 )
Change subject: soc/amd/picasso: Create a hybrid romstage to begin in DRAM ......................................................................
Patch Set 16: Code-Review+1
(4 comments)
A few nits.
https://review.coreboot.org/c/coreboot/+/33759/16/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/33759/16/src/soc/amd/picasso/includ... PS16, Line 21: #include <arch/cpu.h> Is this needed here for the function definitions?
https://review.coreboot.org/c/coreboot/+/33759/16/src/soc/amd/picasso/romsta... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/33759/16/src/soc/amd/picasso/romsta... PS16, Line 53: : static void romstage_soc_init(int s3_resume) : { : fch_early_init(); : } Call it directly? Is the intention to make the first_stage_entry() an architectural default with callbacks like these?
https://review.coreboot.org/c/coreboot/+/33759/16/src/soc/amd/picasso/romsta... PS16, Line 100: BIT(23) 8 * MiB would be more readable
https://review.coreboot.org/c/coreboot/+/33759/16/src/soc/amd/picasso/romsta... PS16, Line 101: backup_top_of_low_cacheable(top_of_mem); Is using this API really necessary if it's fetched from just one MSR? Note CB:36144 where you only need a romstage implementation. So if you can fetch cbmem_top easily on S3 resume there is no need for caching anything at all.
-
Arthur Heymans (Code Review)