Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33512
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: set use of legacy 8254 timer
The Enable8254ClockGating/Enable8254ClockGatingOnS3 UPDs default to enabled, but need to be disabled on Coffeelake/Whiskeylake when SeaBIOS is used as the payload. Add a Kconfig option to set it as such.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index dac3522..19b9679 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -315,4 +315,13 @@ Setting non-zero value will allow to use DBC or DCI to debug SOC. PlatformDebugConsent in FspmUpd.h has the details.
+config USE_LEGACY_8254_TIMER + bool + default n if (SOC_INTEL_COFFEELAKE || SOC_INTEL_WHISKEYLAKE) && PAYLOAD_SEABIOS + default y + help + This sets the Enable8254ClockGating and Enable8254ClockGatingOnS3 UPDs. + On Coffeelake/Whiskeylake, these UPDs need to be disabled in order to + boot SeabIOS, but should otherwise be enabled. + endif diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index dd93882..a518541 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -244,6 +244,10 @@ /* disable Legacy PME */ memset(params->PcieRpPmSci, 0, sizeof(params->PcieRpPmSci));
+ /* Legacy 8254 timer support */ + params->Enable8254ClockGating = CONFIG_USE_LEGACY_8254_TIMER; + params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER; + /* USB */ for (i = 0; i < ARRAY_SIZE(config->usb2_ports); i++) { params->PortUsb20Enable[i] = config->usb2_ports[i].enable;
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 1:
I made a similar change a while ago here: https://review.coreboot.org/c/coreboot/+/31534
I believe it is also required for the default Tianocore build, using the 8254 timer instead of HPET.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 1:
I made a similar change a while ago here: https://review.coreboot.org/c/coreboot/+/31534
I believe it is also required for the default Tianocore build, using the 8254 timer instead of HPET.
I made this change based on Nico's comments there :) I'll double check if it's needed for Tianocore as well.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 1:
I'll double check if it's needed for Tianocore as well.
Tiano booted just fine with USE_LEGACY_8254_TIMER=y and TIANOCORE_USE_8254_TIMER=n
The FSP integration guide says "Make sure [Enable8254ClockGating] is disabled to support boot legacy OS using 8254 timer" -- so perhaps the Kconfig should be 'DISABLE_LEGACY_8254_TIMER' vs 'USE_LEGACY_8254_TIMER' or something to that effect?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 1:
(2 comments)
Should be synchronized with coreboot code in `soc/intel/cannonlake/lpc.c`. Or maybe drop the coreboot code. It seems redundant?
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/Kconfig@320 PS1, Line 320: (SOC_INTEL_COFFEELAKE || SOC_INTEL_WHISKEYLAKE) What is different on CNL/CML?
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 248: params->Enable8254ClockGating = CONFIG_USE_LEGACY_8254_TIMER; Clock gating disables things...
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 1:
Patch Set 1:
I'll double check if it's needed for Tianocore as well.
Tiano booted just fine with USE_LEGACY_8254_TIMER=y and TIANOCORE_USE_8254_TIMER=n
Cool, that's good
The FSP integration guide says "Make sure [Enable8254ClockGating] is disabled to support boot legacy OS using 8254 timer" -- so perhaps the Kconfig should be 'DISABLE_LEGACY_8254_TIMER' vs 'USE_LEGACY_8254_TIMER' or something to that effect?
Yep. I also recommend removing the if COFFEELAKE || WHISKEYLAKE condition and only relying on the SeaBIOS condition, to satisfy Nico's comment.
CometLake will also require it.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 249: params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER; This is likely not needed because SeaBIOS doesn't run on the resume path. Also, the description of this UPD mentions some "SMI requirement". This could be due to the POSTBOOT_SAI again. If that's the case the coreboot code is dead already.
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: set use of legacy 8254 timer
The Enable8254ClockGating/Enable8254ClockGatingOnS3 UPDs default to enabled, but need to be disabled on Coffeelake/Whiskeylake when SeaBIOS is used as the payload. Add a Kconfig option to set it as such.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/Kconfig@320 PS1, Line 320: (SOC_INTEL_COFFEELAKE || SOC_INTEL_WHISKEYLAKE)
What is different on CNL/CML?
Done
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 248: params->Enable8254ClockGating = CONFIG_USE_LEGACY_8254_TIMER;
Clock gating disables things...
Done
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 249: params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER;
This is likely not needed because SeaBIOS doesn't run on the resume path. […]
Done
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: set use of legacy 8254 timer
The Enable8254ClockGating UPD defaults to enabled, but needs disabled when SeaBIOS is used as the payload. Add a Kconfig option to set it as such.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/3
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 3: Code-Review+1
This looks good now
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33512/3/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/33512/3/src/soc/intel/cannonlake/Kconfig@320 PS3, Line 320: default n if PAYLOAD_SEABIOS : default y is this correct? as per your commit msg, i guess default should be "n" that mean 8254 is not disable and "y" in case of PAYLOAD_SEABIOS to know only disable for seabios payload to boot ?
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: set use of legacy 8254 timer
The Enable8254ClockGating UPD defaults to enabled, but needs to be disabled when SeaBIOS is used as the payload. Add a Kconfig option to set it as such.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/4
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33512/3/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/33512/3/src/soc/intel/cannonlake/Kconfig@320 PS3, Line 320: default n if PAYLOAD_SEABIOS : default y
is this correct? as per your commit msg, i guess default should be "n" that mean 8254 is not disable […]
nope, reversed the meaning of the Kconfig but not the logic. fixed in #4
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: set use of legacy 8254 timer
The Enable8254ClockGating UPD defaults to enabled, but needs to be disabled when SeaBIOS is used as the payload. Add a Kconfig option to set it as such.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 5: Code-Review+2
care to make the same change for ICL as well?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 5: Code-Review-1
There is still competing coreboot code in `lpc.c`.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 5: -Code-Review
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: set use of legacy 8254 timer ......................................................................
Patch Set 5: Code-Review+1
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: fix use of legacy 8254 timer
FSP sets the use of the 8254 timer via the Enable8254ClockGating UPD, which defaults to enabled, overriding what is set by coreboot. Per the FSP integration guide, this UPD needs to be disabled when a legacy OS is booted (ie, when SeaBIOS is used as the payload).
Add a Kconfig option to set the UPD properly based on payload selection, and remove the existing coreboot code in lpc.c since it is either ineffective or being overridden by FSP.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/lpc.c 3 files changed, 11 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 249: params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER;
Done
Sorry, I should have been more specific. I think we should set it, just not conditionally. As it shouldn't matter for SeaBIOS, I would have hardcoded it to `true`. But that would have to be tested, I guess. Alternatively, just add it back and ignore me ;)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/lpc.c File src/soc/intel/cannonlake/lpc.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/lpc.c@214 PS1, Line 214: clock_gate_8254 This should be removed from `chip.h` too.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 249: params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER;
Sorry, I should have been more specific. I think we should set […]
given that it defaults to 1/enabled in FSP, is there a need to explicitly set it?
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/lpc.c File src/soc/intel/cannonlake/lpc.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/lpc.c@214 PS1, Line 214: clock_gate_8254
This should be removed from `chip.h` too.
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 249: params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER;
given that it defaults to 1/enabled in FSP, is there a need to explicitly set it?
Yes, we don't know if the binary has been altered.
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: fix use of legacy 8254 timer
FSP sets the use of the 8254 timer via the Enable8254ClockGating UPD, which defaults to enabled, overriding what is set by coreboot. Per the FSP integration guide, this UPD needs to be disabled when a legacy OS is booted (ie, when SeaBIOS is used as the payload).
Add a Kconfig option to set the UPD properly based on payload selection, and remove the existing coreboot code in lpc.c since it is either ineffective or being overridden by FSP.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/lpc.c 4 files changed, 12 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/7
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 249: params->Enable8254ClockGatingOnS3 = CONFIG_USE_LEGACY_8254_TIMER;
Yes, we don't know if the binary has been altered.
Done
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/lpc.c File src/soc/intel/cannonlake/lpc.c:
https://review.coreboot.org/#/c/33512/1/src/soc/intel/cannonlake/lpc.c@214 PS1, Line 214: clock_gate_8254
Ack
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33512/7/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/33512/7/src/soc/intel/cannonlake/Kconfig@319 PS7, Line 319: bool please add a string so it's exposed to users (and can be changed through .config files). Not everybody uses the integrated payload mechanism.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33512/7/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/33512/7/src/soc/intel/cannonlake/Kconfig@319 PS7, Line 319: bool
please add a string so it's exposed to users (and can be changed through .config files). […]
Done
Hello Aaron Durbin, Subrata Banik, Jeremy Soller, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33512
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: fix use of legacy 8254 timer
FSP sets the use of the 8254 timer via the Enable8254ClockGating UPD, which defaults to enabled, overriding what is set by coreboot. Per the FSP integration guide, this UPD needs to be disabled when a legacy OS is booted (ie, when SeaBIOS is used as the payload).
Add a Kconfig option to set the UPD properly based on payload selection, and remove the existing coreboot code in lpc.c since it is either ineffective or being overridden by FSP.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/lpc.c 4 files changed, 12 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33512/8
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
Patch Set 8: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33512 )
Change subject: soc/intel/cannonlake: fix use of legacy 8254 timer ......................................................................
soc/intel/cannonlake: fix use of legacy 8254 timer
FSP sets the use of the 8254 timer via the Enable8254ClockGating UPD, which defaults to enabled, overriding what is set by coreboot. Per the FSP integration guide, this UPD needs to be disabled when a legacy OS is booted (ie, when SeaBIOS is used as the payload).
Add a Kconfig option to set the UPD properly based on payload selection, and remove the existing coreboot code in lpc.c since it is either ineffective or being overridden by FSP.
Test: build/boot out-of-tree WHL board with both SeaBIOS and Tianocore, ensure 8254 timer usage set correctly for each.
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I0e888bf754cb72093f14fc02f39bddcd6d288203 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33512 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/lpc.c 4 files changed, 12 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index 2279df2..679efce 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -314,4 +314,12 @@ Setting non-zero value will allow to use DBC or DCI to debug SOC. PlatformDebugConsent in FspmUpd.h has the details.
+config USE_LEGACY_8254_TIMER + bool "Use Legacy 8254 Timer" + default y if PAYLOAD_SEABIOS + default n + help + This sets the Enable8254ClockGating UPD, which according to the FSP Integration + guide needs to be disabled in order to boot SeaBIOS, but should otherwise be enabled. + endif diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 3b4c980..71aa208 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -246,8 +246,6 @@ /* Enable/Disable EIST. 1b:Enabled, 0b:Disabled */ uint8_t eist_enable;
- /* Statically clock gate 8254 PIT. */ - uint8_t clock_gate_8254; /* Enable C6 DRAM */ uint8_t enable_c6dram; /* diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index dd93882..783ebb7 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -244,6 +244,10 @@ /* disable Legacy PME */ memset(params->PcieRpPmSci, 0, sizeof(params->PcieRpPmSci));
+ /* Legacy 8254 timer support */ + params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER; + params->Enable8254ClockGatingOnS3 = 1; + /* USB */ for (i = 0; i < ARRAY_SIZE(config->usb2_ports); i++) { params->PortUsb20Enable[i] = config->usb2_ports[i].enable; diff --git a/src/soc/intel/cannonlake/lpc.c b/src/soc/intel/cannonlake/lpc.c index 56fefa5..6cc3451 100644 --- a/src/soc/intel/cannonlake/lpc.c +++ b/src/soc/intel/cannonlake/lpc.c @@ -207,16 +207,6 @@ outb(0x70, (1 << 7)); };
-static void clock_gate_8254(const struct device *dev) -{ - const config_t *config = dev->chip_info; - - if (!config->clock_gate_8254) - return; - - itss_clock_gate_8254(); -} - void lpc_soc_init(struct device *dev) { /* Legacy initialization */ @@ -237,7 +227,6 @@ soc_pch_pirq_init(dev); setup_i8259(); i8259_configure_irq_trigger(9, 1); - clock_gate_8254(dev); soc_mirror_dmi_pcr_io_dec(); }
-
Jeremy Soller (Code Review) -
Martin Roth (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)