[RFC PATCH v2 0/7] Introduce automatic DMA configuration for IOMMU masters

[RFC PATCH v2 0/7] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

Hello again,

Hot on the heels of my initial RFC, here's a v2 of the posting from here:

  RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html

The main difference since v1 is that I've introduced some generic
structures to represent IOMMUs and their mappings (struct iommu_data and
struct iommu_dma_mapping). This allows us to propagate the per-instance
data all the way down to the DMA-mapping code, which can then manage a
per-instance domain. Note that the ARM dma-mapping implementation does
not currently make use of this information.

Since there was a bit of confusion about how this is supposed to work,
the way I envisage it hanging together is:

  (1) An IOMMU driver uses IOMMU_OF_DECLARE to register an early
      initialisation callback.

  (2) The architecture code calls iommu_init() before kicking off the
      usual device probing (e.g. of_platform_populate), which executes
      the IOMMU initialisation callback for each IOMMU node in the
      device-tree. At this point, the IOMMU driver is expected to
      allocate iommu_data structure and initialise the priv field before
      calling of_iommu_set_data on its device_node.

  (3) For each master, of_iommu_configure will call of_xlate on the
      corresponding IOMMU(s), passing of_phandle_args. This allows the
      IOMMU driver to retrieve the iommu_data with of_iommu_get_data and
      update its internal data structures with the IDs for the new
      master. At this point, the per-iommu-instance iommu_data is
      inserted into a per-device iommu_dma_mapping struct. This allows
      multiple IOMMU domains to be linked together for devices that
      master through multiple IOMMUs (but note that iommu_configure
      currently doesn't not allow this because I'm lazy).

  (4) The newly allocated iommu_dma_mapping is passed to
      arch_setup_dma_ops, which can use it to set the correct dma_ops
      for the device. If IOMMU-capable ops are in-use, the domain and
      iova allocator for each iommu_data entry should be initialised (if
      they haven't been already) and used for managing the
      per-iommu-instance address space.

There are a bunch of things that could be done on top of this series:

  - Port IOMMU driver(s) to it
  - Remove the add_device call from iommu_ops
  - Add support for devices that master through multiple IOMMUs
  - Do something more intelligent with the DMA mask
  - Add generic support for device isolation (ie. one domain per device)
  - Wire up a different bus (e.g. PCI)
  - Move ARM's IOMMU dma-mapping code over to using iommu_dma_mapping
    instead of the arch-specific dma_iommu_mapping

Anyway, feedback welcome. There are certainly a few different ways of
doing this.

Will

--->8

Will Deacon (7):
  iommu: provide early initialisation hook for IOMMU drivers
  dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
  iommu: add new iommu_ops callback for adding an OF device
  iommu: provide helper function to configure an IOMMU for an of master
  dma-mapping: detect and configure IOMMU in of_dma_configure
  arm: call iommu_init before of_platform_populate
  arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

 arch/arm/include/asm/dma-mapping.h | 11 +++---
 arch/arm/kernel/setup.c            |  5 ++-
 arch/arm/mm/dma-mapping.c          | 72 +++++++++++++++++++++++++++++++++-----
 drivers/iommu/Kconfig              |  2 +-
 drivers/iommu/of_iommu.c           | 66 ++++++++++++++++++++++++++++++++++
 drivers/of/platform.c              | 54 +++++++++++-----------------
 include/asm-generic/vmlinux.lds.h  |  2 ++
 include/linux/dma-mapping.h        | 18 +++++++---
 include/linux/iommu.h              | 11 ++++++
 include/linux/of_iommu.h           | 31 ++++++++++++++++
 10 files changed, 218 insertions(+), 54 deletions(-)

-- 
2.1.0

[RFC PATCH v2 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Will Deacon]

IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
set some per-instance data on their of_node in the form of a new
iommu_data structure. This can later be used when parsing OF masters to
identify the IOMMU instance in question.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/iommu/of_iommu.c          | 14 ++++++++++++++
 include/asm-generic/vmlinux.lds.h |  2 ++
 include/linux/iommu.h             |  6 ++++++
 include/linux/of_iommu.h          | 23 +++++++++++++++++++++++
 4 files changed, 45 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..f9209666157c 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -89,3 +89,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+	struct device_node *np;
+	const struct of_device_id *match, *matches = &__iommu_of_table;
+
+	for_each_matching_node_and_match(np, matches, &match) {
+		const int (*init_fn)(struct device_node *) = match->data;
+
+		if (init_fn(np))
+			pr_err("Failed to initialise IOMMU %s\n",
+				of_node_full_name(np));
+	}
+}
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 5ba0360663a7..b75ede8f99ae 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -162,6 +162,7 @@
 #define CLKSRC_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
 #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
 #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()	OF_TABLE(CONFIG_OF_IOMMU, iommu)
 #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
 #define CPU_METHOD_OF_TABLES()	OF_TABLE(CONFIG_SMP, cpu_method)
 #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -495,6 +496,7 @@
 	CLK_OF_TABLES()							\
 	RESERVEDMEM_OF_TABLES()						\
 	CLKSRC_OF_TABLES()						\
+	IOMMU_OF_TABLES()						\
 	CPU_METHOD_OF_TABLES()						\
 	KERNEL_DTB()							\
 	IRQCHIP_OF_MATCH_TABLE()					\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a527922a..fdddb14cd8f5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -57,6 +57,12 @@ struct iommu_domain {
 	struct iommu_domain_geometry geometry;
 };
 
+struct iommu_data {
+	struct iommu_domain *domain;
+	struct iova_domain *iovad;
+	void *priv;
+};
+
 #define IOMMU_CAP_CACHE_COHERENCY	0x1
 #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
 
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..a39e2d78f735 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     int index, unsigned long *busno, dma_addr_t *addr,
 			     size_t *size);
 
+extern void of_iommu_init(void);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -16,6 +21,24 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 	return -EINVAL;
 }
 
+static inline void of_iommu_init(void) { }
+
 #endif	/* CONFIG_OF_IOMMU */
 
+static inline void of_iommu_set_data(struct device_node *np,
+				     struct iommu_data *data)
+{
+	np->data = data;
+}
+
+static inline struct iommu_data *of_iommu_get_data(struct device_node *np)
+{
+	return np->data;
+}
+
+extern struct of_device_id __iommu_of_table;
+
+#define IOMMU_OF_DECLARE(name, compat, fn) \
+	OF_DECLARE_1(iommu, name, compat, fn)
+
 #endif /* __OF_IOMMU_H */
-- 
2.1.0

