[PATCH v5 0/3] Tango PCIe controller support

[PATCH v5 0/3] Tango PCIe controller support

[Marc Gonzalez]

Spin v5 to address comments from Marc and Bjorn.

Changes from v4 to v5

- Split the DT binding into a third patch
- Introduce host bridge support first
- Put all MSI stuff in the MSI patch
- Inline 2 single-use functions
- Fix tango_ack
- Allocate irq at probe start
- Print dire message in probe, and taint kernel
	("Lasciate ogne speranza, voi ch'intrate")

Marc Gonzalez (3):
  PCI: Add DT binding for tango PCIe controller
  PCI: Add tango PCIe host bridge support
  PCI: Add tango MSI controller support

 Documentation/devicetree/bindings/pci/tango-pcie.txt |  30 +++
 drivers/pci/host/Kconfig                             |   8 +
 drivers/pci/host/Makefile                            |   1 +
 drivers/pci/host/pcie-tango.c                        | 386 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h                              |   2 +
 5 files changed, 427 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
 create mode 100644 drivers/pci/host/pcie-tango.c

-- 
2.11.0

[PATCH v5 1/3] PCI: Add DT binding for tango PCIe controller

[Marc Gonzalez]

Binding for the Sigma Designs SMP8759 SoC.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/pci/tango-pcie.txt | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
new file mode 100644
index 000000000000..35ef2c811a27
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
@@ -0,0 +1,30 @@
+Sigma Designs Tango PCIe controller
+
+Required properties:
+
+- compatible: "sigma,smp8759-pcie"
+- reg: address/size of PCI configuration space, address/size of register area
+- device_type: "pci"
+- #size-cells: <2>
+- #address-cells: <3>
+- msi-controller
+- ranges: translation from system to bus addresses
+- interrupts: spec for misc interrupts, spec for MSI
+
+http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
+http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
+
+Example:
+
+	pcie@2e000 {
+		compatible = "sigma,smp8759-pcie";
+		reg = <0x50000000 SZ_4M>, <0x2e000 0x100>;
+		device_type = "pci";
+		#size-cells = <2>;
+		#address-cells = <3>;
+		msi-controller;
+		ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 SZ_60M>;
+		interrupts =
+			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
+			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
+	};
-- 
2.11.0

Re: [PATCH v5 1/3] PCI: Add DT binding for tango PCIe controller

[Rob Herring]

On Wed, May 31, 2017 at 03:30:26PM +0200, Marc Gonzalez wrote:
> Binding for the Sigma Designs SMP8759 SoC.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> new file mode 100644
> index 000000000000..35ef2c811a27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> @@ -0,0 +1,30 @@
> +Sigma Designs Tango PCIe controller
> +
> +Required properties:
> +
> +- compatible: "sigma,smp8759-pcie"
> +- reg: address/size of PCI configuration space, address/size of register area
> +- device_type: "pci"
> +- #size-cells: <2>
> +- #address-cells: <3>
> +- msi-controller
> +- ranges: translation from system to bus addresses
> +- interrupts: spec for misc interrupts, spec for MSI
> +
> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

Why are these here?

There's several standard properties you are missing like bus-range. 
Build your dts with "W=2". dtc recently gained some checks for PCI 
bindings.

> +
> +Example:
> +
> +	pcie@2e000 {
> +		compatible = "sigma,smp8759-pcie";
> +		reg = <0x50000000 SZ_4M>, <0x2e000 0x100>;
> +		device_type = "pci";
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		msi-controller;
> +		ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 SZ_60M>;

I don't think SZ_60M exists or is available to dts files. Just put the 
number in.

> +		interrupts =
> +			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
> +			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
> +	};
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] PCI: Add DT binding for tango PCIe controller

[Mason]

Hello Rob,

On 07/06/2017 23:29, Rob Herring wrote:
> On Wed, May 31, 2017 at 03:30:26PM +0200, Marc Gonzalez wrote:
>> Binding for the Sigma Designs SMP8759 SoC.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> new file mode 100644
>> index 000000000000..35ef2c811a27
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> @@ -0,0 +1,30 @@
>> +Sigma Designs Tango PCIe controller
>> +
>> +Required properties:
>> +
>> +- compatible: "sigma,smp8759-pcie"
>> +- reg: address/size of PCI configuration space, address/size of register area
>> +- device_type: "pci"
>> +- #size-cells: <2>
>> +- #address-cells: <3>
>> +- msi-controller
>> +- ranges: translation from system to bus addresses
>> +- interrupts: spec for misc interrupts, spec for MSI
>> +
>> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
>> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> 
> Why are these here?

I found these references very helpful when writing the node.
Where would you put them? In the example?

> There's several standard properties you are missing like bus-range.

My reasoning for omitting "bus-range" was that the PCI core computes
it by itself (1M per "bus" so SZ_4M => 4 devices). I thought redundant
information was bad form?

> Build your dts with "W=2". dtc recently gained some checks for PCI 
> bindings.

I'll give it a try. Did v4.9 already support it?

