Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
Patch Set 4:
(2 comments)
Please stop marking comments as resolved that are obviously not, it's quite offensive.
https://review.coreboot.org/c/coreboot/+/38992/1/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/38992/1/src/soc/intel/icelake/fsp_p... PS1, Line 97: params->GtFreqMax = 2; : params->CdClock = 3;
did u look at https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p...
anything else you are looking for. i'm unable to understand what details you need apart from IGD initialization and GFX PEIM to perform device initialization been mentioned here in comment.
I lack the obvious details: Your commit message states that the if conditions are redundant. Yet they are not literally the same. Hence, proving that they are redundant is more subtle.
If, for instance, the FSP code would do it this way:
if (params->PeiGraphicsPeimInit) { ... something_with_freq(params->GtFreqMax); ... something_with_cdclk(params->CdClock); ... }
and wouldn't consider `GtFreqMax` and `CdClock` otherwise. Then you were right, the conditions would be redundant.
But if, for instance, the FSP code would do something like this:
something_with_freq(params->GtFreqMax); ... if (params->PeiGraphicsPeimInit) { ... }
Then, your original assumption and this change would be completely wrong.
The question is, does FSP something with these parameters even if `PeiGraphicsPeimInit == 0`? if not, just document it please.
https://review.coreboot.org/c/coreboot/+/38992/4/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/38992/4/src/soc/intel/icelake/fsp_p... PS4, Line 97: perform IGD initialization Is all "IGD initialization" part of the "GFX PEIM module"?