[PATCH v2 0/3] pci: add support for firmware initialized designware RCs

[PATCH v2 0/3] pci: add support for firmware initialized designware RCs

[Ard Biesheuvel]

UEFI based systems incorporating a Synopsys Designware PCIe controller
in RC mode will typically configure it before entering the OS. If this
configuration is fully static and ECAM compliant, there is no need to
expose particulars of the device to the OS, and we can simply describe
it as "pci-host-ecam-generic".

However, the Synopsys IP may be synthesized in a way where a quirk is
needed for config space accesses to the first bus. It makes little sense
to instantiate yet another pcie-designware driver that contains all the
low level setup code, but it is also not justified to add quirks handling
to the generic ECAM driver.

So instead, create a variant of the generic ECAM driver that filters config
space accesses directed at device #1 and up on the first bus.

Also, add a binding and driver to support the MSI functionality available
in some versions of this IP. This allows the MSI routing to be described
at the DT level rather than hardcoding it in the driver.

v2: - use dev->fwnode directly
    - replace an instance of pr_err with dev_err, and clarify the error message
    - fix Kconfig/Makefile dependency errors reported by kbuild

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Graeme Gregory <graeme.gregory@linaro.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>

Ard Biesheuvel (3):
  pci: designware: add driver for DWC controller in ECAM shift mode
  pci: designware: add separate driver for the MSI part of the RC
  dt-bindings: designware: add binding for Designware PCIe in ECAM mode

 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt |  56 +++++
 drivers/pci/dwc/Kconfig                                        |  11 +
 drivers/pci/dwc/Makefile                                       |   4 +-
 drivers/pci/dwc/pcie-designware-ecam.c                         |  75 ++++++
 drivers/pci/dwc/pcie-designware-msi.c                          | 255 ++++++++++++++++++++
 5 files changed, 400 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
 create mode 100644 drivers/pci/dwc/pcie-designware-ecam.c
 create mode 100644 drivers/pci/dwc/pcie-designware-msi.c

-- 
2.11.0

[PATCH v2 1/3] pci: designware: add driver for DWC controller in ECAM shift mode

[Ard Biesheuvel]

Some implementations of the Synopsys Designware PCIe controller implement
a so-called ECAM shift mode, which allows a static memory window to be
configured that covers the configuration space of the entire bus range.

If the firmware performs all the low level configuration that is required
to expose this controller in a fully ECAM compatible manner, we can
simply describe it as "pci-host-ecam-generic" and be done with it.
However, it appears that in some cases (one of which is the Armada 80x0),
the IP is synthesized with an ATU window size that does not allow the
first bus to be mapped in a way that prevents the device on the
downstream port from appearing more than once.

So implement a driver that relies on the firmware to perform all low
level initialization, and drives the controller in ECAM mode, but
overrides the config space accessors to take the above quirk into
account.

Note that, unlike most drivers for this IP, this driver does not expose
a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
given that this is not a true bridge, and does not require any windows
to be configured in order for the downstream device to operate correctly.
Omitting it also prevents the PCI resource allocation routines from
handing out BAR space to it unnecessarily.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/pci/dwc/Kconfig                | 11 +++
 drivers/pci/dwc/Makefile               |  1 +
 drivers/pci/dwc/pcie-designware-ecam.c | 75 ++++++++++++++++++++
 3 files changed, 87 insertions(+)

diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
index d275aadc47ee..477576d07911 100644
--- a/drivers/pci/dwc/Kconfig
+++ b/drivers/pci/dwc/Kconfig
@@ -169,4 +169,15 @@ config PCIE_KIRIN
 	  Say Y here if you want PCIe controller support
 	  on HiSilicon Kirin series SoCs.
 
+config PCIE_DW_HOST_ECAM
+	bool "Synopsys DesignWare PCIe controller in ECAM mode"
+	depends on OF && PCI
+	select PCI_HOST_COMMON
+	select IRQ_DOMAIN
+	help
+	  Add support for Synopsys DesignWare PCIe controllers configured
+	  by the firmware into ECAM shift mode. In some cases, these are
+	  fully ECAM compliant, in which case the pci-host-generic driver
+	  may be used instead.
+
 endmenu
diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
index c61be9738cce..7d5a23e5b767 100644
--- a/drivers/pci/dwc/Makefile
+++ b/drivers/pci/dwc/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_PCIE_DW) += pcie-designware.o
 obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
+obj-$(CONFIG_PCIE_DW_HOST_ECAM) += pcie-designware-ecam.o
 obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
 ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
diff --git a/drivers/pci/dwc/pcie-designware-ecam.c b/drivers/pci/dwc/pcie-designware-ecam.c
new file mode 100644
index 000000000000..0f495bd2e33e
--- /dev/null
+++ b/drivers/pci/dwc/pcie-designware-ecam.c
@@ -0,0 +1,75 @@
+/*
+ * Driver for mostly ECAM compatible Synopsys dw PCIe controllers
+ * configured by the firmware into RC mode
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Copyright (C) 2014 ARM Limited
+ * Copyright (C) 2017 Linaro Limited
+ *
+ * Authors: Will Deacon <will.deacon@arm.com>
+ *          Ard Biesheuvel <ard.biesheuvel@linaro.org>
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/pci-ecam.h>
+#include <linux/platform_device.h>
+
+static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
+				   int size, u32 *val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	/*
+	 * The Synopsys dw PCIe controller in RC mode will not filter type 0
+	 * config TLPs sent to devices 1 and up on its downstream port,
+	 * resulting in devices appearing multiple times on bus 0 unless we
+	 * filter them here.
+	 */
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) {
+		*val = 0xffffffff;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+	return pci_generic_config_read(bus, devfn, where, size, val);
+}
+
+static int pci_dw_ecam_config_write(struct pci_bus *bus, u32 devfn, int where,
+				    int size, u32 val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return pci_generic_config_write(bus, devfn, where, size, val);
+}
+
+static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
+	.pci_ops.map_bus		= pci_ecam_map_bus,
+	.pci_ops.read			= pci_dw_ecam_config_read,
+	.pci_ops.write			= pci_dw_ecam_config_write,
+	.bus_shift			= 20,
+};
+
+static const struct of_device_id pci_dw_ecam_of_match[] = {
+	{ .compatible = "snps,dw-pcie-ecam" },
+	{ },
+};
+
+static int pci_dw_ecam_probe(struct platform_device *pdev)
+{
+	return pci_host_common_probe(pdev, &pci_dw_ecam_bus_ops);
+}
+
+static struct platform_driver pci_dw_ecam_driver = {
+	.driver.name			= "pcie-designware-ecam",
+	.driver.of_match_table		= pci_dw_ecam_of_match,
+	.driver.suppress_bind_attrs	= true,
+	.probe				= pci_dw_ecam_probe,
+};
+builtin_platform_driver(pci_dw_ecam_driver);
-- 
2.11.0

[PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC

[Ard Biesheuvel]

Most drivers that currently exist for the Synopsys Designware PCIe
controller in RC mode hardcode the relation with the embedded MSI
controller. This makes it more difficult than necessary to use a
generic driver to drive the RC, which is especially unfortunate
in cases where the firmware already configures the RC to the extent
that it can be driven by the generic ECAM driver. It also makes it
impossible to use an existing driver but use another IP block for
MSI support, i.e., a GICv2m or GICv3-ITS.

So add a separate driver for the MSI part, which can be referenced
from the DT node describing the RC via its msi-parent property.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/pci/dwc/Makefile              |   3 +-
 drivers/pci/dwc/pcie-designware-msi.c | 255 ++++++++++++++++++++
 2 files changed, 257 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
index 7d5a23e5b767..7607a2b686a0 100644
--- a/drivers/pci/dwc/Makefile
+++ b/drivers/pci/dwc/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_PCIE_DW) += pcie-designware.o
-obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
+msi-obj-$(CONFIG_PCI_MSI_IRQ_DOMAIN) += pcie-designware-msi.o
+obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o $(msi-obj-y)
 obj-$(CONFIG_PCIE_DW_HOST_ECAM) += pcie-designware-ecam.o
 obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