>> +Example:
>> +
>> +	pcie@2e000 {
>> +		compatible = "sigma,smp8759-pcie";
>> +		reg = <0x50000000 SZ_4M>, <0x2e000 0x100>;
>> +		device_type = "pci";
>> +		#size-cells = <2>;
>> +		#address-cells = <3>;
>> +		msi-controller;
>> +		ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 SZ_60M>;
> 
> I don't think SZ_60M exists or is available to dts files. Just put the 
> number in.

I #defined it at the top of my DTS.
Using symbolic constants in DTS is not acceptable?

Thanks for having a look.

Regards.

Re: [PATCH v5 1/3] PCI: Add DT binding for tango PCIe controller

[Marc Zyngier]

On 07/06/17 23:34, Mason wrote:
> Hello Rob,
> 
> On 07/06/2017 23:29, Rob Herring wrote:
>> On Wed, May 31, 2017 at 03:30:26PM +0200, Marc Gonzalez wrote:
>>> Binding for the Sigma Designs SMP8759 SoC.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  Documentation/devicetree/bindings/pci/tango-pcie.txt | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> new file mode 100644
>>> index 000000000000..35ef2c811a27
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> @@ -0,0 +1,30 @@
>>> +Sigma Designs Tango PCIe controller
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: "sigma,smp8759-pcie"
>>> +- reg: address/size of PCI configuration space, address/size of register area
>>> +- device_type: "pci"
>>> +- #size-cells: <2>
>>> +- #address-cells: <3>
>>> +- msi-controller
>>> +- ranges: translation from system to bus addresses
>>> +- interrupts: spec for misc interrupts, spec for MSI
>>> +
>>> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
>>> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
>>
>> Why are these here?
> 
> I found these references very helpful when writing the node.
> Where would you put them? In the example?

I don't think they belong in this document at all.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 1/3] PCI: Add DT binding for tango PCIe controller

[Rob Herring]

On Wed, Jun 7, 2017 at 5:34 PM, Mason <slash.tmp@free.fr> wrote:
> Hello Rob,
>
> On 07/06/2017 23:29, Rob Herring wrote:
>> On Wed, May 31, 2017 at 03:30:26PM +0200, Marc Gonzalez wrote:
>>> Binding for the Sigma Designs SMP8759 SoC.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  Documentation/devicetree/bindings/pci/tango-pcie.txt | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> new file mode 100644
>>> index 000000000000..35ef2c811a27
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> @@ -0,0 +1,30 @@
>>> +Sigma Designs Tango PCIe controller
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: "sigma,smp8759-pcie"
>>> +- reg: address/size of PCI configuration space, address/size of register area
>>> +- device_type: "pci"
>>> +- #size-cells: <2>
>>> +- #address-cells: <3>
>>> +- msi-controller
>>> +- ranges: translation from system to bus addresses
>>> +- interrupts: spec for misc interrupts, spec for MSI
>>> +
>>> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
>>> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
>>
>> Why are these here?
>
> I found these references very helpful when writing the node.

Yes, I refer to them regularly.

> Where would you put them? In the example?

They are useful to every PCI binding. That doesn't mean we should link
to them in for every single PCI host binding doc. They belong in a DT
howto or something if not there already.

Rob

>
>> There's several standard properties you are missing like bus-range.
>
> My reasoning for omitting "bus-range" was that the PCI core computes
> it by itself (1M per "bus" so SZ_4M => 4 devices). I thought redundant
> information was bad form?
>
>> Build your dts with "W=2". dtc recently gained some checks for PCI
>> bindings.
>
> I'll give it a try. Did v4.9 already support it?

No, v4.12.

>
>>> +Example:
>>> +
>>> +    pcie@2e000 {
>>> +            compatible = "sigma,smp8759-pcie";
>>> +            reg = <0x50000000 SZ_4M>, <0x2e000 0x100>;
>>> +            device_type = "pci";
>>> +            #size-cells = <2>;
>>> +            #address-cells = <3>;
>>> +            msi-controller;
>>> +            ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 SZ_60M>;
>>
>> I don't think SZ_60M exists or is available to dts files. Just put the
>> number in.
>
> I #defined it at the top of my DTS.
> Using symbolic constants in DTS is not acceptable?

We generally don't use them here (i.e. for reg). The main use is for
things also used by drivers like GPIO flags or IDs such as clock IDs.
So please drop these.

Rob

[PATCH v6 1/3] PCI: Add DT binding for tango PCIe controller

[Marc Gonzalez]

Binding for the Sigma Designs SMP8759 SoC.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v5 to v6
o Delete links to elinux.org
o Use explicit hex numbers instead of symbolic constants for sizes (in the example)
o Add bus-range to "Required properties"
---
 .../devicetree/bindings/pci/tango-pcie.txt         | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
new file mode 100644
index 000000000000..244683836a79
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
@@ -0,0 +1,29 @@
+Sigma Designs Tango PCIe controller
+
+Required properties:
+
+- compatible: "sigma,smp8759-pcie"
+- reg: address/size of PCI configuration space, address/size of register area
+- bus-range: defined by size of PCI configuration space
+- device_type: "pci"
+- #size-cells: <2>
+- #address-cells: <3>
+- msi-controller
+- ranges: translation from system to bus addresses
+- interrupts: spec for misc interrupts, spec for MSI
+
+Example:
+
+	pcie@2e000 {
+		compatible = "sigma,smp8759-pcie";
+		reg = <0x50000000 0x400000>, <0x2e000 0x100>;
+		bus-range = <0 3>;
+		device_type = "pci";
+		#size-cells = <2>;
+		#address-cells = <3>;
+		msi-controller;
+		ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 0x3c00000>;
+		interrupts =
+			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
+			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
+	};
-- 
2.11.0

Re: [PATCH v6 1/3] PCI: Add DT binding for tango PCIe controller

[Rob Herring]

On Tue, Jun 13, 2017 at 03:51:32PM +0200, Marc Gonzalez wrote:
> Binding for the Sigma Designs SMP8759 SoC.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> Changes from v5 to v6
> o Delete links to elinux.org
> o Use explicit hex numbers instead of symbolic constants for sizes (in the example)
> o Add bus-range to "Required properties"
> ---
>  .../devicetree/bindings/pci/tango-pcie.txt         | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

This driver is required to work around several hardware bugs
in the PCIe controller.

NB: Revision 1 does not support legacy interrupts, or IO space.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/pci/host/Kconfig      |   8 +++
 drivers/pci/host/Makefile     |   1 +
 drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h       |   2 +
 4 files changed, 175 insertions(+)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a827c3..5183d9095c3a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -285,6 +285,14 @@ config PCIE_ROCKCHIP
 	  There is 1 internal PCIe port available to support GEN2 with
 	  4 slots.
 
+config PCIE_TANGO
+	bool "Tango PCIe controller"
+	depends on ARCH_TANGO && PCI_MSI && OF
+	select PCI_HOST_COMMON
+	help
+	  Say Y here to enable PCIe controller support for Sigma Designs
+	  Tango systems, such as SMP8759 and later chips.
+
 config VMD
 	depends on PCI_MSI && X86_64
 	tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 084cb4983645..fc7ea90196f3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
new file mode 100644
index 000000000000..67aaadcc1c5e
--- /dev/null
+++ b/drivers/pci/host/pcie-tango.c
@@ -0,0 +1,164 @@
+#include <linux/pci-ecam.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+
+#define MSI_MAX 256
+
+#define SMP8759_MUX		0x48
+#define SMP8759_TEST_OUT	0x74
+
+struct tango_pcie {
+	void __iomem *mux;
+};
+
+/*** HOST BRIDGE SUPPORT ***/
+
+static int smp8759_config_read(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 *val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	/*
+	 * QUIRK #1
+	 * Reads in configuration space outside devfn 0 return garbage.
+	 */
+	if (devfn != 0)
+		return PCIBIOS_FUNC_NOT_SUPPORTED;
+
+	/*
+	 * QUIRK #2
+	 * Unfortunately, config and mem spaces are muxed.
+	 * Linux does not support such a setting, since drivers are free
+	 * to access mem space directly, at any time.
+	 * Therefore, we can only PRAY that config and mem space accesses
+	 * NEVER occur concurrently.
+	 */
+	writel_relaxed(1, pcie->mux);
+	ret = pci_generic_config_read(bus, devfn, where, size, val);
+	writel_relaxed(0, pcie->mux);
+
+	return ret;
+}
+
+static int smp8759_config_write(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	writel_relaxed(1, pcie->mux);
+	ret = pci_generic_config_write(bus, devfn, where, size, val);
+	writel_relaxed(0, pcie->mux);
+
+	return ret;
+}
+
+static struct pci_ecam_ops smp8759_ecam_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= smp8759_config_read,
+		.write		= smp8759_config_write,
+	}
+};
+
+static const struct of_device_id tango_pcie_ids[] = {
+	{ .compatible = "sigma,smp8759-pcie" },
+	{ /* sentinel */ },
+};
+
+static int tango_check_pcie_link(void __iomem *test_out)
+{
+	int i;
+
+	writel_relaxed(16, test_out);
+	for (i = 0; i < 10; ++i) {
+		u32 ltssm_state = readl_relaxed(test_out) >> 8;
+		if ((ltssm_state & 0x1f) == 0xf) /* L0 */
+			return 0;
+		usleep_range(3000, 4000);
+	}
+
+	return -ENODEV;
+}
+
+static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	pcie->mux		= base + SMP8759_MUX;
+
+	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
+}
+
+static int tango_pcie_probe(struct platform_device *pdev)
+{
+	int ret = -EINVAL;
+	void __iomem *base;
+	struct resource *res;
+	struct tango_pcie *pcie;
+	struct device *dev = &pdev->dev;
+
+	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
+	pr_err("Tainting kernel... Use driver at your own risk\n");
+	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pcie);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
+		ret = smp8759_init(pcie, base);
+
+	if (ret)
+		return ret;
+
+	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
+}
+
+static struct platform_driver tango_pcie_driver = {
+	.probe	= tango_pcie_probe,
+	.driver	= {
+		.name = KBUILD_MODNAME,
+		.of_match_table = tango_pcie_ids,
+	},
+};
+
+builtin_platform_driver(tango_pcie_driver);
+
+/*
+ * QUIRK #3
+ * The root complex advertizes the wrong device class.
+ * Header Type 1 is for PCI-to-PCI bridges.
+ */
+static void tango_fixup_class(struct pci_dev *dev)
+{
+	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_class);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_class);
+
+/*
+ * QUIRK #4
+ * The root complex exposes a "fake" BAR, which is used to filter
+ * bus-to-system accesses. Only accesses within the range defined
+ * by this BAR are forwarded to the host, others are ignored.
+ *
+ * By default, the DMA framework expects an identity mapping,
+ * and DRAM0 is mapped at 0x80000000.
+ */
+static void tango_fixup_bar(struct pci_dev *dev)
+{
+	dev->non_compliant_bars = true;
+	pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_bar);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index f020ab4079d3..b577dbe46f8f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1369,6 +1369,8 @@
 #define PCI_DEVICE_ID_TTI_HPT374	0x0008
 #define PCI_DEVICE_ID_TTI_HPT372N	0x0009	/* apparently a 372N variant? */
 
+#define PCI_VENDOR_ID_SIGMA		0x1105
+
 #define PCI_VENDOR_ID_VIA		0x1106
 #define PCI_DEVICE_ID_VIA_8763_0	0x0198
 #define PCI_DEVICE_ID_VIA_8380_0	0x0204
-- 
2.11.0

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

On 31/05/2017 15:33, Marc Gonzalez wrote:

> +static int tango_pcie_probe(struct platform_device *pdev)
> +{
> +	int ret = -EINVAL;
> +	void __iomem *base;
> +	struct resource *res;
> +	struct tango_pcie *pcie;
> +	struct device *dev = &pdev->dev;
> +
> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
> +	pr_err("Tainting kernel... Use driver at your own risk\n");
> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);

Hello Bjorn,

In v4, your only comment was
"[muxing config and mem spaces] is a major issue and possibly even
a security problem [which requires at least an error message and a
kernel taint].

Were there any other issues with the host bridge support?
Or is the HW quirk/bug too severe to mainline the driver?
(I would hate having to discard all that work though.)

Regards.

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

On 07/06/2017 10:19, Marc Gonzalez wrote:

> On 31/05/2017 15:33, Marc Gonzalez wrote:
> 
>> +static int tango_pcie_probe(struct platform_device *pdev)
>> +{
>> +	int ret = -EINVAL;
>> +	void __iomem *base;
>> +	struct resource *res;
>> +	struct tango_pcie *pcie;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
>> +	pr_err("Tainting kernel... Use driver at your own risk\n");
>> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> 
> Hello Bjorn,
> 
> In v4, your only comment was
> "[muxing config and mem spaces] is a major issue and possibly even
> a security problem [which requires at least an error message and a
> kernel taint].
> 
> Were there any other issues with the host bridge support?

Hello Bjorn,

I imagine you are now only waiting for Marc Z's Ack of patch 3/3 ?
(AFAIU, all issues have been addressed as of v8 3/3.)

Rob has Acked patch 1/3 -- thanks Rob.

Assuming all issues with patch 2/3 have already been addressed,
then this driver can land in time for 4.13 right?

Regards.

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Zyngier]