[RFC PATCH v2 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[Will Deacon]

set_arch_dma_coherent_ops is called from of_dma_configure in order to
swizzle the architectural dma-mapping functions over to a cache-coherent
implementation. This is currently implemented only for ARM.

In anticipation of re-using this mechanism for IOMMU-backed dma-mapping
ops too, this patch replaces the function with a broader
arch_setup_dma_ops callback which is also responsible for setting the
DMA mask and offset as well as selecting the correct mapping functions.

A further advantage of this split is that it nicely isolates the
of-specific code from the dma-mapping code, allowing potential reuse by
other buses (e.g. PCI) in the future.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/dma-mapping.h | 13 ++++++++----
 drivers/of/platform.c              | 42 ++++++++++----------------------------
 include/linux/dma-mapping.h        |  8 +++-----
 3 files changed, 23 insertions(+), 40 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c45b61a4b4a5..dad006dabbe6 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,12 +121,17 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline int set_arch_dma_coherent_ops(struct device *dev)
+static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
+				      unsigned long offset, bool coherent)
 {
-	set_dma_ops(dev, &arm_coherent_dma_ops);
-	return 0;
+	dev->coherent_dma_mask	= mask;
+	dev->dma_mask		= &dev->coherent_dma_mask;
+	dev->dma_pfn_offset	= offset;
+
+	if (coherent)
+		set_dma_ops(dev, &arm_coherent_dma_ops);
 }
-#define set_arch_dma_coherent_ops(dev)	set_arch_dma_coherent_ops(dev)
+#define arch_setup_dma_ops arch_setup_dma_ops
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0197725e033a..484c558c63a6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -164,43 +164,23 @@ static void of_dma_configure(struct platform_device *pdev)
 {
 	u64 dma_addr, paddr, size;
 	int ret;
+	bool coherent;
+	unsigned long offset;
 	struct device *dev = &pdev->dev;
 
-	/*
-	 * Set default dma-mask to 32 bit. Drivers are expected to setup
-	 * the correct supported dma_mask.
-	 */
-	dev->coherent_dma_mask = DMA_BIT_MASK(32);
-
-	/*
-	 * Set it to coherent_dma_mask by default if the architecture
-	 * code has not set it.
-	 */
-	if (!dev->dma_mask)
-		dev->dma_mask = &dev->coherent_dma_mask;
+	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+	offset = ret < 0 ? 0 : PFN_DOWN(paddr - dma_addr);
+	dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
 
-	/*
-	 * if dma-coherent property exist, call arch hook to setup
-	 * dma coherent operations.
-	 */
-	if (of_dma_is_coherent(dev->of_node)) {
-		set_arch_dma_coherent_ops(dev);
-		dev_dbg(dev, "device is dma coherent\n");
-	}
+	coherent = of_dma_is_coherent(dev->of_node);
+	dev_dbg(dev, "device is%sdma coherent\n",
+		coherent ? " " : " not ");
 
 	/*
-	 * if dma-ranges property doesn't exist - just return else
-	 * setup the dma offset
+	 * Set default dma-mask to 32 bit. Drivers are expected to setup
+	 * the correct supported dma_mask.
 	 */
-	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
-	if (ret < 0) {
-		dev_dbg(dev, "no dma range information to setup\n");
-		return;
-	}
-
-	/* DMA ranges found. Calculate and set dma_pfn_offset */
-	dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
-	dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
 }
 
 /**
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 931b70986272..0f7f7b68b0db 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -129,11 +129,9 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 
 extern u64 dma_get_required_mask(struct device *dev);
 
-#ifndef set_arch_dma_coherent_ops
-static inline int set_arch_dma_coherent_ops(struct device *dev)
-{
-	return 0;
-}
+#ifndef arch_setup_dma_ops
+static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
+				      unsigned long offset, bool coherent) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.0

[RFC PATCH v2 3/7] iommu: add new iommu_ops callback for adding an OF device

[Will Deacon]

This patch adds a new function to the iommu_ops structure to allow an
OF device to be added to a specific IOMMU instance using the recently
merged generic devicetree binding for IOMMUs. The callback (of_xlate)
takes a struct device representing the master and an of_phandle_args
representing the IOMMU and the correspondong IDs for the new master.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 include/linux/iommu.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fdddb14cd8f5..3e766b85daa3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -21,6 +21,7 @@
 
 #include <linux/errno.h>
 #include <linux/err.h>
+#include <linux/of.h>
 #include <linux/types.h>
 #include <trace/events/iommu.h>
 
@@ -136,6 +137,10 @@ struct iommu_ops {
 	/* Get the numer of window per domain */
 	u32 (*domain_get_windows)(struct iommu_domain *domain);
 
+#ifdef CONFIG_OF_IOMMU
+	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
+#endif
+
 	unsigned long pgsize_bitmap;
 };
 
-- 
2.1.0

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

The generic IOMMU device-tree bindings can be used to add arbitrary OF
masters to an IOMMU with a compliant binding.

This patch introduces of_iommu_configure, which does exactly that.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/of_iommu.c    | 52 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h |  7 ++++++
 include/linux/of_iommu.h    |  8 +++++++
 4 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index dd5112265cc9..6d13f962f156 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -15,7 +15,7 @@ if IOMMU_SUPPORT
 
 config OF_IOMMU
        def_bool y
-       depends on OF
+       depends on OF && IOMMU_API
 
 config FSL_PAMU
 	bool "Freescale IOMMU support"
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index f9209666157c..1188c929ffa7 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -18,6 +18,7 @@
  */
 
 #include <linux/export.h>
+#include <linux/iommu.h>
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
@@ -90,6 +91,57 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct iommu_dma_mapping *mapping;
+	struct bus_type *bus = dev->bus;
+	const struct iommu_ops *ops = bus->iommu_ops;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	if (!iommu_present(bus) || !ops->of_xlate)
+		return NULL;
+
+	/*
+	 * We don't currently walk up the tree looking for a parent IOMMU.
+	 * See the `Notes:' section of
+	 * Documentation/devicetree/bindings/iommu/iommu.txt
+	 */
+	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+					   "#iommu-cells", idx,
+					   &iommu_spec)) {
+		struct device_node *np = iommu_spec.np;
+		struct iommu_data *data = of_iommu_get_data(np);
+
+		if (!iommu) {
+			if (!ops->of_xlate(dev, &iommu_spec))
+				iommu = data;
+		} else if (iommu != data) {
+			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+				dev_name(dev));
+			iommu = NULL;
+		}
+
+		of_node_put(np);
+
+		if (!iommu)
+			break;
+
+		idx++;
+	}
+
+	if (!iommu)
+		return NULL;
+
+	mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)
+		return NULL;
+
+	mapping->iommu = iommu;
+	return mapping;
+}
+
 void __init of_iommu_init(void)
 {
 	struct device_node *np;
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0f7f7b68b0db..16bf92f71c3e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -62,6 +62,13 @@ struct dma_map_ops {
 	int is_phys;
 };
 
+struct iommu_data;
+
+struct iommu_dma_mapping {
+	struct iommu_data *iommu;
+	struct list_head node;
+};
+
 #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
 
 #define DMA_MASK_NONE	0x0ULL
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index a39e2d78f735..f2d3b1e780d7 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,9 +1,12 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/device.h>
 #include <linux/iommu.h>
 #include <linux/of.h>
 
+struct iommu_dma_mapping;
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -11,6 +14,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     size_t *size);
 
 extern void of_iommu_init(void);
+extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev);
 
 #else
 
@@ -22,6 +26,10 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 }
 
 static inline void of_iommu_init(void) { }
+static inline struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	return NULL;
+}
 
 #endif	/* CONFIG_OF_IOMMU */
 
-- 
2.1.0

[RFC PATCH v2 5/7] dma-mapping: detect and configure IOMMU in of_dma_configure

[Will Deacon]

This patch extends of_dma_configure so that it sets up the IOMMU for a
device, as well as the coherent/non-coherent DMA mapping ops.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/dma-mapping.h |  5 ++++-
 drivers/of/platform.c              | 36 ++++++++++++++++++++++--------------
 include/linux/dma-mapping.h        |  5 ++++-
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index dad006dabbe6..788b8eef40e8 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -122,7 +122,10 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
 static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
-				      unsigned long offset, bool coherent)
+				      u64 dma_base, u64 size,
+				      unsigned long offset,
+				      struct iommu_dma_mapping *iommu,
+				      bool coherent)
 {
 	dev->coherent_dma_mask	= mask;
 	dev->dma_mask		= &dev->coherent_dma_mask;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 484c558c63a6..6067f6423674 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -162,25 +163,37 @@ EXPORT_SYMBOL(of_device_alloc);
  */
 static void of_dma_configure(struct platform_device *pdev)
 {
-	u64 dma_addr, paddr, size;
+	u64 dma_addr, paddr, size, mask;
 	int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 
+	/*
+	 * Set default dma-mask to 32 bit. Drivers are expected to setup
+	 * the correct supported dma_mask.
+	 */
+	mask = DMA_BIT_MASK(32);
+
 	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
-	offset = ret < 0 ? 0 : PFN_DOWN(paddr - dma_addr);
-	dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	if (ret < 0) {
+		dma_addr = offset = 0;
+		size = mask;
+	} else {
+		offset = PFN_DOWN(paddr - dma_addr);
+		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	}
 
 	coherent = of_dma_is_coherent(dev->of_node);
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	/*
-	 * Set default dma-mask to 32 bit. Drivers are expected to setup
-	 * the correct supported dma_mask.
-	 */
-	arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, mask, dma_addr, size, offset, iommu, coherent);
 }
 
 /**
@@ -209,14 +222,9 @@ static struct platform_device *of_platform_device_create_pdata(
 	if (!dev)
 		goto err_clear_flag;
 
-	of_dma_configure(dev);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
-
-	/* We do not fill the DMA ops for platform devices by default.
-	 * This is currently the responsibility of the platform code
-	 * to do such, possibly using a device notifier
-	 */
+	of_dma_configure(dev);
 
 	if (of_device_add(dev) != 0) {
 		platform_device_put(dev);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 16bf92f71c3e..19a31f55a5d0 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -138,7 +138,10 @@ extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef arch_setup_dma_ops
 static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
-				      unsigned long offset, bool coherent) { }
+				      u64 dma_base, u64 size,
+				      unsigned long offset,
+				      struct iommu_dma_mapping *iommu,
+				      bool coherent) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.0

[RFC PATCH v2 6/7] arm: call iommu_init before of_platform_populate

[Will Deacon]

We need to ensure that the IOMMUs in the system have a chance to perform
some basic initialisation before we start adding masters to them.

This patch adds a call to of_iommu_init before of_platform_populate.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/setup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 84db893dedc2..38a0203523af 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -18,6 +18,7 @@
 #include <linux/bootmem.h>
 #include <linux/seq_file.h>
 #include <linux/screen_info.h>
+#include <linux/of_iommu.h>
 #include <linux/of_platform.h>
 #include <linux/init.h>
 #include <linux/kexec.h>
@@ -803,9 +804,11 @@ static int __init customize_machine(void)
 	if (machine_desc->init_machine)
 		machine_desc->init_machine();
 #ifdef CONFIG_OF
-	else
+	else {
+		of_iommu_init();
 		of_platform_populate(NULL, of_default_bus_match_table,
 					NULL, NULL);
+	}
 #endif
 	return 0;
 }
-- 
2.1.0

[RFC PATCH v2 7/7] arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

[Will Deacon]

This patch plumbs the existing ARM IOMMU DMA infrastructure (which isn't
actually called outside of a few drivers) into arch_setup_dma_ops, so
that we can use IOMMUs for DMA transfers in a more generic fashion.

Since this significantly complicates the arch_setup_dma_ops function,
it is moved out of line into dma-mapping.c. If CONFIG_ARM_DMA_USE_IOMMU
is not set, the iommu paramater is ignored and the normal ops are used
instead.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/dma-mapping.h | 17 +++------
 arch/arm/mm/dma-mapping.c          | 72 +++++++++++++++++++++++++++++++++-----
 2 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 788b8eef40e8..2f648b9b000d 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,20 +121,11 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
-				      u64 dma_base, u64 size,
-				      unsigned long offset,
-				      struct iommu_dma_mapping *iommu,
-				      bool coherent)
-{
-	dev->coherent_dma_mask	= mask;
-	dev->dma_mask		= &dev->coherent_dma_mask;
-	dev->dma_pfn_offset	= offset;
-
-	if (coherent)
-		set_dma_ops(dev, &arm_coherent_dma_ops);
-}
 #define arch_setup_dma_ops arch_setup_dma_ops
+extern void arch_setup_dma_ops(struct device *dev, u64 mask, u64 dma_base,
+			       u64 size, unsigned long offset,
+			       struct iommu_dma_mapping *iommu,
+			       bool coherent);
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7a996aaa061e..07179b2003eb 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2041,10 +2041,9 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
  * @mapping: io address space mapping structure (returned from
  *	arm_iommu_create_mapping)
  *
- * Attaches specified io address space mapping to the provided device,
- * this replaces the dma operations (dma_map_ops pointer) with the
- * IOMMU aware version. More than one client might be attached to
- * the same io address space mapping.
+ * Attaches specified io address space mapping to the provided device.
+ * More than one client might be attached to the same io address space
+ * mapping.
  */
 int arm_iommu_attach_device(struct device *dev,
 			    struct dma_iommu_mapping *mapping)