diff --git a/drivers/pci/dwc/pcie-designware-msi.c b/drivers/pci/dwc/pcie-designware-msi.c
new file mode 100644
index 000000000000..16b02151177d
--- /dev/null
+++ b/drivers/pci/dwc/pcie-designware-msi.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2017 Linaro Limited <ard.biesheuvel@linaro.org>
+ *
+ * Based on code posted for the tango platform by
+ *                            Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+
+#include "pcie-designware.h"
+
+struct dw_pcie_msi {
+	void __iomem		*regbase;
+	int			irq;
+	struct irq_domain	*irqd;
+	struct irq_domain	*msid;
+	DECLARE_BITMAP(		used_msi, MAX_MSI_IRQS);
+	spinlock_t		used_msi_lock;
+	u32			doorbell;
+};
+
+static void dw_pcie_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct dw_pcie_msi *dw_msi = irq_desc_get_handler_data(desc);
+	unsigned long status, base, virq, idx, pos;
+
+	chained_irq_enter(chip, desc);
+	spin_lock(&dw_msi->used_msi_lock);
+
+	for (pos = 0; pos < MAX_MSI_IRQS;
+	     pos = find_next_bit(dw_msi->used_msi, MAX_MSI_IRQS, pos)) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(dw_msi->regbase + PCIE_MSI_INTR0_STATUS +
+				       (base / 32) * 12);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(dw_msi->irqd, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock(&dw_msi->used_msi_lock);
+	chained_irq_exit(chip, desc);
+}
+
+static void dw_pcie_ack(struct irq_data *d)
+{
+	struct dw_pcie_msi *dw_msi = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 12;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, dw_msi->regbase + PCIE_MSI_INTR0_STATUS + offset);
+}
+
+static void dw_pcie_update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct dw_pcie_msi *dw_msi = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 12;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&dw_msi->used_msi_lock, flags);
+	val = readl_relaxed(dw_msi->regbase + PCIE_MSI_INTR0_ENABLE + offset);
+	val = unmask ? (val | bit) : (val & ~bit);
+	writel_relaxed(val, dw_msi->regbase + PCIE_MSI_INTR0_ENABLE + offset);
+	spin_unlock_irqrestore(&dw_msi->used_msi_lock, flags);
+}
+
+static void dw_pcie_mask(struct irq_data *d)
+{
+	dw_pcie_update_msi_enable(d, false);
+}
+
+static void dw_pcie_unmask(struct irq_data *d)
+{
+	dw_pcie_update_msi_enable(d, true);
+}
+
+static int dw_pcie_set_affinity(struct irq_data *d,
+				    const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void dw_pcie_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct dw_pcie_msi *dw_msi = d->chip_data;
+
+	msg->address_lo = lower_32_bits(virt_to_phys(&dw_msi->doorbell));
+	msg->address_hi = upper_32_bits(virt_to_phys(&dw_msi->doorbell));
+	msg->data = d->hwirq;
+}
+
+static struct irq_chip dw_pcie_chip = {
+	.irq_ack		= dw_pcie_ack,
+	.irq_mask		= dw_pcie_mask,
+	.irq_unmask		= dw_pcie_unmask,
+	.irq_set_affinity	= dw_pcie_set_affinity,
+	.irq_compose_msi_msg	= dw_pcie_compose_msi_msg,
+};
+
+static void dw_pcie_msi_ack(struct irq_data *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void dw_pcie_msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void dw_pcie_msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip dw_pcie_msi_chip = {
+	.name			= "DW-MSI",
+	.irq_ack		= dw_pcie_msi_ack,
+	.irq_mask		= dw_pcie_msi_mask,
+	.irq_unmask		= dw_pcie_msi_unmask,
+};
+
+static struct msi_domain_info dw_pcie_msi_dom_info = {
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &dw_pcie_msi_chip,
+};
+
+static int dw_pcie_msi_irq_domain_alloc(struct irq_domain *dom,
+					unsigned int virq,
+					unsigned int nr_irqs, void *args)
+{
+	struct dw_pcie_msi *dw_msi = dom->host_data;
+	unsigned long flags;
+	int pos;
+
+	spin_lock_irqsave(&dw_msi->used_msi_lock, flags);
+	pos = find_first_zero_bit(dw_msi->used_msi, MAX_MSI_IRQS);
+	if (pos >= MAX_MSI_IRQS) {
+		spin_unlock_irqrestore(&dw_msi->used_msi_lock, flags);
+		return -ENOSPC;
+	}
+	__set_bit(pos, dw_msi->used_msi);
+	spin_unlock_irqrestore(&dw_msi->used_msi_lock, flags);
+	irq_domain_set_info(dom, virq, pos, &dw_pcie_chip, dw_msi,
+			    handle_edge_irq, NULL, NULL);
+
+	return 0;
+}
+
+static void dw_pcie_msi_irq_domain_free(struct irq_domain *dom,
+					unsigned int virq,
+					unsigned int nr_irqs)
+{
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct dw_pcie_msi *dw_msi = d->chip_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dw_msi->used_msi_lock, flags);
+	__clear_bit(d->hwirq, dw_msi->used_msi);
+	spin_unlock_irqrestore(&dw_msi->used_msi_lock, flags);
+}
+
+static const struct irq_domain_ops irq_dom_ops = {
+	.alloc	= dw_pcie_msi_irq_domain_alloc,
+	.free	= dw_pcie_msi_irq_domain_free,
+};
+
+static int dw_pcie_msi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie_msi *dw_msi;
+	struct resource *res;
+
+	dw_msi = devm_kzalloc(dev, sizeof(*dw_msi), GFP_KERNEL);
+	if (!dw_msi)
+		return -ENOMEM;
+
+	/* get the control register and map it */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dw_msi->regbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(dw_msi->regbase))
+		return PTR_ERR(dw_msi->regbase);
+
+	/* get the wired interrupt that gets raised when we receive an MSI */
+	dw_msi->irq = platform_get_irq(pdev, 0);
+	if (dw_msi->irq <= 0) {
+		dev_err(dev, "No IRQ resource found\n");
+		return -ENXIO;
+	}
+
+	dw_msi->irqd = irq_domain_create_linear(dev->fwnode, MAX_MSI_IRQS,
+						&irq_dom_ops, dw_msi);
+	if (!dw_msi->irqd) {
+		dev_err(dev, "Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	dw_msi->msid = pci_msi_create_irq_domain(dev->fwnode,
+						 &dw_pcie_msi_dom_info,
+						 dw_msi->irqd);
+	if (!dw_msi->msid) {
+		dev_err(dev, "Failed to create MSI domain\n");
+		irq_domain_remove(dw_msi->irqd);
+		return -ENOMEM;
+	}
+
+	irq_set_chained_handler_and_data(dw_msi->irq, dw_pcie_msi_isr, dw_msi);
+	platform_set_drvdata(pdev, dw_msi);
+
+	/* program the msi_data */
+	writel_relaxed(lower_32_bits(virt_to_phys(&dw_msi->doorbell)),
+		       dw_msi->regbase + PCIE_MSI_ADDR_LO);
+	writel_relaxed(upper_32_bits(virt_to_phys(&dw_msi->doorbell)),
+		       dw_msi->regbase + PCIE_MSI_ADDR_HI);
+
+	return 0;
+}
+
+static int dw_pcie_msi_remove(struct platform_device *pdev)
+{
+	struct dw_pcie_msi *dw_msi = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(dw_msi->irq, NULL, NULL);
+	irq_domain_remove(dw_msi->msid);
+	irq_domain_remove(dw_msi->irqd);
+
+	return 0;
+}
+
+static const struct of_device_id dw_pcie_dw_msi_of_match[] = {
+	{ .compatible = "snps,dw-pcie-msi" },
+	{ },
+};
+
+static struct platform_driver pci_dw_msi_driver = {
+	.driver.name			= "pcie-designware-msi",
+	.driver.of_match_table		= dw_pcie_dw_msi_of_match,
+	.probe				= dw_pcie_msi_probe,
+	.remove				= dw_pcie_msi_remove,
+};
+builtin_platform_driver(pci_dw_msi_driver);
-- 
2.11.0

