[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Marek Vasut]

Some boards do not have a PCIe reset GPIO. To avoid probe
failure on these boards, make the reset GPIO optional as
well.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
 drivers/pci/host/pci-imx6.c                        | 29 +++++++++++-----------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index d5d26d4..b7a2279 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -19,9 +19,9 @@ Required properties:
 	to define the mapping of the PCIe interface to interrupt
 	numbers.
 - num-lanes: number of lanes to use
-- reset-gpio: gpio pin number of power good signal
 
 Optional properties for fsl,imx6q-pcie
+- reset-gpio: gpio pin number of power good signal
 - power-on-gpio: gpio pin number of power-enable signal
 - wake-up-gpio: gpio pin number of incoming wakeup signal
 - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index bd70af8..52027ad 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
 			IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
 
-	gpio_set_value(imx6_pcie->reset_gpio, 0);
-	msleep(100);
-	gpio_set_value(imx6_pcie->reset_gpio, 1);
+	/* Some boards don't have PCIe reset GPIO. */
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		gpio_set_value(imx6_pcie->reset_gpio, 0);
+		msleep(100);
+		gpio_set_value(imx6_pcie->reset_gpio, 1);
+	}
 
 	return 0;
 }
@@ -439,17 +442,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 
 	/* Fetch GPIOs */
 	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
-	if (!gpio_is_valid(imx6_pcie->reset_gpio)) {
-		dev_err(&pdev->dev, "no reset-gpio defined\n");
-		ret = -ENODEV;
-	}
-	ret = devm_gpio_request_one(&pdev->dev,
-				imx6_pcie->reset_gpio,
-				GPIOF_OUT_INIT_LOW,
-				"PCIe reset");
-	if (ret) {
-		dev_err(&pdev->dev, "unable to get reset gpio\n");
-		goto err;
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		ret = devm_gpio_request_one(&pdev->dev,
+					imx6_pcie->reset_gpio,
+					GPIOF_OUT_INIT_LOW,
+					"PCIe reset");
+		if (ret) {
+			dev_err(&pdev->dev, "unable to get reset gpio\n");
+			goto err;
+		}
 	}
 
 	imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0);
-- 
1.8.4.3

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Marek Vasut]

From: Tim Harvey <tharvey@gateworks.com>

The IMX6 iATU is used for address translation between the AXI bus
address space and PCI address space.  This is used for type0 and type1
config cycles but is not necessary for outbound io/mem regions.

This patch removes the calls that inappropriately re-configures the ATU
viewport for outbound memory and IO after config cycles and removes them
altogether as they are not necessary.

This resolves issues with PCI devices behind switches and has been tested with
a Gige device behind a PLX PEX860x switch.  More testing is needed for other
configurations.

From: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host/pcie-designware.c | 41 +++-----------------------------------
 1 file changed, 3 insertions(+), 38 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index ed9b11b..01d76ad 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -46,7 +46,6 @@
 #define PCIE_ATU_VIEWPORT		0x900
 #define PCIE_ATU_REGION_INBOUND		(0x1 << 31)
 #define PCIE_ATU_REGION_OUTBOUND	(0x0 << 31)
-#define PCIE_ATU_REGION_INDEX1		(0x1 << 0)
 #define PCIE_ATU_REGION_INDEX0		(0x0 << 0)
 #define PCIE_ATU_CR1			0x904
 #define PCIE_ATU_TYPE_MEM		(0x0 << 0)
@@ -494,8 +493,8 @@ static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
 
 static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
 {
-	/* Program viewport 1 : OUTBOUND : CFG1 */
-	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
+	/* Program viewport 0 : OUTBOUND : CFG1 */
+	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
 			  PCIE_ATU_VIEWPORT);
 	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
 	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
@@ -505,38 +504,8 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
-}
-
-static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
-{
-	/* Program viewport 0 : OUTBOUND : MEM */
-	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
-			  PCIE_ATU_VIEWPORT);
-	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
-	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
-	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
-			  PCIE_ATU_LIMIT);
-	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
-	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
-			  PCIE_ATU_UPPER_TARGET);
-}
-
-static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
-{
-	/* Program viewport 1 : OUTBOUND : IO */
-	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
-			  PCIE_ATU_VIEWPORT);
-	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
+	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
 	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
-	dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1,
-			  PCIE_ATU_LIMIT);
-	dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET);
-	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr),
-			  PCIE_ATU_UPPER_TARGET);
 }
 
 static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
@@ -552,11 +521,9 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 	if (bus->parent->number == pp->root_bus_nr) {
 		dw_pcie_prog_viewport_cfg0(pp, busdev);
 		ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
-		dw_pcie_prog_viewport_mem_outbound(pp);
 	} else {
 		dw_pcie_prog_viewport_cfg1(pp, busdev);
 		ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
-		dw_pcie_prog_viewport_io_outbound(pp);
 	}
 
 	return ret;
@@ -575,11 +542,9 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 	if (bus->parent->number == pp->root_bus_nr) {
 		dw_pcie_prog_viewport_cfg0(pp, busdev);
 		ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
-		dw_pcie_prog_viewport_mem_outbound(pp);
 	} else {
 		dw_pcie_prog_viewport_cfg1(pp, busdev);
 		ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
-		dw_pcie_prog_viewport_io_outbound(pp);
 	}
 
 	return ret;
-- 
1.8.4.3

[PATCH 3/7] PCI: imx6: Fix waiting for link up

[Marek Vasut]

While waiting for the PHY to report the PCIe link is up, we might
hit a situation where the link training is still in progress, while
the PHY already reports the link is up. Add additional check for
this condition.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host/pci-imx6.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 52027ad..4d5be4e 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -48,6 +48,8 @@ struct imx6_pcie {
 #define PL_OFFSET 0x700
 #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
 #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
+#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING	(1 << 29)
+#define PCIE_PHY_DEBUG_R1_XMLH_LINK_UP		(1 << 4)
 
 #define PCIE_PHY_CTRL (PL_OFFSET + 0x114)
 #define PCIE_PHY_CTRL_DATA_LOC 0
@@ -338,10 +340,17 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 {
 	u32 rc, ltssm, rx_valid, temp;
 
-	/* link is debug bit 36, debug register 1 starts@bit 32 */
-	rc = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1) & (0x1 << (36 - 32));
-	if (rc)
-		return -EAGAIN;
+	/*
+	 * Test if the PHY reports that the link is up and also that
+	 * the link training finished. It might happen that the PHY
+	 * reports the link is already up, but the link training bit
+	 * is still set, so make sure to check the training is done
+	 * as well here.
+	 */
+	rc = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
+	if ((rc & PCIE_PHY_DEBUG_R1_XMLH_LINK_UP) &&
+	    !(rc & PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING))
+		return 1;
 
 	/*
 	 * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
-- 
1.8.4.3

[PATCH 4/7] PCI: imx6: Split away the PHY reset

[Marek Vasut]

Split the PCIe PHY reset from the link up function to make
the code a little more structured.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host/pci-imx6.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 4d5be4e..a9781e7 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -336,6 +336,23 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 	return;
 }
 
+static void imx6_pcie_reset_phy(struct pcie_port *pp)
+{
+	uint32_t temp;
+
+	pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &temp);
+	temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN |
+		 PHY_RX_OVRD_IN_LO_RX_PLL_EN);
+	pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, temp);
+
+	usleep_range(2000, 3000);
+
+	pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &temp);
+	temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN |
+		  PHY_RX_OVRD_IN_LO_RX_PLL_EN);
+	pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, temp);
+}
+
 static int imx6_pcie_link_up(struct pcie_port *pp)
 {
 	u32 rc, ltssm, rx_valid, temp;
@@ -370,21 +387,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 
 	dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
 
-	pcie_phy_read(pp->dbi_base,
-		PHY_RX_OVRD_IN_LO, &temp);
-	temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN
-		| PHY_RX_OVRD_IN_LO_RX_PLL_EN);
-	pcie_phy_write(pp->dbi_base,
-		PHY_RX_OVRD_IN_LO, temp);
-
-	usleep_range(2000, 3000);
-
-	pcie_phy_read(pp->dbi_base,
-		PHY_RX_OVRD_IN_LO, &temp);
-	temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN
-		| PHY_RX_OVRD_IN_LO_RX_PLL_EN);
-	pcie_phy_write(pp->dbi_base,
-		PHY_RX_OVRD_IN_LO, temp);
+	imx6_pcie_reset_phy(pp);
 
 	return 0;
 }
-- 
1.8.4.3

[PATCH 5/7] PCI: imx6: Split away the link up wait loop

[Marek Vasut]

Split the function that waits for the PCIe link to come up from the
rest if the host init function. We will find this change useful in
the subsequent patch, since this will be called twice then.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host/pci-imx6.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a9781e7..5d85fc2 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -304,6 +304,25 @@ static void imx6_pcie_init_phy(struct pcie_port *pp)
 			IMX6Q_GPR8_TX_SWING_LOW, 127 << 25);
 }
 
+static int imx6_pcie_wait_for_link(struct pcie_port *pp)
+{
+	int count = 200;
+
+	while (!dw_pcie_link_up(pp)) {
+		usleep_range(100, 1000);
+		if (--count)
+			continue;
+
+		dev_err(pp->dev, "phy link never came up\n");
+		dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n",
+			readl(pp->dbi_base + PCIE_PHY_DEBUG_R0),
+			readl(pp->dbi_base + PCIE_PHY_DEBUG_R1));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void imx6_pcie_host_init(struct pcie_port *pp)
 {
 	int count = 0;
@@ -320,18 +339,7 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 			IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
 
-	while (!dw_pcie_link_up(pp)) {
-		usleep_range(100, 1000);
-		count++;
-		if (count >= 200) {
-			dev_err(pp->dev, "phy link never came up\n");
-			dev_dbg(pp->dev,
-				"DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n",
-				readl(pp->dbi_base + PCIE_PHY_DEBUG_R0),
-				readl(pp->dbi_base + PCIE_PHY_DEBUG_R1));
-			break;
-		}
-	}
+	imx6_pcie_wait_for_link(pp);
 
 	return;
 }
-- 
1.8.4.3

[PATCH 6/7] PCI: imx6: Fix link start operation

[Marek Vasut]

This patch first forces the link into Gen1 mode before starting up
the link and only after the link is up requests the link to start
negotiating possible Gen2 mode operation. This is because without
such sequence, some PCIe switches are not detected at all.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host/pci-imx6.c | 78 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 72 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 5d85fc2..ea1a222 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -44,6 +44,12 @@ struct imx6_pcie {
 	void __iomem		*mem_base;
 };
 
+/* PCIe Root Complex registers (memory-mapped) */
+#define PCIE_RC_LCR				0x7c
+#define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1	0x1
+#define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2	0x2
+#define PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK	0xf
+
 /* PCIe Port Logic registers (memory-mapped) */
 #define PL_OFFSET 0x700
 #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
@@ -61,6 +67,9 @@ struct imx6_pcie {
 #define PCIE_PHY_STAT (PL_OFFSET + 0x110)
 #define PCIE_PHY_STAT_ACK_LOC 16
 
+#define PCIE_LINK_WIDTH_SPEED_CONTROL	0x80C
+#define PORT_LOGIC_SPEED_CHANGE		(0x1 << 17)
+
 /* PHY registers (not memory-mapped) */
 #define PCIE_PHY_RX_ASIC_OUT 0x100D
 
@@ -323,11 +332,71 @@ static int imx6_pcie_wait_for_link(struct pcie_port *pp)
 	return 0;
 }
 
-static void imx6_pcie_host_init(struct pcie_port *pp)
+static int imx6_pcie_start_link(struct pcie_port *pp)
 {
-	int count = 0;
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+	uint32_t tmp;
+	int ret, count;
+
+	/*
+	 * Force Gen1 operation when starting the link. In case the link is
+	 * started in Gen2 mode, there is a possibility the devices on the
+	 * bus will not be detected@all. This happens with PCIe switches.
+	 */
+	tmp = readl(pp->dbi_base + PCIE_RC_LCR);
+	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
+	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;
+	writel(tmp, pp->dbi_base + PCIE_RC_LCR);
+
+	/* Start LTSSM. */
+	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+			IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+
+	ret = imx6_pcie_wait_for_link(pp);
+	if (ret)
+		return ret;
+
+	/* Allow Gen2 mode after the link is up. */
+	tmp = readl(pp->dbi_base + PCIE_RC_LCR);
+	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
+	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2;
+	writel(tmp, pp->dbi_base + PCIE_RC_LCR);
+
+	/*
+	 * Start Directed Speed Change so the best possible speed both link
+	 * partners support can be negotiated.
+	 */
+	tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
+	tmp |= PORT_LOGIC_SPEED_CHANGE;
+	writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
+
+	count = 200;
+	while (count--) {
+		tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
+		/* Test if the speed change finished. */
+		if (!(tmp & PORT_LOGIC_SPEED_CHANGE))
+			break;
+		usleep_range(100, 1000);
+	}
+
+	/* Make sure link training is finished as well! */
+	if (count)
+		ret = imx6_pcie_wait_for_link(pp);
+	else
+		ret = -EINVAL;
 
+	if (ret) {
+		dev_err(pp->dev, "Failed to bring link up!\n");
+	} else {
+		tmp = readl(pp->dbi_base + 0x80);
+		dev_err(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf);
+	}
+
+	return ret;
+}
+
+static void imx6_pcie_host_init(struct pcie_port *pp)
+{
 	imx6_pcie_assert_core_reset(pp);
 
 	imx6_pcie_init_phy(pp);
@@ -336,10 +405,7 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 
 	dw_pcie_setup_rc(pp);
 
-	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-			IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
-
-	imx6_pcie_wait_for_link(pp);
+	imx6_pcie_start_link(pp);
 
 	return;
 }
-- 
1.8.4.3

[PATCH 7/7] ARM: dts: imx6q-sabrelite: Enable PCI express

[Marek Vasut]

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 0f003d0..73d7e7e 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -191,6 +191,10 @@
 	};
 };
 
+&pcie {
+	status = "okay";
+};
+
 &sata {
 	status = "okay";
 };
-- 
1.8.4.3

[PATCH 6/7] PCI: imx6: Fix link start operation

[Marek Vasut]

Dear Marek Vasut,

> This patch first forces the link into Gen1 mode before starting up
> the link and only after the link is up requests the link to start
> negotiating possible Gen2 mode operation. This is because without
> such sequence, some PCIe switches are not detected at all.

