Change in coreboot[master]: arch/x86/acpi_device: Add a helper function to write PCI device
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR object defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/40674/1
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 9f1710e..9e7a58b 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */
+#include <assert.h> #include <string.h> #include <arch/acpi.h> #include <arch/acpi_device.h> @@ -927,3 +928,24 @@
return gpio; } + +void acpi_device_write_pci_dev(struct device *dev) +{ + const char *scope = acpi_device_scope(dev); + const char *name = acpi_device_name(dev); + + assert(dev->path.type == DEVICE_PATH_PCI); + assert(name); + assert(scope); + + if (!dev->enabled) + return; + + acpigen_write_scope(scope); + acpigen_write_device(name); + + acpigen_write_ADR_pci_device(dev); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ +} diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index e9c5cd4..7b9e169 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -501,4 +501,7 @@ /* Write Device Property hierarchy and clean up resources */ void acpi_dp_write(struct acpi_dp *table);
+/* Helper function to write a PCI device with _ADR object defined. */ +void acpi_device_write_pci_dev(struct device *dev); + #endif
Furquan Shaikh has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR object defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 36 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/40674/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 943: struct const struct ?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/include/arch/a... PS2, Line 504: PCI device It is worth mentioning here that if the device is created in the SSDT it can't be accessed from the DSDT, which may be perfectly fine depending on the setup and hopefully something we can get to more often by generating more of the code..
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 943: struct
const struct ?
This function can be used directly by .acpi_fill_ssdt which unfortunately has a function signature without const for the parameter. I plan to fix that as a wider clean up later on.
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/include/arch/a... PS2, Line 504: PCI device
It is worth mentioning here that if the device is created in the SSDT it can't be accessed from the […]
Good point. I will mention that.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 952: dev->enabled Since the device is still present and the kernel re-enumerates anyway, should we write out the node but have an _STA that says the device is disabled?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 952: dev->enabled
Since the device is still present and the kernel re-enumerates anyway, should we write out the node […]
Makes sense. We can use acpi_device_status() to determine that.
Hello build bot (Jenkins), Duncan Laurie, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40674
to look at the new patch set (#3).
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR object defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/40674/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 952: dev->enabled
Makes sense. We can use acpi_device_status() to determine that.
Done
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/include/arch/a... PS2, Line 504: PCI device
Good point. I will mention that.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 3: Code-Review+2
Hello build bot (Jenkins), Raul Rangel, Duncan Laurie, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40674
to look at the new patch set (#4).
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR object defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/40674/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 943: struct
This function can be used directly by . […]
Oh. You want to use it directly as acpi_fill_ssdt. Ya. That should be const. Do you want me to flip that?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/4/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/40674/4/src/arch/x86/include/arch/a... PS4, Line 507: Device that is added to SSDT cannot be accessed from DSDT. So, if there are any : * references to this PCI device required from static asl files, do not use this function and : * instead add the device to DSDT as well Can't an External reference be placed there and CondRefOf used to safely access it?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/4/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/40674/4/src/arch/x86/include/arch/a... PS4, Line 507: Device that is added to SSDT cannot be accessed from DSDT. So, if there are any : * references to this PCI device required from static asl files, do not use this function and : * instead add the device to DSDT as well
Can't an External reference be placed there and CondRefOf used to safely access it?
It's been a while I played with it. But I know that you cannot scope under device X in DSDT if that device is defined in SSDT. Probably reference might work, but something to test.
Hello build bot (Jenkins), Raul Rangel, Duncan Laurie, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40674
to look at the new patch set (#5).
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR object defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/40674/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/4/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/40674/4/src/arch/x86/include/arch/a... PS4, Line 507: Device that is added to SSDT cannot be accessed from DSDT. So, if there are any : * references to this PCI device required from static asl files, do not use this function and : * instead add the device to DSDT as well
It's been a while I played with it. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/2/src/arch/x86/acpi_device.c@... PS2, Line 943: struct
Oh. You want to use it directly as acpi_fill_ssdt. Ya. That should be const. […]
Done. Sorry, I had missed your comment earlier. I have pushed a set of follow-up patches to change the param to const.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/5/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/5/src/arch/x86/acpi_device.c@... PS5, Line 939: _ADR You also write _STA
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 5: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40674/5/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/40674/5/src/arch/x86/acpi_device.c@... PS5, Line 939: _ADR
You also write _STA
Done
Hello build bot (Jenkins), Raul Rangel, Duncan Laurie, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40674
to look at the new patch set (#6).
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR and _STA defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/40674/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 8: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
arch/x86/acpi_device: Add a helper function to write PCI device
This change adds a helper function to write a PCI device with _ADR and _STA defined for it.
BUG=b:153858769
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I932af917d91198876fe8e90af9bb7a2531bd8960 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40674 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 41 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 9f1710e..2c46155 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */
+#include <assert.h> #include <string.h> #include <arch/acpi.h> #include <arch/acpi_device.h> @@ -927,3 +928,34 @@
return gpio; } + +/* + * This function writes a PCI device with _ADR object: + * Example: + * Scope (_SB.PCI0) + * { + * Device (IGFX) + * { + * Name (_ADR, 0x0000000000000000) + * Method (_STA, 0, NotSerialized) { Return (status) } + * } + * } + */ +void acpi_device_write_pci_dev(struct device *dev) +{ + const char *scope = acpi_device_scope(dev); + const char *name = acpi_device_name(dev); + + assert(dev->path.type == DEVICE_PATH_PCI); + assert(name); + assert(scope); + + acpigen_write_scope(scope); + acpigen_write_device(name); + + acpigen_write_ADR_pci_device(dev); + acpigen_write_STA(acpi_device_status(dev)); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ +} diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index e9c5cd4..362efc4 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -501,4 +501,13 @@ /* Write Device Property hierarchy and clean up resources */ void acpi_dp_write(struct acpi_dp *table);
+/* + * Helper function to write a PCI device with _ADR object defined. + * + * IMPORTANT: Scope of a device created in SSDT cannot be used to add ACPI nodes under that + * scope in DSDT. So, if there are any references to this PCI device scope required from static + * asl files, do not use this function and instead add the device to DSDT as well. + */ +void acpi_device_write_pci_dev(struct device *dev); + #endif
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40674 )
Change subject: arch/x86/acpi_device: Add a helper function to write PCI device ......................................................................
Patch Set 9:
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/2798 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2797 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2796 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2795
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Raul Rangel (Code Review)