PCIe: Add PCIe runtime D3cold support

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 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?

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 17 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 */

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

Thanks,
Rafael

Lin Ming

2012-04-16 00:48 UTC #5 lore raw

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.

Thanks,
Lin Ming

Yan, Zheng

author

2012-04-16 02:23 UTC #7 lore raw

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 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;
 }

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 13 lines

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?

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

Quoted 53 lines

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 */

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

Thanks,
Rafael

Yan, Zheng

author

2012-04-16 07:49 UTC #19 lore raw

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.

Regards
Yan, Zheng

huang ying

2012-04-16 08:15 UTC #3 lore raw

In reply to Alex He

Quoted 1 line · Alex He

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

[snip]

Quoted 9 lines

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?

Yes.

Quoted 3 lines

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.

Yes. We put PCI device into D3hot via PMCSR firstly, then we use ACPI _PS3 and _PR3 to put device into D3cold.

Best Regards,
Huang Ying

huang ying

2012-04-16 08:58 UTC #21 lore raw

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 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,
  };

Quoted 8 lines

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?

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
@@ -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

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

Best Regards,
Huang Ying

Rafael J. Wysocki

2012-04-16 16:22 UTC #6 lore raw

In reply to Lin Ming

Quoted 35 lines · Lin Ming

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.

Quoted 5 lines

Hi Rafael,
Have you started to write the patch?
If not, I can do it.

Please do if you have the time. I'd like this one to be cleaned up ASAP, to reduce the amount of confusion taking place.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-16 17:02 UTC #8 lore raw

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 54 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?

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 13 lines

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?

Quoted 2 lines

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

This is a non-standard thing again, so please don't slap it on the code supposed to handle _standard_ PCI Express.

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.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-16 21:07 UTC #20 lore raw

Quoted 12 lines · Zheng

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 8 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).
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.

What exactly do you mean by "setup ACPI wakeup"?

Rafael

Rafael J. Wysocki

2012-04-16 21:25 UTC #22 lore raw

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 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?

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

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.

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 */

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?

Rafael

huang ying

2012-04-17 02:02 UTC #23 lore raw

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 43 lines

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

Quoted 5 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.
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 107 lines

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 6 lines

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.

Maybe we can have some other way to check whether the link is reestablished. I will check the this.

Quoted 55 lines

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 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.

Best Regards,
Huang Ying

huang ying

2012-04-17 02:07 UTC #9 lore raw

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?

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

Yan, Zheng

author

2012-04-17 02:12 UTC #13 lore raw

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 11 lines

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.

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 3 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 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 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 5 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.

Regards
Yan, Zheng

huang ying

2012-04-17 05:13 UTC #30 lore raw

Quoted 17 lines · Rafael J. Wysocki

Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
+ return 0;
drivers/pci/pcie/portdrv_pci.c +3 -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?
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.

Quoted 5 lines

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.

After checking PCIe spec, I found that the 100ms here has its standard origin :)

In PCI Express Base Specification Revision 2.0:

Section 6.6.1 Conventional Reset

"
To allow components to perform internal initialization, system software must wait for at least
100 ms from the end of a Conventional Reset of one or more devices before it is permitted to
issue Configuration Requests to those devices
"

But I think we should move the 100ms delay here to PCIe bus code or PCIe/ACPI code, because that is needed by all PCIe devices for D3cold support.

Best Regards,
Huang Ying

huang ying

2012-04-17 05:32 UTC #14 lore raw

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.

Best Regards,
Huang Ying

Rafael J. Wysocki

2012-04-17 20:16 UTC #10 lore raw

Quoted 7 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.

Quoted 5 lines

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?

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.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-17 20:30 UTC #18 lore raw

Quoted 130 lines · Zheng

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 8 lines

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.

So there's a bug somewhere, because the port (bridge) is the parent of the device being resumed, so it should be resumed automatically by runtime PM before that device. Do you have any idea why that doesn't happen?

Quoted 5 lines

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.

And #WAKE triggers a GPE in turn?

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 8 lines

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

It would be good to verify that theory, so that we have a clear picture.

Quoted 5 lines

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.

