[PATCH v6 0/3] PCI hotplug feature

[PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

These patches bring in PCI hotplug support for iproc family chipsets.

It includes DT binding documentation and, the implementation in
iproc pcie RC driver.

Changes since v5:
[RESEND]

Changes since v4:
Rebased to pci-next
Added; Acked-by: Rob Herring <robh@kernel.org>

Changes since v3:
Resend. just to be in sync previous in-flight patches.

Changes since v2:
Addressed Rob Herring's comments.
Changed subject to "dt-bindings: PCI:..."
Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
Rebased the patches on top of Lorenzo's patches.

Oza Pawandeep (3):
  dt-bindings: PCI: Add PCI hotplug property
  dt-bindings: PCI iproc: Implement optional property prsnt-gpios
  PCI: iproc: Implement PCI hotplug support

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
 Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
 drivers/pci/host/pcie-iproc-platform.c             |   3 +
 drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
 drivers/pci/host/pcie-iproc.h                      |   7 +
 5 files changed, 187 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

These patches bring in PCI hotplug support for iproc family chipsets.

It includes DT binding documentation and, the implementation in
iproc pcie RC driver.

Changes since v5:
[RESEND]

Changes since v4:
Rebased to pci-next
Added; Acked-by: Rob Herring <robh@kernel.org>

Changes since v3:
Resend. just to be in sync previous in-flight patches.

Changes since v2:
Addressed Rob Herring's comments.
Changed subject to "dt-bindings: PCI:..."
Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
Rebased the patches on top of Lorenzo's patches.

Oza Pawandeep (3):
  dt-bindings: PCI: Add PCI hotplug property
  dt-bindings: PCI iproc: Implement optional property prsnt-gpios
  PCI: iproc: Implement PCI hotplug support

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
 Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
 drivers/pci/host/pcie-iproc-platform.c             |   3 +
 drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
 drivers/pci/host/pcie-iproc.h                      |   7 +
 5 files changed, 187 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

These patches bring in PCI hotplug support for iproc family chipsets.

It includes DT binding documentation and, the implementation in
iproc pcie RC driver.

Changes since v5:
[RESEND]

Changes since v4:
Rebased to pci-next
Added; Acked-by: Rob Herring <robh@kernel.org>

Changes since v3:
Resend. just to be in sync previous in-flight patches.

Changes since v2:
Addressed Rob Herring's comments.
Changed subject to "dt-bindings: PCI:..."
Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
Rebased the patches on top of Lorenzo's patches.

Oza Pawandeep (3):
  dt-bindings: PCI: Add PCI hotplug property
  dt-bindings: PCI iproc: Implement optional property prsnt-gpios
  PCI: iproc: Implement PCI hotplug support

 .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
 Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
 drivers/pci/host/pcie-iproc-platform.c             |   3 +
 drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
 drivers/pci/host/pcie-iproc.h                      |   7 +
 5 files changed, 187 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v6 1/3] dt-bindings: PCI: Add PCI hotplug property

[Oza Pawandeep]

Host drivers have the requirement of implementing PCI hotplug
based on the how their SOC supports it.

Couple of properties have been added. the one to enable
the hotplug feature itself, and the other caters to
the PCI hotplug implementation with the use of gpios.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index 50f9e2c..0bf25a1 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,18 @@ driver implementation may support the following properties:
    unsupported link speed, for instance, trying to do training for
    unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
    for gen2, and '1' for gen1. Any other values are invalid.
+
+- slot-pluggable:
+   PCI hotplug feature is supported.
+   PCI hotplug implementation is SOC/Board specific, and also it depends on
+   how add-in card is designed (e.g. how many present pins are implemented).
+   If the slot-pluggable property is present, the following propertey could
+   become effective.
+   - prsnt-gpios:
+      Array of gpios, could be present if hotplug is supported.
+      This property defines gpio based hotplug implementation.
+      Example:
+      If x8 card is connected, then it might be possible that all the
+      3 present pins could go low, or at least one pin goes low.
+      If x4 card is connected, then it might be possible that 2 present
+      pins go low, or at least one pin goes low.
-- 
1.9.1

[PATCH v6 1/3] dt-bindings: PCI: Add PCI hotplug property

[Oza Pawandeep]

Host drivers have the requirement of implementing PCI hotplug
based on the how their SOC supports it.

Couple of properties have been added. the one to enable
the hotplug feature itself, and the other caters to
the PCI hotplug implementation with the use of gpios.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index 50f9e2c..0bf25a1 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,18 @@ driver implementation may support the following properties:
    unsupported link speed, for instance, trying to do training for
    unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
    for gen2, and '1' for gen1. Any other values are invalid.
+
+- slot-pluggable:
+   PCI hotplug feature is supported.
+   PCI hotplug implementation is SOC/Board specific, and also it depends on
+   how add-in card is designed (e.g. how many present pins are implemented).
+   If the slot-pluggable property is present, the following propertey could
+   become effective.
+   - prsnt-gpios:
+      Array of gpios, could be present if hotplug is supported.
+      This property defines gpio based hotplug implementation.
+      Example:
+      If x8 card is connected, then it might be possible that all the
+      3 present pins could go low, or at least one pin goes low.
+      If x4 card is connected, then it might be possible that 2 present
+      pins go low, or at least one pin goes low.
-- 
1.9.1

[PATCH v6 1/3] dt-bindings: PCI: Add PCI hotplug property

[Oza Pawandeep]

Host drivers have the requirement of implementing PCI hotplug
based on the how their SOC supports it.

Couple of properties have been added. the one to enable
the hotplug feature itself, and the other caters to
the PCI hotplug implementation with the use of gpios.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index 50f9e2c..0bf25a1 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,18 @@ driver implementation may support the following properties:
    unsupported link speed, for instance, trying to do training for
    unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
    for gen2, and '1' for gen1. Any other values are invalid.
+
+- slot-pluggable:
+   PCI hotplug feature is supported.
+   PCI hotplug implementation is SOC/Board specific, and also it depends on
+   how add-in card is designed (e.g. how many present pins are implemented).
+   If the slot-pluggable property is present, the following propertey could
+   become effective.
+   - prsnt-gpios:
+      Array of gpios, could be present if hotplug is supported.
+      This property defines gpio based hotplug implementation.
+      Example:
+      If x8 card is connected, then it might be possible that all the
+      3 present pins could go low, or at least one pin goes low.
+      If x4 card is connected, then it might be possible that 2 present
+      pins go low, or at least one pin goes low.
-- 
1.9.1

[PATCH v6 2/3] dt-bindings: PCI iproc: Implement optional property prsnt-gpios

[Oza Pawandeep]

Add description for optional device tree property
'prsnt-gpios' for PCI hotplug feature.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index b8e48b4..0c5f631 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -72,6 +72,20 @@ Optional properties:
 - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
 require the interrupt enable registers to be set explicitly to enable MSI
 
+Optional properties:
+- slot-pluggable: PCI hotplug feature is supported.
+- prsnt-gpios: Array of gpios, needs to be present if Hotplug is supported.
+
+Refer to the following binding documents for more detailed description on
+the use of 'slot-pluggable' and 'prsnt-gpios'.
+  Documentation/devicetree/bindings/pci/pci.txt
+
+Example:
+prsnt-gpios: <&pca9505 32 1>, <&pca9505 33 1>;
+This is x4 connector: monitoring max 2 present lines.
+prsnt-gpios: <&pca9505 32 1>, <&pca9505 33 1>, <&pca9505 34 1>;
+This is x8 connector: monitoring max 3 present lines.
+
 Example:
 	pcie0: pcie@18012000 {
 		compatible = "brcm,iproc-pcie";
-- 
1.9.1

[PATCH v6 2/3] dt-bindings: PCI iproc: Implement optional property prsnt-gpios

[Oza Pawandeep]

Add description for optional device tree property
'prsnt-gpios' for PCI hotplug feature.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index b8e48b4..0c5f631 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -72,6 +72,20 @@ Optional properties:
 - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
 require the interrupt enable registers to be set explicitly to enable MSI
 
+Optional properties:
+- slot-pluggable: PCI hotplug feature is supported.
+- prsnt-gpios: Array of gpios, needs to be present if Hotplug is supported.
+
+Refer to the following binding documents for more detailed description on
+the use of 'slot-pluggable' and 'prsnt-gpios'.
+  Documentation/devicetree/bindings/pci/pci.txt
+
+Example:
+prsnt-gpios: <&pca9505 32 1>, <&pca9505 33 1>;
+This is x4 connector: monitoring max 2 present lines.
+prsnt-gpios: <&pca9505 32 1>, <&pca9505 33 1>, <&pca9505 34 1>;
+This is x8 connector: monitoring max 3 present lines.
+
 Example:
 	pcie0: pcie@18012000 {
 		compatible = "brcm,iproc-pcie";
-- 
1.9.1

[PATCH v6 2/3] dt-bindings: PCI iproc: Implement optional property prsnt-gpios

[Oza Pawandeep]

Add description for optional device tree property
'prsnt-gpios' for PCI hotplug feature.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Acked-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index b8e48b4..0c5f631 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -72,6 +72,20 @@ Optional properties:
 - brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
 require the interrupt enable registers to be set explicitly to enable MSI
 
+Optional properties:
+- slot-pluggable: PCI hotplug feature is supported.
+- prsnt-gpios: Array of gpios, needs to be present if Hotplug is supported.
+
+Refer to the following binding documents for more detailed description on
+the use of 'slot-pluggable' and 'prsnt-gpios'.
+  Documentation/devicetree/bindings/pci/pci.txt
+
+Example:
+prsnt-gpios: <&pca9505 32 1>, <&pca9505 33 1>;
+This is x4 connector: monitoring max 2 present lines.
+prsnt-gpios: <&pca9505 32 1>, <&pca9505 33 1>, <&pca9505 34 1>;
+This is x8 connector: monitoring max 3 present lines.
+
 Example:
 	pcie0: pcie at 18012000 {
 		compatible = "brcm,iproc-pcie";
-- 
1.9.1

[PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Oza Pawandeep]

This patch implements PCI hotplug support for iproc family chipsets.

iproc based SOC (e.g. Stingray) does not have hotplug controller
integrated.
Hence, standard PCI hotplug framework hooks can-not be used.
e.g. controlled power up/down of slot.

The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
PCI present lines are input to GPIOs depending on the type of
connector (x2, x4, x8).

GPIO array needs to be present if hotplug is supported.
HW implementation is SOC/Board specific, and also it depends on how
add-in card is designed
(e.g. how many present pins are implemented).

If x8 card is connected, then it might be possible that all the
3 present pins could go low, or at least one pin goes low.
If x4 card is connected, then it might be possible that 2 present
pins go low, or at least one pin goes low.

The implementation essentially takes care of following:
> Initializing hotplug irq thread.
> Detecting the endpoint device based on link state.
> Handling PERST and detecting the plugged devices.
> Ordered Hot plug-out, where User is expected
  to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> Handling spurious interrupt
> Handling multiple interrupts and makes sure that card is
  enumerated only once.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index a5073a9..6287a43 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		pcie->need_ob_cfg = true;
 	}
 
+	if (of_property_read_bool(np, "slot-pluggable"))
+		pcie->enable_hotplug = true;
+
 	/* PHY use is optional */
 	pcie->phy = devm_phy_get(dev, "pcie-phy");
 	if (IS_ERR(pcie->phy)) {
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 8bd5e54..2b4d830 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -28,6 +28,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/phy/phy.h>
+#include <linux/gpio.h>
 
 #include "pcie-iproc.h"
 
@@ -65,6 +66,17 @@
 #define PCIE_DL_ACTIVE_SHIFT         2
 #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
 
+#define CFG_RC_LTSSM                 0x1cf8
+#define CFG_RC_PHY_CTL               0x1804
+#define CFG_RC_LTSSM_TIMEOUT         1000
+#define CFG_RC_LTSSM_STATE_MASK      0xff
+#define CFG_RC_LTSSM_STATE_L1        0x1
+
+#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
+#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
+#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
+#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
+
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
@@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
 	return 0;
 }
 
+static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)
+{
+	struct pci_bus *bus = pcie->root_bus;
+	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
+
+	/* Clear LTSSM history. */
+	pci_bus_read_config_dword(pcie->root_bus, 0,
+				  CFG_RC_PHY_CTL, &val);
+	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
+				   val | CFG_RC_CLR_RECOV_HIST_MASK |
+				   CFG_RC_CLR_LTSSM_HIST_MASK);
+	/* write back the origional value. */
+	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
+
+	do {
+		pci_bus_read_config_dword(pcie->root_bus, 0,
+					  CFG_RC_LTSSM, &val);
+		/* check link state to see if link moved to L1 state. */
+		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
+		     CFG_RC_LTSSM_STATE_L1)
+			return true;
+		timeout--;
+		usleep_range(500, 1000);
+	} while (timeout);
+
+	return false;
+}
+
+static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
+{
+	struct iproc_pcie *pcie = data;
+	struct pci_bus *bus = pcie->root_bus, *child;
+	bool link_status;
+
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
+
+	link_status = iproc_pci_hp_check_ltssm(pcie);
+
+	if (link_status &&
+	    !iproc_pcie_check_link(pcie) &&
+	    !pcie->ep_is_present) {
+		pci_rescan_bus(bus);
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+		pcie->ep_is_present = true;
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device detected and enumerated>\n");
+	} else if (link_status && pcie->ep_is_present)
+		/*
+		 * ep_is_present makes sure, enumuration done only once.
+		 * So it can handle spurious intrrupts, and also if we
+		 * get multiple interrupts for all the implemented pins,
+		 * we handle it only once.
+		 */
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device already present>\n");
+	else {
+		iproc_pcie_perst_ctrl(pcie, true);
+		pcie->ep_is_present = false;
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device removed>\n");
+	}
+	return IRQ_HANDLED;
+}
+
+static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
+{
+	struct gpio_descs *hp_gpiod;
+	struct device *dev = pcie->dev;
+	int i;
+
+	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
+	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
+		for (i = 0; i < hp_gpiod->ndescs; ++i) {
+			gpiod_direction_input(hp_gpiod->desc[i]);
+			if (request_threaded_irq(gpiod_to_irq
+						 (hp_gpiod->desc[i]),
+						 NULL, iproc_pci_hotplug_thread,
+						 IRQF_TRIGGER_FALLING,
+						 "PCI-hotplug", pcie))
+				dev_err(dev,
+					"PCI hotplug prsnt: request irq failed\n");
+			}
+	}
+	pcie->ep_is_present = false;
+
+	return 0;
+}
+
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	struct device *dev;
 	int ret;
 	void *sysdata;
-	struct pci_bus *child;
+	struct pci_bus *bus, *child;
 	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	bool link_not_active;
 
 	dev = pcie->dev;
 
@@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
+	if (pcie->enable_hotplug) {
+		ret = iproc_pci_hp_gpio_irq_get(pcie);
+		if (ret < 0)
+			return ret;
+	}
+
 	iproc_pcie_perst_ctrl(pcie, true);
 	iproc_pcie_perst_ctrl(pcie, false);
 
@@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	sysdata = pcie;
 #endif
 
