[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Tim Harvey]

An add-in card used on the Ventana IMX6 SoC based family of boards has a
TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings between the
bridge and the four mini-PCI slots are swapped (INTD/C/B/A).

This series adds legacy PCI interrupt mapping to support PCI bridges on
IMX6 PCIe host controllers, adds the ability to use a host-specific
custom swizzle function, then uses that to fix the IRQ mapping.

I'm hoping this is the right way to perform such a workaround - please provide
feedback if not.  Patch1 will likely collide with the owrk Lucas is doing
with device-tree bindings and MSI support.  I'm happy to re-base if/when
needed.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Jingoo Han <jg1.han@samsung.com>
---
Tim Harvey (3):
  PCI: designware: add legacy PCI interrupt mapping
  PCI: designware: add ability for custom swizzle
  PCI: imx6: ventana: fixup for IRQ mismapping

 arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
 drivers/pci/host/pci-exynos.c      |  7 ++++--
 drivers/pci/host/pci-imx6.c        | 47 ++++++++++++++++++++++++++++++++++----
 drivers/pci/host/pcie-designware.c | 11 ++++++++-
 drivers/pci/host/pcie-designware.h |  3 ++-
 include/linux/pci_ids.h            |  1 +
 6 files changed, 62 insertions(+), 9 deletions(-)

-- 
1.8.3.2

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Tim Harvey]

The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
This allows a PCIe-to-PCI bridge to function properly.

The irq field of the pcie_host struct is expanded to 4 interrupts to
allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
all from devicetree.  I'm not clear if the Exynos driver has this
capability so it places the same interrupt in all 4 slots.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Jingoo Han <jg1.han@samsung.com>
---
 arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
 drivers/pci/host/pci-exynos.c      |  7 +++++--
 drivers/pci/host/pci-imx6.c        | 11 +++++++----
 drivers/pci/host/pcie-designware.c |  8 +++++++-
 drivers/pci/host/pcie-designware.h |  2 +-
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index eed6897..fad8d90 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -126,7 +126,7 @@
 				  0x81000000 0 0          0x01f80000 0 0x00010000 /* downstream I/O */
 				  0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable memory */
 			num-lanes = <1>;