For now. That may change in the future, though.

Quoted 1 line

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

You're right.

Quoted 2 lines

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

Well, I don't think that PME interrupts are necessary at all on this platform, because it uses ACPI for wakeup signaling anyway.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-17 20:39 UTC #15 lore raw

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.

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?

Rafael

Rafael J. Wysocki

2012-04-17 20:58 UTC #24 lore raw

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 5 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?

Quoted 7 lines

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.

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?

Rafael

Rafael J. Wysocki

2012-04-17 21:06 UTC #31 lore raw

Quoted 38 lines · huang ying

Quoted 17 lines · Rafael J. Wysocki
Quoted 13 lines
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?
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.
Quoted 5 lines
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.
After checking PCIe spec, I found that the 100ms here has its standard origin :)
In PCI Express Base Specification Revision 2.0:
Section 6.6.1 Conventional Reset
"
To allow components to perform internal initialization, system software must wait for at least
100 ms from the end of a Conventional Reset of one or more devices before it is permitted to
issue Configuration Requests to those devices
"

Quoted 3 lines

But I think we should move the 100ms delay here to PCIe bus code or PCIe/ACPI code, because that is needed by all PCIe devices for D3cold support.

I think it should be sufficient to wait for the PME message to arrive at the root port (which will cause the PME interrupt to appear), at which point the device that sent it should be able to receive configuration requests.

At this point, I need to konw what exactly happens when the GPE is triggered by WAKE#.

Thanks,
Rafael

huang ying

2012-04-18 01:19 UTC #11 lore raw

Quoted 10 lines · Rafael J. Wysocki

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 7 lines

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.

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"?

Best Regards,
Huang Ying

huang ying

2012-04-18 01:22 UTC #16 lore raw

Quoted 21 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.

Quoted 7 lines

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?

Yes.

Best Regards,
Huang Ying

huang ying

2012-04-18 01:45 UTC #25 lore raw

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

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 8 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.
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

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.

Best Regards,
Huang Ying

huang ying

2012-04-18 02:01 UTC #32 lore raw

Quoted 39 lines · Rafael J. Wysocki

Quoted 38 lines · huang ying
Quoted 17 lines · Rafael J. Wysocki
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
+ return 0;
drivers/pci/pcie/portdrv_pci.c +3 -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?
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.
Quoted 5 lines
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.
After checking PCIe spec, I found that the 100ms here has its standard origin :)
In PCI Express Base Specification Revision 2.0:
Section 6.6.1 Conventional Reset
"
To allow components to perform internal initialization, system software must wait for at least
100 ms from the end of a Conventional Reset of one or more devices before it is permitted to
issue Configuration Requests to those devices
"

Quoted 8 lines

Quoted 3 lines
But I think we should move the 100ms delay here to PCIe bus code or PCIe/ACPI code, because that is needed by all PCIe devices for D3cold support.
I think it should be sufficient to wait for the PME message to arrive at the root port (which will cause the PME interrupt to appear), at which point the device that sent it should be able to receive configuration requests.

For remote wake up, it is sufficient. But for host wake up, we still need to wait 100ms.

Quoted 2 lines

At this point, I need to konw what exactly happens when the GPE is triggered by WAKE#.

- Lxx handler will be executed
- in Lxx handler, Notify the ACPI handle PCIe port
- Linux has registered a handler for the ACPI handle of PCIe port, in the handler, turn on _PR0 and execute _PS0, which will power on the link.

Best Regards,
Huang Ying

Rafael J. Wysocki

2012-04-18 19:46 UTC #12 lore raw

Quoted 15 lines · huang ying

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 7 lines

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?

No, it isn't. User space shouldn't be able to enable functionality that's going to break on a subset of systems.

Quoted 1 line

Or you suggest something like "white list"?

Yes, something like this.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-18 19:48 UTC #17 lore raw

Quoted 24 lines · huang ying

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 7 lines

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.

So I wonder what that _Lxx method looks like.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-18 20:47 UTC #33 lore raw

Quoted 44 lines · huang ying