-	ret = iproc_pcie_check_link(pcie);
-	if (ret) {
+	link_not_active = iproc_pcie_check_link(pcie);
+	if (link_not_active && pcie->enable_hotplug) {
+		/*
+		 * When link is not active and PCI hotplug
+		 * is supported, do not turn off phy, let probe
+		 * go ahead.
+		 */
+		dev_err(dev, "no PCIe EP device detected\n");
+		iproc_pcie_perst_ctrl(pcie, true);
+	} else if (link_not_active) {
 		dev_err(dev, "no PCIe EP device detected\n");
 		goto err_power_off_phy;
 	}
@@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		if (iproc_pcie_msi_enable(pcie))
 			dev_info(dev, "not using iProc MSI\n");
 
-	list_splice_init(res, &host->windows);
-	host->busnr = 0;
-	host->dev.parent = dev;
-	host->ops = &iproc_pcie_ops;
-	host->sysdata = sysdata;
-	host->map_irq = pcie->map_irq;
-	host->swizzle_irq = pci_common_swizzle;
+	if (!link_not_active) {
+		list_splice_init(res, &host->windows);
+		host->busnr = 0;
+		host->dev.parent = dev;
+		host->ops = &iproc_pcie_ops;
+		host->sysdata = sysdata;
+		host->map_irq = pcie->map_irq;
+		host->swizzle_irq = pci_common_swizzle;
+
+		ret = pci_scan_root_bus_bridge(host);
+		if (ret < 0) {
+			dev_err(dev, "failed to scan host: %d\n", ret);
+			goto err_power_off_phy;
+		}
 
-	ret = pci_scan_root_bus_bridge(host);
-	if (ret < 0) {
-		dev_err(dev, "failed to scan host: %d\n", ret);
-		goto err_power_off_phy;
+		pci_assign_unassigned_bus_resources(host->bus);
+		pcie->root_bus = host->bus;
+	} else {
+		bus = pci_create_root_bus(dev, 0,
+					  &iproc_pcie_ops, sysdata, res);
+		if (!bus) {
+			dev_err(dev, "unable to create PCI root bus\n");
+			ret = -ENOMEM;
+			goto err_power_off_phy;
+		}
+		pcie->root_bus = bus;
 	}
 
-	pci_assign_unassigned_bus_resources(host->bus);
-
-	pcie->root_bus = host->bus;
-
 	list_for_each_entry(child, &host->bus->children, node)
 		pcie_bus_configure_settings(child);
 
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index a6b55ce..e5d0cd4 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -77,6 +77,10 @@ struct iproc_pcie_ib {
  * @ib: inbound mapping related parameters
  * @ib_map: outbound mapping region related parameters
  *
+ * @enable_hotplug: indicates PCI hotplug feature is enabled
+ * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
+ * indicate whether or not the endpoint device is present
+ *
  * @need_msi_steer: indicates additional configuration of the iProc PCIe
  * controller is required to steer MSI writes to external interrupt controller
  * @msi: MSI data
@@ -104,6 +108,9 @@ struct iproc_pcie {
 	struct iproc_pcie_ib ib;
 	const struct iproc_pcie_ib_map *ib_map;
 
+	bool enable_hotplug;
+	bool ep_is_present;
+
 	bool need_msi_steer;
 	struct iproc_msi *msi;
 };
-- 
1.9.1

[PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Oza Pawandeep]

This patch implements PCI hotplug support for iproc family chipsets.

iproc based SOC (e.g. Stingray) does not have hotplug controller
integrated.
Hence, standard PCI hotplug framework hooks can-not be used.
e.g. controlled power up/down of slot.

The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
PCI present lines are input to GPIOs depending on the type of
connector (x2, x4, x8).

GPIO array needs to be present if hotplug is supported.
HW implementation is SOC/Board specific, and also it depends on how
add-in card is designed
(e.g. how many present pins are implemented).

If x8 card is connected, then it might be possible that all the
3 present pins could go low, or at least one pin goes low.
If x4 card is connected, then it might be possible that 2 present
pins go low, or at least one pin goes low.

The implementation essentially takes care of following:
> Initializing hotplug irq thread.
> Detecting the endpoint device based on link state.
> Handling PERST and detecting the plugged devices.
> Ordered Hot plug-out, where User is expected
  to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> Handling spurious interrupt
> Handling multiple interrupts and makes sure that card is
  enumerated only once.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index a5073a9..6287a43 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		pcie->need_ob_cfg = true;
 	}
 
+	if (of_property_read_bool(np, "slot-pluggable"))
+		pcie->enable_hotplug = true;
+
 	/* PHY use is optional */
 	pcie->phy = devm_phy_get(dev, "pcie-phy");
 	if (IS_ERR(pcie->phy)) {
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 8bd5e54..2b4d830 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -28,6 +28,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/phy/phy.h>
+#include <linux/gpio.h>
 
 #include "pcie-iproc.h"
 
@@ -65,6 +66,17 @@
 #define PCIE_DL_ACTIVE_SHIFT         2
 #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
 
+#define CFG_RC_LTSSM                 0x1cf8
+#define CFG_RC_PHY_CTL               0x1804
+#define CFG_RC_LTSSM_TIMEOUT         1000
+#define CFG_RC_LTSSM_STATE_MASK      0xff
+#define CFG_RC_LTSSM_STATE_L1        0x1
+
+#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
+#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
+#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
+#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
+
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
@@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
 	return 0;
 }
 
+static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)
+{
+	struct pci_bus *bus = pcie->root_bus;
+	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
+
+	/* Clear LTSSM history. */
+	pci_bus_read_config_dword(pcie->root_bus, 0,
+				  CFG_RC_PHY_CTL, &val);
+	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
+				   val | CFG_RC_CLR_RECOV_HIST_MASK |
+				   CFG_RC_CLR_LTSSM_HIST_MASK);
+	/* write back the origional value. */
+	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
+
+	do {
+		pci_bus_read_config_dword(pcie->root_bus, 0,
+					  CFG_RC_LTSSM, &val);
+		/* check link state to see if link moved to L1 state. */
+		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
+		     CFG_RC_LTSSM_STATE_L1)
+			return true;
+		timeout--;
+		usleep_range(500, 1000);
+	} while (timeout);
+
+	return false;
+}
+
+static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
+{
+	struct iproc_pcie *pcie = data;
+	struct pci_bus *bus = pcie->root_bus, *child;
+	bool link_status;
+
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
+
+	link_status = iproc_pci_hp_check_ltssm(pcie);
+
+	if (link_status &&
+	    !iproc_pcie_check_link(pcie) &&
+	    !pcie->ep_is_present) {
+		pci_rescan_bus(bus);
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+		pcie->ep_is_present = true;
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device detected and enumerated>\n");
+	} else if (link_status && pcie->ep_is_present)
+		/*
+		 * ep_is_present makes sure, enumuration done only once.
+		 * So it can handle spurious intrrupts, and also if we
+		 * get multiple interrupts for all the implemented pins,
+		 * we handle it only once.
+		 */
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device already present>\n");
+	else {
+		iproc_pcie_perst_ctrl(pcie, true);
+		pcie->ep_is_present = false;
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device removed>\n");
+	}
+	return IRQ_HANDLED;
+}
+
+static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
+{
+	struct gpio_descs *hp_gpiod;
+	struct device *dev = pcie->dev;
+	int i;
+
+	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
+	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
+		for (i = 0; i < hp_gpiod->ndescs; ++i) {
+			gpiod_direction_input(hp_gpiod->desc[i]);
+			if (request_threaded_irq(gpiod_to_irq
+						 (hp_gpiod->desc[i]),
+						 NULL, iproc_pci_hotplug_thread,
+						 IRQF_TRIGGER_FALLING,
+						 "PCI-hotplug", pcie))
+				dev_err(dev,
+					"PCI hotplug prsnt: request irq failed\n");
+			}
+	}
+	pcie->ep_is_present = false;
+
+	return 0;
+}
+
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	struct device *dev;
 	int ret;
 	void *sysdata;
-	struct pci_bus *child;
+	struct pci_bus *bus, *child;
 	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	bool link_not_active;
 
 	dev = pcie->dev;
 
@@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
+	if (pcie->enable_hotplug) {
+		ret = iproc_pci_hp_gpio_irq_get(pcie);
+		if (ret < 0)
+			return ret;
+	}
+
 	iproc_pcie_perst_ctrl(pcie, true);
 	iproc_pcie_perst_ctrl(pcie, false);
 
@@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	sysdata = pcie;
 #endif
 
-	ret = iproc_pcie_check_link(pcie);
-	if (ret) {
+	link_not_active = iproc_pcie_check_link(pcie);
+	if (link_not_active && pcie->enable_hotplug) {
+		/*
+		 * When link is not active and PCI hotplug
+		 * is supported, do not turn off phy, let probe
+		 * go ahead.
+		 */
+		dev_err(dev, "no PCIe EP device detected\n");
+		iproc_pcie_perst_ctrl(pcie, true);
+	} else if (link_not_active) {
 		dev_err(dev, "no PCIe EP device detected\n");
 		goto err_power_off_phy;
 	}
@@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		if (iproc_pcie_msi_enable(pcie))
 			dev_info(dev, "not using iProc MSI\n");
 
-	list_splice_init(res, &host->windows);
-	host->busnr = 0;
-	host->dev.parent = dev;
-	host->ops = &iproc_pcie_ops;
-	host->sysdata = sysdata;
-	host->map_irq = pcie->map_irq;
-	host->swizzle_irq = pci_common_swizzle;
+	if (!link_not_active) {
+		list_splice_init(res, &host->windows);
+		host->busnr = 0;
+		host->dev.parent = dev;
+		host->ops = &iproc_pcie_ops;
+		host->sysdata = sysdata;
+		host->map_irq = pcie->map_irq;
+		host->swizzle_irq = pci_common_swizzle;
+
+		ret = pci_scan_root_bus_bridge(host);
+		if (ret < 0) {
+			dev_err(dev, "failed to scan host: %d\n", ret);
+			goto err_power_off_phy;
+		}
 
-	ret = pci_scan_root_bus_bridge(host);
-	if (ret < 0) {
-		dev_err(dev, "failed to scan host: %d\n", ret);
-		goto err_power_off_phy;
+		pci_assign_unassigned_bus_resources(host->bus);
+		pcie->root_bus = host->bus;
+	} else {
+		bus = pci_create_root_bus(dev, 0,
+					  &iproc_pcie_ops, sysdata, res);
+		if (!bus) {
+			dev_err(dev, "unable to create PCI root bus\n");
+			ret = -ENOMEM;
+			goto err_power_off_phy;
+		}
+		pcie->root_bus = bus;
 	}
 
-	pci_assign_unassigned_bus_resources(host->bus);
-
-	pcie->root_bus = host->bus;
-
 	list_for_each_entry(child, &host->bus->children, node)
 		pcie_bus_configure_settings(child);
 
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index a6b55ce..e5d0cd4 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -77,6 +77,10 @@ struct iproc_pcie_ib {
  * @ib: inbound mapping related parameters
  * @ib_map: outbound mapping region related parameters
  *
+ * @enable_hotplug: indicates PCI hotplug feature is enabled
+ * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
+ * indicate whether or not the endpoint device is present
+ *
  * @need_msi_steer: indicates additional configuration of the iProc PCIe
  * controller is required to steer MSI writes to external interrupt controller
  * @msi: MSI data
@@ -104,6 +108,9 @@ struct iproc_pcie {
 	struct iproc_pcie_ib ib;
 	const struct iproc_pcie_ib_map *ib_map;
 
+	bool enable_hotplug;
+	bool ep_is_present;
+
 	bool need_msi_steer;
 	struct iproc_msi *msi;
 };
-- 
1.9.1

[PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Oza Pawandeep]

This patch implements PCI hotplug support for iproc family chipsets.

iproc based SOC (e.g. Stingray) does not have hotplug controller
integrated.
Hence, standard PCI hotplug framework hooks can-not be used.
e.g. controlled power up/down of slot.

The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
PCI present lines are input to GPIOs depending on the type of
connector (x2, x4, x8).

GPIO array needs to be present if hotplug is supported.
HW implementation is SOC/Board specific, and also it depends on how
add-in card is designed
(e.g. how many present pins are implemented).

If x8 card is connected, then it might be possible that all the
3 present pins could go low, or at least one pin goes low.
If x4 card is connected, then it might be possible that 2 present
pins go low, or at least one pin goes low.

The implementation essentially takes care of following:
> Initializing hotplug irq thread.
> Detecting the endpoint device based on link state.
> Handling PERST and detecting the plugged devices.
> Ordered Hot plug-out, where User is expected
  to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> Handling spurious interrupt
> Handling multiple interrupts and makes sure that card is
  enumerated only once.

Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index a5073a9..6287a43 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		pcie->need_ob_cfg = true;
 	}
 
+	if (of_property_read_bool(np, "slot-pluggable"))
+		pcie->enable_hotplug = true;
+
 	/* PHY use is optional */
 	pcie->phy = devm_phy_get(dev, "pcie-phy");
 	if (IS_ERR(pcie->phy)) {
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 8bd5e54..2b4d830 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -28,6 +28,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/phy/phy.h>
+#include <linux/gpio.h>
 
 #include "pcie-iproc.h"
 
@@ -65,6 +66,17 @@
 #define PCIE_DL_ACTIVE_SHIFT         2
 #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
 
+#define CFG_RC_LTSSM                 0x1cf8
+#define CFG_RC_PHY_CTL               0x1804
+#define CFG_RC_LTSSM_TIMEOUT         1000
+#define CFG_RC_LTSSM_STATE_MASK      0xff
+#define CFG_RC_LTSSM_STATE_L1        0x1
+
+#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
+#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
+#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
+#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
+
 #define APB_ERR_EN_SHIFT             0
 #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
 
@@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
 	return 0;
 }
 
+static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)
+{
+	struct pci_bus *bus = pcie->root_bus;
+	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
+
+	/* Clear LTSSM history. */
+	pci_bus_read_config_dword(pcie->root_bus, 0,
+				  CFG_RC_PHY_CTL, &val);
+	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
+				   val | CFG_RC_CLR_RECOV_HIST_MASK |
+				   CFG_RC_CLR_LTSSM_HIST_MASK);
+	/* write back the origional value. */
+	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
+
+	do {
+		pci_bus_read_config_dword(pcie->root_bus, 0,
+					  CFG_RC_LTSSM, &val);
+		/* check link state to see if link moved to L1 state. */
+		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
+		     CFG_RC_LTSSM_STATE_L1)
+			return true;
+		timeout--;
+		usleep_range(500, 1000);
+	} while (timeout);
+
+	return false;
+}
+
+static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
+{
+	struct iproc_pcie *pcie = data;
+	struct pci_bus *bus = pcie->root_bus, *child;
+	bool link_status;
+
+	iproc_pcie_perst_ctrl(pcie, true);
+	iproc_pcie_perst_ctrl(pcie, false);
+
+	link_status = iproc_pci_hp_check_ltssm(pcie);
+
+	if (link_status &&
+	    !iproc_pcie_check_link(pcie) &&
+	    !pcie->ep_is_present) {
+		pci_rescan_bus(bus);
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+		pcie->ep_is_present = true;
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device detected and enumerated>\n");
+	} else if (link_status && pcie->ep_is_present)
+		/*
+		 * ep_is_present makes sure, enumuration done only once.
+		 * So it can handle spurious intrrupts, and also if we
+		 * get multiple interrupts for all the implemented pins,
+		 * we handle it only once.
+		 */
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device already present>\n");
+	else {
+		iproc_pcie_perst_ctrl(pcie, true);
+		pcie->ep_is_present = false;
+		dev_info(pcie->dev,
+			 "PCI Hotplug: <device removed>\n");
+	}
+	return IRQ_HANDLED;
+}
+
+static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
+{
+	struct gpio_descs *hp_gpiod;
+	struct device *dev = pcie->dev;
+	int i;
+
+	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
+	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
+		for (i = 0; i < hp_gpiod->ndescs; ++i) {
+			gpiod_direction_input(hp_gpiod->desc[i]);
+			if (request_threaded_irq(gpiod_to_irq
+						 (hp_gpiod->desc[i]),
+						 NULL, iproc_pci_hotplug_thread,
+						 IRQF_TRIGGER_FALLING,
+						 "PCI-hotplug", pcie))
+				dev_err(dev,
+					"PCI hotplug prsnt: request irq failed\n");
+			}
+	}
+	pcie->ep_is_present = false;
+
+	return 0;
+}
+
 int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	struct device *dev;
 	int ret;
 	void *sysdata;
-	struct pci_bus *child;
+	struct pci_bus *bus, *child;
 	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	bool link_not_active;
 
 	dev = pcie->dev;
 
