dmaengine: rcar-dmac: add iommu support for slave transfers

A MMIO mapped resource can not be represented by a struct page so a new debug type is needed to handle this. This patch add such type and functionality to add/remove entries and how to translate them to a physical address.

Signed-off-by Niklas Söderlund <[email protected]>

---
 include/linux/dma-debug.h | 19 +++++++++++++++++
 lib/dma-debug.c           | 52 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 2 deletions(-)

include/linux/dma-debug.h +0 -0

index fe8cb61..c7d844f 100644
@@ -56,6 +56,13 @@ extern void debug_dma_alloc_coherent(struct device *dev,

size_t size, extern void debug_dma_free_coherent(struct device *dev, size_t size, void *virt, dma_addr_t addr);

include/linux/dma-debug.h +5 -0

+extern void debug_dma_map_resource(struct device *dev, phys_addr_t addr,
+				   size_t size, int direction,
+				   dma_addr_t dma_addr);
+
+extern void debug_dma_unmap_resource(struct device *dev, dma_addr_t

dma_addr, + size_t size, int direction);

include/linux/dma-debug.h +12 -0

+
 extern void debug_dma_sync_single_for_cpu(struct device *dev,
 					  dma_addr_t dma_handle, size_t size,
 					  int direction);

@@ -141,6 +148,18 @@ static inline void debug_dma_free_coherent(struct
 device *dev, size_t size, {
 }
 
+static inline void debug_dma_map_resource(struct device *dev, phys_addr_t
 addr, +					  size_t size, int direction,
+					  dma_addr_t dma_addr)
+{
+}
+
+static inline void debug_dma_unmap_resource(struct device *dev,
+					    dma_addr_t dma_addr, size_t size,
+					    int direction)
+{
+}
+
 static inline void debug_dma_sync_single_for_cpu(struct device *dev,
 						 dma_addr_t dma_handle,
 						 size_t size, int direction)

lib/dma-debug.c +49 -2

index fcfa193..2ba086b 100644
@@ -43,6 +43,7 @@ enum {
 	dma_debug_page,
 	dma_debug_sg,
 	dma_debug_coherent,
+	dma_debug_resource,
 };
 
 enum map_err_types {

@@ -150,8 +151,9 @@ static const char *const maperr2str[] = {
 	[MAP_ERR_CHECKED] = "dma map error checked",
 };
 
-static const char *type2name[4] = { "single", "page",
-				    "scather-gather", "coherent" };
+static const char *type2name[5] = { "single", "page",
+				    "scather-gather", "coherent",
+				    "resource" };
 
 static const char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",
 				   "DMA_FROM_DEVICE", "DMA_NONE" };

@@ -399,6 +401,9 @@ static void hash_bucket_del(struct dma_debug_entry
 *entry)
 
 static unsigned long long phys_addr(struct dma_debug_entry *entry)
 {
+	if (entry->type == dma_debug_resource)
+		return __pfn_to_phys(entry->pfn) + entry->offset;
+
 	return page_to_phys(pfn_to_page(entry->pfn)) + entry->offset;
 }

@@ -1495,6 +1500,49 @@ void debug_dma_free_coherent(struct device *dev,
 size_t size, }
 EXPORT_SYMBOL(debug_dma_free_coherent);
 
+void debug_dma_map_resource(struct device *dev, phys_addr_t addr, size_t
 size, +			    int direction, dma_addr_t dma_addr)
+{
+	struct dma_debug_entry *entry;
+
+	if (unlikely(dma_debug_disabled()))
+		return;
+
+	entry = dma_entry_alloc();
+	if (!entry)
+		return;
+
+	entry->type		= dma_debug_resource;
+	entry->dev		= dev;
+	entry->pfn		= __phys_to_pfn(addr);
+	entry->offset		= offset_in_page(addr);
+	entry->size		= size;
+	entry->dev_addr		= dma_addr;
+	entry->direction	= direction;
+	entry->map_err_type	= MAP_ERR_NOT_CHECKED;
+
+	add_dma_entry(entry);
+}
+EXPORT_SYMBOL(debug_dma_map_resource);
+
+void debug_dma_unmap_resource(struct device *dev, dma_addr_t dma_addr,
+			      size_t size, int direction)
+{
+	struct dma_debug_entry ref = {
+		.type           = dma_debug_resource,
+		.dev            = dev,
+		.dev_addr       = dma_addr,
+		.size           = size,
+		.direction      = direction,
+	};
+
+	if (unlikely(dma_debug_disabled()))
+		return;
+
+	check_unmap(&ref);
+}
+EXPORT_SYMBOL(debug_dma_unmap_resource);
+
 void debug_dma_sync_single_for_cpu(struct device *dev, dma_addr_t
 dma_handle, size_t size, int direction)
 {

Map/Unmap a device MMIO resource from a physical address. If no dma_map_ops method is available the operation is a no-op.

Signed-off-by Niklas Söderlund <[email protected]>

---
 Documentation/DMA-API.txt   | 22 +++++++++++++++++-----
 include/linux/dma-mapping.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 5 deletions(-)

Documentation/DMA-API.txt +0 -0

index 1d26eeb6..6b20128 100644
@@ -277,14 +277,26 @@ and <size> parameters are provided to do partial page

mapping, it is recommended that you never use these unless you really know what the cache width is.

Documentation/DMA-API.txt +12 -1

+dma_addr_t
+dma_map_resource(struct device *dev, phys_addr_t phys_addr, size_t size,
+		 enum dma_data_direction dir, unsigned long attrs)
+
+void
+dma_unmap_resource(struct device *dev, dma_addr_t addr, size_t size,
+		   enum dma_data_direction dir, unsigned long attrs)
+
+API for mapping and unmapping for MMIO resources. All the notes and
+warnings for the other mapping APIs apply here. The API should only be
+used to map device MMIO resources, mapping of RAM is not permitted.
+
 int
 dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 
-In some circumstances dma_map_single() and dma_map_page() will fail to

create -a mapping. A driver can check for these errors by testing the returned -DMA address with dma_mapping_error(). A non-zero return value means the mapping -could not be created and the driver should take appropriate action (e.g. -reduce current DMA mapping usage or delay and try again later).
+In some circumstances dma_map_single(), dma_map_page() and dma_map_resource() +will fail to create a mapping. A driver can check for these errors by testing +the returned DMA address with dma_mapping_error(). A non-zero return value +means the mapping could not be created and the driver should take appropriate +action (e.g. reduce current DMA mapping usage or delay and try again later).

Documentation/DMA-API.txt +0 -0

 	int
 	dma_map_sg(struct device *dev, struct scatterlist *sg,

include/linux/dma-mapping.h +16 -0

index e3bd27f..9a07882 100644
@@ -264,6 +264,42 @@ static inline void dma_unmap_page(struct device *dev,
 dma_addr_t addr, debug_dma_unmap_page(dev, addr, size, dir, false);
 }
 
+static inline dma_addr_t dma_map_resource(struct device *dev,
+					  phys_addr_t phys_addr,
+					  size_t size,
+					  enum dma_data_direction dir,
+					  unsigned long attrs)
+{
+	struct dma_map_ops *ops = get_dma_ops(dev);
+	unsigned long pfn = __phys_to_pfn(phys_addr);
+	dma_addr_t addr;
+
+	BUG_ON(!valid_dma_direction(dir));
+
+	/* Don't allow RAM to be mapped */
+	BUG_ON(pfn_valid(pfn));
+
+	addr = phys_addr;

include/linux/dma-mapping.h +10 -0

+				      unsigned long attrs)
+{
+	struct dma_map_ops *ops = get_dma_ops(dev);
+
+	BUG_ON(!valid_dma_direction(dir));
+	if (ops->unmap_resource)
+		ops->unmap_resource(dev, addr, size, dir, attrs);
+	debug_dma_unmap_resource(dev, addr, size, dir);
+}
+
 static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t
 addr, size_t size,
 					   enum dma_data_direction dir)

Add methods to map/unmap device resources addresses for dma_map_ops that are IOMMU aware. This is needed to map a device MMIO register from a physical address.

Signed-off-by Niklas S?derlund <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

---
 arch/arm/mm/dma-mapping.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

arch/arm/mm/dma-mapping.c +63 -0

index c6834c0..746eb29 100644
@@ -2014,6 +2014,63 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle,
 	__free_iova(mapping, iova, len);
 }
 
+/**
+ * arm_iommu_map_resource - map a device resource for DMA
+ * @dev: valid struct device pointer
+ * @phys_addr: physical address of resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static dma_addr_t arm_iommu_map_resource(struct device *dev,
+		phys_addr_t phys_addr, size_t size,
+		enum dma_data_direction dir, unsigned long attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t dma_addr;
+	int ret, prot;
+	phys_addr_t addr = phys_addr & PAGE_MASK;
+	unsigned int offset = phys_addr & ~PAGE_MASK;
+	size_t len = PAGE_ALIGN(size + offset);
+
+	dma_addr = __alloc_iova(mapping, len);
+	if (dma_addr == DMA_ERROR_CODE)
+		return dma_addr;
+
+	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
+
+	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
+	if (ret < 0)
+		goto fail;
+
+	return dma_addr + offset;
+fail:
+	__free_iova(mapping, dma_addr, len);
+	return DMA_ERROR_CODE;
+}
+
+/**
+ * arm_iommu_unmap_resource - unmap a device DMA resource
+ * @dev: valid struct device pointer
+ * @dma_handle: DMA address to resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
+		size_t size, enum dma_data_direction dir,
+		unsigned long attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t iova = dma_handle & PAGE_MASK;
+	unsigned int offset = dma_handle & ~PAGE_MASK;
+	size_t len = PAGE_ALIGN(size + offset);
+
+	if (!iova)
+		return;
+
+	iommu_unmap(mapping->domain, iova, len);
+	__free_iova(mapping, iova, len);
+}
+
 static void arm_iommu_sync_single_for_cpu(struct device *dev,
 		dma_addr_t handle, size_t size, enum dma_data_direction dir)
 {

@@ -2057,6 +2114,9 @@ struct dma_map_ops iommu_ops = {
 	.unmap_sg		= arm_iommu_unmap_sg,
 	.sync_sg_for_cpu	= arm_iommu_sync_sg_for_cpu,
 	.sync_sg_for_device	= arm_iommu_sync_sg_for_device,
+
+	.map_resource		= arm_iommu_map_resource,
+	.unmap_resource		= arm_iommu_unmap_resource,
 };
 
 struct dma_map_ops iommu_coherent_ops = {

@@ -2070,6 +2130,9 @@ struct dma_map_ops iommu_coherent_ops = {
 
 	.map_sg		= arm_coherent_iommu_map_sg,
 	.unmap_sg	= arm_coherent_iommu_unmap_sg,
+
+	.map_resource	= arm_iommu_map_resource,
+	.unmap_resource	= arm_iommu_unmap_resource,
 };
 
 /**

Signature · 1 line

2.9.2

Hi Russell,

If you have the time can you please have a look at this patch? This series have been out for some time now and Vinod is willing to take it through the dmaengine tree but a ACK is needed on this patch from you first.

Quoted 111 lines

Add methods to map/unmap device resources addresses for dma_map_ops that are IOMMU aware. This is needed to map a device MMIO register from a physical address.

Signed-off-by Niklas Söderlund <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

---

 arch/arm/mm/dma-mapping.c | 63 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

arch/arm/mm/dma-mapping.c +59 -0

index c6834c0..746eb29 100644
@@ -2014,6 +2014,63 @@ static void arm_iommu_unmap_page(struct device
 *dev, dma_addr_t handle,> 
 	__free_iova(mapping, iova, len);
 
 }
 
+/**
+ * arm_iommu_map_resource - map a device resource for DMA
+ * @dev: valid struct device pointer
+ * @phys_addr: physical address of resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static dma_addr_t arm_iommu_map_resource(struct device *dev,
+		phys_addr_t phys_addr, size_t size,
+		enum dma_data_direction dir, unsigned long attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t dma_addr;
+	int ret, prot;
+	phys_addr_t addr = phys_addr & PAGE_MASK;
+	unsigned int offset = phys_addr & ~PAGE_MASK;
+	size_t len = PAGE_ALIGN(size + offset);
+
+	dma_addr = __alloc_iova(mapping, len);
+	if (dma_addr == DMA_ERROR_CODE)
+		return dma_addr;
+
+	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
+
+	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
+	if (ret < 0)
+		goto fail;
+
+	return dma_addr + offset;
+fail:
+	__free_iova(mapping, dma_addr, len);
+	return DMA_ERROR_CODE;
+}
+
+/**
+ * arm_iommu_unmap_resource - unmap a device DMA resource
+ * @dev: valid struct device pointer
+ * @dma_handle: DMA address to resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t
 dma_handle, +		size_t size, enum dma_data_direction dir,
+		unsigned long attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t iova = dma_handle & PAGE_MASK;
+	unsigned int offset = dma_handle & ~PAGE_MASK;
+	size_t len = PAGE_ALIGN(size + offset);
+
+	if (!iova)
+		return;
+
+	iommu_unmap(mapping->domain, iova, len);
+	__free_iova(mapping, iova, len);
+}
+
 
 static void arm_iommu_sync_single_for_cpu(struct device *dev,
 
 		dma_addr_t handle, size_t size, enum dma_data_direction dir)
 
 {

@@ -2057,6 +2114,9 @@ struct dma_map_ops iommu_ops = {
 
 	.unmap_sg		= arm_iommu_unmap_sg,
 	.sync_sg_for_cpu	= arm_iommu_sync_sg_for_cpu,
 	.sync_sg_for_device	= arm_iommu_sync_sg_for_device,
 
+
+	.map_resource		= arm_iommu_map_resource,
+	.unmap_resource		= arm_iommu_unmap_resource,
 

};

struct dma_map_ops iommu_coherent_ops = {

arch/arm/mm/dma-mapping.c +3 -0

@@ -2070,6 +2130,9 @@ struct dma_map_ops iommu_coherent_ops = {
 
 	.map_sg		= arm_coherent_iommu_map_sg,
 	.unmap_sg	= arm_coherent_iommu_unmap_sg,
 
+
+	.map_resource	= arm_iommu_map_resource,
+	.unmap_resource	= arm_iommu_unmap_resource,
 
 };

/**

Enable slave transfers to a device behind a IPMMU by mapping the slave addresses using the dma-mapping API.

Signed-off-by Niklas Söderlund <[email protected]>

---
drivers/dma/sh/rcar-dmac.c | 82 ++++++++++++++++++++++++++++++++++++++-----

1 file changed, 74 insertions(+), 8 deletions(-)

drivers/dma/sh/rcar-dmac.c +16 -0

index cf983a9..22a5e40 100644
@@ -128,6 +128,18 @@ struct rcar_dmac_chan_slave {
 };
 
 /*
+ * struct rcar_dmac_chan_map - Map of slave device phys to dma address
+ * @addr: slave dma address
+ * @dir: direction of mapping
+ * @slave: slave configuration that is mapped
+ */
+struct rcar_dmac_chan_map {
+	dma_addr_t addr;
+	enum dma_data_direction dir;
+	struct rcar_dmac_chan_slave slave;
+};
+
+/*
  * struct rcar_dmac_chan - R-Car Gen2 DMA Controller Channel
  * @chan: base DMA channel object
  * @iomem: channel I/O memory base

@@ -152,6 +164,7 @@ struct rcar_dmac_chan {
 
 	struct rcar_dmac_chan_slave src;
 	struct rcar_dmac_chan_slave dst;
+	struct rcar_dmac_chan_map map;
 	int mid_rid;
 
 	spinlock_t lock;

@@ -1029,13 +1042,65 @@ rcar_dmac_prep_dma_memcpy(struct dma_chan *chan,
 dma_addr_t dma_dest, DMA_MEM_TO_MEM, flags, false);
 }
 
+static int rcar_dmac_map_slave_addr(struct dma_chan *chan,
+				    enum dma_transfer_direction dir)
+{

drivers/dma/sh/rcar-dmac.c +6 -0

+	} else {
+		dev_addr = rchan->dst.slave_addr;
+		dev_size = rchan->dst.xfer_size;
+		dev_dir = DMA_FROM_DEVICE;
+	}
+

drivers/dma/sh/rcar-dmac.c +6 -0

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+

drivers/dma/sh/rcar-dmac.c +12 -4

+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}
+
 static struct dma_async_tx_descriptor *
 rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			unsigned int sg_len, enum dma_transfer_direction dir,
 			unsigned long flags, void *context)
 {
 	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
-	dma_addr_t dev_addr;
 
 	/* Someone calling slave DMA on a generic channel? */
 	if (rchan->mid_rid < 0 || !sg_len) {

@@ -1045,9 +1110,10 @@ rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct
 scatterlist *sgl, return NULL;
 	}
 
-	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
-	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
+	if (rcar_dmac_map_slave_addr(chan, dir))
+		return NULL;
+
+	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->map.addr,
 				      dir, flags, false);
 }
 