[...]

> +	/* Make sure link training is finished as well! */
> +	if (count)
> +		ret = imx6_pcie_wait_for_link(pp);
> +	else
> +		ret = -EINVAL;
> 
> +	if (ret) {
> +		dev_err(pp->dev, "Failed to bring link up!\n");
> +	} else {
> +		tmp = readl(pp->dbi_base + 0x80);
> +		dev_err(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf);

Hm meh, this dev_err() shouldn't be here of course. But I'd be interested if you 
people now get stable Gen2 link up properly all the time. Please test. Thanks!

[...]
Best regards,
Marek Vasut

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Pratyush Anand]

Dear Marek Vasut,

Please replace imx6 with designware in subject line  of this patch.

On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> From: Tim Harvey <tharvey@gateworks.com>
> 
> The IMX6 iATU is used for address translation between the AXI bus
> address space and PCI address space.  This is used for type0 and type1
> config cycles but is not necessary for outbound io/mem regions.
> 
> This patch removes the calls that inappropriately re-configures the ATU
> viewport for outbound memory and IO after config cycles and removes them
> altogether as they are not necessary.

Yes, they are not necessary for all the cases where translation is one
to one. So so for sure all the platform till now introduced should
work.
But, what about a platform where memory translation is not one to one?

Existing code should work with all sort of memory translation on a
platform having atleast two viewports capable of programming any type
of outbound transaction.

> 
> This resolves issues with PCI devices behind switches and has been tested with
> a Gige device behind a PLX PEX860x switch.  More testing is needed for other
> configurations.

I do not understand if MX6 has 4 Outbound Viewport then how this patch
helps?
-- PCI devices behind switches would not have been working because
CFG1 transaction would not have been correct.
-- It works with this patch. This patch changes viewport for CFG1 from
1 to 0.
-- Can it be possible that MX6 has some restriction on viewport
programming capability. I mean,like only viewport0 can be programmed
for CFG0/1?

Otherwise I do not find any convincing reason that why this patch
helps in resolving issues with PCI devices behind switches.


Regards
Pratyush

> 
> From: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Frank Li <lznuaa@gmail.com>
> Cc: Harro Haan <hrhaan@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> Cc: Pratyush Anand <pratyush.anand@st.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> Cc: Tim Harvey <tharvey@gateworks.com>
> Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/host/pcie-designware.c | 41 +++-----------------------------------
>  1 file changed, 3 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index ed9b11b..01d76ad 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -46,7 +46,6 @@
>  #define PCIE_ATU_VIEWPORT		0x900
>  #define PCIE_ATU_REGION_INBOUND		(0x1 << 31)
>  #define PCIE_ATU_REGION_OUTBOUND	(0x0 << 31)
> -#define PCIE_ATU_REGION_INDEX1		(0x1 << 0)
>  #define PCIE_ATU_REGION_INDEX0		(0x0 << 0)
>  #define PCIE_ATU_CR1			0x904
>  #define PCIE_ATU_TYPE_MEM		(0x0 << 0)
> @@ -494,8 +493,8 @@ static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
>  
>  static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
>  {
> -	/* Program viewport 1 : OUTBOUND : CFG1 */
> -	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
> +	/* Program viewport 0 : OUTBOUND : CFG1 */
> +	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
>  			  PCIE_ATU_VIEWPORT);
>  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
>  	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> @@ -505,38 +504,8 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
>  			  PCIE_ATU_LIMIT);
>  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
>  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> -}
> -
> -static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
> -{
> -	/* Program viewport 0 : OUTBOUND : MEM */
> -	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
> -			  PCIE_ATU_VIEWPORT);
> -	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> -	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> -	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
> -	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
> -	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
> -			  PCIE_ATU_LIMIT);
> -	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
> -	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
> -			  PCIE_ATU_UPPER_TARGET);
> -}
> -
> -static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
> -{
> -	/* Program viewport 1 : OUTBOUND : IO */
> -	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
> -			  PCIE_ATU_VIEWPORT);
> -	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
> +	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
>  	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> -	dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE);
> -	dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE);
> -	dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1,
> -			  PCIE_ATU_LIMIT);
> -	dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET);
> -	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr),
> -			  PCIE_ATU_UPPER_TARGET);
>  }
>  
>  static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> @@ -552,11 +521,9 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  	if (bus->parent->number == pp->root_bus_nr) {
>  		dw_pcie_prog_viewport_cfg0(pp, busdev);
>  		ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
> -		dw_pcie_prog_viewport_mem_outbound(pp);
>  	} else {
>  		dw_pcie_prog_viewport_cfg1(pp, busdev);
>  		ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
> -		dw_pcie_prog_viewport_io_outbound(pp);
>  	}
>  
>  	return ret;
> @@ -575,11 +542,9 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  	if (bus->parent->number == pp->root_bus_nr) {
>  		dw_pcie_prog_viewport_cfg0(pp, busdev);
>  		ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
> -		dw_pcie_prog_viewport_mem_outbound(pp);
>  	} else {
>  		dw_pcie_prog_viewport_cfg1(pp, busdev);
>  		ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
> -		dw_pcie_prog_viewport_io_outbound(pp);
>  	}
>  
>  	return ret;
> -- 
> 1.8.4.3

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Marek Vasut]

Dear Pratyush Anand,

> Dear Marek Vasut,
> 
> Please replace imx6 with designware in subject line  of this patch.

OK, this one has to be fixed anyway. We need to figure out why do we even need 
this one.

> On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > From: Tim Harvey <tharvey@gateworks.com>
> > 
> > The IMX6 iATU is used for address translation between the AXI bus
> > address space and PCI address space.  This is used for type0 and type1
> > config cycles but is not necessary for outbound io/mem regions.
> > 
> > This patch removes the calls that inappropriately re-configures the ATU
> > viewport for outbound memory and IO after config cycles and removes them
> > altogether as they are not necessary.
> 
> Yes, they are not necessary for all the cases where translation is one
> to one. So so for sure all the platform till now introduced should
> work.
> But, what about a platform where memory translation is not one to one?
> 
> Existing code should work with all sort of memory translation on a
> platform having atleast two viewports capable of programming any type
> of outbound transaction.

Full ACK, that's why it's called RFC.

> > This resolves issues with PCI devices behind switches and has been tested
> > with a Gige device behind a PLX PEX860x switch.  More testing is needed
> > for other configurations.
> 
> I do not understand if MX6 has 4 Outbound Viewport then how this patch
> helps?
> -- PCI devices behind switches would not have been working because
> CFG1 transaction would not have been correct.
> -- It works with this patch. This patch changes viewport for CFG1 from
> 1 to 0.
> -- Can it be possible that MX6 has some restriction on viewport
> programming capability. I mean,like only viewport0 can be programmed
> for CFG0/1?

Tim ?

Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2 describe 
the iATU configuration on MX6. My understanding from this description is that 
the MX6 has 4 inbound and 4 outbound iATU regions. Am I wrong ?

[1] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf

> Otherwise I do not find any convincing reason that why this patch
> helps in resolving issues with PCI devices behind switches.
> 
> 
> Regards
> Pratyush
> 
> > From: Tim Harvey <tharvey@gateworks.com>
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Frank Li <lznuaa@gmail.com>
> > Cc: Harro Haan <hrhaan@gmail.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> > Cc: Pratyush Anand <pratyush.anand@st.com>
> > Cc: Richard Zhu <r65037@freescale.com>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Sean Cross <xobs@kosagi.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> > Cc: Tim Harvey <tharvey@gateworks.com>
> > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > ---
> > 
> >  drivers/pci/host/pcie-designware.c | 41
> >  +++----------------------------------- 1 file changed, 3 insertions(+),
> >  38 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pcie-designware.c
> > b/drivers/pci/host/pcie-designware.c index ed9b11b..01d76ad 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -46,7 +46,6 @@
> > 
> >  #define PCIE_ATU_VIEWPORT		0x900
> >  #define PCIE_ATU_REGION_INBOUND		(0x1 << 31)
> >  #define PCIE_ATU_REGION_OUTBOUND	(0x0 << 31)
> > 
> > -#define PCIE_ATU_REGION_INDEX1		(0x1 << 0)
> > 
> >  #define PCIE_ATU_REGION_INDEX0		(0x0 << 0)
> >  #define PCIE_ATU_CR1			0x904
> >  #define PCIE_ATU_TYPE_MEM		(0x0 << 0)
> > 
> > @@ -494,8 +493,8 @@ static void dw_pcie_prog_viewport_cfg0(struct
> > pcie_port *pp, u32 busdev)
> > 
> >  static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
> >  {
> > 
> > -	/* Program viewport 1 : OUTBOUND : CFG1 */
> > -	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> > PCIE_ATU_REGION_INDEX1, +	/* Program viewport 0 : OUTBOUND : CFG1 */
> > +	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> > PCIE_ATU_REGION_INDEX0,
> > 
> >  			  PCIE_ATU_VIEWPORT);
> >  	
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > 
> > @@ -505,38 +504,8 @@ static void dw_pcie_prog_viewport_cfg1(struct
> > pcie_port *pp, u32 busdev)
> > 
> >  			  PCIE_ATU_LIMIT);
> >  	
> >  	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
> >  	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> > 
> > -}
> > -
> > -static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
> > -{
> > -	/* Program viewport 0 : OUTBOUND : MEM */
> > -	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> > PCIE_ATU_REGION_INDEX0, -			  PCIE_ATU_VIEWPORT);
> > -	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> > -	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > -	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
> > -			  PCIE_ATU_LIMIT);
> > -	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
> > -	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
> > -			  PCIE_ATU_UPPER_TARGET);
> > -}
> > -
> > -static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
> > -{
> > -	/* Program viewport 1 : OUTBOUND : IO */
> > -	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> > PCIE_ATU_REGION_INDEX1, -			  PCIE_ATU_VIEWPORT);
> > -	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
> > +	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> > 
> >  	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > 
> > -	dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE);
> > -	dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE);
> > -	dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1,
> > -			  PCIE_ATU_LIMIT);
> > -	dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET);
> > -	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr),
> > -			  PCIE_ATU_UPPER_TARGET);
> > 
> >  }
> >  
> >  static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus
> >  *bus,
> > 
> > @@ -552,11 +521,9 @@ static int dw_pcie_rd_other_conf(struct pcie_port
> > *pp, struct pci_bus *bus,
> > 
> >  	if (bus->parent->number == pp->root_bus_nr) {
> >  	
> >  		dw_pcie_prog_viewport_cfg0(pp, busdev);
> >  		ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
> > 
> > -		dw_pcie_prog_viewport_mem_outbound(pp);
> > 
> >  	} else {
> >  	
> >  		dw_pcie_prog_viewport_cfg1(pp, busdev);
> >  		ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
> > 
> > -		dw_pcie_prog_viewport_io_outbound(pp);
> > 
> >  	}
> >  	
> >  	return ret;
> > 
> > @@ -575,11 +542,9 @@ static int dw_pcie_wr_other_conf(struct pcie_port
> > *pp, struct pci_bus *bus,
> > 
> >  	if (bus->parent->number == pp->root_bus_nr) {
> >  	
> >  		dw_pcie_prog_viewport_cfg0(pp, busdev);
> >  		ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
> > 
> > -		dw_pcie_prog_viewport_mem_outbound(pp);
> > 
> >  	} else {
> >  	
> >  		dw_pcie_prog_viewport_cfg1(pp, busdev);
> >  		ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
> > 
> > -		dw_pcie_prog_viewport_io_outbound(pp);
> > 
> >  	}
> >  	
> >  	return ret;

[PATCH 4/7] PCI: imx6: Split away the PHY reset

[Harro Haan]

On 26 November 2013 22:10, Marek Vasut <marex@denx.de> wrote:
> Split the PCIe PHY reset from the link up function to make
> the code a little more structured.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Frank Li <lznuaa@gmail.com>
> Cc: Harro Haan <hrhaan@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> Cc: Pratyush Anand <pratyush.anand@st.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> Cc: Tim Harvey <tharvey@gateworks.com>
> Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/host/pci-imx6.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 4d5be4e..a9781e7 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -336,6 +336,23 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
>         return;
>  }
>
> +static void imx6_pcie_reset_phy(struct pcie_port *pp)
> +{
> +       uint32_t temp;
> +
> +       pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &temp);
> +       temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN |
> +                PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> +       pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, temp);
> +
> +       usleep_range(2000, 3000);
> +
> +       pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &temp);
> +       temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN |
> +                 PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> +       pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, temp);
> +}
> +
>  static int imx6_pcie_link_up(struct pcie_port *pp)
>  {
>         u32 rc, ltssm, rx_valid, temp;
> @@ -370,21 +387,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
>
>         dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
>
> -       pcie_phy_read(pp->dbi_base,
> -               PHY_RX_OVRD_IN_LO, &temp);
> -       temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN
> -               | PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> -       pcie_phy_write(pp->dbi_base,
> -               PHY_RX_OVRD_IN_LO, temp);
> -
> -       usleep_range(2000, 3000);
> -
> -       pcie_phy_read(pp->dbi_base,
> -               PHY_RX_OVRD_IN_LO, &temp);
> -       temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN
> -               | PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> -       pcie_phy_write(pp->dbi_base,
> -               PHY_RX_OVRD_IN_LO, temp);
> +       imx6_pcie_reset_phy(pp);
>
>         return 0;
>  }
> --
> 1.8.4.3
>

Thanks Marek,

A minor:
drivers/pci/host/pci-imx6.c: In function 'imx6_pcie_link_up':
drivers/pci/host/pci-imx6.c:353:27: warning: unused variable 'temp'
[-Wunused-variable]

See the following mail for more info which tests I did with patch 1/7,
3/7, 4/7, 5/7 and 6/7:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/214640.html

Regards,

Harro

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Jingoo Han]

