[PATCH v2 0/3] of, irqchip/gicv3-its: Handle "msi-map" properties.

[PATCH v2 0/3] of, irqchip/gicv3-its: Handle "msi-map" properties.

[David Daney]

From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

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 adds a parser to get the required information out of
the device tree.

The third and final patch adds the "msi-map" parsing to gicv3-its.

Changes from v1: Factor out the device tree access code to a separate
function in drivers/of/irq.c

David Daney (2):
  of/irq: Add new function of_msi_map_rid()
  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          |   3 +-
 drivers/of/irq.c                                  |  86 +++++++++
 include/linux/of_irq.h                            |   7 +
 4 files changed, 315 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-msi.txt

-- 
1.9.1

--
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 v2 1/3] Docs: dt: Add PCI MSI map bindings

[David Daney]

From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

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-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
 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

--
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

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

[Marc Zyngier]

On Tue, 22 Sep 2015 17:00:04 -0700
David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> 
> 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-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

I thought I had done it already, but nevertheless:

Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

	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 v2 2/3] of/irq: Add new function of_msi_map_rid()

[David Daney]

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

The device tree property "msi-map" specifies how to create the PCI
requester id used in some MSI controllers.  Add a new function
of_msi_map_rid() that finds the msi-map property and applies its
translation to a given requester id.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/of/irq.c       | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_irq.h |  7 ++++
 2 files changed, 93 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 55317fa..3f64d2e 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -598,3 +598,89 @@ void of_msi_configure(struct device *dev, struct device_node *np)
 		d = irq_find_host(msi_np);
 	dev_set_msi_domain(dev, d);
 }