@@ -2057,7 +2056,6 @@ int arm_iommu_attach_device(struct device *dev,
 
 	kref_get(&mapping->kref);
 	dev->archdata.mapping = mapping;
-	set_dma_ops(dev, &iommu_ops);
 
 	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
 	return 0;
@@ -2069,7 +2067,6 @@ EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
  * @dev: valid struct device pointer
  *
  * Detaches the provided device from a previously attached map.
- * This voids the dma operations (dma_map_ops pointer)
  */
 void arm_iommu_detach_device(struct device *dev)
 {
@@ -2084,10 +2081,69 @@ void arm_iommu_detach_device(struct device *dev)
 	iommu_detach_device(mapping->domain, dev);
 	kref_put(&mapping->kref, release_iommu_mapping);
 	dev->archdata.mapping = NULL;
-	set_dma_ops(dev, NULL);
 
 	pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));
 }
 EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
 
-#endif
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
+{
+	struct dma_iommu_mapping *mapping;
+
+	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
+	if (IS_ERR(mapping)) {
+		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
+				size, dev_name(dev));
+		return false;
+	}
+
+	if (arm_iommu_attach_device(dev, mapping)) {
+		pr_warn("Failed to attach device %s to IOMMU mapping\n",
+				dev_name(dev));
+		arm_iommu_release_mapping(mapping);
+		return false;
+	}
+
+	return true;
+}
+
+static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
+{
+	return coherent ? &iommu_coherent_ops : &iommu_ops;
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
+{
+	return false;
+}
+
+#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
+
+#endif /* CONFIG_ARM_DMA_USE_IOMMU */
+
+static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
+{
+	return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
+}
+
+void arch_setup_dma_ops(struct device *dev, u64 mask, u64 dma_base, u64 size,
+			unsigned long offset, struct iommu_dma_mapping *iommu,
+			bool coherent)
+{
+	struct dma_map_ops *dma_ops;
+
+	dev->coherent_dma_mask	= mask;
+	dev->dma_mask		= &dev->coherent_dma_mask;
+
+	if (iommu && arm_setup_iommu_dma_ops(dev, dma_base, size)) {
+		dma_ops = arm_get_iommu_dma_map_ops(coherent);
+	} else {
+		dev->dma_pfn_offset = offset;
+		dma_ops = arm_get_dma_map_ops(coherent);
+	}
+
+	set_dma_ops(dev, dma_ops);
+}
+EXPORT_SYMBOL_GPL(arch_setup_dma_ops);
-- 
2.1.0

[RFC PATCH v2 6/7] arm: call iommu_init before of_platform_populate

[Arnd Bergmann]

On Tuesday 02 September 2014 18:56:26 Will Deacon wrote:
> @@ -803,9 +804,11 @@ static int __init customize_machine(void)
>         if (machine_desc->init_machine)
>                 machine_desc->init_machine();
>  #ifdef CONFIG_OF
> -       else
> +       else {
> +               of_iommu_init();
>                 of_platform_populate(NULL, of_default_bus_match_table,
>                                         NULL, NULL);
> +       }
>  #endif
>         return 0;
>  }

I think it would be better to not have this depend on the presence
of an init_machine callback. Why not move of_iommu_init() in front
of the if()? It shouldn't do anything unless we have an iommu
driver that we should be using.

	Arnd

[RFC PATCH v2 7/7] arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

[Arnd Bergmann]

On Tuesday 02 September 2014 18:56:27 Will Deacon wrote:
> This patch plumbs the existing ARM IOMMU DMA infrastructure (which isn't
> actually called outside of a few drivers) into arch_setup_dma_ops, so
> that we can use IOMMUs for DMA transfers in a more generic fashion.
> 
> Since this significantly complicates the arch_setup_dma_ops function,
> it is moved out of line into dma-mapping.c. If CONFIG_ARM_DMA_USE_IOMMU
> is not set, the iommu paramater is ignored and the normal ops are used
> instead.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> 

Looks great to me.

	Arnd

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Tuesday 02 September 2014 18:56:24 Will Deacon wrote:
> +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> +{
> +       struct of_phandle_args iommu_spec;
> +       struct iommu_dma_mapping *mapping;
> +       struct bus_type *bus = dev->bus;
> +       const struct iommu_ops *ops = bus->iommu_ops;

I think it would be best to not even introduce the tight coupling
between bus_type and iommu_ops here, one of the main reasons we
are doing this is to break that connection.

Better put the iommu_ops into the iommu_data pointer that gets looked
up per iommu device.

> +       struct iommu_data *iommu = NULL;
> +       int idx = 0;
> +
> +       if (!iommu_present(bus) || !ops->of_xlate)
> +               return NULL;
> +
> +       /*
> +        * We don't currently walk up the tree looking for a parent IOMMU.
> +        * See the `Notes:' section of
> +        * Documentation/devicetree/bindings/iommu/iommu.txt
> +        */
> +        */
> +       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +                                          "#iommu-cells", idx,
> +                                          &iommu_spec)) {
> +               struct device_node *np = iommu_spec.np;
> +               struct iommu_data *data = of_iommu_get_data(np);
> +
> +               if (!iommu) {
> +                       if (!ops->of_xlate(dev, &iommu_spec))
> +                               iommu = data;

I would make the first argument of the of_xlate function the iommu_data,
so the code can find the right instance.

Maybe also add an extra argument at the end that can be used by the
PCI code and potentially other drivers with multiple master IDs
behind one "iommus" property to pass some value identifying which of
the masters is meant.

The format of that is unfortunately bus specific and our platform_bus
really covers a number of different hardware buses (AXI, AHB, OPB, ...)
but the caller should be able to provide the data in the right form
that the iommu understands. I would try using a single u64 argument
as a start, hoping that this covers all the buses we need. At least
it's enough for PCI (bus/device/function) and for AXI (requester-id?).

> +               } else if (iommu != data) {
> +                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> +                               dev_name(dev));
> +                       iommu = NULL;
> +               }
> +
> +               of_node_put(np);
> +
> +               if (!iommu)
> +                       break;
> +
> +               idx++;
> +       }
> +
> +       if (!iommu)
> +               return NULL;
> +
> +       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
> +       if (!mapping)
> +               return NULL;
> 

I don't think it's safe to use devm_* functions here: this is during
device discovery, and this data will be freed if probe() fails or
the device gets removed from a driver.

	Arnd

[RFC PATCH v2 0/7] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Tuesday 02 September 2014 18:56:20 Will Deacon wrote:
> The main difference since v1 is that I've introduced some generic
> structures to represent IOMMUs and their mappings (struct iommu_data and
> struct iommu_dma_mapping). This allows us to propagate the per-instance
> data all the way down to the DMA-mapping code, which can then manage a
> per-instance domain. Note that the ARM dma-mapping implementation does
> not currently make use of this information.
> 

Overall I'm pretty happy with how this is turning out so far.
I have commented on a few patches, everything else looks good to
me.

	Arnd

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

Hi Arnd,

Thanks for the review.

