Change in coreboot[master]: soc/intel/cannonlake: Disable ACPI PM Timer to enable XTAL OSC shutdown
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM Timer to enable XTAL OSC shutdown ......................................................................
soc/intel/cannonlake: Disable ACPI PM Timer to enable XTAL OSC shutdown
Keeping ACPI PM timer alive prevents XTAL OSC shutdown in S0ix which has a power impact.
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1.
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/1
diff --git a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb index 6ff90f8..eb4be43 100644 --- a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb @@ -47,6 +47,8 @@ # Unlock GPIO pads register "PchUnlockGpioPads" = "1"
+ register "PmTimerDisabled" = "1" + # VR Settings Configuration for 4 Domains #+----------------+-------+-------+-------+-------+ #| Domain/Setting | SA | IA | GTUS | GTS | diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index cbaa710..6a2c038 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -366,6 +366,9 @@ params->PchPwrOptEnable = config->dmipwroptimize; params->SataPwrOptEnable = config->satapwroptimize;
+ /* Disable PCH ACPI timer */ + params->EnableTcoTimer = !config->PmTimerDisabled; + /* Apply minimum assertion width settings if non-zero */ if (config->PchPmSlpS3MinAssert) params->PchPmSlpS3MinAssert = config->PchPmSlpS3MinAssert;
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM Timer to enable XTAL OSC shutdown ......................................................................
Patch Set 1:
Furquan, can you please take a look into this, as it might applicable to corresponding bug ?
Hello Patrick Rudolph, Aamir Bohra, V Sowmya, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34506
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Disable ACPI PM Timer to enable XTAL OSC shutdown ......................................................................
soc/intel/cannonlake: Disable ACPI PM Timer to enable XTAL OSC shutdown
Keeping ACPI PM timer alive in S0ix has a power impact.
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1.
With PmTimerDisabled = 0
iotools mmio_read8 0xfe0018fc
0x00
With PmTimerDisabled = 1
iotools mmio_read8 0xfe0018fc
0x02
Bit 1: ACPI Timer Disable (ACPI_TIM_DIS): This bit determines whether the ACPI Timer is enabled to run. - 0: ACPI Timer is enabled - 1: ACPI Timer is disabled
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/2
Hello Patrick Rudolph, Aamir Bohra, V Sowmya, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34506
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Disable ACPI PM Timer due to power impact in S0ix ......................................................................
soc/intel/cannonlake: Disable ACPI PM Timer due to power impact in S0ix
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1.
With PmTimerDisabled = 0
iotools mmio_read8 0xfe0018fc
0x00
With PmTimerDisabled = 1
iotools mmio_read8 0xfe0018fc
0x02
Bit 1: ACPI Timer Disable (ACPI_TIM_DIS): This bit determines whether the ACPI Timer is enabled to run. - 0: ACPI Timer is enabled - 1: ACPI Timer is disabled
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM Timer due to power impact in S0ix ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG@7 PS4, Line 7: soc/intel/cannonlake: Disable ACPI PM Timer due to power impact in S0ix Maybe:
Disable ACPI PM timer to reduce S0ix power usage
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG@8 PS4, Line 8: Maybe add that the TCO timer is used instead (was it before)?
Did you measure the power difference?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM Timer due to power impact in S0ix ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG@8 PS4, Line 8:
Maybe add that the TCO timer is used instead (was it before)? […]
its ~10mW
Hello Patrick Rudolph, Aamir Bohra, Paul Menzel, V Sowmya, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34506
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1.
With PmTimerDisabled = 0
iotools mmio_read8 0xfe0018fc
0x00
With PmTimerDisabled = 1
iotools mmio_read8 0xfe0018fc
0x02
Bit 1: ACPI Timer Disable (ACPI_TIM_DIS): This bit determines whether the ACPI Timer is enabled to run. - 0: ACPI Timer is enabled - 1: ACPI Timer is disabled
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG@7 PS4, Line 7: soc/intel/cannonlake: Disable ACPI PM Timer due to power impact in S0ix
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/34506/4//COMMIT_MSG@8 PS4, Line 8:
its ~10mW
Done
Hello Patrick Rudolph, Aamir Bohra, Paul Menzel, V Sowmya, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34506
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1.
With PmTimerDisabled = 0
iotools mmio_read8 0xfe0018fc
0x00
With PmTimerDisabled = 1
iotools mmio_read8 0xfe0018fc
0x02
Bit 1: ACPI Timer Disable (ACPI_TIM_DIS): This bit determines whether the ACPI Timer is enabled to run. - 0: ACPI Timer is enabled - 1: ACPI Timer is disabled
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/6
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 6: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34506/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/7//COMMIT_MSG@12 PS7, Line 12: devicetree set to 1. Could you please add the note, that 10 mW are saved after the sentence?
Hello Patrick Rudolph, Aamir Bohra, Paul Menzel, V Sowmya, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34506
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1. This changes save ~10mW power in S0ix.
With PmTimerDisabled = 0
iotools mmio_read8 0xfe0018fc
0x00
With PmTimerDisabled = 1
iotools mmio_read8 0xfe0018fc
0x02
Bit 1: ACPI Timer Disable (ACPI_TIM_DIS): This bit determines whether the ACPI Timer is enabled to run. - 0: ACPI Timer is enabled - 1: ACPI Timer is disabled
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34506/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/7//COMMIT_MSG@12 PS7, Line 12: devicetree set to 1.
Could you please add the note, that 10 mW are saved after the sentence?
Ack
Aamir Bohra has uploaded a new patch set (#9) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage
BRANCH=none BUG=b:138152075 TEST=Build for cometlake board with the PmTimerDisabled policy in devicetree set to 1.
With PmTimerDisabled = 0
iotools mmio_read8 0xfe0018fc
0x00
With PmTimerDisabled = 1
iotools mmio_read8 0xfe0018fc
0x02
Bit 1: ACPI Timer Disable (ACPI_TIM_DIS): This bit determines whether the ACPI Timer is enabled to run. - 0: ACPI Timer is enabled - 1: ACPI Timer is disabled
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/9
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34506/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/9//COMMIT_MSG@6 PS9, Line 6: : soc/intel/cannonlake Can you please split the CL into two? One for soc and other for mainboard?
Hello Patrick Rudolph, Aamir Bohra, Paul Menzel, V Sowmya, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34506
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage
This patch overrides EnableTcoTimer FSP UPD default value based on PmTimerDisabled coreboot devcietree config.
BRANCH=none BUG=b:138152075
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/34506/10
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 10:
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34611/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34506/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34506/9//COMMIT_MSG@6 PS9, Line 6: : soc/intel/cannonlake
Can you please split the CL into two? One for soc and other for mainboard?
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 10:
based on crossbug power number update, what should be our next step ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 10: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 10: Code-Review+2
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
Patch Set 10: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34506 )
Change subject: soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage ......................................................................
soc/intel/cannonlake: Disable ACPI PM timer to reduce S0ix power usage
This patch overrides EnableTcoTimer FSP UPD default value based on PmTimerDisabled coreboot devcietree config.
BRANCH=none BUG=b:138152075
Change-Id: I347c15c7b65fb4c19b9680f127980d4ddab8df51 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34506 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Aamir Bohra: Looks good to me, approved V Sowmya: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index f696f79..3cc426a 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -367,6 +367,9 @@ params->PchPwrOptEnable = config->dmipwroptimize; params->SataPwrOptEnable = config->satapwroptimize;
+ /* Disable PCH ACPI timer */ + params->EnableTcoTimer = !config->PmTimerDisabled; + /* Apply minimum assertion width settings if non-zero */ if (config->PchPmSlpS3MinAssert) params->PchPmSlpS3MinAssert = config->PchPmSlpS3MinAssert;
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)