@@ -1061,7 +1127,6 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,

dma_addr_t buf_addr, struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);

drivers/dma/sh/rcar-dmac.c +4 -4

 	struct dma_async_tx_descriptor *desc;
 	struct scatterlist *sgl;
-	dma_addr_t dev_addr;
 	unsigned int sg_len;
 	unsigned int i;

@@ -1073,6 +1138,9 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,
 dma_addr_t buf_addr, return NULL;
 	}
 
+	if (rcar_dmac_map_slave_addr(chan, dir))
+		return NULL;
+
 	sg_len = buf_len / period_len;
 	if (sg_len > RCAR_DMAC_MAX_SG_LEN) {
 		dev_err(chan->device->dev,

@@ -1100,9 +1168,7 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,
 dma_addr_t buf_addr, sg_dma_len(&sgl[i]) = period_len;
 	}
 
-	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
-	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
+	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->map.addr,
 				      dir, flags, true);
 
 	kfree(sgl);

Hi Niklas,

Thank you for the patch.

Quoted 43 lines · On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

Enable slave transfers to a device behind a IPMMU by mapping the slave addresses using the dma-mapping API.

Signed-off-by Niklas S?derlund <[email protected]>

---
drivers/dma/sh/rcar-dmac.c | 82 ++++++++++++++++++++++++++++++++++++++-----

