Change in coreboot[master]: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 --- M src/soc/intel/tigerlake/elog.c 1 file changed, 42 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/47182/1
diff --git a/src/soc/intel/tigerlake/elog.c b/src/soc/intel/tigerlake/elog.c index 84f0a7e..256675d 100644 --- a/src/soc/intel/tigerlake/elog.c +++ b/src/soc/intel/tigerlake/elog.c @@ -2,11 +2,15 @@
#include <bootstate.h> #include <console/console.h> -#include <stdint.h> +#include <device/pci_ops.h> #include <elog.h> #include <intelblocks/pmclib.h> #include <soc/pci_devs.h> #include <soc/pm.h> +#include <stdint.h> +#include <types.h> + +#define PCIE_ROOT_PORT_STATUS 0x60
static void pch_log_gpio_gpe(u32 gpe0_sts, u32 gpe0_en, int start) { @@ -20,6 +24,41 @@ } }
+static void pch_log_rp_wake_source(void) +{ + size_t i; + struct pme_map { + pci_devfn_t devfn; + unsigned int wake_source; + }; + + const struct pme_map pme_map[] = { + { PCH_DEVFN_PCIE1, ELOG_WAKE_SOURCE_PME_PCIE1 }, + { PCH_DEVFN_PCIE2, ELOG_WAKE_SOURCE_PME_PCIE2 }, + { PCH_DEVFN_PCIE3, ELOG_WAKE_SOURCE_PME_PCIE3 }, + { PCH_DEVFN_PCIE4, ELOG_WAKE_SOURCE_PME_PCIE4 }, + { PCH_DEVFN_PCIE5, ELOG_WAKE_SOURCE_PME_PCIE5 }, + { PCH_DEVFN_PCIE6, ELOG_WAKE_SOURCE_PME_PCIE6 }, + { PCH_DEVFN_PCIE7, ELOG_WAKE_SOURCE_PME_PCIE7 }, + { PCH_DEVFN_PCIE8, ELOG_WAKE_SOURCE_PME_PCIE8 }, + { PCH_DEVFN_PCIE9, ELOG_WAKE_SOURCE_PME_PCIE9 }, + { PCH_DEVFN_PCIE10, ELOG_WAKE_SOURCE_PME_PCIE10 }, + { PCH_DEVFN_PCIE11, ELOG_WAKE_SOURCE_PME_PCIE11 }, + { PCH_DEVFN_PCIE12, ELOG_WAKE_SOURCE_PME_PCIE12 }, + }; + + for (i = 0; i < MIN(CONFIG_MAX_ROOT_PORTS, ARRAY_SIZE(pme_map)); ++i) { + uint32_t pme_status = pci_s_read_config32( + pme_map[i].devfn, + PCIE_ROOT_PORT_STATUS); + + if (pme_status == 0xFFFFFFFF || !(pme_status & BIT(16))) + continue; + + elog_add_event_wake(pme_map[i].wake_source, 0); + } +} + static void pch_log_wake_source(struct chipset_power_state *ps) { /* Power Button */ @@ -30,9 +69,9 @@ if (ps->pm1_sts & RTC_STS) elog_add_event_wake(ELOG_WAKE_SOURCE_RTC, 0);
- /* PCI Express (TODO: determine wake device) */ + /* PCI Express */ if (ps->pm1_sts & PCIEXPWAK_STS) - elog_add_event_wake(ELOG_WAKE_SOURCE_PCIE, 0); + pch_log_rp_wake_source();
/* PME (TODO: determine wake device) */ if (ps->gpe0_sts[GPE_STD] & PME_STS)
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/1/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/1/src/soc/intel/tigerlake/elo... PS1, Line 53: PCIE_ROOT_PORT_STATUS Should we be using pci_dev_is_wake_source() instead of checking PCIE_ROOT_PORT_STATUS?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47182
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 --- M src/soc/intel/tigerlake/elog.c 1 file changed, 38 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/47182/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 2:
(1 comment)
here's a weird error: ``` vboot SHA256 built with tight loops (slower, smaller code size) CREATE GBB (without BMPFV) make[1]: /cb-build/coreboot-gerrit.0/default/sharedutils/futility/futility: Permission denied make[1]: *** [src/security/vboot/Makefile.inc:242: /cb-build/coreboot-gerrit.0/default/GOOGLE_DEDEDE/gbb.stub] Error 127 make[1]: Leaving directory '/home/coreboot/node-root/workspace/coreboot-gerrit'
```
https://review.coreboot.org/c/coreboot/+/47182/1/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/1/src/soc/intel/tigerlake/elo... PS1, Line 53: PCIE_ROOT_PORT_STATUS
Should we be using pci_dev_is_wake_source() instead of checking PCIE_ROOT_PORT_STATUS?
Ah yes, that simplifies this; I had forgotten that there was a capability reg for PM.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... PS2, Line 53: pci_dev_is_wake_source It looks like `pci_dev_is_wake_source()` is defined in pci_device.c which is not included in smm- stage. I think we should move it to pci_ops.c.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
here's a weird error:
vboot SHA256 built with tight loops (slower, smaller code size) CREATE GBB (without BMPFV) make[1]: /cb-build/coreboot-gerrit.0/default/sharedutils/futility/futility: Permission denied make[1]: *** [src/security/vboot/Makefile.inc:242: /cb-build/coreboot-gerrit.0/default/GOOGLE_DEDEDE/gbb.stub] Error 127 make[1]: Leaving directory '/home/coreboot/node-root/workspace/coreboot-gerrit'
Did you try rebasing on top of Tot to see if the error goes away?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... PS2, Line 53: pci_dev_is_wake_source
It looks like `pci_dev_is_wake_source()` is defined in pci_device. […]
Ah yes, I see. It still compiles/links because we haven't added an elog_gsmi_cb... handler for TGL yet. I will move pci_dev_is_wake and I will add the gsmi handler.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
(1 comment)
here's a weird error:
vboot SHA256 built with tight loops (slower, smaller code size) CREATE GBB (without BMPFV) make[1]: /cb-build/coreboot-gerrit.0/default/sharedutils/futility/futility: Permission denied make[1]: *** [src/security/vboot/Makefile.inc:242: /cb-build/coreboot-gerrit.0/default/GOOGLE_DEDEDE/gbb.stub] Error 127 make[1]: Leaving directory '/home/coreboot/node-root/workspace/coreboot-gerrit'Did you try rebasing on top of Tot to see if the error goes away?
*crosses fingers*
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... PS2, Line 53: pci_dev_is_wake_source
Ah yes, I see. It still compiles/links because we haven't added an elog_gsmi_cb... […]
That handler is present in this file on line 159. However, elog.c wasn't included in smm- until very recently. I see that the parent CL for this change doesn't include CB:47141 and that is probably why Jenkins doesn't complain.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... PS2, Line 53: pci_dev_is_wake_source
Ah yes, I see. It still compiles/links because we haven't added an elog_gsmi_cb... […]
nm you recently added the gsmi handler 😊
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47182
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 --- M src/soc/intel/tigerlake/elog.c 1 file changed, 41 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/47182/4
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/4/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/4/src/soc/intel/tigerlake/elo... PS4, Line 51: uint32_t pme_status = pci_s_read_config32( : pme_map[i].devfn, : PCIE_ROOT_PORT_STATUS); : : if (pme_status == 0xFFFFFFFF || !(pme_status & BIT(16))) : continue; : : elog_add_event_wake(pme_map[i].wake_source, 0); Not sure if you forgot to update the tigerlake in the same way as jasperlake.
Karthik Ramasubramanian has uploaded a new patch set (#5) to the change originally created by Tim Wawrzynczak. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 --- M src/soc/intel/tigerlake/elog.c 1 file changed, 41 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/47182/5
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47182
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 --- M src/soc/intel/tigerlake/elog.c 1 file changed, 39 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/47182/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47182/6/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/6/src/soc/intel/tigerlake/elo... PS6, Line 13: #define PCIE_ROOT_PORT_STATUS 0x60 Not required anymore.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47182
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 --- M src/soc/intel/tigerlake/elog.c 1 file changed, 37 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/47182/7
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/47182/1/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/1/src/soc/intel/tigerlake/elo... PS1, Line 53: PCIE_ROOT_PORT_STATUS
Ah yes, that simplifies this; I had forgotten that there was a capability reg for PM.
Done
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/2/src/soc/intel/tigerlake/elo... PS2, Line 53: pci_dev_is_wake_source
nm you recently added the gsmi handler 😊
Ack
https://review.coreboot.org/c/coreboot/+/47182/4/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/4/src/soc/intel/tigerlake/elo... PS4, Line 51: uint32_t pme_status = pci_s_read_config32( : pme_map[i].devfn, : PCIE_ROOT_PORT_STATUS); : : if (pme_status == 0xFFFFFFFF || !(pme_status & BIT(16))) : continue; : : elog_add_event_wake(pme_map[i].wake_source, 0);
Not sure if you forgot to update the tigerlake in the same way as jasperlake.
Ack
https://review.coreboot.org/c/coreboot/+/47182/6/src/soc/intel/tigerlake/elo... File src/soc/intel/tigerlake/elog.c:
https://review.coreboot.org/c/coreboot/+/47182/6/src/soc/intel/tigerlake/elo... PS6, Line 13: #define PCIE_ROOT_PORT_STATUS 0x60
Not required anymore.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 7: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47182 )
Change subject: soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log ......................................................................
soc/intel/tigerlake: Add PCH PCIe RPs wake up events to event log
All wakes by a PCH PCIe root port were lumped under one event source; this commit splits them up so each root port gets its own ID in the event log.
BUG=b:172279061 BRANCH=volteer
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Icebcac3b69c605ecf6df37733b641397ea3c3ad0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47182 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/soc/intel/tigerlake/elog.c 1 file changed, 37 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/elog.c b/src/soc/intel/tigerlake/elog.c index 84f0a7e..a46afbb 100644 --- a/src/soc/intel/tigerlake/elog.c +++ b/src/soc/intel/tigerlake/elog.c @@ -2,11 +2,13 @@
#include <bootstate.h> #include <console/console.h> -#include <stdint.h> +#include <device/pci_ops.h> #include <elog.h> #include <intelblocks/pmclib.h> #include <soc/pci_devs.h> #include <soc/pm.h> +#include <stdint.h> +#include <types.h>
static void pch_log_gpio_gpe(u32 gpe0_sts, u32 gpe0_en, int start) { @@ -20,6 +22,39 @@ } }
+static void pch_log_rp_wake_source(void) +{ + size_t i; + struct pme_map { + pci_devfn_t devfn; + unsigned int wake_source; + }; + + const struct pme_map pme_map[] = { + { PCH_DEVFN_PCIE1, ELOG_WAKE_SOURCE_PME_PCIE1 }, + { PCH_DEVFN_PCIE2, ELOG_WAKE_SOURCE_PME_PCIE2 }, + { PCH_DEVFN_PCIE3, ELOG_WAKE_SOURCE_PME_PCIE3 }, + { PCH_DEVFN_PCIE4, ELOG_WAKE_SOURCE_PME_PCIE4 }, + { PCH_DEVFN_PCIE5, ELOG_WAKE_SOURCE_PME_PCIE5 }, + { PCH_DEVFN_PCIE6, ELOG_WAKE_SOURCE_PME_PCIE6 }, + { PCH_DEVFN_PCIE7, ELOG_WAKE_SOURCE_PME_PCIE7 }, + { PCH_DEVFN_PCIE8, ELOG_WAKE_SOURCE_PME_PCIE8 }, + { PCH_DEVFN_PCIE9, ELOG_WAKE_SOURCE_PME_PCIE9 }, + { PCH_DEVFN_PCIE10, ELOG_WAKE_SOURCE_PME_PCIE10 }, + { PCH_DEVFN_PCIE11, ELOG_WAKE_SOURCE_PME_PCIE11 }, + { PCH_DEVFN_PCIE12, ELOG_WAKE_SOURCE_PME_PCIE12 }, + }; + + for (i = 0; i < MIN(CONFIG_MAX_ROOT_PORTS, ARRAY_SIZE(pme_map)); ++i) { + const struct device *dev = pcidev_path_on_root(pme_map[i].devfn); + if (!dev) + continue; + + if (pci_dev_is_wake_source(dev)) + elog_add_event_wake(pme_map[i].wake_source, 0); + } +} + static void pch_log_wake_source(struct chipset_power_state *ps) { /* Power Button */ @@ -32,7 +67,7 @@
/* PCI Express (TODO: determine wake device) */ if (ps->pm1_sts & PCIEXPWAK_STS) - elog_add_event_wake(ELOG_WAKE_SOURCE_PCIE, 0); + pch_log_rp_wake_source();
/* PME (TODO: determine wake device) */ if (ps->gpe0_sts[GPE_STD] & PME_STS)
-
Furquan Shaikh (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)