-			interrupts = <0 123 0x04>;
+			interrupts = <0 123 0x04>, <0 122 0x04>, <0 121 0x04>, <0 120 0x04>;
 			clocks = <&clks 189>, <&clks 187>, <&clks 206>, <&clks 144>;
 			clock-names = "pcie_ref_125m", "sata_ref_100m", "lvds_gate", "pcie_axi";
 			status = "disabled";
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index 3de6bfb..d85dcb0 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -515,11 +515,14 @@ static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
 {
 	int ret;
 
-	pp->irq = platform_get_irq(pdev, 1);
-	if (!pp->irq) {
+	pp->irq[0] = platform_get_irq(pdev, 1);
+	if (!pp->irq[0]) {
 		dev_err(&pdev->dev, "failed to get irq\n");
 		return -ENODEV;
 	}
+	pp->irq[1] = pp->irq[0];
+	pp->irq[2] = pp->irq[0];
+	pp->irq[3] = pp->irq[0];
 	ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
 				IRQF_SHARED, "exynos-pcie", pp);
 	if (ret) {
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index e8663a8..aaa05c8 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -470,11 +470,14 @@ static int imx6_add_pcie_port(struct pcie_port *pp,
 			struct platform_device *pdev)
 {
 	int ret;
+	int i;
 
-	pp->irq = platform_get_irq(pdev, 0);
-	if (!pp->irq) {
-		dev_err(&pdev->dev, "failed to get irq\n");
-		return -ENODEV;
+	for (i = 0; i < 4; i++) {
+		pp->irq[i] = platform_get_irq(pdev, i);
+		if (!pp->irq[i]) {
+			dev_err(&pdev->dev, "failed to get irq%d\n", i);
+			return -ENODEV;
+		}
 	}
 
 	pp->root_bus_nr = -1;
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 17ce88f..5808177 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -738,8 +738,14 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
+	int irq = -1;
 
-	return pp->irq;
+	if (pin > 0 && pin < 5)
+		irq = pp->irq[pin - 1];
+	dev_info(&dev->dev, "map_irq: %04x:%04x slot%d pin%d irq%d\n",
+		dev->vendor, dev->device, slot, pin, irq);
+
+	return irq;
 }
 
 static void dw_pcie_add_bus(struct pci_bus *bus)
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 3063b35..5c596c0 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -46,7 +46,7 @@ struct pcie_port {
 	struct resource		io;
 	struct resource		mem;
 	struct pcie_port_info	config;
-	int			irq;
+	int			irq[4];
 	u32			lanes;
 	struct pcie_host_ops	*ops;
 	int			msi_irq;
-- 
1.8.3.2

[PATCH 2/3] PCI: designware: add ability for custom swizzle

[Tim Harvey]

Add the ability for a platform driver to provide a host-specific
swizzle function.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Jingoo Han <jg1.han@samsung.com>
---
 drivers/pci/host/pcie-designware.c | 3 +++
 drivers/pci/host/pcie-designware.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 5808177..a143f2a 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -480,6 +480,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 	if (pp->ops->host_init)
 		pp->ops->host_init(pp);
 
+	if (pp->swizzle)
+		dw_pci.swizzle = pp->swizzle;
+
 	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
 
 	/* program correct class for RC */
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 5c596c0..8631d56 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -53,6 +53,7 @@ struct pcie_port {
 	struct irq_domain	*irq_domain;
 	unsigned long		msi_data;
 	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
+	u8			(*swizzle)(struct pci_dev *, u8 *);
 };
 
 struct pcie_host_ops {
-- 
1.8.3.2

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Tim Harvey]

The TI XIO2001 PCIe-to-PCI bridge used on Ventana expansion boards
has its slot-to-bridge IRQ mapping reversed from the PCI specification:

  INTA->INTD
  INTB->INTC
  INTC->INTB
  INTD->INTA

Implement a custom swizzle function that does a fixup on the interrupt for
devices on the first TI XIO2001 bridge in the tree.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Sean Cross <xobs@kosagi.com>
Cc: Jingoo Han <jg1.han@samsung.com>
---
 drivers/pci/host/pci-imx6.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h     |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index aaa05c8..b171a21 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -493,6 +493,39 @@ static int imx6_add_pcie_port(struct pcie_port *pp,
 	return 0;
 }
 
+/* TI XIO2001 PCIe-to-PCI bridge on GW16082 exp card has IRQs reversed */
+u8 ventana_swizzle(struct pci_dev *dev, u8 *pin)
+{
+	u8 i = 0;
+	struct pci_dev *pdev = dev;
+
+	/* count number of TI XIO2001 bridges on bus */
+	while (!pci_is_root_bus(pdev->bus)) {
+		if (pdev->bus && pdev->bus->self &&
+		    (pdev->bus->self->vendor == PCI_VENDOR_ID_TI) &&
+		    (pdev->bus->self->device == PCI_DEVICE_ID_TI_XIO2001)) {
+			i++;
+		}
+		pdev = pdev->bus->self;
+	}
+	while (!pci_is_root_bus(dev->bus)) {
+		/* if we are directly downstream from 1st TI XIO2001 bridge */
+		if (dev->bus && dev->bus->self &&
+		    (dev->bus->self->vendor == PCI_VENDOR_ID_TI) &&
+		    (dev->bus->self->device == PCI_DEVICE_ID_TI_XIO2001)) {
+			if (--i == 0) {
+				/* swap IRQs and swizzle backwards */
+				*pin = (15 - PCI_SLOT(dev->devfn)) + 1;
+				dev = dev->bus->self;
+				continue;
+			}
+		}
+		*pin = pci_swizzle_interrupt_pin(dev, *pin);
+		dev = dev->bus->self;
+	}
+	return PCI_SLOT(dev->devfn);
+}
+
 static int __init imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct imx6_pcie *imx6_pcie;
@@ -601,6 +634,9 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(imx6_pcie->iomuxc_gpr);
 	}
 
+	if (of_machine_is_compatible("gw,ventana"))
+		pp->swizzle = ventana_swizzle;
+
 	ret = imx6_add_pcie_port(pp, pdev);
 	if (ret < 0)
 		return ret;
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 97fbecd..4ca334f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -822,6 +822,7 @@
 #define PCI_DEVICE_ID_TI_XX12		0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
+#define PCI_DEVICE_ID_TI_XIO2001	0x8240
 #define PCI_DEVICE_ID_TI_1130		0xac12
 #define PCI_DEVICE_ID_TI_1031		0xac13
 #define PCI_DEVICE_ID_TI_1131		0xac15
-- 
1.8.3.2

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Jingoo Han]

On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> 
> An add-in card used on the Ventana IMX6 SoC based family of boards has a
> TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings between the
> bridge and the four mini-PCI slots are swapped (INTD/C/B/A).

(+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)

This problem happens from the 'Board', not a 'SoC'.
'TI XIO2001 PCIe-to-PCI bridge' is not a 'SoC'.
'pci-imx6.c' is the driver for 'IMX6 PCI IP', not for 'IMX6 SoC based
board'. Isn't it?

Then, the workaround code for board problem should NOT be
included to './drivers/pci/host/' side.

Also, please add the following members to CC list. They will give
important comments.

: Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG

Best regards,
Jingoo Han

> 
> This series adds legacy PCI interrupt mapping to support PCI bridges on
> IMX6 PCIe host controllers, adds the ability to use a host-specific
> custom swizzle function, then uses that to fix the IRQ mapping.
> 
> I'm hoping this is the right way to perform such a workaround - please provide
> feedback if not.  Patch1 will likely collide with the owrk Lucas is doing
> with device-tree bindings and MSI support.  I'm happy to re-base if/when
> needed.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> ---
> Tim Harvey (3):
>   PCI: designware: add legacy PCI interrupt mapping
>   PCI: designware: add ability for custom swizzle
>   PCI: imx6: ventana: fixup for IRQ mismapping
> 
>  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
>  drivers/pci/host/pci-exynos.c      |  7 ++++--
>  drivers/pci/host/pci-imx6.c        | 47 ++++++++++++++++++++++++++++++++++----
>  drivers/pci/host/pcie-designware.c | 11 ++++++++-
>  drivers/pci/host/pcie-designware.h |  3 ++-
>  include/linux/pci_ids.h            |  1 +
>  6 files changed, 62 insertions(+), 9 deletions(-)
> 
> --
> 1.8.3.2

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Jingoo Han]

On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> 
> The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
> This allows a PCIe-to-PCI bridge to function properly.
> 
> The irq field of the pcie_host struct is expanded to 4 interrupts to
> allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
> all from devicetree.  I'm not clear if the Exynos driver has this
> capability so it places the same interrupt in all 4 slots.

(+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)

In the case of Exynos,
'INTA/B/C/D' are mapped to only one interrupt (<0 20 0>).
Thus, the current code works properly on Exynos platform.

There are three interrupts for Exynos PCIe; INTx, MSI, PHY Link,
respectively as below. 

./arch/arm/boot/dts/exynos5440.dtsi
	interrupts = <0 20 0>, <0 21 0>, <0 22 0>;

<0 20 0>: PCIe RC0 pulse interrupt,
             INTA, INTB, INTC and INTD, etc
<0 21 0>: PCIe RC0 level interrupt,
             MSI, etc
<0 22 0>: PCIe RC0 special interrupt,
             PHY Link related interrupts, etc

Of course, legacy INTx is handled as message only.

Mohit, Kishon,
How about the other SoCs? INTx is mapped to single interrupt
such as Exynos, or separate interrupts such as i.MX6?

Best regards,
Jingoo Han

> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
>  drivers/pci/host/pci-exynos.c      |  7 +++++--
>  drivers/pci/host/pci-imx6.c        | 11 +++++++----
>  drivers/pci/host/pcie-designware.c |  8 +++++++-
>  drivers/pci/host/pcie-designware.h |  2 +-
>  5 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index eed6897..fad8d90 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -126,7 +126,7 @@
>  				  0x81000000 0 0          0x01f80000 0 0x00010000 /* downstream I/O */
>  				  0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable
> memory */
>  			num-lanes = <1>;
> -			interrupts = <0 123 0x04>;
> +			interrupts = <0 123 0x04>, <0 122 0x04>, <0 121 0x04>, <0 120 0x04>;
>  			clocks = <&clks 189>, <&clks 187>, <&clks 206>, <&clks 144>;
>  			clock-names = "pcie_ref_125m", "sata_ref_100m", "lvds_gate", "pcie_axi";
>  			status = "disabled";
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index 3de6bfb..d85dcb0 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -515,11 +515,14 @@ static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
>  {
>  	int ret;
> 
> -	pp->irq = platform_get_irq(pdev, 1);
> -	if (!pp->irq) {
> +	pp->irq[0] = platform_get_irq(pdev, 1);
> +	if (!pp->irq[0]) {
>  		dev_err(&pdev->dev, "failed to get irq\n");
>  		return -ENODEV;
>  	}
> +	pp->irq[1] = pp->irq[0];
> +	pp->irq[2] = pp->irq[0];
> +	pp->irq[3] = pp->irq[0];
>  	ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
>  				IRQF_SHARED, "exynos-pcie", pp);
>  	if (ret) {
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index e8663a8..aaa05c8 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -470,11 +470,14 @@ static int imx6_add_pcie_port(struct pcie_port *pp,
>  			struct platform_device *pdev)
>  {
>  	int ret;
> +	int i;
> 
> -	pp->irq = platform_get_irq(pdev, 0);
> -	if (!pp->irq) {
> -		dev_err(&pdev->dev, "failed to get irq\n");
> -		return -ENODEV;
> +	for (i = 0; i < 4; i++) {
> +		pp->irq[i] = platform_get_irq(pdev, i);
> +		if (!pp->irq[i]) {
> +			dev_err(&pdev->dev, "failed to get irq%d\n", i);
> +			return -ENODEV;
> +		}
>  	}
> 
>  	pp->root_bus_nr = -1;
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 17ce88f..5808177 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -738,8 +738,14 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>  static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>  {
>  	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
> +	int irq = -1;
> 
> -	return pp->irq;
> +	if (pin > 0 && pin < 5)
> +		irq = pp->irq[pin - 1];
> +	dev_info(&dev->dev, "map_irq: %04x:%04x slot%d pin%d irq%d\n",
> +		dev->vendor, dev->device, slot, pin, irq);
> +
> +	return irq;
>  }
> 
>  static void dw_pcie_add_bus(struct pci_bus *bus)
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index 3063b35..5c596c0 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -46,7 +46,7 @@ struct pcie_port {
>  	struct resource		io;
>  	struct resource		mem;
>  	struct pcie_port_info	config;
> -	int			irq;
> +	int			irq[4];
>  	u32			lanes;
>  	struct pcie_host_ops	*ops;
>  	int			msi_irq;
> --
> 1.8.3.2

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Shawn Guo]

On Thu, Feb 27, 2014 at 05:19:52PM -0800, Tim Harvey wrote:
> The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
> This allows a PCIe-to-PCI bridge to function properly.
> 
> The irq field of the pcie_host struct is expanded to 4 interrupts to
> allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
> all from devicetree.  I'm not clear if the Exynos driver has this
> capability so it places the same interrupt in all 4 slots.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-

This change should be in a separate patch that goes to IMX tree.  And
please be aware of Arnd's comment [1] there.

Shawn

[1] http://www.spinics.net/lists/arm-kernel/msg311615.html

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Jingoo Han]

On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> 
> The TI XIO2001 PCIe-to-PCI bridge used on Ventana expansion boards
> has its slot-to-bridge IRQ mapping reversed from the PCI specification:
> 
>   INTA->INTD
>   INTB->INTC
>   INTC->INTB
>   INTD->INTA

(+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)

As I mention earlier, 'i.MX6 PCIe IP' is not a culprit.
The 'TI XIO2001 PCIe-to-PCI bridge' is the culprit, right?

Then, './drivers/pci/host/pci-imx6.c' is not a good place
to fix the board problems.

Look at other i.MX6 boards. They don't have the 'TI XIO2001
PCIe-to-PCI bridge'. Please don't add this board specific 
code to SoC specific driver code.

Best regards,
Jingoo Han

> 
> Implement a custom swizzle function that does a fixup on the interrupt for
> devices on the first TI XIO2001 bridge in the tree.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Richard Zhu <r65037@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Sean Cross <xobs@kosagi.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> ---
>  drivers/pci/host/pci-imx6.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h     |  1 +
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index aaa05c8..b171a21 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -493,6 +493,39 @@ static int imx6_add_pcie_port(struct pcie_port *pp,
>  	return 0;
>  }
> 
> +/* TI XIO2001 PCIe-to-PCI bridge on GW16082 exp card has IRQs reversed */
> +u8 ventana_swizzle(struct pci_dev *dev, u8 *pin)
> +{
> +	u8 i = 0;
> +	struct pci_dev *pdev = dev;
> +
> +	/* count number of TI XIO2001 bridges on bus */
> +	while (!pci_is_root_bus(pdev->bus)) {
> +		if (pdev->bus && pdev->bus->self &&
> +		    (pdev->bus->self->vendor == PCI_VENDOR_ID_TI) &&
> +		    (pdev->bus->self->device == PCI_DEVICE_ID_TI_XIO2001)) {
> +			i++;
> +		}
> +		pdev = pdev->bus->self;
> +	}
> +	while (!pci_is_root_bus(dev->bus)) {
> +		/* if we are directly downstream from 1st TI XIO2001 bridge */
> +		if (dev->bus && dev->bus->self &&
> +		    (dev->bus->self->vendor == PCI_VENDOR_ID_TI) &&
> +		    (dev->bus->self->device == PCI_DEVICE_ID_TI_XIO2001)) {
> +			if (--i == 0) {
> +				/* swap IRQs and swizzle backwards */
> +				*pin = (15 - PCI_SLOT(dev->devfn)) + 1;
> +				dev = dev->bus->self;
> +				continue;
> +			}
> +		}
> +		*pin = pci_swizzle_interrupt_pin(dev, *pin);
> +		dev = dev->bus->self;
> +	}
> +	return PCI_SLOT(dev->devfn);
> +}
> +
>  static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie;
> @@ -601,6 +634,9 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx6_pcie->iomuxc_gpr);
>  	}
> 
> +	if (of_machine_is_compatible("gw,ventana"))
> +		pp->swizzle = ventana_swizzle;
> +
>  	ret = imx6_add_pcie_port(pp, pdev);
>  	if (ret < 0)
>  		return ret;
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 97fbecd..4ca334f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -822,6 +822,7 @@
>  #define PCI_DEVICE_ID_TI_XX12		0x8039
>  #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
>  #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
> +#define PCI_DEVICE_ID_TI_XIO2001	0x8240
>  #define PCI_DEVICE_ID_TI_1130		0xac12
>  #define PCI_DEVICE_ID_TI_1031		0xac13
>  #define PCI_DEVICE_ID_TI_1131		0xac15
> --
> 1.8.3.2

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Tim Harvey]

On Thu, Feb 27, 2014 at 5:50 PM, Jingoo Han <jg1.han@samsung.com> wrote:
>
> On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> >
> > An add-in card used on the Ventana IMX6 SoC based family of boards has a
> > TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings between the
> > bridge and the four mini-PCI slots are swapped (INTD/C/B/A).
>
> (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
>
> This problem happens from the 'Board', not a 'SoC'.
> 'TI XIO2001 PCIe-to-PCI bridge' is not a 'SoC'.
> 'pci-imx6.c' is the driver for 'IMX6 PCI IP', not for 'IMX6 SoC based
> board'. Isn't it?

Jingoo,

Correct, this is an issue in the way the XIO2001 was hooked up to the
PCI slots, so it should be viewed as a board issue.

>
> Then, the workaround code for board problem should NOT be
> included to './drivers/pci/host/' side.

I would agree, but to overcome this sort of interrupt mapping issue
one would need to either implement a custom swizzle or perhaps a
custom map_irq and both of those are hooked into the pcie driver core.
 Do you have any suggestions on where/how I would better hook into
those?

>
> Also, please add the following members to CC list. They will give
> important comments.
>
> : Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG
>
> Best regards,
> Jingoo Han

Thanks,

Tim

>
> >
> > This series adds legacy PCI interrupt mapping to support PCI bridges on
> > IMX6 PCIe host controllers, adds the ability to use a host-specific
> > custom swizzle function, then uses that to fix the IRQ mapping.
> >
> > I'm hoping this is the right way to perform such a workaround - please provide
> > feedback if not.  Patch1 will likely collide with the owrk Lucas is doing
> > with device-tree bindings and MSI support.  I'm happy to re-base if/when
> > needed.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Richard Zhu <r65037@freescale.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Sean Cross <xobs@kosagi.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > ---
> > Tim Harvey (3):
> >   PCI: designware: add legacy PCI interrupt mapping
> >   PCI: designware: add ability for custom swizzle
> >   PCI: imx6: ventana: fixup for IRQ mismapping
> >
> >  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
> >  drivers/pci/host/pci-exynos.c      |  7 ++++--
> >  drivers/pci/host/pci-imx6.c        | 47 ++++++++++++++++++++++++++++++++++----
> >  drivers/pci/host/pcie-designware.c | 11 ++++++++-
> >  drivers/pci/host/pcie-designware.h |  3 ++-
> >  include/linux/pci_ids.h            |  1 +
> >  6 files changed, 62 insertions(+), 9 deletions(-)
> >
> > --
> > 1.8.3.2
>

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Tim Harvey]

On Thu, Feb 27, 2014 at 6:00 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
>>
>> The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
>> This allows a PCIe-to-PCI bridge to function properly.
>>
>> The irq field of the pcie_host struct is expanded to 4 interrupts to
>> allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
>> all from devicetree.  I'm not clear if the Exynos driver has this
>> capability so it places the same interrupt in all 4 slots.
>
> (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
>
> In the case of Exynos,
> 'INTA/B/C/D' are mapped to only one interrupt (<0 20 0>).
> Thus, the current code works properly on Exynos platform.
>
> There are three interrupts for Exynos PCIe; INTx, MSI, PHY Link,
> respectively as below.
>
> ./arch/arm/boot/dts/exynos5440.dtsi
>         interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
>
> <0 20 0>: PCIe RC0 pulse interrupt,
>              INTA, INTB, INTC and INTD, etc
> <0 21 0>: PCIe RC0 level interrupt,
>              MSI, etc
> <0 22 0>: PCIe RC0 special interrupt,
>              PHY Link related interrupts, etc
>
> Of course, legacy INTx is handled as message only.
>
> Mohit, Kishon,
> How about the other SoCs? INTx is mapped to single interrupt
> such as Exynos, or separate interrupts such as i.MX6?

Jingoo,

Ok - so at least the I.MX6 and Exynos, which both use the designware
IP need different IRQ mappings.  It seems to me then that the map_irq
should be moved out of drivers/pci/host/pcie-designware.c and into the
SoC specific host controller drivers (pci-imx.c and pci-exynos.c).  If
that becomes the consensus I can submit a patch that does that.

Thanks,

Tim

>
> Best regards,
> Jingoo Han
>
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Richard Zhu <r65037@freescale.com>
>> Cc: Shawn Guo <shawn.guo@linaro.org>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Sean Cross <xobs@kosagi.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> ---
>>  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
>>  drivers/pci/host/pci-exynos.c      |  7 +++++--
>>  drivers/pci/host/pci-imx6.c        | 11 +++++++----
>>  drivers/pci/host/pcie-designware.c |  8 +++++++-
>>  drivers/pci/host/pcie-designware.h |  2 +-
>>  5 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>> index eed6897..fad8d90 100644
>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>> @@ -126,7 +126,7 @@
>>                                 0x81000000 0 0          0x01f80000 0 0x00010000 /* downstream I/O */
>>                                 0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable
>> memory */
>>                       num-lanes = <1>;
>> -                     interrupts = <0 123 0x04>;
>> +                     interrupts = <0 123 0x04>, <0 122 0x04>, <0 121 0x04>, <0 120 0x04>;
>>                       clocks = <&clks 189>, <&clks 187>, <&clks 206>, <&clks 144>;
>>                       clock-names = "pcie_ref_125m", "sata_ref_100m", "lvds_gate", "pcie_axi";
>>                       status = "disabled";
>> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
>> index 3de6bfb..d85dcb0 100644
>> --- a/drivers/pci/host/pci-exynos.c
>> +++ b/drivers/pci/host/pci-exynos.c
>> @@ -515,11 +515,14 @@ static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
>>  {
>>       int ret;
>>
>> -     pp->irq = platform_get_irq(pdev, 1);
>> -     if (!pp->irq) {
>> +     pp->irq[0] = platform_get_irq(pdev, 1);
>> +     if (!pp->irq[0]) {
>>               dev_err(&pdev->dev, "failed to get irq\n");
>>               return -ENODEV;
>>       }
>> +     pp->irq[1] = pp->irq[0];
>> +     pp->irq[2] = pp->irq[0];
>> +     pp->irq[3] = pp->irq[0];
>>       ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
>>                               IRQF_SHARED, "exynos-pcie", pp);
>>       if (ret) {
>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
>> index e8663a8..aaa05c8 100644
>> --- a/drivers/pci/host/pci-imx6.c
>> +++ b/drivers/pci/host/pci-imx6.c
>> @@ -470,11 +470,14 @@ static int imx6_add_pcie_port(struct pcie_port *pp,
>>                       struct platform_device *pdev)
>>  {
>>       int ret;
>> +     int i;
>>
>> -     pp->irq = platform_get_irq(pdev, 0);
>> -     if (!pp->irq) {
>> -             dev_err(&pdev->dev, "failed to get irq\n");
>> -             return -ENODEV;
>> +     for (i = 0; i < 4; i++) {
>> +             pp->irq[i] = platform_get_irq(pdev, i);
>> +             if (!pp->irq[i]) {
>> +                     dev_err(&pdev->dev, "failed to get irq%d\n", i);
>> +                     return -ENODEV;
>> +             }
>>       }
>>
>>       pp->root_bus_nr = -1;
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index 17ce88f..5808177 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -738,8 +738,14 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>>  static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>>  {
>>       struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
>> +     int irq = -1;
>>
>> -     return pp->irq;
>> +     if (pin > 0 && pin < 5)
>> +             irq = pp->irq[pin - 1];
>> +     dev_info(&dev->dev, "map_irq: %04x:%04x slot%d pin%d irq%d\n",
>> +             dev->vendor, dev->device, slot, pin, irq);
>> +
>> +     return irq;
>>  }
>>
>>  static void dw_pcie_add_bus(struct pci_bus *bus)
>> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
>> index 3063b35..5c596c0 100644
>> --- a/drivers/pci/host/pcie-designware.h
>> +++ b/drivers/pci/host/pcie-designware.h
>> @@ -46,7 +46,7 @@ struct pcie_port {
>>       struct resource         io;
>>       struct resource         mem;
>>       struct pcie_port_info   config;
>> -     int                     irq;
>> +     int                     irq[4];
>>       u32                     lanes;
>>       struct pcie_host_ops    *ops;
>>       int                     msi_irq;
>> --
>> 1.8.3.2
>

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Jingoo Han]

On Friday, February 28, 2014 1:16 PM, Tim Harvey wrote:
> On Thu, Feb 27, 2014 at 5:50 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> >
> > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> > >
> > > An add-in card used on the Ventana IMX6 SoC based family of boards has a
> > > TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings between the
> > > bridge and the four mini-PCI slots are swapped (INTD/C/B/A).
> >
> > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
> >
> > This problem happens from the 'Board', not a 'SoC'.
> > 'TI XIO2001 PCIe-to-PCI bridge' is not a 'SoC'.
> > 'pci-imx6.c' is the driver for 'IMX6 PCI IP', not for 'IMX6 SoC based
> > board'. Isn't it?
> 
> Jingoo,
> 
> Correct, this is an issue in the way the XIO2001 was hooked up to the
> PCI slots, so it should be viewed as a board issue.

(+CC Arnd Bergmann)

Then, this board fixup code should NOT be placed in './drivers/pci/host/'
side. 

> 
> >
> > Then, the workaround code for board problem should NOT be
> > included to './drivers/pci/host/' side.
> 
> I would agree, but to overcome this sort of interrupt mapping issue
> one would need to either implement a custom swizzle or perhaps a
> custom map_irq and both of those are hooked into the pcie driver core.
>  Do you have any suggestions on where/how I would better hook into
> those?

Anyway, 'TI XIO2001 PCIe-to-PCI bridge' chip on the board is the
culprit. So, the board specific side is a good place.
For instance, ./arch/arm/mach-imx/

I don't know how to handle this problem.
But, there is no reason that 'pcie-designware.c' should take a care
of the board specific issue.

Arnd Bergmann,
Would you give your opinions?

Best regards,
Jingoo Han

> 
> >
> > Also, please add the following members to CC list. They will give
> > important comments.
> >
> > : Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG
> >
> > Best regards,
> > Jingoo Han
> 
> Thanks,
> 
> Tim
> 
> >
> > >
> > > This series adds legacy PCI interrupt mapping to support PCI bridges on
> > > IMX6 PCIe host controllers, adds the ability to use a host-specific
> > > custom swizzle function, then uses that to fix the IRQ mapping.
> > >
> > > I'm hoping this is the right way to perform such a workaround - please provide
> > > feedback if not.  Patch1 will likely collide with the owrk Lucas is doing
> > > with device-tree bindings and MSI support.  I'm happy to re-base if/when
> > > needed.
> > >
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Richard Zhu <r65037@freescale.com>
> > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: Sean Cross <xobs@kosagi.com>
> > > Cc: Jingoo Han <jg1.han@samsung.com>
> > > ---
> > > Tim Harvey (3):
> > >   PCI: designware: add legacy PCI interrupt mapping
> > >   PCI: designware: add ability for custom swizzle
> > >   PCI: imx6: ventana: fixup for IRQ mismapping
> > >
> > >  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
> > >  drivers/pci/host/pci-exynos.c      |  7 ++++--
> > >  drivers/pci/host/pci-imx6.c        | 47 ++++++++++++++++++++++++++++++++++----
> > >  drivers/pci/host/pcie-designware.c | 11 ++++++++-
> > >  drivers/pci/host/pcie-designware.h |  3 ++-
> > >  include/linux/pci_ids.h            |  1 +
> > >  6 files changed, 62 insertions(+), 9 deletions(-)
> > >
> > > --
> > > 1.8.3.2
> >

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Jingoo Han]

On Friday, February 28, 2014 1:25 PM, Tim Harvey wrote:
> On Thu, Feb 27, 2014 at 6:00 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> >>
> >> The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
> >> This allows a PCIe-to-PCI bridge to function properly.
> >>
> >> The irq field of the pcie_host struct is expanded to 4 interrupts to
> >> allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
> >> all from devicetree.  I'm not clear if the Exynos driver has this
> >> capability so it places the same interrupt in all 4 slots.
> >
> > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
> >
> > In the case of Exynos,
> > 'INTA/B/C/D' are mapped to only one interrupt (<0 20 0>).
> > Thus, the current code works properly on Exynos platform.
> >
> > There are three interrupts for Exynos PCIe; INTx, MSI, PHY Link,
> > respectively as below.
> >
> > ./arch/arm/boot/dts/exynos5440.dtsi
> >         interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> >
> > <0 20 0>: PCIe RC0 pulse interrupt,
> >              INTA, INTB, INTC and INTD, etc
> > <0 21 0>: PCIe RC0 level interrupt,
> >              MSI, etc
> > <0 22 0>: PCIe RC0 special interrupt,
> >              PHY Link related interrupts, etc
> >
> > Of course, legacy INTx is handled as message only.
> >
> > Mohit, Kishon,
> > How about the other SoCs? INTx is mapped to single interrupt
> > such as Exynos, or separate interrupts such as i.MX6?
> 
> Jingoo,
> 
> Ok - so at least the I.MX6 and Exynos, which both use the designware
> IP need different IRQ mappings.  It seems to me then that the map_irq
> should be moved out of drivers/pci/host/pcie-designware.c and into the
> SoC specific host controller drivers (pci-imx.c and pci-exynos.c).  If
> that becomes the consensus I can submit a patch that does that.

(+CC Arnd Bergmann)

If you want to split dw_pcie_map_irq(), the following would be better.

./drivers/pci/host/pcie-designware.c
static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
{
	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);

	if (pp->ops->writel_rc)
		return pp->ops->map_irq(pp, pin);
	else
		return pp->irq;
}

./drivers/pci/host/pci-imx6.c
static int imx6_pcie_map_irq(struct pcie_port *pp, u8 pin)
{
	.....
}

Also, please add additional 'irq[4]' variable to 'struct imx6_pcie',
instead of 'struct pcie_port'. Other SoCs does not use four separate
INTx, as far as I know.

Best regards,
Jingoo Han

> 
> Thanks,
> 
> Tim
> 
> >
> > Best regards,
> > Jingoo Han
> >
> >>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Richard Zhu <r65037@freescale.com>
> >> Cc: Shawn Guo <shawn.guo@linaro.org>
> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> Cc: Sean Cross <xobs@kosagi.com>
> >> Cc: Jingoo Han <jg1.han@samsung.com>
> >> ---
> >>  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
> >>  drivers/pci/host/pci-exynos.c      |  7 +++++--
> >>  drivers/pci/host/pci-imx6.c        | 11 +++++++----
> >>  drivers/pci/host/pcie-designware.c |  8 +++++++-
> >>  drivers/pci/host/pcie-designware.h |  2 +-
> >>  5 files changed, 21 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> >> index eed6897..fad8d90 100644
> >> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> >> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> >> @@ -126,7 +126,7 @@
> >>                                 0x81000000 0 0          0x01f80000 0 0x00010000 /* downstream I/O */
> >>                                 0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable
> >> memory */
> >>                       num-lanes = <1>;
> >> -                     interrupts = <0 123 0x04>;
> >> +                     interrupts = <0 123 0x04>, <0 122 0x04>, <0 121 0x04>, <0 120 0x04>;
> >>                       clocks = <&clks 189>, <&clks 187>, <&clks 206>, <&clks 144>;
> >>                       clock-names = "pcie_ref_125m", "sata_ref_100m", "lvds_gate", "pcie_axi";
> >>                       status = "disabled";
> >> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> >> index 3de6bfb..d85dcb0 100644
> >> --- a/drivers/pci/host/pci-exynos.c
> >> +++ b/drivers/pci/host/pci-exynos.c
> >> @@ -515,11 +515,14 @@ static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
> >>  {
> >>       int ret;
> >>
> >> -     pp->irq = platform_get_irq(pdev, 1);
> >> -     if (!pp->irq) {
> >> +     pp->irq[0] = platform_get_irq(pdev, 1);
> >> +     if (!pp->irq[0]) {
> >>               dev_err(&pdev->dev, "failed to get irq\n");
> >>               return -ENODEV;
> >>       }
> >> +     pp->irq[1] = pp->irq[0];
> >> +     pp->irq[2] = pp->irq[0];
> >> +     pp->irq[3] = pp->irq[0];
> >>       ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
> >>                               IRQF_SHARED, "exynos-pcie", pp);
> >>       if (ret) {
> >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> >> index e8663a8..aaa05c8 100644
> >> --- a/drivers/pci/host/pci-imx6.c
> >> +++ b/drivers/pci/host/pci-imx6.c
> >> @@ -470,11 +470,14 @@ static int imx6_add_pcie_port(struct pcie_port *pp,
> >>                       struct platform_device *pdev)
> >>  {
> >>       int ret;
> >> +     int i;
> >>
> >> -     pp->irq = platform_get_irq(pdev, 0);
> >> -     if (!pp->irq) {
> >> -             dev_err(&pdev->dev, "failed to get irq\n");
> >> -             return -ENODEV;
> >> +     for (i = 0; i < 4; i++) {
> >> +             pp->irq[i] = platform_get_irq(pdev, i);
> >> +             if (!pp->irq[i]) {
> >> +                     dev_err(&pdev->dev, "failed to get irq%d\n", i);
> >> +                     return -ENODEV;
> >> +             }
> >>       }
> >>
> >>       pp->root_bus_nr = -1;
> >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> >> index 17ce88f..5808177 100644
> >> --- a/drivers/pci/host/pcie-designware.c
> >> +++ b/drivers/pci/host/pcie-designware.c
> >> @@ -738,8 +738,14 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> >>  static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> >>  {
> >>       struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
> >> +     int irq = -1;
> >>
> >> -     return pp->irq;
> >> +     if (pin > 0 && pin < 5)
> >> +             irq = pp->irq[pin - 1];
> >> +     dev_info(&dev->dev, "map_irq: %04x:%04x slot%d pin%d irq%d\n",
> >> +             dev->vendor, dev->device, slot, pin, irq);
> >> +
> >> +     return irq;
> >>  }
> >>
> >>  static void dw_pcie_add_bus(struct pci_bus *bus)
> >> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> >> index 3063b35..5c596c0 100644
> >> --- a/drivers/pci/host/pcie-designware.h
> >> +++ b/drivers/pci/host/pcie-designware.h
> >> @@ -46,7 +46,7 @@ struct pcie_port {
> >>       struct resource         io;
> >>       struct resource         mem;
> >>       struct pcie_port_info   config;
> >> -     int                     irq;
> >> +     int                     irq[4];
> >>       u32                     lanes;
> >>       struct pcie_host_ops    *ops;
> >>       int                     msi_irq;
> >> --
> >> 1.8.3.2
> >

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Arnd Bergmann]

On Thursday 27 February 2014 17:19:54 Tim Harvey wrote:
> The TI XIO2001 PCIe-to-PCI bridge used on Ventana expansion boards
> has its slot-to-bridge IRQ mapping reversed from the PCI specification:
> 
>   INTA->INTD
>   INTB->INTC
>   INTC->INTB
>   INTD->INTA
> 
> Implement a custom swizzle function that does a fixup on the interrupt for
> devices on the first TI XIO2001 bridge in the tree.
> 

I'm pretty sure you can express that by using a more specific
'interrupt-map' property that defines the correct mapping for
the PCIe-to-PCI bridge in the board file.

	Arnd

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Marek Vasut]

On Friday, February 28, 2014 at 08:01:46 AM, Jingoo Han wrote:
> On Friday, February 28, 2014 1:25 PM, Tim Harvey wrote:
> > On Thu, Feb 27, 2014 at 6:00 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> > >> The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
> > >> This allows a PCIe-to-PCI bridge to function properly.
> > >> 
> > >> The irq field of the pcie_host struct is expanded to 4 interrupts to
> > >> allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
> > >> all from devicetree.  I'm not clear if the Exynos driver has this
> > >> capability so it places the same interrupt in all 4 slots.
> > > 
> > > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR
> > > DCG)
> > > 
> > > In the case of Exynos,
> > > 'INTA/B/C/D' are mapped to only one interrupt (<0 20 0>).
> > > Thus, the current code works properly on Exynos platform.
> > > 
> > > There are three interrupts for Exynos PCIe; INTx, MSI, PHY Link,
> > > respectively as below.
> > > 
> > > ./arch/arm/boot/dts/exynos5440.dtsi
> > > 
> > >         interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> > > 
> > > <0 20 0>: PCIe RC0 pulse interrupt,
> > > 
> > >              INTA, INTB, INTC and INTD, etc
> > > 
> > > <0 21 0>: PCIe RC0 level interrupt,
> > > 
> > >              MSI, etc
> > > 
> > > <0 22 0>: PCIe RC0 special interrupt,
> > > 
> > >              PHY Link related interrupts, etc
> > > 
> > > Of course, legacy INTx is handled as message only.
> > > 
> > > Mohit, Kishon,
> > > How about the other SoCs? INTx is mapped to single interrupt
> > > such as Exynos, or separate interrupts such as i.MX6?
> > 
> > Jingoo,
> > 
> > Ok - so at least the I.MX6 and Exynos, which both use the designware
> > IP need different IRQ mappings.  It seems to me then that the map_irq
> > should be moved out of drivers/pci/host/pcie-designware.c and into the
> > SoC specific host controller drivers (pci-imx.c and pci-exynos.c).  If
> > that becomes the consensus I can submit a patch that does that.
> 
> (+CC Arnd Bergmann)
> 
> If you want to split dw_pcie_map_irq(), the following would be better.
> 
> ./drivers/pci/host/pcie-designware.c
> static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> {
> 	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
> 
> 	if (pp->ops->writel_rc)
> 		return pp->ops->map_irq(pp, pin);
> 	else
> 		return pp->irq;
> }
> 
> ./drivers/pci/host/pci-imx6.c
> static int imx6_pcie_map_irq(struct pcie_port *pp, u8 pin)
> {
> 	.....
> }
> 
> Also, please add additional 'irq[4]' variable to 'struct imx6_pcie',
> instead of 'struct pcie_port'. Other SoCs does not use four separate
> INTx, as far as I know.

I agree with this.

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Marek Vasut]

On Friday, February 28, 2014 at 07:22:52 AM, Jingoo Han wrote:
> On Friday, February 28, 2014 1:16 PM, Tim Harvey wrote:
> > On Thu, Feb 27, 2014 at 5:50 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> > > > An add-in card used on the Ventana IMX6 SoC based family of boards
> > > > has a TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings
> > > > between the bridge and the four mini-PCI slots are swapped
> > > > (INTD/C/B/A).
> > > 
> > > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR
> > > DCG)
> > > 
> > > This problem happens from the 'Board', not a 'SoC'.
> > > 'TI XIO2001 PCIe-to-PCI bridge' is not a 'SoC'.
> > > 'pci-imx6.c' is the driver for 'IMX6 PCI IP', not for 'IMX6 SoC based
> > > board'. Isn't it?
> > 
> > Jingoo,
> > 
> > Correct, this is an issue in the way the XIO2001 was hooked up to the
> > PCI slots, so it should be viewed as a board issue.
> 
> (+CC Arnd Bergmann)
> 
> Then, this board fixup code should NOT be placed in './drivers/pci/host/'
> side.
> 
> > > Then, the workaround code for board problem should NOT be
> > > included to './drivers/pci/host/' side.
> > 
> > I would agree, but to overcome this sort of interrupt mapping issue
> > one would need to either implement a custom swizzle or perhaps a
> > custom map_irq and both of those are hooked into the pcie driver core.
> > 
> >  Do you have any suggestions on where/how I would better hook into
> > 
> > those?
> 
> Anyway, 'TI XIO2001 PCIe-to-PCI bridge' chip on the board is the
> culprit. So, the board specific side is a good place.
> For instance, ./arch/arm/mach-imx/
> 
> I don't know how to handle this problem.
> But, there is no reason that 'pcie-designware.c' should take a care
> of the board specific issue.

Can you not just supply the INTA...INTD IRQ numbers in reverse order via DT for 
this board ?

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Lucas Stach]

Am Freitag, den 28.02.2014, 16:01 +0900 schrieb Jingoo Han:
> On Friday, February 28, 2014 1:25 PM, Tim Harvey wrote:
> > On Thu, Feb 27, 2014 at 6:00 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> > >>
> > >> The IMX6 maps INTA/B/C/D to ARM GIC IRQ 155/154/153/152 respectively.
> > >> This allows a PCIe-to-PCI bridge to function properly.
> > >>
> > >> The irq field of the pcie_host struct is expanded to 4 interrupts to
> > >> allow for INTA/B/C/D and the IMX6 PCIe host driver will populate them
> > >> all from devicetree.  I'm not clear if the Exynos driver has this
> > >> capability so it places the same interrupt in all 4 slots.
> > >
> > > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
> > >
> > > In the case of Exynos,
> > > 'INTA/B/C/D' are mapped to only one interrupt (<0 20 0>).
> > > Thus, the current code works properly on Exynos platform.
> > >
> > > There are three interrupts for Exynos PCIe; INTx, MSI, PHY Link,
> > > respectively as below.
> > >
> > > ./arch/arm/boot/dts/exynos5440.dtsi
> > >         interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> > >
> > > <0 20 0>: PCIe RC0 pulse interrupt,
> > >              INTA, INTB, INTC and INTD, etc
> > > <0 21 0>: PCIe RC0 level interrupt,
> > >              MSI, etc
> > > <0 22 0>: PCIe RC0 special interrupt,
> > >              PHY Link related interrupts, etc
> > >
> > > Of course, legacy INTx is handled as message only.
> > >
> > > Mohit, Kishon,
> > > How about the other SoCs? INTx is mapped to single interrupt
> > > such as Exynos, or separate interrupts such as i.MX6?
> > 
> > Jingoo,
> > 
> > Ok - so at least the I.MX6 and Exynos, which both use the designware
> > IP need different IRQ mappings.  It seems to me then that the map_irq
> > should be moved out of drivers/pci/host/pcie-designware.c and into the
> > SoC specific host controller drivers (pci-imx.c and pci-exynos.c).  If
> > that becomes the consensus I can submit a patch that does that.
> 
> (+CC Arnd Bergmann)
> 
> If you want to split dw_pcie_map_irq(), the following would be better.
> 
> ./drivers/pci/host/pcie-designware.c
> static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> {
> 	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
> 
> 	if (pp->ops->writel_rc)
> 		return pp->ops->map_irq(pp, pin);
> 	else
> 		return pp->irq;
> }
> 
> ./drivers/pci/host/pci-imx6.c
> static int imx6_pcie_map_irq(struct pcie_port *pp, u8 pin)
> {
> 	.....
> }
> 
> Also, please add additional 'irq[4]' variable to 'struct imx6_pcie',
> instead of 'struct pcie_port'. Other SoCs does not use four separate
> INTx, as far as I know.
> 
It seems that dw_pcie_map_irq shouldn't even exist and should be
replaced with of_irq_parse_and_map_pci().

As I understand the Exynos hardware is mapping all legacy PCI IRQs to
one single GIC IRQ. Now I wonder why you have two different IRQs in your
DT? See this snippet:

interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 0>;
interrupt-map = <0x0 0 &gic 53>;

The driver uses the second device IRQ to map all PCI IRQs to, which is
GIC IRQ 21 in this example. Your interrupt-map property in contrary
indicates that all PCI IRQs should be mapped to GIC IRQ 53.
What is the right thing to do here?

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Arnd Bergmann]

On Friday 28 February 2014 12:27:36 Lucas Stach wrote:
>
> It seems that dw_pcie_map_irq shouldn't even exist and should be
> replaced with of_irq_parse_and_map_pci().

Right.

> As I understand the Exynos hardware is mapping all legacy PCI IRQs to
> one single GIC IRQ. Now I wonder why you have two different IRQs in your
> DT? See this snippet:
> 
> interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> #interrupt-cells = <1>;
> interrupt-map-mask = <0 0 0 0>;
> interrupt-map = <0x0 0 &gic 53>;
> 
> The driver uses the second device IRQ to map all PCI IRQs to, which is
> GIC IRQ 21 in this example. Your interrupt-map property in contrary
> indicates that all PCI IRQs should be mapped to GIC IRQ 53.
> What is the right thing to do here?

The interrupt-map doesn't even look well-formed, since it refers to
a gic that has #interrupt-cells=<3>, but only has one cell with the
value. This clearly cannot work at all. It should probably be

	interrupt-map = <0x0 0 &gic GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;

Note that '53' is the IRQ number that is commonly assigned to
SPI interrupt 21, since SPI starts at number 32 on GIC.


	Arnd

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Lucas Stach]

Am Freitag, den 28.02.2014, 12:37 +0100 schrieb Arnd Bergmann:
> On Friday 28 February 2014 12:27:36 Lucas Stach wrote:
> >
> > It seems that dw_pcie_map_irq shouldn't even exist and should be
> > replaced with of_irq_parse_and_map_pci().
> 
> Right.
> 
> > As I understand the Exynos hardware is mapping all legacy PCI IRQs to
> > one single GIC IRQ. Now I wonder why you have two different IRQs in your
> > DT? See this snippet:
> > 
> > interrupts = <0 20 0>, <0 21 0>, <0 22 0>;
> > #interrupt-cells = <1>;
> > interrupt-map-mask = <0 0 0 0>;
> > interrupt-map = <0x0 0 &gic 53>;
> > 
> > The driver uses the second device IRQ to map all PCI IRQs to, which is
> > GIC IRQ 21 in this example. Your interrupt-map property in contrary
> > indicates that all PCI IRQs should be mapped to GIC IRQ 53.
> > What is the right thing to do here?
> 
> The interrupt-map doesn't even look well-formed, since it refers to
> a gic that has #interrupt-cells=<3>, but only has one cell with the
> value. This clearly cannot work at all. It should probably be
> 
> 	interrupt-map = <0x0 0 &gic GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> 
> Note that '53' is the IRQ number that is commonly assigned to
> SPI interrupt 21, since SPI starts at number 32 on GIC.
> 
Ah, I see. Thanks for the explanation. So we have a bogus Exynos DT,
which will break existing systems once the kernel starts to do the right
thing. This is starting to look great. ;)

