Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
soc/intel/icelake: Avoid redundant if condition
This patch overrides "GtFreqMax" and "CdClock" FSP UPD as part of existing IGD enable if condition check itself.
Change-Id: Ie500dd5fad5cd358ea3fad4d5c0be1b0c148584b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38992/1
diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c index 448b82c..8efdb56 100644 --- a/src/soc/intel/icelake/fsp_params.c +++ b/src/soc/intel/icelake/fsp_params.c @@ -92,14 +92,16 @@ mainboard_silicon_init_params(params);
dev = pcidev_path_on_root(SA_DEVFN_IGD); - if (CONFIG(RUN_FSP_GOP) && dev && dev->enabled) + if (CONFIG(RUN_FSP_GOP) && dev && dev->enabled) { params->PeiGraphicsPeimInit = 1; - else - params->PeiGraphicsPeimInit = 0; - if (dev && dev->enabled) { params->GtFreqMax = 2; params->CdClock = 3; - } + } else + /* + * Skip IGD initialization in FSP in case device is disabled + * in the devicetree.cb. + */ + params->PeiGraphicsPeimInit = 0;
/* Unlock upper 8 bytes of RTC RAM */ params->PchLockDownRtcMemoryLock = 0;
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 1:
(2 comments)
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; So, are these two directly related to PeiGraphicsPeimInit (i.e. the GOP driver)? I can't tell because relations between FSP UPDs are still undocumented.
If that is the case, please document it. Preferably in FspsUpd.h, but if that is impossible maybe a comment here?
https://review.coreboot.org/c/coreboot/+/38992/1/src/soc/intel/icelake/fsp_p... PS1, Line 102: * in the devicetree.cb. `or GOP gfx init was disabled in the config`?
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 1:
(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 102: * in the devicetree.cb.
`or GOP gfx init was disabled in the config`?
make sense
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Aamir Bohra, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38992
to look at the new patch set (#2).
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
soc/intel/icelake: Avoid redundant if condition
This patch overrides "GtFreqMax" and "CdClock" FSP UPD as part of existing IGD enable if condition check itself.
Change-Id: Ie500dd5fad5cd358ea3fad4d5c0be1b0c148584b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c 1 file changed, 14 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38992/2
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 2:
(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;
So, are these two directly related to PeiGraphicsPeimInit (i.e. the GOP […]
Ack
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38992/2/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/38992/2/src/soc/intel/icelake/fsp_p... PS2, Line 109: * in the devicetree.cb or GFX PEIM initailization is disable in the config 'initailization' may be misspelled - perhaps 'initialization'?
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Aamir Bohra, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38992
to look at the new patch set (#3).
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
soc/intel/icelake: Avoid redundant if condition
This patch overrides "GtFreqMax" and "CdClock" FSP UPD as part of existing IGD enable if condition check itself.
Change-Id: Ie500dd5fad5cd358ea3fad4d5c0be1b0c148584b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c 1 file changed, 14 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38992/3
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 2:
(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;
Ack
Absolutely not. I feel completely ignored. Repeating the names of options in comments doesn't explain how they work together.
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 3:
(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;
Ack […]
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.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 104: /* Set CdClock Frequency */ Put the comment behind the line? In this case, I’d even remove the comment, as it’s clear from the variable name.
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 109: disable disabled
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 111: params->PeiGraphicsPeimInit = 0; If one branch in the if statement uses {}, all branches need to use it.
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Aamir Bohra, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38992
to look at the new patch set (#4).
Change subject: soc/intel/icelake: Avoid redundant if condition ......................................................................
soc/intel/icelake: Avoid redundant if condition
This patch overrides "GtFreqMax" and "CdClock" FSP UPD as part of existing IGD enable if condition check itself.
Change-Id: Ie500dd5fad5cd358ea3fad4d5c0be1b0c148584b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c 1 file changed, 18 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38992/4
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:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 104: /* Set CdClock Frequency */
Put the comment behind the line? In this case, I’d even remove the comment, as it’s clear from the v […]
Ack
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 109: disable
disabled
Ack
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 109: disable
disabled
Ack
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 111: params->PeiGraphicsPeimInit = 0;
If one branch in the if statement uses {}, all branches need to use it.
Ack
https://review.coreboot.org/c/coreboot/+/38992/3/src/soc/intel/icelake/fsp_p... PS3, Line 111: params->PeiGraphicsPeimInit = 0;
If one branch in the if statement uses {}, all branches need to use it.
Ack
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.... […]
Ack
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;
Ack
@nico, please have a look into latest patchset comment section, i hope i have provide details that i can bring out. i respect your concern and its kind off valid to understand the dependency in UPDs.
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"?
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
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/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"?
yes
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: Code-Review-1
Somebody should confirm the claim that IGD initialization is 100% part of the GOP/GFX PEIM. I was just asked not to, so somebody else will have to do it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Skip FSP-S IGD related UPD override ......................................................................
Patch Set 6:
This change is ready for review.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Skip FSP-S IGD related UPD override ......................................................................
Patch Set 6:
(2 comments)
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.... […]
finally we can get rid of this if condition and additional GtFreq and CDClk override
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
yes
Ack
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Skip FSP-S IGD related UPD override ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Skip FSP-S IGD related UPD override ......................................................................
soc/intel/icelake: Skip FSP-S IGD related UPD override
Default FSP values for "GtFreqMax" and "CdClock" UPDs are "Auto", hence related FSP-S UPD override can be avoided from coreboot.
As per FSP-S UPD Header (FspsUpd.h)
/** Offset 0x020E - GT Frequency Limit 0xFF: Auto(Default) **/ UINT8 GtFreqMax;
/** Offset 0x0209 - CdClock Frequency selection 0: (Default) Auto **/ UINT8 CdClock;
TEST=Able to get Pre-OS display on ICLRVP and Dragonegg platform.
Change-Id: Ie500dd5fad5cd358ea3fad4d5c0be1b0c148584b Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38992 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/icelake/fsp_params.c 1 file changed, 0 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified V Sowmya: Looks good to me, approved
diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c index 448b82c..7514be1 100644 --- a/src/soc/intel/icelake/fsp_params.c +++ b/src/soc/intel/icelake/fsp_params.c @@ -96,10 +96,6 @@ params->PeiGraphicsPeimInit = 1; else params->PeiGraphicsPeimInit = 0; - if (dev && dev->enabled) { - params->GtFreqMax = 2; - params->CdClock = 3; - }
/* Unlock upper 8 bytes of RTC RAM */ params->PchLockDownRtcMemoryLock = 0;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38992 )
Change subject: soc/intel/icelake: Skip FSP-S IGD related UPD override ......................................................................
Patch Set 7:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/790 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/789 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/788
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
build bot (Jenkins) (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)