Change in coreboot[master]: device: Add pci_s_dev_is_wake_source function

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47260 )

Change subject: device: Add pci_s_dev_is_wake_source function
......................................................................

device: Add pci_s_dev_is_wake_source function

This function is the SIMPLE_DEVICE equivalent of
`pci_dev_is_wake_source`, which is required so that elog can use this
function, which since it runs in SMM as well, it uses the simple device
model.

Signed-off-by: Tim Wawrzynczak 
Change-Id: Ie5583c04366c9a16bc1b00a6892d39eeafe5da49
---
M src/device/pci_ops.c
M src/include/device/pci_ops.h
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/47260/1

diff --git a/src/device/pci_ops.c b/src/device/pci_ops.c
index 90f45bf..cb833f8 100644
--- a/src/device/pci_ops.c
+++ b/src/device/pci_ops.c
@@ -78,3 +78,18 @@
 {
    die("PCI: dev is NULL!\n");
 }
+
+bool pci_s_dev_is_wake_source(pci_devfn_t dev)
+{
+	unsigned int pm_cap;
+	uint16_t pmcs;
+
+	pm_cap = pci_s_find_capability(dev, PCI_CAP_ID_PM);
+	if (!pm_cap)
+		return false;
+
+	pmcs = pci_s_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_ops.h b/src/include/device/pci_ops.h
index cdf02d6..1738cd1 100644
--- a/src/include/device/pci_ops.h
+++ b/src/include/device/pci_ops.h
@@ -209,4 +209,12 @@
    return pci_s_find_capability(PCI_BDF(dev), cap);
 }

+/*
+ * 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_s_dev_is_wake_source(pci_devfn_t dev);
+
 #endif /* PCI_OPS_H */

[Furquan Shaikh (Code Review)]

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

Change subject: device: Add pci_s_dev_is_wake_source function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c 
File src/device/pci_ops.c:

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c@82 
PS1, Line 82: pci_s_dev_is_wake_source
Do we need this separate _s_ call implemented for this function? `pci_dev_is_wake_source()` calls pci_find_capability and pci_read_config16 -- both functions work irrespective of whether ENV_PCI_SIMPLE_DEVICE is defined or not. So, it should be safe to just move `pci_dev_is_wake_source()` from pci_device.c to this file.

[Tim Wawrzynczak (Code Review)]

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

Change subject: device: Add pci_s_dev_is_wake_source function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c 
File src/device/pci_ops.c:

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c@82 
PS1, Line 82: pci_s_dev_is_wake_source
> Do we need this separate _s_ call implemented for this function? `pci_dev_is_wake_source()` calls pc […]

A few problems:
1) the check for the dev->type in the original, which you can't do
2)

src/device/pci_ops.c: In function 'pci_dev_is_wake_source':
src/device/pci_ops.c:95:27: error: passing argument 1 of 'pci_s_read_config16' makes integer from pointer without a cast [-Werror=int-conversion]
  pmcs = pci_read_config16(dev, pm_cap + PCI_PM_CTRL);
                           ^~~
In file included from src/arch/x86/include/arch/pci_ops.h:7,
                 from src/include/device/pci_ops.h:9,
                 from src/include/device/pci.h:25,
                 from src/device/pci_ops.c:5:
src/include/device/pci_mmio_cfg.h:116:42: note: expected 'pci_devfn_t' {aka 'unsigned int'} but argument is of type 'const struct device *'
 uint16_t pci_s_read_config16(pci_devfn_t dev, uint16_t reg)

blah blah, which means also the original would have to change to call pci_s_read_config16 and grab the BDF manually, i.e. see the top of pci_ops.h:

```
#if ENV_PCI_SIMPLE_DEVICE

/* Avoid name collisions as different stages have different signature                                                                                                                                              
 * for these functions. The _s_ stands for simple, fundamental IO or                                                                                                                                               
 * MMIO variant.                                                                                                                                                                                                   
 */
#define pci_read_config8 pci_s_read_config8
#define pci_read_config16 pci_s_read_config16
#define pci_read_config32 pci_s_read_config32
#define pci_write_config8 pci_s_write_config8
#define pci_write_config16 pci_s_write_config16
#define pci_write_config32 pci_s_write_config32

```

Change in coreboot[master]: device: Move pci_dev_is_wake_source function

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Karthik Ramasubramanian,

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

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

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

Change subject: device: Move pci_dev_is_wake_source function
......................................................................

device: Move pci_dev_is_wake_source function

Move this function to pci_ops.c, which is already included in the smm
build. This is required to use this function in elog functionality,
which is called from SMM.

Signed-off-by: Tim Wawrzynczak 
Change-Id: Ie5583c04366c9a16bc1b00a6892d39eeafe5da49
---
M src/device/pci_device.c
M src/device/pci_ops.c
M src/include/device/pci.h
M src/include/device/pci_ops.h
4 files changed, 26 insertions(+), 27 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/47260/2

Change in coreboot[master]: device: Move pci_dev_is_wake_source function

[Tim Wawrzynczak (Code Review)]

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

Change subject: device: Move pci_dev_is_wake_source function
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c 
File src/device/pci_ops.c:

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c@82 
PS1, Line 82: pci_s_dev_is_wake_source
> A few problems: […]

got it, see new patchset

Change in coreboot[master]: device: Move pci_dev_is_wake_source function

[Furquan Shaikh (Code Review)]

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

Change subject: device: Move pci_dev_is_wake_source function
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: device: Move pci_dev_is_wake_source function

[Furquan Shaikh (Code Review)]

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

Change subject: device: Move pci_dev_is_wake_source function
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c 
File src/device/pci_ops.c:

https://review.coreboot.org/c/coreboot/+/47260/1/src/device/pci_ops.c@82 
PS1, Line 82: pci_s_dev_is_wake_source
> got it, see new patchset

Ack

Change in coreboot[master]: device: Move pci_dev_is_wake_source function

[Karthik Ramasubramanian (Code Review)]

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

Change subject: device: Move pci_dev_is_wake_source function
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: device: Move pci_dev_is_wake_source function

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47260 )

Change subject: device: Move pci_dev_is_wake_source function
......................................................................

device: Move pci_dev_is_wake_source function

Move this function to pci_ops.c, which is already included in the smm
build. This is required to use this function in elog functionality,
which is called from SMM.

Signed-off-by: Tim Wawrzynczak 
Change-Id: Ie5583c04366c9a16bc1b00a6892d39eeafe5da49
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47260
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Karthik Ramasubramanian 
---
M src/device/pci_device.c
M src/device/pci_ops.c
M src/include/device/pci.h
M src/include/device/pci_ops.h
4 files changed, 26 insertions(+), 27 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Karthik Ramasubramanian: Looks good to me, approved

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 691ad6b..6075ebe 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -1648,21 +1648,3 @@
    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/device/pci_ops.c b/src/device/pci_ops.c
index 90f45bf..aaa9f95 100644
--- a/src/device/pci_ops.c
+++ b/src/device/pci_ops.c
@@ -78,3 +78,21 @@
 {
    die("PCI: dev is NULL!\n");
 }
+
+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_s_read_config16(PCI_BDF(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 777f030..045eec1 100644
--- a/src/include/device/pci.h
+++ b/src/include/device/pci.h
@@ -79,15 +79,6 @@
 void pci_bus_reset(struct bus *bus);
 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));
diff --git a/src/include/device/pci_ops.h b/src/include/device/pci_ops.h
index cdf02d6..7fe7d42 100644
--- a/src/include/device/pci_ops.h
+++ b/src/include/device/pci_ops.h
@@ -209,4 +209,12 @@
    return pci_s_find_capability(PCI_BDF(dev), cap);
 }

+/*
+ * 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);
+
 #endif /* PCI_OPS_H */