PCIe: Add PCIe runtime D3cold support

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +1 -1

index 0f150f2..e210e8cb 100644
@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

drivers/pci/pci-acpi.c +0 -0

 	if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +2 -2

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	int error;
 
 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

drivers/pci/pci.c +16 -1

 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 
+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = dev->bus->self;
+
+	/* don't poll the pme bit if parent is in low power state */
+	if (bridge && bridge->current_state != PCI_D0)
+		return;
+
+	pci_pme_wakeup(dev, NULL);
+}
+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +25 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);
+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_restore_state(pdev);
+	if (pdev->runtime_d3cold)
+		msleep(100);
+
+	return 0;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume = pcie_port_runtime_resume,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)

@@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
+	pm_runtime_put_noidle(&dev->dev);
 
 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	pm_runtime_get_noresume(&dev->dev);
 	pcie_port_device_remove(dev);
 	pci_disable_device(dev);
 }

include/linux/pci.h +1 -1

index e444f5b..b41d9a1 100644
@@ -281,6 +281,7 @@ struct pci_dev {
 	unsigned int	no_d1d2:1;	/* Only allow D0 and D3 */
 	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
 						   decoding during bar sizing */
+	unsigned int	runtime_d3cold:1;
 	unsigned int	wakeup_prepared:1;
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 
--

To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 26 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +1 -1

index 0f150f2..e210e8cb 100644
@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 301

Quoted 6 lines

drivers/pci/pci-acpi.c +2 -1

 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)
 		return 0;

PCI BUS PM Interface spec says that PMEs can be support for D3COLD. Please check section 3.2.4 , 5.4.2 and 5.4.3.

Quoted 21 lines

drivers/pci/pci-acpi.c +0 -0

 	if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +2 -2

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	int error;
 
 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 754

Quoted 3 lines

drivers/pci/pci.c +2 -1

-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+					PCI_D3hot : state);

Do you want to set the pci device to D3hot if the state = D3Code through PMCSR register? I am agree if you realy want that but what is the meaninig to distingulish D3cold with D3hot in this patch. We can't put the PCI device through PMCSR register.

Quoted 127 lines

drivers/pci/pci.c +16 -1

 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 
+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = dev->bus->self;
+
+	/* don't poll the pme bit if parent is in low power state */
+	if (bridge && bridge->current_state != PCI_D0)
+		return;
+
+	pci_pme_wakeup(dev, NULL);
+}
+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +25 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);
+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_restore_state(pdev);
+	if (pdev->runtime_d3cold)
+		msleep(100);
+
+	return 0;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume = pcie_port_runtime_resume,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)

@@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
+	pm_runtime_put_noidle(&dev->dev);
 
 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	pm_runtime_get_noresume(&dev->dev);
 	pcie_port_device_remove(dev);
 	pci_disable_device(dev);
 }

include/linux/pci.h +1 -1

index e444f5b..b41d9a1 100644
@@ -281,6 +281,7 @@ struct pci_dev {
 	unsigned int	no_d1d2:1;	/* Only allow D0 and D3 */
 	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
 						   decoding during bar sizing */
+	unsigned int	runtime_d3cold:1;
 	unsigned int	wakeup_prepared:1;
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 
--

To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Fri, Apr 13, 2012 at 10:35:34AM +0800, Yan, Zheng wrote:

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 754

Quoted 7 lines

drivers/pci/pci.c +2 -1

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
       if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
               return 0;
 
-     error = pci_raw_set_power_state(dev, state);
+     error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+                                     PCI_D3hot : state);

Do you want to set the pci device to D3hot if the state = D3Code through PMCSR register?

I am agree if you realy want that but what is the meaninig
to distingulish D3cold with D3hot in this patch. We can't put the PCI device through PMCSR register.

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci.c +4 -3

 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+					PCI_D3hot : state);
 
 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 

drivers/pci/pci.c +6 -1

+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

drivers/pci/pcie/portdrv_pci.c +6 -0

+
+	return 0;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume = pcie_port_runtime_resume,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)

drivers/pci/pcie/portdrv_pci.c +1 -0

 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	pm_runtime_get_noresume(&dev->dev);
 	pcie_port_device_remove(dev);
 	pci_disable_device(dev);
 }

include/linux/pci.h +0 -0

index e444f5b..b41d9a1 100644

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 30 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

Quoted 3 lines

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

Hi Rafael,

Have you started to write the patch?

If not, I can do it.

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 7 lines

drivers/pci/pci-acpi.c +2 -1

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 
 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

 		return 0;
 
 	if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	int error;
 

Guys, please. Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

Quoted 22 lines

drivers/pci/pci.c +4 -3

 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+					PCI_D3hot : state);
 
 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = dev->bus->self;
+
+	/* don't poll the pme bit if parent is in low power state */
+	if (bridge && bridge->current_state != PCI_D0)
+		return;
+
+	pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

Quoted 47 lines

drivers/pci/pci.c +6 -1

+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 5 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);

Are you sure this is sufficient?

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 118

Quoted 11 lines

drivers/pci/pcie/portdrv_pci.c +10 -0

+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_restore_state(pdev);
+	if (pdev->runtime_d3cold)
+		msleep(100);

What's _that_ supposed to do?

Quoted 18 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+
+	return 0;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume = pcie_port_runtime_resume,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)

drivers/pci/pcie/portdrv_pci.c static int __devinit pcie_portdrv_probe(struct pci_dev *dev, · line 170

Quoted 4 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

@@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
+	pm_runtime_put_noidle(&dev->dev);

What's the purpose of this?

Quoted 14 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	pm_runtime_get_noresume(&dev->dev);
 	pcie_port_device_remove(dev);
 	pci_disable_device(dev);
 }

include/linux/pci.h +0 -0

index e444f5b..b41d9a1 100644

include/linux/pci.h struct pci_dev { · line 286

Quoted 6 lines

include/linux/pci.h +1 -0

@@ -281,6 +281,7 @@ struct pci_dev {
 	unsigned int	no_d1d2:1;	/* Only allow D0 and D3 */
 	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
 						   decoding during bar sizing */
+	unsigned int	runtime_d3cold:1;
 	unsigned int	wakeup_prepared:1;
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */

OK

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Thanks,
Rafael

Quoted 45 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 7 lines

drivers/pci/pci-acpi.c +2 -1

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 
 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

Quoted 4 lines

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

The reason I introduced the runtime_d3cold flag is I can't make the PME interrupt work in the D3cold case in our test platform. Document for our test platform says A device in D3cold can only generate wake event through the WAKE# pin.

Quoted 39 lines

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

 		return 0;
 
 	if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	int error;
 

Guys, please. Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

Quoted 22 lines

drivers/pci/pci.c +4 -3

 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+					PCI_D3hot : state);
 
 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 

Quoted 13 lines

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = dev->bus->self;
+
+	/* don't poll the pme bit if parent is in low power state */
+	if (bridge && bridge->current_state != PCI_D0)
+		return;
+
+	pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

put it into pci_pme_wakeup will break ACPI wakeup, because ACPI wakeup also uses pci_pme_wakeup. When a device exits from D3cold by asserting the WAKE# signal, device power is restored automatically by ACPI, then pci_pme_wakeup is called to check device's PME bit. pci_dev->current_state is PCI_D3hot during the checking.

Quoted 46 lines

Quoted 46 lines

drivers/pci/pci.c +6 -1

+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 

Quoted 7 lines

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 5 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);

Are you sure this is sufficient?

What else should I do?

Quoted 4 lines

Quoted 4 lines

drivers/pci/pcie/portdrv_pci.c +4 -0

+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)

Quoted 8 lines

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 118

Quoted 6 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_restore_state(pdev);
+	if (pdev->runtime_d3cold)
+		msleep(100);

