Change in coreboot[master]: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
Paul Fagerburg has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
The "internal PME" wake-up source could be from integrated LAN, HD audio/audio DSP, SATA, XHCI, CNVi, or an ME maskable host wake. chromium:1680839 adds USB port details to the wake-up when the XHCI causes the wake-up. Expand the logging for wake-up details to identify and log the other wake-up sources with more details. Note that wake on Integrated LAN (GbE), SATA, and ME Maskable Host Wake are not in use on Hatch, so these will not be tested.
BUG=b:128936450 BRANCH=none TEST=``FW_NAME=hatch emerge-hatch chromeos-ec depthcharge vboot_reference libpayload coreboot-private-files intel-cmlfsp coreboot-private-files-hatch coreboot chromeos-bootimage`` Ensure /build/hatch/firmware/image-hatch.serial.bin has been built. Program image-hatch.serial.bin into the DUT using flashrom. Switch the DUT to the console (Ctrl-Alt-F2, or use the AP console via servo).
XHCI USB 2.0 * Plug a USB keyboard into a USB-A port * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Press a key on the USB keyboard * ``mosys eventlog list`` shows: 12 | 2019-06-26 14:52:23 | S0ix Enter 13 | 2019-06-26 14:53:07 | S0ix Exit 14 | 2019-06-26 14:53:07 | Wake Source | PME - XHCI (USB 2.0 port) | 3 15 | 2019-06-26 14:53:07 | Wake Source | GPE # | 109
CNVi (connected to Wi-Fi): * Enable wake on disconnect via ``iw phy0 wowlan enable disconnect`` * Set up a hotspot on an Android phone * Connect the Chromebook to th hotspot * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Turn off the hotspot on the phone * ``mosys eventlog list`` shows: 8 | 2019-07-11 10:58:17 | S0ix Enter 9 | 2019-07-11 10:59:17 | S0ix Exit 10 | 2019-07-11 10:59:17 | Wake Source | PME - WIFI | 0 11 | 2019-07-11 10:59:17 | Wake Source | GPE # | 109
XHCI USB 3.0 * TBD
HD Audio * TBD
Change-Id: I2c71f6a56b4e1658a7427f67fa78af773b97ec7f Signed-off-by: Paul Fagerburg pfagerburg@chromium.org --- M src/soc/intel/cannonlake/elog.c 1 file changed, 83 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34289/1
diff --git a/src/soc/intel/cannonlake/elog.c b/src/soc/intel/cannonlake/elog.c index 141aa45..7476d7e 100644 --- a/src/soc/intel/cannonlake/elog.c +++ b/src/soc/intel/cannonlake/elog.c @@ -17,6 +17,7 @@ #include <bootstate.h> #include <cbmem.h> #include <console/console.h> +#include <device/pci_ops.h> #include <stdint.h> #include <elog.h> #include <intelblocks/pmclib.h> @@ -36,6 +37,87 @@ .num_usb3_ports = XHCI_USB3_PORT_NUM, };
+struct pme_status_info { +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev; +#else + struct device *dev; +#endif + uint8_t reg_offset; + uint32_t elog_event; +}; + +#define PME_STS_BIT (1 << 15) + +#ifdef __SIMPLE_DEVICE__ +static void pch_log_add_elog_event(const struct pme_status_info *info, + pci_devfn_t dev) +#else +static void pch_log_add_elog_event(const struct pme_status_info *info, + struct device *dev) +#endif +{ + /* + * If wake source is XHCI, check for detailed wake source events on + * USB2/3 ports. + */ + if ((info->dev == PCH_DEV_XHCI) && + pch_xhci_update_wake_event(&usb_info)) + return; + + elog_add_event_wake(info->elog_event, 0); +} + +static void pch_log_pme_internal_wake_source(void) +{ + size_t i; +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev; +#else + struct device *dev; +#endif + uint16_t val; + bool dev_found = false; + + struct pme_status_info pme_status_info[] = { + { PCH_DEV_HDA, 0x54, ELOG_WAKE_SOURCE_PME_HDA }, + { PCH_DEV_GBE, 0xcc, ELOG_WAKE_SOURCE_PME_GBE }, + { PCH_DEV_SATA, 0x74, ELOG_WAKE_SOURCE_PME_SATA }, + { PCH_DEV_CSE, 0x54, ELOG_WAKE_SOURCE_PME_CSE }, + { PCH_DEV_XHCI, 0x74, ELOG_WAKE_SOURCE_PME_XHCI }, + { PCH_DEV_USBOTG, 0x84, ELOG_WAKE_SOURCE_PME_XDCI }, + // Not listed in the cannonlake PCH, see b/136673612 + { PCH_DEV_CNViWIFI,0xcc, ELOG_WAKE_SOURCE_PME_WIFI }, + }; + + for (i = 0; i < ARRAY_SIZE(pme_status_info); i++) { + dev = pme_status_info[i].dev; + if (!dev) + continue; + + val = pci_read_config16(dev, pme_status_info[i].reg_offset); + + if ((val == 0xFFFF) || !(val & PME_STS_BIT)) + continue; + + pch_log_add_elog_event(&pme_status_info[i], dev); + dev_found = true; + } + + /* + * If device is still not found, but the wake source is internal PME, + * try probing XHCI ports to see if any of the USB2/3 ports indicate + * that it was the wake source. This path would be taken in case of GSMI + * logging with S0ix where the pci_pm_resume_noirq runs and clears the + * PME_STS_BIT in controller register. + */ + if (!dev_found) + dev_found = pch_xhci_update_wake_event(&usb_info); + + if (!dev_found) + elog_add_event_wake(ELOG_WAKE_SOURCE_PME_INTERNAL, 0); +} + static void pch_log_gpio_gpe(u32 gpe0_sts, u32 gpe0_en, int start) { int i; @@ -68,7 +150,7 @@
/* XHCI - "Power Management Event Bus 0" events include XHCI */ if (ps->gpe0_sts[GPE_STD] & PME_B0_STS) - pch_xhci_update_wake_event(&usb_info); + pch_log_pme_internal_wake_source();
/* SMBUS Wake */ if (ps->gpe0_sts[GPE_STD] & SMB_WAK_STS)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 90: { PCH_DEV_CNViWIFI,0xcc, ELOG_WAKE_SOURCE_PME_WIFI }, space required after that ',' (ctx:VxV)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 89: b/136673612 This has no meaning for upstream coreboot since b/ is accessible only to Googlers. It is better to just post a comment referencing doc# or have a generic comment that explains the context.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 57: struct device *dev We don't seem to use this parameter inside this function. Do we need it?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
Also maybe rebase on top of Karthik's change (add SoC specific function to get XHCI USB info) ?
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 114: if (!dev_found) : dev_found = pch_xhci_update_wake_event(&usb_info); : I still don't understand why this works (i.e., calling it a second time); if the wake source is XHCI, it should get checked when iterating over the table above?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 114: if (!dev_found) : dev_found = pch_xhci_update_wake_event(&usb_info); :
I still don't understand why this works (i.e. […]
I too thought about it. The only way that can happen is for some reason PME_STS_BIT is not set in the XHCI PME status register, but the wakeup bit is set in one of the individual USB port's status register.
I am not sure if there is a legacy reason to carry this logic forward.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 114: if (!dev_found) : dev_found = pch_xhci_update_wake_event(&usb_info); :
I too thought about it. […]
In case of S0ix, XHCI driver in kernel resumes before GSMI driver runs and XHCI driver ends up clearing PME_STS_BIT. But the individual port connect status is serviced later on and hence that can be used to detect the wake source for USB.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 114: if (!dev_found) : dev_found = pch_xhci_update_wake_event(&usb_info); :
In case of S0ix, XHCI driver in kernel resumes before GSMI driver runs and XHCI driver ends up clear […]
Interesting... that sort of makes this solution Linux- and XHCI-driver specific, doesn't it? What do you think about a weak mainboard hook here to implement this seemingly board-specific functionality?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 114: if (!dev_found) : dev_found = pch_xhci_update_wake_event(&usb_info); :
Interesting... […]
The whole elog for S0ix is kind of Linux specific :). But again that is true for other parts of elog as well i.e. logging kernel shutdown, etc. i.e. it depends on the Linux kernel GSMI driver to make a call into SMI handler to allow it to log certain conditions.
Hello Patrick Rudolph, Karthik Ramasubramanian, Rajat Jain, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34289
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
The "internal PME" wake-up source could be from integrated LAN, HD audio/audio DSP, SATA, XHCI, CNVi, or an ME maskable host wake. chromium:1680839 adds USB port details to the wake-up when the XHCI causes the wake-up. Expand the logging for wake-up details to identify and log the other wake-up sources with more details. Note that wake on Integrated LAN (GbE), SATA, and ME Maskable Host Wake are not in use on Hatch, so these will not be tested.
BUG=b:128936450 BRANCH=none TEST=``FW_NAME=hatch emerge-hatch chromeos-ec depthcharge vboot_reference libpayload coreboot-private-files intel-cmlfsp coreboot-private-files-hatch coreboot chromeos-bootimage`` Ensure /build/hatch/firmware/image-hatch.serial.bin has been built. Program image-hatch.serial.bin into the DUT using flashrom. Switch the DUT to the console (Ctrl-Alt-F2, or use the AP console via servo).
XHCI USB 2.0 * Plug a USB keyboard into a USB-A port * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Press a key on the USB keyboard * ``mosys eventlog list`` shows: 12 | 2019-06-26 14:52:23 | S0ix Enter 13 | 2019-06-26 14:53:07 | S0ix Exit 14 | 2019-06-26 14:53:07 | Wake Source | PME - XHCI (USB 2.0 port) | 3 15 | 2019-06-26 14:53:07 | Wake Source | GPE # | 109
CNVi (connected to Wi-Fi): * Enable wake on disconnect via ``iw phy0 wowlan enable disconnect`` * Set up a hotspot on an Android phone * Connect the Chromebook to th hotspot * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Turn off the hotspot on the phone * ``mosys eventlog list`` shows: 8 | 2019-07-11 10:58:17 | S0ix Enter 9 | 2019-07-11 10:59:17 | S0ix Exit 10 | 2019-07-11 10:59:17 | Wake Source | PME - WIFI | 0 11 | 2019-07-11 10:59:17 | Wake Source | GPE # | 109
XHCI USB 3.0 * TBD
HD Audio * TBD
Change-Id: I2c71f6a56b4e1658a7427f67fa78af773b97ec7f Signed-off-by: Paul Fagerburg pfagerburg@chromium.org --- M src/soc/intel/cannonlake/elog.c 1 file changed, 79 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34289/2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 57: struct device *dev
We don't seem to use this parameter inside this function. […]
Leftover from the skylake code (which BTW also does not seem to use the param). Removed.
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 89: b/136673612
This has no meaning for upstream coreboot since b/ is accessible only to Googlers. […]
Added an explanation of where the offset comes from. Removed reference to buganizer.
https://review.coreboot.org/c/coreboot/+/34289/1/src/soc/intel/cannonlake/el... PS1, Line 114: if (!dev_found) : dev_found = pch_xhci_update_wake_event(&usb_info); :
The whole elog for S0ix is kind of Linux specific :). […]
Per comments in the code and Furquan's comments, no need to change any code here. Also, this is what skylake did.
Hello Patrick Rudolph, Karthik Ramasubramanian, Rajat Jain, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34289
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
The "internal PME" wake-up source could be from integrated LAN, HD audio/audio DSP, SATA, XHCI, CNVi, or an ME maskable host wake. chromium:1680839 adds USB port details to the wake-up when the XHCI causes the wake-up. Expand the logging for wake-up details to identify and log the other wake-up sources with more details. Note that wake on Integrated LAN (GbE), SATA, and ME Maskable Host Wake are not in use on Hatch, so these will not be tested.
BUG=b:128936450 BRANCH=none TEST=``FW_NAME=hatch emerge-hatch chromeos-ec depthcharge vboot_reference libpayload coreboot-private-files intel-cmlfsp coreboot-private-files-hatch coreboot chromeos-bootimage`` Ensure /build/hatch/firmware/image-hatch.serial.bin has been built. Program image-hatch.serial.bin into the DUT using flashrom. Switch the DUT to the console (Ctrl-Alt-F2, or use the AP console via servo).
XHCI USB 2.0 * Plug a USB keyboard into a USB-A port * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Press a key on the USB keyboard * ``mosys eventlog list`` shows: 12 | 2019-06-26 14:52:23 | S0ix Enter 13 | 2019-06-26 14:53:07 | S0ix Exit 14 | 2019-06-26 14:53:07 | Wake Source | PME - XHCI (USB 2.0 port) | 3 15 | 2019-06-26 14:53:07 | Wake Source | GPE # | 109
CNVi (connected to Wi-Fi): * Enable wake on disconnect via ``iw phy0 wowlan enable disconnect`` * Set up a hotspot on an Android phone * Connect the Chromebook to th hotspot * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Turn off the hotspot on the phone * ``mosys eventlog list`` shows: 8 | 2019-07-11 10:58:17 | S0ix Enter 9 | 2019-07-11 10:59:17 | S0ix Exit 10 | 2019-07-11 10:59:17 | Wake Source | PME - WIFI | 0 11 | 2019-07-11 10:59:17 | Wake Source | GPE # | 109
XHCI USB 3.0 * TBD
HD Audio * TBD
Change-Id: I2c71f6a56b4e1658a7427f67fa78af773b97ec7f Signed-off-by: Paul Fagerburg pfagerburg@chromium.org --- M src/soc/intel/cannonlake/elog.c 1 file changed, 80 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34289/3
Hello Patrick Rudolph, Karthik Ramasubramanian, Rajat Jain, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34289
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
The "internal PME" wake-up source could be from integrated LAN, HD audio/audio DSP, SATA, XHCI, CNVi, or an ME maskable host wake. chromium:1680839 adds USB port details to the wake-up when the XHCI causes the wake-up. Expand the logging for wake-up details to identify and log the other wake-up sources with more details. Note that wake on Integrated LAN (GbE), SATA, and ME Maskable Host Wake are not in use on Hatch, so these will not be tested.
BUG=b:128936450 BRANCH=none TEST=``FW_NAME=hatch emerge-hatch chromeos-ec depthcharge vboot_reference libpayload coreboot-private-files intel-cmlfsp coreboot-private-files-hatch coreboot chromeos-bootimage`` Ensure /build/hatch/firmware/image-hatch.serial.bin has been built. Program image-hatch.serial.bin into the DUT using flashrom. Switch the DUT to the console (Ctrl-Alt-F2, or use the AP console via servo).
XHCI USB 2.0 * Plug a USB keyboard into a USB-A port * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Press a key on the USB keyboard * ``mosys eventlog list`` shows: 12 | 2019-06-26 14:52:23 | S0ix Enter 13 | 2019-06-26 14:53:07 | S0ix Exit 14 | 2019-06-26 14:53:07 | Wake Source | PME - XHCI (USB 2.0 port) | 3 15 | 2019-06-26 14:53:07 | Wake Source | GPE # | 109
CNVi (connected to Wi-Fi): * Enable wake on disconnect via ``iw phy0 wowlan enable disconnect`` * Set up a hotspot on an Android phone * Connect the Chromebook to th hotspot * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Turn off the hotspot on the phone * ``mosys eventlog list`` shows: 8 | 2019-07-11 10:58:17 | S0ix Enter 9 | 2019-07-11 10:59:17 | S0ix Exit 10 | 2019-07-11 10:59:17 | Wake Source | PME - WIFI | 0 11 | 2019-07-11 10:59:17 | Wake Source | GPE # | 109
XHCI USB 3.0 * TBD
HD Audio * TBD
Change-Id: I2c71f6a56b4e1658a7427f67fa78af773b97ec7f Signed-off-by: Paul Fagerburg pfagerburg@chromium.org --- M src/soc/intel/cannonlake/elog.c 1 file changed, 79 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34289/4
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 4:
Rebased to resolve the merge conflict with https://review.coreboot.org/c/coreboot/+/34290
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 4: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... PS4, Line 73: // register is at offset 0xCC. Nit: Not sure if we should stick to C style comments.
Hello Patrick Rudolph, Karthik Ramasubramanian, Rajat Jain, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34289
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
The "internal PME" wake-up source could be from integrated LAN, HD audio/audio DSP, SATA, XHCI, CNVi, or an ME maskable host wake. chromium:1680839 adds USB port details to the wake-up when the XHCI causes the wake-up. Expand the logging for wake-up details to identify and log the other wake-up sources with more details. Note that wake on Integrated LAN (GbE), SATA, and ME Maskable Host Wake are not in use on Hatch, so these will not be tested.
BUG=b:128936450 BRANCH=none TEST=``FW_NAME=hatch emerge-hatch chromeos-ec depthcharge vboot_reference libpayload coreboot-private-files intel-cmlfsp coreboot-private-files-hatch coreboot chromeos-bootimage`` Ensure /build/hatch/firmware/image-hatch.serial.bin has been built. Program image-hatch.serial.bin into the DUT using flashrom. Switch the DUT to the console (Ctrl-Alt-F2, or use the AP console via servo).
XHCI USB 2.0 * Plug a USB keyboard into a USB-A port * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Press a key on the USB keyboard * ``mosys eventlog list`` shows: 12 | 2019-06-26 14:52:23 | S0ix Enter 13 | 2019-06-26 14:53:07 | S0ix Exit 14 | 2019-06-26 14:53:07 | Wake Source | PME - XHCI (USB 2.0 port) | 3 15 | 2019-06-26 14:53:07 | Wake Source | GPE # | 109
CNVi (connected to Wi-Fi): * Enable wake on disconnect via ``iw phy0 wowlan enable disconnect`` * Set up a hotspot on an Android phone * Connect the Chromebook to th hotspot * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Turn off the hotspot on the phone * ``mosys eventlog list`` shows: 8 | 2019-07-11 10:58:17 | S0ix Enter 9 | 2019-07-11 10:59:17 | S0ix Exit 10 | 2019-07-11 10:59:17 | Wake Source | PME - WIFI | 0 11 | 2019-07-11 10:59:17 | Wake Source | GPE # | 109
XHCI USB 3.0 * TBD
HD Audio * TBD
Change-Id: I2c71f6a56b4e1658a7427f67fa78af773b97ec7f Signed-off-by: Paul Fagerburg pfagerburg@chromium.org --- M src/soc/intel/cannonlake/elog.c 1 file changed, 81 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34289/5
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... PS4, Line 73: // register is at offset 0xCC.
Nit: Not sure if we should stick to C style comments.
Old habits die hard. I'll fix it.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... PS4, Line 73: // register is at offset 0xCC.
Old habits die hard. I'll fix it.
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... File src/soc/intel/cannonlake/elog.c:
https://review.coreboot.org/c/coreboot/+/34289/4/src/soc/intel/cannonlake/el... PS4, Line 73: // register is at offset 0xCC.
Done
Thanks.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34289 )
Change subject: soc/intel/cannonlake: Split the "internal PME" wake-up into more detail ......................................................................
soc/intel/cannonlake: Split the "internal PME" wake-up into more detail
The "internal PME" wake-up source could be from integrated LAN, HD audio/audio DSP, SATA, XHCI, CNVi, or an ME maskable host wake. chromium:1680839 adds USB port details to the wake-up when the XHCI causes the wake-up. Expand the logging for wake-up details to identify and log the other wake-up sources with more details. Note that wake on Integrated LAN (GbE), SATA, and ME Maskable Host Wake are not in use on Hatch, so these will not be tested.
BUG=b:128936450 BRANCH=none TEST=``FW_NAME=hatch emerge-hatch chromeos-ec depthcharge vboot_reference libpayload coreboot-private-files intel-cmlfsp coreboot-private-files-hatch coreboot chromeos-bootimage`` Ensure /build/hatch/firmware/image-hatch.serial.bin has been built. Program image-hatch.serial.bin into the DUT using flashrom. Switch the DUT to the console (Ctrl-Alt-F2, or use the AP console via servo).
XHCI USB 2.0 * Plug a USB keyboard into a USB-A port * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Press a key on the USB keyboard * ``mosys eventlog list`` shows: 12 | 2019-06-26 14:52:23 | S0ix Enter 13 | 2019-06-26 14:53:07 | S0ix Exit 14 | 2019-06-26 14:53:07 | Wake Source | PME - XHCI (USB 2.0 port) | 3 15 | 2019-06-26 14:53:07 | Wake Source | GPE # | 109
CNVi (connected to Wi-Fi): * Enable wake on disconnect via ``iw phy0 wowlan enable disconnect`` * Set up a hotspot on an Android phone * Connect the Chromebook to th hotspot * ``powerd_dbus_suspend`` * Verify low power mode by issuing the ``powerinfo`` command on the EC console (via servo). Expect to see ``power state 4 = S0ix``. * Turn off the hotspot on the phone * ``mosys eventlog list`` shows: 8 | 2019-07-11 10:58:17 | S0ix Enter 9 | 2019-07-11 10:59:17 | S0ix Exit 10 | 2019-07-11 10:59:17 | Wake Source | PME - WIFI | 0 11 | 2019-07-11 10:59:17 | Wake Source | GPE # | 109
XHCI USB 3.0 * TBD
HD Audio * TBD
Change-Id: I2c71f6a56b4e1658a7427f67fa78af773b97ec7f Signed-off-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/34289 Reviewed-by: Karthik Ramasubramanian kramasub@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/elog.c 1 file changed, 81 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/elog.c b/src/soc/intel/cannonlake/elog.c index 0bccdb7..a2c359f 100644 --- a/src/soc/intel/cannonlake/elog.c +++ b/src/soc/intel/cannonlake/elog.c @@ -17,6 +17,7 @@ #include <bootstate.h> #include <cbmem.h> #include <console/console.h> +#include <device/pci_ops.h> #include <stdint.h> #include <elog.h> #include <intelblocks/pmclib.h> @@ -24,6 +25,85 @@ #include <soc/pci_devs.h> #include <soc/pm.h>
+struct pme_status_info { +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev; +#else + struct device *dev; +#endif + uint8_t reg_offset; + uint32_t elog_event; +}; + +#define PME_STS_BIT (1 << 15) + +static void pch_log_add_elog_event(const struct pme_status_info *info) +{ + /* + * If wake source is XHCI, check for detailed wake source events on + * USB2/3 ports. + */ + if ((info->dev == PCH_DEV_XHCI) && + pch_xhci_update_wake_event(soc_get_xhci_usb_info())) + return; + + elog_add_event_wake(info->elog_event, 0); +} + +static void pch_log_pme_internal_wake_source(void) +{ + size_t i; +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev; +#else + struct device *dev; +#endif + uint16_t val; + bool dev_found = false; + + struct pme_status_info pme_status_info[] = { + { PCH_DEV_HDA, 0x54, ELOG_WAKE_SOURCE_PME_HDA }, + { PCH_DEV_GBE, 0xcc, ELOG_WAKE_SOURCE_PME_GBE }, + { PCH_DEV_SATA, 0x74, ELOG_WAKE_SOURCE_PME_SATA }, + { PCH_DEV_CSE, 0x54, ELOG_WAKE_SOURCE_PME_CSE }, + { PCH_DEV_XHCI, 0x74, ELOG_WAKE_SOURCE_PME_XHCI }, + { PCH_DEV_USBOTG, 0x84, ELOG_WAKE_SOURCE_PME_XDCI }, + /* + * The power management control/status register is not + * listed in the cannonlake PCH EDS. We have been told + * that the PMCS register is at offset 0xCC. + */ + { PCH_DEV_CNViWIFI, 0xcc, ELOG_WAKE_SOURCE_PME_WIFI }, + }; + + for (i = 0; i < ARRAY_SIZE(pme_status_info); i++) { + dev = pme_status_info[i].dev; + if (!dev) + continue; + + val = pci_read_config16(dev, pme_status_info[i].reg_offset); + + if ((val == 0xFFFF) || !(val & PME_STS_BIT)) + continue; + + pch_log_add_elog_event(&pme_status_info[i]); + dev_found = true; + } + + /* + * If device is still not found, but the wake source is internal PME, + * try probing XHCI ports to see if any of the USB2/3 ports indicate + * that it was the wake source. This path would be taken in case of GSMI + * logging with S0ix where the pci_pm_resume_noirq runs and clears the + * PME_STS_BIT in controller register. + */ + if (!dev_found) + dev_found = pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + + if (!dev_found) + elog_add_event_wake(ELOG_WAKE_SOURCE_PME_INTERNAL, 0); +} + static void pch_log_gpio_gpe(u32 gpe0_sts, u32 gpe0_en, int start) { int i; @@ -56,7 +136,7 @@
/* XHCI - "Power Management Event Bus 0" events include XHCI */ if (ps->gpe0_sts[GPE_STD] & PME_B0_STS) - pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + pch_log_pme_internal_wake_source();
/* SMBUS Wake */ if (ps->gpe0_sts[GPE_STD] & SMB_WAK_STS)
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)
-
Tim Wawrzynczak (Code Review)