[PATCH v2 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Ard Biesheuvel]

Describe the binding for firmware-configured instances of the Synopsys
Designware PCIe controller in RC mode.

Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
new file mode 100644
index 000000000000..b8127b19c220
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
@@ -0,0 +1,56 @@
+* Synopsys Designware PCIe root complex in ECAM mode
+
+In some cases, firmware may already have configured the Synopsys Designware
+PCIe controller in RC mode with static ATU window mappings that cover all
+config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
+In this case, there is no need for the OS to perform any low level setup
+of clocks or device registers, nor is there any reason for the driver to
+reconfigure ATU windows for config and/or IO space accesses at runtime.
+
+Such hardware configurations should be described as "pci-host-ecam-generic"
+if they are truly ECAM compatible. Configurations that require no low-level
+setup by the OS nor any ATU window reconfiguration at runtime, but do
+require special handling for type 0 config TLPs may instead be described as
+"snps,dw-pcie-ecam".
+
+Required properties:
+- compatible: should contain "snps,dw-pcie-ecam".
+
+Please refer to the binding document of "pci-host-ecam-generic" in the
+file host-generic-pci.txt for a description of the remaining required
+and optional properties.
+
+
+* MSI support for Synopsys Designware PCIe root complex in ECAM mode
+
+Platforms that elect to perform all configuration of the RC in firmware
+and use the "pci-host-ecam-generic" or "snps,dw-pcie-ecam" binding to
+describe it to the OS may include a separate description of the embedded
+MSI controller in case no MSI support is available in the core interrupt
+controller.
+
+Required properties:
+- compatible:      should contain "snps,dw-pcie-msi".
+- reg:             a single region describing the device registers.
+- interrupts:      interrupt specifier for the interrupt that is asserted when
+                   an MSI is received by the RC.
+- msi-controller:  empty property identifying this device as an MSI controller.
+
+Example for an implementation that routes all legacy INTx interrupts via SPI
+#188 and all MSI interrupts via SPI #190:
+
+  pcie@20000000 {
+    compatible = "snps,dw-pcie-ecam";
+    device_type = "pci";
+    msi-parent = <&msi0>;
+    interrupt-map-mask = <0x0 0x0 0x0 0x0>;
+    interrupt-map = <0x0 0x0 0x0 0x0 &gic GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+    ...
+  };
+
+  msi0: msi@10000000 {
+    compatible = "snps,dw-pcie-msi";
+    reg = <0x0 0x10000000 0x0 0x10000>;
+    interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>;
+    msi-controller;
+  };
-- 
2.11.0

