Change in coreboot[master]: soc/amd/common/acpi: Create platform.asl to define acpi transitions
Josie Nordrum has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
soc/amd/common/acpi: Create platform.asl to define acpi transitions
Define _WAK, _PTS, and _INI acpi Methods for device.
BUG=b:158087989 BRANCH=Zork TEST=tested backlight during reboot and suspend
Signed-Off-By: Josie Nordrum josienordrum@google.com Change-Id: I8020173a15db1d310459d5c1de3600949b173b00 --- A src/soc/amd/common/acpi/platform.asl 1 file changed, 28 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/46669/1
diff --git a/src/soc/amd/common/acpi/platform.asl b/src/soc/amd/common/acpi/platform.asl new file mode 100644 index 0000000..c0fd8fa --- /dev/null +++ b/src/soc/amd/common/acpi/platform.asl @@ -0,0 +1,28 @@ +External(_SB.MPTS, MethodObj) +External(_SB.MWAK, MethodObj) +External(_SB.MINI, MethodObj) + +/* Platform initialization methods */ +Method (_INI, 0, NotSerialized) +{ + If (CondRefOf (_SB.MINI)) { + _SB.MINI() + } +} + +/* Platform-wide wake methods */ +Method (_WAK, 1, NotSerialized) +{ + If (CondRefOf (_SB.MWAK)){ + _SB.MWAK() + } + Return (Package (){ 0, 0 }) +} + +/* Platform-wide Put To Sleep (suspend) methods */ +Method (_PTS, 1, NotSerialized) +{ + If (CondRefOf (_SB.MPTS)){ + _SB.MPTS() + } +} \ No newline at end of file
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46669/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46669/1//COMMIT_MSG@9 PS1, Line 9: Define _WAK, _PTS, and _INI acpi Methods for device. Add detail that it makes callbacks into mainboard methods if provided.
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
PS1: License is missing.
/* SPDX-License-Identifier: GPL-2.0-only */
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 1: External(_SB.MPTS, MethodObj) : External(_SB.MWAK, MethodObj) : External(_SB.MINI, MethodObj) Can you please add a comment indicating that these are callback methods that a mainboard is expected to implement.
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 6: \ I think _INI needs to be under a device. Probably: Scope (_SB) { Method (_INI, ...
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 16: { space before {
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 25: { space before {
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46669
to look at the new patch set (#2).
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
soc/amd/common/acpi: Create platform.asl to define acpi transitions
Define device _WAK, _PTS, and _INI acpi methods with callbacks into mainboard methods if provided.
BUG=b:158087989 BRANCH=Zork TEST=tested backlight during reboot and suspend
Signed-Off-By: Josie Nordrum josienordrum@google.com Change-Id: I8020173a15db1d310459d5c1de3600949b173b00 --- A src/soc/amd/common/acpi/platform.asl 1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/46669/2
Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46669/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46669/1//COMMIT_MSG@9 PS1, Line 9: Define _WAK, _PTS, and _INI acpi Methods for device.
Add detail that it makes callbacks into mainboard methods if provided.
Done
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
PS1:
License is missing. […]
Done
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 1: External(_SB.MPTS, MethodObj) : External(_SB.MWAK, MethodObj) : External(_SB.MINI, MethodObj)
Can you please add a comment indicating that these are callback methods that a mainboard is expected […]
Done
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 6: \
I think _INI needs to be under a device. Probably: […]
Done
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 16: {
space before {
Done
https://review.coreboot.org/c/coreboot/+/46669/1/src/soc/amd/common/acpi/pla... PS1, Line 25: {
space before {
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/46669/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46669/2//COMMIT_MSG@16 PS2, Line 16: Signed-Off-By Jenkins is complaining: "No Signed-off-by line in commit message"
O and B need to be lowercase. "Signed-off-by"
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... PS2, Line 8: { nit: space before {
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 2:
(2 comments)
You'll need to rebase this patch set on the current ToT. The patch that it's currently based on isn't building correctly.
https://review.coreboot.org/c/coreboot/+/46669/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46669/2//COMMIT_MSG@16 PS2, Line 16: Signed-Off-By
Jenkins is complaining: "No Signed-off-by line in commit message" […]
Git will do this for you - you don't need to do it by hand. git commit -s
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... PS2, Line 8: Scope(_SB){ Why does _INI need to go in this scope? We define the method at the root level, so it doesn't seem like it needs a different scope.
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46669
to look at the new patch set (#4).
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
soc/amd/common/acpi: Create platform.asl to define acpi transitions
Define device _WAK, _PTS, and _INI acpi methods with callbacks into mainboard methods if provided.
BUG=b:158087989 BRANCH=Zork TEST=tested backlight during reboot and suspend
Signed-off-by: Josie Nordrum josienordrum@google.com Change-Id: I8020173a15db1d310459d5c1de3600949b173b00 --- A src/soc/amd/common/acpi/platform.asl 1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/46669/4
Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46669/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46669/2//COMMIT_MSG@16 PS2, Line 16: Signed-Off-By
Git will do this for you - you don't need to do it by hand. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... PS2, Line 8: Scope(_SB){
Why does _INI need to go in this scope? We define the method at the root level, so it doesn't seem […]
Yes, it needs to be either _INI under _SB or _INI. I was not sure if _INI at root level works okay or if it needs to be under non-root device. Hence, I suggested adding _SB. With the _SB scope, the method should be just _INI
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46669
to look at the new patch set (#5).
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
soc/amd/common/acpi: Create platform.asl to define acpi transitions
Define device _WAK, _PTS, and _INI acpi methods with callbacks into mainboard methods if provided.
BUG=b:158087989 BRANCH=Zork TEST=tested backlight during reboot and suspend
Signed-off-by: Josie Nordrum josienordrum@google.com Change-Id: I8020173a15db1d310459d5c1de3600949b173b00 --- A src/soc/amd/common/acpi/platform.asl 1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/46669/5
Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... PS2, Line 8: Scope(_SB){
Yes, it needs to be either _INI under _SB or _INI. […]
Done
https://review.coreboot.org/c/coreboot/+/46669/2/src/soc/amd/common/acpi/pla... PS2, Line 8: {
nit: space before {
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46669/5/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/46669/5/src/soc/amd/common/acpi/pla... PS5, Line 8: I think the comment on earlier patchset wasn't visible correctly on gerrit. The space isn't needed before _SB but between Scope and ( and between ) and {.
i.e. Scope (_SB) {
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46669
to look at the new patch set (#6).
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
soc/amd/common/acpi: Create platform.asl to define acpi transitions
Define device _WAK, _PTS, and _INI acpi methods with callbacks into mainboard methods if provided.
BUG=b:158087989 BRANCH=Zork TEST=tested backlight during reboot and suspend
Signed-off-by: Josie Nordrum josienordrum@google.com Change-Id: I8020173a15db1d310459d5c1de3600949b173b00 --- A src/soc/amd/common/acpi/platform.asl 1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/46669/6
Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46669/5/src/soc/amd/common/acpi/pla... File src/soc/amd/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/46669/5/src/soc/amd/common/acpi/pla... PS5, Line 8:
I think the comment on earlier patchset wasn't visible correctly on gerrit. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
Patch Set 6: Code-Review+2
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46669 )
Change subject: soc/amd/common/acpi: Create platform.asl to define acpi transitions ......................................................................
soc/amd/common/acpi: Create platform.asl to define acpi transitions
Define device _WAK, _PTS, and _INI acpi methods with callbacks into mainboard methods if provided.
BUG=b:158087989 BRANCH=Zork TEST=tested backlight during reboot and suspend
Signed-off-by: Josie Nordrum josienordrum@google.com Change-Id: I8020173a15db1d310459d5c1de3600949b173b00 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46669 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- A src/soc/amd/common/acpi/platform.asl 1 file changed, 33 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/common/acpi/platform.asl b/src/soc/amd/common/acpi/platform.asl new file mode 100644 index 0000000..6db12e3 --- /dev/null +++ b/src/soc/amd/common/acpi/platform.asl @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* Callback methods to be implemented by mainboard */ +External(_SB.MPTS, MethodObj) +External(_SB.MWAK, MethodObj) +External(_SB.MINI, MethodObj) + +Scope (_SB){ + /* Platform initialization methods */ + Method (_INI, 0, NotSerialized) + { + If (CondRefOf (_SB.MINI)) { + _SB.MINI() + } + } +} + +/* Platform-wide wake methods */ +Method (_WAK, 1, NotSerialized) +{ + If (CondRefOf (_SB.MWAK)) { + _SB.MWAK() + } + Return (Package (){ 0, 0 }) +} + +/* Platform-wide Put To Sleep (suspend) methods */ +Method (_PTS, 1, NotSerialized) +{ + If (CondRefOf (_SB.MPTS)) { + _SB.MPTS() + } +}
-
Furquan Shaikh (Code Review) -
Josie Nordrum (Code Review)
-
Martin Roth (Code Review)
-
Tim Wawrzynczak (Code Review)