David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29371 )
Change subject: drivers/intel/fsp1_1/raminit.c: Always check FSP HOBs ......................................................................
Patch Set 9:
(3 comments)
I don't work with this code enough to have a sense of how useful those prints are, so please take my suggestion with a grain of salt.
https://review.coreboot.org/#/c/29371/9/src/drivers/intel/fsp1_1/raminit.c File src/drivers/intel/fsp1_1/raminit.c:
https://review.coreboot.org/#/c/29371/9/src/drivers/intel/fsp1_1/raminit.c@a... PS9, Line 209: Is it be worth preserving these prints in a static display_hob_info() function like in ramstage.c?
https://review.coreboot.org/#/c/29371/9/src/drivers/intel/fsp1_1/raminit.c@a... PS9, Line 224: Same comment as above - Perhaps it's worth preserving these in a display_hob_info() function that gets called if CONFIG(DISPLAY_HOBS) is true.
https://review.coreboot.org/#/c/29371/9/src/drivers/intel/fsp1_1/ramstage.c File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/#/c/29371/9/src/drivers/intel/fsp1_1/ramstage.c@... PS9, Line 63: if (CONFIG(DISPLAY_HOBS)) It feels awkward to call display_hob_info() and check CONFIG(DISPLAY_HOBS) multiple times. Why would we call this function if we don't want to display HOB info?
How about checking CONFIG(DISPLAY_HOBS) on line 143 before calling this function, and remove the checks from here? I think that makes the code cleaner.
-
David Hendricks (Code Review)