Re: [PATCH v2 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Rob Herring]

Cc the DT list for bindings please.

On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> Describe the binding for firmware-configured instances of the Synopsys
> Designware PCIe controller in RC mode.
>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
> new file mode 100644
> index 000000000000..b8127b19c220
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
> @@ -0,0 +1,56 @@
> +* Synopsys Designware PCIe root complex in ECAM mode
> +
> +In some cases, firmware may already have configured the Synopsys Designware
> +PCIe controller in RC mode with static ATU window mappings that cover all
> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
> +In this case, there is no need for the OS to perform any low level setup
> +of clocks or device registers, nor is there any reason for the driver to
> +reconfigure ATU windows for config and/or IO space accesses at runtime.
> +
> +Such hardware configurations should be described as "pci-host-ecam-generic"
> +if they are truly ECAM compatible. Configurations that require no low-level
> +setup by the OS nor any ATU window reconfiguration at runtime, but do
> +require special handling for type 0 config TLPs may instead be described as
> +"snps,dw-pcie-ecam".

Humm, what happens when we have the next exception that's SoC specific
or another vendor? Seems like perhaps "firmware initialized" should
have been a separate property flag for bootloaders to add rather than
a compatible string.

I'd rather see this done in a way that does not require DT updates if
quirks have to be handled/added later.

Rob

Re: [PATCH v2 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Ard Biesheuvel]