On Tue, Sep 02, 2014 at 08:10:10PM +0100, Arnd Bergmann wrote:
> On Tuesday 02 September 2014 18:56:24 Will Deacon wrote:
> > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> > +{
> > +       struct of_phandle_args iommu_spec;
> > +       struct iommu_dma_mapping *mapping;
> > +       struct bus_type *bus = dev->bus;
> > +       const struct iommu_ops *ops = bus->iommu_ops;
> 
> I think it would be best to not even introduce the tight coupling
> between bus_type and iommu_ops here, one of the main reasons we
> are doing this is to break that connection.
> 
> Better put the iommu_ops into the iommu_data pointer that gets looked
> up per iommu device.

Yes, I'll add that. It's a bit weird, because those same ops will later
be duplicated in iommu_data->domain->ops, but that's an artifact of how
the domain is currently constructed by iommu_domain_alloc.

> > +       struct iommu_data *iommu = NULL;
> > +       int idx = 0;
> > +
> > +       if (!iommu_present(bus) || !ops->of_xlate)
> > +               return NULL;
> > +
> > +       /*
> > +        * We don't currently walk up the tree looking for a parent IOMMU.
> > +        * See the `Notes:' section of
> > +        * Documentation/devicetree/bindings/iommu/iommu.txt
> > +        */
> > +        */
> > +       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> > +                                          "#iommu-cells", idx,
> > +                                          &iommu_spec)) {
> > +               struct device_node *np = iommu_spec.np;
> > +               struct iommu_data *data = of_iommu_get_data(np);
> > +
> > +               if (!iommu) {
> > +                       if (!ops->of_xlate(dev, &iommu_spec))
> > +                               iommu = data;
> 
> I would make the first argument of the of_xlate function the iommu_data,
> so the code can find the right instance.

Oops, that's what I intended. Well spotted.

> Maybe also add an extra argument at the end that can be used by the
> PCI code and potentially other drivers with multiple master IDs
> behind one "iommus" property to pass some value identifying which of
> the masters is meant.
> 
> The format of that is unfortunately bus specific and our platform_bus
> really covers a number of different hardware buses (AXI, AHB, OPB, ...)
> but the caller should be able to provide the data in the right form
> that the iommu understands. I would try using a single u64 argument
> as a start, hoping that this covers all the buses we need. At least
> it's enough for PCI (bus/device/function) and for AXI (requester-id?).

Yeah, I think that will work. It's kind of like a device `index' for a
device that sits behind a bridge (trying to avoid yet another ID parameter
:).

> 
> > +               } else if (iommu != data) {
> > +                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> > +                               dev_name(dev));
> > +                       iommu = NULL;
> > +               }
> > +
> > +               of_node_put(np);
> > +
> > +               if (!iommu)
> > +                       break;
> > +
> > +               idx++;
> > +       }
> > +
> > +       if (!iommu)
> > +               return NULL;
> > +
> > +       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
> > +       if (!mapping)
> > +               return NULL;
> > 
> 
> I don't think it's safe to use devm_* functions here: this is during
> device discovery, and this data will be freed if probe() fails or
> the device gets removed from a driver.

So I can make this a standard kzalloc, but I have no idea where the
corresponding kfree should live. Is there something equivalent to
of_dma_unconfigure, or is this data that is expected to persist?

Will

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Thursday 04 September 2014 12:26:25 Will Deacon wrote:
> > 
> > > +               } else if (iommu != data) {
> > > +                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> > > +                               dev_name(dev));
> > > +                       iommu = NULL;
> > > +               }
> > > +
> > > +               of_node_put(np);
> > > +
> > > +               if (!iommu)
> > > +                       break;
> > > +
> > > +               idx++;
> > > +       }
> > > +
> > > +       if (!iommu)
> > > +               return NULL;
> > > +
> > > +       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
> > > +       if (!mapping)
> > > +               return NULL;
> > > 
> > 
> > I don't think it's safe to use devm_* functions here: this is during
> > device discovery, and this data will be freed if probe() fails or
> > the device gets removed from a driver.
> 
> So I can make this a standard kzalloc, but I have no idea where the
> corresponding kfree should live. Is there something equivalent to
> of_dma_unconfigure, or is this data that is expected to persist?
> 

Can this be a simple kfree in of_platform_device_create_pdata?

	Arnd

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

On Thu, Sep 04, 2014 at 12:59:50PM +0100, Arnd Bergmann wrote:
> On Thursday 04 September 2014 12:26:25 Will Deacon wrote:
> > > 
> > > > +               } else if (iommu != data) {
> > > > +                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> > > > +                               dev_name(dev));
> > > > +                       iommu = NULL;
> > > > +               }
> > > > +
> > > > +               of_node_put(np);
> > > > +
> > > > +               if (!iommu)
> > > > +                       break;
> > > > +
> > > > +               idx++;
> > > > +       }
> > > > +
> > > > +       if (!iommu)
> > > > +               return NULL;
> > > > +
> > > > +       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
> > > > +       if (!mapping)
> > > > +               return NULL;
> > > > 
> > > 
> > > I don't think it's safe to use devm_* functions here: this is during
> > > device discovery, and this data will be freed if probe() fails or
> > > the device gets removed from a driver.
> > 
> > So I can make this a standard kzalloc, but I have no idea where the
> > corresponding kfree should live. Is there something equivalent to
> > of_dma_unconfigure, or is this data that is expected to persist?
> > 
> 
> Can this be a simple kfree in of_platform_device_create_pdata?

We could certainly hook into the error path there (when of_device_add
fails), yes, but I was also thinking about device removal and whether we
need to care about that. I guess not for the OF case.

Will