What's _that_ supposed to do?

Bad thing happens if device is accessed immediately after restoring power. Document for our test platform states 100ms PERST delay is required.

Moreover, I really don't think it's a good idea to put PCI Express ports into low-power states in general.  It might work on your platform, whatever it is, but that doesn't mean it's going to work on every PCI Express system out there.  I actually know of a number of such systems where it surely won't work at all.

Quoted 5 lines · Rafael J. Wysocki

Moreover, I really don't think it's a good idea to put PCI Express ports into low-power states in general. It might work on your platform, whatever it is, but that doesn't mean it's going to work on every PCI Express system out there. I actually know of a number of such systems where it surely won't work at all.

The runtime PM support for PCIe port is not turned on by default, we need

# echo auto > /sys/devices/pcixxxx:xx/<pci id>/power/control

to turn on it. And we will turn on it on system it works. Is this sufficient?

Quoted 9 lines · huang ying

Quoted 5 lines · Rafael J. Wysocki

Moreover, I really don't think it's a good idea to put PCI Express ports into low-power states in general.  It might work on your platform, whatever it is, but that doesn't mean it's going to work on every PCI Express system out there.  I actually know of a number of such systems where it surely won't work at all.

The runtime PM support for PCIe port is not turned on by default, we need

Quoted 3 lines

# echo auto > /sys/devices/pcixxxx:xx/<pci id>/power/control

to turn on it.  And we will turn on it on system it works.  Is this sufficient?

No, it is not.  In some cases it shouldn't be enabled at all for PCI Express ports, as far as I can say, so to be on the safe side we should only enable it on platforms where PCI Express ports are known to work correctly with runtime PM.

Quoted 14 lines · Rafael J. Wysocki

Quoted 12 lines · huang ying

Quoted 5 lines · Rafael J. Wysocki

Moreover, I really don't think it's a good idea to put PCI Express ports into low-power states in general. It might work on your platform, whatever it is, but that doesn't mean it's going to work on every PCI Express system out there. I actually know of a number of such systems where it surely won't work at all.

The runtime PM support for PCIe port is not turned on by default, we need

# echo auto > /sys/devices/pcixxxx:xx/<pci id>/power/control

to turn on it. And we will turn on it on system it works. Is this sufficient?

Quoted 3 lines

No, it is not. In some cases it shouldn't be enabled at all for PCI Express ports, as far as I can say, so to be on the safe side we should only enable it on platforms where PCI Express ports are known to work correctly with runtime PM.

Sorry, I did not catch your idea exactly. I think disable it by default and enable it by sysfs interface is what you suggested, isn't it?

Or you suggest something like "white list"?

Hi,

Quoted 46 lines · Zheng

Quoted 45 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 7 lines

drivers/pci/pci-acpi.c +2 -1

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 
 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

Quoted 7 lines

Quoted 4 lines

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

The reason I introduced the runtime_d3cold flag is I can't make the PME interrupt work in the D3cold case in our test platform. Document for our test platform says A device in D3cold can only generate wake event through the WAKE# pin.

OK, so that clearly is not a standard PCI device, so please don't try to make our PCI bus type handle non-standard devices.

Quoted 53 lines

Quoted 39 lines

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

 		return 0;
 
 	if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	int error;
 

Guys, please. Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

Quoted 22 lines

drivers/pci/pci.c +4 -3

 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+					PCI_D3hot : state);
 
 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 

Quoted 13 lines

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = dev->bus->self;
+
+	/* don't poll the pme bit if parent is in low power state */
+	if (bridge && bridge->current_state != PCI_D0)
+		return;
+
+	pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

Quoted 5 lines

put it into pci_pme_wakeup will break ACPI wakeup, because ACPI wakeup also uses pci_pme_wakeup. When a device exits from D3cold by asserting the WAKE# signal, device power is restored automatically by ACPI, then pci_pme_wakeup is called to check device's PME bit. pci_dev->current_state is PCI_D3hot during the checking.

You seem to be talking about two different device here. One device is dev, which is the one being checked for the PME bit and the second one is the bridge that should be in D0 so that dev is accessible, right?

So if you put this code into pci_pme_wakeup():

	struct pci_dev *bridge = pci_dev->bus->self;

	...

	if (bridge && bridge->current_state != PCI_D0)
		return 0;

then it should work just like your code above.

BTW, can you please explain to me what the #WAKE signal is and how it is different from PME#?

Quoted 55 lines

Quoted 54 lines

Quoted 44 lines

drivers/pci/pci.c +6 -1

+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 8 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);

Are you sure this is sufficient?

Quoted 1 line

What else should I do?

First off, you need not do pci_save_state() here, most likely, because pci_pm_runtime_suspend() will do it for you later.

Second, you're suspending a device that has children here. Namely, the PCIe service devices are children of the port they are associated with. I wonder if they are suspended and how exactly, if so?

Quoted 2 lines · Rafael J. Wysocki

BTW, can you please explain to me what the #WAKE signal is and how it is different from PME#?

#WAKE signal is triggered by a pin connected to the root complex or other motherboard logic. PME# is triggered by PME message sent to the port.

Quoted 7 lines · Zheng

Quoted 2 lines · Rafael J. Wysocki

BTW, can you please explain to me what the #WAKE signal is and how it is different from PME#?

#WAKE signal is triggered by a pin connected to the root complex or other motherboard logic. PME# is triggered by PME message sent to the port.

PME# is a PCI pin, while WAKE# is a PCI Express pin. In PCI Express, there is no PME#, PME is delivered between end point device and root port or root complex event collector via PME message, and the PME message will trigger IRQ on root port or root complex event collector.
WAKE# is not used for PCI Express D1, D2 and D3hot, it is just used
by D3cold. When remote wakeup detected by end point device, it will assert WAKE# to notify power controller (implemented via ACPI on some platform), then power controller will turn on power for main link, after link goes back to L0, PME message will be sent to root port or root complex event collector by end point device.

Quoted 9 lines · huang ying

Quoted 7 lines · Zheng

Quoted 2 lines · Rafael J. Wysocki

BTW, can you please explain to me what the #WAKE signal is and how it is different from PME#?

#WAKE signal is triggered by a pin connected to the root complex or other motherboard logic. PME# is triggered by PME message sent to the port.

Quoted 10 lines

PME# is a PCI pin, while WAKE# is a PCI Express pin.  In PCI Express, there is no PME#, PME is delivered between end point device and root port or root complex event collector via PME message, and the PME message will trigger IRQ on root port or root complex event collector. WAKE# is not used for PCI Express D1, D2 and D3hot, it is just used by D3cold.  When remote wakeup detected by end point device, it will assert WAKE# to notify power controller (implemented via ACPI on some platform), then power controller will turn on power for main link, after link goes back to L0, PME message will be sent to root port or root complex event collector by end point device.

OK

So do I understand correctly that the WAKE# signal here is the one described in Section 5.3.3.2 Link Wakeup of PCI Express Base spec. 2.0?

So what happens is that it triggers a GPE and that GPE has a _Lxx method associated with it, I suppose.  Is that correct?

Quoted 23 lines · Rafael J. Wysocki

Quoted 9 lines · huang ying

Quoted 7 lines · Zheng

Quoted 2 lines · Rafael J. Wysocki

BTW, can you please explain to me what the #WAKE signal is and how it is different from PME#?

#WAKE signal is triggered by a pin connected to the root complex or other motherboard logic. PME# is triggered by PME message sent to the port.

Quoted 10 lines