Regards,
Lucas

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Arnd Bergmann]

On Friday 28 February 2014 13:09:34 Lucas Stach wrote:
> Am Freitag, den 28.02.2014, 12:37 +0100 schrieb Arnd Bergmann:
> > On Friday 28 February 2014 12:27:36 Lucas Stach wrote:
> > The interrupt-map doesn't even look well-formed, since it refers to
> > a gic that has #interrupt-cells=<3>, but only has one cell with the
> > value. This clearly cannot work at all. It should probably be
> > 
> >       interrupt-map = <0x0 0 &gic GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> > 
> > Note that '53' is the IRQ number that is commonly assigned to
> > SPI interrupt 21, since SPI starts at number 32 on GIC.
> > 
> Ah, I see. Thanks for the explanation. So we have a bogus Exynos DT,
> which will break existing systems once the kernel starts to do the right
> thing. This is starting to look great. 
 
It seems tegra and r-car have the same bug. We can probably fix it
in a backwards-compatible way by changing the function to call
the generic helper first and if that fails call printk_once()
to emit a warning and return the hardwired IRQ number.

	Arnd

[PATCH 1/3] PCI: designware: add legacy PCI interrupt mapping

[Lucas Stach]

Am Freitag, den 28.02.2014, 13:27 +0100 schrieb Arnd Bergmann:
> On Friday 28 February 2014 13:09:34 Lucas Stach wrote:
> > Am Freitag, den 28.02.2014, 12:37 +0100 schrieb Arnd Bergmann:
> > > On Friday 28 February 2014 12:27:36 Lucas Stach wrote:
> > > The interrupt-map doesn't even look well-formed, since it refers to
> > > a gic that has #interrupt-cells=<3>, but only has one cell with the
> > > value. This clearly cannot work at all. It should probably be
> > > 
> > >       interrupt-map = <0x0 0 &gic GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> > > 
> > > Note that '53' is the IRQ number that is commonly assigned to
> > > SPI interrupt 21, since SPI starts at number 32 on GIC.
> > > 
> > Ah, I see. Thanks for the explanation. So we have a bogus Exynos DT,
> > which will break existing systems once the kernel starts to do the right
> > thing. This is starting to look great. 
>  
> It seems tegra and r-car have the same bug. We can probably fix it
> in a backwards-compatible way by changing the function to call
> the generic helper first and if that fails call printk_once()
> to emit a warning and return the hardwired IRQ number.
> 