On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote:
> Cc the DT list for bindings please.
>
> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> Describe the binding for firmware-configured instances of the Synopsys
>> Designware PCIe controller in RC mode.
>>
>> Cc: Rob Herring <robh@kernel.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>> new file mode 100644
>> index 000000000000..b8127b19c220
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>> @@ -0,0 +1,56 @@
>> +* Synopsys Designware PCIe root complex in ECAM mode
>> +
>> +In some cases, firmware may already have configured the Synopsys Designware
>> +PCIe controller in RC mode with static ATU window mappings that cover all
>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
>> +In this case, there is no need for the OS to perform any low level setup
>> +of clocks or device registers, nor is there any reason for the driver to
>> +reconfigure ATU windows for config and/or IO space accesses at runtime.
>> +
>> +Such hardware configurations should be described as "pci-host-ecam-generic"
>> +if they are truly ECAM compatible. Configurations that require no low-level
>> +setup by the OS nor any ATU window reconfiguration at runtime, but do
>> +require special handling for type 0 config TLPs may instead be described as
>> +"snps,dw-pcie-ecam".
>
> Humm, what happens when we have the next exception that's SoC specific
> or another vendor?

This is not SoC specific, but IP specific. We are working with two
different SoCs from completely different vendors that both synthesized
this IP with a 64 KB ATU window size, not expecting this to break ECAM
compatibility.

> Seems like perhaps "firmware initialized" should
> have been a separate property flag for bootloaders to add rather than
> a compatible string.
>

Yes, but then you still have 10 different drivers that all retain the
low-level bits that are all different between SoCs. That is exactly
what I want to get rid of, and usually we can do that with existing
bindings, because we simply expose it as pci-host-ecam-generic. Only
in this particular case, that doesn't fly due to the quirk.

> I'd rather see this done in a way that does not require DT updates if
> quirks have to be handled/added later.
>

Do you see a way that still allows us to keep the abstraction? I don't
want a flag, I simply don't want to expose any low-level specifics
about the device to the OS, beyond what it needs to use it in its
configured state.

Re: [PATCH v2 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Rob Herring]

On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote:
>> Cc the DT list for bindings please.
>>
>> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> Describe the binding for firmware-configured instances of the Synopsys
>>> Designware PCIe controller in RC mode.
>>>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>>> new file mode 100644
>>> index 000000000000..b8127b19c220
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>>> @@ -0,0 +1,56 @@
>>> +* Synopsys Designware PCIe root complex in ECAM mode
>>> +
>>> +In some cases, firmware may already have configured the Synopsys Designware
>>> +PCIe controller in RC mode with static ATU window mappings that cover all
>>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
>>> +In this case, there is no need for the OS to perform any low level setup
>>> +of clocks or device registers, nor is there any reason for the driver to
>>> +reconfigure ATU windows for config and/or IO space accesses at runtime.
>>> +
>>> +Such hardware configurations should be described as "pci-host-ecam-generic"
>>> +if they are truly ECAM compatible. Configurations that require no low-level
>>> +setup by the OS nor any ATU window reconfiguration at runtime, but do
>>> +require special handling for type 0 config TLPs may instead be described as
>>> +"snps,dw-pcie-ecam".
>>
>> Humm, what happens when we have the next exception that's SoC specific
>> or another vendor?
>
> This is not SoC specific, but IP specific. We are working with two
> different SoCs from completely different vendors that both synthesized
> this IP with a 64 KB ATU window size, not expecting this to break ECAM
> compatibility.

This case is not SoC specific, but quirks/bugs show up in both places.
Plus "dw-pcie" is fairly meaningless because the IP is both
configurable and has multiple releases.

>> Seems like perhaps "firmware initialized" should
>> have been a separate property flag for bootloaders to add rather than
>> a compatible string.
>>
>
> Yes, but then you still have 10 different drivers that all retain the
> low-level bits that are all different between SoCs. That is exactly
> what I want to get rid of, and usually we can do that with existing
> bindings, because we simply expose it as pci-host-ecam-generic. Only
> in this particular case, that doesn't fly due to the quirk.

You can expose it as "vendor,soc-pcie", "pci-host-ecam-generic" and
then the kernel can decide. Then you can use: the default ecam driver,
the ecam driver but match on "vendor,soc-pcie" for quirks, or a vendor
specific driver. The only thing that wouldn't work would be having
both quirks in the ecam driver and a vendor driver which IMO we just
shouldn't support anyway.

Now if you have 10 SoCs all needing this same quirk, then maybe adding
"snps,dw-pcie-ecam" addtionally to the above makes sense. But that
only saves you match strings.