On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > From: Tim Harvey <tharvey@gateworks.com>
> > >
> > > The IMX6 iATU is used for address translation between the AXI bus
> > > address space and PCI address space.  This is used for type0 and type1
> > > config cycles but is not necessary for outbound io/mem regions.
> > >
> > > This patch removes the calls that inappropriately re-configures the ATU
> > > viewport for outbound memory and IO after config cycles and removes them
> > > altogether as they are not necessary.
> >
> > Yes, they are not necessary for all the cases where translation is one
> > to one. So so for sure all the platform till now introduced should
> > work.
> > But, what about a platform where memory translation is not one to one?
> >
> > Existing code should work with all sort of memory translation on a
> > platform having atleast two viewports capable of programming any type
> > of outbound transaction.
> 
> Full ACK, that's why it's called RFC.
> 
> > > This resolves issues with PCI devices behind switches and has been tested
> > > with a Gige device behind a PLX PEX860x switch.  More testing is needed
> > > for other configurations.
> >
> > I do not understand if MX6 has 4 Outbound Viewport then how this patch
> > helps?
> > -- PCI devices behind switches would not have been working because
> > CFG1 transaction would not have been correct.
> > -- It works with this patch. This patch changes viewport for CFG1 from
> > 1 to 0.
> > -- Can it be possible that MX6 has some restriction on viewport
> > programming capability. I mean,like only viewport0 can be programmed
> > for CFG0/1?
> 
> Tim ?
> 
> Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2 describe
> the iATU configuration on MX6. My understanding from this description is that
> the MX6 has 4 inbound and 4 outbound iATU regions. Am I wrong ?

The numbers of inbound and outbound are hardware-configurable features.
Thus, these numbers  can be confirmed by Freescale  hardware engineers.
How about asking Freescale hardware engineers, if possible?

Best regards,
Jingoo Han
> 
> [1] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf
> 
> > Otherwise I do not find any convincing reason that why this patch
> > helps in resolving issues with PCI devices behind switches.

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Marek Vasut]

Dear Jingoo Han,

> On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> > Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > 
> > > > The IMX6 iATU is used for address translation between the AXI bus
> > > > address space and PCI address space.  This is used for type0 and
> > > > type1 config cycles but is not necessary for outbound io/mem
> > > > regions.
> > > > 
> > > > This patch removes the calls that inappropriately re-configures the
> > > > ATU viewport for outbound memory and IO after config cycles and
> > > > removes them altogether as they are not necessary.
> > > 
> > > Yes, they are not necessary for all the cases where translation is one
> > > to one. So so for sure all the platform till now introduced should
> > > work.
> > > But, what about a platform where memory translation is not one to one?
> > > 
> > > Existing code should work with all sort of memory translation on a
> > > platform having atleast two viewports capable of programming any type
> > > of outbound transaction.
> > 
> > Full ACK, that's why it's called RFC.
> > 
> > > > This resolves issues with PCI devices behind switches and has been
> > > > tested with a Gige device behind a PLX PEX860x switch.  More testing
> > > > is needed for other configurations.
> > > 
> > > I do not understand if MX6 has 4 Outbound Viewport then how this patch
> > > helps?
> > > -- PCI devices behind switches would not have been working because
> > > CFG1 transaction would not have been correct.
> > > -- It works with this patch. This patch changes viewport for CFG1 from
> > > 1 to 0.
> > > -- Can it be possible that MX6 has some restriction on viewport
> > > programming capability. I mean,like only viewport0 can be programmed
> > > for CFG0/1?
> > 
> > Tim ?
> > 
> > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2
> > describe the iATU configuration on MX6. My understanding from this
> > description is that the MX6 has 4 inbound and 4 outbound iATU regions.
> > Am I wrong ?
> 
> The numbers of inbound and outbound are hardware-configurable features.
> Thus, these numbers  can be confirmed by Freescale  hardware engineers.
> How about asking Freescale hardware engineers, if possible?

Richard is on the CC. Richard, can you please confirm those?

Thanks!

> Best regards,
> Jingoo Han
> 
> > [1] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf
> > 
> > > Otherwise I do not find any convincing reason that why this patch
> > > helps in resolving issues with PCI devices behind switches.

Best regards,
Marek Vasut

[PATCH 4/7] PCI: imx6: Split away the PHY reset

[Marek Vasut]

Dear Harro Haan,

> On 26 November 2013 22:10, Marek Vasut <marex@denx.de> wrote:
> > Split the PCIe PHY reset from the link up function to make
> > the code a little more structured.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Frank Li <lznuaa@gmail.com>
> > Cc: Harro Haan <hrhaan@gmail.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> > Cc: Pratyush Anand <pratyush.anand@st.com>
> > Cc: Richard Zhu <r65037@freescale.com>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Sean Cross <xobs@kosagi.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> > Cc: Tim Harvey <tharvey@gateworks.com>
> > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > ---
> > 
> >  drivers/pci/host/pci-imx6.c | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> > index 4d5be4e..a9781e7 100644
> > --- a/drivers/pci/host/pci-imx6.c
> > +++ b/drivers/pci/host/pci-imx6.c
> > @@ -336,6 +336,23 @@ static void imx6_pcie_host_init(struct pcie_port
> > *pp)
> > 
> >         return;
> >  
> >  }
> > 
> > +static void imx6_pcie_reset_phy(struct pcie_port *pp)
> > +{
> > +       uint32_t temp;
> > +
> > +       pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &temp);
> > +       temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN |
> > +                PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> > +       pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, temp);
> > +
> > +       usleep_range(2000, 3000);
> > +
> > +       pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &temp);
> > +       temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN |
> > +                 PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> > +       pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, temp);
> > +}
> > +
> > 
> >  static int imx6_pcie_link_up(struct pcie_port *pp)
> >  {
> >  
> >         u32 rc, ltssm, rx_valid, temp;
> > 
> > @@ -370,21 +387,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
> > 
> >         dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
> > 
> > -       pcie_phy_read(pp->dbi_base,
> > -               PHY_RX_OVRD_IN_LO, &temp);
> > -       temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN
> > -               | PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> > -       pcie_phy_write(pp->dbi_base,
> > -               PHY_RX_OVRD_IN_LO, temp);
> > -
> > -       usleep_range(2000, 3000);
> > -
> > -       pcie_phy_read(pp->dbi_base,
> > -               PHY_RX_OVRD_IN_LO, &temp);
> > -       temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN
> > -               | PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> > -       pcie_phy_write(pp->dbi_base,
> > -               PHY_RX_OVRD_IN_LO, temp);
> > +       imx6_pcie_reset_phy(pp);
> > 
> >         return 0;
> >  
> >  }
> > 
> > --
> > 1.8.4.3
> 
> Thanks Marek,
> 
> A minor:
> drivers/pci/host/pci-imx6.c: In function 'imx6_pcie_link_up':
> drivers/pci/host/pci-imx6.c:353:27: warning: unused variable 'temp'
> [-Wunused-variable]
> 
> See the following mail for more info which tests I did with patch 1/7,
> 3/7, 4/7, 5/7 and 6/7:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/214640.
> html

Thanks for the find and testing!

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Tim Harvey]

On Wed, Nov 27, 2013 at 12:44 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Pratyush Anand,
>
>> Dear Marek Vasut,
>>
>> Please replace imx6 with designware in subject line  of this patch.
>
> OK, this one has to be fixed anyway. We need to figure out why do we even need
> this one.
>
>> On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
>> > From: Tim Harvey <tharvey@gateworks.com>
>> >
>> > The IMX6 iATU is used for address translation between the AXI bus
>> > address space and PCI address space.  This is used for type0 and type1
>> > config cycles but is not necessary for outbound io/mem regions.
>> >
>> > This patch removes the calls that inappropriately re-configures the ATU
>> > viewport for outbound memory and IO after config cycles and removes them
>> > altogether as they are not necessary.
>>
>> Yes, they are not necessary for all the cases where translation is one
>> to one. So so for sure all the platform till now introduced should
>> work.
>> But, what about a platform where memory translation is not one to one?
>>
>> Existing code should work with all sort of memory translation on a
>> platform having atleast two viewports capable of programming any type
>> of outbound transaction.
>
> Full ACK, that's why it's called RFC.
>
>> > This resolves issues with PCI devices behind switches and has been tested
>> > with a Gige device behind a PLX PEX860x switch.  More testing is needed
>> > for other configurations.
>>
>> I do not understand if MX6 has 4 Outbound Viewport then how this patch
>> helps?
>> -- PCI devices behind switches would not have been working because
>> CFG1 transaction would not have been correct.
>> -- It works with this patch. This patch changes viewport for CFG1 from
>> 1 to 0.
>> -- Can it be possible that MX6 has some restriction on viewport
>> programming capability. I mean,like only viewport0 can be programmed
>> for CFG0/1?
>
> Tim ?
>
> Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2 describe
> the iATU configuration on MX6. My understanding from this description is that
> the MX6 has 4 inbound and 4 outbound iATU regions. Am I wrong ?
>
> [1] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf
>
>> Otherwise I do not find any convincing reason that why this patch
>> helps in resolving issues with PCI devices behind switches.

In my configuration I have a PLX switch off the IMX root complex with
several devices behind it.  I find that the devices enumerate
correctly but as soon as data is transferred to or from (not sure
which) the device the system hangs (INFO: rcu_sched detected stalls on
CPUs/tasks: { 0} (detected by 3, t=2135 jiffies, g=20, c=19, q=66)).
My current test case is a GigE ethernet device and when I connect a
network to the device I hange when a packet is responded to.

I can't claim to fully understand PCI resource mappings or the iATU
and I don't understand why dw_pcie_rd_other_conf/dw_pcie_wr_other_conf
need to change the viewport then change it back instead of using
multiple viewports (perhaps because some hardware may not have more
than the 2 viewports currently being used?).  The current driver uses
viewport0 for cfg0 and mem and viewport1 for cfg1 and io.  If I remove
the call to dw_pcie_prog_viewport_io_outbound to reconfigure viewport1
for io after its altered for type1 cfg cycles, devices behind the
bridge work for me:

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designwa
index e33b68b..a064bc5 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -556,7 +556,7 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struc
        } else {
                dw_pcie_prog_viewport_cfg1(pp, busdev);
                ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
-               dw_pcie_prog_viewport_io_outbound(pp);
+//               dw_pcie_prog_viewport_io_outbound(pp);
        }

        return ret;
@@ -579,7 +579,7 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struc
        } else {
                dw_pcie_prog_viewport_cfg1(pp, busdev);
                ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
-               dw_pcie_prog_viewport_io_outbound(pp);
+//               dw_pcie_prog_viewport_io_outbound(pp);
        }

Can anyone explain whats going on here?   I would think this would
perhaps break devices that use IO resources?

Thanks,

Tim

