Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
soc/amd: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: Id7f79fc959766813d60f847482567579a02db124 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/acpi.c M src/soc/amd/stoneyridge/finalize.c 4 files changed, 18 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/36811/1
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 8597c4e..2a7ae0b 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -98,15 +98,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - acpi_disable_sci(); - } else { - fadt->smi_cmd = 0; /* disable system management mode */ - fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ - fadt->acpi_disable = 0; /* unused if SMI_CMD = 0 */ - fadt->s4bios_req = 0; /* unused if SMI_CMD = 0 */ - fadt->pstate_cnt = 0; /* unused if SMI_CMD = 0 */ - fadt->cst_cnt = 0x00; /* unused if SMI_CMD = 0 */ - acpi_enable_sci(); }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/soc/amd/picasso/finalize.c b/src/soc/amd/picasso/finalize.c index 0ec7bd9..3e66d88 100644 --- a/src/soc/amd/picasso/finalize.c +++ b/src/soc/amd/picasso/finalize.c @@ -13,12 +13,14 @@ * GNU General Public License for more details. */
+#include <arch/acpi.h> #include <cpu/x86/mp.h> #include <cpu/x86/msr.h> #include <cpu/amd/msr.h> #include <bootstate.h> #include <timer.h> #include <console/console.h> +#include <soc/acpi.h>
static void per_core_finalize(void *unused) { @@ -53,6 +55,13 @@ { finalize_cores();
+ if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + acpi_disable_sci(); + else + acpi_enable_sci(); + } + post_code(POST_OS_BOOT); }
diff --git a/src/soc/amd/stoneyridge/acpi.c b/src/soc/amd/stoneyridge/acpi.c index d1ea24f..2327ed9 100644 --- a/src/soc/amd/stoneyridge/acpi.c +++ b/src/soc/amd/stoneyridge/acpi.c @@ -100,15 +100,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - acpi_disable_sci(); - } else { - fadt->smi_cmd = 0; /* disable system management mode */ - fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ - fadt->acpi_disable = 0; /* unused if SMI_CMD = 0 */ - fadt->s4bios_req = 0; /* unused if SMI_CMD = 0 */ - fadt->pstate_cnt = 0; /* unused if SMI_CMD = 0 */ - fadt->cst_cnt = 0x00; /* unused if SMI_CMD = 0 */ - acpi_enable_sci(); }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/soc/amd/stoneyridge/finalize.c b/src/soc/amd/stoneyridge/finalize.c index 0ec7bd9..3e66d88 100644 --- a/src/soc/amd/stoneyridge/finalize.c +++ b/src/soc/amd/stoneyridge/finalize.c @@ -13,12 +13,14 @@ * GNU General Public License for more details. */
+#include <arch/acpi.h> #include <cpu/x86/mp.h> #include <cpu/x86/msr.h> #include <cpu/amd/msr.h> #include <bootstate.h> #include <timer.h> #include <console/console.h> +#include <soc/acpi.h>
static void per_core_finalize(void *unused) { @@ -53,6 +55,13 @@ { finalize_cores();
+ if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + acpi_disable_sci(); + else + acpi_enable_sci(); + } + post_code(POST_OS_BOOT); }
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36811
to look at the new patch set (#2).
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
soc/amd: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: Id7f79fc959766813d60f847482567579a02db124 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/acpi.c M src/soc/amd/stoneyridge/finalize.c 4 files changed, 18 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/36811/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 3:
CB:36828 will revisit this code.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c@... PS3, Line 105: = fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ fadt->acpi_disable = 0; /* unused if SMI_CMD = 0 */ should still remain in the else section.
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c@... PS3, Line 98: fadt->s4bios_req = 0; /* Not supported */ : fadt->pstate_cnt = 0; /* Not supported */ : Same comment as for stoney, 3 lines setting values to 0 should be outside the if/else
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... File src/soc/amd/stoneyridge/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 105: fadt->smi_cmd = 0; /* disable system management mode */ : fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ : These 3 line: fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ fadt->acpi_disable = 0; /* unused if SMI_CMD = 0 * Were not moved anywhere, so should still be within an else branch.
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 100: fadt->s4bios_req = 0; /* Not supported */ These 3 lines (setting variables to 0) were common on both sides of the if/else, so now it should be outside the if.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... File src/soc/amd/stoneyridge/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 81: memset((void *)fadt, 0, sizeof(acpi_fadt_t)); What's the motivation for zero-initializing each field explicitly?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... File src/soc/amd/stoneyridge/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 81: memset((void *)fadt, 0, sizeof(acpi_fadt_t));
What's the motivation for zero-initializing each field explicitly?
I would say only code clarity, as the whole area is 0 initialized. However, if you think that some should be removed (as you did initially) than all should be removed. And that would lose code clarity, though I will accept if you do so.
Hello Aaron Durbin, Marshall Dawson, Richard Spiegel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36811
to look at the new patch set (#5).
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
soc/amd: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: Id7f79fc959766813d60f847482567579a02db124 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/acpi.c M src/soc/amd/stoneyridge/finalize.c 4 files changed, 18 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/36811/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c@... PS3, Line 105: =
fadt->smi_cmd = 0; /* disable system management mode */ […]
Done
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c@... PS3, Line 98: fadt->s4bios_req = 0; /* Not supported */ : fadt->pstate_cnt = 0; /* Not supported */ :
Same comment as for stoney, 3 lines setting values to 0 should be outside the if/else
Done
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... File src/soc/amd/stoneyridge/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 105: fadt->smi_cmd = 0; /* disable system management mode */ : fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ :
These 3 line: […]
Done
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 81: memset((void *)fadt, 0, sizeof(acpi_fadt_t));
I would say only code clarity, as the whole area is 0 initialized. […]
Done
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 100: fadt->s4bios_req = 0; /* Not supported */
These 3 lines (setting variables to 0) were common on both sides of the if/else, so now it should be […]
Done
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 5: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 5: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
soc/amd: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: Id7f79fc959766813d60f847482567579a02db124 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36811 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/acpi.c M src/soc/amd/stoneyridge/finalize.c 4 files changed, 18 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 8597c4e..7b70ec6 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -98,7 +98,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - acpi_disable_sci(); } else { fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ @@ -106,7 +105,6 @@ fadt->s4bios_req = 0; /* unused if SMI_CMD = 0 */ fadt->pstate_cnt = 0; /* unused if SMI_CMD = 0 */ fadt->cst_cnt = 0x00; /* unused if SMI_CMD = 0 */ - acpi_enable_sci(); }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/soc/amd/picasso/finalize.c b/src/soc/amd/picasso/finalize.c index 0ec7bd9..5ea52c6 100644 --- a/src/soc/amd/picasso/finalize.c +++ b/src/soc/amd/picasso/finalize.c @@ -13,12 +13,14 @@ * GNU General Public License for more details. */
+#include <arch/acpi.h> #include <cpu/x86/mp.h> #include <cpu/x86/msr.h> #include <cpu/amd/msr.h> #include <bootstate.h> #include <timer.h> #include <console/console.h> +#include <amdblocks/acpi.h>
static void per_core_finalize(void *unused) { @@ -53,6 +55,13 @@ { finalize_cores();
+ if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + acpi_disable_sci(); + else + acpi_enable_sci(); + } + post_code(POST_OS_BOOT); }
diff --git a/src/soc/amd/stoneyridge/acpi.c b/src/soc/amd/stoneyridge/acpi.c index d1ea24f..13020ed 100644 --- a/src/soc/amd/stoneyridge/acpi.c +++ b/src/soc/amd/stoneyridge/acpi.c @@ -100,7 +100,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - acpi_disable_sci(); } else { fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ @@ -108,7 +107,6 @@ fadt->s4bios_req = 0; /* unused if SMI_CMD = 0 */ fadt->pstate_cnt = 0; /* unused if SMI_CMD = 0 */ fadt->cst_cnt = 0x00; /* unused if SMI_CMD = 0 */ - acpi_enable_sci(); }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/soc/amd/stoneyridge/finalize.c b/src/soc/amd/stoneyridge/finalize.c index 0ec7bd9..5ea52c6 100644 --- a/src/soc/amd/stoneyridge/finalize.c +++ b/src/soc/amd/stoneyridge/finalize.c @@ -13,12 +13,14 @@ * GNU General Public License for more details. */
+#include <arch/acpi.h> #include <cpu/x86/mp.h> #include <cpu/x86/msr.h> #include <cpu/amd/msr.h> #include <bootstate.h> #include <timer.h> #include <console/console.h> +#include <amdblocks/acpi.h>
static void per_core_finalize(void *unused) { @@ -53,6 +55,13 @@ { finalize_cores();
+ if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + acpi_disable_sci(); + else + acpi_enable_sci(); + } + post_code(POST_OS_BOOT); }
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Richard Spiegel (Code Review)