Tim Chu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: configs/builder/config.ocp.deltalake: Add PMC support ......................................................................
configs/builder/config.ocp.deltalake: Add PMC support
Enable PMC support on Deltalake and change the state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289 and build for Deltalake. The following Kconfig options must select: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 --- M configs/builder/config.ocp.deltalake 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42976/1
diff --git a/configs/builder/config.ocp.deltalake b/configs/builder/config.ocp.deltalake index 4a8cda1..c849dbb 100644 --- a/configs/builder/config.ocp.deltalake +++ b/configs/builder/config.ocp.deltalake @@ -15,3 +15,4 @@ CONFIG_FSP_S_FILE="site-local/deltalake/Server_S.fd" CONFIG_ME_BIN_PATH="site-local/deltalake/flashregion_2_intel_me.bin" CONFIG_IFD_BIN_PATH="site-local/deltalake/flashregion_0_flashdescriptor.bin" +CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
Tim Chu has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Configs: Set PCH power policy register to S5
Enable PMC support on Deltalake and change the state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289 and build for Deltalake. The following Kconfig options must select: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 --- M configs/builder/config.ocp.deltalake 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42976/2
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y Why not add this Kconfig to DeltaLake Kconfig in mainboard directory?
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
Why not add this Kconfig to DeltaLake Kconfig in mainboard directory?
I've tried to add it in mainboard directory, but the power state didn't change to S5. And the way I use is adding "select POWER_STATE_OFF_AFTER_FAILURE" in mainboard directory.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
I've tried to add it in mainboard directory, but the power state didn't change to S5. […]
After you change Kconfig files, please run "make clean && make olddefconfig". You can diff the .config before and after "make olddefconfig" to be sure.
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y Actually, I've removed .config and rebuild it. It seems adding "select POWER_STATE_OFF_AFTER_FAILURE" in mainboard directory can not override the used settings in /soc/intel/common/block/pmc/Kconfig.
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
Actually, I've removed .config and rebuild it. […]
Or we can add "config MAINBOARD_POWER_FAILURE_STATE" to Kconfig in mainboard directory and set the default value to 0 in order to override the used setting? This way may revise the value directly without select any option.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
Or we can add "config MAINBOARD_POWER_FAILURE_STATE" to Kconfig in mainboard directory and set the d […]
Makes sense to set a mainboard default, instead of in SOC.
Hello build bot (Jenkins), Jonathan Zhang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42976
to look at the new patch set (#3).
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Configs: Set PCH power policy register to S5
Enable PMC support on Deltalake and change the state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289 and build for Deltalake. The following Kconfig options must select: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 --- M src/mainboard/ocp/deltalake/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42976/3
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... File configs/builder/config.ocp.deltalake:
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
Makes sense to set a mainboard default, instead of in SOC.
Done
https://review.coreboot.org/c/coreboot/+/42976/2/configs/builder/config.ocp.... PS2, Line 18: CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y
After you change Kconfig files, please run "make clean && make olddefconfig". You can diff the . […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: Configs: Set PCH power policy register to S5 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42976/3//COMMIT_MSG@7 PS3, Line 7: Configs: Set PCH power policy register to S5 The commit message does not match the change in this patchset
Suggest change title to: /mb/ocp/deltalake: disable MAINBOARD_POWER_FAILURE_STATE
Might update rest of comment.
Hello build bot (Jenkins), Jonathan Zhang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42976
to look at the new patch set (#4).
Change subject: /mb/ocp/deltalake: disable MAINBOARD_POWER_FAILURE_STATE ......................................................................
/mb/ocp/deltalake: disable MAINBOARD_POWER_FAILURE_STATE
Change PCH power policy. Set the state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289 and build for Deltalake. The following Kconfig options must select: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 --- M src/mainboard/ocp/deltalake/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42976/4
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: /mb/ocp/deltalake: disable MAINBOARD_POWER_FAILURE_STATE ......................................................................
Patch Set 4:
(1 comment)
Thanks.
https://review.coreboot.org/c/coreboot/+/42976/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42976/3//COMMIT_MSG@7 PS3, Line 7: Configs: Set PCH power policy register to S5
The commit message does not match the change in this patchset […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: /mb/ocp/deltalake: disable MAINBOARD_POWER_FAILURE_STATE ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Kconfig:
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... PS4, Line 16: config MAINBOARD_POWER_FAILURE_STATE This is supposed to be controlled by the user choice in the "Mainboard" menu. If you override it here, the user may not get what they selected.
The default for the user choice is controlled with POWER_STATE_DEFAULT_ ON_AFTER_FAILURE. Currently it's selected by common Intel code, which has to change if you want to override it.
I guess the following could work:
* In `soc/intel/common/block/pmc/Kconfig` remove the `select` and add
config POWER_STATE_DEFAULT_ON_AFTER_FAILURE default y
* And here
config POWER_STATE_DEFAULT_ON_AFTER_FAILURE default n
Hello build bot (Jenkins), Jonathan Zhang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42976
to look at the new patch set (#5).
Change subject: /mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
/mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE
Change PCH power policy. Set default of POWER_STATE_DEFAULT_ON_AFTER_FAILURE to n in order to change power state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289, CB:43338 and build for Deltalake. The following Kconfig options must select: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 --- M src/mainboard/ocp/deltalake/Kconfig 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42976/5
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: /mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 5:
(1 comment)
Thanks a lot.
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Kconfig:
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... PS4, Line 16: config MAINBOARD_POWER_FAILURE_STATE
This is supposed to be controlled by the user choice in the "Mainboard" […]
Done
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: /mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Kconfig:
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... PS4, Line 16: config MAINBOARD_POWER_FAILURE_STATE
This is supposed to be controlled by the user choice in the "Mainboard" […]
How about another option: Keep all Kconfig files intact, but update configs/builder/config.ocp.deltalake?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: /mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@7 PS5, Line 7: /mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE Please remove the / in the start.
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@7 PS5, Line 7: Deselect Maybe use *Unset*, as that is what Kconfig uses?
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@14 PS5, Line 14: select be selected
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@17 PS5, Line 17: select CPU_INTEL_COMMON_SMM Please also indent with a tab.
Hello build bot (Jenkins), Jonathan Zhang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42976
to look at the new patch set (#6).
Change subject: mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE
Change PCH power policy. Set default of POWER_STATE_DEFAULT_ON_AFTER_FAILURE to n in order to change power state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289, CB:43338 and build for Deltalake. The following Kconfig options must be selected: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 --- M src/mainboard/ocp/deltalake/Kconfig 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42976/6
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@7 PS5, Line 7: Deselect
Maybe use *Unset*, as that is what Kconfig uses?
Done
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@7 PS5, Line 7: /mb/ocp/deltalake: Deselect POWER_STATE_DEFAULT_ON_AFTER_FAILURE
Please remove the / in the start.
Done
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@14 PS5, Line 14: select
be selected
Done
https://review.coreboot.org/c/coreboot/+/42976/5//COMMIT_MSG@17 PS5, Line 17: select CPU_INTEL_COMMON_SMM
Please also indent with a tab.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Kconfig:
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... PS4, Line 16: config MAINBOARD_POWER_FAILURE_STATE
How about another option: […]
Is this resolved?
Tim Chu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
Patch Set 6:
(1 comment)
Thanks.
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Kconfig:
https://review.coreboot.org/c/coreboot/+/42976/4/src/mainboard/ocp/deltalake... PS4, Line 16: config MAINBOARD_POWER_FAILURE_STATE
Is this resolved?
Done
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42976 )
Change subject: mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE ......................................................................
mb/ocp/deltalake: Unset POWER_STATE_DEFAULT_ON_AFTER_FAILURE
Change PCH power policy. Set default of POWER_STATE_DEFAULT_ON_AFTER_FAILURE to n in order to change power state to S5 when power is reapplied after power failure.
TEST=Base on CB:42289, CB:43338 and build for Deltalake. The following Kconfig options must be selected: select SOC_INTEL_COMMON_BLOCK_PMC select ACPI_INTEL_HARDWARE_SLEEP_VALUES select CPU_INTEL_COMMON_SMM
Boot the system and check the last bit of GEN_PMCON_B is set to 1 through ITP with command: pch.pm_dump
Signed-off-by: Tim Chu Tim.Chu@quantatw.com Change-Id: I4d4f14bdfc18740976171fd5d369b2d79a916dc4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42976 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Jonathan Zhang jonzhang@fb.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/ocp/deltalake/Kconfig 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved Jonathan Zhang: Looks good to me, but someone else must approve
diff --git a/src/mainboard/ocp/deltalake/Kconfig b/src/mainboard/ocp/deltalake/Kconfig index b4e88b5..17f81bd 100644 --- a/src/mainboard/ocp/deltalake/Kconfig +++ b/src/mainboard/ocp/deltalake/Kconfig @@ -14,6 +14,9 @@ select VPD select VPD_SMBIOS_VERSION
+config POWER_STATE_DEFAULT_ON_AFTER_FAILURE + default n + config IPMI_KCS_REGISTER_SPACING int default 4
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Jonathan Zhang (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Tim Chu (Code Review)