[PATCH 0/2] irqchip/gicv3-its: Handle "msi-map" properties.

[PATCH 0/2] irqchip/gicv3-its: Handle "msi-map" properties.

[David Daney]

From: David Daney <david.daney@cavium.com>

The first patch from Mark Rutland adds the OF device tree binding
description, which explains what we are attempting to do here.  For
MSI messages on GICv3 systems there is some side-band data that
accompanies the message, this data is specified in the OF device tree
"msi-map" property of the PCI host driver.

The second patch parses the required information out of the device tree.

David Daney (1):
  irqchip/gicv3-its:  Handle OF device tree "msi-map" properties.

Mark Rutland (1):
  Docs: dt: Add PCI MSI map bindings

 Documentation/devicetree/bindings/pci/pci-msi.txt | 220 ++++++++++++++++++++++
 drivers/irqchip/irq-gic-v3-its-pci-msi.c          |  73 +++++++
 2 files changed, 293 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

-- 
1.9.1

[PATCH 0/2] irqchip/gicv3-its: Handle "msi-map" properties.

[David Daney]

From: David Daney <david.daney@cavium.com>

The first patch from Mark Rutland adds the OF device tree binding
description, which explains what we are attempting to do here.  For
MSI messages on GICv3 systems there is some side-band data that
accompanies the message, this data is specified in the OF device tree
"msi-map" property of the PCI host driver.

The second patch parses the required information out of the device tree.

David Daney (1):
  irqchip/gicv3-its:  Handle OF device tree "msi-map" properties.

Mark Rutland (1):
  Docs: dt: Add PCI MSI map bindings

 Documentation/devicetree/bindings/pci/pci-msi.txt | 220 ++++++++++++++++++++++
 drivers/irqchip/irq-gic-v3-its-pci-msi.c          |  73 +++++++
 2 files changed, 293 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

-- 
1.9.1

[PATCH 1/2] Docs: dt: Add PCI MSI map bindings

[David Daney]

From: Mark Rutland <mark.rutland@arm.com>

Currently msi-parent is used by a few bindings to describe the
relationship between a PCI root complex and a single MSI controller, but
this property does not have a generic binding document.

Additionally, msi-parent is insufficient to describe more complex
relationships between MSI controllers and devices under a root complex,
where devices may be able to target multiple MSI controllers, or where
MSI controllers use (non-probeable) sideband information to distinguish
devices.

This patch adds a generic binding for mapping PCI devices to MSI
controllers. This document covers msi-parent, and a new msi-map property
(specific to PCI*) which may be used to map devices (identified by their
Requester ID) to sideband data for each MSI controller that they may
target.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 Documentation/devicetree/bindings/pci/pci-msi.txt | 220 ++++++++++++++++++++++
 1 file changed, 220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
new file mode 100644
index 0000000..9b3cc81
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
@@ -0,0 +1,220 @@
+This document describes the generic device tree binding for describing the
+relationship between PCI devices and MSI controllers.
+
+Each PCI device under a root complex is uniquely identified by its Requester ID
+(AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
+Function number.
+
+For the purpose of this document, when treated as a numeric value, a RID is
+formatted such that:
+
+* Bits [15:8] are the Bus number.
+* Bits [7:3] are the Device number.
+* Bits [2:0] are the Function number.
+* Any other bits required for padding must be zero.
+
+MSIs may be distinguished in part through the use of sideband data accompanying
+writes. In the case of PCI devices, this sideband data may be derived from the
+Requester ID. A mechanism is required to associate a device with both the MSI
+controllers it can address, and the sideband data that will be associated with
+its writes to those controllers.
+
+For generic MSI bindings, see
+Documentation/devicetree/bindings/interrupt-controller/msi.txt.
+
+
+PCI root complex
+================
+
+Optional properties
+-------------------
+
+- msi-map: Maps a Requester ID to an MSI controller and associated
+  msi-specifier data. The property is an arbitrary number of tuples of
+  (rid-base,msi-controller,msi-base,length), where:
+
+  * rid-base is a single cell describing the first RID matched by the entry.
+
+  * msi-controller is a single phandle to an MSI controller
+
+  * msi-base is an msi-specifier describing the msi-specifier produced for the
+    first RID matched by the entry.
+
+  * length is a single cell describing how many consecutive RIDs are matched
+    following the rid-base.
+
+  Any RID r in the interval [rid-base, rid-base + length) is associated with
+  the listed msi-controller, with the msi-specifier (r - rid-base + msi-base).
+
+- msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
+  to an msi-specifier per the msi-map property.
+
+- msi-parent: Describes the MSI parent of the root complex itself. Where
+  the root complex and MSI controller do not pass sideband data with MSI
+  writes, this property may be used to describe the MSI controller(s)
+  used by PCI devices under the root complex, if defined as such in the
+  binding for the root complex.
+
+
+Example (1)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, identity-mapped.
+		 */
+		msi-map = <0x0 &msi_a 0x0 0x10000>,
+	};
+};
+
+
+Example (2)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, masked to only the device and function bits.
+		 */
+		msi-map = <0x0 &msi_a 0x0 0x100>,
+		msi-map-mask = <0xff>
+	};
+};
+
+
+Example (3)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, but the high bit of the bus number is
+		 * ignored.
+		 */
+		msi-map = <0x0000 &msi 0x0000 0x8000>,
+			  <0x8000 &msi 0x0000 0x8000>;
+	};
+};
+
+
+Example (4)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, but the high bit of the bus number is
+		 * negated.
+		 */
+		msi-map = <0x0000 &msi 0x8000 0x8000>,
+			  <0x8000 &msi 0x0000 0x8000>;
+	};
+};
+
+
+Example (5)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi_a: msi-controller@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	msi_b: msi-controller@b {
+		reg = <0xb 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	msi_c: msi-controller@c {
+		reg = <0xc 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci@c {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to MSI controller a is the
+		 * RID, but the high bit of the bus number is negated.
+		 * The sideband data provided to MSI controller b is the
+		 * RID, identity-mapped.
+		 * MSI controller c is not addressable.
+		 */
+		msi-map = <0x0000 &msi_a 0x8000 0x08000>,
+			  <0x8000 &msi_a 0x0000 0x08000>,
+			  <0x0000 &msi_b 0x0000 0x10000>;
+	};
+};
-- 
1.9.1

[PATCH 1/2] Docs: dt: Add PCI MSI map bindings

[David Daney]

From: Mark Rutland <mark.rutland@arm.com>

Currently msi-parent is used by a few bindings to describe the
relationship between a PCI root complex and a single MSI controller, but
this property does not have a generic binding document.

Additionally, msi-parent is insufficient to describe more complex
relationships between MSI controllers and devices under a root complex,
where devices may be able to target multiple MSI controllers, or where
MSI controllers use (non-probeable) sideband information to distinguish
devices.

This patch adds a generic binding for mapping PCI devices to MSI
controllers. This document covers msi-parent, and a new msi-map property
(specific to PCI*) which may be used to map devices (identified by their
Requester ID) to sideband data for each MSI controller that they may
target.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 Documentation/devicetree/bindings/pci/pci-msi.txt | 220 ++++++++++++++++++++++
 1 file changed, 220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
new file mode 100644
index 0000000..9b3cc81
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
@@ -0,0 +1,220 @@
+This document describes the generic device tree binding for describing the
+relationship between PCI devices and MSI controllers.
+
+Each PCI device under a root complex is uniquely identified by its Requester ID
+(AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
+Function number.
+
+For the purpose of this document, when treated as a numeric value, a RID is
+formatted such that:
+
+* Bits [15:8] are the Bus number.
+* Bits [7:3] are the Device number.
+* Bits [2:0] are the Function number.
+* Any other bits required for padding must be zero.
+
+MSIs may be distinguished in part through the use of sideband data accompanying
+writes. In the case of PCI devices, this sideband data may be derived from the
+Requester ID. A mechanism is required to associate a device with both the MSI
+controllers it can address, and the sideband data that will be associated with
+its writes to those controllers.
+
+For generic MSI bindings, see
+Documentation/devicetree/bindings/interrupt-controller/msi.txt.
+
+
+PCI root complex
+================
+
+Optional properties
+-------------------
+
+- msi-map: Maps a Requester ID to an MSI controller and associated
+  msi-specifier data. The property is an arbitrary number of tuples of
+  (rid-base,msi-controller,msi-base,length), where:
+
+  * rid-base is a single cell describing the first RID matched by the entry.
+
+  * msi-controller is a single phandle to an MSI controller
+
+  * msi-base is an msi-specifier describing the msi-specifier produced for the
+    first RID matched by the entry.
+
+  * length is a single cell describing how many consecutive RIDs are matched
+    following the rid-base.
+
+  Any RID r in the interval [rid-base, rid-base + length) is associated with
+  the listed msi-controller, with the msi-specifier (r - rid-base + msi-base).
+
+- msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
+  to an msi-specifier per the msi-map property.
+
+- msi-parent: Describes the MSI parent of the root complex itself. Where
+  the root complex and MSI controller do not pass sideband data with MSI
+  writes, this property may be used to describe the MSI controller(s)
+  used by PCI devices under the root complex, if defined as such in the
+  binding for the root complex.
+
+
+Example (1)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci at f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, identity-mapped.
+		 */
+		msi-map = <0x0 &msi_a 0x0 0x10000>,
+	};
+};
+
+
+Example (2)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci at f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, masked to only the device and function bits.
+		 */
+		msi-map = <0x0 &msi_a 0x0 0x100>,
+		msi-map-mask = <0xff>
+	};
+};
+
+
+Example (3)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci at f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, but the high bit of the bus number is
+		 * ignored.
+		 */
+		msi-map = <0x0000 &msi 0x0000 0x8000>,
+			  <0x8000 &msi 0x0000 0x8000>;
+	};
+};
+
+
+Example (4)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi: msi-controller at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci at f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the MSI controller is
+		 * the RID, but the high bit of the bus number is
+		 * negated.
+		 */
+		msi-map = <0x0000 &msi 0x8000 0x8000>,
+			  <0x8000 &msi 0x0000 0x8000>;
+	};
+};
+
+
+Example (5)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	msi_a: msi-controller at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	msi_b: msi-controller at b {
+		reg = <0xb 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	msi_c: msi-controller at c {
+		reg = <0xc 0x1>;
+		compatible = "vendor,some-controller";
+		msi-controller;
+		#msi-cells = <1>;
+	};
+
+	pci: pci at c {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to MSI controller a is the
+		 * RID, but the high bit of the bus number is negated.
+		 * The sideband data provided to MSI controller b is the
+		 * RID, identity-mapped.
+		 * MSI controller c is not addressable.
+		 */
+		msi-map = <0x0000 &msi_a 0x8000 0x08000>,
+			  <0x8000 &msi_a 0x0000 0x08000>,
+			  <0x0000 &msi_b 0x0000 0x10000>;
+	};
+};
-- 
1.9.1

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

From: David Daney <david.daney@cavium.com>

Search up the device hierarchy to find devices with a "msi-map"
property, if found apply the mapping to the GIC device id.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index cf351c6..aa61cef 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 	struct pci_dev *pdev;
 	struct its_pci_alias dev_alias;
 	struct msi_domain_info *msi_info;
+	struct device *parent_dev;
+	struct device_node *msi_controller_node = NULL;
 
 	if (!dev_is_pci(dev))
 		return -EINVAL;
@@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 	dev_alias.count = nvec;
 
 	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
