[PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports"

[PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports"

[Qipeng Zha]

This reverts commit fe9a743a2601 ("PCI/PM: Drop unused runtime
PM support code for PCIe ports"). To support PCIe ports
runtime PM for Intel Broxton platform.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pci/pcie/portdrv_pci.c | 68 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index be35da2..1e4a562 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -93,6 +93,71 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+struct d3cold_info {
+	bool no_d3cold;
+	unsigned int d3cold_delay;
+};
+
+static int pci_dev_d3cold_info(struct pci_dev *pdev, void *data)
+{
+	struct d3cold_info *info = data;
+
+	info->d3cold_delay = max_t(unsigned int, pdev->d3cold_delay,
+				   info->d3cold_delay);
+	if (pdev->no_d3cold)
+		info->no_d3cold = true;
+	return 0;
+}
+
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct d3cold_info d3cold_info = {
+		.no_d3cold	= false,
+		.d3cold_delay	= PCI_PM_D3_WAIT,
+	};
+
+	/*
+	 * If any subordinate device disable D3cold, we should not put
+	 * the port into D3cold.  The D3cold delay of port should be
+	 * the max of that of all subordinate devices.
+	 */
+	pci_walk_bus(pdev->subordinate, pci_dev_d3cold_info, &d3cold_info);
+	pdev->no_d3cold = d3cold_info.no_d3cold;
+	pdev->d3cold_delay = d3cold_info.d3cold_delay;
+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int pci_dev_pme_poll(struct pci_dev *pdev, void *data)
+{
+	bool *pme_poll = data;
+
+	if (pdev->pme_poll)
+		*pme_poll = true;
+	return 0;
+}
+
+static int pcie_port_runtime_idle(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool pme_poll = false;
+
+	/*
+	 * If any subordinate device needs pme poll, we should keep
+	 * the port in D0, because we need port in D0 to poll it.
+	 */
+	pci_walk_bus(pdev->subordinate, pci_dev_pme_poll, &pme_poll);
+	/* Delay for a short while to prevent too frequent suspend/resume */
+	if (!pme_poll)
+		pm_schedule_suspend(dev, 10);
+	return -EBUSY;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,
@@ -101,6 +166,9 @@ 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,
+	.runtime_idle	= pcie_port_runtime_idle,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
-- 
1.8.3.2

Re: [PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports"

[Rafael J. Wysocki]

On Thursday, February 18, 2016 05:43:30 PM Qipeng Zha wrote:
> This reverts commit fe9a743a2601 ("PCI/PM: Drop unused runtime
> PM support code for PCIe ports"). To support PCIe ports
> runtime PM for Intel Broxton platform.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>

No, this isn't the right approach.

I've already said that for several times and I haven't changed my mind.

Mika is currently working on implementing this in a better way, please talk
to him.

Thanks,
Rafael

[PATCH 2/2] PCI/PM: enable runtime PM support for Intel Broxton platform.

[Qipeng Zha]

The PCIe runtime PM is disabled by default, add special treatment
to allow runtime PM for Intel Broxton platform.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pci/quirks.c    | 13 +++++++++++++
 include/linux/pci_ids.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7e32730..a745d06 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -25,6 +25,7 @@
 #include <linux/sched.h>
 #include <linux/ktime.h>
 #include <linux/mm.h>
+#include <linux/pm_runtime.h>
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
 
@@ -2989,6 +2990,18 @@ static void quirk_intel_ntb(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0e08, quirk_intel_ntb);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0e0d, quirk_intel_ntb);
 
+
+/*PCIe ports on Intel Broxton should support runtime PM*/
+static void quirk_pcie_enable_rtpm(struct pci_dev *dev)
+{
+	pm_runtime_put_noidle(&dev->dev);
+	pm_runtime_allow(&dev->dev);
+}
+DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_INTEL,
+	PCI_DEVICE_ID_INTEL_BXT_B0_PCIe_0, quirk_pcie_enable_rtpm);
+DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_INTEL,
+	PCI_DEVICE_ID_INTEL_BXT_B0_PCIe_1, quirk_pcie_enable_rtpm);
+
 static ktime_t fixup_debug_start(struct pci_dev *dev,
 				 void (*fn)(struct pci_dev *dev))
 {
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index d9ba49c..731f05f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2596,6 +2596,8 @@
 #define PCI_DEVICE_ID_INTEL_82840_HB	0x1a21
 #define PCI_DEVICE_ID_INTEL_82845_HB	0x1a30
 #define PCI_DEVICE_ID_INTEL_IOAT	0x1a38
+#define PCI_DEVICE_ID_INTEL_BXT_B0_PCIe_0		0x1ad6
+#define PCI_DEVICE_ID_INTEL_BXT_B0_PCIe_1		0x1ad7
 #define PCI_DEVICE_ID_INTEL_COUGARPOINT_LPC_MIN	0x1c41
 #define PCI_DEVICE_ID_INTEL_COUGARPOINT_LPC_MAX	0x1c5f
 #define PCI_DEVICE_ID_INTEL_PATSBURG_LPC_0	0x1d40
-- 
1.8.3.2

[PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports"

[Qipeng Zha]

This reverts commit fe9a743a2601 ("PCI/PM: Drop unused runtime
PM support code for PCIe ports"). To support PCIe ports
runtime PM for Intel Broxton platform.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pci/pcie/portdrv_pci.c | 74 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index be35da2..2ccc9b9 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -93,6 +93,77 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+struct d3cold_info {
+	bool no_d3cold;
+	unsigned int d3cold_delay;
+};
+
+static int pci_dev_d3cold_info(struct pci_dev *pdev, void *data)
+{
+	struct d3cold_info *info = data;
+
+	info->d3cold_delay = max_t(unsigned int, pdev->d3cold_delay,
+				   info->d3cold_delay);
+	if (pdev->no_d3cold)
+		info->no_d3cold = true;
+	return 0;
+}
+
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct d3cold_info d3cold_info = {
+		.no_d3cold	= false,
+		.d3cold_delay	= PCI_PM_D3_WAIT,
+	};
+
+	/*
+	 * If any subordinate device disable D3cold, we should not put
+	 * the port into D3cold.  The D3cold delay of port should be
+	 * the max of that of all subordinate devices.
+	 */
+	pci_walk_bus(pdev->subordinate, pci_dev_d3cold_info, &d3cold_info);
+	pdev->no_d3cold = d3cold_info.no_d3cold;
+	pdev->d3cold_delay = d3cold_info.d3cold_delay;
+	return 0;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int pci_dev_pme_poll(struct pci_dev *pdev, void *data)
+{
+	bool *pme_poll = data;
+
+	if (pdev->pme_poll)
+		*pme_poll = true;
+	return 0;
+}
+
+static int pcie_port_runtime_idle(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool pme_poll = false;
+
+	/*
+	 * If any subordinate device needs pme poll, we should keep
+	 * the port in D0, because we need port in D0 to poll it.
+	 */
+	pci_walk_bus(pdev->subordinate, pci_dev_pme_poll, &pme_poll);
+	/* Delay for a short while to prevent too frequent suspend/resume */
+	if (!pme_poll)
+		pm_schedule_suspend(dev, 10);
+	return -EBUSY;
+}
+#else
+#define pcie_port_runtime_suspend	NULL
+#define pcie_port_runtime_resume	NULL
+#define pcie_port_runtime_idle		NULL
+#endif
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,
@@ -101,6 +172,9 @@ 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,
+	.runtime_idle	= pcie_port_runtime_idle,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
-- 
1.8.3.2

Re: [PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports"

[Bjorn Helgaas]

[+cc Rafael, author of fe9a743a2601]

On Fri, Jan 15, 2016 at 10:27:44PM +0800, Qipeng Zha wrote:
> This reverts commit fe9a743a2601 ("PCI/PM: Drop unused runtime
> PM support code for PCIe ports"). To support PCIe ports
> runtime PM for Intel Broxton platform.

This changelog is not as useful as it could be.  It's true that this
reverts fe9a743a2601, but what's important is that this adds support
code for runtime PM of PCIe ports, so that should be mentioned first.

The fe9a743a2601 changelog says "If we are to support runtime PM of
PCIe ports, it will have to be done in a different way most likely
anyway."  I don't know what Rafael had in mind there, but we certainly
need to ask him before adding it back the same way it was.

> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 74 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index be35da2..2ccc9b9 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -93,6 +93,77 @@ static int pcie_port_resume_noirq(struct device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_RUNTIME
> +struct d3cold_info {
> +	bool no_d3cold;
> +	unsigned int d3cold_delay;
> +};
> +
> +static int pci_dev_d3cold_info(struct pci_dev *pdev, void *data)
> +{
> +	struct d3cold_info *info = data;
> +
> +	info->d3cold_delay = max_t(unsigned int, pdev->d3cold_delay,
> +				   info->d3cold_delay);
> +	if (pdev->no_d3cold)
> +		info->no_d3cold = true;
> +	return 0;
> +}
> +
> +static int pcie_port_runtime_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct d3cold_info d3cold_info = {
> +		.no_d3cold	= false,
> +		.d3cold_delay	= PCI_PM_D3_WAIT,
> +	};
> +
> +	/*
> +	 * If any subordinate device disable D3cold, we should not put
> +	 * the port into D3cold.  The D3cold delay of port should be
> +	 * the max of that of all subordinate devices.
> +	 */
> +	pci_walk_bus(pdev->subordinate, pci_dev_d3cold_info, &d3cold_info);
> +	pdev->no_d3cold = d3cold_info.no_d3cold;
> +	pdev->d3cold_delay = d3cold_info.d3cold_delay;
> +	return 0;
> +}
> +
> +static int pcie_port_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int pci_dev_pme_poll(struct pci_dev *pdev, void *data)
> +{
> +	bool *pme_poll = data;
> +
> +	if (pdev->pme_poll)
> +		*pme_poll = true;
> +	return 0;
> +}
> +
> +static int pcie_port_runtime_idle(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	bool pme_poll = false;
> +
> +	/*
> +	 * If any subordinate device needs pme poll, we should keep
> +	 * the port in D0, because we need port in D0 to poll it.
> +	 */
> +	pci_walk_bus(pdev->subordinate, pci_dev_pme_poll, &pme_poll);
> +	/* Delay for a short while to prevent too frequent suspend/resume */
> +	if (!pme_poll)
> +		pm_schedule_suspend(dev, 10);
> +	return -EBUSY;
> +}
> +#else
> +#define pcie_port_runtime_suspend	NULL
> +#define pcie_port_runtime_resume	NULL
> +#define pcie_port_runtime_idle		NULL
> +#endif
> +
>  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.suspend	= pcie_port_device_suspend,
>  	.resume		= pcie_port_device_resume,
> @@ -101,6 +172,9 @@ 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,
> +	.runtime_idle	= pcie_port_runtime_idle,
>  };
>  
>  #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
> -- 
> 1.8.3.2
> 

Re: [PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports"

[Rafael J. Wysocki]

On Tuesday, January 19, 2016 03:52:34 PM Bjorn Helgaas wrote:
> [+cc Rafael, author of fe9a743a2601]
> 
> On Fri, Jan 15, 2016 at 10:27:44PM +0800, Qipeng Zha wrote:
> > This reverts commit fe9a743a2601 ("PCI/PM: Drop unused runtime
> > PM support code for PCIe ports"). To support PCIe ports
> > runtime PM for Intel Broxton platform.
> 
> This changelog is not as useful as it could be.  It's true that this
> reverts fe9a743a2601, but what's important is that this adds support
> code for runtime PM of PCIe ports, so that should be mentioned first.
> 
> The fe9a743a2601 changelog says "If we are to support runtime PM of
> PCIe ports, it will have to be done in a different way most likely
> anyway."  I don't know what Rafael had in mind there, but we certainly
> need to ask him before adding it back the same way it was.

Thanks!

The previous approach had a few problems that showed up in testing and
that's why the code was first disabled and then removed from the kernel.

We can't re-enable it or those problems will show up again.

I have not looked at adding PCIe port runtime PM support in a cleaner
way yet, but it is on my radar.

Thanks,
Rafael