Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
[RFC] AMD APM_CNT and SMI enablement
Some suspicious callsite caught my eye.
Change-Id: I8d1287566061bc5d6f05e687f2b30c6bcb66c999 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 2 files changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/42563/1
diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 0ff5606..89b49f6 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -233,6 +233,7 @@ u32 reg; msr_t cst_addr;
+ /* We use some of these ports in SMM regardless of whether or not * ACPI tables are generated. Enable these ports indiscriminately. */ diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index e90fe1b..8a45fda 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -470,6 +470,7 @@ { u32 reg;
+ /* We use some of these ports in SMM regardless of whether or not * ACPI tables are generated. Enable these ports indiscriminately. */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42563/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/1/src/soc/amd/picasso/southbr... PS1, Line 238: * ACPI tables are generated. Enable these ports indiscriminately. There is initiative CB:42377 to track failures to write to APM_CNT. Looks like the code here enables SMI's via outb(xx, APM_CNT) mechanism late in ramstage. This should possibly happen right after PARALLEL_MP or even before SMM relocation runs.
PARALLEL_MP raises SMI via lapic?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
See also CB:42987 where at end of MP init there is write to APM_CNT.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Restored
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
please ping me again about this in a month or so when i'm working on ramstage for cezanne; will likely also do some reworks for picasso and possibly stoney ridge then. also the referenced patch gives me a "Not Found"
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42563/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/1/src/soc/amd/picasso/southbr... PS1, Line 238: * ACPI tables are generated. Enable these ports indiscriminately. The link of CB:42377 doesn't resolve to anything for me. Oh, you must mean in the case where the write fails to trigger the SMI and the APM_CNT isn't serviced.
This should possibly happen right after PARALLEL_MP
I agree.
...or even before SMM relocation runs.
I would probably do it after. The relocation happens as part of mp_init_with_smm() so that call should ensure APM_CNT isn't used or required until it completes. mp_init_with_smm() takes struct mp_ops and has a .post_mp_init that looks like the right place to program the address.
PARALLEL_MP raises SMI via lapic?
Right. You should be able to find smm_initiate_relocation_parallel() in mp_init.c and see the LAPIC write there.
Attention is currently required from: Marshall Dawson, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/comment/ba84f3dc_ef441927 PS1, Line 238: * ACPI tables are generated. Enable these ports indiscriminately.
The link of CB:42377 doesn't resolve to anything for me. […]
CB:42987
Attention is currently required from: Marshall Dawson, Felix Held. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(2 comments)
File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/comment/eddcdfe0_28ad415b PS1, Line 238: * ACPI tables are generated. Enable these ports indiscriminately.
CB:42987
The non-resolving link was probably some follow-up on CB:41971. I am still undecided if I should take my works from 2020 to another repo so there is a bunch of these references to works I forcefully deleted from gerrit.
https://review.coreboot.org/c/coreboot/+/42563/comment/77a3c0be_9f50386e PS1, Line 244: pm_write16(PM_GPE0_BLK, ACPI_GPE0_BLK); Now that I revisit this, PM1 and GPE0 sound like something chipset_power_state would need to eg. detect previoous sleep state of S3 and wake source. That access would be already (early) in romstage.
Attention is currently required from: Marshall Dawson, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/comment/9f1e7f32_58188a83 PS1, Line 244: pm_write16(PM_GPE0_BLK, ACPI_GPE0_BLK);
Now that I revisit this, PM1 and GPE0 sound like something chipset_power_state would need to eg. […]
without having looked into that, this sounds like a bug to me
Attention is currently required from: Marshall Dawson, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/comment/f88ca5b7_83eb29e1 PS1, Line 244: pm_write16(PM_GPE0_BLK, ACPI_GPE0_BLK);
without having looked into that, this sounds like a bug to me
from a quick look i'd say that it's not a bug, since the mmio mapping is used there and not the legacy io mapping that gets set up here. i'll try to have a closer look soon
Attention is currently required from: Marshall Dawson, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/42563/comment/77890bbd_d6a3ab7d PS1, Line 244: pm_write16(PM_GPE0_BLK, ACPI_GPE0_BLK);
from a quick look i'd say that it's not a bug, since the mmio mapping is used there and not the lega […]
the accesses in romstage to the pm1 and gpe0 registers is via the mmio mapping in the acpimmio region and not via the legacy io mappings that get configured in ramstage. the acpimmio region gets configured early in bootblock (enable_acpimmio_decode_pm04() in fch_pre_init). so this is correct. as far as i've seen the legacy io access is mostly needed for/used in the fadt
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42563 )
Change subject: [RFC] AMD APM_CNT and SMI enablement ......................................................................
Abandoned
-
Felix Held (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)