>>
>>
>> Regards
>> Pratyush
>>
>> > From: Tim Harvey <tharvey@gateworks.com>
>> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > Cc: Frank Li <lznuaa@gmail.com>
>> > Cc: Harro Haan <hrhaan@gmail.com>
>> > Cc: Jingoo Han <jg1.han@samsung.com>
>> > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
>> > Cc: Pratyush Anand <pratyush.anand@st.com>
>> > Cc: Richard Zhu <r65037@freescale.com>
>> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> > Cc: Sean Cross <xobs@kosagi.com>
>> > Cc: Shawn Guo <shawn.guo@linaro.org>
>> > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
>> > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
>> > Cc: Tim Harvey <tharvey@gateworks.com>
>> > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
>> > Cc: Yinghai Lu <yinghai@kernel.org>
>> > ---
>> >
>> >  drivers/pci/host/pcie-designware.c | 41
>> >  +++----------------------------------- 1 file changed, 3 insertions(+),
>> >  38 deletions(-)
>> >
>> > diff --git a/drivers/pci/host/pcie-designware.c
>> > b/drivers/pci/host/pcie-designware.c index ed9b11b..01d76ad 100644
>> > --- a/drivers/pci/host/pcie-designware.c
>> > +++ b/drivers/pci/host/pcie-designware.c
>> > @@ -46,7 +46,6 @@
>> >
>> >  #define PCIE_ATU_VIEWPORT          0x900
>> >  #define PCIE_ATU_REGION_INBOUND            (0x1 << 31)
>> >  #define PCIE_ATU_REGION_OUTBOUND   (0x0 << 31)
>> >
>> > -#define PCIE_ATU_REGION_INDEX1             (0x1 << 0)
>> >
>> >  #define PCIE_ATU_REGION_INDEX0             (0x0 << 0)
>> >  #define PCIE_ATU_CR1                       0x904
>> >  #define PCIE_ATU_TYPE_MEM          (0x0 << 0)
>> >
>> > @@ -494,8 +493,8 @@ static void dw_pcie_prog_viewport_cfg0(struct
>> > pcie_port *pp, u32 busdev)
>> >
>> >  static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
>> >  {
>> >
>> > -   /* Program viewport 1 : OUTBOUND : CFG1 */
>> > -   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>> > PCIE_ATU_REGION_INDEX1, +   /* Program viewport 0 : OUTBOUND : CFG1 */
>> > +   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>> > PCIE_ATU_REGION_INDEX0,
>> >
>> >                       PCIE_ATU_VIEWPORT);
>> >
>> >     dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
>> >     dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>> >
>> > @@ -505,38 +504,8 @@ static void dw_pcie_prog_viewport_cfg1(struct
>> > pcie_port *pp, u32 busdev)
>> >
>> >                       PCIE_ATU_LIMIT);
>> >
>> >     dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
>> >     dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
>> >
>> > -}
>> > -
>> > -static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
>> > -{
>> > -   /* Program viewport 0 : OUTBOUND : MEM */
>> > -   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>> > PCIE_ATU_REGION_INDEX0, -                     PCIE_ATU_VIEWPORT);
>> > -   dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
>> > -   dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>> > -   dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
>> > -   dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
>> > -   dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
>> > -                     PCIE_ATU_LIMIT);
>> > -   dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
>> > -   dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
>> > -                     PCIE_ATU_UPPER_TARGET);
>> > -}
>> > -
>> > -static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
>> > -{
>> > -   /* Program viewport 1 : OUTBOUND : IO */
>> > -   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
>> > PCIE_ATU_REGION_INDEX1, -                     PCIE_ATU_VIEWPORT);
>> > -   dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
>> > +   dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
>> >
>> >     dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>> >
>> > -   dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE);
>> > -   dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE);
>> > -   dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1,
>> > -                     PCIE_ATU_LIMIT);
>> > -   dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET);
>> > -   dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr),
>> > -                     PCIE_ATU_UPPER_TARGET);
>> >
>> >  }
>> >
>> >  static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus
>> >  *bus,
>> >
>> > @@ -552,11 +521,9 @@ static int dw_pcie_rd_other_conf(struct pcie_port
>> > *pp, struct pci_bus *bus,
>> >
>> >     if (bus->parent->number == pp->root_bus_nr) {
>> >
>> >             dw_pcie_prog_viewport_cfg0(pp, busdev);
>> >             ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
>> >
>> > -           dw_pcie_prog_viewport_mem_outbound(pp);
>> >
>> >     } else {
>> >
>> >             dw_pcie_prog_viewport_cfg1(pp, busdev);
>> >             ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
>> >
>> > -           dw_pcie_prog_viewport_io_outbound(pp);
>> >
>> >     }
>> >
>> >     return ret;
>> >
>> > @@ -575,11 +542,9 @@ static int dw_pcie_wr_other_conf(struct pcie_port
>> > *pp, struct pci_bus *bus,
>> >
>> >     if (bus->parent->number == pp->root_bus_nr) {
>> >
>> >             dw_pcie_prog_viewport_cfg0(pp, busdev);
>> >             ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
>> >
>> > -           dw_pcie_prog_viewport_mem_outbound(pp);
>> >
>> >     } else {
>> >
>> >             dw_pcie_prog_viewport_cfg1(pp, busdev);
>> >             ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
>> >
>> > -           dw_pcie_prog_viewport_io_outbound(pp);
>> >
>> >     }
>> >
>> >     return ret;

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Richard Zhu]

Hi Marek:

Best Regards
Richard Zhu


> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Friday, November 29, 2013 1:56 AM
> To: Jingoo Han
> Cc: 'Tim Harvey'; 'Pratyush Anand'; linux-arm-kernel at lists.infradead.org;
> linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank Li'; 'Harro Haan'; 'Mohit
> KUMAR DCG'; Zhu Richard-R65037; 'Sascha Hauer'; 'Sean Cross'; 'Shawn Guo';
> 'Siva Reddy Kallam'; 'Srikanth T Shivanand'; 'Troy Kisky'; 'Yinghai Lu'
> Subject: Re: [PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region
> mapping
> 
> Dear Jingoo Han,
> 
> > On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> > > Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > > > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > >
> > > > > The IMX6 iATU is used for address translation between the AXI
> > > > > bus address space and PCI address space.  This is used for type0
> > > > > and
> > > > > type1 config cycles but is not necessary for outbound io/mem
> > > > > regions.
> > > > >
> > > > > This patch removes the calls that inappropriately re-configures
> > > > > the ATU viewport for outbound memory and IO after config cycles
> > > > > and removes them altogether as they are not necessary.
> > > >
> > > > Yes, they are not necessary for all the cases where translation is
> > > > one to one. So so for sure all the platform till now introduced
> > > > should work.
> > > > But, what about a platform where memory translation is not one to one?
> > > >
> > > > Existing code should work with all sort of memory translation on a
> > > > platform having atleast two viewports capable of programming any
> > > > type of outbound transaction.
> > >
> > > Full ACK, that's why it's called RFC.
> > >
> > > > > This resolves issues with PCI devices behind switches and has
> > > > > been tested with a Gige device behind a PLX PEX860x switch.
> > > > > More testing is needed for other configurations.
> > > >
> > > > I do not understand if MX6 has 4 Outbound Viewport then how this
> > > > patch helps?
> > > > -- PCI devices behind switches would not have been working because
> > > > CFG1 transaction would not have been correct.
> > > > -- It works with this patch. This patch changes viewport for CFG1
> > > > from
> > > > 1 to 0.
> > > > -- Can it be possible that MX6 has some restriction on viewport
> > > > programming capability. I mean,like only viewport0 can be
> > > > programmed for CFG0/1?
> > >
> > > Tim ?
> > >
> > > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2
> > > describe the iATU configuration on MX6. My understanding from this
> > > description is that the MX6 has 4 inbound and 4 outbound iATU regions.
> > > Am I wrong ?
> >
> > The numbers of inbound and outbound are hardware-configurable features.
> > Thus, these numbers  can be confirmed by Freescale  hardware engineers.
> > How about asking Freescale hardware engineers, if possible?
> 
> Richard is on the CC. Richard, can you please confirm those?
> 
[Richard] One Pericom PI7C9X2G303EL pcie switch, and two pcie ep deivces(one is intel e1000e nic,
  the other is one xhci device) are tested on imx6q sabresd board.
Without removing outbound io/mem regions view map during the cfg0/1
 read/write cycle, both of these devices can't work well at my side.
Works well after remove them during the cfg0/1 read/write cycles.
Same to Tim's situation about switch implementation.

> Thanks!
> 
> > Best regards,
> > Jingoo Han
> >
> > > [1]
> > > http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf
> > >
> > > > Otherwise I do not find any convincing reason that why this patch
> > > > helps in resolving issues with PCI devices behind switches.
> 
> Best regards,
> Marek Vasut

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Marek Vasut]

On Wednesday, December 04, 2013 at 06:49:55 AM, Richard Zhu wrote:
> Hi Marek:
> 
> Best Regards
> Richard Zhu
> 
> > -----Original Message-----
> > From: Marek Vasut [mailto:marex at denx.de]
> > Sent: Friday, November 29, 2013 1:56 AM
> > To: Jingoo Han
> > Cc: 'Tim Harvey'; 'Pratyush Anand'; linux-arm-kernel at lists.infradead.org;
> > linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank Li'; 'Harro Haan';
> > 'Mohit KUMAR DCG'; Zhu Richard-R65037; 'Sascha Hauer'; 'Sean Cross';
> > 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand'; 'Troy Kisky';
> > 'Yinghai Lu' Subject: Re: [PATCH 2/7] [RFC] PCI: imx6: remove outbound
> > io/mem ATU region mapping
> > 
> > Dear Jingoo Han,
> > 
> > > On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> > > > Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > > > > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > > > 
> > > > > > The IMX6 iATU is used for address translation between the AXI
> > > > > > bus address space and PCI address space.  This is used for type0
> > > > > > and
> > > > > > type1 config cycles but is not necessary for outbound io/mem
> > > > > > regions.
> > > > > > 
> > > > > > This patch removes the calls that inappropriately re-configures
> > > > > > the ATU viewport for outbound memory and IO after config cycles
> > > > > > and removes them altogether as they are not necessary.
> > > > > 
> > > > > Yes, they are not necessary for all the cases where translation is
> > > > > one to one. So so for sure all the platform till now introduced
> > > > > should work.
> > > > > But, what about a platform where memory translation is not one to
> > > > > one?
> > > > > 
> > > > > Existing code should work with all sort of memory translation on a
> > > > > platform having atleast two viewports capable of programming any
> > > > > type of outbound transaction.
> > > > 
> > > > Full ACK, that's why it's called RFC.
> > > > 
> > > > > > This resolves issues with PCI devices behind switches and has
> > > > > > been tested with a Gige device behind a PLX PEX860x switch.
> > > > > > More testing is needed for other configurations.
> > > > > 
> > > > > I do not understand if MX6 has 4 Outbound Viewport then how this
> > > > > patch helps?
> > > > > -- PCI devices behind switches would not have been working because
> > > > > CFG1 transaction would not have been correct.
> > > > > -- It works with this patch. This patch changes viewport for CFG1
> > > > > from
> > > > > 1 to 0.
> > > > > -- Can it be possible that MX6 has some restriction on viewport
> > > > > programming capability. I mean,like only viewport0 can be
> > > > > programmed for CFG0/1?
> > > > 
> > > > Tim ?
> > > > 
> > > > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2
> > > > describe the iATU configuration on MX6. My understanding from this
> > > > description is that the MX6 has 4 inbound and 4 outbound iATU
> > > > regions. Am I wrong ?
> > > 
> > > The numbers of inbound and outbound are hardware-configurable features.
> > > Thus, these numbers  can be confirmed by Freescale  hardware engineers.
> > > How about asking Freescale hardware engineers, if possible?
> > 
> > Richard is on the CC. Richard, can you please confirm those?
> 
> [Richard] One Pericom PI7C9X2G303EL pcie switch, and two pcie ep
> deivces(one is intel e1000e nic, the other is one xhci device) are tested
> on imx6q sabresd board. Without removing outbound io/mem regions view map
> during the cfg0/1 read/write cycle, both of these devices can't work well
> at my side. Works well after remove them during the cfg0/1 read/write
> cycles.
> Same to Tim's situation about switch implementation.

Thanks Richard! I am just wondering what kind of conclusion can we draw from 
this. Let me reiterate what we have:

- i.MX6 has 4 inbound and 4 outbound iATU regions, is that correct, Richard?
- If we program the iATU regions, the i.MX6 PCIe doesn't work with switch.

Is there some special sause to the iATU on MX6 maybe ?

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Pratyush Anand]

Hi Tim,

On Wed, Dec 04, 2013 at 04:14:47AM +0800, Tim Harvey wrote:
> On Wed, Nov 27, 2013 at 12:44 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Pratyush Anand,
> >
> >> Dear Marek Vasut,
> >>
> >> Please replace imx6 with designware in subject line  of this patch.
> >
> > OK, this one has to be fixed anyway. We need to figure out why do we even need
> > this one.
> >
> >> On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> >> > From: Tim Harvey <tharvey@gateworks.com>
> >> >
> >> > The IMX6 iATU is used for address translation between the AXI bus
> >> > address space and PCI address space.  This is used for type0 and type1
> >> > config cycles but is not necessary for outbound io/mem regions.
> >> >
> >> > This patch removes the calls that inappropriately re-configures the ATU
> >> > viewport for outbound memory and IO after config cycles and removes them
> >> > altogether as they are not necessary.
> >>
> >> Yes, they are not necessary for all the cases where translation is one
> >> to one. So so for sure all the platform till now introduced should
> >> work.
> >> But, what about a platform where memory translation is not one to one?
> >>
> >> Existing code should work with all sort of memory translation on a
> >> platform having atleast two viewports capable of programming any type
> >> of outbound transaction.
> >
> > Full ACK, that's why it's called RFC.
> >
> >> > This resolves issues with PCI devices behind switches and has been tested
> >> > with a Gige device behind a PLX PEX860x switch.  More testing is needed
> >> > for other configurations.
> >>
> >> I do not understand if MX6 has 4 Outbound Viewport then how this patch
> >> helps?
> >> -- PCI devices behind switches would not have been working because
> >> CFG1 transaction would not have been correct.
> >> -- It works with this patch. This patch changes viewport for CFG1 from
> >> 1 to 0.
> >> -- Can it be possible that MX6 has some restriction on viewport
> >> programming capability. I mean,like only viewport0 can be programmed
> >> for CFG0/1?
> >
> > Tim ?
> >
> > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2 describe
> > the iATU configuration on MX6. My understanding from this description is that
> > the MX6 has 4 inbound and 4 outbound iATU regions. Am I wrong ?
> >
> > [1] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf
> >
> >> Otherwise I do not find any convincing reason that why this patch
> >> helps in resolving issues with PCI devices behind switches.
> 
> In my configuration I have a PLX switch off the IMX root complex with
> several devices behind it.  I find that the devices enumerate
> correctly but as soon as data is transferred to or from (not sure
> which) the device the system hangs (INFO: rcu_sched detected stalls on
> CPUs/tasks: { 0} (detected by 3, t=2135 jiffies, g=20, c=19, q=66)).
> My current test case is a GigE ethernet device and when I connect a
> network to the device I hange when a packet is responded to.
> 
> I can't claim to fully understand PCI resource mappings or the iATU
> and I don't understand why dw_pcie_rd_other_conf/dw_pcie_wr_other_conf
> need to change the viewport then change it back instead of using
> multiple viewports (perhaps because some hardware may not have more
> than the 2 viewports currently being used?).  The current driver uses

Yes, driver has been written keeping in mind that there exist minimum
of two programmable outbound viewport.

> viewport0 for cfg0 and mem and viewport1 for cfg1 and io.  If I remove
> the call to dw_pcie_prog_viewport_io_outbound to reconfigure viewport1
> for io after its altered for type1 cfg cycles, devices behind the
> bridge work for me:

This is strange !!!
When you say it does not work, what exactly happens? What does kernel
crash log says? Which register access forces CPU to hang? Is it
possible to capture traffic with PCIe analyzer?

> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designwa
> index e33b68b..a064bc5 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -556,7 +556,7 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struc
>         } else {
>                 dw_pcie_prog_viewport_cfg1(pp, busdev);
>                 ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
> -               dw_pcie_prog_viewport_io_outbound(pp);
> +//               dw_pcie_prog_viewport_io_outbound(pp);
>         }
> 
>         return ret;
> @@ -579,7 +579,7 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struc
>         } else {
>                 dw_pcie_prog_viewport_cfg1(pp, busdev);
>                 ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
> -               dw_pcie_prog_viewport_io_outbound(pp);
> +//               dw_pcie_prog_viewport_io_outbound(pp);
>         }
> 
> Can anyone explain whats going on here?   I would think this would
> perhaps break devices that use IO resources?

Yes, without this devices with IO resource will not work. However, I
see some issues with IO translation implementation, and as per my
understanding IO transfer should not work with current implementation. 
Anyway, that is unrelated to the issue you are facing..so better I
will discuss that in another thread.