On 19/06/17 15:50, Marc Gonzalez wrote:
> On 07/06/2017 10:19, Marc Gonzalez wrote:
> 
>> On 31/05/2017 15:33, Marc Gonzalez wrote:
>>
>>> +static int tango_pcie_probe(struct platform_device *pdev)
>>> +{
>>> +	int ret = -EINVAL;
>>> +	void __iomem *base;
>>> +	struct resource *res;
>>> +	struct tango_pcie *pcie;
>>> +	struct device *dev = &pdev->dev;
>>> +
>>> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
>>> +	pr_err("Tainting kernel... Use driver at your own risk\n");
>>> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
>>
>> Hello Bjorn,
>>
>> In v4, your only comment was
>> "[muxing config and mem spaces] is a major issue and possibly even
>> a security problem [which requires at least an error message and a
>> kernel taint].
>>
>> Were there any other issues with the host bridge support?
> 
> Hello Bjorn,
> 
> I imagine you are now only waiting for Marc Z's Ack of patch 3/3 ?
> (AFAIU, all issues have been addressed as of v8 3/3.)

Posting partial series is really not ideal, specially as you posted 3 of
them in less than 24 hours (and the latest was only 5 days ago).

The maintainer is not going to chase you about potentially missing
patches...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

On 19/06/2017 17:58, Marc Zyngier wrote:
> On 19/06/17 15:50, Marc Gonzalez wrote:
>> On 07/06/2017 10:19, Marc Gonzalez wrote:
>>
>>> On 31/05/2017 15:33, Marc Gonzalez wrote:
>>>
>>>> +static int tango_pcie_probe(struct platform_device *pdev)
>>>> +{
>>>> +	int ret = -EINVAL;
>>>> +	void __iomem *base;
>>>> +	struct resource *res;
>>>> +	struct tango_pcie *pcie;
>>>> +	struct device *dev = &pdev->dev;
>>>> +
>>>> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
>>>> +	pr_err("Tainting kernel... Use driver at your own risk\n");
>>>> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
>>>
>>> Hello Bjorn,
>>>
>>> In v4, your only comment was
>>> "[muxing config and mem spaces] is a major issue and possibly even
>>> a security problem [which requires at least an error message and a
>>> kernel taint].
>>>
>>> Were there any other issues with the host bridge support?
>>
>> Hello Bjorn,
>>
>> I imagine you are now only waiting for Marc Z's Ack of patch 3/3 ?
>> (AFAIU, all issues have been addressed as of v8 3/3.)
> 
> Posting partial series is really not ideal, specially as you posted 3 of
> them in less than 24 hours (and the latest was only 5 days ago).
> 
> The maintainer is not going to chase you about potentially missing
> patches...

So the best course would be posting a new series with all patches
bumped to v8 then?

Regards.

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Zyngier]

On 19/06/17 17:01, Marc Gonzalez wrote:
> On 19/06/2017 17:58, Marc Zyngier wrote:
>> On 19/06/17 15:50, Marc Gonzalez wrote:
>>> On 07/06/2017 10:19, Marc Gonzalez wrote:
>>>
>>>> On 31/05/2017 15:33, Marc Gonzalez wrote:
>>>>
>>>>> +static int tango_pcie_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	int ret = -EINVAL;
>>>>> +	void __iomem *base;
>>>>> +	struct resource *res;
>>>>> +	struct tango_pcie *pcie;
>>>>> +	struct device *dev = &pdev->dev;
>>>>> +
>>>>> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
>>>>> +	pr_err("Tainting kernel... Use driver at your own risk\n");
>>>>> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
>>>>
>>>> Hello Bjorn,
>>>>
>>>> In v4, your only comment was
>>>> "[muxing config and mem spaces] is a major issue and possibly even
>>>> a security problem [which requires at least an error message and a
>>>> kernel taint].
>>>>
>>>> Were there any other issues with the host bridge support?
>>>
>>> Hello Bjorn,
>>>
>>> I imagine you are now only waiting for Marc Z's Ack of patch 3/3 ?
>>> (AFAIU, all issues have been addressed as of v8 3/3.)
>>
>> Posting partial series is really not ideal, specially as you posted 3 of
>> them in less than 24 hours (and the latest was only 5 days ago).
>>
>> The maintainer is not going to chase you about potentially missing
>> patches...
> 
> So the best course would be posting a new series with all patches
> bumped to v8 then?

No, because there is still the ambiguity of *which* v8 that is. Post a
v9 that supersedes all the previously posted patches.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

On 19/06/2017 16:50, Marc Gonzalez wrote:

> On 07/06/2017 10:19, Marc Gonzalez wrote:
> 
>> On 31/05/2017 15:33, Marc Gonzalez wrote:
>>
>>> +static int tango_pcie_probe(struct platform_device *pdev)
>>> +{
>>> +	int ret = -EINVAL;
>>> +	void __iomem *base;
>>> +	struct resource *res;
>>> +	struct tango_pcie *pcie;
>>> +	struct device *dev = &pdev->dev;
>>> +
>>> +	pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
>>> +	pr_err("Tainting kernel... Use driver at your own risk\n");
>>> +	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
>>
>> Hello Bjorn,
>>
>> In v4, your only comment was
>> "[muxing config and mem spaces] is a major issue and possibly even
>> a security problem [which requires at least an error message and a
>> kernel taint].
>>
>> Were there any other issues with the host bridge support?
> 
> Hello Bjorn,
> 
> I imagine you are now only waiting for Marc Z's Ack of patch 3/3 ?
> (AFAIU, all issues have been addressed as of v8 3/3.)
> 
> Rob has Acked patch 1/3 -- thanks Rob.
> 
> Assuming all issues with patch 2/3 have already been addressed,
> then this driver can land in time for 4.13 right?

Hello Bjorn, Marc,

Patch series v9 supersedes all previously posted patches.

Regards.

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Ard Biesheuvel]

On 31 May 2017 at 13:33, Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:
> This driver is required to work around several hardware bugs
> in the PCIe controller.
>
> NB: Revision 1 does not support legacy interrupts, or IO space.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/pci/host/Kconfig      |   8 +++
>  drivers/pci/host/Makefile     |   1 +
>  drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h       |   2 +
>  4 files changed, 175 insertions(+)
>
[...]
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> new file mode 100644
> index 000000000000..67aaadcc1c5e
> --- /dev/null
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -0,0 +1,164 @@
> +#include <linux/pci-ecam.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +
> +#define MSI_MAX 256
> +
> +#define SMP8759_MUX            0x48
> +#define SMP8759_TEST_OUT       0x74
> +
> +struct tango_pcie {
> +       void __iomem *mux;
> +};
> +
> +/*** HOST BRIDGE SUPPORT ***/
> +
> +static int smp8759_config_read(struct pci_bus *bus,
> +               unsigned int devfn, int where, int size, u32 *val)
> +{
> +       int ret;
> +       struct pci_config_window *cfg = bus->sysdata;
> +       struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> +       /*
> +        * QUIRK #1
> +        * Reads in configuration space outside devfn 0 return garbage.
> +        */
> +       if (devfn != 0)
> +               return PCIBIOS_FUNC_NOT_SUPPORTED;
> +

Does this mean multi-function devices are not supported? E.g., on my
system I have

-[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00
           +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01
           +-02.1-[01]----00.0  Renesas Technology Corp. uPD720201 USB 3.0
           \-02.2-[02]--+-00.0  NVIDIA Corporation GT218 [GeForce 210]
                        \-00.1  NVIDIA Corporation HD Audio Controller

where the HDMI audio is on devfn 00.1 on bus 2


> +       /*
> +        * QUIRK #2
> +        * Unfortunately, config and mem spaces are muxed.
> +        * Linux does not support such a setting, since drivers are free
> +        * to access mem space directly, at any time.
> +        * Therefore, we can only PRAY that config and mem space accesses
> +        * NEVER occur concurrently.
> +        */
> +       writel_relaxed(1, pcie->mux);
> +       ret = pci_generic_config_read(bus, devfn, where, size, val);
> +       writel_relaxed(0, pcie->mux);
> +
> +       return ret;
> +}
> +
> +static int smp8759_config_write(struct pci_bus *bus,
> +               unsigned int devfn, int where, int size, u32 val)
> +{
> +       int ret;
> +       struct pci_config_window *cfg = bus->sysdata;
> +       struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> +       writel_relaxed(1, pcie->mux);
> +       ret = pci_generic_config_write(bus, devfn, where, size, val);
> +       writel_relaxed(0, pcie->mux);
> +
> +       return ret;
> +}
> +
> +static struct pci_ecam_ops smp8759_ecam_ops = {
> +       .bus_shift      = 20,
> +       .pci_ops        = {
> +               .map_bus        = pci_ecam_map_bus,
> +               .read           = smp8759_config_read,
> +               .write          = smp8759_config_write,
> +       }
> +};
> +
> +static const struct of_device_id tango_pcie_ids[] = {
> +       { .compatible = "sigma,smp8759-pcie" },
> +       { /* sentinel */ },
> +};
> +
> +static int tango_check_pcie_link(void __iomem *test_out)
> +{
> +       int i;
> +
> +       writel_relaxed(16, test_out);
> +       for (i = 0; i < 10; ++i) {
> +               u32 ltssm_state = readl_relaxed(test_out) >> 8;
> +               if ((ltssm_state & 0x1f) == 0xf) /* L0 */
> +                       return 0;
> +               usleep_range(3000, 4000);
> +       }
> +
> +       return -ENODEV;
> +}
> +
> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> +       pcie->mux               = base + SMP8759_MUX;
> +
> +       return tango_check_pcie_link(base + SMP8759_TEST_OUT);
> +}
> +
> +static int tango_pcie_probe(struct platform_device *pdev)
> +{
> +       int ret = -EINVAL;
> +       void __iomem *base;
> +       struct resource *res;
> +       struct tango_pcie *pcie;
> +       struct device *dev = &pdev->dev;
> +
> +       pr_err("MAJOR ISSUE: PCIe config and mem spaces are muxed\n");
> +       pr_err("Tainting kernel... Use driver at your own risk\n");
> +       add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> +
> +       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> +       if (!pcie)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, pcie);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
> +               ret = smp8759_init(pcie, base);
> +
> +       if (ret)
> +               return ret;
> +
> +       return pci_host_common_probe(pdev, &smp8759_ecam_ops);
> +}
> +
> +static struct platform_driver tango_pcie_driver = {
> +       .probe  = tango_pcie_probe,
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +               .of_match_table = tango_pcie_ids,
> +       },
> +};
> +
> +builtin_platform_driver(tango_pcie_driver);
> +
> +/*
> + * QUIRK #3
> + * The root complex advertizes the wrong device class.
> + * Header Type 1 is for PCI-to-PCI bridges.
> + */
> +static void tango_fixup_class(struct pci_dev *dev)
> +{
> +       dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_class);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_class);
> +
> +/*
> + * QUIRK #4
> + * The root complex exposes a "fake" BAR, which is used to filter
> + * bus-to-system accesses. Only accesses within the range defined
> + * by this BAR are forwarded to the host, others are ignored.
> + *
> + * By default, the DMA framework expects an identity mapping,
> + * and DRAM0 is mapped at 0x80000000.
> + */
> +static void tango_fixup_bar(struct pci_dev *dev)
> +{
> +       dev->non_compliant_bars = true;
> +       pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_bar);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_bar);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index f020ab4079d3..b577dbe46f8f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1369,6 +1369,8 @@
>  #define PCI_DEVICE_ID_TTI_HPT374       0x0008
>  #define PCI_DEVICE_ID_TTI_HPT372N      0x0009  /* apparently a 372N variant? */
>
> +#define PCI_VENDOR_ID_SIGMA            0x1105
> +
>  #define PCI_VENDOR_ID_VIA              0x1106
>  #define PCI_DEVICE_ID_VIA_8763_0       0x0198
>  #define PCI_DEVICE_ID_VIA_8380_0       0x0204
> --
> 2.11.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/3] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

