Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset.
TEST=Verified on hatch. Yet to verify on Puff
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h 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/40/42440/1
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 4b48a21..dca9339 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -312,6 +312,15 @@ uint8_t PchPmSlpAMinAssert;
/* + * Reset Power Cycle Duration + * 1 = 1sec + * 2 = 2sec + * 3 = 3sec + * 4 = 4sec (default) + */ + uint8_t PchPmPwrCycDur; + + /* * SerialIO device mode selection: * * Device index: diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index a3b5588..3eb0dfb 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -411,6 +411,8 @@ params->PchPmSlpSusMinAssert = config->PchPmSlpSusMinAssert; if (config->PchPmSlpAMinAssert) params->PchPmSlpAMinAssert = config->PchPmSlpAMinAssert; + if (config->PchPmPwrCycDur) + params->PchPmPwrCycDur = config->PchPmPwrCycDur;
/* Set TccActivationOffset */ tconfig->TccActivationOffset = config->tcc_offset;
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#2).
Change subject: soc/intel: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset.
TEST=Verified on hatch.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h 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/40/42440/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42440/2//COMMIT_MSG@7 PS2, Line 7: soc/intel soc/intel/cannonlake
Hello build bot (Jenkins), Jamie Ryu, Furquan Shaikh, Patrick Georgi, Rizwan Qureshi, Tim Wawrzynczak, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset.
TEST=Verified on hatch.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h 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/40/42440/3
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42440/2//COMMIT_MSG@7 PS2, Line 7: soc/intel
soc/intel/cannonlake
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42440/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42440/3//COMMIT_MSG@9 PS3, Line 9: Add PchPmPwrCycDur to chip options to control the UPD : FSPS PchPmPwrCycDur from devicetree. The UPD determines the : minimum time a platform will stay in reset during host partition : reset with power cycle or global reset. Please re-flow for 75 characters per line.
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... PS3, Line 319: * 4 = 4sec (default) SI unit is *s*.
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... PS3, Line 321: uint8_t PchPmPwrCycDur; `src/vendorcode/intel/fsp/fsp2_0/cannonlake/FspsUpd.h` says that 0 is the default.
``` /** Offset 0x068A - PCH Pm Reset Power Cycle Duration Could be customized in the unit of second. Please refer to EDS for all support settings. 0 is default, 1 is 1 second, 2 is 2 seconds, ... **/ UINT8 PchPmPwrCycDur; ```
Also, all non-negative integers could be used?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... PS3, Line 321: uint8_t PchPmPwrCycDur;
`src/vendorcode/intel/fsp/fsp2_0/cannonlake/FspsUpd.h` says that 0 is the default. […]
As per EDS, only few values are really supported. I believe the default here means that if this UPD is left as 0, then FSP configures it as 4 seconds by default.
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur You will need a check to ensure that this is not lesser than the other assertion widths as recommended by the EDS.
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Rizwan Qureshi, Tim Wawrzynczak, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset.
TEST=Verified on hatch.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h 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/40/42440/4
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42440/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42440/3//COMMIT_MSG@9 PS3, Line 9: Add PchPmPwrCycDur to chip options to control the UPD : FSPS PchPmPwrCycDur from devicetree. The UPD determines the : minimum time a platform will stay in reset during host partition : reset with power cycle or global reset.
Please re-flow for 75 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... PS3, Line 319: * 4 = 4sec (default)
SI unit is *s*.
Ack
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... PS3, Line 321: uint8_t PchPmPwrCycDur;
`src/vendorcode/intel/fsp/fsp2_0/cannonlake/FspsUpd.h` says that 0 is the default. […]
@Furquan, your comments are correct.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
You will need a check to ensure that this is not lesser than the other assertion widths as recommend […]
suggestion: if config->PchPmPwrCycDur is less than any of the MinAssert times, set it its max (4 I think) and print out a message saying it was extended and why.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/4/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42440/4/src/soc/intel/cannonlake/ch... PS4, Line 316: * 1 = 1s maybe add a note that 0 = FSP will program the default 0 ?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
suggestion: if config->PchPmPwrCycDur is less than any of the MinAssert times, set it its max (4 I t […]
As designated Pin signal assertion width values are static, so I prefer add a note for PchPmPwrCycDur rather checking each signals width (as specified in the PCH) to determine the higher assertion width. Thoughts?
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset.
TEST=Verified on hatch.
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/42440/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
As designated Pin signal assertion width values are static, so I prefer add a note for PchPmPwrCycDu […]
What happens when the power cycle duration is less than any of the others?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
As designated Pin signal assertion width values are static, so I prefer add a note for PchPmPwrCycDu […]
I think its safer to have a sanity check since it is clearly in violation of the EDS if the PwrCycDur is less than any of the assert times. It can potentially lead to difficult-to-debug problems.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
I think its safer to have a sanity check since it is clearly in violation of the EDS if the PwrCycDu […]
Thing is, supported minimum assertion duration list of a signal varies from micro seonds to seconds. For example, if we see PchPmSlpS3MinAssert's supported min. assertion duration list : /* slp_s3_asst_dur_list: 50ms, 60us, 1ms, 50ms (Default), 2s */ slp_s3_asst_dur_list[] = { 0.05, 0.00006, 0.001, 0.05, 2}; Note: Index value points to PchPmSlpS3MinAssert UPD value
So, we have to normalize the assertion duration of these signal's duration list into one common unit(either seconds or usec) to find higher assertion width. This translates list values into floating points if we convert lists into seconds. Othewise bigger number if we represent durations into useconds. So, that's why I prefer documenting while documenting EDS recommendation.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42440/4/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42440/4/src/soc/intel/cannonlake/ch... PS4, Line 316: * 1 = 1s
maybe add a note that 0 = FSP will program the default 0 ?
Done
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/ch... PS3, Line 321: uint8_t PchPmPwrCycDur;
@Furquan, your comments are correct.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
Thing is, supported minimum assertion duration list of a signal varies from micro seonds to seconds. […]
I think this would be a good chance to convert these all into enums then.
Something like: enum SlpS3AssertDuration { SlpS3_50ms, SlpS3_60us, SlpS3_1ms, ... etc. };
same for the others.
Not necessarily in this change, but it would make these easier to read.
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset.
TEST=Verified on Hatch and Puff boards
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/42440/8
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
I think this would be a good chance to convert these all into enums then. […]
@Furquan, @Tim, Are you fine with just adding EDS documentation of PwrCyCDur requirement to the corresponding CB code? Please let me know.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
@Furquan, @Tim, Are you fine with just adding EDS documentation of PwrCyCDur requirement to the corr […]
I understand they've never been there before, but it is nice to have checks against "illegal" configurations 😊
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset. This patch also ensures configured PchPmPwrCycDur value doesn't violate the PCH EDS specification.
TEST=Verified on Hatch and Puff boards
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/42440/9
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/3/src/soc/intel/cannonlake/fs... PS3, Line 415: PchPmPwrCycDur
I understand they've never been there before, but it is nice to have checks against "illegal" config […]
Done 😊
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 51: asst to my eyes, at a glance this reads like "assistant"
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 108: nit: extra space
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 111: /* slp_s4_asst_dur_list : 1s, 1s, 2s, 3s, 4s(Default) */ : enum min_asst_dur slp_s4_asst_dur_list[] = { : MinAsstDur1s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : }; : : /* slp_s3_asst_dur_list: 50ms, 60us, 50ms (Default), 2s */ : enum min_asst_dur slp_s3_asst_dur_list[] = { : MinAsstDur50ms, MinAsstDur60us, MinAsstDur50ms, MinAsstDur2s : }; : : /* slp_a_asst_dur_list: 2s, 0s, 4s, 98ms, 2s(Default) */ : enum min_asst_dur slp_a_asst_dur_list[] = { : MinAsstDur2s, MinAsstDur0s, MinAsstDur4s, MinAsstDur98ms, MinAsstDur2s : }; : : /* pm_pwr_cyc_dur_list: 4s(Default), 1s, 2s, 3s, 4s */ : enum min_asst_dur pm_pwr_cyc_dur_list[] = { : MinAsstDur4s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : }; Kind of a bummer that the default often has to be repeated twice, but that would also require changes to all of the devictree entries too...
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 536: if (config->PchPmPwrCycDur) So this is basically turning PchPmPwrCycDur into a boolean? So if selected, it will set the minimum possible assertion width, otherwise it won't program anything?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 111: /* slp_s4_asst_dur_list : 1s, 1s, 2s, 3s, 4s(Default) */ : enum min_asst_dur slp_s4_asst_dur_list[] = { : MinAsstDur1s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : }; : : /* slp_s3_asst_dur_list: 50ms, 60us, 50ms (Default), 2s */ : enum min_asst_dur slp_s3_asst_dur_list[] = { : MinAsstDur50ms, MinAsstDur60us, MinAsstDur50ms, MinAsstDur2s : }; : : /* slp_a_asst_dur_list: 2s, 0s, 4s, 98ms, 2s(Default) */ : enum min_asst_dur slp_a_asst_dur_list[] = { : MinAsstDur2s, MinAsstDur0s, MinAsstDur4s, MinAsstDur98ms, MinAsstDur2s : }; : : /* pm_pwr_cyc_dur_list: 4s(Default), 1s, 2s, 3s, 4s */ : enum min_asst_dur pm_pwr_cyc_dur_list[] = { : MinAsstDur4s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : };
Kind of a bummer that the default often has to be repeated twice, but that would also require change […]
The list contains values which are translation of devicetree values. The lists are defined in accordance with FSP encoding. If user doesn't define a minimum assertion value for signals (SLP_S3/SLP_A/SLP_S4), then these variables are set to 0. For easy implementation, I set default value at 0th index. As said, these lists have been defined inline with FSP encoding.So, no change is required to all of the devicetree entries.
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 536: if (config->PchPmPwrCycDur)
So this is basically turning PchPmPwrCycDur into a boolean? So if selected, it will set the minimum […]
If PchPmPwrCycDur is configured (or not at all configured) to 0 , then FSP sets the default value(4s). The assertion duration 4s is highest assertion duration for any signal, hence we don't need to check the PCH violation .
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 99: n cfg_asst_dur-> can we do like below
max_wdth = slp_s4 if (max_wdth < slps3) max_wdth = slps3 if (max_wdth < slpsa) max_wdth = slpsa
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 111: /* slp_s4_asst_dur_list : 1s, 1s, 2s, 3s, 4s(Default) */ : enum min_asst_dur slp_s4_asst_dur_list[] = { : MinAsstDur1s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : }; : : /* slp_s3_asst_dur_list: 50ms, 60us, 50ms (Default), 2s */ : enum min_asst_dur slp_s3_asst_dur_list[] = { : MinAsstDur50ms, MinAsstDur60us, MinAsstDur50ms, MinAsstDur2s : }; : : /* slp_a_asst_dur_list: 2s, 0s, 4s, 98ms, 2s(Default) */ : enum min_asst_dur slp_a_asst_dur_list[] = { : MinAsstDur2s, MinAsstDur0s, MinAsstDur4s, MinAsstDur98ms, MinAsstDur2s : }; : : /* pm_pwr_cyc_dur_list: 4s(Default), 1s, 2s, 3s, 4s */ : enum min_asst_dur pm_pwr_cyc_dur_list[] = { : MinAsstDur4s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : };
The list contains values which are translation of devicetree values. […]
Please add this as a comment in the code
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Rizwan Qureshi, Tim Wawrzynczak, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset. This patch also ensures configured PchPmPwrCycDur value doesn't violate the PCH EDS specification.
TEST=Verified on Hatch and Puff boards
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 148 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/42440/10
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 51: asst
to my eyes, at a glance this reads like "assistant"
:) Yes, I renamed it.
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 99: n cfg_asst_dur->
can we do like below […]
Ack
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 108:
nit: extra space
Ack
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 111: /* slp_s4_asst_dur_list : 1s, 1s, 2s, 3s, 4s(Default) */ : enum min_asst_dur slp_s4_asst_dur_list[] = { : MinAsstDur1s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : }; : : /* slp_s3_asst_dur_list: 50ms, 60us, 50ms (Default), 2s */ : enum min_asst_dur slp_s3_asst_dur_list[] = { : MinAsstDur50ms, MinAsstDur60us, MinAsstDur50ms, MinAsstDur2s : }; : : /* slp_a_asst_dur_list: 2s, 0s, 4s, 98ms, 2s(Default) */ : enum min_asst_dur slp_a_asst_dur_list[] = { : MinAsstDur2s, MinAsstDur0s, MinAsstDur4s, MinAsstDur98ms, MinAsstDur2s : }; : : /* pm_pwr_cyc_dur_list: 4s(Default), 1s, 2s, 3s, 4s */ : enum min_asst_dur pm_pwr_cyc_dur_list[] = { : MinAsstDur4s, MinAsstDur1s, MinAsstDur2s, MinAsstDur3s, MinAsstDur4s : };
Please add this as a comment in the code
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 10:
(11 comments)
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 37: MinAssrtDur0s = 0, : MinAssrtDur60us = 60, : MinAssrtDur1ms = 1000, : MinAssrtDur50ms = 50000, : MinAssrtDur98ms = 98000, : MinAssrtDur500ms = 500000, : MinAssrtDur1s = 1000000, : MinAssrtDur2s = 2000000, : MinAssrtDur3s = 3000000, : MinAssrtDur4s = 4000000, I think you need another tab before `=` on everything that's not 500ms
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 93: struct const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 110: extra blank line
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 116: enum min_assrt_dur slp_s4_asst_dur_list[] = { const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 121: enum const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 126: enum const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 131: enum const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 155: .slp_a = MinAssrtDur2s, : .slp_s4 = MinAssrtDur4s, : .slp_s3 = MinAssrtDur50ms, : .pm_pwr_cyc_dur = MinAssrtDur4s line up the right side, please
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164: nit: register-encoded
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164: Converts register encoded values into assertion durations(in microseconds) and get : * the configured values. You could just say something like "Convert assertion durations from register-encoded to microseconds."
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 177: "Set PmPwrCycDur to 4s as given PmPwrCycDur(%d) violates PCH EDS spec\n", this line is 1 char too wide
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 10:
also thank you Sridhar for adding the checks 😊
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset. This patch also ensures configured PchPmPwrCycDur value doesn't violate the PCH EDS specification.
TEST=Verified on Hatch and Puff boards
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/42440/11
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Patrick Georgi, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Sowmya V, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42440
to look at the new patch set (#12).
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset. This patch also ensures configured PchPmPwrCycDur value doesn't violate the PCH EDS specification.
TEST=Verified on Hatch and Puff boards
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/42440/12
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 12:
(12 comments)
Patch Set 10:
also thank you Sridhar for adding the checks 😊
Thanks for review 😊 😊
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 536: if (config->PchPmPwrCycDur)
If PchPmPwrCycDur is configured (or not at all configured) to 0 , then FSP sets the default value(4s […]
Done
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 37: MinAssrtDur0s = 0, : MinAssrtDur60us = 60, : MinAssrtDur1ms = 1000, : MinAssrtDur50ms = 50000, : MinAssrtDur98ms = 98000, : MinAssrtDur500ms = 500000, : MinAssrtDur1s = 1000000, : MinAssrtDur2s = 2000000, : MinAssrtDur3s = 3000000, : MinAssrtDur4s = 4000000,
I think you need another tab before `=` on everything that's not 500ms
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 93: struct
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 110:
extra blank line
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 116: enum min_assrt_dur slp_s4_asst_dur_list[] = {
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 121: enum
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 126: enum
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 131: enum
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 155: .slp_a = MinAssrtDur2s, : .slp_s4 = MinAssrtDur4s, : .slp_s3 = MinAssrtDur50ms, : .pm_pwr_cyc_dur = MinAssrtDur4s
line up the right side, please
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164:
nit: register-encoded
Done
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164: Converts register encoded values into assertion durations(in microseconds) and get : * the configured values.
You could just say something like "Convert assertion durations from register-encoded to microseconds […]
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 177: "Set PmPwrCycDur to 4s as given PmPwrCycDur(%d) violates PCH EDS spec\n",
this line is 1 char too wide
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 12: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
soc/intel/cannonlake: Add PchPmPwrCycDur to chip options
Add PchPmPwrCycDur to chip options to control the UPD FSPS PchPmPwrCycDur from devicetree. The UPD determines the minimum time a platform will stay in reset during host partition reset with power cycle or global reset. This patch also ensures configured PchPmPwrCycDur value doesn't violate the PCH EDS specification.
TEST=Verified on Hatch and Puff boards
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I55e836c78fab34e34d57b04428a1498b7dc7174b Reviewed-on: https://review.coreboot.org/c/coreboot/+/42440 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 144 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 3ebbc5e..57922e1 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -315,6 +315,23 @@ uint8_t PchPmSlpAMinAssert;
/* + * PCH PM Reset Power Cycle Duration + * 0 = 4s + * 1 = 1s + * 2 = 2s + * 3 = 3s + * 4 = 4s (default) + * + * NOTE: Duration programmed in the PchPmPwrCycDur should never be smaller than the + * stretch duration programmed in the following registers - + * - GEN_PMCON_A.SLP_S3_MIN_ASST_WDTH (PchPmSlpS3MinAssert) + * - GEN_PMCON_A.S4MAW (PchPmSlpS4MinAssert) + * - PM_CFG.SLP_A_MIN_ASST_WDTH (PchPmSlpAMinAssert) + * - PM_CFG.SLP_LAN_MIN_ASST_WDTH + */ + uint8_t PchPmPwrCycDur; + + /* * SerialIO device mode selection: * * Device index: diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index a3b5588..bd2d7fc 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -32,6 +32,39 @@ PCH_DEVFN_UART2 };
+/* List of Minimum Assertion durations in microseconds */ +enum min_assrt_dur { + MinAssrtDur0s = 0, + MinAssrtDur60us = 60, + MinAssrtDur1ms = 1000, + MinAssrtDur50ms = 50000, + MinAssrtDur98ms = 98000, + MinAssrtDur500ms = 500000, + MinAssrtDur1s = 1000000, + MinAssrtDur2s = 2000000, + MinAssrtDur3s = 3000000, + MinAssrtDur4s = 4000000, +}; + + +/* Signal Assertion duration values */ +struct cfg_assrt_dur { + /* Minimum assertion duration of SLP_A signal */ + enum min_assrt_dur slp_a; + + /* Minimum assertion duration of SLP_4 signal */ + enum min_assrt_dur slp_s4; + + /* Minimum assertion duration of SLP_3 signal */ + enum min_assrt_dur slp_s3; + + /* PCH PM Power Cycle duration */ + enum min_assrt_dur pm_pwr_cyc_dur; +}; + +/* Default value of PchPmPwrCycDur */ +#define PCH_PM_PWR_CYC_DUR 4 + /* * Given an enum for PCH_SERIAL_IO_MODE, 1 needs to be subtracted to get the FSP * UPD expected value for Serial IO since valid enum index starts from 1. @@ -57,6 +90,93 @@ }
#if CONFIG(SOC_INTEL_COMETLAKE) +static enum min_assrt_dur get_high_asst_width(const struct cfg_assrt_dur *cfg_assrt_dur) +{ + enum min_assrt_dur max_assert_dur = cfg_assrt_dur->slp_s4; + + if (max_assert_dur < cfg_assrt_dur->slp_s3) + max_assert_dur = cfg_assrt_dur->slp_s3; + + if (max_assert_dur < cfg_assrt_dur->slp_a) + max_assert_dur = cfg_assrt_dur->slp_a; + + return max_assert_dur; +} + +static void get_min_assrt_dur(uint8_t slp_s4_min_asst, uint8_t slp_s3_min_asst, + uint8_t slp_a_min_asst, uint8_t pm_pwr_cyc_dur, + struct cfg_assrt_dur *cfg_assrt_dur) +{ + /* + * Ensure slp_x_dur_list[] elements are in sync with devicetree config to FSP encoded + * values. + * slp_s4_asst_dur_list : 1s, 1s, 2s, 3s, 4s(Default) + */ + const enum min_assrt_dur slp_s4_asst_dur_list[] = { + MinAssrtDur1s, MinAssrtDur1s, MinAssrtDur2s, MinAssrtDur3s, MinAssrtDur4s + }; + + /* slp_s3_asst_dur_list: 50ms, 60us, 50ms (Default), 2s */ + const enum min_assrt_dur slp_s3_asst_dur_list[] = { + MinAssrtDur50ms, MinAssrtDur60us, MinAssrtDur50ms, MinAssrtDur2s + }; + + /* slp_a_asst_dur_list: 2s, 0s, 4s, 98ms, 2s(Default) */ + const enum min_assrt_dur slp_a_asst_dur_list[] = { + MinAssrtDur2s, MinAssrtDur0s, MinAssrtDur4s, MinAssrtDur98ms, MinAssrtDur2s + }; + + /* pm_pwr_cyc_dur_list: 4s(Default), 1s, 2s, 3s, 4s */ + const enum min_assrt_dur pm_pwr_cyc_dur_list[] = { + MinAssrtDur4s, MinAssrtDur1s, MinAssrtDur2s, MinAssrtDur3s, MinAssrtDur4s + }; + + /* Get signal assertion width */ + if (slp_s4_min_asst < ARRAY_SIZE(slp_s4_asst_dur_list)) + cfg_assrt_dur->slp_s4 = slp_s4_asst_dur_list[slp_s4_min_asst]; + + if (slp_s3_min_asst < ARRAY_SIZE(slp_s3_asst_dur_list)) + cfg_assrt_dur->slp_s3 = slp_s3_asst_dur_list[slp_s3_min_asst]; + + if (slp_a_min_asst < ARRAY_SIZE(slp_a_asst_dur_list)) + cfg_assrt_dur->slp_a = slp_a_asst_dur_list[slp_a_min_asst]; + + if (pm_pwr_cyc_dur < ARRAY_SIZE(pm_pwr_cyc_dur_list)) + cfg_assrt_dur->pm_pwr_cyc_dur = pm_pwr_cyc_dur_list[pm_pwr_cyc_dur]; +} + + +static uint8_t get_pm_pwr_cyc_dur(uint8_t slp_s4_min_asst, uint8_t slp_s3_min_asst, + uint8_t slp_a_min_asst, uint8_t pm_pwr_cyc_dur) +{ + /* Sets default minimum asserton duration values */ + struct cfg_assrt_dur cfg_assrt_dur = { + .slp_a = MinAssrtDur2s, + .slp_s4 = MinAssrtDur4s, + .slp_s3 = MinAssrtDur50ms, + .pm_pwr_cyc_dur = MinAssrtDur4s + }; + + enum min_assrt_dur high_asst_width; + + /* Convert assertion durations from register-encoded to microseconds */ + get_min_assrt_dur(slp_s4_min_asst, slp_s3_min_asst, slp_a_min_asst, pm_pwr_cyc_dur, + &cfg_assrt_dur); + + /* Get the higher assertion duration among PCH EDS specified signals for pwr_cyc_dur */ + high_asst_width = get_high_asst_width(&cfg_assrt_dur); + + if (cfg_assrt_dur.pm_pwr_cyc_dur >= high_asst_width) + return pm_pwr_cyc_dur; + + printk(BIOS_DEBUG, + "Set PmPwrCycDur to 4s as configured PmPwrCycDur(%d) violates PCH EDS " + "spec\n", pm_pwr_cyc_dur); + + return PCH_PM_PWR_CYC_DUR; +} + + static void parse_devicetree_param(const config_t *config, FSP_S_CONFIG *params) { uint32_t dev_offset = 0; @@ -412,6 +532,13 @@ if (config->PchPmSlpAMinAssert) params->PchPmSlpAMinAssert = config->PchPmSlpAMinAssert;
+#if CONFIG(SOC_INTEL_COMETLAKE) + if (config->PchPmPwrCycDur) + params->PchPmPwrCycDur = get_pm_pwr_cyc_dur(config->PchPmSlpS4MinAssert, + config->PchPmSlpS3MinAssert, config->PchPmSlpAMinAssert, + config->PchPmPwrCycDur); +#endif + /* Set TccActivationOffset */ tconfig->TccActivationOffset = config->tcc_offset;
-
Edward O'Callaghan (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)