@@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		goto err_exit_phy;
 	}
 
+	if (pcie->enable_hotplug) {
+		ret = iproc_pci_hp_gpio_irq_get(pcie);
+		if (ret < 0)
+			return ret;
+	}
+
 	iproc_pcie_perst_ctrl(pcie, true);
 	iproc_pcie_perst_ctrl(pcie, false);
 
@@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	sysdata = pcie;
 #endif
 
-	ret = iproc_pcie_check_link(pcie);
-	if (ret) {
+	link_not_active = iproc_pcie_check_link(pcie);
+	if (link_not_active && pcie->enable_hotplug) {
+		/*
+		 * When link is not active and PCI hotplug
+		 * is supported, do not turn off phy, let probe
+		 * go ahead.
+		 */
+		dev_err(dev, "no PCIe EP device detected\n");
+		iproc_pcie_perst_ctrl(pcie, true);
+	} else if (link_not_active) {
 		dev_err(dev, "no PCIe EP device detected\n");
 		goto err_power_off_phy;
 	}
@@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 		if (iproc_pcie_msi_enable(pcie))
 			dev_info(dev, "not using iProc MSI\n");
 
-	list_splice_init(res, &host->windows);
-	host->busnr = 0;
-	host->dev.parent = dev;
-	host->ops = &iproc_pcie_ops;
-	host->sysdata = sysdata;
-	host->map_irq = pcie->map_irq;
-	host->swizzle_irq = pci_common_swizzle;
+	if (!link_not_active) {
+		list_splice_init(res, &host->windows);
+		host->busnr = 0;
+		host->dev.parent = dev;
+		host->ops = &iproc_pcie_ops;
+		host->sysdata = sysdata;
+		host->map_irq = pcie->map_irq;
+		host->swizzle_irq = pci_common_swizzle;
+
+		ret = pci_scan_root_bus_bridge(host);
+		if (ret < 0) {
+			dev_err(dev, "failed to scan host: %d\n", ret);
+			goto err_power_off_phy;
+		}
 
-	ret = pci_scan_root_bus_bridge(host);
-	if (ret < 0) {
-		dev_err(dev, "failed to scan host: %d\n", ret);
-		goto err_power_off_phy;
+		pci_assign_unassigned_bus_resources(host->bus);
+		pcie->root_bus = host->bus;
+	} else {
+		bus = pci_create_root_bus(dev, 0,
+					  &iproc_pcie_ops, sysdata, res);
+		if (!bus) {
+			dev_err(dev, "unable to create PCI root bus\n");
+			ret = -ENOMEM;
+			goto err_power_off_phy;
+		}
+		pcie->root_bus = bus;
 	}
 
-	pci_assign_unassigned_bus_resources(host->bus);
-
-	pcie->root_bus = host->bus;
-
 	list_for_each_entry(child, &host->bus->children, node)
 		pcie_bus_configure_settings(child);
 
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index a6b55ce..e5d0cd4 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -77,6 +77,10 @@ struct iproc_pcie_ib {
  * @ib: inbound mapping related parameters
  * @ib_map: outbound mapping region related parameters
  *
+ * @enable_hotplug: indicates PCI hotplug feature is enabled
+ * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
+ * indicate whether or not the endpoint device is present
+ *
  * @need_msi_steer: indicates additional configuration of the iProc PCIe
  * controller is required to steer MSI writes to external interrupt controller
  * @msi: MSI data
@@ -104,6 +108,9 @@ struct iproc_pcie {
 	struct iproc_pcie_ib ib;
 	const struct iproc_pcie_ib_map *ib_map;
 
+	bool enable_hotplug;
+	bool ep_is_present;
+
 	bool need_msi_steer;
 	struct iproc_msi *msi;
 };
-- 
1.9.1

Re: [PATCH v6 0/3] PCI hotplug feature

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
> These patches bring in PCI hotplug support for iproc family chipsets.
> 
> It includes DT binding documentation and, the implementation in
> iproc pcie RC driver.
> 
> Changes since v5:
> [RESEND]

Not sure what the point of resending this as v6 was, since v5 was
posted 6 minutes earlier and you don't mention any changes in v6?

> Changes since v4:
> Rebased to pci-next
> Added; Acked-by: Rob Herring <robh@kernel.org>
> 
> Changes since v3:
> Resend. just to be in sync previous in-flight patches.
> 
> Changes since v2:
> Addressed Rob Herring's comments.
> Changed subject to "dt-bindings: PCI:..."
> Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
> Rebased the patches on top of Lorenzo's patches.
> 
> Oza Pawandeep (3):
>   dt-bindings: PCI: Add PCI hotplug property
>   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
>   PCI: iproc: Implement PCI hotplug support
> 
>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
>  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
>  drivers/pci/host/pcie-iproc-platform.c             |   3 +
>  drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
>  drivers/pci/host/pcie-iproc.h                      |   7 +
>  5 files changed, 187 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH v6 0/3] PCI hotplug feature

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
> These patches bring in PCI hotplug support for iproc family chipsets.
> 
> It includes DT binding documentation and, the implementation in
> iproc pcie RC driver.
> 
> Changes since v5:
> [RESEND]

Not sure what the point of resending this as v6 was, since v5 was
posted 6 minutes earlier and you don't mention any changes in v6?

> Changes since v4:
> Rebased to pci-next
> Added; Acked-by: Rob Herring <robh@kernel.org>
> 
> Changes since v3:
> Resend. just to be in sync previous in-flight patches.
> 
> Changes since v2:
> Addressed Rob Herring's comments.
> Changed subject to "dt-bindings: PCI:..."
> Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
> Rebased the patches on top of Lorenzo's patches.
> 
> Oza Pawandeep (3):
>   dt-bindings: PCI: Add PCI hotplug property
>   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
>   PCI: iproc: Implement PCI hotplug support
> 
>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
>  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
>  drivers/pci/host/pcie-iproc-platform.c             |   3 +
>  drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
>  drivers/pci/host/pcie-iproc.h                      |   7 +
>  5 files changed, 187 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 0/3] PCI hotplug feature

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
> These patches bring in PCI hotplug support for iproc family chipsets.
> 
> It includes DT binding documentation and, the implementation in
> iproc pcie RC driver.
> 
> Changes since v5:
> [RESEND]

Not sure what the point of resending this as v6 was, since v5 was
posted 6 minutes earlier and you don't mention any changes in v6?

> Changes since v4:
> Rebased to pci-next
> Added; Acked-by: Rob Herring <robh@kernel.org>
> 
> Changes since v3:
> Resend. just to be in sync previous in-flight patches.
> 
> Changes since v2:
> Addressed Rob Herring's comments.
> Changed subject to "dt-bindings: PCI:..."
> Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
> Rebased the patches on top of Lorenzo's patches.
> 
> Oza Pawandeep (3):
>   dt-bindings: PCI: Add PCI hotplug property
>   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
>   PCI: iproc: Implement PCI hotplug support
> 
>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
>  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
>  drivers/pci/host/pcie-iproc-platform.c             |   3 +
>  drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
>  drivers/pci/host/pcie-iproc.h                      |   7 +
>  5 files changed, 187 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

My apologies,
It looked to me that v5 went out twice, might be confusing.

Hence sent out v6.

Regards,
Oza.

On 31-Aug-2017 7:09 PM, "Bjorn Helgaas" <helgaas@kernel.org> wrote:

> On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
> > These patches bring in PCI hotplug support for iproc family chipsets.
> >
> > It includes DT binding documentation and, the implementation in
> > iproc pcie RC driver.
> >
> > Changes since v5:
> > [RESEND]
>
> Not sure what the point of resending this as v6 was, since v5 was
> posted 6 minutes earlier and you don't mention any changes in v6?
>
> > Changes since v4:
> > Rebased to pci-next
> > Added; Acked-by: Rob Herring <robh@kernel.org>
> >
> > Changes since v3:
> > Resend. just to be in sync previous in-flight patches.
> >
> > Changes since v2:
> > Addressed Rob Herring's comments.
> > Changed subject to "dt-bindings: PCI:..."
> > Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
> > Rebased the patches on top of Lorenzo's patches.
> >
> > Oza Pawandeep (3):
> >   dt-bindings: PCI: Add PCI hotplug property
> >   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
> >   PCI: iproc: Implement PCI hotplug support
> >
> >  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
> >  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
> >  drivers/pci/host/pcie-iproc-platform.c             |   3 +
> >  drivers/pci/host/pcie-iproc.c                      | 166
> ++++++++++++++++++---
> >  drivers/pci/host/pcie-iproc.h                      |   7 +
> >  5 files changed, 187 insertions(+), 18 deletions(-)
> >
> > --
> > 1.9.1
> >
>

[-- Attachment #2: Type: text/html, Size: 2436 bytes --]

Re: [PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

On Thu, Aug 31, 2017 at 7:09 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
>> These patches bring in PCI hotplug support for iproc family chipsets.
>>
>> It includes DT binding documentation and, the implementation in
>> iproc pcie RC driver.
>>
>> Changes since v5:
>> [RESEND]
>
> Not sure what the point of resending this as v6 was, since v5 was
> posted 6 minutes earlier and you don't mention any changes in v6?

My apologies,
It looked to me that v5 went out twice, might be confusing.

Hence sent out v6.

Regards,
Oza.
>
>> Changes since v4:
>> Rebased to pci-next
>> Added; Acked-by: Rob Herring <robh@kernel.org>
>>
>> Changes since v3:
>> Resend. just to be in sync previous in-flight patches.
>>
>> Changes since v2:
>> Addressed Rob Herring's comments.
>> Changed subject to "dt-bindings: PCI:..."
>> Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
>> Rebased the patches on top of Lorenzo's patches.
>>
>> Oza Pawandeep (3):
>>   dt-bindings: PCI: Add PCI hotplug property
>>   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
>>   PCI: iproc: Implement PCI hotplug support
>>
>>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
>>  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
>>  drivers/pci/host/pcie-iproc-platform.c             |   3 +
>>  drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
>>  drivers/pci/host/pcie-iproc.h                      |   7 +
>>  5 files changed, 187 insertions(+), 18 deletions(-)
>>
>> --
>> 1.9.1
>>

Re: [PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

On Thu, Aug 31, 2017 at 7:09 PM, Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
>> These patches bring in PCI hotplug support for iproc family chipsets.
>>
>> It includes DT binding documentation and, the implementation in
>> iproc pcie RC driver.
>>
>> Changes since v5:
>> [RESEND]
>
> Not sure what the point of resending this as v6 was, since v5 was
> posted 6 minutes earlier and you don't mention any changes in v6?

My apologies,
It looked to me that v5 went out twice, might be confusing.

Hence sent out v6.

Regards,
Oza.
>
>> Changes since v4:
>> Rebased to pci-next
>> Added; Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>
>> Changes since v3:
>> Resend. just to be in sync previous in-flight patches.
>>
>> Changes since v2:
>> Addressed Rob Herring's comments.
>> Changed subject to "dt-bindings: PCI:..."
>> Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
>> Rebased the patches on top of Lorenzo's patches.
>>
>> Oza Pawandeep (3):
>>   dt-bindings: PCI: Add PCI hotplug property
>>   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
>>   PCI: iproc: Implement PCI hotplug support
>>
>>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
>>  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
>>  drivers/pci/host/pcie-iproc-platform.c             |   3 +
>>  drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
>>  drivers/pci/host/pcie-iproc.h                      |   7 +
>>  5 files changed, 187 insertions(+), 18 deletions(-)
>>
>> --
>> 1.9.1
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 0/3] PCI hotplug feature

[Oza Pawandeep]

On Thu, Aug 31, 2017 at 7:09 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 31, 2017 at 10:20:26AM +0530, Oza Pawandeep wrote:
>> These patches bring in PCI hotplug support for iproc family chipsets.
>>
>> It includes DT binding documentation and, the implementation in
>> iproc pcie RC driver.
>>
>> Changes since v5:
>> [RESEND]
>
> Not sure what the point of resending this as v6 was, since v5 was
> posted 6 minutes earlier and you don't mention any changes in v6?

My apologies,
It looked to me that v5 went out twice, might be confusing.

Hence sent out v6.

Regards,
Oza.
>
>> Changes since v4:
>> Rebased to pci-next
>> Added; Acked-by: Rob Herring <robh@kernel.org>
>>
>> Changes since v3:
>> Resend. just to be in sync previous in-flight patches.
>>
>> Changes since v2:
>> Addressed Rob Herring's comments.
>> Changed subject to "dt-bindings: PCI:..."
>> Made generic PCI hotplug properties 'slot-pluggable' and 'prsnt-gpios'
>> Rebased the patches on top of Lorenzo's patches.
>>
>> Oza Pawandeep (3):
>>   dt-bindings: PCI: Add PCI hotplug property
>>   dt-bindings: PCI iproc: Implement optional property prsnt-gpios
>>   PCI: iproc: Implement PCI hotplug support
>>
>>  .../devicetree/bindings/pci/brcm,iproc-pcie.txt    |  14 ++
>>  Documentation/devicetree/bindings/pci/pci.txt      |  15 ++
>>  drivers/pci/host/pcie-iproc-platform.c             |   3 +
>>  drivers/pci/host/pcie-iproc.c                      | 166 ++++++++++++++++++---
>>  drivers/pci/host/pcie-iproc.h                      |   7 +
>>  5 files changed, 187 insertions(+), 18 deletions(-)
>>
>> --
>> 1.9.1
>>

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> This patch implements PCI hotplug support for iproc family chipsets.
> 
> iproc based SOC (e.g. Stingray) does not have hotplug controller
> integrated.
> Hence, standard PCI hotplug framework hooks can-not be used.
> e.g. controlled power up/down of slot.
> 
> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> PCI present lines are input to GPIOs depending on the type of
> connector (x2, x4, x8).
> 
> GPIO array needs to be present if hotplug is supported.
> HW implementation is SOC/Board specific, and also it depends on how
> add-in card is designed
> (e.g. how many present pins are implemented).
> 
> If x8 card is connected, then it might be possible that all the
> 3 present pins could go low, or at least one pin goes low.
> If x4 card is connected, then it might be possible that 2 present
> pins go low, or at least one pin goes low.
> 
> The implementation essentially takes care of following:
> > Initializing hotplug irq thread.
> > Detecting the endpoint device based on link state.
> > Handling PERST and detecting the plugged devices.
> > Ordered Hot plug-out, where User is expected
>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> > Handling spurious interrupt
> > Handling multiple interrupts and makes sure that card is
>   enumerated only once.

I haven't forgotten this, but I am dragging my feet a little bit.
There is a standard way for hardware to support PCIe hotplug, and it's
hard enough to get the software for that right.  I really, really
don't want to see a bunch of one-off implementations that sort of look
like standard hotplug but not really.

I have a few trivial comments below but haven't really reviewed the
whole thing.

> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index a5073a9..6287a43 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  		pcie->need_ob_cfg = true;
>  	}
>  
> +	if (of_property_read_bool(np, "slot-pluggable"))
> +		pcie->enable_hotplug = true;
> +
>  	/* PHY use is optional */
>  	pcie->phy = devm_phy_get(dev, "pcie-phy");
>  	if (IS_ERR(pcie->phy)) {
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 8bd5e54..2b4d830 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -28,6 +28,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy/phy.h>
> +#include <linux/gpio.h>
>  
>  #include "pcie-iproc.h"
>  
> @@ -65,6 +66,17 @@
>  #define PCIE_DL_ACTIVE_SHIFT         2
>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>  
> +#define CFG_RC_LTSSM                 0x1cf8
> +#define CFG_RC_PHY_CTL               0x1804
> +#define CFG_RC_LTSSM_TIMEOUT         1000
> +#define CFG_RC_LTSSM_STATE_MASK      0xff
> +#define CFG_RC_LTSSM_STATE_L1        0x1
> +
> +#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
> +#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
> +#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
> +#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
> +
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> @@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>  	return 0;
>  }
>  
> +static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)

*_check_*() is a terrible name for a function because it doesn't give
any hint about what a "true" return value means.

This looks sort of like dw_pcie_wait_for_link() and
advk_pcie_wait_for_link().  Please use a name more like them and
structure the code more like them.

> +{
> +	struct pci_bus *bus = pcie->root_bus;
> +	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
> +
> +	/* Clear LTSSM history. */
> +	pci_bus_read_config_dword(pcie->root_bus, 0,
> +				  CFG_RC_PHY_CTL, &val);
> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
> +				   val | CFG_RC_CLR_RECOV_HIST_MASK |
> +				   CFG_RC_CLR_LTSSM_HIST_MASK);
> +	/* write back the origional value. */

s/origional/original/

> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
> +
> +	do {
> +		pci_bus_read_config_dword(pcie->root_bus, 0,
> +					  CFG_RC_LTSSM, &val);
> +		/* check link state to see if link moved to L1 state. */
> +		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
> +		     CFG_RC_LTSSM_STATE_L1)
> +			return true;
> +		timeout--;
> +		usleep_range(500, 1000);
> +	} while (timeout);
> +
> +	return false;
> +}
> +
> +static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
> +{
> +	struct iproc_pcie *pcie = data;

  struct device *dev = pcie->dev;

Then you don't have to repeat "pcie->dev" below.

> +	struct pci_bus *bus = pcie->root_bus, *child;
> +	bool link_status;
> +
> +	iproc_pcie_perst_ctrl(pcie, true);
> +	iproc_pcie_perst_ctrl(pcie, false);
> +
> +	link_status = iproc_pci_hp_check_ltssm(pcie);
> +
> +	if (link_status &&
> +	    !iproc_pcie_check_link(pcie) &&

iproc_pcie_check_link() already exists, but is still a poor name.

Please add some preliminary patches to rename and restructure it along
the lines of the other *_link_up() functions, e.g.,
advk_pcie_link_up(), nwl_pcie_link_up(), spear13xx_pcie_link_up(),
etc.

iproc_pcie_check_link() does a bunch of other stuff that should be
moved to other functions.  Some of it looks similar to the
*_establish_link() functions in other drivers.

> +	    !pcie->ep_is_present) {
> +		pci_rescan_bus(bus);
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +		pcie->ep_is_present = true;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device detected and enumerated>\n");
> +	} else if (link_status && pcie->ep_is_present)
> +		/*
> +		 * ep_is_present makes sure, enumuration done only once.

s/enumuration/enumeration/

> +		 * So it can handle spurious intrrupts, and also if we

s/intrrupts/interrupts/

> +		 * get multiple interrupts for all the implemented pins,
> +		 * we handle it only once.
> +		 */
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device already present>\n");
> +	else {
> +		iproc_pcie_perst_ctrl(pcie, true);
> +		pcie->ep_is_present = false;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device removed>\n");
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
> +{
> +	struct gpio_descs *hp_gpiod;
> +	struct device *dev = pcie->dev;
> +	int i;
> +
> +	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
> +	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
> +		for (i = 0; i < hp_gpiod->ndescs; ++i) {
> +			gpiod_direction_input(hp_gpiod->desc[i]);
> +			if (request_threaded_irq(gpiod_to_irq
> +						 (hp_gpiod->desc[i]),
> +						 NULL, iproc_pci_hotplug_thread,
> +						 IRQF_TRIGGER_FALLING,
> +						 "PCI-hotplug", pcie))
> +				dev_err(dev,
> +					"PCI hotplug prsnt: request irq failed\n");
> +			}
> +	}
> +	pcie->ep_is_present = false;
> +
> +	return 0;
> +}
> +
>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  {
>  	struct device *dev;
>  	int ret;
>  	void *sysdata;
> -	struct pci_bus *child;
> +	struct pci_bus *bus, *child;
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +	bool link_not_active;
>  
>  	dev = pcie->dev;
>  
> @@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		goto err_exit_phy;
>  	}
>  
> +	if (pcie->enable_hotplug) {
> +		ret = iproc_pci_hp_gpio_irq_get(pcie);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> @@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	sysdata = pcie;
>  #endif
>  
> -	ret = iproc_pcie_check_link(pcie);
> -	if (ret) {
> +	link_not_active = iproc_pcie_check_link(pcie);
> +	if (link_not_active && pcie->enable_hotplug) {
> +		/*
> +		 * When link is not active and PCI hotplug
> +		 * is supported, do not turn off phy, let probe
> +		 * go ahead.
> +		 */
> +		dev_err(dev, "no PCIe EP device detected\n");
> +		iproc_pcie_perst_ctrl(pcie, true);
> +	} else if (link_not_active) {
>  		dev_err(dev, "no PCIe EP device detected\n");
>  		goto err_power_off_phy;
>  	}
> @@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		if (iproc_pcie_msi_enable(pcie))
>  			dev_info(dev, "not using iProc MSI\n");
>  
> -	list_splice_init(res, &host->windows);
> -	host->busnr = 0;
> -	host->dev.parent = dev;
> -	host->ops = &iproc_pcie_ops;
> -	host->sysdata = sysdata;
> -	host->map_irq = pcie->map_irq;
> -	host->swizzle_irq = pci_common_swizzle;
> +	if (!link_not_active) {

Double negation.  If you pick a better name, e.g., "link_active", this
will read much better.  But I don't understand why you want to check
whether the link is active here anyway.  pci_scan_root_bus_bridge()
should work fine even if there's no device present below the bridge.
Won't the root port be there always, even if there's no downstream
device?

> +		list_splice_init(res, &host->windows);
> +		host->busnr = 0;
> +		host->dev.parent = dev;
> +		host->ops = &iproc_pcie_ops;
> +		host->sysdata = sysdata;
> +		host->map_irq = pcie->map_irq;
> +		host->swizzle_irq = pci_common_swizzle;
> +
> +		ret = pci_scan_root_bus_bridge(host);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to scan host: %d\n", ret);
> +			goto err_power_off_phy;
> +		}
>  
> -	ret = pci_scan_root_bus_bridge(host);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to scan host: %d\n", ret);
> -		goto err_power_off_phy;
> +		pci_assign_unassigned_bus_resources(host->bus);
> +		pcie->root_bus = host->bus;
> +	} else {
> +		bus = pci_create_root_bus(dev, 0,
> +					  &iproc_pcie_ops, sysdata, res);
> +		if (!bus) {
> +			dev_err(dev, "unable to create PCI root bus\n");
> +			ret = -ENOMEM;
> +			goto err_power_off_phy;
> +		}
> +		pcie->root_bus = bus;
>  	}
>  
> -	pci_assign_unassigned_bus_resources(host->bus);
> -
> -	pcie->root_bus = host->bus;
> -
>  	list_for_each_entry(child, &host->bus->children, node)
>  		pcie_bus_configure_settings(child);
>  
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index a6b55ce..e5d0cd4 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -77,6 +77,10 @@ struct iproc_pcie_ib {
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @enable_hotplug: indicates PCI hotplug feature is enabled
> + * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
> + * indicate whether or not the endpoint device is present
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc PCIe
>   * controller is required to steer MSI writes to external interrupt controller
>   * @msi: MSI data
> @@ -104,6 +108,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
>  
> +	bool enable_hotplug;
> +	bool ep_is_present;

Are you suggesting that only an endpoint can be hotplugged?  You can't
hotplug a switch?

>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };
> -- 
> 1.9.1
> 

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> This patch implements PCI hotplug support for iproc family chipsets.
> 
> iproc based SOC (e.g. Stingray) does not have hotplug controller
> integrated.
> Hence, standard PCI hotplug framework hooks can-not be used.
> e.g. controlled power up/down of slot.
> 
> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> PCI present lines are input to GPIOs depending on the type of
> connector (x2, x4, x8).
> 
> GPIO array needs to be present if hotplug is supported.
> HW implementation is SOC/Board specific, and also it depends on how
> add-in card is designed
> (e.g. how many present pins are implemented).
> 
> If x8 card is connected, then it might be possible that all the
> 3 present pins could go low, or at least one pin goes low.
> If x4 card is connected, then it might be possible that 2 present
> pins go low, or at least one pin goes low.
> 
> The implementation essentially takes care of following:
> > Initializing hotplug irq thread.
> > Detecting the endpoint device based on link state.
> > Handling PERST and detecting the plugged devices.
> > Ordered Hot plug-out, where User is expected
>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> > Handling spurious interrupt
> > Handling multiple interrupts and makes sure that card is
>   enumerated only once.

I haven't forgotten this, but I am dragging my feet a little bit.
There is a standard way for hardware to support PCIe hotplug, and it's
hard enough to get the software for that right.  I really, really
don't want to see a bunch of one-off implementations that sort of look
like standard hotplug but not really.

I have a few trivial comments below but haven't really reviewed the
whole thing.

> Signed-off-by: Oza Pawandeep <oza.oza-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index a5073a9..6287a43 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  		pcie->need_ob_cfg = true;
>  	}
>  
> +	if (of_property_read_bool(np, "slot-pluggable"))
> +		pcie->enable_hotplug = true;
> +
>  	/* PHY use is optional */
>  	pcie->phy = devm_phy_get(dev, "pcie-phy");
>  	if (IS_ERR(pcie->phy)) {
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 8bd5e54..2b4d830 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -28,6 +28,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy/phy.h>
> +#include <linux/gpio.h>
>  
>  #include "pcie-iproc.h"
>  
> @@ -65,6 +66,17 @@
>  #define PCIE_DL_ACTIVE_SHIFT         2
>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>  
> +#define CFG_RC_LTSSM                 0x1cf8
> +#define CFG_RC_PHY_CTL               0x1804
> +#define CFG_RC_LTSSM_TIMEOUT         1000
> +#define CFG_RC_LTSSM_STATE_MASK      0xff
> +#define CFG_RC_LTSSM_STATE_L1        0x1
> +
> +#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
> +#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
> +#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
> +#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
> +
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> @@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>  	return 0;
>  }
>  
> +static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)

