Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
AGESA,binaryPI: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: I37810771090dd9b0377f9a72c7a17ef1564ccf68 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/amd/agesa/hudson/fadt.c M src/southbridge/amd/agesa/hudson/lpc.c M src/southbridge/amd/pi/hudson/fadt.c M src/southbridge/amd/pi/hudson/lpc.c 4 files changed, 24 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/36812/1
diff --git a/src/southbridge/amd/agesa/hudson/fadt.c b/src/southbridge/amd/agesa/hudson/fadt.c index 28f035c..586fbdb 100644 --- a/src/southbridge/amd/agesa/hudson/fadt.c +++ b/src/southbridge/amd/agesa/hudson/fadt.c @@ -20,7 +20,6 @@ #include <string.h> #include <console/console.h> #include <arch/acpi.h> -#include <arch/io.h> #include <device/device.h> #include <version.h>
@@ -78,15 +77,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ - } 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 */ - outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/southbridge/amd/agesa/hudson/lpc.c b/src/southbridge/amd/agesa/hudson/lpc.c index 4cfbb64..7759758 100644 --- a/src/southbridge/amd/agesa/hudson/lpc.c +++ b/src/southbridge/amd/agesa/hudson/lpc.c @@ -23,6 +23,7 @@ #include <device/pci_def.h> #include <pc80/mc146818rtc.h> #include <pc80/isa-dma.h> +#include <arch/io.h> #include <arch/ioapic.h> #include <arch/acpi.h> #include <pc80/i8254.h> @@ -335,6 +336,16 @@ return current; }
+static void lpc_final(struct device *dev) +{ + if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ + else + outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ + } +} + static struct pci_operations lops_pci = { .set_subsystem = pci_dev_set_subsystem, }; @@ -347,6 +358,7 @@ .write_acpi_tables = acpi_write_hpet, #endif .init = lpc_init, + .init = lpc_final, .scan_bus = scan_static_bus, .ops_pci = &lops_pci, .acpi_name = lpc_acpi_name, diff --git a/src/southbridge/amd/pi/hudson/fadt.c b/src/southbridge/amd/pi/hudson/fadt.c index 28e20d3..5d07624 100644 --- a/src/southbridge/amd/pi/hudson/fadt.c +++ b/src/southbridge/amd/pi/hudson/fadt.c @@ -20,7 +20,6 @@ #include <string.h> #include <console/console.h> #include <arch/acpi.h> -#include <arch/io.h> #include <device/device.h> #include <version.h>
@@ -70,15 +69,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ - } 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 */ - outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/southbridge/amd/pi/hudson/lpc.c b/src/southbridge/amd/pi/hudson/lpc.c index 1e080a0..6654830 100644 --- a/src/southbridge/amd/pi/hudson/lpc.c +++ b/src/southbridge/amd/pi/hudson/lpc.c @@ -23,6 +23,7 @@ #include <device/pci_def.h> #include <pc80/mc146818rtc.h> #include <pc80/isa-dma.h> +#include <arch/io.h> #include <arch/ioapic.h> #include <arch/acpi.h> #include <pc80/i8254.h> @@ -351,6 +352,16 @@ return NULL; }
+static void lpc_final(struct device *dev) +{ + if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ + else + outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ + } +} + static struct pci_operations lops_pci = { .set_subsystem = pci_dev_set_subsystem, }; @@ -363,6 +374,7 @@ .write_acpi_tables = acpi_write_hpet, #endif .init = lpc_init, + .init = lpc_final, .scan_bus = scan_static_bus, .ops_pci = &lops_pci, .acpi_name = lpc_acpi_name,
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 1:
If I understand correctly, clearing a SCI is AMD dependent? Intel silicon is free of this flaw?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 1:
Patch Set 1:
If I understand correctly, clearing a SCI is AMD dependent? Intel silicon is free of this flaw?
It's not really a flaw. I am just going through the various places were we have used HAVE_SMI_HANDLER conditional and enforcing some consistency here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/agesa/h... File src/southbridge/amd/agesa/hudson/lpc.c:
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/agesa/h... PS2, Line 361: .init = lpc_final, Oh wow.. no complaints on this.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/lpc.c:
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/pi/huds... PS2, Line 377: .init = lpc_final, Same here
.final = lpc_final, ?
Hello build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36812
to look at the new patch set (#3).
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
AGESA,binaryPI: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: I37810771090dd9b0377f9a72c7a17ef1564ccf68 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/amd/agesa/hudson/fadt.c M src/southbridge/amd/agesa/hudson/lpc.c M src/southbridge/amd/pi/hudson/fadt.c M src/southbridge/amd/pi/hudson/lpc.c 4 files changed, 24 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/36812/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 3: Code-Review+1
When I test this, will give +2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 3:
(2 comments)
You may want to delay testing until discussions in CB:36828 start and complete.
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/agesa/h... File src/southbridge/amd/agesa/hudson/lpc.c:
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/agesa/h... PS2, Line 361: .init = lpc_final,
Oh wow.. no complaints on this.
Done
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/lpc.c:
https://review.coreboot.org/c/coreboot/+/36812/2/src/southbridge/amd/pi/huds... PS2, Line 377: .init = lpc_final,
Same here […]
Done
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 3: Code-Review+1
Does not break anything for G505S which has hudson-like bolton
Hello Mike Banon, Marshall Dawson, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36812
to look at the new patch set (#5).
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
AGESA,binaryPI: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: I37810771090dd9b0377f9a72c7a17ef1564ccf68 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/amd/agesa/hudson/fadt.c M src/southbridge/amd/agesa/hudson/lpc.c M src/southbridge/amd/pi/hudson/fadt.c M src/southbridge/amd/pi/hudson/lpc.c 4 files changed, 24 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/36812/5
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 5: Code-Review+2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
Patch Set 6: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36812 )
Change subject: AGESA,binaryPI: Move SCI enable outside table creation ......................................................................
AGESA,binaryPI: Move SCI enable outside table creation
Preferably, coreboot tables creation is kept hardware-invariant.
Change-Id: I37810771090dd9b0377f9a72c7a17ef1564ccf68 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36812 Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/amd/agesa/hudson/fadt.c M src/southbridge/amd/agesa/hudson/lpc.c M src/southbridge/amd/pi/hudson/fadt.c M src/southbridge/amd/pi/hudson/lpc.c 4 files changed, 24 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved Michał Żygowski: Looks good to me, approved
diff --git a/src/southbridge/amd/agesa/hudson/fadt.c b/src/southbridge/amd/agesa/hudson/fadt.c index 28f035c..425a084 100644 --- a/src/southbridge/amd/agesa/hudson/fadt.c +++ b/src/southbridge/amd/agesa/hudson/fadt.c @@ -20,7 +20,6 @@ #include <string.h> #include <console/console.h> #include <arch/acpi.h> -#include <arch/io.h> #include <device/device.h> #include <version.h>
@@ -78,7 +77,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ } else { fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ @@ -86,7 +84,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 */ - outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/southbridge/amd/agesa/hudson/lpc.c b/src/southbridge/amd/agesa/hudson/lpc.c index 4cfbb64..eed1aec 100644 --- a/src/southbridge/amd/agesa/hudson/lpc.c +++ b/src/southbridge/amd/agesa/hudson/lpc.c @@ -23,6 +23,7 @@ #include <device/pci_def.h> #include <pc80/mc146818rtc.h> #include <pc80/isa-dma.h> +#include <arch/io.h> #include <arch/ioapic.h> #include <arch/acpi.h> #include <pc80/i8254.h> @@ -335,6 +336,16 @@ return current; }
+static void lpc_final(struct device *dev) +{ + if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ + else + outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ + } +} + static struct pci_operations lops_pci = { .set_subsystem = pci_dev_set_subsystem, }; @@ -347,6 +358,7 @@ .write_acpi_tables = acpi_write_hpet, #endif .init = lpc_init, + .final = lpc_final, .scan_bus = scan_static_bus, .ops_pci = &lops_pci, .acpi_name = lpc_acpi_name, diff --git a/src/southbridge/amd/pi/hudson/fadt.c b/src/southbridge/amd/pi/hudson/fadt.c index 28e20d3..61e046d 100644 --- a/src/southbridge/amd/pi/hudson/fadt.c +++ b/src/southbridge/amd/pi/hudson/fadt.c @@ -20,7 +20,6 @@ #include <string.h> #include <console/console.h> #include <arch/acpi.h> -#include <arch/io.h> #include <device/device.h> #include <version.h>
@@ -70,7 +69,6 @@ fadt->s4bios_req = 0; /* Not supported */ fadt->pstate_cnt = 0; /* Not supported */ fadt->cst_cnt = 0; /* Not supported */ - outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ } else { fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ @@ -78,7 +76,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 */ - outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ }
fadt->pm1a_evt_blk = ACPI_PM_EVT_BLK; diff --git a/src/southbridge/amd/pi/hudson/lpc.c b/src/southbridge/amd/pi/hudson/lpc.c index 1e080a0..e65fd83 100644 --- a/src/southbridge/amd/pi/hudson/lpc.c +++ b/src/southbridge/amd/pi/hudson/lpc.c @@ -23,6 +23,7 @@ #include <device/pci_def.h> #include <pc80/mc146818rtc.h> #include <pc80/isa-dma.h> +#include <arch/io.h> #include <arch/ioapic.h> #include <arch/acpi.h> #include <pc80/i8254.h> @@ -351,6 +352,16 @@ return NULL; }
+static void lpc_final(struct device *dev) +{ + if (!acpi_is_wakeup_s3()) { + if (CONFIG(HAVE_SMI_HANDLER)) + outl(0x0, ACPI_PM1_CNT_BLK); /* clear SCI_EN */ + else + outl(0x1, ACPI_PM1_CNT_BLK); /* set SCI_EN */ + } +} + static struct pci_operations lops_pci = { .set_subsystem = pci_dev_set_subsystem, }; @@ -363,6 +374,7 @@ .write_acpi_tables = acpi_write_hpet, #endif .init = lpc_init, + .final = lpc_final, .scan_bus = scan_static_bus, .ops_pci = &lops_pci, .acpi_name = lpc_acpi_name,