On Mon, May 5, 2014 at 2:55 PM, Arnd Bergmann [off-list ref] wrote:

On Monday 05 May 2014 14:45:21 Bjorn Helgaas wrote:

quoted

[+cc Ben, Chris]

On Fri, May 2, 2014 at 12:59 PM, Arnd Bergmann [off-list ref] wrote:

quoted

On Friday 02 May 2014 10:54:59 Bjorn Helgaas wrote:

quoted

quoted

+static void of_dma_configure(struct device *dev)
+{
+     u64 dma_addr, paddr, size;
+     int ret;
+
+     dev->coherent_dma_mask = DMA_BIT_MASK(32);
+     if (!dev->dma_mask)
+             dev->dma_mask = &dev->coherent_dma_mask;
+
+     /*
+      * 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");
+     }
+
+     /*
+      * if dma-ranges property doesn't exist - just return else
+      * setup the dma offset
+      */
+     ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+     if ((ret == -ENODEV) || (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);

Is this effectively the same as an IOMMU that applies a constant offset
to the bus address?  Could or should this be done by adding a simple IOMMU
driver instead of adding dma_pfn_offset to struct device?

We currently have two dma_map_ops variants on ARM (plus another set for
coherent/noncoherent differences, but we can ignore that for the sake
of this discussion): one that handles linear mappings and one that
handles IOMMUs by calling into the linux/iommu.h APIs.

I guess what you mean by 'a simple IOMMU driver' would be another
dma_map_ops implementation that is separate from real IOMMUs, right?

I suppose so; it seems like the offset could be managed inside
arm_dma_ops.  My idea of an IOMMU is something that maps bus addresses
to physical memory addresses.  That's what we're doing here; it's just
that the mapping function is very simple.  So why add something new in
struct device for it?

I think powerpc and tile do something similar in dma_direct_map_page()
and tile_pci_dma_map_page() (they store the offset in struct
dev_archdata).

Ah, so the only question is whether to store it in dev_archdata or
in device. I think the argument for using struct device directly
was that it lets us access this field from architecture independent
code. It's not important to the overall design though: we could easily
put it in dev_archdata and call an architecture specific function to
set it, if there are good reasons for keeping it out of the generic
device structure.

Well, it wasn't my *only* question, since I didn't know about the
powerpc and tile usage originally :).  Putting it in dev_archdata does
seem better because it's a similar solution to a similar problem, and
it's less public.

I still wonder whether arm, powerpc, and tile (and I just noticed
microblaze also has a similar dma_direct_map_page()) could all be
handled by attaching devices to a generic trivial IOMMU driver
parameterized with the appropriate constant offset.

What arch-independent code would access this data?  I expect that
drivers will use dma_map_page(), etc., so they should already have
both the bus and the physical address and wouldn't need it.  Shouldn't
generic code just rely on the DMA API, which might use an IOMMU or
this dma_pfn_offset internally?

Bjorn