[RFC PATCH v2 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[Grygorii Strashko]

Hi Will,

On 09/02/2014 08:56 PM, Will Deacon wrote:
> set_arch_dma_coherent_ops is called from of_dma_configure in order to
> swizzle the architectural dma-mapping functions over to a cache-coherent
> implementation. This is currently implemented only for ARM.
> 
> In anticipation of re-using this mechanism for IOMMU-backed dma-mapping
> ops too, this patch replaces the function with a broader
> arch_setup_dma_ops callback which is also responsible for setting the
> DMA mask and offset as well as selecting the correct mapping functions.
> 
> A further advantage of this split is that it nicely isolates the
> of-specific code from the dma-mapping code, allowing potential reuse by
> other buses (e.g. PCI) in the future.

I think this patch can introduce a regression if it will be used as is :(

When this code was initially created there ware a lot of discussion about
and finally it was decided to configure all common (for all arches) DMA 
specific parameters for devices in common code, while strictly arch 
specific things using arch specific APIs/callbacks.

The following parameters are common now:
dev->coherent_dma_mask	= mask;
dev->dma_mask		= &dev->coherent_dma_mask;
dev->dma_pfn_offset	= offset;

and they need to be set always, otherwise it will affect on other
DT-based arches.

Links:
- [PATCH v2 0/7] ARM: dma: Support dma-ranges and dma-coherent
http://www.spinics.net/lists/arm-kernel/msg311678.html

- [PATCH 0/7] of: setup dma parameters using dma-ranges and dma-coherent
https://lkml.org/lkml/2014/3/6/186

- [PATCH v2 0/7] of: setup dma parameters using dma-ranges and dma-coherent
https://lkml.org/lkml/2014/4/19/80

- [PATCH v3 0/7] of: setup dma parameters using dma-ranges and dma-coherent
http://thread.gmane.org/gmane.linux.kernel/1690224/focus=319246

> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>   arch/arm/include/asm/dma-mapping.h | 13 ++++++++----
>   drivers/of/platform.c              | 42 ++++++++++----------------------------
>   include/linux/dma-mapping.h        |  8 +++-----
>   3 files changed, 23 insertions(+), 40 deletions(-)
> 

[...]

> +	arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
>   }
>   
>   /**
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 931b70986272..0f7f7b68b0db 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -129,11 +129,9 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
>   
>   extern u64 dma_get_required_mask(struct device *dev);
>   
> -#ifndef set_arch_dma_coherent_ops
> -static inline int set_arch_dma_coherent_ops(struct device *dev)
> -{
> -	return 0;
> -}
> +#ifndef arch_setup_dma_ops
> +static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
> +				      unsigned long offset, bool coherent) { }
>   #endif
>   
>   static inline unsigned int dma_get_max_seg_size(struct device *dev)
> 

Regards,
-grygorii

[RFC PATCH v2 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[Will Deacon]

On Fri, Sep 05, 2014 at 04:37:41PM +0100, Grygorii Strashko wrote:
> Hi Will,

Hi Grygorii,

> On 09/02/2014 08:56 PM, Will Deacon wrote:
> > set_arch_dma_coherent_ops is called from of_dma_configure in order to
> > swizzle the architectural dma-mapping functions over to a cache-coherent
> > implementation. This is currently implemented only for ARM.
> > 
> > In anticipation of re-using this mechanism for IOMMU-backed dma-mapping
> > ops too, this patch replaces the function with a broader
> > arch_setup_dma_ops callback which is also responsible for setting the
> > DMA mask and offset as well as selecting the correct mapping functions.
> > 
> > A further advantage of this split is that it nicely isolates the
> > of-specific code from the dma-mapping code, allowing potential reuse by
> > other buses (e.g. PCI) in the future.
> 
> I think this patch can introduce a regression if it will be used as is :(
> 
> When this code was initially created there ware a lot of discussion about
> and finally it was decided to configure all common (for all arches) DMA 
> specific parameters for devices in common code, while strictly arch 
> specific things using arch specific APIs/callbacks.
> 
> The following parameters are common now:
> dev->coherent_dma_mask	= mask;
> dev->dma_mask		= &dev->coherent_dma_mask;
> dev->dma_pfn_offset	= offset;
> 
> and they need to be set always, otherwise it will affect on other
> DT-based arches.

Ok, in which case we can either configure the struct device in
of_dma_configure before calling arch_setup_dma_ops, or we can do the work
in a generic version of arch_setup_dma_ops (that I haven't added yet).

The former is how it's done in mainline atm, so I'll stick with that.

Thanks,

Will

[RFC PATCH v2 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[Grygorii Strashko]

Hi Will,

On 09/08/2014 01:31 PM, Will Deacon wrote:
> On Fri, Sep 05, 2014 at 04:37:41PM +0100, Grygorii Strashko wrote:
> 
>> On 09/02/2014 08:56 PM, Will Deacon wrote:
>>> set_arch_dma_coherent_ops is called from of_dma_configure in order to
>>> swizzle the architectural dma-mapping functions over to a cache-coherent
>>> implementation. This is currently implemented only for ARM.
>>>
>>> In anticipation of re-using this mechanism for IOMMU-backed dma-mapping
>>> ops too, this patch replaces the function with a broader
>>> arch_setup_dma_ops callback which is also responsible for setting the
>>> DMA mask and offset as well as selecting the correct mapping functions.
>>>
>>> A further advantage of this split is that it nicely isolates the
>>> of-specific code from the dma-mapping code, allowing potential reuse by
>>> other buses (e.g. PCI) in the future.
>>
>> I think this patch can introduce a regression if it will be used as is :(
>>
>> When this code was initially created there ware a lot of discussion about
>> and finally it was decided to configure all common (for all arches) DMA
>> specific parameters for devices in common code, while strictly arch
>> specific things using arch specific APIs/callbacks.
>>
>> The following parameters are common now:
>> dev->coherent_dma_mask	= mask;
>> dev->dma_mask		= &dev->coherent_dma_mask;
>> dev->dma_pfn_offset	= offset;
>>
>> and they need to be set always, otherwise it will affect on other
>> DT-based arches.
> 
> Ok, in which case we can either configure the struct device in
> of_dma_configure before calling arch_setup_dma_ops, or we can do the work
> in a generic version of arch_setup_dma_ops (that I haven't added yet).

Just think that it would more simpler to add arch-API:
  arch_setup_iommu(struct device *dev, struct iommu_dma_mapping *iommu)

and call it at the end of of_dma_configure() as following:

  arch_setup_iommu(dev, of_iommu_configure(dev));

so it will setup IOMMU parameters for device and overwrite DMA
parameters respectively.

if IOMMU is not present/disabled arch_setup_iommu() will be a NOP and 
system will roll back to default (old) behavior.

Also it will simplify ARM dma-mapping changes -
 only IOMMU code will need to be added.

Best regards,
-grygorii

[RFC PATCH v2 3/7] iommu: add new iommu_ops callback for adding an OF device

[Marek Szyprowski]

Hello,

On 2014-09-02 19:56, Will Deacon wrote:
> This patch adds a new function to the iommu_ops structure to allow an
> OF device to be added to a specific IOMMU instance using the recently
> merged generic devicetree binding for IOMMUs. The callback (of_xlate)
> takes a struct device representing the master and an of_phandle_args
> representing the IOMMU and the correspondong IDs for the new master.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>   include/linux/iommu.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fdddb14cd8f5..3e766b85daa3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -21,6 +21,7 @@
>   
>   #include <linux/errno.h>
>   #include <linux/err.h>
> +#include <linux/of.h>
>   #include <linux/types.h>
>   #include <trace/events/iommu.h>
>   
> @@ -136,6 +137,10 @@ struct iommu_ops {
>   	/* Get the numer of window per domain */
>   	u32 (*domain_get_windows)(struct iommu_domain *domain);
>   
> +#ifdef CONFIG_OF_IOMMU
> +	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);

If I understand correctly how it is designed to work, then it should be:

