Change in coreboot[master]: pci_device: Add a helper function for logging PCI device as wake source

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for logging PCI device as wake source
......................................................................

pci_device: Add a helper function for logging PCI device as wake source

This change adds a helper function `pci_dev_log_wake()` that checks
PME_STATUS and PME_ENABLE bits in PM control and status register to
determine if the given device is the source of wake and logs it in
eventlog if CONFIG_ELOG is selected. The caller is expected to pass in
the source_id and instance that will be passed into
`elog_add_event_wake()`.

BUG=b:169802515
BRANCH=zork

Change-Id: I06e9530b568543ab2f05a4f38dc5c3a527ff391e
Signed-off-by: Furquan Shaikh 
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46030/1

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 8a6f123..06e1228 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -20,6 +20,7 @@
 #include <device/pcix.h>
 #include <device/pciexp.h>
 #include <device/hypertransport.h>
+#include <elog.h>
 #include <pc80/i8259.h>
 #include <security/vboot/vbnv.h>
 #include <timestamp.h>
@@ -1643,3 +1644,28 @@
    pci_update_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MASTER, 0x0);
 }
 #endif
+
+void pci_dev_log_wake(const struct device *dev, uint8_t source_id, uint32_t instance)
+{
+	unsigned int pm_cap;
+	uint16_t pmcs;
+
+	if (!CONFIG(ELOG))
+		return;
+
+	if (dev->path.type != DEVICE_PATH_PCI)
+		return;
+
+	pm_cap = pci_find_capability(dev, PCI_CAP_ID_PM);
+	if (!pm_cap)
+		return;
+
+	pmcs = pci_read_config16(dev, pm_cap + PCI_PM_CTRL);
+	if (!(pmcs & PCI_PM_CTRL_PME_ENABLE))
+		return;
+
+	if (!(pmcs & PCI_PM_CTRL_PME_STATUS))
+		return;
+
+	elog_add_event_wake(source_id, instance);
+}
diff --git a/src/include/device/pci.h b/src/include/device/pci.h
index 6e28cb7..87702e2 100644
--- a/src/include/device/pci.h
+++ b/src/include/device/pci.h
@@ -80,6 +80,13 @@
 struct device *pci_probe_dev(struct device *dev, struct bus *bus,
    			unsigned int devfn);

+/*
+ * Determine if the given PCI device is the source of wake from sleep by checking PME_STATUS and
+ * PME_ENABLE bits in PM control and status register and log the given source_id and instance in
+ * elog.
+ */
+void pci_dev_log_wake(const struct device *dev, uint8_t source_id, uint32_t instance);
+
 void do_pci_scan_bridge(struct device *dev,
    void (*do_scan_bus)(struct bus *bus,
    	unsigned int min_devfn, unsigned int max_devfn));

[Rob Barnes (Code Review)]

Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for logging PCI device as wake source
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/46030/1/src/include/device/pci.h 
File src/include/device/pci.h:

https://review.coreboot.org/c/coreboot/+/46030/1/src/include/device/pci.h@88 
PS1, Line 88: void pci_dev_log_wake(const struct device *dev, uint8_t source_id, uint32_t instance);
Could split out a lower level utility function: pci_dev_is_wake_src. Then the caller would have more control over logging.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for logging PCI device as wake source
......................................................................

Patch Set 1: Code-Review+1

Change in coreboot[master]: pci_device: Add a helper function for determining if PCI device is wa...

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Duncan Laurie, Rob Barnes, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46030

to look at the new patch set (#2).

Change subject: pci_device: Add a helper function for determining if PCI device is wake source
......................................................................

pci_device: Add a helper function for determining if PCI device is wake source

This change adds a helper function `pci_dev_is_wake_source()` that
checks PME_STATUS and PME_ENABLE bits in PM control and status
register to determine if the given device is the source of wake.

BUG=b:169802515
BRANCH=zork

Change-Id: I06e9530b568543ab2f05a4f38dc5c3a527ff391e
Signed-off-by: Furquan Shaikh 
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 26 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46030/2

Change in coreboot[master]: pci_device: Add a helper function for determining if PCI device is wa...

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for determining if PCI device is wake source
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46030/1/src/include/device/pci.h 
File src/include/device/pci.h:

https://review.coreboot.org/c/coreboot/+/46030/1/src/include/device/pci.h@88 
PS1, Line 88: void pci_dev_log_wake(const struct device *dev, uint8_t source_id, uint32_t instance);
> Could split out a lower level utility function: pci_dev_is_wake_src. […]

Done

Change in coreboot[master]: pci_device: Add a helper function for determining if PCI device is wa...

[Rob Barnes (Code Review)]

Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for determining if PCI device is wake source
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: pci_device: Add a helper function for determining if PCI device is wa...

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for determining if PCI device is wake source
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: pci_device: Add a helper function for determining if PCI device is wa...

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46030 )

Change subject: pci_device: Add a helper function for determining if PCI device is wake source
......................................................................

pci_device: Add a helper function for determining if PCI device is wake source

This change adds a helper function `pci_dev_is_wake_source()` that
checks PME_STATUS and PME_ENABLE bits in PM control and status
register to determine if the given device is the source of wake.

BUG=b:169802515
BRANCH=zork

Change-Id: I06e9530b568543ab2f05a4f38dc5c3a527ff391e
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46030
Tested-by: build bot (Jenkins) 
Reviewed-by: Rob Barnes 
Reviewed-by: Tim Wawrzynczak 
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 26 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tim Wawrzynczak: Looks good to me, approved
  Rob Barnes: Looks good to me, but someone else must approve

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 8a6f123..ce3e509 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -1643,3 +1643,21 @@
    pci_update_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MASTER, 0x0);
 }
 #endif
+
+bool pci_dev_is_wake_source(const struct device *dev)
+{
+	unsigned int pm_cap;
+	uint16_t pmcs;
+
+	if (dev->path.type != DEVICE_PATH_PCI)
+		return false;
+
+	pm_cap = pci_find_capability(dev, PCI_CAP_ID_PM);
+	if (!pm_cap)
+		return false;
+
+	pmcs = pci_read_config16(dev, pm_cap + PCI_PM_CTRL);
+
+	/* PCI Device is a wake source if PME_ENABLE and PME_STATUS are set in PMCS register. */
+	return (pmcs & PCI_PM_CTRL_PME_ENABLE) && (pmcs & PCI_PM_CTRL_PME_STATUS);
+}
diff --git a/src/include/device/pci.h b/src/include/device/pci.h
index 6e28cb7..58f5904 100644
--- a/src/include/device/pci.h
+++ b/src/include/device/pci.h
@@ -80,6 +80,14 @@
 struct device *pci_probe_dev(struct device *dev, struct bus *bus,
    			unsigned int devfn);

+/*
+ * Determine if the given PCI device is the source of wake from sleep by checking PME_STATUS and
+ * PME_ENABLE bits in PM control and status register.
+ *
+ * Returns true if PCI device is wake source, false otherwise.
+ */
+bool pci_dev_is_wake_source(const struct device *dev);
+
 void do_pci_scan_bridge(struct device *dev,
    void (*do_scan_bus)(struct bus *bus,
    	unsigned int min_devfn, unsigned int max_devfn));