>> I'd rather see this done in a way that does not require DT updates if
>> quirks have to be handled/added later.
>>
>
> Do you see a way that still allows us to keep the abstraction? I don't
> want a flag, I simply don't want to expose any low-level specifics
> about the device to the OS, beyond what it needs to use it in its
> configured state.

That's not how DT works. Detailed information is exposed to the OS and
the OS decides if it can handle things generically or not because that
decision can change over time. Whenever we don't make things specific
enough, we've gotten burned.

Rob

Re: [PATCH v2 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Ard Biesheuvel]

On 24 August 2017 at 23:12, Rob Herring <robh@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote:
>>> Cc the DT list for bindings please.
>>>
>>> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> Describe the binding for firmware-configured instances of the Synopsys
>>>> Designware PCIe controller in RC mode.
>>>>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++
>>>>  1 file changed, 56 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>>>> new file mode 100644
>>>> index 000000000000..b8127b19c220
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>>>> @@ -0,0 +1,56 @@
>>>> +* Synopsys Designware PCIe root complex in ECAM mode
>>>> +
>>>> +In some cases, firmware may already have configured the Synopsys Designware
>>>> +PCIe controller in RC mode with static ATU window mappings that cover all
>>>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
>>>> +In this case, there is no need for the OS to perform any low level setup
>>>> +of clocks or device registers, nor is there any reason for the driver to
>>>> +reconfigure ATU windows for config and/or IO space accesses at runtime.
>>>> +
>>>> +Such hardware configurations should be described as "pci-host-ecam-generic"
>>>> +if they are truly ECAM compatible. Configurations that require no low-level
>>>> +setup by the OS nor any ATU window reconfiguration at runtime, but do
>>>> +require special handling for type 0 config TLPs may instead be described as
>>>> +"snps,dw-pcie-ecam".
>>>
>>> Humm, what happens when we have the next exception that's SoC specific
>>> or another vendor?
>>
>> This is not SoC specific, but IP specific. We are working with two
>> different SoCs from completely different vendors that both synthesized
>> this IP with a 64 KB ATU window size, not expecting this to break ECAM
>> compatibility.
>
> This case is not SoC specific, but quirks/bugs show up in both places.
> Plus "dw-pcie" is fairly meaningless because the IP is both
> configurable and has multiple releases.
>
>>> Seems like perhaps "firmware initialized" should
>>> have been a separate property flag for bootloaders to add rather than
>>> a compatible string.
>>>
>>
>> Yes, but then you still have 10 different drivers that all retain the
>> low-level bits that are all different between SoCs. That is exactly
>> what I want to get rid of, and usually we can do that with existing
>> bindings, because we simply expose it as pci-host-ecam-generic. Only
>> in this particular case, that doesn't fly due to the quirk.
>
> You can expose it as "vendor,soc-pcie", "pci-host-ecam-generic" and
> then the kernel can decide. Then you can use: the default ecam driver,
> the ecam driver but match on "vendor,soc-pcie" for quirks, or a vendor
> specific driver. The only thing that wouldn't work would be having
> both quirks in the ecam driver and a vendor driver which IMO we just
> shouldn't support anyway.
>
> Now if you have 10 SoCs all needing this same quirk, then maybe adding
> "snps,dw-pcie-ecam" addtionally to the above makes sense. But that
> only saves you match strings.
>

Exposing this as "pci-host-ecam-generic" doesn't work, or we wouldn't
be having this discussion.

So if I understand you correctly, you would rather have

marvell,armada8k-pcie-ecam
socionext,synquacer-pcie-ecam

and add more of those to the list if needed, and update the
binding+driver to use those and drop the generic identifier? I take it
that applies to the description of the MSI frame as well?

>>> I'd rather see this done in a way that does not require DT updates if
>>> quirks have to be handled/added later.
>>>
>>
>> Do you see a way that still allows us to keep the abstraction? I don't
>> want a flag, I simply don't want to expose any low-level specifics
>> about the device to the OS, beyond what it needs to use it in its
>> configured state.
>
> That's not how DT works. Detailed information is exposed to the OS and
> the OS decides if it can handle things generically or not because that
> decision can change over time. Whenever we don't make things specific
> enough, we've gotten burned.
>

