Change in coreboot[master]: soc/intel/cannonlake: Move GPIO PM configuration to soc level
EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl M src/mainboard/google/hatch/mainboard.asl M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 4 files changed, 31 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/1
diff --git a/src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl b/src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl index 608e4e0..b5cc268 100644 --- a/src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl +++ b/src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl @@ -16,15 +16,6 @@ #define CAM_EN GPP_B11 /* Active low */ #define TS_PD GPP_E7 #define HDMI_PD GPP_E16 -#include <intelblocks/gpio.h> - -Method (LOCL, 1, Serialized) -{ - For (Local0 = 0, Local0 < 5, Local0++) - { - _SB.PCI0.CGPM (Local0, Arg0) - } -}
/* Method called from LPIT prior to enter s0ix state */ Method (MS0X, 1) @@ -34,13 +25,11 @@ _SB.PCI0.STXS (CAM_EN) /* Turn off HDMI power */ _SB.PCI0.CTXS (HDMI_PD) - LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) } Else { /* Turn on camera power */ _SB.PCI0.CTXS (CAM_EN) /* Turn on HDMI power */ _SB.PCI0.STXS (HDMI_PD) - LOCL (0) } }
@@ -53,12 +42,14 @@ _SB.PCI0.CTXS (TS_PD) /* Clear HDMI power to avoid leakage */ _SB.PCI0.CTXS (HDMI_PD) - LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) + /* Enable GPIO PM */ + _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) }
/* Method called from _WAK prior to wakeup */ Method (MWAK, 1) { _SB.PCI0.LPCB.EC0.WAK (Arg0) - LOCL (0) + /* Disable GPIO PM */ + _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) } diff --git a/src/mainboard/google/hatch/mainboard.asl b/src/mainboard/google/hatch/mainboard.asl index dff1a75..6a72fe8 100644 --- a/src/mainboard/google/hatch/mainboard.asl +++ b/src/mainboard/google/hatch/mainboard.asl @@ -13,23 +13,13 @@ * GNU General Public License for more details. */
-#include <intelblocks/gpio.h> - -Method (LOCL, 1, Serialized) -{ - For (Local0 = 0, Local0 < 5, Local0++) - { - _SB.PCI0.CGPM (Local0, Arg0) - } -} - /* * Method called from _PTS prior to system sleep state entry * Enables dynamic clock gating for all 5 GPIO communities */ Method (MPTS, 1, Serialized) { - LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) + _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) }
/* @@ -38,20 +28,6 @@ */ Method (MWAK, 1, Serialized) { - LOCL (0) + _SB.PCI0.LOCL (0) }
-/* - * S0ix Entry/Exit Notifications - * Called from _SB.LPID._DSM - */ -Method (MS0X, 1, Serialized) -{ - If (Arg0 == 1) { - /* S0ix Entry */ - LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) - } Else { - /* S0ix Exit */ - LOCL (0) - } -} diff --git a/src/soc/intel/cannonlake/acpi/gpio.asl b/src/soc/intel/cannonlake/acpi/gpio.asl index 65332ad..aecb8c3 100644 --- a/src/soc/intel/cannonlake/acpi/gpio.asl +++ b/src/soc/intel/cannonlake/acpi/gpio.asl @@ -157,3 +157,12 @@ PCRO (Local0, GPIO_MISCCFG, And (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG)) } } + +Method (LOCL, 1, Serialized) +{ + For (Local0 = 0, Local0 < 5, Local0++) + { + _SB.PCI0.CGPM (Local0, Arg0) + } +} + diff --git a/src/soc/intel/cannonlake/acpi/lpit.asl b/src/soc/intel/cannonlake/acpi/lpit.asl index 74d4fe6..bba7264 100644 --- a/src/soc/intel/cannonlake/acpi/lpit.asl +++ b/src/soc/intel/cannonlake/acpi/lpit.asl @@ -14,9 +14,19 @@ * GNU General Public License for more details. */
+#include <intelblocks/gpio.h> + External(_SB.MS0X, MethodObj) External(_SB.PCI0.LPCB.EC0.S0IX, MethodObj)
+Method (LOCL, 1, Serialized) +{ + For (Local0 = 0, Local0 < 5, Local0++) + { + _SB.PCI0.CGPM (Local0, Arg0) + } +} + scope(_SB) { Device(LPID) { @@ -73,6 +83,9 @@ If (CondRefOf (_SB.MS0X)) { _SB.MS0X(1) } + + /* Enable GPIO PM */ + _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) } /* * Function 6 - Low Power S0 Exit Notification @@ -87,6 +100,9 @@ If (CondRefOf (_SB.MS0X)) { _SB.MS0X(0) } + + /* Disable GPIO PM */ + _SB.PCI0.LOCL (0) } } Return(Buffer(One) {0x00})
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl M src/mainboard/google/hatch/mainboard.asl M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 4 files changed, 30 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : } duplicate https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... ?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : }
duplicate https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac.... […]
oh I forgot remove this.. Thanks
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl M src/mainboard/google/hatch/mainboard.asl M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 4 files changed, 20 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/3
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : }
oh I forgot remove this.. […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : }
Done
Please don't rebase your CL on https://review.coreboot.org/c/coreboot/+/37665/3, make your CL independent
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : }
Please don't rebase your CL on https://review.coreboot. […]
oh, but I need clean up the LOCL together with hatch. I need the dependency or it will conflict. Or we can abandon another?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : }
oh, but I need clean up the LOCL together with hatch. I need the dependency or it will conflict. […]
please abandon other one.
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl M src/mainboard/google/hatch/mainboard.asl M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 4 files changed, 23 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/4
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl M src/mainboard/google/hatch/mainboard.asl M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 4 files changed, 24 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/5
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/2/src/soc/intel/cannonlake/ac... PS2, Line 22: Method (LOCL, 1, Serialized) : { : For (Local0 = 0, Local0 < 5, Local0++) : { : _SB.PCI0.CGPM (Local0, Arg0) : } : }
please abandon other one.
Hope this time is fine.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 5:
(7 comments)
This change should be split into three: a) SoC change -- To add support for LOCL or a better named method b) Hatch -- Update to using this LOCL method from SoC c) Drallion - Add support to using LOCL method from SoC.
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... PS5, Line 56: MISCCFG_ENABLE_GPIO_PM_CONFIG This is not disabling GPIO PM config. Instead it is being enabled.
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 160: Can you please add a comment to indicate what this method does? and what the arg is?
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 161: LOCL I am trying to remember what LOCL stands for...
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 163: 5 TOTAL_GPIO_COMM
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 165: _SB.PCI0.CGPM Just "CGPM" should work?
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG) Why is this being called here? This call is already made from mainboard and that should be sufficient?
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 96: /* Disable GPIO PM */ : _SB.PCI0.LOCL (0) Same comment as above.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Why is this being called here? This call is already made from mainboard and that should be sufficien […]
Duncan suggest move into common code in https://review.coreboot.org/c/coreboot/+/37665. Since we want this be a common method for CNL/CML.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... PS5, Line 56: MISCCFG_ENABLE_GPIO_PM_CONFIG
This is not disabling GPIO PM config. Instead it is being enabled.
my fault this should pass 0 not MISCCFG_ENABLE_GPIO_PM_CONFIG
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 160:
Can you please add a comment to indicate what this method does? and what the arg is?
sure.
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 165: _SB.PCI0.CGPM
Just "CGPM" should work?
yes :) I just copy paste
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 161: LOCL
I am trying to remember what LOCL stands for...
I just honor Tim's CL. I don't remember either.
Hello Patrick Rudolph, Mathew King, Duncan Laurie, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/6
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... PS5, Line 56: MISCCFG_ENABLE_GPIO_PM_CONFIG
my fault this should pass 0 not MISCCFG_ENABLE_GPIO_PM_CONFIG
Done
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 160:
sure.
Done
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 161: LOCL
I just honor Tim's CL. I don't remember either.
Done
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 163: 5
TOTAL_GPIO_COMM
Done
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 165: _SB.PCI0.CGPM
yes :) I just copy paste
Done
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Duncan suggest move into common code in https://review.coreboot.org/c/coreboot/+/37665. […]
Ack
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 96: /* Disable GPIO PM */ : _SB.PCI0.LOCL (0)
Same comment as above.
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 161: LOCL
Done
"Local" as in a local method not to be called externally :) Naming things is hard!
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
Does anyone like this name? Please help review this :)
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
Are we confident enough that all cannonlake boards will want this by default?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37685/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/6//COMMIT_MSG@11 PS6, Line 11: BUG=?
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 162: Configure GPIO all Community Power Management nit: Configure Power Management bits for all GPIO communities?
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 166: GCPM nit: CGPM and GCPM look kind of very similar. Maybe CAPM? [(C)onfigure (A)ll GPIO community (P)ower (M)anagement].
You can also get rid of the two functions and instead move all functionality in CGPM into GCPM.
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Ack
I agree that the LOCL() (or GCPM() as you renamed it now) method can be moved to SoC. But until now the GPIO PM bits were set based on mainboard selection. For all Google boards, in general, I would say that we would want to follow the same policy i.e. setting the PM bits to 0 any time we are in S0, but in S0ix they should be set to MISCCFG_ENABLE_GPIO_PM_CONFIG. So, this change works okay. But, I am worried if this really works the same way for other boards. It might not be possible to make that decision right away until we start seeing more boards upstream.
But, I think we should at least be consistent in the way this is being implemented: 1. Mainboard is allowed to set gpio_override_pm and gpio_pm[] bits for each community. Examples: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
2. With the above change, coreboot will configure GPIO MISCCFG as per the gpio_pm[] bits.
3. Once the device enters S0ix, this change will set it to MISCCFG_ENABLE_GPIO_PM_CONFIG which seems okay.
4. But, when it exits S0ix, this change will set it to 0 which might not really be the same as gpio_pm[] bits that were provided in mainboard devicetree.
In general, I think we should provide consistent behavior since the above change can lead to hard to track problems. One thing that we can potentially do (if we really want to go down the route of setting PM bits to 0 or MISCCFG_ENABLE_GPIO_PM_CONFIG only):
--> Get rid of gpio_override_pm and gpio_pm[] from devicetree and chip.h --> Add a new config gpio_pm_s0_disable which basically ensures that GPIO PM configuration is disabled in S0 and enabled only in lower power states. --> SoC code can honor gpio_pm_override_s0 to determine whether it should set MISCCFG registers to 0 for S0 or to leave untouched. --> Additionally, add a new config to GNVS which indicates whether runtime configuration of GPIO PM is required based on the value of gpio_pm_override_s0. --> In this method, now you can check that GNVS variable to decide if the MISCCFG registers should be touched or left as is.
This forces all mainboards to: --> Either stay with default settings of GPIO_MISCCFG or --> Disable all PM bits in S0 and enable all PM bits in lower power states.
Since we do not have enough data on what other mainboards would want, we can start with this since it at least keeps the behavior consistent. In the future, if there are more mainboards with varied requirements, we can always update the behavior. Thoughts?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
I agree that the LOCL() (or GCPM() as you renamed it now) method can be moved to SoC. […]
I think GNVS is a good idea that we can keep the original setting from coreboot. Others, we should leave to Intel. @Subrata, how about your point of view?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
I think GNVS is a good idea that we can keep the original setting from coreboot. […]
I prefer we can keep the device tree setting and store it into GNVS. This can have more flexibility like https://review.coreboot.org/c/coreboot/+/32848/4/src/mainboard/google/sarien.... We still can have some power saving in S0.
Hello Patrick Rudolph, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl 2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/7
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 6:
Are we confident enough that all cannonlake boards will want this by default?
You mean saving power under s0ix?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
I prefer we can keep the device tree setting and store it into GNVS. […]
i like the GNVS way of doing things and this is good method to pass value from .c to .asl.
Do you want me to implement that part, please let me know
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
i like the GNVS way of doing things and this is good method to pass value from .c to .asl. […]
Yes, please. Then I can based you CL to do the next.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Yes, please. Then I can based you CL to do the next.
Sure, i will do the needful
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 6:
(4 comments)
One more thought here, for the consistent. We can remove the overwrite flag. And put all board default as enable PM if they don't want overwrite it. So we can keep the same behavior for all boards.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 6:
(4 comments)
One more thought here, for the consistent. We can remove the overwrite flag. And put all board default as enable PM if they don't want overwrite it. So we can keep the same behavior for all boards.
i want to handle it little different than what Furquan said but i will try to make use of existing code more and see how i can pass the existing GPIO_PM config from .c (via .cb from mainboard) to soc (gnvs) and make it available for runtime S0ix entry and exit
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
(4 comments)
One more thought here, for the consistent. We can remove the overwrite flag. And put all board default as enable PM if they don't want overwrite it. So we can keep the same behavior for all boards.
i want to handle it little different than what Furquan said but i will try to make use of existing code more and see how i can pass the existing GPIO_PM config from .c (via .cb from mainboard) to soc (gnvs) and make it available for runtime S0ix entry and exit
Oh, I mean we can get rid off gpio_override_pm flag and keep the config in devicetree.cb. Keep the default as MISCCFG_ENABLE_GPIO_PM_CONFIG if there is no intended to overwrite it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
(4 comments)
One more thought here, for the consistent. We can remove the overwrite flag. And put all board default as enable PM if they don't want overwrite it. So we can keep the same behavior for all boards.
i want to handle it little different than what Furquan said but i will try to make use of existing code more and see how i can pass the existing GPIO_PM config from .c (via .cb from mainboard) to soc (gnvs) and make it available for runtime S0ix entry and exit
Oh, I mean we can get rid off gpio_override_pm flag and keep the config in devicetree.cb. Keep the default as MISCCFG_ENABLE_GPIO_PM_CONFIG if there is no intended to overwrite it.
Can we get some review here https://review.coreboot.org/q/topic:%2522144002424%2522+(status:open)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
(4 comments)
One more thought here, for the consistent. We can remove the overwrite flag. And put all board default as enable PM if they don't want overwrite it. So we can keep the same behavior for all boards.
i want to handle it little different than what Furquan said but i will try to make use of existing code more and see how i can pass the existing GPIO_PM config from .c (via .cb from mainboard) to soc (gnvs) and make it available for runtime S0ix entry and exit
Oh, I mean we can get rid off gpio_override_pm flag and keep the config in devicetree.cb. Keep the default as MISCCFG_ENABLE_GPIO_PM_CONFIG if there is no intended to overwrite it.
Can we get some review here https://review.coreboot.org/q/topic:%2522144002424%2522+(status:open)
Oh, I get it. This could work if we keep gpio_override_pm. I can sue it in the ASL for some tricks and reduce useless rewrite:)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Should we move the S3 _PTS/_WAK to SOC level as well?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 166: GCPM
nit: CGPM and GCPM look kind of very similar. […]
Agreed, if we're moving the S0ix PM bits settings into the device tree, then may as well lump these two methods together.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Sure, i will do the needful
If we think setting MISCCFG_ENABLE_GPIO_PM_CONFIG on the way into S0ix on all systems is OK then could we save the existing value in _PTS and then restore in _WAK instead of needing something in NVS? It isn't quite as flexible but NVS is a bit of a pain..
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
If we think setting MISCCFG_ENABLE_GPIO_PM_CONFIG on the way into S0ix on all systems is OK then cou […]
if we only need to decide between 0 or 0x3f then we can avoid NVS and do the required changes in _PTS and _WAK but if we bother about other bit options as devicetree.cb might not only have 0 or 0x3f rather user might device to enable selective bits between 0-5 then S0ix implementation won't honor that part. I guess this was the reason why Furquan said NVS might take those data from .cb to .asl ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
if we only need to decide between 0 or 0x3f then we can avoid NVS and do the required changes in _PT […]
@Duncan, Do you know if _MPTS and _MWAK is getting called during runtime suspend flow as well. Kane has brought a point that those functions are not getting called during runtime idle and he might need to set those 0-5 bits in runtime idle entry as well
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
PTS/WAK only call from SX like S0/S3/S5 state not s0ix. Currently s0ix only call lpit. but we can store the variable into local variable when system boot into OS. Then get this use by s0ix method.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 166: GCPM
Agreed, if we're moving the S0ix PM bits settings into the device tree, then may as well lump these […]
I would like to change it. RGPM (restore GPIO PM) and EGPM (enable GPIO PM).
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
@Duncan, […]
To clarify, I don't see _MPTS is called either idle or powerd_dbus_suspend.
Also, LPID._DSM is only called when I powerd_dbus_suspend. it won't be called when idle / screen on. is this expected?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
If we think setting MISCCFG_ENABLE_GPIO_PM_CONFIG on the way into S0ix on all systems is OK then could we save the existing value in _PTS and then restore in _WAK instead of needing something in NVS? It isn't quite as flexible but NVS is a bit of a pain..
I actually like this. My initial proposal was to use a single flag in NVS. But, as you said if this policy looks okay:
--> When in S0 use the values provided in devicetree (Configured by coreboot after FSP-S runs) --> When entering any low power state (S0ix, S3, S5, ...), _MPTS and LPIT S0ix entry methods can save the current state of MISCCFG PM bits. --> On resume from low power state, _MWAK and LPIT S0ix exit methods can restore the saved state into MISCCFG PM bits.
then it can actually reduce a lot of NVS complexity.
Why this works well is because: --> MISCCFG PM bits would be retained after any kind of low power mode entry/exit. --> It also gives the flexibility to allow mainboards to have any MISCCFG PM bit settings. --> Additionally, if any entity changes MISCCFG PM bits after coreboot, those would still be honored. --> No need to worry about different TOTAL_GPIO_COMM in nvs.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Do you know if _MPTS and _MWAK is getting called during runtime suspend flow as well.
During ACPI sleep/wake, _PTS/_WAK combination is called. Thus, it applies to S3, S5, etc.
During suspend S0ix, LPIT DSM methods are called. _PTS/_WAK combination is not used.
During runtime S0ix, software is not aware of it and hence neither LPIT DSM nor _PTS/_WAK are called.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
So current plan is before entry s0ix/S3 store PM bit then enable PM. And restore PM bit when resume? I think we don't have to consider S5 or this is needed? Not sure the CPU behaviour under S5.
Hello Patrick Rudolph, Kane Chen, Mathew King, Duncan Laurie, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/8
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 8:
I made a simple code like this. I hope this can help the imagination.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
Do you know if _MPTS and _MWAK is getting called during runtime suspend flow as well. […]
I like that idea as well, then no need to have NVS
Hello Patrick Rudolph, Kane Chen, Mathew King, Duncan Laurie, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 64 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/9
Hello Patrick Rudolph, Kane Chen, Mathew King, Duncan Laurie, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/10
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
I like that idea as well, then no need to have NVS
@Furquan
During runtime S0ix, software is not aware of it and hence neither LPIT DSM nor _PTS/_WAK are called
I guess Kane told that _DSM is getting called
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
LPID_DSM will be called when kernel do s0ix suspend. This is why I hooked MS0X here for power saving under s0ix.
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
@Furquan […]
the _DSM is only called when i run power_dbus_suspend.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
(1 comment)
I am sure lid close and idle can work on Sarien, because if the DSM not be called. EC will have issue. But not sure the echo freeze to /sys/power/state.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
(1 comment)
@Kane, if you have Sarien on hand. You can check the KB backlight or light of power button is off or not after enter s0ix. If DSM is not called, the light will keep on.
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10:
(1 comment)
@Kane, if you have Sarien on hand. You can check the KB backlight or light of power button is off or not after enter s0ix. If DSM is not called, the light will keep on.
i'm using hatch, i added debug code and enable acpi debug in kernel. The DSM s0ix exit/entry functions are only called after i powerd_dbus_suspend.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
(1 comment)
@Kane, if you have Sarien on hand. You can check the KB backlight or light of power button is off or not after enter s0ix. If DSM is not called, the light will keep on.
i'm using hatch, i added debug code and enable acpi debug in kernel. The DSM s0ix exit/entry functions are only called after i powerd_dbus_suspend.
So, you mean original CL is not working as well? Tim put the enable PM in MS0X. I can try hatch if I have time.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
(1 comment)
@Kane, if you have Sarien on hand. You can check the KB backlight or light of power button is off or not after enter s0ix. If DSM is not called, the light will keep on.
i'm using hatch, i added debug code and enable acpi debug in kernel. The DSM s0ix exit/entry functions are only called after i powerd_dbus_suspend.
@Kane, Lid, idle, powerd, echo freeze all can work on Sarien. I will check Hatch later. I print ACPI debug with echo 1 > /sys/module/acpi/parameters/aml_debug_output
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
(1 comment)
@Kane, if you have Sarien on hand. You can check the KB backlight or light of power button is off or not after enter s0ix. If DSM is not called, the light will keep on.
i'm using hatch, i added debug code and enable acpi debug in kernel. The DSM s0ix exit/entry functions are only called after i powerd_dbus_suspend.
@Kane, Lid, idle, powerd, echo freeze all can work on Sarien. I will check Hatch later. I print ACPI debug with echo 1 > /sys/module/acpi/parameters/aml_debug_output
when you say idle, you simply just idle/screen on and then Function 5 - Low Power S0 Entry Notification and Function 6 - Low Power S0 Exit Notification got called?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10:
(1 comment)
@Kane, if you have Sarien on hand. You can check the KB backlight or light of power button is off or not after enter s0ix. If DSM is not called, the light will keep on.
i'm using hatch, i added debug code and enable acpi debug in kernel. The DSM s0ix exit/entry functions are only called after i powerd_dbus_suspend.
@Kane, Lid, idle, powerd, echo freeze all can work on Sarien. I will check Hatch later. I print ACPI debug with echo 1 > /sys/module/acpi/parameters/aml_debug_output
when you say idle, you simply just idle/screen on and then Function 5 - Low Power S0 Entry Notification and Function 6 - Low Power S0 Exit Notification got called?
Yes, this is called by kernel even CPU is not go into C10.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
After talked with Kane. I get the point he pointed. That's true, runtime suspend kernel is not aware of it.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10:
@googler, let move back to the topic. Can we have review before X'max? We have following testing after device come from factory. This is impact drallion power consumption as well.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 10: Code-Review+1
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37685/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/6//COMMIT_MSG@11 PS6, Line 11:
BUG=?
Done
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 162: Configure GPIO all Community Power Management
nit: Configure Power Management bits for all GPIO communities?
Done
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 166: GCPM
I would like to change it. RGPM (restore GPIO PM) and EGPM (enable GPIO PM).
Done
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
the _DSM is only called when i run power_dbus_suspend.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj) Since this is common ACPI code, can we add a comment here indicating that if an SoC wants to control GPIO PM bits during suspend states, it will need to define these methods?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
Since this is common ACPI code, can we add a comment here indicating that if an SoC wants to control […]
If we want this, maybe we can pass from device tree. Maybe put in GNVS... then I can just use it? I consider this as well. We can discuss it or do this in another CL?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
If we want this, maybe we can pass from device tree. Maybe put in GNVS... […]
Or we can just simply don't overwrite it and let it control by overwrite flag. This is bit complicated if we want different suspend PM bits control.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
Or we can just simply don't overwrite it and let it control by overwrite flag. […]
Exactly. I think it's totally fine to have these methods defined in common code, but I feel like each mainboard should be able to choose if it wants to change the PM bits at S0ix entry/exit.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
Patch Set 11:
(1 comment)
@Subrata, please restore you CL back for the PM over write flag pass from device tree :). Seems we need it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
@Subrata, please restore you CL back for the PM over write flag pass from device tree :). Seems we need it.
Done Eric, i think you only need this https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/...
so i will remove other GPIO community variable and only keep "0x30 - Override GPIO Power Management"
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
@Subrata, please restore you CL back for the PM over write flag pass from device tree :). Seems we need it.
Done Eric, i think you only need this https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/...
so i will remove other GPIO community variable and only keep "0x30 - Override GPIO Power Management"
@Eric, i have rebased https://review.coreboot.org/q/topic:%2522144002424%2522+(status:open) CL with "only" GPIO PM override configuration from .cb to ASL
Hello Patrick Rudolph, Kane Chen, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#12).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/12
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 12:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
@Subrata, please restore you CL back for the PM over write flag pass from device tree :). Seems we need it.
Done Eric, i think you only need this https://review.coreboot.org/c/coreboot/+/37793/1/src/soc/intel/common/block/...
so i will remove other GPIO community variable and only keep "0x30 - Override GPIO Power Management"
@Eric, i have rebased https://review.coreboot.org/q/topic:%2522144002424%2522+(status:open) CL with "only" GPIO PM override configuration from .cb to ASL
@Subrata, any idea of SIEMENS_MC_APL1 can't recognize the OGPM?
Hello Patrick Rudolph, Kane Chen, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#13).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable/disable GPIO clock gating when enter/exit s0ix is common request on CNL/CML. Move it from board level to soc level.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/13
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
looks like its common vs specific SOC changes issue
now we have 2 option 1. Do you really need to save and restore GPIO PM setting in _PTS and _WAK, because _PTS/_WAK will only call for S5/S3 case where anyway your bootblock changes are in place to fix GPIO PM issue, isn't its duplicate ? i guess you have to only bother about MPTS/MWAK ?
2. Like SKL has its own platform.asl rather using common one. Its not feasible to make GPIO PM override feature flag for APL where this feature doesn't exist.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
looks like its common vs specific SOC changes issue
now we have 2 option
Do you really need to save and restore GPIO PM setting in _PTS and _WAK, because _PTS/_WAK will only call for S5/S3 case where anyway your bootblock changes are in place to fix GPIO PM issue, isn't its duplicate ? i guess you have to only bother about MPTS/MWAK ?
Like SKL has its own platform.asl rather using common one. Its not feasible to make GPIO PM override feature flag for APL where this feature doesn't exist.
I think the bootblock is called when resume not when suspend. _WAK is not needed, but _PTS still needed. I prefer we can add a config and I can use it during compile time. Maybe like CONFIG_SUPPORT_GPIO_PM etc.. in soc Kconfig. How do you think?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
looks like its common vs specific SOC changes issue
now we have 2 option
Do you really need to save and restore GPIO PM setting in _PTS and _WAK, because _PTS/_WAK will only call for S5/S3 case where anyway your bootblock changes are in place to fix GPIO PM issue, isn't its duplicate ? i guess you have to only bother about MPTS/MWAK ?
Like SKL has its own platform.asl rather using common one. Its not feasible to make GPIO PM override feature flag for APL where this feature doesn't exist.
I think the bootblock is called when resume not when suspend. _WAK is not needed, but _PTS still needed.
@Eric, i didn't get this, if _WAK is not required when why you think _PTS is required ? Can you please let me know what you want achieve using _PTS ? May be i'm missing something here :(
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
looks like its common vs specific SOC changes issue
now we have 2 option
Do you really need to save and restore GPIO PM setting in _PTS and _WAK, because _PTS/_WAK will only call for S5/S3 case where anyway your bootblock changes are in place to fix GPIO PM issue, isn't its duplicate ? i guess you have to only bother about MPTS/MWAK ?
Like SKL has its own platform.asl rather using common one. Its not feasible to make GPIO PM override feature flag for APL where this feature doesn't exist.
I think the bootblock is called when resume not when suspend. _WAK is not needed, but _PTS still needed.
@Eric, i didn't get this, if _WAK is not required when why you think _PTS is required ? Can you please let me know what you want achieve using _PTS ? May be i'm missing something here :(
We want enable PM before suspend, this is why we need _PTS. I think we don't have S3 handler in the coreboot. And for the S3 resume it will call bootblock and this will do the things we want do in _WAK which is store the PM bits from device tree.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
looks like its common vs specific SOC changes issue
now we have 2 option
Do you really need to save and restore GPIO PM setting in _PTS and _WAK, because _PTS/_WAK will only call for S5/S3 case where anyway your bootblock changes are in place to fix GPIO PM issue, isn't its duplicate ? i guess you have to only bother about MPTS/MWAK ?
Like SKL has its own platform.asl rather using common one. Its not feasible to make GPIO PM override feature flag for APL where this feature doesn't exist.
I think the bootblock is called when resume not when suspend. _WAK is not needed, but _PTS still needed.
@Eric, i didn't get this, if _WAK is not required when why you think _PTS is required ? Can you please let me know what you want achieve using _PTS ? May be i'm missing something here :(
We want enable PM before suspend, this is why we need _PTS.
Any idea why we might want to do this? i mean what would be the benefit because during resume bootblock code will override based on .cb config.
I think we don't have S3 handler in the coreboot.
can you please take a look into this https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
we can create a soc callback from this common code to soc if required for S3 and S5 case. These are S3/S5 handler for coreboot.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
@Subrata, I found the issue here. APL use the common platform.asl but not use the common globalnvs.asl that cause this issue. How can we fix this?
looks like its common vs specific SOC changes issue
now we have 2 option
Do you really need to save and restore GPIO PM setting in _PTS and _WAK, because _PTS/_WAK will only call for S5/S3 case where anyway your bootblock changes are in place to fix GPIO PM issue, isn't its duplicate ? i guess you have to only bother about MPTS/MWAK ?
Like SKL has its own platform.asl rather using common one. Its not feasible to make GPIO PM override feature flag for APL where this feature doesn't exist.
I think the bootblock is called when resume not when suspend. _WAK is not needed, but _PTS still needed.
@Eric, i didn't get this, if _WAK is not required when why you think _PTS is required ? Can you please let me know what you want achieve using _PTS ? May be i'm missing something here :(
We want enable PM before suspend, this is why we need _PTS.
Any idea why we might want to do this? i mean what would be the benefit because during resume bootblock code will override based on .cb config.
I think we don't have S3 handler in the coreboot.
can you please take a look into this https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
we can create a soc callback from this common code to soc if required for S3 and S5 case. These are S3/S5 handler for coreboot.
I knew this, but we don't use it. We can discuss with google about this. Since @Tim added this. S3 always be a back up for s0ix. And IMO, we rarely use it expect for validation. So you mean I just remove this for PTS and WAK? Or you will add a flag like I suggested. I am fine with any.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
@Subrata, another point is for better debugging and maintaining. If we put the same code in asl and semi handler, it will makes some difficult to trace issue.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
(10 comments)
https://review.coreboot.org/c/coreboot/+/37685/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/13//COMMIT_MSG@9 PS13, Line 9: Enable/disable GPIO clock gating when enter/exit s0ix is common : request on CNL/CML. Move it from board level to soc level. This description does not really capture the changes that are being done in this CL.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 171: Store (GPID (Local0), Local1) : Store (PCRR (Local1, GPIO_MISCCFG), Index(GPMB, Local0)) Can this be simply written as:
Local1 = GPID (Local0) GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG)
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 172: Store (PCRR (Local1, GPIO_MISCCFG), Index(GPMB, Local0)) Aren't you saving the entire GPIO_MISCCFG register here and not just the GPIO PM config bits?
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 183: Index(GPMB, Local0) Can this be simply written as "GPMB[Local0]" in ASL2.0 syntax?
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 192: /* Store current setting */ : SGPM () Why is store called within a method that is supposed to "Enable GPIO Power Management bits"?
Comment for the method should be updated to clearly reflect what the method is doing i.e. it is not just enabling GPIO PM bits.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 79: Enable GPIO PM This is not just enable. It also saves the current state of GPIO PM bits.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 79: * space before *
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 80: OGPM I don't understand why we need to have NVS now? Can't we just always follow the policy of saving and enabling GPIO PM bits when entering S0ix and restoring when exiting S0ix?
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 100: OGPM Same question as above.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/common/acpi/... PS13, Line 46: * space before *
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 80: OGPM
I don't understand why we need to have NVS now? Can't we just always follow the policy of saving and […]
@Furquan, this is Tim wanted. I think it okay to let mainboard can choice they want this or not.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 100: OGPM
Same question as above.
Ack
Hello Patrick Rudolph, Kane Chen, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#14).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable GPIO clock gating when enter s0ix/Sx and save the PM bits. Restore the PM bits when exit s0ix/Sx.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/14
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(7 comments)
https://review.coreboot.org/c/coreboot/+/37685/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/13//COMMIT_MSG@9 PS13, Line 9: Enable/disable GPIO clock gating when enter/exit s0ix is common : request on CNL/CML. Move it from board level to soc level.
This description does not really capture the changes that are being done in this CL.
Done
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 171: Store (GPID (Local0), Local1) : Store (PCRR (Local1, GPIO_MISCCFG), Index(GPMB, Local0))
Can this be simply written as: […]
Done
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 192: /* Store current setting */ : SGPM ()
Why is store called within a method that is supposed to "Enable GPIO Power Management bits"? […]
Done
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 79: Enable GPIO PM
This is not just enable. It also saves the current state of GPIO PM bits.
Done
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 79: *
space before *
Done
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 80: OGPM
@Furquan, this is Tim wanted. I think it okay to let mainboard can choice they want this or not.
Ack
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/common/acpi/... PS13, Line 46: *
space before *
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/14/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/14/src/soc/intel/cannonlake/a... PS14, Line 192: /* Save current setting and using when resume */ correct the word later. replace using to restore.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
Exactly. […]
but I feel like each mainboard should be able to choose if it wants to change the PM bits at S0ix entry/exit.
I agree with this statement
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
but I feel like each mainboard should be able to choose if it wants to change the PM bits at S0i […]
But, the implementation is conflating the "override of GPIO PM bits in coreboot" and "save/restore PM bits across S0ix entry/exit". As I understand it, the config in devicetree currently is meant to indicate to common SoC code that the mainboard wants to use its own version of GPIO PM bits when booting up.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
As I understand it, the config in devicetree currently is meant to indicate to common SoC code that the mainboard wants to use its own version of GPIO PM bits when booting up.
Yes, i guess that should be the direction and having a kconfig and gpio_override chip.h variable might serves same purpose
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
As I understand it, the config in devicetree currently is meant to indicate to common SoC code tha […]
In general, I think it should be okay to just have a policy that all SoCs which support GPIO PM bits will save/restore those bits across S0ix entry/exit. Saves us from having another config and NVS variable when all boards would just want to set it (keeps the implementation simple).
Thinking about it some more -- There are currently two possibilities: a) Mainboard doesn't care about overriding GPIO PM bits. In this case, gpio_override_pm would be set to 0. This will set GPIO PM bits to MISCCFG_ENABLE_GPIO_PM_CONFIG. In this case, doing the save/restore would be simply a NOP.
b) Mainboard sets gpio_override_pm. In this case, GPIO PM bits would be set to something other than MISCCFG_ENABLE_GPIO_PM_CONFIG. As per current implementation, gpio_override_pm is used as an indication from mainboard that PM bits should be saved/restored.
Thus, I don't see a lot of advantage having the added complexity of a new NVS variable and using that to decide whether we should save/restore GPIO PM bits. Instead, doing it unconditionally would be just okay. In the future, if we really come across a board that needs to avoid save/restore of GPIO PM bits, we would anyways have to add a new chip config for that.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
In general, I think it should be okay to just have a policy that all SoCs which support GPIO PM bits […]
@Furquan, I want to discuss the final goal here. Do we want let only "coreboot" configure the PM bit without FSP-S, right? If so, gpio_override_pm flag is meaningful. Otherwise, it not so significant like you said.
Here is my point, if we let coreboot configure the PM bits, means we will use it in devicetree.cb in all boards come after. So we don't have to add condition in coreboot but with a choice in the OS level for s0ix/Sx.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
Do we want let only "coreboot" configure the PM bit without FSP-S, right? If so, gpio_override_pm flag is meaningful. Otherwise, it not so significant like you said.
Actually, gpio_override_pm config means that the mainboard wants to override the GPIO PM config bits. If that is not set, then SoC would by default set the PM bits to MISCCFG_ENABLE_GPIO_PM_CONFIG. Thus, in both cases, we don't care about what FSP is doing. Coreboot itself is doing the GPIO PM bit configuration in SoC code. Either the PM bits are set to MISCCFG_ENABLE_GPIO_PM_CONFIG if mainboard does not want an override. Else, if mainboard wants an override, it can set gpio_override_pm and provide the values of PM bits.
Here is my point, if we let coreboot configure the PM bits, means we will use it in devicetree.cb in all boards come after.
Not necessary. Coreboot would always configure GPIO PM bits. Whether it would be MISCCFG_ENABLE_GPIO_PM_CONFIG or something that mainboard provides depends on the value of gpio_override_pm.
So we don't have to add condition in coreboot but with a choice in the OS level for s0ix/Sx.
Where in the OS? Sorry, I don't think I understand this statement.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
Do we want let only "coreboot" configure the PM bit without FSP-S, right? If so, gpio_override_pm […]
So, do we really need gpio_override_pm flag in coreboot? Anyway we will program it as no matter default(MISCCFG_ENABLE_GPIO_PM_CONFIG) or others. We can just leave this in device tree for all boards, so we don't care the FSP, right?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
So, do we really need gpio_override_pm flag in coreboot? Anyway we will program it as no matter default(MISCCFG_ENABLE_GPIO_PM_CONFIG) or others. We can just leave this in device tree for all boards, so we don't care the FSP, right?
Reason to have the gpio_override_pm is boards that want to stick to using MISCCFG_ENABLE_GPIO_PM_CONFIG. They technically don't need the override flag to be set in devicetree or any other PM bits to be provided by mainboard. It needs to be set only by those boards that need some configuration that is different than the default.
I think most Intel mainboards wouldn't want to use this override.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
So, do we really need gpio_override_pm flag in coreboot? Anyway we will program it as no matter de […]
Oh, I thought Intel put configure gpio PM code in common, but only in CNL. Because I always think if we can put the GPIO PM bit in all devicetree.cb then we don't care the override flag. I can accept this and agree we can add this if necessary. I will change back if Tim is in the team as well :)
Hello Patrick Rudolph, Kane Chen, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#15).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable GPIO clock gating when enter s0ix/Sx and save the PM bits. Restore the PM bits when exit s0ix/Sx.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/15
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 15:
Since about midnight in Taiwan. I pushed it in advanced for review. And let it open for Tim's opinion. Nice to discuss with you all. It always learn something, and sorry for my poor English. I was try hard to express my thoughts.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 15:
Patch Set 15:
Since about midnight in Taiwan. I pushed it in advanced for review. And let it open for Tim's opinion. Nice to discuss with you all. It always learn something, and sorry for my poor English. I was try hard to express my thoughts.
I'll defer to Furquan's expertise here :)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 15: Code-Review+2
(5 comments)
Thanks for the patience with the back and forth Eric and Subrata!
Eric: some minor nits, but otherwise LGTM.
https://review.coreboot.org/c/coreboot/+/37685/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/15//COMMIT_MSG@13 PS15, Line 13: measure power consumption under s0ix Can you please confirm the following things: 1. Check that GPIO PM bits are configured as per the board configuration in devicetree even after jumping to OS. This can be done by dumping MISCCFG registers for all communities
2. Check that GPIO PM bits are configured as per the board configuration in devicetree after a cycle of S0ix enter/exit. Dump MISCCFG registers for all communities before entering S0ix and after exiting S0ix.
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 188: Enable GPIO Power Management bits and save the current setting nit: Order is not correct ;). It should be "Save current setting of GPIO Power Management bits and enable all Power Management bits for all communities"
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 192: * nit: space before *
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 79: Enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG and : * save the current PM bits nit: Order is reversed.
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/common/acpi/... PS15, Line 46: Enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG and : * save the current PM bits nit: order is reversed.
Hello Patrick Rudolph, Kane Chen, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#16).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable GPIO clock gating when enter s0ix/Sx and save the PM bits. Restore the PM bits when exit s0ix/Sx.
BUG=b:144002424 TEST=measure power consumption under s0ix
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/16
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 16: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 16:
(9 comments)
https://review.coreboot.org/c/coreboot/+/37685/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/15//COMMIT_MSG@13 PS15, Line 13: measure power consumption under s0ix
Can you please confirm the following things: […]
I will add print in the SGPM and RGPM. Feedback to you later. I will find _PS0 method to print as well.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 172: Store (PCRR (Local1, GPIO_MISCCFG), Index(GPMB, Local0))
Aren't you saving the entire GPIO_MISCCFG register here and not just the GPIO PM config bits?
Done
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 183: Index(GPMB, Local0)
Can this be simply written as "GPMB[Local0]" in ASL2. […]
Done
https://review.coreboot.org/c/coreboot/+/37685/14/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/14/src/soc/intel/cannonlake/a... PS14, Line 192: /* Save current setting and using when resume */
correct the word later. replace using to restore.
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 188: Enable GPIO Power Management bits and save the current setting
nit: Order is not correct ;). […]
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 192: *
nit: space before *
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 79: Enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG and : * save the current PM bits
nit: Order is reversed.
Done
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
Oh, I thought Intel put configure gpio PM code in common, but only in CNL. […]
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/common/acpi/... PS15, Line 46: Enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG and : * save the current PM bits
nit: order is reversed.
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 16: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 16:
Oh, I think I have some issue with the buffer. I can't convert it for print. If I use printf %o, it will show refer object.. Still trying.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 16:
BUFFER is 8bit and PCRR return 32bit... I need add filter ;(
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/16/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/16/src/soc/intel/cannonlake/a... PS16, Line 172: GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG) to be fixed here
Hello Patrick Rudolph, Subrata Banik, Kane Chen, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37685
to look at the new patch set (#17).
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable GPIO clock gating when enter s0ix/Sx and save the PM bits. Restore the PM bits when exit s0ix/Sx.
BUG=b:144002424 TEST=Check GPIO PM bits when enter/exit s0ix are expected
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37685/17
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
(1 comment)
Change Buffer to Package that can apply Integer. So we don't do any convert. Verified the PM bits when enter OS, before s0ix and after. Also verify after Enable PM bits as well. The result is good :) Merry X'max to you all.
https://review.coreboot.org/c/coreboot/+/37685/16/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/16/src/soc/intel/cannonlake/a... PS16, Line 172: GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG)
to be fixed here
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Change Buffer to Package that can apply Integer. So we don't do any convert.
Sorry, I don't understand why Buffer was changed to Package?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
Change Buffer to Package that can apply Integer. So we don't do any convert.
Sorry, I don't understand why Buffer was changed to Package?
PCRR return Integer and one buffer is a byte.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
Change Buffer to Package that can apply Integer. So we don't do any convert.
Sorry, I don't understand why Buffer was changed to Package?
If I use buffer will cause CGPM issue exception. And I can't print the buffer neither. So I guess it's the type issue. Then I change it to package, and no issue there.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
Change Buffer to Package that can apply Integer. So we don't do any convert.
Sorry, I don't understand why Buffer was changed to Package?
Another reason I guess is Arg1 will treat as pointer. So if I use GPMB[local] as parameter will cause the memory issue.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
Change Buffer to Package that can apply Integer. So we don't do any convert.
Sorry, I don't understand why Buffer was changed to Package?
Another interesting thing is we may consider the object. I can't printf %o GPMB[local] or DeRefOf(GPMB[local]). The log message of both will show [reference object] not value. If I used package GPMB[local] will show [reference object] and DeRefOf(GPMB[local]) will show the value :) You can play it if you have interesting as well.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
PCRR return Integer and one buffer is a byte.
Yes, that is correct. If you intend to use Buffer(), then it will have to be something like this: GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG) & MISCCFG_ENABLE_GPIO_PM_CONFIG to ensure that the data is smaller than a byte.
Another interesting thing is we may consider the object. I can't printf %o GPMB[local] or DeRefOf(GPMB[local]). The log message of both will show [reference object] not value
That doesn't sound correct. For both Buffer() and Package(), the right way to access an element is using DerefOf(). Section 5.4 of this doc has some good examples: https://acpica.org/sites/acpica/files/asl_tutorial_v20190625.pdf
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
PCRR return Integer and one buffer is a byte.
Yes, that is correct. If you intend to use Buffer(), then it will have to be something like this: GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG) & MISCCFG_ENABLE_GPIO_PM_CONFIG to ensure that the data is smaller than a byte.
Another interesting thing is we may consider the object. I can't printf %o GPMB[local] or DeRefOf(GPMB[local]). The log message of both will show [reference object] not value
That doesn't sound correct. For both Buffer() and Package(), the right way to access an element is using DerefOf(). Section 5.4 of this doc has some good examples: https://acpica.org/sites/acpica/files/asl_tutorial_v20190625.pdf
I tried the & but I checked the ACPI SPEC, it still an Integer operation. Because PCRR is return Integer. I have no idea to convert the Integer to byte, so I choice use the Package that we can avoid any convert. Yap, I thought DeRefOf works on Buffer as well but not in this case. Maybe it already has problem after I made it buffer overflow. I will try this if I have time. @Furquan, Have a great vacation. Thanks a lot.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
PCRR return Integer and one buffer is a byte.
Yes, that is correct. If you intend to use Buffer(), then it will have to be something like this: GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG) & MISCCFG_ENABLE_GPIO_PM_CONFIG to ensure that the data is smaller than a byte.
Another interesting thing is we may consider the object. I can't printf %o GPMB[local] or DeRefOf(GPMB[local]). The log message of both will show [reference object] not value
That doesn't sound correct. For both Buffer() and Package(), the right way to access an element is using DerefOf(). Section 5.4 of this doc has some good examples: https://acpica.org/sites/acpica/files/asl_tutorial_v20190625.pdf
I tried Local2 = PCRR (Local1, GPIO_MISCCFG) & MISCCFG_ENABLE_GPIO_PM_CONFIG then print Local2. It seems still a whole Integer. Below is spec in ACPI6.0 AndTerm := And ( Source1, // TermArg => Integer Source2, // TermArg => Integer Result // Target ) => Integer
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 17:
Patch Set 17:
Patch Set 17:
PCRR return Integer and one buffer is a byte.
Yes, that is correct. If you intend to use Buffer(), then it will have to be something like this: GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG) & MISCCFG_ENABLE_GPIO_PM_CONFIG to ensure that the data is smaller than a byte.
Another interesting thing is we may consider the object. I can't printf %o GPMB[local] or DeRefOf(GPMB[local]). The log message of both will show [reference object] not value
That doesn't sound correct. For both Buffer() and Package(), the right way to access an element is using DerefOf(). Section 5.4 of this doc has some good examples: https://acpica.org/sites/acpica/files/asl_tutorial_v20190625.pdf
I tried Local2 = PCRR (Local1, GPIO_MISCCFG) & MISCCFG_ENABLE_GPIO_PM_CONFIG then print Local2. It seems still a whole Integer. Below is spec in ACPI6.0 AndTerm := And ( Source1, // TermArg => Integer Source2, // TermArg => Integer Result // Target ) => Integer
Thanks for checking that Eric. I think it is okay to go ahead with Package() instead of Buffer(). You too have a great vacation!
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
soc/intel/cannonlake: Move GPIO PM configuration to soc level
Enable GPIO clock gating when enter s0ix/Sx and save the PM bits. Restore the PM bits when exit s0ix/Sx.
BUG=b:144002424 TEST=Check GPIO PM bits when enter/exit s0ix are expected
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I120f8369b8d3cf7ac821332bdfa124f6ed0570e9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/37685 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/cannonlake/acpi/gpio.asl M src/soc/intel/cannonlake/acpi/lpit.asl M src/soc/intel/common/acpi/platform.asl 3 files changed, 73 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/acpi/gpio.asl b/src/soc/intel/cannonlake/acpi/gpio.asl index 65332ad..7702ad5 100644 --- a/src/soc/intel/cannonlake/acpi/gpio.asl +++ b/src/soc/intel/cannonlake/acpi/gpio.asl @@ -157,3 +157,44 @@ PCRO (Local0, GPIO_MISCCFG, And (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG)) } } + +/* GPIO Power Management bits */ +Name(GPMB, Package(TOTAL_GPIO_COMM) {0}) + +/* + * Save GPIO Power Management bits + */ +Method (SGPM, 0, Serialized) +{ + For (Local0 = 0, Local0 < TOTAL_GPIO_COMM, Local0++) + { + Local1 = GPID (Local0) + GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG) + } +} + +/* + * Restore GPIO Power Management bits + */ +Method (RGPM, 0, Serialized) +{ + For (Local0 = 0, Local0 < TOTAL_GPIO_COMM, Local0++) + { + CGPM (Local0, DerefOf(GPMB[Local0])) + } +} + +/* + * Save current setting of GPIO Power Management bits and + * enable all Power Management bits for all communities + */ +Method (EGPM, 0, Serialized) +{ + /* Save current setting and will restore it when resuming */ + SGPM () + /* Enable PM bits */ + For (Local0 = 0, Local0 < TOTAL_GPIO_COMM, Local0++) + { + CGPM (Local0, MISCCFG_ENABLE_GPIO_PM_CONFIG) + } +} diff --git a/src/soc/intel/cannonlake/acpi/lpit.asl b/src/soc/intel/cannonlake/acpi/lpit.asl index 74d4fe6..e0e23ca 100644 --- a/src/soc/intel/cannonlake/acpi/lpit.asl +++ b/src/soc/intel/cannonlake/acpi/lpit.asl @@ -16,6 +16,8 @@
External(_SB.MS0X, MethodObj) External(_SB.PCI0.LPCB.EC0.S0IX, MethodObj) +External(_SB.PCI0.EGPM, MethodObj) +External(_SB.PCI0.RGPM, MethodObj)
scope(_SB) { @@ -73,6 +75,15 @@ If (CondRefOf (_SB.MS0X)) { _SB.MS0X(1) } + + /* + * Save the current PM bits then + * enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG + */ + If (CondRefOf (_SB.PCI0.EGPM)) + { + _SB.PCI0.EGPM () + } } /* * Function 6 - Low Power S0 Exit Notification @@ -87,6 +98,12 @@ If (CondRefOf (_SB.MS0X)) { _SB.MS0X(0) } + + /* Restore GPIO all Community PM */ + If (CondRefOf (_SB.PCI0.RGPM)) + { + _SB.PCI0.RGPM () + } } } Return(Buffer(One) {0x00}) diff --git a/src/soc/intel/common/acpi/platform.asl b/src/soc/intel/common/acpi/platform.asl index 9aa2edc..c41ccbe 100644 --- a/src/soc/intel/common/acpi/platform.asl +++ b/src/soc/intel/common/acpi/platform.asl @@ -19,6 +19,8 @@
External(_SB.MPTS, MethodObj) External(_SB.MWAK, MethodObj) +External(_SB.PCI0.EGPM, MethodObj) +External(_SB.PCI0.RGPM, MethodObj)
/* Port 80 POST */
@@ -41,6 +43,14 @@ { _SB.MPTS (Arg0) } + /* + * Save the current PM bits then + * enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG + */ + If (CondRefOf (_SB.PCI0.EGPM)) + { + _SB.PCI0.EGPM () + } }
/* The _WAK method is called on system wakeup */ @@ -53,6 +63,11 @@ { _SB.MWAK (Arg0) } + /* Restore GPIO all Community PM */ + If (CondRefOf (_SB.PCI0.RGPM)) + { + _SB.PCI0.RGPM () + }
Return (Package(){0,0}) }
-
Duncan Laurie (Code Review) -
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Kane Chen (Code Review)
-
Mathew King (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)