Regards
Pratyush
> 
> Thanks,
> 
> Tim
> 
> >>
> >>
> >> Regards
> >> Pratyush
> >>
> >> > From: Tim Harvey <tharvey@gateworks.com>
> >> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> > Cc: Frank Li <lznuaa@gmail.com>
> >> > Cc: Harro Haan <hrhaan@gmail.com>
> >> > Cc: Jingoo Han <jg1.han@samsung.com>
> >> > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> >> > Cc: Pratyush Anand <pratyush.anand@st.com>
> >> > Cc: Richard Zhu <r65037@freescale.com>
> >> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >> > Cc: Sean Cross <xobs@kosagi.com>
> >> > Cc: Shawn Guo <shawn.guo@linaro.org>
> >> > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> >> > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> >> > Cc: Tim Harvey <tharvey@gateworks.com>
> >> > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> >> > Cc: Yinghai Lu <yinghai@kernel.org>
> >> > ---
> >> >
> >> >  drivers/pci/host/pcie-designware.c | 41
> >> >  +++----------------------------------- 1 file changed, 3 insertions(+),
> >> >  38 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/host/pcie-designware.c
> >> > b/drivers/pci/host/pcie-designware.c index ed9b11b..01d76ad 100644
> >> > --- a/drivers/pci/host/pcie-designware.c
> >> > +++ b/drivers/pci/host/pcie-designware.c
> >> > @@ -46,7 +46,6 @@
> >> >
> >> >  #define PCIE_ATU_VIEWPORT          0x900
> >> >  #define PCIE_ATU_REGION_INBOUND            (0x1 << 31)
> >> >  #define PCIE_ATU_REGION_OUTBOUND   (0x0 << 31)
> >> >
> >> > -#define PCIE_ATU_REGION_INDEX1             (0x1 << 0)
> >> >
> >> >  #define PCIE_ATU_REGION_INDEX0             (0x0 << 0)
> >> >  #define PCIE_ATU_CR1                       0x904
> >> >  #define PCIE_ATU_TYPE_MEM          (0x0 << 0)
> >> >
> >> > @@ -494,8 +493,8 @@ static void dw_pcie_prog_viewport_cfg0(struct
> >> > pcie_port *pp, u32 busdev)
> >> >
> >> >  static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
> >> >  {
> >> >
> >> > -   /* Program viewport 1 : OUTBOUND : CFG1 */
> >> > -   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> >> > PCIE_ATU_REGION_INDEX1, +   /* Program viewport 0 : OUTBOUND : CFG1 */
> >> > +   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> >> > PCIE_ATU_REGION_INDEX0,
> >> >
> >> >                       PCIE_ATU_VIEWPORT);
> >> >
> >> >     dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> >> >     dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> >> >
> >> > @@ -505,38 +504,8 @@ static void dw_pcie_prog_viewport_cfg1(struct
> >> > pcie_port *pp, u32 busdev)
> >> >
> >> >                       PCIE_ATU_LIMIT);
> >> >
> >> >     dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
> >> >     dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
> >> >
> >> > -}
> >> > -
> >> > -static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
> >> > -{
> >> > -   /* Program viewport 0 : OUTBOUND : MEM */
> >> > -   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> >> > PCIE_ATU_REGION_INDEX0, -                     PCIE_ATU_VIEWPORT);
> >> > -   dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> >> > -   dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> >> > -   dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
> >> > -   dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
> >> > -   dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
> >> > -                     PCIE_ATU_LIMIT);
> >> > -   dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
> >> > -   dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
> >> > -                     PCIE_ATU_UPPER_TARGET);
> >> > -}
> >> > -
> >> > -static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
> >> > -{
> >> > -   /* Program viewport 1 : OUTBOUND : IO */
> >> > -   dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
> >> > PCIE_ATU_REGION_INDEX1, -                     PCIE_ATU_VIEWPORT);
> >> > -   dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
> >> > +   dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
> >> >
> >> >     dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> >> >
> >> > -   dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE);
> >> > -   dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE);
> >> > -   dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1,
> >> > -                     PCIE_ATU_LIMIT);
> >> > -   dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET);
> >> > -   dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr),
> >> > -                     PCIE_ATU_UPPER_TARGET);
> >> >
> >> >  }
> >> >
> >> >  static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus
> >> >  *bus,
> >> >
> >> > @@ -552,11 +521,9 @@ static int dw_pcie_rd_other_conf(struct pcie_port
> >> > *pp, struct pci_bus *bus,
> >> >
> >> >     if (bus->parent->number == pp->root_bus_nr) {
> >> >
> >> >             dw_pcie_prog_viewport_cfg0(pp, busdev);
> >> >             ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
> >> >
> >> > -           dw_pcie_prog_viewport_mem_outbound(pp);
> >> >
> >> >     } else {
> >> >
> >> >             dw_pcie_prog_viewport_cfg1(pp, busdev);
> >> >             ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
> >> >
> >> > -           dw_pcie_prog_viewport_io_outbound(pp);
> >> >
> >> >     }
> >> >
> >> >     return ret;
> >> >
> >> > @@ -575,11 +542,9 @@ static int dw_pcie_wr_other_conf(struct pcie_port
> >> > *pp, struct pci_bus *bus,
> >> >
> >> >     if (bus->parent->number == pp->root_bus_nr) {
> >> >
> >> >             dw_pcie_prog_viewport_cfg0(pp, busdev);
> >> >             ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
> >> >
> >> > -           dw_pcie_prog_viewport_mem_outbound(pp);
> >> >
> >> >     } else {
> >> >
> >> >             dw_pcie_prog_viewport_cfg1(pp, busdev);
> >> >             ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
> >> >
> >> > -           dw_pcie_prog_viewport_io_outbound(pp);
> >> >
> >> >     }
> >> >
> >> >     return ret;

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Tim Harvey]

On Wed, Dec 4, 2013 at 2:33 AM, Pratyush Anand <pratyush.anand@st.com> wrote:
> Hi Tim,
>
>>
>> In my configuration I have a PLX switch off the IMX root complex with
>> several devices behind it.  I find that the devices enumerate
>> correctly but as soon as data is transferred to or from (not sure
>> which) the device the system hangs (INFO: rcu_sched detected stalls on
>> CPUs/tasks: { 0} (detected by 3, t=2135 jiffies, g=20, c=19, q=66)).
>> My current test case is a GigE ethernet device and when I connect a
>> network to the device I hange when a packet is responded to.
>>
>> I can't claim to fully understand PCI resource mappings or the iATU
>> and I don't understand why dw_pcie_rd_other_conf/dw_pcie_wr_other_conf
>> need to change the viewport then change it back instead of using
>> multiple viewports (perhaps because some hardware may not have more
>> than the 2 viewports currently being used?).  The current driver uses
>
> Yes, driver has been written keeping in mind that there exist minimum
> of two programmable outbound viewport.
>
>> viewport0 for cfg0 and mem and viewport1 for cfg1 and io.  If I remove
>> the call to dw_pcie_prog_viewport_io_outbound to reconfigure viewport1
>> for io after its altered for type1 cfg cycles, devices behind the
>> bridge work for me:
>
> This is strange !!!
> When you say it does not work, what exactly happens? What does kernel
> crash log says? Which register access forces CPU to hang? Is it
> possible to capture traffic with PCIe analyzer?
>

Pratyush,

The device behind the switch is a Marvell Yukon 2 GigE 88E8057:

$ lspci -n
00:00.0 0604: 16c3:abcd (rev 01)
01:00.0 0604: 10b5:8609 (rev ba)
01:00.1 0880: 10b5:8609 (rev ba)
02:01.0 0604: 10b5:8609 (rev ba)
02:04.0 0604: 10b5:8609 (rev ba)
02:05.0 0604: 10b5:8609 (rev ba)
02:06.0 0604: 10b5:8609 (rev ba)
02:07.0 0604: 10b5:8609 (rev ba)
02:08.0 0604: 10b5:8609 (rev ba)
02:09.0 0604: 10b5:8609 (rev ba)
07:00.0 0280: 168c:002b (rev 01)
08:00.0 0200: 11ab:4380
$ lspci -s 08:00.0 -v
08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057
PCI-E Gigabit Ethernet Controller
        Subsystem: Marvell Technology Group Ltd. 88E8057 PCI-E Gigabit
Ethernet Controller
        Flags: bus master, fast devsel, latency 0, IRQ 155
        Memory at 01200000 (64-bit, non-prefetchable) [size=16K]
        I/O ports at 1000 [size=256]
        Capabilities: [48] Power Management version 3
        Capabilities: [5c] MSI: Enable- Count=1/1 Maskable- 64bit+
        Capabilities: [c0] Express Legacy Endpoint, MSI 00
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [130] Device Serial Number 00-00-00-00-00-00-00-00
        Kernel driver in use: sky2

$ ifconfig eth1
eth1      Link encap:Ethernet  HWaddr 00:D0:12:9D:EF:E7
          BROADCAST MULTICAST  MTU:1500  Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000
          RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
          Interrupt:155

$ ifconfig eth1 up
[   70.207493] sky2 0000:08:00.0 eth1: enabling interface
[   70.212855] IPv6: ADDRCONF(NETDEV_UP): eth1: link is not ready
[   72.703741] sky2 0000:08:00.0 eth1: Link is up at 1000 Mbps, full
duplex, flow control both
[   72.712392] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready

board hangs here for several minutes then an rcu_sched stall is
detected minutes later (I can't explain why the printk time below is
wrong... it seems to within several minutes after the hang)

[   63.559915] INFO: rcu_sched detected stalls on CPUs/tasks: { 0}
(detected by 1, t=2102 jiffies, g=4294967231, c=4294967230, q=10)

I do not have an anaylzer that can get on the PCIe bus.

Tim

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Marek Vasut]

On Wednesday, December 04, 2013 at 07:51:22 PM, Tim Harvey wrote:
> On Wed, Dec 4, 2013 at 2:33 AM, Pratyush Anand <pratyush.anand@st.com> wrote:
> > Hi Tim,
> > 
> >> In my configuration I have a PLX switch off the IMX root complex with
> >> several devices behind it.  I find that the devices enumerate
> >> correctly but as soon as data is transferred to or from (not sure
> >> which) the device the system hangs (INFO: rcu_sched detected stalls on
> >> CPUs/tasks: { 0} (detected by 3, t=2135 jiffies, g=20, c=19, q=66)).
> >> My current test case is a GigE ethernet device and when I connect a
> >> network to the device I hange when a packet is responded to.
> >> 
> >> I can't claim to fully understand PCI resource mappings or the iATU
> >> and I don't understand why dw_pcie_rd_other_conf/dw_pcie_wr_other_conf
> >> need to change the viewport then change it back instead of using
> >> multiple viewports (perhaps because some hardware may not have more
> >> than the 2 viewports currently being used?).  The current driver uses
> > 
> > Yes, driver has been written keeping in mind that there exist minimum
> > of two programmable outbound viewport.
> > 
> >> viewport0 for cfg0 and mem and viewport1 for cfg1 and io.  If I remove
> >> the call to dw_pcie_prog_viewport_io_outbound to reconfigure viewport1
> >> for io after its altered for type1 cfg cycles, devices behind the
> > 
> >> bridge work for me:
> > This is strange !!!
> > When you say it does not work, what exactly happens? What does kernel
> > crash log says? Which register access forces CPU to hang? Is it
> > possible to capture traffic with PCIe analyzer?
> 
> Pratyush,
> 
> The device behind the switch is a Marvell Yukon 2 GigE 88E8057:
> 
> $ lspci -n
> 00:00.0 0604: 16c3:abcd (rev 01)
> 01:00.0 0604: 10b5:8609 (rev ba)
> 01:00.1 0880: 10b5:8609 (rev ba)
> 02:01.0 0604: 10b5:8609 (rev ba)
> 02:04.0 0604: 10b5:8609 (rev ba)
> 02:05.0 0604: 10b5:8609 (rev ba)
> 02:06.0 0604: 10b5:8609 (rev ba)
> 02:07.0 0604: 10b5:8609 (rev ba)
> 02:08.0 0604: 10b5:8609 (rev ba)
> 02:09.0 0604: 10b5:8609 (rev ba)
> 07:00.0 0280: 168c:002b (rev 01)
> 08:00.0 0200: 11ab:4380
> $ lspci -s 08:00.0 -v
> 08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057
> PCI-E Gigabit Ethernet Controller
>         Subsystem: Marvell Technology Group Ltd. 88E8057 PCI-E Gigabit
> Ethernet Controller
>         Flags: bus master, fast devsel, latency 0, IRQ 155
>         Memory at 01200000 (64-bit, non-prefetchable) [size=16K]
>         I/O ports at 1000 [size=256]
>         Capabilities: [48] Power Management version 3
>         Capabilities: [5c] MSI: Enable- Count=1/1 Maskable- 64bit+
>         Capabilities: [c0] Express Legacy Endpoint, MSI 00
>         Capabilities: [100] Advanced Error Reporting
>         Capabilities: [130] Device Serial Number 00-00-00-00-00-00-00-00
>         Kernel driver in use: sky2
> 
> $ ifconfig eth1
> eth1      Link encap:Ethernet  HWaddr 00:D0:12:9D:EF:E7
>           BROADCAST MULTICAST  MTU:1500  Metric:1
>           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:1000
>           RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
>           Interrupt:155
> 
> $ ifconfig eth1 up
> [   70.207493] sky2 0000:08:00.0 eth1: enabling interface
> [   70.212855] IPv6: ADDRCONF(NETDEV_UP): eth1: link is not ready
> [   72.703741] sky2 0000:08:00.0 eth1: Link is up at 1000 Mbps, full
> duplex, flow control both
> [   72.712392] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
> 
> board hangs here for several minutes then an rcu_sched stall is
> detected minutes later (I can't explain why the printk time below is
> wrong... it seems to within several minutes after the hang)
> 
> [   63.559915] INFO: rcu_sched detected stalls on CPUs/tasks: { 0}
> (detected by 1, t=2102 jiffies, g=4294967231, c=4294967230, q=10)
> 
> I do not have an anaylzer that can get on the PCIe bus.

I wonder, if you remove all IO space accesses from your Yukon driver, will the 
system hang as well? That way, we can be sure that the IO accesses are what 
kills the system.

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Richard Zhu]

Hi Marek:



> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Wednesday, December 04, 2013 3:52 PM
> To: Zhu Richard-R65037
> Cc: Jingoo Han; 'Tim Harvey'; 'Pratyush Anand'; linux-arm-
> kernel at lists.infradead.org; linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank
> Li'; 'Harro Haan'; 'Mohit KUMAR DCG'; 'Sascha Hauer'; 'Sean Cross'; 'Shawn
> Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand'; 'Troy Kisky'; 'Yinghai Lu'
> Subject: Re: [PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region
> mapping
> 
> On Wednesday, December 04, 2013 at 06:49:55 AM, Richard Zhu wrote:
> > Hi Marek:
> >
> > Best Regards
> > Richard Zhu
> >
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex at denx.de]
> > > Sent: Friday, November 29, 2013 1:56 AM
> > > To: Jingoo Han
> > > Cc: 'Tim Harvey'; 'Pratyush Anand';
> > > linux-arm-kernel at lists.infradead.org;
> > > linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank Li'; 'Harro
> > > Haan'; 'Mohit KUMAR DCG'; Zhu Richard-R65037; 'Sascha Hauer'; 'Sean
> > > Cross'; 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand';
> > > 'Troy Kisky'; 'Yinghai Lu' Subject: Re: [PATCH 2/7] [RFC] PCI: imx6:
> > > remove outbound io/mem ATU region mapping
> > >
> > > Dear Jingoo Han,
> > >
> > > > On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> > > > > Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > > > > > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > > > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > > > >
> > > > > > > The IMX6 iATU is used for address translation between the
> > > > > > > AXI bus address space and PCI address space.  This is used
> > > > > > > for type0 and
> > > > > > > type1 config cycles but is not necessary for outbound io/mem
> > > > > > > regions.
> > > > > > >
> > > > > > > This patch removes the calls that inappropriately
> > > > > > > re-configures the ATU viewport for outbound memory and IO
> > > > > > > after config cycles and removes them altogether as they are not
> necessary.
> > > > > >
> > > > > > Yes, they are not necessary for all the cases where
> > > > > > translation is one to one. So so for sure all the platform
> > > > > > till now introduced should work.
> > > > > > But, what about a platform where memory translation is not one
> > > > > > to one?
> > > > > >
> > > > > > Existing code should work with all sort of memory translation
> > > > > > on a platform having atleast two viewports capable of
> > > > > > programming any type of outbound transaction.
> > > > >
> > > > > Full ACK, that's why it's called RFC.
> > > > >
> > > > > > > This resolves issues with PCI devices behind switches and
> > > > > > > has been tested with a Gige device behind a PLX PEX860x switch.
> > > > > > > More testing is needed for other configurations.
> > > > > >
> > > > > > I do not understand if MX6 has 4 Outbound Viewport then how
> > > > > > this patch helps?
> > > > > > -- PCI devices behind switches would not have been working
> > > > > > because
> > > > > > CFG1 transaction would not have been correct.
> > > > > > -- It works with this patch. This patch changes viewport for
> > > > > > CFG1 from
> > > > > > 1 to 0.
> > > > > > -- Can it be possible that MX6 has some restriction on
> > > > > > viewport programming capability. I mean,like only viewport0
> > > > > > can be programmed for CFG0/1?
> > > > >
> > > > > Tim ?
> > > > >
> > > > > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and
> > > > > 48.3.9.1.2 describe the iATU configuration on MX6. My
> > > > > understanding from this description is that the MX6 has 4
> > > > > inbound and 4 outbound iATU regions. Am I wrong ?
> > > >
> > > > The numbers of inbound and outbound are hardware-configurable features.
> > > > Thus, these numbers  can be confirmed by Freescale  hardware engineers.
> > > > How about asking Freescale hardware engineers, if possible?
> > >
> > > Richard is on the CC. Richard, can you please confirm those?
> >
> > [Richard] One Pericom PI7C9X2G303EL pcie switch, and two pcie ep
> > deivces(one is intel e1000e nic, the other is one xhci device) are
> > tested on imx6q sabresd board. Without removing outbound io/mem
> > regions view map during the cfg0/1 read/write cycle, both of these
> > devices can't work well at my side. Works well after remove them
> > during the cfg0/1 read/write cycles.
> > Same to Tim's situation about switch implementation.
> 
> Thanks Richard! I am just wondering what kind of conclusion can we draw from
> this. Let me reiterate what we have:
> 
> - i.MX6 has 4 inbound and 4 outbound iATU regions, is that correct, Richard?
> - If we program the iATU regions, the i.MX6 PCIe doesn't work with switch.
> 
> Is there some special sause to the iATU on MX6 maybe ?
[Richard] welcome, it's correct, imx6 pcie has 4 inbount and 4 outbound iATU regions.
Imx6 pcie doesn't work with switch if the following view switches are not removed in
The cfg0/1 read/write cycles.

@@ -516,9 +560,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
        if (bus->parent->number == pp->root_bus_nr) {
                dw_pcie_prog_viewport_cfg0(pp, busdev);
                ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
+               // dw_pcie_prog_viewport_mem_outbound(pp);
        } else {
                dw_pcie_prog_viewport_cfg1(pp, busdev);
                ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
+               // dw_pcie_prog_viewport_io_outbound(pp);
        }

        return ret;
@@ -537,9 +583,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
        if (bus->parent->number == pp->root_bus_nr) {
                dw_pcie_prog_viewport_cfg0(pp, busdev);
                ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
+               // dw_pcie_prog_viewport_mem_outbound(pp);
        } else {
                dw_pcie_prog_viewport_cfg1(pp, busdev);
                ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
+               // dw_pcie_prog_viewport_io_outbound(pp);
        }

        return ret;

Best Regards
Richard Zhu

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Richard Zhu]

Hi Merak:


> -----Original Message-----
> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-owner at vger.kernel.org]
> On Behalf Of Richard Zhu
> Sent: Thursday, December 05, 2013 8:45 AM
> To: Marek Vasut
> Cc: Jingoo Han; 'Tim Harvey'; 'Pratyush Anand'; linux-arm-
> kernel at lists.infradead.org; linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank
> Li'; 'Harro Haan'; 'Mohit KUMAR DCG'; 'Sascha Hauer'; 'Sean Cross'; 'Shawn
> Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand'; 'Troy Kisky'; 'Yinghai Lu'
> Subject: RE: [PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region
> mapping
> 
> Hi Marek:
> 
> 
> 
> > -----Original Message-----
> > From: Marek Vasut [mailto:marex at denx.de]
> > Sent: Wednesday, December 04, 2013 3:52 PM
> > To: Zhu Richard-R65037
> > Cc: Jingoo Han; 'Tim Harvey'; 'Pratyush Anand'; linux-arm-
> > kernel at lists.infradead.org; linux-pci at vger.kernel.org; 'Bjorn
> > Helgaas'; 'Frank Li'; 'Harro Haan'; 'Mohit KUMAR DCG'; 'Sascha Hauer';
> > 'Sean Cross'; 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand';
> 'Troy Kisky'; 'Yinghai Lu'
> > Subject: Re: [PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU
> > region mapping
> >
> > On Wednesday, December 04, 2013 at 06:49:55 AM, Richard Zhu wrote:
> > > Hi Marek:
> > >
> > > Best Regards
> > > Richard Zhu
> > >
> > > > -----Original Message-----
> > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > Sent: Friday, November 29, 2013 1:56 AM
> > > > To: Jingoo Han
> > > > Cc: 'Tim Harvey'; 'Pratyush Anand';
> > > > linux-arm-kernel at lists.infradead.org;
> > > > linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank Li'; 'Harro
> > > > Haan'; 'Mohit KUMAR DCG'; Zhu Richard-R65037; 'Sascha Hauer';
> > > > 'Sean Cross'; 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T
> > > > Shivanand'; 'Troy Kisky'; 'Yinghai Lu' Subject: Re: [PATCH 2/7] [RFC]
> PCI: imx6:
> > > > remove outbound io/mem ATU region mapping
> > > >
> > > > Dear Jingoo Han,
> > > >
> > > > > On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> > > > > > Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > > > > > > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > > > > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > > > > >
> > > > > > > > The IMX6 iATU is used for address translation between the
> > > > > > > > AXI bus address space and PCI address space.  This is used
> > > > > > > > for type0 and
> > > > > > > > type1 config cycles but is not necessary for outbound
> > > > > > > > io/mem regions.
> > > > > > > >
> > > > > > > > This patch removes the calls that inappropriately
> > > > > > > > re-configures the ATU viewport for outbound memory and IO
> > > > > > > > after config cycles and removes them altogether as they
> > > > > > > > are not
> > necessary.
> > > > > > >
> > > > > > > Yes, they are not necessary for all the cases where
> > > > > > > translation is one to one. So so for sure all the platform
> > > > > > > till now introduced should work.
> > > > > > > But, what about a platform where memory translation is not
> > > > > > > one to one?
> > > > > > >
> > > > > > > Existing code should work with all sort of memory
> > > > > > > translation on a platform having atleast two viewports
> > > > > > > capable of programming any type of outbound transaction.
> > > > > >
> > > > > > Full ACK, that's why it's called RFC.
> > > > > >
> > > > > > > > This resolves issues with PCI devices behind switches and
> > > > > > > > has been tested with a Gige device behind a PLX PEX860x switch.
> > > > > > > > More testing is needed for other configurations.
> > > > > > >
> > > > > > > I do not understand if MX6 has 4 Outbound Viewport then how
> > > > > > > this patch helps?
> > > > > > > -- PCI devices behind switches would not have been working
> > > > > > > because
> > > > > > > CFG1 transaction would not have been correct.
> > > > > > > -- It works with this patch. This patch changes viewport for
> > > > > > > CFG1 from
> > > > > > > 1 to 0.
> > > > > > > -- Can it be possible that MX6 has some restriction on
> > > > > > > viewport programming capability. I mean,like only viewport0
> > > > > > > can be programmed for CFG0/1?
> > > > > >
> > > > > > Tim ?
> > > > > >
> > > > > > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and
> > > > > > 48.3.9.1.2 describe the iATU configuration on MX6. My
> > > > > > understanding from this description is that the MX6 has 4
> > > > > > inbound and 4 outbound iATU regions. Am I wrong ?
> > > > >
> > > > > The numbers of inbound and outbound are hardware-configurable features.
> > > > > Thus, these numbers  can be confirmed by Freescale  hardware engineers.
> > > > > How about asking Freescale hardware engineers, if possible?
> > > >
> > > > Richard is on the CC. Richard, can you please confirm those?
> > >
> > > [Richard] One Pericom PI7C9X2G303EL pcie switch, and two pcie ep
> > > deivces(one is intel e1000e nic, the other is one xhci device) are
> > > tested on imx6q sabresd board. Without removing outbound io/mem
> > > regions view map during the cfg0/1 read/write cycle, both of these
> > > devices can't work well at my side. Works well after remove them
> > > during the cfg0/1 read/write cycles.
> > > Same to Tim's situation about switch implementation.
> >
> > Thanks Richard! I am just wondering what kind of conclusion can we
> > draw from this. Let me reiterate what we have:
> >
> > - i.MX6 has 4 inbound and 4 outbound iATU regions, is that correct, Richard?
> > - If we program the iATU regions, the i.MX6 PCIe doesn't work with switch.
> >
> > Is there some special sause to the iATU on MX6 maybe ?
> [Richard] welcome, it's correct, imx6 pcie has 4 inbount and 4 outbound iATU
> regions.
> Imx6 pcie doesn't work with switch if the following view switches are not
> removed in The cfg0/1 read/write cycles.
> 
> @@ -516,9 +560,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp,
> struct pci_bus *bus,
>         if (bus->parent->number == pp->root_bus_nr) {
>                 dw_pcie_prog_viewport_cfg0(pp, busdev);
>                 ret = cfg_read(pp->va_cfg0_base + address, where, size, val);
> +               // dw_pcie_prog_viewport_mem_outbound(pp);
>         } else {
>                 dw_pcie_prog_viewport_cfg1(pp, busdev);
>                 ret = cfg_read(pp->va_cfg1_base + address, where, size, val);
> +               // dw_pcie_prog_viewport_io_outbound(pp);
>         }
> 
>         return ret;
> @@ -537,9 +583,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp,
> struct pci_bus *bus,
>         if (bus->parent->number == pp->root_bus_nr) {
>                 dw_pcie_prog_viewport_cfg0(pp, busdev);
>                 ret = cfg_write(pp->va_cfg0_base + address, where, size, val);
> +               // dw_pcie_prog_viewport_mem_outbound(pp);
>         } else {
>                 dw_pcie_prog_viewport_cfg1(pp, busdev);
>                 ret = cfg_write(pp->va_cfg1_base + address, where, size, val);
> +               // dw_pcie_prog_viewport_io_outbound(pp);
>         }
> 
>         return ret;
> 
> Best Regards
> Richard Zhu
> 
[Richard] Bisect the code-modifications, only the dw_pcie_prog_viewport_io_outbound()
 should be masked if I want the switch works in my imx6 pcie switch support tests.
Suspect that the mem emulated io space introduce this problem, debug is on-going.
> 
Best Regards
Richard Zhu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the
> body of a message to majordomo at vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping

[Marek Vasut]

On Thursday, December 05, 2013 at 02:05:40 AM, Richard Zhu wrote:
> Hi Merak:
> > -----Original Message-----
> > From: linux-pci-owner at vger.kernel.org
> > [mailto:linux-pci-owner at vger.kernel.org] On Behalf Of Richard Zhu
> > Sent: Thursday, December 05, 2013 8:45 AM
> > To: Marek Vasut
> > Cc: Jingoo Han; 'Tim Harvey'; 'Pratyush Anand'; linux-arm-
> > kernel at lists.infradead.org; linux-pci at vger.kernel.org; 'Bjorn Helgaas';
> > 'Frank Li'; 'Harro Haan'; 'Mohit KUMAR DCG'; 'Sascha Hauer'; 'Sean
> > Cross'; 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand'; 'Troy
> > Kisky'; 'Yinghai Lu' Subject: RE: [PATCH 2/7] [RFC] PCI: imx6: remove
> > outbound io/mem ATU region mapping
> > 
> > Hi Marek:
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex at denx.de]
> > > Sent: Wednesday, December 04, 2013 3:52 PM
> > > To: Zhu Richard-R65037
> > > Cc: Jingoo Han; 'Tim Harvey'; 'Pratyush Anand'; linux-arm-
> > > kernel at lists.infradead.org; linux-pci at vger.kernel.org; 'Bjorn
> > > Helgaas'; 'Frank Li'; 'Harro Haan'; 'Mohit KUMAR DCG'; 'Sascha Hauer';
> > > 'Sean Cross'; 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T Shivanand';
> > 
> > 'Troy Kisky'; 'Yinghai Lu'
> > 
> > > Subject: Re: [PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU
> > > region mapping
> > > 
> > > On Wednesday, December 04, 2013 at 06:49:55 AM, Richard Zhu wrote:
> > > > Hi Marek:
> > > > 
> > > > Best Regards
> > > > Richard Zhu
> > > > 
> > > > > -----Original Message-----
> > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > Sent: Friday, November 29, 2013 1:56 AM
> > > > > To: Jingoo Han
> > > > > Cc: 'Tim Harvey'; 'Pratyush Anand';
> > > > > linux-arm-kernel at lists.infradead.org;
> > > > > linux-pci at vger.kernel.org; 'Bjorn Helgaas'; 'Frank Li'; 'Harro
> > > > > Haan'; 'Mohit KUMAR DCG'; Zhu Richard-R65037; 'Sascha Hauer';
> > > > > 'Sean Cross'; 'Shawn Guo'; 'Siva Reddy Kallam'; 'Srikanth T
> > > > > Shivanand'; 'Troy Kisky'; 'Yinghai Lu' Subject: Re: [PATCH 2/7]
> > > > > [RFC]
> > 
> > PCI: imx6:
> > > > > remove outbound io/mem ATU region mapping
> > > > > 
> > > > > Dear Jingoo Han,
> > > > > 
> > > > > > On Wednesday, November 27, 2013 5:44 PM, Marek Vasut wrote:
> > > > > > > Wednesday, November 27, 2013 1:30 PM, Pratyush Anand wrote:
> > > > > > > > On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote:
> > > > > > > > > From: Tim Harvey <tharvey@gateworks.com>
> > > > > > > > > 
> > > > > > > > > The IMX6 iATU is used for address translation between the
> > > > > > > > > AXI bus address space and PCI address space.  This is used
> > > > > > > > > for type0 and
> > > > > > > > > type1 config cycles but is not necessary for outbound
> > > > > > > > > io/mem regions.
> > > > > > > > > 
> > > > > > > > > This patch removes the calls that inappropriately
> > > > > > > > > re-configures the ATU viewport for outbound memory and IO
> > > > > > > > > after config cycles and removes them altogether as they
> > > > > > > > > are not
> > > 
> > > necessary.
> > > 
> > > > > > > > Yes, they are not necessary for all the cases where
> > > > > > > > translation is one to one. So so for sure all the platform
> > > > > > > > till now introduced should work.
> > > > > > > > But, what about a platform where memory translation is not
> > > > > > > > one to one?
> > > > > > > > 
> > > > > > > > Existing code should work with all sort of memory
> > > > > > > > translation on a platform having atleast two viewports
> > > > > > > > capable of programming any type of outbound transaction.
> > > > > > > 
> > > > > > > Full ACK, that's why it's called RFC.
> > > > > > > 
> > > > > > > > > This resolves issues with PCI devices behind switches and
> > > > > > > > > has been tested with a Gige device behind a PLX PEX860x
> > > > > > > > > switch. More testing is needed for other configurations.
> > > > > > > > 
> > > > > > > > I do not understand if MX6 has 4 Outbound Viewport then how
> > > > > > > > this patch helps?
> > > > > > > > -- PCI devices behind switches would not have been working
> > > > > > > > because
> > > > > > > > CFG1 transaction would not have been correct.
> > > > > > > > -- It works with this patch. This patch changes viewport for
> > > > > > > > CFG1 from
> > > > > > > > 1 to 0.
> > > > > > > > -- Can it be possible that MX6 has some restriction on
> > > > > > > > viewport programming capability. I mean,like only viewport0
> > > > > > > > can be programmed for CFG0/1?
> > > > > > > 
> > > > > > > Tim ?
> > > > > > > 
> > > > > > > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and
> > > > > > > 48.3.9.1.2 describe the iATU configuration on MX6. My
> > > > > > > understanding from this description is that the MX6 has 4
> > > > > > > inbound and 4 outbound iATU regions. Am I wrong ?
> > > > > > 
> > > > > > The numbers of inbound and outbound are hardware-configurable
> > > > > > features. Thus, these numbers  can be confirmed by Freescale 
> > > > > > hardware engineers. How about asking Freescale hardware
> > > > > > engineers, if possible?
> > > > > 
> > > > > Richard is on the CC. Richard, can you please confirm those?
> > > > 
> > > > [Richard] One Pericom PI7C9X2G303EL pcie switch, and two pcie ep
> > > > deivces(one is intel e1000e nic, the other is one xhci device) are
> > > > tested on imx6q sabresd board. Without removing outbound io/mem
> > > > regions view map during the cfg0/1 read/write cycle, both of these
> > > > devices can't work well at my side. Works well after remove them
> > > > during the cfg0/1 read/write cycles.
> > > > Same to Tim's situation about switch implementation.
> > > 
> > > Thanks Richard! I am just wondering what kind of conclusion can we
> > > draw from this. Let me reiterate what we have:
> > > 
> > > - i.MX6 has 4 inbound and 4 outbound iATU regions, is that correct,
> > > Richard? - If we program the iATU regions, the i.MX6 PCIe doesn't work
> > > with switch.
> > > 
> > > Is there some special sause to the iATU on MX6 maybe ?
> > 
> > [Richard] welcome, it's correct, imx6 pcie has 4 inbount and 4 outbound
> > iATU regions.
> > Imx6 pcie doesn't work with switch if the following view switches are not
> > removed in The cfg0/1 read/write cycles.
> > 
> > @@ -516,9 +560,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port
> > *pp, struct pci_bus *bus,
> > 
> >         if (bus->parent->number == pp->root_bus_nr) {
> >         
> >                 dw_pcie_prog_viewport_cfg0(pp, busdev);
> >                 ret = cfg_read(pp->va_cfg0_base + address, where, size,
> >                 val);
> > 
> > +               // dw_pcie_prog_viewport_mem_outbound(pp);
> > 
> >         } else {
> >         
> >                 dw_pcie_prog_viewport_cfg1(pp, busdev);
> >                 ret = cfg_read(pp->va_cfg1_base + address, where, size,
> >                 val);
> > 
> > +               // dw_pcie_prog_viewport_io_outbound(pp);
> > 
> >         }
> >         
> >         return ret;
> > 
> > @@ -537,9 +583,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port
> > *pp, struct pci_bus *bus,
> > 
> >         if (bus->parent->number == pp->root_bus_nr) {
> >         
> >                 dw_pcie_prog_viewport_cfg0(pp, busdev);
> >                 ret = cfg_write(pp->va_cfg0_base + address, where, size,
> >                 val);
> > 
> > +               // dw_pcie_prog_viewport_mem_outbound(pp);
> > 
> >         } else {
> >         
> >                 dw_pcie_prog_viewport_cfg1(pp, busdev);
> >                 ret = cfg_write(pp->va_cfg1_base + address, where, size,
> >                 val);
> > 
> > +               // dw_pcie_prog_viewport_io_outbound(pp);
> > 
> >         }
> >         
> >         return ret;
> > 
> > Best Regards
> > Richard Zhu
> 
> [Richard] Bisect the code-modifications, only the
> dw_pcie_prog_viewport_io_outbound() should be masked if I want the switch
> works in my imx6 pcie switch support tests. Suspect that the mem emulated
> io space introduce this problem, debug is on-going.

Freescale is now looking into this issue ? Cool, thanks! :)

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Marek Vasut]