*_check_*() is a terrible name for a function because it doesn't give
any hint about what a "true" return value means.

This looks sort of like dw_pcie_wait_for_link() and
advk_pcie_wait_for_link().  Please use a name more like them and
structure the code more like them.

> +{
> +	struct pci_bus *bus = pcie->root_bus;
> +	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
> +
> +	/* Clear LTSSM history. */
> +	pci_bus_read_config_dword(pcie->root_bus, 0,
> +				  CFG_RC_PHY_CTL, &val);
> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
> +				   val | CFG_RC_CLR_RECOV_HIST_MASK |
> +				   CFG_RC_CLR_LTSSM_HIST_MASK);
> +	/* write back the origional value. */

s/origional/original/

> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
> +
> +	do {
> +		pci_bus_read_config_dword(pcie->root_bus, 0,
> +					  CFG_RC_LTSSM, &val);
> +		/* check link state to see if link moved to L1 state. */
> +		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
> +		     CFG_RC_LTSSM_STATE_L1)
> +			return true;
> +		timeout--;
> +		usleep_range(500, 1000);
> +	} while (timeout);
> +
> +	return false;
> +}
> +
> +static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
> +{
> +	struct iproc_pcie *pcie = data;

  struct device *dev = pcie->dev;

Then you don't have to repeat "pcie->dev" below.

> +	struct pci_bus *bus = pcie->root_bus, *child;
> +	bool link_status;
> +
> +	iproc_pcie_perst_ctrl(pcie, true);
> +	iproc_pcie_perst_ctrl(pcie, false);
> +
> +	link_status = iproc_pci_hp_check_ltssm(pcie);
> +
> +	if (link_status &&
> +	    !iproc_pcie_check_link(pcie) &&

iproc_pcie_check_link() already exists, but is still a poor name.

Please add some preliminary patches to rename and restructure it along
the lines of the other *_link_up() functions, e.g.,
advk_pcie_link_up(), nwl_pcie_link_up(), spear13xx_pcie_link_up(),
etc.

iproc_pcie_check_link() does a bunch of other stuff that should be
moved to other functions.  Some of it looks similar to the
*_establish_link() functions in other drivers.

> +	    !pcie->ep_is_present) {
> +		pci_rescan_bus(bus);
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +		pcie->ep_is_present = true;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device detected and enumerated>\n");
> +	} else if (link_status && pcie->ep_is_present)
> +		/*
> +		 * ep_is_present makes sure, enumuration done only once.

s/enumuration/enumeration/

> +		 * So it can handle spurious intrrupts, and also if we

s/intrrupts/interrupts/

> +		 * get multiple interrupts for all the implemented pins,
> +		 * we handle it only once.
> +		 */
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device already present>\n");
> +	else {
> +		iproc_pcie_perst_ctrl(pcie, true);
> +		pcie->ep_is_present = false;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device removed>\n");
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
> +{
> +	struct gpio_descs *hp_gpiod;
> +	struct device *dev = pcie->dev;
> +	int i;
> +
> +	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
> +	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
> +		for (i = 0; i < hp_gpiod->ndescs; ++i) {
> +			gpiod_direction_input(hp_gpiod->desc[i]);
> +			if (request_threaded_irq(gpiod_to_irq
> +						 (hp_gpiod->desc[i]),
> +						 NULL, iproc_pci_hotplug_thread,
> +						 IRQF_TRIGGER_FALLING,
> +						 "PCI-hotplug", pcie))
> +				dev_err(dev,
> +					"PCI hotplug prsnt: request irq failed\n");
> +			}
> +	}
> +	pcie->ep_is_present = false;
> +
> +	return 0;
> +}
> +
>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  {
>  	struct device *dev;
>  	int ret;
>  	void *sysdata;
> -	struct pci_bus *child;
> +	struct pci_bus *bus, *child;
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +	bool link_not_active;
>  
>  	dev = pcie->dev;
>  
> @@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		goto err_exit_phy;
>  	}
>  
> +	if (pcie->enable_hotplug) {
> +		ret = iproc_pci_hp_gpio_irq_get(pcie);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> @@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	sysdata = pcie;
>  #endif
>  
> -	ret = iproc_pcie_check_link(pcie);
> -	if (ret) {
> +	link_not_active = iproc_pcie_check_link(pcie);
> +	if (link_not_active && pcie->enable_hotplug) {
> +		/*
> +		 * When link is not active and PCI hotplug
> +		 * is supported, do not turn off phy, let probe
> +		 * go ahead.
> +		 */
> +		dev_err(dev, "no PCIe EP device detected\n");
> +		iproc_pcie_perst_ctrl(pcie, true);
> +	} else if (link_not_active) {
>  		dev_err(dev, "no PCIe EP device detected\n");
>  		goto err_power_off_phy;
>  	}
> @@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		if (iproc_pcie_msi_enable(pcie))
>  			dev_info(dev, "not using iProc MSI\n");
>  
> -	list_splice_init(res, &host->windows);
> -	host->busnr = 0;
> -	host->dev.parent = dev;
> -	host->ops = &iproc_pcie_ops;
> -	host->sysdata = sysdata;
> -	host->map_irq = pcie->map_irq;
> -	host->swizzle_irq = pci_common_swizzle;
> +	if (!link_not_active) {

Double negation.  If you pick a better name, e.g., "link_active", this
will read much better.  But I don't understand why you want to check
whether the link is active here anyway.  pci_scan_root_bus_bridge()
should work fine even if there's no device present below the bridge.
Won't the root port be there always, even if there's no downstream
device?

> +		list_splice_init(res, &host->windows);
> +		host->busnr = 0;
> +		host->dev.parent = dev;
> +		host->ops = &iproc_pcie_ops;
> +		host->sysdata = sysdata;
> +		host->map_irq = pcie->map_irq;
> +		host->swizzle_irq = pci_common_swizzle;
> +
> +		ret = pci_scan_root_bus_bridge(host);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to scan host: %d\n", ret);
> +			goto err_power_off_phy;
> +		}
>  
> -	ret = pci_scan_root_bus_bridge(host);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to scan host: %d\n", ret);
> -		goto err_power_off_phy;
> +		pci_assign_unassigned_bus_resources(host->bus);
> +		pcie->root_bus = host->bus;
> +	} else {
> +		bus = pci_create_root_bus(dev, 0,
> +					  &iproc_pcie_ops, sysdata, res);
> +		if (!bus) {
> +			dev_err(dev, "unable to create PCI root bus\n");
> +			ret = -ENOMEM;
> +			goto err_power_off_phy;
> +		}
> +		pcie->root_bus = bus;
>  	}
>  
> -	pci_assign_unassigned_bus_resources(host->bus);
> -
> -	pcie->root_bus = host->bus;
> -
>  	list_for_each_entry(child, &host->bus->children, node)
>  		pcie_bus_configure_settings(child);
>  
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index a6b55ce..e5d0cd4 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -77,6 +77,10 @@ struct iproc_pcie_ib {
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @enable_hotplug: indicates PCI hotplug feature is enabled
> + * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
> + * indicate whether or not the endpoint device is present
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc PCIe
>   * controller is required to steer MSI writes to external interrupt controller
>   * @msi: MSI data
> @@ -104,6 +108,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
>  
> +	bool enable_hotplug;
> +	bool ep_is_present;

Are you suggesting that only an endpoint can be hotplugged?  You can't
hotplug a switch?

>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> This patch implements PCI hotplug support for iproc family chipsets.
> 
> iproc based SOC (e.g. Stingray) does not have hotplug controller
> integrated.
> Hence, standard PCI hotplug framework hooks can-not be used.
> e.g. controlled power up/down of slot.
> 
> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> PCI present lines are input to GPIOs depending on the type of
> connector (x2, x4, x8).
> 
> GPIO array needs to be present if hotplug is supported.
> HW implementation is SOC/Board specific, and also it depends on how
> add-in card is designed
> (e.g. how many present pins are implemented).
> 
> If x8 card is connected, then it might be possible that all the
> 3 present pins could go low, or at least one pin goes low.
> If x4 card is connected, then it might be possible that 2 present
> pins go low, or at least one pin goes low.
> 
> The implementation essentially takes care of following:
> > Initializing hotplug irq thread.
> > Detecting the endpoint device based on link state.
> > Handling PERST and detecting the plugged devices.
> > Ordered Hot plug-out, where User is expected
>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> > Handling spurious interrupt
> > Handling multiple interrupts and makes sure that card is
>   enumerated only once.

I haven't forgotten this, but I am dragging my feet a little bit.
There is a standard way for hardware to support PCIe hotplug, and it's
hard enough to get the software for that right.  I really, really
don't want to see a bunch of one-off implementations that sort of look
like standard hotplug but not really.

I have a few trivial comments below but haven't really reviewed the
whole thing.

> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index a5073a9..6287a43 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  		pcie->need_ob_cfg = true;
>  	}
>  
> +	if (of_property_read_bool(np, "slot-pluggable"))
> +		pcie->enable_hotplug = true;
> +
>  	/* PHY use is optional */
>  	pcie->phy = devm_phy_get(dev, "pcie-phy");
>  	if (IS_ERR(pcie->phy)) {
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 8bd5e54..2b4d830 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -28,6 +28,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy/phy.h>
> +#include <linux/gpio.h>
>  
>  #include "pcie-iproc.h"
>  
> @@ -65,6 +66,17 @@
>  #define PCIE_DL_ACTIVE_SHIFT         2
>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>  
> +#define CFG_RC_LTSSM                 0x1cf8
> +#define CFG_RC_PHY_CTL               0x1804
> +#define CFG_RC_LTSSM_TIMEOUT         1000
> +#define CFG_RC_LTSSM_STATE_MASK      0xff
> +#define CFG_RC_LTSSM_STATE_L1        0x1
> +
> +#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
> +#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
> +#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
> +#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
> +
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> @@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>  	return 0;
>  }
>  
> +static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)

*_check_*() is a terrible name for a function because it doesn't give
any hint about what a "true" return value means.

This looks sort of like dw_pcie_wait_for_link() and
advk_pcie_wait_for_link().  Please use a name more like them and
structure the code more like them.

> +{
> +	struct pci_bus *bus = pcie->root_bus;
> +	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
> +
> +	/* Clear LTSSM history. */
> +	pci_bus_read_config_dword(pcie->root_bus, 0,
> +				  CFG_RC_PHY_CTL, &val);
> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
> +				   val | CFG_RC_CLR_RECOV_HIST_MASK |
> +				   CFG_RC_CLR_LTSSM_HIST_MASK);
> +	/* write back the origional value. */

s/origional/original/

> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
> +
> +	do {
> +		pci_bus_read_config_dword(pcie->root_bus, 0,
> +					  CFG_RC_LTSSM, &val);
> +		/* check link state to see if link moved to L1 state. */
> +		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
> +		     CFG_RC_LTSSM_STATE_L1)
> +			return true;
> +		timeout--;
> +		usleep_range(500, 1000);
> +	} while (timeout);
> +
> +	return false;
> +}
> +
> +static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
> +{
> +	struct iproc_pcie *pcie = data;

  struct device *dev = pcie->dev;

Then you don't have to repeat "pcie->dev" below.

> +	struct pci_bus *bus = pcie->root_bus, *child;
> +	bool link_status;
> +
> +	iproc_pcie_perst_ctrl(pcie, true);
> +	iproc_pcie_perst_ctrl(pcie, false);
> +
> +	link_status = iproc_pci_hp_check_ltssm(pcie);
> +
> +	if (link_status &&
> +	    !iproc_pcie_check_link(pcie) &&

iproc_pcie_check_link() already exists, but is still a poor name.

Please add some preliminary patches to rename and restructure it along
the lines of the other *_link_up() functions, e.g.,
advk_pcie_link_up(), nwl_pcie_link_up(), spear13xx_pcie_link_up(),
etc.

iproc_pcie_check_link() does a bunch of other stuff that should be
moved to other functions.  Some of it looks similar to the
*_establish_link() functions in other drivers.

> +	    !pcie->ep_is_present) {
> +		pci_rescan_bus(bus);
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +		pcie->ep_is_present = true;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device detected and enumerated>\n");
> +	} else if (link_status && pcie->ep_is_present)
> +		/*
> +		 * ep_is_present makes sure, enumuration done only once.

s/enumuration/enumeration/

> +		 * So it can handle spurious intrrupts, and also if we

s/intrrupts/interrupts/

> +		 * get multiple interrupts for all the implemented pins,
> +		 * we handle it only once.
> +		 */
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device already present>\n");
> +	else {
> +		iproc_pcie_perst_ctrl(pcie, true);
> +		pcie->ep_is_present = false;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device removed>\n");
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
> +{
> +	struct gpio_descs *hp_gpiod;
> +	struct device *dev = pcie->dev;
> +	int i;
> +
> +	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
> +	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
> +		for (i = 0; i < hp_gpiod->ndescs; ++i) {
> +			gpiod_direction_input(hp_gpiod->desc[i]);
> +			if (request_threaded_irq(gpiod_to_irq
> +						 (hp_gpiod->desc[i]),
> +						 NULL, iproc_pci_hotplug_thread,
> +						 IRQF_TRIGGER_FALLING,
> +						 "PCI-hotplug", pcie))
> +				dev_err(dev,
> +					"PCI hotplug prsnt: request irq failed\n");
> +			}
> +	}
> +	pcie->ep_is_present = false;
> +
> +	return 0;
> +}
> +
>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  {
>  	struct device *dev;
>  	int ret;
>  	void *sysdata;
> -	struct pci_bus *child;
> +	struct pci_bus *bus, *child;
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +	bool link_not_active;
>  
>  	dev = pcie->dev;
>  
> @@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		goto err_exit_phy;
>  	}
>  
> +	if (pcie->enable_hotplug) {
> +		ret = iproc_pci_hp_gpio_irq_get(pcie);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> @@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	sysdata = pcie;
>  #endif
>  
> -	ret = iproc_pcie_check_link(pcie);
> -	if (ret) {
> +	link_not_active = iproc_pcie_check_link(pcie);
> +	if (link_not_active && pcie->enable_hotplug) {
> +		/*
> +		 * When link is not active and PCI hotplug
> +		 * is supported, do not turn off phy, let probe
> +		 * go ahead.
> +		 */
> +		dev_err(dev, "no PCIe EP device detected\n");
> +		iproc_pcie_perst_ctrl(pcie, true);
> +	} else if (link_not_active) {
>  		dev_err(dev, "no PCIe EP device detected\n");
>  		goto err_power_off_phy;
>  	}
> @@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		if (iproc_pcie_msi_enable(pcie))
>  			dev_info(dev, "not using iProc MSI\n");
>  
> -	list_splice_init(res, &host->windows);
> -	host->busnr = 0;
> -	host->dev.parent = dev;
> -	host->ops = &iproc_pcie_ops;
> -	host->sysdata = sysdata;
> -	host->map_irq = pcie->map_irq;
> -	host->swizzle_irq = pci_common_swizzle;
> +	if (!link_not_active) {

Double negation.  If you pick a better name, e.g., "link_active", this
will read much better.  But I don't understand why you want to check
whether the link is active here anyway.  pci_scan_root_bus_bridge()
should work fine even if there's no device present below the bridge.
Won't the root port be there always, even if there's no downstream
device?

> +		list_splice_init(res, &host->windows);
> +		host->busnr = 0;
> +		host->dev.parent = dev;
> +		host->ops = &iproc_pcie_ops;
> +		host->sysdata = sysdata;
> +		host->map_irq = pcie->map_irq;
> +		host->swizzle_irq = pci_common_swizzle;
> +
> +		ret = pci_scan_root_bus_bridge(host);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to scan host: %d\n", ret);
> +			goto err_power_off_phy;
> +		}
>  
> -	ret = pci_scan_root_bus_bridge(host);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to scan host: %d\n", ret);
> -		goto err_power_off_phy;
> +		pci_assign_unassigned_bus_resources(host->bus);
> +		pcie->root_bus = host->bus;
> +	} else {
> +		bus = pci_create_root_bus(dev, 0,
> +					  &iproc_pcie_ops, sysdata, res);
> +		if (!bus) {
> +			dev_err(dev, "unable to create PCI root bus\n");
> +			ret = -ENOMEM;
> +			goto err_power_off_phy;
> +		}
> +		pcie->root_bus = bus;
>  	}
>  
> -	pci_assign_unassigned_bus_resources(host->bus);
> -
> -	pcie->root_bus = host->bus;
> -
>  	list_for_each_entry(child, &host->bus->children, node)
>  		pcie_bus_configure_settings(child);
>  
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index a6b55ce..e5d0cd4 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -77,6 +77,10 @@ struct iproc_pcie_ib {
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @enable_hotplug: indicates PCI hotplug feature is enabled
> + * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
> + * indicate whether or not the endpoint device is present
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc PCIe
>   * controller is required to steer MSI writes to external interrupt controller
>   * @msi: MSI data
> @@ -104,6 +108,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
>  
> +	bool enable_hotplug;
> +	bool ep_is_present;

Are you suggesting that only an endpoint can be hotplugged?  You can't
hotplug a switch?

>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };
> -- 
> 1.9.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> This patch implements PCI hotplug support for iproc family chipsets.
> 
> iproc based SOC (e.g. Stingray) does not have hotplug controller
> integrated.
> Hence, standard PCI hotplug framework hooks can-not be used.
> e.g. controlled power up/down of slot.
> 
> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> PCI present lines are input to GPIOs depending on the type of
> connector (x2, x4, x8).
> 
> GPIO array needs to be present if hotplug is supported.
> HW implementation is SOC/Board specific, and also it depends on how
> add-in card is designed
> (e.g. how many present pins are implemented).
> 
> If x8 card is connected, then it might be possible that all the
> 3 present pins could go low, or at least one pin goes low.
> If x4 card is connected, then it might be possible that 2 present
> pins go low, or at least one pin goes low.
> 
> The implementation essentially takes care of following:
> > Initializing hotplug irq thread.
> > Detecting the endpoint device based on link state.
> > Handling PERST and detecting the plugged devices.
> > Ordered Hot plug-out, where User is expected
>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> > Handling spurious interrupt
> > Handling multiple interrupts and makes sure that card is
>   enumerated only once.

I haven't forgotten this, but I am dragging my feet a little bit.
There is a standard way for hardware to support PCIe hotplug, and it's
hard enough to get the software for that right.  I really, really
don't want to see a bunch of one-off implementations that sort of look
like standard hotplug but not really.

I have a few trivial comments below but haven't really reviewed the
whole thing.

> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index a5073a9..6287a43 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  		pcie->need_ob_cfg = true;
>  	}
>  
> +	if (of_property_read_bool(np, "slot-pluggable"))
> +		pcie->enable_hotplug = true;
> +
>  	/* PHY use is optional */
>  	pcie->phy = devm_phy_get(dev, "pcie-phy");
>  	if (IS_ERR(pcie->phy)) {
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 8bd5e54..2b4d830 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -28,6 +28,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/phy/phy.h>
> +#include <linux/gpio.h>
>  
>  #include "pcie-iproc.h"
>  
> @@ -65,6 +66,17 @@
>  #define PCIE_DL_ACTIVE_SHIFT         2
>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>  
> +#define CFG_RC_LTSSM                 0x1cf8
> +#define CFG_RC_PHY_CTL               0x1804
> +#define CFG_RC_LTSSM_TIMEOUT         1000
> +#define CFG_RC_LTSSM_STATE_MASK      0xff
> +#define CFG_RC_LTSSM_STATE_L1        0x1
> +
> +#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
> +#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
> +#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
> +#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
> +
>  #define APB_ERR_EN_SHIFT             0
>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>  
> @@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>  	return 0;
>  }
>  
> +static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)