Re: [PATCH v2 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Rob Herring]

On Thu, Aug 24, 2017 at 5:37 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 24 August 2017 at 23:12, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote:
>>>> Cc the DT list for bindings please.
>>>>
>>>> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> Describe the binding for firmware-configured instances of the Synopsys
>>>>> Designware PCIe controller in RC mode.
>>>>>
>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++
>>>>>  1 file changed, 56 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>>>>> new file mode 100644
>>>>> index 000000000000..b8127b19c220
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
>>>>> @@ -0,0 +1,56 @@
>>>>> +* Synopsys Designware PCIe root complex in ECAM mode
>>>>> +
>>>>> +In some cases, firmware may already have configured the Synopsys Designware
>>>>> +PCIe controller in RC mode with static ATU window mappings that cover all
>>>>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
>>>>> +In this case, there is no need for the OS to perform any low level setup
>>>>> +of clocks or device registers, nor is there any reason for the driver to
>>>>> +reconfigure ATU windows for config and/or IO space accesses at runtime.
>>>>> +
>>>>> +Such hardware configurations should be described as "pci-host-ecam-generic"
>>>>> +if they are truly ECAM compatible. Configurations that require no low-level
>>>>> +setup by the OS nor any ATU window reconfiguration at runtime, but do
>>>>> +require special handling for type 0 config TLPs may instead be described as
>>>>> +"snps,dw-pcie-ecam".
>>>>
>>>> Humm, what happens when we have the next exception that's SoC specific
>>>> or another vendor?
>>>
>>> This is not SoC specific, but IP specific. We are working with two
>>> different SoCs from completely different vendors that both synthesized
>>> this IP with a 64 KB ATU window size, not expecting this to break ECAM
>>> compatibility.
>>
>> This case is not SoC specific, but quirks/bugs show up in both places.
>> Plus "dw-pcie" is fairly meaningless because the IP is both
>> configurable and has multiple releases.
>>
>>>> Seems like perhaps "firmware initialized" should
>>>> have been a separate property flag for bootloaders to add rather than
>>>> a compatible string.
>>>>
>>>
>>> Yes, but then you still have 10 different drivers that all retain the
>>> low-level bits that are all different between SoCs. That is exactly
>>> what I want to get rid of, and usually we can do that with existing
>>> bindings, because we simply expose it as pci-host-ecam-generic. Only
>>> in this particular case, that doesn't fly due to the quirk.
>>
>> You can expose it as "vendor,soc-pcie", "pci-host-ecam-generic" and
>> then the kernel can decide. Then you can use: the default ecam driver,
>> the ecam driver but match on "vendor,soc-pcie" for quirks, or a vendor
>> specific driver. The only thing that wouldn't work would be having
>> both quirks in the ecam driver and a vendor driver which IMO we just
>> shouldn't support anyway.
>>
>> Now if you have 10 SoCs all needing this same quirk, then maybe adding
>> "snps,dw-pcie-ecam" addtionally to the above makes sense. But that
>> only saves you match strings.
>>
>
> Exposing this as "pci-host-ecam-generic" doesn't work, or we wouldn't
> be having this discussion.

Exposing it *alone* won't work, but you could match on it for the
driver and then in the probe function have an:

if (of_device_is_compatible(np, "marvell,armada8k-pcie-ecam"))
  // handle device quirk

I haven't looked at what exactly you need to handle though. I don't
have the rest of the series.

But since you already know the problem, you could just drop
"pci-host-ecam-generic" in this case.

>
> So if I understand you correctly, you would rather have
>
> marvell,armada8k-pcie-ecam
> socionext,synquacer-pcie-ecam
>
> and add more of those to the list if needed, and update the
> binding+driver to use those and drop the generic identifier?

Yes, but dropping the generic identifier depends if you know you have
some problem up front. You do here, but that's not always the case.

The other part is that "pci-host-ecam-generic" alone should not be
considered valid other than for VM guests. But I can't really enforce
that.

> I take it
> that applies to the description of the MSI frame as well?

Probably so. Is there something special about it, too?

Rob