+	/*
+	 * Walk up the device parent links looking for one with a
+	 * "msi-map" property.
+	 */
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
+		u32 msi_mask, masked_devid;
+		u32 rid_base, msi_base, rid_len, phandle;
+		int msi_map_len;
+		const __be32 *msi_map;
+		bool matched;
+
+		if (!parent_dev->of_node)
+			continue;
+
+		msi_map = of_get_property(parent_dev->of_node,
+					  "msi-map", &msi_map_len);
+		if (!msi_map)
+			continue;
+
+		/* The default is to select all bits. */
+		msi_mask = 0xffffffff;
+
+		/*
+		 * Can be overridden by "msi-mask" property.  If
+		 * of_property_read_u32() fails, the default is
+		 * used.
+		 */
+		of_property_read_u32(parent_dev->of_node,
+				     "msi-mask", &msi_mask);
+
+		masked_devid = msi_mask & dev_alias.dev_id;
+		matched = false;
+		while (msi_map_len >= 4 * sizeof(__be32)) {
+			rid_base = be32_to_cpup(msi_map + 0);
+			phandle = be32_to_cpup(msi_map + 1);
+			msi_base = be32_to_cpup(msi_map + 2);
+			rid_len = be32_to_cpup(msi_map + 3);
+
+			if (masked_devid < rid_base ||
+			    masked_devid >= rid_base + rid_len) {
+				msi_map_len -= 4 * sizeof(__be32);
+				msi_map += 4;
+				continue;
+			}
+			matched = true;
+			break;
+		}
+		if (!matched) {
+			dev_err(dev,
+				"No match in \"msi-map\" of %s for dev_id: %x\n",
+				dev_name(parent_dev), dev_alias.dev_id);
+			break;
+		}
+
+		msi_controller_node = of_find_node_by_phandle(phandle);
+		if (domain->of_node != msi_controller_node) {
+			dev_err(dev,
+				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
+				domain->of_node->full_name,
+				msi_controller_node ? NULL : msi_controller_node->full_name);
+			break;
+		}
+		dev_dbg(dev,
+			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
+			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
+			msi_base, rid_len, dev_alias.dev_id);
+		dev_alias.dev_id = masked_devid + msi_base;
+		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
+		break;
+	}
+	of_node_put(msi_controller_node);
 
 	/* ITS specific DeviceID, as the core ITS ignores dev. */
 	info->scratchpad[0].ul = dev_alias.dev_id;
-- 
1.9.1

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

From: David Daney <david.daney@cavium.com>

Search up the device hierarchy to find devices with a "msi-map"
property, if found apply the mapping to the GIC device id.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index cf351c6..aa61cef 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 	struct pci_dev *pdev;
 	struct its_pci_alias dev_alias;
 	struct msi_domain_info *msi_info;
+	struct device *parent_dev;
+	struct device_node *msi_controller_node = NULL;
 
 	if (!dev_is_pci(dev))
 		return -EINVAL;
@@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 	dev_alias.count = nvec;
 
 	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
+	/*
+	 * Walk up the device parent links looking for one with a
+	 * "msi-map" property.
+	 */
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
+		u32 msi_mask, masked_devid;
+		u32 rid_base, msi_base, rid_len, phandle;
+		int msi_map_len;
+		const __be32 *msi_map;
+		bool matched;
+
+		if (!parent_dev->of_node)
+			continue;
+
+		msi_map = of_get_property(parent_dev->of_node,
+					  "msi-map", &msi_map_len);
+		if (!msi_map)
+			continue;
+
+		/* The default is to select all bits. */
+		msi_mask = 0xffffffff;
+
+		/*
+		 * Can be overridden by "msi-mask" property.  If
+		 * of_property_read_u32() fails, the default is
+		 * used.
+		 */
+		of_property_read_u32(parent_dev->of_node,
+				     "msi-mask", &msi_mask);
+
+		masked_devid = msi_mask & dev_alias.dev_id;
+		matched = false;
+		while (msi_map_len >= 4 * sizeof(__be32)) {
+			rid_base = be32_to_cpup(msi_map + 0);
+			phandle = be32_to_cpup(msi_map + 1);
+			msi_base = be32_to_cpup(msi_map + 2);
+			rid_len = be32_to_cpup(msi_map + 3);
+
+			if (masked_devid < rid_base ||
+			    masked_devid >= rid_base + rid_len) {
+				msi_map_len -= 4 * sizeof(__be32);
+				msi_map += 4;
+				continue;
+			}
+			matched = true;
+			break;
+		}
+		if (!matched) {
+			dev_err(dev,
+				"No match in \"msi-map\" of %s for dev_id: %x\n",
+				dev_name(parent_dev), dev_alias.dev_id);
+			break;
+		}
+
+		msi_controller_node = of_find_node_by_phandle(phandle);
+		if (domain->of_node != msi_controller_node) {
+			dev_err(dev,
+				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
+				domain->of_node->full_name,
+				msi_controller_node ? NULL : msi_controller_node->full_name);
+			break;
+		}
+		dev_dbg(dev,
+			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
+			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
+			msi_base, rid_len, dev_alias.dev_id);
+		dev_alias.dev_id = masked_devid + msi_base;
+		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
+		break;
+	}
+	of_node_put(msi_controller_node);
 
 	/* ITS specific DeviceID, as the core ITS ignores dev. */
 	info->scratchpad[0].ul = dev_alias.dev_id;
-- 
1.9.1

Re: [PATCH 1/2] Docs: dt: Add PCI MSI map bindings

[Rob Herring]

