Kyösti Mälkki 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 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/Kconfig... PS5, Line 88: uncompressed size of romstage. Fix comment. I see no reason why these two should be visible options.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/Kconfig... PS5, Line 99: and not usable by the OS. Is romstage executed on S3 resume path? Comment of marking regions reserved also applies to where romstage program resides.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/exit_ca... File src/soc/amd/picasso/exit_car.S:
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/exit_ca... PS5, Line 23: * this file can be removed. Little reason to drag along TODOs about postcar-stage. Switch POSTCAR_STAGE to NO_CAR_GLOBAL_MIGRATION in Kconfig, call run_ramstage() instead of postcar_frame_xxx() at end of romstage and you should be done without need to add this file.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... File src/soc/amd/picasso/reset_vector.S:
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... PS5, Line 145: and $0xfffffff0, %esp Need 'subl $4, %esp' here, as you push 12 bytes below.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... PS5, Line 154: call hybrid_romstage_entry Thanks, much cleaner this way.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... PS5, Line 165: _start: IMHO _start should come after _start16bit in execution flow. This could be _reset. Again, I leave it to someone else to decide if it matters.
-
Kyösti Mälkki (Code Review)