1 file changed, 74 insertions(+), 8 deletions(-)

drivers/dma/sh/rcar-dmac.c +13 -0

index cf983a9..22a5e40 100644
@@ -128,6 +128,18 @@ struct rcar_dmac_chan_slave {
 };
 
 /*
+ * struct rcar_dmac_chan_map - Map of slave device phys to dma address
+ * @addr: slave dma address
+ * @dir: direction of mapping
+ * @slave: slave configuration that is mapped
+ */
+struct rcar_dmac_chan_map {
+	dma_addr_t addr;
+	enum dma_data_direction dir;
+	struct rcar_dmac_chan_slave slave;
+};
+
+/*
  * struct rcar_dmac_chan - R-Car Gen2 DMA Controller Channel
  * @chan: base DMA channel object
  * @iomem: channel I/O memory base

@@ -152,6 +164,7 @@ struct rcar_dmac_chan {
 
 	struct rcar_dmac_chan_slave src;
 	struct rcar_dmac_chan_slave dst;
+	struct rcar_dmac_chan_map map;
 	int mid_rid;
 
 	spinlock_t lock;

@@ -1029,13 +1042,65 @@ rcar_dmac_prep_dma_memcpy(struct dma_chan *chan,
 dma_addr_t dma_dest, DMA_MEM_TO_MEM, flags, false);
 }
 

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest, · line 1057

Quoted 13 lines

drivers/dma/sh/rcar-dmac.c +13 -0

+static int rcar_dmac_map_slave_addr(struct dma_chan *chan,
+				    enum dma_transfer_direction dir)
+{
+	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
+	struct rcar_dmac_chan_map *map = &rchan->map;
+	phys_addr_t dev_addr;
+	size_t dev_size;
+	enum dma_data_direction dev_dir;
+
+	if (dir == DMA_DEV_TO_MEM) {
+		dev_addr = rchan->src.slave_addr;
+		dev_size = rchan->src.xfer_size;
+		dev_dir = DMA_TO_DEVICE;

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

Quoted 6 lines

drivers/dma/sh/rcar-dmac.c +6 -0

+	} else {
+		dev_addr = rchan->dst.slave_addr;
+		dev_size = rchan->dst.xfer_size;
+		dev_dir = DMA_FROM_DEVICE;
+	}
+

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest, · line 1073

Quoted 10 lines

drivers/dma/sh/rcar-dmac.c +10 -0

+	/* Reuse current map if possible. */
+	if (dev_addr == map->slave.slave_addr &&
+	    dev_size == map->slave.xfer_size &&
+	    dev_dir == map->dir)
+		return 0;
+
+	/* Remove old mapping if present. */
+	if (map->slave.xfer_size)
+		dma_unmap_resource(chan->device->dev, map->addr,
+				   map->slave.xfer_size, map->dir, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel resources, will it ?

Quoted 6 lines

drivers/dma/sh/rcar-dmac.c +6 -0

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest,

Quoted 10 lines

drivers/dma/sh/rcar-dmac.c +10 -0

+	if (dma_mapping_error(chan->device->dev, map->addr)) {
+		dev_err(chan->device->dev,
+			"chan%u: failed to map %zx@%pap", rchan->index,
+			dev_size, &dev_addr);
+		return -EIO;
+	}
+
+	dev_dbg(chan->device->dev, "chan%u: map %zx@%pap to %pad dir: %s\n",
+		rchan->index, dev_size, &dev_addr, &map->addr,
+		dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" : 

"DMA_FROM_DEVICE");

Quoted 62 lines

drivers/dma/sh/rcar-dmac.c +12 -4

+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}
+
 static struct dma_async_tx_descriptor *
 rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			unsigned int sg_len, enum dma_transfer_direction dir,
 			unsigned long flags, void *context)
 {
 	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
-	dma_addr_t dev_addr;
 
 	/* Someone calling slave DMA on a generic channel? */
 	if (rchan->mid_rid < 0 || !sg_len) {

@@ -1045,9 +1110,10 @@ rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct
 scatterlist *sgl, return NULL;
 	}
 
-	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
-	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
+	if (rcar_dmac_map_slave_addr(chan, dir))
+		return NULL;
+
+	return rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->map.addr,
 				      dir, flags, false);
 }
 