Yes, seems like the way to go. I'll prepare some patches for that. It
seems of_irq_parse_and_map_pci() already emits an error message if it
can't find a valid mapping. This should be enough to get people to fix
their DTs.

Regards,
Lucas 

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Tim Harvey]

On Thu, Feb 27, 2014 at 10:22 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> On Friday, February 28, 2014 1:16 PM, Tim Harvey wrote:
>> On Thu, Feb 27, 2014 at 5:50 PM, Jingoo Han <jg1.han@samsung.com> wrote:
>> >
>> > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
>> > >
>> > > An add-in card used on the Ventana IMX6 SoC based family of boards has a
>> > > TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings between the
>> > > bridge and the four mini-PCI slots are swapped (INTD/C/B/A).
>> >
>> > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
>> >
>> > This problem happens from the 'Board', not a 'SoC'.
>> > 'TI XIO2001 PCIe-to-PCI bridge' is not a 'SoC'.
>> > 'pci-imx6.c' is the driver for 'IMX6 PCI IP', not for 'IMX6 SoC based
>> > board'. Isn't it?
>>
>> Jingoo,
>>
>> Correct, this is an issue in the way the XIO2001 was hooked up to the
>> PCI slots, so it should be viewed as a board issue.
>
> (+CC Arnd Bergmann)
>
> Then, this board fixup code should NOT be placed in './drivers/pci/host/'
> side.
>
>>
>> >
>> > Then, the workaround code for board problem should NOT be
>> > included to './drivers/pci/host/' side.
>>
>> I would agree, but to overcome this sort of interrupt mapping issue
>> one would need to either implement a custom swizzle or perhaps a
>> custom map_irq and both of those are hooked into the pcie driver core.
>>  Do you have any suggestions on where/how I would better hook into
>> those?
>
> Anyway, 'TI XIO2001 PCIe-to-PCI bridge' chip on the board is the
> culprit. So, the board specific side is a good place.
> For instance, ./arch/arm/mach-imx/
>
> I don't know how to handle this problem.
> But, there is no reason that 'pcie-designware.c' should take a care
> of the board specific issue.
>
> Arnd Bergmann,
> Would you give your opinions?