Quoted 43 lines · Rafael J. Wysocki
Quoted 41 lines · huang ying
Quoted 17 lines · Rafael J. Wysocki
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4 -0
+     return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +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 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.
Quoted 5 lines
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.
After checking PCIe spec, I found that the 100ms here has its standard origin :)
In PCI Express Base Specification Revision 2.0:
Section 6.6.1 Conventional Reset
"
To allow components to perform internal initialization, system software must wait for at least
100 ms from the end of a Conventional Reset of one or more devices before it is permitted to
issue Configuration Requests to those devices
"
But I think we should move the 100ms delay here to PCIe bus code or PCIe/ACPI code, because that is needed by all PCIe devices for D3cold support.

Quoted 7 lines

Quoted 4 lines
I think it should be sufficient to wait for the PME message to arrive at the root port (which will cause the PME interrupt to appear), at which point the device that sent it should be able to receive configuration requests.
For remote wake up, it is sufficient. But for host wake up, we still need to wait 100ms.

Yes, we do.

Quoted 8 lines

Quoted 2 lines
At this point, I need to konw what exactly happens when the GPE is triggered by WAKE#.
- Lxx handler will be executed
- in Lxx handler, Notify the ACPI handle PCIe port
- Linux has registered a handler for the ACPI handle of PCIe port, in the handler, turn on _PR0 and execute _PS0, which will power on the link.

But the handler we have is not the handler we want here.

In fact, there are two handlers, pci_acpi_wake_bus() and pci_acpi_wake_dev() and they only do useful things for ACPI_NOTIFY_DEVICE_WAKE. Is that the event type we receive from that _Lxx?

Even if so, these routines don't seem to be suitable to handle the case at hand.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-18 20:56 UTC #26 lore raw

Quoted 125 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 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 8 lines

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.

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

Quoted 8 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
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?

Rafael

huang ying

2012-04-19 02:08 UTC #34 lore raw

Quoted 64 lines · Rafael J. Wysocki

Quoted 44 lines · huang ying
Quoted 43 lines · Rafael J. Wysocki
Quoted 41 lines · huang ying
Quoted 17 lines · Rafael J. Wysocki
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
+ return 0;
(quoted diff) +3 -0
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +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 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.
Quoted 5 lines
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.
After checking PCIe spec, I found that the 100ms here has its standard origin :)
In PCI Express Base Specification Revision 2.0:
Section 6.6.1 Conventional Reset
"
To allow components to perform internal initialization, system software must wait for at least
100 ms from the end of a Conventional Reset of one or more devices before it is permitted to
issue Configuration Requests to those devices
"
But I think we should move the 100ms delay here to PCIe bus code or PCIe/ACPI code, because that is needed by all PCIe devices for D3cold support.
Quoted 7 lines
Quoted 4 lines
I think it should be sufficient to wait for the PME message to arrive at the root port (which will cause the PME interrupt to appear), at which point the device that sent it should be able to receive configuration requests.
For remote wake up, it is sufficient. But for host wake up, we still need to wait 100ms.
Yes, we do.
Quoted 8 lines
Quoted 2 lines
At this point, I need to konw what exactly happens when the GPE is triggered by WAKE#.
- Lxx handler will be executed
- in Lxx handler, Notify the ACPI handle PCIe port
- Linux has registered a handler for the ACPI handle of PCIe port, in the handler, turn on _PR0 and execute _PS0, which will power on the link.

Quoted 5 lines

But the handler we have is not the handler we want here.
In fact, there are two handlers, pci_acpi_wake_bus() and pci_acpi_wake_dev() and they only do useful things for ACPI_NOTIFY_DEVICE_WAKE. Is that the event type we receive from that _Lxx?

I check the DSDT, in _Lxx, there is

Notify (\_SB.PCI0.RP03, 0x02)

That is, the event type is ACPI_NOTIFY_DEVICE_WAKE.

Quoted 1 line

Even if so, these routines don't seem to be suitable to handle the case at hand.

Yes. Maybe add a flag named like "come_from_d3cold", and if come_from_d3cold == true, resume the dev itself without checking pme bits, because the PCIe main link is not available now.

Best Regards,
Huang Ying

huang ying

