[PATCH v1 00/13] PCI: Device configuration cleanup

[PATCH v1 00/13] PCI: Device configuration cleanup

[Bjorn Helgaas]

The motivation for this is to fix Linus' complaint about too many _HPP
messages (https://bugzilla.kernel.org/show_bug.cgi?id=84391):

    pci 0000:00:00.0: no hotplug settings from platform
    pci 0000:00:00.0: using default PCI settings

The first patch drops the messages and I intend it for v3.17.  The rest
are candidates for v3.18.

But the excessive *number* of messages is an indication that we're
calling pci_configure_slot() way too many times to begin with.  The
reason this happens is because:

    - acpiphp supports many slots on a single bus
    - on a Bus Check, we call acpiphp_check_bridge() to check each slot
      for changes
    - acpiphp_check_bridge() calls enable_slot() for every valid device it
      finds on a bus
    - enable_slot() calls pci_configure_slot() for every device on the bus

The typical result is that we find a bunch of devices on bus 00, and
each time we find one, we configure every device on bus 00.

I think the best way to fix this is to move device configuration out of
the hotplug drivers and into the PCI core.  That way we can do it once
when the device is enumerated and added.

This series adds a pci_configure_device() function that does the
configuration (similar to the existing pci_configure_slot()), and calls
it from pci_device_add(), where we already call pci_init_capabilities()
to initialize MSI, power management, ARI, etc.

Then it drops the existing pci_configure_slot() calls (this requires
keeping the pcie_bus_configure_settings() MPS and MRRS configuration
that was done by pci_configure_slot() but is not done by
pci_configure_device(), since it's not a simple per-device thing).

Finally, it tweaks several _HPP/_HPX things where we do things not
required by the spec and I haven't seen any other good reason for them:

    - We don't apply _HPP to PCIe devices, and I think we should
    - We clear SERR and PERR bits, and the spec says we shouldn't
    - We don't apply _HPP to certain bridge types (PCI-ISA, etc), and
      I don't see a reason to avoid them
    - We fiddle MPS and MRRS settings based on _HPX, even though Linux
      should control these
    - We only configure hot-added devices, but I think we should
      configure things present at boot the same way

I know some of these might be controversial, so I split them all into
separate patches and we can debate and drop them if necessary.

---

Bjorn Helgaas (13):
      PCI: Remove "no hotplug settings from platform" warning
      PCI: Move pci_configure_slot() to drivers/pci/probe.c
      PCI: Add pci_configure_device() during enumeration
      PCI: pciehp: Configure hot-added display devices
      PCI: pciehp: Remove pci_configure_slot() usage
      PCI: shpchp: Remove pci_configure_slot() usage
      ACPI / hotplug / PCI: Remove pci_configure_slot() usage
      PCI: Remove unused pci_configure_slot()
      PCI: Apply _HPP settings to PCIe devices as well as PCI and PCI-X
      PCI: Preserve BIOS PCI_COMMAND_SERR and PCI_COMMAND_PARITY settings
      PCI: Apply _HPP settings to all hot-added PCI devices
      PCI: Preserve MPS and MRRS when applying _HPX settings
      PCI: Configure *all* devices, not just hot-added ones


 drivers/pci/hotplug/Makefile       |    2 
 drivers/pci/hotplug/acpiphp_glue.c |   11 --
 drivers/pci/hotplug/pciehp_pci.c   |    9 --
 drivers/pci/hotplug/pcihp_slot.c   |  180 ------------------------------------
 drivers/pci/hotplug/shpchp_pci.c   |    8 --
 drivers/pci/probe.c                |  134 +++++++++++++++++++++++++++
 include/linux/pci_hotplug.h        |    2 
 7 files changed, 138 insertions(+), 208 deletions(-)
 delete mode 100644 drivers/pci/hotplug/pcihp_slot.c

[PATCH v1 01/13] PCI: Remove "no hotplug settings from platform" warning

[Bjorn Helgaas]

We print way too many messages like this:

    pci 0000:00:00.0: no hotplug settings from platform
    pci 0000:00:00.0: using default PCI settings

This usually happens when the platform doesn't supply an ACPI _HPP method,
but the method is optional, so there's no point in warning about it.

Not only are the messages useless, but we call pci_configure_slot() far too
many times, so they're repeated many times.  I'll fix the overuse of
pci_configure_slot() too, but that will wait until the next merge window.

For now, just remove both log messages.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=84391
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pcihp_slot.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
index e246a10a0d2c..3e36ec8d708a 100644
--- a/drivers/pci/hotplug/pcihp_slot.c
+++ b/drivers/pci/hotplug/pcihp_slot.c
@@ -46,7 +46,6 @@ static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
 		 */
 		if (pci_is_pcie(dev))
 			return;
-		dev_info(&dev->dev, "using default PCI settings\n");
 		hpp = &pci_default_type0;
 	}
 
@@ -153,7 +152,6 @@ void pci_configure_slot(struct pci_dev *dev)
 {
 	struct pci_dev *cdev;
 	struct hotplug_params hpp;
-	int ret;
 
 	if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
 			(dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
@@ -163,9 +161,7 @@ void pci_configure_slot(struct pci_dev *dev)
 	pcie_bus_configure_settings(dev->bus);
 
 	memset(&hpp, 0, sizeof(hpp));
-	ret = pci_get_hp_params(dev, &hpp);
-	if (ret)
-		dev_warn(&dev->dev, "no hotplug settings from platform\n");
+	pci_get_hp_params(dev, &hpp);
 
 	program_hpp_type2(dev, hpp.t2);
 	program_hpp_type1(dev, hpp.t1);

[PATCH v1 02/13] PCI: Move pci_configure_slot() to drivers/pci/probe.c

[Bjorn Helgaas]

Move pci_configure_slot() and related functions from
drivers/pci/hotplug/pcihp_slot to drivers/pci/probe.c.

This is to prepare for doing device configuration during the normal
enumeration process instead of just after hot-add.

No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/Makefile     |    2 
 drivers/pci/hotplug/pcihp_slot.c |  176 --------------------------------------
 drivers/pci/probe.c              |  148 ++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+), 177 deletions(-)
 delete mode 100644 drivers/pci/hotplug/pcihp_slot.c

diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
index 3e6532b945c1..4a9aa08b08f1 100644
--- a/drivers/pci/hotplug/Makefile
+++ b/drivers/pci/hotplug/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
 
 obj-$(CONFIG_HOTPLUG_PCI_ACPI_IBM)	+= acpiphp_ibm.o
 
-pci_hotplug-objs	:=	pci_hotplug_core.o pcihp_slot.o
+pci_hotplug-objs	:=	pci_hotplug_core.o
 
 ifdef CONFIG_HOTPLUG_PCI_CPCI
 pci_hotplug-objs	+=	cpci_hotplug_core.o	\
diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
deleted file mode 100644
index 3e36ec8d708a..000000000000
--- a/drivers/pci/hotplug/pcihp_slot.c
+++ /dev/null
@@ -1,176 +0,0 @@
-/*
- * Copyright (C) 1995,2001 Compaq Computer Corporation
- * Copyright (C) 2001 Greg Kroah-Hartman (greg@kroah.com)
- * Copyright (C) 2001 IBM Corp.
- * Copyright (C) 2003-2004 Intel Corporation
- * (c) Copyright 2009 Hewlett-Packard Development Company, L.P.
- *
- * All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or (at
- * your option) any later version.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT.  See the GNU General Public License for more
- * details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- */
-
-#include <linux/pci.h>
-#include <linux/export.h>
-#include <linux/pci_hotplug.h>
-
-static struct hpp_type0 pci_default_type0 = {
-	.revision = 1,
-	.cache_line_size = 8,
-	.latency_timer = 0x40,
-	.enable_serr = 0,
-	.enable_perr = 0,
-};
-
-static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
-{
-	u16 pci_cmd, pci_bctl;
-
-	if (!hpp) {
-		/*
-		 * Perhaps we *should* use default settings for PCIe, but
-		 * pciehp didn't, so we won't either.
-		 */
-		if (pci_is_pcie(dev))
-			return;
-		hpp = &pci_default_type0;
-	}
-
-	if (hpp->revision > 1) {
-		dev_warn(&dev->dev,
-			 "PCI settings rev %d not supported; using defaults\n",
-			 hpp->revision);
-		hpp = &pci_default_type0;
-	}
-
-	pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, hpp->cache_line_size);
-	pci_write_config_byte(dev, PCI_LATENCY_TIMER, hpp->latency_timer);
-	pci_read_config_word(dev, PCI_COMMAND, &pci_cmd);
-	if (hpp->enable_serr)
-		pci_cmd |= PCI_COMMAND_SERR;
-	else
-		pci_cmd &= ~PCI_COMMAND_SERR;
-	if (hpp->enable_perr)
-		pci_cmd |= PCI_COMMAND_PARITY;
-	else
-		pci_cmd &= ~PCI_COMMAND_PARITY;
-	pci_write_config_word(dev, PCI_COMMAND, pci_cmd);
-
-	/* Program bridge control value */
-	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
-		pci_write_config_byte(dev, PCI_SEC_LATENCY_TIMER,
-				      hpp->latency_timer);
-		pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &pci_bctl);
-		if (hpp->enable_serr)
-			pci_bctl |= PCI_BRIDGE_CTL_SERR;
-		else
-			pci_bctl &= ~PCI_BRIDGE_CTL_SERR;
-		if (hpp->enable_perr)
-			pci_bctl |= PCI_BRIDGE_CTL_PARITY;
-		else
-			pci_bctl &= ~PCI_BRIDGE_CTL_PARITY;
-		pci_write_config_word(dev, PCI_BRIDGE_CONTROL, pci_bctl);
-	}
-}
-
-static void program_hpp_type1(struct pci_dev *dev, struct hpp_type1 *hpp)
-{
-	if (hpp)
-		dev_warn(&dev->dev, "PCI-X settings not supported\n");
-}
-
-static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
-{
-	int pos;
-	u32 reg32;
-
-	if (!hpp)
-		return;
-
-	if (hpp->revision > 1) {
-		dev_warn(&dev->dev, "PCIe settings rev %d not supported\n",
-			 hpp->revision);
-		return;
-	}
-
-	/* Initialize Device Control Register */
-	pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
-			~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);
-
-	/* Initialize Link Control Register */
-	if (dev->subordinate)
-		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
-			~hpp->pci_exp_lnkctl_and, hpp->pci_exp_lnkctl_or);
-
-	/* Find Advanced Error Reporting Enhanced Capability */
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
-	if (!pos)
-		return;
-
-	/* Initialize Uncorrectable Error Mask Register */
-	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, &reg32);
-	reg32 = (reg32 & hpp->unc_err_mask_and) | hpp->unc_err_mask_or;
-	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, reg32);
-
-	/* Initialize Uncorrectable Error Severity Register */
-	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &reg32);
-	reg32 = (reg32 & hpp->unc_err_sever_and) | hpp->unc_err_sever_or;
-	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, reg32);
-
-	/* Initialize Correctable Error Mask Register */
-	pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, &reg32);
-	reg32 = (reg32 & hpp->cor_err_mask_and) | hpp->cor_err_mask_or;
-	pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, reg32);
-
-	/* Initialize Advanced Error Capabilities and Control Register */
-	pci_read_config_dword(dev, pos + PCI_ERR_CAP, &reg32);
-	reg32 = (reg32 & hpp->adv_err_cap_and) | hpp->adv_err_cap_or;
-	pci_write_config_dword(dev, pos + PCI_ERR_CAP, reg32);
-
-	/*
-	 * FIXME: The following two registers are not supported yet.
-	 *
-	 *   o Secondary Uncorrectable Error Severity Register
-	 *   o Secondary Uncorrectable Error Mask Register
-	 */
-}
-
-void pci_configure_slot(struct pci_dev *dev)
-{
-	struct pci_dev *cdev;
-	struct hotplug_params hpp;
-
-	if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
-			(dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
-			(dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
-		return;
-
-	pcie_bus_configure_settings(dev->bus);
-
-	memset(&hpp, 0, sizeof(hpp));
-	pci_get_hp_params(dev, &hpp);
-
-	program_hpp_type2(dev, hpp.t2);
-	program_hpp_type1(dev, hpp.t1);
-	program_hpp_type0(dev, hpp.t0);
-
-	if (dev->subordinate) {
-		list_for_each_entry(cdev, &dev->subordinate->devices,
-				    bus_list)
-			pci_configure_slot(cdev);
-	}
-}
-EXPORT_SYMBOL_GPL(pci_configure_slot);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index e3cf8a2e6292..5d0cc646817a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
@@ -1236,6 +1237,153 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static struct hpp_type0 pci_default_type0 = {
+	.revision = 1,
+	.cache_line_size = 8,
+	.latency_timer = 0x40,
+	.enable_serr = 0,
+	.enable_perr = 0,
+};
+
+static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
+{
+	u16 pci_cmd, pci_bctl;
+
+	if (!hpp) {
+		/*
+		 * Perhaps we *should* use default settings for PCIe, but
+		 * pciehp didn't, so we won't either.
+		 */
+		if (pci_is_pcie(dev))
+			return;
+		hpp = &pci_default_type0;
+	}
+
+	if (hpp->revision > 1) {
+		dev_warn(&dev->dev,
+			 "PCI settings rev %d not supported; using defaults\n",
+			 hpp->revision);
+		hpp = &pci_default_type0;
+	}
+
+	pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, hpp->cache_line_size);
+	pci_write_config_byte(dev, PCI_LATENCY_TIMER, hpp->latency_timer);
+	pci_read_config_word(dev, PCI_COMMAND, &pci_cmd);
+	if (hpp->enable_serr)
+		pci_cmd |= PCI_COMMAND_SERR;
+	else
+		pci_cmd &= ~PCI_COMMAND_SERR;
+	if (hpp->enable_perr)
+		pci_cmd |= PCI_COMMAND_PARITY;
+	else
+		pci_cmd &= ~PCI_COMMAND_PARITY;
+	pci_write_config_word(dev, PCI_COMMAND, pci_cmd);
+
+	/* Program bridge control value */
+	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
+		pci_write_config_byte(dev, PCI_SEC_LATENCY_TIMER,
+				      hpp->latency_timer);
+		pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &pci_bctl);
+		if (hpp->enable_serr)
+			pci_bctl |= PCI_BRIDGE_CTL_SERR;
+		else
+			pci_bctl &= ~PCI_BRIDGE_CTL_SERR;
+		if (hpp->enable_perr)
+			pci_bctl |= PCI_BRIDGE_CTL_PARITY;
+		else
+			pci_bctl &= ~PCI_BRIDGE_CTL_PARITY;
+		pci_write_config_word(dev, PCI_BRIDGE_CONTROL, pci_bctl);
+	}
+}
+
+static void program_hpp_type1(struct pci_dev *dev, struct hpp_type1 *hpp)
+{
+	if (hpp)
+		dev_warn(&dev->dev, "PCI-X settings not supported\n");
+}
+
+static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
+{
+	int pos;
+	u32 reg32;
+
+	if (!hpp)
+		return;
+
+	if (hpp->revision > 1) {
+		dev_warn(&dev->dev, "PCIe settings rev %d not supported\n",
+			 hpp->revision);
+		return;
+	}
+
+	/* Initialize Device Control Register */
+	pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
+			~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);
+
+	/* Initialize Link Control Register */
+	if (dev->subordinate)
+		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
+			~hpp->pci_exp_lnkctl_and, hpp->pci_exp_lnkctl_or);
+
+	/* Find Advanced Error Reporting Enhanced Capability */
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	if (!pos)
+		return;
+
+	/* Initialize Uncorrectable Error Mask Register */
+	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, &reg32);
+	reg32 = (reg32 & hpp->unc_err_mask_and) | hpp->unc_err_mask_or;
+	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, reg32);
+
+	/* Initialize Uncorrectable Error Severity Register */
+	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &reg32);
+	reg32 = (reg32 & hpp->unc_err_sever_and) | hpp->unc_err_sever_or;
+	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, reg32);
+
+	/* Initialize Correctable Error Mask Register */
+	pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, &reg32);
+	reg32 = (reg32 & hpp->cor_err_mask_and) | hpp->cor_err_mask_or;
+	pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, reg32);
+
+	/* Initialize Advanced Error Capabilities and Control Register */
+	pci_read_config_dword(dev, pos + PCI_ERR_CAP, &reg32);
+	reg32 = (reg32 & hpp->adv_err_cap_and) | hpp->adv_err_cap_or;
+	pci_write_config_dword(dev, pos + PCI_ERR_CAP, reg32);
+
+	/*
+	 * FIXME: The following two registers are not supported yet.
+	 *
+	 *   o Secondary Uncorrectable Error Severity Register
+	 *   o Secondary Uncorrectable Error Mask Register
+	 */
+}
+
+void pci_configure_slot(struct pci_dev *dev)
+{
+	struct pci_dev *cdev;
+	struct hotplug_params hpp;
+
+	if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
+			(dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
+			(dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
+		return;
+
+	pcie_bus_configure_settings(dev->bus);
+
+	memset(&hpp, 0, sizeof(hpp));
+	pci_get_hp_params(dev, &hpp);
+
+	program_hpp_type2(dev, hpp.t2);
+	program_hpp_type1(dev, hpp.t1);
+	program_hpp_type0(dev, hpp.t0);
+
+	if (dev->subordinate) {
+		list_for_each_entry(cdev, &dev->subordinate->devices,
+				    bus_list)
+			pci_configure_slot(cdev);
+	}
+}
+EXPORT_SYMBOL_GPL(pci_configure_slot);
 static void pci_release_capabilities(struct pci_dev *dev)
 {
 	pci_vpd_release(dev);

[PATCH v1 03/13] PCI: Add pci_configure_device() during enumeration

[Bjorn Helgaas]

Some platforms can tell the OS how to configure PCI devices, e.g., how to
set cache line size, error reporting enables, etc.  ACPI defines _HPP and
_HPX methods for this purpose.

This configuration was previously done by some of the hotplug drivers using
pci_configure_slot().  But not all hotplug drivers did this, and per the
spec (ACPI rev 5.0, sec 6.2.7), we can also do it for "devices not
configured by the BIOS at system boot."

Move this configuration into the PCI core by adding pci_configure_device()
and calling it from pci_device_add(), so we do this for all devices as we
enumerate them.

This is based on pci_configure_slot(), which is used by hotplug drivers.
I omitted:

  - pcie_bus_configure_settings() because it configures MPS and MRRS, which
    requires global knowledge of the fabric and must be done later, and

  - configuration of subordinate devices; that will happen when we call
    pci_device_add() for those devices.

Because pci_configure_slot() was only done by hotplug drivers, this initial
version of pci_configure_device() only configures hot-added devices,
ignoring anything added during boot.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5d0cc646817a..93d2b41163fa 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1384,6 +1384,30 @@ void pci_configure_slot(struct pci_dev *dev)
 	}
 }
 EXPORT_SYMBOL_GPL(pci_configure_slot);
+
+static void pci_configure_device(struct pci_dev *dev)
+{
+	struct hotplug_params hpp;
+	int ret;
+
+	if (system_state == SYSTEM_BOOTING)
+		return;
+
+	if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
+			(dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
+			(dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
+		return;
+
+	memset(&hpp, 0, sizeof(hpp));
+	ret = pci_get_hp_params(dev, &hpp);
+	if (ret)
+		return;
+
+	program_hpp_type2(dev, hpp.t2);
+	program_hpp_type1(dev, hpp.t1);
+	program_hpp_type0(dev, hpp.t0);
+}
+
 static void pci_release_capabilities(struct pci_dev *dev)
 {
 	pci_vpd_release(dev);
@@ -1521,6 +1545,8 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	int ret;
 
+	pci_configure_device(dev);
+
 	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
 

[PATCH v1 04/13] PCI: pciehp: Configure hot-added display devices

[Bjorn Helgaas]

We configure cache line size and other settings of hot-added devices, e.g.,
based on ACPI _HPP or _HPX methods.  Previously we skipped this
configuration for display devices, but there is no spec requirement for
that.

Remove the check so we configure display devices the same way we configure
other devices.

See also ac81860ea073 ("PCI: hotplug: pciehp: Removed check for hotplug of
display devices").

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_pci.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 5f871f4c4af1..b66812703415 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -66,12 +66,8 @@ int pciehp_configure_device(struct slot *p_slot)
 
 	pci_assign_unassigned_bridge_resources(bridge);
 
-	list_for_each_entry(dev, &parent->devices, bus_list) {
-		if ((dev->class >> 16) == PCI_BASE_CLASS_DISPLAY)
-			continue;
-
+	list_for_each_entry(dev, &parent->devices, bus_list)
 		pci_configure_slot(dev);
-	}
 
 	pci_bus_add_devices(parent);
 

[PATCH v1 05/13] PCI: pciehp: Remove pci_configure_slot() usage

[Bjorn Helgaas]

We now configure each PCI device as it is enumerated, in pci_device_add(),
so remove the configuration done in pciehp.

That configuration, in pci_configure_device(), does not include the
MPS/MRRS configuration done by pcie_bus_configure_settings(), so keep
that here.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_pci.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index b66812703415..9e69403be632 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -65,10 +65,7 @@ int pciehp_configure_device(struct slot *p_slot)
 			pci_hp_add_bridge(dev);
 
 	pci_assign_unassigned_bridge_resources(bridge);
-
-	list_for_each_entry(dev, &parent->devices, bus_list)
-		pci_configure_slot(dev);
-
+	pcie_bus_configure_settings(parent);
 	pci_bus_add_devices(parent);
 
  out:

[PATCH v1 06/13] PCI: shpchp: Remove pci_configure_slot() usage

[Bjorn Helgaas]

We now configure each PCI device as it is enumerated, in pci_device_add(),
so remove the configuration done in shpchp.

That configuration, in pci_configure_device(), does not include the
MPS/MRRS configuration done by pcie_bus_configure_settings(), so keep
that here.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/shpchp_pci.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp_pci.c b/drivers/pci/hotplug/shpchp_pci.c
index 469454e0cc48..f8cd3a27e351 100644
--- a/drivers/pci/hotplug/shpchp_pci.c
+++ b/drivers/pci/hotplug/shpchp_pci.c
@@ -69,13 +69,7 @@ int shpchp_configure_device(struct slot *p_slot)
 	}
 
 	pci_assign_unassigned_bridge_resources(bridge);
-
-	list_for_each_entry(dev, &parent->devices, bus_list) {
-		if (PCI_SLOT(dev->devfn) != p_slot->device)
-			continue;
-		pci_configure_slot(dev);
-	}
-
+	pcie_bus_configure_settings(parent);
 	pci_bus_add_devices(parent);
 
  out:

[PATCH v1 07/13] ACPI / hotplug / PCI: Remove pci_configure_slot() usage

[Bjorn Helgaas]

We now configure each PCI device as it is enumerated, in pci_device_add(),
so remove the configuration done in acpiphp.

That configuration, in pci_configure_device(), does not include the
MPS/MRRS configuration done by pcie_bus_configure_settings(), so keep
that here.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 70741c8c46a0..a6f8e0ba0bfe 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -61,7 +61,6 @@ static DEFINE_MUTEX(bridge_mutex);
 static int acpiphp_hotplug_notify(struct acpi_device *adev, u32 type);
 static void acpiphp_post_dock_fixup(struct acpi_device *adev);
 static void acpiphp_sanitize_bus(struct pci_bus *bus);
-static void acpiphp_set_hpp_values(struct pci_bus *bus);
 static void hotplug_event(u32 type, struct acpiphp_context *context);
 static void free_bridge(struct kref *kref);
 
@@ -510,7 +509,7 @@ static void enable_slot(struct acpiphp_slot *slot)
 	__pci_bus_assign_resources(bus, &add_list, NULL);
 
 	acpiphp_sanitize_bus(bus);
-	acpiphp_set_hpp_values(bus);
+	pcie_bus_configure_settings(bus);
 	acpiphp_set_acpi_region(slot);
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -702,14 +701,6 @@ static void acpiphp_check_bridge(struct acpiphp_bridge *bridge)
 	}
 }
 
-static void acpiphp_set_hpp_values(struct pci_bus *bus)
-{
-	struct pci_dev *dev;
-
-	list_for_each_entry(dev, &bus->devices, bus_list)
-		pci_configure_slot(dev);
-}
-
 /*
  * Remove devices for which we could not assign resources, call
  * arch specific code to fix-up the bus

[PATCH v1 08/13] PCI: Remove unused pci_configure_slot()

[Bjorn Helgaas]

All pci_configure_slot() uses have been removed, so remove the definition
as well.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c         |   27 ---------------------------
 include/linux/pci_hotplug.h |    2 --
 2 files changed, 29 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 93d2b41163fa..4b3b29bc7a82 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1358,33 +1358,6 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
 	 */
 }
 
-void pci_configure_slot(struct pci_dev *dev)
-{
-	struct pci_dev *cdev;
-	struct hotplug_params hpp;
-
-	if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
-			(dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
-			(dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
-		return;
-
-	pcie_bus_configure_settings(dev->bus);
-
-	memset(&hpp, 0, sizeof(hpp));
-	pci_get_hp_params(dev, &hpp);
-
-	program_hpp_type2(dev, hpp.t2);
-	program_hpp_type1(dev, hpp.t1);
-	program_hpp_type0(dev, hpp.t0);
-
-	if (dev->subordinate) {
-		list_for_each_entry(cdev, &dev->subordinate->devices,
-				    bus_list)
-			pci_configure_slot(cdev);
-	}
-}
-EXPORT_SYMBOL_GPL(pci_configure_slot);
-
 static void pci_configure_device(struct pci_dev *dev)
 {
 	struct hotplug_params hpp;
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 5f2e559af6b0..2706ee9a4327 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -187,6 +187,4 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
 	return -ENODEV;
 }
 #endif
-
-void pci_configure_slot(struct pci_dev *dev);
 #endif

[PATCH v1 09/13] PCI: Apply _HPP settings to PCIe devices as well as PCI and PCI-X

[Bjorn Helgaas]

The ACPI _HPP method was defined before PCIe existed, so its documentation
only mentions PCI.  The _HPX Type 0 setting record is essentially identical
to _HPP, but the spec (ACPI rev 5.0, sec 6.2.8.1) says it should be applied
to PCI, PCI-X, and PCIe devices, with settings being ignored if they are
not applicable.

Some platforms with both conventional PCI and PCIe devices provide only
_HPP (not _HPX), so treat _HPP the same way as an _HPX Type 0 record and
apply it to PCIe devices as well as PCI and PCI-X.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4b3b29bc7a82..003d112a783d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1249,15 +1249,8 @@ static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
 {
 	u16 pci_cmd, pci_bctl;
 
-	if (!hpp) {
-		/*
-		 * Perhaps we *should* use default settings for PCIe, but
-		 * pciehp didn't, so we won't either.
-		 */
-		if (pci_is_pcie(dev))
-			return;
+	if (!hpp)
 		hpp = &pci_default_type0;
-	}
 
 	if (hpp->revision > 1) {
 		dev_warn(&dev->dev,

[PATCH v1 10/13] PCI: Preserve BIOS PCI_COMMAND_SERR and PCI_COMMAND_PARITY settings

[Bjorn Helgaas]

Do not clear PCI_COMMAND_SERR or PCI_COMMAND_PARITY based on _HPP.  The
spec (ACPI rev 5.0, sec 6.2.7) says that when "Enable SERR" is set to 1,
we should enable SERR in the command register.  It says nothing about
*disabling* SERR or PERR; in fact, the example in 6.2.7.1 says we should
leave PERR alone unless "Enable PERR" is 1.

For hot-added devices, this probably doesn't matter because they power up
with these bits cleared.  But in addition to hot-plugged devices, the spec
allows the platform to use _HPP for "configuration of PCI devices not
configured by the BIOS at system boot," and it may make a difference for
devices present at boot.

This change means that if BIOS enables SERR or PERR on a device, and it
supplies _HPP or _HPX with the SERR or PERR bits *cleared*, we will leave
SERR or PERR reporting enabled on that device instead of disabling it.

See also 40abb96c51bb ("pciehp: Fix programming hotplug parameters"), where
this code was first added.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 003d112a783d..a16b3472b70d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1264,12 +1264,8 @@ static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
 	pci_read_config_word(dev, PCI_COMMAND, &pci_cmd);
 	if (hpp->enable_serr)
 		pci_cmd |= PCI_COMMAND_SERR;
-	else
-		pci_cmd &= ~PCI_COMMAND_SERR;
 	if (hpp->enable_perr)
 		pci_cmd |= PCI_COMMAND_PARITY;
-	else
-		pci_cmd &= ~PCI_COMMAND_PARITY;
 	pci_write_config_word(dev, PCI_COMMAND, pci_cmd);
 
 	/* Program bridge control value */
@@ -1279,12 +1275,8 @@ static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
 		pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &pci_bctl);
 		if (hpp->enable_serr)
 			pci_bctl |= PCI_BRIDGE_CTL_SERR;
-		else
-			pci_bctl &= ~PCI_BRIDGE_CTL_SERR;
 		if (hpp->enable_perr)
 			pci_bctl |= PCI_BRIDGE_CTL_PARITY;
-		else
-			pci_bctl &= ~PCI_BRIDGE_CTL_PARITY;
 		pci_write_config_word(dev, PCI_BRIDGE_CONTROL, pci_bctl);
 	}
 }

[PATCH v1 11/13] PCI: Apply _HPP settings to all hot-added PCI devices

[Bjorn Helgaas]

We currently apply _HPP settings only to:

    - non-bridge devices, and
    - PCI-to-PCI bridges

i.e., we do not apply them to PCI-to-ISA bridges and the like.  It has been
that way since _HPP support was added by 40abb96c51bb ("pciehp: Fix
programming hotplug parameters"), but I don't think there's any reason to
exclude these other bridges.

Apply _HPP settings to hot-added PCI devices of any type.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a16b3472b70d..1ff2105ba401 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1351,11 +1351,6 @@ static void pci_configure_device(struct pci_dev *dev)
 	if (system_state == SYSTEM_BOOTING)
 		return;
 
-	if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
-			(dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
-			(dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
-		return;
-
 	memset(&hpp, 0, sizeof(hpp));
 	ret = pci_get_hp_params(dev, &hpp);
 	if (ret)

[PATCH v1 12/13] PCI: Preserve MPS and MRRS when applying _HPX settings

[Bjorn Helgaas]

Linux manages MPS and MRRS settings to keep them consistent across the PCIe
fabric.  BIOS doesn't participate in Linux scheme, so ignore that part of
any _HPX settings it supplies.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 1ff2105ba401..cb411fbb6435 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1301,6 +1301,16 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
 		return;
 	}
 
+	/*
+	 * Don't allow _HPX to change MPS or MRRS settings.  We manage
+	 * those to make sure they're consistent with the rest of the
+	 * platform.
+	 */
+	hpp->pci_exp_devctl_and |= PCI_EXP_DEVCTL_PAYLOAD |
+				    PCI_EXP_DEVCTL_READRQ;
+	hpp->pci_exp_devctl_or &= ~(PCI_EXP_DEVCTL_PAYLOAD |
+				    PCI_EXP_DEVCTL_READRQ);
+
 	/* Initialize Device Control Register */
 	pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
 			~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);

[PATCH v1 13/13] PCI: Configure *all* devices, not just hot-added ones

[Bjorn Helgaas]

There's not really a good way to determine whether firmware has already
configured a device with _HPP/_HPX settings.  On legacy systems, the BIOS
has probably configured everything, but on UEFI systems it is not required
to do so.

Per the PCI Firmware Specification, rev 3.1, sec 3.5, if PCI_COMMAND_IO or
PCI_COMMAND_MEMORY is set, we can assume firmware has set the corresponding
BARs and maybe we can assume it has configured the rest of the device.  And
if a bridge has PCI_COMMAND_PARITY or PCI_COMMAND_SERR set, we can assume
firmware has configured the bridge.  But we can't tell much about devices
without BARs.

I think it should be safe to apply _HPP and _HPX settings anyway, even if
firmware has already configured the device, so configure everything we
find.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cb411fbb6435..290c657da0b9 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1358,9 +1358,6 @@ static void pci_configure_device(struct pci_dev *dev)
 	struct hotplug_params hpp;
 	int ret;
 
-	if (system_state == SYSTEM_BOOTING)
-		return;
-
 	memset(&hpp, 0, sizeof(hpp));
 	ret = pci_get_hp_params(dev, &hpp);
 	if (ret)

Re: [PATCH v1 00/13] PCI: Device configuration cleanup

[Linus Torvalds]

On Fri, Sep 12, 2014 at 11:02 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> Finally, it tweaks several _HPP/_HPX things where we do things not
> required by the spec and I haven't seen any other good reason for them:

Looks ok to me, although, as always, resource management changes just
makes me worry that things worked "by mistake" before.

          Linus

Re: [PATCH v1 02/13] PCI: Move pci_configure_slot() to drivers/pci/probe.c

[Yinghai Lu]

On Fri, Sep 12, 2014 at 11:03 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Move pci_configure_slot() and related functions from
> drivers/pci/hotplug/pcihp_slot to drivers/pci/probe.c.
>
> This is to prepare for doing device configuration during the normal
> enumeration process instead of just after hot-add.
>
> No functional change.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/Makefile     |    2
>  drivers/pci/hotplug/pcihp_slot.c |  176 --------------------------------------
>  drivers/pci/probe.c              |  148 ++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+), 177 deletions(-)
>  delete mode 100644 drivers/pci/hotplug/pcihp_slot.c
>
> diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
> index 3e6532b945c1..4a9aa08b08f1 100644
> --- a/drivers/pci/hotplug/Makefile
> +++ b/drivers/pci/hotplug/Makefile
> @@ -24,7 +24,7 @@ obj-$(CONFIG_HOTPLUG_PCI_S390)                += s390_pci_hpc.o
>
>  obj-$(CONFIG_HOTPLUG_PCI_ACPI_IBM)     += acpiphp_ibm.o
>
> -pci_hotplug-objs       :=      pci_hotplug_core.o pcihp_slot.o
> +pci_hotplug-objs       :=      pci_hotplug_core.o
>
>  ifdef CONFIG_HOTPLUG_PCI_CPCI
>  pci_hotplug-objs       +=      cpci_hotplug_core.o     \
> diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
> deleted file mode 100644
> index 3e36ec8d708a..000000000000
> --- a/drivers/pci/hotplug/pcihp_slot.c
> +++ /dev/null
> @@ -1,176 +0,0 @@
> -/*
> - * Copyright (C) 1995,2001 Compaq Computer Corporation
> - * Copyright (C) 2001 Greg Kroah-Hartman (greg@kroah.com)
> - * Copyright (C) 2001 IBM Corp.
> - * Copyright (C) 2003-2004 Intel Corporation
> - * (c) Copyright 2009 Hewlett-Packard Development Company, L.P.
> - *
> - * All rights reserved.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or (at
> - * your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> - * NON INFRINGEMENT.  See the GNU General Public License for more
> - * details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> - */
> -
> -#include <linux/pci.h>
> -#include <linux/export.h>
> -#include <linux/pci_hotplug.h>
> -
> -static struct hpp_type0 pci_default_type0 = {
> -       .revision = 1,
> -       .cache_line_size = 8,
> -       .latency_timer = 0x40,
> -       .enable_serr = 0,
> -       .enable_perr = 0,
> -};
> -
> -static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
> -{
> -       u16 pci_cmd, pci_bctl;
> -
> -       if (!hpp) {
> -               /*
> -                * Perhaps we *should* use default settings for PCIe, but
> -                * pciehp didn't, so we won't either.
> -                */
> -               if (pci_is_pcie(dev))
> -                       return;
> -               hpp = &pci_default_type0;
> -       }
> -
> -       if (hpp->revision > 1) {
> -               dev_warn(&dev->dev,
> -                        "PCI settings rev %d not supported; using defaults\n",
> -                        hpp->revision);
> -               hpp = &pci_default_type0;
> -       }
> -
> -       pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, hpp->cache_line_size);
> -       pci_write_config_byte(dev, PCI_LATENCY_TIMER, hpp->latency_timer);
> -       pci_read_config_word(dev, PCI_COMMAND, &pci_cmd);
> -       if (hpp->enable_serr)
> -               pci_cmd |= PCI_COMMAND_SERR;
> -       else
> -               pci_cmd &= ~PCI_COMMAND_SERR;
> -       if (hpp->enable_perr)
> -               pci_cmd |= PCI_COMMAND_PARITY;
> -       else
> -               pci_cmd &= ~PCI_COMMAND_PARITY;
> -       pci_write_config_word(dev, PCI_COMMAND, pci_cmd);
> -
> -       /* Program bridge control value */
> -       if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> -               pci_write_config_byte(dev, PCI_SEC_LATENCY_TIMER,
> -                                     hpp->latency_timer);
> -               pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &pci_bctl);
> -               if (hpp->enable_serr)
> -                       pci_bctl |= PCI_BRIDGE_CTL_SERR;
> -               else
> -                       pci_bctl &= ~PCI_BRIDGE_CTL_SERR;
> -               if (hpp->enable_perr)
> -                       pci_bctl |= PCI_BRIDGE_CTL_PARITY;
> -               else
> -                       pci_bctl &= ~PCI_BRIDGE_CTL_PARITY;
> -               pci_write_config_word(dev, PCI_BRIDGE_CONTROL, pci_bctl);
> -       }
> -}
> -
> -static void program_hpp_type1(struct pci_dev *dev, struct hpp_type1 *hpp)
> -{
> -       if (hpp)
> -               dev_warn(&dev->dev, "PCI-X settings not supported\n");
> -}
> -
> -static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
> -{
> -       int pos;
> -       u32 reg32;
> -
> -       if (!hpp)
> -               return;
> -
> -       if (hpp->revision > 1) {
> -               dev_warn(&dev->dev, "PCIe settings rev %d not supported\n",
> -                        hpp->revision);
> -               return;
> -       }
> -
> -       /* Initialize Device Control Register */
> -       pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> -                       ~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);
> -
> -       /* Initialize Link Control Register */
> -       if (dev->subordinate)
> -               pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> -                       ~hpp->pci_exp_lnkctl_and, hpp->pci_exp_lnkctl_or);
> -
> -       /* Find Advanced Error Reporting Enhanced Capability */
> -       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> -       if (!pos)
> -               return;
> -
> -       /* Initialize Uncorrectable Error Mask Register */
> -       pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, &reg32);
> -       reg32 = (reg32 & hpp->unc_err_mask_and) | hpp->unc_err_mask_or;
> -       pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, reg32);
> -
> -       /* Initialize Uncorrectable Error Severity Register */
> -       pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &reg32);
> -       reg32 = (reg32 & hpp->unc_err_sever_and) | hpp->unc_err_sever_or;
> -       pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, reg32);
> -
> -       /* Initialize Correctable Error Mask Register */
> -       pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, &reg32);
> -       reg32 = (reg32 & hpp->cor_err_mask_and) | hpp->cor_err_mask_or;
> -       pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, reg32);
> -
> -       /* Initialize Advanced Error Capabilities and Control Register */
> -       pci_read_config_dword(dev, pos + PCI_ERR_CAP, &reg32);
> -       reg32 = (reg32 & hpp->adv_err_cap_and) | hpp->adv_err_cap_or;
> -       pci_write_config_dword(dev, pos + PCI_ERR_CAP, reg32);
> -
> -       /*
> -        * FIXME: The following two registers are not supported yet.
> -        *
> -        *   o Secondary Uncorrectable Error Severity Register
> -        *   o Secondary Uncorrectable Error Mask Register
> -        */
> -}
> -
> -void pci_configure_slot(struct pci_dev *dev)
> -{
> -       struct pci_dev *cdev;
> -       struct hotplug_params hpp;
> -
> -       if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
> -                       (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> -                       (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
> -               return;
> -
> -       pcie_bus_configure_settings(dev->bus);
> -
> -       memset(&hpp, 0, sizeof(hpp));
> -       pci_get_hp_params(dev, &hpp);
> -
> -       program_hpp_type2(dev, hpp.t2);
> -       program_hpp_type1(dev, hpp.t1);
> -       program_hpp_type0(dev, hpp.t0);
> -
> -       if (dev->subordinate) {
> -               list_for_each_entry(cdev, &dev->subordinate->devices,
> -                                   bus_list)
> -                       pci_configure_slot(cdev);
> -       }
> -}
> -EXPORT_SYMBOL_GPL(pci_configure_slot);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index e3cf8a2e6292..5d0cc646817a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -6,6 +6,7 @@
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/pci.h>
> +#include <linux/pci_hotplug.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/cpumask.h>
> @@ -1236,6 +1237,153 @@ int pci_setup_device(struct pci_dev *dev)
>         return 0;
>  }
>
> +static struct hpp_type0 pci_default_type0 = {
> +       .revision = 1,
> +       .cache_line_size = 8,
> +       .latency_timer = 0x40,
> +       .enable_serr = 0,
> +       .enable_perr = 0,
> +};
> +
> +static void program_hpp_type0(struct pci_dev *dev, struct hpp_type0 *hpp)
> +{
> +       u16 pci_cmd, pci_bctl;
> +
> +       if (!hpp) {
> +               /*
> +                * Perhaps we *should* use default settings for PCIe, but
> +                * pciehp didn't, so we won't either.
> +                */
> +               if (pci_is_pcie(dev))
> +                       return;
> +               hpp = &pci_default_type0;
> +       }
> +
> +       if (hpp->revision > 1) {
> +               dev_warn(&dev->dev,
> +                        "PCI settings rev %d not supported; using defaults\n",
> +                        hpp->revision);
> +               hpp = &pci_default_type0;
> +       }
> +
> +       pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, hpp->cache_line_size);
> +       pci_write_config_byte(dev, PCI_LATENCY_TIMER, hpp->latency_timer);
> +       pci_read_config_word(dev, PCI_COMMAND, &pci_cmd);
> +       if (hpp->enable_serr)
> +               pci_cmd |= PCI_COMMAND_SERR;
> +       else
> +               pci_cmd &= ~PCI_COMMAND_SERR;
> +       if (hpp->enable_perr)
> +               pci_cmd |= PCI_COMMAND_PARITY;
> +       else
> +               pci_cmd &= ~PCI_COMMAND_PARITY;
> +       pci_write_config_word(dev, PCI_COMMAND, pci_cmd);
> +
> +       /* Program bridge control value */
> +       if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> +               pci_write_config_byte(dev, PCI_SEC_LATENCY_TIMER,
> +                                     hpp->latency_timer);
> +               pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &pci_bctl);
> +               if (hpp->enable_serr)
> +                       pci_bctl |= PCI_BRIDGE_CTL_SERR;
> +               else
> +                       pci_bctl &= ~PCI_BRIDGE_CTL_SERR;
> +               if (hpp->enable_perr)
> +                       pci_bctl |= PCI_BRIDGE_CTL_PARITY;
> +               else
> +                       pci_bctl &= ~PCI_BRIDGE_CTL_PARITY;
> +               pci_write_config_word(dev, PCI_BRIDGE_CONTROL, pci_bctl);
> +       }
> +}
> +
> +static void program_hpp_type1(struct pci_dev *dev, struct hpp_type1 *hpp)
> +{
> +       if (hpp)
> +               dev_warn(&dev->dev, "PCI-X settings not supported\n");
> +}
> +
> +static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
> +{
> +       int pos;
> +       u32 reg32;
> +
> +       if (!hpp)
> +               return;
> +
> +       if (hpp->revision > 1) {
> +               dev_warn(&dev->dev, "PCIe settings rev %d not supported\n",
> +                        hpp->revision);
> +               return;
> +       }
> +
> +       /* Initialize Device Control Register */
> +       pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> +                       ~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);
> +
> +       /* Initialize Link Control Register */
> +       if (dev->subordinate)
> +               pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> +                       ~hpp->pci_exp_lnkctl_and, hpp->pci_exp_lnkctl_or);
> +
> +       /* Find Advanced Error Reporting Enhanced Capability */
> +       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +       if (!pos)
> +               return;
> +
> +       /* Initialize Uncorrectable Error Mask Register */
> +       pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, &reg32);
> +       reg32 = (reg32 & hpp->unc_err_mask_and) | hpp->unc_err_mask_or;
> +       pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, reg32);
> +
> +       /* Initialize Uncorrectable Error Severity Register */
> +       pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &reg32);
> +       reg32 = (reg32 & hpp->unc_err_sever_and) | hpp->unc_err_sever_or;
> +       pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, reg32);
> +
> +       /* Initialize Correctable Error Mask Register */
> +       pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, &reg32);
> +       reg32 = (reg32 & hpp->cor_err_mask_and) | hpp->cor_err_mask_or;
> +       pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, reg32);
> +
> +       /* Initialize Advanced Error Capabilities and Control Register */
> +       pci_read_config_dword(dev, pos + PCI_ERR_CAP, &reg32);
> +       reg32 = (reg32 & hpp->adv_err_cap_and) | hpp->adv_err_cap_or;
> +       pci_write_config_dword(dev, pos + PCI_ERR_CAP, reg32);
> +
> +       /*
> +        * FIXME: The following two registers are not supported yet.
> +        *
> +        *   o Secondary Uncorrectable Error Severity Register
> +        *   o Secondary Uncorrectable Error Mask Register
> +        */
> +}
> +
> +void pci_configure_slot(struct pci_dev *dev)
> +{
> +       struct pci_dev *cdev;
> +       struct hotplug_params hpp;
> +
> +       if (!(dev->hdr_type == PCI_HEADER_TYPE_NORMAL ||
> +                       (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> +                       (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
> +               return;
> +
> +       pcie_bus_configure_settings(dev->bus);
> +
> +       memset(&hpp, 0, sizeof(hpp));
> +       pci_get_hp_params(dev, &hpp);
> +
> +       program_hpp_type2(dev, hpp.t2);
> +       program_hpp_type1(dev, hpp.t1);
> +       program_hpp_type0(dev, hpp.t0);
> +
> +       if (dev->subordinate) {
> +               list_for_each_entry(cdev, &dev->subordinate->devices,
> +                                   bus_list)
> +                       pci_configure_slot(cdev);
> +       }
> +}
> +EXPORT_SYMBOL_GPL(pci_configure_slot);
>  static void pci_release_capabilities(struct pci_dev *dev)
>  {
>         pci_vpd_release(dev);
>

Would be better if those acpi related code could be moved to
drivers/pci/pci-acpi.c. and should put pci_configure_device() from
next patch to pci-acpi.c.

Other that,  for the whole patchset,

Acked-by: Yinghai Lu <yinghai@kernel.org>

Re: [PATCH v1 02/13] PCI: Move pci_configure_slot() to drivers/pci/probe.c

[Bjorn Helgaas]

On Fri, Sep 12, 2014 at 3:31 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 12, 2014 at 11:03 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Move pci_configure_slot() and related functions from
>> drivers/pci/hotplug/pcihp_slot to drivers/pci/probe.c.
>>
>> This is to prepare for doing device configuration during the normal
>> enumeration process instead of just after hot-add.
>>
>> ...

> Would be better if those acpi related code could be moved to
> drivers/pci/pci-acpi.c. and should put pci_configure_device() from
> next patch to pci-acpi.c.

Good point.  I just moved a bunch of the stuff from
drivers/pci/hotplug/acpi_pcihp.c to pci-acpi.h, because acpi_pcihp.c
is not compiled unless CONFIG_PCI_HOTPLUG is set.  And this stuff
probably should go there, too.

> Other that,  for the whole patchset,
>
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Re: [PATCH v1 02/13] PCI: Move pci_configure_slot() to drivers/pci/probe.c

[Bjorn Helgaas]

On Fri, Sep 12, 2014 at 3:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Sep 12, 2014 at 3:31 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Sep 12, 2014 at 11:03 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> Move pci_configure_slot() and related functions from
>>> drivers/pci/hotplug/pcihp_slot to drivers/pci/probe.c.
>>>
>>> This is to prepare for doing device configuration during the normal
>>> enumeration process instead of just after hot-add.
>>>
>>> ...
>
>> Would be better if those acpi related code could be moved to
>> drivers/pci/pci-acpi.c. and should put pci_configure_device() from
>> next patch to pci-acpi.c.
>
> Good point.  I just moved a bunch of the stuff from
> drivers/pci/hotplug/acpi_pcihp.c to pci-acpi.h, because acpi_pcihp.c
> is not compiled unless CONFIG_PCI_HOTPLUG is set.  And this stuff
> probably should go there, too.

On second thought, none of the stuff from pcihp_slot.c
(pci_configure_slot(), program_hpp_type0(), program_hpp_type1(),
program_hpp2()) actually mentions ACPI or uses any ACPI interfaces,
and it is all compiled into pcihp_slot.o today even without
CONFIG_ACPI.  So my patch really doesn't change any of that, except to
make it available when CONFIG_PCI_HOTPLUG is not set.  I'll have to
think about this some more.  It certainly is all implicitly driven by
the ACPI spec and the "hpp", "type0", "type1", etc. names are from
ACPI, so maybe it could be abstracted better somehow.

>> Other that,  for the whole patchset,
>>
>> Acked-by: Yinghai Lu <yinghai@kernel.org>