struct iommu_data *(*of_xlate)(struct device *dev, struct 
of_phandle_args *args);

> +#endif
> +
>   	unsigned long pgsize_bitmap;
>   };
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH v2 3/7] iommu: add new iommu_ops callback for adding an OF device

[Will Deacon]

On Wed, Sep 10, 2014 at 12:16:06PM +0100, Marek Szyprowski wrote:
> On 2014-09-02 19:56, Will Deacon wrote:
> > This patch adds a new function to the iommu_ops structure to allow an
> > OF device to be added to a specific IOMMU instance using the recently
> > merged generic devicetree binding for IOMMUs. The callback (of_xlate)
> > takes a struct device representing the master and an of_phandle_args
> > representing the IOMMU and the correspondong IDs for the new master.
> >
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >   include/linux/iommu.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index fdddb14cd8f5..3e766b85daa3 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -21,6 +21,7 @@
> >   
> >   #include <linux/errno.h>
> >   #include <linux/err.h>
> > +#include <linux/of.h>
> >   #include <linux/types.h>
> >   #include <trace/events/iommu.h>
> >   
> > @@ -136,6 +137,10 @@ struct iommu_ops {
> >   	/* Get the numer of window per domain */
> >   	u32 (*domain_get_windows)(struct iommu_domain *domain);
> >   
> > +#ifdef CONFIG_OF_IOMMU
> > +	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
> 
> If I understand correctly how it is designed to work, then it should be:
> 
> struct iommu_data *(*of_xlate)(struct device *dev, struct 
> of_phandle_args *args);

Yeah, that might be cleaner than the of_iommu_get_data call in
of_dma_configure. I'm currently cooking a v3, so I'll include this change.

Will

[RFC PATCH v2 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Marek Szyprowski]

Hello,

On 2014-09-02 19:56, Will Deacon wrote:
> IOMMU drivers must be initialised before any of their upstream devices,
> otherwise the relevant iommu_ops won't be configured for the bus in
> question. To solve this, a number of IOMMU drivers use initcalls to
> initialise the driver before anything has a chance to be probed.
>
> Whilst this solves the immediate problem, it leaves the job of probing
> the IOMMU completely separate from the iommu_ops to configure the IOMMU,
> which are called on a per-bus basis and require the driver to figure out
> exactly which instance of the IOMMU is being requested. In particular,
> the add_device callback simply passes a struct device to the driver,
> which then has to parse firmware tables or probe buses to identify the
> relevant IOMMU instance.
>
> This patch takes the first step in addressing this problem by adding an
> early initialisation pass for IOMMU drivers, giving them the ability to
> set some per-instance data on their of_node in the form of a new
> iommu_data structure. This can later be used when parsing OF masters to
> identify the IOMMU instance in question.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>   drivers/iommu/of_iommu.c          | 14 ++++++++++++++
>   include/asm-generic/vmlinux.lds.h |  2 ++
>   include/linux/iommu.h             |  6 ++++++
>   include/linux/of_iommu.h          | 23 +++++++++++++++++++++++
>   4 files changed, 45 insertions(+)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e550ccb7634e..f9209666157c 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -89,3 +89,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(of_get_dma_window);
> +
> +void __init of_iommu_init(void)
> +{
> +	struct device_node *np;
> +	const struct of_device_id *match, *matches = &__iommu_of_table;
> +
> +	for_each_matching_node_and_match(np, matches, &match) {
> +		const int (*init_fn)(struct device_node *) = match->data;
> +
> +		if (init_fn(np))
> +			pr_err("Failed to initialise IOMMU %s\n",
> +				of_node_full_name(np));
> +	}
> +}
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 5ba0360663a7..b75ede8f99ae 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -162,6 +162,7 @@
>   #define CLKSRC_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
>   #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
>   #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
> +#define IOMMU_OF_TABLES()	OF_TABLE(CONFIG_OF_IOMMU, iommu)
>   #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
>   #define CPU_METHOD_OF_TABLES()	OF_TABLE(CONFIG_SMP, cpu_method)
>   #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
> @@ -495,6 +496,7 @@
>   	CLK_OF_TABLES()							\
>   	RESERVEDMEM_OF_TABLES()						\
>   	CLKSRC_OF_TABLES()						\
> +	IOMMU_OF_TABLES()						\
>   	CPU_METHOD_OF_TABLES()						\
>   	KERNEL_DTB()							\
>   	IRQCHIP_OF_MATCH_TABLE()					\
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20f9a527922a..fdddb14cd8f5 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -57,6 +57,12 @@ struct iommu_domain {
>   	struct iommu_domain_geometry geometry;
>   };
>   
> +struct iommu_data {
> +	struct iommu_domain *domain;
> +	struct iova_domain *iovad;
> +	void *priv;
> +};
> +
>   #define IOMMU_CAP_CACHE_COHERENCY	0x1
>   #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
>   
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 51a560f34bca..a39e2d78f735 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -1,12 +1,17 @@
>   #ifndef __OF_IOMMU_H
>   #define __OF_IOMMU_H
>   
> +#include <linux/iommu.h>
> +#include <linux/of.h>
> +
>   #ifdef CONFIG_OF_IOMMU
>   
>   extern int of_get_dma_window(struct device_node *dn, const char *prefix,
>   			     int index, unsigned long *busno, dma_addr_t *addr,
>   			     size_t *size);
>   
> +extern void of_iommu_init(void);
> +
>   #else
>   
>   static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
> @@ -16,6 +21,24 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
>   	return -EINVAL;
>   }
>   
> +static inline void of_iommu_init(void) { }
> +
>   #endif	/* CONFIG_OF_IOMMU */
>   
> +static inline void of_iommu_set_data(struct device_node *np,
> +				     struct iommu_data *data)
> +{
> +	np->data = data;
> +}
> +
> +static inline struct iommu_data *of_iommu_get_data(struct device_node *np)
> +{
> +	return np->data;
> +}
> +
> +extern struct of_device_id __iommu_of_table;
> +
> +#define IOMMU_OF_DECLARE(name, compat, fn) \
> +	OF_DECLARE_1(iommu, name, compat, fn)

OF_DECLARE_1 assumes that init_fn doesn't return anything, however in of_iommu_init()
you expect the init function to return an error code, so the above lines should be
changed to:

typedef int (*of_iommu_init_fn)(struct device_node *);

#define IOMMU_OF_DECLARE(name, compat, fn) \
         _OF_DECLARE(iommu, name, compat, fn, of_iommu_init_fn)


> +
>   #endif /* __OF_IOMMU_H */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH v2 3/7] iommu: add new iommu_ops callback for adding an OF device

[Will Deacon]