2012-04-19 02:47 UTC #27 lore raw

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

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:

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

Best Regards,
Huang Ying

Rafael J. Wysocki

2012-04-19 12:26 UTC #28 lore raw

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

Well, that's quite obvious. :-)

Quoted 2 lines

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

Alternatively, we can just call pci_restore_state() on them and put them into PCI_D3hot again without actually resuming.

Quoted 7 lines

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).

I think that's overkill in the case of PCI devices under a PCIe port. We only need to walk the bus below the port and do something for each device in there then.

Thanks,
Rafael

Rafael J. Wysocki

2012-04-19 12:32 UTC #35 lore raw

Quoted 75 lines · huang ying

Quoted 64 lines · Rafael J. Wysocki
Quoted 44 lines · huang ying
Quoted 43 lines · Rafael J. Wysocki
Quoted 41 lines · huang ying
Quoted 17 lines · Rafael J. Wysocki
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4 -0
+     return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +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 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.
Quoted 5 lines
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.
After checking PCIe spec, I found that the 100ms here has its standard origin :)
In PCI Express Base Specification Revision 2.0:
Section 6.6.1 Conventional Reset
"
To allow components to perform internal initialization, system software must wait for at least
100 ms from the end of a Conventional Reset of one or more devices before it is permitted to
issue Configuration Requests to those devices
"
But I think we should move the 100ms delay here to PCIe bus code or PCIe/ACPI code, because that is needed by all PCIe devices for D3cold support.
Quoted 7 lines
Quoted 4 lines
I think it should be sufficient to wait for the PME message to arrive at the root port (which will cause the PME interrupt to appear), at which point the device that sent it should be able to receive configuration requests.
For remote wake up, it is sufficient. But for host wake up, we still need to wait 100ms.
Yes, we do.
Quoted 8 lines
Quoted 2 lines
At this point, I need to konw what exactly happens when the GPE is triggered by WAKE#.
- Lxx handler will be executed
- in Lxx handler, Notify the ACPI handle PCIe port
- Linux has registered a handler for the ACPI handle of PCIe port, in the handler, turn on _PR0 and execute _PS0, which will power on the link.
Quoted 5 lines
But the handler we have is not the handler we want here.
In fact, there are two handlers, pci_acpi_wake_bus() and pci_acpi_wake_dev() and they only do useful things for ACPI_NOTIFY_DEVICE_WAKE. Is that the event type we receive from that _Lxx?
I check the DSDT, in _Lxx, there is
Notify (\_SB.PCI0.RP03, 0x02)

Quoted 7 lines

That is, the event type is ACPI_NOTIFY_DEVICE_WAKE.
Quoted 1 line
Even if so, these routines don't seem to be suitable to handle the case at hand.
Yes. Maybe add a flag named like "come_from_d3cold", and if come_from_d3cold == true, resume the dev itself without checking pme bits, because the PCIe main link is not available now.

Actually, I think we can do the full resume (however with the 100 ms wait) in that case, because we're going to resume the device shortly anyway.

The PME would only be useful as a kind of handshake with the device, so we know we can access its registers, but as you pointed that out, we need the 100 ms delay in the host wakeup case anyway, so perhaps it's better to make remote wakeup and host wakeup behave identically in that respect.

Thanks,
Rafael

huang ying

2012-04-20 00:48 UTC #29 lore raw

Quoted 211 lines · Rafael J. Wysocki

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

Quoted 7 lines

Well, that's quite obvious. :-)
Quoted 2 lines
- when put PCIe port from D3_cold to D0, resume all subordinate devices too.
Alternatively, we can just call pci_restore_state() on them and put them into PCI_D3hot again without actually resuming.

That is better. But as the first step, I prefer the simpler way to just resume the device. In this way, the synchronization between remote resume, host resume and system suspend can be easier.

Quoted 11 lines

Quoted 7 lines
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).
I think that's overkill in the case of PCI devices under a PCIe port. We only need to walk the bus below the port and do something for each device in there then.

Yes. That is something like power domain, if the power resources are shared by multiple devices.

huang ying

2012-04-20 00:53 UTC #36 lore raw