[PATCH 0/4] PCI/PM: D3cold support for system suspend

[PATCH 0/4] PCI/PM: D3cold support for system suspend

[Huang Ying]

[PATCH 1/4] PCI/ACPI: Add target state as parameter to pci_platform_pm_ops->run_wake
[PATCH 2/4] PCI: Rename pci_dev->runtime_d3cold to pci_dev->set_d3cold
[PATCH 3/4] PCI/PM: Set pci_dev->set_d3cold in pci_set_power_state
[PATCH 4/4] PCI/PM: Enable D3cold support for system suspend

Huang Ying
Best Regards,

[PATCH 1/4] PCI/ACPI: Add target state as parameter to pci_platform_pm_ops->run_wake

[Huang Ying]

Normally, if PCI device uses native PME interrupt for runtime wake up,
platform need not to do anything for runtime wake up.

But per PCI Express Base Specification Revision 2.0 section 5.3.3.2
Link Wakeup, platform support is needed for D3cold waking up to power
on the main link even if there is native PME interrupt support for
D3cold.

So the needed work for platform runtime wake up is different among
different target state.  Originally, pci_dev->runtime_d3cold flag is
used for that, but we want to restrict its usage.  Now the target
state is added as parameter to pci_platform_pm_ops->run_wake to solve
the issue.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/pci/pci-acpi.c |    5 +++--
 drivers/pci/pci.c      |    9 +++++----
 drivers/pci/pci.h      |    2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -261,7 +261,8 @@ static void acpi_pci_propagate_run_wake(
 		acpi_pm_device_run_wake(bus->bridge, enable);
 }
 
-static int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
+static int acpi_pci_run_wake(struct pci_dev *dev, bool enable,
+			     pci_power_t state)
 {
 	/*
 	 * Per PCI Express Base Specification Revision 2.0 section
@@ -269,7 +270,7 @@ static int acpi_pci_run_wake(struct pci_
 	 * waking up to power on the main link even if there is PME
 	 * support for D3cold
 	 */
-	if (dev->pme_interrupt && !dev->runtime_d3cold)
+	if (dev->pme_interrupt && state != PCI_D3cold)
 		return 0;
 
 	if (!acpi_pm_device_run_wake(&dev->dev, enable))
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -484,10 +484,11 @@ static inline int platform_pci_sleep_wak
 			pci_platform_pm->sleep_wake(dev, enable) : -ENODEV;
 }
 
-static inline int platform_pci_run_wake(struct pci_dev *dev, bool enable)
+static inline int platform_pci_run_wake(struct pci_dev *dev, bool enable,
+					pci_power_t state)
 {
 	return pci_platform_pm ?
-			pci_platform_pm->run_wake(dev, enable) : -ENODEV;
+			pci_platform_pm->run_wake(dev, enable, state) : -ENODEV;
 }
 
 /**
@@ -1687,7 +1688,7 @@ int __pci_enable_wake(struct pci_dev *de
 			pci_pme_active(dev, true);
 		else
 			ret = 1;
-		error = runtime ? platform_pci_run_wake(dev, true) :
+		error = runtime ? platform_pci_run_wake(dev, true, state) :
 					platform_pci_sleep_wake(dev, true);
 		if (ret)
 			ret = error;
@@ -1695,7 +1696,7 @@ int __pci_enable_wake(struct pci_dev *de
 			dev->wakeup_prepared = true;
 	} else {
 		if (runtime)
-			platform_pci_run_wake(dev, false);
+			platform_pci_run_wake(dev, false, state);
 		else
 			platform_pci_sleep_wake(dev, false);
 		pci_pme_active(dev, false);
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -61,7 +61,7 @@ struct pci_platform_pm_ops {
 	pci_power_t (*choose_state)(struct pci_dev *dev);
 	bool (*can_wakeup)(struct pci_dev *dev);
 	int (*sleep_wake)(struct pci_dev *dev, bool enable);
-	int (*run_wake)(struct pci_dev *dev, bool enable);
+	int (*run_wake)(struct pci_dev *dev, bool enable, pci_power_t state);
 };
 
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);

[PATCH 2/4] PCI: Rename pci_dev->runtime_d3cold to pci_dev->set_d3cold

[Huang Ying]

Will use this flag for system suspend in addition to runtime suspend.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/pci/pci-driver.c |    2 +-
 drivers/pci/pci.c        |    6 +++---
 include/linux/pci.h      |    7 +++----
 3 files changed, 7 insertions(+), 8 deletions(-)

--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1036,7 +1036,7 @@ static int pci_pm_runtime_resume(struct
 
 	rc = pm->runtime_resume(dev);
 
-	pci_dev->runtime_d3cold = false;
+	pci_dev->set_d3cold = false;
 
 	return rc;
 }
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -681,7 +681,7 @@ static void __pci_start_power_transition
 		 * devices powered on/off by corresponding bridge,
 		 * because have already delayed for the bridge.
 		 */