On Wed, Sep 10, 2014 at 12:22:13PM +0100, Will Deacon wrote:
> On Wed, Sep 10, 2014 at 12:16:06PM +0100, Marek Szyprowski wrote:
> > On 2014-09-02 19:56, Will Deacon wrote:
> > > +#ifdef CONFIG_OF_IOMMU
> > > +	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
> > 
> > If I understand correctly how it is designed to work, then it should be:
> > 
> > struct iommu_data *(*of_xlate)(struct device *dev, struct 
> > of_phandle_args *args);
> 
> Yeah, that might be cleaner than the of_iommu_get_data call in
> of_dma_configure. I'm currently cooking a v3, so I'll include this change.

Actually, I spoke too soon. If we make of_xlate return the iommu_data, then
we have to continue getting the iommu_ops from the bus_type, otherwise we
have no way to call the right of_xlate.

So I'd rather leave of_dma_configure using of_iommu_get_data (I'm currently
ripping out the bus code in there) so that we can embed the iommu_ops in
iommu_data instead.

Will

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Laurent Pinchart]

Hi Will,

Thank you for the patch.

On Tuesday 02 September 2014 18:56:24 Will Deacon wrote:
> The generic IOMMU device-tree bindings can be used to add arbitrary OF
> masters to an IOMMU with a compliant binding.
> 
> This patch introduces of_iommu_configure, which does exactly that.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  drivers/iommu/Kconfig       |  2 +-
>  drivers/iommu/of_iommu.c    | 52 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-mapping.h |  7 ++++++
>  include/linux/of_iommu.h    |  8 +++++++
>  4 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index dd5112265cc9..6d13f962f156 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -15,7 +15,7 @@ if IOMMU_SUPPORT
> 
>  config OF_IOMMU
>         def_bool y
> -       depends on OF
> +       depends on OF && IOMMU_API
> 
>  config FSL_PAMU
>  	bool "Freescale IOMMU support"
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index f9209666157c..1188c929ffa7 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -18,6 +18,7 @@
>   */
> 
>  #include <linux/export.h>
> +#include <linux/iommu.h>
>  #include <linux/limits.h>
>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
> @@ -90,6 +91,57 @@ int of_get_dma_window(struct device_node *dn, const char
> *prefix, int index, }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
> 
> +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> +{
> +	struct of_phandle_args iommu_spec;
> +	struct iommu_dma_mapping *mapping;
> +	struct bus_type *bus = dev->bus;
> +	const struct iommu_ops *ops = bus->iommu_ops;
> +	struct iommu_data *iommu = NULL;
> +	int idx = 0;
> +
> +	if (!iommu_present(bus) || !ops->of_xlate)
> +		return NULL;
> +
> +	/*
> +	 * We don't currently walk up the tree looking for a parent IOMMU.
> +	 * See the `Notes:' section of
> +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> +	 */
> +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +					   "#iommu-cells", idx,
> +					   &iommu_spec)) {
> +		struct device_node *np = iommu_spec.np;
> +		struct iommu_data *data = of_iommu_get_data(np);
> +
> +		if (!iommu) {
> +			if (!ops->of_xlate(dev, &iommu_spec))
> +				iommu = data;

If I understand the code correctly, this will call of_xlate for the first 
IOMMU reference only and silently ignore all subsequent references if they 
point to the same IOMMU device but with different stream/requester IDs. Is 
that the desired behaviour ?

> +		} else if (iommu != data) {
> +			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> +				dev_name(dev));
> +			iommu = NULL;
> +		}
> +
> +		of_node_put(np);
> +
> +		if (!iommu)
> +			break;
> +
> +		idx++;
> +	}
> +
> +	if (!iommu)
> +		return NULL;
> +
> +	mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
> +	if (!mapping)
> +		return NULL;
> +
> +	mapping->iommu = iommu;
> +	return mapping;
> +}
> +
>  void __init of_iommu_init(void)
>  {
>  	struct device_node *np;
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 0f7f7b68b0db..16bf92f71c3e 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -62,6 +62,13 @@ struct dma_map_ops {
>  	int is_phys;
>  };
> 
> +struct iommu_data;
> +
> +struct iommu_dma_mapping {
> +	struct iommu_data *iommu;
> +	struct list_head node;
> +};
> +
>  #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> 
>  #define DMA_MASK_NONE	0x0ULL
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index a39e2d78f735..f2d3b1e780d7 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -1,9 +1,12 @@
>  #ifndef __OF_IOMMU_H
>  #define __OF_IOMMU_H
> 
> +#include <linux/device.h>
>  #include <linux/iommu.h>
>  #include <linux/of.h>
> 
> +struct iommu_dma_mapping;
> +
>  #ifdef CONFIG_OF_IOMMU
> 
>  extern int of_get_dma_window(struct device_node *dn, const char *prefix,
> @@ -11,6 +14,7 @@ extern int of_get_dma_window(struct device_node *dn, const
> char *prefix, size_t *size);
> 
>  extern void of_iommu_init(void);
> +extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev);
> 
>  #else
> 
> @@ -22,6 +26,10 @@ static inline int of_get_dma_window(struct device_node
> *dn, const char *prefix, }
> 
>  static inline void of_iommu_init(void) { }
> +static inline struct iommu_dma_mapping *of_iommu_configure(struct device
> *dev) +{
> +	return NULL;
> +}
> 
>  #endif	/* CONFIG_OF_IOMMU */

-- 
Regards,

Laurent Pinchart

[RFC PATCH v2 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

Hi Laurent,

Cheers for the review.

On Wed, Sep 10, 2014 at 02:01:34PM +0100, Laurent Pinchart wrote:
> On Tuesday 02 September 2014 18:56:24 Will Deacon wrote:
> > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> > +{
> > +	struct of_phandle_args iommu_spec;
> > +	struct iommu_dma_mapping *mapping;
> > +	struct bus_type *bus = dev->bus;
> > +	const struct iommu_ops *ops = bus->iommu_ops;
> > +	struct iommu_data *iommu = NULL;
> > +	int idx = 0;
> > +
> > +	if (!iommu_present(bus) || !ops->of_xlate)
> > +		return NULL;
> > +
> > +	/*
> > +	 * We don't currently walk up the tree looking for a parent IOMMU.
> > +	 * See the `Notes:' section of
> > +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> > +	 */
> > +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> > +					   "#iommu-cells", idx,
> > +					   &iommu_spec)) {
> > +		struct device_node *np = iommu_spec.np;
> > +		struct iommu_data *data = of_iommu_get_data(np);
> > +
> > +		if (!iommu) {
> > +			if (!ops->of_xlate(dev, &iommu_spec))
> > +				iommu = data;
> 
> If I understand the code correctly, this will call of_xlate for the first 
> IOMMU reference only and silently ignore all subsequent references if they 
> point to the same IOMMU device but with different stream/requester IDs. Is 
> that the desired behaviour ?

No; I just fixed that actually. I'll send a v3 soon which has a bunch of
fixes like this.

Will