@@ -1061,7 +1127,6 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,

dma_addr_t buf_addr, struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);

drivers/dma/sh/rcar-dmac.c +4 -4

 	struct dma_async_tx_descriptor *desc;
 	struct scatterlist *sgl;
-	dma_addr_t dev_addr;
 	unsigned int sg_len;
 	unsigned int i;

@@ -1073,6 +1138,9 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,
 dma_addr_t buf_addr, return NULL;
 	}
 
+	if (rcar_dmac_map_slave_addr(chan, dir))
+		return NULL;
+
 	sg_len = buf_len / period_len;
 	if (sg_len > RCAR_DMAC_MAX_SG_LEN) {
 		dev_err(chan->device->dev,

@@ -1100,9 +1168,7 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,
 dma_addr_t buf_addr, sg_dma_len(&sgl[i]) = period_len;
 	}
 
-	dev_addr = dir == DMA_DEV_TO_MEM
-		 ? rchan->src.slave_addr : rchan->dst.slave_addr;
-	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, dev_addr,
+	desc = rcar_dmac_chan_prep_sg(rchan, sgl, sg_len, rchan->map.addr,
 				      dir, flags, true);
 
 	kfree(sgl);

Quoted 12 lines · On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

Enable slave transfers to a device behind a IPMMU by mapping the slave addresses using the dma-mapping API.

Signed-off-by Niklas S?derlund <[email protected]>

---
drivers/dma/sh/rcar-dmac.c | 82 ++++++++++++++++++++++++++++++++++++-----

1 file changed, 74 insertions(+), 8 deletions(-)

drivers/dma/sh/rcar-dmac.c +0 -0

index cf983a9..22a5e40 100644

Quoted 7 lines

drivers/dma/sh/rcar-dmac.c +7 -0

+static int rcar_dmac_map_slave_addr(struct dma_chan *chan,
+				    enum dma_transfer_direction dir)
+{
+	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
+	struct rcar_dmac_chan_map *map = &rchan->map;
+	phys_addr_t dev_addr;
+	size_t dev_size;

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest, · line 1057

Quoted 6 lines

drivers/dma/sh/rcar-dmac.c +6 -0

+	enum dma_data_direction dev_dir;
+
+	if (dir == DMA_DEV_TO_MEM) {
+		dev_addr = rchan->src.slave_addr;
+		dev_size = rchan->src.xfer_size;
+		dev_dir = DMA_TO_DEVICE;

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

Quoted 11 lines

drivers/dma/sh/rcar-dmac.c +11 -0

+	} else {
+		dev_addr = rchan->dst.slave_addr;
+		dev_size = rchan->dst.xfer_size;
+		dev_dir = DMA_FROM_DEVICE;
+	}
+
+	/* Reuse current map if possible. */
+	if (dev_addr == map->slave.slave_addr &&
+	    dev_size == map->slave.xfer_size &&
+	    dev_dir == map->dir)
+		return 0;

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest, · line 1073

Quoted 5 lines

drivers/dma/sh/rcar-dmac.c +5 -0

+
+	/* Remove old mapping if present. */
+	if (map->slave.xfer_size)
+		dma_unmap_resource(chan->device->dev, map->addr,
+				   map->slave.xfer_size, map->dir, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel resources, will it ?

Quoted 7 lines

drivers/dma/sh/rcar-dmac.c +7 -0

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	if (dma_mapping_error(chan->device->dev, map->addr)) {

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest,

Quoted 10 lines

drivers/dma/sh/rcar-dmac.c +9 -0

+		dev_err(chan->device->dev,
+			"chan%u: failed to map %zx@%pap", rchan->index,
+			dev_size, &dev_addr);
+		return -EIO;
+	}
+
+	dev_dbg(chan->device->dev, "chan%u: map %zx@%pap to %pad dir: %s\n",
+		rchan->index, dev_size, &dev_addr, &map->addr,
 
+		dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" :

"DMA_FROM_DEVICE");

Quoted 7 lines

drivers/dma/sh/rcar-dmac.c +7 -0

+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}

Hi Niklas,

Quoted 13 lines · On Monday 05 Sep 2016 12:52:44 Laurent Pinchart wrote:

Quoted 12 lines · On Wednesday 10 Aug 2016 13:22:19 Niklas S?derlund wrote:

Enable slave transfers to a device behind a IPMMU by mapping the slave addresses using the dma-mapping API.

Signed-off-by Niklas S?derlund <[email protected]>

---
drivers/dma/sh/rcar-dmac.c | 82 ++++++++++++++++++++++++++++++++++++-----

1 file changed, 74 insertions(+), 8 deletions(-)

drivers/dma/sh/rcar-dmac.c +0 -0

index cf983a9..22a5e40 100644

[snip]

Quoted 7 lines

Quoted 7 lines

drivers/dma/sh/rcar-dmac.c +7 -0

+static int rcar_dmac_map_slave_addr(struct dma_chan *chan,
+				    enum dma_transfer_direction dir)
+{
+	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
+	struct rcar_dmac_chan_map *map = &rchan->map;
+	phys_addr_t dev_addr;
+	size_t dev_size;

Quoted 8 lines

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest, · line 1057

Quoted 6 lines

drivers/dma/sh/rcar-dmac.c +6 -0

+	enum dma_data_direction dev_dir;
+
+	if (dir == DMA_DEV_TO_MEM) {
+		dev_addr = rchan->src.slave_addr;
+		dev_size = rchan->src.xfer_size;
+		dev_dir = DMA_TO_DEVICE;

Shouldn't this be DMA_FROM_DEVICE, and DMA_TO_DEVICE below ?

This comment can be ignored (thank you Robin for the explanation), but ...

Quoted 11 lines

Quoted 11 lines

drivers/dma/sh/rcar-dmac.c +11 -0

+	} else {
+		dev_addr = rchan->dst.slave_addr;
+		dev_size = rchan->dst.xfer_size;
+		dev_dir = DMA_FROM_DEVICE;
+	}
+
+	/* Reuse current map if possible. */
+	if (dev_addr == map->slave.slave_addr &&
+	    dev_size == map->slave.xfer_size &&
+	    dev_dir == map->dir)
+		return 0;

Quoted 8 lines

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest, · line 1073

Quoted 5 lines

drivers/dma/sh/rcar-dmac.c +5 -0

+
+	/* Remove old mapping if present. */
+	if (map->slave.xfer_size)
+		dma_unmap_resource(chan->device->dev, map->addr,
+				   map->slave.xfer_size, map->dir, 0);

Unless I'm mistaken the resource will not be unmapped when freeing channel resources, will it ?

I believe this one still needs to be addressed.

Quoted 27 lines

Quoted 8 lines

drivers/dma/sh/rcar-dmac.c +7 -0

+	map->slave.xfer_size = 0;
+
+	/* Create new slave address map. */
+	map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
+				     dev_dir, 0);
+
+	if (dma_mapping_error(chan->device->dev, map->addr)) {

drivers/dma/sh/rcar-dmac.c rcar_dmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest,

Quoted 10 lines

drivers/dma/sh/rcar-dmac.c +9 -0

+		dev_err(chan->device->dev,
+			"chan%u: failed to map %zx@%pap", rchan->index,
+			dev_size, &dev_addr);
+		return -EIO;
+	}
+
+	dev_dbg(chan->device->dev, "chan%u: map %zx@%pap to %pad dir: %s\n",
+		rchan->index, dev_size, &dev_addr, &map->addr,
 
+		dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" :

"DMA_FROM_DEVICE");

Quoted 7 lines

drivers/dma/sh/rcar-dmac.c +7 -0

+
+	map->slave.slave_addr = dev_addr;
+	map->slave.xfer_size = dev_size;
+	map->dir = dev_dir;
+
+	return 0;
+}

[snip]

Signature · 3 lines

Regards,

Laurent Pinchart

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Quoted 19 lines · Niklas S�derlund

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

Quoted 4 lines

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

As I said in last one, the dmaengine parts look fine to me. But to go thru dmaengine tree I would need ACK on non dmaengine patches.

Quoted 25 lines · Vinod Koul

Quoted 23 lines · Niklas S?derlund

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Quoted 2 lines

As I said in last one, the dmaengine parts look fine to me. But to go thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now. If anyone has any objects, please speak up now...

Also ACKs welcome...

Quoted 27 lines · CEST Vinod Koul

Quoted 26 lines · Vinod Koul

Quoted 24 lines · Niklas Söderlund

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Quoted 7 lines

Quoted 2 lines

As I said in last one, the dmaengine parts look fine to me. But to go thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now. If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely reasonable to me, so

Acked-by Arnd Bergmann <[email protected]>

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

I don't have any idea for how this could be handled in a generic way, so my best answer here is to hope we never get there, and if we do, handle it using some local hack in the driver.

Hi Arnd,

Quoted 39 lines · On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

Quoted 27 lines · CEST Vinod Koul

Quoted 26 lines · Vinod Koul

Quoted 24 lines · Niklas S?derlund

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Quoted 7 lines

Quoted 2 lines

As I said in last one, the dmaengine parts look fine to me. But to go thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now. If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely reasonable to me, so

Quoted 7 lines

Acked-by Arnd Bergmann <[email protected]>

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go through the IOMMU while others do not ? We indeed have no solution today for such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs. I recall discussing this in the past with you, and the solution you proposed was to add a channel index to struct dma_attrs seems good to me. To support the case where some channels don't go through an IOMMU we would only need support for null entries in the IOMMUs list associated with a device (for instance in the DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3

Author: Krzysztof Kozlowski <[email protected]>
Date:   Wed Aug 3 13:46:00 2016 -0700

dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but I'm not very happy with that patch as I can see how a future proposal to handle the channel number through the DMA attributes will get rejected on the grounds of bits starvation then :-(

Quoted 3 lines

I don't have any idea for how this could be handled in a generic way, so my best answer here is to hope we never get there, and if we do, handle it using some local hack in the driver.

Quoted 53 lines · Laurent Pinchart

Quoted 39 lines · On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

Quoted 27 lines · CEST Vinod Koul

Quoted 26 lines · Vinod Koul

Quoted 24 lines · Niklas Söderlund

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Quoted 7 lines

Quoted 2 lines

As I said in last one, the dmaengine parts look fine to me. But to go thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now. If anyone has any objects, please speak up now...

Also ACKs welcome...

I had not looked at the series earlier, but this version looks entirely reasonable to me, so

Quoted 8 lines

Acked-by Arnd Bergmann <[email protected]>

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go through the IOMMU while others do not ? We indeed have no solution today for such a situation.

Quoted 7 lines

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs. I recall discussing this in the past with you, and the solution you proposed was to add a channel index to struct dma_attrs seems good to me. To support the case where some channels don't go through an IOMMU we would only need support for null entries in the IOMMUs list associated with a device (for instance in the DT case null entries in the iommus property).

I think at that point we just create the channels as child devices of the main dmaengine device so they each get their own DMA ops, and can do whatever. The Qualcomm HIDMA driver already does that for a very similar reason (so that the IOMMU can map individual channels into different guest VMs).

Quoted 16 lines

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3

Author: Krzysztof Kozlowski <[email protected]>
Date:   Wed Aug 3 13:46:00 2016 -0700

dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but I'm not very happy with that patch as I can see how a future proposal to handle the channel number through the DMA attributes will get rejected on the grounds of bits starvation then :-(

Quoted 3 lines

I don't have any idea for how this could be handled in a generic way, so my best answer here is to hope we never get there, and if we do, handle it using some local hack in the driver.

Quoted 18 lines

Quoted 10 lines

Quoted 5 lines

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go through the IOMMU while others do not ? We indeed have no solution today for such a situation.

Quoted 7 lines

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs. I recall discussing this in the past with you, and the solution you proposed was to add a channel index to struct dma_attrs seems good to me. To support the case where some channels don't go through an IOMMU we would only need support for null entries in the IOMMUs list associated with a device (for instance in the DT case null entries in the iommus property).

Quoted 5 lines

I think at that point we just create the channels as child devices of the main dmaengine device so they each get their own DMA ops, and can do whatever. The Qualcomm HIDMA driver already does that for a very similar reason (so that the IOMMU can map individual channels into different guest VMs).

That's another option, but it seems more like a workaround to me, instead of a proper solution to fix the more global problem of multiple memory paths within a single device. I have other hardware devices that can act as bus masters through different paths (for instance a display-related device that fetches data and commands through different paths). Luckily so far all those paths are served by the same IOMMU, but there's no guarantee this will remain true in the future. Furthermore, even today, the IOMMU connected to that device has the ability to selectively enable and disable its ports. I have to keep them all enabled due to the lack of channel information in the DMA mapping and IOMMU APIs, leading to increased power consumption.

Quoted 17 lines

Quoted 17 lines

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3

Author: Krzysztof Kozlowski <[email protected]>
Date:   Wed Aug 3 13:46:00 2016 -0700

dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but I'm not very happy with that patch as I can see how a future proposal to handle the channel number through the DMA attributes will get rejected on the grounds of bits starvation then :-(

Quoted 3 lines

I don't have any idea for how this could be handled in a generic way, so my best answer here is to hope we never get there, and if we do, handle it using some local hack in the driver.

On 16/09/16 13:05, Laurent Pinchart wrote:
[...]

Quoted 25 lines

Quoted 19 lines

Quoted 10 lines

Quoted 5 lines

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go through the IOMMU while others do not ? We indeed have no solution today for such a situation.

Quoted 8 lines

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs. I recall discussing this in the past with you, and the solution you proposed was to add a channel index to struct dma_attrs seems good to me. To support the case where some channels don't go through an IOMMU we would only need support for null entries in the IOMMUs list associated with a device (for instance in the DT case null entries in the iommus property).

Quoted 5 lines

I think at that point we just create the channels as child devices of the main dmaengine device so they each get their own DMA ops, and can do whatever. The Qualcomm HIDMA driver already does that for a very similar reason (so that the IOMMU can map individual channels into different guest VMs).

Quoted 11 lines

That's another option, but it seems more like a workaround to me, instead of a proper solution to fix the more global problem of multiple memory paths within a single device. I have other hardware devices that can act as bus masters through different paths (for instance a display-related device that fetches data and commands through different paths). Luckily so far all those paths are served by the same IOMMU, but there's no guarantee this will remain true in the future. Furthermore, even today, the IOMMU connected to that device has the ability to selectively enable and disable its ports. I have to keep them all enabled due to the lack of channel information in the DMA mapping and IOMMU APIs, leading to increased power consumption.

Indeed, I think both the Exynos and Rockchip IOMMU drivers already do cater for a device mastering though multiple discrete IOMMUs, not being the fancy multi-port multi-context ones like yours and mine.

I guess what we could really do with is a decent abstraction of multi-master peripherals at the device level; a "threads within the same process" sort of granularity, as it were. I'd envisage it more along the lines of how we handle NUMA, i.e. dma_map_page_attrs(...) becomes a wrapper for dma_map_page_attrs_multi(..., CHANNEL_ALL), and trickier users can call the latter with the a more specific channel(s) argument (maybe it's a bitmask rather than an index).