-		if (dev->runtime_d3cold) {
+		if (dev->set_d3cold) {
 			msleep(dev->d3cold_delay);
 			/*
 			 * When powering on a bridge from D3cold, the
@@ -1833,7 +1833,7 @@ int pci_finish_runtime_suspend(struct pc
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	dev->runtime_d3cold = target_state == PCI_D3cold;
+	dev->set_d3cold = target_state == PCI_D3cold;
 
 	__pci_enable_wake(dev, target_state, true, pci_dev_run_wake(dev));
 
@@ -1841,7 +1841,7 @@ int pci_finish_runtime_suspend(struct pc
 
 	if (error) {
 		__pci_enable_wake(dev, target_state, true, false);
-		dev->runtime_d3cold = false;
+		dev->set_d3cold = false;
 	}
 
 	return error;
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -263,10 +263,9 @@ struct pci_dev {
 	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
 						   decoding during bar sizing */
 	unsigned int	wakeup_prepared:1;
-	unsigned int	runtime_d3cold:1;	/* whether go through runtime
-						   D3cold, not set for devices
-						   powered on/off by the
-						   corresponding bridge */
+	unsigned int	set_d3cold:1;	/* whether go through runtime D3cold,
+					   not set for devices powered on/off
+					   by the corresponding bridge */
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
 

[PATCH 3/4] PCI/PM: Set pci_dev->set_d3cold in pci_set_power_state

[Huang Ying]

Because now pci_dev->set_d3cold is only used after
pci_set_power_state(, PCI_D3cold) and before pci_set_power_state(,
PCI_D0).  And we will use pci_dev->set_d3cold for D3cold support
during system suspend too, but now pci_dev->set_d3cold is set only in
runtime power management code path now.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/pci/pci-driver.c |    2 --
 drivers/pci/pci.c        |   12 +++++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1036,8 +1036,6 @@ static int pci_pm_runtime_resume(struct
 
 	rc = pm->runtime_resume(dev);
 
-	pci_dev->set_d3cold = false;
-
 	return rc;
 }
 
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -775,6 +775,9 @@ int pci_set_power_state(struct pci_dev *
 	if (dev->current_state == state)
 		return 0;
 
+	if (state == PCI_D3cold)
+		dev->set_d3cold = true;
+
 	__pci_start_power_transition(dev, state);
 
 	/* This device is quirked not to be put into D3, so
@@ -798,6 +801,9 @@ int pci_set_power_state(struct pci_dev *
 	if (!error && dev->bus->self)
 		pcie_aspm_powersave_config_link(dev->bus->self);
 
+	if (error || state != PCI_D3cold)
+		dev->set_d3cold = false;
+
 	return error;
 }
 
@@ -1833,16 +1839,12 @@ int pci_finish_runtime_suspend(struct pc
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	dev->set_d3cold = target_state == PCI_D3cold;
-
 	__pci_enable_wake(dev, target_state, true, pci_dev_run_wake(dev));
 
 	error = pci_set_power_state(dev, target_state);
 
-	if (error) {
+	if (error)
 		__pci_enable_wake(dev, target_state, true, false);
-		dev->set_d3cold = false;
-	}
 
 	return error;
 }

[PATCH 4/4] PCI/PM: Enable D3cold support for system suspend

[Huang Ying]

Device may need to be put in D3cold on some platforms, especially
because we treat ACPI_STATE_D3 as ACPI_STATE_D3_COLD now.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/pci/pci-driver.c |    5 +++++
 drivers/pci/pci.c        |    4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -579,6 +579,7 @@ static bool pci_has_legacy_pm_support(st
 static int pci_pm_prepare(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	int error = 0;
 
 	/*
@@ -592,6 +593,10 @@ static int pci_pm_prepare(struct device
 	 */
 	pm_runtime_resume(dev);
 
+	if (!pci_dev->d3cold_allowed)
+		pci_dev->no_d3cold = true;
+	else
+		pci_dev->no_d3cold = false;
 	if (drv && drv->pm && drv->pm->prepare)
 		error = drv->pm->prepare(dev);
 
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1798,10 +1798,6 @@ int pci_prepare_to_sleep(struct pci_dev
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	/* D3cold during system suspend/hibernate is not supported */
-	if (target_state > PCI_D3hot)
-		target_state = PCI_D3hot;
-
 	pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
 
 	error = pci_set_power_state(dev, target_state);

Re: [PATCH 0/4] PCI/PM: D3cold support for system suspend

[Bjorn Helgaas]

On Mon, Jan 28, 2013 at 9:34 PM, Huang Ying <ying.huang@intel.com> wrote:
> [PATCH 1/4] PCI/ACPI: Add target state as parameter to pci_platform_pm_ops->run_wake
> [PATCH 2/4] PCI: Rename pci_dev->runtime_d3cold to pci_dev->set_d3cold
> [PATCH 3/4] PCI/PM: Set pci_dev->set_d3cold in pci_set_power_state
> [PATCH 4/4] PCI/PM: Enable D3cold support for system suspend

Do we still need these?

What problem do they solve?  Or what new functionality do they add?

Re: [PATCH 0/4] PCI/PM: D3cold support for system suspend

[Rafael J. Wysocki]

On Thursday, April 04, 2013 12:26:52 PM Bjorn Helgaas wrote:
> On Mon, Jan 28, 2013 at 9:34 PM, Huang Ying <ying.huang@intel.com> wrote:
> > [PATCH 1/4] PCI/ACPI: Add target state as parameter to pci_platform_pm_ops->run_wake
> > [PATCH 2/4] PCI: Rename pci_dev->runtime_d3cold to pci_dev->set_d3cold
> > [PATCH 3/4] PCI/PM: Set pci_dev->set_d3cold in pci_set_power_state
> > [PATCH 4/4] PCI/PM: Enable D3cold support for system suspend
> 
> Do we still need these?
> 
> What problem do they solve?  Or what new functionality do they add?

They were supposed to allow PCI devices to be put into D3cold (via ACPI)
during system suspend, but I think it's better not to do that at this
point.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.