linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] pci: add support for firmware initialized designware RCs
@ 2017-08-21 19:29 Ard Biesheuvel
  2017-08-21 19:29 ` [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-21 19:29 UTC (permalink / raw)
  To: linux-arm-kernel

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.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode
  2017-08-21 19:29 [PATCH 0/3] pci: add support for firmware initialized designware RCs Ard Biesheuvel
@ 2017-08-21 19:29 ` Ard Biesheuvel
  2017-08-24 16:24   ` kbuild test robot
  2017-08-24 16:25   ` kbuild test robot
  2017-08-21 19:29 ` [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC Ard Biesheuvel
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-21 19:29 UTC (permalink / raw)
  To: linux-arm-kernel

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..f2afa2c519c1 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
+	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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2017-08-21 19:29 [PATCH 0/3] pci: add support for firmware initialized designware RCs Ard Biesheuvel
  2017-08-21 19:29 ` [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
@ 2017-08-21 19:29 ` Ard Biesheuvel
  2017-08-24 16:42   ` Bjorn Helgaas
                     ` (2 more replies)
  2017-08-21 19:29 ` [PATCH 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel
  2017-08-24 16:46 ` [PATCH 0/3] pci: add support for firmware initialized designware RCs Bjorn Helgaas
  3 siblings, 3 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-21 19:29 UTC (permalink / raw)
  To: linux-arm-kernel

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..5891211b04be 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) += 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..c8c454145c0d
--- /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 fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+	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) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	dw_msi->irqd = irq_domain_create_linear(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(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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode
  2017-08-21 19:29 [PATCH 0/3] pci: add support for firmware initialized designware RCs Ard Biesheuvel
  2017-08-21 19:29 ` [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
  2017-08-21 19:29 ` [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC Ard Biesheuvel
@ 2017-08-21 19:29 ` Ard Biesheuvel
  2017-08-24 16:46 ` [PATCH 0/3] pci: add support for firmware initialized designware RCs Bjorn Helgaas
  3 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-21 19:29 UTC (permalink / raw)
  To: linux-arm-kernel

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 at 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 at 10000000 {
+    compatible = "snps,dw-pcie-msi";
+    reg = <0x0 0x10000000 0x0 0x10000>;
+    interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>;
+    msi-controller;
+  };
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode
  2017-08-21 19:29 ` [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
@ 2017-08-24 16:24   ` kbuild test robot
  2017-08-24 16:25   ` kbuild test robot
  1 sibling, 0 replies; 21+ messages in thread
From: kbuild test robot @ 2017-08-24 16:24 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ard,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.13-rc6 next-20170824]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/pci-add-support-for-firmware-initialized-designware-RCs/20170824-232638
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All warnings (new ones prefixed by >>):

warning: (PCIE_DW_HOST_ECAM) selects PCI_HOST_COMMON which has unmet direct dependencies (MMU && PCI)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 43721 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170825/22c0e570/attachment-0001.gz>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode
  2017-08-21 19:29 ` [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
  2017-08-24 16:24   ` kbuild test robot
@ 2017-08-24 16:25   ` kbuild test robot
  1 sibling, 0 replies; 21+ messages in thread
From: kbuild test robot @ 2017-08-24 16:25 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ard,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.13-rc6 next-20170824]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/pci-add-support-for-firmware-initialized-designware-RCs/20170824-232638
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All warnings (new ones prefixed by >>):

warning: (PCIE_DW_HOST_ECAM) selects PCI_HOST_COMMON which has unmet direct dependencies (PCI)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 47113 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170825/d2fac168/attachment-0001.gz>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2017-08-21 19:29 ` [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC Ard Biesheuvel
@ 2017-08-24 16:42   ` Bjorn Helgaas
  2017-08-24 16:43     ` Ard Biesheuvel
  2017-08-24 16:48   ` Robin Murphy
  2020-02-15  0:54   ` Alan Mikhak
  2 siblings, 1 reply; 21+ messages in thread
From: Bjorn Helgaas @ 2017-08-24 16:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Aug 21, 2017 at 08:29:06PM +0100, Ard Biesheuvel wrote:
> 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>

> +static int dw_pcie_msi_probe(struct platform_device *pdev)
> +{
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +	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) {
> +		pr_err("Failed to map IRQ\n");

dev_err()

I'm not sure "failed to *map* IRQ" is the most informative text.
Other callers often use some variant of "failed to get IRQ" or "no IRQ
resource found".

> +		return -ENXIO;
> +	}
> +
> +	dw_msi->irqd = irq_domain_create_linear(fwnode, MAX_MSI_IRQS,
> +						&irq_dom_ops, dw_msi);
> +	if (!dw_msi->irqd) {
> +		dev_err(dev, "Failed to create IRQ domain\n");
> +		return -ENOMEM;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2017-08-24 16:42   ` Bjorn Helgaas
@ 2017-08-24 16:43     ` Ard Biesheuvel
  0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-24 16:43 UTC (permalink / raw)
  To: linux-arm-kernel

On 24 August 2017 at 17:42, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Aug 21, 2017 at 08:29:06PM +0100, Ard Biesheuvel wrote:
>> 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>
>
>> +static int dw_pcie_msi_probe(struct platform_device *pdev)
>> +{
>> +     struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +     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) {
>> +             pr_err("Failed to map IRQ\n");
>
> dev_err()
>
> I'm not sure "failed to *map* IRQ" is the most informative text.
> Other callers often use some variant of "failed to get IRQ" or "no IRQ
> resource found".
>

Good point. I will fix that.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 0/3] pci: add support for firmware initialized designware RCs
  2017-08-21 19:29 [PATCH 0/3] pci: add support for firmware initialized designware RCs Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2017-08-21 19:29 ` [PATCH 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel
@ 2017-08-24 16:46 ` Bjorn Helgaas
  2017-08-31 19:21   ` Ard Biesheuvel
  3 siblings, 1 reply; 21+ messages in thread
From: Bjorn Helgaas @ 2017-08-24 16:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Aug 21, 2017 at 08:29:04PM +0100, Ard Biesheuvel wrote:
> 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".

I *love* the idea of moving more of this to firmware.  I spend an
incredible amount of time looking at all these native drivers, and
*zero* time looking at the ACPI driver (pci_root.c).  And it's not
like all this code in the native drivers is adding features or
performance.  It's just extra work for no benefit.

I'm assuming there'll be a v2 soon for the kbuild warnings.

> 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.
> 
> 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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2017-08-21 19:29 ` [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC Ard Biesheuvel
  2017-08-24 16:42   ` Bjorn Helgaas
@ 2017-08-24 16:48   ` Robin Murphy
  2017-08-24 16:50     ` Ard Biesheuvel
  2020-02-15  0:54   ` Alan Mikhak
  2 siblings, 1 reply; 21+ messages in thread
From: Robin Murphy @ 2017-08-24 16:48 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ard,

On 21/08/17 20:29, Ard Biesheuvel wrote:
[...]
> +static int dw_pcie_msi_probe(struct platform_device *pdev)
> +{
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);

Mini-nit: since fairly recently (f94277af03ea) dev->fwnode should
already be set appropriately by of_platform_device_create(), so you
should be able to make this entirely firmware-agnostic if you like.

Robin.

> +	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) {
> +		pr_err("Failed to map IRQ\n");
> +		return -ENXIO;
> +	}
> +
> +	dw_msi->irqd = irq_domain_create_linear(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(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);
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2017-08-24 16:48   ` Robin Murphy
@ 2017-08-24 16:50     ` Ard Biesheuvel
  0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-24 16:50 UTC (permalink / raw)
  To: linux-arm-kernel

On 24 August 2017 at 17:48, Robin Murphy <robin.murphy@arm.com> wrote:
> Hi Ard,
>
> On 21/08/17 20:29, Ard Biesheuvel wrote:
> [...]
>> +static int dw_pcie_msi_probe(struct platform_device *pdev)
>> +{
>> +     struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
>
> Mini-nit: since fairly recently (f94277af03ea) dev->fwnode should
> already be set appropriately by of_platform_device_create(), so you
> should be able to make this entirely firmware-agnostic if you like.
>

Thanks for pointing that out. I'm not sure yet what it means exactly,
but I'm sure I will figure it out :-)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 0/3] pci: add support for firmware initialized designware RCs
  2017-08-24 16:46 ` [PATCH 0/3] pci: add support for firmware initialized designware RCs Bjorn Helgaas
@ 2017-08-31 19:21   ` Ard Biesheuvel
  0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-08-31 19:21 UTC (permalink / raw)
  To: linux-arm-kernel

On 24 August 2017 at 17:46, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Aug 21, 2017 at 08:29:04PM +0100, Ard Biesheuvel wrote:
>> 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".
>
> I *love* the idea of moving more of this to firmware.

Enough to consider the v3 of this series for v4.14? Rob has acked the
binding, and as far as I am aware. it is ultimately up to you whether
and when you take them as a pair. Also, I don't mind being listed as
maintainer and/or reviewer either if it helps.

Thanks,
Ard.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2017-08-21 19:29 ` [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC Ard Biesheuvel
  2017-08-24 16:42   ` Bjorn Helgaas
  2017-08-24 16:48   ` Robin Murphy
@ 2020-02-15  0:54   ` Alan Mikhak
  2020-02-15  9:35     ` Ard Biesheuvel
  2 siblings, 1 reply; 21+ messages in thread
From: Alan Mikhak @ 2020-02-15  0:54 UTC (permalink / raw)
  To: ard.biesheuvel
  Cc: Joao.Pinto, graeme.gregory, marc.zyngier, jingoohan1,
	leif.lindholm, linux-pci, bhelgaas, linux-arm-kernel

Hi..

What is the right approach for adding MSI support for the generic
Linux PCI host driver?

I came across this patch which seems to address a similar
situation. It seems to have been dropped in v3 of the patchset
with the explanation "drop MSI patch [for now], since it
turns out we may not need it".

[PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/

[PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/

[PATCH v3 0/2] pci: add support for firmware initialized designware RCs
https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/


Regards,
Alan Mikhak

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-15  0:54   ` Alan Mikhak
@ 2020-02-15  9:35     ` Ard Biesheuvel
  2020-02-15 10:36       ` Marc Zyngier
  0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2020-02-15  9:35 UTC (permalink / raw)
  To: Alan Mikhak
  Cc: Marc Zyngier, Joao Pinto, Graeme Gregory, Jingoo Han, linux-pci,
	Bjorn Helgaas, Leif Lindholm, linux-arm-kernel

(updated some email addresses in cc, including my own)

On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
>
> Hi..
>
> What is the right approach for adding MSI support for the generic
> Linux PCI host driver?
>
> I came across this patch which seems to address a similar
> situation. It seems to have been dropped in v3 of the patchset
> with the explanation "drop MSI patch [for now], since it
> turns out we may not need it".
>
> [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
>
> [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
>
> [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
>

For the platform in question, it turned out that we could use the MSI
block of the core's GIC interrupt controller directly, which is a much
better solution.

In general, turning MSIs into wired interrupts is not a great idea,
since the whole point of MSIs is that they are sufficiently similar to
other DMA transactions to ensure that the interrupt won't arrive
before the related memory transactions have completed.

If your interrupt controller does not have this capability, then yes,
you are stuck with this little widget that decodes an inbound write to
a magic address and turns it into a wired interrupt.

I'll leave it up to the Synopsys folks to comment on whether this
feature is generic enough to describe it like this, but if so, I think
it still makes sense to model it this way rather than fold it into the
RC driver and description.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-15  9:35     ` Ard Biesheuvel
@ 2020-02-15 10:36       ` Marc Zyngier
  2020-02-18 19:09         ` Alan Mikhak
  0 siblings, 1 reply; 21+ messages in thread
From: Marc Zyngier @ 2020-02-15 10:36 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Joao Pinto, Graeme Gregory, Jingoo Han, Alan Mikhak, linux-pci,
	Bjorn Helgaas, Leif Lindholm, linux-arm-kernel

On Sat, 15 Feb 2020 09:35:56 +0000,
Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> (updated some email addresses in cc, including my own)
> 
> On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> >
> > Hi..
> >
> > What is the right approach for adding MSI support for the generic
> > Linux PCI host driver?
> >
> > I came across this patch which seems to address a similar
> > situation. It seems to have been dropped in v3 of the patchset
> > with the explanation "drop MSI patch [for now], since it
> > turns out we may not need it".
> >
> > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> >
> > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> >
> > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> >
> 
> For the platform in question, it turned out that we could use the MSI
> block of the core's GIC interrupt controller directly, which is a much
> better solution.
> 
> In general, turning MSIs into wired interrupts is not a great idea,
> since the whole point of MSIs is that they are sufficiently similar to
> other DMA transactions to ensure that the interrupt won't arrive
> before the related memory transactions have completed.
>
> If your interrupt controller does not have this capability, then yes,
> you are stuck with this little widget that decodes an inbound write to
> a magic address and turns it into a wired interrupt.

I can only second this. It is much better to have a generic block
implementing MSI *in a non multiplexed way*, for multiple reasons:

- the interrupt vs DMA race that Ard mentions above,

- MSIs are very often used to describe the state of per-CPU queues. If
  you multiplex MSIs behind a single multiplexing interrupt, it is
  always the same CPU that gets interrupted, and you don't benefit
  from having multiple queues at all.

Even if you have to implement the support as a bunch of wired
interrupts, there is still a lot of value in keeping a 1:1 mapping
between MSIs and wires.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-15 10:36       ` Marc Zyngier
@ 2020-02-18 19:09         ` Alan Mikhak
  2020-02-19  8:11           ` Marc Zyngier
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Mikhak @ 2020-02-18 19:09 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Joao Pinto, Graeme Gregory, Jingoo Han, linux-pci, Bjorn Helgaas,
	Leif Lindholm, Ard Biesheuvel, linux-arm-kernel

On Sat, Feb 15, 2020 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sat, 15 Feb 2020 09:35:56 +0000,
> Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > (updated some email addresses in cc, including my own)
> >
> > On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > >
> > > Hi..
> > >
> > > What is the right approach for adding MSI support for the generic
> > > Linux PCI host driver?
> > >
> > > I came across this patch which seems to address a similar
> > > situation. It seems to have been dropped in v3 of the patchset
> > > with the explanation "drop MSI patch [for now], since it
> > > turns out we may not need it".
> > >
> > > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> > >
> > > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> > >
> > > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> > >
> >
> > For the platform in question, it turned out that we could use the MSI
> > block of the core's GIC interrupt controller directly, which is a much
> > better solution.
> >
> > In general, turning MSIs into wired interrupts is not a great idea,
> > since the whole point of MSIs is that they are sufficiently similar to
> > other DMA transactions to ensure that the interrupt won't arrive
> > before the related memory transactions have completed.
> >
> > If your interrupt controller does not have this capability, then yes,
> > you are stuck with this little widget that decodes an inbound write to
> > a magic address and turns it into a wired interrupt.
>
> I can only second this. It is much better to have a generic block
> implementing MSI *in a non multiplexed way*, for multiple reasons:
>
> - the interrupt vs DMA race that Ard mentions above,
>
> - MSIs are very often used to describe the state of per-CPU queues. If
>   you multiplex MSIs behind a single multiplexing interrupt, it is
>   always the same CPU that gets interrupted, and you don't benefit
>   from having multiple queues at all.
>
> Even if you have to implement the support as a bunch of wired
> interrupts, there is still a lot of value in keeping a 1:1 mapping
> between MSIs and wires.
>
> Thanks,
>
>         M.
>
> --
> Jazz is not dead, it just smells funny.

Ard and Marc, Thanks for you comments. I will take a look at the code
related to MSI block of GIC interrupt controller for some reference.

I am looking into supporting MSI in a non-multiplexed way when using
ECAM and the generic Linux PCI host driver when Linux is booted
from U-Boot.

Specifically, what is the right approach for sharing the physical
address of the MSI data block used in Linux with U-Boot?

I imagine the Linux driver for MSI interrupt controller allocates
some DMA-able memory for use as the MSI data block. The
U-Boot PCIe driver would program an inbound ATU region to
map mem writes from endpoint devices to that MSI data block
before booting Linux.

Comments are appreciated.

Alan

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-18 19:09         ` Alan Mikhak
@ 2020-02-19  8:11           ` Marc Zyngier
  2020-02-19  8:17             ` Ard Biesheuvel
  0 siblings, 1 reply; 21+ messages in thread
From: Marc Zyngier @ 2020-02-19  8:11 UTC (permalink / raw)
  To: Alan Mikhak
  Cc: Joao Pinto, Graeme Gregory, Jingoo Han, linux-pci, Bjorn Helgaas,
	Leif Lindholm, Ard Biesheuvel, linux-arm-kernel

On Tue, 18 Feb 2020 11:09:10 -0800
Alan Mikhak <alan.mikhak@sifive.com> wrote:

> On Sat, Feb 15, 2020 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Sat, 15 Feb 2020 09:35:56 +0000,
> > Ard Biesheuvel <ardb@kernel.org> wrote:  
> > >
> > > (updated some email addresses in cc, including my own)
> > >
> > > On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:  
> > > >
> > > > Hi..
> > > >
> > > > What is the right approach for adding MSI support for the generic
> > > > Linux PCI host driver?
> > > >
> > > > I came across this patch which seems to address a similar
> > > > situation. It seems to have been dropped in v3 of the patchset
> > > > with the explanation "drop MSI patch [for now], since it
> > > > turns out we may not need it".
> > > >
> > > > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> > > >
> > > > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> > > >
> > > > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > > > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> > > >  
> > >
> > > For the platform in question, it turned out that we could use the MSI
> > > block of the core's GIC interrupt controller directly, which is a much
> > > better solution.
> > >
> > > In general, turning MSIs into wired interrupts is not a great idea,
> > > since the whole point of MSIs is that they are sufficiently similar to
> > > other DMA transactions to ensure that the interrupt won't arrive
> > > before the related memory transactions have completed.
> > >
> > > If your interrupt controller does not have this capability, then yes,
> > > you are stuck with this little widget that decodes an inbound write to
> > > a magic address and turns it into a wired interrupt.  
> >
> > I can only second this. It is much better to have a generic block
> > implementing MSI *in a non multiplexed way*, for multiple reasons:
> >
> > - the interrupt vs DMA race that Ard mentions above,
> >
> > - MSIs are very often used to describe the state of per-CPU queues. If
> >   you multiplex MSIs behind a single multiplexing interrupt, it is
> >   always the same CPU that gets interrupted, and you don't benefit
> >   from having multiple queues at all.
> >
> > Even if you have to implement the support as a bunch of wired
> > interrupts, there is still a lot of value in keeping a 1:1 mapping
> > between MSIs and wires.
> >
> > Thanks,
> >
> >         M.
> >
> > --
> > Jazz is not dead, it just smells funny.  
> 
> Ard and Marc, Thanks for you comments. I will take a look at the code
> related to MSI block of GIC interrupt controller for some reference.

GICv2m or GICv3 MBI are probably your best bets. Don't get anywhere near
the GICv3 ITS, there lies madness. ;-)

> I am looking into supporting MSI in a non-multiplexed way when using
> ECAM and the generic Linux PCI host driver when Linux is booted
> from U-Boot.

I don't really get the relationship between ECAM and MSIs. They should
be fairly independent, unless that has to do with the allowing the MSI
doorbell to be reached from the PCIe endpoint.

> Specifically, what is the right approach for sharing the physical
> address of the MSI data block used in Linux with U-Boot?
> 
> I imagine the Linux driver for MSI interrupt controller allocates
> some DMA-able memory for use as the MSI data block. The
> U-Boot PCIe driver would program an inbound ATU region to
> map mem writes from endpoint devices to that MSI data block
> before booting Linux.

The "MSI block" is really a piece of HW, not memory. So whatever you
have to program in the PCIe RC must allow an endpoint to reach that
device with a 32bit write.

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

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-19  8:11           ` Marc Zyngier
@ 2020-02-19  8:17             ` Ard Biesheuvel
  2020-02-19 20:24               ` Alan Mikhak
  0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2020-02-19  8:17 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Joao Pinto, Graeme Gregory, Jingoo Han, Alan Mikhak, linux-pci,
	Bjorn Helgaas, Leif Lindholm, linux-arm-kernel

On Wed, 19 Feb 2020 at 09:11, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 18 Feb 2020 11:09:10 -0800
> Alan Mikhak <alan.mikhak@sifive.com> wrote:
>
> > On Sat, Feb 15, 2020 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Sat, 15 Feb 2020 09:35:56 +0000,
> > > Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > (updated some email addresses in cc, including my own)
> > > >
> > > > On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > > > >
> > > > > Hi..
> > > > >
> > > > > What is the right approach for adding MSI support for the generic
> > > > > Linux PCI host driver?
> > > > >
> > > > > I came across this patch which seems to address a similar
> > > > > situation. It seems to have been dropped in v3 of the patchset
> > > > > with the explanation "drop MSI patch [for now], since it
> > > > > turns out we may not need it".
> > > > >
> > > > > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> > > > >
> > > > > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> > > > >
> > > > > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > > > > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> > > > >
> > > >
> > > > For the platform in question, it turned out that we could use the MSI
> > > > block of the core's GIC interrupt controller directly, which is a much
> > > > better solution.
> > > >
> > > > In general, turning MSIs into wired interrupts is not a great idea,
> > > > since the whole point of MSIs is that they are sufficiently similar to
> > > > other DMA transactions to ensure that the interrupt won't arrive
> > > > before the related memory transactions have completed.
> > > >
> > > > If your interrupt controller does not have this capability, then yes,
> > > > you are stuck with this little widget that decodes an inbound write to
> > > > a magic address and turns it into a wired interrupt.
> > >
> > > I can only second this. It is much better to have a generic block
> > > implementing MSI *in a non multiplexed way*, for multiple reasons:
> > >
> > > - the interrupt vs DMA race that Ard mentions above,
> > >
> > > - MSIs are very often used to describe the state of per-CPU queues. If
> > >   you multiplex MSIs behind a single multiplexing interrupt, it is
> > >   always the same CPU that gets interrupted, and you don't benefit
> > >   from having multiple queues at all.
> > >
> > > Even if you have to implement the support as a bunch of wired
> > > interrupts, there is still a lot of value in keeping a 1:1 mapping
> > > between MSIs and wires.
> > >
> > > Thanks,
> > >
> > >         M.
> > >
> > > --
> > > Jazz is not dead, it just smells funny.
> >
> > Ard and Marc, Thanks for you comments. I will take a look at the code
> > related to MSI block of GIC interrupt controller for some reference.
>
> GICv2m or GICv3 MBI are probably your best bets. Don't get anywhere near
> the GICv3 ITS, there lies madness. ;-)
>

True, but for the record, it is the GICv3 ITS that I used on the
platform in question, allowing me to ignore the pseudo-MSI widget
entirely.

> > I am looking into supporting MSI in a non-multiplexed way when using
> > ECAM and the generic Linux PCI host driver when Linux is booted
> > from U-Boot.
>
> I don't really get the relationship between ECAM and MSIs. They should
> be fairly independent, unless that has to do with the allowing the MSI
> doorbell to be reached from the PCIe endpoint.
>

The idea is that the PCIe RC is programmed by firmware, and exposed to
the OS as generic ECAM. If you have enough iATU registers and enough
free address space, that is perfectly feasible.

The problem is that the generic ECAM binding does not have any
provisions for MSI doorbell widgets that turn inbound writes to a
magic address into a wired interrupt. My patch models this as a
separate device, which allows a generic ECAM DT node to refer to it as
its MSI parent.


> > Specifically, what is the right approach for sharing the physical
> > address of the MSI data block used in Linux with U-Boot?
> >
> > I imagine the Linux driver for MSI interrupt controller allocates
> > some DMA-able memory for use as the MSI data block. The
> > U-Boot PCIe driver would program an inbound ATU region to
> > map mem writes from endpoint devices to that MSI data block
> > before booting Linux.
>
> The "MSI block" is really a piece of HW, not memory. So whatever you
> have to program in the PCIe RC must allow an endpoint to reach that
> device with a 32bit write.
>

Indeed. Either your interrupt controller or your PCIe RC needs to
implement the doorbell, but using the former is by far the preferred
option.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-19  8:17             ` Ard Biesheuvel
@ 2020-02-19 20:24               ` Alan Mikhak
  2020-02-19 21:06                 ` Ard Biesheuvel
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Mikhak @ 2020-02-19 20:24 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Joao Pinto, Graeme Gregory, Marc Zyngier, linux-pci, Jingoo Han,
	Bjorn Helgaas, Leif Lindholm, linux-arm-kernel

On Wed, Feb 19, 2020 at 12:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 19 Feb 2020 at 09:11, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 18 Feb 2020 11:09:10 -0800
> > Alan Mikhak <alan.mikhak@sifive.com> wrote:
> >
> > > On Sat, Feb 15, 2020 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Sat, 15 Feb 2020 09:35:56 +0000,
> > > > Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >
> > > > > (updated some email addresses in cc, including my own)
> > > > >
> > > > > On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > > > > >
> > > > > > Hi..
> > > > > >
> > > > > > What is the right approach for adding MSI support for the generic
> > > > > > Linux PCI host driver?
> > > > > >
> > > > > > I came across this patch which seems to address a similar
> > > > > > situation. It seems to have been dropped in v3 of the patchset
> > > > > > with the explanation "drop MSI patch [for now], since it
> > > > > > turns out we may not need it".
> > > > > >
> > > > > > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> > > > > >
> > > > > > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> > > > > >
> > > > > > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > > > > > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> > > > > >
> > > > >
> > > > > For the platform in question, it turned out that we could use the MSI
> > > > > block of the core's GIC interrupt controller directly, which is a much
> > > > > better solution.
> > > > >
> > > > > In general, turning MSIs into wired interrupts is not a great idea,
> > > > > since the whole point of MSIs is that they are sufficiently similar to
> > > > > other DMA transactions to ensure that the interrupt won't arrive
> > > > > before the related memory transactions have completed.
> > > > >
> > > > > If your interrupt controller does not have this capability, then yes,
> > > > > you are stuck with this little widget that decodes an inbound write to
> > > > > a magic address and turns it into a wired interrupt.
> > > >
> > > > I can only second this. It is much better to have a generic block
> > > > implementing MSI *in a non multiplexed way*, for multiple reasons:
> > > >
> > > > - the interrupt vs DMA race that Ard mentions above,
> > > >
> > > > - MSIs are very often used to describe the state of per-CPU queues. If
> > > >   you multiplex MSIs behind a single multiplexing interrupt, it is
> > > >   always the same CPU that gets interrupted, and you don't benefit
> > > >   from having multiple queues at all.
> > > >
> > > > Even if you have to implement the support as a bunch of wired
> > > > interrupts, there is still a lot of value in keeping a 1:1 mapping
> > > > between MSIs and wires.
> > > >
> > > > Thanks,
> > > >
> > > >         M.
> > > >
> > > > --
> > > > Jazz is not dead, it just smells funny.
> > >
> > > Ard and Marc, Thanks for you comments. I will take a look at the code
> > > related to MSI block of GIC interrupt controller for some reference.
> >
> > GICv2m or GICv3 MBI are probably your best bets. Don't get anywhere near
> > the GICv3 ITS, there lies madness. ;-)
> >
>
> True, but for the record, it is the GICv3 ITS that I used on the
> platform in question, allowing me to ignore the pseudo-MSI widget
> entirely.
>
> > > I am looking into supporting MSI in a non-multiplexed way when using
> > > ECAM and the generic Linux PCI host driver when Linux is booted
> > > from U-Boot.
> >
> > I don't really get the relationship between ECAM and MSIs. They should
> > be fairly independent, unless that has to do with the allowing the MSI
> > doorbell to be reached from the PCIe endpoint.
> >
>
> The idea is that the PCIe RC is programmed by firmware, and exposed to
> the OS as generic ECAM. If you have enough iATU registers and enough
> free address space, that is perfectly feasible.
>
> The problem is that the generic ECAM binding does not have any
> provisions for MSI doorbell widgets that turn inbound writes to a
> magic address into a wired interrupt. My patch models this as a
> separate device, which allows a generic ECAM DT node to refer to it as
> its MSI parent.
>
>
> > > Specifically, what is the right approach for sharing the physical
> > > address of the MSI data block used in Linux with U-Boot?
> > >
> > > I imagine the Linux driver for MSI interrupt controller allocates
> > > some DMA-able memory for use as the MSI data block. The
> > > U-Boot PCIe driver would program an inbound ATU region to
> > > map mem writes from endpoint devices to that MSI data block
> > > before booting Linux.
> >
> > The "MSI block" is really a piece of HW, not memory. So whatever you
> > have to program in the PCIe RC must allow an endpoint to reach that
> > device with a 32bit write.
> >
>
> Indeed. Either your interrupt controller or your PCIe RC needs to
> implement the doorbell, but using the former is by far the preferred
> option.

Ard and Marc, Thank you so much for your insightful comments.

The generic PCI host driver uses ECAM as the access method to
read/write PCI configuration registers but has no support for MSI.
I imagine I could use the MSI widget model from Art's patch to
implement a separate Linux interrupt handler for MSI interrupts.

I'm not sure but the MSI widget seems to multiplex MSI interrupts
to one wired interrupt since its MSI doorbell is a u32 value. The widget
also has code for programming the address of the doorbell into
Designware PCIe IP registers.

I imagine I would separate the lines of code that programs the
PCIe IP MSI registers and move that non-generic PCIe code from
Linux to U-Boot "firmware". The MSI interrupt handler would
then become more of a generic PCI MSI interrupt handler.

The "MSI block" I refer to is a page of memory that I see being
allocated and mapped for dma access from endpoint devices
in the Designware PCI host driver function dw_pcie_msi_init().
The physical address of this MSI data block is programmed into
Designware PCIe IP MSI registers by Designware host driver.
I believe this is the target memory where endpoint MSI write
requests would be targeted to. I imagine an inbound ATU region
maps the bus transaction to a physical address within this MSI
data block to support non-multiplexed MSI interrupt handling.

Whether the doorbell is a u32 value or a block of memory,
the chicken or the egg dilemma I have is how to share the
address of the MSI data block between Linux and U-Boot.
Since all programming code for PCIe IP would reside in the
U-Boot PCIe driver, U-Boot would need to know the address
of the MSI data block before it boots Linux. However, if the MSI
interrupt widget dynamically allocates the MSI data block, it
would contain no code to program the address into the PCIe IP.

I wonder if the MSI data block can be a reserved block of
memory whose physical address is predetermined and shared
via the "reg" entry for the MSI widget between Linux and
U-Boot? Would that make sense?

Regards,
Alan

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-19 20:24               ` Alan Mikhak
@ 2020-02-19 21:06                 ` Ard Biesheuvel
  2020-02-19 21:35                   ` Alan Mikhak
  0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2020-02-19 21:06 UTC (permalink / raw)
  To: Alan Mikhak
  Cc: Joao Pinto, Graeme Gregory, Marc Zyngier, linux-pci, Jingoo Han,
	Bjorn Helgaas, Leif Lindholm, linux-arm-kernel

On Wed, 19 Feb 2020 at 21:24, Alan Mikhak <alan.mikhak@sifive.com> wrote:
>
> On Wed, Feb 19, 2020 at 12:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 19 Feb 2020 at 09:11, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Tue, 18 Feb 2020 11:09:10 -0800
> > > Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > >
> > > > On Sat, Feb 15, 2020 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > On Sat, 15 Feb 2020 09:35:56 +0000,
> > > > > Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >
> > > > > > (updated some email addresses in cc, including my own)
> > > > > >
> > > > > > On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > > > > > >
> > > > > > > Hi..
> > > > > > >
> > > > > > > What is the right approach for adding MSI support for the generic
> > > > > > > Linux PCI host driver?
> > > > > > >
> > > > > > > I came across this patch which seems to address a similar
> > > > > > > situation. It seems to have been dropped in v3 of the patchset
> > > > > > > with the explanation "drop MSI patch [for now], since it
> > > > > > > turns out we may not need it".
> > > > > > >
> > > > > > > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > > > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> > > > > > >
> > > > > > > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > > > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> > > > > > >
> > > > > > > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > > > > > > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> > > > > > >
> > > > > >
> > > > > > For the platform in question, it turned out that we could use the MSI
> > > > > > block of the core's GIC interrupt controller directly, which is a much
> > > > > > better solution.
> > > > > >
> > > > > > In general, turning MSIs into wired interrupts is not a great idea,
> > > > > > since the whole point of MSIs is that they are sufficiently similar to
> > > > > > other DMA transactions to ensure that the interrupt won't arrive
> > > > > > before the related memory transactions have completed.
> > > > > >
> > > > > > If your interrupt controller does not have this capability, then yes,
> > > > > > you are stuck with this little widget that decodes an inbound write to
> > > > > > a magic address and turns it into a wired interrupt.
> > > > >
> > > > > I can only second this. It is much better to have a generic block
> > > > > implementing MSI *in a non multiplexed way*, for multiple reasons:
> > > > >
> > > > > - the interrupt vs DMA race that Ard mentions above,
> > > > >
> > > > > - MSIs are very often used to describe the state of per-CPU queues. If
> > > > >   you multiplex MSIs behind a single multiplexing interrupt, it is
> > > > >   always the same CPU that gets interrupted, and you don't benefit
> > > > >   from having multiple queues at all.
> > > > >
> > > > > Even if you have to implement the support as a bunch of wired
> > > > > interrupts, there is still a lot of value in keeping a 1:1 mapping
> > > > > between MSIs and wires.
> > > > >
> > > > > Thanks,
> > > > >
> > > > >         M.
> > > > >
> > > > > --
> > > > > Jazz is not dead, it just smells funny.
> > > >
> > > > Ard and Marc, Thanks for you comments. I will take a look at the code
> > > > related to MSI block of GIC interrupt controller for some reference.
> > >
> > > GICv2m or GICv3 MBI are probably your best bets. Don't get anywhere near
> > > the GICv3 ITS, there lies madness. ;-)
> > >
> >
> > True, but for the record, it is the GICv3 ITS that I used on the
> > platform in question, allowing me to ignore the pseudo-MSI widget
> > entirely.
> >
> > > > I am looking into supporting MSI in a non-multiplexed way when using
> > > > ECAM and the generic Linux PCI host driver when Linux is booted
> > > > from U-Boot.
> > >
> > > I don't really get the relationship between ECAM and MSIs. They should
> > > be fairly independent, unless that has to do with the allowing the MSI
> > > doorbell to be reached from the PCIe endpoint.
> > >
> >
> > The idea is that the PCIe RC is programmed by firmware, and exposed to
> > the OS as generic ECAM. If you have enough iATU registers and enough
> > free address space, that is perfectly feasible.
> >
> > The problem is that the generic ECAM binding does not have any
> > provisions for MSI doorbell widgets that turn inbound writes to a
> > magic address into a wired interrupt. My patch models this as a
> > separate device, which allows a generic ECAM DT node to refer to it as
> > its MSI parent.
> >
> >
> > > > Specifically, what is the right approach for sharing the physical
> > > > address of the MSI data block used in Linux with U-Boot?
> > > >
> > > > I imagine the Linux driver for MSI interrupt controller allocates
> > > > some DMA-able memory for use as the MSI data block. The
> > > > U-Boot PCIe driver would program an inbound ATU region to
> > > > map mem writes from endpoint devices to that MSI data block
> > > > before booting Linux.
> > >
> > > The "MSI block" is really a piece of HW, not memory. So whatever you
> > > have to program in the PCIe RC must allow an endpoint to reach that
> > > device with a 32bit write.
> > >
> >
> > Indeed. Either your interrupt controller or your PCIe RC needs to
> > implement the doorbell, but using the former is by far the preferred
> > option.
>
> Ard and Marc, Thank you so much for your insightful comments.
>
> The generic PCI host driver uses ECAM as the access method to
> read/write PCI configuration registers but has no support for MSI.
> I imagine I could use the MSI widget model from Art's patch to
> implement a separate Linux interrupt handler for MSI interrupts.
>
> I'm not sure but the MSI widget seems to multiplex MSI interrupts
> to one wired interrupt since its MSI doorbell is a u32 value. The widget
> also has code for programming the address of the doorbell into
> Designware PCIe IP registers.
>

Indeed. So this is the magic address that will receive special
treatment from the RC when a device performs DMA to it. Instead of
relaying the DMA access to memory, it is dropped and instead, the
wired interrupt is asserted.

> I imagine I would separate the lines of code that programs the
> PCIe IP MSI registers and move that non-generic PCIe code from
> Linux to U-Boot "firmware". The MSI interrupt handler would
> then become more of a generic PCI MSI interrupt handler.
>

Not sure that could be done generically enough, tbh.

> The "MSI block" I refer to is a page of memory that I see being
> allocated and mapped for dma access from endpoint devices
> in the Designware PCI host driver function dw_pcie_msi_init().
> The physical address of this MSI data block is programmed into
> Designware PCIe IP MSI registers by Designware host driver.
> I believe this is the target memory where endpoint MSI write
> requests would be targeted to. I imagine an inbound ATU region
> maps the bus transaction to a physical address within this MSI
> data block to support non-multiplexed MSI interrupt handling.
>

As far as I understand it, the memory allocation is only done to
ensure that the MSI magic address doesn't shadow a real memory address
that you'd want to DMA to, given the allocating the page for this
purpose will ensure that it is not used for anything else.

> Whether the doorbell is a u32 value or a block of memory,
> the chicken or the egg dilemma I have is how to share the
> address of the MSI data block between Linux and U-Boot.
> Since all programming code for PCIe IP would reside in the
> U-Boot PCIe driver, U-Boot would need to know the address
> of the MSI data block before it boots Linux. However, if the MSI
> interrupt widget dynamically allocates the MSI data block, it
> would contain no code to program the address into the PCIe IP.
>
> I wonder if the MSI data block can be a reserved block of
> memory whose physical address is predetermined and shared
> via the "reg" entry for the MSI widget between Linux and
> U-Boot? Would that make sense?
>

The address doesn't really matter. What matters is the fact that you
need to code service the wired interrupt in response to the MSIs. Even
if you program the registers from firmware to configure the widget,
you'll still need something to drive it at runtime.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
  2020-02-19 21:06                 ` Ard Biesheuvel
@ 2020-02-19 21:35                   ` Alan Mikhak
  0 siblings, 0 replies; 21+ messages in thread
From: Alan Mikhak @ 2020-02-19 21:35 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Joao Pinto, Graeme Gregory, Marc Zyngier, linux-pci, Jingoo Han,
	Bjorn Helgaas, Leif Lindholm, linux-arm-kernel

On Wed, Feb 19, 2020 at 1:06 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 19 Feb 2020 at 21:24, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> >
> > On Wed, Feb 19, 2020 at 12:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 19 Feb 2020 at 09:11, Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Tue, 18 Feb 2020 11:09:10 -0800
> > > > Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > > >
> > > > > On Sat, Feb 15, 2020 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > > >
> > > > > > On Sat, 15 Feb 2020 09:35:56 +0000,
> > > > > > Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > > >
> > > > > > > (updated some email addresses in cc, including my own)
> > > > > > >
> > > > > > > On Sat, 15 Feb 2020 at 01:54, Alan Mikhak <alan.mikhak@sifive.com> wrote:
> > > > > > > >
> > > > > > > > Hi..
> > > > > > > >
> > > > > > > > What is the right approach for adding MSI support for the generic
> > > > > > > > Linux PCI host driver?
> > > > > > > >
> > > > > > > > I came across this patch which seems to address a similar
> > > > > > > > situation. It seems to have been dropped in v3 of the patchset
> > > > > > > > with the explanation "drop MSI patch [for now], since it
> > > > > > > > turns out we may not need it".
> > > > > > > >
> > > > > > > > [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > > > > https://lore.kernel.org/linux-pci/20170821192907.8695-3-ard.biesheuvel@linaro.org/
> > > > > > > >
> > > > > > > > [PATCH v2 2/3] pci: designware: add separate driver for the MSI part of the RC
> > > > > > > > https://lore.kernel.org/linux-pci/20170824184321.19432-3-ard.biesheuvel@linaro.org/
> > > > > > > >
> > > > > > > > [PATCH v3 0/2] pci: add support for firmware initialized designware RCs
> > > > > > > > https://lore.kernel.org/linux-pci/20170828180437.2646-1-ard.biesheuvel@linaro.org/
> > > > > > > >
> > > > > > >
> > > > > > > For the platform in question, it turned out that we could use the MSI
> > > > > > > block of the core's GIC interrupt controller directly, which is a much
> > > > > > > better solution.
> > > > > > >
> > > > > > > In general, turning MSIs into wired interrupts is not a great idea,
> > > > > > > since the whole point of MSIs is that they are sufficiently similar to
> > > > > > > other DMA transactions to ensure that the interrupt won't arrive
> > > > > > > before the related memory transactions have completed.
> > > > > > >
> > > > > > > If your interrupt controller does not have this capability, then yes,
> > > > > > > you are stuck with this little widget that decodes an inbound write to
> > > > > > > a magic address and turns it into a wired interrupt.
> > > > > >
> > > > > > I can only second this. It is much better to have a generic block
> > > > > > implementing MSI *in a non multiplexed way*, for multiple reasons:
> > > > > >
> > > > > > - the interrupt vs DMA race that Ard mentions above,
> > > > > >
> > > > > > - MSIs are very often used to describe the state of per-CPU queues. If
> > > > > >   you multiplex MSIs behind a single multiplexing interrupt, it is
> > > > > >   always the same CPU that gets interrupted, and you don't benefit
> > > > > >   from having multiple queues at all.
> > > > > >
> > > > > > Even if you have to implement the support as a bunch of wired
> > > > > > interrupts, there is still a lot of value in keeping a 1:1 mapping
> > > > > > between MSIs and wires.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > >         M.
> > > > > >
> > > > > > --
> > > > > > Jazz is not dead, it just smells funny.
> > > > >
> > > > > Ard and Marc, Thanks for you comments. I will take a look at the code
> > > > > related to MSI block of GIC interrupt controller for some reference.
> > > >
> > > > GICv2m or GICv3 MBI are probably your best bets. Don't get anywhere near
> > > > the GICv3 ITS, there lies madness. ;-)
> > > >
> > >
> > > True, but for the record, it is the GICv3 ITS that I used on the
> > > platform in question, allowing me to ignore the pseudo-MSI widget
> > > entirely.
> > >
> > > > > I am looking into supporting MSI in a non-multiplexed way when using
> > > > > ECAM and the generic Linux PCI host driver when Linux is booted
> > > > > from U-Boot.
> > > >
> > > > I don't really get the relationship between ECAM and MSIs. They should
> > > > be fairly independent, unless that has to do with the allowing the MSI
> > > > doorbell to be reached from the PCIe endpoint.
> > > >
> > >
> > > The idea is that the PCIe RC is programmed by firmware, and exposed to
> > > the OS as generic ECAM. If you have enough iATU registers and enough
> > > free address space, that is perfectly feasible.
> > >
> > > The problem is that the generic ECAM binding does not have any
> > > provisions for MSI doorbell widgets that turn inbound writes to a
> > > magic address into a wired interrupt. My patch models this as a
> > > separate device, which allows a generic ECAM DT node to refer to it as
> > > its MSI parent.
> > >
> > >
> > > > > Specifically, what is the right approach for sharing the physical
> > > > > address of the MSI data block used in Linux with U-Boot?
> > > > >
> > > > > I imagine the Linux driver for MSI interrupt controller allocates
> > > > > some DMA-able memory for use as the MSI data block. The
> > > > > U-Boot PCIe driver would program an inbound ATU region to
> > > > > map mem writes from endpoint devices to that MSI data block
> > > > > before booting Linux.
> > > >
> > > > The "MSI block" is really a piece of HW, not memory. So whatever you
> > > > have to program in the PCIe RC must allow an endpoint to reach that
> > > > device with a 32bit write.
> > > >
> > >
> > > Indeed. Either your interrupt controller or your PCIe RC needs to
> > > implement the doorbell, but using the former is by far the preferred
> > > option.
> >
> > Ard and Marc, Thank you so much for your insightful comments.
> >
> > The generic PCI host driver uses ECAM as the access method to
> > read/write PCI configuration registers but has no support for MSI.
> > I imagine I could use the MSI widget model from Art's patch to
> > implement a separate Linux interrupt handler for MSI interrupts.
> >
> > I'm not sure but the MSI widget seems to multiplex MSI interrupts
> > to one wired interrupt since its MSI doorbell is a u32 value. The widget
> > also has code for programming the address of the doorbell into
> > Designware PCIe IP registers.
> >
>
> Indeed. So this is the magic address that will receive special
> treatment from the RC when a device performs DMA to it. Instead of
> relaying the DMA access to memory, it is dropped and instead, the
> wired interrupt is asserted.
>
> > I imagine I would separate the lines of code that programs the
> > PCIe IP MSI registers and move that non-generic PCIe code from
> > Linux to U-Boot "firmware". The MSI interrupt handler would
> > then become more of a generic PCI MSI interrupt handler.
> >
>
> Not sure that could be done generically enough, tbh.

Not sure either now. Not enough to be generic.

>
> > The "MSI block" I refer to is a page of memory that I see being
> > allocated and mapped for dma access from endpoint devices
> > in the Designware PCI host driver function dw_pcie_msi_init().
> > The physical address of this MSI data block is programmed into
> > Designware PCIe IP MSI registers by Designware host driver.
> > I believe this is the target memory where endpoint MSI write
> > requests would be targeted to. I imagine an inbound ATU region
> > maps the bus transaction to a physical address within this MSI
> > data block to support non-multiplexed MSI interrupt handling.
> >
>
> As far as I understand it, the memory allocation is only done to
> ensure that the MSI magic address doesn't shadow a real memory address
> that you'd want to DMA to, given the allocating the page for this
> purpose will ensure that it is not used for anything else.

That is a great clarification. Thanks Ard.

>
> > Whether the doorbell is a u32 value or a block of memory,
> > the chicken or the egg dilemma I have is how to share the
> > address of the MSI data block between Linux and U-Boot.
> > Since all programming code for PCIe IP would reside in the
> > U-Boot PCIe driver, U-Boot would need to know the address
> > of the MSI data block before it boots Linux. However, if the MSI
> > interrupt widget dynamically allocates the MSI data block, it
> > would contain no code to program the address into the PCIe IP.
> >
> > I wonder if the MSI data block can be a reserved block of
> > memory whose physical address is predetermined and shared
> > via the "reg" entry for the MSI widget between Linux and
> > U-Boot? Would that make sense?
> >
>
> The address doesn't really matter. What matters is the fact that you
> need to code service the wired interrupt in response to the MSIs. Even
> if you program the registers from firmware to configure the widget,
> you'll still need something to drive it at runtime.

Thanks Ard, That part is clear. MSI agent running in Linux would
have the code to service the interrupt in response to the MSI.

Regards,
Alan

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-02-19 21:35 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-21 19:29 [PATCH 0/3] pci: add support for firmware initialized designware RCs Ard Biesheuvel
2017-08-21 19:29 ` [PATCH 1/3] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
2017-08-24 16:24   ` kbuild test robot
2017-08-24 16:25   ` kbuild test robot
2017-08-21 19:29 ` [PATCH 2/3] pci: designware: add separate driver for the MSI part of the RC Ard Biesheuvel
2017-08-24 16:42   ` Bjorn Helgaas
2017-08-24 16:43     ` Ard Biesheuvel
2017-08-24 16:48   ` Robin Murphy
2017-08-24 16:50     ` Ard Biesheuvel
2020-02-15  0:54   ` Alan Mikhak
2020-02-15  9:35     ` Ard Biesheuvel
2020-02-15 10:36       ` Marc Zyngier
2020-02-18 19:09         ` Alan Mikhak
2020-02-19  8:11           ` Marc Zyngier
2020-02-19  8:17             ` Ard Biesheuvel
2020-02-19 20:24               ` Alan Mikhak
2020-02-19 21:06                 ` Ard Biesheuvel
2020-02-19 21:35                   ` Alan Mikhak
2017-08-21 19:29 ` [PATCH 3/3] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel
2017-08-24 16:46 ` [PATCH 0/3] pci: add support for firmware initialized designware RCs Bjorn Helgaas
2017-08-31 19:21   ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).