EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:144768001 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl M src/mainboard/google/brya/mainboard.c 3 files changed, 25 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/1
diff --git a/src/mainboard/google/brya/Kconfig b/src/mainboard/google/brya/Kconfig index 3c08757..e448e18 100644 --- a/src/mainboard/google/brya/Kconfig +++ b/src/mainboard/google/brya/Kconfig @@ -1,6 +1,7 @@ config BOARD_GOOGLE_BASEBOARD_BRYA def_bool n select BOARD_ROMSIZE_KB_32768 + select HAVE_ACPI_RESUME select HAVE_ACPI_TABLES select SOC_INTEL_ALDERLAKE
diff --git a/src/mainboard/google/brya/dsdt.asl b/src/mainboard/google/brya/dsdt.asl index 10d08e2..f15f25e 100644 --- a/src/mainboard/google/brya/dsdt.asl +++ b/src/mainboard/google/brya/dsdt.asl @@ -11,4 +11,19 @@ 0x20110725 // OEM revision ) { + /* Some generic macros */ + #include <soc/intel/common/acpi/platform.asl> + /* global NVS and variables */ + #include <soc/intel/common/block/acpi/acpi/globalnvs.asl> + /* CPU */ + #include <cpu/intel/common/acpi/cpu.asl> + Scope (_SB) { + Device (PCI0) + { + #include <soc/intel/common/block/acpi/acpi/northbridge.asl> + #include <soc/intel/alderlake/acpi/southbridge.asl> + } + } + /* Chipset specific sleep states */ + #include <southbridge/intel/common/acpi/sleepstates.asl> } diff --git a/src/mainboard/google/brya/mainboard.c b/src/mainboard/google/brya/mainboard.c index d7c3156..27a94dc 100644 --- a/src/mainboard/google/brya/mainboard.c +++ b/src/mainboard/google/brya/mainboard.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <acpi/acpi.h> #include <baseboard/variants.h> #include <device/device.h>
@@ -11,9 +12,16 @@ gpio_configure_pads(pads, num); }
+static unsigned long mainboard_write_acpi_tables( + const struct device *device, unsigned long current, acpi_rsdp_t *rsdp) +{ + return current; +} + static void mainboard_enable(struct device *dev) { - /* TODO: Enable mainboard */ + dev->ops->write_acpi_tables = mainboard_write_acpi_tables; + dev->ops->acpi_inject_dsdt = chromeos_dsdt_generator; }
struct chip_operations mainboard_ops = {
Hello Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48069
to look at the new patch set (#2).
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:144768001 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl M src/mainboard/google/brya/mainboard.c 3 files changed, 26 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/d... File src/mainboard/google/brya/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/d... PS3, Line 15: #include <soc/intel/common/acpi/platform.asl> : /* global NVS and variables */ : #include <soc/intel/common/block/acpi/acpi/globalnvs.asl> : /* CPU */ : #include <cpu/intel/common/acpi/cpu.asl> nit: blank lines between
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/d... PS3, Line 27: /* Chipset specific sleep states */ nit: blank line after last `}`
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 25: dev->ops->acpi_inject_dsdt = chromeos_dsdt_generator; should this go in CB:48111?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 16: static unsigned long mainboard_write_acpi_tables( : const struct device *device, unsigned long current, acpi_rsdp_t *rsdp) : { : return current; : } Do you have a plan for using this later?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 16: static unsigned long mainboard_write_acpi_tables( : const struct device *device, unsigned long current, acpi_rsdp_t *rsdp) : { : return current; : }
Do you have a plan for using this later?
I think we can skip this function for now. This was used in the past when we had to write audio NHLT tables. But, that is not required anymore. So, it just remains unused.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 16: static unsigned long mainboard_write_acpi_tables( : const struct device *device, unsigned long current, acpi_rsdp_t *rsdp) : { : return current; : }
I think we can skip this function for now. […]
hmm, so keep this or remove this?
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 25: dev->ops->acpi_inject_dsdt = chromeos_dsdt_generator;
should this go in CB:48111?
okay, let me change this.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/d... File src/mainboard/google/brya/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/d... PS3, Line 15: #include <soc/intel/common/acpi/platform.asl> : /* global NVS and variables */ : #include <soc/intel/common/block/acpi/acpi/globalnvs.asl> : /* CPU */ : #include <cpu/intel/common/acpi/cpu.asl>
nit: blank lines between
Done
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/d... PS3, Line 27: /* Chipset specific sleep states */
nit: blank line after last `}`
Done
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 25: dev->ops->acpi_inject_dsdt = chromeos_dsdt_generator;
okay, let me change this.
Done
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48069
to look at the new patch set (#4).
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:144768001 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl M src/mainboard/google/brya/mainboard.c 3 files changed, 29 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 16: static unsigned long mainboard_write_acpi_tables( : const struct device *device, unsigned long current, acpi_rsdp_t *rsdp) : { : return current; : }
hmm, so keep this or remove this?
remove this
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/3/src/mainboard/google/brya/m... PS3, Line 16: static unsigned long mainboard_write_acpi_tables( : const struct device *device, unsigned long current, acpi_rsdp_t *rsdp) : { : return current; : }
remove this
Got it.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48069
to look at the new patch set (#5).
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:144768001 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl M src/mainboard/google/brya/mainboard.c 3 files changed, 22 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/5/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/5/src/mainboard/google/brya/m... PS5, Line 14: : static void mainboard_enable(struct device *dev) : { : dev->ops->acpi_inject_dsdt = NULL; : } May as well drop this function here and add it in 48111?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/5/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48069/5/src/mainboard/google/brya/m... PS5, Line 14: : static void mainboard_enable(struct device *dev) : { : dev->ops->acpi_inject_dsdt = NULL; : }
May as well drop this function here and add it in 48111?
LGTM
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48069
to look at the new patch set (#6).
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:144768001 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl M src/mainboard/google/brya/mainboard.c 3 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/6/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
PS6: Not sure this file change needs to be here 😊
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/6/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
PS6:
Not sure this file change needs to be here 😊
oh.. looks like not ACPI relative. remove this to 48111?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/6/src/mainboard/google/brya/m... File src/mainboard/google/brya/mainboard.c:
PS6:
oh.. looks like not ACPI relative. […]
I wasn't aware the original one lol.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48069
to look at the new patch set (#7).
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:144768001 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/7
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48069/7//COMMIT_MSG@11 PS7, Line 11: 144768001 174266035 😉
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48069
to look at the new patch set (#8).
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:174266035 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/48069/8
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48069/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48069/7//COMMIT_MSG@11 PS7, Line 11: 144768001
174266035 😉
Done
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48069 )
Change subject: mb/google/brya: Enable ACPI and add ACPI table ......................................................................
mb/google/brya: Enable ACPI and add ACPI table
Enable ACPI configuration and add DSDT ACPI table.
BUG=b:174266035 TEST=Build Test
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I08513ec159b69535f742a1fd70cdec9ec845d414 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48069 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/dsdt.asl 2 files changed, 20 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/brya/Kconfig b/src/mainboard/google/brya/Kconfig index 3c08757..e448e18 100644 --- a/src/mainboard/google/brya/Kconfig +++ b/src/mainboard/google/brya/Kconfig @@ -1,6 +1,7 @@ config BOARD_GOOGLE_BASEBOARD_BRYA def_bool n select BOARD_ROMSIZE_KB_32768 + select HAVE_ACPI_RESUME select HAVE_ACPI_TABLES select SOC_INTEL_ALDERLAKE
diff --git a/src/mainboard/google/brya/dsdt.asl b/src/mainboard/google/brya/dsdt.asl index 10d08e2..ebb6ec5 100644 --- a/src/mainboard/google/brya/dsdt.asl +++ b/src/mainboard/google/brya/dsdt.asl @@ -11,4 +11,23 @@ 0x20110725 // OEM revision ) { + /* Some generic macros */ + #include <soc/intel/common/acpi/platform.asl> + + /* global NVS and variables */ + #include <soc/intel/common/block/acpi/acpi/globalnvs.asl> + + /* CPU */ + #include <cpu/intel/common/acpi/cpu.asl> + + Scope (_SB) { + Device (PCI0) + { + #include <soc/intel/common/block/acpi/acpi/northbridge.asl> + #include <soc/intel/alderlake/acpi/southbridge.asl> + } + } + + /* Chipset specific sleep states */ + #include <southbridge/intel/common/acpi/sleepstates.asl> }
-
EricR Lai (Code Review) -
Furquan Shaikh (Code Review)
-
Tim Wawrzynczak (Code Review)