+
+/**
+ * of_msi_map_rid - Map a MSI requester ID for a device.
+ * @dev: device for which the mapping is to be done.
+ * @msi_np: device node of the expected msi controller.
+ * @rid_in: unmapped MSI requester ID for the device.
+ *
+ * Walk up the device hierarchy looking for devices with a "msi-map"
+ * property.  If found, apply the mapping to @rid_in.
+ *
+ * Returns the mapped MSI requester ID.
+ */
+u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
+{
+	struct device *parent_dev;
+	struct device_node *msi_controller_node;
+	const __be32 *msi_map;
+	u32 map_mask, masked_rid;
+	u32 rid_base, msi_base, rid_len, phandle;
+	int msi_map_len;
+	bool matched;
+	u32 rid_out = rid_in;
+
+	/*
+	 * Walk up the device parent links looking for one with a
+	 * "msi-map" property.
+	 */
+	msi_map = NULL;
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
+		if (!parent_dev->of_node)
+			continue;
+
+		msi_map = of_get_property(parent_dev->of_node,
+					  "msi-map", &msi_map_len);
+		if (!msi_map)
+			continue;
+
+		if (msi_map_len % (4 * sizeof(__be32))) {
+			dev_err(parent_dev, "Error: Bad msi-map length: %d\n",
+				msi_map_len);
+			goto out;
+		}
+		/* We have a good parent_dev and msi_map, let's use them. */
+		break;
+	}
+	if (!msi_map)
+		goto out;
+
+	/* The default is to select all bits. */
+	map_mask = 0xffffffff;
+
+	/*
+	 * Can be overridden by "msi-map-mask" property.  If
+	 * of_property_read_u32() fails, the default is used.
+	 */
+	of_property_read_u32(parent_dev->of_node, "msi-map-mask", &map_mask);
+
+	masked_rid = map_mask & rid_in;
+	matched = false;
+	while (!matched && 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);
+
+		msi_controller_node = of_find_node_by_phandle(phandle);
+
+		matched = masked_rid >= rid_base &&
+			masked_rid < rid_base + rid_len &&
+			msi_np == msi_controller_node;
+
+		of_node_put(msi_controller_node);
+		msi_map_len -= 4 * sizeof(__be32);
+		msi_map += 4;
+	}
+	if (!matched)
+		goto out;
+
+	rid_out = masked_rid + msi_base;
+	dev_dbg(dev,
+		"msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
+		dev_name(parent_dev), map_mask, rid_base, msi_base,
+		rid_len, rid_in, rid_out);
+out:
+	return rid_out;
+}
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 4bcbd58..8cd9334 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -75,6 +75,7 @@ static inline int of_irq_to_resource_table(struct device_node *dev,
 extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
 extern void of_msi_configure(struct device *dev, struct device_node *np);
+u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
 
 #else /* !CONFIG_OF */
 static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
@@ -87,6 +88,12 @@ static inline void *of_irq_find_parent(struct device_node *child)
 {
 	return NULL;
 }
+
+static inline u32 of_msi_map_rid(struct device *dev,
+				 struct device_node *msi_np, u32 rid_in)
+{
+	return rid_in;
+}
 #endif /* !CONFIG_OF */
 
 #endif /* __OF_IRQ_H */
-- 
1.9.1

Re: [PATCH v2 2/3] of/irq: Add new function of_msi_map_rid()

[Marc Zyngier]

On Tue, 22 Sep 2015 17:00:05 -0700
David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> The device tree property "msi-map" specifies how to create the PCI
> requester id used in some MSI controllers.  Add a new function
> of_msi_map_rid() that finds the msi-map property and applies its
> translation to a given requester id.
> 
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/of/irq.c       | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_irq.h |  7 ++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 55317fa..3f64d2e 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -598,3 +598,89 @@ void of_msi_configure(struct device *dev, struct device_node *np)
>  		d = irq_find_host(msi_np);
>  	dev_set_msi_domain(dev, d);
>  }
> +
> +/**
> + * of_msi_map_rid - Map a MSI requester ID for a device.
> + * @dev: device for which the mapping is to be done.
> + * @msi_np: device node of the expected msi controller.
> + * @rid_in: unmapped MSI requester ID for the device.
> + *
> + * Walk up the device hierarchy looking for devices with a "msi-map"
> + * property.  If found, apply the mapping to @rid_in.
> + *
> + * Returns the mapped MSI requester ID.
> + */
> +u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
> +{
> +	struct device *parent_dev;
> +	struct device_node *msi_controller_node;
> +	const __be32 *msi_map;
> +	u32 map_mask, masked_rid;
> +	u32 rid_base, msi_base, rid_len, phandle;
> +	int msi_map_len;
> +	bool matched;
> +	u32 rid_out = rid_in;
> +
> +	/*
> +	 * Walk up the device parent links looking for one with a
> +	 * "msi-map" property.
> +	 */
> +	msi_map = NULL;
> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> +		if (!parent_dev->of_node)
> +			continue;
> +
> +		msi_map = of_get_property(parent_dev->of_node,
> +					  "msi-map", &msi_map_len);
> +		if (!msi_map)
> +			continue;
> +
> +		if (msi_map_len % (4 * sizeof(__be32))) {
> +			dev_err(parent_dev, "Error: Bad msi-map length: %d\n",
> +				msi_map_len);
> +			goto out;
> +		}
> +		/* We have a good parent_dev and msi_map, let's use them. */
> +		break;
> +	}
> +	if (!msi_map)
> +		goto out;
> +
> +	/* The default is to select all bits. */
> +	map_mask = 0xffffffff;
> +
> +	/*
> +	 * Can be overridden by "msi-map-mask" property.  If
> +	 * of_property_read_u32() fails, the default is used.
> +	 */
> +	of_property_read_u32(parent_dev->of_node, "msi-map-mask", &map_mask);
> +
> +	masked_rid = map_mask & rid_in;
> +	matched = false;
> +	while (!matched && 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);

Rob did suggest to use of_property_read_u32_index() instead of these
be32_to_cpup(). Not sure if that'd be a major improvement though.