PME# is a PCI pin, while WAKE# is a PCI Express pin. In PCI Express, there is no PME#, PME is delivered between end point device and root port or root complex event collector via PME message, and the PME message will trigger IRQ on root port or root complex event collector.
WAKE# is not used for PCI Express D1, D2 and D3hot, it is just used
by D3cold. When remote wakeup detected by end point device, it will assert WAKE# to notify power controller (implemented via ACPI on some platform), then power controller will turn on power for main link, after link goes back to L0, PME message will be sent to root port or root complex event collector by end point device.

OK

Quoted 5 lines

So do I understand correctly that the WAKE# signal here is the one described in Section 5.3.3.2 Link Wakeup of PCI Express Base spec. 2.0?

So what happens is that it triggers a GPE and that GPE has a _Lxx method associated with it, I suppose. Is that correct?

Yes.

Quoted 119 lines · Rafael J. Wysocki

Hi,

Quoted 46 lines · Zheng

Quoted 45 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 7 lines

drivers/pci/pci-acpi.c +2 -1

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 
 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

Quoted 7 lines

Quoted 4 lines

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

The reason I introduced the runtime_d3cold flag is I can't make the PME interrupt work in the D3cold case in our test platform. Document for our test platform says A device in D3cold can only generate wake event through the WAKE# pin.

OK, so that clearly is not a standard PCI device, so please don't try to make our PCI bus type handle non-standard devices.

Quoted 53 lines

Quoted 39 lines

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

 		return 0;
 
 	if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	int error;
 

Guys, please. Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

Quoted 22 lines

drivers/pci/pci.c +4 -3

 	/* bound the state we're entering */
-	if (state > PCI_D3hot)
-		state = PCI_D3hot;
+	if (state > PCI_D3cold)
+		state = PCI_D3cold;
 	else if (state < PCI_D0)
 		state = PCI_D0;
 	else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+					PCI_D3hot : state);
 
 	if (!__pci_complete_power_transition(dev, state))
 		error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 

Quoted 13 lines

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = dev->bus->self;
+
+	/* don't poll the pme bit if parent is in low power state */
+	if (bridge && bridge->current_state != PCI_D0)
+		return;
+
+	pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

Quoted 5 lines

put it into pci_pme_wakeup will break ACPI wakeup, because ACPI wakeup also uses pci_pme_wakeup. When a device exits from D3cold by asserting the WAKE# signal, device power is restored automatically by ACPI, then pci_pme_wakeup is called to check device's PME bit. pci_dev->current_state is PCI_D3hot during the checking.

Quoted 3 lines

You seem to be talking about two different device here. One device is dev, which is the one being checked for the PME bit and the second one is the bridge that should be in D0 so that dev is accessible, right?

Yes

Quoted 5 lines

So if you put this code into pci_pme_wakeup():

	struct pci_dev *bridge = pci_dev->bus->self;

Quoted 6 lines

	...

	if (bridge && bridge->current_state != PCI_D0)
		return 0;

then it should work just like your code above.

No. In the wakeup case, bridge's power is restored automatically by ACPI, but bridge->current_state does not reflect the power state, it's still PCI_D3hot.

Quoted 2 lines

BTW, can you please explain to me what the #WAKE signal is and how it is different from PME#?

#WAKE signal is triggered by a pin connected to the root complex or other motherboard logic. PME# is triggered by PME message sent to the port.

Quoted 54 lines

Quoted 54 lines

Quoted 53 lines

Quoted 43 lines

drivers/pci/pci.c +6 -1