Meanwhile, dev->archdata.dma_ops may point to a device-specific array of dma_map_ops, which the DMA API backend iterates over if necessary.

Strangely, that doesn't actually sound too horrible.

Quoted 9 lines · On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

Quoted 2 lines · CEST Vinod Koul

Quoted 1 line · Vinod Koul

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas S?derlund wrote:

I had not looked at the series earlier, but this version looks entirely reasonable to me, so

Acked-by Arnd Bergmann <[email protected]>

Quoted 4 lines

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Do you mean systems where some of the channels of a specific DMA engine go through the IOMMU while others do not ? We indeed have no solution today for such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs.

I recall
discussing this in the past with you, and the solution you proposed was to add a channel index to struct dma_attrs seems good to me. To support the case where some channels don't go through an IOMMU we would only need support for null entries in the IOMMUs list associated with a device (for instance in the DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3

Author: Krzysztof Kozlowski <[email protected]>
Date:   Wed Aug 3 13:46:00 2016 -0700

dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but I'm not very happy with that patch as I can see how a future proposal to handle the channel number through the DMA attributes will get rejected on the grounds of bits starvation then :-(

Quoted 17 lines · CEST Laurent Pinchart

Quoted 7 lines · On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:

Quoted 2 lines · CEST Vinod Koul