Some boards do not have a PCIe reset GPIO. To avoid probe
failure on these boards, make the reset GPIO optional as
well.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Frank Li <lznuaa@gmail.com>
Cc: Harro Haan <hrhaan@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
Cc: Tim Harvey <tharvey@gateworks.com>
Cc: Troy Kisky <troy.kisky@boundarydevices.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
 drivers/pci/host/pci-imx6.c                        | 29 +++++++++++-----------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index d5d26d4..b7a2279 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -19,9 +19,9 @@ Required properties:
 	to define the mapping of the PCIe interface to interrupt
 	numbers.
 - num-lanes: number of lanes to use
-- reset-gpio: gpio pin number of power good signal
 
 Optional properties for fsl,imx6q-pcie
+- reset-gpio: gpio pin number of power good signal
 - power-on-gpio: gpio pin number of power-enable signal
 - wake-up-gpio: gpio pin number of incoming wakeup signal
 - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index bd70af8..52027ad 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
 			IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
 
-	gpio_set_value(imx6_pcie->reset_gpio, 0);
-	msleep(100);
-	gpio_set_value(imx6_pcie->reset_gpio, 1);
+	/* Some boards don't have PCIe reset GPIO. */
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		gpio_set_value(imx6_pcie->reset_gpio, 0);
+		msleep(100);
+		gpio_set_value(imx6_pcie->reset_gpio, 1);
+	}
 
 	return 0;
 }
@@ -439,17 +442,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 
 	/* Fetch GPIOs */
 	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
-	if (!gpio_is_valid(imx6_pcie->reset_gpio)) {
-		dev_err(&pdev->dev, "no reset-gpio defined\n");
-		ret = -ENODEV;
-	}
-	ret = devm_gpio_request_one(&pdev->dev,
-				imx6_pcie->reset_gpio,
-				GPIOF_OUT_INIT_LOW,
-				"PCIe reset");
-	if (ret) {
-		dev_err(&pdev->dev, "unable to get reset gpio\n");
-		goto err;
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		ret = devm_gpio_request_one(&pdev->dev,
+					imx6_pcie->reset_gpio,
+					GPIOF_OUT_INIT_LOW,
+					"PCIe reset");
+		if (ret) {
+			dev_err(&pdev->dev, "unable to get reset gpio\n");
+			goto err;
+		}
 	}
 
 	imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0);
-- 
1.8.4.3

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Jingoo Han]

On Wednesday, December 11, 2013 7:30 PM, Marek Vasut wrote:
> 
> Some boards do not have a PCIe reset GPIO. To avoid probe
> failure on these boards, make the reset GPIO optional as
> well.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Frank Li <lznuaa@gmail.com>
> Cc: Harro Haan <hrhaan@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> Cc: Pratyush Anand <pratyush.anand@st.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> Cc: Tim Harvey <tharvey@gateworks.com>
> Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> ---
>  .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
>  drivers/pci/host/pci-imx6.c                        | 29 +++++++++++-----------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index d5d26d4..b7a2279 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -19,9 +19,9 @@ Required properties:
>  	to define the mapping of the PCIe interface to interrupt
>  	numbers.
>  - num-lanes: number of lanes to use
> -- reset-gpio: gpio pin number of power good signal
> 
>  Optional properties for fsl,imx6q-pcie
> +- reset-gpio: gpio pin number of power good signal

(+cc Kishon Vijay Abraham I)

As I said earlier,
'reset-gpio' property can be used for 'samsung,exynos5440-pcie'.
Thus, it is not specific for 'fsl,imx6q-pcie'.

Please, modify it as below:

--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -19,6 +19,8 @@ Required properties:
        to define the mapping of the PCIe interface to interrupt
        numbers.
 - num-lanes: number of lanes to use
+
+Optional properties
 - reset-gpio: gpio pin number of power good signal

 Optional properties for fsl,imx6q-pcie

Best regards,
Jingoo Han

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Tim Harvey]

On Wed, Dec 11, 2013 at 2:30 AM, Marek Vasut <marex@denx.de> wrote:
>
> Some boards do not have a PCIe reset GPIO. To avoid probe
> failure on these boards, make the reset GPIO optional as
> well.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Frank Li <lznuaa@gmail.com>
> Cc: Harro Haan <hrhaan@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> Cc: Pratyush Anand <pratyush.anand@st.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> Cc: Tim Harvey <tharvey@gateworks.com>
> Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> ---
>  .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
>  drivers/pci/host/pci-imx6.c                        | 29 +++++++++++-----------
>  2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index d5d26d4..b7a2279 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -19,9 +19,9 @@ Required properties:
>         to define the mapping of the PCIe interface to interrupt
>         numbers.
>  - num-lanes: number of lanes to use
> -- reset-gpio: gpio pin number of power good signal
>
>  Optional properties for fsl,imx6q-pcie
> +- reset-gpio: gpio pin number of power good signal
>  - power-on-gpio: gpio pin number of power-enable signal
>  - wake-up-gpio: gpio pin number of incoming wakeup signal
>  - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index bd70af8..52027ad 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
>         regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
>                         IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
>
> -       gpio_set_value(imx6_pcie->reset_gpio, 0);
> -       msleep(100);
> -       gpio_set_value(imx6_pcie->reset_gpio, 1);
> +       /* Some boards don't have PCIe reset GPIO. */
> +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +               gpio_set_value(imx6_pcie->reset_gpio, 0);
> +               msleep(100);
> +               gpio_set_value(imx6_pcie->reset_gpio, 1);
> +       }
>
>         return 0;
>  }

Marek,

Though not the fault of your patch, I noticed while looking at this
that the PCI Express specification is not being properly met with
regards to PERST# and the reference clock.  The spec states that
PERST# must be kept asserted until after the reference clock is stable
(I'm not entirely clear how long of a delay is needed for the clock to
become stable but I think the value is typically the 100ms).  I see in
the current pci-imx6.c code that imx6_pcie_host_init calls
imx6_pcie_assert_core_reset first, then imx6_pcie_init_phy, followed
by imx6_pcie_deassert_core_reset.  Despite the function names,
imx6_pcie_assert_core_reset as shown above asserts then de-asserts
PERST# before the clock is enabled in imx6_pcie_deassert_core_reset.
This seems to me to be a violation of the spec and I believe the
msleep(100) and de-assertion of the option reset_gpio should be done
in imx6_pcie_deassert_core reset after the clock is brought up.

If you agree with my assessment, would you mind resolving this issue
at the same time?  If not, I'm happy to follow-up with a patch to
resolve it after your patch is accepted.

Thanks,

Tim

> @@ -439,17 +442,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>
>         /* Fetch GPIOs */
>         imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> -       if (!gpio_is_valid(imx6_pcie->reset_gpio)) {
> -               dev_err(&pdev->dev, "no reset-gpio defined\n");
> -               ret = -ENODEV;
> -       }
> -       ret = devm_gpio_request_one(&pdev->dev,
> -                               imx6_pcie->reset_gpio,
> -                               GPIOF_OUT_INIT_LOW,
> -                               "PCIe reset");
> -       if (ret) {
> -               dev_err(&pdev->dev, "unable to get reset gpio\n");
> -               goto err;
> +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +               ret = devm_gpio_request_one(&pdev->dev,
> +                                       imx6_pcie->reset_gpio,
> +                                       GPIOF_OUT_INIT_LOW,
> +                                       "PCIe reset");
> +               if (ret) {
> +                       dev_err(&pdev->dev, "unable to get reset gpio\n");
> +                       goto err;
> +               }
>         }
>
>         imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0);
> --
> 1.8.4.3
>

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Marek Vasut]