*_check_*() is a terrible name for a function because it doesn't give
any hint about what a "true" return value means.

This looks sort of like dw_pcie_wait_for_link() and
advk_pcie_wait_for_link().  Please use a name more like them and
structure the code more like them.

> +{
> +	struct pci_bus *bus = pcie->root_bus;
> +	u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
> +
> +	/* Clear LTSSM history. */
> +	pci_bus_read_config_dword(pcie->root_bus, 0,
> +				  CFG_RC_PHY_CTL, &val);
> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
> +				   val | CFG_RC_CLR_RECOV_HIST_MASK |
> +				   CFG_RC_CLR_LTSSM_HIST_MASK);
> +	/* write back the origional value. */

s/origional/original/

> +	pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
> +
> +	do {
> +		pci_bus_read_config_dword(pcie->root_bus, 0,
> +					  CFG_RC_LTSSM, &val);
> +		/* check link state to see if link moved to L1 state. */
> +		if ((val & CFG_RC_LTSSM_STATE_MASK) ==
> +		     CFG_RC_LTSSM_STATE_L1)
> +			return true;
> +		timeout--;
> +		usleep_range(500, 1000);
> +	} while (timeout);
> +
> +	return false;
> +}
> +
> +static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
> +{
> +	struct iproc_pcie *pcie = data;

  struct device *dev = pcie->dev;

Then you don't have to repeat "pcie->dev" below.

> +	struct pci_bus *bus = pcie->root_bus, *child;
> +	bool link_status;
> +
> +	iproc_pcie_perst_ctrl(pcie, true);
> +	iproc_pcie_perst_ctrl(pcie, false);
> +
> +	link_status = iproc_pci_hp_check_ltssm(pcie);
> +
> +	if (link_status &&
> +	    !iproc_pcie_check_link(pcie) &&

iproc_pcie_check_link() already exists, but is still a poor name.

Please add some preliminary patches to rename and restructure it along
the lines of the other *_link_up() functions, e.g.,
advk_pcie_link_up(), nwl_pcie_link_up(), spear13xx_pcie_link_up(),
etc.

iproc_pcie_check_link() does a bunch of other stuff that should be
moved to other functions.  Some of it looks similar to the
*_establish_link() functions in other drivers.

> +	    !pcie->ep_is_present) {
> +		pci_rescan_bus(bus);
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +		pcie->ep_is_present = true;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device detected and enumerated>\n");
> +	} else if (link_status && pcie->ep_is_present)
> +		/*
> +		 * ep_is_present makes sure, enumuration done only once.

s/enumuration/enumeration/

> +		 * So it can handle spurious intrrupts, and also if we

s/intrrupts/interrupts/

> +		 * get multiple interrupts for all the implemented pins,
> +		 * we handle it only once.
> +		 */
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device already present>\n");
> +	else {
> +		iproc_pcie_perst_ctrl(pcie, true);
> +		pcie->ep_is_present = false;
> +		dev_info(pcie->dev,
> +			 "PCI Hotplug: <device removed>\n");
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
> +{
> +	struct gpio_descs *hp_gpiod;
> +	struct device *dev = pcie->dev;
> +	int i;
> +
> +	hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
> +	if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
> +		for (i = 0; i < hp_gpiod->ndescs; ++i) {
> +			gpiod_direction_input(hp_gpiod->desc[i]);
> +			if (request_threaded_irq(gpiod_to_irq
> +						 (hp_gpiod->desc[i]),
> +						 NULL, iproc_pci_hotplug_thread,
> +						 IRQF_TRIGGER_FALLING,
> +						 "PCI-hotplug", pcie))
> +				dev_err(dev,
> +					"PCI hotplug prsnt: request irq failed\n");
> +			}
> +	}
> +	pcie->ep_is_present = false;
> +
> +	return 0;
> +}
> +
>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  {
>  	struct device *dev;
>  	int ret;
>  	void *sysdata;
> -	struct pci_bus *child;
> +	struct pci_bus *bus, *child;
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +	bool link_not_active;
>  
>  	dev = pcie->dev;
>  
> @@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		goto err_exit_phy;
>  	}
>  
> +	if (pcie->enable_hotplug) {
> +		ret = iproc_pci_hp_gpio_irq_get(pcie);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> @@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	sysdata = pcie;
>  #endif
>  
> -	ret = iproc_pcie_check_link(pcie);
> -	if (ret) {
> +	link_not_active = iproc_pcie_check_link(pcie);
> +	if (link_not_active && pcie->enable_hotplug) {
> +		/*
> +		 * When link is not active and PCI hotplug
> +		 * is supported, do not turn off phy, let probe
> +		 * go ahead.
> +		 */
> +		dev_err(dev, "no PCIe EP device detected\n");
> +		iproc_pcie_perst_ctrl(pcie, true);
> +	} else if (link_not_active) {
>  		dev_err(dev, "no PCIe EP device detected\n");
>  		goto err_power_off_phy;
>  	}
> @@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  		if (iproc_pcie_msi_enable(pcie))
>  			dev_info(dev, "not using iProc MSI\n");
>  
> -	list_splice_init(res, &host->windows);
> -	host->busnr = 0;
> -	host->dev.parent = dev;
> -	host->ops = &iproc_pcie_ops;
> -	host->sysdata = sysdata;
> -	host->map_irq = pcie->map_irq;
> -	host->swizzle_irq = pci_common_swizzle;
> +	if (!link_not_active) {

Double negation.  If you pick a better name, e.g., "link_active", this
will read much better.  But I don't understand why you want to check
whether the link is active here anyway.  pci_scan_root_bus_bridge()
should work fine even if there's no device present below the bridge.
Won't the root port be there always, even if there's no downstream
device?

> +		list_splice_init(res, &host->windows);
> +		host->busnr = 0;
> +		host->dev.parent = dev;
> +		host->ops = &iproc_pcie_ops;
> +		host->sysdata = sysdata;
> +		host->map_irq = pcie->map_irq;
> +		host->swizzle_irq = pci_common_swizzle;
> +
> +		ret = pci_scan_root_bus_bridge(host);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to scan host: %d\n", ret);
> +			goto err_power_off_phy;
> +		}
>  
> -	ret = pci_scan_root_bus_bridge(host);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to scan host: %d\n", ret);
> -		goto err_power_off_phy;
> +		pci_assign_unassigned_bus_resources(host->bus);
> +		pcie->root_bus = host->bus;
> +	} else {
> +		bus = pci_create_root_bus(dev, 0,
> +					  &iproc_pcie_ops, sysdata, res);
> +		if (!bus) {
> +			dev_err(dev, "unable to create PCI root bus\n");
> +			ret = -ENOMEM;
> +			goto err_power_off_phy;
> +		}
> +		pcie->root_bus = bus;
>  	}
>  
> -	pci_assign_unassigned_bus_resources(host->bus);
> -
> -	pcie->root_bus = host->bus;
> -
>  	list_for_each_entry(child, &host->bus->children, node)
>  		pcie_bus_configure_settings(child);
>  
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index a6b55ce..e5d0cd4 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -77,6 +77,10 @@ struct iproc_pcie_ib {
>   * @ib: inbound mapping related parameters
>   * @ib_map: outbound mapping region related parameters
>   *
> + * @enable_hotplug: indicates PCI hotplug feature is enabled
> + * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
> + * indicate whether or not the endpoint device is present
> + *
>   * @need_msi_steer: indicates additional configuration of the iProc PCIe
>   * controller is required to steer MSI writes to external interrupt controller
>   * @msi: MSI data
> @@ -104,6 +108,9 @@ struct iproc_pcie {
>  	struct iproc_pcie_ib ib;
>  	const struct iproc_pcie_ib_map *ib_map;
>  
> +	bool enable_hotplug;
> +	bool ep_is_present;

Are you suggesting that only an endpoint can be hotplugged?  You can't
hotplug a switch?

>  	bool need_msi_steer;
>  	struct iproc_msi *msi;
>  };
> -- 
> 1.9.1
> 

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Oza Pawandeep]

On Fri, Oct 6, 2017 at 5:07 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
>> This patch implements PCI hotplug support for iproc family chipsets.
>>
>> iproc based SOC (e.g. Stingray) does not have hotplug controller
>> integrated.
>> Hence, standard PCI hotplug framework hooks can-not be used.
>> e.g. controlled power up/down of slot.
>>
>> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
>> PCI present lines are input to GPIOs depending on the type of
>> connector (x2, x4, x8).
>>
>> GPIO array needs to be present if hotplug is supported.
>> HW implementation is SOC/Board specific, and also it depends on how
>> add-in card is designed
>> (e.g. how many present pins are implemented).
>>
>> If x8 card is connected, then it might be possible that all the
>> 3 present pins could go low, or at least one pin goes low.
>> If x4 card is connected, then it might be possible that 2 present
>> pins go low, or at least one pin goes low.
>>
>> The implementation essentially takes care of following:
>> > Initializing hotplug irq thread.
>> > Detecting the endpoint device based on link state.
>> > Handling PERST and detecting the plugged devices.
>> > Ordered Hot plug-out, where User is expected
>>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
>> > Handling spurious interrupt
>> > Handling multiple interrupts and makes sure that card is
>>   enumerated only once.
>
> I haven't forgotten this, but I am dragging my feet a little bit.
> There is a standard way for hardware to support PCIe hotplug, and it's
> hard enough to get the software for that right.  I really, really
> don't want to see a bunch of one-off implementations that sort of look
> like standard hotplug but not really.
>
> I have a few trivial comments below but haven't really reviewed the
> whole thing.
>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
>> index a5073a9..6287a43 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>>               pcie->need_ob_cfg = true;
>>       }
>>
>> +     if (of_property_read_bool(np, "slot-pluggable"))
>> +             pcie->enable_hotplug = true;
>> +
>>       /* PHY use is optional */
>>       pcie->phy = devm_phy_get(dev, "pcie-phy");
>>       if (IS_ERR(pcie->phy)) {
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 8bd5e54..2b4d830 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/phy/phy.h>
>> +#include <linux/gpio.h>
>>
>>  #include "pcie-iproc.h"
>>
>> @@ -65,6 +66,17 @@
>>  #define PCIE_DL_ACTIVE_SHIFT         2
>>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>>
>> +#define CFG_RC_LTSSM                 0x1cf8
>> +#define CFG_RC_PHY_CTL               0x1804
>> +#define CFG_RC_LTSSM_TIMEOUT         1000
>> +#define CFG_RC_LTSSM_STATE_MASK      0xff
>> +#define CFG_RC_LTSSM_STATE_L1        0x1
>> +
>> +#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
>> +#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
>> +#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
>> +#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
>> +
>>  #define APB_ERR_EN_SHIFT             0
>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>
>> @@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>>       return 0;
>>  }
>>
>> +static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)
>
> *_check_*() is a terrible name for a function because it doesn't give
> any hint about what a "true" return value means.
>
> This looks sort of like dw_pcie_wait_for_link() and
> advk_pcie_wait_for_link().  Please use a name more like them and
> structure the code more like them.
>
>> +{
>> +     struct pci_bus *bus = pcie->root_bus;
>> +     u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
>> +
>> +     /* Clear LTSSM history. */
>> +     pci_bus_read_config_dword(pcie->root_bus, 0,
>> +                               CFG_RC_PHY_CTL, &val);
>> +     pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
>> +                                val | CFG_RC_CLR_RECOV_HIST_MASK |
>> +                                CFG_RC_CLR_LTSSM_HIST_MASK);
>> +     /* write back the origional value. */
>
> s/origional/original/
>
>> +     pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
>> +
>> +     do {
>> +             pci_bus_read_config_dword(pcie->root_bus, 0,
>> +                                       CFG_RC_LTSSM, &val);
>> +             /* check link state to see if link moved to L1 state. */
>> +             if ((val & CFG_RC_LTSSM_STATE_MASK) ==
>> +                  CFG_RC_LTSSM_STATE_L1)
>> +                     return true;
>> +             timeout--;
>> +             usleep_range(500, 1000);
>> +     } while (timeout);
>> +
>> +     return false;
>> +}
>> +
>> +static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
>> +{
>> +     struct iproc_pcie *pcie = data;
>
>   struct device *dev = pcie->dev;
>
> Then you don't have to repeat "pcie->dev" below.
>
>> +     struct pci_bus *bus = pcie->root_bus, *child;
>> +     bool link_status;
>> +
>> +     iproc_pcie_perst_ctrl(pcie, true);
>> +     iproc_pcie_perst_ctrl(pcie, false);
>> +
>> +     link_status = iproc_pci_hp_check_ltssm(pcie);
>> +
>> +     if (link_status &&
>> +         !iproc_pcie_check_link(pcie) &&
>
> iproc_pcie_check_link() already exists, but is still a poor name.
>
> Please add some preliminary patches to rename and restructure it along
> the lines of the other *_link_up() functions, e.g.,
> advk_pcie_link_up(), nwl_pcie_link_up(), spear13xx_pcie_link_up(),
> etc.
>
> iproc_pcie_check_link() does a bunch of other stuff that should be
> moved to other functions.  Some of it looks similar to the
> *_establish_link() functions in other drivers.
>
>> +         !pcie->ep_is_present) {
>> +             pci_rescan_bus(bus);
>> +             list_for_each_entry(child, &bus->children, node)
>> +                     pcie_bus_configure_settings(child);
>> +             pcie->ep_is_present = true;
>> +             dev_info(pcie->dev,
>> +                      "PCI Hotplug: <device detected and enumerated>\n");
>> +     } else if (link_status && pcie->ep_is_present)
>> +             /*
>> +              * ep_is_present makes sure, enumuration done only once.
>
> s/enumuration/enumeration/
>
>> +              * So it can handle spurious intrrupts, and also if we
>
> s/intrrupts/interrupts/
>
>> +              * get multiple interrupts for all the implemented pins,
>> +              * we handle it only once.
>> +              */
>> +             dev_info(pcie->dev,
>> +                      "PCI Hotplug: <device already present>\n");
>> +     else {
>> +             iproc_pcie_perst_ctrl(pcie, true);
>> +             pcie->ep_is_present = false;
>> +             dev_info(pcie->dev,
>> +                      "PCI Hotplug: <device removed>\n");
>> +     }
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
>> +{
>> +     struct gpio_descs *hp_gpiod;
>> +     struct device *dev = pcie->dev;
>> +     int i;
>> +
>> +     hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
>> +     if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
>> +             return -EPROBE_DEFER;
>> +
>> +     if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
>> +             for (i = 0; i < hp_gpiod->ndescs; ++i) {
>> +                     gpiod_direction_input(hp_gpiod->desc[i]);
>> +                     if (request_threaded_irq(gpiod_to_irq
>> +                                              (hp_gpiod->desc[i]),
>> +                                              NULL, iproc_pci_hotplug_thread,
>> +                                              IRQF_TRIGGER_FALLING,
>> +                                              "PCI-hotplug", pcie))
>> +                             dev_err(dev,
>> +                                     "PCI hotplug prsnt: request irq failed\n");
>> +                     }
>> +     }
>> +     pcie->ep_is_present = false;
>> +
>> +     return 0;
>> +}
>> +
>>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>  {
>>       struct device *dev;
>>       int ret;
>>       void *sysdata;
>> -     struct pci_bus *child;
>> +     struct pci_bus *bus, *child;
>>       struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
>> +     bool link_not_active;
>>
>>       dev = pcie->dev;
>>
>> @@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>               goto err_exit_phy;
>>       }
>>
>> +     if (pcie->enable_hotplug) {
>> +             ret = iproc_pci_hp_gpio_irq_get(pcie);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }
>> +
>>       iproc_pcie_perst_ctrl(pcie, true);
>>       iproc_pcie_perst_ctrl(pcie, false);
>>
>> @@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>       sysdata = pcie;
>>  #endif
>>
>> -     ret = iproc_pcie_check_link(pcie);
>> -     if (ret) {
>> +     link_not_active = iproc_pcie_check_link(pcie);
>> +     if (link_not_active && pcie->enable_hotplug) {
>> +             /*
>> +              * When link is not active and PCI hotplug
>> +              * is supported, do not turn off phy, let probe
>> +              * go ahead.
>> +              */
>> +             dev_err(dev, "no PCIe EP device detected\n");
>> +             iproc_pcie_perst_ctrl(pcie, true);
>> +     } else if (link_not_active) {
>>               dev_err(dev, "no PCIe EP device detected\n");
>>               goto err_power_off_phy;
>>       }
>> @@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>               if (iproc_pcie_msi_enable(pcie))
>>                       dev_info(dev, "not using iProc MSI\n");
>>
>> -     list_splice_init(res, &host->windows);
>> -     host->busnr = 0;
>> -     host->dev.parent = dev;
>> -     host->ops = &iproc_pcie_ops;
>> -     host->sysdata = sysdata;
>> -     host->map_irq = pcie->map_irq;
>> -     host->swizzle_irq = pci_common_swizzle;
>> +     if (!link_not_active) {
>
> Double negation.  If you pick a better name, e.g., "link_active", this
> will read much better.  But I don't understand why you want to check
> whether the link is active here anyway.  pci_scan_root_bus_bridge()
> should work fine even if there's no device present below the bridge.
> Won't the root port be there always, even if there's no downstream
> device?
>

will change all the names and spellings as you are suggesting.

pci_scan_root_bus_bridge crashes when link is not active. when I last
tested, that is what I observed.

because in previous code when we dont support hotplug, we just got to
err, and remove root-bus in case link is not active.
we dont do scan bus.

if fact so far none of the RC driver implements hotplug so this might
not have been exercised.

Ray and Vikram: please help to see these comments implemented.


>> +             list_splice_init(res, &host->windows);
>> +             host->busnr = 0;
>> +             host->dev.parent = dev;
>> +             host->ops = &iproc_pcie_ops;
>> +             host->sysdata = sysdata;
>> +             host->map_irq = pcie->map_irq;
>> +             host->swizzle_irq = pci_common_swizzle;
>> +
>> +             ret = pci_scan_root_bus_bridge(host);
>> +             if (ret < 0) {
>> +                     dev_err(dev, "failed to scan host: %d\n", ret);
>> +                     goto err_power_off_phy;
>> +             }
>>
>> -     ret = pci_scan_root_bus_bridge(host);
>> -     if (ret < 0) {
>> -             dev_err(dev, "failed to scan host: %d\n", ret);
>> -             goto err_power_off_phy;
>> +             pci_assign_unassigned_bus_resources(host->bus);
>> +             pcie->root_bus = host->bus;
>> +     } else {
>> +             bus = pci_create_root_bus(dev, 0,
>> +                                       &iproc_pcie_ops, sysdata, res);
>> +             if (!bus) {
>> +                     dev_err(dev, "unable to create PCI root bus\n");
>> +                     ret = -ENOMEM;
>> +                     goto err_power_off_phy;
>> +             }
>> +             pcie->root_bus = bus;
>>       }
>>
>> -     pci_assign_unassigned_bus_resources(host->bus);
>> -
>> -     pcie->root_bus = host->bus;
>> -
>>       list_for_each_entry(child, &host->bus->children, node)
>>               pcie_bus_configure_settings(child);
>>
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> index a6b55ce..e5d0cd4 100644
>> --- a/drivers/pci/host/pcie-iproc.h
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -77,6 +77,10 @@ struct iproc_pcie_ib {
>>   * @ib: inbound mapping related parameters
>>   * @ib_map: outbound mapping region related parameters
>>   *
>> + * @enable_hotplug: indicates PCI hotplug feature is enabled
>> + * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
>> + * indicate whether or not the endpoint device is present
>> + *
>>   * @need_msi_steer: indicates additional configuration of the iProc PCIe
>>   * controller is required to steer MSI writes to external interrupt controller
>>   * @msi: MSI data
>> @@ -104,6 +108,9 @@ struct iproc_pcie {
>>       struct iproc_pcie_ib ib;
>>       const struct iproc_pcie_ib_map *ib_map;
>>
>> +     bool enable_hotplug;
>> +     bool ep_is_present;
>
> Are you suggesting that only an endpoint can be hotplugged?  You can't
> hotplug a switch?
>

ok, we can change the name, PCI switch also can be hot-plugged.

>>       bool need_msi_steer;
>>       struct iproc_msi *msi;
>>  };
>> --
>> 1.9.1
>>

Regards,
Oza.

[PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Oza Pawandeep]

On Fri, Oct 6, 2017 at 5:07 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
>> This patch implements PCI hotplug support for iproc family chipsets.
>>
>> iproc based SOC (e.g. Stingray) does not have hotplug controller
>> integrated.
>> Hence, standard PCI hotplug framework hooks can-not be used.
>> e.g. controlled power up/down of slot.
>>
>> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
>> PCI present lines are input to GPIOs depending on the type of
>> connector (x2, x4, x8).
>>
>> GPIO array needs to be present if hotplug is supported.
>> HW implementation is SOC/Board specific, and also it depends on how
>> add-in card is designed
>> (e.g. how many present pins are implemented).
>>
>> If x8 card is connected, then it might be possible that all the
>> 3 present pins could go low, or at least one pin goes low.
>> If x4 card is connected, then it might be possible that 2 present
>> pins go low, or at least one pin goes low.
>>
>> The implementation essentially takes care of following:
>> > Initializing hotplug irq thread.
>> > Detecting the endpoint device based on link state.
>> > Handling PERST and detecting the plugged devices.
>> > Ordered Hot plug-out, where User is expected
>>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
>> > Handling spurious interrupt
>> > Handling multiple interrupts and makes sure that card is
>>   enumerated only once.
>
> I haven't forgotten this, but I am dragging my feet a little bit.
> There is a standard way for hardware to support PCIe hotplug, and it's
> hard enough to get the software for that right.  I really, really
> don't want to see a bunch of one-off implementations that sort of look
> like standard hotplug but not really.
>
> I have a few trivial comments below but haven't really reviewed the
> whole thing.
>
>> Signed-off-by: Oza Pawandeep <oza.oza@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
>> index a5073a9..6287a43 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -92,6 +92,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>>               pcie->need_ob_cfg = true;
>>       }
>>
>> +     if (of_property_read_bool(np, "slot-pluggable"))
>> +             pcie->enable_hotplug = true;
>> +
>>       /* PHY use is optional */
>>       pcie->phy = devm_phy_get(dev, "pcie-phy");
>>       if (IS_ERR(pcie->phy)) {
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 8bd5e54..2b4d830 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/phy/phy.h>
>> +#include <linux/gpio.h>
>>
>>  #include "pcie-iproc.h"
>>
>> @@ -65,6 +66,17 @@
>>  #define PCIE_DL_ACTIVE_SHIFT         2
>>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>>
>> +#define CFG_RC_LTSSM                 0x1cf8
>> +#define CFG_RC_PHY_CTL               0x1804
>> +#define CFG_RC_LTSSM_TIMEOUT         1000
>> +#define CFG_RC_LTSSM_STATE_MASK      0xff
>> +#define CFG_RC_LTSSM_STATE_L1        0x1
>> +
>> +#define CFG_RC_CLR_LTSSM_HIST_SHIFT  29
>> +#define CFG_RC_CLR_LTSSM_HIST_MASK   BIT(CFG_RC_CLR_LTSSM_HIST_SHIFT)
>> +#define CFG_RC_CLR_RECOV_HIST_SHIFT  31
>> +#define CFG_RC_CLR_RECOV_HIST_MASK   BIT(CFG_RC_CLR_RECOV_HIST_SHIFT)
>> +
>>  #define APB_ERR_EN_SHIFT             0
>>  #define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
>>
>> @@ -1354,13 +1366,107 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>>       return 0;
>>  }
>>
>> +static bool iproc_pci_hp_check_ltssm(struct iproc_pcie *pcie)
>
> *_check_*() is a terrible name for a function because it doesn't give
> any hint about what a "true" return value means.
>
> This looks sort of like dw_pcie_wait_for_link() and
> advk_pcie_wait_for_link().  Please use a name more like them and
> structure the code more like them.
>
>> +{
>> +     struct pci_bus *bus = pcie->root_bus;
>> +     u32 val, timeout = CFG_RC_LTSSM_TIMEOUT;
>> +
>> +     /* Clear LTSSM history. */
>> +     pci_bus_read_config_dword(pcie->root_bus, 0,
>> +                               CFG_RC_PHY_CTL, &val);
>> +     pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL,
>> +                                val | CFG_RC_CLR_RECOV_HIST_MASK |
>> +                                CFG_RC_CLR_LTSSM_HIST_MASK);
>> +     /* write back the origional value. */
>
> s/origional/original/
>
>> +     pci_bus_write_config_dword(bus, 0, CFG_RC_PHY_CTL, val);
>> +
>> +     do {
>> +             pci_bus_read_config_dword(pcie->root_bus, 0,
>> +                                       CFG_RC_LTSSM, &val);
>> +             /* check link state to see if link moved to L1 state. */
>> +             if ((val & CFG_RC_LTSSM_STATE_MASK) ==
>> +                  CFG_RC_LTSSM_STATE_L1)
>> +                     return true;
>> +             timeout--;
>> +             usleep_range(500, 1000);
>> +     } while (timeout);
>> +
>> +     return false;
>> +}
>> +
>> +static irqreturn_t iproc_pci_hotplug_thread(int irq, void *data)
>> +{
>> +     struct iproc_pcie *pcie = data;
>
>   struct device *dev = pcie->dev;
>
> Then you don't have to repeat "pcie->dev" below.
>
>> +     struct pci_bus *bus = pcie->root_bus, *child;
>> +     bool link_status;
>> +
>> +     iproc_pcie_perst_ctrl(pcie, true);
>> +     iproc_pcie_perst_ctrl(pcie, false);
>> +
>> +     link_status = iproc_pci_hp_check_ltssm(pcie);
>> +
>> +     if (link_status &&
>> +         !iproc_pcie_check_link(pcie) &&
>
> iproc_pcie_check_link() already exists, but is still a poor name.
>
> Please add some preliminary patches to rename and restructure it along
> the lines of the other *_link_up() functions, e.g.,
> advk_pcie_link_up(), nwl_pcie_link_up(), spear13xx_pcie_link_up(),
> etc.
>
> iproc_pcie_check_link() does a bunch of other stuff that should be
> moved to other functions.  Some of it looks similar to the
> *_establish_link() functions in other drivers.
>
>> +         !pcie->ep_is_present) {
>> +             pci_rescan_bus(bus);
>> +             list_for_each_entry(child, &bus->children, node)
>> +                     pcie_bus_configure_settings(child);
>> +             pcie->ep_is_present = true;
>> +             dev_info(pcie->dev,
>> +                      "PCI Hotplug: <device detected and enumerated>\n");
>> +     } else if (link_status && pcie->ep_is_present)
>> +             /*
>> +              * ep_is_present makes sure, enumuration done only once.
>
> s/enumuration/enumeration/
>
>> +              * So it can handle spurious intrrupts, and also if we
>
> s/intrrupts/interrupts/
>
>> +              * get multiple interrupts for all the implemented pins,
>> +              * we handle it only once.
>> +              */
>> +             dev_info(pcie->dev,
>> +                      "PCI Hotplug: <device already present>\n");
>> +     else {
>> +             iproc_pcie_perst_ctrl(pcie, true);
>> +             pcie->ep_is_present = false;
>> +             dev_info(pcie->dev,
>> +                      "PCI Hotplug: <device removed>\n");
>> +     }
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int iproc_pci_hp_gpio_irq_get(struct iproc_pcie *pcie)
>> +{
>> +     struct gpio_descs *hp_gpiod;
>> +     struct device *dev = pcie->dev;
>> +     int i;
>> +
>> +     hp_gpiod = devm_gpiod_get_array(dev, "prsnt", GPIOD_IN);
>> +     if (PTR_ERR(hp_gpiod) == -EPROBE_DEFER)
>> +             return -EPROBE_DEFER;
>> +
>> +     if (!IS_ERR(hp_gpiod) && (hp_gpiod->ndescs > 0)) {
>> +             for (i = 0; i < hp_gpiod->ndescs; ++i) {
>> +                     gpiod_direction_input(hp_gpiod->desc[i]);
>> +                     if (request_threaded_irq(gpiod_to_irq
>> +                                              (hp_gpiod->desc[i]),
>> +                                              NULL, iproc_pci_hotplug_thread,
>> +                                              IRQF_TRIGGER_FALLING,
>> +                                              "PCI-hotplug", pcie))
>> +                             dev_err(dev,
>> +                                     "PCI hotplug prsnt: request irq failed\n");
>> +                     }
>> +     }
>> +     pcie->ep_is_present = false;
>> +
>> +     return 0;
>> +}
>> +
>>  int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>  {
>>       struct device *dev;
>>       int ret;
>>       void *sysdata;
>> -     struct pci_bus *child;
>> +     struct pci_bus *bus, *child;
>>       struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
>> +     bool link_not_active;
>>
>>       dev = pcie->dev;
>>
>> @@ -1386,6 +1492,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>               goto err_exit_phy;
>>       }
>>
>> +     if (pcie->enable_hotplug) {
>> +             ret = iproc_pci_hp_gpio_irq_get(pcie);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }
>> +
>>       iproc_pcie_perst_ctrl(pcie, true);
>>       iproc_pcie_perst_ctrl(pcie, false);
>>
>> @@ -1408,8 +1520,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>       sysdata = pcie;
>>  #endif
>>
>> -     ret = iproc_pcie_check_link(pcie);
>> -     if (ret) {
>> +     link_not_active = iproc_pcie_check_link(pcie);
>> +     if (link_not_active && pcie->enable_hotplug) {
>> +             /*
>> +              * When link is not active and PCI hotplug
>> +              * is supported, do not turn off phy, let probe
>> +              * go ahead.
>> +              */
>> +             dev_err(dev, "no PCIe EP device detected\n");
>> +             iproc_pcie_perst_ctrl(pcie, true);
>> +     } else if (link_not_active) {
>>               dev_err(dev, "no PCIe EP device detected\n");
>>               goto err_power_off_phy;
>>       }
>> @@ -1420,24 +1540,34 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>>               if (iproc_pcie_msi_enable(pcie))
>>                       dev_info(dev, "not using iProc MSI\n");
>>
>> -     list_splice_init(res, &host->windows);
>> -     host->busnr = 0;
>> -     host->dev.parent = dev;
>> -     host->ops = &iproc_pcie_ops;
>> -     host->sysdata = sysdata;
>> -     host->map_irq = pcie->map_irq;
>> -     host->swizzle_irq = pci_common_swizzle;
>> +     if (!link_not_active) {
>
> Double negation.  If you pick a better name, e.g., "link_active", this
> will read much better.  But I don't understand why you want to check
> whether the link is active here anyway.  pci_scan_root_bus_bridge()
> should work fine even if there's no device present below the bridge.
> Won't the root port be there always, even if there's no downstream
> device?
>

will change all the names and spellings as you are suggesting.

pci_scan_root_bus_bridge crashes when link is not active. when I last
tested, that is what I observed.

because in previous code when we dont support hotplug, we just got to
err, and remove root-bus in case link is not active.
we dont do scan bus.

if fact so far none of the RC driver implements hotplug so this might
not have been exercised.

Ray and Vikram: please help to see these comments implemented.


>> +             list_splice_init(res, &host->windows);
>> +             host->busnr = 0;
>> +             host->dev.parent = dev;
>> +             host->ops = &iproc_pcie_ops;
>> +             host->sysdata = sysdata;
>> +             host->map_irq = pcie->map_irq;
>> +             host->swizzle_irq = pci_common_swizzle;
>> +
>> +             ret = pci_scan_root_bus_bridge(host);
>> +             if (ret < 0) {
>> +                     dev_err(dev, "failed to scan host: %d\n", ret);
>> +                     goto err_power_off_phy;
>> +             }
>>
>> -     ret = pci_scan_root_bus_bridge(host);
>> -     if (ret < 0) {
>> -             dev_err(dev, "failed to scan host: %d\n", ret);
>> -             goto err_power_off_phy;
>> +             pci_assign_unassigned_bus_resources(host->bus);
>> +             pcie->root_bus = host->bus;
>> +     } else {
>> +             bus = pci_create_root_bus(dev, 0,
>> +                                       &iproc_pcie_ops, sysdata, res);
>> +             if (!bus) {
>> +                     dev_err(dev, "unable to create PCI root bus\n");
>> +                     ret = -ENOMEM;
>> +                     goto err_power_off_phy;
>> +             }
>> +             pcie->root_bus = bus;
>>       }
>>
>> -     pci_assign_unassigned_bus_resources(host->bus);
>> -
>> -     pcie->root_bus = host->bus;
>> -
>>       list_for_each_entry(child, &host->bus->children, node)
>>               pcie_bus_configure_settings(child);
>>
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> index a6b55ce..e5d0cd4 100644
>> --- a/drivers/pci/host/pcie-iproc.h
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -77,6 +77,10 @@ struct iproc_pcie_ib {
>>   * @ib: inbound mapping related parameters
>>   * @ib_map: outbound mapping region related parameters
>>   *
>> + * @enable_hotplug: indicates PCI hotplug feature is enabled
>> + * @ep_is_present: when PCIe hotplug is enabled, this flag is used to
>> + * indicate whether or not the endpoint device is present
>> + *
>>   * @need_msi_steer: indicates additional configuration of the iProc PCIe
>>   * controller is required to steer MSI writes to external interrupt controller
>>   * @msi: MSI data
>> @@ -104,6 +108,9 @@ struct iproc_pcie {
>>       struct iproc_pcie_ib ib;
>>       const struct iproc_pcie_ib_map *ib_map;
>>
>> +     bool enable_hotplug;
>> +     bool ep_is_present;
>
> Are you suggesting that only an endpoint can be hotplugged?  You can't
> hotplug a switch?
>

ok, we can change the name, PCI switch also can be hot-plugged.

>>       bool need_msi_steer;
>>       struct iproc_msi *msi;
>>  };
>> --
>> 1.9.1
>>

Regards,
Oza.

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 08:03:52PM +0530, Oza Pawandeep wrote:
> On Fri, Oct 6, 2017 at 5:07 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> >> This patch implements PCI hotplug support for iproc family chipsets.
> >>
> >> iproc based SOC (e.g. Stingray) does not have hotplug controller
> >> integrated.
> >> Hence, standard PCI hotplug framework hooks can-not be used.
> >> e.g. controlled power up/down of slot.
> >>
> >> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> >> PCI present lines are input to GPIOs depending on the type of
> >> connector (x2, x4, x8).
> >>
> >> GPIO array needs to be present if hotplug is supported.
> >> HW implementation is SOC/Board specific, and also it depends on how
> >> add-in card is designed
> >> (e.g. how many present pins are implemented).
> >>
> >> If x8 card is connected, then it might be possible that all the
> >> 3 present pins could go low, or at least one pin goes low.
> >> If x4 card is connected, then it might be possible that 2 present
> >> pins go low, or at least one pin goes low.
> >>
> >> The implementation essentially takes care of following:
> >> > Initializing hotplug irq thread.
> >> > Detecting the endpoint device based on link state.
> >> > Handling PERST and detecting the plugged devices.
> >> > Ordered Hot plug-out, where User is expected
> >>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> >> > Handling spurious interrupt
> >> > Handling multiple interrupts and makes sure that card is
> >>   enumerated only once.
> >
> > I haven't forgotten this, but I am dragging my feet a little bit.
> > There is a standard way for hardware to support PCIe hotplug, and it's
> > hard enough to get the software for that right.  I really, really
> > don't want to see a bunch of one-off implementations that sort of look
> > like standard hotplug but not really.
> >
> > I have a few trivial comments below but haven't really reviewed the
> > whole thing.
> ...

> >> -     list_splice_init(res, &host->windows);
> >> -     host->busnr = 0;
> >> -     host->dev.parent = dev;
> >> -     host->ops = &iproc_pcie_ops;
> >> -     host->sysdata = sysdata;
> >> -     host->map_irq = pcie->map_irq;
> >> -     host->swizzle_irq = pci_common_swizzle;
> >> +     if (!link_not_active) {
> >
> > Double negation.  If you pick a better name, e.g., "link_active", this
> > will read much better.  But I don't understand why you want to check
> > whether the link is active here anyway.  pci_scan_root_bus_bridge()
> > should work fine even if there's no device present below the bridge.
> > Won't the root port be there always, even if there's no downstream
> > device?
> ...
> 
> pci_scan_root_bus_bridge crashes when link is not active. when I last
> tested, that is what I observed.

I think that would mean a bug in your config accessors.  The PCI core
enumeration itself doesn't depend on the link being up.  But I don't
see anything obvious in your accessors that looks like an issue.

If you post details about the crash, maybe we can help figure it out.

> because in previous code when we dont support hotplug, we just got to
> err, and remove root-bus in case link is not active.
> we dont do scan bus.
> 
> if fact so far none of the RC driver implements hotplug so this might
> not have been exercised.

If the RC implements PCIe hotplug correctly, the RC driver shouldn't
need to do anything special to support hotplug.  It should just work.
That's why I'm so reluctant to merge stuff like this.  Your hardware
folks are making work for themselves by designing a new scheme, and
that makes work for software folks to support it.  None of that should
be necessary.

> ...
> >> @@ -104,6 +108,9 @@ struct iproc_pcie {
> >>       struct iproc_pcie_ib ib;
> >>       const struct iproc_pcie_ib_map *ib_map;
> >>
> >> +     bool enable_hotplug;
> >> +     bool ep_is_present;
> >
> > Are you suggesting that only an endpoint can be hotplugged?  You can't
> > hotplug a switch?
> >
> 
> ok, we can change the name, PCI switch also can be hot-plugged.

I'm dubious about the need for such a variable in the first place.
It's always going to be unreliable because you might test it after a
device has been hotplugged but before the variable has been updated.

Bjorn

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 08:03:52PM +0530, Oza Pawandeep wrote:
> On Fri, Oct 6, 2017 at 5:07 PM, Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> >> This patch implements PCI hotplug support for iproc family chipsets.
> >>
> >> iproc based SOC (e.g. Stingray) does not have hotplug controller
> >> integrated.
> >> Hence, standard PCI hotplug framework hooks can-not be used.
> >> e.g. controlled power up/down of slot.
> >>
> >> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> >> PCI present lines are input to GPIOs depending on the type of
> >> connector (x2, x4, x8).
> >>
> >> GPIO array needs to be present if hotplug is supported.
> >> HW implementation is SOC/Board specific, and also it depends on how
> >> add-in card is designed
> >> (e.g. how many present pins are implemented).
> >>
> >> If x8 card is connected, then it might be possible that all the
> >> 3 present pins could go low, or at least one pin goes low.
> >> If x4 card is connected, then it might be possible that 2 present
> >> pins go low, or at least one pin goes low.
> >>
> >> The implementation essentially takes care of following:
> >> > Initializing hotplug irq thread.
> >> > Detecting the endpoint device based on link state.
> >> > Handling PERST and detecting the plugged devices.
> >> > Ordered Hot plug-out, where User is expected
> >>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> >> > Handling spurious interrupt
> >> > Handling multiple interrupts and makes sure that card is
> >>   enumerated only once.
> >
> > I haven't forgotten this, but I am dragging my feet a little bit.
> > There is a standard way for hardware to support PCIe hotplug, and it's
> > hard enough to get the software for that right.  I really, really
> > don't want to see a bunch of one-off implementations that sort of look
> > like standard hotplug but not really.
> >
> > I have a few trivial comments below but haven't really reviewed the
> > whole thing.
> ...

> >> -     list_splice_init(res, &host->windows);
> >> -     host->busnr = 0;
> >> -     host->dev.parent = dev;
> >> -     host->ops = &iproc_pcie_ops;
> >> -     host->sysdata = sysdata;
> >> -     host->map_irq = pcie->map_irq;
> >> -     host->swizzle_irq = pci_common_swizzle;
> >> +     if (!link_not_active) {
> >
> > Double negation.  If you pick a better name, e.g., "link_active", this
> > will read much better.  But I don't understand why you want to check
> > whether the link is active here anyway.  pci_scan_root_bus_bridge()
> > should work fine even if there's no device present below the bridge.
> > Won't the root port be there always, even if there's no downstream
> > device?
> ...
> 
> pci_scan_root_bus_bridge crashes when link is not active. when I last
> tested, that is what I observed.

I think that would mean a bug in your config accessors.  The PCI core
enumeration itself doesn't depend on the link being up.  But I don't
see anything obvious in your accessors that looks like an issue.

If you post details about the crash, maybe we can help figure it out.

> because in previous code when we dont support hotplug, we just got to
> err, and remove root-bus in case link is not active.
> we dont do scan bus.
> 
> if fact so far none of the RC driver implements hotplug so this might
> not have been exercised.

If the RC implements PCIe hotplug correctly, the RC driver shouldn't
need to do anything special to support hotplug.  It should just work.
That's why I'm so reluctant to merge stuff like this.  Your hardware
folks are making work for themselves by designing a new scheme, and
that makes work for software folks to support it.  None of that should
be necessary.

> ...
> >> @@ -104,6 +108,9 @@ struct iproc_pcie {
> >>       struct iproc_pcie_ib ib;
> >>       const struct iproc_pcie_ib_map *ib_map;
> >>
> >> +     bool enable_hotplug;
> >> +     bool ep_is_present;
> >
> > Are you suggesting that only an endpoint can be hotplugged?  You can't
> > hotplug a switch?
> >
> 
> ok, we can change the name, PCI switch also can be hot-plugged.

I'm dubious about the need for such a variable in the first place.
It's always going to be unreliable because you might test it after a
device has been hotplugged but before the variable has been updated.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 08:03:52PM +0530, Oza Pawandeep wrote:
> On Fri, Oct 6, 2017 at 5:07 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> >> This patch implements PCI hotplug support for iproc family chipsets.
> >>
> >> iproc based SOC (e.g. Stingray) does not have hotplug controller
> >> integrated.
> >> Hence, standard PCI hotplug framework hooks can-not be used.
> >> e.g. controlled power up/down of slot.
> >>
> >> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> >> PCI present lines are input to GPIOs depending on the type of
> >> connector (x2, x4, x8).
> >>
> >> GPIO array needs to be present if hotplug is supported.
> >> HW implementation is SOC/Board specific, and also it depends on how
> >> add-in card is designed
> >> (e.g. how many present pins are implemented).
> >>
> >> If x8 card is connected, then it might be possible that all the
> >> 3 present pins could go low, or at least one pin goes low.
> >> If x4 card is connected, then it might be possible that 2 present
> >> pins go low, or at least one pin goes low.
> >>
> >> The implementation essentially takes care of following:
> >> > Initializing hotplug irq thread.
> >> > Detecting the endpoint device based on link state.
> >> > Handling PERST and detecting the plugged devices.
> >> > Ordered Hot plug-out, where User is expected
> >>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> >> > Handling spurious interrupt
> >> > Handling multiple interrupts and makes sure that card is
> >>   enumerated only once.
> >
> > I haven't forgotten this, but I am dragging my feet a little bit.
> > There is a standard way for hardware to support PCIe hotplug, and it's
> > hard enough to get the software for that right.  I really, really
> > don't want to see a bunch of one-off implementations that sort of look
> > like standard hotplug but not really.
> >
> > I have a few trivial comments below but haven't really reviewed the
> > whole thing.
> ...

> >> -     list_splice_init(res, &host->windows);
> >> -     host->busnr = 0;
> >> -     host->dev.parent = dev;
> >> -     host->ops = &iproc_pcie_ops;
> >> -     host->sysdata = sysdata;
> >> -     host->map_irq = pcie->map_irq;
> >> -     host->swizzle_irq = pci_common_swizzle;
> >> +     if (!link_not_active) {
> >
> > Double negation.  If you pick a better name, e.g., "link_active", this
> > will read much better.  But I don't understand why you want to check
> > whether the link is active here anyway.  pci_scan_root_bus_bridge()
> > should work fine even if there's no device present below the bridge.
> > Won't the root port be there always, even if there's no downstream
> > device?
> ...
> 
> pci_scan_root_bus_bridge crashes when link is not active. when I last
> tested, that is what I observed.

I think that would mean a bug in your config accessors.  The PCI core
enumeration itself doesn't depend on the link being up.  But I don't
see anything obvious in your accessors that looks like an issue.

If you post details about the crash, maybe we can help figure it out.

> because in previous code when we dont support hotplug, we just got to
> err, and remove root-bus in case link is not active.
> we dont do scan bus.
> 
> if fact so far none of the RC driver implements hotplug so this might
> not have been exercised.

If the RC implements PCIe hotplug correctly, the RC driver shouldn't
need to do anything special to support hotplug.  It should just work.
That's why I'm so reluctant to merge stuff like this.  Your hardware
folks are making work for themselves by designing a new scheme, and
that makes work for software folks to support it.  None of that should
be necessary.

> ...
> >> @@ -104,6 +108,9 @@ struct iproc_pcie {
> >>       struct iproc_pcie_ib ib;
> >>       const struct iproc_pcie_ib_map *ib_map;
> >>
> >> +     bool enable_hotplug;
> >> +     bool ep_is_present;
> >
> > Are you suggesting that only an endpoint can be hotplugged?  You can't
> > hotplug a switch?
> >
> 
> ok, we can change the name, PCI switch also can be hot-plugged.

I'm dubious about the need for such a variable in the first place.
It's always going to be unreliable because you might test it after a
device has been hotplugged but before the variable has been updated.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 3/3] PCI: iproc: Implement PCI hotplug support

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 08:03:52PM +0530, Oza Pawandeep wrote:
> On Fri, Oct 6, 2017 at 5:07 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Aug 31, 2017 at 10:20:29AM +0530, Oza Pawandeep wrote:
> >> This patch implements PCI hotplug support for iproc family chipsets.
> >>
> >> iproc based SOC (e.g. Stingray) does not have hotplug controller
> >> integrated.
> >> Hence, standard PCI hotplug framework hooks can-not be used.
> >> e.g. controlled power up/down of slot.
> >>
> >> The mechanism, for e.g. Stingray has adopted for PCI hotplug is as follows:
> >> PCI present lines are input to GPIOs depending on the type of
> >> connector (x2, x4, x8).
> >>
> >> GPIO array needs to be present if hotplug is supported.
> >> HW implementation is SOC/Board specific, and also it depends on how
> >> add-in card is designed
> >> (e.g. how many present pins are implemented).
> >>
> >> If x8 card is connected, then it might be possible that all the
> >> 3 present pins could go low, or at least one pin goes low.
> >> If x4 card is connected, then it might be possible that 2 present
> >> pins go low, or at least one pin goes low.
> >>
> >> The implementation essentially takes care of following:
> >> > Initializing hotplug irq thread.
> >> > Detecting the endpoint device based on link state.
> >> > Handling PERST and detecting the plugged devices.
> >> > Ordered Hot plug-out, where User is expected
> >>   to write 1 to /sys/bus/pci/devices/<pci_dev>/remove
> >> > Handling spurious interrupt
> >> > Handling multiple interrupts and makes sure that card is
> >>   enumerated only once.
> >
> > I haven't forgotten this, but I am dragging my feet a little bit.
> > There is a standard way for hardware to support PCIe hotplug, and it's
> > hard enough to get the software for that right.  I really, really
> > don't want to see a bunch of one-off implementations that sort of look
> > like standard hotplug but not really.
> >
> > I have a few trivial comments below but haven't really reviewed the
> > whole thing.
> ...

> >> -     list_splice_init(res, &host->windows);
> >> -     host->busnr = 0;
> >> -     host->dev.parent = dev;
> >> -     host->ops = &iproc_pcie_ops;
> >> -     host->sysdata = sysdata;
> >> -     host->map_irq = pcie->map_irq;
> >> -     host->swizzle_irq = pci_common_swizzle;
> >> +     if (!link_not_active) {
> >
> > Double negation.  If you pick a better name, e.g., "link_active", this
> > will read much better.  But I don't understand why you want to check
> > whether the link is active here anyway.  pci_scan_root_bus_bridge()
> > should work fine even if there's no device present below the bridge.
> > Won't the root port be there always, even if there's no downstream
> > device?
> ...
> 
> pci_scan_root_bus_bridge crashes when link is not active. when I last
> tested, that is what I observed.

I think that would mean a bug in your config accessors.  The PCI core
enumeration itself doesn't depend on the link being up.  But I don't
see anything obvious in your accessors that looks like an issue.

If you post details about the crash, maybe we can help figure it out.

> because in previous code when we dont support hotplug, we just got to
> err, and remove root-bus in case link is not active.
> we dont do scan bus.
> 
> if fact so far none of the RC driver implements hotplug so this might
> not have been exercised.

If the RC implements PCIe hotplug correctly, the RC driver shouldn't
need to do anything special to support hotplug.  It should just work.
That's why I'm so reluctant to merge stuff like this.  Your hardware
folks are making work for themselves by designing a new scheme, and
that makes work for software folks to support it.  None of that should
be necessary.

> ...
> >> @@ -104,6 +108,9 @@ struct iproc_pcie {
> >>       struct iproc_pcie_ib ib;
> >>       const struct iproc_pcie_ib_map *ib_map;
> >>
> >> +     bool enable_hotplug;
> >> +     bool ep_is_present;
> >
> > Are you suggesting that only an endpoint can be hotplugged?  You can't
> > hotplug a switch?
> >
> 
> ok, we can change the name, PCI switch also can be hot-plugged.

I'm dubious about the need for such a variable in the first place.
It's always going to be unreliable because you might test it after a
device has been hotplugged but before the variable has been updated.

Bjorn