Quoted 1 line · Vinod Koul

On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:

I had not looked at the series earlier, but this version looks entirely reasonable to me, so

Quoted 8 lines

Acked-by Arnd Bergmann <[email protected]>

One concern I have is that we might get an awkward situation if we ever encounter one DMA engine hardware that is used in different systems that all have an IOMMU, but on some of them the connection between the DMA master and the slave FIFO bypasses the IOMMU while on others the IOMMU is required.

Quoted 3 lines

Do you mean systems where some of the channels of a specific DMA engine go through the IOMMU while others do not ? We indeed have no solution today for such a situation.

I wasn't thinking quite that far, though that is also a theoretical problem. However, the simple solution would be to have a bit in the DMA specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine either goes through an IOMMU or doesn't (depending on the integration into the SoC), so we'd have to find out through some DT property or compatible string in the DMA enginen driver.

Quoted 2 lines

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

Quoted 12 lines

I recall discussing this in the past with you, and the solution you proposed was to add a channel index to struct dma_attrs seems good to me. To support the case where some channels don't go through an IOMMU we would only need support for null entries in the IOMMUs list associated with a device (for instance in the DT case null entries in the iommus property).

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3

Author: Krzysztof Kozlowski <[email protected]>
Date:   Wed Aug 3 13:46:00 2016 -0700

