Change in coreboot[master]: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT
This change: 1. Adds PCI device for graphics controller in ACPI SSDT tables using acpi_device_write_pci_dev(). 2. Gets rid of IGFX device from picasso acpi/northbridge.asl.
This makes it easier to ensure that we don't accidentally make the DSDT and SSDT entries inconsistent w.r.t. ACPI name and scope.
BUG=b:153858769
Change-Id: I3a967cdc43b74f786e645d3fb666506070851a99 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/graphics/graphics.c M src/soc/amd/picasso/acpi/northbridge.asl 2 files changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/40677/1
diff --git a/src/soc/amd/common/block/graphics/graphics.c b/src/soc/amd/common/block/graphics/graphics.c index 8e14aa7..8ad0d7a 100644 --- a/src/soc/amd/common/block/graphics/graphics.c +++ b/src/soc/amd/common/block/graphics/graphics.c @@ -5,6 +5,12 @@ #include <device/pci.h> #include <device/pci_ids.h>
+static void graphics_fill_ssdt(struct device *dev) +{ + acpi_device_write_pci_dev(dev); + pci_rom_ssdt(dev); +} + static const struct *graphics_acpi_name(const struct device *dev) { return "IGFX"; @@ -17,7 +23,7 @@ .init = pci_dev_init, .ops_pci = &pci_dev_ops_pci, .write_acpi_tables = pci_rom_write_acpi_tables, - .acpi_fill_ssdt_generator = pci_rom_ssdt, + .acpi_fill_ssdt_generator = graphics_fill_ssdt, .acpi_name = graphics_acpi_name, };
diff --git a/src/soc/amd/picasso/acpi/northbridge.asl b/src/soc/amd/picasso/acpi/northbridge.asl index c807601..6b6bd7c 100644 --- a/src/soc/amd/picasso/acpi/northbridge.asl +++ b/src/soc/amd/picasso/acpi/northbridge.asl @@ -32,11 +32,6 @@ Name(_ADR, 0x00000000) } /* end AMRT */
-/* Internal Graphics */ -Device(IGFX) { - Name(_ADR, 0x00010000) -} - /* Gpp 0 */ Device(PBR4) { Name(_ADR, 0x00020001)
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40677
to look at the new patch set (#2).
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT
This change: 1. Adds PCI device for graphics controller in ACPI SSDT tables using acpi_device_write_pci_dev(). 2. Gets rid of IGFX device from picasso acpi/northbridge.asl.
This makes it easier to ensure that we don't accidentally make the DSDT and SSDT entries inconsistent w.r.t. ACPI name and scope.
BUG=b:153858769
Change-Id: I3a967cdc43b74f786e645d3fb666506070851a99 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/graphics/graphics.c M src/soc/amd/picasso/acpi/northbridge.asl 2 files changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/40677/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40677/2/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/40677/2/src/soc/amd/common/block/gr... PS2, Line 8: static void graphics_fill_ssdt(struct device *dev) wire this up on line 26?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
It would help to have this code build-tested
https://review.coreboot.org/c/coreboot/+/40677/2/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/40677/2/src/soc/amd/common/block/gr... PS2, Line 8: static void graphics_fill_ssdt(struct device *dev)
wire this up on line 26?
Indeed, it would be very nice if we actually put this function to good use somewhere! 😄
Hello build bot (Jenkins), Raul Rangel, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40677
to look at the new patch set (#3).
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT
This change: 1. Adds PCI device for graphics controller in ACPI SSDT tables using acpi_device_write_pci_dev(). 2. Gets rid of IGFX device from picasso acpi/northbridge.asl.
This makes it easier to ensure that we don't accidentally make the DSDT and SSDT entries inconsistent w.r.t. ACPI name and scope.
BUG=b:153858769
Change-Id: I3a967cdc43b74f786e645d3fb666506070851a99 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/graphics/graphics.c M src/soc/amd/picasso/acpi/northbridge.asl 2 files changed, 8 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/40677/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40677/2/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/40677/2/src/soc/amd/common/block/gr... PS2, Line 8: static void graphics_fill_ssdt(struct device *dev)
Indeed, it would be very nice if we actually put this function to good use somewhere! 😄
Ugh! Messed up as I was juggling patches upstream and in internal tree. Sorry, I will fix this up. Also, I am build and boot testing, just pushed the wrong version here.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 3: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40677/4/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/40677/4/src/soc/amd/common/block/gr... PS4, Line 26: graphics_fill_ssdt I don't know the history here. Why do we add it to the SSDT instead of the DSDT?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 4:
I like the acpi_device_write_pci_dev() idea and all. But so far this graphics.c doesn't seem to be vendor/chip specific. Is there more to come? Shouldn't we extend `default_pci_ops_dev` instead?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 4:
Patch Set 4:
I like the acpi_device_write_pci_dev() idea and all. But so far this graphics.c doesn't seem to be vendor/chip specific. Is there more to come? Shouldn't we extend `default_pci_ops_dev` instead?
Yes, there is more to come. Still working on a series of patches. And this is not something that will be used only by graphics device. I will need another couple of days to push rest of my patches.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 6:
I like the acpi_device_write_pci_dev() idea and all. But so far this graphics.c doesn't seem to be vendor/chip specific. Is there more to come? Shouldn't we extend `default_pci_ops_dev` instead?
Yes, there is more to come. Still working on a series of patches. And this is not something that will be used only by graphics device. I will need another couple of days to push rest of my patches.
Well, I meant more for the `graphics.c` because so far it's not chip specific. Just boilerplate that could also live in common code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 7:
Patch Set 6:
I like the acpi_device_write_pci_dev() idea and all. But so far this graphics.c doesn't seem to be vendor/chip specific. Is there more to come? Shouldn't we extend `default_pci_ops_dev` instead?
Yes, there is more to come. Still working on a series of patches. And this is not something that will be used only by graphics device. I will need another couple of days to push rest of my patches.
Well, I meant more for the `graphics.c` because so far it's not chip specific. Just boilerplate that could also live in common code.
Not all platforms would want to have acpi_device_write_pci_dev() done by default for graphics device. Also, at that point, it is not just about graphics device. PCI devices handled by default_pci_ops_dev are currently added statically to DSDT on most platforms. There will have to be a bigger clean up if we want to extend default_pci_ops_dev.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40677/4/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/40677/4/src/soc/amd/common/block/gr... PS4, Line 26: graphics_fill_ssdt
I don't know the history here. […]
In general my understanding has been that we add all runtime ACPI information to SSDT. Yes, there are some DSDT injectors as well, but probably those are for anything that *has* to live only in DSDT. I could be wrong about the history here.
Anyways, keeping this here for not changing functionality. If it has to be moved, I think it can be done as a separate change.
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40677 )
Change subject: soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT ......................................................................
soc/amd/{common,picasso}: Move GFX device from static ASL to SSDT
This change: 1. Adds PCI device for graphics controller in ACPI SSDT tables using acpi_device_write_pci_dev(). 2. Gets rid of IGFX device from picasso acpi/northbridge.asl.
This makes it easier to ensure that we don't accidentally make the DSDT and SSDT entries inconsistent w.r.t. ACPI name and scope.
BUG=b:153858769
Change-Id: I3a967cdc43b74f786e645d3fb666506070851a99 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40677 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/soc/amd/common/block/graphics/graphics.c M src/soc/amd/picasso/acpi/northbridge.asl 2 files changed, 8 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/common/block/graphics/graphics.c b/src/soc/amd/common/block/graphics/graphics.c index 880573b..a40aadd 100644 --- a/src/soc/amd/common/block/graphics/graphics.c +++ b/src/soc/amd/common/block/graphics/graphics.c @@ -1,9 +1,16 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ /* This file is part of the coreboot project. */
+#include <arch/acpi_device.h> #include <device/pci.h> #include <device/pci_ids.h>
+static void graphics_fill_ssdt(struct device *dev) +{ + acpi_device_write_pci_dev(dev); + pci_rom_ssdt(dev); +} + static const char *graphics_acpi_name(const struct device *dev) { return "IGFX"; @@ -16,7 +23,7 @@ .init = pci_dev_init, .ops_pci = &pci_dev_ops_pci, .write_acpi_tables = pci_rom_write_acpi_tables, - .acpi_fill_ssdt = pci_rom_ssdt, + .acpi_fill_ssdt = graphics_fill_ssdt, .acpi_name = graphics_acpi_name, };
diff --git a/src/soc/amd/picasso/acpi/northbridge.asl b/src/soc/amd/picasso/acpi/northbridge.asl index c807601..6b6bd7c 100644 --- a/src/soc/amd/picasso/acpi/northbridge.asl +++ b/src/soc/amd/picasso/acpi/northbridge.asl @@ -32,11 +32,6 @@ Name(_ADR, 0x00000000) } /* end AMRT */
-/* Internal Graphics */ -Device(IGFX) { - Name(_ADR, 0x00010000) -} - /* Gpp 0 */ Device(PBR4) { Name(_ADR, 0x00020001)
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)
-
Raul Rangel (Code Review)