> +
> +		msi_controller_node = of_find_node_by_phandle(phandle);
> +
> +		matched = masked_rid >= rid_base &&
> +			masked_rid < rid_base + rid_len &&
> +			msi_np == msi_controller_node;
> +
> +		of_node_put(msi_controller_node);
> +		msi_map_len -= 4 * sizeof(__be32);
> +		msi_map += 4;
> +	}
> +	if (!matched)
> +		goto out;
> +
> +	rid_out = masked_rid + msi_base;
> +	dev_dbg(dev,
> +		"msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
> +		dev_name(parent_dev), map_mask, rid_base, msi_base,
> +		rid_len, rid_in, rid_out);
> +out:
> +	return rid_out;
> +}
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 4bcbd58..8cd9334 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -75,6 +75,7 @@ static inline int of_irq_to_resource_table(struct device_node *dev,
>  extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>  extern void of_msi_configure(struct device *dev, struct device_node *np);
> +u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
>  
>  #else /* !CONFIG_OF */
>  static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
> @@ -87,6 +88,12 @@ static inline void *of_irq_find_parent(struct device_node *child)
>  {
>  	return NULL;
>  }
> +
> +static inline u32 of_msi_map_rid(struct device *dev,
> +				 struct device_node *msi_np, u32 rid_in)
> +{
> +	return rid_in;
> +}
>  #endif /* !CONFIG_OF */
>  
>  #endif /* __OF_IRQ_H */

Otherwise looks good to me.

Reviewed-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

	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

Re: [PATCH v2 2/3] of/irq: Add new function of_msi_map_rid()

[David Daney]

On 09/23/2015 09:52 AM, Marc Zyngier wrote:
> On Tue, 22 Sep 2015 17:00:05 -0700
> David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>
>> The device tree property "msi-map" specifies how to create the PCI
>> requester id used in some MSI controllers.  Add a new function
>> of_msi_map_rid() that finds the msi-map property and applies its
>> translation to a given requester id.
>>
>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/of/irq.c       | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_irq.h |  7 ++++
>>   2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 55317fa..3f64d2e 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -598,3 +598,89 @@ void of_msi_configure(struct device *dev, struct device_node *np)
[...]
>> +	while (!matched && 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);
>
> Rob did suggest to use of_property_read_u32_index() instead of these
> be32_to_cpup(). Not sure if that'd be a major improvement though.
>

I did think about that, but the micro-optimizer in me didn't feel 
comfortable with the overhead of rerunning of_find_property() for each 
value.  I decided to run of_find_property() once, and then iterate 
through the individual values within the property.

If there are strong objections to doing it this way, we can change it.

David Daney
--
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

Re: [PATCH v2 2/3] of/irq: Add new function of_msi_map_rid()

[Rob Herring]

On Tue, Sep 22, 2015 at 7:00 PM, David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>
> The device tree property "msi-map" specifies how to create the PCI
> requester id used in some MSI controllers.  Add a new function
> of_msi_map_rid() that finds the msi-map property and applies its
> translation to a given requester id.
>
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/of/irq.c       | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_irq.h |  7 ++++
>  2 files changed, 93 insertions(+)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 55317fa..3f64d2e 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -598,3 +598,89 @@ void of_msi_configure(struct device *dev, struct device_node *np)
>                 d = irq_find_host(msi_np);
>         dev_set_msi_domain(dev, d);
>  }
> +
> +/**
> + * of_msi_map_rid - Map a MSI requester ID for a device.
> + * @dev: device for which the mapping is to be done.
> + * @msi_np: device node of the expected msi controller.
> + * @rid_in: unmapped MSI requester ID for the device.
> + *
> + * Walk up the device hierarchy looking for devices with a "msi-map"
> + * property.  If found, apply the mapping to @rid_in.
> + *
> + * Returns the mapped MSI requester ID.
> + */
> +u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)

Generally, the OF API does not use struct device except for what is in
of/device.c. I'm guessing you need it here because you have devices
without DT nodes?

We may want to handle this more like NUMA node IDs where we populate
data in struct device at creation time and looking at struct device
parents is independent of DT. How would this information be retrieved
with ACPI?

> +{
> +       struct device *parent_dev;
> +       struct device_node *msi_controller_node;
> +       const __be32 *msi_map;

> +       u32 map_mask, masked_rid;
> +       u32 rid_base, msi_base, rid_len, phandle;

One line please.

> +       int msi_map_len;
> +       bool matched;
> +       u32 rid_out = rid_in;
> +
> +       /*
> +        * Walk up the device parent links looking for one with a
> +        * "msi-map" property.
> +        */
> +       msi_map = NULL;

init in the declaration.

> +       for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> +               if (!parent_dev->of_node)
> +                       continue;
> +
> +               msi_map = of_get_property(parent_dev->of_node,
> +                                         "msi-map", &msi_map_len);
> +               if (!msi_map)
> +                       continue;
> +
> +               if (msi_map_len % (4 * sizeof(__be32))) {
> +                       dev_err(parent_dev, "Error: Bad msi-map length: %d\n",
> +                               msi_map_len);
> +                       goto out;

Just return here.

> +               }
> +               /* We have a good parent_dev and msi_map, let's use them. */
> +               break;
> +       }
> +       if (!msi_map)
> +               goto out;

