Hi Will,

On Monday 22 September 2014 18:50:27 Will Deacon wrote:

On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:

quoted

On Thu, Sep 18, 2014 at 02:17:33PM +0300, Laurent Pinchart wrote:

quoted

On Friday 12 September 2014 17:34:53 Will Deacon wrote:

quoted

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

 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 24 +++++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h

b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3
100644

--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h

@@ -121,7 +121,9 @@ 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, bool
coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64
dma_base,
+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent)
 {
 	if (coherent)
 		set_dma_ops(dev, &arm_coherent_dma_ops);

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 946dd7ae0394..95ebd38db545 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>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct

platform_device *pdev)
 	int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 	
 	/*

@@ -195,7 +197,19 @@ static void of_dma_configure(struct

platform_device *pdev)
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+	if (iommu)
+		kref_put(&iommu->kref, of_iommu_deconfigure);

What's the expected life cycle of the iommu_dma_mapping structure ? It
gets created by of_iommu_configure() and the initial reference gets
dropped here. I suppose you expect arch code to need to keep a reference
to the structure, but your implementation in patch 7/7 doesn't. As far as
I can see, you don't even use the contents of the structure in the ARM
arch_setup_dma_ops() implementation. Do you expect that to change later,
or other architectures to need it ?

By the way, now that I think about it, I find struct iommu_dma_mapping
and struct dma_iommu_mapping very confusing.

Agreed. I wonder how useful it is to know the set of IOMMU instances
that each device can master through. Wouldn't it be more useful to keep
a list of master interfaces for each device? The set of IOMMU instances
can trivially be derived from that.

I'm struggling to think how that would look. What do you mean by `master
interfaces' in terms of the code we have in Linux? At the end of the day,
the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you
and Laurent have use-cases involving devices mastering through multiple
IOMMUs. If it doesn't work for you, it might be best for you to send me
the patch ;)

Just for the record, I've brought up the topic of masters being served by 
multiple IOMMUs, but don't have a use case for it (yet at least). I do have 
masters served through multiple streams with separate stream IDs, but all by 
the same IOMMU.

-- 
Regards,

Laurent Pinchart