Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1309887 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports" Qipeng Zha <qipeng.zha@intel.com> - 2016-01-15 07:30 +0100
    [PATCH 2/2] PCI/PM: enable runtime PM support for Intel Broxton platform. Qipeng Zha <qipeng.zha@intel.com> - 2016-01-15 07:30 +0100
      Re: [PATCH 2/2] PCI/PM: enable runtime PM support for Intel Broxton  platform. Bjorn Helgaas <helgaas@kernel.org> - 2016-01-19 23:00 +0100
    Re: [PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM  support code for PCIe ports" Bjorn Helgaas <helgaas@kernel.org> - 2016-01-19 23:00 +0100
      Re: [PATCH 1/2] PCI/PM: Revert "PCI/PM: Drop unused runtime PM support code for PCIe ports" "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-19 23:10 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[+cc Rafael]

On Fri, Jan 15, 2016 at 10:27:45PM +0800, Qipeng Zha wrote:
> The PCIe runtime PM is disabled by default, add special treatment
> to allow runtime PM for Intel Broxton platform.

Hmmm.  I assume this only applies to Broxton now, but will likely
apply to future Intel platforms as well.  I really don't want to be
adding quirks for every new Intel platform that comes along.

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

Missing spaces at beginning and end of comment.

> +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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

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

[+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
> 

[toc] | [prev] | [next] | [standalone]

#1312550

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web