Just return here.

> +
> +       /* The default is to select all bits. */
> +       map_mask = 0xffffffff;
> +
> +       /*
> +        * Can be overridden by "msi-map-mask" property.  If
> +        * of_property_read_u32() fails, the default is used.
> +        */
> +       of_property_read_u32(parent_dev->of_node, "msi-map-mask", &map_mask);
> +
> +       masked_rid = map_mask & rid_in;
> +       matched = false;
> +       while (!matched && 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);
> +
> +               msi_controller_node = of_find_node_by_phandle(phandle);
> +
> +               matched = masked_rid >= rid_base &&
> +                       masked_rid < rid_base + rid_len &&
> +                       msi_np == msi_controller_node;
> +
> +               of_node_put(msi_controller_node);
> +               msi_map_len -= 4 * sizeof(__be32);
> +               msi_map += 4;
> +       }
> +       if (!matched)
> +               goto out;

Just return here.

> +
> +       rid_out = masked_rid + msi_base;
> +       dev_dbg(dev,
> +               "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
> +               dev_name(parent_dev), map_mask, rid_base, msi_base,
> +               rid_len, rid_in, rid_out);
> +out:
> +       return rid_out;
> +}
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 4bcbd58..8cd9334 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -75,6 +75,7 @@ static inline int of_irq_to_resource_table(struct device_node *dev,
>  extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
>  extern struct device_node *of_irq_find_parent(struct device_node *child);
>  extern void of_msi_configure(struct device *dev, struct device_node *np);
> +u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
>
>  #else /* !CONFIG_OF */
>  static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
> @@ -87,6 +88,12 @@ static inline void *of_irq_find_parent(struct device_node *child)
>  {
>         return NULL;
>  }
> +
> +static inline u32 of_msi_map_rid(struct device *dev,
> +                                struct device_node *msi_np, u32 rid_in)
> +{
> +       return rid_in;
> +}
>  #endif /* !CONFIG_OF */
>
>  #endif /* __OF_IRQ_H */
> --
> 1.9.1
>
--
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

Re: [PATCH v2 2/3] of/irq: Add new function of_msi_map_rid()

[David Daney]

On 09/23/2015 10:07 AM, Rob Herring wrote:
> On Tue, Sep 22, 2015 at 7:00 PM, David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>
>> The device tree property "msi-map" specifies how to create the PCI
>> requester id used in some MSI controllers.  Add a new function
>> of_msi_map_rid() that finds the msi-map property and applies its
>> translation to a given requester id.
>>
>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/of/irq.c       | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_irq.h |  7 ++++
>>   2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 55317fa..3f64d2e 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -598,3 +598,89 @@ void of_msi_configure(struct device *dev, struct device_node *np)
>>                  d = irq_find_host(msi_np);
>>          dev_set_msi_domain(dev, d);
>>   }
>> +
>> +/**
>> + * of_msi_map_rid - Map a MSI requester ID for a device.
>> + * @dev: device for which the mapping is to be done.
>> + * @msi_np: device node of the expected msi controller.
>> + * @rid_in: unmapped MSI requester ID for the device.
>> + *
>> + * Walk up the device hierarchy looking for devices with a "msi-map"
>> + * property.  If found, apply the mapping to @rid_in.
>> + *
>> + * Returns the mapped MSI requester ID.
>> + */
>> +u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
>
> Generally, the OF API does not use struct device except for what is in
> of/device.c. I'm guessing you need it here because you have devices
> without DT nodes?

Exactly right.  As you can see we walk up the hierarchy until we find a 
device with the desired of_node ("msi-map").

>
> We may want to handle this more like NUMA node IDs where we populate
> data in struct device at creation time and looking at struct device
> parents is independent of DT. How would this information be retrieved
> with ACPI?