I agree with what your saying, but I don't see a way to put a custom
map_irq or swizzle in arch/arm/mach-imx when the imx6 pcie driver is
in drivers/pci/host.   Many machines sitll have their pcie host
controller drivers in arch/arm/mach-* but the imx6 was placed in
drivers/host/pci because it shares the designware core with other
boards (exynos if not more).

If you look at the PATCH 3/3, I did take care to work around this in
pci-imx6.c 'not' pcie-designware.c (I had to add support for assigning
a custom swizzle - see PATH 2/3).

I'm looking for suggestions as to if/how to get it into someplace more
'board-specific' (arch/arm/mach-imx) or 'device-specific' (ie pci
fixup for xio2001).  As far as I can tell this has to be resolved with
either a custom swizzle or map_irq which are function pointers in
struct hw_pci which is passed in to pci_common_init from pci host
controller drivers.

Perhaps there is a way to re-assign the swizzle function after
pcie-designware.c has called pci_common_init?  I see that
pcibios_init_hw assignes the map_irq/swizzle passed in from the struct
hw_pci to a struct pci_sys_data yet I'm not sure if I can get to this
structure within a pci fixup.

Thanks,

Tim

>
> Best regards,
> Jingoo Han
>
>>
>> >
>> > Also, please add the following members to CC list. They will give
>> > important comments.
>> >
>> > : Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG
>> >
>> > Best regards,
>> > Jingoo Han
>>
>> Thanks,
>>
>> Tim
>>
>> >
>> > >
>> > > This series adds legacy PCI interrupt mapping to support PCI bridges on
>> > > IMX6 PCIe host controllers, adds the ability to use a host-specific
>> > > custom swizzle function, then uses that to fix the IRQ mapping.
>> > >
>> > > I'm hoping this is the right way to perform such a workaround - please provide
>> > > feedback if not.  Patch1 will likely collide with the owrk Lucas is doing
>> > > with device-tree bindings and MSI support.  I'm happy to re-base if/when
>> > > needed.
>> > >
>> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > > Cc: Richard Zhu <r65037@freescale.com>
>> > > Cc: Shawn Guo <shawn.guo@linaro.org>
>> > > Cc: Lucas Stach <l.stach@pengutronix.de>
>> > > Cc: Sean Cross <xobs@kosagi.com>
>> > > Cc: Jingoo Han <jg1.han@samsung.com>
>> > > ---
>> > > Tim Harvey (3):
>> > >   PCI: designware: add legacy PCI interrupt mapping
>> > >   PCI: designware: add ability for custom swizzle
>> > >   PCI: imx6: ventana: fixup for IRQ mismapping
>> > >
>> > >  arch/arm/boot/dts/imx6qdl.dtsi     |  2 +-
>> > >  drivers/pci/host/pci-exynos.c      |  7 ++++--
>> > >  drivers/pci/host/pci-imx6.c        | 47 ++++++++++++++++++++++++++++++++++----
>> > >  drivers/pci/host/pcie-designware.c | 11 ++++++++-
>> > >  drivers/pci/host/pcie-designware.h |  3 ++-
>> > >  include/linux/pci_ids.h            |  1 +
>> > >  6 files changed, 62 insertions(+), 9 deletions(-)
>> > >
>> > > --
>> > > 1.8.3.2
>> >
>