+
 static void pci_pme_list_scan(struct work_struct *work)
 {
 	struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
 	if (!list_empty(&pci_pme_list)) {
 		list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
 			if (pme_dev->dev->pme_poll) {
-				pci_pme_wakeup(pme_dev->dev, NULL);
+				pci_pme_poll_wakeup(pme_dev->dev);
 			} else {
 				list_del(&pme_dev->list);
 				kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 	if (enable) {
 		int error;
 
+		if (runtime && state >= PCI_D3cold)
+			dev->runtime_d3cold = true;
+		else
+			dev->runtime_d3cold = false;
 		if (pci_pme_capable(dev, state))
 			pci_pme_active(dev, true);
 		else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 8 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);

Are you sure this is sufficient?

Quoted 4 lines

Quoted 1 line

What else should I do?

First off, you need not do pci_save_state() here, most likely, because pci_pm_runtime_suspend() will do it for you later.

Our test platform behaves strangely if this pci_save_state is removed. I guess it's because pci_fixup_device is called before pci_save_state in pci_pm_runtime_suspend

Quoted 4 lines

Second, you're suspending a device that has children here. Namely, the PCIe service devices are children of the port they are associated with. I wonder if they are suspended and how exactly, if so?

Only the PME service has the PM callbacks.

I don't think we should stop the PME service in the runtime suspend case.

For example, when both device and port are
in D3hot, No PME interrupt can be received if we stop the PME service.

Quoted 7 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 7 lines

drivers/pci/pci-acpi.c +2 -1

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 
 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

Quoted 11 lines · Rafael J. Wysocki

Quoted 7 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 7 lines

drivers/pci/pci-acpi.c +2 -1

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 
 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-	if (dev->pme_interrupt)
+	/* PME interrupt isn't available in the D3cold case */
+	if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

Quoted 4 lines

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

I rechecked this. The port does trigger PME interrupt, but after the WAKE# signal restores device power. So my comments "PME interrupt isn't available in the D3cold case" is wrong, but setup ACPI wakeup is still required.

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
               [PCI_D1] = ACPI_STATE_D1,
               [PCI_D2] = ACPI_STATE_D2,
               [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
       };
       int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead.  I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

  static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
  {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong.  First off, I don't think that the runtime_d3cold flag makes any sense.  We already cover that in dev->pme_support.

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

               return 0;
 
       if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
       int error;
 

Guys, please.  Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

Quoted 22 lines

drivers/pci/pci.c +4 -3

       /* bound the state we're entering */
-     if (state > PCI_D3hot)
-             state = PCI_D3hot;
+     if (state > PCI_D3cold)
+             state = PCI_D3cold;
       else if (state < PCI_D0)
               state = PCI_D0;
       else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
       if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
               return 0;
 
-     error = pci_raw_set_power_state(dev, state);
+     error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+                                     PCI_D3hot : state);
 
       if (!__pci_complete_power_transition(dev, state))
               error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
       return !!(dev->pme_support & (1 << state));
  }
 

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+     struct pci_dev *bridge = dev->bus->self;
+
+     /* don't poll the pme bit if parent is in low power state */
+     if (bridge && bridge->current_state != PCI_D0)
+             return;
+
+     pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

Quoted 46 lines

drivers/pci/pci.c +6 -1

+
  static void pci_pme_list_scan(struct work_struct *work)
  {
       struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
       if (!list_empty(&pci_pme_list)) {
               list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
                       if (pme_dev->dev->pme_poll) {
-                             pci_pme_wakeup(pme_dev->dev, NULL);
+                             pci_pme_poll_wakeup(pme_dev->dev);
                       } else {
                               list_del(&pme_dev->list);
                               kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
       if (enable) {
               int error;
 
+             if (runtime && state >= PCI_D3cold)
+                     dev->runtime_d3cold = true;
+             else
+                     dev->runtime_d3cold = false;
               if (pci_pme_capable(dev, state))
                       pci_pme_active(dev, true);

              else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
  #include <linux/kernel.h>
  #include <linux/errno.h>
  #include <linux/pm.h>
+#include <linux/pm_runtime.h>
  #include <linux/init.h>
  #include <linux/pcieport_if.h>
  #include <linux/aer.h>
  #include <linux/dmi.h>
  #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
  #include "portdrv.h"
  #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
       return 0;
  }
 

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 5 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

+static int pcie_port_runtime_suspend(struct device *dev)
+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     pci_save_state(pdev);

Are you sure this is sufficient?

Quoted 4 lines

drivers/pci/pcie/portdrv_pci.c +4 -0

+     return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 118

Quoted 6 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     pci_restore_state(pdev);
+     if (pdev->runtime_d3cold)
+             msleep(100);

What's _that_ supposed to do?

Quoted 15 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+
+     return 0;
+}
+
  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
       .suspend        = pcie_port_device_suspend,
       .resume         = pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
       .poweroff       = pcie_port_device_suspend,
       .restore        = pcie_port_device_resume,
       .resume_noirq   = pcie_port_resume_noirq,
+     .runtime_suspend = pcie_port_runtime_suspend,
+     .runtime_resume = pcie_port_runtime_resume,
  };
 

drivers/pci/pcie/portdrv_pci.c static int __devinit pcie_portdrv_probe(struct pci_dev *dev, · line 170

Quoted 6 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

  #define PCIE_PORTDRV_PM_OPS  (&pcie_portdrv_pm_ops)

@@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
               return status;
 
       pci_save_state(dev);
+     pm_runtime_put_noidle(&dev->dev);

What's the purpose of this?

Quoted 14 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

       return 0;
  }
 
  static void pcie_portdrv_remove(struct pci_dev *dev)
  {
+     pm_runtime_get_noresume(&dev->dev);
       pcie_port_device_remove(dev);
       pci_disable_device(dev);
  }

include/linux/pci.h +0 -0

index e444f5b..b41d9a1 100644
@@ -281,6 +281,7 @@ struct pci_dev {

Quoted 6 lines

      unsigned int    no_d1d2:1;      /* Only allow D0 and D3 */ unsigned int    mmio_always_on:1;       /* disallow turning off io/mem decoding during bar sizing */
+     unsigned int    runtime_d3cold:1;
      unsigned int    wakeup_prepared:1;
      unsigned int    d3_delay;       /* D3->D0 transition time in ms */

OK

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
              [PCI_D1] = ACPI_STATE_D1,
              [PCI_D2] = ACPI_STATE_D2,
              [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
      };
      int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold. But we still need ACPI to setup run wake for the device.

So we need a way to inform acpi_pci_run_wake(), one way is using a flag, there is other ways such as adding another parameter to acpi_pci_run_wake() to tell it which power state to go.

Quoted 3 lines

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

As above, that may happend after ACPI GPE.

Quoted 13 lines

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

              return 0;
 
      if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
      int error;
 

Guys, please. Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

This patch is sent as RFC, I think this is to call for discussing instead of merging, isn't it?

Quoted 2 lines

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

We need to program ACPI to make the PCI device go into D3_cold. That is called in this function.

pci_set_power_sate
-> __pci_complete_power_transition

    -> pci_platform_power_transition

Quoted 82 lines

Quoted 22 lines

drivers/pci/pci.c +4 -3

      /* bound the state we're entering */
-     if (state > PCI_D3hot)
-             state = PCI_D3hot;
+     if (state > PCI_D3cold)
+             state = PCI_D3cold;
      else if (state < PCI_D0)
              state = PCI_D0;
      else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
      if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
              return 0;
 
-     error = pci_raw_set_power_state(dev, state);
+     error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+                                     PCI_D3hot : state);
 
      if (!__pci_complete_power_transition(dev, state))
              error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
      return !!(dev->pme_support & (1 << state));
 }
 

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+     struct pci_dev *bridge = dev->bus->self;
+
+     /* don't poll the pme bit if parent is in low power state */
+     if (bridge && bridge->current_state != PCI_D0)
+             return;
+
+     pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

Quoted 46 lines

drivers/pci/pci.c +6 -1

+
 static void pci_pme_list_scan(struct work_struct *work)
 {
      struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
      if (!list_empty(&pci_pme_list)) {
              list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
                      if (pme_dev->dev->pme_poll) {
-                             pci_pme_wakeup(pme_dev->dev, NULL);
+                             pci_pme_poll_wakeup(pme_dev->dev);
                      } else {
                              list_del(&pme_dev->list);
                              kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
      if (enable) {
              int error;
 
+             if (runtime && state >= PCI_D3cold)
+                     dev->runtime_d3cold = true;
+             else
+                     dev->runtime_d3cold = false;
              if (pci_pme_capable(dev, state))
                      pci_pme_active(dev, true);
              else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/init.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
 #include "portdrv.h"
 #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
      return 0;
 }
 

Quoted 7 lines

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 5 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

+static int pcie_port_runtime_suspend(struct device *dev)
+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     pci_save_state(pdev);

Are you sure this is sufficient?

Maybe we need to pay attention to the pcie port drivers? Run ->runtime_

Quoted 4 lines

Quoted 4 lines

drivers/pci/pcie/portdrv_pci.c +4 -0

+     return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)

Quoted 8 lines

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 118

Quoted 6 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     pci_restore_state(pdev);
+     if (pdev->runtime_d3cold)
+             msleep(100);

What's _that_ supposed to do?

When resume from d3cold, PCIe main link will be powered on again, it will take quite some time before the main link go into L0 state. Otherwise, accessing devices under the port may return wrong result.

Quoted 19 lines

Quoted 19 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+
+     return 0;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
      .suspend        = pcie_port_device_suspend,
      .resume         = pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
      .poweroff       = pcie_port_device_suspend,
      .restore        = pcie_port_device_resume,
      .resume_noirq   = pcie_port_resume_noirq,
+     .runtime_suspend = pcie_port_runtime_suspend,
+     .runtime_resume = pcie_port_runtime_resume,
 };
 
 #define PCIE_PORTDRV_PM_OPS  (&pcie_portdrv_pm_ops)

@@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
              return status;
 

Quoted 4 lines

drivers/pci/pcie/portdrv_pci.c static int __devinit pcie_portdrv_probe(struct pci_dev *dev, · line 170

Quoted 2 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

      pci_save_state(dev);
+     pm_runtime_put_noidle(&dev->dev);

What's the purpose of this?

I think this is needed by PCI runtime PM support. Before ->probe, pm_runtime_get_xxx() is executed on device.

Quoted 23 lines

Quoted 14 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

      return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+     pm_runtime_get_noresume(&dev->dev);
      pcie_port_device_remove(dev);
      pci_disable_device(dev);
 }

include/linux/pci.h +0 -0

index e444f5b..b41d9a1 100644

include/linux/pci.h struct pci_dev { · line 286

Quoted 6 lines

include/linux/pci.h +1 -0

@@ -281,6 +281,7 @@ struct pci_dev {
      unsigned int    no_d1d2:1;      /* Only allow D0 and D3 */
      unsigned int    mmio_always_on:1;       /* disallow turning off io/mem
                                                 decoding during bar sizing */
+     unsigned int    runtime_d3cold:1;
      unsigned int    wakeup_prepared:1;
      unsigned int    d3_delay;       /* D3->D0 transition time in ms */

OK

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
               [PCI_D1] = ACPI_STATE_D1,
               [PCI_D2] = ACPI_STATE_D2,
               [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
       };
       int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead.  I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

  static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
  {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong.  First off, I don't think that the runtime_d3cold flag makes any sense.  We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory.  If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there.  dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold.  But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

Quoted 3 lines

So we need a way to inform acpi_pci_run_wake(), one way is using a flag, there is other ways such as adding another parameter to acpi_pci_run_wake() to tell it which power state to go.

I'm not sure what to do with that.  It looks like we should use ACPI for signaling remote wakeup on this platform and additionally provide a dummy interrupt handler for root ports that will only clear PME Status.

Quoted 5 lines

Quoted 3 lines

Second, pme_interrupt means that the _root_ _port_, not the device itself will trigger an interrupt whenever the device sends the PME message to it (which very well may happen for a device in D3_cold woken up by an external signal).

As above, that may happend after ACPI GPE.

Because it's not a standard PCIe system.

Quoted 20 lines

Quoted 13 lines

drivers/pci/pci.c int pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 732

Quoted 10 lines

drivers/pci/pci-acpi.c +0 -0

               return 0;
 
       if (!acpi_pm_device_run_wake(&dev->dev, enable))

drivers/pci/pci.c +0 -0

index 8156744..bc16869 100644
@@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
       int error;
 

Guys, please.  Never, _ever_, touch pci_set_power_state() without discussing your ideas with someone who knows how it works and _why_ it works this way.

This patch is sent as RFC, I think this is to call for discussing instead of merging, isn't it?

Quoted 2 lines

The problem here is that you can't program a PCI device into D3_cold, so it doesn't even make sense to have a helper for that.

Quoted 6 lines

We need to program ACPI to make the PCI device go into D3_cold.  That is called in this function.

pci_set_power_sate
  -> __pci_complete_power_transition
    -> pci_platform_power_transition

So don't use pci_set_power_state() for that, because it's essentially a different operation.  You need a pci_platform_remove_power() helper or similar for that.

What ACPI method exactly is used to remove power from the device?

Quoted 90 lines

Quoted 82 lines

Quoted 22 lines

drivers/pci/pci.c +4 -3

       /* bound the state we're entering */
-     if (state > PCI_D3hot)
-             state = PCI_D3hot;
+     if (state > PCI_D3cold)
+             state = PCI_D3cold;
       else if (state < PCI_D0)
               state = PCI_D0;
       else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))

@@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
       if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
               return 0;
 
-     error = pci_raw_set_power_state(dev, state);
+     error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+                                     PCI_D3hot : state);
 
       if (!__pci_complete_power_transition(dev, state))
               error = 0;

@@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
       return !!(dev->pme_support & (1 << state));
  }
 

drivers/pci/pci.c bool pci_pme_capable(struct pci_dev *dev, pci_power_t state) · line 1495

Quoted 10 lines

drivers/pci/pci.c +10 -0

+static void pci_pme_poll_wakeup(struct pci_dev *dev)
+{
+     struct pci_dev *bridge = dev->bus->self;
+
+     /* don't poll the pme bit if parent is in low power state */
+     if (bridge && bridge->current_state != PCI_D0)
+             return;
+
+     pci_pme_wakeup(dev, NULL);
+}

This one actually makes some sense, although it might be better to put the test into pci_pme_wakeup() itself.

Quoted 46 lines

drivers/pci/pci.c +6 -1

+
  static void pci_pme_list_scan(struct work_struct *work)
  {
       struct pci_pme_device *pme_dev, *n;

@@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
       if (!list_empty(&pci_pme_list)) {
               list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
                       if (pme_dev->dev->pme_poll) {
-                             pci_pme_wakeup(pme_dev->dev, NULL);
+                             pci_pme_poll_wakeup(pme_dev->dev);
                       } else {
                               list_del(&pme_dev->list);
                               kfree(pme_dev);

@@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
       if (enable) {
               int error;
 
+             if (runtime && state >= PCI_D3cold)
+                     dev->runtime_d3cold = true;
+             else
+                     dev->runtime_d3cold = false;
               if (pci_pme_capable(dev, state))
                       pci_pme_active(dev, true);

              else

drivers/pci/pcie/portdrv_pci.c +2 -0

index e0610bd..d66b7e9 100644
@@ -11,11 +11,13 @@
  #include <linux/kernel.h>
  #include <linux/errno.h>
  #include <linux/pm.h>
+#include <linux/pm_runtime.h>
  #include <linux/init.h>
  #include <linux/pcieport_if.h>
  #include <linux/aer.h>
  #include <linux/dmi.h>
  #include <linux/pci-aspm.h>
+#include <linux/delay.h>
 
  #include "portdrv.h"
  #include "aer/aerdrv.h"

@@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
       return 0;
  }
 

Quoted 7 lines

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 108

Quoted 5 lines

drivers/pci/pcie/portdrv_pci.c +5 -0

+static int pcie_port_runtime_suspend(struct device *dev)
+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     pci_save_state(pdev);

Are you sure this is sufficient?

Quoted 1 line

Maybe we need to pay attention to the pcie port drivers?  Run ->runtime_

Yes.

Quoted 13 lines

Quoted 12 lines

drivers/pci/pcie/portdrv_pci.c static int pcie_port_resume_noirq(struct device *dev) · line 118

Quoted 10 lines

drivers/pci/pcie/portdrv_pci.c +10 -0

+     return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+     struct pci_dev *pdev = to_pci_dev(dev);
+
+     pci_restore_state(pdev);
+     if (pdev->runtime_d3cold)
+             msleep(100);

What's _that_ supposed to do?

Quoted 3 lines

When resume from d3cold, PCIe main link will be powered on again, it will take quite some time before the main link go into L0 state. Otherwise, accessing devices under the port may return wrong result.

OK, but this is generic code and as per the standard the link should have been reestablished at this point already.

Please don't put some nonstandard-platform-specific quirks like this into code that's supposed to handle _every_ PCIe system.

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

drivers/pci/pcie/portdrv_pci.c +6 -0

+
+     return 0;
+}
+
  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
       .suspend        = pcie_port_device_suspend,
       .resume         = pcie_port_device_resume,

@@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
       .poweroff       = pcie_port_device_suspend,
       .restore        = pcie_port_device_resume,
       .resume_noirq   = pcie_port_resume_noirq,
+     .runtime_suspend = pcie_port_runtime_suspend,
+     .runtime_resume = pcie_port_runtime_resume,
  };
 
  #define PCIE_PORTDRV_PM_OPS  (&pcie_portdrv_pm_ops)

@@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
               return status;
 

Quoted 7 lines

Quoted 4 lines

drivers/pci/pcie/portdrv_pci.c static int __devinit pcie_portdrv_probe(struct pci_dev *dev, · line 170

Quoted 2 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

       pci_save_state(dev);
+     pm_runtime_put_noidle(&dev->dev);

What's the purpose of this?

I think this is needed by PCI runtime PM support.  Before ->probe, pm_runtime_get_xxx() is executed on device.

ok

Quoted 23 lines

Quoted 23 lines

Quoted 14 lines

drivers/pci/pcie/portdrv_pci.c +1 -0

       return 0;
  }
 
  static void pcie_portdrv_remove(struct pci_dev *dev)
  {
+     pm_runtime_get_noresume(&dev->dev);
       pcie_port_device_remove(dev);
       pci_disable_device(dev);
  }

include/linux/pci.h +0 -0

index e444f5b..b41d9a1 100644
@@ -281,6 +281,7 @@ struct pci_dev {

Quoted 6 lines

      unsigned int    no_d1d2:1;      /* Only allow D0 and D3 */ unsigned int    mmio_always_on:1;       /* disallow turning off io/mem decoding during bar sizing */
+     unsigned int    runtime_d3cold:1;
      unsigned int    wakeup_prepared:1;
      unsigned int    d3_delay;       /* D3->D0 transition time in ms */

OK

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

Quoted 70 lines · Rafael J. Wysocki

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
              [PCI_D1] = ACPI_STATE_D1,
              [PCI_D2] = ACPI_STATE_D2,
              [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
      };
      int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory. If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there. dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 5 lines

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold. But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

This is the standard behavior. Please refer to PCI Express Base Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup. In D1, D2 and D3hot state, PCIe device can transit the link from L1 to L0 state, and send the PME message. In D3cold, the main link is powered off, PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup firstly, then platform (power controller in spec) will power on the main link for the device, after main link is back to L0, the PME message is send to root port, pme interrupt is generated. So in theory, the wake up process can be divided into platform part (which power on the main link) and PCIe part (which send PME).

Quoted 4 lines

So don't use pci_set_power_state() for that, because it's essentially a different operation. You need a pci_platform_remove_power() helper or similar for that.

Quoted 1 line

What ACPI method exactly is used to remove power from the device?

The ACPI method executed is as follow:

- _PS3 (if exist)
- Power resources in _PR3 is turned on
- Power resources in _PR0 is turned off
- Power resources in _PR3 is turned off

I think the process can fit pci_set_power_state() quite well, so why invent another helper for that?

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

You mean the runtime_d3cold flag? That flag is used to tell acpi_pci_run_wake() that we need ACPI wakeup setup for the device because that is needed by D3cold. The ACPI wakeup setup here means turn on power resources needed by wake up (_PRW) and execute _DSW.

If you mean the whole patch, we want to implement runtime D3cold support, which can save more power than D3hot.

Quoted 77 lines · huang ying

Quoted 70 lines · Rafael J. Wysocki

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
               [PCI_D1] = ACPI_STATE_D1,
               [PCI_D2] = ACPI_STATE_D2,
               [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
       };
       int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead.  I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

  static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
  {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong.  First off, I don't think that the runtime_d3cold flag makes any sense.  We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory.  If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there.  dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 5 lines

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold.  But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

Quoted 10 lines

This is the standard behavior.  Please refer to PCI Express Base Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup.  In D1, D2 and D3hot state, PCIe device can transit the link from L1 to L0 state, and send the PME message.  In D3cold, the main link is powered off, PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup firstly, then platform (power controller in spec) will power on the main link for the device, after main link is back to L0, the PME message is send to root port, pme interrupt is generated.  So in theory, the wake up process can be divided into platform part (which power on the main link) and PCIe part (which send PME).

That's fine.  However, the platform part should be completely transparent to the PCI bus type, then.  It just should power up the link to allow a PME message to be transmitted through it.

[...]

Quoted 4 lines

Quoted 4 lines

So don't use pci_set_power_state() for that, because it's essentially a different operation.  You need a pci_platform_remove_power() helper or similar for that.

Quoted 8 lines

Quoted 1 line

What ACPI method exactly is used to remove power from the device?

The ACPI method executed is as follow:

- _PS3 (if exist)
- Power resources in _PR3 is turned on
- Power resources in _PR0 is turned off
- Power resources in _PR3 is turned off

I'd rather think

- make sure that _PR3 resources are referenced
- drop references (from this device) for all other power resources
- execute _PS3 (if any)
- drop references for _PR3 resources

if Section 7.2.11 of ACPI 5.0 is to be followed.

Quoted 2 lines

I think the process can fit pci_set_power_state() quite well, so why invent another helper for that?

OK, we can special case it, perhaps.

Suppose we have a "this device may be put into D3_cold" flag.

Who's going to decide whether to put it into D3_hot or D3_cold?

[...]

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

You mean the runtime_d3cold flag? That flag is used to tell acpi_pci_run_wake() that we need ACPI wakeup setup for the device because that is needed by D3cold.  The ACPI wakeup setup here means turn on power resources needed by wake up (_PRW) and execute _DSW.

Quoted 2 lines

If you mean the whole patch, we want to implement runtime D3cold support, which can save more power than D3hot.

So, do I think correctly that you'd like to put devices into D3_cold if that's possible via ACPI and to be able to wake it up from that state using remote wakeup?

Quoted 89 lines · Rafael J. Wysocki

Quoted 77 lines · huang ying

Quoted 70 lines · Rafael J. Wysocki

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
              [PCI_D1] = ACPI_STATE_D1,
              [PCI_D2] = ACPI_STATE_D2,
              [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
      };
      int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory. If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there. dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 5 lines

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold. But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

Quoted 10 lines

This is the standard behavior. Please refer to PCI Express Base Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup. In D1, D2 and D3hot state, PCIe device can transit the link from L1 to L0 state, and send the PME message. In D3cold, the main link is powered off, PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup firstly, then platform (power controller in spec) will power on the main link for the device, after main link is back to L0, the PME message is send to root port, pme interrupt is generated. So in theory, the wake up process can be divided into platform part (which power on the main link) and PCIe part (which send PME).

Quoted 3 lines

That's fine. However, the platform part should be completely transparent to the PCI bus type, then. It just should power up the link to allow a PME message to be transmitted through it.

Yes.

Quoted 15 lines

[...]

Quoted 4 lines

Quoted 4 lines

So don't use pci_set_power_state() for that, because it's essentially a different operation. You need a pci_platform_remove_power() helper or similar for that.

Quoted 8 lines

Quoted 1 line

What ACPI method exactly is used to remove power from the device?

The ACPI method executed is as follow:

- _PS3 (if exist)
- Power resources in _PR3 is turned on
- Power resources in _PR0 is turned off
- Power resources in _PR3 is turned off

Quoted 8 lines

I'd rather think

- make sure that _PR3 resources are referenced
- drop references (from this device) for all other power resources
- execute _PS3 (if any)
- drop references for _PR3 resources

if Section 7.2.11 of ACPI 5.0 is to be followed.

Yes. You are right.

Quoted 3 lines

Quoted 2 lines

I think the process can fit pci_set_power_state() quite well, so why invent another helper for that?

Quoted 5 lines

OK, we can special case it, perhaps.

Suppose we have a "this device may be put into D3_cold" flag.

Who's going to decide whether to put it into D3_hot or D3_cold?

In most cases, I think it is OK to put device into D3_cold if that is supported.

But there should be some special case where D3_cold is not desirable, for example, we can put SSD into D3_cold safely, but it is not quite safe to put HDD into D3_cold. So we want to introduce a
flag: "may_power_off" like in the following patch

https://lkml.org/lkml/2012/3/29/41

It gives device driver a chance to prevent the device to be put into D3_cold.

Quoted 12 lines

[...]

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

You mean the runtime_d3cold flag? That flag is used to tell acpi_pci_run_wake() that we need ACPI wakeup setup for the device because that is needed by D3cold. The ACPI wakeup setup here means turn on power resources needed by wake up (_PRW) and execute _DSW.

Quoted 6 lines

Quoted 2 lines

If you mean the whole patch, we want to implement runtime D3cold support, which can save more power than D3hot.

So, do I think correctly that you'd like to put devices into D3_cold if that's possible via ACPI and to be able to wake it up from that state using remote wakeup?

Yes. Support both remote wakeup and host wakeup.

Quoted 127 lines · huang ying

Quoted 89 lines · Rafael J. Wysocki

Quoted 77 lines · huang ying

Quoted 70 lines · Rafael J. Wysocki

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
               [PCI_D1] = ACPI_STATE_D1,
               [PCI_D2] = ACPI_STATE_D2,
               [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
       };
       int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead.  I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

  static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
  {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong.  First off, I don't think that the runtime_d3cold flag makes any sense.  We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory.  If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there.  dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 5 lines

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold.  But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

Quoted 10 lines

This is the standard behavior.  Please refer to PCI Express Base Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup.  In D1, D2 and D3hot state, PCIe device can transit the link from L1 to L0 state, and send the PME message.  In D3cold, the main link is powered off, PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup firstly, then platform (power controller in spec) will power on the main link for the device, after main link is back to L0, the PME message is send to root port, pme interrupt is generated.  So in theory, the wake up process can be divided into platform part (which power on the main link) and PCIe part (which send PME).

Quoted 3 lines

That's fine.  However, the platform part should be completely transparent to the PCI bus type, then.  It just should power up the link to allow a PME message to be transmitted through it.

Yes.

Quoted 15 lines

[...]

Quoted 4 lines

Quoted 4 lines

So don't use pci_set_power_state() for that, because it's essentially a different operation.  You need a pci_platform_remove_power() helper or similar for that.

Quoted 8 lines

Quoted 1 line

What ACPI method exactly is used to remove power from the device?

The ACPI method executed is as follow:

- _PS3 (if exist)
- Power resources in _PR3 is turned on
- Power resources in _PR0 is turned off
- Power resources in _PR3 is turned off

Quoted 8 lines

I'd rather think

- make sure that _PR3 resources are referenced
- drop references (from this device) for all other power resources
- execute _PS3 (if any)
- drop references for _PR3 resources

if Section 7.2.11 of ACPI 5.0 is to be followed.

Yes.  You are right.

Quoted 5 lines

Quoted 2 lines

I think the process can fit pci_set_power_state() quite well, so why invent another helper for that?

OK, we can special case it, perhaps.

Quoted 6 lines

Quoted 3 lines

Suppose we have a "this device may be put into D3_cold" flag.

Who's going to decide whether to put it into D3_hot or D3_cold?

In most cases, I think it is OK to put device into D3_cold if that is supported.

Well, there may be PM QoS latency requirements preventing us from doing so.

Quoted 5 lines

But there should be some special case where D3_cold is not desirable, for example, we can put SSD into D3_cold safely, but it is not quite safe to put HDD into D3_cold.  So we want to introduce a
flag: "may_power_off" like in the following patch

Quoted 3 lines

https://lkml.org/lkml/2012/3/29/41

It gives device driver a chance to prevent the device to be put into D3_cold.

I see.  So your proposal is that the flag might be used to indicate to whoever carries out power transitions of devices that power must not be removed from this particular device, right?

In that case we can put that flag into struct dev_pm_info after all, but perhaps the name should indicate more precisely what it is about.  Something like "power_must_be_on" maybe?

Quoted 12 lines

Quoted 12 lines

[...]

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

You mean the runtime_d3cold flag? That flag is used to tell acpi_pci_run_wake() that we need ACPI wakeup setup for the device because that is needed by D3cold.  The ACPI wakeup setup here means turn on power resources needed by wake up (_PRW) and execute _DSW.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

If you mean the whole patch, we want to implement runtime D3cold support, which can save more power than D3hot.

So, do I think correctly that you'd like to put devices into D3_cold if that's possible via ACPI and to be able to wake it up from that state using remote wakeup?

Yes.  Support both remote wakeup and host wakeup.

OK, so we need to clean up the ACPI D3_HOT/D3_COLD mess first.  Then, we need to change PCI so that devices are not put into D3cold (by ACPI) if only D3hot is requested by the caller of pci_set_power_state().

Having done that, we can modify pci_set_power_state() to handle D3cold as a special case (essentially, it should check that case before doing anything else).  Finally, we need to teach the ACPI notify handler about the WAKE# event and we need to add the 100 ms wait to the device resume code path somewhere (I guess in pci_set_power_state() for the D3cold->D0 transition).

Now, there's one more thing to consider.  Namely, if a PCIe endpoint is put into D3hot (via native PM) and then the port it is connected to is put into D3_hot (via native PM), does that transfer the endpoint into D3cold?

Quoted 128 lines · Rafael J. Wysocki

Quoted 127 lines · huang ying

Quoted 89 lines · Rafael J. Wysocki

Quoted 77 lines · huang ying

Quoted 70 lines · Rafael J. Wysocki

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
              [PCI_D1] = ACPI_STATE_D1,
              [PCI_D2] = ACPI_STATE_D2,
              [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
      };
      int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead. I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

 static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
 {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong. First off, I don't think that the runtime_d3cold flag makes any sense. We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory. If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there. dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 5 lines

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold. But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

Quoted 10 lines

This is the standard behavior. Please refer to PCI Express Base Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup. In D1, D2 and D3hot state, PCIe device can transit the link from L1 to L0 state, and send the PME message. In D3cold, the main link is powered off, PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup firstly, then platform (power controller in spec) will power on the main link for the device, after main link is back to L0, the PME message is send to root port, pme interrupt is generated. So in theory, the wake up process can be divided into platform part (which power on the main link) and PCIe part (which send PME).

Quoted 3 lines

That's fine. However, the platform part should be completely transparent to the PCI bus type, then. It just should power up the link to allow a PME message to be transmitted through it.

Yes.

Quoted 15 lines

[...]

Quoted 4 lines

Quoted 4 lines

So don't use pci_set_power_state() for that, because it's essentially a different operation. You need a pci_platform_remove_power() helper or similar for that.

Quoted 8 lines

Quoted 1 line

What ACPI method exactly is used to remove power from the device?

The ACPI method executed is as follow:

- _PS3 (if exist)
- Power resources in _PR3 is turned on
- Power resources in _PR0 is turned off
- Power resources in _PR3 is turned off

Quoted 8 lines

I'd rather think

- make sure that _PR3 resources are referenced
- drop references (from this device) for all other power resources
- execute _PS3 (if any)
- drop references for _PR3 resources

if Section 7.2.11 of ACPI 5.0 is to be followed.

Yes. You are right.

Quoted 5 lines

Quoted 2 lines

I think the process can fit pci_set_power_state() quite well, so why invent another helper for that?

OK, we can special case it, perhaps.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Suppose we have a "this device may be put into D3_cold" flag.

Who's going to decide whether to put it into D3_hot or D3_cold?

In most cases, I think it is OK to put device into D3_cold if that is supported.

Well, there may be PM QoS latency requirements preventing us from doing so.

Yes.

Quoted 5 lines

Quoted 5 lines

But there should be some special case where D3_cold is not desirable, for example, we can put SSD into D3_cold safely, but it is not quite safe to put HDD into D3_cold. So we want to introduce a
flag: "may_power_off" like in the following patch

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/3/29/41

It gives device driver a chance to prevent the device to be put into D3_cold.

I see. So your proposal is that the flag might be used to indicate to whoever carries out power transitions of devices that power must not be removed from this particular device, right?

Yes.

Quoted 3 lines

In that case we can put that flag into struct dev_pm_info after all, but perhaps the name should indicate more precisely what it is about. Something like "power_must_be_on" maybe?

I am not good at naming in English :)
I will accept your proposal.

Quoted 25 lines

Quoted 12 lines

Quoted 12 lines

[...]

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

You mean the runtime_d3cold flag? That flag is used to tell acpi_pci_run_wake() that we need ACPI wakeup setup for the device because that is needed by D3cold. The ACPI wakeup setup here means turn on power resources needed by wake up (_PRW) and execute _DSW.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

If you mean the whole patch, we want to implement runtime D3cold support, which can save more power than D3hot.

So, do I think correctly that you'd like to put devices into D3_cold if that's possible via ACPI and to be able to wake it up from that state using remote wakeup?

Yes. Support both remote wakeup and host wakeup.

OK, so we need to clean up the ACPI D3_HOT/D3_COLD mess first. Then, we need to change PCI so that devices are not put into D3cold (by ACPI) if only D3hot is requested by the caller of pci_set_power_state().

Quoted 5 lines

Having done that, we can modify pci_set_power_state() to handle D3cold as a special case (essentially, it should check that case before doing anything else). Finally, we need to teach the ACPI notify handler about the WAKE# event and we need to add the 100 ms wait to the device resume code path somewhere (I guess in pci_set_power_state() for the D3cold->D0 transition).

Yes. Sound good to me.

Quoted 3 lines

Now, there's one more thing to consider. Namely, if a PCIe endpoint is put into D3hot (via native PM) and then the port it is connected to is put into D3_hot (via native PM), does that transfer the endpoint into D3cold?

No.

But if a PCIe endpoint is put into D3hot and then the port it is connected is put into D3_cold (via ACPI), this will transfer the endpoint into D3_cold, and if the port is put into D0 afterwards, all subordinate endpoint devices will be put into D0 (because of power on reset). I think what we need to do here is:

- when choose power state, if any subordinate device has power_must_be_on set, will not choose D3_cold

- when put PCIe port from D3_cold to D0, resume all subordinate devices too.

We design a method to do that in following patch:

https://lkml.org/lkml/2012/3/29/38

Where we will register all subordinate devices via acpi_power_resource_register_device(endpoint_device, bridget_acpi_handle).

Quoted 201 lines · huang ying

Quoted 128 lines · Rafael J. Wysocki

Quoted 127 lines · huang ying

Quoted 89 lines · Rafael J. Wysocki

Quoted 77 lines · huang ying

Quoted 70 lines · Rafael J. Wysocki

Quoted 52 lines · huang ying

Hi,

Quoted 36 lines · Rafael J. Wysocki

Hi,

Quoted 16 lines · Zheng

Hi all,

This patch adds PCIe runtime D3cold support, namely cut power supply for functions beneath a PCIe port when they all have entered D3. A device in D3cold can only generate wake event through the WAKE# pin. Because we can not access to a device's configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.

Any comment will be appreciated.

Signed-off-by Zheng Yan <[email protected]>

---

drivers/pci/pci-acpi.c +0 -0

index 0f150f2..e210e8cb 100644

drivers/pci/pci-acpi.c static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) · line 230

Quoted 8 lines

drivers/pci/pci-acpi.c +1 -1

@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
               [PCI_D1] = ACPI_STATE_D1,
               [PCI_D2] = ACPI_STATE_D2,
               [PCI_D3hot] = ACPI_STATE_D3,
-             [PCI_D3cold] = ACPI_STATE_D3
+             [PCI_D3cold] = ACPI_STATE_D3_COLD
       };
       int error = -EINVAL;
 

Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.

We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT instead.  I'll prepare a patch for that over the weekend if no one has done that already.

Quoted 2 lines

drivers/pci/pci-acpi.c +0 -0

@@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
 

Quoted 8 lines

drivers/pci/pci-acpi.c static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable) · line 300

Quoted 5 lines

drivers/pci/pci-acpi.c +2 -1

  static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
  {
-     if (dev->pme_interrupt)
+     /* PME interrupt isn't available in the D3cold case */
+     if (dev->pme_interrupt && !dev->runtime_d3cold)

This whole thing is wrong.  First off, I don't think that the runtime_d3cold flag makes any sense.  We already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support (that is, dev->pme_interrupt = true), but the process of remote wakeup from D3cold is as follow:

Quoted 8 lines

1) In D3cold, the power of the main link is turned off, aux power is provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the power of the main link is turned on, after a while, link goes into L0 state
4) The PME message is sent to root port, pme interrupt generated

This isn't how it's supposed to work in theory.  If the device can signal PME from D3cold, it should be able to reestablish the link and send a PME message from there.  dev->pme_interrupt set means exactly that.

ACPI is only supposed to be needed for things that don't send PME messages (in your case the PME interrupt generated by the port is essentially useless, because the wakeup event has already been signaled through ACPI).

Quoted 5 lines

Quoted 3 lines

So, for deivce, dev->pme_interrupt = true and dev->pme_support advocate it support PME in D3cold.  But we still need ACPI to setup run wake for the device.

OK, so this is nonstandard.

Quoted 10 lines

This is the standard behavior.  Please refer to PCI Express Base Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup.  In D1, D2 and D3hot state, PCIe device can transit the link from L1 to L0 state, and send the PME message.  In D3cold, the main link is powered off, PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup firstly, then platform (power controller in spec) will power on the main link for the device, after main link is back to L0, the PME message is send to root port, pme interrupt is generated.  So in theory, the wake up process can be divided into platform part (which power on the main link) and PCIe part (which send PME).

Quoted 3 lines

That's fine.  However, the platform part should be completely transparent to the PCI bus type, then.  It just should power up the link to allow a PME message to be transmitted through it.

Yes.

Quoted 15 lines

[...]

Quoted 4 lines

Quoted 4 lines

So don't use pci_set_power_state() for that, because it's essentially a different operation.  You need a pci_platform_remove_power() helper or similar for that.

Quoted 8 lines

Quoted 1 line

What ACPI method exactly is used to remove power from the device?

The ACPI method executed is as follow:

- _PS3 (if exist)
- Power resources in _PR3 is turned on
- Power resources in _PR0 is turned off
- Power resources in _PR3 is turned off

Quoted 8 lines

I'd rather think

- make sure that _PR3 resources are referenced
- drop references (from this device) for all other power resources
- execute _PS3 (if any)
- drop references for _PR3 resources

if Section 7.2.11 of ACPI 5.0 is to be followed.

Yes.  You are right.

Quoted 5 lines

Quoted 2 lines

I think the process can fit pci_set_power_state() quite well, so why invent another helper for that?

OK, we can special case it, perhaps.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Suppose we have a "this device may be put into D3_cold" flag.

Who's going to decide whether to put it into D3_hot or D3_cold?

In most cases, I think it is OK to put device into D3_cold if that is supported.

Well, there may be PM QoS latency requirements preventing us from doing so.

Yes.

Quoted 5 lines

Quoted 5 lines

But there should be some special case where D3_cold is not desirable, for example, we can put SSD into D3_cold safely, but it is not quite safe to put HDD into D3_cold.  So we want to introduce a
flag: "may_power_off" like in the following patch

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/3/29/41

It gives device driver a chance to prevent the device to be put into D3_cold.

I see.  So your proposal is that the flag might be used to indicate to whoever carries out power transitions of devices that power must not be removed from this particular device, right?

Yes.

Quoted 3 lines

In that case we can put that flag into struct dev_pm_info after all, but perhaps the name should indicate more precisely what it is about.  Something like "power_must_be_on" maybe?

I am not good at naming in English :)
I will accept your proposal.

Quoted 25 lines

Quoted 12 lines

Quoted 12 lines

[...]

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

So now please tell me what exactly you want to achieve and why you want to do that in the first place.

Well, is there any chance to get that information?

You mean the runtime_d3cold flag? That flag is used to tell acpi_pci_run_wake() that we need ACPI wakeup setup for the device because that is needed by D3cold.  The ACPI wakeup setup here means turn on power resources needed by wake up (_PRW) and execute _DSW.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

If you mean the whole patch, we want to implement runtime D3cold support, which can save more power than D3hot.

So, do I think correctly that you'd like to put devices into D3_cold if that's possible via ACPI and to be able to wake it up from that state using remote wakeup?

Yes.  Support both remote wakeup and host wakeup.

OK, so we need to clean up the ACPI D3_HOT/D3_COLD mess first.  Then, we need to change PCI so that devices are not put into D3cold (by ACPI) if only D3hot is requested by the caller of pci_set_power_state().

Quoted 5 lines

Having done that, we can modify pci_set_power_state() to handle D3cold as a special case (essentially, it should check that case before doing anything else).  Finally, we need to teach the ACPI notify handler about the WAKE# event and we need to add the 100 ms wait to the device resume code path somewhere (I guess in pci_set_power_state() for the D3cold->D0 transition).

Yes.  Sound good to me.

Quoted 3 lines

Now, there's one more thing to consider.  Namely, if a PCIe endpoint is put into D3hot (via native PM) and then the port it is connected to is put into D3_hot (via native PM), does that transfer the endpoint into D3cold?

No.

Quoted 8 lines

But if a PCIe endpoint is put into D3hot and then the port it is connected is put into D3_cold (via ACPI), this will transfer the endpoint into D3_cold, and if the port is put into D0 afterwards, all subordinate endpoint devices will be put into D0 (because of power on reset).  I think what we need to do here is:

- when choose power state, if any subordinate device has power_must_be_on set, will not choose D3_cold