Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
kbl boards / fsp2.0: remove redundant CdClock setting
CdClock for KabyLake FSP2.0 already defaults to 3. Hence, don't set it again.
Change-Id: Ie3bd7f3dc4c795691a04d2eaba0e2458ee50aabb Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/asrock/h110m/ramstage.c M src/mainboard/intel/kblrvp/ramstage.c 2 files changed, 0 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/35880/1
diff --git a/src/mainboard/asrock/h110m/ramstage.c b/src/mainboard/asrock/h110m/ramstage.c index a247b72..93b926d 100644 --- a/src/mainboard/asrock/h110m/ramstage.c +++ b/src/mainboard/asrock/h110m/ramstage.c @@ -22,6 +22,4 @@ /* Configure pads prior to SiliconInit() in case there's any * dependencies during hardware initialization. */ gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); - - params->CdClock = 3; } diff --git a/src/mainboard/intel/kblrvp/ramstage.c b/src/mainboard/intel/kblrvp/ramstage.c index a19e96e..646a4ac 100644 --- a/src/mainboard/intel/kblrvp/ramstage.c +++ b/src/mainboard/intel/kblrvp/ramstage.c @@ -24,7 +24,6 @@ /* Configure pads prior to SiliconInit() in case there's any * dependencies during hardware initialization. */ gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); - params->CdClock = 3; }
static void ioexpander_init(void *unused)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3 That's an assumption about the binary used?
I wouldn't mind if we make it the default in soc/ scope, though.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3
That's an assumption about the binary used? […]
Yes, this applies to the "default" fsp binary. Well, shouldn't we then set the default values for *all* possible parameters?
However, for SKL/KBL there is no 0=auto value like in CFL/CNL/... is 3 a value we CAN default to explicitely or SKL/KBL (I guess yes, but I'm not 100% certain)?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
(1 comment)
To be honest I don't even know why FSP has this option. Since Broadwell, CDClk selection is a responsibility of the gfx driver.
Naresh, do you remember why you explicitly set this for the KBLRVP?
It seems the line was just copied to H110M later.
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3
Yes, this applies to the "default" fsp binary. Well, shouldn't we then set the default values for *all* possible parameters?
Yes. Otherwise proper maintenance becomes a pain. Don't you feel it?
However, for SKL/KBL there is no 0=auto value like in CFL/CNL/... is 3 a value we CAN default to explicitely or SKL/KBL (I guess yes, but I'm not 100% certain)?
That's the problem. We don't know if any of the other board ports rely on a different value in the binary (not everybody is using stock, unaltered binaries). It would be easier to reason about this if we knew why the option exists at all. Considering the whole software stack (including the OS driver) this option shouldn't matter at all :-/
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
Naresh, do you remember why you explicitly set this for the KBLRVP?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3
Yes, this applies to the "default" fsp binary. Well, shouldn't we then set the default values for *all* possible parameters?
Yes. Otherwise proper maintenance becomes a pain. Don't you feel it?
Yep, definitely... and that's why I asked :S meh.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3
Yes, this applies to the "default" fsp binary. […]
I really think we should make this a rule for future platforms: memset() the UPDs to zero instead of copying them from the binary. It will increase the burden of the initial port, but save the same effort ten times per year of maintenance.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3
I really think we should make this a rule for future platforms: […]
That sounds like a really good idea... I would even like to "fix" the existing ones but this is something where we need ~20 people for help \o/
Naresh Solanki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
Patch Set 1:
Naresh, do you remember why you explicitly set this for the KBLRVP?
I'm not sure what FSP version I was using when working on this. But I guess the HW/Display needed this frequency.
For reference : FSP master:(Kaby Lake FSP 3.6.0 ) https://github.com/IntelFsp/FSP/blob/master/KabylakeFspBinPkg/Include/FspsUp...
FSP older version (Kaby Lake FSP 3.1.0) https://github.com/IntelFsp/FSP/blob/16d143d63854938fceec60b762ae1b25aa48dbf...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
Naresh, do you remember why you explicitly set this for the KBLRVP?
I'm not sure what FSP version I was using when working on this. But I guess the HW/Display needed this frequency.
For reference : FSP master:(Kaby Lake FSP 3.6.0 ) https://github.com/IntelFsp/FSP/blob/master/KabylakeFspBinPkg/Include/FspsUp...
FSP older version (Kaby Lake FSP 3.1.0) https://github.com/IntelFsp/FSP/blob/16d143d63854938fceec60b762ae1b25aa48dbf...
It's not even clear to me why this UPD exists. Is it a work- around for the GOP because it can't decide the frequency based on the display mode it sets?
If these boards need the explicit `CdClock = 3` for proper FSP/GOP operation, I would leave the code as is. Maybe with a comment, why that is necessary.
OTOH, if we'd move the `CdClock = 3` to soc/ code, we could just guard it with `if (CONFIG(USE_FSP_GOP))`. Maybe even add an `else CdClock = 0;` if we'd want to test that. It should work with any reasonable gfx driver and would save some power if we don't initialize the display early.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
Patch Set 1:
Naresh, do you remember why you explicitly set this for the KBLRVP?
I'm not sure what FSP version I was using when working on this. But I guess the HW/Display needed this frequency.
For reference : FSP master:(Kaby Lake FSP 3.6.0 ) https://github.com/IntelFsp/FSP/blob/master/KabylakeFspBinPkg/Include/FspsUp...
FSP older version (Kaby Lake FSP 3.1.0) https://github.com/IntelFsp/FSP/blob/16d143d63854938fceec60b762ae1b25aa48dbf...
It's not even clear to me why this UPD exists. Is it a work- around for the GOP because it can't decide the frequency based on the display mode it sets?
If these boards need the explicit `CdClock = 3` for proper FSP/GOP operation, I would leave the code as is. Maybe with a comment, why that is necessary.
OTOH, if we'd move the `CdClock = 3` to soc/ code, we could just guard it with `if (CONFIG(USE_FSP_GOP))`. Maybe even add an `else CdClock = 0;` if we'd want to test that. It should work with any reasonable gfx driver and would save some power if we don't initialize the display early.
sounds resonable; I will update this CB
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Naresh, do you remember why you explicitly set this for the KBLRVP?
I'm not sure what FSP version I was using when working on this. But I guess the HW/Display needed this frequency.
For reference : FSP master:(Kaby Lake FSP 3.6.0 ) https://github.com/IntelFsp/FSP/blob/master/KabylakeFspBinPkg/Include/FspsUp...
FSP older version (Kaby Lake FSP 3.1.0) https://github.com/IntelFsp/FSP/blob/16d143d63854938fceec60b762ae1b25aa48dbf...
It's not even clear to me why this UPD exists. Is it a work- around for the GOP because it can't decide the frequency based on the display mode it sets?
If these boards need the explicit `CdClock = 3` for proper FSP/GOP operation, I would leave the code as is. Maybe with a comment, why that is necessary.
OTOH, if we'd move the `CdClock = 3` to soc/ code, we could just guard it with `if (CONFIG(USE_FSP_GOP))`. Maybe even add an `else CdClock = 0;` if we'd want to test that. It should work with any reasonable gfx driver and would save some power if we don't initialize the display early.
sounds resonable; I will update this CB
\o/ I just found some similiar approch in icelake... we should use that :D
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: kbl boards / fsp2.0: remove redundant CdClock setting ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Naresh, do you remember why you explicitly set this for the KBLRVP?
I'm not sure what FSP version I was using when working on this. But I guess the HW/Display needed this frequency.
For reference : FSP master:(Kaby Lake FSP 3.6.0 ) https://github.com/IntelFsp/FSP/blob/master/KabylakeFspBinPkg/Include/FspsUp...
FSP older version (Kaby Lake FSP 3.1.0) https://github.com/IntelFsp/FSP/blob/16d143d63854938fceec60b762ae1b25aa48dbf...
It's not even clear to me why this UPD exists. Is it a work- around for the GOP because it can't decide the frequency based on the display mode it sets?
If these boards need the explicit `CdClock = 3` for proper FSP/GOP operation, I would leave the code as is. Maybe with a comment, why that is necessary.
OTOH, if we'd move the `CdClock = 3` to soc/ code, we could just guard it with `if (CONFIG(USE_FSP_GOP))`. Maybe even add an `else CdClock = 0;` if we'd want to test that. It should work with any reasonable gfx driver and would save some power if we don't initialize the display early.
sounds resonable; I will update this CB
\o/ I just found some similiar approch in icelake... we should use that :D
wtf. in icelike cdclock is only set when PeiGraphicsInitPeim is enabled, too.... I wonder if setting cdclock in skl/kbl has any effect at all currently o.O
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Uploaded patch set 2.
Hello Naresh Solanki, Kyösti Mälkki, Hung-Te Lin, Subrata Banik, Maxim Polyakov, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35880
to look at the new patch set (#2).
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init
There are mainboards that do not have any graphics ports connected to the SoC, so it would be senseless to initialize the iGD, thus skip it.
This is accomplished by the following changes: - add Kconfig option MAINBOARD_NO_FSP_GOP and select where appropriate - set FSP parameters in accordance to support of iGD and Kconfig setting - move setting of CdClock from board to soc level and set it to zero when graphics is unavailable or disabled
Change-Id: Ie3bd7f3dc4c795691a04d2eaba0e2458ee50aabb Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/Kconfig M src/mainboard/asrock/h110m/ramstage.c M src/mainboard/intel/kblrvp/ramstage.c M src/mainboard/supermicro/x10slm-f/Kconfig M src/mainboard/supermicro/x11-lga1151-series/Kconfig M src/soc/intel/apollolake/chip.c M src/soc/intel/braswell/chip.c M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/icelake/fsp_params.c M src/soc/intel/skylake/chip_fsp20.c 10 files changed, 72 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/35880/2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35880/1//COMMIT_MSG@9 PS1, Line 9: already defaults to 3
That sounds like a really good idea... […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 2:
(6 comments)
Started to review, but erm, sorry, this is a mess. There's a list in the commit message. Which is asking me to ask you to split it into separate commits.
https://review.coreboot.org/c/coreboot/+/35880/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm-f/Kconfig:
https://review.coreboot.org/c/coreboot/+/35880/2/src/mainboard/supermicro/x1... PS2, Line 22: select CPU_INTEL_HASWELL I doubt this board is using FSP at all.
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 754: #if CONFIG(HAVE_FSP_GOP) We don't use preprocessor #if's unless necessary. Also, we know that it is selected...
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 757: RUN_FSP_GOP ...and this even covers HAVE_FSP_GOP too...
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 759: params->GtFreqMax = 2; Does it exist?
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 760: params->CdClock = 3; Seems like a random choice? and what APL device needs it this high?
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 761: } else { ...and why would we not run the else path without HAVE_FSP_GOP?
Hello Naresh Solanki, Kyösti Mälkki, Patrick Rudolph, Hung-Te Lin, Subrata Banik, Tristan Corrick, Maxim Polyakov, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35880
to look at the new patch set (#3).
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init
There are mainboards that do not have any graphics ports connected to the SoC, so it would be senseless to initialize the iGD, thus skip it.
This is accomplished by the following changes: - add Kconfig option MAINBOARD_NO_FSP_GOP and select where appropriate - set FSP parameters in accordance to support of iGD and Kconfig setting - move setting of CdClock from board to soc level and set it to zero when graphics is unavailable or disabled
Change-Id: Ie3bd7f3dc4c795691a04d2eaba0e2458ee50aabb Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/Kconfig M src/mainboard/asrock/h110m/ramstage.c M src/mainboard/intel/kblrvp/ramstage.c M src/mainboard/supermicro/x11-lga1151-series/Kconfig M src/soc/intel/apollolake/chip.c M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/icelake/fsp_params.c M src/soc/intel/skylake/chip_fsp20.c 8 files changed, 53 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/35880/3
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Uploaded patch set 3.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 754: #if CONFIG(HAVE_FSP_GOP)
We don't use preprocessor #if's unless necessary. Also, we know that […]
Done
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 757: RUN_FSP_GOP
...and this even covers HAVE_FSP_GOP too...
Done
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 759: params->GtFreqMax = 2;
Does it exist?
Done
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 760: params->CdClock = 3;
Seems like a random choice? and what APL device needs it this high?
on APL the current default that is set/copied is 4: $gBroxtonFspPkgTokenSpaceGuid_CdClock 1 bytes $_DEFAULT_ = 0x04
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 761: } else {
... […]
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 3:
Patch Set 2:
(6 comments)
Started to review, but erm, sorry, this is a mess. There's a list in the commit message. Which is asking me to ask you to split it into separate commits.
The first two of the list, are the same change.....
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 2:
(6 comments)
Started to review, but erm, sorry, this is a mess. There's a list in the commit message. Which is asking me to ask you to split it into separate commits.
The first two of the list, are the same change.....
It wouldn't make sense to add a Kconfig without functionality^^
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 2:
(6 comments)
Started to review, but erm, sorry, this is a mess. There's a list in the commit message. Which is asking me to ask you to split it into separate commits.
The first two of the list, are the same change.....
It wouldn't make sense to add a Kconfig without functionality^^
... and the third change is dropping two tiny lines, that are replaced by the rest. So I really don't see a reason to split this...
Hello Naresh Solanki, Kyösti Mälkki, Patrick Rudolph, Hung-Te Lin, Subrata Banik, Tristan Corrick, Maxim Polyakov, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35880
to look at the new patch set (#4).
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init
There are mainboards that do not have any graphics ports connected to the SoC, so it would be senseless to initialize the iGD, thus skip it. This can accomplished by adding a Kconfig option MAINBOARD_NO_FSP_GOP to set FSP parameters in accordance to iGD support of the mainboard.
Change-Id: Ie3bd7f3dc4c795691a04d2eaba0e2458ee50aabb Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/Kconfig M src/mainboard/asrock/h110m/ramstage.c M src/mainboard/intel/kblrvp/ramstage.c M src/mainboard/supermicro/x11-lga1151-series/Kconfig M src/soc/intel/apollolake/chip.c M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/icelake/fsp_params.c M src/soc/intel/skylake/chip_fsp20.c 8 files changed, 47 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/35880/4
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Uploaded patch set 4.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35880/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm-f/Kconfig:
https://review.coreboot.org/c/coreboot/+/35880/2/src/mainboard/supermicro/x1... PS2, Line 22: select CPU_INTEL_HASWELL
I doubt this board is using FSP at all.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 4:
I see up to 6 commits here. In two independent groups:
* Add MAINBOARD_NO_FSP_GOP including its functionality (the depends on) * Use it for the Supermicro boards
* Move CdClock setting on Skylake (this is different from the other platforms because some boards already set it) * Adapt Icelake * Add code to Apollo Lake * Add code to Cannon Lake
I have no idea what `GtFreqMax` does and why it should depend on the GOP execution. So I can only review the first two.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 4:
Patch Set 4:
I see up to 6 commits here. In two independent groups:
Add MAINBOARD_NO_FSP_GOP including its functionality (the depends on)
Use it for the Supermicro boards
Move CdClock setting on Skylake (this is different from the other platforms because some boards already set it)
Adapt Icelake
Add code to Apollo Lake
Add code to Cannon Lake
I have no idea what `GtFreqMax` does and why it should depend on the GOP execution. So I can only review the first two.
Aggreed, will split this; GtFreqMax sets the max. graphics core frequency, see doc#335695-001US 7.87
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 4: Code-Review-1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: [RFC] src/soc/intel: FSP2.0 platforms: FSP param CdClock ......................................................................
Uploaded patch set 5.
Hello Naresh Solanki, Kyösti Mälkki, Patrick Rudolph, Hung-Te Lin, Subrata Banik, Tristan Corrick, Maxim Polyakov, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35880
to look at the new patch set (#5).
Change subject: [RFC] src/soc/intel: FSP2.0 platforms: FSP param CdClock ......................................................................
[RFC] src/soc/intel: FSP2.0 platforms: FSP param CdClock
This is not for review but is kept for discussion. Some of the changes of this CB have been split off to CB:36348 and following.
Currently CdClock is set per-board (asrock/h110m, intel/kblrvp) or kept as set in the FSP binary defaults. According to Intel's Open Source Graphics PRM [1] it must be set SoC-dependent.
I see two options: a) make CdClock (and probably GtFreqMax, too) a mainboard-specific devicetree setting. b) set it, like VR config is done, depending on the detected SoC
[1] https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-disp...
Change-Id: Ie3bd7f3dc4c795691a04d2eaba0e2458ee50aabb Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/asrock/h110m/ramstage.c M src/mainboard/intel/kblrvp/ramstage.c M src/soc/intel/apollolake/chip.c M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/skylake/chip_fsp20.c 5 files changed, 19 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/35880/5
Hello Naresh Solanki, Kyösti Mälkki, Patrick Rudolph, Hung-Te Lin, Subrata Banik, Tristan Corrick, Maxim Polyakov, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35880
to look at the new patch set (#6).
Change subject: [RFC] src/soc/intel: FSP2.0 platforms: FSP param CdClock ......................................................................
[RFC] src/soc/intel: FSP2.0 platforms: FSP param CdClock
This is not for review but is kept for discussion. Some of the changes of this CB have been split off to CB:36348 and following.
Currently CdClock is set per-board (asrock/h110m, intel/kblrvp) or kept as set in the FSP binary defaults. According to Intel's Open Source Graphics PRM [1] it must be set SoC-dependent.
I see two options: a) make CdClock (and probably GtFreqMax, too) a mainboard-specific devicetree setting. b) set it, like VR config is done, depending on the detected SoC
[1] https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-disp...
Change-Id: Ie3bd7f3dc4c795691a04d2eaba0e2458ee50aabb Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/asrock/h110m/ramstage.c M src/mainboard/intel/kblrvp/ramstage.c M src/soc/intel/apollolake/chip.c M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/skylake/chip_fsp20.c 5 files changed, 19 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/35880/6
Michael Niewöhner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: [RFC] src/soc/intel: FSP2.0 platforms: FSP param CdClock ......................................................................
Abandoned
we don't need to bother with GOP... we have an open source alternative.
-
Michael Niewöhner (Code Review) -
Naresh Solanki (Code Review)
-
Nico Huber (Code Review)