Quoted 6 lines

dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number, but I'm not very happy with that patch as I can see how a future proposal to handle the channel number through the DMA attributes will get rejected on the grounds of bits starvation then :-(

Agreed, that can become interesting.

Quoted 8 lines

I wasn't thinking quite that far, though that is also a theoretical problem. However, the simple solution would be to have a bit in the DMA specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine either goes through an IOMMU or doesn't (depending on the integration into the SoC), so we'd have to find out through some DT property or compatible string in the DMA enginen driver.

Don't we already get that information from the iommus DT property ? If the DMA engine goes through an IOMMU the property will be set, otherwise it will not.

Quoted 4 lines

Quoted 2 lines

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

That's theoretical. The problem I'm facing today is a DMA engine whose channels are served by different ports of the same IOMMU. This works in a suboptimal way because I have to keep all the IOMMU ports enabled regardless of whether they're used or not, as the DMA engine and IOMMU APIs don't carry channel information.

Quoted 9 lines · CEST Laurent Pinchart

Quoted 8 lines

I wasn't thinking quite that far, though that is also a theoretical problem. However, the simple solution would be to have a bit in the DMA specifier let the driver know whether translation is needed or not.

The simpler case I was thinking of is where the entire DMA engine either goes through an IOMMU or doesn't (depending on the integration into the SoC), so we'd have to find out through some DT property or compatible string in the DMA enginen driver.