On 05/07/2017 11:36, Ard Biesheuvel wrote:

> On 31 May 2017 at 13:33, Marc Gonzalez wrote:
>
>> This driver is required to work around several hardware bugs
>> in the PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> +       /*
>> +        * QUIRK #1
>> +        * Reads in configuration space outside devfn 0 return garbage.
>> +        */
>> +       if (devfn != 0)
>> +               return PCIBIOS_FUNC_NOT_SUPPORTED;
>> +
> 
> Does this mean multi-function devices are not supported? E.g., on my
> system I have
> 
> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00
>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01
>            +-02.1-[01]----00.0  Renesas Technology Corp. uPD720201 USB 3.0
>            \-02.2-[02]--+-00.0  NVIDIA Corporation GT218 [GeForce 210]
>                         \-00.1  NVIDIA Corporation HD Audio Controller
> 
> where the HDMI audio is on devfn 00.1 on bus 2

Thanks for having a look. Here's the situation.

# lspci -v
00:00.0 PCI bridge: Sigma Designs, Inc. Device 0024 (rev 01) (prog-if 00 [Normal decode])
        Flags: bus master, fast devsel, latency 0, IRQ 31
        Memory at <ignored> (64-bit, non-prefetchable)
        Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
        I/O behind bridge: 00000000-00000fff
        Memory behind bridge: 00400000-004fffff
        Prefetchable memory behind bridge: 00000000-000fffff
        Capabilities: [50] MSI: Enable+ Count=1/4 Maskable- 64bit+
        Capabilities: [78] Power Management version 3
        Capabilities: [80] Express Root Port (Slot-), MSI 03
        Capabilities: [100] Virtual Channel
        Capabilities: [800] Advanced Error Reporting
        Kernel driver in use: pcieport

01:00.0 USB controller: Renesas Technology Corp. uPD720201 USB 3.0 Host Controller (rev 03) (prog-if 30 [XHCI])
        Flags: bus master, fast devsel, latency 0, IRQ 21
        Memory at 50400000 (64-bit, non-prefetchable) [size=8K]
        Capabilities: [50] Power Management version 3
        Capabilities: [70] MSI: Enable- Count=1/8 Maskable- 64bit+
        Capabilities: [90] MSI-X: Enable+ Count=8 Masked-
        Capabilities: [a0] Express Endpoint, MSI 00
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [150] Latency Tolerance Reporting
        Kernel driver in use: xhci_hcd


IIUC, bus 0 will always be the PCIe host bridge.
On bus 0, reads outside of devfn 0 return garbage.
I think (?) this is not an issue, because the host bridge
is not multi-function.

There is a /separate/ erratum for bus 1. The HW returns
garbage when enumerating non-existent devfn.
IIUC, there is an OOB SoC-specific error-reporting mechanism,
so it might be possible to check for an error after every read,
and replace the garbage with -1u on error.

The errata list does not mention buses > 1 but I would assume
they are, at a minimum, affected by the "bus 1" errata --
such setups were not tested at all. (It would require some
kind of PCIe switch, I suppose.)

As a first-order approximation, I just conflated the two
errata, and ignored multi-function EPs.

NB: the "typical" use-case for the PCIe slot is adding a validated
WiFi card, because the SoC doesn't support WiFi natively.
Some customers also consider using a USB3 card, instead of the
native USB2 Chipidea controller.

Regards.

[PATCH v5 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/pci/host/pcie-tango.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 222 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..7d99ef26173f 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,225 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used, MSI_MAX);
+	spinlock_t lock;
 	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_enable;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_dom;
