Hello Martin Roth, Marshall Dawson, Rob Barnes,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41437
to review the following change.
Change subject: soc/amd/picasso/acpi: _PIC method must be at root ......................................................................
soc/amd/picasso/acpi: _PIC method must be at root
The _PIC method sets the interrupt model (PIC or APIC). It needs to be defined at the root level for the kernel to find it. Previously this method was never getting called, so we were always stuck in APIC mode.
BUG=b:139429446 BRANCH=none TEST=Saw the method getting called [ 1.251774] ACPI Debug: "PIC MODE: 0000000000000001"
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: Idd5e9646df8d56e7cbec2be8b4016c36d81e5fb8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Reviewed-by: Marshall Dawson marshall.dawson@amd.corp-partner.google.com Reviewed-by: Rob Barnes robbarnes@google.com Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/picasso/acpi/pci_int.asl 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/41437/1
diff --git a/src/soc/amd/picasso/acpi/pci_int.asl b/src/soc/amd/picasso/acpi/pci_int.asl index 0f3d882..f89a14e 100644 --- a/src/soc/amd/picasso/acpi/pci_int.asl +++ b/src/soc/amd/picasso/acpi/pci_int.asl @@ -103,12 +103,13 @@ P3PR, 1, }
- Method(_PIC, 0x01, NotSerialized) + Method(_PIC, 0x01, NotSerialized) { If (Arg0) { _SB.CIRQ() } + printf("PIC MODE: %o", Arg0) Store(Arg0, PMOD) }
Hello Martin Roth, Marshall Dawson, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41437
to look at the new patch set (#2).
Change subject: soc/amd/picasso/acpi: _PIC method must be at root ......................................................................
soc/amd/picasso/acpi: _PIC method must be at root
The _PIC method sets the interrupt model (PIC or APIC). It needs to be defined at the root level for the kernel to find it. Previously this method was never getting called, so we were always stuck in APIC mode.
BUG=b:139429446, b:147042464 BRANCH=none TEST=Saw the method getting called [ 1.251774] ACPI Debug: "PIC MODE: 0000000000000001"
Change-Id: Idd5e9646df8d56e7cbec2be8b4016c36d81e5fb8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Signed-off-by: Raul E Rangel rrangel@chromium.org --- M src/soc/amd/picasso/acpi/pci_int.asl 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/41437/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: _PIC method must be at root ......................................................................
Uploaded patch set 2: Commit message was updated.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: _PIC method must be at root ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... File src/soc/amd/picasso/acpi/pci_int.asl:
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... PS2, Line 112: printf("PIC MODE: %o", Arg0) remove?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: _PIC method must be at root ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... File src/soc/amd/picasso/acpi/pci_int.asl:
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... PS2, Line 112: printf("PIC MODE: %o", Arg0)
remove?
I decided to leave it. It will only be active if the kernel is compiled with ACPI_DEBUG and the ACPI layer is enabled.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: _PIC method must be at root ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41437/2//COMMIT_MSG@7 PS2, Line 7: soc/amd/picasso/acpi: _PIC method must be at root Please make it a statement about the change (and not problem description):
Define _PIC method at root level
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
Uploaded patch set 3.
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41437/2//COMMIT_MSG@7 PS2, Line 7: soc/amd/picasso/acpi: _PIC method must be at root
Please make it a statement about the change (and not problem description): […]
Done
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Rob Barnes, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41437
to look at the new patch set (#3).
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
soc/amd/picasso/acpi: Move _PIC method to root namespace
The _PIC method sets the interrupt model (PIC or APIC). It needs to be defined at the root level for the kernel to find it. Previously this method was never getting called, so we were always stuck in APIC mode.
BUG=b:139429446, b:147042464 BRANCH=none TEST=Saw the method getting called [ 1.251774] ACPI Debug: "PIC MODE: 0000000000000001"
Change-Id: Idd5e9646df8d56e7cbec2be8b4016c36d81e5fb8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Signed-off-by: Raul E Rangel rrangel@chromium.org --- M src/soc/amd/picasso/acpi/pci_int.asl 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/41437/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... File src/soc/amd/picasso/acpi/pci_int.asl:
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... PS2, Line 112: printf("PIC MODE: %o", Arg0)
I decided to leave it. […]
ok
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... File src/soc/amd/picasso/acpi/pci_int.asl:
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... PS2, Line 112: printf("PIC MODE: %o", Arg0)
ok
Felix you should mark these as resolved then.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... File src/soc/amd/picasso/acpi/pci_int.asl:
https://review.coreboot.org/c/coreboot/+/41437/2/src/soc/amd/picasso/acpi/pc... PS2, Line 112: printf("PIC MODE: %o", Arg0)
Felix you should mark these as resolved then.
not sure what you mean; it is marked as resolved here
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
soc/amd/picasso/acpi: Move _PIC method to root namespace
The _PIC method sets the interrupt model (PIC or APIC). It needs to be defined at the root level for the kernel to find it. Previously this method was never getting called, so we were always stuck in APIC mode.
BUG=b:139429446, b:147042464 BRANCH=none TEST=Saw the method getting called [ 1.251774] ACPI Debug: "PIC MODE: 0000000000000001"
Change-Id: Idd5e9646df8d56e7cbec2be8b4016c36d81e5fb8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41437 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/acpi/pci_int.asl 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/soc/amd/picasso/acpi/pci_int.asl b/src/soc/amd/picasso/acpi/pci_int.asl index 0f3d882..f89a14e 100644 --- a/src/soc/amd/picasso/acpi/pci_int.asl +++ b/src/soc/amd/picasso/acpi/pci_int.asl @@ -103,12 +103,13 @@ P3PR, 1, }
- Method(_PIC, 0x01, NotSerialized) + Method(_PIC, 0x01, NotSerialized) { If (Arg0) { _SB.CIRQ() } + printf("PIC MODE: %o", Arg0) Store(Arg0, PMOD) }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41437 )
Change subject: soc/amd/picasso/acpi: Move _PIC method to root namespace ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3586 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3585 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3584 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3583
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Felix Held (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)