On Thursday, December 12, 2013 at 06:10:31 AM, Tim Harvey wrote:
> On Wed, Dec 11, 2013 at 2:30 AM, Marek Vasut <marex@denx.de> wrote:
> > Some boards do not have a PCIe reset GPIO. To avoid probe
> > failure on these boards, make the reset GPIO optional as
> > well.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Frank Li <lznuaa@gmail.com>
> > Cc: Harro Haan <hrhaan@gmail.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> > Cc: Pratyush Anand <pratyush.anand@st.com>
> > Cc: Richard Zhu <r65037@freescale.com>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Sean Cross <xobs@kosagi.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> > Cc: Tim Harvey <tharvey@gateworks.com>
> > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > ---
> > 
> >  .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
> >  drivers/pci/host/pci-imx6.c                        | 29
> >  +++++++++++----------- 2 files changed, 16 insertions(+), 15
> >  deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > b/Documentation/devicetree/bindings/pci/designware-pcie.txt index
> > d5d26d4..b7a2279 100644
> > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > 
> > @@ -19,9 +19,9 @@ Required properties:
> >         to define the mapping of the PCIe interface to interrupt
> >         numbers.
> >  
> >  - num-lanes: number of lanes to use
> > 
> > -- reset-gpio: gpio pin number of power good signal
> > 
> >  Optional properties for fsl,imx6q-pcie
> > 
> > +- reset-gpio: gpio pin number of power good signal
> > 
> >  - power-on-gpio: gpio pin number of power-enable signal
> >  - wake-up-gpio: gpio pin number of incoming wakeup signal
> >  - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable
> >  signal
> > 
> > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> > index bd70af8..52027ad 100644
> > --- a/drivers/pci/host/pci-imx6.c
> > +++ b/drivers/pci/host/pci-imx6.c
> > @@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct
> > pcie_port *pp)
> > 
> >         regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> >         
> >                         IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > 
> > -       gpio_set_value(imx6_pcie->reset_gpio, 0);
> > -       msleep(100);
> > -       gpio_set_value(imx6_pcie->reset_gpio, 1);
> > +       /* Some boards don't have PCIe reset GPIO. */
> > +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > +               gpio_set_value(imx6_pcie->reset_gpio, 0);
> > +               msleep(100);
> > +               gpio_set_value(imx6_pcie->reset_gpio, 1);
> > +       }
> > 
> >         return 0;
> >  
> >  }
> 
> Marek,
> 
> Though not the fault of your patch, I noticed while looking at this
> that the PCI Express specification is not being properly met with
> regards to PERST# and the reference clock.  The spec states that
> PERST# must be kept asserted until after the reference clock is stable
> (I'm not entirely clear how long of a delay is needed for the clock to
> become stable but I think the value is typically the 100ms).  I see in
> the current pci-imx6.c code that imx6_pcie_host_init calls
> imx6_pcie_assert_core_reset first, then imx6_pcie_init_phy, followed
> by imx6_pcie_deassert_core_reset.  Despite the function names,
> imx6_pcie_assert_core_reset as shown above asserts then de-asserts
> PERST# before the clock is enabled in imx6_pcie_deassert_core_reset.
> This seems to me to be a violation of the spec and I believe the
> msleep(100) and de-assertion of the option reset_gpio should be done
> in imx6_pcie_deassert_core reset after the clock is brought up.
> 
> If you agree with my assessment, would you mind resolving this issue
> at the same time?  If not, I'm happy to follow-up with a patch to
> resolve it after your patch is accepted.

Is this not resolved by patch 0006 in this series please?

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Tim Harvey]

On Thu, Dec 12, 2013 at 2:22 AM, Marek Vasut <marex@denx.de> wrote:
>
> On Thursday, December 12, 2013 at 06:10:31 AM, Tim Harvey wrote:
> > On Wed, Dec 11, 2013 at 2:30 AM, Marek Vasut <marex@denx.de> wrote:
> > > Some boards do not have a PCIe reset GPIO. To avoid probe
> > > failure on these boards, make the reset GPIO optional as
> > > well.
> > >
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Frank Li <lznuaa@gmail.com>
> > > Cc: Harro Haan <hrhaan@gmail.com>
> > > Cc: Jingoo Han <jg1.han@samsung.com>
> > > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> > > Cc: Pratyush Anand <pratyush.anand@st.com>
> > > Cc: Richard Zhu <r65037@freescale.com>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Sean Cross <xobs@kosagi.com>
> > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> > > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> > > Cc: Tim Harvey <tharvey@gateworks.com>
> > > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> > > Cc: Yinghai Lu <yinghai@kernel.org>
> > > ---
> > >
> > >  .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
> > >  drivers/pci/host/pci-imx6.c                        | 29
> > >  +++++++++++----------- 2 files changed, 16 insertions(+), 15
> > >  deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > > b/Documentation/devicetree/bindings/pci/designware-pcie.txt index
> > > d5d26d4..b7a2279 100644
> > > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > >
> > > @@ -19,9 +19,9 @@ Required properties:
> > >         to define the mapping of the PCIe interface to interrupt
> > >         numbers.
> > >
> > >  - num-lanes: number of lanes to use
> > >
> > > -- reset-gpio: gpio pin number of power good signal
> > >
> > >  Optional properties for fsl,imx6q-pcie
> > >
> > > +- reset-gpio: gpio pin number of power good signal
> > >
> > >  - power-on-gpio: gpio pin number of power-enable signal
> > >  - wake-up-gpio: gpio pin number of incoming wakeup signal
> > >  - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable
> > >  signal
> > >
> > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> > > index bd70af8..52027ad 100644
> > > --- a/drivers/pci/host/pci-imx6.c
> > > +++ b/drivers/pci/host/pci-imx6.c
> > > @@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct
> > > pcie_port *pp)
> > >
> > >         regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > >
> > >                         IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > >
> > > -       gpio_set_value(imx6_pcie->reset_gpio, 0);
> > > -       msleep(100);
> > > -       gpio_set_value(imx6_pcie->reset_gpio, 1);
> > > +       /* Some boards don't have PCIe reset GPIO. */
> > > +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > > +               gpio_set_value(imx6_pcie->reset_gpio, 0);
> > > +               msleep(100);
> > > +               gpio_set_value(imx6_pcie->reset_gpio, 1);
> > > +       }
> > >
> > >         return 0;
> > >
> > >  }
> >
> > Marek,
> >
> > Though not the fault of your patch, I noticed while looking at this
> > that the PCI Express specification is not being properly met with
> > regards to PERST# and the reference clock.  The spec states that
> > PERST# must be kept asserted until after the reference clock is stable
> > (I'm not entirely clear how long of a delay is needed for the clock to
> > become stable but I think the value is typically the 100ms).  I see in
> > the current pci-imx6.c code that imx6_pcie_host_init calls
> > imx6_pcie_assert_core_reset first, then imx6_pcie_init_phy, followed
> > by imx6_pcie_deassert_core_reset.  Despite the function names,
> > imx6_pcie_assert_core_reset as shown above asserts then de-asserts
> > PERST# before the clock is enabled in imx6_pcie_deassert_core_reset.
> > This seems to me to be a violation of the spec and I believe the
> > msleep(100) and de-assertion of the option reset_gpio should be done
> > in imx6_pcie_deassert_core reset after the clock is brought up.
> >
> > If you agree with my assessment, would you mind resolving this issue
> > at the same time?  If not, I'm happy to follow-up with a patch to
> > resolve it after your patch is accepted.
>
> Is this not resolved by patch 0006 in this series please?

Marek,

Yes it is addressed there.  Sorry - I missed that.

Tim

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Marek Vasut]

On Thursday, December 12, 2013 at 07:16:18 PM, Tim Harvey wrote:
> On Thu, Dec 12, 2013 at 2:22 AM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, December 12, 2013 at 06:10:31 AM, Tim Harvey wrote:
> > > On Wed, Dec 11, 2013 at 2:30 AM, Marek Vasut <marex@denx.de> wrote:
> > > > Some boards do not have a PCIe reset GPIO. To avoid probe
> > > > failure on these boards, make the reset GPIO optional as
> > > > well.
> > > > 
> > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Frank Li <lznuaa@gmail.com>
> > > > Cc: Harro Haan <hrhaan@gmail.com>
> > > > Cc: Jingoo Han <jg1.han@samsung.com>
> > > > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> > > > Cc: Pratyush Anand <pratyush.anand@st.com>
> > > > Cc: Richard Zhu <r65037@freescale.com>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Sean Cross <xobs@kosagi.com>
> > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Siva Reddy Kallam <siva.kallam@samsung.com>
> > > > Cc: Srikanth T Shivanand <ts.srikanth@samsung.com>
> > > > Cc: Tim Harvey <tharvey@gateworks.com>
> > > > Cc: Troy Kisky <troy.kisky@boundarydevices.com>
> > > > Cc: Yinghai Lu <yinghai@kernel.org>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/pci/designware-pcie.txt    |  2 +-
> > > >  drivers/pci/host/pci-imx6.c                        | 29
> > > >  +++++++++++----------- 2 files changed, 16 insertions(+), 15
> > > >  deletions(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > > > b/Documentation/devicetree/bindings/pci/designware-pcie.txt index
> > > > d5d26d4..b7a2279 100644
> > > > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > > > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > > > 
> > > > @@ -19,9 +19,9 @@ Required properties:
> > > >         to define the mapping of the PCIe interface to interrupt
> > > >         numbers.
> > > >  
> > > >  - num-lanes: number of lanes to use
> > > > 
> > > > -- reset-gpio: gpio pin number of power good signal
> > > > 
> > > >  Optional properties for fsl,imx6q-pcie
> > > > 
> > > > +- reset-gpio: gpio pin number of power good signal
> > > > 
> > > >  - power-on-gpio: gpio pin number of power-enable signal
> > > >  - wake-up-gpio: gpio pin number of incoming wakeup signal
> > > >  - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable
> > > >  signal
> > > > 
> > > > diff --git a/drivers/pci/host/pci-imx6.c
> > > > b/drivers/pci/host/pci-imx6.c index bd70af8..52027ad 100644
> > > > --- a/drivers/pci/host/pci-imx6.c
> > > > +++ b/drivers/pci/host/pci-imx6.c
> > > > @@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct
> > > > pcie_port *pp)
> > > > 
> > > >         regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > > >         
> > > >                         IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > > > 
> > > > -       gpio_set_value(imx6_pcie->reset_gpio, 0);
> > > > -       msleep(100);
> > > > -       gpio_set_value(imx6_pcie->reset_gpio, 1);
> > > > +       /* Some boards don't have PCIe reset GPIO. */
> > > > +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > > > +               gpio_set_value(imx6_pcie->reset_gpio, 0);
> > > > +               msleep(100);
> > > > +               gpio_set_value(imx6_pcie->reset_gpio, 1);
> > > > +       }
> > > > 
> > > >         return 0;
> > > >  
> > > >  }
> > > 
> > > Marek,
> > > 
> > > Though not the fault of your patch, I noticed while looking at this
> > > that the PCI Express specification is not being properly met with
> > > regards to PERST# and the reference clock.  The spec states that
> > > PERST# must be kept asserted until after the reference clock is stable
> > > (I'm not entirely clear how long of a delay is needed for the clock to
> > > become stable but I think the value is typically the 100ms).  I see in
> > > the current pci-imx6.c code that imx6_pcie_host_init calls
> > > imx6_pcie_assert_core_reset first, then imx6_pcie_init_phy, followed
> > > by imx6_pcie_deassert_core_reset.  Despite the function names,
> > > imx6_pcie_assert_core_reset as shown above asserts then de-asserts
> > > PERST# before the clock is enabled in imx6_pcie_deassert_core_reset.
> > > This seems to me to be a violation of the spec and I believe the
> > > msleep(100) and de-assertion of the option reset_gpio should be done
> > > in imx6_pcie_deassert_core reset after the clock is brought up.
> > > 
> > > If you agree with my assessment, would you mind resolving this issue
> > > at the same time?  If not, I'm happy to follow-up with a patch to
> > > resolve it after your patch is accepted.
> > 
> > Is this not resolved by patch 0006 in this series please?
> 
> Marek,
> 
> Yes it is addressed there.  Sorry - I missed that.

No problem, thanks for checking it :)

Bjorn, would you mind picking 2/7...6/7 of this series so we are done with 
those? I will re-post 1/7 as a self-standing patch afterwards, it has no impact 
on the rest of the series.

Shawn, can you please pick 7/7 ?

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 11:25 AM, Marek Vasut <marex@denx.de> wrote:
> Bjorn, would you mind picking 2/7...6/7 of this series so we are done with
> those? I will re-post 1/7 as a self-standing patch afterwards, it has no impact
> on the rest of the series.

Sure.  I can't really evaluate these myself, so I'm looking for acks
from Richard or Shawn.

I noticed Pratyush, Tim, and Harro had some questions the first time
around, and I don't know whether they got resolved.

Bjorn

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Bjorn Helgaas]

On Thu, Dec 12, 2013 at 2:07 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Dec 12, 2013 at 11:25 AM, Marek Vasut <marex@denx.de> wrote:
>> Bjorn, would you mind picking 2/7...6/7 of this series so we are done with
>> those? I will re-post 1/7 as a self-standing patch afterwards, it has no impact
>> on the rest of the series.
>
> Sure.  I can't really evaluate these myself, so I'm looking for acks
> from Richard or Shawn.
>
> I noticed Pratyush, Tim, and Harro had some questions the first time
> around, and I don't know whether they got resolved.

By the way, I tried to apply patches 2-6 to my pci/host-imx6 branch,
but patch 6 ("PCI: imx6: Fix bugs in PCIe startup code") didn't apply
cleanly.

I pushed the branch to
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-imx6
if you want to try it yourself.

Bjorn

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Marek Vasut]

On Thursday, December 12, 2013 at 10:20:30 PM, Bjorn Helgaas wrote:
> On Thu, Dec 12, 2013 at 2:07 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Thu, Dec 12, 2013 at 11:25 AM, Marek Vasut <marex@denx.de> wrote:
> >> Bjorn, would you mind picking 2/7...6/7 of this series so we are done
> >> with those? I will re-post 1/7 as a self-standing patch afterwards, it
> >> has no impact on the rest of the series.
> > 
> > Sure.  I can't really evaluate these myself, so I'm looking for acks
> > from Richard or Shawn.
> > 
> > I noticed Pratyush, Tim, and Harro had some questions the first time
> > around, and I don't know whether they got resolved.
> 
> By the way, I tried to apply patches 2-6 to my pci/host-imx6 branch,
> but patch 6 ("PCI: imx6: Fix bugs in PCIe startup code") didn't apply
> cleanly.
> 
> I pushed the branch to
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host
> -imx6 if you want to try it yourself.

That's because of Fabio's patch. I will rebase and repost, thanks.

Best regards,
Marek Vasut

[PATCH 1/7] PCI: imx6: Make reset-gpio optional

[Harro Haan]

On 12 December 2013 22:07, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Dec 12, 2013 at 11:25 AM, Marek Vasut <marex@denx.de> wrote:
>> Bjorn, would you mind picking 2/7...6/7 of this series so we are done with
>> those? I will re-post 1/7 as a self-standing patch afterwards, it has no impact
>> on the rest of the series.
>
> Sure.  I can't really evaluate these myself, so I'm looking for acks
> from Richard or Shawn.
>
> I noticed Pratyush, Tim, and Harro had some questions the first time
> around, and I don't know whether they got resolved.

Yes, Marek resolved the build warning comment in [PATCH V2 3/7]. Regards, Harro