+	struct irq_domain *msi_dom;
+	int irq;
 };
 
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned long status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+
+	while ((pos = find_next_bit(pcie->used, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->hwirq;
+}
+
+static struct irq_chip tango_chip = {
+	.irq_ack		= tango_ack,
+	.irq_mask		= tango_mask,
+	.irq_unmask		= tango_unmask,
+	.irq_set_affinity	= tango_set_affinity,
+	.irq_compose_msi_msg	= tango_compose_msi_msg,
+};
+
+static void msi_ack(struct irq_data *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip msi_chip = {
+	.name = "MSI",
+	.irq_ack = msi_ack,
+	.irq_mask = msi_mask,
+	.irq_unmask = msi_unmask,
+};
+
+static struct msi_domain_info msi_dom_info = {
+	.flags	= MSI_FLAG_PCI_MSIX
+		| MSI_FLAG_USE_DEF_DOM_OPS
+		| MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &msi_chip,
+};
+
+static int tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	unsigned long flags;
+	int pos, err = -ENOSPC;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->lock, flags);
+	pos = find_first_zero_bit(pcie->used, MSI_MAX);
+	if (pos < MSI_MAX) {
+		err = 0;
+		__set_bit(pos, pcie->used);
+		irq_domain_set_info(dom, virq, pos,
+				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
+	}
+	spin_unlock_irqrestore(&pcie->lock, flags);
+
+	return err;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	unsigned long flags;
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->lock, flags);
+	__clear_bit(d->hwirq, pcie->used);
+	spin_unlock_irqrestore(&pcie->lock, flags);
+}
+
+static const struct irq_domain_ops irq_dom_ops = {
+	.alloc	= tango_irq_domain_alloc,
+	.free	= tango_irq_domain_free,
+};
+
+static int tango_msi_remove(struct platform_device *pdev)
+{
+	struct tango_pcie *pcie = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+	irq_domain_remove(pcie->msi_dom);
+	irq_domain_remove(pcie->irq_dom);
+
+	return 0;
+}
+
+static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
+{
+	int i, virq;
+	struct irq_domain *msi_dom, *irq_dom;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
+	spin_lock_init(&pcie->lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
+	if (!irq_dom) {
+		pr_err("Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
+	if (!msi_dom) {
+		pr_err("Failed to create MSI domain\n");
+		return -ENOMEM;
+	}
+
+	pcie->irq_dom = irq_dom;
+	pcie->msi_dom = msi_dom;
+	pcie->irq = virq;
+
+	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
+
+	return 0;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +297,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +333,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = tango_msi_probe(pdev, pcie);
+	if (ret)
+		return ret;
+
 	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
 }
 
+static int tango_pcie_remove(struct platform_device *pdev)
+{
+	return tango_msi_remove(pdev);
+}
+
 static struct platform_driver tango_pcie_driver = {
 	.probe	= tango_pcie_probe,
+	.remove	= tango_pcie_remove,
 	.driver	= {
 		.name = KBUILD_MODNAME,
 		.of_match_table = tango_pcie_ids,
-- 
2.11.0

Re: [PATCH v5 3/3] PCI: Add tango MSI controller support

[Marc Zyngier]

On 31/05/17 14:35, Marc Gonzalez wrote:
> The MSI controller in Tango supports 256 message-signaled interrupts,
> and a single doorbell address.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/pci/host/pcie-tango.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 222 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> index 67aaadcc1c5e..7d99ef26173f 100644
> --- a/drivers/pci/host/pcie-tango.c
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -1,16 +1,225 @@
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/delay.h>
> +#include <linux/msi.h>
>  #include <linux/of.h>
>  
>  #define MSI_MAX 256
>  
>  #define SMP8759_MUX		0x48
>  #define SMP8759_TEST_OUT	0x74
> +#define SMP8759_STATUS		0x80
> +#define SMP8759_ENABLE		0xa0
> +#define SMP8759_DOORBELL	0xa002e07c
>  
>  struct tango_pcie {
> +	DECLARE_BITMAP(used, MSI_MAX);
> +	spinlock_t lock;

These two fields have pretty generic names. Consider naming them in a
way that indicates their purpose (used_msi? used_msi_lock?).

>  	void __iomem *mux;
> +	void __iomem *msi_status;
> +	void __iomem *msi_enable;
> +	phys_addr_t msi_doorbell;
> +	struct irq_domain *irq_dom;
> +	struct irq_domain *msi_dom;
> +	int irq;
>  };
>  
> +/*** MSI CONTROLLER SUPPORT ***/
> +
> +static void tango_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
> +	unsigned long status, base, virq, idx, pos = 0;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	while ((pos = find_next_bit(pcie->used, MSI_MAX, pos)) < MSI_MAX) {

The pcie->used bitmap read can race against another CPU accessing this
bitmap in the alloc/free paths.

> +		base = round_down(pos, 32);
> +		status = readl_relaxed(pcie->msi_status + base / 8);
> +		for_each_set_bit(idx, &status, 32) {
> +			virq = irq_find_mapping(pcie->irq_dom, base + idx);
> +			generic_handle_irq(virq);
> +		}
> +		pos = base + 32;
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void tango_ack(struct irq_data *d)
> +{
> +	struct tango_pcie *pcie = d->chip_data;
> +	u32 offset = (d->hwirq / 32) * 4;
> +	u32 bit = BIT(d->hwirq % 32);
> +
> +	writel_relaxed(bit, pcie->msi_status + offset);
> +}
> +
> +static void update_msi_enable(struct irq_data *d, bool unmask)
> +{
> +	unsigned long flags;
> +	struct tango_pcie *pcie = d->chip_data;
> +	u32 offset = (d->hwirq / 32) * 4;
> +	u32 bit = BIT(d->hwirq % 32);
> +	u32 val;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	val = readl_relaxed(pcie->msi_enable + offset);
> +	val = unmask ? val | bit : val & ~bit;
> +	writel_relaxed(val, pcie->msi_enable + offset);
> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +}
> +
> +static void tango_mask(struct irq_data *d)
> +{
> +	update_msi_enable(d, false);
> +}
> +
> +static void tango_unmask(struct irq_data *d)
> +{
> +	update_msi_enable(d, true);
> +}
> +
> +static int tango_set_affinity(struct irq_data *d,
> +		const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> +	struct tango_pcie *pcie = d->chip_data;
> +	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
> +	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
> +	msg->data = d->hwirq;
> +}
> +
> +static struct irq_chip tango_chip = {
> +	.irq_ack		= tango_ack,
> +	.irq_mask		= tango_mask,
> +	.irq_unmask		= tango_unmask,
> +	.irq_set_affinity	= tango_set_affinity,
> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
> +};
> +
> +static void msi_ack(struct irq_data *d)
> +{
> +	irq_chip_ack_parent(d);
> +}
> +
> +static void msi_mask(struct irq_data *d)
> +{
> +	pci_msi_mask_irq(d);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void msi_unmask(struct irq_data *d)
> +{
> +	pci_msi_unmask_irq(d);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +static struct irq_chip msi_chip = {
> +	.name = "MSI",
> +	.irq_ack = msi_ack,
> +	.irq_mask = msi_mask,
> +	.irq_unmask = msi_unmask,
> +};
> +
> +static struct msi_domain_info msi_dom_info = {
> +	.flags	= MSI_FLAG_PCI_MSIX
> +		| MSI_FLAG_USE_DEF_DOM_OPS
> +		| MSI_FLAG_USE_DEF_CHIP_OPS,
> +	.chip	= &msi_chip,
> +};
> +
> +static int tango_irq_domain_alloc(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs, void *args)
> +{
> +	unsigned long flags;
> +	int pos, err = -ENOSPC;
> +	struct tango_pcie *pcie = dom->host_data;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	pos = find_first_zero_bit(pcie->used, MSI_MAX);
> +	if (pos < MSI_MAX) {
> +		err = 0;
> +		__set_bit(pos, pcie->used);
> +		irq_domain_set_info(dom, virq, pos,
> +				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
> +	}
> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +
> +	return err;
> +}
> +
> +static void tango_irq_domain_free(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs)
> +{
> +	unsigned long flags;
> +	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
> +	struct tango_pcie *pcie = d->chip_data;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	__clear_bit(d->hwirq, pcie->used);
> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +}
> +
> +static const struct irq_domain_ops irq_dom_ops = {
> +	.alloc	= tango_irq_domain_alloc,
> +	.free	= tango_irq_domain_free,
> +};
> +
> +static int tango_msi_remove(struct platform_device *pdev)
> +{
> +	struct tango_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +	irq_domain_remove(pcie->msi_dom);
> +	irq_domain_remove(pcie->irq_dom);
> +
> +	return 0;
> +}
> +
> +static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
> +{
> +	int i, virq;
> +	struct irq_domain *msi_dom, *irq_dom;
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +
> +	spin_lock_init(&pcie->lock);
> +	for (i = 0; i < MSI_MAX / 32; ++i)
> +		writel_relaxed(0, pcie->msi_enable + i * 4);
> +
> +	virq = platform_get_irq(pdev, 1);
> +	if (virq <= 0) {
> +		pr_err("Failed to map IRQ\n");
> +		return -ENXIO;
> +	}
> +
> +	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
> +	if (!irq_dom) {
> +		pr_err("Failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
> +	if (!msi_dom) {
> +		pr_err("Failed to create MSI domain\n");
> +		return -ENOMEM;

You're now leaking irq_dom.

> +	}
> +
> +	pcie->irq_dom = irq_dom;
> +	pcie->msi_dom = msi_dom;
> +	pcie->irq = virq;
> +
> +	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
> +
> +	return 0;
> +}
> +
>  /*** HOST BRIDGE SUPPORT ***/
>  
>  static int smp8759_config_read(struct pci_bus *bus,
> @@ -88,6 +297,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
>  static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>  {
>  	pcie->mux		= base + SMP8759_MUX;
> +	pcie->msi_status	= base + SMP8759_STATUS;
> +	pcie->msi_enable	= base + SMP8759_ENABLE;
> +	pcie->msi_doorbell	= SMP8759_DOORBELL;
>  
>  	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
>  }
> @@ -121,11 +333,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = tango_msi_probe(pdev, pcie);
> +	if (ret)
> +		return ret;
> +
>  	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
>  }
>  
> +static int tango_pcie_remove(struct platform_device *pdev)
> +{
> +	return tango_msi_remove(pdev);
> +}
> +
>  static struct platform_driver tango_pcie_driver = {
>  	.probe	= tango_pcie_probe,
> +	.remove	= tango_pcie_remove,
>  	.driver	= {
>  		.name = KBUILD_MODNAME,
>  		.of_match_table = tango_pcie_ids,
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v6 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v5 to v6
o Rename 'used' bitmap to 'used_msi'
o Rename 'lock' spinlock to 'used_msi_lock'
o Take lock in interrupt handler
o Remove irq_dom in error path
---
 drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..b06446b23bc8 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,228 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_lock;
 	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_enable;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_dom;
+	struct irq_domain *msi_dom;
+	int irq;
 };
 
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned long flags, status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+
+	while ((pos = find_next_bit(pcie->used_msi, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->hwirq;
+}
+
+static struct irq_chip tango_chip = {
+	.irq_ack		= tango_ack,
+	.irq_mask		= tango_mask,
+	.irq_unmask		= tango_unmask,
+	.irq_set_affinity	= tango_set_affinity,
+	.irq_compose_msi_msg	= tango_compose_msi_msg,
+};
+
+static void msi_ack(struct irq_data *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip msi_chip = {
+	.name = "MSI",
+	.irq_ack = msi_ack,
+	.irq_mask = msi_mask,
+	.irq_unmask = msi_unmask,
+};
+
+static struct msi_domain_info msi_dom_info = {
+	.flags	= MSI_FLAG_PCI_MSIX
+		| MSI_FLAG_USE_DEF_DOM_OPS
+		| MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &msi_chip,
+};
+
+static int tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	unsigned long flags;
+	int pos, err = -ENOSPC;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	pos = find_first_zero_bit(pcie->used_msi, MSI_MAX);
+	if (pos < MSI_MAX) {
+		err = 0;
+		__set_bit(pos, pcie->used_msi);
+		irq_domain_set_info(dom, virq, pos,
+				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
+	}
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+
+	return err;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	unsigned long flags;
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	__clear_bit(d->hwirq, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static const struct irq_domain_ops irq_dom_ops = {
+	.alloc	= tango_irq_domain_alloc,
+	.free	= tango_irq_domain_free,
+};
+
+static int tango_msi_remove(struct platform_device *pdev)
+{
+	struct tango_pcie *pcie = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+	irq_domain_remove(pcie->msi_dom);
+	irq_domain_remove(pcie->irq_dom);
+
+	return 0;
+}
+
+static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
+{
+	int i, virq;
+	struct irq_domain *msi_dom, *irq_dom;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
+	spin_lock_init(&pcie->used_msi_lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
+	if (!irq_dom) {
+		pr_err("Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
+	if (!msi_dom) {
+		pr_err("Failed to create MSI domain\n");
+		irq_domain_remove(irq_dom);
+		return -ENOMEM;
+	}
+
+	pcie->irq_dom = irq_dom;
+	pcie->msi_dom = msi_dom;
+	pcie->irq = virq;
+
+	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
+
+	return 0;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +300,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +336,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = tango_msi_probe(pdev, pcie);
+	if (ret)
+		return ret;
+
 	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
 }
 
+static int tango_pcie_remove(struct platform_device *pdev)
+{
+	return tango_msi_remove(pdev);
+}
+
 static struct platform_driver tango_pcie_driver = {
 	.probe	= tango_pcie_probe,
+	.remove	= tango_pcie_remove,
 	.driver	= {
 		.name = KBUILD_MODNAME,
 		.of_match_table = tango_pcie_ids,
-- 
2.11.0

Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

[Marc Zyngier]

On 13/06/17 15:01, Marc Gonzalez wrote:
> The MSI controller in Tango supports 256 message-signaled interrupts,
> and a single doorbell address.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> Changes from v5 to v6
> o Rename 'used' bitmap to 'used_msi'
> o Rename 'lock' spinlock to 'used_msi_lock'
> o Take lock in interrupt handler
> o Remove irq_dom in error path
> ---
>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 225 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> index 67aaadcc1c5e..b06446b23bc8 100644
> --- a/drivers/pci/host/pcie-tango.c
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -1,16 +1,228 @@
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/delay.h>
> +#include <linux/msi.h>
>  #include <linux/of.h>
>  
>  #define MSI_MAX 256
>  
>  #define SMP8759_MUX		0x48
>  #define SMP8759_TEST_OUT	0x74
> +#define SMP8759_STATUS		0x80
> +#define SMP8759_ENABLE		0xa0
> +#define SMP8759_DOORBELL	0xa002e07c
>  
>  struct tango_pcie {
> +	DECLARE_BITMAP(used_msi, MSI_MAX);
> +	spinlock_t used_msi_lock;
>  	void __iomem *mux;
> +	void __iomem *msi_status;
> +	void __iomem *msi_enable;
> +	phys_addr_t msi_doorbell;
> +	struct irq_domain *irq_dom;
> +	struct irq_domain *msi_dom;
> +	int irq;
>  };
>  
> +/*** MSI CONTROLLER SUPPORT ***/
> +
> +static void tango_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
> +	unsigned long flags, status, base, virq, idx, pos = 0;
> +
> +	chained_irq_enter(chip, desc);
> +	spin_lock_irqsave(&pcie->used_msi_lock, flags);

You're already in interrupt context, so there is no need to disable
interrupts any further. spin_lock() should do the trick

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

On 13/06/2017 16:22, Marc Zyngier wrote:
> On 13/06/17 15:01, Marc Gonzalez wrote:
>> The MSI controller in Tango supports 256 message-signaled interrupts,
>> and a single doorbell address.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>> Changes from v5 to v6
>> o Rename 'used' bitmap to 'used_msi'
>> o Rename 'lock' spinlock to 'used_msi_lock'
>> o Take lock in interrupt handler
>> o Remove irq_dom in error path
>> ---
>>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 225 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
>> index 67aaadcc1c5e..b06446b23bc8 100644
>> --- a/drivers/pci/host/pcie-tango.c
>> +++ b/drivers/pci/host/pcie-tango.c
>> @@ -1,16 +1,228 @@
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>>  #include <linux/pci-ecam.h>
>>  #include <linux/delay.h>
>> +#include <linux/msi.h>
>>  #include <linux/of.h>
>>  
>>  #define MSI_MAX 256
>>  
>>  #define SMP8759_MUX		0x48
>>  #define SMP8759_TEST_OUT	0x74
>> +#define SMP8759_STATUS		0x80
>> +#define SMP8759_ENABLE		0xa0
>> +#define SMP8759_DOORBELL	0xa002e07c
>>  
>>  struct tango_pcie {
>> +	DECLARE_BITMAP(used_msi, MSI_MAX);
>> +	spinlock_t used_msi_lock;
>>  	void __iomem *mux;
>> +	void __iomem *msi_status;
>> +	void __iomem *msi_enable;
>> +	phys_addr_t msi_doorbell;
>> +	struct irq_domain *irq_dom;
>> +	struct irq_domain *msi_dom;
>> +	int irq;
>>  };
>>  
>> +/*** MSI CONTROLLER SUPPORT ***/
>> +
>> +static void tango_msi_isr(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
>> +	unsigned long flags, status, base, virq, idx, pos = 0;
>> +
>> +	chained_irq_enter(chip, desc);
>> +	spin_lock_irqsave(&pcie->used_msi_lock, flags);
> 
> You're already in interrupt context, so there is no need to disable
> interrupts any further. spin_lock() should do the trick

Thanks for the hint.

I am confused, because Documentation/locking/spinlocks.txt states:

> If you have a case where you have to protect a data structure across
> several CPU's and you want to use spinlocks you can potentially use
> cheaper versions of the spinlocks. IFF you know that the spinlocks are
> never used in interrupt handlers, you can use the non-irq versions:
> 
> 	spin_lock(&lock);
> 	...
> 	spin_unlock(&lock);
> 
> (and the equivalent read-write versions too, of course). The spinlock will
> guarantee the same kind of exclusive access, and it will be much faster.
> This is useful if you know that the data in question is only ever
> manipulated from a "process context", ie no interrupts involved.
> 
> The reasons you mustn't use these versions if you have interrupts that
> play with the spinlock is that you can get deadlocks:
> 
> 	spin_lock(&lock);
> 	...
> 		<- interrupt comes in:
> 			spin_lock(&lock);
> 
> where an interrupt tries to lock an already locked variable. This is ok if
> the other interrupt happens on another CPU, but it is _not_ ok if the
> interrupt happens on the same CPU that already holds the lock, because the
> lock will obviously never be released (because the interrupt is waiting
> for the lock, and the lock-holder is interrupted by the interrupt and will
> not continue until the interrupt has been processed).
> 
> (This is also the reason why the irq-versions of the spinlocks only need
> to disable the _local_ interrupts - it's ok to use spinlocks in interrupts
> on other CPU's, because an interrupt on another CPU doesn't interrupt the
> CPU that holds the lock, so the lock-holder can continue and eventually
> releases the lock).

Isn't this saying that it is not safe to call spin_lock() from
the interrupt handler? (Sorry if I misunderstood.)

Regards.

Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

[Marc Zyngier]

On 13/06/17 15:47, Marc Gonzalez wrote:
> On 13/06/2017 16:22, Marc Zyngier wrote:
>> On 13/06/17 15:01, Marc Gonzalez wrote:
>>> The MSI controller in Tango supports 256 message-signaled interrupts,
>>> and a single doorbell address.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>> Changes from v5 to v6
>>> o Rename 'used' bitmap to 'used_msi'
>>> o Rename 'lock' spinlock to 'used_msi_lock'
>>> o Take lock in interrupt handler
>>> o Remove irq_dom in error path
>>> ---
>>>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 225 insertions(+)
>>>
>>> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
>>> index 67aaadcc1c5e..b06446b23bc8 100644
>>> --- a/drivers/pci/host/pcie-tango.c
>>> +++ b/drivers/pci/host/pcie-tango.c
>>> @@ -1,16 +1,228 @@
>>> +#include <linux/irqchip/chained_irq.h>
>>> +#include <linux/irqdomain.h>
>>>  #include <linux/pci-ecam.h>
>>>  #include <linux/delay.h>
>>> +#include <linux/msi.h>
>>>  #include <linux/of.h>
>>>  
>>>  #define MSI_MAX 256
>>>  
>>>  #define SMP8759_MUX		0x48
>>>  #define SMP8759_TEST_OUT	0x74
>>> +#define SMP8759_STATUS		0x80
>>> +#define SMP8759_ENABLE		0xa0
>>> +#define SMP8759_DOORBELL	0xa002e07c
>>>  
>>>  struct tango_pcie {
>>> +	DECLARE_BITMAP(used_msi, MSI_MAX);
>>> +	spinlock_t used_msi_lock;
>>>  	void __iomem *mux;
>>> +	void __iomem *msi_status;
>>> +	void __iomem *msi_enable;
>>> +	phys_addr_t msi_doorbell;
>>> +	struct irq_domain *irq_dom;
>>> +	struct irq_domain *msi_dom;
>>> +	int irq;
>>>  };
>>>  
>>> +/*** MSI CONTROLLER SUPPORT ***/
>>> +
>>> +static void tango_msi_isr(struct irq_desc *desc)
>>> +{
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
>>> +	unsigned long flags, status, base, virq, idx, pos = 0;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +	spin_lock_irqsave(&pcie->used_msi_lock, flags);
>>
>> You're already in interrupt context, so there is no need to disable
>> interrupts any further. spin_lock() should do the trick
> 
> Thanks for the hint.
> 
> I am confused, because Documentation/locking/spinlocks.txt states:
> 
>> If you have a case where you have to protect a data structure across
>> several CPU's and you want to use spinlocks you can potentially use
>> cheaper versions of the spinlocks. IFF you know that the spinlocks are
>> never used in interrupt handlers, you can use the non-irq versions:
>>
>> 	spin_lock(&lock);
>> 	...
>> 	spin_unlock(&lock);
>>
>> (and the equivalent read-write versions too, of course). The spinlock will
>> guarantee the same kind of exclusive access, and it will be much faster.
>> This is useful if you know that the data in question is only ever
>> manipulated from a "process context", ie no interrupts involved.
>>
>> The reasons you mustn't use these versions if you have interrupts that
>> play with the spinlock is that you can get deadlocks:
>>
>> 	spin_lock(&lock);
>> 	...
>> 		<- interrupt comes in:
>> 			spin_lock(&lock);
>>
>> where an interrupt tries to lock an already locked variable. This is ok if
>> the other interrupt happens on another CPU, but it is _not_ ok if the
>> interrupt happens on the same CPU that already holds the lock, because the
>> lock will obviously never be released (because the interrupt is waiting
>> for the lock, and the lock-holder is interrupted by the interrupt and will
>> not continue until the interrupt has been processed).
>>
>> (This is also the reason why the irq-versions of the spinlocks only need
>> to disable the _local_ interrupts - it's ok to use spinlocks in interrupts
>> on other CPU's, because an interrupt on another CPU doesn't interrupt the
>> CPU that holds the lock, so the lock-holder can continue and eventually
>> releases the lock).
> 
> Isn't this saying that it is not safe to call spin_lock() from
> the interrupt handler? (Sorry if I misunderstood.)

It is saying exactly the opposite.

If you take a spinlock and can be interrupted by an interrupt that takes
the same spinlock, then you must use the irq-safe version *outside of
the interrupt handler*. That's because Linux interrupts are not
preemptible (well, in general -- it is different with RT, but let's not
get there). If you're guaranteed that no interrupt handler will take
this spinlock, then you don't have to use the irq-safe version.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v7 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v6 to v7
o Call spin_lock() not spin_lock_irqsave() in the ISR
---
 drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..d4b3520b5a03 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,228 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_lock;
 	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_enable;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_dom;
+	struct irq_domain *msi_dom;
+	int irq;
 };
 
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned long status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock(&pcie->used_msi_lock);
+
+	while ((pos = find_next_bit(pcie->used_msi, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock(&pcie->used_msi_lock);
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->hwirq;
+}
+
+static struct irq_chip tango_chip = {
+	.irq_ack		= tango_ack,
+	.irq_mask		= tango_mask,
+	.irq_unmask		= tango_unmask,
+	.irq_set_affinity	= tango_set_affinity,
+	.irq_compose_msi_msg	= tango_compose_msi_msg,
+};
+
+static void msi_ack(struct irq_data *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip msi_chip = {
+	.name = "MSI",
+	.irq_ack = msi_ack,
+	.irq_mask = msi_mask,
+	.irq_unmask = msi_unmask,
+};
+
+static struct msi_domain_info msi_dom_info = {
+	.flags	= MSI_FLAG_PCI_MSIX
+		| MSI_FLAG_USE_DEF_DOM_OPS
+		| MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &msi_chip,
+};
+
+static int tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	unsigned long flags;
+	int pos, err = -ENOSPC;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	pos = find_first_zero_bit(pcie->used_msi, MSI_MAX);
+	if (pos < MSI_MAX) {
+		err = 0;
+		__set_bit(pos, pcie->used_msi);
+		irq_domain_set_info(dom, virq, pos,
+				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
+	}
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+
+	return err;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	unsigned long flags;
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	__clear_bit(d->hwirq, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static const struct irq_domain_ops irq_dom_ops = {
+	.alloc	= tango_irq_domain_alloc,
+	.free	= tango_irq_domain_free,
+};
+
+static int tango_msi_remove(struct platform_device *pdev)
+{
+	struct tango_pcie *pcie = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+	irq_domain_remove(pcie->msi_dom);
+	irq_domain_remove(pcie->irq_dom);
+
+	return 0;
+}
+
+static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
+{
+	int i, virq;
+	struct irq_domain *msi_dom, *irq_dom;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
+	spin_lock_init(&pcie->used_msi_lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
+	if (!irq_dom) {
+		pr_err("Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
+	if (!msi_dom) {
+		pr_err("Failed to create MSI domain\n");
+		irq_domain_remove(irq_dom);
+		return -ENOMEM;
+	}
+
+	pcie->irq_dom = irq_dom;
+	pcie->msi_dom = msi_dom;
+	pcie->irq = virq;
+
+	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
+
+	return 0;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +300,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +336,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = tango_msi_probe(pdev, pcie);
+	if (ret)
+		return ret;
+
 	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
 }
 
+static int tango_pcie_remove(struct platform_device *pdev)
+{
+	return tango_msi_remove(pdev);
+}
+
 static struct platform_driver tango_pcie_driver = {
 	.probe	= tango_pcie_probe,
+	.remove	= tango_pcie_remove,
 	.driver	= {
 		.name = KBUILD_MODNAME,
 		.of_match_table = tango_pcie_ids,
-- 
2.11.0

Re: [PATCH v7 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

On 14/06/2017 11:00, Marc Gonzalez wrote:

> The MSI controller in Tango supports 256 message-signaled interrupts,
> and a single doorbell address.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> Changes from v6 to v7
> o Call spin_lock() not spin_lock_irqsave() in the ISR
> ---
>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 225 insertions(+)

Someone on IRC suggested testing the driver with LOCKDEP.

If I understand the warning below correctly, I am not supposed
to call irq_domain_set_info() while holding used_msi_lock?

NB: in probe, my driver calls

	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);

This should not break LOCKDEP analysis, right?

Regards.


[    0.685615] ======================================================
[    0.685621] [ INFO: possible circular locking dependency detected ]
[    0.685632] 4.9.20-1-rc3 #2 Tainted: G          I    
[    0.685638] -------------------------------------------------------
[    0.685644] swapper/0/1 is trying to acquire lock:
[    0.685650]  (&(&pcie->used_msi_lock)->rlock){......}, at: [<c0352e5c>] update_msi_enable+0x2c/0x58

[    0.685692] but task is already holding lock:
[    0.685698]  (&irq_desc_lock_class){......}, at: [<c017586c>] __setup_irq+0xa4/0x5f0

[    0.685718] which lock already depends on the new lock.
[    0.685718] 
[    0.685725] 
[    0.685725] the existing dependency chain (in reverse order) is:
[    0.685731] 
[    0.685731] -> #1 (&irq_desc_lock_class){......}:
[    0.685758]        __irq_get_desc_lock+0x54/0x94
[    0.685768]        __irq_set_handler+0x24/0x54
[    0.685778]        irq_domain_set_info+0x38/0x4c
[    0.685786]        tango_irq_domain_alloc+0x98/0xbc
[    0.685795]        msi_domain_alloc+0x68/0x130
[    0.685802]        __irq_domain_alloc_irqs+0x11c/0x2ac
[    0.685809]        msi_domain_alloc_irqs+0x78/0x188
[    0.685816]        __pci_enable_msi_range+0x200/0x37c
[    0.685824]        pcie_port_device_register+0x3cc/0x494
[    0.685831]        pcie_portdrv_probe+0x2c/0xa4
[    0.685844]        pci_device_probe+0x8c/0xe8
[    0.685852]        driver_probe_device+0x1c8/0x2ac
[    0.685863]        bus_for_each_drv+0x60/0x94
[    0.685870]        __device_attach+0xb4/0x118
[    0.685883]        pci_bus_add_device+0x44/0x90
[    0.685891]        pci_bus_add_devices+0x3c/0x80
[    0.685898]        pci_host_common_probe+0x100/0x314
[    0.685906]        platform_drv_probe+0x50/0xb0
[    0.685912]        driver_probe_device+0x21c/0x2ac
[    0.685918]        __driver_attach+0xc0/0xc4
[    0.685926]        bus_for_each_dev+0x68/0x9c
[    0.685933]        bus_add_driver+0x108/0x214
[    0.685939]        driver_register+0x78/0xf4
[    0.685947]        do_one_initcall+0x44/0x174
[    0.685961]        kernel_init_freeable+0x158/0x1e8
[    0.685971]        kernel_init+0x8/0x10c
[    0.685980]        ret_from_fork+0x14/0x24
[    0.685985] 
[    0.685985] -> #0 (&(&pcie->used_msi_lock)->rlock){......}:
[    0.686003]        _raw_spin_lock_irqsave+0x44/0x58
[    0.686012]        update_msi_enable+0x2c/0x58
[    0.686019]        irq_enable+0x30/0x44
[    0.686025]        irq_startup+0x80/0x84
[    0.686031]        __setup_irq+0x558/0x5f0
[    0.686038]        request_threaded_irq+0xe4/0x184
[    0.686044]        pcie_pme_probe+0xc4/0x1f0
[    0.686051]        pcie_port_probe_service+0x34/0x70
[    0.686057]        driver_probe_device+0x21c/0x2ac
[    0.686065]        bus_for_each_drv+0x60/0x94
[    0.686071]        __device_attach+0xb4/0x118
[    0.686079]        bus_probe_device+0x88/0x90
[    0.686085]        device_add+0x3f4/0x584
[    0.686092]        pcie_port_device_register+0x228/0x494
[    0.686099]        pcie_portdrv_probe+0x2c/0xa4
[    0.686106]        pci_device_probe+0x8c/0xe8
[    0.686113]        driver_probe_device+0x1c8/0x2ac
[    0.686120]        bus_for_each_drv+0x60/0x94
[    0.686126]        __device_attach+0xb4/0x118
[    0.686134]        pci_bus_add_device+0x44/0x90
[    0.686141]        pci_bus_add_devices+0x3c/0x80
[    0.686149]        pci_host_common_probe+0x100/0x314
[    0.686155]        platform_drv_probe+0x50/0xb0
[    0.686161]        driver_probe_device+0x21c/0x2ac
[    0.686167]        __driver_attach+0xc0/0xc4
[    0.686175]        bus_for_each_dev+0x68/0x9c
[    0.686182]        bus_add_driver+0x108/0x214
[    0.686188]        driver_register+0x78/0xf4
[    0.686194]        do_one_initcall+0x44/0x174
[    0.686203]        kernel_init_freeable+0x158/0x1e8
[    0.686210]        kernel_init+0x8/0x10c
[    0.686218]        ret_from_fork+0x14/0x24
[    0.686222] 
[    0.686222] other info that might help us debug this:
[    0.686222] 
[    0.686229]  Possible unsafe locking scenario:
[    0.686229] 
[    0.686235]        CPU0                    CPU1
[    0.686239]        ----                    ----
[    0.686243]   lock(&irq_desc_lock_class);
[    0.686251]                                lock(&(&pcie->used_msi_lock)->rlock);
[    0.686260]                                lock(&irq_desc_lock_class);
[    0.686267]   lock(&(&pcie->used_msi_lock)->rlock);
[    0.686275] 
[    0.686275]  *** DEADLOCK ***
[    0.686275] 
[    0.686283] 5 locks held by swapper/0/1:
[    0.686288]  #0:  (&dev->mutex){......}, at: [<c03939e4>] __driver_attach+0x50/0xc4
[    0.686303]  #1:  (&dev->mutex){......}, at: [<c03939f4>] __driver_attach+0x60/0xc4
[    0.686318]  #2:  (&dev->mutex){......}, at: [<c0393574>] __device_attach+0x20/0x118
[    0.686333]  #3:  (&dev->mutex){......}, at: [<c0393574>] __device_attach+0x20/0x118
[    0.686348]  #4:  (&irq_desc_lock_class){......}, at: [<c017586c>] __setup_irq+0xa4/0x5f0
[    0.686363] 
[    0.686363] stack backtrace:
[    0.686373] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G          I     4.9.20-1-rc3 #2
[    0.686379] Hardware name: Sigma Tango DT
[    0.686398] [<c010f8a0>] (unwind_backtrace) from [<c010b540>] (show_stack+0x10/0x14)
[    0.686411] [<c010b540>] (show_stack) from [<c0309f40>] (dump_stack+0x98/0xc4)
[    0.686423] [<c0309f40>] (dump_stack) from [<c0165ce4>] (print_circular_bug+0x214/0x334)
[    0.686432] [<c0165ce4>] (print_circular_bug) from [<c016934c>] (__lock_acquire+0x171c/0x1a28)
[    0.686441] [<c016934c>] (__lock_acquire) from [<c0169a28>] (lock_acquire+0x6c/0x88)
[    0.686452] [<c0169a28>] (lock_acquire) from [<c0533004>] (_raw_spin_lock_irqsave+0x44/0x58)
[    0.686464] [<c0533004>] (_raw_spin_lock_irqsave) from [<c0352e5c>] (update_msi_enable+0x2c/0x58)
[    0.686475] [<c0352e5c>] (update_msi_enable) from [<c0177614>] (irq_enable+0x30/0x44)
[    0.686484] [<c0177614>] (irq_enable) from [<c01776a8>] (irq_startup+0x80/0x84)
[    0.686493] [<c01776a8>] (irq_startup) from [<c0175d20>] (__setup_irq+0x558/0x5f0)
[    0.686502] [<c0175d20>] (__setup_irq) from [<c0175f60>] (request_threaded_irq+0xe4/0x184)
[    0.686511] [<c0175f60>] (request_threaded_irq) from [<c034fed0>] (pcie_pme_probe+0xc4/0x1f0)
[    0.686520] [<c034fed0>] (pcie_pme_probe) from [<c034d8a8>] (pcie_port_probe_service+0x34/0x70)
[    0.686530] [<c034d8a8>] (pcie_port_probe_service) from [<c0393904>] (driver_probe_device+0x21c/0x2ac)
[    0.686540] [<c0393904>] (driver_probe_device) from [<c0391d18>] (bus_for_each_drv+0x60/0x94)
[    0.686550] [<c0391d18>] (bus_for_each_drv) from [<c0393608>] (__device_attach+0xb4/0x118)
[    0.686560] [<c0393608>] (__device_attach) from [<c0392b14>] (bus_probe_device+0x88/0x90)
[    0.686570] [<c0392b14>] (bus_probe_device) from [<c0390ef0>] (device_add+0x3f4/0x584)
[    0.686580] [<c0390ef0>] (device_add) from [<c034dba4>] (pcie_port_device_register+0x228/0x494)
[    0.686589] [<c034dba4>] (pcie_port_device_register) from [<c034e26c>] (pcie_portdrv_probe+0x2c/0xa4)
[    0.686600] [<c034e26c>] (pcie_portdrv_probe) from [<c0341ff0>] (pci_device_probe+0x8c/0xe8)
[    0.686611] [<c0341ff0>] (pci_device_probe) from [<c03938b0>] (driver_probe_device+0x1c8/0x2ac)
[    0.686620] [<c03938b0>] (driver_probe_device) from [<c0391d18>] (bus_for_each_drv+0x60/0x94)
[    0.686630] [<c0391d18>] (bus_for_each_drv) from [<c0393608>] (__device_attach+0xb4/0x118)
[    0.686641] [<c0393608>] (__device_attach) from [<c03381fc>] (pci_bus_add_device+0x44/0x90)
[    0.686651] [<c03381fc>] (pci_bus_add_device) from [<c0338284>] (pci_bus_add_devices+0x3c/0x80)
[    0.686662] [<c0338284>] (pci_bus_add_devices) from [<c0352bf4>] (pci_host_common_probe+0x100/0x314)
[    0.686673] [<c0352bf4>] (pci_host_common_probe) from [<c03950dc>] (platform_drv_probe+0x50/0xb0)
[    0.686682] [<c03950dc>] (platform_drv_probe) from [<c0393904>] (driver_probe_device+0x21c/0x2ac)
[    0.686690] [<c0393904>] (driver_probe_device) from [<c0393a54>] (__driver_attach+0xc0/0xc4)
[    0.686700] [<c0393a54>] (__driver_attach) from [<c0391c70>] (bus_for_each_dev+0x68/0x9c)
[    0.686711] [<c0391c70>] (bus_for_each_dev) from [<c0392d2c>] (bus_add_driver+0x108/0x214)
[    0.686721] [<c0392d2c>] (bus_add_driver) from [<c0394170>] (driver_register+0x78/0xf4)
[    0.686730] [<c0394170>] (driver_register) from [<c01017dc>] (do_one_initcall+0x44/0x174)
[    0.686742] [<c01017dc>] (do_one_initcall) from [<c0800dc0>] (kernel_init_freeable+0x158/0x1e8)
[    0.686754] [<c0800dc0>] (kernel_init_freeable) from [<c052c918>] (kernel_init+0x8/0x10c)
[    0.686765] [<c052c918>] (kernel_init) from [<c01077d0>] (ret_from_fork+0x14/0x24)

[    0.686824] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
[    0.686835] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
[    0.686849] pcie_pme 0000:00:00.0:pcie001: service driver pcie_pme loaded
[    0.687049] aer 0000:00:00.0:pcie002: service driver aer loaded
[    0.687276] pci 0000:01:00.0: enabling device (0140 -> 0142)

Re: [PATCH v7 3/3] PCI: Add tango MSI controller support

[Marc Zyngier]

On 14/06/17 10:19, Marc Gonzalez wrote:
> On 14/06/2017 11:00, Marc Gonzalez wrote:
> 
>> The MSI controller in Tango supports 256 message-signaled interrupts,
>> and a single doorbell address.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>> Changes from v6 to v7
>> o Call spin_lock() not spin_lock_irqsave() in the ISR
>> ---
>>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 225 insertions(+)
> 
> Someone on IRC suggested testing the driver with LOCKDEP.
> 
> If I understand the warning below correctly, I am not supposed
> to call irq_domain_set_info() while holding used_msi_lock?

Indeed. This creates an AB/BA situation, which will eventually deadlock.
Once again, lockdep saves the day.

> NB: in probe, my driver calls
> 
> 	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> 
> This should not break LOCKDEP analysis, right?

It doesn't. The code is provably wrong, and lockdep proved that it is wrong.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v8 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v7 to v8
o Reorganize tango_irq_domain_alloc() so as not to call irq_domain_set_info()
  with pcie->used_msi_lock held => Bug diagnosed by LOCKDEP
---
 drivers/pci/host/pcie-tango.c | 226 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 226 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..5c47a4cc03e3 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,229 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_lock;
 	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_enable;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_dom;
+	struct irq_domain *msi_dom;
+	int irq;
 };
 
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned long status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock(&pcie->used_msi_lock);
+
+	while ((pos = find_next_bit(pcie->used_msi, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock(&pcie->used_msi_lock);
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->hwirq;
+}
+
+static struct irq_chip tango_chip = {
+	.irq_ack		= tango_ack,
+	.irq_mask		= tango_mask,
+	.irq_unmask		= tango_unmask,
+	.irq_set_affinity	= tango_set_affinity,
+	.irq_compose_msi_msg	= tango_compose_msi_msg,
+};
+
+static void msi_ack(struct irq_data *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip msi_chip = {
+	.name = "MSI",
+	.irq_ack = msi_ack,
+	.irq_mask = msi_mask,
+	.irq_unmask = msi_unmask,
+};
+
+static struct msi_domain_info msi_dom_info = {
+	.flags	= MSI_FLAG_PCI_MSIX
+		| MSI_FLAG_USE_DEF_DOM_OPS
+		| MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &msi_chip,
+};
+
+static int tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	int pos;
+	unsigned long flags;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	pos = find_first_zero_bit(pcie->used_msi, MSI_MAX);
+	if (pos >= MSI_MAX) {
+		spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+		return -ENOSPC;
+	}
+	__set_bit(pos, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+	irq_domain_set_info(dom, virq, pos, &tango_chip,
+			pcie, handle_edge_irq, NULL, NULL);
+
+	return 0;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	unsigned long flags;
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	__clear_bit(d->hwirq, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static const struct irq_domain_ops irq_dom_ops = {
+	.alloc	= tango_irq_domain_alloc,
+	.free	= tango_irq_domain_free,
+};
+
+static int tango_msi_remove(struct platform_device *pdev)
+{
+	struct tango_pcie *pcie = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+	irq_domain_remove(pcie->msi_dom);
+	irq_domain_remove(pcie->irq_dom);
+
+	return 0;
+}
+
+static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
+{
+	int i, virq;
+	struct irq_domain *msi_dom, *irq_dom;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
+	spin_lock_init(&pcie->used_msi_lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
+	if (!irq_dom) {
+		pr_err("Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
+	if (!msi_dom) {
+		pr_err("Failed to create MSI domain\n");
+		irq_domain_remove(irq_dom);
+		return -ENOMEM;
+	}
+
+	pcie->irq_dom = irq_dom;
+	pcie->msi_dom = msi_dom;
+	pcie->irq = virq;
+
+	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
+
+	return 0;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +301,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +337,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = tango_msi_probe(pdev, pcie);
+	if (ret)
+		return ret;
+
 	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
 }
 
+static int tango_pcie_remove(struct platform_device *pdev)
+{
+	return tango_msi_remove(pdev);
+}
+
 static struct platform_driver tango_pcie_driver = {
 	.probe	= tango_pcie_probe,
+	.remove	= tango_pcie_remove,
 	.driver	= {
 		.name = KBUILD_MODNAME,
 		.of_match_table = tango_pcie_ids,
-- 
2.11.0