On Thu, Sep 17, 2015 at 1:00 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> From: Mark Rutland <mark.rutland@arm.com>
>
> Currently msi-parent is used by a few bindings to describe the
> relationship between a PCI root complex and a single MSI controller, but
> this property does not have a generic binding document.
>
> Additionally, msi-parent is insufficient to describe more complex
> relationships between MSI controllers and devices under a root complex,
> where devices may be able to target multiple MSI controllers, or where
> MSI controllers use (non-probeable) sideband information to distinguish
> devices.
>
> This patch adds a generic binding for mapping PCI devices to MSI
> controllers. This document covers msi-parent, and a new msi-map property
> (specific to PCI*) which may be used to map devices (identified by their
> Requester ID) to sideband data for each MSI controller that they may
> target.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  Documentation/devicetree/bindings/pci/pci-msi.txt | 220 ++++++++++++++++++++++
>  1 file changed, 220 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
> new file mode 100644
> index 0000000..9b3cc81
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
> @@ -0,0 +1,220 @@
> +This document describes the generic device tree binding for describing the
> +relationship between PCI devices and MSI controllers.
> +
> +Each PCI device under a root complex is uniquely identified by its Requester ID
> +(AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
> +Function number.
> +
> +For the purpose of this document, when treated as a numeric value, a RID is
> +formatted such that:
> +
> +* Bits [15:8] are the Bus number.
> +* Bits [7:3] are the Device number.
> +* Bits [2:0] are the Function number.
> +* Any other bits required for padding must be zero.
> +
> +MSIs may be distinguished in part through the use of sideband data accompanying
> +writes. In the case of PCI devices, this sideband data may be derived from the
> +Requester ID. A mechanism is required to associate a device with both the MSI
> +controllers it can address, and the sideband data that will be associated with
> +its writes to those controllers.
> +
> +For generic MSI bindings, see
> +Documentation/devicetree/bindings/interrupt-controller/msi.txt.
> +
> +
> +PCI root complex
> +================
> +
> +Optional properties
> +-------------------
> +
> +- msi-map: Maps a Requester ID to an MSI controller and associated
> +  msi-specifier data. The property is an arbitrary number of tuples of
> +  (rid-base,msi-controller,msi-base,length), where:
> +
> +  * rid-base is a single cell describing the first RID matched by the entry.
> +
> +  * msi-controller is a single phandle to an MSI controller
> +
> +  * msi-base is an msi-specifier describing the msi-specifier produced for the
> +    first RID matched by the entry.
> +
> +  * length is a single cell describing how many consecutive RIDs are matched
> +    following the rid-base.
> +
> +  Any RID r in the interval [rid-base, rid-base + length) is associated with
> +  the listed msi-controller, with the msi-specifier (r - rid-base + msi-base).
> +
> +- msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
> +  to an msi-specifier per the msi-map property.
> +
> +- msi-parent: Describes the MSI parent of the root complex itself. Where
> +  the root complex and MSI controller do not pass sideband data with MSI
> +  writes, this property may be used to describe the MSI controller(s)
> +  used by PCI devices under the root complex, if defined as such in the
> +  binding for the root complex.
> +
> +
> +Example (1)
> +===========
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       msi: msi-controller@a {
> +               reg = <0xa 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       pci: pci@f {
> +               reg = <0xf 0x1>;
> +               compatible = "vendor,pcie-root-complex";
> +               device_type = "pci";
> +
> +               /*
> +                * The sideband data provided to the MSI controller is
> +                * the RID, identity-mapped.
> +                */
> +               msi-map = <0x0 &msi_a 0x0 0x10000>,
> +       };
> +};
> +
> +
> +Example (2)
> +===========
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       msi: msi-controller@a {
> +               reg = <0xa 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       pci: pci@f {
> +               reg = <0xf 0x1>;
> +               compatible = "vendor,pcie-root-complex";
> +               device_type = "pci";
> +
> +               /*
> +                * The sideband data provided to the MSI controller is
> +                * the RID, masked to only the device and function bits.
> +                */
> +               msi-map = <0x0 &msi_a 0x0 0x100>,
> +               msi-map-mask = <0xff>
> +       };
> +};
> +
> +
> +Example (3)
> +===========
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       msi: msi-controller@a {
> +               reg = <0xa 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       pci: pci@f {
> +               reg = <0xf 0x1>;
> +               compatible = "vendor,pcie-root-complex";
> +               device_type = "pci";
> +
> +               /*
> +                * The sideband data provided to the MSI controller is
> +                * the RID, but the high bit of the bus number is
> +                * ignored.
> +                */
> +               msi-map = <0x0000 &msi 0x0000 0x8000>,
> +                         <0x8000 &msi 0x0000 0x8000>;
> +       };
> +};
> +
> +
> +Example (4)
> +===========
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       msi: msi-controller@a {
> +               reg = <0xa 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       pci: pci@f {
> +               reg = <0xf 0x1>;
> +               compatible = "vendor,pcie-root-complex";
> +               device_type = "pci";
> +
> +               /*
> +                * The sideband data provided to the MSI controller is
> +                * the RID, but the high bit of the bus number is
> +                * negated.
> +                */
> +               msi-map = <0x0000 &msi 0x8000 0x8000>,
> +                         <0x8000 &msi 0x0000 0x8000>;
> +       };
> +};
> +
> +
> +Example (5)
> +===========
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       msi_a: msi-controller@a {
> +               reg = <0xa 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       msi_b: msi-controller@b {
> +               reg = <0xb 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       msi_c: msi-controller@c {
> +               reg = <0xc 0x1>;
> +               compatible = "vendor,some-controller";
> +               msi-controller;
> +               #msi-cells = <1>;
> +       };
> +
> +       pci: pci@c {
> +               reg = <0xf 0x1>;
> +               compatible = "vendor,pcie-root-complex";
> +               device_type = "pci";
> +
> +               /*
> +                * The sideband data provided to MSI controller a is the
> +                * RID, but the high bit of the bus number is negated.
> +                * The sideband data provided to MSI controller b is the
> +                * RID, identity-mapped.
> +                * MSI controller c is not addressable.
> +                */
> +               msi-map = <0x0000 &msi_a 0x8000 0x08000>,
> +                         <0x8000 &msi_a 0x0000 0x08000>,
> +                         <0x0000 &msi_b 0x0000 0x10000>;
> +       };
> +};
> --
> 1.9.1
>

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Thu, 17 Sep 2015 11:00:59 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

Hi David,

> From: David Daney <david.daney@cavium.com>
> 
> Search up the device hierarchy to find devices with a "msi-map"
> property, if found apply the mapping to the GIC device id.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index cf351c6..aa61cef 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	struct pci_dev *pdev;
>  	struct its_pci_alias dev_alias;
>  	struct msi_domain_info *msi_info;
> +	struct device *parent_dev;
> +	struct device_node *msi_controller_node = NULL;
>  
>  	if (!dev_is_pci(dev))
>  		return -EINVAL;
> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	dev_alias.count = nvec;
>  
>  	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> +	/*
> +	 * Walk up the device parent links looking for one with a
> +	 * "msi-map" property.
> +	 */

My first objection is the location of this parsing. It shouldn't be
driver specific, but instead be part of the generic OF handling
(nothing in these properties is GICv3 specific, even if the ITS is the
only user so far).

> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {

Is there a limit how far we should go up the parent chain to find a
msi-map? My hunch is that you should stop at the first device that does
have an of_node, as it is the one that should contain the msi-map
property.

> +		u32 msi_mask, masked_devid;
> +		u32 rid_base, msi_base, rid_len, phandle;
> +		int msi_map_len;
> +		const __be32 *msi_map;
> +		bool matched;
> +
> +		if (!parent_dev->of_node)
> +			continue;
> +
> +		msi_map = of_get_property(parent_dev->of_node,
> +					  "msi-map", &msi_map_len);
> +		if (!msi_map)
> +			continue;

At this point, you know you do have a msi-map, and anything below this
point won't result in another iteration - they can be taken out of the
loop, avoiding most of your break statements.

> +
> +		/* The default is to select all bits. */
> +		msi_mask = 0xffffffff;
> +
> +		/*
> +		 * Can be overridden by "msi-mask" property.  If
> +		 * of_property_read_u32() fails, the default is
> +		 * used.
> +		 */
> +		of_property_read_u32(parent_dev->of_node,
> +				     "msi-mask", &msi_mask);

This should be "msi-map-mask", if I read Mark's binding correctly.

> +
> +		masked_devid = msi_mask & dev_alias.dev_id;
> +		matched = false;
> +		while (msi_map_len >= 4 * sizeof(__be32)) {
> +			rid_base = be32_to_cpup(msi_map + 0);
> +			phandle = be32_to_cpup(msi_map + 1);
> +			msi_base = be32_to_cpup(msi_map + 2);
> +			rid_len = be32_to_cpup(msi_map + 3);

Ouch. I wonder if that kind of thing should deserve a generic helper.
of_property_read_u32_array_from_index()? Rob, what do you think?

Also, worth checking that msi_map_len is multiple of 4 (and shout if
it isn't).

> +
> +			if (masked_devid < rid_base ||
> +			    masked_devid >= rid_base + rid_len) {
> +				msi_map_len -= 4 * sizeof(__be32);
> +				msi_map += 4;
> +				continue;
> +			}
> +			matched = true;
> +			break;
> +		}
> +		if (!matched) {
> +			dev_err(dev,
> +				"No match in \"msi-map\" of %s for dev_id: %x\n",
> +				dev_name(parent_dev), dev_alias.dev_id);

It would probably be useful to also print the node containing the
msi-map property, as this is likely to be the source of the problem.

> +			break;
> +		}
> +
> +		msi_controller_node = of_find_node_by_phandle(phandle);
> +		if (domain->of_node != msi_controller_node) {
> +			dev_err(dev,
> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> +				domain->of_node->full_name,
> +				msi_controller_node ? NULL : msi_controller_node->full_name);

Why is that an error? a RC can be configured to master multiple
MSI-controllers, and the kernel picks one of them for a given device.
This is illustrated by "Example (5)" in the binding, where a device can
master two MSI controllers.

> +			break;
> +		}
> +		dev_dbg(dev,
> +			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
> +			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
> +			msi_base, rid_len, dev_alias.dev_id);
> +		dev_alias.dev_id = masked_devid + msi_base;
> +		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
> +		break;
> +	}
> +	of_node_put(msi_controller_node);
>  
>  	/* ITS specific DeviceID, as the core ITS ignores dev. */
>  	info->scratchpad[0].ul = dev_alias.dev_id;


Thanks,

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

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Thu, 17 Sep 2015 11:00:59 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

Hi David,

> From: David Daney <david.daney@cavium.com>
> 
> Search up the device hierarchy to find devices with a "msi-map"
> property, if found apply the mapping to the GIC device id.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index cf351c6..aa61cef 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	struct pci_dev *pdev;
>  	struct its_pci_alias dev_alias;
>  	struct msi_domain_info *msi_info;
> +	struct device *parent_dev;
> +	struct device_node *msi_controller_node = NULL;
>  
>  	if (!dev_is_pci(dev))
>  		return -EINVAL;
> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	dev_alias.count = nvec;
>  
>  	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> +	/*
> +	 * Walk up the device parent links looking for one with a
> +	 * "msi-map" property.
> +	 */

My first objection is the location of this parsing. It shouldn't be
driver specific, but instead be part of the generic OF handling
(nothing in these properties is GICv3 specific, even if the ITS is the
only user so far).

> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {

Is there a limit how far we should go up the parent chain to find a
msi-map? My hunch is that you should stop at the first device that does
have an of_node, as it is the one that should contain the msi-map
property.

> +		u32 msi_mask, masked_devid;
> +		u32 rid_base, msi_base, rid_len, phandle;
> +		int msi_map_len;
> +		const __be32 *msi_map;
> +		bool matched;
> +
> +		if (!parent_dev->of_node)
> +			continue;
> +
> +		msi_map = of_get_property(parent_dev->of_node,
> +					  "msi-map", &msi_map_len);
> +		if (!msi_map)
> +			continue;

At this point, you know you do have a msi-map, and anything below this
point won't result in another iteration - they can be taken out of the
loop, avoiding most of your break statements.

> +
> +		/* The default is to select all bits. */
> +		msi_mask = 0xffffffff;
> +
> +		/*
> +		 * Can be overridden by "msi-mask" property.  If
> +		 * of_property_read_u32() fails, the default is
> +		 * used.
> +		 */
> +		of_property_read_u32(parent_dev->of_node,
> +				     "msi-mask", &msi_mask);

This should be "msi-map-mask", if I read Mark's binding correctly.

> +
> +		masked_devid = msi_mask & dev_alias.dev_id;
> +		matched = false;
> +		while (msi_map_len >= 4 * sizeof(__be32)) {
> +			rid_base = be32_to_cpup(msi_map + 0);
> +			phandle = be32_to_cpup(msi_map + 1);
> +			msi_base = be32_to_cpup(msi_map + 2);
> +			rid_len = be32_to_cpup(msi_map + 3);

Ouch. I wonder if that kind of thing should deserve a generic helper.
of_property_read_u32_array_from_index()? Rob, what do you think?

Also, worth checking that msi_map_len is multiple of 4 (and shout if
it isn't).

> +
> +			if (masked_devid < rid_base ||
> +			    masked_devid >= rid_base + rid_len) {
> +				msi_map_len -= 4 * sizeof(__be32);
> +				msi_map += 4;
> +				continue;
> +			}
> +			matched = true;
> +			break;
> +		}
> +		if (!matched) {
> +			dev_err(dev,
> +				"No match in \"msi-map\" of %s for dev_id: %x\n",
> +				dev_name(parent_dev), dev_alias.dev_id);

It would probably be useful to also print the node containing the
msi-map property, as this is likely to be the source of the problem.

> +			break;
> +		}
> +
> +		msi_controller_node = of_find_node_by_phandle(phandle);
> +		if (domain->of_node != msi_controller_node) {
> +			dev_err(dev,
> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> +				domain->of_node->full_name,
> +				msi_controller_node ? NULL : msi_controller_node->full_name);

Why is that an error? a RC can be configured to master multiple
MSI-controllers, and the kernel picks one of them for a given device.
This is illustrated by "Example (5)" in the binding, where a device can
master two MSI controllers.

> +			break;
> +		}
> +		dev_dbg(dev,
> +			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
> +			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
> +			msi_base, rid_len, dev_alias.dev_id);
> +		dev_alias.dev_id = masked_devid + msi_base;
> +		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
> +		break;
> +	}
> +	of_node_put(msi_controller_node);
>  
>  	/* ITS specific DeviceID, as the core ITS ignores dev. */
>  	info->scratchpad[0].ul = dev_alias.dev_id;


Thanks,

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

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Thu, 17 Sep 2015 11:00:59 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

Hi David,

> From: David Daney <david.daney@cavium.com>
> 
> Search up the device hierarchy to find devices with a "msi-map"
> property, if found apply the mapping to the GIC device id.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index cf351c6..aa61cef 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	struct pci_dev *pdev;
>  	struct its_pci_alias dev_alias;
>  	struct msi_domain_info *msi_info;
> +	struct device *parent_dev;
> +	struct device_node *msi_controller_node = NULL;
>  
>  	if (!dev_is_pci(dev))
>  		return -EINVAL;
> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	dev_alias.count = nvec;
>  
>  	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> +	/*
> +	 * Walk up the device parent links looking for one with a
> +	 * "msi-map" property.
> +	 */

My first objection is the location of this parsing. It shouldn't be
driver specific, but instead be part of the generic OF handling
(nothing in these properties is GICv3 specific, even if the ITS is the
only user so far).

> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {

Is there a limit how far we should go up the parent chain to find a
msi-map? My hunch is that you should stop at the first device that does
have an of_node, as it is the one that should contain the msi-map
property.

> +		u32 msi_mask, masked_devid;
> +		u32 rid_base, msi_base, rid_len, phandle;
> +		int msi_map_len;
> +		const __be32 *msi_map;
> +		bool matched;
> +
> +		if (!parent_dev->of_node)
> +			continue;
> +
> +		msi_map = of_get_property(parent_dev->of_node,
> +					  "msi-map", &msi_map_len);
> +		if (!msi_map)
> +			continue;

At this point, you know you do have a msi-map, and anything below this
point won't result in another iteration - they can be taken out of the
loop, avoiding most of your break statements.

> +
> +		/* The default is to select all bits. */
> +		msi_mask = 0xffffffff;
> +
> +		/*
> +		 * Can be overridden by "msi-mask" property.  If
> +		 * of_property_read_u32() fails, the default is
> +		 * used.
> +		 */
> +		of_property_read_u32(parent_dev->of_node,
> +				     "msi-mask", &msi_mask);

This should be "msi-map-mask", if I read Mark's binding correctly.

> +
> +		masked_devid = msi_mask & dev_alias.dev_id;
> +		matched = false;
> +		while (msi_map_len >= 4 * sizeof(__be32)) {
> +			rid_base = be32_to_cpup(msi_map + 0);
> +			phandle = be32_to_cpup(msi_map + 1);
> +			msi_base = be32_to_cpup(msi_map + 2);
> +			rid_len = be32_to_cpup(msi_map + 3);

Ouch. I wonder if that kind of thing should deserve a generic helper.
of_property_read_u32_array_from_index()? Rob, what do you think?

Also, worth checking that msi_map_len is multiple of 4 (and shout if
it isn't).

> +
> +			if (masked_devid < rid_base ||
> +			    masked_devid >= rid_base + rid_len) {
> +				msi_map_len -= 4 * sizeof(__be32);
> +				msi_map += 4;
> +				continue;
> +			}
> +			matched = true;
> +			break;
> +		}
> +		if (!matched) {
> +			dev_err(dev,
> +				"No match in \"msi-map\" of %s for dev_id: %x\n",
> +				dev_name(parent_dev), dev_alias.dev_id);

It would probably be useful to also print the node containing the
msi-map property, as this is likely to be the source of the problem.

> +			break;
> +		}
> +
> +		msi_controller_node = of_find_node_by_phandle(phandle);
> +		if (domain->of_node != msi_controller_node) {
> +			dev_err(dev,
> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> +				domain->of_node->full_name,
> +				msi_controller_node ? NULL : msi_controller_node->full_name);

Why is that an error? a RC can be configured to master multiple
MSI-controllers, and the kernel picks one of them for a given device.
This is illustrated by "Example (5)" in the binding, where a device can
master two MSI controllers.

> +			break;
> +		}
> +		dev_dbg(dev,
> +			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
> +			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
> +			msi_base, rid_len, dev_alias.dev_id);
> +		dev_alias.dev_id = masked_devid + msi_base;
> +		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
> +		break;
> +	}
> +	of_node_put(msi_controller_node);
>  
>  	/* ITS specific DeviceID, as the core ITS ignores dev. */
>  	info->scratchpad[0].ul = dev_alias.dev_id;


Thanks,

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

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> On Thu, 17 Sep 2015 11:00:59 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
> Hi David,
>
>> From: David Daney <david.daney@cavium.com>
>>
>> Search up the device hierarchy to find devices with a "msi-map"
>> property, if found apply the mapping to the GIC device id.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> index cf351c6..aa61cef 100644
>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	struct pci_dev *pdev;
>>   	struct its_pci_alias dev_alias;
>>   	struct msi_domain_info *msi_info;
>> +	struct device *parent_dev;
>> +	struct device_node *msi_controller_node = NULL;
>>
>>   	if (!dev_is_pci(dev))
>>   		return -EINVAL;
>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	dev_alias.count = nvec;
>>
>>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>> +	/*
>> +	 * Walk up the device parent links looking for one with a
>> +	 * "msi-map" property.
>> +	 */
>
> My first objection is the location of this parsing. It shouldn't be
> driver specific, but instead be part of the generic OF handling
> (nothing in these properties is GICv3 specific, even if the ITS is the
> only user so far).

OK, I agree that this should eventually end up in generic OF handling 
code.  I just wanted to get something out to initiate discussion.

The next patch revision will move this to a more generic home.

>
>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>
> Is there a limit how far we should go up the parent chain to find a
> msi-map? My hunch is that you should stop at the first device that does
> have an of_node, as it is the one that should contain the msi-map
> property.

I think there is the possibility of finding something like a bridge that 
has an of_node, but does not have the "msi-map" property.  I currently 
have exactly this configuration, as some of the on-SoC devices sit 
behind a bridge, but need an of_node to obtain unprobable properties and 
children (the MDIO bus devices are like this).

So if we want to abort the walk early, we should at least go up until we 
find "msi-map" in the of_node.

>
>> +		u32 msi_mask, masked_devid;
>> +		u32 rid_base, msi_base, rid_len, phandle;
>> +		int msi_map_len;
>> +		const __be32 *msi_map;
>> +		bool matched;
>> +
>> +		if (!parent_dev->of_node)
>> +			continue;
>> +
>> +		msi_map = of_get_property(parent_dev->of_node,
>> +					  "msi-map", &msi_map_len);
>> +		if (!msi_map)
>> +			continue;
>
> At this point, you know you do have a msi-map, and anything below this
> point won't result in another iteration - they can be taken out of the
> loop, avoiding most of your break statements.


OK.  I will make this simplification.

>
>> +
>> +		/* The default is to select all bits. */
>> +		msi_mask = 0xffffffff;
>> +
>> +		/*
>> +		 * Can be overridden by "msi-mask" property.  If
>> +		 * of_property_read_u32() fails, the default is
>> +		 * used.
>> +		 */
>> +		of_property_read_u32(parent_dev->of_node,
>> +				     "msi-mask", &msi_mask);
>
> This should be "msi-map-mask", if I read Mark's binding correctly.

Good catch.  It was a typo on my part.  I am using the default, so my 
device tree doesn't have this property.

>
>> +
>> +		masked_devid = msi_mask & dev_alias.dev_id;
>> +		matched = false;
>> +		while (msi_map_len >= 4 * sizeof(__be32)) {
>> +			rid_base = be32_to_cpup(msi_map + 0);
>> +			phandle = be32_to_cpup(msi_map + 1);
>> +			msi_base = be32_to_cpup(msi_map + 2);
>> +			rid_len = be32_to_cpup(msi_map + 3);
>
> Ouch. I wonder if that kind of thing should deserve a generic helper.
> of_property_read_u32_array_from_index()? Rob, what do you think?

I think it is possible to add too many wrapper functions.  IMO, this is 
not too unreadable.

>
> Also, worth checking that msi_map_len is multiple of 4 (and shout if
> it isn't).

I initially had that, but thought that the fact that any trailing short 
entry would result in a non-functional device, so that would be enough. 
  But I will add it back in.


>
>> +
>> +			if (masked_devid < rid_base ||
>> +			    masked_devid >= rid_base + rid_len) {
>> +				msi_map_len -= 4 * sizeof(__be32);
>> +				msi_map += 4;
>> +				continue;
>> +			}
>> +			matched = true;
>> +			break;
>> +		}
>> +		if (!matched) {
>> +			dev_err(dev,
>> +				"No match in \"msi-map\" of %s for dev_id: %x\n",
>> +				dev_name(parent_dev), dev_alias.dev_id);
>
> It would probably be useful to also print the node containing the
> msi-map property, as this is likely to be the source of the problem.

Hmm, I will see what I can add...

>
>> +			break;
>> +		}
>> +
>> +		msi_controller_node = of_find_node_by_phandle(phandle);
>> +		if (domain->of_node != msi_controller_node) {
>> +			dev_err(dev,
>> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
>> +				domain->of_node->full_name,
>> +				msi_controller_node ? NULL : msi_controller_node->full_name);
>
> Why is that an error? a RC can be configured to master multiple
> MSI-controllers,

Something has already associated the PCI device with this 
MSI-controller.  Therefore I think the reference in the map must refer 
to this ITS MSI-controller instance.


> and the kernel picks one of them for a given device.
> This is illustrated by "Example (5)" in the binding, where a device can
> master two MSI controllers.

The PCI host may have many MSI controllers, but I think a given PCI 
device will have only one (based on bus:devfn) that is looked up in the map.

>
>> +			break;
>> +		}
>> +		dev_dbg(dev,
>> +			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
>> +			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
>> +			msi_base, rid_len, dev_alias.dev_id);
>> +		dev_alias.dev_id = masked_devid + msi_base;
>> +		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
>> +		break;
>> +	}
>> +	of_node_put(msi_controller_node);
>>
>>   	/* ITS specific DeviceID, as the core ITS ignores dev. */
>>   	info->scratchpad[0].ul = dev_alias.dev_id;
>
>
> Thanks,
>
> 	M.
>

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> On Thu, 17 Sep 2015 11:00:59 -0700
> David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> Hi David,
>
>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>
>> Search up the device hierarchy to find devices with a "msi-map"
>> property, if found apply the mapping to the GIC device id.
>>
>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> index cf351c6..aa61cef 100644
>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	struct pci_dev *pdev;
>>   	struct its_pci_alias dev_alias;
>>   	struct msi_domain_info *msi_info;
>> +	struct device *parent_dev;
>> +	struct device_node *msi_controller_node = NULL;
>>
>>   	if (!dev_is_pci(dev))
>>   		return -EINVAL;
>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	dev_alias.count = nvec;
>>
>>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>> +	/*
>> +	 * Walk up the device parent links looking for one with a
>> +	 * "msi-map" property.
>> +	 */
>
> My first objection is the location of this parsing. It shouldn't be
> driver specific, but instead be part of the generic OF handling
> (nothing in these properties is GICv3 specific, even if the ITS is the
> only user so far).

OK, I agree that this should eventually end up in generic OF handling 
code.  I just wanted to get something out to initiate discussion.

The next patch revision will move this to a more generic home.

>
>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>
> Is there a limit how far we should go up the parent chain to find a
> msi-map? My hunch is that you should stop at the first device that does
> have an of_node, as it is the one that should contain the msi-map
> property.

I think there is the possibility of finding something like a bridge that 
has an of_node, but does not have the "msi-map" property.  I currently 
have exactly this configuration, as some of the on-SoC devices sit 
behind a bridge, but need an of_node to obtain unprobable properties and 
children (the MDIO bus devices are like this).

So if we want to abort the walk early, we should at least go up until we 
find "msi-map" in the of_node.

>
>> +		u32 msi_mask, masked_devid;
>> +		u32 rid_base, msi_base, rid_len, phandle;
>> +		int msi_map_len;
>> +		const __be32 *msi_map;
>> +		bool matched;
>> +
>> +		if (!parent_dev->of_node)
>> +			continue;
>> +
>> +		msi_map = of_get_property(parent_dev->of_node,
>> +					  "msi-map", &msi_map_len);
>> +		if (!msi_map)
>> +			continue;
>
> At this point, you know you do have a msi-map, and anything below this
> point won't result in another iteration - they can be taken out of the
> loop, avoiding most of your break statements.


OK.  I will make this simplification.

>
>> +
>> +		/* The default is to select all bits. */
>> +		msi_mask = 0xffffffff;
>> +
>> +		/*
>> +		 * Can be overridden by "msi-mask" property.  If
>> +		 * of_property_read_u32() fails, the default is
>> +		 * used.
>> +		 */
>> +		of_property_read_u32(parent_dev->of_node,
>> +				     "msi-mask", &msi_mask);
>
> This should be "msi-map-mask", if I read Mark's binding correctly.

Good catch.  It was a typo on my part.  I am using the default, so my 
device tree doesn't have this property.

>
>> +
>> +		masked_devid = msi_mask & dev_alias.dev_id;
>> +		matched = false;
>> +		while (msi_map_len >= 4 * sizeof(__be32)) {
>> +			rid_base = be32_to_cpup(msi_map + 0);
>> +			phandle = be32_to_cpup(msi_map + 1);
>> +			msi_base = be32_to_cpup(msi_map + 2);
>> +			rid_len = be32_to_cpup(msi_map + 3);
>
> Ouch. I wonder if that kind of thing should deserve a generic helper.
> of_property_read_u32_array_from_index()? Rob, what do you think?

I think it is possible to add too many wrapper functions.  IMO, this is 
not too unreadable.

>
> Also, worth checking that msi_map_len is multiple of 4 (and shout if
> it isn't).

I initially had that, but thought that the fact that any trailing short 
entry would result in a non-functional device, so that would be enough. 
  But I will add it back in.


>
>> +
>> +			if (masked_devid < rid_base ||
>> +			    masked_devid >= rid_base + rid_len) {
>> +				msi_map_len -= 4 * sizeof(__be32);
>> +				msi_map += 4;
>> +				continue;
>> +			}
>> +			matched = true;
>> +			break;
>> +		}
>> +		if (!matched) {
>> +			dev_err(dev,
>> +				"No match in \"msi-map\" of %s for dev_id: %x\n",
>> +				dev_name(parent_dev), dev_alias.dev_id);
>
> It would probably be useful to also print the node containing the
> msi-map property, as this is likely to be the source of the problem.

Hmm, I will see what I can add...

>
>> +			break;
>> +		}
>> +
>> +		msi_controller_node = of_find_node_by_phandle(phandle);
>> +		if (domain->of_node != msi_controller_node) {
>> +			dev_err(dev,
>> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
>> +				domain->of_node->full_name,
>> +				msi_controller_node ? NULL : msi_controller_node->full_name);
>
> Why is that an error? a RC can be configured to master multiple
> MSI-controllers,

Something has already associated the PCI device with this 
MSI-controller.  Therefore I think the reference in the map must refer 
to this ITS MSI-controller instance.


> and the kernel picks one of them for a given device.
> This is illustrated by "Example (5)" in the binding, where a device can
> master two MSI controllers.

The PCI host may have many MSI controllers, but I think a given PCI 
device will have only one (based on bus:devfn) that is looked up in the map.

>
>> +			break;
>> +		}
>> +		dev_dbg(dev,
>> +			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
>> +			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
>> +			msi_base, rid_len, dev_alias.dev_id);
>> +		dev_alias.dev_id = masked_devid + msi_base;
>> +		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
>> +		break;
>> +	}
>> +	of_node_put(msi_controller_node);
>>
>>   	/* ITS specific DeviceID, as the core ITS ignores dev. */
>>   	info->scratchpad[0].ul = dev_alias.dev_id;
>
>
> Thanks,
>
> 	M.
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> On Thu, 17 Sep 2015 11:00:59 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
> Hi David,
>
>> From: David Daney <david.daney@cavium.com>
>>
>> Search up the device hierarchy to find devices with a "msi-map"
>> property, if found apply the mapping to the GIC device id.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> index cf351c6..aa61cef 100644
>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	struct pci_dev *pdev;
>>   	struct its_pci_alias dev_alias;
>>   	struct msi_domain_info *msi_info;
>> +	struct device *parent_dev;
>> +	struct device_node *msi_controller_node = NULL;
>>
>>   	if (!dev_is_pci(dev))
>>   		return -EINVAL;
>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	dev_alias.count = nvec;
>>
>>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>> +	/*
>> +	 * Walk up the device parent links looking for one with a
>> +	 * "msi-map" property.
>> +	 */
>
> My first objection is the location of this parsing. It shouldn't be
> driver specific, but instead be part of the generic OF handling
> (nothing in these properties is GICv3 specific, even if the ITS is the
> only user so far).

OK, I agree that this should eventually end up in generic OF handling 
code.  I just wanted to get something out to initiate discussion.

The next patch revision will move this to a more generic home.

>
>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>
> Is there a limit how far we should go up the parent chain to find a
> msi-map? My hunch is that you should stop at the first device that does
> have an of_node, as it is the one that should contain the msi-map
> property.

I think there is the possibility of finding something like a bridge that 
has an of_node, but does not have the "msi-map" property.  I currently 
have exactly this configuration, as some of the on-SoC devices sit 
behind a bridge, but need an of_node to obtain unprobable properties and 
children (the MDIO bus devices are like this).

So if we want to abort the walk early, we should at least go up until we 
find "msi-map" in the of_node.

>
>> +		u32 msi_mask, masked_devid;
>> +		u32 rid_base, msi_base, rid_len, phandle;
>> +		int msi_map_len;
>> +		const __be32 *msi_map;
>> +		bool matched;
>> +
>> +		if (!parent_dev->of_node)
>> +			continue;
>> +
>> +		msi_map = of_get_property(parent_dev->of_node,
>> +					  "msi-map", &msi_map_len);
>> +		if (!msi_map)
>> +			continue;
>
> At this point, you know you do have a msi-map, and anything below this
> point won't result in another iteration - they can be taken out of the
> loop, avoiding most of your break statements.


OK.  I will make this simplification.

>
>> +
>> +		/* The default is to select all bits. */
>> +		msi_mask = 0xffffffff;
>> +
>> +		/*
>> +		 * Can be overridden by "msi-mask" property.  If
>> +		 * of_property_read_u32() fails, the default is
>> +		 * used.
>> +		 */
>> +		of_property_read_u32(parent_dev->of_node,
>> +				     "msi-mask", &msi_mask);
>
> This should be "msi-map-mask", if I read Mark's binding correctly.

Good catch.  It was a typo on my part.  I am using the default, so my 
device tree doesn't have this property.

>
>> +
>> +		masked_devid = msi_mask & dev_alias.dev_id;
>> +		matched = false;
>> +		while (msi_map_len >= 4 * sizeof(__be32)) {
>> +			rid_base = be32_to_cpup(msi_map + 0);
>> +			phandle = be32_to_cpup(msi_map + 1);
>> +			msi_base = be32_to_cpup(msi_map + 2);
>> +			rid_len = be32_to_cpup(msi_map + 3);
>
> Ouch. I wonder if that kind of thing should deserve a generic helper.
> of_property_read_u32_array_from_index()? Rob, what do you think?

I think it is possible to add too many wrapper functions.  IMO, this is 
not too unreadable.

>
> Also, worth checking that msi_map_len is multiple of 4 (and shout if
> it isn't).

I initially had that, but thought that the fact that any trailing short 
entry would result in a non-functional device, so that would be enough. 
  But I will add it back in.


>
>> +
>> +			if (masked_devid < rid_base ||
>> +			    masked_devid >= rid_base + rid_len) {
>> +				msi_map_len -= 4 * sizeof(__be32);
>> +				msi_map += 4;
>> +				continue;
>> +			}
>> +			matched = true;
>> +			break;
>> +		}
>> +		if (!matched) {
>> +			dev_err(dev,
>> +				"No match in \"msi-map\" of %s for dev_id: %x\n",
>> +				dev_name(parent_dev), dev_alias.dev_id);
>
> It would probably be useful to also print the node containing the
> msi-map property, as this is likely to be the source of the problem.

Hmm, I will see what I can add...

>
>> +			break;
>> +		}
>> +
>> +		msi_controller_node = of_find_node_by_phandle(phandle);
>> +		if (domain->of_node != msi_controller_node) {
>> +			dev_err(dev,
>> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
>> +				domain->of_node->full_name,
>> +				msi_controller_node ? NULL : msi_controller_node->full_name);
>
> Why is that an error? a RC can be configured to master multiple
> MSI-controllers,

Something has already associated the PCI device with this 
MSI-controller.  Therefore I think the reference in the map must refer 
to this ITS MSI-controller instance.


> and the kernel picks one of them for a given device.
> This is illustrated by "Example (5)" in the binding, where a device can
> master two MSI controllers.

The PCI host may have many MSI controllers, but I think a given PCI 
device will have only one (based on bus:devfn) that is looked up in the map.

>
>> +			break;
>> +		}
>> +		dev_dbg(dev,
>> +			"msi-map at: %s, len: %d, using mask %08x, rid: %08x, msi: %08x, rid_len: %08x, dev_id: %08x\n",
>> +			dev_name(parent_dev), msi_map_len, msi_mask, rid_base,
>> +			msi_base, rid_len, dev_alias.dev_id);
>> +		dev_alias.dev_id = masked_devid + msi_base;
>> +		dev_dbg(dev, "New dev_id: %08x\n", dev_alias.dev_id);
>> +		break;
>> +	}
>> +	of_node_put(msi_controller_node);
>>
>>   	/* ITS specific DeviceID, as the core ITS ignores dev. */
>>   	info->scratchpad[0].ul = dev_alias.dev_id;
>
>
> Thanks,
>
> 	M.
>

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Rob Herring]

On Fri, Sep 18, 2015 at 12:54 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
>>
>> On Thu, 17 Sep 2015 11:00:59 -0700
>> David Daney <ddaney.cavm@gmail.com> wrote:
>>
>> Hi David,
>>
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> Search up the device hierarchy to find devices with a "msi-map"
>>> property, if found apply the mapping to the GIC device id.

[...]

>>> +               masked_devid = msi_mask & dev_alias.dev_id;
>>> +               matched = false;
>>> +               while (msi_map_len >= 4 * sizeof(__be32)) {
>>> +                       rid_base = be32_to_cpup(msi_map + 0);
>>> +                       phandle = be32_to_cpup(msi_map + 1);
>>> +                       msi_base = be32_to_cpup(msi_map + 2);
>>> +                       rid_len = be32_to_cpup(msi_map + 3);
>>
>>
>> Ouch. I wonder if that kind of thing should deserve a generic helper.
>> of_property_read_u32_array_from_index()? Rob, what do you think?
>
>
> I think it is possible to add too many wrapper functions.  IMO, this is not
> too unreadable.

Given you are not reading into an array, I don't think a new helper
would help. You could just use of_property_read_u32_index though.

Rob

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Rob Herring]

On Fri, Sep 18, 2015 at 12:54 PM, David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org> wrote:
> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
>>
>> On Thu, 17 Sep 2015 11:00:59 -0700
>> David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> Hi David,
>>
>>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>
>>> Search up the device hierarchy to find devices with a "msi-map"
>>> property, if found apply the mapping to the GIC device id.

[...]

>>> +               masked_devid = msi_mask & dev_alias.dev_id;
>>> +               matched = false;
>>> +               while (msi_map_len >= 4 * sizeof(__be32)) {
>>> +                       rid_base = be32_to_cpup(msi_map + 0);
>>> +                       phandle = be32_to_cpup(msi_map + 1);
>>> +                       msi_base = be32_to_cpup(msi_map + 2);
>>> +                       rid_len = be32_to_cpup(msi_map + 3);
>>
>>
>> Ouch. I wonder if that kind of thing should deserve a generic helper.
>> of_property_read_u32_array_from_index()? Rob, what do you think?
>
>
> I think it is possible to add too many wrapper functions.  IMO, this is not
> too unreadable.

Given you are not reading into an array, I don't think a new helper
would help. You could just use of_property_read_u32_index though.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Rob Herring]

On Fri, Sep 18, 2015 at 12:54 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
>>
>> On Thu, 17 Sep 2015 11:00:59 -0700
>> David Daney <ddaney.cavm@gmail.com> wrote:
>>
>> Hi David,
>>
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> Search up the device hierarchy to find devices with a "msi-map"
>>> property, if found apply the mapping to the GIC device id.

[...]

>>> +               masked_devid = msi_mask & dev_alias.dev_id;
>>> +               matched = false;
>>> +               while (msi_map_len >= 4 * sizeof(__be32)) {
>>> +                       rid_base = be32_to_cpup(msi_map + 0);
>>> +                       phandle = be32_to_cpup(msi_map + 1);
>>> +                       msi_base = be32_to_cpup(msi_map + 2);
>>> +                       rid_len = be32_to_cpup(msi_map + 3);
>>
>>
>> Ouch. I wonder if that kind of thing should deserve a generic helper.
>> of_property_read_u32_array_from_index()? Rob, what do you think?
>
>
> I think it is possible to add too many wrapper functions.  IMO, this is not
> too unreadable.

Given you are not reading into an array, I don't think a new helper
would help. You could just use of_property_read_u32_index though.

Rob

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Fri, 18 Sep 2015 10:54:02 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> > On Thu, 17 Sep 2015 11:00:59 -0700
> > David Daney <ddaney.cavm@gmail.com> wrote:
> >
> > Hi David,
> >
> >> From: David Daney <david.daney@cavium.com>
> >>
> >> Search up the device hierarchy to find devices with a "msi-map"
> >> property, if found apply the mapping to the GIC device id.
> >>
> >> Signed-off-by: David Daney <david.daney@cavium.com>
> >> ---
> >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>   1 file changed, 73 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> index cf351c6..aa61cef 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	struct pci_dev *pdev;
> >>   	struct its_pci_alias dev_alias;
> >>   	struct msi_domain_info *msi_info;
> >> +	struct device *parent_dev;
> >> +	struct device_node *msi_controller_node = NULL;
> >>
> >>   	if (!dev_is_pci(dev))
> >>   		return -EINVAL;
> >> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	dev_alias.count = nvec;
> >>
> >>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >> +	/*
> >> +	 * Walk up the device parent links looking for one with a
> >> +	 * "msi-map" property.
> >> +	 */
> >
> > My first objection is the location of this parsing. It shouldn't be
> > driver specific, but instead be part of the generic OF handling
> > (nothing in these properties is GICv3 specific, even if the ITS is the
> > only user so far).
> 
> OK, I agree that this should eventually end up in generic OF handling 
> code.  I just wanted to get something out to initiate discussion.
> 
> The next patch revision will move this to a more generic home.
> 
> >
> >> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >
> > Is there a limit how far we should go up the parent chain to find a
> > msi-map? My hunch is that you should stop at the first device that does
> > have an of_node, as it is the one that should contain the msi-map
> > property.
> 
> I think there is the possibility of finding something like a bridge that 
> has an of_node, but does not have the "msi-map" property.  I currently 
> have exactly this configuration, as some of the on-SoC devices sit 
> behind a bridge, but need an of_node to obtain unprobable properties and 
> children (the MDIO bus devices are like this).
> 
> So if we want to abort the walk early, we should at least go up until we 
> find "msi-map" in the of_node.

I don't really see a case where we would traverse a series of nodes
where the msi-map property wouldn't be in the first node. Could you
please give me an example?

[...]

> >> +		msi_controller_node = of_find_node_by_phandle(phandle);
> >> +		if (domain->of_node != msi_controller_node) {
> >> +			dev_err(dev,
> >> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> >> +				domain->of_node->full_name,
> >> +				msi_controller_node ? NULL : msi_controller_node->full_name);
> >
> > Why is that an error? a RC can be configured to master multiple
> > MSI-controllers,
> 
> Something has already associated the PCI device with this 
> MSI-controller.  Therefore I think the reference in the map must refer 
> to this ITS MSI-controller instance.
> 
> 
> > and the kernel picks one of them for a given device.
> > This is illustrated by "Example (5)" in the binding, where a device can
> > master two MSI controllers.
> 
> The PCI host may have many MSI controllers, but I think a given PCI 
> device will have only one (based on bus:devfn) that is looked up in the map.

A PCI device will only be configured to talk to a single MSI
controller, but here you stop parsing the msi-map on the first match,
and assume that you must have found the right MSI controller:

I think this should read:

+			if (masked_devid < rid_base ||
+			    masked_devid >= rid_base + rid_len ||
			    domain->of_node != of_find_node_by_phandle(phandle)) {
+				msi_map_len -= 4 * sizeof(__be32);
+				msi_map += 4;
+				continue;
+			}
+			matched = true;
+			break;

Thanks,

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

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Fri, 18 Sep 2015 10:54:02 -0700
David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org> wrote:

> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> > On Thu, 17 Sep 2015 11:00:59 -0700
> > David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > Hi David,
> >
> >> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >>
> >> Search up the device hierarchy to find devices with a "msi-map"
> >> property, if found apply the mapping to the GIC device id.
> >>
> >> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >> ---
> >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>   1 file changed, 73 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> index cf351c6..aa61cef 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	struct pci_dev *pdev;
> >>   	struct its_pci_alias dev_alias;
> >>   	struct msi_domain_info *msi_info;
> >> +	struct device *parent_dev;
> >> +	struct device_node *msi_controller_node = NULL;
> >>
> >>   	if (!dev_is_pci(dev))
> >>   		return -EINVAL;
> >> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	dev_alias.count = nvec;
> >>
> >>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >> +	/*
> >> +	 * Walk up the device parent links looking for one with a
> >> +	 * "msi-map" property.
> >> +	 */
> >
> > My first objection is the location of this parsing. It shouldn't be
> > driver specific, but instead be part of the generic OF handling
> > (nothing in these properties is GICv3 specific, even if the ITS is the
> > only user so far).
> 
> OK, I agree that this should eventually end up in generic OF handling 
> code.  I just wanted to get something out to initiate discussion.
> 
> The next patch revision will move this to a more generic home.
> 
> >
> >> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >
> > Is there a limit how far we should go up the parent chain to find a
> > msi-map? My hunch is that you should stop at the first device that does
> > have an of_node, as it is the one that should contain the msi-map
> > property.
> 
> I think there is the possibility of finding something like a bridge that 
> has an of_node, but does not have the "msi-map" property.  I currently 
> have exactly this configuration, as some of the on-SoC devices sit 
> behind a bridge, but need an of_node to obtain unprobable properties and 
> children (the MDIO bus devices are like this).
> 
> So if we want to abort the walk early, we should at least go up until we 
> find "msi-map" in the of_node.

I don't really see a case where we would traverse a series of nodes
where the msi-map property wouldn't be in the first node. Could you
please give me an example?

[...]

> >> +		msi_controller_node = of_find_node_by_phandle(phandle);
> >> +		if (domain->of_node != msi_controller_node) {
> >> +			dev_err(dev,
> >> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> >> +				domain->of_node->full_name,
> >> +				msi_controller_node ? NULL : msi_controller_node->full_name);
> >
> > Why is that an error? a RC can be configured to master multiple
> > MSI-controllers,
> 
> Something has already associated the PCI device with this 
> MSI-controller.  Therefore I think the reference in the map must refer 
> to this ITS MSI-controller instance.
> 
> 
> > and the kernel picks one of them for a given device.
> > This is illustrated by "Example (5)" in the binding, where a device can
> > master two MSI controllers.
> 
> The PCI host may have many MSI controllers, but I think a given PCI 
> device will have only one (based on bus:devfn) that is looked up in the map.

A PCI device will only be configured to talk to a single MSI
controller, but here you stop parsing the msi-map on the first match,
and assume that you must have found the right MSI controller:

I think this should read:

+			if (masked_devid < rid_base ||
+			    masked_devid >= rid_base + rid_len ||
			    domain->of_node != of_find_node_by_phandle(phandle)) {
+				msi_map_len -= 4 * sizeof(__be32);
+				msi_map += 4;
+				continue;
+			}
+			matched = true;
+			break;

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Fri, 18 Sep 2015 10:54:02 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> > On Thu, 17 Sep 2015 11:00:59 -0700
> > David Daney <ddaney.cavm@gmail.com> wrote:
> >
> > Hi David,
> >
> >> From: David Daney <david.daney@cavium.com>
> >>
> >> Search up the device hierarchy to find devices with a "msi-map"
> >> property, if found apply the mapping to the GIC device id.
> >>
> >> Signed-off-by: David Daney <david.daney@cavium.com>
> >> ---
> >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>   1 file changed, 73 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> index cf351c6..aa61cef 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	struct pci_dev *pdev;
> >>   	struct its_pci_alias dev_alias;
> >>   	struct msi_domain_info *msi_info;
> >> +	struct device *parent_dev;
> >> +	struct device_node *msi_controller_node = NULL;
> >>
> >>   	if (!dev_is_pci(dev))
> >>   		return -EINVAL;
> >> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	dev_alias.count = nvec;
> >>
> >>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >> +	/*
> >> +	 * Walk up the device parent links looking for one with a
> >> +	 * "msi-map" property.
> >> +	 */
> >
> > My first objection is the location of this parsing. It shouldn't be
> > driver specific, but instead be part of the generic OF handling
> > (nothing in these properties is GICv3 specific, even if the ITS is the
> > only user so far).
> 
> OK, I agree that this should eventually end up in generic OF handling 
> code.  I just wanted to get something out to initiate discussion.
> 
> The next patch revision will move this to a more generic home.
> 
> >
> >> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >
> > Is there a limit how far we should go up the parent chain to find a
> > msi-map? My hunch is that you should stop at the first device that does
> > have an of_node, as it is the one that should contain the msi-map
> > property.
> 
> I think there is the possibility of finding something like a bridge that 
> has an of_node, but does not have the "msi-map" property.  I currently 
> have exactly this configuration, as some of the on-SoC devices sit 
> behind a bridge, but need an of_node to obtain unprobable properties and 
> children (the MDIO bus devices are like this).
> 
> So if we want to abort the walk early, we should at least go up until we 
> find "msi-map" in the of_node.

I don't really see a case where we would traverse a series of nodes
where the msi-map property wouldn't be in the first node. Could you
please give me an example?

[...]

> >> +		msi_controller_node = of_find_node_by_phandle(phandle);
> >> +		if (domain->of_node != msi_controller_node) {
> >> +			dev_err(dev,
> >> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> >> +				domain->of_node->full_name,
> >> +				msi_controller_node ? NULL : msi_controller_node->full_name);
> >
> > Why is that an error? a RC can be configured to master multiple
> > MSI-controllers,
> 
> Something has already associated the PCI device with this 
> MSI-controller.  Therefore I think the reference in the map must refer 
> to this ITS MSI-controller instance.
> 
> 
> > and the kernel picks one of them for a given device.
> > This is illustrated by "Example (5)" in the binding, where a device can
> > master two MSI controllers.
> 
> The PCI host may have many MSI controllers, but I think a given PCI 
> device will have only one (based on bus:devfn) that is looked up in the map.

A PCI device will only be configured to talk to a single MSI
controller, but here you stop parsing the msi-map on the first match,
and assume that you must have found the right MSI controller:

I think this should read:

+			if (masked_devid < rid_base ||
+			    masked_devid >= rid_base + rid_len ||
			    domain->of_node != of_find_node_by_phandle(phandle)) {
+				msi_map_len -= 4 * sizeof(__be32);
+				msi_map += 4;
+				continue;
+			}
+			matched = true;
+			break;

Thanks,

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

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

On 09/21/2015 08:58 AM, Marc Zyngier wrote:
> On Fri, 18 Sep 2015 10:54:02 -0700
> David Daney <ddaney@caviumnetworks.com> wrote:
>
>> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
>>> On Thu, 17 Sep 2015 11:00:59 -0700
>>> David Daney <ddaney.cavm@gmail.com> wrote:
>>>
>>> Hi David,
>>>
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> Search up the device hierarchy to find devices with a "msi-map"
>>>> property, if found apply the mapping to the GIC device id.
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>> ---
>>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> index cf351c6..aa61cef 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>    	struct pci_dev *pdev;
>>>>    	struct its_pci_alias dev_alias;
>>>>    	struct msi_domain_info *msi_info;
>>>> +	struct device *parent_dev;
>>>> +	struct device_node *msi_controller_node = NULL;
>>>>
>>>>    	if (!dev_is_pci(dev))
>>>>    		return -EINVAL;
>>>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>    	dev_alias.count = nvec;
>>>>
>>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>>>> +	/*
>>>> +	 * Walk up the device parent links looking for one with a
>>>> +	 * "msi-map" property.
>>>> +	 */
>>>
>>> My first objection is the location of this parsing. It shouldn't be
>>> driver specific, but instead be part of the generic OF handling
>>> (nothing in these properties is GICv3 specific, even if the ITS is the
>>> only user so far).
>>
>> OK, I agree that this should eventually end up in generic OF handling
>> code.  I just wanted to get something out to initiate discussion.
>>
>> The next patch revision will move this to a more generic home.
>>
>>>
>>>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>>>
>>> Is there a limit how far we should go up the parent chain to find a
>>> msi-map? My hunch is that you should stop at the first device that does
>>> have an of_node, as it is the one that should contain the msi-map
>>> property.
>>
>> I think there is the possibility of finding something like a bridge that
>> has an of_node, but does not have the "msi-map" property.  I currently
>> have exactly this configuration, as some of the on-SoC devices sit
>> behind a bridge, but need an of_node to obtain unprobable properties and
>> children (the MDIO bus devices are like this).
>>
>> So if we want to abort the walk early, we should at least go up until we
>> find "msi-map" in the of_node.
>
> I don't really see a case where we would traverse a series of nodes
> where the msi-map property wouldn't be in the first node. Could you
> please give me an example?
>

OK, how about this:

	pcie0: pcie0@8480,00000000 {
		compatible = "pci-host-ecam-generic";
		device_type = "pci";
		msi-parent = <&its>;
		msi-map = <0 &its 0x80000 0x10000>;
		bus-range = <0 255>;
		#size-cells = <2>;
		#address-cells = <3>;
		#stream-id-cells = <1>;
		reg = <0x8480 0x00000000 0 0x10000000>;	 /* Configuration space */
		ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x070 
0x00000000>, /* mem ranges */
			 <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x020 0x00000000>;

		/* Other devices that use MSI, like USB xHCI, are here
		on the same bus as the bridge.  They have no firmware
		node as sufficient information can be probed as part
		of normal PCI probing. */

		mrml-bridge0@1,0 {
			compatible = "cavium,thunder-8890-mrml-bridge";
			#size-cells = <2>;
			#address-cells = <3>;
			ranges = <0x03000000 0x87e0 0x00000000 0x03000000 0x87e0 0x00000000 
0x10 0x00000000>;
			reg = <0x0800 0 0 0 0>; /* DEVFN = 0x08 (1:0) */

			mdio-nexus@1,3 {
				compatible = "cavium,thunder-8890-mdio-nexus";
				#address-cells = <2>;
				#size-cells = <2>;
				reg = <0x0b00 0 0 0 0>; /* DEVFN = 0x0b (1:3) */
				assigned-addresses = <0x03000000 0x87e0 0x05000000 0x0 0x800000>;
				ranges = <0x87e0 0x05000000 0x03000000 0x87e0 0x05000000 0x0 0x800000>;

				mdio0@87e0,05003800 {
					compatible = "cavium,thunder-8890-mdio";
					#address-cells = <1>;
					#size-cells = <0>;
					reg = <0x87e0 0x05003800 0x0 0x30>;

					sgmii00: sgmii00 {
						reg = <0> ;
						compatible = "marvell,88e1240", "ethernet-phy-ieee802.3-c22";
					};
				};
			};
			bgx0 {
				#address-cells = <1>;
				#size-cells = <0>;
				reg = <0x8000 0 0 0 0>; /* DEVFN = 0x80 (16:0) */
				sgmii00 {
					reg = <0>;
					phy-handle = <&sgmii00>;
				};

			};

		};


The "msi-map" is specified in the PICe host controller node, but there
is a bridge between the device generating interrupts "bgx0" and the
host controller.


> [...]
>
>>>> +		msi_controller_node = of_find_node_by_phandle(phandle);
>>>> +		if (domain->of_node != msi_controller_node) {
>>>> +			dev_err(dev,
>>>> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
>>>> +				domain->of_node->full_name,
>>>> +				msi_controller_node ? NULL : msi_controller_node->full_name);
>>>
>>> Why is that an error? a RC can be configured to master multiple
>>> MSI-controllers,
>>
>> Something has already associated the PCI device with this
>> MSI-controller.  Therefore I think the reference in the map must refer
>> to this ITS MSI-controller instance.
>>
>>
>>> and the kernel picks one of them for a given device.
>>> This is illustrated by "Example (5)" in the binding, where a device can
>>> master two MSI controllers.
>>
>> The PCI host may have many MSI controllers, but I think a given PCI
>> device will have only one (based on bus:devfn) that is looked up in the map.
>
> A PCI device will only be configured to talk to a single MSI
> controller, but here you stop parsing the msi-map on the first match,
> and assume that you must have found the right MSI controller:
>
> I think this should read:
>
> +			if (masked_devid < rid_base ||
> +			    masked_devid >= rid_base + rid_len ||
> 			    domain->of_node != of_find_node_by_phandle(phandle)) {
> +				msi_map_len -= 4 * sizeof(__be32);
> +				msi_map += 4;
> +				continue;
> +			}
> +			matched = true;
> +			break;
>

Good, I will incorporate that too.

In practice, I don't know if we would ever find a system with multiple 
"msi-map" on a path from the device to the root, but we should probably 
attempt to handle it "just in case".


> Thanks,
>
> 	M.
>

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

On 09/21/2015 08:58 AM, Marc Zyngier wrote:
> On Fri, 18 Sep 2015 10:54:02 -0700
> David Daney <ddaney@caviumnetworks.com> wrote:
>
>> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
>>> On Thu, 17 Sep 2015 11:00:59 -0700
>>> David Daney <ddaney.cavm@gmail.com> wrote:
>>>
>>> Hi David,
>>>
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> Search up the device hierarchy to find devices with a "msi-map"
>>>> property, if found apply the mapping to the GIC device id.
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>> ---
>>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> index cf351c6..aa61cef 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>    	struct pci_dev *pdev;
>>>>    	struct its_pci_alias dev_alias;
>>>>    	struct msi_domain_info *msi_info;
>>>> +	struct device *parent_dev;
>>>> +	struct device_node *msi_controller_node = NULL;
>>>>
>>>>    	if (!dev_is_pci(dev))
>>>>    		return -EINVAL;
>>>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>    	dev_alias.count = nvec;
>>>>
>>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>>>> +	/*
>>>> +	 * Walk up the device parent links looking for one with a
>>>> +	 * "msi-map" property.
>>>> +	 */
>>>
>>> My first objection is the location of this parsing. It shouldn't be
>>> driver specific, but instead be part of the generic OF handling
>>> (nothing in these properties is GICv3 specific, even if the ITS is the
>>> only user so far).
>>
>> OK, I agree that this should eventually end up in generic OF handling
>> code.  I just wanted to get something out to initiate discussion.
>>
>> The next patch revision will move this to a more generic home.
>>
>>>
>>>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>>>
>>> Is there a limit how far we should go up the parent chain to find a
>>> msi-map? My hunch is that you should stop at the first device that does
>>> have an of_node, as it is the one that should contain the msi-map
>>> property.
>>
>> I think there is the possibility of finding something like a bridge that
>> has an of_node, but does not have the "msi-map" property.  I currently
>> have exactly this configuration, as some of the on-SoC devices sit
>> behind a bridge, but need an of_node to obtain unprobable properties and
>> children (the MDIO bus devices are like this).
>>
>> So if we want to abort the walk early, we should at least go up until we
>> find "msi-map" in the of_node.
>
> I don't really see a case where we would traverse a series of nodes
> where the msi-map property wouldn't be in the first node. Could you
> please give me an example?
>

OK, how about this:

	pcie0: pcie0@8480,00000000 {
		compatible = "pci-host-ecam-generic";
		device_type = "pci";
		msi-parent = <&its>;
		msi-map = <0 &its 0x80000 0x10000>;
		bus-range = <0 255>;
		#size-cells = <2>;
		#address-cells = <3>;
		#stream-id-cells = <1>;
		reg = <0x8480 0x00000000 0 0x10000000>;	 /* Configuration space */
		ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x070 
0x00000000>, /* mem ranges */
			 <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x020 0x00000000>;

		/* Other devices that use MSI, like USB xHCI, are here
		on the same bus as the bridge.  They have no firmware
		node as sufficient information can be probed as part
		of normal PCI probing. */

		mrml-bridge0@1,0 {
			compatible = "cavium,thunder-8890-mrml-bridge";
			#size-cells = <2>;
			#address-cells = <3>;
			ranges = <0x03000000 0x87e0 0x00000000 0x03000000 0x87e0 0x00000000 
0x10 0x00000000>;
			reg = <0x0800 0 0 0 0>; /* DEVFN = 0x08 (1:0) */

			mdio-nexus@1,3 {
				compatible = "cavium,thunder-8890-mdio-nexus";
				#address-cells = <2>;
				#size-cells = <2>;
				reg = <0x0b00 0 0 0 0>; /* DEVFN = 0x0b (1:3) */
				assigned-addresses = <0x03000000 0x87e0 0x05000000 0x0 0x800000>;
				ranges = <0x87e0 0x05000000 0x03000000 0x87e0 0x05000000 0x0 0x800000>;

				mdio0@87e0,05003800 {
					compatible = "cavium,thunder-8890-mdio";
					#address-cells = <1>;
					#size-cells = <0>;
					reg = <0x87e0 0x05003800 0x0 0x30>;

					sgmii00: sgmii00 {
						reg = <0> ;
						compatible = "marvell,88e1240", "ethernet-phy-ieee802.3-c22";
					};
				};
			};
			bgx0 {
				#address-cells = <1>;
				#size-cells = <0>;
				reg = <0x8000 0 0 0 0>; /* DEVFN = 0x80 (16:0) */
				sgmii00 {
					reg = <0>;
					phy-handle = <&sgmii00>;
				};

			};

		};


The "msi-map" is specified in the PICe host controller node, but there
is a bridge between the device generating interrupts "bgx0" and the
host controller.


> [...]
>
>>>> +		msi_controller_node = of_find_node_by_phandle(phandle);
>>>> +		if (domain->of_node != msi_controller_node) {
>>>> +			dev_err(dev,
>>>> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
>>>> +				domain->of_node->full_name,
>>>> +				msi_controller_node ? NULL : msi_controller_node->full_name);
>>>
>>> Why is that an error? a RC can be configured to master multiple
>>> MSI-controllers,
>>
>> Something has already associated the PCI device with this
>> MSI-controller.  Therefore I think the reference in the map must refer
>> to this ITS MSI-controller instance.
>>
>>
>>> and the kernel picks one of them for a given device.
>>> This is illustrated by "Example (5)" in the binding, where a device can
>>> master two MSI controllers.
>>
>> The PCI host may have many MSI controllers, but I think a given PCI
>> device will have only one (based on bus:devfn) that is looked up in the map.
>
> A PCI device will only be configured to talk to a single MSI
> controller, but here you stop parsing the msi-map on the first match,
> and assume that you must have found the right MSI controller:
>
> I think this should read:
>
> +			if (masked_devid < rid_base ||
> +			    masked_devid >= rid_base + rid_len ||
> 			    domain->of_node != of_find_node_by_phandle(phandle)) {
> +				msi_map_len -= 4 * sizeof(__be32);
> +				msi_map += 4;
> +				continue;
> +			}
> +			matched = true;
> +			break;
>

Good, I will incorporate that too.

In practice, I don't know if we would ever find a system with multiple 
"msi-map" on a path from the device to the root, but we should probably 
attempt to handle it "just in case".


> Thanks,
>
> 	M.
>

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

On 09/21/2015 08:58 AM, Marc Zyngier wrote:
> On Fri, 18 Sep 2015 10:54:02 -0700
> David Daney <ddaney@caviumnetworks.com> wrote:
>
>> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
>>> On Thu, 17 Sep 2015 11:00:59 -0700
>>> David Daney <ddaney.cavm@gmail.com> wrote:
>>>
>>> Hi David,
>>>
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> Search up the device hierarchy to find devices with a "msi-map"
>>>> property, if found apply the mapping to the GIC device id.
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>> ---
>>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> index cf351c6..aa61cef 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>    	struct pci_dev *pdev;
>>>>    	struct its_pci_alias dev_alias;
>>>>    	struct msi_domain_info *msi_info;
>>>> +	struct device *parent_dev;
>>>> +	struct device_node *msi_controller_node = NULL;
>>>>
>>>>    	if (!dev_is_pci(dev))
>>>>    		return -EINVAL;
>>>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>    	dev_alias.count = nvec;
>>>>
>>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>>>> +	/*
>>>> +	 * Walk up the device parent links looking for one with a
>>>> +	 * "msi-map" property.
>>>> +	 */
>>>
>>> My first objection is the location of this parsing. It shouldn't be
>>> driver specific, but instead be part of the generic OF handling
>>> (nothing in these properties is GICv3 specific, even if the ITS is the
>>> only user so far).
>>
>> OK, I agree that this should eventually end up in generic OF handling
>> code.  I just wanted to get something out to initiate discussion.
>>
>> The next patch revision will move this to a more generic home.
>>
>>>
>>>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>>>
>>> Is there a limit how far we should go up the parent chain to find a
>>> msi-map? My hunch is that you should stop at the first device that does
>>> have an of_node, as it is the one that should contain the msi-map
>>> property.
>>
>> I think there is the possibility of finding something like a bridge that
>> has an of_node, but does not have the "msi-map" property.  I currently
>> have exactly this configuration, as some of the on-SoC devices sit
>> behind a bridge, but need an of_node to obtain unprobable properties and
>> children (the MDIO bus devices are like this).
>>
>> So if we want to abort the walk early, we should at least go up until we
>> find "msi-map" in the of_node.
>
> I don't really see a case where we would traverse a series of nodes
> where the msi-map property wouldn't be in the first node. Could you
> please give me an example?
>

OK, how about this:

	pcie0: pcie0 at 8480,00000000 {
		compatible = "pci-host-ecam-generic";
		device_type = "pci";
		msi-parent = <&its>;
		msi-map = <0 &its 0x80000 0x10000>;
		bus-range = <0 255>;
		#size-cells = <2>;
		#address-cells = <3>;
		#stream-id-cells = <1>;
		reg = <0x8480 0x00000000 0 0x10000000>;	 /* Configuration space */
		ranges = <0x03000000 0x8010 0x00000000 0x8010 0x00000000 0x070 
0x00000000>, /* mem ranges */
			 <0x03000000 0x87e0 0x00000000 0x87e0 0x00000000 0x020 0x00000000>;

		/* Other devices that use MSI, like USB xHCI, are here
		on the same bus as the bridge.  They have no firmware
		node as sufficient information can be probed as part
		of normal PCI probing. */

		mrml-bridge0 at 1,0 {
			compatible = "cavium,thunder-8890-mrml-bridge";
			#size-cells = <2>;
			#address-cells = <3>;
			ranges = <0x03000000 0x87e0 0x00000000 0x03000000 0x87e0 0x00000000 
0x10 0x00000000>;
			reg = <0x0800 0 0 0 0>; /* DEVFN = 0x08 (1:0) */

			mdio-nexus at 1,3 {
				compatible = "cavium,thunder-8890-mdio-nexus";
				#address-cells = <2>;
				#size-cells = <2>;
				reg = <0x0b00 0 0 0 0>; /* DEVFN = 0x0b (1:3) */
				assigned-addresses = <0x03000000 0x87e0 0x05000000 0x0 0x800000>;
				ranges = <0x87e0 0x05000000 0x03000000 0x87e0 0x05000000 0x0 0x800000>;

				mdio0 at 87e0,05003800 {
					compatible = "cavium,thunder-8890-mdio";
					#address-cells = <1>;
					#size-cells = <0>;
					reg = <0x87e0 0x05003800 0x0 0x30>;

					sgmii00: sgmii00 {
						reg = <0> ;
						compatible = "marvell,88e1240", "ethernet-phy-ieee802.3-c22";
					};
				};
			};
			bgx0 {
				#address-cells = <1>;
				#size-cells = <0>;
				reg = <0x8000 0 0 0 0>; /* DEVFN = 0x80 (16:0) */
				sgmii00 {
					reg = <0>;
					phy-handle = <&sgmii00>;
				};

			};

		};


The "msi-map" is specified in the PICe host controller node, but there
is a bridge between the device generating interrupts "bgx0" and the
host controller.


> [...]
>
>>>> +		msi_controller_node = of_find_node_by_phandle(phandle);
>>>> +		if (domain->of_node != msi_controller_node) {
>>>> +			dev_err(dev,
>>>> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
>>>> +				domain->of_node->full_name,
>>>> +				msi_controller_node ? NULL : msi_controller_node->full_name);
>>>
>>> Why is that an error? a RC can be configured to master multiple
>>> MSI-controllers,
>>
>> Something has already associated the PCI device with this
>> MSI-controller.  Therefore I think the reference in the map must refer
>> to this ITS MSI-controller instance.
>>
>>
>>> and the kernel picks one of them for a given device.
>>> This is illustrated by "Example (5)" in the binding, where a device can
>>> master two MSI controllers.
>>
>> The PCI host may have many MSI controllers, but I think a given PCI
>> device will have only one (based on bus:devfn) that is looked up in the map.
>
> A PCI device will only be configured to talk to a single MSI
> controller, but here you stop parsing the msi-map on the first match,
> and assume that you must have found the right MSI controller:
>
> I think this should read:
>
> +			if (masked_devid < rid_base ||
> +			    masked_devid >= rid_base + rid_len ||
> 			    domain->of_node != of_find_node_by_phandle(phandle)) {
> +				msi_map_len -= 4 * sizeof(__be32);
> +				msi_map += 4;
> +				continue;
> +			}
> +			matched = true;
> +			break;
>

Good, I will incorporate that too.

In practice, I don't know if we would ever find a system with multiple 
"msi-map" on a path from the device to the root, but we should probably 
attempt to handle it "just in case".


> Thanks,
>
> 	M.
>

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Mon, 21 Sep 2015 09:35:51 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 09/21/2015 08:58 AM, Marc Zyngier wrote:
> > On Fri, 18 Sep 2015 10:54:02 -0700
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >
> >> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> >>> On Thu, 17 Sep 2015 11:00:59 -0700
> >>> David Daney <ddaney.cavm@gmail.com> wrote:
> >>>
> >>> Hi David,
> >>>
> >>>> From: David Daney <david.daney@cavium.com>
> >>>>
> >>>> Search up the device hierarchy to find devices with a "msi-map"
> >>>> property, if found apply the mapping to the GIC device id.
> >>>>
> >>>> Signed-off-by: David Daney <david.daney@cavium.com>
> >>>> ---
> >>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 73 insertions(+)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> index cf351c6..aa61cef 100644
> >>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>>>    	struct pci_dev *pdev;
> >>>>    	struct its_pci_alias dev_alias;
> >>>>    	struct msi_domain_info *msi_info;
> >>>> +	struct device *parent_dev;
> >>>> +	struct device_node *msi_controller_node = NULL;
> >>>>
> >>>>    	if (!dev_is_pci(dev))
> >>>>    		return -EINVAL;
> >>>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>>>    	dev_alias.count = nvec;
> >>>>
> >>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >>>> +	/*
> >>>> +	 * Walk up the device parent links looking for one with a
> >>>> +	 * "msi-map" property.
> >>>> +	 */
> >>>
> >>> My first objection is the location of this parsing. It shouldn't be
> >>> driver specific, but instead be part of the generic OF handling
> >>> (nothing in these properties is GICv3 specific, even if the ITS is the
> >>> only user so far).
> >>
> >> OK, I agree that this should eventually end up in generic OF handling
> >> code.  I just wanted to get something out to initiate discussion.
> >>
> >> The next patch revision will move this to a more generic home.
> >>
> >>>
> >>>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>>
> >>> Is there a limit how far we should go up the parent chain to find a
> >>> msi-map? My hunch is that you should stop at the first device that does
> >>> have an of_node, as it is the one that should contain the msi-map
> >>> property.
> >>
> >> I think there is the possibility of finding something like a bridge that
> >> has an of_node, but does not have the "msi-map" property.  I currently
> >> have exactly this configuration, as some of the on-SoC devices sit
> >> behind a bridge, but need an of_node to obtain unprobable properties and
> >> children (the MDIO bus devices are like this).
> >>
> >> So if we want to abort the walk early, we should at least go up until we
> >> find "msi-map" in the of_node.
> >
> > I don't really see a case where we would traverse a series of nodes
> > where the msi-map property wouldn't be in the first node. Could you
> > please give me an example?
> >
> 
> OK, how about this:

[...]

> The "msi-map" is specified in the PICe host controller node, but there
> is a bridge between the device generating interrupts "bgx0" and the
> host controller.

OK, I can now see why you're doing that, thanks.


> >> The PCI host may have many MSI controllers, but I think a given PCI
> >> device will have only one (based on bus:devfn) that is looked up in the map.
> >
> > A PCI device will only be configured to talk to a single MSI
> > controller, but here you stop parsing the msi-map on the first match,
> > and assume that you must have found the right MSI controller:
> >
> > I think this should read:
> >
> > +			if (masked_devid < rid_base ||
> > +			    masked_devid >= rid_base + rid_len ||
> > 			    domain->of_node != of_find_node_by_phandle(phandle)) {
> > +				msi_map_len -= 4 * sizeof(__be32);
> > +				msi_map += 4;
> > +				continue;
> > +			}
> > +			matched = true;
> > +			break;
> >
> 
> Good, I will incorporate that too.
> 
> In practice, I don't know if we would ever find a system with multiple 
> "msi-map" on a path from the device to the root, but we should probably 
> attempt to handle it "just in case".

There are systems in the wild with exactly that kind of topology, and
I'd like to support them out of the box.

Thanks,

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

Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Mon, 21 Sep 2015 09:35:51 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 09/21/2015 08:58 AM, Marc Zyngier wrote:
> > On Fri, 18 Sep 2015 10:54:02 -0700
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >
> >> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> >>> On Thu, 17 Sep 2015 11:00:59 -0700
> >>> David Daney <ddaney.cavm@gmail.com> wrote:
> >>>
> >>> Hi David,
> >>>
> >>>> From: David Daney <david.daney@cavium.com>
> >>>>
> >>>> Search up the device hierarchy to find devices with a "msi-map"
> >>>> property, if found apply the mapping to the GIC device id.
> >>>>
> >>>> Signed-off-by: David Daney <david.daney@cavium.com>
> >>>> ---
> >>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 73 insertions(+)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> index cf351c6..aa61cef 100644
> >>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>>>    	struct pci_dev *pdev;
> >>>>    	struct its_pci_alias dev_alias;
> >>>>    	struct msi_domain_info *msi_info;
> >>>> +	struct device *parent_dev;
> >>>> +	struct device_node *msi_controller_node = NULL;
> >>>>
> >>>>    	if (!dev_is_pci(dev))
> >>>>    		return -EINVAL;
> >>>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>>>    	dev_alias.count = nvec;
> >>>>
> >>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >>>> +	/*
> >>>> +	 * Walk up the device parent links looking for one with a
> >>>> +	 * "msi-map" property.
> >>>> +	 */
> >>>
> >>> My first objection is the location of this parsing. It shouldn't be
> >>> driver specific, but instead be part of the generic OF handling
> >>> (nothing in these properties is GICv3 specific, even if the ITS is the
> >>> only user so far).
> >>
> >> OK, I agree that this should eventually end up in generic OF handling
> >> code.  I just wanted to get something out to initiate discussion.
> >>
> >> The next patch revision will move this to a more generic home.
> >>
> >>>
> >>>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>>
> >>> Is there a limit how far we should go up the parent chain to find a
> >>> msi-map? My hunch is that you should stop at the first device that does
> >>> have an of_node, as it is the one that should contain the msi-map
> >>> property.
> >>
> >> I think there is the possibility of finding something like a bridge that
> >> has an of_node, but does not have the "msi-map" property.  I currently
> >> have exactly this configuration, as some of the on-SoC devices sit
> >> behind a bridge, but need an of_node to obtain unprobable properties and
> >> children (the MDIO bus devices are like this).
> >>
> >> So if we want to abort the walk early, we should at least go up until we
> >> find "msi-map" in the of_node.
> >
> > I don't really see a case where we would traverse a series of nodes
> > where the msi-map property wouldn't be in the first node. Could you
> > please give me an example?
> >
> 
> OK, how about this:

[...]

> The "msi-map" is specified in the PICe host controller node, but there
> is a bridge between the device generating interrupts "bgx0" and the
> host controller.

OK, I can now see why you're doing that, thanks.


> >> The PCI host may have many MSI controllers, but I think a given PCI
> >> device will have only one (based on bus:devfn) that is looked up in the map.
> >
> > A PCI device will only be configured to talk to a single MSI
> > controller, but here you stop parsing the msi-map on the first match,
> > and assume that you must have found the right MSI controller:
> >
> > I think this should read:
> >
> > +			if (masked_devid < rid_base ||
> > +			    masked_devid >= rid_base + rid_len ||
> > 			    domain->of_node != of_find_node_by_phandle(phandle)) {
> > +				msi_map_len -= 4 * sizeof(__be32);
> > +				msi_map += 4;
> > +				continue;
> > +			}
> > +			matched = true;
> > +			break;
> >
> 
> Good, I will incorporate that too.
> 
> In practice, I don't know if we would ever find a system with multiple 
> "msi-map" on a path from the device to the root, but we should probably 
> attempt to handle it "just in case".

There are systems in the wild with exactly that kind of topology, and
I'd like to support them out of the box.

Thanks,

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

[PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier]

On Mon, 21 Sep 2015 09:35:51 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 09/21/2015 08:58 AM, Marc Zyngier wrote:
> > On Fri, 18 Sep 2015 10:54:02 -0700
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >
> >> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> >>> On Thu, 17 Sep 2015 11:00:59 -0700
> >>> David Daney <ddaney.cavm@gmail.com> wrote:
> >>>
> >>> Hi David,
> >>>
> >>>> From: David Daney <david.daney@cavium.com>
> >>>>
> >>>> Search up the device hierarchy to find devices with a "msi-map"
> >>>> property, if found apply the mapping to the GIC device id.
> >>>>
> >>>> Signed-off-by: David Daney <david.daney@cavium.com>
> >>>> ---
> >>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 73 insertions(+)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> index cf351c6..aa61cef 100644
> >>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >>>> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>>>    	struct pci_dev *pdev;
> >>>>    	struct its_pci_alias dev_alias;
> >>>>    	struct msi_domain_info *msi_info;
> >>>> +	struct device *parent_dev;
> >>>> +	struct device_node *msi_controller_node = NULL;
> >>>>
> >>>>    	if (!dev_is_pci(dev))
> >>>>    		return -EINVAL;
> >>>> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>>>    	dev_alias.count = nvec;
> >>>>
> >>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >>>> +	/*
> >>>> +	 * Walk up the device parent links looking for one with a
> >>>> +	 * "msi-map" property.
> >>>> +	 */
> >>>
> >>> My first objection is the location of this parsing. It shouldn't be
> >>> driver specific, but instead be part of the generic OF handling
> >>> (nothing in these properties is GICv3 specific, even if the ITS is the
> >>> only user so far).
> >>
> >> OK, I agree that this should eventually end up in generic OF handling
> >> code.  I just wanted to get something out to initiate discussion.
> >>
> >> The next patch revision will move this to a more generic home.
> >>
> >>>
> >>>> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>>
> >>> Is there a limit how far we should go up the parent chain to find a
> >>> msi-map? My hunch is that you should stop at the first device that does
> >>> have an of_node, as it is the one that should contain the msi-map
> >>> property.
> >>
> >> I think there is the possibility of finding something like a bridge that
> >> has an of_node, but does not have the "msi-map" property.  I currently
> >> have exactly this configuration, as some of the on-SoC devices sit
> >> behind a bridge, but need an of_node to obtain unprobable properties and
> >> children (the MDIO bus devices are like this).
> >>
> >> So if we want to abort the walk early, we should at least go up until we
> >> find "msi-map" in the of_node.
> >
> > I don't really see a case where we would traverse a series of nodes
> > where the msi-map property wouldn't be in the first node. Could you
> > please give me an example?
> >
> 
> OK, how about this:

[...]

> The "msi-map" is specified in the PICe host controller node, but there
> is a bridge between the device generating interrupts "bgx0" and the
> host controller.

OK, I can now see why you're doing that, thanks.


> >> The PCI host may have many MSI controllers, but I think a given PCI
> >> device will have only one (based on bus:devfn) that is looked up in the map.
> >
> > A PCI device will only be configured to talk to a single MSI
> > controller, but here you stop parsing the msi-map on the first match,
> > and assume that you must have found the right MSI controller:
> >
> > I think this should read:
> >
> > +			if (masked_devid < rid_base ||
> > +			    masked_devid >= rid_base + rid_len ||
> > 			    domain->of_node != of_find_node_by_phandle(phandle)) {
> > +				msi_map_len -= 4 * sizeof(__be32);
> > +				msi_map += 4;
> > +				continue;
> > +			}
> > +			matched = true;
> > +			break;
> >
> 
> Good, I will incorporate that too.
> 
> In practice, I don't know if we would ever find a system with multiple 
> "msi-map" on a path from the device to the root, but we should probably 
> attempt to handle it "just in case".

There are systems in the wild with exactly that kind of topology, and
I'd like to support them out of the box.

Thanks,

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