ACPI has a table called _IORT that contains similar information.  It 
would have to be extracted with an analogous facility.

>
>> +{
>> +       struct device *parent_dev;
>> +       struct device_node *msi_controller_node;
>> +       const __be32 *msi_map;
>
>> +       u32 map_mask, masked_rid;
>> +       u32 rid_base, msi_base, rid_len, phandle;
>
> One line please.
OK.

It looks like another revision of the patch is in order.

>
>> +       int msi_map_len;
>> +       bool matched;
>> +       u32 rid_out = rid_in;
>> +
>> +       /*
>> +        * Walk up the device parent links looking for one with a
>> +        * "msi-map" property.
>> +        */
>> +       msi_map = NULL;
>
> init in the declaration.

OK...
>
>> +       for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>> +               if (!parent_dev->of_node)
>> +                       continue;
>> +
>> +               msi_map = of_get_property(parent_dev->of_node,
>> +                                         "msi-map", &msi_map_len);
>> +               if (!msi_map)
>> +                       continue;
>> +
>> +               if (msi_map_len % (4 * sizeof(__be32))) {
>> +                       dev_err(parent_dev, "Error: Bad msi-map length: %d\n",
>> +                               msi_map_len);
>> +                       goto out;
>
> Just return here.
>
>> +               }
>> +               /* We have a good parent_dev and msi_map, let's use them. */
>> +               break;
>> +       }
>> +       if (!msi_map)
>> +               goto out;
>
> Just return here.
>
>> +
>> +       /* The default is to select all bits. */
>> +       map_mask = 0xffffffff;
>> +
>> +       /*
>> +        * Can be overridden by "msi-map-mask" property.  If
>> +        * of_property_read_u32() fails, the default is used.
>> +        */
>> +       of_property_read_u32(parent_dev->of_node, "msi-map-mask", &map_mask);
>> +
>> +       masked_rid = map_mask & rid_in;
>> +       matched = false;
>> +       while (!matched && 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);
>> +
>> +               msi_controller_node = of_find_node_by_phandle(phandle);
>> +
>> +               matched = masked_rid >= rid_base &&
>> +                       masked_rid < rid_base + rid_len &&
>> +                       msi_np == msi_controller_node;
>> +
>> +               of_node_put(msi_controller_node);
>> +               msi_map_len -= 4 * sizeof(__be32);
>> +               msi_map += 4;
>> +       }
>> +       if (!matched)
>> +               goto out;
>
> Just return here.
>
>> +
>> +       rid_out = masked_rid + msi_base;
>> +       dev_dbg(dev,
>> +               "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
>> +               dev_name(parent_dev), map_mask, rid_base, msi_base,
>> +               rid_len, rid_in, rid_out);
>> +out:
>> +       return rid_out;
>> +}
>> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
>> index 4bcbd58..8cd9334 100644
>> --- a/include/linux/of_irq.h
>> +++ b/include/linux/of_irq.h
>> @@ -75,6 +75,7 @@ static inline int of_irq_to_resource_table(struct device_node *dev,
>>   extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
>>   extern struct device_node *of_irq_find_parent(struct device_node *child);
>>   extern void of_msi_configure(struct device *dev, struct device_node *np);
>> +u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
>>
>>   #else /* !CONFIG_OF */
>>   static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
>> @@ -87,6 +88,12 @@ static inline void *of_irq_find_parent(struct device_node *child)
>>   {
>>          return NULL;
>>   }
>> +
>> +static inline u32 of_msi_map_rid(struct device *dev,
>> +                                struct device_node *msi_np, u32 rid_in)
>> +{
>> +       return rid_in;
>> +}
>>   #endif /* !CONFIG_OF */
>>
>>   #endif /* __OF_IRQ_H */
>> --
>> 1.9.1
>>

--
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 v2 3/3] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[David Daney]

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

Call of_msi_map_rid() to handle mapping of the requester id.

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

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index cf351c6..8b1c938 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
 
 	/* ITS specific DeviceID, as the core ITS ignores dev. */
-	info->scratchpad[0].ul = dev_alias.dev_id;
+	info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
+						dev_alias.dev_id);
 
 	return msi_info->ops->msi_prepare(domain->parent,
 					  dev, dev_alias.count, info);
