Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add function to get sleep number from Pm1 ......................................................................
cpu/x86/acpi: Add function to get sleep number from Pm1
Add an inline function to convert the value in Pm1Cnt[SlpTyp] to an integer guaranteed to equal the sleep number. That is, for example, a return value 3 always means S3. This can be used instead of relying on the enuerated values remaining consistent over time.
BUG=b:153854742 TEST=Verify S3 works using other WIP changes
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 --- M src/arch/x86/include/arch/acpi.h 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40338/1
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 644f52f..9f445d8 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -963,7 +963,7 @@
#if CONFIG(ACPI_INTEL_HARDWARE_SLEEP_VALUES) \ || CONFIG(ACPI_AMD_HARDWARE_SLEEP_VALUES) -/* Given the provided PM1 control register return the ACPI sleep type. */ +/* Given the provided PM1 control register return the ACPI sleep type enum. */ static inline int acpi_sleep_from_pm1(uint32_t pm1_cnt) { switch (((pm1_cnt) & SLP_TYP) >> SLP_TYP_SHIFT) { @@ -975,6 +975,19 @@ } return -1; } + +/* Given the provided PM1 control register return the ACPI sleep number. */ +static inline int acpi_snum_from_pm1(uint32_t pm1_cnt) +{ + switch (((pm1_cnt) & SLP_TYP) >> SLP_TYP_SHIFT) { + case SLP_TYP_S0: return 0; + case SLP_TYP_S1: return 1; + case SLP_TYP_S3: return 3; + case SLP_TYP_S4: return 4; + case SLP_TYP_S5: return 5; + } + return -1; +} #endif
/* Returns ACPI_Sx values. */
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add function to get sleep number from Pm1 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... PS1, Line 956: ACPI_S0 Instead of adding acpi_snum_from_pm1, can we just guarantee the values of these? ACPI_S0 = 0, ACPI_S1 = 1, ACPI_S5 = 5,
It avoids the switch statement in `send_sx_info`
It would also be nice to name this enum so it could be used as a type. This doesn't have to be done in this CL though.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add function to get sleep number from Pm1 ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... PS1, Line 956: ACPI_S0
Instead of adding acpi_snum_from_pm1, can we just guarantee the values of these? […]
I made the same comment in a chromium CL. And add a comment that we want those explicit values.
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... PS1, Line 980: acpi_snum_from_pm1 This function is the exact same as acpi_sleep_from_pm1. If we want the reversal function we should we should add it accordingly. This function doesn't make sense in practice.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40338
to look at the new patch set (#2).
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
cpu/x86/acpi: Add assignments to ACPI_Sn enums
Explicitely assign numerical values to the enumerated sleep state values.
BUG=b:153854742
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 --- M src/arch/x86/include/arch/acpi.h 1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40338/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 2: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/2/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/2/src/arch/x86/include/arch/a... PS2, Line 956: ACPI_S0 = 0, Please add a comment like I noted previously. It should be explicitly commented that we desire and want these values for the enum.
Hello build bot (Jenkins), Raul Rangel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40338
to look at the new patch set (#3).
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
cpu/x86/acpi: Add assignments to ACPI_Sn enums
Explicitely assign numerical values to the enumerated sleep state values.
BUG=b:153854742
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 --- M src/arch/x86/include/arch/acpi.h 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40338/3
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/2/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/2/src/arch/x86/include/arch/a... PS2, Line 956: ACPI_S0 = 0,
Please add a comment like I noted previously. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 3: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 3: Code-Review+2
Felix Held has uploaded a new patch set (#4) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
cpu/x86/acpi: Add assignments to ACPI_Sn enums
Explicitely assign numerical values to the enumerated sleep state values.
BUG=b:153854742
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 --- M src/arch/x86/include/arch/acpi.h 1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40338/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... PS1, Line 956: ACPI_S0
I made the same comment in a chromium CL. And add a comment that we want those explicit values.
Done
https://review.coreboot.org/c/coreboot/+/40338/1/src/arch/x86/include/arch/a... PS1, Line 980: acpi_snum_from_pm1
This function is the exact same as acpi_sleep_from_pm1. […]
removed in later versions
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/4/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/4/src/arch/x86/include/arch/a... PS4, Line 955: enum { Your rebase lost Marshall's comment change..
Felix Held has uploaded a new patch set (#5) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
cpu/x86/acpi: Add assignments to ACPI_Sn enums
Explicitely assign numerical values to the enumerated sleep state values.
BUG=b:153854742
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 --- M src/arch/x86/include/arch/acpi.h 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40338/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/4/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/c/coreboot/+/40338/4/src/arch/x86/include/arch/a... PS4, Line 955: enum {
Your rebase lost Marshall's comment change..
fixed
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40338/5//COMMIT_MSG@9 PS5, Line 9: Explicitely Explicitly
Felix Held has uploaded a new patch set (#6) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
cpu/x86/acpi: Add assignments to ACPI_Sn enums
Explicitly assign numerical values to the enumerated sleep state values.
BUG=b:153854742
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 --- M src/arch/x86/include/arch/acpi.h 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/40338/6
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40338/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40338/5//COMMIT_MSG@9 PS5, Line 9: Explicitely
Explicitly
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40338 )
Change subject: cpu/x86/acpi: Add assignments to ACPI_Sn enums ......................................................................
cpu/x86/acpi: Add assignments to ACPI_Sn enums
Explicitly assign numerical values to the enumerated sleep state values.
BUG=b:153854742
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I1de2e7f65a2dc3f8a9a1c5fd83d164871a4a2b96 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40338 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/include/arch/acpi.h 1 file changed, 7 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 0ed89d1..fc250d7 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -979,13 +979,14 @@ void mainboard_suspend_resume(void); void *acpi_find_wakeup_vector(void);
+/* ACPI_Sn assignments are defined to always equal the sleep state numbers */ enum { - ACPI_S0, - ACPI_S1, - ACPI_S2, - ACPI_S3, - ACPI_S4, - ACPI_S5, + ACPI_S0 = 0, + ACPI_S1 = 1, + ACPI_S2 = 2, + ACPI_S3 = 3, + ACPI_S4 = 4, + ACPI_S5 = 5, };
#if CONFIG(ACPI_INTEL_HARDWARE_SLEEP_VALUES) \
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)