[RFC PATCH 0/3] PCI: imx6: fixup for add-in card IRQ mismapping

[Lucas Stach]

Am Freitag, den 28.02.2014, 08:52 -0800 schrieb Tim Harvey:
> On Thu, Feb 27, 2014 at 10:22 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Friday, February 28, 2014 1:16 PM, Tim Harvey wrote:
> >> On Thu, Feb 27, 2014 at 5:50 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> >> >
> >> > On Friday, February 28, 2014 10:20 AM, Tim Harvey wrote:
> >> > >
> >> > > An add-in card used on the Ventana IMX6 SoC based family of boards has a
> >> > > TI XIO2001 PCIe-to-PCI bridge where the INTA/B/C/D mappings between the
> >> > > bridge and the four mini-PCI slots are swapped (INTD/C/B/A).
> >> >
> >> > (+cc Marek Vasut, Pratyush Anand, Kishon Vijay Abraham I, Mohit KUMAR DCG)
> >> >
> >> > This problem happens from the 'Board', not a 'SoC'.
> >> > 'TI XIO2001 PCIe-to-PCI bridge' is not a 'SoC'.
> >> > 'pci-imx6.c' is the driver for 'IMX6 PCI IP', not for 'IMX6 SoC based
> >> > board'. Isn't it?
> >>
> >> Jingoo,
> >>
> >> Correct, this is an issue in the way the XIO2001 was hooked up to the
> >> PCI slots, so it should be viewed as a board issue.
> >
> > (+CC Arnd Bergmann)
> >
> > Then, this board fixup code should NOT be placed in './drivers/pci/host/'
> > side.
> >
> >>
> >> >
> >> > Then, the workaround code for board problem should NOT be
> >> > included to './drivers/pci/host/' side.
> >>
> >> I would agree, but to overcome this sort of interrupt mapping issue
> >> one would need to either implement a custom swizzle or perhaps a
> >> custom map_irq and both of those are hooked into the pcie driver core.
> >>  Do you have any suggestions on where/how I would better hook into
> >> those?
> >
> > Anyway, 'TI XIO2001 PCIe-to-PCI bridge' chip on the board is the
> > culprit. So, the board specific side is a good place.
> > For instance, ./arch/arm/mach-imx/
> >
> > I don't know how to handle this problem.
> > But, there is no reason that 'pcie-designware.c' should take a care
> > of the board specific issue.
> >
> > Arnd Bergmann,
> > Would you give your opinions?
> 
> I agree with what your saying, but I don't see a way to put a custom
> map_irq or swizzle in arch/arm/mach-imx when the imx6 pcie driver is
> in drivers/pci/host.   Many machines sitll have their pcie host
> controller drivers in arch/arm/mach-* but the imx6 was placed in
> drivers/host/pci because it shares the designware core with other
> boards (exynos if not more).
> 
> If you look at the PATCH 3/3, I did take care to work around this in
> pci-imx6.c 'not' pcie-designware.c (I had to add support for assigning
> a custom swizzle - see PATH 2/3).
> 
> I'm looking for suggestions as to if/how to get it into someplace more
> 'board-specific' (arch/arm/mach-imx) or 'device-specific' (ie pci
> fixup for xio2001).  As far as I can tell this has to be resolved with
> either a custom swizzle or map_irq which are function pointers in
> struct hw_pci which is passed in to pci_common_init from pci host
> controller drivers.
> 
> Perhaps there is a way to re-assign the swizzle function after
> pcie-designware.c has called pci_common_init?  I see that
> pcibios_init_hw assignes the map_irq/swizzle passed in from the struct
> hw_pci to a struct pci_sys_data yet I'm not sure if I can get to this
> structure within a pci fixup.
> 

I'll send a series to fix the IRQ mapping in the host driver in a few
minutes. After this you should be able to trivially represent you
swizzled IRQ lines by adding your PCIe-to-PCI bridge in the devicetree
of your board and set the correct IRQ mapping table.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Jason Gunthorpe]

On Fri, Feb 28, 2014 at 10:27:13AM +0100, Arnd Bergmann wrote:
> On Thursday 27 February 2014 17:19:54 Tim Harvey wrote:
> > The TI XIO2001 PCIe-to-PCI bridge used on Ventana expansion boards
> > has its slot-to-bridge IRQ mapping reversed from the PCI specification:
> > 
> >   INTA->INTD
> >   INTB->INTC
> >   INTC->INTB
> >   INTD->INTA
> > 
> > Implement a custom swizzle function that does a fixup on the interrupt for
> > devices on the first TI XIO2001 bridge in the tree.
> > 
> 
> I'm pretty sure you can express that by using a more specific
> 'interrupt-map' property that defines the correct mapping for
> the PCIe-to-PCI bridge in the board file.

Yes, the correct way to handle this is to define a stanza for the
PCIe-PCI bridge and then place an interrupt map in that stanza that
describes the downstream translation.

Nobody should be messing with IRQ numbers in code on DT platforms.