-- 
1.9.1

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

[Marc Zyngier]

On Tue, 22 Sep 2015 17:00:06 -0700
David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> Call of_msi_map_rid() to handle mapping of the requester id.
> 
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index cf351c6..8b1c938 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>  
>  	/* ITS specific DeviceID, as the core ITS ignores dev. */
> -	info->scratchpad[0].ul = dev_alias.dev_id;
> +	info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
> +						dev_alias.dev_id);
>  
>  	return msi_info->ops->msi_prepare(domain->parent,
>  					  dev, dev_alias.count, info);

I really wonder if that shouldn't be part of the pci_for_each_dma_alias
call. It would make a lot more sense for this functionality to be an
integral part of the core code, and would probably make the integration
of _IORT (which has the exact same requirements) a bit easier.

Thoughts?

	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

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

[David Daney]

On 09/23/2015 10:01 AM, Marc Zyngier wrote:
> On Tue, 22 Sep 2015 17:00:06 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
>> From: David Daney <david.daney@cavium.com>
>>
>> Call of_msi_map_rid() to handle mapping of the requester id.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> index cf351c6..8b1c938 100644
>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>> @@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>>
>>   	/* ITS specific DeviceID, as the core ITS ignores dev. */
>> -	info->scratchpad[0].ul = dev_alias.dev_id;
>> +	info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
>> +						dev_alias.dev_id);
>>
>>   	return msi_info->ops->msi_prepare(domain->parent,
>>   					  dev, dev_alias.count, info);
>
> I really wonder if that shouldn't be part of the pci_for_each_dma_alias
> call. It would make a lot more sense for this functionality to be an
> integral part of the core code, and would probably make the integration
> of _IORT (which has the exact same requirements) a bit easier.
>
> Thoughts?
>

I am a proponent of pushing things like this as far into the core code 
as possible.  So, from that point of view, I think it would probably be 
a good idea.

I can prepare a patch that does that, but it would also be nice hear 
from other maintainers and get their thoughts on this.


> 	M.
>

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

[Will Deacon]

On Wed, Sep 23, 2015 at 06:08:39PM +0100, David Daney wrote:
> On 09/23/2015 10:01 AM, Marc Zyngier wrote:
> > On Tue, 22 Sep 2015 17:00:06 -0700
> > David Daney <ddaney.cavm@gmail.com> wrote:
> >
> >> From: David Daney <david.daney@cavium.com>
> >>
> >> Call of_msi_map_rid() to handle mapping of the requester id.
> >>
> >> Signed-off-by: David Daney <david.daney@cavium.com>
> >> ---
> >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> index cf351c6..8b1c938 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> @@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >>
> >>   	/* ITS specific DeviceID, as the core ITS ignores dev. */
> >> -	info->scratchpad[0].ul = dev_alias.dev_id;
> >> +	info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
> >> +						dev_alias.dev_id);
> >>
> >>   	return msi_info->ops->msi_prepare(domain->parent,
> >>   					  dev, dev_alias.count, info);
> >
> > I really wonder if that shouldn't be part of the pci_for_each_dma_alias
> > call. It would make a lot more sense for this functionality to be an
> > integral part of the core code, and would probably make the integration
> > of _IORT (which has the exact same requirements) a bit easier.
> >
> > Thoughts?
> >
> 
> I am a proponent of pushing things like this as far into the core code 
> as possible.  So, from that point of view, I think it would probably be 
> a good idea.
> 
> I can prepare a patch that does that, but it would also be nice hear 
> from other maintainers and get their thoughts on this.

Hmm, we use pci_for_each_dma_alias in the SMMU drivers to get the SID,
so I'm not sure that using the MSI mapping is necessarily the right thing
to do there. Maybe we should instead have dma_alias_to_msi_id helpers or
something?

Will

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

[Marc Zyngier]

On Wed, 23 Sep 2015 18:52:59 +0100
Will Deacon <will.deacon@arm.com> wrote:

> On Wed, Sep 23, 2015 at 06:08:39PM +0100, David Daney wrote:
> > On 09/23/2015 10:01 AM, Marc Zyngier wrote:
> > > On Tue, 22 Sep 2015 17:00:06 -0700
> > > David Daney <ddaney.cavm@gmail.com> wrote:
> > >
> > >> From: David Daney <david.daney@cavium.com>
> > >>
> > >> Call of_msi_map_rid() to handle mapping of the requester id.
> > >>
> > >> Signed-off-by: David Daney <david.daney@cavium.com>
> > >> ---
> > >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
> > >>   1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> > >> index cf351c6..8b1c938 100644
> > >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> > >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> > >> @@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> > >>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> > >>
> > >>   	/* ITS specific DeviceID, as the core ITS ignores dev. */
> > >> -	info->scratchpad[0].ul = dev_alias.dev_id;
> > >> +	info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
> > >> +						dev_alias.dev_id);
> > >>
> > >>   	return msi_info->ops->msi_prepare(domain->parent,
> > >>   					  dev, dev_alias.count, info);
> > >
> > > I really wonder if that shouldn't be part of the pci_for_each_dma_alias
> > > call. It would make a lot more sense for this functionality to be an
> > > integral part of the core code, and would probably make the integration
> > > of _IORT (which has the exact same requirements) a bit easier.
> > >
> > > Thoughts?
> > >
> > 
> > I am a proponent of pushing things like this as far into the core code 
> > as possible.  So, from that point of view, I think it would probably be 
> > a good idea.
> > 
> > I can prepare a patch that does that, but it would also be nice hear 
> > from other maintainers and get their thoughts on this.
> 
> Hmm, we use pci_for_each_dma_alias in the SMMU drivers to get the SID,
> so I'm not sure that using the MSI mapping is necessarily the right thing
> to do there. Maybe we should instead have dma_alias_to_msi_id helpers or
> something?

Yes, that's probably a sensible solution.

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

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

[Robin Murphy]

On 23/09/15 19:18, Marc Zyngier wrote:
> On Wed, 23 Sep 2015 18:52:59 +0100
> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>
>> On Wed, Sep 23, 2015 at 06:08:39PM +0100, David Daney wrote:
>>> On 09/23/2015 10:01 AM, Marc Zyngier wrote:
>>>> On Tue, 22 Sep 2015 17:00:06 -0700
>>>> David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> Call of_msi_map_rid() to handle mapping of the requester id.
>>>>>
>>>>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>    drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>>> index cf351c6..8b1c938 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
>>>>> @@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>>    	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
>>>>>
>>>>>    	/* ITS specific DeviceID, as the core ITS ignores dev. */
>>>>> -	info->scratchpad[0].ul = dev_alias.dev_id;
>>>>> +	info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
>>>>> +						dev_alias.dev_id);
>>>>>
>>>>>    	return msi_info->ops->msi_prepare(domain->parent,
>>>>>    					  dev, dev_alias.count, info);
>>>>
>>>> I really wonder if that shouldn't be part of the pci_for_each_dma_alias
>>>> call. It would make a lot more sense for this functionality to be an
>>>> integral part of the core code, and would probably make the integration
>>>> of _IORT (which has the exact same requirements) a bit easier.
>>>>
>>>> Thoughts?
>>>>
>>>
>>> I am a proponent of pushing things like this as far into the core code
>>> as possible.  So, from that point of view, I think it would probably be
>>> a good idea.
>>>
>>> I can prepare a patch that does that, but it would also be nice hear
>>> from other maintainers and get their thoughts on this.
>>
>> Hmm, we use pci_for_each_dma_alias in the SMMU drivers to get the SID,
>> so I'm not sure that using the MSI mapping is necessarily the right thing
>> to do there. Maybe we should instead have dma_alias_to_msi_id helpers or
>> something?
>
> Yes, that's probably a sensible solution.

Seconded; take a look at all the additional bus-walking 
iommu_group_get_for_pci_dev does between the call to 
pci_for_each_dma_alias and the group = iommu_group_alloc() line - I'd 
say it's only at the latter point, when there are no aliases found on 
the PCI side, that it would then need to check the external RID-SID 
mapping to see if there is any further aliasing downstream of the root 
complex (and possibly liaise with the IOMMU driver to retrieve an 
appropriate group, but let's worry about that bit later).

Robin.

>
> 	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