Subrata Banik 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:
(1 comment)
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.
Let me be frank, i guess rather reviewing this simple code where unnecessary double if statement has written to do the same FSP-S UPD assignment, you have started sniffing into FSP-S code, i respect your intention and i'm here not the explain what FSP-S code does as this is not the place. Rather i have provided information what is required to review this code and provide details to understand how this FSP-S UPDs are related.
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) { ... }
i don't think those details are even relevant for this review about how FSP inside works as i have explained those UPDs are dedicated for IGD initialization and not relevant when FSP GOP is not selected or device tree.cb has make IGD off, I don't understand how this code is different than original code ? it just avoided redundant check
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.
i don't think we have discussed anywhere to document the dependency of UPD so far. relevant comments been added already to expain what is the expectation for those UPD overrides