DTs using the mvebu driver have an example of this:

                pcie-controller {
                        compatible = "marvell,kirkwood-pcie";
                        status = "disabled";
                        device_type = "pci";

                        pcie at 1,0 {
                                device_type = "pci";
                                reg = <0x0800 0 0 0 0>;
                                      ^^^^^^^ This is the PCI device
				      ID of the bridge
                                interrupt-map-mask = <0 0 0 0>;
                                interrupt-map = <0 0 0 0 &intc 9>;

Which says 'INTA/B/C/D of devices downstream of bridge 00:01.0 map to
&intc input 9'

In particular, this is probably not a TI XIO2001 problem, but a board
design problem - the swizzle rules were not properly followed when
wiring up the PCI ISDEL and INTx pins on the downstream PCI bus.

Jason

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Tim Harvey]

On Fri, Feb 28, 2014 at 9:39 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Fri, Feb 28, 2014 at 10:27:13AM +0100, Arnd Bergmann wrote:
>> On Thursday 27 February 2014 17:19:54 Tim Harvey wrote:
>> > The TI XIO2001 PCIe-to-PCI bridge used on Ventana expansion boards
>> > has its slot-to-bridge IRQ mapping reversed from the PCI specification:
>> >
>> >   INTA->INTD
>> >   INTB->INTC
>> >   INTC->INTB
>> >   INTD->INTA
>> >
>> > Implement a custom swizzle function that does a fixup on the interrupt for
>> > devices on the first TI XIO2001 bridge in the tree.
>> >
>>
>> I'm pretty sure you can express that by using a more specific
>> 'interrupt-map' property that defines the correct mapping for
>> the PCIe-to-PCI bridge in the board file.
>
> Yes, the correct way to handle this is to define a stanza for the
> PCIe-PCI bridge and then place an interrupt map in that stanza that
> describes the downstream translation.
>
> Nobody should be messing with IRQ numbers in code on DT platforms.
>
> DTs using the mvebu driver have an example of this:
>
>                 pcie-controller {
>                         compatible = "marvell,kirkwood-pcie";
>                         status = "disabled";
>                         device_type = "pci";
>
>                         pcie at 1,0 {
>                                 device_type = "pci";
>                                 reg = <0x0800 0 0 0 0>;
>                                       ^^^^^^^ This is the PCI device
>                                       ID of the bridge
>                                 interrupt-map-mask = <0 0 0 0>;
>                                 interrupt-map = <0 0 0 0 &intc 9>;
>
> Which says 'INTA/B/C/D of devices downstream of bridge 00:01.0 map to
> &intc input 9'
>

Jason,

In our case, the IMX6 is on a basebaord and we have optional expansion
boards.  One such board has a PEX 8609 9-port PCIe switch, the other
has a TI XIO2001 PCIe-to-PCI bridge.  So, depending on how you connect
these expansion boards the bus numbers change.

For example, one use case of the baseboard+XIO2001-expansion is: IMX6
-> XIO2001 PCIe-to-PCI bridge:
The following shows one endpoint off the bridge (bus2 slot12):
$ lspci -n
00:00.0 0604: 16c3:abcd (rev 01)
01:00.0 0604: 104c:8240
02:0c.0 0280: 168c:0027 (rev 01)
$ lspci -tnv
-[0000:00]---00.0-[01-02]----00.0-[02]----0c.0  168c:0027

Another use case of the same baseboard + PEX8609-expansion +
XIO2001-expansion is:
$ 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)
04:00.0 0604: 104c:8240
05:0c.0 0280: 168c:0027 (rev 01)
$ lspci -tnv
-[0000:00]---00.0-[01-0a]--+-00.0-[02-0a]--+-01.0-[03]--
                           |
+-04.0-[04-05]----00.0-[05]----0c.0  168c:0027
                           |               +-05.0-[06]--
                           |               +-06.0-[07]--
                           |               +-07.0-[08]--
                           |               +-08.0-[09]--
                           |               \-09.0-[0a]--
                           \-00.1  10b5:8609

> In particular, this is probably not a TI XIO2001 problem, but a board
> design problem - the swizzle rules were not properly followed when
> wiring up the PCI ISDEL and INTx pins on the downstream PCI bus.

Correct - its not a TI XIO2001 problem, its that the interrupts from
the slots to the XIO2001 don't follow the interrupt mapping called out
in the spec correctly (board problem on the 'add-in' board, not the
'baseboard').

Regardless of the issue of not knowing the bus topology before-hand,
I'm still trying to understand how to describe the bridge in
devicetree using your example above.  If I were able to define a DT
node for the XIO2001 bridge and apply an interrupt-map there, how does
that map get encorporated into the swizzle in the case the the bridge
is in the middle of the chain of devices?   In looking at the
of_irq_parse_map_pci() function that will likely be called from
map_irq() it would end up calling of_irq_parse_raw to map the irq and
I'm not understanding how that will take into account the fact that a
bridge possibly in the middle of the bus-tree had invalid mapping.

Thanks,

Tim


>
> Jason

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Jason Gunthorpe]

On Fri, Feb 28, 2014 at 04:52:05PM -0800, Tim Harvey wrote:

> > In particular, this is probably not a TI XIO2001 problem, but a board
> > design problem - the swizzle rules were not properly followed when
> > wiring up the PCI ISDEL and INTx pins on the downstream PCI bus.
> 
> Correct - its not a TI XIO2001 problem, its that the interrupts from
> the slots to the XIO2001 don't follow the interrupt mapping called out
> in the spec correctly (board problem on the 'add-in' board, not the
> 'baseboard').

So broken hardware requires explicit DT representation, the
auto-probing mechanism only work for compliant hardware.

> Regardless of the issue of not knowing the bus topology before-hand,

To solve this problem, you reall need to know the bus topology before
hand, so any systems that include a borken TI XIO2001 board need a
special DT.

Including stuff like this in code means you hobble every one who might
use the TI chip correctly. It is certainly inappropriate to include a
host driver, or generic PCI fixup.

> I'm still trying to understand how to describe the bridge in
> devicetree using your example above.  If I were able to define a DT
> node for the XIO2001 bridge and apply an interrupt-map there, how does
> that map get encorporated into the swizzle in the case the the bridge
> is in the middle of the chain of devices?   

You keep nesting the PCI-PCI bridges until you get to the bottom,
basically following along the lspci -t output, cast into DT notation:

Your first example:
                pcie-controller {
                        compatible = "marvell,kirkwood-pcie";
                        status = "disabled";
                        device_type = "pci";

                        pcie at 0,0 {
                                device_type = "pci";
				// Presumably this is the root port bridge?
                                // 00:00.0 0604: 16c3:abcd
                                reg = <PCI_ID(0,0,0) 0 0 0 0>;
				pcie at 0,0 {
					 // This is the broken TI board:
					 // 01:00.0 0604: 104c:8240
                                	 reg = <PCI_ID(1,0,0) 0 0 0 0>;

Second example:
                pcie-controller {
                        compatible = "marvell,kirkwood-pcie";
                        status = "disabled";
                        device_type = "pci";

                        pcie at 0,0 {
                                device_type = "pci";
				// Presumably this is the root port bridge?
                                // 00:00.0 0604: 16c3:abcd
                                reg = <PCI_ID(0,0,0) 0 0 0 0>;
				pcie at 0,0 {
					 // This is the PEX switch bridge to internal
					 // 01:00.0 0604: 10b5:8609 (rev ba)
                                	 reg = <PCI_ID(1,0,0) 0 0 0 0>;
					 pcie at 4,0 {
					 	  // This is the PEX port bridge to
						  // To the TI part
						  // 02:04.0 0604: 10b5:8609 (rev ba)
						  reg = <PCI_ID(2,4,0) 0 0 0 0>;
					 	  pcie at 0,0 {
						  	   // This is the broken TI board:
							   // 04:00.0 0604: 104c:8240
                                	 		   reg =
							   <PCI_ID(4,0,0)
							   0 0 0 0>;


Also, bear in mind that every single explicitly declared stanza requires
a correct interrupt-map. 

> of_irq_parse_map_pci() function that will likely be called from
> map_irq() it would end up calling of_irq_parse_raw to map the irq and
> I'm not understanding how that will take into account the fact that a
> bridge possibly in the middle of the bus-tree had invalid mapping.

First the PCI core matches the DT nodes to the discovered topology.

Then the interrupt mapper starts from a probed PCI node and traces up
the tree to the root. Each time it goes up it swizzles.

When it finds a node with a DT mapping it immediately switches to DT
to resolve the interrupt, which uses the first interrupt map found by
traversing up from the match'd DT node. Once it switches to DT mode
there is no swizzling, the DT must exactly describe the
interconnection.

Jason

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Tim Harvey]

On Fri, Feb 28, 2014 at 5:22 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Fri, Feb 28, 2014 at 04:52:05PM -0800, Tim Harvey wrote:
>
>> > In particular, this is probably not a TI XIO2001 problem, but a board
>> > design problem - the swizzle rules were not properly followed when
>> > wiring up the PCI ISDEL and INTx pins on the downstream PCI bus.
>>
>> Correct - its not a TI XIO2001 problem, its that the interrupts from
>> the slots to the XIO2001 don't follow the interrupt mapping called out
>> in the spec correctly (board problem on the 'add-in' board, not the
>> 'baseboard').
>
> So broken hardware requires explicit DT representation, the
> auto-probing mechanism only work for compliant hardware.

ok - makes sense

>
>> Regardless of the issue of not knowing the bus topology before-hand,
>
> To solve this problem, you reall need to know the bus topology before
> hand, so any systems that include a borken TI XIO2001 board need a
> special DT.

I suppose I could go about this from the bootloader as well.  The
bootloader could enumerate the bus and build the DT with the nodes
required to handle the broken TI XIO2001 add-in cards.

>
> Including stuff like this in code means you hobble every one who might
> use the TI chip correctly. It is certainly inappropriate to include a
> host driver, or generic PCI fixup.

Understood - trust me... I'm not happy about this situation.

A pci fixup for the XIO2001 could at least be placed in
arch/arm/mach-imx/mach-imx6q.c with a check for
of_machine_is_compatible("gw, ventana").  This is currently done in
order to handle GPIO on the PEX890x bridge which is used as PERST# for
downstream slots (see
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-imx/mach-imx6q.c#n87).

This of course, assumes that I can come up with a way for a pci fixup
to replace the swizzle function of the host controller before
enumeration.

>
>> I'm still trying to understand how to describe the bridge in
>> devicetree using your example above.  If I were able to define a DT
>> node for the XIO2001 bridge and apply an interrupt-map there, how does
>> that map get encorporated into the swizzle in the case the the bridge
>> is in the middle of the chain of devices?
>
> You keep nesting the PCI-PCI bridges until you get to the bottom,
> basically following along the lspci -t output, cast into DT notation:
>
> Your first example:
>                 pcie-controller {
>                         compatible = "marvell,kirkwood-pcie";
>                         status = "disabled";
>                         device_type = "pci";
>
>                         pcie at 0,0 {
>                                 device_type = "pci";
>                                 // Presumably this is the root port bridge?
>                                 // 00:00.0 0604: 16c3:abcd
>                                 reg = <PCI_ID(0,0,0) 0 0 0 0>;

I'm still not understanding what needs to go in the 'reg' property for
a DT node representing a PCI dev (what you are using PCI_ID() for).
I've looked at the PCI Bus Binding to Open Firmware doc and still
don't understand (its very terse, and lacks examples).  By looking
through the drivers/of code it seemed that this should be devfn but
after some digging through examples in
Documentation/devicetree/bindings/pci it seems that it really needs to
be devfn<<8.  I'm not clear why the <<8 is needed.

I also determined that for fsl.imx6q-pcie I needed to define
#address-cells = <3> and #size-cells = <2> as the enw pcie dev nodes
kept defaulting to something else - I didn't realize those were not
inherited from the parent DT node.

In all, your examples here were extremely helpful to me.  I now know
how to properly describe my GigE PCI dev node so that the driver can
have a hope of getting its MAC from DT.  Thank you!

>                                 pcie at 0,0 {
>                                          // This is the broken TI board:
>                                          // 01:00.0 0604: 104c:8240
>                                          reg = <PCI_ID(1,0,0) 0 0 0 0>;
>
<snip>
>
> Also, bear in mind that every single explicitly declared stanza requires
> a correct interrupt-map.

if not specified it wouldn't default to standard bridge mapping such
that I only need to provide interrupt-map for the node with the the TI
bridge?

>
>> of_irq_parse_map_pci() function that will likely be called from
>> map_irq() it would end up calling of_irq_parse_raw to map the irq and
>> I'm not understanding how that will take into account the fact that a
>> bridge possibly in the middle of the bus-tree had invalid mapping.
>
> First the PCI core matches the DT nodes to the discovered topology.
>
> Then the interrupt mapper starts from a probed PCI node and traces up
> the tree to the root. Each time it goes up it swizzles.
>
> When it finds a node with a DT mapping it immediately switches to DT
> to resolve the interrupt, which uses the first interrupt map found by
> traversing up from the match'd DT node. Once it switches to DT mode
> there is no swizzling, the DT must exactly describe the
> interconnection.

Ok - so this assumes that the host controller driver _is_ calling
something like of_irq_parse_and_map_pci() instead of returning based
on some static mapping (like pp->irq) right?

What confuses me is that swizzling is done to the pin before the call
to map_irq(dev, slot, pin).  It looks like that this common swizzling
is ignored when using of_irq_parse_and_map_pci() which instead would
perform what you describe above correct?

Thanks for all the help here!

Tim

>
> Jason

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Jason Gunthorpe]

On Mon, Mar 03, 2014 at 11:59:51AM -0800, Tim Harvey wrote:

> A pci fixup for the XIO2001 could at least be placed in
> arch/arm/mach-imx/mach-imx6q.c with a check for
> of_machine_is_compatible("gw, ventana").  This is currently done in
> order to handle GPIO on the PEX890x bridge which is used as PERST# for
> downstream slots (see
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-imx/mach-imx6q.c#n87).

That is an odd looking thing .. PERST# of a subordinate port should be
tied to the PCI_BRIDGE_CTL_BUS_RESET bit in the port's bridge
configuration space - is more configuration of the PLX chip required?

Also, be aware that PCI specifies a minimum time after
reset-deassertion before accessing the bus, and I don't see a sleep...

> I'm still not understanding what needs to go in the 'reg' property for
> a DT node representing a PCI dev (what you are using PCI_ID() for).

It is a 3 DW address in this format:

phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
phys.low cell: llllllll llllllll llllllll llllllll

And in this context you use 'ss' = 0 for configuration:

* npt000ss bbbbbbbb dddddfff rrrrrrrr
= 00000000 00000000 dddddfff 00000000

So devfn << 8 sounds right to me.

I *think* you can use 0 for the bus in this context.

> #address-cells = <3> and #size-cells = <2> as the enw pcie dev nodes
> kept defaulting to something else - I didn't realize those were not
> inherited from the parent DT node.

Right, sorry, I was making it terse..
 
> In all, your examples here were extremely helpful to me.  I now know
> how to properly describe my GigE PCI dev node so that the driver can
> have a hope of getting its MAC from DT.  Thank you!

Right!

> > Also, bear in mind that every single explicitly declared stanza requires
> > a correct interrupt-map.
> 
> if not specified it wouldn't default to standard bridge mapping such
> that I only need to provide interrupt-map for the node with the the TI
> bridge?

Nope, there are two distinct mechanisms - finding the closes DT node
to the actual PCI device (of_irq_parse_pci) - this swizzles as it
searches. The rational here is to support hot pluggable devices that
were not discovered during initialization time.

The second is the completely generic interrupt-map mechanism
(of_irq_parse_raw), which takes the interrupt specification from the
first phase and transates it into an actual interrupt. This isn't PCI
aware and doesn't change the description in any way.

So if you specify a node, you have to specify an interrupt-map that
covers that node. I recommend placing the map in the node itself so
the bus number is not required.

> Ok - so this assumes that the host controller driver _is_ calling
> something like of_irq_parse_and_map_pci() instead of returning based
> on some static mapping (like pp->irq) right?

Yes, absoultely. All DT based PCI drivers need to use this mechanism
to get the expected interrupt-map behavior.

> What confuses me is that swizzling is done to the pin before the call
> to map_irq(dev, slot, pin).  It looks like that this common swizzling
> is ignored when using of_irq_parse_and_map_pci() which instead would
> perform what you describe above correct?

Right, the pin # from the PCI core isn't used in the OF code:

 * of_irq_parse_and_map_pci() - Decode a PCI irq from the device tree and map to a virq
 * @slot and @pin are unused, but included in the function so that this
 * function can be used directly as the map_irq callback to pci_fixup_irqs().

It re-reads the device's pin number directly and feeds that through
the standard swizzle and OF's UIS system.

Jason

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Tim Harvey]

On Mon, Mar 3, 2014 at 3:37 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Mon, Mar 03, 2014 at 11:59:51AM -0800, Tim Harvey wrote:
>
>> A pci fixup for the XIO2001 could at least be placed in
>> arch/arm/mach-imx/mach-imx6q.c with a check for
>> of_machine_is_compatible("gw, ventana").  This is currently done in
>> order to handle GPIO on the PEX890x bridge which is used as PERST# for
>> downstream slots (see
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-imx/mach-imx6q.c#n87).
>
> That is an odd looking thing .. PERST# of a subordinate port should be
> tied to the PCI_BRIDGE_CTL_BUS_RESET bit in the port's bridge
> configuration space - is more configuration of the PLX chip required?

PCI_BRIDGE_CTL_BUS_RESET does cause a hot reset on the corresponding
downstream link however the minPCIe card slot has a PERST# pin that is
for a functional reset to the card.  The GPIO's I mention on the PLX
bridge are routed to that signal for the various slots.  Regardless of
what a device may do when it gets a hot-reset some devices still may
require a hard reset.  Think of MiniPCIe form-factor USB cellular
modems for example which don't even use the PCIe bus.

>
> Also, be aware that PCI specifies a minimum time after
> reset-deassertion before accessing the bus, and I don't see a sleep...

there is an msleep(100) there after the de-assertion.

>
>> I'm still not understanding what needs to go in the 'reg' property for
>> a DT node representing a PCI dev (what you are using PCI_ID() for).
>
> It is a 3 DW address in this format:
>
> phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
> phys.low cell: llllllll llllllll llllllll llllllll
>
> And in this context you use 'ss' = 0 for configuration:
>
> * npt000ss bbbbbbbb dddddfff rrrrrrrr
> = 00000000 00000000 dddddfff 00000000
>
> So devfn << 8 sounds right to me.
>
> I *think* you can use 0 for the bus in this context.

yes, it would appear bus can be 0 from my findings.  Thanks for
clarifying that verbiage!

>
>> #address-cells = <3> and #size-cells = <2> as the enw pcie dev nodes
>> kept defaulting to something else - I didn't realize those were not
>> inherited from the parent DT node.
>
> Right, sorry, I was making it terse..
>
>> In all, your examples here were extremely helpful to me.  I now know
>> how to properly describe my GigE PCI dev node so that the driver can
>> have a hope of getting its MAC from DT.  Thank you!
>
> Right!
>
>> > Also, bear in mind that every single explicitly declared stanza requires
>> > a correct interrupt-map.
>>
>> if not specified it wouldn't default to standard bridge mapping such
>> that I only need to provide interrupt-map for the node with the the TI
>> bridge?
>
> Nope, there are two distinct mechanisms - finding the closes DT node
> to the actual PCI device (of_irq_parse_pci) - this swizzles as it
> searches. The rational here is to support hot pluggable devices that
> were not discovered during initialization time.
>
> The second is the completely generic interrupt-map mechanism
> (of_irq_parse_raw), which takes the interrupt specification from the
> first phase and transates it into an actual interrupt. This isn't PCI
> aware and doesn't change the description in any way.
>
> So if you specify a node, you have to specify an interrupt-map that
> covers that node. I recommend placing the map in the node itself so
> the bus number is not required.
>
>> Ok - so this assumes that the host controller driver _is_ calling
>> something like of_irq_parse_and_map_pci() instead of returning based
>> on some static mapping (like pp->irq) right?
>
> Yes, absoultely. All DT based PCI drivers need to use this mechanism
> to get the expected interrupt-map behavior.
>
>> What confuses me is that swizzling is done to the pin before the call
>> to map_irq(dev, slot, pin).  It looks like that this common swizzling
>> is ignored when using of_irq_parse_and_map_pci() which instead would
>> perform what you describe above correct?
>
> Right, the pin # from the PCI core isn't used in the OF code:
>
>  * of_irq_parse_and_map_pci() - Decode a PCI irq from the device tree and map to a virq
>  * @slot and @pin are unused, but included in the function so that this
>  * function can be used directly as the map_irq callback to pci_fixup_irqs().
>
> It re-reads the device's pin number directly and feeds that through
> the standard swizzle and OF's UIS system.
>
> Jason

Ok - that all makes sense and hopefully when I get the legacy PCI
interrupt mapping via DT working for IMX6 I can prove it out.

What did you think of my two ideas above to resolve this issue on our
add-in card where the bus-topology isn't known ahead of time:

 1. bootloader builds out DT with proper interrupt-map's to handle the
mis-map after enumerating the bus to understand the toplogy.

or

 2. a pci fixup for xio2001 that further qualifies the DT machine and
the 'depth' of the xio2001 (ie make sure its the first xio2001 bridge
on the bus) that replaces the swizzle function.  This would be in
arch/arm/mach-imx/mach-imx6q.c

Thanks,

Tim

[PATCH 3/3] PCI: imx6: ventana: fixup for IRQ mismapping

[Jason Gunthorpe]

On Mon, Mar 03, 2014 at 04:38:10PM -0800, Tim Harvey wrote:
> On Mon, Mar 3, 2014 at 3:37 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > On Mon, Mar 03, 2014 at 11:59:51AM -0800, Tim Harvey wrote:
> >
> >> A pci fixup for the XIO2001 could at least be placed in
> >> arch/arm/mach-imx/mach-imx6q.c with a check for
> >> of_machine_is_compatible("gw, ventana").  This is currently done in
> >> order to handle GPIO on the PEX890x bridge which is used as PERST# for
> >> downstream slots (see
> >> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-imx/mach-imx6q.c#n87).
> >
> > That is an odd looking thing .. PERST# of a subordinate port should be
> > tied to the PCI_BRIDGE_CTL_BUS_RESET bit in the port's bridge
> > configuration space - is more configuration of the PLX chip required?
> 
> PCI_BRIDGE_CTL_BUS_RESET does cause a hot reset on the corresponding
> downstream link however the minPCIe card slot has a PERST# pin that is
> for a functional reset to the card.  

Ah, I'm getting my PCI versions mixed up, that all sounds right to me
then..

> > Also, be aware that PCI specifies a minimum time after
> > reset-deassertion before accessing the bus, and I don't see a sleep...
> 
> there is an msleep(100) there after the de-assertion.

.. and I'm blind, looks good.

> Ok - that all makes sense and hopefully when I get the legacy PCI
> interrupt mapping via DT working for IMX6 I can prove it out.
> 
> What did you think of my two ideas above to resolve this issue on our
> add-in card where the bus-topology isn't known ahead of time:
> 
>  1. bootloader builds out DT with proper interrupt-map's to handle the
> mis-map after enumerating the bus to understand the toplogy.

This is how the OF stuff for PCI has historically been used, so it is
a nice OS-agnostic way to implement this work around, particularly if
you have some kind of daughter card specific information you can rely
on (eg a SPD that could indicate the problematic PCB revision)

If you do that you could also hoist the required fixup for your switch
into firmware too.

>  2. a pci fixup for xio2001 that further qualifies the DT machine and
> the 'depth' of the xio2001 (ie make sure its the first xio2001 bridge
> on the bus) that replaces the swizzle function.  This would be in
> arch/arm/mach-imx/mach-imx6q.c

You can probably make this work, but given there is already a DT based
fix I'm not sure if people will like a .c version, especially if it
requires more single-use 'general purpose' interfaces from the core
code...

Jason