Quoted 3 lines

Don't we already get that information from the iommus DT property ? If the DMA engine goes through an IOMMU the property will be set, otherwise it will not.

It depends. A dmaengine typically at least has two DMA masters, possibly more. It's likely that some dmaengine implementations are connected to RAM through an IOMMU, but have direct access to an I/O bus for the slave FIFOs.

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

The problem is a bit broader than that, we'll also have an issue with DMA engines that have different channels served by different IOMMUs.

Do you mean a theoretical problem, or a chip that you already know exists?

That's theoretical. The problem I'm facing today is a DMA engine whose channels are served by different ports of the same IOMMU. This works in a suboptimal way because I have to keep all the IOMMU ports enabled regardless of whether they're used or not, as the DMA engine and IOMMU APIs don't carry channel information.

Ok

Quoted 25 lines · Vinod Koul

Quoted 23 lines · Niklas S?derlund

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Quoted 2 lines

As I said in last one, the dmaengine parts look fine to me. But to go thru dmaengine tree I would need ACK on non dmaengine patches.

I havent heard back from this one and I am inclined to merge this one now. If anyone has any objects, please speak up now...

Also ACKs welcome...

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

Hi,

This series tries to solve the problem with DMA with device registers (MMIO registers) that are behind an IOMMU for the rcar-dmac driver. A recent patch '9575632 (dmaengine: make slave address physical)' clarifies that DMA slave address provided by clients is the physical address. This puts the task of mapping the DMA slave address from a phys_addr_t to a dma_addr_t on the DMA engine.

Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are the same and no special care is needed. However if you have a IOMMU you need to map the DMA slave phys_addr_t to a dma_addr_t using something like this.

This series is based on top of v4.8-rc1. And I'm hoping to be able to collect a Ack from Russell King on patch 4/6 that adds the ARM specific part and then be able to take the whole series through the dmaengine tree. If this is not the best route I'm more then happy to do it another way.

It's tested on a Koelsch with CONFIG_IPMMU_VMSA and by enabling the ipmmu_ds node in r8a7791.dtsi. I verified operation by interacting with /dev/mmcblk1, i2c and the serial console which are devices behind the iommu.

Furthermore I have audited to the best of my ability all call paths involved to make sure that the dma_addr_t obtained from dma_map_resource() to is not used in a way where it would be expected for the mapping to be RAM (have a struct page). Many thanks to Christoph Hellwig and Laurent Pinchart for there input in this effort.